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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 12 Jan 2022 00:42:00 +0000
From:   Robert Hancock <robert.hancock@...ian.com>
To:     "andrew@...n.ch" <andrew@...n.ch>
CC:     "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: Re: [PATCH net-next v2 2/3] net: phy: at803x: add fiber support

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://www.digikey.ca/en/datasheets/qualcomm/qualcommar8031dsatherosrev10aug2011
 - these names were chosen to match what it uses.

> 
>  
> >  #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).

> 
> > +	bool is_1000basex;
> >  	struct regulator_dev *vddio_rdev;
> >  	struct regulator_dev *vddh_rdev;
> >  	struct regulator *vddio;
> > @@ -784,7 +798,33 @@ static int at803x_probe(struct phy_device *phydev)
> >  			return ret;
> >  	}
> >  
> > +	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > +		int ccr = phy_read(phydev, AT803X_REG_CHIP_CONFIG);
> > +		int mode_cfg;
> > +
> > +		if (ccr < 0)
> > +			goto err;
> > +		mode_cfg = ccr & AT803X_MODE_CFG_MASK;
> > +
> > +		switch (mode_cfg) {
> > +		case AT803X_MODE_CFG_BX1000_RGMII_50:
> > +		case AT803X_MODE_CFG_BX1000_RGMII_75:
> > +			priv->is_1000basex = true;
> > +			fallthrough;
> > +		case AT803X_MODE_CFG_FX100_RGMII_50:
> > +		case AT803X_MODE_CFG_FX100_RGMII_75:
> > +			priv->is_fiber = true;
> 
> O.K, now i'm wondering what AT803X_MODE_CFG_FX100_* actually means. I
> was thinking it indicated 100BaseFX? But the fall through suggests
> otherwise.

is_1000basex is a subset of is_fiber here, so 1000Base-X sets both flags,
100Base-FX sets only is_fiber.

> 
> >  static int at803x_config_init(struct phy_device *phydev)
> >  {
> > +	struct at803x_priv *priv = phydev->priv;
> >  	int ret;
> >  
> >  	if (phydev->drv->phy_id == ATH8031_PHY_ID) {
> > -	       /* Some bootloaders leave the fiber page selected.
> > -		* Switch to the copper page, as otherwise we read
> > -		* the PHY capabilities from the fiber side.
> > -		*/
> > +		/* Some bootloaders leave the fiber page selected.
> 
> Looks like you have a tab vs space problem with the previous patch?
> Otherwise this first line should not of changed.

Indeed, looks like patch 1 replaced some tabs with spaces when the code was
moved, and this patch removed them. Odd that checkpatch didn't pick that up.
Can fix that up in a new rev..

> 
> 	  Andrew
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com