Re: [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:

quoted

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.

quoted

 
 	/* 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.

quoted

 
 	/* 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.