Re: [PATCH net-next v3 1/2] net: dsa: realtek: rtl8365mb: add SGMII support for RTL8367S

[Maxime Chevallier]

Hi Johan,

On 6/14/26 01:22, Johan Alvarado wrote:
> The RTL8367S can mux its embedded SerDes to external interface 1,
> which is typically used to connect the switch to a CPU port. The chip
> info table already declares SGMII as a supported interface mode for
> this chip, but the driver only implements RGMII so far.
> 
> Implement SGMII support, with the configuration sequence derived from
> the GPL-licensed Realtek rtl8367c vendor driver as distributed in the
> Mercusys MR80X GPL code drop:
> 
>  - Add accessors for the SerDes indirect access registers (SDS_INDACS),
>    through which the SerDes internal registers are reached.
> 
>  - Keep the embedded DW8051 microcontroller in reset and disabled. The
>    vendor driver loads firmware into it to manage the SerDes link, but
>    analysis of that firmware shows it only duplicates the link
>    management phylink already performs: it polls the port status and
>    writes the external interface force registers behind the driver's
>    back.
> 
>  - Clear the line rate bypass bit for the external interface, tune the
>    SerDes with the vendor-prescribed parameters, mux the SerDes to MAC8
>    in SGMII mode and only then take the SerDes out of reset, as the
>    vendor driver does.
> 
>  - After deasserting the SerDes reset, reset the SerDes data path via
>    the SerDes BMCR register to flush the FIFOs and resync the PLL.
>    This mirrors what the vendor firmware does right after deasserting
>    the SerDes reset, and ensures a clean link state from cold boot.
> 
>  - Force the SGMII link parameters (link, speed, duplex, flow control)
>    in the SDS_MISC register from the phylink mac_link_up/down
>    operations, in addition to the usual external interface force
>    configuration. SGMII in-band autonegotiation is disabled, so only
>    fixed-link and conventional PHY setups are supported, just like
>    RGMII.
> 
> Tested on a Mercusys MR80X v2.20, where the RTL8367S is connected to
> the SoC over SGMII.
> 
> Suggested-by: Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>
> Signed-off-by: Johan Alvarado <contact@xxxxxxxx>
> ---

[...]

> +static int rtl8365mb_ext_config_sgmii(struct realtek_priv *priv, int port)
> +{
> +	const struct rtl8365mb_extint *extint =
> +		rtl8365mb_get_port_extint(priv, port);
> +	u16 val;
> +	int ret;
> +	int i;
> +
> +	if (!extint)
> +		return -ENODEV;
> +
> +	/* The SerDes can only be muxed to external interface 1 */
> +	if (extint->id != 1)
> +		return -EOPNOTSUPP;
> +
> +	/* Hold the embedded DW8051 microcontroller in reset and keep it
> +	 * disabled. The vendor driver loads firmware into it to manage the
> +	 * SerDes link, but the firmware only duplicates work that phylink
> +	 * already does: it polls the port status and forces the external
> +	 * interface configuration in the very registers this driver manages.
> +	 * Letting it run would race with phylink.
> +	 */
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
> +				 RTL8365MB_CHIP_RESET_DW8051_MASK,
> +				 RTL8365MB_CHIP_RESET_DW8051_MASK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0_REG,
> +				 RTL8365MB_MISC_CFG0_DW8051_EN_MASK, 0);
> +	if (ret)
> +		return ret;
> +
> +	/* The vendor driver clears the line rate bypass for all interface
> +	 * modes except TMII.
> +	 */
> +	ret = regmap_update_bits(priv->map, RTL8365MB_BYPASS_LINE_RATE_REG,
> +				 BIT(extint->id), 0);
> +	if (ret)
> +		return ret;
> +
> +	/* Tune the SerDes with vendor-prescribed parameters */
> +	for (i = 0; i < ARRAY_SIZE(rtl8365mb_sds_jam_sgmii); i++) {
> +		ret = rtl8365mb_sds_write(priv, 0,
> +					  rtl8365mb_sds_jam_sgmii[i].reg,
> +					  rtl8365mb_sds_jam_sgmii[i].val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Mux the SerDes to MAC8 in SGMII mode */
> +	ret = regmap_update_bits(priv->map, RTL8365MB_SDS_MISC_REG,
> +				 RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK |
> +					 RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_MASK,
> +				 RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK);
> +	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),
> +		RTL8365MB_EXT_PORT_MODE_SGMII
> +			<< RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(
> +				   extint->id));
> +	if (ret)
> +		return ret;
> +
> +	/* Take the SerDes out of reset. The vendor driver does this only
> +	 * after the SerDes mux and the interface mode are configured.
> +	 */
> +	ret = rtl8365mb_sds_write(priv, 0, RTL8365MB_SDS_REG_RESET,
> +				  RTL8365MB_SDS_RESET_DEASSERT);
> +	if (ret)
> +		return ret;
> +
> +	/* Reset the SerDes data path and resync its PLL, mirroring what the
> +	 * vendor firmware does right after deasserting the SerDes reset.
> +	 * This flushes the FIFOs and ensures a clean state for the link,
> +	 * preventing silent drops and CRC errors.
> +	 */
> +	ret = rtl8365mb_sds_write(priv, 0, RTL8365MB_SDS_REG_BMCR,
> +				  RTL8365MB_SDS_BMCR_DPRST_PHASE1);
> +	if (ret)
> +		return ret;
> +
> +	ret = rtl8365mb_sds_write(priv, 0, RTL8365MB_SDS_REG_BMCR,
> +				  RTL8365MB_SDS_BMCR_DPRST_PHASE2);
> +	if (ret)
> +		return ret;
> +
> +	/* Disable SGMII in-band autonegotiation: the link parameters are
> +	 * forced in rtl8365mb_phylink_mac_link_up.
> +	 */

This comment implies that you could deal with SGMII aneg at some point. This, and
the fact that you end-up with more complex mac_link_up/down sequences that set the
"ext" settings then the "sds" settings while in SGMII makes me wonder if this whole
SGMII/2500BaseX series should be represented as a PCS phylink driver.

It would make more sense, and should also make the code easier to maintain in the
long run. Have you considered converting this to the phylink_pcs ops, or is there
something that doesn't quite fit the model here ? There are quite a few DSA switches
that makes use of this (grep for phylink_pcs), you should have plenty of examples
to pick from :)

Maxime