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

[Wong Vee Khee]

On Thu, Sep 30, 2021 at 10:24:39AM +0100, Russell King (Oracle) wrote:
> On Thu, Sep 30, 2021 at 12:44:21PM +0800, Wong Vee Khee wrote:
> > According to Synopsys DesignWare Cores Ethernet PCS databook, it is
> > required to disable Clause 37 auto-negotiation by programming bit-12
> > (AN_ENABLE) to 0 if it is already enabled, before programming various
> > fields of VR_MII_AN_CTRL registers.
> > 
> > After all these programming are done, it is then required to enable
> > Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1.
> > 
> > Fixes: b97b5331b8ab ("net: pcs: add C37 SGMII AN support for intel mGbE controller")
> > Cc: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> > Signed-off-by: Wong Vee Khee <vee.khee.wong@xxxxxxxxxxxxxxx>
> > ---
> > v2 -> v3:
> >  - Added error handling after xpcs_write().
> >  - Added 'changed' flag.
> >  - Added fixes tag.
> > v1 -> v2:
> >  - Removed use of xpcs_modify() helper function.
> >  - Add conditional check on inband auto-negotiation.
> > ---
> >  drivers/net/pcs/pcs-xpcs.c | 41 +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 36 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index fb0a83dc09ac..d2126f5d5016 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -697,14 +697,18 @@ EXPORT_SYMBOL_GPL(xpcs_config_eee);
> >  
> >  static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
> >  {
> > -	int ret;
> > +	int ret, reg_val;
> > +	int changed = 0;
> >  
> >  	/* For AN for C37 SGMII mode, the settings are :-
> > -	 * 1) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN)
> > -	 * 2) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII)
> > +	 * 1) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 0b (Disable SGMII AN in case
> > +	      it is already enabled)
> > +	 * 2) VR_MII_AN_CTRL Bit(2:1)[PCS_MODE] = 10b (SGMII AN)
> > +	 * 3) VR_MII_AN_CTRL Bit(3) [TX_CONFIG] = 0b (MAC side SGMII)
> >  	 *    DW xPCS used with DW EQoS MAC is always MAC side SGMII.
> > -	 * 3) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic
> > +	 * 4) VR_MII_DIG_CTRL1 Bit(9) [MAC_AUTO_SW] = 1b (Automatic
> >  	 *    speed/duplex mode change by HW after SGMII AN complete)
> > +	 * 5) VR_MII_MMD_CTRL Bit(12) [AN_ENABLE] = 1b (Enable SGMII AN)
> >  	 *
> >  	 * Note: Since it is MAC side SGMII, there is no need to set
> >  	 *	 SR_MII_AN_ADV. MAC side SGMII receives AN Tx Config from
> > @@ -712,6 +716,19 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
> >  	 *	 between PHY and Link Partner. There is also no need to
> >  	 *	 trigger AN restart for MAC-side SGMII.
> >  	 */
> > +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (ret & AN_CL37_EN) {
> > +		changed = 1;
> > +		reg_val = ret & ~AN_CL37_EN;
> > +		ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> > +				 reg_val);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> 
> I don't think you need to record "changed" here - just maintain a
> local copy of the current state of the register, e.g:
>

You're right. Will remove it on v4.
 
> +	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg_val = ret;
> +	if (reg_val & AN_CL37_EN) {
> +		reg_val &= ~AN_CL37_EN;
> ...
> 
> What you're wanting to do is ensure this bit is clear before you do the
> register changes, and once complete, set the bit back to one. If the bit
> was previously clear, you can omit this write, otherwise the write was
> needed - as you have the code. However, the point is that once you're
> past this code, the state of the register in the device will have the
> AN_CL37_EN clear. See below for the rest of my comments on this...
> 
> > @@ -736,7 +753,21 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs, unsigned int mode)
> >  	else
> >  		ret &= ~DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
> >  
> > -	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret);
> > +	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (changed) {
> > +		if (phylink_autoneg_inband(mode))
> > +			reg_val |= AN_CL37_EN;
> > +		else
> > +			reg_val &= ~AN_CL37_EN;
> > +
> > +		return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL,
> > +				  reg_val);
> > +	}
> 
> As I say above, here, you are _guaranteed_ that the AN_CL27_EN bit is
> clear in the register due to the effects of your change above. You say
> in the commit text:
> 
>   After all these programming are done, it is then required to enable
>   Clause 37 auto-negotiation by programming bit-12 (AN_ENABLE) to 1.
> 
> So that makes me think that you _always_ need to write back a value
> with AN_CL27_EN set. So:
> 
> 	reg_val |= AN_CL37_EN;
> 	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_MMD_CTRL, reg_val);
> 
> If that is not correct, the commit message is misleading and needs
> fixing.
> 

It is written in the Synopsys PCS Databook:

8. Enable CL37 Auto-negotiation, by programming bit[12] of the
   SR_MII_CTRL Register to 1.

So I think we always need to write back the value. Will fix this in v4.

> Lastly, I would recommend a much better name for this variable rather
> than the bland "reg_val" since you're needing the value preserved over
> a chunk of code. "ctrl" maybe?
> 

I will rename it to 'mdio_ctrl'.

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