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

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

[Johan Alvarado]

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@gmail.com>
Signed-off-by: Johan Alvarado <contact@c127.dev>
---
 drivers/net/dsa/realtek/rtl8365mb_main.c | 298 ++++++++++++++++++++++-
 1 file changed, 295 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
index 5ac091bf93c9..89b7608aaa3a 100644
--- a/drivers/net/dsa/realtek/rtl8365mb_main.c
+++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
@@ -40,7 +40,8 @@
  * driver has only been tested with a fixed-link, but in principle it should not
  * matter.
  *
- * NOTE: Currently, only the RGMII interface is implemented in this driver.
+ * NOTE: Currently, only the RGMII interface is implemented in this driver. On
+ * the RTL8367S, the SGMII interface is also supported.
  *
  * The interrupt line is asserted on link UP/DOWN events. The driver creates a
  * custom irqchip to handle this interrupt and demultiplex the events by reading
@@ -129,6 +130,7 @@
 
 /* Chip reset register */
 #define RTL8365MB_CHIP_RESET_REG	0x1322
+#define RTL8365MB_CHIP_RESET_DW8051_MASK	0x0010
 #define RTL8365MB_CHIP_RESET_SW_MASK	0x0002
 #define RTL8365MB_CHIP_RESET_HW_MASK	0x0001
 
@@ -238,6 +240,49 @@
 #define   RTL8365MB_EXT_RGMXF_RXDELAY_MASK	0x0007
 #define   RTL8365MB_EXT_RGMXF_TXDELAY_MASK	0x0008
 
+/* External interface line rate bypass register - one bit per interface */
+#define RTL8365MB_BYPASS_LINE_RATE_REG	0x03F7
+
+/* SerDes indirect access registers */
+#define RTL8365MB_SDS_INDACS_CMD_REG		0x6600
+#define   RTL8365MB_SDS_INDACS_CMD_RUN_MASK	0x0080
+#define   RTL8365MB_SDS_INDACS_CMD_WR_MASK	0x0040
+#define   RTL8365MB_SDS_INDACS_CMD_INDEX_MASK	0x0007
+#define RTL8365MB_SDS_INDACS_ADR_REG		0x6601
+#define RTL8365MB_SDS_INDACS_DATA_REG		0x6602
+
+/* SerDes miscellaneous configuration register */
+#define RTL8365MB_SDS_MISC_REG				0x1D11
+#define   RTL8365MB_SDS_MISC_SGMII_RXFC_MASK		0x4000
+#define   RTL8365MB_SDS_MISC_SGMII_TXFC_MASK		0x2000
+#define   RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_MASK	0x0800
+#define   RTL8365MB_SDS_MISC_SGMII_FDUP_MASK		0x0400
+#define   RTL8365MB_SDS_MISC_SGMII_LINK_MASK		0x0200
+#define   RTL8365MB_SDS_MISC_SGMII_SPD_MASK		0x0180
+#define   RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK	0x0040
+
+/* SerDes internal registers, accessed via the SDS_INDACS registers. The
+ * BMCR data path reset values set BMCR_ANENABLE | BMCR_ISOLATE (0x1400),
+ * and toggling the vendor-specific low bits from 0x1 to 0x3 triggers a
+ * data path reset and PLL resync.
+ */
+#define RTL8365MB_SDS_REG_BMCR			0x0000
+#define   RTL8365MB_SDS_BMCR_DPRST_PHASE1	0x1401
+#define   RTL8365MB_SDS_BMCR_DPRST_PHASE2	0x1403
+#define RTL8365MB_SDS_REG_NWAY			0x0002
+#define   RTL8365MB_SDS_NWAY_EN_MASK		0x0200
+#define   RTL8365MB_SDS_NWAY_RESTART_MASK	0x0100
+#define RTL8365MB_SDS_REG_RESET			0x0003
+#define   RTL8365MB_SDS_RESET_DEASSERT		0x7106
+
+/* 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
+ * the firmware would otherwise do.
+ */
+#define RTL8365MB_MISC_CFG0_REG			0x130C
+#define   RTL8365MB_MISC_CFG0_DW8051_EN_MASK	0x0020
+
 /* External interface port speed values - used in DIGITAL_INTERFACE_FORCE */
 #define RTL8365MB_PORT_SPEED_10M	0
 #define RTL8365MB_PORT_SPEED_100M	1
@@ -551,6 +596,13 @@ static const struct rtl8365mb_jam_tbl_entry rtl8365mb_init_jam_common[] = {
 	{ 0x1D32, 0x0002 },
 };
 
+/* SGMII SerDes tuning parameters, lifted from the vendor driver sources */
+static const struct rtl8365mb_jam_tbl_entry rtl8365mb_sds_jam_sgmii[] = {
+	{ 0x0480, 0x04D7 }, { 0x0481, 0xF994 }, { 0x0482, 0x2420 },
+	{ 0x0483, 0x6960 }, { 0x0484, 0x9728 }, { 0x0423, 0x9D85 },
+	{ 0x0424, 0xD810 }, { 0x002E, 0x83F2 },
+};
+
 enum rtl8365mb_phy_interface_mode {
 	RTL8365MB_PHY_INTERFACE_MODE_INVAL = 0,
 	RTL8365MB_PHY_INTERFACE_MODE_INTERNAL = BIT(0),
@@ -1042,6 +1094,212 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
 	return 0;
 }
 
+static int rtl8365mb_sds_write(struct realtek_priv *priv, int sds, u16 addr,
+			       u16 data)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA_REG, data);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR_REG, addr);
+	if (ret)
+		return ret;
+
+	val = RTL8365MB_SDS_INDACS_CMD_RUN_MASK |
+	      RTL8365MB_SDS_INDACS_CMD_WR_MASK |
+	      FIELD_PREP(RTL8365MB_SDS_INDACS_CMD_INDEX_MASK, sds);
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD_REG, val);
+	if (ret)
+		return ret;
+
+	/* The vendor driver does not poll for completion, but a short delay
+	 * is needed before issuing the next command.
+	 */
+	usleep_range(10, 50);
+
+	return 0;
+}
+
+static int rtl8365mb_sds_read(struct realtek_priv *priv, int sds, u16 addr,
+			      u16 *data)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR_REG, addr);
+	if (ret)
+		return ret;
+
+	val = RTL8365MB_SDS_INDACS_CMD_RUN_MASK |
+	      FIELD_PREP(RTL8365MB_SDS_INDACS_CMD_INDEX_MASK, sds);
+	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD_REG, val);
+	if (ret)
+		return ret;
+
+	usleep_range(10, 50);
+
+	ret = regmap_read(priv->map, RTL8365MB_SDS_INDACS_DATA_REG, &val);
+	if (ret)
+		return ret;
+
+	*data = val;
+
+	return 0;
+}
+
+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.
+	 */
+	ret = rtl8365mb_sds_read(priv, 0, RTL8365MB_SDS_REG_NWAY, &val);
+	if (ret)
+		return ret;
+
+	val &= ~RTL8365MB_SDS_NWAY_EN_MASK;
+	val |= RTL8365MB_SDS_NWAY_RESTART_MASK;
+
+	return rtl8365mb_sds_write(priv, 0, RTL8365MB_SDS_REG_NWAY, val);
+}
+
+static bool rtl8365mb_interface_is_serdes(phy_interface_t interface)
+{
+	return interface == PHY_INTERFACE_MODE_SGMII;
+}
+
+static int rtl8365mb_sds_config_forcemode(struct realtek_priv *priv, bool link,
+					  int speed, int duplex, bool tx_pause,
+					  bool rx_pause)
+{
+	u32 mask = RTL8365MB_SDS_MISC_SGMII_RXFC_MASK |
+		   RTL8365MB_SDS_MISC_SGMII_TXFC_MASK |
+		   RTL8365MB_SDS_MISC_SGMII_FDUP_MASK |
+		   RTL8365MB_SDS_MISC_SGMII_LINK_MASK |
+		   RTL8365MB_SDS_MISC_SGMII_SPD_MASK;
+	u32 val = 0;
+	u32 r_speed;
+
+	if (link) {
+		if (speed == SPEED_1000) {
+			r_speed = RTL8365MB_PORT_SPEED_1000M;
+		} else if (speed == SPEED_100) {
+			r_speed = RTL8365MB_PORT_SPEED_100M;
+		} else if (speed == SPEED_10) {
+			r_speed = RTL8365MB_PORT_SPEED_10M;
+		} else {
+			dev_err(priv->dev, "unsupported port speed %s\n",
+				phy_speed_to_str(speed));
+			return -EINVAL;
+		}
+
+		val |= RTL8365MB_SDS_MISC_SGMII_LINK_MASK;
+		val |= FIELD_PREP(RTL8365MB_SDS_MISC_SGMII_SPD_MASK, r_speed);
+
+		if (duplex == DUPLEX_FULL)
+			val |= RTL8365MB_SDS_MISC_SGMII_FDUP_MASK;
+
+		if (tx_pause)
+			val |= RTL8365MB_SDS_MISC_SGMII_TXFC_MASK;
+
+		if (rx_pause)
+			val |= RTL8365MB_SDS_MISC_SGMII_RXFC_MASK;
+	}
+
+	return regmap_update_bits(priv->map, RTL8365MB_SDS_MISC_REG, mask,
+				  val);
+}
+
 static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
 					  bool link, int speed, int duplex,
 					  bool tx_pause, bool rx_pause)
@@ -1141,6 +1399,10 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
 
 	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_RGMII)
 		phy_interface_set_rgmii(config->supported_interfaces);
+
+	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_SGMII)
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  config->supported_interfaces);
 }
 
 static void rtl8365mb_phylink_mac_config(struct phylink_config *config,
@@ -1168,6 +1430,15 @@ static void rtl8365mb_phylink_mac_config(struct phylink_config *config,
 		return;
 	}
 
+	if (rtl8365mb_interface_is_serdes(state->interface)) {
+		ret = rtl8365mb_ext_config_sgmii(priv, port);
+		if (ret)
+			dev_err(priv->dev,
+				"failed to configure SGMII mode on port %d: %d\n",
+				port, ret);
+		return;
+	}
+
 	/* TODO: Implement MII and RMII modes, which the RTL8365MB-VC also
 	 * supports
 	 */
@@ -1188,7 +1459,8 @@ static void rtl8365mb_phylink_mac_link_down(struct phylink_config *config,
 	p = &mb->ports[port];
 	cancel_delayed_work_sync(&p->mib_work);
 
-	if (phy_interface_mode_is_rgmii(interface)) {
+	if (phy_interface_mode_is_rgmii(interface) ||
+	    rtl8365mb_interface_is_serdes(interface)) {
 		ret = rtl8365mb_ext_config_forcemode(priv, port, false, 0, 0,
 						     false, false);
 		if (ret)
@@ -1196,6 +1468,15 @@ static void rtl8365mb_phylink_mac_link_down(struct phylink_config *config,
 				"failed to reset forced mode on port %d: %pe\n",
 				port, ERR_PTR(ret));
 
+		if (rtl8365mb_interface_is_serdes(interface)) {
+			ret = rtl8365mb_sds_config_forcemode(priv, false, 0, 0,
+							     false, false);
+			if (ret)
+				dev_err(priv->dev,
+					"failed to reset forced SGMII link on port %d: %d\n",
+					port, ret);
+		}
+
 		return;
 	}
 }
@@ -1218,7 +1499,8 @@ 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)) {
+	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);
@@ -1227,6 +1509,16 @@ static void rtl8365mb_phylink_mac_link_up(struct phylink_config *config,
 				"failed to force mode on port %d: %pe\n", port,
 				ERR_PTR(ret));
 
+		if (rtl8365mb_interface_is_serdes(interface)) {
+			ret = rtl8365mb_sds_config_forcemode(priv, true, speed,
+							     duplex, tx_pause,
+							     rx_pause);
+			if (ret)
+				dev_err(priv->dev,
+					"failed to force SGMII link on port %d: %d\n",
+					port, ret);
+		}
+
 		return;
 	}
 }

base-commit: 8f4695fb67b259b2cae0be1eef55859bfc559058
-- 
2.54.0

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

[Johan Alvarado]

In addition to SGMII, the RTL8367S SerDes also supports HSGMII, which
carries 2.5 Gbps with the same signaling as SGMII at 2.5x clock rate.
The chip info table already declares HSGMII as a supported interface
mode for external interface 1.

Extend the SGMII configuration path to handle HSGMII, which phylink
represents as 2500base-x:

 - Use the HSGMII SerDes tuning parameters and external interface mode,
   and mux the SerDes to MAC8 in HSGMII mode. The parameters are again
   lifted from the GPL-licensed Realtek rtl8367c vendor driver.

 - Advertise 2500base-x and MAC_2500FD on ports whose external
   interface supports HSGMII.

 - Accept SPEED_2500 in the force mode configuration. The MAC speed
   field has no 2.5 Gbps value: the rate is determined by the HSGMII
   SerDes configuration, and the vendor driver programs the 1 Gbps
   value here, so do the same.

Tested on a Mercusys MR80X v2.20, where the RTL8367S is connected to
the SoC over HSGMII.

Signed-off-by: Johan Alvarado <contact@c127.dev>
---
 drivers/net/dsa/realtek/rtl8365mb_main.c | 66 ++++++++++++++++++------
 1 file changed, 51 insertions(+), 15 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb_main.c b/drivers/net/dsa/realtek/rtl8365mb_main.c
index 89b7608aaa3a..ac5348f505b9 100644
--- a/drivers/net/dsa/realtek/rtl8365mb_main.c
+++ b/drivers/net/dsa/realtek/rtl8365mb_main.c
@@ -41,7 +41,7 @@
  * matter.
  *
  * NOTE: Currently, only the RGMII interface is implemented in this driver. On
- * the RTL8367S, the SGMII interface is also supported.
+ * the RTL8367S, the SGMII and HSGMII interfaces are also supported.
  *
  * The interrupt line is asserted on link UP/DOWN events. The driver creates a
  * custom irqchip to handle this interrupt and demultiplex the events by reading
@@ -603,6 +603,13 @@ static const struct rtl8365mb_jam_tbl_entry rtl8365mb_sds_jam_sgmii[] = {
 	{ 0x0424, 0xD810 }, { 0x002E, 0x83F2 },
 };
 
+/* HSGMII SerDes tuning parameters, lifted from the vendor driver sources */
+static const struct rtl8365mb_jam_tbl_entry rtl8365mb_sds_jam_hsgmii[] = {
+	{ 0x0500, 0x82F0 }, { 0x0501, 0xF195 }, { 0x0502, 0x31A2 },
+	{ 0x0503, 0x7960 }, { 0x0504, 0x9728 }, { 0x0423, 0x9D85 },
+	{ 0x0424, 0xD810 }, { 0x0001, 0x0F80 }, { 0x002E, 0x83F2 },
+};
+
 enum rtl8365mb_phy_interface_mode {
 	RTL8365MB_PHY_INTERFACE_MODE_INVAL = 0,
 	RTL8365MB_PHY_INTERFACE_MODE_INTERNAL = BIT(0),
@@ -1150,10 +1157,14 @@ static int rtl8365mb_sds_read(struct realtek_priv *priv, int sds, u16 addr,
 	return 0;
 }
 
-static int rtl8365mb_ext_config_sgmii(struct realtek_priv *priv, int port)
+static int rtl8365mb_ext_config_sgmii(struct realtek_priv *priv, int port,
+				      phy_interface_t interface)
 {
 	const struct rtl8365mb_extint *extint =
 		rtl8365mb_get_port_extint(priv, port);
+	const struct rtl8365mb_jam_tbl_entry *sds_jam;
+	size_t sds_jam_size;
+	u32 mode;
 	u16 val;
 	int ret;
 	int i;
@@ -1165,6 +1176,16 @@ static int rtl8365mb_ext_config_sgmii(struct realtek_priv *priv, int port)
 	if (extint->id != 1)
 		return -EOPNOTSUPP;
 
+	if (interface == PHY_INTERFACE_MODE_2500BASEX) {
+		sds_jam = rtl8365mb_sds_jam_hsgmii;
+		sds_jam_size = ARRAY_SIZE(rtl8365mb_sds_jam_hsgmii);
+		mode = RTL8365MB_EXT_PORT_MODE_HSGMII;
+	} else {
+		sds_jam = rtl8365mb_sds_jam_sgmii;
+		sds_jam_size = ARRAY_SIZE(rtl8365mb_sds_jam_sgmii);
+		mode = RTL8365MB_EXT_PORT_MODE_SGMII;
+	}
+
 	/* 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
@@ -1192,28 +1213,28 @@ static int rtl8365mb_ext_config_sgmii(struct realtek_priv *priv, int port)
 		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);
+	for (i = 0; i < sds_jam_size; i++) {
+		ret = rtl8365mb_sds_write(priv, 0, sds_jam[i].reg,
+					  sds_jam[i].val);
 		if (ret)
 			return ret;
 	}
 
-	/* Mux the SerDes to MAC8 in SGMII mode */
+	/* Mux the SerDes to MAC8 in the requested 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);
+				 mode == RTL8365MB_EXT_PORT_MODE_SGMII ?
+					 RTL8365MB_SDS_MISC_MAC8_SEL_SGMII_MASK :
+					 RTL8365MB_SDS_MISC_MAC8_SEL_HSGMII_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));
+		mode << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(
+			       extint->id));
 	if (ret)
 		return ret;
 
@@ -1255,7 +1276,8 @@ static int rtl8365mb_ext_config_sgmii(struct realtek_priv *priv, int port)
 
 static bool rtl8365mb_interface_is_serdes(phy_interface_t interface)
 {
-	return interface == PHY_INTERFACE_MODE_SGMII;
+	return interface == PHY_INTERFACE_MODE_SGMII ||
+	       interface == PHY_INTERFACE_MODE_2500BASEX;
 }
 
 static int rtl8365mb_sds_config_forcemode(struct realtek_priv *priv, bool link,
@@ -1271,7 +1293,11 @@ static int rtl8365mb_sds_config_forcemode(struct realtek_priv *priv, bool link,
 	u32 r_speed;
 
 	if (link) {
-		if (speed == SPEED_1000) {
+		/* The speed field has no value for 2.5 Gbps: the rate is
+		 * determined by the HSGMII SerDes configuration, and the
+		 * vendor driver programs the 1 Gbps value here.
+		 */
+		if (speed == SPEED_2500 || speed == SPEED_1000) {
 			r_speed = RTL8365MB_PORT_SPEED_1000M;
 		} else if (speed == SPEED_100) {
 			r_speed = RTL8365MB_PORT_SPEED_100M;
@@ -1323,7 +1349,11 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
 		r_rx_pause = rx_pause ? 1 : 0;
 		r_tx_pause = tx_pause ? 1 : 0;
 
-		if (speed == SPEED_1000) {
+		/* The speed field has no value for 2.5 Gbps: the rate is
+		 * determined by the HSGMII SerDes configuration, and the
+		 * vendor driver programs the 1 Gbps value here.
+		 */
+		if (speed == SPEED_2500 || speed == SPEED_1000) {
 			r_speed = RTL8365MB_PORT_SPEED_1000M;
 		} else if (speed == SPEED_100) {
 			r_speed = RTL8365MB_PORT_SPEED_100M;
@@ -1403,6 +1433,12 @@ static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
 	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_SGMII)
 		__set_bit(PHY_INTERFACE_MODE_SGMII,
 			  config->supported_interfaces);
+
+	if (extint->supported_interfaces & RTL8365MB_PHY_INTERFACE_MODE_HSGMII) {
+		__set_bit(PHY_INTERFACE_MODE_2500BASEX,
+			  config->supported_interfaces);
+		config->mac_capabilities |= MAC_2500FD;
+	}
 }
 
 static void rtl8365mb_phylink_mac_config(struct phylink_config *config,
@@ -1431,7 +1467,7 @@ static void rtl8365mb_phylink_mac_config(struct phylink_config *config,
 	}
 
 	if (rtl8365mb_interface_is_serdes(state->interface)) {
-		ret = rtl8365mb_ext_config_sgmii(priv, port);
+		ret = rtl8365mb_ext_config_sgmii(priv, port, state->interface);
 		if (ret)
 			dev_err(priv->dev,
 				"failed to configure SGMII mode on port %d: %d\n",
-- 
2.54.0

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@gmail.com>
> Signed-off-by: Johan Alvarado <contact@c127.dev>
> ---

[...]

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

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

[Johan Alvarado]

> This comment implies that you could deal with SGMII aneg at some point.
> [...] makes me wonder if this whole SGMII/2500BaseX series should be
> represented as a PCS phylink driver.

Hi Maxime,

You're right, and I'll convert the SerDes path to a phylink_pcs for v4.
It splits the MAC and SerDes layers cleanly, drops the "ext then sds"
branches in mac_link_up/down, and makes future in-band aneg an additive
change instead of a rewrite.

One point I'd like to confirm on scope: I can only test the forced-link
path on my MR80X (fixed-link / conventional PHY), and I have no setup to
exercise SGMII in-band autonegotiation. My plan is to do the PCS refactor
keeping the link forced (outband / no in-band AN), and leave actual
in-band aneg support for a follow-up once I have hardware to validate it.
Does limiting v4 to the forced path sound acceptable, or would you prefer
in-band aneg implemented up front? I'd rather not add a code path I can't
test.

I'll also reword the misleading "disable in-band aneg" comment.

net-next being closed until the 29th gives me time to do this properly,
so v4 will carry the PCS conversion, retested on the MR80X v2.20.

Best regards,
Johan

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

[Maxime Chevallier]

Hi Johan,

On 6/15/26 22:41, Johan Alvarado wrote:
>> This comment implies that you could deal with SGMII aneg at some point.
>> [...] makes me wonder if this whole SGMII/2500BaseX series should be
>> represented as a PCS phylink driver.
> 
> Hi Maxime,
> 
> You're right, and I'll convert the SerDes path to a phylink_pcs for v4.
> It splits the MAC and SerDes layers cleanly, drops the "ext then sds"
> branches in mac_link_up/down, and makes future in-band aneg an additive
> change instead of a rewrite.

great !

> 
> One point I'd like to confirm on scope: I can only test the forced-link
> path on my MR80X (fixed-link / conventional PHY), and I have no setup to
> exercise SGMII in-band autonegotiation. My plan is to do the PCS refactor
> keeping the link forced (outband / no in-band AN), and leave actual
> in-band aneg support for a follow-up once I have hardware to validate it.
> Does limiting v4 to the forced path sound acceptable, or would you prefer
> in-band aneg implemented up front? I'd rather not add a code path I can't
> test.

That's fine by me, it's usually better to have something smaller but fully
tested :)

> 
> I'll also reword the misleading "disable in-band aneg" comment.
> 
> net-next being closed until the 29th gives me time to do this properly,
> so v4 will carry the PCS conversion, retested on the MR80X v2.20.

Thanks,

Maxime

> 
> Best regards,
> Johan

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

[Johan Alvarado]

Hi Luiz,

Sorry for the slow reply — I wanted to test the changes on hardware
before answering rather than reply blind. I'm also adding the list
back to CC, since this is review of an on-list patch and the
discussion is useful for the archive; hope that's OK.

Thanks for the very thorough review. Almost everything is addressed in
v4, which I'll post once net-next reopens. Replies inline.

> You might want to omit any specific model as there are multiple
> possible supported models that have SGMII. Just use "only the RGMII
> and SGMII interface...". Determining which device supports what is a
> job for chip_info.

Done — the NOTE comment no longer names a model, it just lists the
implemented interfaces.

> > +#define   RTL8365MB_SDS_INDACS_CMD_INDEX_MASK  0x0007
>
> Isn't this MASK larger? I was expecting 0x003F.
>
> Please use GENMASK/BIT whenever possible. It makes it clearer when
> there are holes or overlaps in the reg.
> Once a register macro is added with a bunch of bits described, I think
> it is better to describe all bits we can, even if not in use in this
> driver. Realtek normally maps bits sequentially and, with GENMASK/BIT,
> it is visually easier to spot errors.

The CMD index field is gone in v4: I dropped the always-zero SerDes
index argument, so that mask was removed with it.

On GENMASK/BIT and documenting all bits: agreed in principle, but the
driver currently uses raw hex masks almost everywhere (~75 raw _MASK
defines vs ~22 BIT/GENMASK), so converting only the new SDS defines
would make the file more inconsistent rather than less. Options as I
see them: (1) convert just the new defines, (2) keep raw hex to match
the surrounding style, or (3) a separate file-wide cleanup patch on
top of this series. I'd lean towards 3, keeping this series focused on
the feature and doing the style modernization as its own patch, but
I'm happy to do 1 if you'd rather the new code lead by example. Same
question for documenting currently-unused bits. Which would you prefer?

> > +#define RTL8365MB_SDS_INDACS_ADR_REG           0x6601
>
> This reg is formed by two parts but, in this case, it might be
> pedantic to add the descriptions as well.
> PAGE_MASK    0x7E0
> REGAD_MASK    0x1F

Noted. Since the addresses are passed as whole values and the page/reg
split isn't used in the driver, I've left it as a single address for
now, but I can add the sub-field masks if you'd prefer them documented.

> > +#define   RTL8365MB_SDS_BMCR_DPRST_PHASE1      0x1401
> > +#define   RTL8365MB_SDS_BMCR_DPRST_PHASE2      0x1403
>
> I do not like magic numbers. You could do the BMCR_ANENABLE |
> BMCR_ISOLATE in the macro or code instead of just keeping it in a
> comment. It would give more semantics to the code.

Done — the phase values are now built from the standard bits:

  #define RTL8365MB_SDS_BMCR_DPRST_PHASE1 (BMCR_ANENABLE | BMCR_ISOLATE | 0x1)
  #define RTL8365MB_SDS_BMCR_DPRST_PHASE2 (BMCR_ANENABLE | BMCR_ISOLATE | 0x3)

> > +static const struct rtl8365mb_jam_tbl_entry rtl8365mb_sds_jam_sgmii[] = {
>
> I guess you got this from vendor's redData. However, that sequence is
> for the case when RTL8365MB_CHIP_OPTION_REG(0x13C1) == 0. In my tests,
> rtl8367s returns 1 for that reg, which would select the redDataSB
> variant in the vendor's code. Did you test both or check register
> 0x13C1 [...] HSGMII also has a similar test in vendor's code.

Good catch. I checked 0x13C1 on the MR80X (with the magic
unlock/relock via 0x13C0): it reads option = 1, so the committed
tables are already the SB/HB variants. For SGMII the only difference
between redData and redDataSB is reg 0x482 (0x21A2 for option 0 vs
0x2420 for option 1), and my table has 0x2420; the HSGMII table
matches redDataHB likewise. I captured the full vendor write sequence
on hardware by chainloading a patched U-Boot, so both tables are
confirmed against the live silicon.

v4 reads 0x13C1 at runtime and returns -EOPNOTSUPP for the option-0
variant rather than driving the SerDes with values I cannot verify on
available hardware.

> > +       if (extint->id != 1)
> > +               return -EOPNOTSUPP;
>
> The model RTL8370MB is also a member of RTL8367C [...] Can't you just
> check extint supported_interfaces? [...] This type of hardcoded
> assumption just makes the job harder.

In v4 this hardcoded check is gone. With the phylink_pcs conversion
(see below), the SerDes is gated through supported_interfaces in
get_caps() and mac_select_pcs(), so whether a port uses the SerDes is
driven by chip_info rather than a hardcoded id.

> > +       ret = regmap_update_bits(priv->map, RTL8365MB_BYPASS_LINE_RATE_REG,
> > +                                BIT(extint->id), 0);
>
> BYPASS_LINE_RATE is actually indexed by port number starting at 5
> [...] Describe [it] with a parametric macro, receiving the port number
> and returning the BIT(5-port) as mask [...] For RTL8367R [...] it uses
> bit 0 for int 1 and bit 1 for int 0 or 2.

Done — it's now a parametric macro:

  #define RTL8365MB_BYPASS_LINE_RATE_MASK(_port)  BIT((_port) - 5)

with a comment noting port 5 is the base and that other families (e.g.
the RTL8367R's (id + 1) % 2) index it differently, so this mapping
only holds for the RTL8367C-style parts the driver supports. One small
thing: your mail had BIT(5 - port), which inverts it (port 6 would be
BIT(-1)); I used BIT(port - 5) so port 5 maps to bit 0 — let me know if
you meant something different.

> > +       usleep_range(10, 50);
>
> An arbitrary wait is not ideal but Mieczyslaw already suggested a
> better solution.

Done — the usleep is gone. Writes are fire-and-forget and reads poll
the self-clearing BUSY bit with regmap_read_poll_timeout(), matching
the vendor's getAsicSdsReg. I instrumented it on the MR80X: the BUSY
bit is never even observed set (the access completes within the
register transaction over MDIO), so no sleep is needed.

> > +       ret = regmap_update_bits(
> > +               priv->map, RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(extint->id),
> > +               [...]
>
> Sometimes it is just easier to use a temp variable instead of fighting
> with the 80-col limit.

Done, the mode value goes into a temporary now.

> The lack of test devices is holding me back from making further
> improvements. [...] I think that, with some limitations, the rtl8365mb
> driver [...] could support the full range from RTL8370/RTL8367 up to
> RTL8367D.

Makes sense, and I tried to keep v4 from baking in RTL8367S-specific
assumptions where I could (the SerDes gating and the bypass macro
above). I can only test on the MR80X (RTL8367S) myself, so I've kept
the scope to what I can verify, but I'm happy to keep the code friendly
to that wider range.

One more thing from the wider discussion: following Maxime's review I
converted the whole SerDes path to a phylink_pcs in v4, so the SerDes
handling now lives in pcs_config()/pcs_get_state()/pcs_link_up() rather
than the ext/sds split in mac_link_up/down you saw in v3. pcs_get_state()
now reads the real SerDes link status (reg 0x3d) instead of reporting
the forced value.

Thanks again — this review made the series substantially better.

Best regards,
Johan