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 <redacted>
Suggested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Suggested-by: Mieczyslaw Nalewaj <redacted>
Signed-off-by: Johan Alvarado <redacted>
---

[...]

+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 :)

[...]

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