[PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

From: Tristram Ha <tristram.ha@microchip.com>

The SGMII module of KSZ9477 switch can be setup in 3 ways: direct connect
without using any SFP, SGMII mode with 10/100/1000Base-T SFP, and SerDes
mode with 1000BaseX SFP, which can be fiber or copper.  Note some
1000Base-T copper SFPs advertise themselves as SGMII but start in
1000BaseX mode, and the PHY driver of the PHY inside will change it to
SGMII mode.

The SGMII module can only support basic link status of the SFP, so the
port can be simulated as having a regular internal PHY when SFP cage
logic is not present.  The original KSZ9477 evaluation board operates in
this way and so requires the simulation code.

A PCS driver for the SGMII port is provided to support the SFP cage
logic used in the phylink code.  It is used to confirm the link is up
and process the SGMII interrupt.

One issue for the 1000BaseX SFP is there is no link down interrupt, so
the driver has to use polling to detect link off when the link is up.

Note the SGMII interrupt cannot be masked in hardware.  Also the module
is not reset when the switch is reset.  It is important to reset the
module properly to make sure interrupt is not triggered prematurely.

One side effect is the SGMII interrupt is triggered when an internal PHY
is powered down and powered up.  This happens when a port using internal
PHY is turned off and then turned on.

Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
v2
 - use standard MDIO names when programming MMD registers
 - use pcs_config API to setup SGMII module
 - remove the KSZ9477 device tree example as it was deemed unnecessary

 drivers/net/dsa/microchip/ksz9477.c     | 455 +++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz9477.h     |   9 +-
 drivers/net/dsa/microchip/ksz9477_reg.h |   1 +
 drivers/net/dsa/microchip/ksz_common.c  | 111 +++++-
 drivers/net/dsa/microchip/ksz_common.h  |  23 +-
 5 files changed, 588 insertions(+), 11 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 29fe79ea74cd..3613eea1e3fb 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -2,7 +2,7 @@
 /*
  * Microchip KSZ9477 switch driver main logic
  *
- * Copyright (C) 2017-2024 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #include <linux/kernel.h>
@@ -12,6 +12,8 @@
 #include <linux/phy.h>
 #include <linux/if_bridge.h>
 #include <linux/if_vlan.h>
+#include <linux/irqdomain.h>
+#include <linux/phylink.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
 
@@ -161,6 +163,415 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
 					10, 1000);
 }
 
+static void port_sgmii_s(struct ksz_device *dev, uint port, u16 devid, u16 reg,
+			 u16 len)
+{
+	u32 data;
+
+	data = (devid & MII_MMD_CTRL_DEVAD_MASK) << 16;
+	data |= reg;
+	if (len > 1)
+		data |= PORT_SGMII_AUTO_INCR;
+	ksz_pwrite32(dev, port, REG_PORT_SGMII_ADDR__4, data);
+}
+
+static void port_sgmii_r(struct ksz_device *dev, uint port, u16 devid, u16 reg,
+			 u16 *buf, u16 len)
+{
+	u32 data;
+
+	mutex_lock(&dev->sgmii_mutex);
+	port_sgmii_s(dev, port, devid, reg, len);
+	while (len) {
+		ksz_pread32(dev, port, REG_PORT_SGMII_DATA__4, &data);
+		*buf++ = (u16)data;
+		len--;
+	}
+	mutex_unlock(&dev->sgmii_mutex);
+}
+
+static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
+			 u16 *buf, u16 len)
+{
+	u32 data;
+
+	mutex_lock(&dev->sgmii_mutex);
+	port_sgmii_s(dev, port, devid, reg, len);
+	while (len) {
+		data = *buf++;
+		ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, data);
+		len--;
+	}
+	mutex_unlock(&dev->sgmii_mutex);
+}
+
+static void port_sgmii_reset(struct ksz_device *dev, uint p)
+{
+	u16 ctrl = BMCR_RESET;
+
+	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
+}
+
+static phy_interface_t port_sgmii_detect(struct ksz_device *dev, uint p)
+{
+	phy_interface_t interface = PHY_INTERFACE_MODE_1000BASEX;
+	u16 buf[6];
+	int i = 0;
+
+	/* Read all 6 registers to spend more time waiting for valid result. */
+	do {
+		port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMCR, buf, 6);
+		i++;
+	} while (!buf[5] && i < 10);
+	if ((buf[5] & LPA_LPACK) &&
+	    (!(buf[5] & (LPA_1000XHALF | LPA_1000XFULL))))
+		interface = PHY_INTERFACE_MODE_SGMII;
+	return interface;
+}
+
+static void port_sgmii_setup(struct ksz_device *dev, uint p, bool pcs,
+			     bool master, bool autoneg, bool intr, int speed,
+			     int duplex)
+{
+	u16 ctrl;
+	u16 cfg;
+	u16 adv;
+
+	cfg = 0;
+	if (pcs)
+		cfg |= SR_MII_PCS_SGMII << SR_MII_PCS_MODE_S;
+	if (master) {
+		cfg |= SR_MII_TX_CFG_PHY_MASTER;
+		cfg |= SR_MII_SGMII_LINK_UP;
+	}
+	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_CTRL, &cfg, 1);
+
+	/* Need to write to advertise register to send correct signal. */
+	/* Default value is 0x0020. */
+	adv = ADVERTISE_1000XPSE_ASYM | ADVERTISE_1000XPAUSE;
+	adv |= (duplex == DUPLEX_FULL) ?
+	       ADVERTISE_1000XFULL : ADVERTISE_1000XHALF;
+	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_ADVERTISE, &adv, 1);
+	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
+	if (master || !autoneg) {
+		ctrl &= ~(BMCR_SPEED1000 | BMCR_SPEED100 | BMCR_FULLDPLX);
+		switch (speed) {
+		case SPEED_100:
+			ctrl |= BMCR_SPEED100;
+			break;
+		case SPEED_1000:
+			ctrl |= BMCR_SPEED1000;
+			break;
+		}
+		if (duplex == DUPLEX_FULL)
+			ctrl |= BMCR_FULLDPLX;
+	}
+	if (!autoneg) {
+		ctrl &= ~BMCR_ANENABLE;
+		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
+		goto sgmii_setup_last;
+	} else if (!(ctrl & BMCR_ANENABLE)) {
+		ctrl |= BMCR_ANENABLE;
+		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
+	}
+	if (master && autoneg) {
+		ctrl |= BMCR_ANRESTART;
+		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
+	}
+
+sgmii_setup_last:
+	if (intr) {
+		cfg |= SR_MII_AUTO_NEG_COMPLETE_INTR;
+		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_CTRL,
+			     &cfg, 1);
+	}
+}
+
+#define PORT_LINK_UP		BIT(0)
+#define PORT_LINK_CHANGE	BIT(1)
+#define PORT_LINK_INVALID	BIT(2)
+
+static int sgmii_port_get_speed(struct ksz_device *dev, uint p)
+{
+	struct ksz_port *info = &dev->ports[p];
+	struct ksz_pcs *priv = info->pcs_priv;
+	int ret = 0;
+	u16 status;
+	u16 speed;
+	u16 data;
+	u8 link;
+
+	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
+	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
+	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_STATUS, &data,
+		     1);
+
+	/* Typical register values with different SFPs.
+	 * 10/100/1000: 1f0001 = 01ad  1f0005 = 4000  1f8002 = 0008
+	 *              1f0001 = 01bd  1f0005 = d000  1f8002 = 001a
+	 * 1000:        1f0001 = 018d  1f0005 = 0000  1f8002 = 0000
+	 *              1f0001 = 01ad  1f0005 = 40a0  1f8002 = 0001
+	 *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001
+	 * fiber:       1f0001 = 0189  1f0005 = 0000  1f8002 = 0000
+	 *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001
+	 */
+
+	if (data & SR_MII_AUTO_NEG_COMPLETE_INTR) {
+		data &= ~SR_MII_AUTO_NEG_COMPLETE_INTR;
+		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_STATUS,
+			     &data, 1);
+	}
+
+	/* Not running in SGMII mode where data indicates link status. */
+	if (info->interface != PHY_INTERFACE_MODE_SGMII && !data) {
+		u16 link_up = BMSR_LSTATUS;
+
+		if (info->interface == PHY_INTERFACE_MODE_1000BASEX)
+			link_up |= BMSR_ANEGCOMPLETE;
+		if ((status & link_up) == link_up)
+			data = SR_MII_STAT_LINK_UP |
+			       (SR_MII_STAT_1000_MBPS << SR_MII_STAT_S) |
+			       SR_MII_STAT_FULL_DUPLEX;
+	}
+	if (data & SR_MII_STAT_LINK_UP)
+		ret = PORT_LINK_UP;
+
+	link = (data & ~SR_MII_AUTO_NEG_COMPLETE_INTR);
+	if (priv->link == link)
+		return ret;
+
+	if (data & SR_MII_STAT_LINK_UP) {
+		u16 ctrl = 0;
+
+		/* Need to update control register with same link setting. */
+		if (info->interface != PHY_INTERFACE_MODE_INTERNAL)
+			ctrl = BMCR_ANENABLE;
+		speed = (data >> SR_MII_STAT_S) & SR_MII_STAT_M;
+		if (speed == SR_MII_STAT_1000_MBPS)
+			ctrl |= BMCR_SPEED1000;
+		else if (speed == SR_MII_STAT_100_MBPS)
+			ctrl |= BMCR_SPEED100;
+		if (data & SR_MII_STAT_FULL_DUPLEX)
+			ctrl |= BMCR_FULLDPLX;
+		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
+
+		info->phydev.speed = SPEED_10;
+		if (speed == SR_MII_STAT_1000_MBPS)
+			info->phydev.speed = SPEED_1000;
+		else if (speed == SR_MII_STAT_100_MBPS)
+			info->phydev.speed = SPEED_100;
+
+		info->phydev.duplex = 0;
+		if (data & SR_MII_STAT_FULL_DUPLEX)
+			info->phydev.duplex = 1;
+	}
+	ret |= PORT_LINK_CHANGE;
+	priv->link = link;
+	info->phydev.link = (ret & PORT_LINK_UP);
+	return ret;
+}
+
+static bool sgmii_need_polling(struct ksz_device *dev, struct ksz_port *p)
+{
+	struct ksz_pcs *priv = p->pcs_priv;
+
+	/* SGMII mode has link up and link down interrupts. */
+	if (p->interface == PHY_INTERFACE_MODE_SGMII && priv->has_intr)
+		return false;
+
+	/* SerDes mode has link up interrupt but not link down interrupt. */
+	if (p->interface == PHY_INTERFACE_MODE_1000BASEX && priv->has_intr &&
+	    !p->phydev.link)
+		return false;
+
+	/* Direct connect mode has no link change. */
+	if (p->interface == PHY_INTERFACE_MODE_INTERNAL)
+		return false;
+	return true;
+}
+
+static void ksz9477_sgmii_setup(struct ksz_device *dev, int port,
+				phy_interface_t intf)
+{
+	struct ksz_port *p = &dev->ports[port];
+	struct ksz_pcs *priv = p->pcs_priv;
+	struct phy_device *phydev = NULL;
+	bool autoneg, master, pcs;
+
+	if (!priv->using_sfp && dev->ds->user_mii_bus)
+		phydev = mdiobus_get_phy(dev->ds->user_mii_bus, port);
+	if (phydev || p->interface == PHY_INTERFACE_MODE_INTERNAL)
+		intf = p->interface;
+
+	/* PHY driver can change the mode to PHY_INTERFACE_MODE_SGMII from
+	 * PHY_INTERFACE_MODE_1000BASEX, so this function can be called again
+	 * after the interface is changed.
+	 */
+	if (intf != p->interface) {
+		dev_info(dev->dev, "switching to %s after %s was detected.\n",
+			 phy_modes(intf), phy_modes(p->interface));
+		p->interface = intf;
+	}
+	switch (p->interface) {
+	case PHY_INTERFACE_MODE_SGMII:
+		autoneg = true;
+		master = false;
+		pcs = true;
+		break;
+	case PHY_INTERFACE_MODE_1000BASEX:
+		autoneg = true;
+		master = true;
+		pcs = false;
+		break;
+	default:
+		autoneg = false;
+		master = true;
+		pcs = true;
+		break;
+	}
+	port_sgmii_setup(dev, port, pcs, master, autoneg, priv->has_intr,
+			 SPEED_1000, DUPLEX_FULL);
+
+	sgmii_port_get_speed(dev, port);
+
+	/* Need to check link down if using 1000BASEX SFP. */
+	if (sgmii_need_polling(dev, p))
+		schedule_delayed_work(&dev->sgmii_check,
+				      msecs_to_jiffies(500));
+}
+
+static void sgmii_update_link(struct ksz_device *dev)
+{
+	u8 port = dev->info->sgmii_port - 1;
+	struct ksz_port *p = &dev->ports[port];
+	int ret;
+
+	ret = sgmii_port_get_speed(dev, port);
+	if (ret & PORT_LINK_CHANGE) {
+		struct phy_device *phydev;
+
+		/* When simulating PHY. */
+		p->phydev.interrupts = PHY_INTERRUPT_ENABLED;
+		phydev = mdiobus_get_phy(dev->ds->user_mii_bus, port);
+		if (phydev)
+			phy_trigger_machine(phydev);
+
+		/* When using SFP code. */
+		dsa_port_phylink_mac_change(dev->ds, port, p->phydev.link);
+	}
+
+	/* No interrupt for link down. */
+	if (sgmii_need_polling(dev, p))
+		schedule_delayed_work(&dev->sgmii_check,
+				      msecs_to_jiffies(500));
+}
+
+static void sgmii_check_work(struct work_struct *work)
+{
+	struct ksz_device *dev = container_of(work, struct ksz_device,
+					      sgmii_check.work);
+
+	sgmii_update_link(dev);
+}
+
+static irqreturn_t ksz9477_sgmii_irq_thread_fn(int irq, void *dev_id)
+{
+	struct ksz_pcs *priv = dev_id;
+	struct ksz_device *dev = priv->dev;
+	u8 port = priv->port;
+	u16 data16 = 0;
+
+	port_sgmii_w(dev, port, SR_MII, MMD_SR_MII_AUTO_NEG_STATUS, &data16, 1);
+	sgmii_update_link(dev);
+	return IRQ_HANDLED;
+}
+
+static void sgmii_initial_setup(struct ksz_device *dev, int port)
+{
+	struct ksz_port *p = &dev->ports[port];
+	struct ksz_pcs *priv = p->pcs_priv;
+	int irq, ret;
+
+	irq = irq_find_mapping(p->pirq.domain, PORT_SGMII_INT_LOC);
+	if (irq > 0) {
+		ret = request_threaded_irq(irq, NULL,
+					   ksz9477_sgmii_irq_thread_fn,
+					   IRQF_ONESHOT, "SGMII", priv);
+		if (!ret)
+			priv->has_intr = 1;
+	}
+
+	/* Make invalid so the correct value is set. */
+	priv->link = 0xff;
+
+	INIT_DELAYED_WORK(&dev->sgmii_check, sgmii_check_work);
+}
+
+int ksz9477_pcs_create(struct ksz_device *dev)
+{
+	/* This chip has a SGMII port. */
+	if (dev->info->sgmii_port > 0) {
+		int port = dev->info->sgmii_port - 1;
+		struct ksz_port *p = &dev->ports[port];
+		struct ksz_pcs *pcs_priv;
+
+		pcs_priv = devm_kzalloc(dev->dev, sizeof(struct ksz_pcs),
+					GFP_KERNEL);
+		if (!pcs_priv)
+			return -ENOMEM;
+		p->pcs_priv = pcs_priv;
+		pcs_priv->dev = dev;
+		pcs_priv->port = port;
+		pcs_priv->pcs.neg_mode = true;
+
+		/* Switch reset does not reset SGMII module. */
+		port_sgmii_reset(dev, port);
+
+		/* Detect which mode to use if not using direct connect. */
+		if (p->interface != PHY_INTERFACE_MODE_INTERNAL)
+			p->interface = port_sgmii_detect(dev, port);
+	}
+	return 0;
+}
+
+int ksz9477_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+		       phy_interface_t interface,
+		       const unsigned long *advertising)
+{
+	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
+	struct ksz_device *dev = priv->dev;
+	struct ksz_port *p = &dev->ports[priv->port];
+
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+		p->pcs_priv->using_sfp = 1;
+	ksz9477_sgmii_setup(dev, priv->port, interface);
+	return 0;
+}
+
+void ksz9477_pcs_get_state(struct phylink_pcs *pcs,
+			   struct phylink_link_state *state)
+{
+	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
+	struct ksz_device *dev = priv->dev;
+	struct ksz_port *p = &dev->ports[priv->port];
+	u8 status;
+	int ret;
+
+	ksz_pread8(dev, priv->port, REG_PORT_STATUS_0, &status);
+	ret = sgmii_port_get_speed(dev, priv->port);
+	if (!(ret & PORT_LINK_UP))
+		state->link = false;
+	state->duplex = p->phydev.duplex;
+	state->speed = p->phydev.speed;
+	state->pause &= ~(MLO_PAUSE_RX | MLO_PAUSE_TX);
+	if (status & PORT_RX_FLOW_CTRL)
+		state->pause |= MLO_PAUSE_RX;
+	if (status & PORT_TX_FLOW_CTRL)
+		state->pause |= MLO_PAUSE_TX;
+	if (state->interface == PHY_INTERFACE_MODE_SGMII)
+		state->an_complete = state->link;
+}
+
 int ksz9477_reset_switch(struct ksz_device *dev)
 {
 	u8 data8;
@@ -345,7 +756,7 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
 	 * A fixed PHY can be setup in the device tree, but this function is
 	 * still called for that port during initialization.
 	 * For RGMII PHY there is no way to access it so the fixed PHY should
-	 * be used.  For SGMII PHY the supporting code will be added later.
+	 * be used.  SGMII PHY is simulated as a regular PHY.
 	 */
 	if (!dev->info->internal_phy[addr]) {
 		struct ksz_port *p = &dev->ports[addr];
@@ -355,7 +766,10 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
 			val = 0x1140;
 			break;
 		case MII_BMSR:
-			val = 0x796d;
+			if (p->phydev.link)
+				val = 0x796d;
+			else
+				val = 0x7949;
 			break;
 		case MII_PHYSID1:
 			val = 0x0022;
@@ -368,6 +782,10 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
 			break;
 		case MII_LPA:
 			val = 0xc5e1;
+			if (p->phydev.speed == SPEED_10)
+				val &= ~0x0180;
+			if (p->phydev.duplex == 0)
+				val &= ~0x0140;
 			break;
 		case MII_CTRL1000:
 			val = 0x0700;
@@ -378,6 +796,24 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
 			else
 				val = 0;
 			break;
+		case MII_ESTATUS:
+			val = 0x3000;
+			break;
+
+		/* This register holds the PHY interrupt status. */
+		case MII_TPISTATUS:
+			val = (LINK_DOWN_INT | LINK_UP_INT) << 8;
+			if (p->phydev.interrupts == PHY_INTERRUPT_ENABLED) {
+				if (p->phydev.link)
+					val |= LINK_UP_INT;
+				else
+					val |= LINK_DOWN_INT;
+			}
+			p->phydev.interrupts = 0;
+			break;
+		default:
+			val = 0;
+			break;
 		}
 	} else {
 		ret = ksz_pread16(dev, addr, 0x100 + (reg << 1), &val);
@@ -978,6 +1414,13 @@ void ksz9477_get_caps(struct ksz_device *dev, int port,
 
 	if (dev->info->gbit_capable[port])
 		config->mac_capabilities |= MAC_1000FD;
+
+	if (dev->info->sgmii_port == port + 1) {
+		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+			  config->supported_interfaces);
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  config->supported_interfaces);
+	}
 }
 
 int ksz9477_set_ageing_time(struct ksz_device *dev, unsigned int msecs)
@@ -1079,6 +1522,9 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 		     PORT_FORCE_TX_FLOW_CTRL | PORT_FORCE_RX_FLOW_CTRL,
 		     !dev->info->internal_phy[port]);
 
+	if (dev->info->sgmii_port == port + 1)
+		sgmii_initial_setup(dev, port);
+
 	if (cpu_port)
 		member = dsa_user_ports(ds);
 	else
@@ -1348,6 +1794,9 @@ int ksz9477_switch_init(struct ksz_device *dev)
 
 void ksz9477_switch_exit(struct ksz_device *dev)
 {
+	if (dev->info->sgmii_port > 0 &&
+	    delayed_work_pending(&dev->sgmii_check))
+		cancel_delayed_work_sync(&dev->sgmii_check);
 	ksz9477_reset_switch(dev);
 }
 
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index d2166b0d881e..cf9f9e4d7d41 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -2,7 +2,7 @@
 /*
  * Microchip KSZ9477 series Header file
  *
- * Copyright (C) 2017-2022 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #ifndef __KSZ9477_H
@@ -97,4 +97,11 @@ void ksz9477_acl_match_process_l2(struct ksz_device *dev, int port,
 				  u16 ethtype, u8 *src_mac, u8 *dst_mac,
 				  unsigned long cookie, u32 prio);
 
+int ksz9477_pcs_create(struct ksz_device *dev);
+int ksz9477_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+		       phy_interface_t interface,
+		       const unsigned long *advertising);
+void ksz9477_pcs_get_state(struct phylink_pcs *pcs,
+			   struct phylink_link_state *state);
+
 #endif
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index ff579920078e..db646b97a2ff 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -803,6 +803,7 @@
 #define REG_PORT_INT_STATUS		0x001B
 #define REG_PORT_INT_MASK		0x001F
 
+#define PORT_SGMII_INT_LOC		3
 #define PORT_SGMII_INT			BIT(3)
 #define PORT_PTP_INT			BIT(2)
 #define PORT_PHY_INT			BIT(1)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 89f0796894af..0101a706bdd6 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2,7 +2,7 @@
 /*
  * Microchip switch driver main logic
  *
- * Copyright (C) 2017-2024 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #include <linux/delay.h>
@@ -354,10 +354,26 @@ static void ksz9477_phylink_mac_link_up(struct phylink_config *config,
 					int speed, int duplex, bool tx_pause,
 					bool rx_pause);
 
+static struct phylink_pcs *
+ksz_phylink_mac_select_pcs(struct phylink_config *config,
+			   phy_interface_t interface)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct ksz_device *dev = dp->ds->priv;
+	struct ksz_port *p = &dev->ports[dp->index];
+
+	if (dev->info->sgmii_port == dp->index + 1 &&
+	    (interface == PHY_INTERFACE_MODE_SGMII ||
+	    interface == PHY_INTERFACE_MODE_1000BASEX))
+		return &p->pcs_priv->pcs;
+	return NULL;
+}
+
 static const struct phylink_mac_ops ksz9477_phylink_mac_ops = {
 	.mac_config	= ksz_phylink_mac_config,
 	.mac_link_down	= ksz_phylink_mac_link_down,
 	.mac_link_up	= ksz9477_phylink_mac_link_up,
+	.mac_select_pcs	= ksz_phylink_mac_select_pcs,
 };
 
 static const struct ksz_dev_ops ksz9477_dev_ops = {
@@ -395,6 +411,9 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.reset = ksz9477_reset_switch,
 	.init = ksz9477_switch_init,
 	.exit = ksz9477_switch_exit,
+	.pcs_create = ksz9477_pcs_create,
+	.pcs_config = ksz9477_pcs_config,
+	.pcs_get_state = ksz9477_pcs_get_state,
 };
 
 static const struct phylink_mac_ops lan937x_phylink_mac_ops = {
@@ -1035,8 +1054,7 @@ static const struct regmap_range ksz9477_valid_regs[] = {
 	regmap_reg_range(0x701b, 0x701b),
 	regmap_reg_range(0x701f, 0x7020),
 	regmap_reg_range(0x7030, 0x7030),
-	regmap_reg_range(0x7200, 0x7203),
-	regmap_reg_range(0x7206, 0x7207),
+	regmap_reg_range(0x7200, 0x7207),
 	regmap_reg_range(0x7300, 0x7301),
 	regmap_reg_range(0x7400, 0x7401),
 	regmap_reg_range(0x7403, 0x7403),
@@ -1552,6 +1570,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 				   true, false, false},
 		.gbit_capable	= {true, true, true, true, true, true, true},
 		.ptp_capable = true,
+		.sgmii_port = 7,
 		.wr_table = &ksz9477_register_set,
 		.rd_table = &ksz9477_register_set,
 	},
@@ -1944,6 +1963,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.internal_phy	= {true, true, true, true,
 				   true, false, false},
 		.gbit_capable	= {true, true, true, true, true, true, true},
+		.sgmii_port = 7,
 		.wr_table = &ksz9477_register_set,
 		.rd_table = &ksz9477_register_set,
 	},
@@ -2018,6 +2038,40 @@ static void ksz_phylink_get_caps(struct dsa_switch *ds, int port,
 		dev->dev_ops->get_caps(dev, port, config);
 }
 
+static int ksz_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
+			  phy_interface_t interface,
+			  const unsigned long *advertising,
+			  bool permit_pause_to_mac)
+{
+	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
+	struct ksz_device *dev = priv->dev;
+
+	if (dev->dev_ops->pcs_config)
+		return dev->dev_ops->pcs_config(pcs, neg_mode, interface,
+						advertising);
+	return 0;
+}
+
+static void ksz_pcs_get_state(struct phylink_pcs *pcs,
+			      struct phylink_link_state *state)
+{
+	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
+	struct ksz_device *dev = priv->dev;
+
+	if (dev->dev_ops->pcs_get_state)
+		dev->dev_ops->pcs_get_state(pcs, state);
+}
+
+static void ksz_pcs_an_restart(struct phylink_pcs *pcs)
+{
+}
+
+static const struct phylink_pcs_ops ksz_pcs_ops = {
+	.pcs_config = ksz_pcs_config,
+	.pcs_an_restart = ksz_pcs_an_restart,
+	.pcs_get_state = ksz_pcs_get_state,
+};
+
 void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 {
 	struct ethtool_pause_stats *pstats;
@@ -2067,7 +2121,7 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 
 	spin_unlock(&mib->stats64_lock);
 
-	if (dev->info->phy_errata_9477) {
+	if (dev->info->phy_errata_9477 && dev->info->sgmii_port != port + 1) {
 		ret = ksz9477_errata_monitor(dev, port, raw->tx_late_col);
 		if (ret)
 			dev_err(dev->dev, "Failed to monitor transmission halt\n");
@@ -2342,7 +2396,9 @@ static int ksz_phy_addr_to_port(struct ksz_device *dev, int addr)
 	struct dsa_port *dp;
 
 	dsa_switch_for_each_user_port(dp, ds) {
-		if (dev->info->internal_phy[dp->index] &&
+		/* Allow SGMII port to act as having a PHY. */
+		if ((dev->info->internal_phy[dp->index] ||
+		     dev->info->sgmii_port == dp->index + 1) &&
 		    dev->phy_addr_map[dp->index] == addr)
 			return dp->index;
 	}
@@ -2434,11 +2490,15 @@ static int ksz_parse_dt_phy_config(struct ksz_device *dev, struct mii_bus *bus,
 	int ret;
 
 	dsa_switch_for_each_user_port(dp, dev->ds) {
-		if (!dev->info->internal_phy[dp->index])
+		/* Allow SGMII port to act as having a PHY. */
+		if (!dev->info->internal_phy[dp->index] &&
+		    dev->info->sgmii_port != dp->index + 1)
 			continue;
 
 		phy_node = of_parse_phandle(dp->dn, "phy-handle", 0);
 		if (!phy_node) {
+			if (dev->info->sgmii_port == dp->index + 1)
+				continue;
 			dev_err(dev->dev, "failed to parse phy-handle for port %d.\n",
 				dp->index);
 			phys_are_valid = false;
@@ -2775,6 +2835,17 @@ static int ksz_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	if (dev->info->sgmii_port > 0) {
+		if (dev->dev_ops->pcs_create) {
+			ret = dev->dev_ops->pcs_create(dev);
+			if (ret)
+				return ret;
+			p = &dev->ports[dev->info->sgmii_port - 1];
+			if (p->pcs_priv)
+				p->pcs_priv->pcs.ops = &ksz_pcs_ops;
+		}
+	}
+
 	/* set broadcast storm protection 10% rate */
 	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
 			   BROADCAST_STORM_RATE,
@@ -3613,6 +3684,10 @@ static void ksz_phylink_mac_config(struct phylink_config *config,
 	if (dev->info->internal_phy[port])
 		return;
 
+	/* No need to configure XMII control register when using SGMII. */
+	if (dev->info->sgmii_port == port + 1)
+		return;
+
 	if (phylink_autoneg_inband(mode)) {
 		dev_err(dev->dev, "In-band AN not supported!\n");
 		return;
@@ -4595,6 +4670,9 @@ static int ksz_suspend(struct dsa_switch *ds)
 	struct ksz_device *dev = ds->priv;
 
 	cancel_delayed_work_sync(&dev->mib_read);
+	if (dev->info->sgmii_port > 0 &&
+	    delayed_work_pending(&dev->sgmii_check))
+		cancel_delayed_work_sync(&dev->sgmii_check);
 	return 0;
 }
 
@@ -4604,6 +4682,9 @@ static int ksz_resume(struct dsa_switch *ds)
 
 	if (dev->mib_read_interval)
 		schedule_delayed_work(&dev->mib_read, dev->mib_read_interval);
+	if (dev->info->sgmii_port > 0)
+		schedule_delayed_work(&dev->sgmii_check,
+				      msecs_to_jiffies(100));
 	return 0;
 }
 
@@ -4755,6 +4836,22 @@ static void ksz_parse_rgmii_delay(struct ksz_device *dev, int port_num,
 	dev->ports[port_num].rgmii_tx_val = tx_delay;
 }
 
+static void ksz_parse_sgmii(struct ksz_device *dev, int port,
+			    struct device_node *dn)
+{
+	const char *managed;
+
+	if (dev->info->sgmii_port != port + 1)
+		return;
+	/* SGMII port can be used without using SFP.
+	 * The sfp declaration is returned as being a fixed link so need to
+	 * check the managed string to know the port is not using sfp.
+	 */
+	if (of_phy_is_fixed_link(dn) &&
+	    of_property_read_string(dn, "managed", &managed))
+		dev->ports[port].interface = PHY_INTERFACE_MODE_INTERNAL;
+}
+
 /**
  * ksz_drive_strength_to_reg() - Convert drive strength value to corresponding
  *				 register value.
@@ -5021,6 +5118,7 @@ int ksz_switch_register(struct ksz_device *dev)
 	mutex_init(&dev->regmap_mutex);
 	mutex_init(&dev->alu_mutex);
 	mutex_init(&dev->vlan_mutex);
+	mutex_init(&dev->sgmii_mutex);
 
 	ret = ksz_switch_detect(dev);
 	if (ret)
@@ -5097,6 +5195,7 @@ int ksz_switch_register(struct ksz_device *dev)
 						&dev->ports[port_num].interface);
 
 				ksz_parse_rgmii_delay(dev, port_num, port);
+				ksz_parse_sgmii(dev, port_num, port);
 			}
 			of_node_put(ports);
 		}
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index af17a9c030d4..962bba382556 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Microchip switch driver common header
  *
- * Copyright (C) 2017-2024 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #ifndef __KSZ_COMMON_H
@@ -93,6 +93,7 @@ struct ksz_chip_data {
 	bool internal_phy[KSZ_MAX_NUM_PORTS];
 	bool gbit_capable[KSZ_MAX_NUM_PORTS];
 	bool ptp_capable;
+	u8 sgmii_port;
 	const struct regmap_access_table *wr_table;
 	const struct regmap_access_table *rd_table;
 };
@@ -121,6 +122,15 @@ struct ksz_switch_macaddr {
 	refcount_t refcount;
 };
 
+struct ksz_pcs {
+	struct phylink_pcs pcs;
+	struct ksz_device *dev;
+	u8 port;
+	u32 has_intr:1;
+	u32 link:8;
+	u32 using_sfp:1;
+};
+
 struct ksz_port {
 	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
 	bool learning;
@@ -141,6 +151,7 @@ struct ksz_port {
 	void *acl_priv;
 	struct ksz_irq pirq;
 	u8 num;
+	struct ksz_pcs *pcs_priv;
 #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
 	struct hwtstamp_config tstamp_config;
 	bool hwts_tx_en;
@@ -162,6 +173,8 @@ struct ksz_device {
 	struct mutex regmap_mutex;	/* regmap access */
 	struct mutex alu_mutex;		/* ALU access */
 	struct mutex vlan_mutex;	/* vlan access */
+	struct mutex sgmii_mutex;	/* SGMII access */
+	const struct phylink_pcs_ops *pcs_ops;
 	const struct ksz_dev_ops *dev_ops;
 
 	struct device *dev;
@@ -188,6 +201,7 @@ struct ksz_device {
 	struct ksz_port *ports;
 	struct delayed_work mib_read;
 	unsigned long mib_read_interval;
+	struct delayed_work sgmii_check;
 	u16 mirror_rx;
 	u16 mirror_tx;
 	u16 port_mask;
@@ -440,6 +454,13 @@ struct ksz_dev_ops {
 	int (*reset)(struct ksz_device *dev);
 	int (*init)(struct ksz_device *dev);
 	void (*exit)(struct ksz_device *dev);
+
+	int (*pcs_create)(struct ksz_device *dev);
+	int (*pcs_config)(struct phylink_pcs *pcs, unsigned int neg_mode,
+			  phy_interface_t interface,
+			  const unsigned long *advertising);
+	void (*pcs_get_state)(struct phylink_pcs *pcs,
+			      struct phylink_link_state *state);
 };
 
 struct ksz_device *ksz_switch_alloc(struct device *base, void *priv);
-- 
2.34.1

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Vladimir Oltean]

On Mon, Jan 13, 2025 at 06:47:04PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The SGMII module of KSZ9477 switch can be setup in 3 ways: direct connect
> without using any SFP, SGMII mode with 10/100/1000Base-T SFP, and SerDes
> mode with 1000BaseX SFP, which can be fiber or copper.  Note some
> 1000Base-T copper SFPs advertise themselves as SGMII but start in
> 1000BaseX mode, and the PHY driver of the PHY inside will change it to
> SGMII mode.
> 
> The SGMII module can only support basic link status of the SFP, so the
> port can be simulated as having a regular internal PHY when SFP cage
> logic is not present.  The original KSZ9477 evaluation board operates in
> this way and so requires the simulation code.

I don't follow what you are saing here. What is the basic link status of
the SFP? It is expected of a SGMII PCS to be able to report:
- the "link up" indication
- the "autoneg complete" indication
- for SGMII: the speed and duplex communicated by the PHY, if in-band
  mode is selected and enabled
- for 1000Base-X: the duplex and pause bits communicated by the link
  partner, if in-band mode is selected and enabled.

What, out of these, is missing? I'm mostly confused about the reference
to the SFP here. The SGMII PCS shouldn't care whether the link goes
through an SFP module or not. It just reports the above things. Higher
layer code (the SFP bus driver) determines if the SFP module wants to
use SGMII or 1000Base-X, based on its EEPROM contents.

Essentially I don't understand the justification for simulating an
internal phylib phy. Why would the SFP cage logic (I assume you mean
CONFIG_SFP) be missing? If you have a phylink-based driver, you have to
have that enabled if you have SFP cages on your board.

> A PCS driver for the SGMII port is provided to support the SFP cage
> logic used in the phylink code.  It is used to confirm the link is up
> and process the SGMII interrupt.
> 
> One issue for the 1000BaseX SFP is there is no link down interrupt, so
> the driver has to use polling to detect link off when the link is up.
> 
> Note the SGMII interrupt cannot be masked in hardware.  Also the module
> is not reset when the switch is reset.  It is important to reset the
> module properly to make sure interrupt is not triggered prematurely.
> 
> One side effect is the SGMII interrupt is triggered when an internal PHY
> is powered down and powered up.  This happens when a port using internal
> PHY is turned off and then turned on.
> 
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> ---
> v2
>  - use standard MDIO names when programming MMD registers
>  - use pcs_config API to setup SGMII module
>  - remove the KSZ9477 device tree example as it was deemed unnecessary
> 
>  drivers/net/dsa/microchip/ksz9477.c     | 455 +++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz9477.h     |   9 +-
>  drivers/net/dsa/microchip/ksz9477_reg.h |   1 +
>  drivers/net/dsa/microchip/ksz_common.c  | 111 +++++-
>  drivers/net/dsa/microchip/ksz_common.h  |  23 +-
>  5 files changed, 588 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 29fe79ea74cd..3613eea1e3fb 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip KSZ9477 switch driver main logic
>   *
> - * Copyright (C) 2017-2024 Microchip Technology Inc.
> + * Copyright (C) 2017-2025 Microchip Technology Inc.
>   */
>  
>  #include <linux/kernel.h>
> @@ -12,6 +12,8 @@
>  #include <linux/phy.h>
>  #include <linux/if_bridge.h>
>  #include <linux/if_vlan.h>
> +#include <linux/irqdomain.h>
> +#include <linux/phylink.h>
>  #include <net/dsa.h>
>  #include <net/switchdev.h>
>  
> @@ -161,6 +163,415 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
>  					10, 1000);
>  }
>  
> +static void port_sgmii_s(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 len)
> +{
> +	u32 data;
> +
> +	data = (devid & MII_MMD_CTRL_DEVAD_MASK) << 16;
> +	data |= reg;
> +	if (len > 1)
> +		data |= PORT_SGMII_AUTO_INCR;
> +	ksz_pwrite32(dev, port, REG_PORT_SGMII_ADDR__4, data);
> +}
> +
> +static void port_sgmii_r(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 *buf, u16 len)
> +{
> +	u32 data;
> +
> +	mutex_lock(&dev->sgmii_mutex);
> +	port_sgmii_s(dev, port, devid, reg, len);
> +	while (len) {
> +		ksz_pread32(dev, port, REG_PORT_SGMII_DATA__4, &data);
> +		*buf++ = (u16)data;
> +		len--;
> +	}
> +	mutex_unlock(&dev->sgmii_mutex);
> +}
> +
> +static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> +			 u16 *buf, u16 len)
> +{
> +	u32 data;
> +
> +	mutex_lock(&dev->sgmii_mutex);
> +	port_sgmii_s(dev, port, devid, reg, len);
> +	while (len) {
> +		data = *buf++;
> +		ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, data);
> +		len--;
> +	}
> +	mutex_unlock(&dev->sgmii_mutex);
> +}
> +
> +static void port_sgmii_reset(struct ksz_device *dev, uint p)
> +{
> +	u16 ctrl = BMCR_RESET;
> +
> +	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +}
> +
> +static phy_interface_t port_sgmii_detect(struct ksz_device *dev, uint p)
> +{
> +	phy_interface_t interface = PHY_INTERFACE_MODE_1000BASEX;
> +	u16 buf[6];
> +	int i = 0;
> +
> +	/* Read all 6 registers to spend more time waiting for valid result. */
> +	do {
> +		port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMCR, buf, 6);
> +		i++;
> +	} while (!buf[5] && i < 10);
> +	if ((buf[5] & LPA_LPACK) &&
> +	    (!(buf[5] & (LPA_1000XHALF | LPA_1000XFULL))))
> +		interface = PHY_INTERFACE_MODE_SGMII;
> +	return interface;
> +}
> +
> +static void port_sgmii_setup(struct ksz_device *dev, uint p, bool pcs,
> +			     bool master, bool autoneg, bool intr, int speed,
> +			     int duplex)
> +{
> +	u16 ctrl;
> +	u16 cfg;
> +	u16 adv;
> +
> +	cfg = 0;
> +	if (pcs)
> +		cfg |= SR_MII_PCS_SGMII << SR_MII_PCS_MODE_S;
> +	if (master) {
> +		cfg |= SR_MII_TX_CFG_PHY_MASTER;
> +		cfg |= SR_MII_SGMII_LINK_UP;
> +	}
> +	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_CTRL, &cfg, 1);
> +
> +	/* Need to write to advertise register to send correct signal. */
> +	/* Default value is 0x0020. */
> +	adv = ADVERTISE_1000XPSE_ASYM | ADVERTISE_1000XPAUSE;
> +	adv |= (duplex == DUPLEX_FULL) ?
> +	       ADVERTISE_1000XFULL : ADVERTISE_1000XHALF;
> +	port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_ADVERTISE, &adv, 1);
> +	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +	if (master || !autoneg) {
> +		ctrl &= ~(BMCR_SPEED1000 | BMCR_SPEED100 | BMCR_FULLDPLX);
> +		switch (speed) {
> +		case SPEED_100:
> +			ctrl |= BMCR_SPEED100;
> +			break;
> +		case SPEED_1000:
> +			ctrl |= BMCR_SPEED1000;
> +			break;
> +		}
> +		if (duplex == DUPLEX_FULL)
> +			ctrl |= BMCR_FULLDPLX;
> +	}
> +	if (!autoneg) {
> +		ctrl &= ~BMCR_ANENABLE;
> +		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +		goto sgmii_setup_last;
> +	} else if (!(ctrl & BMCR_ANENABLE)) {
> +		ctrl |= BMCR_ANENABLE;
> +		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +	}
> +	if (master && autoneg) {
> +		ctrl |= BMCR_ANRESTART;
> +		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +	}
> +
> +sgmii_setup_last:
> +	if (intr) {
> +		cfg |= SR_MII_AUTO_NEG_COMPLETE_INTR;
> +		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_CTRL,
> +			     &cfg, 1);
> +	}
> +}
> +
> +#define PORT_LINK_UP		BIT(0)
> +#define PORT_LINK_CHANGE	BIT(1)
> +#define PORT_LINK_INVALID	BIT(2)
> +
> +static int sgmii_port_get_speed(struct ksz_device *dev, uint p)
> +{
> +	struct ksz_port *info = &dev->ports[p];
> +	struct ksz_pcs *priv = info->pcs_priv;
> +	int ret = 0;
> +	u16 status;
> +	u16 speed;
> +	u16 data;
> +	u8 link;
> +
> +	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
> +	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
> +	port_sgmii_r(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_STATUS, &data,
> +		     1);
> +
> +	/* Typical register values with different SFPs.
> +	 * 10/100/1000: 1f0001 = 01ad  1f0005 = 4000  1f8002 = 0008
> +	 *              1f0001 = 01bd  1f0005 = d000  1f8002 = 001a
> +	 * 1000:        1f0001 = 018d  1f0005 = 0000  1f8002 = 0000
> +	 *              1f0001 = 01ad  1f0005 = 40a0  1f8002 = 0001
> +	 *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001
> +	 * fiber:       1f0001 = 0189  1f0005 = 0000  1f8002 = 0000
> +	 *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001

Hmm, these registers look extremely similar to the DW XPCS.
1f8002 => DW_VR_MII_AN_INTR_STS. Why don't you implement a virtual MDIO
bus for accessing the XPCS registers at MDIO_MMD_VEND2 (0x1f_0000 +
register address) and let drivers/net/pcs/pcs-xpcs.c handle the logic?

It will be better for everybody to understand what is the special
handling that your hardware integration needs, when it is relative to
the common driver.

You can look at sja1105 and its xpcs handling for an example of just this.

> +	 */
> +
> +	if (data & SR_MII_AUTO_NEG_COMPLETE_INTR) {
> +		data &= ~SR_MII_AUTO_NEG_COMPLETE_INTR;
> +		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_STATUS,
> +			     &data, 1);
> +	}
> +
> +	/* Not running in SGMII mode where data indicates link status. */
> +	if (info->interface != PHY_INTERFACE_MODE_SGMII && !data) {
> +		u16 link_up = BMSR_LSTATUS;
> +
> +		if (info->interface == PHY_INTERFACE_MODE_1000BASEX)
> +			link_up |= BMSR_ANEGCOMPLETE;
> +		if ((status & link_up) == link_up)
> +			data = SR_MII_STAT_LINK_UP |
> +			       (SR_MII_STAT_1000_MBPS << SR_MII_STAT_S) |
> +			       SR_MII_STAT_FULL_DUPLEX;
> +	}
> +	if (data & SR_MII_STAT_LINK_UP)
> +		ret = PORT_LINK_UP;
> +
> +	link = (data & ~SR_MII_AUTO_NEG_COMPLETE_INTR);
> +	if (priv->link == link)
> +		return ret;
> +
> +	if (data & SR_MII_STAT_LINK_UP) {
> +		u16 ctrl = 0;
> +
> +		/* Need to update control register with same link setting. */
> +		if (info->interface != PHY_INTERFACE_MODE_INTERNAL)
> +			ctrl = BMCR_ANENABLE;
> +		speed = (data >> SR_MII_STAT_S) & SR_MII_STAT_M;
> +		if (speed == SR_MII_STAT_1000_MBPS)
> +			ctrl |= BMCR_SPEED1000;
> +		else if (speed == SR_MII_STAT_100_MBPS)
> +			ctrl |= BMCR_SPEED100;
> +		if (data & SR_MII_STAT_FULL_DUPLEX)
> +			ctrl |= BMCR_FULLDPLX;
> +		port_sgmii_w(dev, p, MDIO_MMD_VEND2, MII_BMCR, &ctrl, 1);
> +
> +		info->phydev.speed = SPEED_10;
> +		if (speed == SR_MII_STAT_1000_MBPS)
> +			info->phydev.speed = SPEED_1000;
> +		else if (speed == SR_MII_STAT_100_MBPS)
> +			info->phydev.speed = SPEED_100;
> +
> +		info->phydev.duplex = 0;
> +		if (data & SR_MII_STAT_FULL_DUPLEX)
> +			info->phydev.duplex = 1;
> +	}
> +	ret |= PORT_LINK_CHANGE;
> +	priv->link = link;
> +	info->phydev.link = (ret & PORT_LINK_UP);
> +	return ret;
> +}
> +
> +static bool sgmii_need_polling(struct ksz_device *dev, struct ksz_port *p)
> +{
> +	struct ksz_pcs *priv = p->pcs_priv;
> +
> +	/* SGMII mode has link up and link down interrupts. */
> +	if (p->interface == PHY_INTERFACE_MODE_SGMII && priv->has_intr)
> +		return false;
> +
> +	/* SerDes mode has link up interrupt but not link down interrupt. */
> +	if (p->interface == PHY_INTERFACE_MODE_1000BASEX && priv->has_intr &&
> +	    !p->phydev.link)
> +		return false;
> +
> +	/* Direct connect mode has no link change. */
> +	if (p->interface == PHY_INTERFACE_MODE_INTERNAL)
> +		return false;
> +	return true;
> +}
> +
> +static void ksz9477_sgmii_setup(struct ksz_device *dev, int port,
> +				phy_interface_t intf)
> +{
> +	struct ksz_port *p = &dev->ports[port];
> +	struct ksz_pcs *priv = p->pcs_priv;
> +	struct phy_device *phydev = NULL;
> +	bool autoneg, master, pcs;
> +
> +	if (!priv->using_sfp && dev->ds->user_mii_bus)
> +		phydev = mdiobus_get_phy(dev->ds->user_mii_bus, port);
> +	if (phydev || p->interface == PHY_INTERFACE_MODE_INTERNAL)
> +		intf = p->interface;
> +
> +	/* PHY driver can change the mode to PHY_INTERFACE_MODE_SGMII from
> +	 * PHY_INTERFACE_MODE_1000BASEX, so this function can be called again
> +	 * after the interface is changed.
> +	 */
> +	if (intf != p->interface) {
> +		dev_info(dev->dev, "switching to %s after %s was detected.\n",
> +			 phy_modes(intf), phy_modes(p->interface));
> +		p->interface = intf;
> +	}
> +	switch (p->interface) {
> +	case PHY_INTERFACE_MODE_SGMII:
> +		autoneg = true;
> +		master = false;
> +		pcs = true;
> +		break;
> +	case PHY_INTERFACE_MODE_1000BASEX:
> +		autoneg = true;
> +		master = true;
> +		pcs = false;
> +		break;
> +	default:
> +		autoneg = false;
> +		master = true;
> +		pcs = true;
> +		break;
> +	}
> +	port_sgmii_setup(dev, port, pcs, master, autoneg, priv->has_intr,
> +			 SPEED_1000, DUPLEX_FULL);
> +
> +	sgmii_port_get_speed(dev, port);
> +
> +	/* Need to check link down if using 1000BASEX SFP. */
> +	if (sgmii_need_polling(dev, p))
> +		schedule_delayed_work(&dev->sgmii_check,
> +				      msecs_to_jiffies(500));
> +}
> +
> +static void sgmii_update_link(struct ksz_device *dev)
> +{
> +	u8 port = dev->info->sgmii_port - 1;
> +	struct ksz_port *p = &dev->ports[port];
> +	int ret;
> +
> +	ret = sgmii_port_get_speed(dev, port);
> +	if (ret & PORT_LINK_CHANGE) {
> +		struct phy_device *phydev;
> +
> +		/* When simulating PHY. */
> +		p->phydev.interrupts = PHY_INTERRUPT_ENABLED;
> +		phydev = mdiobus_get_phy(dev->ds->user_mii_bus, port);
> +		if (phydev)
> +			phy_trigger_machine(phydev);
> +
> +		/* When using SFP code. */
> +		dsa_port_phylink_mac_change(dev->ds, port, p->phydev.link);
> +	}
> +
> +	/* No interrupt for link down. */
> +	if (sgmii_need_polling(dev, p))
> +		schedule_delayed_work(&dev->sgmii_check,
> +				      msecs_to_jiffies(500));
> +}
> +
> +static void sgmii_check_work(struct work_struct *work)
> +{
> +	struct ksz_device *dev = container_of(work, struct ksz_device,
> +					      sgmii_check.work);
> +
> +	sgmii_update_link(dev);
> +}
> +
> +static irqreturn_t ksz9477_sgmii_irq_thread_fn(int irq, void *dev_id)
> +{
> +	struct ksz_pcs *priv = dev_id;
> +	struct ksz_device *dev = priv->dev;
> +	u8 port = priv->port;
> +	u16 data16 = 0;
> +
> +	port_sgmii_w(dev, port, SR_MII, MMD_SR_MII_AUTO_NEG_STATUS, &data16, 1);
> +	sgmii_update_link(dev);
> +	return IRQ_HANDLED;
> +}
> +
> +static void sgmii_initial_setup(struct ksz_device *dev, int port)
> +{
> +	struct ksz_port *p = &dev->ports[port];
> +	struct ksz_pcs *priv = p->pcs_priv;
> +	int irq, ret;
> +
> +	irq = irq_find_mapping(p->pirq.domain, PORT_SGMII_INT_LOC);
> +	if (irq > 0) {
> +		ret = request_threaded_irq(irq, NULL,
> +					   ksz9477_sgmii_irq_thread_fn,
> +					   IRQF_ONESHOT, "SGMII", priv);
> +		if (!ret)
> +			priv->has_intr = 1;
> +	}
> +
> +	/* Make invalid so the correct value is set. */
> +	priv->link = 0xff;
> +
> +	INIT_DELAYED_WORK(&dev->sgmii_check, sgmii_check_work);
> +}
> +
> +int ksz9477_pcs_create(struct ksz_device *dev)
> +{
> +	/* This chip has a SGMII port. */
> +	if (dev->info->sgmii_port > 0) {
> +		int port = dev->info->sgmii_port - 1;
> +		struct ksz_port *p = &dev->ports[port];
> +		struct ksz_pcs *pcs_priv;
> +
> +		pcs_priv = devm_kzalloc(dev->dev, sizeof(struct ksz_pcs),
> +					GFP_KERNEL);
> +		if (!pcs_priv)
> +			return -ENOMEM;
> +		p->pcs_priv = pcs_priv;
> +		pcs_priv->dev = dev;
> +		pcs_priv->port = port;
> +		pcs_priv->pcs.neg_mode = true;
> +
> +		/* Switch reset does not reset SGMII module. */
> +		port_sgmii_reset(dev, port);
> +
> +		/* Detect which mode to use if not using direct connect. */
> +		if (p->interface != PHY_INTERFACE_MODE_INTERNAL)
> +			p->interface = port_sgmii_detect(dev, port);
> +	}
> +	return 0;
> +}
> +
> +int ksz9477_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +		       phy_interface_t interface,
> +		       const unsigned long *advertising)
> +{
> +	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
> +	struct ksz_device *dev = priv->dev;
> +	struct ksz_port *p = &dev->ports[priv->port];
> +
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +		p->pcs_priv->using_sfp = 1;

When neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED, it does not mean that
we are using an SFP. We can have an on-board SGMII PHY which requires
PHYLINK_PCS_NEG_INBAND_ENABLED as well.

Generally speaking, this implementation seems to depend on aspects which
it really shouldn't be concern about.

> +	ksz9477_sgmii_setup(dev, priv->port, interface);
> +	return 0;
> +}
> +
> +void ksz9477_pcs_get_state(struct phylink_pcs *pcs,
> +			   struct phylink_link_state *state)
> +{
> +	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
> +	struct ksz_device *dev = priv->dev;
> +	struct ksz_port *p = &dev->ports[priv->port];
> +	u8 status;
> +	int ret;
> +
> +	ksz_pread8(dev, priv->port, REG_PORT_STATUS_0, &status);
> +	ret = sgmii_port_get_speed(dev, priv->port);
> +	if (!(ret & PORT_LINK_UP))
> +		state->link = false;
> +	state->duplex = p->phydev.duplex;
> +	state->speed = p->phydev.speed;
> +	state->pause &= ~(MLO_PAUSE_RX | MLO_PAUSE_TX);
> +	if (status & PORT_RX_FLOW_CTRL)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (status & PORT_TX_FLOW_CTRL)
> +		state->pause |= MLO_PAUSE_TX;
> +	if (state->interface == PHY_INTERFACE_MODE_SGMII)
> +		state->an_complete = state->link;
> +}
> +
>  int ksz9477_reset_switch(struct ksz_device *dev)
>  {
>  	u8 data8;
> @@ -345,7 +756,7 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
>  	 * A fixed PHY can be setup in the device tree, but this function is
>  	 * still called for that port during initialization.
>  	 * For RGMII PHY there is no way to access it so the fixed PHY should
> -	 * be used.  For SGMII PHY the supporting code will be added later.
> +	 * be used.  SGMII PHY is simulated as a regular PHY.
>  	 */
>  	if (!dev->info->internal_phy[addr]) {
>  		struct ksz_port *p = &dev->ports[addr];
> @@ -355,7 +766,10 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
>  			val = 0x1140;
>  			break;
>  		case MII_BMSR:
> -			val = 0x796d;
> +			if (p->phydev.link)
> +				val = 0x796d;
> +			else
> +				val = 0x7949;
>  			break;
>  		case MII_PHYSID1:
>  			val = 0x0022;
> @@ -368,6 +782,10 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
>  			break;
>  		case MII_LPA:
>  			val = 0xc5e1;
> +			if (p->phydev.speed == SPEED_10)
> +				val &= ~0x0180;
> +			if (p->phydev.duplex == 0)
> +				val &= ~0x0140;
>  			break;
>  		case MII_CTRL1000:
>  			val = 0x0700;
> @@ -378,6 +796,24 @@ int ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
>  			else
>  				val = 0;
>  			break;
> +		case MII_ESTATUS:
> +			val = 0x3000;
> +			break;
> +
> +		/* This register holds the PHY interrupt status. */
> +		case MII_TPISTATUS:
> +			val = (LINK_DOWN_INT | LINK_UP_INT) << 8;
> +			if (p->phydev.interrupts == PHY_INTERRUPT_ENABLED) {
> +				if (p->phydev.link)
> +					val |= LINK_UP_INT;
> +				else
> +					val |= LINK_DOWN_INT;
> +			}
> +			p->phydev.interrupts = 0;
> +			break;
> +		default:
> +			val = 0;
> +			break;
>  		}
>  	} else {
>  		ret = ksz_pread16(dev, addr, 0x100 + (reg << 1), &val);
> @@ -978,6 +1414,13 @@ void ksz9477_get_caps(struct ksz_device *dev, int port,
>  
>  	if (dev->info->gbit_capable[port])
>  		config->mac_capabilities |= MAC_1000FD;
> +
> +	if (dev->info->sgmii_port == port + 1) {

Can you use a different representation for "doesn't have an SGMII port"
than "dev->info->sgmii_port == 0"? You can hide it behind a wrapper like
ksz_port_is_sgmii(). It is confusing and error-prone to have comparisons
against port + 1 everywhere, as well as to set 7 in the info structure
when in reality its index is 6.

> +		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +			  config->supported_interfaces);
> +		__set_bit(PHY_INTERFACE_MODE_SGMII,
> +			  config->supported_interfaces);
> +	}
>  }
>  
>  int ksz9477_set_ageing_time(struct ksz_device *dev, unsigned int msecs)
> @@ -1079,6 +1522,9 @@ void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
>  		     PORT_FORCE_TX_FLOW_CTRL | PORT_FORCE_RX_FLOW_CTRL,
>  		     !dev->info->internal_phy[port]);
>  
> +	if (dev->info->sgmii_port == port + 1)
> +		sgmii_initial_setup(dev, port);
> +
>  	if (cpu_port)
>  		member = dsa_user_ports(ds);
>  	else
> @@ -1348,6 +1794,9 @@ int ksz9477_switch_init(struct ksz_device *dev)
>  
>  void ksz9477_switch_exit(struct ksz_device *dev)
>  {
> +	if (dev->info->sgmii_port > 0 &&
> +	    delayed_work_pending(&dev->sgmii_check))
> +		cancel_delayed_work_sync(&dev->sgmii_check);
>  	ksz9477_reset_switch(dev);
>  }
>  
> diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
> index d2166b0d881e..cf9f9e4d7d41 100644
> --- a/drivers/net/dsa/microchip/ksz9477.h
> +++ b/drivers/net/dsa/microchip/ksz9477.h
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip KSZ9477 series Header file
>   *
> - * Copyright (C) 2017-2022 Microchip Technology Inc.
> + * Copyright (C) 2017-2025 Microchip Technology Inc.
>   */
>  
>  #ifndef __KSZ9477_H
> @@ -97,4 +97,11 @@ void ksz9477_acl_match_process_l2(struct ksz_device *dev, int port,
>  				  u16 ethtype, u8 *src_mac, u8 *dst_mac,
>  				  unsigned long cookie, u32 prio);
>  
> +int ksz9477_pcs_create(struct ksz_device *dev);
> +int ksz9477_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +		       phy_interface_t interface,
> +		       const unsigned long *advertising);
> +void ksz9477_pcs_get_state(struct phylink_pcs *pcs,
> +			   struct phylink_link_state *state);
> +
>  #endif
> diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
> index ff579920078e..db646b97a2ff 100644
> --- a/drivers/net/dsa/microchip/ksz9477_reg.h
> +++ b/drivers/net/dsa/microchip/ksz9477_reg.h
> @@ -803,6 +803,7 @@
>  #define REG_PORT_INT_STATUS		0x001B
>  #define REG_PORT_INT_MASK		0x001F
>  
> +#define PORT_SGMII_INT_LOC		3
>  #define PORT_SGMII_INT			BIT(3)
>  #define PORT_PTP_INT			BIT(2)
>  #define PORT_PHY_INT			BIT(1)
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 89f0796894af..0101a706bdd6 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip switch driver main logic
>   *
> - * Copyright (C) 2017-2024 Microchip Technology Inc.
> + * Copyright (C) 2017-2025 Microchip Technology Inc.
>   */
>  
>  #include <linux/delay.h>
> @@ -354,10 +354,26 @@ static void ksz9477_phylink_mac_link_up(struct phylink_config *config,
>  					int speed, int duplex, bool tx_pause,
>  					bool rx_pause);
>  
> +static struct phylink_pcs *
> +ksz_phylink_mac_select_pcs(struct phylink_config *config,
> +			   phy_interface_t interface)
> +{
> +	struct dsa_port *dp = dsa_phylink_to_port(config);
> +	struct ksz_device *dev = dp->ds->priv;
> +	struct ksz_port *p = &dev->ports[dp->index];
> +
> +	if (dev->info->sgmii_port == dp->index + 1 &&
> +	    (interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_1000BASEX))
> +		return &p->pcs_priv->pcs;
> +	return NULL;
> +}
> +
>  static const struct phylink_mac_ops ksz9477_phylink_mac_ops = {
>  	.mac_config	= ksz_phylink_mac_config,
>  	.mac_link_down	= ksz_phylink_mac_link_down,
>  	.mac_link_up	= ksz9477_phylink_mac_link_up,
> +	.mac_select_pcs	= ksz_phylink_mac_select_pcs,
>  };
>  
>  static const struct ksz_dev_ops ksz9477_dev_ops = {
> @@ -395,6 +411,9 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
>  	.reset = ksz9477_reset_switch,
>  	.init = ksz9477_switch_init,
>  	.exit = ksz9477_switch_exit,
> +	.pcs_create = ksz9477_pcs_create,
> +	.pcs_config = ksz9477_pcs_config,
> +	.pcs_get_state = ksz9477_pcs_get_state,
>  };
>  
>  static const struct phylink_mac_ops lan937x_phylink_mac_ops = {
> @@ -1035,8 +1054,7 @@ static const struct regmap_range ksz9477_valid_regs[] = {
>  	regmap_reg_range(0x701b, 0x701b),
>  	regmap_reg_range(0x701f, 0x7020),
>  	regmap_reg_range(0x7030, 0x7030),
> -	regmap_reg_range(0x7200, 0x7203),
> -	regmap_reg_range(0x7206, 0x7207),
> +	regmap_reg_range(0x7200, 0x7207),
>  	regmap_reg_range(0x7300, 0x7301),
>  	regmap_reg_range(0x7400, 0x7401),
>  	regmap_reg_range(0x7403, 0x7403),
> @@ -1552,6 +1570,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  				   true, false, false},
>  		.gbit_capable	= {true, true, true, true, true, true, true},
>  		.ptp_capable = true,
> +		.sgmii_port = 7,
>  		.wr_table = &ksz9477_register_set,
>  		.rd_table = &ksz9477_register_set,
>  	},
> @@ -1944,6 +1963,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.internal_phy	= {true, true, true, true,
>  				   true, false, false},
>  		.gbit_capable	= {true, true, true, true, true, true, true},
> +		.sgmii_port = 7,
>  		.wr_table = &ksz9477_register_set,
>  		.rd_table = &ksz9477_register_set,
>  	},
> @@ -2018,6 +2038,40 @@ static void ksz_phylink_get_caps(struct dsa_switch *ds, int port,
>  		dev->dev_ops->get_caps(dev, port, config);
>  }
>  
> +static int ksz_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			  phy_interface_t interface,
> +			  const unsigned long *advertising,
> +			  bool permit_pause_to_mac)
> +{
> +	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
> +	struct ksz_device *dev = priv->dev;
> +
> +	if (dev->dev_ops->pcs_config)
> +		return dev->dev_ops->pcs_config(pcs, neg_mode, interface,
> +						advertising);
> +	return 0;
> +}
> +
> +static void ksz_pcs_get_state(struct phylink_pcs *pcs,
> +			      struct phylink_link_state *state)
> +{
> +	struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
> +	struct ksz_device *dev = priv->dev;
> +
> +	if (dev->dev_ops->pcs_get_state)
> +		dev->dev_ops->pcs_get_state(pcs, state);
> +}
> +
> +static void ksz_pcs_an_restart(struct phylink_pcs *pcs)
> +{
> +}
> +
> +static const struct phylink_pcs_ops ksz_pcs_ops = {
> +	.pcs_config = ksz_pcs_config,
> +	.pcs_an_restart = ksz_pcs_an_restart,
> +	.pcs_get_state = ksz_pcs_get_state,
> +};
> +
>  void ksz_r_mib_stats64(struct ksz_device *dev, int port)
>  {
>  	struct ethtool_pause_stats *pstats;
> @@ -2067,7 +2121,7 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
>  
>  	spin_unlock(&mib->stats64_lock);
>  
> -	if (dev->info->phy_errata_9477) {
> +	if (dev->info->phy_errata_9477 && dev->info->sgmii_port != port + 1) {
>  		ret = ksz9477_errata_monitor(dev, port, raw->tx_late_col);
>  		if (ret)
>  			dev_err(dev->dev, "Failed to monitor transmission halt\n");
> @@ -2342,7 +2396,9 @@ static int ksz_phy_addr_to_port(struct ksz_device *dev, int addr)
>  	struct dsa_port *dp;
>  
>  	dsa_switch_for_each_user_port(dp, ds) {
> -		if (dev->info->internal_phy[dp->index] &&
> +		/* Allow SGMII port to act as having a PHY. */
> +		if ((dev->info->internal_phy[dp->index] ||
> +		     dev->info->sgmii_port == dp->index + 1) &&
>  		    dev->phy_addr_map[dp->index] == addr)
>  			return dp->index;
>  	}
> @@ -2434,11 +2490,15 @@ static int ksz_parse_dt_phy_config(struct ksz_device *dev, struct mii_bus *bus,
>  	int ret;
>  
>  	dsa_switch_for_each_user_port(dp, dev->ds) {
> -		if (!dev->info->internal_phy[dp->index])
> +		/* Allow SGMII port to act as having a PHY. */
> +		if (!dev->info->internal_phy[dp->index] &&
> +		    dev->info->sgmii_port != dp->index + 1)
>  			continue;
>  
>  		phy_node = of_parse_phandle(dp->dn, "phy-handle", 0);
>  		if (!phy_node) {
> +			if (dev->info->sgmii_port == dp->index + 1)
> +				continue;
>  			dev_err(dev->dev, "failed to parse phy-handle for port %d.\n",
>  				dp->index);
>  			phys_are_valid = false;
> @@ -2775,6 +2835,17 @@ static int ksz_setup(struct dsa_switch *ds)
>  	if (ret)
>  		return ret;
>  
> +	if (dev->info->sgmii_port > 0) {
> +		if (dev->dev_ops->pcs_create) {
> +			ret = dev->dev_ops->pcs_create(dev);
> +			if (ret)
> +				return ret;
> +			p = &dev->ports[dev->info->sgmii_port - 1];
> +			if (p->pcs_priv)
> +				p->pcs_priv->pcs.ops = &ksz_pcs_ops;
> +		}
> +	}
> +
>  	/* set broadcast storm protection 10% rate */
>  	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
>  			   BROADCAST_STORM_RATE,
> @@ -3613,6 +3684,10 @@ static void ksz_phylink_mac_config(struct phylink_config *config,
>  	if (dev->info->internal_phy[port])
>  		return;
>  
> +	/* No need to configure XMII control register when using SGMII. */
> +	if (dev->info->sgmii_port == port + 1)
> +		return;
> +
>  	if (phylink_autoneg_inband(mode)) {
>  		dev_err(dev->dev, "In-band AN not supported!\n");
>  		return;
> @@ -4595,6 +4670,9 @@ static int ksz_suspend(struct dsa_switch *ds)
>  	struct ksz_device *dev = ds->priv;
>  
>  	cancel_delayed_work_sync(&dev->mib_read);
> +	if (dev->info->sgmii_port > 0 &&
> +	    delayed_work_pending(&dev->sgmii_check))
> +		cancel_delayed_work_sync(&dev->sgmii_check);
>  	return 0;
>  }
>  
> @@ -4604,6 +4682,9 @@ static int ksz_resume(struct dsa_switch *ds)
>  
>  	if (dev->mib_read_interval)
>  		schedule_delayed_work(&dev->mib_read, dev->mib_read_interval);
> +	if (dev->info->sgmii_port > 0)
> +		schedule_delayed_work(&dev->sgmii_check,
> +				      msecs_to_jiffies(100));
>  	return 0;
>  }
>  
> @@ -4755,6 +4836,22 @@ static void ksz_parse_rgmii_delay(struct ksz_device *dev, int port_num,
>  	dev->ports[port_num].rgmii_tx_val = tx_delay;
>  }
>  
> +static void ksz_parse_sgmii(struct ksz_device *dev, int port,
> +			    struct device_node *dn)
> +{
> +	const char *managed;
> +
> +	if (dev->info->sgmii_port != port + 1)
> +		return;
> +	/* SGMII port can be used without using SFP.
> +	 * The sfp declaration is returned as being a fixed link so need to
> +	 * check the managed string to know the port is not using sfp.
> +	 */
> +	if (of_phy_is_fixed_link(dn) &&
> +	    of_property_read_string(dn, "managed", &managed))
> +		dev->ports[port].interface = PHY_INTERFACE_MODE_INTERNAL;
> +}

There is way too much that seems unjustifiably complex at this stage,
including this. I would like to see a v3 using xpcs + the remaining
required delta for ksz9477, ideally with no internal PHY simulation.
Then we can go from there.

Also please make sure to keep linux@armlinux.org.uk in cc for future
submissions of this feature.

> +
>  /**
>   * ksz_drive_strength_to_reg() - Convert drive strength value to corresponding
>   *				 register value.
> @@ -5021,6 +5118,7 @@ int ksz_switch_register(struct ksz_device *dev)
>  	mutex_init(&dev->regmap_mutex);
>  	mutex_init(&dev->alu_mutex);
>  	mutex_init(&dev->vlan_mutex);
> +	mutex_init(&dev->sgmii_mutex);
>  
>  	ret = ksz_switch_detect(dev);
>  	if (ret)
> @@ -5097,6 +5195,7 @@ int ksz_switch_register(struct ksz_device *dev)
>  						&dev->ports[port_num].interface);
>  
>  				ksz_parse_rgmii_delay(dev, port_num, port);
> +				ksz_parse_sgmii(dev, port_num, port);
>  			}
>  			of_node_put(ports);
>  		}
> diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
> index af17a9c030d4..962bba382556 100644
> --- a/drivers/net/dsa/microchip/ksz_common.h
> +++ b/drivers/net/dsa/microchip/ksz_common.h
> @@ -1,7 +1,7 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  /* Microchip switch driver common header
>   *
> - * Copyright (C) 2017-2024 Microchip Technology Inc.
> + * Copyright (C) 2017-2025 Microchip Technology Inc.
>   */
>  
>  #ifndef __KSZ_COMMON_H
> @@ -93,6 +93,7 @@ struct ksz_chip_data {
>  	bool internal_phy[KSZ_MAX_NUM_PORTS];
>  	bool gbit_capable[KSZ_MAX_NUM_PORTS];
>  	bool ptp_capable;
> +	u8 sgmii_port;
>  	const struct regmap_access_table *wr_table;
>  	const struct regmap_access_table *rd_table;
>  };
> @@ -121,6 +122,15 @@ struct ksz_switch_macaddr {
>  	refcount_t refcount;
>  };
>  
> +struct ksz_pcs {
> +	struct phylink_pcs pcs;
> +	struct ksz_device *dev;
> +	u8 port;
> +	u32 has_intr:1;
> +	u32 link:8;
> +	u32 using_sfp:1;
> +};
> +
>  struct ksz_port {
>  	bool remove_tag;		/* Remove Tag flag set, for ksz8795 only */
>  	bool learning;
> @@ -141,6 +151,7 @@ struct ksz_port {
>  	void *acl_priv;
>  	struct ksz_irq pirq;
>  	u8 num;
> +	struct ksz_pcs *pcs_priv;
>  #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
>  	struct hwtstamp_config tstamp_config;
>  	bool hwts_tx_en;
> @@ -162,6 +173,8 @@ struct ksz_device {
>  	struct mutex regmap_mutex;	/* regmap access */
>  	struct mutex alu_mutex;		/* ALU access */
>  	struct mutex vlan_mutex;	/* vlan access */
> +	struct mutex sgmii_mutex;	/* SGMII access */
> +	const struct phylink_pcs_ops *pcs_ops;
>  	const struct ksz_dev_ops *dev_ops;
>  
>  	struct device *dev;
> @@ -188,6 +201,7 @@ struct ksz_device {
>  	struct ksz_port *ports;
>  	struct delayed_work mib_read;
>  	unsigned long mib_read_interval;
> +	struct delayed_work sgmii_check;
>  	u16 mirror_rx;
>  	u16 mirror_tx;
>  	u16 port_mask;
> @@ -440,6 +454,13 @@ struct ksz_dev_ops {
>  	int (*reset)(struct ksz_device *dev);
>  	int (*init)(struct ksz_device *dev);
>  	void (*exit)(struct ksz_device *dev);
> +
> +	int (*pcs_create)(struct ksz_device *dev);
> +	int (*pcs_config)(struct phylink_pcs *pcs, unsigned int neg_mode,
> +			  phy_interface_t interface,
> +			  const unsigned long *advertising);
> +	void (*pcs_get_state)(struct phylink_pcs *pcs,
> +			      struct phylink_link_state *state);
>  };
>  
>  struct ksz_device *ksz_switch_alloc(struct device *base, void *priv);
> -- 
> 2.34.1
> 

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Vladimir Oltean]

On Tue, Jan 14, 2025 at 06:09:08PM +0200, Vladimir Oltean wrote:
> There is way too much that seems unjustifiably complex at this stage,
> including this. I would like to see a v3 using xpcs + the remaining
> required delta for ksz9477, ideally with no internal PHY simulation.
> Then we can go from there.

You might stumble upon this issue on net-next, which you've helped me
remember I should upstream, so I posted 2 patches just now:
https://lore.kernel.org/netdev/20250114164721.2879380-1-vladimir.oltean@nxp.com/

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Maxime Chevallier]

Hello Vlad, Tristram,

I'm replying to Vlad's review as he correctly points that this looks
very much like XPCS :)

On Tue, 14 Jan 2025 18:09:08 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Mon, Jan 13, 2025 at 06:47:04PM -0800, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <tristram.ha@microchip.com>
> > 
> > The SGMII module of KSZ9477 switch can be setup in 3 ways: direct connect
> > without using any SFP, SGMII mode with 10/100/1000Base-T SFP, and SerDes
> > mode with 1000BaseX SFP, which can be fiber or copper.  Note some
> > 1000Base-T copper SFPs advertise themselves as SGMII but start in
> > 1000BaseX mode, and the PHY driver of the PHY inside will change it to
> > SGMII mode.
> > 
> > The SGMII module can only support basic link status of the SFP, so the
> > port can be simulated as having a regular internal PHY when SFP cage
> > logic is not present.  The original KSZ9477 evaluation board operates in
> > this way and so requires the simulation code.  
> 
> I don't follow what you are saing here. What is the basic link status of
> the SFP? It is expected of a SGMII PCS to be able to report:
> - the "link up" indication
> - the "autoneg complete" indication
> - for SGMII: the speed and duplex communicated by the PHY, if in-band
>   mode is selected and enabled
> - for 1000Base-X: the duplex and pause bits communicated by the link
>   partner, if in-band mode is selected and enabled.
> 
> What, out of these, is missing? I'm mostly confused about the reference
> to the SFP here. The SGMII PCS shouldn't care whether the link goes
> through an SFP module or not. It just reports the above things. Higher
> layer code (the SFP bus driver) determines if the SFP module wants to
> use SGMII or 1000Base-X, based on its EEPROM contents.
> 
> Essentially I don't understand the justification for simulating an
> internal phylib phy. Why would the SFP cage logic (I assume you mean
> CONFIG_SFP) be missing? If you have a phylink-based driver, you have to
> have that enabled if you have SFP cages on your board.
> 
> > A PCS driver for the SGMII port is provided to support the SFP cage
> > logic used in the phylink code.  It is used to confirm the link is up
> > and process the SGMII interrupt.
> > 
> > One issue for the 1000BaseX SFP is there is no link down interrupt, so
> > the driver has to use polling to detect link off when the link is up.
> > 
> > Note the SGMII interrupt cannot be masked in hardware.  Also the module
> > is not reset when the switch is reset.  It is important to reset the
> > module properly to make sure interrupt is not triggered prematurely.
> > 
> > One side effect is the SGMII interrupt is triggered when an internal PHY
> > is powered down and powered up.  This happens when a port using internal
> > PHY is turned off and then turned on.
> > 
> > Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> > ---
> > v2
> >  - use standard MDIO names when programming MMD registers
> >  - use pcs_config API to setup SGMII module
> >  - remove the KSZ9477 device tree example as it was deemed unnecessary
> > 
> >  drivers/net/dsa/microchip/ksz9477.c     | 455 +++++++++++++++++++++++-
> >  drivers/net/dsa/microchip/ksz9477.h     |   9 +-
> >  drivers/net/dsa/microchip/ksz9477_reg.h |   1 +
> >  drivers/net/dsa/microchip/ksz_common.c  | 111 +++++-
> >  drivers/net/dsa/microchip/ksz_common.h  |  23 +-
> >  5 files changed, 588 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> > index 29fe79ea74cd..3613eea1e3fb 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -2,7 +2,7 @@
> >  /*
> >   * Microchip KSZ9477 switch driver main logic
> >   *
> > - * Copyright (C) 2017-2024 Microchip Technology Inc.
> > + * Copyright (C) 2017-2025 Microchip Technology Inc.
> >   */
> >  
> >  #include <linux/kernel.h>
> > @@ -12,6 +12,8 @@
> >  #include <linux/phy.h>
> >  #include <linux/if_bridge.h>
> >  #include <linux/if_vlan.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/phylink.h>
> >  #include <net/dsa.h>
> >  #include <net/switchdev.h>
> >  
> > @@ -161,6 +163,415 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
> >  					10, 1000);
> >  }
> >  
> > +static void port_sgmii_s(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> > +			 u16 len)
> > +{
> > +	u32 data;
> > +
> > +	data = (devid & MII_MMD_CTRL_DEVAD_MASK) << 16;
> > +	data |= reg;
> > +	if (len > 1)
> > +		data |= PORT_SGMII_AUTO_INCR;
> > +	ksz_pwrite32(dev, port, REG_PORT_SGMII_ADDR__4, data);
> > +}
> > +
> > +static void port_sgmii_r(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> > +			 u16 *buf, u16 len)
> > +{
> > +	u32 data;
> > +
> > +	mutex_lock(&dev->sgmii_mutex);
> > +	port_sgmii_s(dev, port, devid, reg, len);
> > +	while (len) {
> > +		ksz_pread32(dev, port, REG_PORT_SGMII_DATA__4, &data);
> > +		*buf++ = (u16)data;
> > +		len--;
> > +	}
> > +	mutex_unlock(&dev->sgmii_mutex);
> > +}
> > +
> > +static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
> > +			 u16 *buf, u16 len)
> > +{
> > +	u32 data;
> > +
> > +	mutex_lock(&dev->sgmii_mutex);
> > +	port_sgmii_s(dev, port, devid, reg, len);
> > +	while (len) {
> > +		data = *buf++;
> > +		ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, data);
> > +		len--;
> > +	}
> > +	mutex_unlock(&dev->sgmii_mutex);
> > +}

These helpers can be converted into mii_bus read_c45/write_c45 mdio
accessors, which would be the first step towards using xpcs here.

[...]
  
> > +static void ksz_parse_sgmii(struct ksz_device *dev, int port,
> > +			    struct device_node *dn)
> > +{
> > +	const char *managed;
> > +
> > +	if (dev->info->sgmii_port != port + 1)
> > +		return;
> > +	/* SGMII port can be used without using SFP.
> > +	 * The sfp declaration is returned as being a fixed link so need to
> > +	 * check the managed string to know the port is not using sfp.
> > +	 */
> > +	if (of_phy_is_fixed_link(dn) &&
> > +	    of_property_read_string(dn, "managed", &managed))
> > +		dev->ports[port].interface = PHY_INTERFACE_MODE_INTERNAL;
> > +}  
> 
> There is way too much that seems unjustifiably complex at this stage,
> including this. I would like to see a v3 using xpcs + the remaining
> required delta for ksz9477, ideally with no internal PHY simulation.
> Then we can go from there.
> 
> Also please make sure to keep linux@armlinux.org.uk in cc for future
> submissions of this feature.

I mentionned on the previous iteration that there's indeed a DW XPCS in
there :
https://lore.kernel.org/netdev/20241129135919.57d59c90@fedora.home/

I have access to a platform with a KSZ9477, and indeed the PHY id
register for the PCS mdio device show the DW XPCS id.

I've been able to get this serdes port working with the XPCS driver
(although on 6.1 due to project constraints), although I couldn't get
1000BaseX autoneg to work.

So all in all I agree with Vlad's comments here, there's a lot of logic
in this series to detect the phy_interface_mode, detect SFP or not,
most of which isn't needed.

The logic should boil down to :

 - Create some helpers to access the PCS through a virtual mdio bus
(basically the current port_sgmii_w/r)

 - Register a virtual mdio bus to access the PCS, hooked in
ksz9477_port_setup() for the serdes port. That would look something
like this :

+       bus = devm_mdiobus_alloc(ds->dev);
(...)
+       bus->read_c45 = ksz9477_sgmii_read;
+       bus->write_c45 = ksz9477_sgmii_write;
(...)
+       ret = devm_mdiobus_register(ds->dev, bus);
+       if (ret)
+               (...)
+
+       port->xpcs = xpcs_create_mdiodev(bus, 0, <iface>);

- Make sure that .phylink_select_pcs() returns a ref to that xpcs

- Write the necessary ksz9477-specific glue logic (adjust the phylink capabilities,
make sure the virual MDIO registers are un the regmap area, etc.)

I will be happy to test any further iterations :)

Thanks,

Maxime

RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> On Mon, Jan 13, 2025 at 06:47:04PM -0800, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <tristram.ha@microchip.com>
> >
> > The SGMII module of KSZ9477 switch can be setup in 3 ways: direct connect
> > without using any SFP, SGMII mode with 10/100/1000Base-T SFP, and SerDes
> > mode with 1000BaseX SFP, which can be fiber or copper.  Note some
> > 1000Base-T copper SFPs advertise themselves as SGMII but start in
> > 1000BaseX mode, and the PHY driver of the PHY inside will change it to
> > SGMII mode.
> >
> > The SGMII module can only support basic link status of the SFP, so the
> > port can be simulated as having a regular internal PHY when SFP cage
> > logic is not present.  The original KSZ9477 evaluation board operates in
> > this way and so requires the simulation code.
> 
> I don't follow what you are saing here. What is the basic link status of
> the SFP? It is expected of a SGMII PCS to be able to report:
> - the "link up" indication
> - the "autoneg complete" indication
> - for SGMII: the speed and duplex communicated by the PHY, if in-band
>   mode is selected and enabled
> - for 1000Base-X: the duplex and pause bits communicated by the link
>   partner, if in-band mode is selected and enabled.
> 
> What, out of these, is missing? I'm mostly confused about the reference
> to the SFP here. The SGMII PCS shouldn't care whether the link goes
> through an SFP module or not. It just reports the above things. Higher
> layer code (the SFP bus driver) determines if the SFP module wants to
> use SGMII or 1000Base-X, based on its EEPROM contents.
> 
> Essentially I don't understand the justification for simulating an
> internal phylib phy. Why would the SFP cage logic (I assume you mean
> CONFIG_SFP) be missing? If you have a phylink-based driver, you have to
> have that enabled if you have SFP cages on your board.

I explained that the KSZ9477 board that is used to verify DSA driver does
not have SFP cage hardware logic so that the EEPROM can be read through
I2C.  The SGMII hardware implementation is used on other boards that do
not use Linux so it is not always required to have that logic.

I do not know whether customers followed that example and setup the
hardware that way.

Anyway the PHY simulation code will be removed and the code will be kept
for internal use only if it is not desirable.

> > +     port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
> > +     port_sgmii_r(dev, p, MDIO_MMD_VEND2, MII_BMSR, &status, 1);
> > +     port_sgmii_r(dev, p, MDIO_MMD_VEND2, MMD_SR_MII_AUTO_NEG_STATUS,
> &data,
> > +                  1);
> > +
> > +     /* Typical register values with different SFPs.
> > +      * 10/100/1000: 1f0001 = 01ad  1f0005 = 4000  1f8002 = 0008
> > +      *              1f0001 = 01bd  1f0005 = d000  1f8002 = 001a
> > +      * 1000:        1f0001 = 018d  1f0005 = 0000  1f8002 = 0000
> > +      *              1f0001 = 01ad  1f0005 = 40a0  1f8002 = 0001
> > +      *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001
> > +      * fiber:       1f0001 = 0189  1f0005 = 0000  1f8002 = 0000
> > +      *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0001
> 
> Hmm, these registers look extremely similar to the DW XPCS.
> 1f8002 => DW_VR_MII_AN_INTR_STS. Why don't you implement a virtual MDIO
> bus for accessing the XPCS registers at MDIO_MMD_VEND2 (0x1f_0000 +
> register address) and let drivers/net/pcs/pcs-xpcs.c handle the logic?
> 
> It will be better for everybody to understand what is the special
> handling that your hardware integration needs, when it is relative to
> the common driver.
> 
> You can look at sja1105 and its xpcs handling for an example of just this.

The XPCS driver available in the kernel does not work in KSZ9477 as the
SGMII hardware implementation is probably too old and so is not
compatible.  Link detection works but the SGMII port does not pass
traffic for some SFPs.  That is the reason that XPCS driver is not used.

> > +int ksz9477_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> > +                    phy_interface_t interface,
> > +                    const unsigned long *advertising)
> > +{
> > +     struct ksz_pcs *priv = container_of(pcs, struct ksz_pcs, pcs);
> > +     struct ksz_device *dev = priv->dev;
> > +     struct ksz_port *p = &dev->ports[priv->port];
> > +
> > +     if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> > +             p->pcs_priv->using_sfp = 1;
> 
> When neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED, it does not mean that
> we are using an SFP. We can have an on-board SGMII PHY which requires
> PHYLINK_PCS_NEG_INBAND_ENABLED as well.
> 
> Generally speaking, this implementation seems to depend on aspects which
> it really shouldn't be concern about.

It is just to confirm whether SFP cage code is used in the phylink
driver or not as the device tree can specify using managed sfp.

> > +
> > +     if (dev->info->sgmii_port == port + 1) {
> 
> Can you use a different representation for "doesn't have an SGMII port"
> than "dev->info->sgmii_port == 0"? You can hide it behind a wrapper like
> ksz_port_is_sgmii(). It is confusing and error-prone to have comparisons
> against port + 1 everywhere, as well as to set 7 in the info structure
> when in reality its index is 6.

Will update that.
The SGMII port is specified in the device info block because it can be a
different port in a different chip like LAN9374.
 
> > +static void ksz_parse_sgmii(struct ksz_device *dev, int port,
> > +                         struct device_node *dn)
> > +{
> > +     const char *managed;
> > +
> > +     if (dev->info->sgmii_port != port + 1)
> > +             return;
> > +     /* SGMII port can be used without using SFP.
> > +      * The sfp declaration is returned as being a fixed link so need to
> > +      * check the managed string to know the port is not using sfp.
> > +      */
> > +     if (of_phy_is_fixed_link(dn) &&
> > +         of_property_read_string(dn, "managed", &managed))
> > +             dev->ports[port].interface = PHY_INTERFACE_MODE_INTERNAL;
> > +}
> 
> There is way too much that seems unjustifiably complex at this stage,
> including this. I would like to see a v3 using xpcs + the remaining
> required delta for ksz9477, ideally with no internal PHY simulation.
> Then we can go from there.

This is used to accommodate direct mode as the SGMII port can be
connected directly with no SFP in between and so will never lose link.

> Also please make sure to keep linux@armlinux.org.uk in cc for future
> submissions of this feature.

Noted.

RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> Hello Vlad, Tristram,
> 
> I'm replying to Vlad's review as he correctly points that this looks
> very much like XPCS :)

> I mentionned on the previous iteration that there's indeed a DW XPCS in
> there :
> https://lore.kernel.org/netdev/20241129135919.57d59c90@fedora.home/
> 
> I have access to a platform with a KSZ9477, and indeed the PHY id
> register for the PCS mdio device show the DW XPCS id.
> 
> I've been able to get this serdes port working with the XPCS driver
> (although on 6.1 due to project constraints), although I couldn't get
> 1000BaseX autoneg to work.
> 
> So all in all I agree with Vlad's comments here, there's a lot of logic
> in this series to detect the phy_interface_mode, detect SFP or not,
> most of which isn't needed.
> 
> The logic should boil down to :
> 
>  - Create some helpers to access the PCS through a virtual mdio bus
> (basically the current port_sgmii_w/r)
> 
>  - Register a virtual mdio bus to access the PCS, hooked in
> ksz9477_port_setup() for the serdes port. That would look something
> like this :
> 
> +       bus = devm_mdiobus_alloc(ds->dev);
> (...)
> +       bus->read_c45 = ksz9477_sgmii_read;
> +       bus->write_c45 = ksz9477_sgmii_write;
> (...)
> +       ret = devm_mdiobus_register(ds->dev, bus);
> +       if (ret)
> +               (...)
> +
> +       port->xpcs = xpcs_create_mdiodev(bus, 0, <iface>);
> 
> - Make sure that .phylink_select_pcs() returns a ref to that xpcs
> 
> - Write the necessary ksz9477-specific glue logic (adjust the phylink capabilities,
> make sure the virual MDIO registers are un the regmap area, etc.)
> 
> I will be happy to test any further iterations :)

The KSZ9477 SGMII module does use DesignWare IP, but its implementation
is probably too old as some registers do not match.  When using XPCS
driver link detection works but the SGMII port does not pass traffic for
some SFPs.  It is probably doable to update the XPCS driver to work in
KSZ9477, but there is no way to submit that patch as that may affect
other hardware implementation.

One thing that is strange is that driver enables interrupt for 1000BaseX
mode but not SGMII mode, but in KSZ9477 SGMII mode can trigger link up
and link down interrupt but 1000BaseX can only trigger link up interrupt.

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Andrew Lunn]

> The KSZ9477 SGMII module does use DesignWare IP, but its implementation
> is probably too old as some registers do not match.

Is there a revision value somewhere in the registers? Maybe the lower
nibble of ID registers 2 and 3?

> When using XPCS
> driver link detection works but the SGMII port does not pass traffic for
> some SFPs.  It is probably doable to update the XPCS driver to work in
> KSZ9477, but there is no way to submit that patch as that may affect
> other hardware implementation.

We have PHY drivers which change their behaviour based on the
revision. So it is possible. And XPCS is used quite a bit, so i don't
think it will be an issue finding somebody to do some regression
testing.

Using a PCS driver is the correct way to go here. So either you need
to copy/paste/edit the XPCS driver to create a version specific for
you hardware, or you need to extend the XPCS driver so it supports
your hardware.

	Andrew

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Vladimir Oltean]

On Fri, Jan 17, 2025 at 12:56:14AM +0000, Tristram.Ha@microchip.com wrote:
> The KSZ9477 SGMII module does use DesignWare IP, but its implementation
> is probably too old as some registers do not match.  When using XPCS
> driver link detection works but the SGMII port does not pass traffic for
> some SFPs.  It is probably doable to update the XPCS driver to work in
> KSZ9477, but there is no way to submit that patch as that may affect
> other hardware implementation.
> 
> One thing that is strange is that driver enables interrupt for 1000BaseX
> mode but not SGMII mode, but in KSZ9477 SGMII mode can trigger link up
> and link down interrupt but 1000BaseX can only trigger link up interrupt.

Sometimes, the "collaborative" aspect of open source projects does work
out, and you might get help, feedback and/or regression testing for
hardware you don't have. Sure, it doesn't always work out, but I suggest
you give it a try and not put the cart before the horses.

RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> > The KSZ9477 SGMII module does use DesignWare IP, but its implementation
> > is probably too old as some registers do not match.
> 
> Is there a revision value somewhere in the registers? Maybe the lower
> nibble of ID registers 2 and 3?
> 

I am not aware of getting the version of the DesignWare IP to
differentiate different implementations.  I will find out from hardware
designers.

> > When using XPCS
> > driver link detection works but the SGMII port does not pass traffic for
> > some SFPs.  It is probably doable to update the XPCS driver to work in
> > KSZ9477, but there is no way to submit that patch as that may affect
> > other hardware implementation.
> 
> We have PHY drivers which change their behaviour based on the
> revision. So it is possible. And XPCS is used quite a bit, so i don't
> think it will be an issue finding somebody to do some regression
> testing.
> 
> Using a PCS driver is the correct way to go here. So either you need
> to copy/paste/edit the XPCS driver to create a version specific for
> you hardware, or you need to extend the XPCS driver so it supports
> your hardware.

Some of the register definitions are not present in the XPCS driver so I
need to add them.

Some register bits programmed by the XPCS driver do not have effect.

Actually KSZ9477 has a bug in SGMII implementation and needs a software
workaround.  I am not sure if the generic XCPS driver can cover that.

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Vladimir Oltean]

On Sat, Jan 18, 2025 at 12:59:25AM +0000, Tristram.Ha@microchip.com wrote:
> Some of the register definitions are not present in the XPCS driver so I
> need to add them.

Not a problem.

> Some register bits programmed by the XPCS driver do not have effect.

Like what?

> Actually KSZ9477 has a bug in SGMII implementation and needs a software
> workaround.  I am not sure if the generic XCPS driver can cover that.

What kind of bug? In the integration or in the IP itself? Anyway,
SJA1105 had what you could call an integration bug too, and that's why
nxp_sja1105_sgmii_pma_config() exists. I am not sure either until you
are more specific.

It is a widespread hardware IP. I don't think it's unreasonable to have
a central driver for it with many quirks, as long as they're well documented
and clearly understood.

RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> On Sat, Jan 18, 2025 at 12:59:25AM +0000, Tristram.Ha@microchip.com wrote:
> > Some of the register definitions are not present in the XPCS driver so I
> > need to add them.
> 
> Not a problem.
> 
> > Some register bits programmed by the XPCS driver do not have effect.
> 
> Like what?

Bit 9 (MAC_AUTO_SW) of 0x1f8000 is written in SGMII mode, but that bit
has no effect on KSZ9477, so it probably does not matter.

KSZ9477 does not need to update the 0x1f8000 register.

0x1f8001 needs to be written 0x18 in 1000BaseX mode, but the XPCS driver
does not do anything, and bit 4 is not defined in the driver.

The driver enables interrupt in 1000BaseX mode when poll is not set, but
it does not do that in SGMII Mode.  In KSZ9477 SGMII mode can trigger
both link up and link down interrupt, but 1000BaseX mode can only trigger
link up interrupt.  It requires polling to detect link down.

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Andrew Lunn]

> 0x1f8001 needs to be written 0x18 in 1000BaseX mode, but the XPCS driver
> does not do anything, and bit 4 is not defined in the driver.

Does the Synopsis data book describe this bit, for the version of the
IP you have? Is it described in later version?

> The driver enables interrupt in 1000BaseX mode when poll is not set, but
> it does not do that in SGMII Mode.  In KSZ9477 SGMII mode can trigger
> both link up and link down interrupt, but 1000BaseX mode can only trigger
> link up interrupt.  It requires polling to detect link down.

I don't know this driver, but a quick look suggest TXGBE requires
polling. It should be easy to piggy back on that and have KSZ9477
always poll. Just because the hardware can do interrupts does not mean
you actually need to use it. My experience is you are more interested
in fast link down notification, so you can trigger routing protocols
to find an alternative route. You are less interested in fast link up.

	Andrew

RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> > 0x1f8001 needs to be written 0x18 in 1000BaseX mode, but the XPCS driver
> > does not do anything, and bit 4 is not defined in the driver.
> 
> Does the Synopsis data book describe this bit, for the version of the
> IP you have? Is it described in later version?

It is definitely defined in Synopsys SGMII IP document Microchip used.
It is named SGMII_LINK_STS where 1 means link up and 0 means link down.
It is used in combination with TX_CONFIG, bit 3.

It appears in latest SGMII IP document where 2.5G is supported.  To use
2.5G some other registers need to be changed to set the clock.  This
procedure does not seem to appear in XPCS driver for the Synopsys IP.  I
think a similar pma_config like NXP can be added, so I wonder if the 2.5G
support is complete.

The KSZ9477 SGMII module has a bug that the MII_BMCR register needs to be
updated with the correct speed when the link speed is changed in SGMII
mode.  The XPCS driver seems to skip that in the link_up API.

The device id for Sysnopsys is 0x7996ced0.  It does not have any revision
so it cannot be used for differentiation.

I can send a patch with RFC label for somebody to verify the code, but I
do not really know how to update the XPCS driver to not break other
DesignWare implementations if the new code is not compatible.

RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> > > 0x1f8001 needs to be written 0x18 in 1000BaseX mode, but the XPCS driver
> > > does not do anything, and bit 4 is not defined in the driver.
> >
> > Does the Synopsis data book describe this bit, for the version of the
> > IP you have? Is it described in later version?
> 
> It is definitely defined in Synopsys SGMII IP document Microchip used.
> It is named SGMII_LINK_STS where 1 means link up and 0 means link down.
> It is used in combination with TX_CONFIG, bit 3.
> 
> It appears in latest SGMII IP document where 2.5G is supported.  To use
> 2.5G some other registers need to be changed to set the clock.  This
> procedure does not seem to appear in XPCS driver for the Synopsys IP.  I
> think a similar pma_config like NXP can be added, so I wonder if the 2.5G
> support is complete.
> 
> The KSZ9477 SGMII module has a bug that the MII_BMCR register needs to be
> updated with the correct speed when the link speed is changed in SGMII
> mode.  The XPCS driver seems to skip that in the link_up API.
> 
> The device id for Sysnopsys is 0x7996ced0.  It does not have any revision
> so it cannot be used for differentiation.
> 
> I can send a patch with RFC label for somebody to verify the code, but I
> do not really know how to update the XPCS driver to not break other
> DesignWare implementations if the new code is not compatible.

Additional comments:

If neg_mode is set to false in the XPCS driver then it works for
1000BaseX mode as auto-negotiation is disabled, but then it does not work
in SGMII mode as auto-negotiation is required.  When 0x18 is written
enabling auto-negotiation still works for 1000BaseX mode.

As MII_BMCR needs to be updated for different speeds in SGMII mode the
check for neg_mode needs to be changed, and auto-negotiation needs to be
enabled.  This bug workaround needs to be done somehow with other means.

The initial check for link returns invalid link up as it just compares
the interrupt bit value.  This is incorrect in SGMII mode.  It can be
fixed with additional check for 1000BaseX mode.

[PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

From: Tristram Ha <tristram.ha@microchip.com>

The KSZ9477 switch driver uses the XPCS driver to operate its SGMII
port.  However there are some hardware bugs in the KSZ9477 SGMII
module so workarounds are needed.  There was a proposal to update the
XPCS driver to accommodate KSZ9477, but the new code is not generic
enough to be used by other vendors.  It is better to do all these
workarounds inside the KSZ9477 driver instead of modifying the XPCS
driver.

There are 3 hardware issues.  The first is the MII_ADVERTISE register
needs to be write once after reset for the correct code word to be
sent.  The XPCS driver disables auto-negotiation first before
configuring the SGMII/1000BASE-X mode and then enables it back.  The
KSZ9477 driver then writes the MII_ADVERTISE register before enabling
auto-negotiation.  In 1000BASE-X mode the MII_ADVERTISE register will
be set, so KSZ9477 driver does not need to write it.

The second issue is the MII_BMCR register needs to set the exact speed
and duplex mode when running in SGMII mode.  During link polling the
KSZ9477 will check the speed and duplex mode are different from
previous ones and update the MII_BMCR register accordingly.

The last issue is 1000BASE-X mode does not work with auto-negotiation
on.  The cause is the local port hardware does not know the link is up
and so network traffic is not forwarded.  The workaround is to write 2
additional bits when 1000BASE-X mode is configured.

Note the SGMII interrupt in the port cannot be masked.  As that
interrupt is not handled in the KSZ9477 driver the SGMII interrupt bit
will not be set even when the XPCS driver sets it.

Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
v2
 - add Kconfig for required XPCS driver build

 drivers/net/dsa/microchip/Kconfig      |   1 +
 drivers/net/dsa/microchip/ksz9477.c    | 191 ++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz9477.h    |   4 +-
 drivers/net/dsa/microchip/ksz_common.c |  36 ++++-
 drivers/net/dsa/microchip/ksz_common.h |  23 ++-
 5 files changed, 248 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/microchip/Kconfig b/drivers/net/dsa/microchip/Kconfig
index 12a86585a77f..c71d3fd5dfeb 100644
--- a/drivers/net/dsa/microchip/Kconfig
+++ b/drivers/net/dsa/microchip/Kconfig
@@ -6,6 +6,7 @@ menuconfig NET_DSA_MICROCHIP_KSZ_COMMON
 	select NET_DSA_TAG_NONE
 	select NET_IEEE8021Q_HELPERS
 	select DCB
+	select PCS_XPCS
 	help
 	  This driver adds support for Microchip KSZ8, KSZ9 and
 	  LAN937X series switch chips, being KSZ8863/8873,
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 29fe79ea74cd..825aa570eed9 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -2,7 +2,7 @@
 /*
  * Microchip KSZ9477 switch driver main logic
  *
- * Copyright (C) 2017-2024 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #include <linux/kernel.h>
@@ -161,6 +161,187 @@ static int ksz9477_wait_alu_sta_ready(struct ksz_device *dev)
 					10, 1000);
 }
 
+static void port_sgmii_s(struct ksz_device *dev, uint port, u16 devid, u16 reg)
+{
+	u32 data;
+
+	data = (devid & MII_MMD_CTRL_DEVAD_MASK) << 16;
+	data |= reg;
+	ksz_pwrite32(dev, port, REG_PORT_SGMII_ADDR__4, data);
+}
+
+static void port_sgmii_r(struct ksz_device *dev, uint port, u16 devid, u16 reg,
+			 u16 *buf)
+{
+	port_sgmii_s(dev, port, devid, reg);
+	ksz_pread16(dev, port, REG_PORT_SGMII_DATA__4 + 2, buf);
+}
+
+static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
+			 u16 buf)
+{
+	port_sgmii_s(dev, port, devid, reg);
+	ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, buf);
+}
+
+static int ksz9477_pcs_read(struct mii_bus *bus, int phy, int mmd, int reg)
+{
+	struct ksz_device *dev = bus->priv;
+	int port = ksz_get_sgmii_port(dev);
+	u16 val;
+
+	port_sgmii_r(dev, port, mmd, reg, &val);
+
+	/* Simulate a value to activate special code in the XPCS driver if
+	 * supported.
+	 */
+	if (mmd == MDIO_MMD_PMAPMD) {
+		if (reg == MDIO_DEVID1)
+			val = 0x9477;
+		else if (reg == MDIO_DEVID2)
+			val = 0x22 << 10;
+	} else if (mmd == MDIO_MMD_VEND2) {
+		struct ksz_port *p = &dev->ports[port];
+
+		/* Need to update MII_BMCR register with the exact speed and
+		 * duplex mode when running in SGMII mode and this register is
+		 * used to detect connected speed in that mode.
+		 */
+		if (reg == MMD_SR_MII_AUTO_NEG_STATUS) {
+			int duplex, speed;
+
+			if (val & SR_MII_STAT_LINK_UP) {
+				speed = (val >> SR_MII_STAT_S) & SR_MII_STAT_M;
+				if (speed == SR_MII_STAT_1000_MBPS)
+					speed = SPEED_1000;
+				else if (speed == SR_MII_STAT_100_MBPS)
+					speed = SPEED_100;
+				else
+					speed = SPEED_10;
+
+				if (val & SR_MII_STAT_FULL_DUPLEX)
+					duplex = DUPLEX_FULL;
+				else
+					duplex = DUPLEX_HALF;
+
+				if (!p->phydev.link ||
+				    p->phydev.speed != speed ||
+				    p->phydev.duplex != duplex) {
+					u16 ctrl;
+
+					p->phydev.link = 1;
+					p->phydev.speed = speed;
+					p->phydev.duplex = duplex;
+					port_sgmii_r(dev, port, mmd, MII_BMCR,
+						     &ctrl);
+					ctrl &= BMCR_ANENABLE;
+					ctrl |= mii_bmcr_encode_fixed(speed,
+								      duplex);
+					port_sgmii_w(dev, port, mmd, MII_BMCR,
+						     ctrl);
+				}
+			} else {
+				p->phydev.link = 0;
+			}
+		} else if (reg == MII_BMSR) {
+			p->phydev.link = (val & BMSR_LSTATUS);
+		}
+	}
+	return val;
+}
+
+static int ksz9477_pcs_write(struct mii_bus *bus, int phy, int mmd, int reg,
+			     u16 val)
+{
+	struct ksz_device *dev = bus->priv;
+	int port = ksz_get_sgmii_port(dev);
+
+	if (mmd == MDIO_MMD_VEND2) {
+		struct ksz_port *p = &dev->ports[port];
+
+		if (reg == MMD_SR_MII_AUTO_NEG_CTRL) {
+			u16 sgmii_mode = SR_MII_PCS_SGMII << SR_MII_PCS_MODE_S;
+
+			/* Need these bits for 1000BASE-X mode to work with
+			 * AN on.
+			 */
+			if (!(val & sgmii_mode))
+				val |= SR_MII_SGMII_LINK_UP |
+				       SR_MII_TX_CFG_PHY_MASTER;
+
+			/* SGMII interrupt in the port cannot be masked, so
+			 * make sure interrupt is not enabled as it is not
+			 * handled.
+			 */
+			val &= ~SR_MII_AUTO_NEG_COMPLETE_INTR;
+		} else if (reg == MII_BMCR) {
+			/* The MII_ADVERTISE register needs to write once
+			 * before doing auto-negotiation for the correct
+			 * config_word to be sent out after reset.
+			 */
+			if ((val & BMCR_ANENABLE) && !p->sgmii_adv_write) {
+				u16 adv;
+
+				/* The SGMII port cannot disable flow contrl
+				 * so it is better to just advertise symmetric
+				 * pause.
+				 */
+				port_sgmii_r(dev, port, mmd, MII_ADVERTISE,
+					     &adv);
+				adv |= ADVERTISE_1000XPAUSE;
+				adv &= ~ADVERTISE_1000XPSE_ASYM;
+				port_sgmii_w(dev, port, mmd, MII_ADVERTISE,
+					     adv);
+				p->sgmii_adv_write = 1;
+			} else if (val & BMCR_RESET) {
+				p->sgmii_adv_write = 0;
+			}
+		} else if (reg == MII_ADVERTISE) {
+			/* XPCS driver writes to this register so there is no
+			 * need to update it for the errata.
+			 */
+			p->sgmii_adv_write = 1;
+		}
+	}
+	port_sgmii_w(dev, port, mmd, reg, val);
+	return 0;
+}
+
+int ksz9477_pcs_create(struct ksz_device *dev)
+{
+	/* This chip has a SGMII port. */
+	if (ksz_has_sgmii_port(dev)) {
+		int port = ksz_get_sgmii_port(dev);
+		struct ksz_port *p = &dev->ports[port];
+		struct phylink_pcs *pcs;
+		struct mii_bus *bus;
+		int ret;
+
+		bus = devm_mdiobus_alloc(dev->dev);
+		if (!bus)
+			return -ENOMEM;
+
+		bus->name = "ksz_pcs_mdio_bus";
+		snprintf(bus->id, MII_BUS_ID_SIZE, "%s-pcs",
+			 dev_name(dev->dev));
+		bus->read_c45 = &ksz9477_pcs_read;
+		bus->write_c45 = &ksz9477_pcs_write;
+		bus->parent = dev->dev;
+		bus->phy_mask = ~0;
+		bus->priv = dev;
+
+		ret = devm_mdiobus_register(dev->dev, bus);
+		if (ret)
+			return ret;
+
+		pcs = xpcs_create_pcs_mdiodev(bus, 0);
+		if (IS_ERR(pcs))
+			return PTR_ERR(pcs);
+		p->pcs = pcs;
+	}
+	return 0;
+}
+
 int ksz9477_reset_switch(struct ksz_device *dev)
 {
 	u8 data8;
@@ -978,6 +1159,14 @@ void ksz9477_get_caps(struct ksz_device *dev, int port,
 
 	if (dev->info->gbit_capable[port])
 		config->mac_capabilities |= MAC_1000FD;
+
+	if (ksz_is_sgmii_port(dev, port)) {
+		struct ksz_port *p = &dev->ports[port];
+
+		phy_interface_or(config->supported_interfaces,
+				 config->supported_interfaces,
+				 p->pcs->supported_interfaces);
+	}
 }
 
 int ksz9477_set_ageing_time(struct ksz_device *dev, unsigned int msecs)
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index d2166b0d881e..0d1a6dfda23e 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -2,7 +2,7 @@
 /*
  * Microchip KSZ9477 series Header file
  *
- * Copyright (C) 2017-2022 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #ifndef __KSZ9477_H
@@ -97,4 +97,6 @@ void ksz9477_acl_match_process_l2(struct ksz_device *dev, int port,
 				  u16 ethtype, u8 *src_mac, u8 *dst_mac,
 				  unsigned long cookie, u32 prio);
 
+int ksz9477_pcs_create(struct ksz_device *dev);
+
 #endif
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index b45052497f8a..c93a567a4c3b 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2,7 +2,7 @@
 /*
  * Microchip switch driver main logic
  *
- * Copyright (C) 2017-2024 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #include <linux/delay.h>
@@ -354,10 +354,26 @@ static void ksz9477_phylink_mac_link_up(struct phylink_config *config,
 					int speed, int duplex, bool tx_pause,
 					bool rx_pause);
 
+static struct phylink_pcs *
+ksz_phylink_mac_select_pcs(struct phylink_config *config,
+			   phy_interface_t interface)
+{
+	struct dsa_port *dp = dsa_phylink_to_port(config);
+	struct ksz_device *dev = dp->ds->priv;
+	struct ksz_port *p = &dev->ports[dp->index];
+
+	if (ksz_is_sgmii_port(dev, dp->index) &&
+	    (interface == PHY_INTERFACE_MODE_SGMII ||
+	    interface == PHY_INTERFACE_MODE_1000BASEX))
+		return p->pcs;
+	return NULL;
+}
+
 static const struct phylink_mac_ops ksz9477_phylink_mac_ops = {
 	.mac_config	= ksz_phylink_mac_config,
 	.mac_link_down	= ksz_phylink_mac_link_down,
 	.mac_link_up	= ksz9477_phylink_mac_link_up,
+	.mac_select_pcs	= ksz_phylink_mac_select_pcs,
 };
 
 static const struct ksz_dev_ops ksz9477_dev_ops = {
@@ -395,6 +411,7 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.reset = ksz9477_reset_switch,
 	.init = ksz9477_switch_init,
 	.exit = ksz9477_switch_exit,
+	.pcs_create = ksz9477_pcs_create,
 };
 
 static const struct phylink_mac_ops lan937x_phylink_mac_ops = {
@@ -1035,8 +1052,7 @@ static const struct regmap_range ksz9477_valid_regs[] = {
 	regmap_reg_range(0x701b, 0x701b),
 	regmap_reg_range(0x701f, 0x7020),
 	regmap_reg_range(0x7030, 0x7030),
-	regmap_reg_range(0x7200, 0x7203),
-	regmap_reg_range(0x7206, 0x7207),
+	regmap_reg_range(0x7200, 0x7207),
 	regmap_reg_range(0x7300, 0x7301),
 	regmap_reg_range(0x7400, 0x7401),
 	regmap_reg_range(0x7403, 0x7403),
@@ -1552,6 +1568,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 				   true, false, false},
 		.gbit_capable	= {true, true, true, true, true, true, true},
 		.ptp_capable = true,
+		.sgmii_port = 7,
 		.wr_table = &ksz9477_register_set,
 		.rd_table = &ksz9477_register_set,
 	},
@@ -1944,6 +1961,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.internal_phy	= {true, true, true, true,
 				   true, false, false},
 		.gbit_capable	= {true, true, true, true, true, true, true},
+		.sgmii_port = 7,
 		.wr_table = &ksz9477_register_set,
 		.rd_table = &ksz9477_register_set,
 	},
@@ -2067,7 +2085,7 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 
 	spin_unlock(&mib->stats64_lock);
 
-	if (dev->info->phy_errata_9477) {
+	if (dev->info->phy_errata_9477 && !ksz_is_sgmii_port(dev, port)) {
 		ret = ksz9477_errata_monitor(dev, port, raw->tx_late_col);
 		if (ret)
 			dev_err(dev->dev, "Failed to monitor transmission halt\n");
@@ -2775,6 +2793,12 @@ static int ksz_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	if (ksz_has_sgmii_port(dev) && dev->dev_ops->pcs_create) {
+		ret = dev->dev_ops->pcs_create(dev);
+		if (ret)
+			return ret;
+	}
+
 	/* set broadcast storm protection 10% rate */
 	regmap_update_bits(ksz_regmap_16(dev), regs[S_BROADCAST_CTRL],
 			   BROADCAST_STORM_RATE,
@@ -3613,6 +3637,10 @@ static void ksz_phylink_mac_config(struct phylink_config *config,
 	if (dev->info->internal_phy[port])
 		return;
 
+	/* No need to configure XMII control register when using SGMII. */
+	if (ksz_is_sgmii_port(dev, port))
+		return;
+
 	if (phylink_autoneg_inband(mode)) {
 		dev_err(dev->dev, "In-band AN not supported!\n");
 		return;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index dd5429ff16ee..84e9e423980d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -1,7 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 /* Microchip switch driver common header
  *
- * Copyright (C) 2017-2024 Microchip Technology Inc.
+ * Copyright (C) 2017-2025 Microchip Technology Inc.
  */
 
 #ifndef __KSZ_COMMON_H
@@ -10,6 +10,7 @@
 #include <linux/etherdevice.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
+#include <linux/pcs/pcs-xpcs.h>
 #include <linux/phy.h>
 #include <linux/regmap.h>
 #include <net/dsa.h>
@@ -93,6 +94,7 @@ struct ksz_chip_data {
 	bool internal_phy[KSZ_MAX_NUM_PORTS];
 	bool gbit_capable[KSZ_MAX_NUM_PORTS];
 	bool ptp_capable;
+	u8 sgmii_port;
 	const struct regmap_access_table *wr_table;
 	const struct regmap_access_table *rd_table;
 };
@@ -132,6 +134,7 @@ struct ksz_port {
 	u32 force:1;
 	u32 read:1;			/* read MIB counters in background */
 	u32 freeze:1;			/* MIB counter freeze is enabled */
+	u32 sgmii_adv_write:1;
 
 	struct ksz_port_mib mib;
 	phy_interface_t interface;
@@ -141,6 +144,7 @@ struct ksz_port {
 	void *acl_priv;
 	struct ksz_irq pirq;
 	u8 num;
+	struct phylink_pcs *pcs;
 #if IS_ENABLED(CONFIG_NET_DSA_MICROCHIP_KSZ_PTP)
 	struct hwtstamp_config tstamp_config;
 	bool hwts_tx_en;
@@ -440,6 +444,8 @@ struct ksz_dev_ops {
 	int (*reset)(struct ksz_device *dev);
 	int (*init)(struct ksz_device *dev);
 	void (*exit)(struct ksz_device *dev);
+
+	int (*pcs_create)(struct ksz_device *dev);
 };
 
 struct ksz_device *ksz_switch_alloc(struct device *base, void *priv);
@@ -731,6 +737,21 @@ static inline bool is_lan937x_tx_phy(struct ksz_device *dev, int port)
 		dev->chip_id == LAN9372_CHIP_ID) && port == KSZ_PORT_4;
 }
 
+static inline int ksz_get_sgmii_port(struct ksz_device *dev)
+{
+	return dev->info->sgmii_port - 1;
+}
+
+static inline bool ksz_has_sgmii_port(struct ksz_device *dev)
+{
+	return dev->info->sgmii_port > 0;
+}
+
+static inline bool ksz_is_sgmii_port(struct ksz_device *dev, int port)
+{
+	return dev->info->sgmii_port == port + 1;
+}
+
 /* STP State Defines */
 #define PORT_TX_ENABLE			BIT(2)
 #define PORT_RX_ENABLE			BIT(1)
-- 
2.34.1

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Maxime Chevallier]

Hi Tristram,

On Tue, 6 May 2025 17:09:11 -0700
<Tristram.Ha@microchip.com> wrote:

> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The KSZ9477 switch driver uses the XPCS driver to operate its SGMII
> port.  However there are some hardware bugs in the KSZ9477 SGMII
> module so workarounds are needed.  There was a proposal to update the
> XPCS driver to accommodate KSZ9477, but the new code is not generic
> enough to be used by other vendors.  It is better to do all these
> workarounds inside the KSZ9477 driver instead of modifying the XPCS
> driver.
> 
> There are 3 hardware issues.  The first is the MII_ADVERTISE register
> needs to be write once after reset for the correct code word to be
> sent.  The XPCS driver disables auto-negotiation first before
> configuring the SGMII/1000BASE-X mode and then enables it back.  The
> KSZ9477 driver then writes the MII_ADVERTISE register before enabling
> auto-negotiation.  In 1000BASE-X mode the MII_ADVERTISE register will
> be set, so KSZ9477 driver does not need to write it.
> 
> The second issue is the MII_BMCR register needs to set the exact speed
> and duplex mode when running in SGMII mode.  During link polling the
> KSZ9477 will check the speed and duplex mode are different from
> previous ones and update the MII_BMCR register accordingly.
> 
> The last issue is 1000BASE-X mode does not work with auto-negotiation
> on.  The cause is the local port hardware does not know the link is up
> and so network traffic is not forwarded.  The workaround is to write 2
> additional bits when 1000BASE-X mode is configured.
> 
> Note the SGMII interrupt in the port cannot be masked.  As that
> interrupt is not handled in the KSZ9477 driver the SGMII interrupt bit
> will not be set even when the XPCS driver sets it.
>
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>

[...]

> +
> +static int ksz9477_pcs_read(struct mii_bus *bus, int phy, int mmd, int reg)
> +{
> +	struct ksz_device *dev = bus->priv;
> +	int port = ksz_get_sgmii_port(dev);
> +	u16 val;
> +
> +	port_sgmii_r(dev, port, mmd, reg, &val);
> +
> +	/* Simulate a value to activate special code in the XPCS driver if
> +	 * supported.
> +	 */
> +	if (mmd == MDIO_MMD_PMAPMD) {
> +		if (reg == MDIO_DEVID1)
> +			val = 0x9477;
> +		else if (reg == MDIO_DEVID2)
> +			val = 0x22 << 10;
> +	} else if (mmd == MDIO_MMD_VEND2) {
> +		struct ksz_port *p = &dev->ports[port];
> +
> +		/* Need to update MII_BMCR register with the exact speed and
> +		 * duplex mode when running in SGMII mode and this register is
> +		 * used to detect connected speed in that mode.
> +		 */
> +		if (reg == MMD_SR_MII_AUTO_NEG_STATUS) {
> +			int duplex, speed;
> +
> +			if (val & SR_MII_STAT_LINK_UP) {
> +				speed = (val >> SR_MII_STAT_S) & SR_MII_STAT_M;
> +				if (speed == SR_MII_STAT_1000_MBPS)
> +					speed = SPEED_1000;
> +				else if (speed == SR_MII_STAT_100_MBPS)
> +					speed = SPEED_100;
> +				else
> +					speed = SPEED_10;
> +
> +				if (val & SR_MII_STAT_FULL_DUPLEX)
> +					duplex = DUPLEX_FULL;
> +				else
> +					duplex = DUPLEX_HALF;
> +
> +				if (!p->phydev.link ||
> +				    p->phydev.speed != speed ||
> +				    p->phydev.duplex != duplex) {
> +					u16 ctrl;
> +
> +					p->phydev.link = 1;
> +					p->phydev.speed = speed;
> +					p->phydev.duplex = duplex;
> +					port_sgmii_r(dev, port, mmd, MII_BMCR,
> +						     &ctrl);
> +					ctrl &= BMCR_ANENABLE;
> +					ctrl |= mii_bmcr_encode_fixed(speed,
> +								      duplex);
> +					port_sgmii_w(dev, port, mmd, MII_BMCR,
> +						     ctrl);
> +				}
> +			} else {
> +				p->phydev.link = 0;
> +			}
> +		} else if (reg == MII_BMSR) {
> +			p->phydev.link = (val & BMSR_LSTATUS);
> +		}
> +	}
> +	return val;
> +}
> +
> +static int ksz9477_pcs_write(struct mii_bus *bus, int phy, int mmd, int reg,
> +			     u16 val)
> +{
> +	struct ksz_device *dev = bus->priv;
> +	int port = ksz_get_sgmii_port(dev);
> +
> +	if (mmd == MDIO_MMD_VEND2) {
> +		struct ksz_port *p = &dev->ports[port];
> +
> +		if (reg == MMD_SR_MII_AUTO_NEG_CTRL) {
> +			u16 sgmii_mode = SR_MII_PCS_SGMII << SR_MII_PCS_MODE_S;
> +
> +			/* Need these bits for 1000BASE-X mode to work with
> +			 * AN on.
> +			 */
> +			if (!(val & sgmii_mode))
> +				val |= SR_MII_SGMII_LINK_UP |
> +				       SR_MII_TX_CFG_PHY_MASTER;
> +
> +			/* SGMII interrupt in the port cannot be masked, so
> +			 * make sure interrupt is not enabled as it is not
> +			 * handled.
> +			 */
> +			val &= ~SR_MII_AUTO_NEG_COMPLETE_INTR;
> +		} else if (reg == MII_BMCR) {
> +			/* The MII_ADVERTISE register needs to write once
> +			 * before doing auto-negotiation for the correct
> +			 * config_word to be sent out after reset.
> +			 */
> +			if ((val & BMCR_ANENABLE) && !p->sgmii_adv_write) {
> +				u16 adv;
> +
> +				/* The SGMII port cannot disable flow contrl
> +				 * so it is better to just advertise symmetric
> +				 * pause.
> +				 */
> +				port_sgmii_r(dev, port, mmd, MII_ADVERTISE,
> +					     &adv);
> +				adv |= ADVERTISE_1000XPAUSE;
> +				adv &= ~ADVERTISE_1000XPSE_ASYM;
> +				port_sgmii_w(dev, port, mmd, MII_ADVERTISE,
> +					     adv);
> +				p->sgmii_adv_write = 1;
> +			} else if (val & BMCR_RESET) {
> +				p->sgmii_adv_write = 0;
> +			}
> +		} else if (reg == MII_ADVERTISE) {
> +			/* XPCS driver writes to this register so there is no
> +			 * need to update it for the errata.
> +			 */
> +			p->sgmii_adv_write = 1;
> +		}
> +	}
> +	port_sgmii_w(dev, port, mmd, reg, val);
> +	return 0;
> +}

I'm a bit confused here, are you intercepting r/w ops that are supposed
to be handled by xpcs ?

Russell has sent a series [1] (not merged yet, I think we were waiting
on some feedback from Synopsys folks ?) to properly support the XPCS
version that's in KSZ9477, and you also had a patchset that didn't
require all this sgmii_r/w snooping [2].

I've been running your previous patchset on top of Russell's for a few
months, if works fine with SGMII as well as 1000BaseX :)

Can we maybe focus on getting pcs-xpcs to properly support this version
of the IP instead of these 2 R/W functions ? Or did I miss something in
the previous discussions ?

Maxime

[1] : https://lore.kernel.org/netdev/Z6NnPm13D1n5-Qlw@shell.armlinux.org.uk/ 
[2] : https://lore.kernel.org/netdev/20250208002417.58634-1-Tristram.Ha@microchip.com/

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Russell King (Oracle)]

On Wed, May 07, 2025 at 09:44:49AM +0200, Maxime Chevallier wrote:
> Hi Tristram,
> 
> On Tue, 6 May 2025 17:09:11 -0700
> <Tristram.Ha@microchip.com> wrote:
> 
> > From: Tristram Ha <tristram.ha@microchip.com>
> > 
> > The KSZ9477 switch driver uses the XPCS driver to operate its SGMII
> > port.  However there are some hardware bugs in the KSZ9477 SGMII
> > module so workarounds are needed.  There was a proposal to update the
> > XPCS driver to accommodate KSZ9477, but the new code is not generic
> > enough to be used by other vendors.  It is better to do all these
> > workarounds inside the KSZ9477 driver instead of modifying the XPCS
> > driver.
> > 
> > There are 3 hardware issues.  The first is the MII_ADVERTISE register
> > needs to be write once after reset for the correct code word to be
> > sent.  The XPCS driver disables auto-negotiation first before
> > configuring the SGMII/1000BASE-X mode and then enables it back.  The
> > KSZ9477 driver then writes the MII_ADVERTISE register before enabling
> > auto-negotiation.  In 1000BASE-X mode the MII_ADVERTISE register will
> > be set, so KSZ9477 driver does not need to write it.
> > 
> > The second issue is the MII_BMCR register needs to set the exact speed
> > and duplex mode when running in SGMII mode.  During link polling the
> > KSZ9477 will check the speed and duplex mode are different from
> > previous ones and update the MII_BMCR register accordingly.
> > 
> > The last issue is 1000BASE-X mode does not work with auto-negotiation
> > on.  The cause is the local port hardware does not know the link is up
> > and so network traffic is not forwarded.  The workaround is to write 2
> > additional bits when 1000BASE-X mode is configured.
> > 
> > Note the SGMII interrupt in the port cannot be masked.  As that
> > interrupt is not handled in the KSZ9477 driver the SGMII interrupt bit
> > will not be set even when the XPCS driver sets it.
> >
> > Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> 
> [...]
> 
> > +
> > +static int ksz9477_pcs_read(struct mii_bus *bus, int phy, int mmd, int reg)
> > +{
> > +	struct ksz_device *dev = bus->priv;
> > +	int port = ksz_get_sgmii_port(dev);
> > +	u16 val;
> > +
> > +	port_sgmii_r(dev, port, mmd, reg, &val);
> > +
> > +	/* Simulate a value to activate special code in the XPCS driver if
> > +	 * supported.
> > +	 */
> > +	if (mmd == MDIO_MMD_PMAPMD) {
> > +		if (reg == MDIO_DEVID1)
> > +			val = 0x9477;
> > +		else if (reg == MDIO_DEVID2)
> > +			val = 0x22 << 10;
> > +	} else if (mmd == MDIO_MMD_VEND2) {
> > +		struct ksz_port *p = &dev->ports[port];
> > +
> > +		/* Need to update MII_BMCR register with the exact speed and
> > +		 * duplex mode when running in SGMII mode and this register is
> > +		 * used to detect connected speed in that mode.
> > +		 */
> > +		if (reg == MMD_SR_MII_AUTO_NEG_STATUS) {
> > +			int duplex, speed;
> > +
> > +			if (val & SR_MII_STAT_LINK_UP) {
> > +				speed = (val >> SR_MII_STAT_S) & SR_MII_STAT_M;
> > +				if (speed == SR_MII_STAT_1000_MBPS)
> > +					speed = SPEED_1000;
> > +				else if (speed == SR_MII_STAT_100_MBPS)
> > +					speed = SPEED_100;
> > +				else
> > +					speed = SPEED_10;
> > +
> > +				if (val & SR_MII_STAT_FULL_DUPLEX)
> > +					duplex = DUPLEX_FULL;
> > +				else
> > +					duplex = DUPLEX_HALF;
> > +
> > +				if (!p->phydev.link ||
> > +				    p->phydev.speed != speed ||
> > +				    p->phydev.duplex != duplex) {
> > +					u16 ctrl;
> > +
> > +					p->phydev.link = 1;
> > +					p->phydev.speed = speed;
> > +					p->phydev.duplex = duplex;
> > +					port_sgmii_r(dev, port, mmd, MII_BMCR,
> > +						     &ctrl);
> > +					ctrl &= BMCR_ANENABLE;
> > +					ctrl |= mii_bmcr_encode_fixed(speed,
> > +								      duplex);
> > +					port_sgmii_w(dev, port, mmd, MII_BMCR,
> > +						     ctrl);
> > +				}
> > +			} else {
> > +				p->phydev.link = 0;
> > +			}
> > +		} else if (reg == MII_BMSR) {
> > +			p->phydev.link = (val & BMSR_LSTATUS);
> > +		}
> > +	}
> > +	return val;
> > +}
> > +
> > +static int ksz9477_pcs_write(struct mii_bus *bus, int phy, int mmd, int reg,
> > +			     u16 val)
> > +{
> > +	struct ksz_device *dev = bus->priv;
> > +	int port = ksz_get_sgmii_port(dev);
> > +
> > +	if (mmd == MDIO_MMD_VEND2) {
> > +		struct ksz_port *p = &dev->ports[port];
> > +
> > +		if (reg == MMD_SR_MII_AUTO_NEG_CTRL) {
> > +			u16 sgmii_mode = SR_MII_PCS_SGMII << SR_MII_PCS_MODE_S;
> > +
> > +			/* Need these bits for 1000BASE-X mode to work with
> > +			 * AN on.
> > +			 */
> > +			if (!(val & sgmii_mode))
> > +				val |= SR_MII_SGMII_LINK_UP |
> > +				       SR_MII_TX_CFG_PHY_MASTER;
> > +
> > +			/* SGMII interrupt in the port cannot be masked, so
> > +			 * make sure interrupt is not enabled as it is not
> > +			 * handled.
> > +			 */
> > +			val &= ~SR_MII_AUTO_NEG_COMPLETE_INTR;
> > +		} else if (reg == MII_BMCR) {
> > +			/* The MII_ADVERTISE register needs to write once
> > +			 * before doing auto-negotiation for the correct
> > +			 * config_word to be sent out after reset.
> > +			 */
> > +			if ((val & BMCR_ANENABLE) && !p->sgmii_adv_write) {
> > +				u16 adv;
> > +
> > +				/* The SGMII port cannot disable flow contrl
> > +				 * so it is better to just advertise symmetric
> > +				 * pause.
> > +				 */
> > +				port_sgmii_r(dev, port, mmd, MII_ADVERTISE,
> > +					     &adv);
> > +				adv |= ADVERTISE_1000XPAUSE;
> > +				adv &= ~ADVERTISE_1000XPSE_ASYM;
> > +				port_sgmii_w(dev, port, mmd, MII_ADVERTISE,
> > +					     adv);
> > +				p->sgmii_adv_write = 1;
> > +			} else if (val & BMCR_RESET) {
> > +				p->sgmii_adv_write = 0;
> > +			}
> > +		} else if (reg == MII_ADVERTISE) {
> > +			/* XPCS driver writes to this register so there is no
> > +			 * need to update it for the errata.
> > +			 */
> > +			p->sgmii_adv_write = 1;
> > +		}
> > +	}
> > +	port_sgmii_w(dev, port, mmd, reg, val);
> > +	return 0;
> > +}
> 
> I'm a bit confused here, are you intercepting r/w ops that are supposed
> to be handled by xpcs ?
> 
> Russell has sent a series [1] (not merged yet, I think we were waiting
> on some feedback from Synopsys folks ?) to properly support the XPCS
> version that's in KSZ9477, and you also had a patchset that didn't
> require all this sgmii_r/w snooping [2].
> 
> I've been running your previous patchset on top of Russell's for a few
> months, if works fine with SGMII as well as 1000BaseX :)
> 
> Can we maybe focus on getting pcs-xpcs to properly support this version
> of the IP instead of these 2 R/W functions ? Or did I miss something in
> the previous discussions ?

Honestly, I don't think Tristram is doing anything unreasonable here,
given what Vladimir has been saying. Essentially, we've been blocking
a way forward on the pcs-xpcs driver. We've had statements from the
hardware designers from Microchip. We've had statements from Synopsys.
The two don't quite agree, but that's not atypical. Yet, we're still
demanding why the Microchip version of XPCS is different.

So what's left for Tristram to do other than to hack around the blockage
we're causing by intercepting the read/write ops and bodging them.

As I understand the situation, this is Jose's response having asked
internally at my request:

https://lore.kernel.org/netdev/DM4PR12MB5088BA650B164D5CEC33CA08D3E82@DM4PR12MB5088.namprd12.prod.outlook.com/

To put it another way, as far as Synopsys can tell us, they are unaware
of the Microchip behaviour, but customers can modify the Synopsys IP.

Maybe Microchip's version is based on an old Synopsys version, but
which was modified by Microchip a long time ago and those engineers
have moved on, and no one really knows anymore. I doubt that we are
ever going to get to the bottom of the different behaviour.

So, what do we do now? Do we continue playing hardball and basically
saying "no" to changing the XPCS driver, demanding information that
doesn't seem to exist anymore? Or do we try to come up with an
approach that works.

I draw attention to the last sentence in Jose's quote in his reply.
As far as the Synopsys folk are concerned, setting these bits to 1
should have no effect provided there aren't customer modifications to
the IP that depend on these being set to zero.

That last bit is where I think the sticking point between Vladimir and
myself is - I'm in favour of keeping things simple and just setting
the bits. Vladimir feels it would be safer to make it conditional,
which leads to more complicated code.

I didn't progress my series because I decided it was a waste of time
to try and progress this any further - I'd dug up the SJA1105 docs to
see what they said, I'd reached out to Synopsys and got a statement
back, and still Vladimir wasn't happy.

With Vladimir continuing to demand information from Tristram that just
didn't exist, I saw that the

[rest of the email got deleted because Linux / X11 / KDE got confused
about the state the backspace key and decided it was going to be
continuously pressed and doing nothing except shutting the laptop
down would stop it.]

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Maxime Chevallier]

On Wed, 7 May 2025 09:31:48 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Wed, May 07, 2025 at 09:44:49AM +0200, Maxime Chevallier wrote:
> > Hi Tristram,
> > 
> > On Tue, 6 May 2025 17:09:11 -0700
> > <Tristram.Ha@microchip.com> wrote:
> >   
> > > From: Tristram Ha <tristram.ha@microchip.com>
> > > 
> > > The KSZ9477 switch driver uses the XPCS driver to operate its SGMII
> > > port.  However there are some hardware bugs in the KSZ9477 SGMII
> > > module so workarounds are needed.  There was a proposal to update the
> > > XPCS driver to accommodate KSZ9477, but the new code is not generic
> > > enough to be used by other vendors.  It is better to do all these
> > > workarounds inside the KSZ9477 driver instead of modifying the XPCS
> > > driver.
> > > 
> > > There are 3 hardware issues.  The first is the MII_ADVERTISE register
> > > needs to be write once after reset for the correct code word to be
> > > sent.  The XPCS driver disables auto-negotiation first before
> > > configuring the SGMII/1000BASE-X mode and then enables it back.  The
> > > KSZ9477 driver then writes the MII_ADVERTISE register before enabling
> > > auto-negotiation.  In 1000BASE-X mode the MII_ADVERTISE register will
> > > be set, so KSZ9477 driver does not need to write it.
> > > 
> > > The second issue is the MII_BMCR register needs to set the exact speed
> > > and duplex mode when running in SGMII mode.  During link polling the
> > > KSZ9477 will check the speed and duplex mode are different from
> > > previous ones and update the MII_BMCR register accordingly.
> > > 
> > > The last issue is 1000BASE-X mode does not work with auto-negotiation
> > > on.  The cause is the local port hardware does not know the link is up
> > > and so network traffic is not forwarded.  The workaround is to write 2
> > > additional bits when 1000BASE-X mode is configured.
> > > 
> > > Note the SGMII interrupt in the port cannot be masked.  As that
> > > interrupt is not handled in the KSZ9477 driver the SGMII interrupt bit
> > > will not be set even when the XPCS driver sets it.
> > >
> > > Signed-off-by: Tristram Ha <tristram.ha@microchip.com>  
> > 
> > [...]
> >   
> > > +
> > > +static int ksz9477_pcs_read(struct mii_bus *bus, int phy, int mmd, int reg)
> > > +{
> > > +	struct ksz_device *dev = bus->priv;
> > > +	int port = ksz_get_sgmii_port(dev);
> > > +	u16 val;
> > > +
> > > +	port_sgmii_r(dev, port, mmd, reg, &val);
> > > +
> > > +	/* Simulate a value to activate special code in the XPCS driver if
> > > +	 * supported.
> > > +	 */
> > > +	if (mmd == MDIO_MMD_PMAPMD) {
> > > +		if (reg == MDIO_DEVID1)
> > > +			val = 0x9477;
> > > +		else if (reg == MDIO_DEVID2)
> > > +			val = 0x22 << 10;
> > > +	} else if (mmd == MDIO_MMD_VEND2) {
> > > +		struct ksz_port *p = &dev->ports[port];
> > > +
> > > +		/* Need to update MII_BMCR register with the exact speed and
> > > +		 * duplex mode when running in SGMII mode and this register is
> > > +		 * used to detect connected speed in that mode.
> > > +		 */
> > > +		if (reg == MMD_SR_MII_AUTO_NEG_STATUS) {
> > > +			int duplex, speed;
> > > +
> > > +			if (val & SR_MII_STAT_LINK_UP) {
> > > +				speed = (val >> SR_MII_STAT_S) & SR_MII_STAT_M;
> > > +				if (speed == SR_MII_STAT_1000_MBPS)
> > > +					speed = SPEED_1000;
> > > +				else if (speed == SR_MII_STAT_100_MBPS)
> > > +					speed = SPEED_100;
> > > +				else
> > > +					speed = SPEED_10;
> > > +
> > > +				if (val & SR_MII_STAT_FULL_DUPLEX)
> > > +					duplex = DUPLEX_FULL;
> > > +				else
> > > +					duplex = DUPLEX_HALF;
> > > +
> > > +				if (!p->phydev.link ||
> > > +				    p->phydev.speed != speed ||
> > > +				    p->phydev.duplex != duplex) {
> > > +					u16 ctrl;
> > > +
> > > +					p->phydev.link = 1;
> > > +					p->phydev.speed = speed;
> > > +					p->phydev.duplex = duplex;
> > > +					port_sgmii_r(dev, port, mmd, MII_BMCR,
> > > +						     &ctrl);
> > > +					ctrl &= BMCR_ANENABLE;
> > > +					ctrl |= mii_bmcr_encode_fixed(speed,
> > > +								      duplex);
> > > +					port_sgmii_w(dev, port, mmd, MII_BMCR,
> > > +						     ctrl);
> > > +				}
> > > +			} else {
> > > +				p->phydev.link = 0;
> > > +			}
> > > +		} else if (reg == MII_BMSR) {
> > > +			p->phydev.link = (val & BMSR_LSTATUS);
> > > +		}
> > > +	}
> > > +	return val;
> > > +}
> > > +
> > > +static int ksz9477_pcs_write(struct mii_bus *bus, int phy, int mmd, int reg,
> > > +			     u16 val)
> > > +{
> > > +	struct ksz_device *dev = bus->priv;
> > > +	int port = ksz_get_sgmii_port(dev);
> > > +
> > > +	if (mmd == MDIO_MMD_VEND2) {
> > > +		struct ksz_port *p = &dev->ports[port];
> > > +
> > > +		if (reg == MMD_SR_MII_AUTO_NEG_CTRL) {
> > > +			u16 sgmii_mode = SR_MII_PCS_SGMII << SR_MII_PCS_MODE_S;
> > > +
> > > +			/* Need these bits for 1000BASE-X mode to work with
> > > +			 * AN on.
> > > +			 */
> > > +			if (!(val & sgmii_mode))
> > > +				val |= SR_MII_SGMII_LINK_UP |
> > > +				       SR_MII_TX_CFG_PHY_MASTER;
> > > +
> > > +			/* SGMII interrupt in the port cannot be masked, so
> > > +			 * make sure interrupt is not enabled as it is not
> > > +			 * handled.
> > > +			 */
> > > +			val &= ~SR_MII_AUTO_NEG_COMPLETE_INTR;
> > > +		} else if (reg == MII_BMCR) {
> > > +			/* The MII_ADVERTISE register needs to write once
> > > +			 * before doing auto-negotiation for the correct
> > > +			 * config_word to be sent out after reset.
> > > +			 */
> > > +			if ((val & BMCR_ANENABLE) && !p->sgmii_adv_write) {
> > > +				u16 adv;
> > > +
> > > +				/* The SGMII port cannot disable flow contrl
> > > +				 * so it is better to just advertise symmetric
> > > +				 * pause.
> > > +				 */
> > > +				port_sgmii_r(dev, port, mmd, MII_ADVERTISE,
> > > +					     &adv);
> > > +				adv |= ADVERTISE_1000XPAUSE;
> > > +				adv &= ~ADVERTISE_1000XPSE_ASYM;
> > > +				port_sgmii_w(dev, port, mmd, MII_ADVERTISE,
> > > +					     adv);
> > > +				p->sgmii_adv_write = 1;
> > > +			} else if (val & BMCR_RESET) {
> > > +				p->sgmii_adv_write = 0;
> > > +			}
> > > +		} else if (reg == MII_ADVERTISE) {
> > > +			/* XPCS driver writes to this register so there is no
> > > +			 * need to update it for the errata.
> > > +			 */
> > > +			p->sgmii_adv_write = 1;
> > > +		}
> > > +	}
> > > +	port_sgmii_w(dev, port, mmd, reg, val);
> > > +	return 0;
> > > +}  
> > 
> > I'm a bit confused here, are you intercepting r/w ops that are supposed
> > to be handled by xpcs ?
> > 
> > Russell has sent a series [1] (not merged yet, I think we were waiting
> > on some feedback from Synopsys folks ?) to properly support the XPCS
> > version that's in KSZ9477, and you also had a patchset that didn't
> > require all this sgmii_r/w snooping [2].
> > 
> > I've been running your previous patchset on top of Russell's for a few
> > months, if works fine with SGMII as well as 1000BaseX :)
> > 
> > Can we maybe focus on getting pcs-xpcs to properly support this version
> > of the IP instead of these 2 R/W functions ? Or did I miss something in
> > the previous discussions ?  
> 
> Honestly, I don't think Tristram is doing anything unreasonable here,
> given what Vladimir has been saying. Essentially, we've been blocking
> a way forward on the pcs-xpcs driver. We've had statements from the
> hardware designers from Microchip. We've had statements from Synopsys.
> The two don't quite agree, but that's not atypical. Yet, we're still
> demanding why the Microchip version of XPCS is different.
> 
> So what's left for Tristram to do other than to hack around the blockage
> we're causing by intercepting the read/write ops and bodging them.
> 
> As I understand the situation, this is Jose's response having asked
> internally at my request:
> 
> https://lore.kernel.org/netdev/DM4PR12MB5088BA650B164D5CEC33CA08D3E82@DM4PR12MB5088.namprd12.prod.outlook.com/
> 
> To put it another way, as far as Synopsys can tell us, they are unaware
> of the Microchip behaviour, but customers can modify the Synopsys IP.
> 
> Maybe Microchip's version is based on an old Synopsys version, but
> which was modified by Microchip a long time ago and those engineers
> have moved on, and no one really knows anymore. I doubt that we are
> ever going to get to the bottom of the different behaviour.
> 
> So, what do we do now? Do we continue playing hardball and basically
> saying "no" to changing the XPCS driver, demanding information that
> doesn't seem to exist anymore? Or do we try to come up with an
> approach that works.

Fair enough, it wasn't clear to me that this was the path forward, but
that does make sense to avoid cluttering xpcs with things that, in that
case, are really KSZ9477 specific.

I'll try to give this patch a try on my side soon-ish, but I'm working
with limited access to HW for the next few days.

> I draw attention to the last sentence in Jose's quote in his reply.
> As far as the Synopsys folk are concerned, setting these bits to 1
> should have no effect provided there aren't customer modifications to
> the IP that depend on these being set to zero.
> 
> That last bit is where I think the sticking point between Vladimir and
> myself is - I'm in favour of keeping things simple and just setting
> the bits. Vladimir feels it would be safer to make it conditional,
> which leads to more complicated code.
> 
> I didn't progress my series because I decided it was a waste of time
> to try and progress this any further - I'd dug up the SJA1105 docs to
> see what they said, I'd reached out to Synopsys and got a statement
> back, and still Vladimir wasn't happy.
> 
> With Vladimir continuing to demand information from Tristram that just
> didn't exist, I saw that the
> 
> [rest of the email got deleted because Linux / X11 / KDE got confused
> about the state the backspace key and decided it was going to be
> continuously pressed and doing nothing except shutting the laptop
> down would stop it.]

Funny how I have the same exact issue on my laptop as well... 

Thanks for the quick reply, and Tristram sorry for the noise then :)

Maxime

RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> > > I'm a bit confused here, are you intercepting r/w ops that are supposed
> > > to be handled by xpcs ?
> > >
> > > Russell has sent a series [1] (not merged yet, I think we were waiting
> > > on some feedback from Synopsys folks ?) to properly support the XPCS
> > > version that's in KSZ9477, and you also had a patchset that didn't
> > > require all this sgmii_r/w snooping [2].
> > >
> > > I've been running your previous patchset on top of Russell's for a few
> > > months, if works fine with SGMII as well as 1000BaseX :)
> > >
> > > Can we maybe focus on getting pcs-xpcs to properly support this version
> > > of the IP instead of these 2 R/W functions ? Or did I miss something in
> > > the previous discussions ?
> >
> > Honestly, I don't think Tristram is doing anything unreasonable here,
> > given what Vladimir has been saying. Essentially, we've been blocking
> > a way forward on the pcs-xpcs driver. We've had statements from the
> > hardware designers from Microchip. We've had statements from Synopsys.
> > The two don't quite agree, but that's not atypical. Yet, we're still
> > demanding why the Microchip version of XPCS is different.
> >
> > So what's left for Tristram to do other than to hack around the blockage
> > we're causing by intercepting the read/write ops and bodging them.
> >
> > As I understand the situation, this is Jose's response having asked
> > internally at my request:
> >
> >
> https://lore.kernel.org/netdev/DM4PR12MB5088BA650B164D5CEC33CA08D3E82@
> DM4PR12MB5088.namprd12.prod.outlook.com/
> >
> > To put it another way, as far as Synopsys can tell us, they are unaware
> > of the Microchip behaviour, but customers can modify the Synopsys IP.
> >
> > Maybe Microchip's version is based on an old Synopsys version, but
> > which was modified by Microchip a long time ago and those engineers
> > have moved on, and no one really knows anymore. I doubt that we are
> > ever going to get to the bottom of the different behaviour.
> >
> > So, what do we do now? Do we continue playing hardball and basically
> > saying "no" to changing the XPCS driver, demanding information that
> > doesn't seem to exist anymore? Or do we try to come up with an
> > approach that works.
> 
> Fair enough, it wasn't clear to me that this was the path forward, but
> that does make sense to avoid cluttering xpcs with things that, in that
> case, are really KSZ9477 specific.
> 
> I'll try to give this patch a try on my side soon-ish, but I'm working
> with limited access to HW for the next few days.
> 
> > I draw attention to the last sentence in Jose's quote in his reply.
> > As far as the Synopsys folk are concerned, setting these bits to 1
> > should have no effect provided there aren't customer modifications to
> > the IP that depend on these being set to zero.
> >
> > That last bit is where I think the sticking point between Vladimir and
> > myself is - I'm in favour of keeping things simple and just setting
> > the bits. Vladimir feels it would be safer to make it conditional,
> > which leads to more complicated code.
> >
> > I didn't progress my series because I decided it was a waste of time
> > to try and progress this any further - I'd dug up the SJA1105 docs to
> > see what they said, I'd reached out to Synopsys and got a statement
> > back, and still Vladimir wasn't happy.
> >
> > With Vladimir continuing to demand information from Tristram that just
> > didn't exist, I saw that the
> >
> > [rest of the email got deleted because Linux / X11 / KDE got confused
> > about the state the backspace key and decided it was going to be
> > continuously pressed and doing nothing except shutting the laptop
> > down would stop it.]
> 
> Funny how I have the same exact issue on my laptop as well...
> 
> Thanks for the quick reply, and Tristram sorry for the noise then :)

Thank you for testing the code.

It seems the proposed XPCS driver code did not get any comments from
other people using different implementations, so I think it will not help
others.  Since the hardware issues are KSZ9477 specific it is better to
implement any workarounds inside the KSZ9477 DSA driver which is easier
to backport to older kernel versions.

As the KSZ9477 driver already returns a software generated value to the
XPCS driver probing I think it is okay to do additional register
programming with each XPCS driver request.

It is almost a year since the first submission to add SGMII port support
to KSZ9477 switch.  Customers were asking for this and there is a product
launch with SGMII port as a main feature.  Microchip management is
pushing for an official software support in the kernel.

Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch

[Jakub Kicinski]

On Tue, 6 May 2025 17:09:11 -0700 Tristram.Ha@microchip.com wrote:
>  drivers/net/dsa/microchip/Kconfig      |   1 +
>  drivers/net/dsa/microchip/ksz9477.c    | 191 ++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz9477.h    |   4 +-
>  drivers/net/dsa/microchip/ksz_common.c |  36 ++++-
>  drivers/net/dsa/microchip/ksz_common.h |  23 ++-

No longer applies cleanly, please rebase:

Applying: net: dsa: microchip: Add SGMII port support to KSZ9477 switch
Using index info to reconstruct a base tree...
M	drivers/net/dsa/microchip/ksz_common.h
Falling back to patching base and 3-way merge...
-- 
pw-bot: cr