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

[Colin Foster <colin.foster@in-advantage.com>]

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.