Re: [PATCH net v4 2/2] net: dsa: b53: restore the NSP IMP port 2 Gbps mode

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new code in b53_phylink_mac_link_up()
  (drivers/net/dsa/b53/b53_common.c) sets speed = 2000 on every…

--

net: dsa: b53: restore the NSP IMP port 2 Gbps mode

On BCM5301x and NSP, fixed-link CPU ports now get forced flow control and a
2 Gbps speed in b53_phylink_mac_link_up(). The goal is to restore the
proprietary 2 Gbps mode on the IMP port. That mode was lost because the
fixed-link description reports 1 Gbps.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 0880310c9ce3f..743fc04da0a48 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1663,9 +1663,15 @@ static void b53_phylink_mac_link_up(struct phylink_config *config,
>  	}
>  
>  	if (mode == MLO_AN_FIXED) {
> -		/* Force flow control on BCM5301x's CPU port */
> -		if (is5301x(dev) && dsa_is_cpu_port(ds, port))
> -			tx_pause = rx_pause = true;
> +		/* Force flow control and 2Gbits/sec on BCM5301x and NSP CPU
> +		 * ports
> +		 */
> +		if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&
> +		    dsa_is_cpu_port(ds, port)) {
> +			tx_pause = true;
> +			rx_pause = true;
> +			speed = 2000;
> +		}

[Severity: Medium]
Does this set up 2 Gbps correctly when the CPU port is not the IMP port?

The check is dsa_is_cpu_port() rather than port == dev->imp_port, so a
fixed-link CPU port on port 5 or 7 also gets speed = 2000. Those ports go
through the GMII override register in b53_force_port_config():

drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
    ...
	} else {
		off = B53_GMII_PORT_OVERRIDE_CTRL(port);
		val = GMII_PO_EN;
	}
    ...
	reg &= ~(0x3 << GMII_PO_SPEED_S);
	if (is5301x(dev) || is58xx(dev))
		reg &= ~PORT_OVERRIDE_SPEED_2000M;

	switch (speed) {
	case 2000:
		reg |= PORT_OVERRIDE_SPEED_2000M;
		fallthrough;
    ...
}

The two registers use different bit layouts in b53_regs.h:

#define   PORT_OVERRIDE_SPEED_2000M	BIT(6)
#define   PORT_OVERRIDE_EN		BIT(7)
...
#define   GMII_PO_EN			BIT(6)
#define   GMII_PO_SPEED_2000M		BIT(7)

So on the GMII register, the 2000 case sets and clears BIT(6), which is
GMII_PO_EN. GMII_PO_SPEED_2000M (BIT(7)) is never written. The port ends up
at 1 Gbps, and its 2 Gbps bit keeps whatever value it already had.

Some upstream 5301x device trees use port 5 as a fixed-link CPU port, for
example bcm53015-meraki-mr26.dts and bcm53016-meraki-mr32.dts. bgmac-bcma
sets BGMAC_FEAT_FORCE_SPEED_2500 for these GMACs, so the speed mismatch the
commit message describes would remain on those links.

The new comment and the commit message both say 2 Gbps is forced on
"BCM5301x and NSP CPU ports". Should this either be limited to
port == dev->imp_port, with the comment updated to match, or should
b53_force_port_config() use GMII_PO_SPEED_2000M for the GMII override
register?

>  
>  		b53_force_port_config(dev, port, speed, duplex,
>  				      tx_pause, rx_pause);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261029211540.4123250-1-florian.fainelli%40broadcom.com