Re: [PATCH net-next v3 3/9] net: dsa: realtek: rtl8365mb: set speed for family D

[netdev-bot+sashiko]

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] The commit message does not describe what the patch changes for
  family D.
- [Low] Concern: no rtl8365mb_chip_infos[] entry sets .family =
  RTL8365MB_FAMILY_D, so the new family D branch in…

--

net: dsa: realtek: rtl8365mb: set speed for family D

This adds family D handling to rtl8365mb_ext_config_forcemode(). Family D
gets a dedicated 2500M speed value (5), and the third speed bit goes into
a new FORCE_SPEED2 field. Family D also programs a per-port force register
and a matching per-port enable register instead of the per-extint force
register.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 840db5ad32efe..8f3295c232362 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1692,20 +1717,44 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  		r_duplex = 0;
>  	}
>  
> -	val = FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK, 1) |
> -	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK,
> +	val = FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK,
>  			 r_tx_pause) |
>  	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_RXPAUSE_MASK,
>  			 r_rx_pause) |
>  	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_LINK_MASK, r_link) |
>  	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK,
>  			 r_duplex) |
> -	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK, r_speed);
> -	ret = regmap_write(priv->map,
> -			   RTL8365MB_DIGITAL_INTERFACE_FORCE_REG(extint->id),
> -			   val);
> -	if (ret)
> -		return ret;
> +	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK,
> +			 r_speed &
> +			 RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK);
> +
> +	if (is_d) {
> +		/* Speed is 3 bits on family D: bits[1:0] go into FORCE_SPEED,
> +		 * bit[2] goes into FORCE_SPEED2 (bit 12); only 2500M sets it,
> +		 * bit 13 of the field is unused.
> +		 */
> +		val |= FIELD_PREP(RTL8365MB_D_DIGITAL_INTERFACE_FORCE_SPEED2_MASK,
> +				  r_speed >>
> +				  RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_WIDTH);
> +		ret = regmap_write(priv->map,
> +				   RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG(port),
> +				   val);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(priv->map,
> +				   RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_EN(port),
> +				   RTL8365MB_D_DIGITAL_INTERFACE_FORCE_EN_ALL_MASK);
> +		if (ret)
> +			return ret;

[Severity: Low]
This isn't a bug, but the subject and commit message only cover the new
3-bit speed encoding. For family D, rtl8365mb_ext_config_forcemode() also
changes these things:

  - All forced-mode programming moves from
    RTL8365MB_DIGITAL_INTERFACE_FORCE_REG(extint->id) to the per-port
    register at 0x12c0 + port. That covers link, duplex, pause and every
    speed, plus the link down path from mac_link_down.

  - FORCE_EN (BIT(12)) is no longer set, because on family D that bit is
    FORCE_SPEED2.

  - Every call, including link down, writes 0xFFFF to a second per-port
    register at 0x12c8 + port.

Could the commit message, or a comment here, explain what the 16 bits in
RTL8365MB_D_DIGITAL_INTERFACE_FORCE_REG_EN() select, and why setting all
of them is correct?

If those bits choose which fields of the force register are forced, then
0xFFFF also forces fields this code writes as zero (bit 7, bits 8-11 and
bits 14-15).

It might also help to say that the family C bit positions for TXPAUSE,
RXPAUSE, LINK, DUPLEX and SPEED are reused for the register at the new
address.

This looks like it follows the vendor RTL8367D sequence. The later patches
in the series leave this code as it is and don't add an explanation.

> +	} else {
> +		val |= FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK, 1);
> +		ret = regmap_write(priv->map,
> +				   RTL8365MB_DIGITAL_INTERFACE_FORCE_REG(extint->id),
> +				   val);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/e84d76ee-03df-49b0-8c9a-b289dfae8728%40yahoo.com