Re: [PATCH net-next v6 4/9] 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] rtl8365mb_phylink_get_caps() drops RGMII without logging anything
  when TOP_CON0.MAC4_SEL_EXT1 (BIT(12) of 0x1d70) is set on family D…
- [Low] At this commit the two parts of the code describe EXT1
  differently.
- [Low] On family D EXT1, rtl8365mb_ext_config_rgmii() writes in this
  order: RGMXF delays, DIGITAL_INTERFACE_SELECT = RGMII, clear…

--

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

This patch adds family D handling for RGMII on extension interface 1. It
clears the EXT1 RGMII TX delay field in EXT_TXC_DLY, binds the pins to
MAC7 through TOP_CON0, and disables SerDes 1. RGMII is also no longer
advertised for port 7 in rtl8365mb_phylink_get_caps() when TOP_CON0 shows
that MAC4 owns EXT1.

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

[ ... ]

> @@ -1270,6 +1290,33 @@ 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) {

[Severity: Low]
At this point in the series, does this model of EXT1 disagree with the
existing SerDes comment and constants near the top of
rtl8365mb_main.c?

/* The embedded SerDes can only be muxed to external interface 1 (MAC8),
 * which is port 6.
 */
#define RTL8365MB_SDS_EXT_INTERFACE_ID		1
#define RTL8365MB_SDS_EXT_INTERFACE_PORT	6

Those apply to every family. The new family D code treats extint id 1 as
the RGMII interface of MAC7 and disables SerDes 1 on it.

Later patches in the series fix this. The comment is rewritten to say
family D has the SerDes on external interface 0 (MAC6), and
rtl8365mb_pcs_config() skips the EXT1 DIGITAL_INTERFACE_SELECT write on
family D. Family D also can't be reached until the last patch.

Could the comment be qualified in this patch so both descriptions agree
at every point in the series?

> +		ret = regmap_update_bits(priv->map,
> +					 RTL8365MB_D_REG_EXT_TXC_DLY,
> +					 RTL8365MB_D_EXT1_RGMII_TX_DLY_MASK, 0);
> +		if (ret)
> +			return ret;
> +
> +		/* Bind the RGMII/MII pins to MAC7. MAC4 does not own them:
> +		 * RGMII is not advertised otherwise, see
> +		 * rtl8365mb_phylink_get_caps().
> +		 */
> +		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;
> +
> +		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;
> +	}

[Severity: Low]
Is this write order intended? The TOP_CON0 pin mux and the SDS1_MISC0
SerDes 1 disable are written only after DIGITAL_INTERFACE_SELECT has
already been switched to RGMII just above.

Suppose SerDes 1 was active before mac_config (from strap or EEPROM, for
example), and the hardware drives the shared pins as soon as RGMII is
selected. Could RGMII and SerDes 1 then both be set up on the same pins
for a few SMI transactions?

Nothing is unwound if one of these writes fails partway through, either.
rtl8365mb_phylink_mac_config() only logs the error:

	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));

After that, rtl8365mb_phylink_mac_link_up() can still force the MAC link
up through rtl8365mb_ext_config_forcemode(), with the mux or TX delay
only partly programmed.

The existing RGMXF and DIGITAL_INTERFACE_SELECT writes, and
rtl8365mb_pcs_config(), behave the same way. The concern also depends on
hardware behaviour that hasn't been confirmed.

Does the vendor RTL8367D code set the mux and disable SerDes 1 before it
selects RGMII mode?

[ ... ]

> @@ -1802,8 +1851,23 @@ 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_get_family(priv) == RTL8365MB_FAMILY_D &&
> +		    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))
> +				phy_interface_set_rgmii(config->supported_interfaces);

[Severity: Low]
When MAC4_SEL_EXT1 is set, RGMII is dropped here with no message. Only
the regmap_read() failure branch warns.

The commit message says:

    If the internal MAC4 block occupies extension 1, do not advertise RGMII
    for port 7 in phylink_get_caps(), so the configuration is refused before
    any EXT1 register is touched.

How would a user find out why it was refused?

The last patch in the series adds the RTL8367S-VB entry, where port 7 is
{ 7, 1, MII|TMII|RMII|RGMII }. MII, TMII and RMII are still not
implemented in rtl8365mb_phylink_mac_config(), so supported_interfaces
ends up empty. phylink_create() then fails with only the generic message:

	if (phy_interface_empty(config->supported_interfaces)) {
		dev_err(config->dev,
			"phylink: error: empty supported_interfaces\n");
		return ERR_PTR(-EINVAL);
	}

If port 7 is the CPU port, DSA tree setup fails. Nothing in the log says
that MAC4 (UTP port 4) owns the EXT1 pins.

Could this branch print a dev_warn() that names the MAC4/EXT1 pin
conflict?

> +		} else {
> +			phy_interface_set_rgmii(config->supported_interfaces);
> +		}
> +	}

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/5a4715dd-89e0-4868-9ffb-c8fb73b18d04%40yahoo.com