Messages in this thread | | | Date | Fri, 1 Oct 2021 10:57:30 +0100 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net v3 1/1] net: pcs: xpcs: fix incorrect CL37 AN sequence |
| |
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!
| |