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

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

- make the bgmac change scoped specifically to Northstar Plus,
  Hurricane2 also uses the compatible "brcm,nsp-amac" and so we need to
  determine the machine type instead

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-platform.c |  3 ++-
 drivers/net/ethernet/broadcom/bgmac.c          |  8 ++++++--
 3 files changed, 17 insertions(+), 6 deletions(-)

-- 
2.53.0

[PATCH net v4 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.

Restrict the platform forced-speed flag to Northstar Plus using the SoC
compatible, since Hurricane2 shares the AMAC compatible. Other platform
AMACs without a phy-handle must continue following the fixed PHY speed.
The BCMA path already selects the flag only for the Northstar family.

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

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 4e266ce41180..4880b1890481 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -243,7 +243,8 @@ static int bgmac_probe(struct platform_device *pdev)
 		bgmac->phy_connect = platform_phy_connect;
 	} else {
 		bgmac->phy_connect = bgmac_phy_connect_direct;
-		bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
+		if (of_machine_is_compatible("brcm,nsp"))
+			bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
 	}
 
 	return bgmac_enet_probe(bgmac);
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 669eb7ce7d40..9ef7307edd41 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1430,11 +1430,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

[PATCH net v4 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 v4 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] After this patch the speed seen by userspace and phylib no
  longer matches the MAC.
- [Low] The patch depends on a switch-side change that is not named.

--

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

When BGMAC_FEAT_FORCE_SPEED_2500 is set, this commit makes
bgmac_adjust_link() keep the MAC at SPEED_2500 instead of following the
fixed PHY's 1 Gbps speed. It also restricts the platform forced-speed
flag to Northstar Plus via of_machine_is_compatible("brcm,nsp").

> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
> index 669eb7ce7d40b..9ef7307edd41e 100644
> --- a/drivers/net/ethernet/broadcom/bgmac.c
> +++ b/drivers/net/ethernet/broadcom/bgmac.c
> @@ -1430,11 +1430,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: Low]
Does this patch depend on the b53 change that puts the IMP port in its
2 Gbps mode? The commit message says:

  Together with configuring the switch IMP port for its 2 Gbps
  mode, this eliminates the output queue discards ...

but it does not name the commit that makes that change.

With only this patch applied, the AMAC stays at SPEED_2500. Meanwhile
b53_force_port_config() still programs the IMP port from the DT
fixed-link (speed = <1000> in bcm-ns.dtsi) and clears the 2G bit:

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

The next patch in this series, "net: dsa: b53: restore the NSP IMP port
2 Gbps mode", fixes this. It sets speed = 2000 for BCM5301x/BCM58XX CPU
ports in b53_phylink_mac_link_up(), so both ends match once the whole
series is applied.

The two patches have different Fixes: tags, though (f6a95a24957a and
b6a8a5477fe9). If a stable backport or a bisect picks up only this
patch, would the MAC run at 2500 while the switch IMP port runs at
1000? Could the b53 commit be named here as a dependency so both are
backported together?

>  	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]
Can phy_print_status() still be reached on these interfaces?

bgmac_open()->bgmac_chip_reset()->bgmac_miiconfig() sets the cached
speed and duplex before phy_start():

drivers/net/ethernet/broadcom/bgmac.c:bgmac_miiconfig() {
	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500) {
		...
		bgmac->mac_speed = SPEED_2500;
		bgmac->mac_duplex = DUPLEX_FULL;
		bgmac_mac_speed(bgmac);
    ...
}

When the fixed PHY reports link up, bgmac_adjust_link() now computes
speed == SPEED_2500, which equals mac_speed, and the duplex also equals
mac_duplex. So update stays false and the link-up message is never
printed. Before this patch, the change from 2500 to 1000 always set
update.

Also, bgmac_ethtool_ops uses phy_ethtool_get_link_ksettings, which
reports phydev->speed. That is still SPEED_1000, either from the fixed
PHY in bgmac_phy_connect_direct() or from the DT fixed-link:

	struct fixed_phy_status fphy_status = {
		.link = 1,
		.speed = SPEED_1000,
		.duplex = DUPLEX_FULL,
	};

The UniMAC, however, is now programmed for 2500.

Will ethtool and netlink report 1000 Mb/s for a link that is actually
running at 2.5 Gbps?

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

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

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

[Florian Fainelli]

On 11/2/26 13:50, 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] After this patch the speed seen by userspace and phylib no
>    longer matches the MAC.
> - [Low] The patch depends on a switch-side change that is not named.
> 
> --
> 
> net: ethernet: bgmac: preserve forced 2.5 Gbps link speed
> 
> When BGMAC_FEAT_FORCE_SPEED_2500 is set, this commit makes
> bgmac_adjust_link() keep the MAC at SPEED_2500 instead of following the
> fixed PHY's 1 Gbps speed. It also restricts the platform forced-speed
> flag to Northstar Plus via of_machine_is_compatible("brcm,nsp").
> 
>> diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
>> index 669eb7ce7d40b..9ef7307edd41e 100644
>> --- a/drivers/net/ethernet/broadcom/bgmac.c
>> +++ b/drivers/net/ethernet/broadcom/bgmac.c
>> @@ -1430,11 +1430,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: Low]
> Does this patch depend on the b53 change that puts the IMP port in its
> 2 Gbps mode? The commit message says:
> 
>    Together with configuring the switch IMP port for its 2 Gbps
>    mode, this eliminates the output queue discards ...
> 
> but it does not name the commit that makes that change.
> 
> With only this patch applied, the AMAC stays at SPEED_2500. Meanwhile
> b53_force_port_config() still programs the IMP port from the DT
> fixed-link (speed = <1000> in bcm-ns.dtsi) and clears the 2G bit:
> 
> drivers/net/dsa/b53/b53_common.c:b53_force_port_config() {
>      ...
> 	if (is5301x(dev) || is58xx(dev))
> 		reg &= ~PORT_OVERRIDE_SPEED_2000M;
>      ...
> }
> 
> The next patch in this series, "net: dsa: b53: restore the NSP IMP port
> 2 Gbps mode", fixes this. It sets speed = 2000 for BCM5301x/BCM58XX CPU
> ports in b53_phylink_mac_link_up(), so both ends match once the whole
> series is applied.
> 
> The two patches have different Fixes: tags, though (f6a95a24957a and
> b6a8a5477fe9). If a stable backport or a bisect picks up only this
> patch, would the MAC run at 2500 while the switch IMP port runs at
> 1000? Could the b53 commit be named here as a dependency so both are
> backported together?
> 
>>   	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]
> Can phy_print_status() still be reached on these interfaces?
> 

No it does not reach on those interfaces.

> bgmac_open()->bgmac_chip_reset()->bgmac_miiconfig() sets the cached
> speed and duplex before phy_start():
> 
> drivers/net/ethernet/broadcom/bgmac.c:bgmac_miiconfig() {
> 	if (bgmac->feature_flags & BGMAC_FEAT_FORCE_SPEED_2500) {
> 		...
> 		bgmac->mac_speed = SPEED_2500;
> 		bgmac->mac_duplex = DUPLEX_FULL;
> 		bgmac_mac_speed(bgmac);
>      ...
> }
> 
> When the fixed PHY reports link up, bgmac_adjust_link() now computes
> speed == SPEED_2500, which equals mac_speed, and the duplex also equals
> mac_duplex. So update stays false and the link-up message is never
> printed. Before this patch, the change from 2500 to 1000 always set
> update.
> 
> Also, bgmac_ethtool_ops uses phy_ethtool_get_link_ksettings, which
> reports phydev->speed. That is still SPEED_1000, either from the fixed
> PHY in bgmac_phy_connect_direct() or from the DT fixed-link:
> 
> 	struct fixed_phy_status fphy_status = {
> 		.link = 1,
> 		.speed = SPEED_1000,
> 		.duplex = DUPLEX_FULL,
> 	};
> 
> The UniMAC, however, is now programmed for 2500.
> 
> Will ethtool and netlink report 1000 Mb/s for a link that is actually
> running at 2.5 Gbps?

Yes, that will happen, teaching phylink, phylib and ethtool about 
Broadcom's proprietary 2Gbits/sec is beyond the scope of this patch.
-- 
Florian

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

[Florian Fainelli]

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

Not necessarily, while it is preferable to use the IMP port, 
functionally it still makes sense to be able to prime the non-IMP CPU 
ports to 2Gbits/sec should we desire so.

There is a separate patch set to address the 2Gbits/sec override.
-- 
Florian