[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openbsd-tech
Subject:    Re: assume RGMII-to-Copper mode by default in eephy(4) for 88E151x PHYs
From:       Mark Kettenis <mark.kettenis () xs4all ! nl>
Date:       2023-12-28 13:13:12
Message-ID: 87h6k2mpw7.fsf () bloch ! sibelius ! xs4all ! nl
[Download RAW message or body]

> Date: Thu, 21 Dec 2023 13:05:14 +0100
> From: Uwe Stuehler <ustuehler@growit.io>
> 
> On Thu, Dec 14, 2023 at 10:43:02PM +0100, Mark Kettenis wrote:
> > > From: Uwe Stuehler <ustuehler@growit.io>
> > > 
> > > The desired MII mode must be programmed explicitly for Marvel Atlantis
> > > 88E1512/88E1514 variants and we already do this for SGMII.
> > > 
> > > This change adds a missing case for RGMII-to-Copper that assumes RGMII
> > > unless the MAC driver sets MII_SGMII before calling mii_attach() or the
> > > mode has already been programmed. (RGMII-to-Copper is also the hardware
> > > default for E1510 and E1518.)
> > > 
> > > stsp@ and I tested this on an Intel Elkhart Lake machine with dwqe(4)
> > > where the change had no effect since the MII mode is configured in the
> > > BIOS during boot.
> > 
> > I don't think this is right as mode may end up uninitialized.  I
> > suppose the intention was to leave mode alone unless it isn't set yet?
> 
> I made a last-minute change before sending the previous diff which
> hadn't actually been tested. *sweat* *cough*
> 
> > So something like:
> 
> Here's a new diff based on your suggestion. Thank you!
> 
> I've been running with this actual change for a while now.
> 
> ok?

ok kettenis@

> ---
>  sys/dev/mii/eephy.c    | 22 +++++++++++++++++-----
>  sys/dev/mii/eephyreg.h |  5 +++++
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/sys/dev/mii/eephy.c b/sys/dev/mii/eephy.c
> index ca68e057014..31688d95a7a 100644
> --- a/sys/dev/mii/eephy.c
> +++ b/sys/dev/mii/eephy.c
> @@ -188,16 +188,28 @@ eephy_attach(struct device *parent, struct device *self, void *aux)
>  		PHY_WRITE(sc, E1000_EADR, page);
>  	}
>  
> -	/* Switch to SGMII-to-copper mode if necessary. */
> -	if (sc->mii_model == MII_MODEL_MARVELL_E1512 &&
> -	    sc->mii_flags & MIIF_SGMII) {
> +	/*
> +	 * GCR1 MII mode defaults to an invalid value on E1512/E1514
> +	 * and must be programmed with the desired mode of operation.
> +	 */
> +	if (sc->mii_model == MII_MODEL_MARVELL_E1512) {
> +		uint32_t mode;
> +
>  		page = PHY_READ(sc, E1000_EADR);
>  		PHY_WRITE(sc, E1000_EADR, 18);
> +
>  		reg = PHY_READ(sc, E1000_GCR1);
> +		mode = reg & E1000_GCR1_MODE_MASK;
> +
> +		if (mode == E1000_GCR1_MODE_UNSET)
> +			mode = E1000_GCR1_MODE_RGMII;
> +		if (sc->mii_flags & MIIF_SGMII)
> +			mode = E1000_GCR1_MODE_SGMII;
> +
>  		reg &= ~E1000_GCR1_MODE_MASK;
> -		reg |= E1000_GCR1_MODE_SGMII;
> -		reg |= E1000_GCR1_RESET;
> +		reg |= E1000_GCR1_RESET | mode;
>  		PHY_WRITE(sc, E1000_GCR1, reg);
> +
>  		PHY_WRITE(sc, E1000_EADR, page);
>  	}
>  
> diff --git a/sys/dev/mii/eephyreg.h b/sys/dev/mii/eephyreg.h
> index 593127a7b4b..d994c218040 100644
> --- a/sys/dev/mii/eephyreg.h
> +++ b/sys/dev/mii/eephyreg.h
> @@ -331,4 +331,9 @@
>  #define E1000_GCR1			0x14	/* General Control Register 1 */
>  #define E1000_GCR1_RESET		0x8000
>  #define E1000_GCR1_MODE_MASK		0x0007
> +#define E1000_GCR1_MODE_RGMII		0x0000
>  #define E1000_GCR1_MODE_SGMII		0x0001
> +#define E1000_GCR1_MODE_RGMII_1000X	0x0002
> +#define E1000_GCR1_MODE_RGMII_100FX	0x0003
> +#define E1000_GCR1_MODE_RGMII_TO_SGMII	0x0004
> +#define E1000_GCR1_MODE_UNSET		0x0007
> 

[prev in list] [next in list] [prev in thread] [next in thread]