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@xxxxxxxxxxxxxxxx>
> ---
>  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.