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

[PATCH net v3 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 v3:

- correct commend in the second patch about what is being forced
  on the CPU port, it's not just flow control, it's also speed

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      | 12 +++++++++---
 drivers/net/ethernet/broadcom/bgmac.c |  8 ++++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

-- 
2.53.0

[PATCH net v3 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 b3a92ae95de3..ec879c203bc5 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1417,11 +1417,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.53.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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 steady-state 2.5G override also hits non-Northstar
  platform AMACs.
- [Low] On forced-2500 devices, the programmed MAC speed and the reported
  link speed now disagree.

--

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

This commit makes bgmac_adjust_link() use SPEED_2500 instead of the fixed
PHY's reported speed when BGMAC_FEAT_FORCE_SPEED_2500 is set. The goal is
to keep the AMAC at 2.5 Gbps on Northstar and Northstar Plus. Without it,
the first link change reprograms the MAC to 1 Gbps.

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index b3a92ae95de30..ec879c203bc59 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1417,11 +1417,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]
Does this reach more than Northstar and Northstar Plus? bgmac_probe() in
bgmac-platform.c sets the flag for any platform AMAC that has no
phy-handle, whatever the SoC family:

drivers/net/ethernet/broadcom/bgmac-platform.c:bgmac_probe() {
    ...
	} else {
		bgmac->phy_connect = bgmac_phy_connect_direct;
		bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
	}
    ...
}

Before this patch, the 2.5G setting from bgmac_miiconfig() only lasted
until the first bgmac_adjust_link(). That call reprogrammed the MAC to the
fixed PHY's 1000. With this change, 2.5G becomes the permanent MAC speed.

The in-tree Cygnus board bcm911360_entphn.dts enables eth0 ("brcm,amac"
in bcm-cygnus.dtsi) without a phy-handle, so it gets the flag. Its switch
peer is "brcm,bcm11360-srab" (BCM583XX_DEVICE_ID), and CPU port@8 there is
a fixed-link with speed = <1000>.

The companion change "net: dsa: b53: restore the NSP IMP port 2 Gbps mode"
only forces the IMP port to 2000 for is5301x() or BCM58XX_DEVICE_ID:

drivers/net/dsa/b53/b53_common.c:b53_phylink_mac_link_up() {
    ...
		if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&
		    dsa_is_cpu_port(ds, port)) {
    ...
}

Would that leave the Cygnus AMAC at 2.5G and its IMP port at 1G? That is
the same mismatch the series describes as the cause of the output queue
discards.

Hurricane2 ("brcm,nsp-amac") and Stingray ("brcm,amac") would be treated
the same way if their DT had no phy-handle. In-tree Stingray boards do set
phy-handle, and no in-tree HR2 board enables amac0.

On parts without idm_base, bgmac_miiconfig() skips the 2.5G IOCTL clock
select. The MAC speed field would still stay at CMD_SPEED_2500:

drivers/net/ethernet/broadcom/bgmac.c:bgmac_miiconfig() {
	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500) {
		if (!(bgmac->feature_flags & BGMAC_FEAT_IDM_MASK)) {
			bgmac_idm_write(bgmac, BCMA_IOCTL,
					bgmac_idm_read(bgmac, BCMA_IOCTL) |
					0x40 | BGMAC_BCMA_IOCTL_SW_CLKEN);
		}
    ...
}

Should the override be limited to the SoCs that really run this link at
2.5G?

>  	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: Low]
After this change, can update ever become true on forced 2.5G devices?

bgmac_chip_reset()->bgmac_miiconfig() caches these values before the PHY
state machine runs:

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

The fixed PHY reports DUPLEX_FULL, so both comparisons in
bgmac_adjust_link() come out false and this block is skipped:

	if (update) {
		bgmac_mac_speed(bgmac);
		phy_print_status(phy_dev);
	}

Before the patch, the first link-up saw 1000 != 2500, reprogrammed the MAC
and printed the "Link is Up" message. That message seems to be gone on
these boards now.

Also, phy_dev->speed stays at 1000. bgmac_phy_connect_direct() registers
the synthetic fixed PHY with .speed = SPEED_1000, the DT fixed-link uses
speed = <1000>, and swphy can't represent 2500 either way.

bgmac_ethtool_ops uses phy_ethtool_get_link_ksettings(). Would ethtool and
/sys/class/net/<if>/speed then report 1000 Mb/s while the MAC is
programmed for 2.5G?

Should the commit message mention these user-visible changes?

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

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

[Florian Fainelli]

On 10/29/26 13:39, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · 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 steady-state 2.5G override also hits non-Northstar
>    platform AMACs.
> - [Low] On forced-2500 devices, the programmed MAC speed and the reported
>    link speed now disagree.
> 
> --
> 
> net: ethernet: bgmac: preserve forced 2.5 Gbps link speed
> 
> This commit makes bgmac_adjust_link() use SPEED_2500 instead of the fixed
> PHY's reported speed when BGMAC_FEAT_FORCE_SPEED_2500 is set. The goal is
> to keep the AMAC at 2.5 Gbps on Northstar and Northstar Plus. Without it,
> the first link change reprograms the MAC to 1 Gbps.
> 
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index b3a92ae95de30..ec879c203bc59 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -1417,11 +1417,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]
> Does this reach more than Northstar and Northstar Plus? bgmac_probe() in
> bgmac-platform.c sets the flag for any platform AMAC that has no
> phy-handle, whatever the SoC family:
> 
> drivers/net/ethernet/broadcom/bgmac-platform.c:bgmac_probe() {
>      ...
> 	} else {
> 		bgmac->phy_connect = bgmac_phy_connect_direct;
> 		bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
> 	}
>      ...
> }
> 
> Before this patch, the 2.5G setting from bgmac_miiconfig() only lasted
> until the first bgmac_adjust_link(). That call reprogrammed the MAC to the
> fixed PHY's 1000. With this change, 2.5G becomes the permanent MAC speed.
> 
> The in-tree Cygnus board bcm911360_entphn.dts enables eth0 ("brcm,amac"
> in bcm-cygnus.dtsi) without a phy-handle, so it gets the flag. Its switch
> peer is "brcm,bcm11360-srab" (BCM583XX_DEVICE_ID), and CPU port@8 there is
> a fixed-link with speed = <1000>.
> 
> The companion change "net: dsa: b53: restore the NSP IMP port 2 Gbps mode"
> only forces the IMP port to 2000 for is5301x() or BCM58XX_DEVICE_ID:
> 
> drivers/net/dsa/b53/b53_common.c:b53_phylink_mac_link_up() {
>      ...
> 		if ((is5301x(dev) || dev->chip_id == BCM58XX_DEVICE_ID) &&
> 		    dsa_is_cpu_port(ds, port)) {
>      ...
> }
> 
> Would that leave the Cygnus AMAC at 2.5G and its IMP port at 1G? That is
> the same mismatch the series describes as the cause of the output queue
> discards.
> 
> Hurricane2 ("brcm,nsp-amac") and Stingray ("brcm,amac") would be treated
> the same way if their DT had no phy-handle. In-tree Stingray boards do set
> phy-handle, and no in-tree HR2 board enables amac0.
> 
> On parts without idm_base, bgmac_miiconfig() skips the 2.5G IOCTL clock
> select. The MAC speed field would still stay at CMD_SPEED_2500:
> 
> drivers/net/ethernet/broadcom/bgmac.c:bgmac_miiconfig() {
> 	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500) {
> 		if (!(bgmac->feature_flags & BGMAC_FEAT_IDM_MASK)) {
> 			bgmac_idm_write(bgmac, BCMA_IOCTL,
> 					bgmac_idm_read(bgmac, BCMA_IOCTL) |
> 					0x40 | BGMAC_BCMA_IOCTL_SW_CLKEN);
> 		}
>      ...
> }
> 
> Should the override be limited to the SoCs that really run this link at
> 2.5G?
> 
>>   	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: Low]
> After this change, can update ever become true on forced 2.5G devices?
> 
> bgmac_chip_reset()->bgmac_miiconfig() caches these values before the PHY
> state machine runs:
> 
> 		bgmac->mac_speed = SPEED_2500;
> 		bgmac->mac_duplex = DUPLEX_FULL;
> 		bgmac_mac_speed(bgmac);
> 
> The fixed PHY reports DUPLEX_FULL, so both comparisons in
> bgmac_adjust_link() come out false and this block is skipped:
> 
> 	if (update) {
> 		bgmac_mac_speed(bgmac);
> 		phy_print_status(phy_dev);
> 	}
> 
> Before the patch, the first link-up saw 1000 != 2500, reprogrammed the MAC
> and printed the "Link is Up" message. That message seems to be gone on
> these boards now.
> 
> Also, phy_dev->speed stays at 1000. bgmac_phy_connect_direct() registers
> the synthetic fixed PHY with .speed = SPEED_1000, the DT fixed-link uses
> speed = <1000>, and swphy can't represent 2500 either way.
> 
> bgmac_ethtool_ops uses phy_ethtool_get_link_ksettings(). Would ethtool and
> /sys/class/net/<if>/speed then report 1000 Mb/s while the MAC is
> programmed for 2.5G?
> 
> Should the commit message mention these user-visible changes?
> 

The scope concern is valid. I will restrict the platform forced-speed 
flag to Northstar Plus; the BCMA path already selects the Northstar 
family explicitly.

I will also decouple link-status printing from MAC reconfiguration. 
Userspace will continue reporting the synthetic fixed PHY’s existing 
1Gbps speed, and I will document that limitation in the changelog.

pw-bot: cr
-- 
Florian

[PATCH net v3 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 | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 0880310c9ce3..743fc04da0a4 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;
+		}
 
 		b53_force_port_config(dev, port, speed, duplex,
 				      tx_pause, rx_pause);
-- 
2.53.0

Re: [PATCH net v3 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 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

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

[Florian Fainelli]

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

The use of 2Gbits/sec on non-IMP port is supported and it requires the 
use of a different offset and bit, which is being fixed by this patch:

http://lore.kernel.org/r/20261028212310.3847553-6-florian.fainelli@broadcom.com

In addition, there will be a treewide change that switches all of the 
configurations to use port 8 which is the only "true" IMP port where 
everything works, there were several discussions on the matter but it 
has not happened yet.
-- 
Florian