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

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Johan,

On 7/2/26 22:47, 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 as a phylink PCS, 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.
> 
>  - Register a phylink_pcs for the SerDes, selected from mac_select_pcs
>    for the SGMII interface, so the SerDes handling lives in the PCS
>    operations rather than in the MAC operations.
> 
>  - Probe the SerDes tuning variant from the chip option register once
>    at setup. The vendor driver keeps two sets of SerDes tuning
>    parameters and selects between them based on this option; only the
>    variant for a non-zero option (which all RTL8367S parts seen so far
>    report) has been validated on hardware, so the SerDes interface
>    modes are only advertised in that case. An unsupported variant thus
>    fails at phylink validation time instead of at link configuration
>    time.
> 
>  - 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) in the SDS_MISC
>    register from pcs_link_up(). SGMII in-band autonegotiation is not
>    implemented, so only fixed-link and conventional PHY setups are
>    supported, just like RGMII. This is reported to phylink through
>    pcs_inband_caps() returning LINK_INBAND_DISABLE, so phylink never
>    selects an in-band-enabled negotiation mode for this PCS.
> 
>  - Program the SerDes pause controls in SDS_MISC from the resolved
>    pause modes when forcing the MAC external interface in mac_link_up,
>    as the vendor driver does, rather than leaving whatever state the
>    boot firmware left there. This is done in the MAC layer because
>    pcs_link_up() carries no pause information.
> 
>  - Implement pcs_get_state() by reading the link status from the
>    SerDes, with the forced speed and duplex read back from SDS_MISC.
>    Although the supported fixed-link and conventional PHY setups do not
>    use it, the PCS owns the SerDes link state, and phylink consults
>    pcs_get_state() to track the physical link when operating in in-band
>    mode with autonegotiation disabled. The SerDes has no link interrupt
>    wired up, so the PCS sets its poll flag.
> 
> Tested on a Mercusys MR80X v2.20, where the RTL8367S is connected to
> the SoC over SGMII.

Ah nice conversion to phylink PCS :) I have a few comments below

> 
> Suggested-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Suggested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> Suggested-by: Mieczyslaw Nalewaj <namiltd@yahoo.com>
> Signed-off-by: Johan Alvarado <contact@c127.dev>
> ---

[...]

> +static int rtl8365mb_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +				phy_interface_t interface,
> +				const unsigned long *advertising,
> +				bool permit_pause_to_mac)
> +{
> +	const int id = RTL8365MB_SDS_EXT_INTERFACE_ID;
> +	struct rtl8365mb *mb = pcs_to_rtl8365mb(pcs);
> +	struct realtek_priv *priv;
> +	u16 val;
> +	int ret;
> +	int i;
> +
> +	priv = mb->priv;
> +
> +	/* This driver does not implement SGMII in-band autonegotiation yet, so
> +	 * the link parameters are forced from rtl8365mb_pcs_link_up() instead.
> +	 * rtl8365mb_pcs_inband_caps() reports this to phylink, which should
> +	 * therefore never select an in-band-enabled negotiation mode.
> +	 */
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +		return -EOPNOTSUPP;

As you implement the .pcs_inband_caps() method, phylink will pass you a valid
mode, no need for that check :)

[...]
> @@ -1218,14 +1680,46 @@ static void rtl8365mb_phylink_mac_link_up(struct phylink_config *config,
>  	p = &mb->ports[port];
>  	schedule_delayed_work(&p->mib_work, 0);
>  
> -	if (phy_interface_mode_is_rgmii(interface)) {
> +	/* The SerDes forced link state is programmed by the PCS in
> +	 * rtl8365mb_pcs_link_up(); here only the MAC external interface force
> +	 * is configured, for both RGMII and SerDes.
> +	 */
> +	if (phy_interface_mode_is_rgmii(interface) ||
> +	    rtl8365mb_interface_is_serdes(interface)) {
>  		ret = rtl8365mb_ext_config_forcemode(priv, port, true, speed,
>  						     duplex, tx_pause,
>  						     rx_pause);
> -		if (ret)
> +		if (ret) {
>  			dev_err(priv->dev,
>  				"failed to force mode on port %d: %pe\n", port,
>  				ERR_PTR(ret));
> +			return;
> +		}
> +
> +		/* The SerDes has its own pause controls; program them from
> +		 * the resolved pause modes, as the vendor driver does when
> +		 * forcing the link on a SerDes external interface. This is
> +		 * done here rather than in rtl8365mb_pcs_link_up() because
> +		 * pcs_link_up() carries no pause information.
> +		 */
> +		if (rtl8365mb_interface_is_serdes(interface)) {
> +			u32 val = 0;
> +
> +			if (tx_pause)
> +				val |= RTL8365MB_SDS_MISC_SGMII_TXFC_MASK;
> +			if (rx_pause)
> +				val |= RTL8365MB_SDS_MISC_SGMII_RXFC_MASK;

Do you know what this does in HW ? Is this so that the PCS lets the Pause frames through
in either directions ?

I suspect this is something that would be only used for inband advertising
of pause settings (in such case, you don't even need that), but ofc I'm not sure :)

You already configure the MAC pause settings, can you test that these bits actually do
anything by exercising a bit flow control and checking if these registers are used ?

Otherwise, this is looking pretty good :)

Maxime