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

[Alvin Šipraga]

On Mon, Nov 02, 2026 at 03:39:47PM +0100, Mieczyslaw Nalewaj wrote:
> On family D, RGMII on extension interface 1 needs two things that the
> other families do not: the EXT1 RGMII TX clock delay field in
> EXT_TXC_DLY (0x13f9, bits [5:3]) must be cleared, and the pin mux in
> the top-level configuration register must be set up.
> 
> The vendor RGMII setup for EXT1 clears the RGMII field of EXT_TXC_DLY
> (0x13f9, bits [5:3]) in addition to programming TXDELAY in RGMXF.
> Do the same. The init jam table writes 0x0090 to this register, which
> leaves the field at 2, so without the clear an extra TX delay would be
> applied on top of tx-internal-delay-ps. Clearing it makes
> tx-internal-delay-ps the only source of TX delay.

Should we not just clear it with the jam table then? It would save some
code here, which is just undoing what the jam table did anyway. Or am I
missing something? If you deviate from the vendor driver's jam table
then a comment inline or in its own commit could be appropriate. But
either way it seems it would save some code here?

> 
> 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. This mirrors the vendor RTL8367D API, which
> returns an error for RGMII on port 7 when UTP port 4 uses the RGMII
> interface. The driver logs a warning naming the conflict. When MAC4 does
> not own the pins, bind the RGMII/MII pins to MAC7. SerDes 1, which shares
> the pins, is always disabled. The TX delay field, the pin mux and the
> SerDes 1 disable are all programmed before RGMII is selected, so the
> shared pins are never driven by RGMII and SerDes 1 at the same time.
> 
> The family D branch is not reachable until the chip table entry is
> added in the last patch.
> 
> Signed-off-by: Mieczyslaw Nalewaj <namiltd@xxxxxxxxx>
> Reviewed-by: Linus Walleij <linusw@xxxxxxxxxx>
> ---
>  drivers/net/dsa/realtek/rtl8365mb_main.c | 72 +++++++++++++++++++++++-
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
> index 2dabc4d..d69eed0 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb_main.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
> @@ -332,6 +332,12 @@
>  #define   RTL8365MB_SDS_OPTION_ARM_KEY		0x0249
>  #define RTL8365MB_SDS_OPTION_REG		0x13C1
>  
> +#define RTL8365MB_D_SDS_MISC_MODE_FIELD_MASK		GENMASK(4, 0)
> +/* Shared "disable" encoding for both SDS_MISC's and SDS1_MISC0's
> + * 5-bit mode fields.
> + */
> +#define RTL8365MB_D_PORT_SDS_MODE_DISABLE		0x1f
> +
>  /* Embedded DW8051 microcontroller control registers. The microcontroller
>   * can run firmware to manage the SerDes link, but this driver keeps it in
>   * reset and disabled: phylink already performs the link management that
> @@ -465,6 +471,20 @@
>  #define   RTL8365MB_PORT_MISC_CFG_VLAN_EGRESS_MODE_MASK		GENMASK(5, 4)
>  #define   RTL8365MB_PORT_MISC_CFG_CONGESTION_SUSTAIN_TIME_MASK	GENMASK(3, 0)
>  
> +/* EXT_TXC_DLY holds a 3-bit TX clock delay per external interface, for
> + * RGMII ([2:0] EXT0, [5:3] EXT1, [8:6] EXT2) and separately for GMII.
> + * Only the EXT1 RGMII field is used here, and it is cleared in RGMII
> + * mode so that tx-internal-delay-ps is the only TX delay applied.
> + */
> +#define RTL8365MB_D_REG_EXT_TXC_DLY				0x13f9
> +#define   RTL8365MB_D_EXT1_RGMII_TX_DLY_MASK			GENMASK(5, 3)
> +
> +#define RTL8365MB_D_REG_TOP_CON0				0x1d70
> +#define   RTL8365MB_D_MAC7_SEL_EXT1_MASK			BIT(13)
> +#define   RTL8365MB_D_MAC4_SEL_EXT1_MASK			BIT(12)
> +
> +#define RTL8365MB_D_REG_SDS1_MISC0				0x1d78
> +
>  /**
>   * enum rtl8365mb_vlan_egress_mode - port VLAN egress mode
>   * @RTL8365MB_VLAN_EGRESS_MODE_ORIGINAL: follow untag mask in VLAN4k table entry
> @@ -1261,6 +1281,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) {
> +		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;
> +	}
> +
>  	ret = regmap_update_bits(
>  		priv->map, RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(extint->id),
>  		RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(extint->id),
> @@ -1773,6 +1820,8 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  		rtl8365mb_get_port_extint(ds->priv, port);
>  	struct realtek_priv *priv = ds->priv;
>  	struct rtl8365mb *mb = priv->chip_data;
> +	u32 data;
> +	int ret;
>  
>  	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
>  				   MAC_10 | MAC_100 | MAC_1000FD;
> @@ -1802,8 +1851,27 @@ 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)
> +				dev_warn(priv->dev,
> +					 "port %d: EXT1 is owned by MAC4, RGMII disabled\n",
> +					 port);

I am a little confused about everything that's going on here, but it's
probably just me. Could you perhaps explain the two scenarios and the
order in which you expect things to happen? I'm sort of wondering when
this warning might be emitted. Part of me also wonders if you can't
avoid doing a regmap_read() here in phylink_get_caps.

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