[PATCH] net: gem: Fix setting PCS auto-negotiation statee

[Ashok Reddy Soma]

Reviewed-by: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>

> -----Original Message-----
> From: Michal Simek <michal.simek at xilinx.com>
> Sent: Monday, March 15, 2021 2:38 PM
> To: Robert Hancock <robert.hancock at calian.com>; Michal Simek
> <michals at xilinx.com>; T Karthik Reddy <tkarthik at xilinx.com>; Ashok Reddy
> Soma <ashokred at xilinx.com>
> Cc: joe.hershberger at ni.com; rfried.dev at gmail.com; u-boot at lists.denx.de
> Subject: Re: [PATCH] net: gem: Fix setting PCS auto-negotiation statee
> 
> 
> 
> On 3/11/21 11:55 PM, Robert Hancock wrote:
> > The code was trying to disable PCS auto-negotiation when a fixed-link
> > node is present and enable it otherwise. However, the PCS registers
> > were being written before the PCSSEL bit was set in the network
> > configuration register, and it appears that in this state, PCS
> > register writes are ignored. The result is that the intended change
> > only took effect on the second network operation that was performed,
> > since at that time PCSSEL is already enabled.
> >
> > Fix the order of register writes so that PCS registers are only
> > written to after the PCS is enabled.
> >
> > Fixes: 26e62cc971 ("net: gem: Disable PCS autonegotiation in case of
> > fixed-link")
> >
> > Signed-off-by: Robert Hancock <robert.hancock at calian.com>
> > ---
> >  drivers/net/zynq_gem.c | 25 +++++++++++++++++--------
> >  1 file changed, 17 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/zynq_gem.c b/drivers/net/zynq_gem.c index
> > baf06a2ad8..ff59982267 100644
> > --- a/drivers/net/zynq_gem.c
> > +++ b/drivers/net/zynq_gem.c
> > @@ -454,14 +454,6 @@ static int zynq_gem_init(struct udevice *dev)
> >  	    priv->int_pcs) {
> >  		nwconfig |= ZYNQ_GEM_NWCFG_SGMII_ENBL |
> >  			    ZYNQ_GEM_NWCFG_PCS_SEL;
> > -#ifdef CONFIG_ARM64
> > -	if (priv->phydev->phy_id != PHY_FIXED_ID)
> > -		writel(readl(&regs->pcscntrl) |
> ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
> > -		       &regs->pcscntrl);
> > -	else
> > -		writel(readl(&regs->pcscntrl) &
> ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
> > -		       &regs->pcscntrl);
> > -#endif
> >  	}
> >
> >  	switch (priv->phydev->speed) {
> > @@ -480,6 +472,23 @@ static int zynq_gem_init(struct udevice *dev)
> >  		break;
> >  	}
> >
> > +#ifdef CONFIG_ARM64
> > +	if (priv->interface == PHY_INTERFACE_MODE_SGMII &&
> > +	    priv->int_pcs) {
> > +		/*
> > +		 * Disable AN for fixed link configuration, enable otherwise.
> > +		 * Must be written after PCS_SEL is set in nwconfig,
> > +		 * otherwise writes will not take effect.
> > +		 */
> > +		if (priv->phydev->phy_id != PHY_FIXED_ID)
> > +			writel(readl(&regs->pcscntrl) |
> ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
> > +			       &regs->pcscntrl);
> > +		else
> > +			writel(readl(&regs->pcscntrl) &
> ~ZYNQ_GEM_PCS_CTL_ANEG_ENBL,
> > +			       &regs->pcscntrl);
> > +	}
> > +#endif
> > +
> >  	ret = clk_set_rate(&priv->tx_clk, clk_rate);
> >  	if (IS_ERR_VALUE(ret)) {
> >  		dev_err(dev, "failed to set tx clock rate\n");
> >
> 
> Karthik/Ashok: Please retest it and reply.

Looks good.

> 
> Thanks,
> Michal