Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

From: Robert Hancock <robert.hancock@calian.com>
To: "andrew@lunn.ch" <andrew@lunn.ch>
Cc: "linux@armlinux.org.uk" <linux@armlinux.org.uk>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"hkallweit1@gmail.com" <hkallweit1@gmail.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>
Subject: Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support
Date: Wed, 12 Jan 2022 23:56:30 +0000	[thread overview]
Message-ID: <3de186ef1c6a06cfe33f5ffe6ae3783d163e5f2c.camel@calian.com> (raw)
In-Reply-To: <Yd8w3a39GRDE7SUw@lunn.ch>

On Wed, 2022-01-12 at 20:49 +0100, Andrew Lunn wrote:
> On Wed, Jan 12, 2022 at 12:42:00AM +0000, Robert Hancock wrote:
> > On Wed, 2022-01-12 at 01:10 +0100, Andrew Lunn wrote:
> > > >  #define AT803X_MODE_CFG_MASK			0x0F
> > > > -#define AT803X_MODE_CFG_SGMII			0x01
> > > > +#define AT803X_MODE_CFG_BASET_RGMII		0x00
> > > > +#define AT803X_MODE_CFG_BASET_SGMII		0x01
> > > > +#define AT803X_MODE_CFG_BX1000_RGMII_50		0x02
> > > > +#define AT803X_MODE_CFG_BX1000_RGMII_75		0x03
> > > > +#define AT803X_MODE_CFG_BX1000_CONV_50		0x04
> > > > +#define AT803X_MODE_CFG_BX1000_CONV_75		0x05
> > > > +#define AT803X_MODE_CFG_FX100_RGMII_50		0x06
> > > > +#define AT803X_MODE_CFG_FX100_CONV_50		0x07
> > > > +#define AT803X_MODE_CFG_RGMII_AUTO_MDET		0x0B
> > > > +#define AT803X_MODE_CFG_FX100_RGMII_75		0x0E
> > > > +#define AT803X_MODE_CFG_FX100_CONV_75		0x0F
> > > 
> > > Hi Robert
> > > 
> > > What do these _50, and _75 mean?
> > 
> > 50 or 75 ohm impedance. Can refer to page 82 of the datasheet at 
> > https://urldefense.com/v3/__https://www.digikey.ca/en/datasheets/qualcomm/qualcommar8031dsatherosrev10aug2011__;!!IOGos0k!xrZprIiCK5nnfhnmAdxtCxOHGIP9149yaVxZRjOIrvXZOnmeXDVFmRH8RB9Mv_rmqdI$ 
> >  - these names were chosen to match what it uses.
> 
> I know they are getting long, but maybe add OHM to the end?

Could probably do that, yeah..

> 
> > > >  #define AT803X_PSSR				0x11	/*PHY-
> > > > Specific Status Register*/
> > > >  #define AT803X_PSSR_MR_AN_COMPLETE		0x0200
> > > > @@ -283,6 +295,8 @@ struct at803x_priv {
> > > >  	u16 clk_25m_mask;
> > > >  	u8 smarteee_lpi_tw_1g;
> > > >  	u8 smarteee_lpi_tw_100m;
> > > > +	bool is_fiber;
> > > 
> > > Is maybe is_100basefx a better name? It makes it clearer it represents
> > > a link mode?
> > 
> > This is meant to indicate the chip is set for any fiber mode (100Base-FX or
> > 1000Base-X).
> 
> O.K, then is_fibre is O.K.
> 
> I noticed code removing the link mode 1000BaseX in the case of
> is_fibre && !is_100basex. Does 100BaseFX need removing for !is_fibre?

That 1000Base-X link mode was coming from what genphy_read_abilities was
parsing out of the device's status registers. That function can't actually
detect 100Base-FX mode so it can't end up in there to need removing at this
point.

> 
> 	Andrew