[PATCH net v2 0/2] Fix TCP retransmissions on NS/NSP SoCs

[PATCH net v2 0/2] Fix TCP retransmissions on NS/NSP SoCs

[Florian Fainelli]

On Northstar and Northstar Plus SoCs we would see about 1000 TCP
retransmissions per second due to a rate mismatch between bgmac and b53,
make sure that we run at the same speed on both side to avoid dropping
packets on the floor.

This issue was found during testing on a Northstar Plus reference board
(BCM958625HR) while running iperf3 to a test machine and noticing
retranmsisions:

# iperf3 -c 192.168.3.1
Connecting to host 192.168.3.1, port 5201
[  4] local 192.168.3.204 port 49856 connected to 192.168.3.1 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  4]   0.00-1.00   sec   111 MBytes   933 Mbits/sec  915   99.0 KBytes
[  4]   1.00-2.00   sec   111 MBytes   928 Mbits/sec  909    109 KBytes
[  4]   2.00-3.00   sec   110 MBytes   925 Mbits/sec  1097   99.0 KBytes
[  4]   3.00-4.00   sec   111 MBytes   928 Mbits/sec  814   58.0 KBytes
[  4]   4.00-5.00   sec   111 MBytes   930 Mbits/sec  1187    109 KBytes
[  4]   5.00-6.00   sec   110 MBytes   923 Mbits/sec  893   72.1 KBytes
[  4]   6.00-7.00   sec   111 MBytes   929 Mbits/sec  843   67.9 KBytes
[  4]   7.00-8.00   sec   111 MBytes   928 Mbits/sec  920   79.2 KBytes
[  4]   8.00-9.00   sec   111 MBytes   929 Mbits/sec  1109   66.5 KBytes
[  4]   9.00-10.00  sec   111 MBytes   930 Mbits/sec  1055   63.6 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  4]   0.00-10.00  sec  1.09 GBytes   933 Mbits/sec  9742
sender
[  4]   0.00-10.01  sec  1.09 GBytes   933 Mbits/sec
receiver

Changes in v2:

- make it apply cleanly to net/main

Florian Fainelli (2):
  net: ethernet: bgmac: preserve forced 2.5 Gbps link speed
  net: dsa: b53: restore the NSP IMP port 2 Gbps mode

 drivers/net/dsa/b53/b53_common.c      | 10 +++++++---
 drivers/net/ethernet/broadcom/bgmac.c |  8 ++++++--
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH net v2 1/2] net: ethernet: bgmac: preserve forced 2.5 Gbps link speed

[Florian Fainelli]

Northstar and Northstar Plus use a 2.5 Gbps internal link between the
AMAC and the switch.  BGMAC_FEAT_FORCE_SPEED_2500 makes bgmac_miiconfig()
configure that speed, but bgmac_adjust_link() subsequently replaces it
with the synthetic fixed PHY's 1 Gbps speed.

Use the forced speed when processing link changes so the MAC remains at
2.5 Gbps.  Together with configuring the switch IMP port for its 2 Gbps
mode, this eliminates the output queue discards that caused approximately
1000 TCP retransmissions per second during an iperf3 transmit test.

Fixes: f6a95a24957a ("net: ethernet: bgmac: Add platform device support")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 3e9c57196a39..11487ff48e0d 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1415,11 +1415,15 @@ void bgmac_adjust_link(struct net_device *net_dev)
 {
 	struct bgmac *bgmac = netdev_priv(net_dev);
 	struct phy_device *phy_dev = net_dev->phydev;
+	int speed = phy_dev->speed;
 	bool update = false;
 
+	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500)
+		speed = SPEED_2500;
+
 	if (phy_dev->link) {
-		if (phy_dev->speed != bgmac->mac_speed) {
-			bgmac->mac_speed = phy_dev->speed;
+		if (speed != bgmac->mac_speed) {
+			bgmac->mac_speed = speed;
 			update = true;
 		}
 
-- 
2.34.1

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

[Florian Fainelli]

The Northstar Plus IMP port uses the switch's proprietary 2 Gbps mode for
its internal link to the AMAC.  Its fixed-link description reports 1 Gbps,
so applying that speed clears PORT_OVERRIDE_SPEED_2000M and leaves the two
ends of the internal link configured at different speeds.

Force the 2 Gbps mode and flow control for BCM5301x and NSP CPU ports.  The
mismatch caused switch output queue discards on both copper and SerDes
ports, with one discard for every TCP retransmission.  With both ends
restored to their high-speed modes, iperf3 reports no retransmissions.

Fixes: b6a8a5477fe9 ("net: dsa: b53: fix resetting speed and pause on forced link")
Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
---
 drivers/net/dsa/b53/b53_common.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 0880310c9ce3..1f4964c28fc5 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 */
+		if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&
+		    dsa_is_cpu_port(ds, port)) {
+			tx_pause = true;
+			rx_pause = true;
+			speed = 2000;
+		}
 
 		b53_force_port_config(dev, port, speed, duplex,
 				      tx_pause, rx_pause);
-- 
2.34.1

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

[Jonas Gorski]

Hi,

On Thu, Oct 22, 2026 at 11:11 PM Florian Fainelli
<florian.fainelli@broadcom.com> wrote:
>
> The Northstar Plus IMP port uses the switch's proprietary 2 Gbps mode for
> its internal link to the AMAC.  Its fixed-link description reports 1 Gbps,
> so applying that speed clears PORT_OVERRIDE_SPEED_2000M and leaves the two
> ends of the internal link configured at different speeds.

Sounds like the devicetrees are wrong. Shouldn't that fixed-link
description be updated to be 2 Gbps then? ;p

> Force the 2 Gbps mode and flow control for BCM5301x and NSP CPU ports.  The
> mismatch caused switch output queue discards on both copper and SerDes
> ports, with one discard for every TCP retransmission.  With both ends
> restored to their high-speed modes, iperf3 reports no retransmissions.
>
> Fixes: b6a8a5477fe9 ("net: dsa: b53: fix resetting speed and pause on forced link")
> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
> ---
>  drivers/net/dsa/b53/b53_common.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 0880310c9ce3..1f4964c28fc5 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 */

The comment is now outdated, and is missing the forced 2G mode.

> +               if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&
> +                   dsa_is_cpu_port(ds, port)) {
> +                       tx_pause = true;
> +                       rx_pause = true;
> +                       speed = 2000;
> +               }
>
>                 b53_force_port_config(dev, port, speed, duplex,
>                                       tx_pause, rx_pause);
> --
> 2.34.1
>

Best regards,
Jonas

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

[Florian Fainelli]

On 10/26/26 11:31, Jonas Gorski wrote:
> Hi,
> 
> On Thu, Oct 22, 2026 at 11:11 PM Florian Fainelli
> <florian.fainelli@broadcom.com> wrote:
>>
>> The Northstar Plus IMP port uses the switch's proprietary 2 Gbps mode for
>> its internal link to the AMAC.  Its fixed-link description reports 1 Gbps,
>> so applying that speed clears PORT_OVERRIDE_SPEED_2000M and leaves the two
>> ends of the internal link configured at different speeds.
> 
> Sounds like the devicetrees are wrong. Shouldn't that fixed-link
> description be updated to be 2 Gbps then? ;p

That's a fair ask, however that would require introducing 2Gbits/sec all 
over the place, and it's not like we really have a good way to report 
that even through emulated PHY registers. This would not be a bug fix 
however, which this patch set is targeting.

> 
>> Force the 2 Gbps mode and flow control for BCM5301x and NSP CPU ports.  The
>> mismatch caused switch output queue discards on both copper and SerDes
>> ports, with one discard for every TCP retransmission.  With both ends
>> restored to their high-speed modes, iperf3 reports no retransmissions.
>>
>> Fixes: b6a8a5477fe9 ("net: dsa: b53: fix resetting speed and pause on forced link")
>> Signed-off-by: Florian Fainelli <florian.fainelli@broadcom.com>
>> ---
>>   drivers/net/dsa/b53/b53_common.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
>> index 0880310c9ce3..1f4964c28fc5 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 */
> 
> The comment is now outdated, and is missing the forced 2G mode.

Ack.
-- 
Florian

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

[Andrew Lunn]

On Mon, Oct 26, 2026 at 11:34:36AM -0700, Florian Fainelli wrote:
> On 10/26/26 11:31, Jonas Gorski wrote:
> > Hi,
> > 
> > On Thu, Oct 22, 2026 at 11:11 PM Florian Fainelli
> > <florian.fainelli@broadcom.com> wrote:
> > > 
> > > The Northstar Plus IMP port uses the switch's proprietary 2 Gbps mode for
> > > its internal link to the AMAC.  Its fixed-link description reports 1 Gbps,
> > > so applying that speed clears PORT_OVERRIDE_SPEED_2000M and leaves the two
> > > ends of the internal link configured at different speeds.
> > 
> > Sounds like the devicetrees are wrong. Shouldn't that fixed-link
> > description be updated to be 2 Gbps then? ;p
> 
> That's a fair ask, however that would require introducing 2Gbits/sec all
> over the place, and it's not like we really have a good way to report that
> even through emulated PHY registers. This would not be a bug fix however,
> which this patch set is targeting.

Hi Florian

As far as i know phylink fixed-link does not care what the speed is,
and it does not try to emulate it in the PHY. So it probably just
works. Assuming the DSA driver is using phylink.

       Andrew

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

[Florian Fainelli]

On 10/26/26 12:11, Andrew Lunn wrote:
> On Mon, Oct 26, 2026 at 11:34:36AM -0700, Florian Fainelli wrote:
>> On 10/26/26 11:31, Jonas Gorski wrote:
>>> Hi,
>>>
>>> On Thu, Oct 22, 2026 at 11:11 PM Florian Fainelli
>>> <florian.fainelli@broadcom.com> wrote:
>>>>
>>>> The Northstar Plus IMP port uses the switch's proprietary 2 Gbps mode for
>>>> its internal link to the AMAC.  Its fixed-link description reports 1 Gbps,
>>>> so applying that speed clears PORT_OVERRIDE_SPEED_2000M and leaves the two
>>>> ends of the internal link configured at different speeds.
>>>
>>> Sounds like the devicetrees are wrong. Shouldn't that fixed-link
>>> description be updated to be 2 Gbps then? ;p
>>
>> That's a fair ask, however that would require introducing 2Gbits/sec all
>> over the place, and it's not like we really have a good way to report that
>> even through emulated PHY registers. This would not be a bug fix however,
>> which this patch set is targeting.
> 
> Hi Florian
> 
> As far as i know phylink fixed-link does not care what the speed is,
> and it does not try to emulate it in the PHY. So it probably just
> works. Assuming the DSA driver is using phylink.

Can we agree this is outside of the scope of what is being fixed here?

Without updates to Device Tree binding, swphy, phylink and the drivers 
that do depend upon supporting SPEED_2000, that speed is going to be 
rejected or not acted on at various layers in the stack.
-- 
Florian

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

[Andrew Lunn]

On Mon, Oct 26, 2026 at 12:34:05PM -0700, Florian Fainelli wrote:
> On 10/26/26 12:11, Andrew Lunn wrote:
> > On Mon, Oct 26, 2026 at 11:34:36AM -0700, Florian Fainelli wrote:
> > > On 10/26/26 11:31, Jonas Gorski wrote:
> > > > Hi,
> > > > 
> > > > On Thu, Oct 22, 2026 at 11:11 PM Florian Fainelli
> > > > <florian.fainelli@broadcom.com> wrote:
> > > > > 
> > > > > The Northstar Plus IMP port uses the switch's proprietary 2 Gbps mode for
> > > > > its internal link to the AMAC.  Its fixed-link description reports 1 Gbps,
> > > > > so applying that speed clears PORT_OVERRIDE_SPEED_2000M and leaves the two
> > > > > ends of the internal link configured at different speeds.
> > > > 
> > > > Sounds like the devicetrees are wrong. Shouldn't that fixed-link
> > > > description be updated to be 2 Gbps then? ;p
> > > 
> > > That's a fair ask, however that would require introducing 2Gbits/sec all
> > > over the place, and it's not like we really have a good way to report that
> > > even through emulated PHY registers. This would not be a bug fix however,
> > > which this patch set is targeting.
> > 
> > Hi Florian
> > 
> > As far as i know phylink fixed-link does not care what the speed is,
> > and it does not try to emulate it in the PHY. So it probably just
> > works. Assuming the DSA driver is using phylink.
> 
> Can we agree this is outside of the scope of what is being fixed here?

Yes, it can be done sometime in the future.

     Andrew

Re: [PATCH net v2 1/2] net: ethernet: bgmac: preserve forced 2.5 Gbps link speed

[netdev-bot+sashiko]

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 override in bgmac_adjust_link()
  (drivers/net/ethernet/broadcom/bgmac.c:1421-1427) sets UMAC_CMD to…
- [Medium] On FORCE_SPEED_2500 devices, the patch makes link reporting
  disagree with the hardware, and the commit message does not say so.
- [Low] In bgmac-bcma.c, bgmac_probe() sets BGMAC_FEAT_FORCE_SPEED_2500
  from the chip ID alone (BCM4707/BCM47094/BCM53018, line 282).

--

net: ethernet: bgmac: preserve forced 2.5 Gbps link speed

This patch changes bgmac_adjust_link() to use SPEED_2500 when
BGMAC_FEAT_FORCE_SPEED_2500 is set. Without it, the 1 Gbps speed of the
fixed PHY overwrites the 2.5 Gbps setting from bgmac_miiconfig().
Together with the switch IMP port 2 Gbps change, it is meant to remove
output queue discards on Northstar and Northstar Plus.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 3e9c57196a397..11487ff48e0d7 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1415,11 +1415,15 @@ void bgmac_adjust_link(struct net_device *net_dev)
>  {
>  	struct bgmac *bgmac = netdev_priv(net_dev);
>  	struct phy_device *phy_dev = net_dev->phydev;
> +	int speed = phy_dev->speed;
>  	bool update = false;
>  
> +	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500)
> +		speed = SPEED_2500;

[Severity: Medium]
This programs CMD_SPEED_2500 on every AMAC that has
BGMAC_FEAT_FORCE_SPEED_2500. Is the switch side always in its 2 Gbps mode
for those AMACs?

The companion b53 change only passes speed = 2000 for MLO_AN_FIXED CPU
ports on is5301x() or BCM58XX_DEVICE_ID, in b53_phylink_mac_link_up():

	if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&
	    dsa_is_cpu_port(ds, port)) {
		tx_pause = true;
		rx_pause = true;
		speed = 2000;

On Northstar, gmac0 and gmac1 connect to switch ports 5 and 7
(bcm-ns.dtsi). On many in-tree boards, port 5 is the CPU link and port 8
is disabled. Examples are bcm4708-netgear-r6250, Luxul XAP/XWR/XBR/ABR,
Buffalo WZR, SmartRG SR400AC, Meraki MR26/MR32 and bcm953012er.

If port 5 has no fixed-link, dsa_shared_port_link_register_of() skips
phylink registration, and b53_phylink_mac_link_up() is never called.

If port 5 has a fixed-link (MR26/MR32), b53_force_port_config() writes
B53_GMII_PORT_OVERRIDE_CTRL(5) with PORT_OVERRIDE_SPEED_2000M, which is
BIT(6):

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

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

In the GMII override register, BIT(6) is the enable bit and the 2G bit is
BIT(7):

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

On these boards, does the MAC now run at 2.5G against a switch port that
the kernel never puts into 2G mode? The port is only in 2G mode if the
bootloader left it that way.

Cygnus looks similar. In bcm911360_entphn.dts, eth0 is brcm,amac with no
phy-handle, so bgmac_probe() in bgmac-platform.c sets
BGMAC_FEAT_FORCE_SPEED_2500.

The switch there is brcm,cygnus-srab, which maps to BCM583XX_DEVICE_ID.
The chip_id == BCM58XX_DEVICE_ID check above does not match that ID, so
b53_force_port_config(dev, 8, 1000, ...) runs. Because is58xx() includes
BCM583XX, it clears PORT_OVERRIDE_SPEED_2000M.

Before this patch, both ends of the Cygnus link were at 1G. After the
series, would the MAC be at 2.5G and the IMP port at 1G? The series names
that kind of mismatch as the cause of the output queue discards.

Also, if this patch is applied alone (a bisect point, or a stable backport
based only on the Fixes: tag), the IMP port boards look mismatched too,
because b6a8a5477fe9 already clears the 2000M bit. The companion b53 patch
covers that case. Should the two patches be kept together for backports?

[Severity: Low]
The override depends only on the feature flag. In bgmac-bcma.c,
bgmac_probe() sets BGMAC_FEAT_FORCE_SPEED_2500 from the chip ID for the
whole 4707 family. bcma_phy_connect() tries a DT phy-handle first:

	phy_dev = of_phy_get_and_connect(bgmac->net_dev, bgmac->dev->of_node,
					 bgmac_adjust_link);

If a phy-handle attached a real autonegotiating PHY, would this ignore the
negotiated 10/100/1000 speed and program CMD_SPEED_2500?

Duplex is still copied from phy_dev->duplex. A half-duplex result would
therefore give CMD_SPEED_2500 | CMD_HD_EN.

No in-tree 4707-family DT does this, and that family registers no MDIO
bus. bgmac-platform.c sets the flag only on the bgmac_phy_connect_direct()
path, so the two glue drivers use the flag differently.

Could the override be limited to the direct fixed PHY case, or could
DUPLEX_FULL be forced together with the speed?

> +
>  	if (phy_dev->link) {
> -		if (phy_dev->speed != bgmac->mac_speed) {
> -			bgmac->mac_speed = phy_dev->speed;
> +		if (speed != bgmac->mac_speed) {
> +			bgmac->mac_speed = speed;
>  			update = true;
>  		}

[Severity: Medium]
Is this branch ever taken on FORCE_SPEED_2500 devices now?

bgmac_open() calls bgmac_chip_reset() before phy_start().
bgmac_chip_reset()->bgmac_miiconfig() has already set:

		bgmac->mac_speed = SPEED_2500;
		bgmac->mac_duplex = DUPLEX_FULL;
		bgmac_mac_speed(bgmac);

When the fixed PHY reports link up at 1000/full, speed is forced to 2500
and both comparisons match, so update stays false. Does that mean
phy_print_status() is never called, and the "Link is Up" message is lost
on every open? Before this patch, the 1000 vs 2500 difference made this
branch run on each link up.

The attached PHY also still reports 1000. That is either the synthetic
fixed PHY in bgmac_phy_connect_direct() (.speed = SPEED_1000) or the DT
fixed-link in bcm-ns.dtsi (speed = <1000>). So ethtool and
/sys/class/net/*/speed show 1 Gbps while UMAC_CMD is set for 2.5 Gbps.

Should the commit message mention this reporting change, or should the
reported speed be made to match?

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

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

[netdev-bot+sashiko]

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

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

[Florian Fainelli]

On 10/26/26 14:24, netdev-bot+sashiko@kernel.org wrote:
> 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?

v3 indicates that the speed is forced as well now, that covers the DT 
speed being ignored by extension.

> 
>> +		}
>>   
>>   		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?

It could, but that's a pre-existing issue so it needs to be addressed 
differently.

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

Correct, there will be a separate set of patches to for the use of IMP 
port being 8, as well as well as reject invalid settings.
-- 
Florian