[PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed

[PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed

[Colin Foster]

Converting the ocelot driver to use phylink, commit e6e12df625f2, uses mac_speed
in ocelot_phylink_mac_link_up instead of the local variable speed. Stale
references to the old variable were missed, and so were always performing
invalid second writes to the DEV_CLOCK_CFG and ANA_PFC_CFG registers.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index c581b955efb3..91a31523be8f 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -566,11 +566,11 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
 	/* Take MAC, Port, Phy (intern) and PCS (SGMII/Serdes) clock out of
 	 * reset
 	 */
-	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(speed),
+	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed),
 			   DEV_CLOCK_CFG);
 
 	/* No PFC */
-	ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(speed),
+	ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(mac_speed),
 			 ANA_PFC_PFC_CFG, port);
 
 	/* Core: Enable port for frame transfer */
-- 
2.25.1

Re: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed

[Colin Foster]

On Tue, Sep 14, 2021 at 11:21:02PM -0700, Colin Foster wrote:
> Converting the ocelot driver to use phylink, commit e6e12df625f2, uses mac_speed
> in ocelot_phylink_mac_link_up instead of the local variable speed. Stale
> references to the old variable were missed, and so were always performing
> invalid second writes to the DEV_CLOCK_CFG and ANA_PFC_CFG registers.

In my negligence I ran check_patch after sending this and didn't add the
title line to my reference commit. I can resubmit if everything else
looks good.

Re: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed

[Vladimir Oltean]

On Tue, Sep 14, 2021 at 11:21:02PM -0700, Colin Foster wrote:
> Converting the ocelot driver to use phylink, commit e6e12df625f2, uses mac_speed
> in ocelot_phylink_mac_link_up instead of the local variable speed. Stale
> references to the old variable were missed, and so were always performing
> invalid second writes to the DEV_CLOCK_CFG and ANA_PFC_CFG registers.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index c581b955efb3..91a31523be8f 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -566,11 +566,11 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
>  	/* Take MAC, Port, Phy (intern) and PCS (SGMII/Serdes) clock out of
>  	 * reset
>  	 */
> -	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(speed),
> +	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed),
>  			   DEV_CLOCK_CFG);

Oh wow, I don't know how this piece did not get deleted. We write twice
to DEV_CLOCK_CFG, once with a good value and once with a bad value.
Please delete the second write entirely.

>  
>  	/* No PFC */
> -	ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(speed),
> +	ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(mac_speed),
>  			 ANA_PFC_PFC_CFG, port);

Both were supposed to be deleted in fact.
See, if priority flow control is disabled, it does not matter what is
the speed the port is operating at, so the write is useless.

Also, setting the FC_LINK_SPEED in ANA_PFC_PFC_CFG to mac_speed is not
quite correct for Felix/Seville, even if we were to enable PFC. The
documentation says:

Configures the link speed. This is used to
evaluate the time specifications in incoming
pause frames.
0: 2500 Mbps
1: 1000 Mbps
2: 100 Mbps
3: 10 Mbps

But mac_speed is always 1000 Mbps for Felix/Seville (1), due to
OCELOT_QUIRK_PCS_PERFORMS_RATE_ADAPTATION. If we were to set the correct
speed for the PFC PAUSE quanta, we'd need to introduce yet a third
variable, fc_link_speed, which is set similar to how mac_fc_cfg is, but
using the ANA_PFC_PFC_CFG_FC_LINK_SPEED macro instead of SYS_MAC_FC_CFG_FC_LINK_SPEED.
In other words, DEV_CLOCK_CFG may be fixed at 1000 Mbps, but if the port
operates at 100 Mbps via PCS rate adaptation, the PAUSE quanta values
in the MAC still need to be adapted.

>  
>  	/* Core: Enable port for frame transfer */
> -- 
> 2.25.1
> 

So please restructure the patch to delete both assignments (maybe even
create two patches, one for each), and rewrite your commit message and
title to a more canonical format.

Example:
"net: mscc: ocelot: remove buggy duplicate write to DEV_CLOCK_CFG")
"net: mscc: ocelot: remove buggy and useless write to ANA_PFC_PFC_CFG")

Be sure to include this at the end:
Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink")

So that it catches the stable maintainers' eyes.

Re: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed

[Colin Foster]

On Wed, Sep 15, 2021 at 12:25:52PM +0000, Vladimir Oltean wrote:
> On Tue, Sep 14, 2021 at 11:21:02PM -0700, Colin Foster wrote:
> > Converting the ocelot driver to use phylink, commit e6e12df625f2, uses mac_speed
> > in ocelot_phylink_mac_link_up instead of the local variable speed. Stale
> > references to the old variable were missed, and so were always performing
> > invalid second writes to the DEV_CLOCK_CFG and ANA_PFC_CFG registers.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> >  drivers/net/ethernet/mscc/ocelot.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> > index c581b955efb3..91a31523be8f 100644
> > --- a/drivers/net/ethernet/mscc/ocelot.c
> > +++ b/drivers/net/ethernet/mscc/ocelot.c
> > @@ -566,11 +566,11 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
> >  	/* Take MAC, Port, Phy (intern) and PCS (SGMII/Serdes) clock out of
> >  	 * reset
> >  	 */
> > -	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(speed),
> > +	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed),
> >  			   DEV_CLOCK_CFG);
> 
> Oh wow, I don't know how this piece did not get deleted. We write twice
> to DEV_CLOCK_CFG, once with a good value and once with a bad value.
> Please delete the second write entirely.

It seemed odd to me at first, but I looked into the datasheet and saw
that multiple writes to the DEV_CLOCK_CFG register in section 5.2.1
https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf

11. Set up the switch port to the new mode of operation. Keep the reset bits in CLOCK_CFG set.
12. Release the switch port from reset by clearing the reset bits in CLOCK_CFG.

From that it seems like maybe the routine must be two-fold. First a
write to set up the link speed with:
ocelot_port_rmwl(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed), 
                 DEV_CLOCK_CFG_LINK_SPEED_M, DEV_CLK_CFG);
ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed),
                   DEV_CLK_CFG);

Of note: I'm currently only using the VSC7512 ports 0-3, which don't
have a PCS and therefore don't have the PCS_RATE_ADAPTATION requirement.
So all of my ports should be good test candidates for this.

> 
> >  
> >  	/* No PFC */
> > -	ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(speed),
> > +	ocelot_write_gix(ocelot, ANA_PFC_PFC_CFG_FC_LINK_SPEED(mac_speed),
> >  			 ANA_PFC_PFC_CFG, port);
> 
> Both were supposed to be deleted in fact.
> See, if priority flow control is disabled, it does not matter what is
> the speed the port is operating at, so the write is useless.
> 
> Also, setting the FC_LINK_SPEED in ANA_PFC_PFC_CFG to mac_speed is not
> quite correct for Felix/Seville, even if we were to enable PFC. The
> documentation says:
> 
> Configures the link speed. This is used to
> evaluate the time specifications in incoming
> pause frames.
> 0: 2500 Mbps
> 1: 1000 Mbps
> 2: 100 Mbps
> 3: 10 Mbps
> 
> But mac_speed is always 1000 Mbps for Felix/Seville (1), due to
> OCELOT_QUIRK_PCS_PERFORMS_RATE_ADAPTATION. If we were to set the correct
> speed for the PFC PAUSE quanta, we'd need to introduce yet a third
> variable, fc_link_speed, which is set similar to how mac_fc_cfg is, but
> using the ANA_PFC_PFC_CFG_FC_LINK_SPEED macro instead of SYS_MAC_FC_CFG_FC_LINK_SPEED.
> In other words, DEV_CLOCK_CFG may be fixed at 1000 Mbps, but if the port
> operates at 100 Mbps via PCS rate adaptation, the PAUSE quanta values
> in the MAC still need to be adapted.

This makes sense. And I'm hopeful once I get around to the rest of the
device's functionality (external phy / fiber) this level of
understanding will be second nature for me.

> 
> >  
> >  	/* Core: Enable port for frame transfer */
> > -- 
> > 2.25.1
> > 
> 
> So please restructure the patch to delete both assignments (maybe even
> create two patches, one for each), and rewrite your commit message and
> title to a more canonical format.

I'll make a submission for the PFC with the suggested formats. Thank you
for the feedback.

With CLOCK_CFG, there could be the fix for the duplicate write, and a
second patch to do the reconfigure while the port is still in reset. Or
a single patch that does both, since this behavior seems to have existed
in ocelot_adjust_link.

> 
> Example:
> "net: mscc: ocelot: remove buggy duplicate write to DEV_CLOCK_CFG")
> "net: mscc: ocelot: remove buggy and useless write to ANA_PFC_PFC_CFG")
> 
> Be sure to include this at the end:
> Fixes: e6e12df625f2 ("net: mscc: ocelot: convert to phylink")
> 
> So that it catches the stable maintainers' eyes.

Thanks as always for your direction.

Re: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed

[Vladimir Oltean]

On Wed, Sep 15, 2021 at 05:29:57PM -0700, Colin Foster wrote:
> On Wed, Sep 15, 2021 at 12:25:52PM +0000, Vladimir Oltean wrote:
> > On Tue, Sep 14, 2021 at 11:21:02PM -0700, Colin Foster wrote:
> > > Converting the ocelot driver to use phylink, commit e6e12df625f2, uses mac_speed
> > > in ocelot_phylink_mac_link_up instead of the local variable speed. Stale
> > > references to the old variable were missed, and so were always performing
> > > invalid second writes to the DEV_CLOCK_CFG and ANA_PFC_CFG registers.
> > > 
> > > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > > ---
> > >  drivers/net/ethernet/mscc/ocelot.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> > > index c581b955efb3..91a31523be8f 100644
> > > --- a/drivers/net/ethernet/mscc/ocelot.c
> > > +++ b/drivers/net/ethernet/mscc/ocelot.c
> > > @@ -566,11 +566,11 @@ void ocelot_phylink_mac_link_up(struct ocelot *ocelot, int port,
> > >  	/* Take MAC, Port, Phy (intern) and PCS (SGMII/Serdes) clock out of
> > >  	 * reset
> > >  	 */
> > > -	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(speed),
> > > +	ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed),
> > >  			   DEV_CLOCK_CFG);
> > 
> > Oh wow, I don't know how this piece did not get deleted. We write twice
> > to DEV_CLOCK_CFG, once with a good value and once with a bad value.
> > Please delete the second write entirely.
> 
> It seemed odd to me at first, but I looked into the datasheet and saw
> that multiple writes to the DEV_CLOCK_CFG register in section 5.2.1
> https://ww1.microchip.com/downloads/en/DeviceDoc/VMDS-10491.pdf
> 
> 11. Set up the switch port to the new mode of operation. Keep the reset bits in CLOCK_CFG set.
> 12. Release the switch port from reset by clearing the reset bits in CLOCK_CFG.
> 
> From that it seems like maybe the routine must be two-fold. First a
> write to set up the link speed with:
> ocelot_port_rmwl(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed), 
>                  DEV_CLOCK_CFG_LINK_SPEED_M, DEV_CLK_CFG);
> ocelot_port_writel(ocelot_port, DEV_CLOCK_CFG_LINK_SPEED(mac_speed),
>                    DEV_CLK_CFG);

Yes, that cycle is supposed to be completed by ocelot_phylink_mac_link_down
which does put the port in reset, and ocelot_phylink_mac_link_up puts it
back out of it. The third write to DEV_CLOCK_CFG is simply nonsensical.

> Of note: I'm currently only using the VSC7512 ports 0-3, which don't
> have a PCS and therefore don't have the PCS_RATE_ADAPTATION requirement.

Observation: as long as Alex's original code is correct, then the
PCS_RATE_ADAPTATION quirk is only needed for an _NXP_ PCS. The VSC7514
family uses a Microchip PCS1G block and the code unconditionally changes
the DEV_CLOCK_CFG_LINK_SPEED, so if this is correct, it means that
different PCS blocks have different requirements.