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

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 3

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 patch never programs the 2 Gbps mode on BCM5301x CPU ports
  other than the IMP port.
- [Medium] The fix leaves out BCM583XX (Cygnus/Omega), which the Fixes:
  commit broke in the same way.
- [Low] Concern that b53_phylink_get_caps() advertises only
  MAC_10|MAC_100|MAC_1000 (b53_common.c:1563-1573), while…
- [Low] The comment in b53_phylink_mac_link_up() (b53_common.c:1666)
  reads "Force flow control on BCM5301x and NSP CPU ports", but the…
- [Low] b53_regs.h documents PORT_OVERRIDE_SPEED_2000M (line 103) and
  GMII_PO_SPEED_2000M (line 149) as "BCM5301X only, requires setting…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  b53_force_port_config() mishandles non-IMP ports on is5301x()/is58xx()…

--

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

For fixed-link CPU ports on BCM5301x and BCM58XX (Northstar Plus)
switches, this commit makes b53_phylink_mac_link_up() force speed 2000
along with tx and rx pause. The goal is to restore the IMP port 2 Gbps
mode that b6a8a5477fe9 started clearing, because clearing it left the
switch and AMAC ends of the internal link at different speeds.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 0880310c9ce3f..1f4964c28fc57 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1663,9 +1663,13 @@ 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 on BCM5301x and NSP CPU ports */

[Severity: Low]
This isn't a bug, but the comment only mentions flow control. The block
below it also forces speed = 2000.

The comment also says CPU ports, but the commit message describes the
2 Gbps mode as a property of the IMP port.

Could the comment mention the speed override and say which port it is
meant for?

> +		if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&

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

The clear added by b6a8a5477fe9 applies to every is58xx() chip:

drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
    ...
	if (is5301x(dev) || is58xx(dev))
		reg &= ~PORT_OVERRIDE_SPEED_2000M;
    ...
}

is58xx() includes BCM583XX_DEVICE_ID, but the restore here only matches
BCM58XX_DEVICE_ID.

Cygnus appears to have the same topology as NSP. In bcm-cygnus.dtsi, the
brcm,bcm11360-srab switch (BCM583XX_DEVICE_ID) uses IMP port@8 as the CPU
port. That port has a 1000 full-duplex fixed-link to eth0, a brcm,amac
with no phy-handle.

For that AMAC, bgmac-platform.c uses bgmac_phy_connect_direct() and sets
BGMAC_FEAT_FORCE_SPEED_2500, the same as for NSP amac0.

On boards like bcm911360_entphn, won't b53_force_port_config() still
clear the IMP 2 Gbps bit while the AMAC runs in high-speed mode? If
BCM583XX is left out on purpose, could the commit message say why?

> +		    dsa_is_cpu_port(ds, port)) {
> +			tx_pause = true;
> +			rx_pause = true;
> +			speed = 2000;

[Severity: Medium]
Is the 2 Gbps mode actually programmed when the CPU port is not the IMP
port?

The condition checks dsa_is_cpu_port() but not port == dev->imp_port.
For non-IMP ports, b53_force_port_config() uses the GMII override
register:

drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
    ...
	if (port == dev->imp_port) {
		off = B53_PORT_OVERRIDE_CTRL;
		val = PORT_OVERRIDE_EN;
	} else if (is5325(dev)) {
		return;
	} else {
		off = B53_GMII_PORT_OVERRIDE_CTRL(port);
		val = GMII_PO_EN;
	}
    ...
	case 2000:
		reg |= PORT_OVERRIDE_SPEED_2000M;
		fallthrough;
    ...
}

The two registers have the enable and 2000M bits swapped:

drivers/net/dsa/b53/b53_regs.h:
#define   PORT_OVERRIDE_SPEED_2000M	BIT(6) /* BCM5301X only, requires setting 1000M */
#define   PORT_OVERRIDE_EN		BIT(7) /* Use the register contents */
...
#define   GMII_PO_EN			BIT(6) /* Use the register contents */
#define   GMII_PO_SPEED_2000M		BIT(7) /* BCM5301X only, requires setting 1000M */

On port 5 or 7, the case 2000 path only sets GMII_PO_EN again.
GMII_PO_SPEED_2000M is never written.

In-tree boards take this path. bcm53015-meraki-mr26.dts and
bcm53016-meraki-mr32.dts use port@5 (ethernet = <&gmac0>) as the CPU
port, with a 1000 full-duplex fixed-link and port@8 disabled. On the
GMAC side, bgmac-bcma.c sets BGMAC_FEAT_FORCE_SPEED_2500 for these SoCs.

Unless the bootloader already set BIT(7), won't the internal link on
these boards stay mismatched? The commit message says:

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

This isn't a regression, because BIT(7) was not touched before this
patch either.

Should b53_force_port_config() set GMII_PO_SPEED_2000M for non-IMP ports?
If not, the condition and the commit message could be narrowed to the
IMP port.

[Severity: Low]
This isn't a bug, but b53_regs.h still documents the bit set here as
BCM5301X only:

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

Now that speed = 2000 is also used for BCM58XX_DEVICE_ID, the header and
the code disagree on whether NSP has this bit.

Should the comments for PORT_OVERRIDE_SPEED_2000M and
GMII_PO_SPEED_2000M in b53_regs.h be updated to include the 58xx family?

[Severity: Low]
This isn't a bug, but b53_phylink_get_caps() only advertises MAC_10,
MAC_100 and MAC_1000, and this line replaces the DT fixed-link speed
with 2000.

Phylink still validates the DT value of 1000, and CPU ports have no user
netdev. No in-tree CPU port fixed-link uses a speed other than 1000, so
I couldn't find a functional problem.

Would it be worth noting in the comment that the DT speed is ignored
here on purpose?

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

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
Doesn't b53_force_port_config() disable the override on non-IMP ports of
is5301x() and is58xx() chips?

drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
    ...
	b53_read8(dev, B53_CTRL_PAGE, off, &reg);
	reg |= val;
    ...
	reg &= ~(0x3 << GMII_PO_SPEED_S);
	if (is5301x(dev) || is58xx(dev))
		reg &= ~PORT_OVERRIDE_SPEED_2000M;
    ...
	b53_write8(dev, B53_CTRL_PAGE, off, reg);
}

For non-IMP ports, val is GMII_PO_EN, which is BIT(6). That is the same
bit as PORT_OVERRIDE_SPEED_2000M.

So for any speed other than 2000, the GMII override register is written
with the override disabled. It stays that way until b53_force_link() ORs
GMII_PO_EN back in.

The real 2 Gbps bit for that register, GMII_PO_SPEED_2000M (BIT(7)), is
never cleared either. Suppose the bootloader left a non-IMP port in
2 Gbps mode and DT forces it to 100M or 10M. Wouldn't that produce the
invalid speed and 2000M combination that b6a8a5477fe9 was meant to
prevent?

This patch doesn't make it worse. Non-IMP CPU ports now pass speed 2000,
which sets BIT(6) again.

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