Re: [PATCH net-next v4 4/9] net: dsa: realtek: rtl8365mb: set RGMII mode 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] On family D, rtl8365mb_ext_config_rgmii()
  (drivers/net/dsa/realtek/rtl8365mb_main.c) reads TOP_CON0 for extint…

--

net: dsa: realtek: rtl8365mb: set RGMII mode for family D

For family D chips using RGMII on extension interface 1, this patch
clears the EXT1 RGMII TX clock delay field in EXT_TXC_DLY. If MAC4 is
not occupying extension 1, it also binds the RGMII/MII pins to MAC7 in
TOP_CON0. SerDes 1, which shares the pins, is always disabled.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 2dabc4da861f4..996ba2e39302c 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1270,6 +1291,35 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  	if (ret)
>  		return ret;
>  
> +	if (rtl8365mb_get_family(priv) == RTL8365MB_FAMILY_D &&
> +	    extint->id == 1) {
> +		ret = regmap_update_bits(priv->map,
> +					 RTL8365MB_D_REG_EXT_TXC_DLY,
> +					 RTL8365MB_D_EXT1_RGMII_TX_DLY_MASK, 0);
> +		if (ret)
> +			return ret;
> +		/* Configure RGMII/MII mux to port 7 if UTP_PORT4 is not in
> +		 * RGMII mode
> +		 */
> +		ret = regmap_read(priv->map, RTL8365MB_D_REG_TOP_CON0, &data);
> +		if (ret)
> +			return ret;
> +		if ((data & RTL8365MB_D_MAC4_SEL_EXT1_MASK) == 0) {
> +			ret = regmap_update_bits(priv->map,
> +						 RTL8365MB_D_REG_TOP_CON0,
> +						 RTL8365MB_D_MAC7_SEL_EXT1_MASK,
> +						 RTL8365MB_D_MAC7_SEL_EXT1_MASK);
> +			if (ret)
> +				return ret;
> +		}

[Severity: Low]
What happens in rtl8365mb_ext_config_rgmii() when strap or EEPROM has
already set RTL8365MB_D_MAC4_SEL_EXT1_MASK in TOP_CON0? In that case MAC4
owns the EXT1 pins, so MAC7_SEL_EXT1 is not set. The function still runs
the SDS1_MISC0 write below and returns 0.

By the time this check runs, these EXT1 registers have already been
changed for port 7:

    RTL8365MB_EXT_RGMXF_REG(extint->id) TX/RX delays from port 7's DT
    RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(extint->id) set to RGMII
    RTL8365MB_D_REG_EXT_TXC_DLY bits [5:3] cleared

If the pins belong to MAC4, could these writes change the RGMII timing
that the MAC4 path depends on?

The only caller, rtl8365mb_phylink_mac_config(), logs a message only on a
non-zero return:

    ret = rtl8365mb_ext_config_rgmii(priv, port, state->interface);
    if (ret)
        dev_err(priv->dev,
            "failed to configure RGMII mode on port %d: %pe\n",
            port, ERR_PTR(ret));

So when the DT describes port 7 as RGMII but the strap gives EXT1 to
MAC4, port 7 appears to be configured but carries no traffic, and
nothing is logged.

Since the code already detects this state, would it be better to return
an error such as -EBUSY before touching the shared EXT1 registers, or at
least print a warning?

> +		ret = regmap_update_bits(priv->map,
> +					 RTL8365MB_D_REG_SDS1_MISC0,
> +					 RTL8365MB_D_SDS_MISC_MODE_FIELD_MASK,
> +					 RTL8365MB_D_PORT_SDS_MODE_DISABLE);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/38872e6f-1264-4a93-9f81-8212173e909d%40yahoo.com