Re: [PATCH net v3 5/8] net: dsa: b53: use the correct override register 2 Gbps bit

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch makes b53_force_port_config()
  (drivers/net/dsa/b53/b53_common.c) write BIT(7) of the per-port GMII…

--

net: dsa: b53: use the correct override register 2 Gbps bit

The IMP and per-port GMII override registers use swapped bit positions
for override-enable and 2 Gbps speed. This patch adds a per-register
speed2000 bit to b53_force_port_config(). The function then uses that bit
both to clear the 2 Gbps selection on 5301x/58xx and to set it for speed
2000.

> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 378f47e7250cd..75033b28100ec 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
> @@ -1370,17 +1370,19 @@ static void b53_force_port_config(struct b53_device *dev, int port,
>  				  int speed, int duplex,
>  				  bool tx_pause, bool rx_pause)
>  {
> -	u8 reg, val, off;
> +	u8 reg, val, off, speed2000;
>  
>  	/* Override the port settings */
>  	if (port == dev->imp_port) {
>  		off = B53_PORT_OVERRIDE_CTRL;
>  		val = PORT_OVERRIDE_EN;
> +		speed2000 = PORT_OVERRIDE_SPEED_2000M;
>  	} else if (is5325(dev)) {
>  		return;
>  	} else {
>  		off = B53_GMII_PORT_OVERRIDE_CTRL(port);
>  		val = GMII_PO_EN;
> +		speed2000 = GMII_PO_SPEED_2000M;
>  	}
>  
>  	b53_read8(dev, B53_CTRL_PAGE, off, &reg);
> @@ -1392,11 +1394,11 @@ static void b53_force_port_config(struct b53_device *dev, int port,
>  
>  	reg &= ~(0x3 << GMII_PO_SPEED_S);
>  	if (is5301x(dev) || is58xx(dev))
> -		reg &= ~PORT_OVERRIDE_SPEED_2000M;
> +		reg &= ~speed2000;
>  
>  	switch (speed) {
>  	case 2000:
> -		reg |= PORT_OVERRIDE_SPEED_2000M;
> +		reg |= speed2000;

[Severity: Low]
Should the set in case 2000 use the same is5301x() / is58xx() check as
the clear above it?

On GMII ports, speed2000 is now GMII_PO_SPEED_2000M, and b53_regs.h
documents it as chip-specific:

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

Before this patch, reg |= PORT_OVERRIDE_SPEED_2000M on a GMII port set
BIT(6). That is GMII_PO_EN, and reg |= val had already set it. So on
chips other than 5301x/58xx, speed 2000 behaved the same as 1000.

With this patch, speed 2000 on a GMII port of any chip that is not
5301x, 58xx or 5325 (for example BCM53125, BCM5395, BCM5389 or BCM63xx)
writes BIT(7) of B53_GMII_PORT_OVERRIDE_CTRL(port). Since the clear is
gated on is5301x(dev) || is58xx(dev), nothing ever clears that bit again
on those chips.

Commit b6a8a5477fe9 gated the clear because this bit position "can have
different meanings on other chips, e.g. for BCM5389 this controls
scanning PHYs". The set path was never gated the same way.

The IMP-port side of this mismatch existed before this patch. This patch
extends it to all GMII ports.

One way to reach it is a DT fixed-link with speed = <2000> on a GMII port:
phylink_parse_fixedlink() only warns and keeps the speed, and phylink then
calls b53_phylink_mac_link_up()->b53_force_port_config() in MLO_AN_FIXED
mode. The 63xx RGMII path in b53_phylink_mac_link_up() reaches the same
function.

Would it be better to gate reg |= speed2000 on is5301x(dev) ||
is58xx(dev), or to reject speed 2000 on other chips?

>  		fallthrough;
>  	case SPEED_1000:
>  		reg |= PORT_OVERRIDE_SPEED_1000M;

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