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@xxxxxxxxxx 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