Re: [PATCH net-next v9 04/11] net: dsa: realtek: rtl8365mb: set RGMII mode for family D

[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] On family D (RTL8367S-VB), rtl8365mb_phylink_get_caps() in
  drivers/net/dsa/realtek/rtl8365mb_main.c does not advertise RGMII for…

--

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

For family D, this patch clears the EXT1 RGMII TX clock delay field in
EXT_TXC_DLY. It binds the RGMII/MII pins to MAC7 in TOP_CON0 and disables
SerDes 1 before RGMII is selected. In phylink_get_caps(), RGMII is no
longer advertised on port 7 when TOP_CON0 shows that MAC4 owns
extension 1.

> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 0d87d6bbe15a7..0b7c124abb1e7 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c

[ ... ]

> @@ -1799,8 +1847,26 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  	 * which remain unimplemented.
>  	 */
>  
> -	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII)
> -		phy_interface_set_rgmii(config->supported_interfaces);
> +	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII) {
> +		/* On family D the EXT1 pins are shared with MAC4 (UTP port 4).
> +		 * If MAC4 owns them, RGMII on this port cannot work.
> +		 */
> +		if (rtl8365mb_is_d(priv) && extint->id == 1) {
> +			ret = regmap_read(priv->map, RTL8365MB_D_REG_TOP_CON0, &data);
> +			if (ret)
> +				dev_warn(priv->dev,
> +					 "failed to read TOP_CON0, RGMII on port %d disabled: %pe\n",
> +					 port, ERR_PTR(ret));
> +			else if (data & RTL8365MB_D_MAC4_SEL_EXT1_MASK)
> +				dev_warn(priv->dev,
> +					 "port %d: EXT1 is owned by MAC4, RGMII disabled\n",
> +					 port);
> +			else
> +				phy_interface_set_rgmii(config->supported_interfaces);

[Severity: Low]
Is a warning the right log level here? Port 7 may end up with no usable
interface at all.

In the RTL8367S-VB chip table entry added later in the series, port 7 is
described as MII | TMII | RMII | RGMII. RGMII is the only one of those
modes the driver implements. When either branch above skips
phy_interface_set_rgmii(), config->supported_interfaces stays empty, and
phylink_create() rejects that:

drivers/net/phy/phylink.c:phylink_create() {
    ...
	if (phy_interface_empty(config->supported_interfaces)) {
		dev_err(config->dev,
			"phylink: error: empty supported_interfaces\n");
		return ERR_PTR(-EINVAL);
	}
    ...
}

When port 7 is a user port, dsa_tree_setup_ports() falls back to
dsa_port_setup_as_unused(). When port 7 is the CPU port (the series
supports an RGMII CPU port there), the whole tree setup is aborted:

net/dsa/dsa.c:dsa_tree_setup_ports() {
    ...
		if (dsa_port_is_dsa(dp) || dsa_port_is_cpu(dp)) {
			err = dsa_port_setup(dp);
			if (err)
				goto teardown;
		}
    ...
}

In that case the switch fails to probe. A warn-level "RGMII disabled"
message suggests the port keeps working in a degraded state.

In the regmap_read() failure branch, the real I/O error is only shown at
warn level. The probe then fails with a generic -EINVAL from phylink.

The commit message says:

  "so the configuration is refused before any EXT1 register is touched"

and:

  "The driver logs a warning naming the conflict."

Could the commit message mention that the port, or the whole switch if
port 7 is the CPU port, will fail to set up? Should these messages be
dev_err() and say that the port cannot be used?

> +		} else {
> +			phy_interface_set_rgmii(config->supported_interfaces);
> +		}
> +	}
>  
>  	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_SGMII &&
>  	    mb->sds_supported)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/9b73858d-fc09-4988-8bab-fd7b88db9412%40yahoo.com