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

[Florian Fainelli <florian.fainelli@broadcom.com>]

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

Agreed. I’ll gate setting the 2 Gbps bit on is5301x() || is58xx(), 
matching the clear path and preserving chip-specific bits on other 
devices. This also addresses the pre-existing IMP-port mismatch.

pw-bot: cr
-- 
Florian