Re: [PATCH net v3 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 3 potential
issue(s) to consider.

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

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() sets speed = 2000
  for any port where dsa_is_cpu_port() is true on BCM5301x.
- [Medium] The fix covers fewer chips than the regression it reverses.
- [Low] b53_regs.h documents PORT_OVERRIDE_SPEED_2000M (line 103) and
  GMII_PO_SPEED_2000M (line 149) as "BCM5301X only, requires setting…

--

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

This commit forces 2 Gbps mode and flow control on fixed-link CPU ports of
BCM5301x and Northstar Plus (BCM58XX) switches in b53_phylink_mac_link_up().
It restores the PORT_OVERRIDE_SPEED_2000M bit, which b53_force_port_config()
clears when the 1 Gbps fixed-link speed is applied. Clearing that bit
caused queue discards and TCP retransmissions.

> 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)) {

[Severity: Medium]
Does this actually force 2 Gbps on BCM5301x CPU ports that are not the IMP
port?

The condition matches any port where dsa_is_cpu_port() is true. However,
b53_force_port_config() only handles 2 Gbps correctly when
port == dev->imp_port, which is 8 on 5301x/58xx. Other ports go through
the GMII override register:

b53_force_port_config() {
    ...
	} else {
		off = B53_GMII_PORT_OVERRIDE_CTRL(port);
		val = GMII_PO_EN;
	}
    ...
	if (is5301x(dev) || is58xx(dev))
		reg &= ~PORT_OVERRIDE_SPEED_2000M;

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

PORT_OVERRIDE_SPEED_2000M is BIT(6) in the IMP register layout. The GMII
per-port register has these two bits the other way round:

#define   GMII_PO_EN			BIT(6) /* Use the register contents */
#define   GMII_PO_SPEED_2000M		BIT(7) /* BCM5301X only, requires setting 1000M */

On a non-IMP CPU port, case 2000 just sets GMII_PO_EN again and falls
through to 1000M. GMII_PO_SPEED_2000M never gets programmed.

This looks reachable in-tree on bcm53015-meraki-mr26.dts and
bcm53016-meraki-mr32.dts. On both boards port@5 is the CPU port, with
ethernet = <&gmac0> from bcm-ns.dtsi and a 1000/full fixed-link, and gmac0
runs with BGMAC_FEAT_FORCE_SPEED_2500. Most other boards that use port@5
as a CPU port have no fixed-link, so mac_link_up is never called for
them.

The Fixes commit b6a8a5477fe9 cleared BIT(6), not BIT(7), on the GMII
register, so it never wiped a GMII_PO_SPEED_2000M set by the bootloader
on port 5. Whether these boards still have a mismatch depends on what the
bootloader set. Either way, this statement in the commit message is only
true for the IMP port:

    Force the 2 Gbps mode and flow control for BCM5301x and NSP CPU ports.

Should the speed override be limited to port == dev->imp_port, with the
commit message reworded? Or should b53_force_port_config() use
GMII_PO_SPEED_2000M for non-IMP ports?

[Severity: Medium]
Should BCM583XX_DEVICE_ID be covered here too?

The clear in b53_force_port_config(), added by b6a8a5477fe9, runs for
every is58xx() chip:

	if (is5301x(dev) || is58xx(dev))
		reg &= ~PORT_OVERRIDE_SPEED_2000M;

is58xx() includes BCM58XX, BCM583XX, BCM7445, BCM7278 and BCM53134. The
restore added here only covers is5301x() and BCM58XX_DEVICE_ID.

BCM583XX (Cygnus/BCM11360/Omega, through the brcm,bcm11360-srab,
brcm,cygnus-srab and brcm,omega-srab compatibles in b53_srab.c) seems to
have the same topology as NSP:

- In bcm-cygnus.dtsi, port@8 is the CPU port, with ethernet = <&eth0>
  and a 1000/full fixed-link.
- eth0 is a brcm,amac with no phy-handle, so bgmac_probe() in
  bgmac-platform.c sets BGMAC_FEAT_FORCE_SPEED_2500.
- bcm911360_entphn.dts enables both &eth0 and &switch.

If the Cygnus IMP uses the same 2 Gbps mode as NSP, the IMP 2000M bit
would still be cleared on every fixed-link link-up while the AMAC stays in
its high-speed mode. Wouldn't that leave the mismatch the commit message
describes in place on that SoC?

Could the clear and the restore use the same chip predicate?

> +			tx_pause = true;
> +			rx_pause = true;
> +			speed = 2000;

[Severity: Low]
This isn't a bug, but b53_regs.h still says the bit is 5301x only:

#define   PORT_OVERRIDE_SPEED_2000M	BIT(6) /* BCM5301X only, requires setting 1000M */

GMII_PO_SPEED_2000M carries the same comment. This change now sets
PORT_OVERRIDE_SPEED_2000M on BCM58XX IMP ports, and b53_force_port_config()
already clears it on all is58xx() chips. Should that comment be updated?

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

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