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

[Ashok Reddy Soma]

Reviewed-by: Ashok Reddy Soma <ashok.reddy.s...@xilinx.com>

> -----Original Message-----
> From: Michal Simek <michal.si...@xilinx.com>
> Sent: Monday, March 15, 2021 2:38 PM
> To: Robert Hancock <robert.hanc...@calian.com>; Michal Simek
> <mich...@xilinx.com>; T Karthik Reddy <tkart...@xilinx.com>; Ashok Reddy
> Soma <ashok...@xilinx.com>
> Cc: joe.hershber...@ni.com; rfried....@gmail.com; u-boot@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.hanc...@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