Re: [PATCH net v3 1/1] net: pcs: xpcs: fix incorrect CL37 AN sequence

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Fri, Oct 01, 2021 at 12:19:52AM +0000, Vladimir Oltean wrote:
> static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
> {
> 	int ret, mdio_ctrl1, old_an_ctrl, an_ctrl, old_dig_ctrl1, dig_ctrl1;
> 
> 	/* Disable SGMII AN in case it is already enabled */
> 	mdio_ctrl1 = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
> 	if (mdio_ctrl1 < 0)
> 		return mdio_ctrl1;
> 
> 	if (mdio_ctrl1 & AN_CL37_EN) {
> 		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> 				 mdio_ctrl1 & ~AN_CL37_EN);
> 		if (ret < 0)
> 			return ret;
> 	}

This is fine...

> 	if (!(mdio_ctrl1 & AN_CL37_EN) && phylink_autoneg_inband(mode)) {
> 		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> 				 mdio_ctrl1 | AN_CL37_EN);
> 		if (ret)
> 			return ret;
> 	}

This is not. If the control register had AN_CL37_EN set initially, then
in the first test above, we clear the bit. However, mdio_ctrl1 will
still contain the bit set. When we get here, we will skip setting the
register bit back to one even if in-band mode was requested.

As I said in a previous email, at this point there is no reason to check
the previous state, because if it was set on entry, we will have cleared
it, so the register state at this point has the bit clear no matter
what. If we need to set it, then we /always/ need to write it here - it
doesn't matter what the initial state was.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!