lkml.org 
[lkml]   [2021]   [Sep]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v1 net] net: ethernet: mscc: ocelot: bug fix when writing MAC speed
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.

\
 
 \ /
  Last update: 2021-09-16 02:31    [W:0.075 / U:3.116 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site