[PATCH net-next 0/4] net: dsa: microchip: add SGMII port support to KSZ9477 switch

[PATCH net-next 0/4] net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Tristram.Ha]

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

This series of patches is to add SGMII port support to KSZ9477 switch.

As the SGMII module has its own interrupt for link indication the common
code needs to be prepared to allow KSZ9477 to handle other interrupts
that are not passed to other drivers such as PHY.

The SGMII port is simulated as having a regular PHY as there is not much
to do with that port except reporting link on/off and connecting speed.

Tristram Ha (4):
  dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477
    switch
  net: dsa: microchip: support global switch interrupt in KSZ DSA driver
  net: dsa: microchip: handle most interrupts in KSZ9477/KSZ9893 switch
    families
  net: dsa: microchip: add SGMII port support to KSZ9477 switch

 .../bindings/net/dsa/microchip,ksz.yaml       |  12 +
 drivers/net/dsa/microchip/ksz9477.c           | 404 +++++++++++++++++-
 drivers/net/dsa/microchip/ksz9477.h           |   6 +-
 drivers/net/dsa/microchip/ksz9477_reg.h       |  10 +-
 drivers/net/dsa/microchip/ksz_common.c        |  45 +-
 drivers/net/dsa/microchip/ksz_common.h        |  11 +-
 6 files changed, 477 insertions(+), 11 deletions(-)

-- 
2.34.1

[PATCH net-next 1/4] dt-bindings: 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: 0 for direct
connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.

SFP is typically used so the default is 1.  The driver can detect
10/100/1000 SFP and change the mode to 2.  For direct connect this mode
has to be explicitly set to 0 as driver cannot detect that
configuration.

Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
 .../devicetree/bindings/net/dsa/microchip,ksz.yaml   | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
index 52acc15ebcbf..b4a9746556bf 100644
--- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
+++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
@@ -71,6 +71,13 @@ properties:
     enum: [2000, 4000, 8000, 12000, 16000, 20000, 24000, 28000]
     default: 8000
 
+  microchip,sgmii-mode:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      SGMII mode to use for the SGMII port
+    enum: [0, 1, 2]
+    default: 1
+
   interrupts:
     maxItems: 1
 
@@ -137,6 +144,7 @@ examples:
             compatible = "microchip,ksz9477";
             reg = <0>;
             reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+            sgmii-mode = <1>;
 
             spi-max-frequency = <44000000>;
 
@@ -173,6 +181,10 @@ examples:
                         full-duplex;
                     };
                 };
+                port@6 {
+                    reg = <6>;
+                    label = "lan6";
+                };
             };
         };
 
-- 
2.34.1

[PATCH net-next 2/4] net: dsa: microchip: support global switch interrupt in KSZ DSA driver

[Tristram.Ha]

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

The KSZ9477 DSA driver outsources interrupt handling to other drivers
such as PHY, but the switch driver still needs to handle some interrupts
itself.  This patch prepares the common code to allow specific switch
drivers to handle all switch interrupts.

The port interrupt handling uses the same function as used by the global
switch interrupt, so the port variable is used to differentiate that.

Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 15 ++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  5 ++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 1491099528be..f328c97f27d1 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-2019 Microchip Technology Inc.
+ * Copyright (C) 2017-2024 Microchip Technology Inc.
  */
 
 #include <linux/delay.h>
@@ -2257,6 +2257,12 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id)
 	if (ret)
 		goto out;
 
+	if (dev->dev_ops->handle_irq) {
+		ret = dev->dev_ops->handle_irq(dev, kirq->port, &data);
+		if (ret == IRQ_HANDLED)
+			++nhandled;
+	}
+
 	for (n = 0; n < kirq->nirqs; ++n) {
 		if (data & BIT(n)) {
 			sub_irq = irq_find_mapping(kirq->domain, n);
@@ -2300,6 +2306,7 @@ static int ksz_girq_setup(struct ksz_device *dev)
 {
 	struct ksz_irq *girq = &dev->girq;
 
+	girq->port = 0;
 	girq->nirqs = dev->info->port_cnt;
 	girq->reg_mask = REG_SW_PORT_INT_MASK__1;
 	girq->reg_status = REG_SW_PORT_INT_STATUS__1;
@@ -2314,6 +2321,7 @@ static int ksz_pirq_setup(struct ksz_device *dev, u8 p)
 {
 	struct ksz_irq *pirq = &dev->ports[p].pirq;
 
+	pirq->port = p + 1;
 	pirq->nirqs = dev->info->port_nirqs;
 	pirq->reg_mask = dev->dev_ops->get_port_addr(p, REG_PORT_INT_MASK);
 	pirq->reg_status = dev->dev_ops->get_port_addr(p, REG_PORT_INT_STATUS);
@@ -2419,6 +2427,11 @@ static int ksz_setup(struct dsa_switch *ds)
 	if (ret)
 		goto out_ptp_clock_unregister;
 
+	if (dev->irq > 0) {
+		if (dev->dev_ops->enable_irq)
+			dev->dev_ops->enable_irq(dev);
+	}
+
 	/* start switch */
 	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
 			   SW_START, SW_START);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 5f0a628b9849..a2547646026f 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-2019 Microchip Technology Inc.
+ * Copyright (C) 2017-2024 Microchip Technology Inc.
  */
 
 #ifndef __KSZ_COMMON_H
@@ -99,6 +99,7 @@ struct ksz_irq {
 	int irq_num;
 	char name[16];
 	struct ksz_device *dev;
+	u8 port;
 };
 
 struct ksz_ptp_irq {
@@ -373,6 +374,8 @@ struct ksz_dev_ops {
 	int (*reset)(struct ksz_device *dev);
 	int (*init)(struct ksz_device *dev);
 	void (*exit)(struct ksz_device *dev);
+	void (*enable_irq)(struct ksz_device *dev);
+	irqreturn_t (*handle_irq)(struct ksz_device *dev, u8 port, u8 *data);
 };
 
 struct ksz_device *ksz_switch_alloc(struct device *base, void *priv);
-- 
2.34.1

[PATCH net-next 3/4] net: dsa: microchip: handle most interrupts in KSZ9477/KSZ9893 switch families

[Tristram.Ha]

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

The KSZ9477 switch driver can handle most interrupts.  It enables address
learning fail interrupt as SQA would like to see such notification during
testing.

Input timestamp interrupt is not implemented yet as that interrupt is
related to PTP operation and so will be handled by the PTP driver.

Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c     | 64 ++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz9477.h     |  4 +-
 drivers/net/dsa/microchip/ksz9477_reg.h |  5 +-
 drivers/net/dsa/microchip/ksz_common.c  |  2 +
 4 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 425e20daf1e9..518ba4a1e34b 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-2019 Microchip Technology Inc.
+ * Copyright (C) 2017-2024 Microchip Technology Inc.
  */
 
 #include <linux/kernel.h>
@@ -1487,6 +1487,68 @@ void ksz9477_switch_exit(struct ksz_device *dev)
 	ksz9477_reset_switch(dev);
 }
 
+static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
+					   u8 *data)
+{
+	struct dsa_switch *ds = dev->ds;
+	struct phy_device *phydev;
+	int cnt = 0;
+
+	phydev = mdiobus_get_phy(ds->user_mii_bus, port);
+	if (*data & PORT_PHY_INT) {
+		/* Handle the interrupt if there is no PHY device or its
+		 * interrupt is not registered yet.
+		 */
+		if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
+			u8 phy_status;
+
+			ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
+				   &phy_status);
+			if (phydev)
+				phy_trigger_machine(phydev);
+			++cnt;
+			*data &= ~PORT_PHY_INT;
+		}
+	}
+	if (*data & PORT_ACL_INT) {
+		ksz_pwrite8(dev, port, REG_PORT_INT_STATUS, PORT_ACL_INT);
+		++cnt;
+		*data &= ~PORT_ACL_INT;
+	}
+
+	return (cnt > 0) ? IRQ_HANDLED : IRQ_NONE;
+}
+
+void ksz9477_enable_irq(struct ksz_device *dev)
+{
+	regmap_update_bits(ksz_regmap_32(dev), REG_SW_INT_MASK__4, LUE_INT, 0);
+	ksz_write8(dev, REG_SW_LUE_INT_ENABLE, LEARN_FAIL_INT | WRITE_FAIL_INT);
+}
+
+irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data)
+{
+	irqreturn_t ret = IRQ_NONE;
+	u32 data32;
+
+	if (port > 0)
+		return ksz9477_handle_port_irq(dev, port - 1, data);
+
+	ksz_read32(dev, REG_SW_INT_STATUS__4, &data32);
+	if (data32 & LUE_INT) {
+		u8 lue;
+
+		ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue);
+		ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue);
+		if (lue & LEARN_FAIL_INT)
+			dev_info_ratelimited(dev->dev, "lue learn fail\n");
+		if (lue & WRITE_FAIL_INT)
+			dev_info_ratelimited(dev->dev, "lue write fail\n");
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
 MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>");
 MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch DSA Driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index 239a281da10b..51252d0d0774 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-2024 Microchip Technology Inc.
  */
 
 #ifndef __KSZ9477_H
@@ -58,6 +58,8 @@ int ksz9477_reset_switch(struct ksz_device *dev);
 int ksz9477_switch_init(struct ksz_device *dev);
 void ksz9477_switch_exit(struct ksz_device *dev);
 void ksz9477_port_queue_split(struct ksz_device *dev, int port);
+void ksz9477_enable_irq(struct ksz_device *dev);
+irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data);
 void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr);
 void ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device *hsr);
 void ksz9477_get_wol(struct ksz_device *dev, int port,
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index d5354c600ea1..da4ef3eb97c7 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -2,7 +2,7 @@
 /*
  * Microchip KSZ9477 register definitions
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2024 Microchip Technology Inc.
  */
 
 #ifndef __KSZ9477_REGS_H
@@ -75,7 +75,8 @@
 #define TRIG_TS_INT			BIT(30)
 #define APB_TIMEOUT_INT			BIT(29)
 
-#define SWITCH_INT_MASK			(TRIG_TS_INT | APB_TIMEOUT_INT)
+#define SWITCH_INT_MASK			\
+	(LUE_INT | TRIG_TS_INT | APB_TIMEOUT_INT)
 
 #define REG_SW_PORT_INT_STATUS__4	0x0018
 #define REG_SW_PORT_INT_MASK__4		0x001C
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index f328c97f27d1..7db74e036c3f 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -357,6 +357,8 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
 	.reset = ksz9477_reset_switch,
 	.init = ksz9477_switch_init,
 	.exit = ksz9477_switch_exit,
+	.enable_irq = ksz9477_enable_irq,
+	.handle_irq = ksz9477_handle_irq,
 };
 
 static const struct phylink_mac_ops lan937x_phylink_mac_ops = {
-- 
2.34.1

[PATCH net-next 4/4] 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: 0 for direct
connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.

SFP is typically used so the default is 1.  The driver can detect
10/100/1000 SFP and change the mode to 2.  For direct connect this mode
has to be explicitly set to 0 as driver cannot detect that
configuration.

The SGMII module can only report basic link status of the SFP, so it is
simulated as a regular internal PHY.

Since the regular PHY in the switch uses interrupt instead of polling the
driver has to handle the SGMII interrupt indicating link on/off.

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

Recent change in the DSA operation can setup the port before the PHY
interrupt handling function is registered.  As the SGMII interrupt can
be triggered after port setup there is extra code in the interrupt
processing to handle this situation.  Otherwise a kernel fault can be
triggered.

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 the interrupt is not triggered prematurely.

Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c     | 340 +++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz9477.h     |   2 +
 drivers/net/dsa/microchip/ksz9477_reg.h |   5 +
 drivers/net/dsa/microchip/ksz_common.c  |  28 +-
 drivers/net/dsa/microchip/ksz_common.h  |   6 +
 5 files changed, 376 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 518ba4a1e34b..6ac0a06a4b74 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -342,6 +342,257 @@ 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 & PORT_SGMII_DEVICE_ID_M;
+	data <<= PORT_SGMII_DEVICE_ID_S;
+	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;
+
+	port_sgmii_s(dev, port, devid, reg, len);
+	while (len) {
+		ksz_pread32(dev, port, REG_PORT_SGMII_DATA__4, &data);
+		*buf++ = (u16)data;
+		len--;
+	}
+}
+
+static void port_sgmii_w(struct ksz_device *dev, uint port, u16 devid, u16 reg,
+			 u16 *buf, u16 len)
+{
+	u32 data;
+
+	port_sgmii_s(dev, port, devid, reg, len);
+	while (len) {
+		data = *buf++;
+		ksz_pwrite32(dev, port, REG_PORT_SGMII_DATA__4, data);
+		len--;
+	}
+}
+
+static int port_sgmii_detect(struct ksz_device *dev, uint p)
+{
+	int ret = 0;
+
+	if (dev->sgmii_mode) {
+		struct ksz_port *port = &dev->ports[p];
+		u16 buf[6];
+
+		port_sgmii_r(dev, p, SR_MII, 0, buf, 6);
+		if (buf[5] & SR_MII_REMOTE_ACK) {
+			if (buf[5] & (SR_MII_REMOTE_HALF_DUPLEX |
+				      SR_MII_REMOTE_FULL_DUPLEX))
+				port->fiber = 1;
+			else if (dev->sgmii_mode == 1)
+				dev->sgmii_mode = 2;
+			ret = 1;
+		} else if (dev->sgmii_mode == 1) {
+			port->fiber = 1;
+			ret = 1;
+		}
+	} else {
+		/* Need to be told to run in direct mode. */
+		ret = 1;
+	}
+	return ret;
+}
+
+static void port_sgmii_reset(struct ksz_device *dev, uint p)
+{
+	u16 ctrl;
+
+	port_sgmii_r(dev, p, SR_MII, MMD_SR_MII_CTRL, &ctrl, 1);
+	ctrl |= SR_MII_RESET;
+	port_sgmii_w(dev, p, SR_MII, MMD_SR_MII_CTRL, &ctrl, 1);
+}
+
+static void port_sgmii_setup(struct ksz_device *dev, uint p, bool pcs,
+			     bool master, bool autoneg, int speed, int duplex)
+{
+	u16 ctrl;
+	u16 cfg;
+	u16 adv;
+
+	/* SGMII registers are not changed by reset. */
+	port_sgmii_r(dev, p, SR_MII, MMD_SR_MII_AUTO_NEG_CTRL, &cfg, 1);
+	if (cfg & SR_MII_AUTO_NEG_COMPLETE_INTR)
+		return;
+	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;
+	}
+	cfg |= SR_MII_AUTO_NEG_COMPLETE_INTR;
+	port_sgmii_w(dev, p, SR_MII, MMD_SR_MII_AUTO_NEG_CTRL, &cfg, 1);
+	port_sgmii_r(dev, p, SR_MII, MMD_SR_MII_CTRL, &ctrl, 1);
+	if (master || !autoneg) {
+		switch (speed) {
+		case 1:
+			ctrl |= SR_MII_SPEED_100MBIT;
+			break;
+		case 2:
+			ctrl |= SR_MII_SPEED_1000MBIT;
+			break;
+		}
+	}
+	if (!autoneg) {
+		ctrl &= ~SR_MII_AUTO_NEG_ENABLE;
+		port_sgmii_w(dev, p, SR_MII, MMD_SR_MII_CTRL, &ctrl, 1);
+		return;
+	} else if (!(ctrl & SR_MII_AUTO_NEG_ENABLE)) {
+		ctrl |= SR_MII_AUTO_NEG_ENABLE;
+		port_sgmii_w(dev, p, SR_MII, MMD_SR_MII_CTRL, &ctrl, 1);
+	}
+
+	/* Need to write to advertise register to send correct signal. */
+	/* Default value is 0x0020. */
+	port_sgmii_r(dev, p, SR_MII, MMD_SR_MII_AUTO_NEGOTIATION, &adv, 1);
+	adv = SR_MII_AUTO_NEG_ASYM_PAUSE_RX << SR_MII_AUTO_NEG_PAUSE_S;
+	if (duplex)
+		adv |= SR_MII_AUTO_NEG_FULL_DUPLEX;
+	else
+		adv |= SR_MII_AUTO_NEG_HALF_DUPLEX;
+	port_sgmii_w(dev, p, SR_MII, MMD_SR_MII_AUTO_NEGOTIATION, &adv, 1);
+	if (master && autoneg) {
+		ctrl |= SR_MII_AUTO_NEG_RESTART;
+		port_sgmii_w(dev, p, SR_MII, MMD_SR_MII_CTRL, &ctrl, 1);
+	}
+}
+
+static int sgmii_port_get_speed(struct ksz_device *dev, uint p)
+{
+	struct ksz_port *info = &dev->ports[p];
+	int ret = 0;
+	u16 status;
+	u16 speed;
+	u16 data;
+	u8 link;
+
+	port_sgmii_r(dev, p, SR_MII, MMD_SR_MII_STATUS, &status, 1);
+	port_sgmii_r(dev, p, SR_MII, MMD_SR_MII_STATUS, &status, 1);
+	port_sgmii_r(dev, p, SR_MII, MMD_SR_MII_AUTO_NEG_STATUS, &data, 1);
+
+	/* Typical register values in different modes.
+	 * 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 = 0000
+	 *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0000
+	 * fiber:       1f0001 = 0189  1f0005 = 0000  1f8002 = 0000
+	 *              1f0001 = 01ad  1f0005 = 41a0  1f8002 = 0000
+	 */
+
+	/* Running in fiber mode. */
+	if (info->fiber && !data &&
+	    (status & (PORT_AUTO_NEG_ACKNOWLEDGE | PORT_LINK_STATUS)) ==
+	    (PORT_AUTO_NEG_ACKNOWLEDGE | PORT_LINK_STATUS)) {
+		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 = 1;
+
+	link = (data & ~SR_MII_AUTO_NEG_COMPLETE_INTR);
+	if (info->sgmii_link == link)
+		return ret;
+
+	if (data & SR_MII_STAT_LINK_UP) {
+		u16 ctrl;
+
+		/* Need to update control register with same link setting. */
+		ctrl = SR_MII_AUTO_NEG_ENABLE;
+		speed = (data >> SR_MII_STAT_S) & SR_MII_STAT_M;
+		if (speed == SR_MII_STAT_1000_MBPS)
+			ctrl |= SR_MII_SPEED_1000MBIT;
+		else if (speed == SR_MII_STAT_100_MBPS)
+			ctrl |= SR_MII_SPEED_100MBIT;
+		if (data & SR_MII_STAT_FULL_DUPLEX)
+			ctrl |= SR_MII_FULL_DUPLEX;
+		port_sgmii_w(dev, p, SR_MII, MMD_SR_MII_CTRL, &ctrl, 1);
+
+		speed = (data >> SR_MII_STAT_S) & SR_MII_STAT_M;
+		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 |= 2;
+	info->sgmii_link = link;
+	info->phydev.link = (ret & 1);
+	return ret;
+}
+
+static void sgmii_check_work(struct work_struct *work)
+{
+	struct ksz_device *dev = container_of(work, struct ksz_device,
+					      sgmii_check.work);
+	struct ksz_port *p = &dev->ports[KSZ9477_SGMII_PORT];
+
+	if (p->sgmii && p->phydev.link) {
+		int ret = sgmii_port_get_speed(dev, KSZ9477_SGMII_PORT);
+		struct dsa_switch *ds = dev->ds;
+		struct phy_device *phydev;
+
+		phydev = mdiobus_get_phy(ds->user_mii_bus, KSZ9477_SGMII_PORT);
+		if ((ret & 2) && phydev)
+			phy_trigger_machine(phydev);
+		if (p->phydev.link)
+			schedule_delayed_work(&dev->sgmii_check,
+					      msecs_to_jiffies(500));
+	}
+}
+
+static void sgmii_initial_setup(struct ksz_device *dev, int port)
+{
+	struct ksz_port *p = &dev->ports[port];
+	/* Assume SGMII mode is 2. */
+	bool master = false;
+	bool autoneg = true;
+	bool pcs = true;
+
+	if (!p->sgmii || p->sgmii_setup)
+		return;
+
+	INIT_DELAYED_WORK(&dev->sgmii_check, sgmii_check_work);
+	if (dev->sgmii_mode == 0) {
+		master = true;
+		autoneg = false;
+	} else if (dev->sgmii_mode == 1) {
+		pcs = false;
+		master = true;
+	}
+	port_sgmii_setup(dev, port, pcs, master, autoneg, 2, 1);
+
+	/* Make invalid so the correct value is set. */
+	p->sgmii_link = 0xff;
+	p->sgmii_setup = 1;
+	sgmii_port_get_speed(dev, port);
+
+	/* Need to check link down if using fiber SFP. */
+	if (dev->sgmii_mode == 1 && p->phydev.link)
+		schedule_delayed_work(&dev->sgmii_check,
+				      msecs_to_jiffies(1000));
+}
+
 int ksz9477_reset_switch(struct ksz_device *dev)
 {
 	u8 data8;
@@ -354,6 +605,14 @@ int ksz9477_reset_switch(struct ksz_device *dev)
 	regmap_update_bits(ksz_regmap_8(dev), REG_SW_GLOBAL_SERIAL_CTRL_0,
 			   SPI_AUTO_EDGE_DETECTION, 0);
 
+	/* Only reset SGMII module when the driver is stopped. */
+	if (dev->chip_id == KSZ9477_CHIP_ID) {
+		struct ksz_port *p = &dev->ports[KSZ9477_SGMII_PORT];
+
+		if (p->sgmii_setup)
+			port_sgmii_reset(dev, KSZ9477_SGMII_PORT);
+	}
+
 	/* default configuration */
 	ksz_write8(dev, REG_SW_LUE_CTRL_1,
 		   SW_AGING_ENABLE | SW_LINK_AUTO_AGING | SW_SRC_ADDR_FILTER);
@@ -510,7 +769,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];
@@ -520,7 +779,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;
@@ -533,6 +795,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;
@@ -543,6 +809,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);
@@ -1143,6 +1427,16 @@ void ksz9477_get_caps(struct ksz_device *dev, int port,
 
 	if (dev->info->gbit_capable[port])
 		config->mac_capabilities |= MAC_1000FD;
+	if (dev->info->supports_sgmii[port]) {
+		struct dsa_switch *ds = dev->ds;
+		struct phy_device *phydev;
+
+		phydev = mdiobus_get_phy(ds->user_mii_bus, port);
+
+		/* Change this port interface to SGMII. */
+		if (phydev)
+			phydev->interface = PHY_INTERFACE_MODE_SGMII;
+	}
 }
 
 int ksz9477_set_ageing_time(struct ksz_device *dev, unsigned int msecs)
@@ -1218,6 +1512,8 @@ 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]);
 
+	sgmii_initial_setup(dev, port);
+
 	if (cpu_port)
 		member = dsa_user_ports(ds);
 	else
@@ -1296,6 +1592,10 @@ void ksz9477_config_cpu_port(struct dsa_switch *ds)
 			continue;
 		ksz_port_stp_state_set(ds, i, BR_STATE_DISABLED);
 	}
+	if (dev->chip_id == KSZ9477_CHIP_ID) {
+		/* Switch reset does not reset SGMII module. */
+		port_sgmii_reset(dev, KSZ9477_SGMII_PORT);
+	}
 }
 
 int ksz9477_enable_stp_addr(struct ksz_device *dev)
@@ -1370,6 +1670,12 @@ int ksz9477_setup(struct dsa_switch *ds)
 	 */
 	ksz_write8(dev, REG_SW_PME_CTRL, 0);
 
+	if (dev->chip_id == KSZ9477_CHIP_ID) {
+		struct ksz_port *p = &dev->ports[KSZ9477_SGMII_PORT];
+
+		p->sgmii = port_sgmii_detect(dev, KSZ9477_SGMII_PORT);
+	}
+
 	return 0;
 }
 
@@ -1484,6 +1790,8 @@ int ksz9477_switch_init(struct ksz_device *dev)
 
 void ksz9477_switch_exit(struct ksz_device *dev)
 {
+	if (delayed_work_pending(&dev->sgmii_check))
+		cancel_delayed_work_sync(&dev->sgmii_check);
 	ksz9477_reset_switch(dev);
 }
 
@@ -1515,6 +1823,34 @@ static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
 		++cnt;
 		*data &= ~PORT_ACL_INT;
 	}
+	if (*data & PORT_SGMII_INT) {
+		u16 data16 = 0;
+		int ret;
+
+		port_sgmii_w(dev, port, SR_MII, MMD_SR_MII_AUTO_NEG_STATUS,
+			     &data16, 1);
+		ret = sgmii_port_get_speed(dev, port);
+		if ((ret & 2)) {
+			struct ksz_port *p = &dev->ports[port];
+
+			p->phydev.interrupts = PHY_INTERRUPT_ENABLED;
+
+			/* No interrupt for link down. */
+			if (dev->sgmii_mode == 1 && p->phydev.link)
+				schedule_delayed_work(&dev->sgmii_check,
+						      msecs_to_jiffies(500));
+		}
+
+		/* Handle the interrupt if there is no PHY device or its
+		 * interrupt is not registered yet.
+		 */
+		if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
+			if (phydev)
+				phy_trigger_machine(phydev);
+			++cnt;
+			*data &= ~PORT_SGMII_INT;
+		}
+	}
 
 	return (cnt > 0) ? IRQ_HANDLED : IRQ_NONE;
 }
diff --git a/drivers/net/dsa/microchip/ksz9477.h b/drivers/net/dsa/microchip/ksz9477.h
index 51252d0d0774..ec318491d536 100644
--- a/drivers/net/dsa/microchip/ksz9477.h
+++ b/drivers/net/dsa/microchip/ksz9477.h
@@ -11,6 +11,8 @@
 #include <net/dsa.h>
 #include "ksz_common.h"
 
+#define KSZ9477_SGMII_PORT		6
+
 int ksz9477_setup(struct dsa_switch *ds);
 u32 ksz9477_get_port_addr(int port, int offset);
 void ksz9477_cfg_port_member(struct ksz_device *dev, int port, u8 member);
diff --git a/drivers/net/dsa/microchip/ksz9477_reg.h b/drivers/net/dsa/microchip/ksz9477_reg.h
index da4ef3eb97c7..20e0b233d55d 100644
--- a/drivers/net/dsa/microchip/ksz9477_reg.h
+++ b/drivers/net/dsa/microchip/ksz9477_reg.h
@@ -1038,6 +1038,11 @@
 #define SR_MII_AUTO_NEG_FULL_DUPLEX	BIT(5)
 
 #define MMD_SR_MII_REMOTE_CAPABILITY	0x0005
+
+#define SR_MII_REMOTE_ACK		BIT(14)
+#define SR_MII_REMOTE_HALF_DUPLEX	BIT(6)
+#define SR_MII_REMOTE_FULL_DUPLEX	BIT(5)
+
 #define MMD_SR_MII_AUTO_NEG_EXP		0x0006
 #define MMD_SR_MII_AUTO_NEG_EXT		0x000F
 
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7db74e036c3f..c16e3388dc1a 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -936,8 +936,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),
@@ -1399,6 +1398,8 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 				   false, true, false},
 		.supports_rgmii = {false, false, false, false,
 				   false, true, false},
+		.supports_sgmii = {false, false, false, false,
+				   false, false, true},
 		.internal_phy	= {true, true, true, true,
 				   true, false, false},
 		.gbit_capable	= {true, true, true, true, true, true, true},
@@ -1806,6 +1807,10 @@ static void ksz_phylink_get_caps(struct dsa_switch *ds, int port,
 	if (dev->info->supports_rgmii[port])
 		phy_interface_set_rgmii(config->supported_interfaces);
 
+	if (dev->info->supports_sgmii[port])
+		__set_bit(PHY_INTERFACE_MODE_SGMII,
+			  config->supported_interfaces);
+
 	if (dev->info->internal_phy[port]) {
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
@@ -2089,14 +2094,19 @@ static int ksz_sw_mdio_write(struct mii_bus *bus, int addr, int regnum,
 static int ksz_irq_phy_setup(struct ksz_device *dev)
 {
 	struct dsa_switch *ds = dev->ds;
+	struct ksz_port *p;
 	int phy;
 	int irq;
 	int ret;
 
 	for (phy = 0; phy < KSZ_MAX_NUM_PORTS; phy++) {
 		if (BIT(phy) & ds->phys_mii_mask) {
+			p = &dev->ports[phy];
+			irq = PORT_SRC_PHY_INT;
+			if (p->sgmii)
+				irq = 3;
 			irq = irq_find_mapping(dev->ports[phy].pirq.domain,
-					       PORT_SRC_PHY_INT);
+					       irq);
 			if (irq < 0) {
 				ret = irq;
 				goto out;
@@ -3222,6 +3232,9 @@ static void ksz_phylink_mac_config(struct phylink_config *config,
 		return;
 	}
 
+	if (state->interface == PHY_INTERFACE_MODE_SGMII)
+		return;
+
 	ksz_set_xmii(dev, port, state->interface);
 
 	if (dev->dev_ops->setup_rgmii_delay)
@@ -4456,9 +4469,18 @@ int ksz_switch_register(struct ksz_device *dev)
 	for (port_num = 0; port_num < dev->info->port_cnt; ++port_num)
 		dev->ports[port_num].interface = PHY_INTERFACE_MODE_NA;
 	if (dev->dev->of_node) {
+		u32 mode;
+
 		ret = of_get_phy_mode(dev->dev->of_node, &interface);
 		if (ret == 0)
 			dev->compat_interface = interface;
+
+		dev->sgmii_mode = 1;
+		ret = of_property_read_u32(dev->dev->of_node,
+					   "microchip,sgmii-mode", &mode);
+		if (ret == 0 && mode <= 2)
+			dev->sgmii_mode = (u8)mode;
+
 		ports = of_get_child_by_name(dev->dev->of_node, "ethernet-ports");
 		if (!ports)
 			ports = of_get_child_by_name(dev->dev->of_node, "ports");
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index a2547646026f..e6cb9304e7db 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -84,6 +84,7 @@ struct ksz_chip_data {
 	bool supports_mii[KSZ_MAX_NUM_PORTS];
 	bool supports_rmii[KSZ_MAX_NUM_PORTS];
 	bool supports_rgmii[KSZ_MAX_NUM_PORTS];
+	bool supports_sgmii[KSZ_MAX_NUM_PORTS];
 	bool internal_phy[KSZ_MAX_NUM_PORTS];
 	bool gbit_capable[KSZ_MAX_NUM_PORTS];
 	const struct regmap_access_table *wr_table;
@@ -126,6 +127,9 @@ struct ksz_port {
 	u32 force:1;
 	u32 read:1;			/* read MIB counters in background */
 	u32 freeze:1;			/* MIB counter freeze is enabled */
+	u32 sgmii:1;			/* port is SGMII */
+	u32 sgmii_link:8;
+	u32 sgmii_setup:1;
 
 	struct ksz_port_mib mib;
 	phy_interface_t interface;
@@ -181,6 +185,8 @@ struct ksz_device {
 	struct ksz_port *ports;
 	struct delayed_work mib_read;
 	unsigned long mib_read_interval;
+	struct delayed_work sgmii_check;
+	u8 sgmii_mode;
 	u16 mirror_rx;
 	u16 mirror_tx;
 	u16 port_mask;
-- 
2.34.1

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Rob Herring (Arm)]

On Fri, 09 Aug 2024 16:38:37 -0700, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> 
> SFP is typically used so the default is 1.  The driver can detect
> 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> has to be explicitly set to 0 as driver cannot detect that
> configuration.
> 
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> ---
>  .../devicetree/bindings/net/dsa/microchip,ksz.yaml   | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/dsa/microchip,ksz.example.dtb: switch@0: Unevaluated properties are not allowed ('sgmii-mode' was unexpected)
	from schema $id: http://devicetree.org/schemas/net/dsa/microchip,ksz.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240809233840.59953-2-Tristram.Ha@microchip.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Krzysztof Kozlowski]

On 10/08/2024 01:38, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.

Binding should say it, not commit msg. But aren't you duplicating
something like phy-connection-type?
	
> 
> SFP is typically used so the default is 1.  The driver can detect
> 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> has to be explicitly set to 0 as driver cannot detect that
> configuration.
> 
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> ---
>  .../devicetree/bindings/net/dsa/microchip,ksz.yaml   | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> index 52acc15ebcbf..b4a9746556bf 100644
> --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml
> @@ -71,6 +71,13 @@ properties:
>      enum: [2000, 4000, 8000, 12000, 16000, 20000, 24000, 28000]
>      default: 8000
>  
> +  microchip,sgmii-mode:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      SGMII mode to use for the SGMII port
> +    enum: [0, 1, 2]
> +    default: 1
> +
>    interrupts:
>      maxItems: 1
>  
> @@ -137,6 +144,7 @@ examples:
>              compatible = "microchip,ksz9477";
>              reg = <0>;
>              reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> +            sgmii-mode = <1>;

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.


Best regards,
Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Andrew Lunn]

On Fri, Aug 09, 2024 at 04:38:37PM -0700, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> 
> SFP is typically used so the default is 1.  The driver can detect
> 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> has to be explicitly set to 0 as driver cannot detect that
> configuration.

Could you explain this in more detail. Other SGMII blocks don't need
this. Why is this block special?

Has this anything to do with in-band signalling?

    Andrew

RE: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts in KSZ9477/KSZ9893 switch families

[Sai Krishna Gajula]

> -----Original Message-----
> From: Tristram.Ha@microchip.com <Tristram.Ha@microchip.com>
> Sent: Saturday, August 10, 2024 5:09 AM
> To: Woojung Huh <woojung.huh@microchip.com>;
> UNGLinuxDriver@microchip.com; devicetree@vger.kernel.org; Andrew Lunn
> <andrew@lunn.ch>; Florian Fainelli <f.fainelli@gmail.com>; Vladimir Oltean
> <olteanv@gmail.com>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski
> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; Marek Vasut <marex@denx.de>;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Tristram Ha
> <tristram.ha@microchip.com>
> Subject: [PATCH net-next 3/4] net: dsa: microchip: handle most
> interrupts in KSZ9477/KSZ9893 switch families
> 
> From: Tristram Ha <tristram. ha@ microchip. com> The KSZ9477 switch driver
> can handle most interrupts. It enables address learning fail interrupt as SQA
> would like to see such notification during testing. Input timestamp interrupt is
> not 
> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The KSZ9477 switch driver can handle most interrupts.  It enables address
> learning fail interrupt as SQA would like to see such notification during
> testing.
> 
> Input timestamp interrupt is not implemented yet as that interrupt is related
> to PTP operation and so will be handled by the PTP driver.
> 
> Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c     | 64 ++++++++++++++++++++++++-
>  drivers/net/dsa/microchip/ksz9477.h     |  4 +-
>  drivers/net/dsa/microchip/ksz9477_reg.h |  5 +-
> drivers/net/dsa/microchip/ksz_common.c  |  2 +
>  4 files changed, 71 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c
> b/drivers/net/dsa/microchip/ksz9477.c
> index 425e20daf1e9..518ba4a1e34b 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-2019 Microchip Technology Inc.
> + * Copyright (C) 2017-2024 Microchip Technology Inc.
>   */
> 
>  #include <linux/kernel.h>
> @@ -1487,6 +1487,68 @@ void ksz9477_switch_exit(struct ksz_device *dev)
>  	ksz9477_reset_switch(dev);
>  }
> 
> +static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
> +					   u8 *data)
> +{
> +	struct dsa_switch *ds = dev->ds;
> +	struct phy_device *phydev;
> +	int cnt = 0;
> +
> +	phydev = mdiobus_get_phy(ds->user_mii_bus, port);
> +	if (*data & PORT_PHY_INT) {
> +		/* Handle the interrupt if there is no PHY device or its
> +		 * interrupt is not registered yet.
> +		 */
> +		if (!phydev || phydev->interrupts !=
> PHY_INTERRUPT_ENABLED) {
> +			u8 phy_status;
> +
> +			ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
> +				   &phy_status);
> +			if (phydev)
> +				phy_trigger_machine(phydev);
> +			++cnt;
> +			*data &= ~PORT_PHY_INT;
> +		}
> +	}
> +	if (*data & PORT_ACL_INT) {
> +		ksz_pwrite8(dev, port, REG_PORT_INT_STATUS,
> PORT_ACL_INT);
> +		++cnt;
> +		*data &= ~PORT_ACL_INT;
> +	}
> +
> +	return (cnt > 0) ? IRQ_HANDLED : IRQ_NONE; }

Until unless there is a need to service both PHY, ACL interrupts simultaneously, "cnt" increment operations can be avoided like this to reduce processing time.

if (*data & PORT_PHY_INT) {
    // Handle PORT_PHY_INT
    return IRQ_HANDLED;
}

if (*data & PORT_ACL_INT) {
    // Handle PORT_ACL_INT
    return IRQ_HANDLED;
}

return IRQ_NONE;
 
> +
> +void ksz9477_enable_irq(struct ksz_device *dev) {
> +	regmap_update_bits(ksz_regmap_32(dev), REG_SW_INT_MASK__4,
> LUE_INT, 0);
> +	ksz_write8(dev, REG_SW_LUE_INT_ENABLE, LEARN_FAIL_INT |
> +WRITE_FAIL_INT); }
> +
> +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8
> +*data) {
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 data32;
> +
> +	if (port > 0)
> +		return ksz9477_handle_port_irq(dev, port - 1, data);
> +
> +	ksz_read32(dev, REG_SW_INT_STATUS__4, &data32);
> +	if (data32 & LUE_INT) {
> +		u8 lue;
> +
> +		ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue);
> +		ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue);
> +		if (lue & LEARN_FAIL_INT)
> +			dev_info_ratelimited(dev->dev, "lue learn fail\n");
> +		if (lue & WRITE_FAIL_INT)
> +			dev_info_ratelimited(dev->dev, "lue write fail\n");
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	return ret;
> +}
> +
>  MODULE_AUTHOR("Woojung Huh <Woojung.Huh@microchip.com>");
> MODULE_DESCRIPTION("Microchip KSZ9477 Series Switch DSA Driver");
> MODULE_LICENSE("GPL"); diff --git a/drivers/net/dsa/microchip/ksz9477.h
> b/drivers/net/dsa/microchip/ksz9477.h
> index 239a281da10b..51252d0d0774 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-2024 Microchip Technology Inc.
>   */
> 
>  #ifndef __KSZ9477_H
> @@ -58,6 +58,8 @@ int ksz9477_reset_switch(struct ksz_device *dev);  int
> ksz9477_switch_init(struct ksz_device *dev);  void ksz9477_switch_exit(struct
> ksz_device *dev);  void ksz9477_port_queue_split(struct ksz_device *dev, int
> port);
> +void ksz9477_enable_irq(struct ksz_device *dev); irqreturn_t
> +ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data);
>  void ksz9477_hsr_join(struct dsa_switch *ds, int port, struct net_device *hsr);
> void ksz9477_hsr_leave(struct dsa_switch *ds, int port, struct net_device
> *hsr);  void ksz9477_get_wol(struct ksz_device *dev, int port, diff --git
> a/drivers/net/dsa/microchip/ksz9477_reg.h
> b/drivers/net/dsa/microchip/ksz9477_reg.h
> index d5354c600ea1..da4ef3eb97c7 100644
> --- a/drivers/net/dsa/microchip/ksz9477_reg.h
> +++ b/drivers/net/dsa/microchip/ksz9477_reg.h
> @@ -2,7 +2,7 @@
>  /*
>   * Microchip KSZ9477 register definitions
>   *
> - * Copyright (C) 2017-2018 Microchip Technology Inc.
> + * Copyright (C) 2017-2024 Microchip Technology Inc.
>   */
> 
>  #ifndef __KSZ9477_REGS_H
> @@ -75,7 +75,8 @@
>  #define TRIG_TS_INT			BIT(30)
>  #define APB_TIMEOUT_INT			BIT(29)
> 
> -#define SWITCH_INT_MASK			(TRIG_TS_INT |
> APB_TIMEOUT_INT)
> +#define SWITCH_INT_MASK			\
> +	(LUE_INT | TRIG_TS_INT | APB_TIMEOUT_INT)
> 
>  #define REG_SW_PORT_INT_STATUS__4	0x0018
>  #define REG_SW_PORT_INT_MASK__4		0x001C
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index f328c97f27d1..7db74e036c3f 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -357,6 +357,8 @@ static const struct ksz_dev_ops ksz9477_dev_ops = {
>  	.reset = ksz9477_reset_switch,
>  	.init = ksz9477_switch_init,
>  	.exit = ksz9477_switch_exit,
> +	.enable_irq = ksz9477_enable_irq,
> +	.handle_irq = ksz9477_handle_irq,
>  };
> 
>  static const struct phylink_mac_ops lan937x_phylink_mac_ops = {
> --
> 2.34.1
> 

Re: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts in KSZ9477/KSZ9893 switch families

[Andrew Lunn]

> +static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
> +					   u8 *data)
> +{
> +	struct dsa_switch *ds = dev->ds;
> +	struct phy_device *phydev;
> +	int cnt = 0;
> +
> +	phydev = mdiobus_get_phy(ds->user_mii_bus, port);
> +	if (*data & PORT_PHY_INT) {
> +		/* Handle the interrupt if there is no PHY device or its
> +		 * interrupt is not registered yet.
> +		 */
> +		if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
> +			u8 phy_status;
> +
> +			ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
> +				   &phy_status);
> +			if (phydev)
> +				phy_trigger_machine(phydev);
> +			++cnt;
> +			*data &= ~PORT_PHY_INT;
> +		}
> +	}

This looks like a layering violation. Why is this needed? An interrupt
controller generally has no idea what the individual interrupt is
about. It just calls into the interrupt core to get the handler
called, and then clears the interrupt. Why does that not work here?

What other DSA drivers do if they need to handle some of the
interrupts is just request the interrupt like any other driver:

https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/pcs-639x.c#L95

> +irqreturn_t ksz9477_handle_irq(struct ksz_device *dev, u8 port, u8 *data)
> +{
> +	irqreturn_t ret = IRQ_NONE;
> +	u32 data32;
> +
> +	if (port > 0)
> +		return ksz9477_handle_port_irq(dev, port - 1, data);
> +
> +	ksz_read32(dev, REG_SW_INT_STATUS__4, &data32);
> +	if (data32 & LUE_INT) {
> +		u8 lue;
> +
> +		ksz_read8(dev, REG_SW_LUE_INT_STATUS, &lue);
> +		ksz_write8(dev, REG_SW_LUE_INT_STATUS, lue);
> +		if (lue & LEARN_FAIL_INT)
> +			dev_info_ratelimited(dev->dev, "lue learn fail\n");
> +		if (lue & WRITE_FAIL_INT)
> +			dev_info_ratelimited(dev->dev, "lue write fail\n");
> +		ret = IRQ_HANDLED;
> +	}

https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/global1_atu.c#L474

	Andrew

Re: [PATCH net-next 4/4] net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Andrew Lunn]

On Fri, Aug 09, 2024 at 04:38:40PM -0700, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> 
> SFP is typically used so the default is 1.  The driver can detect
> 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> has to be explicitly set to 0 as driver cannot detect that
> configuration.
> 
> The SGMII module can only report basic link status of the SFP, so it is
> simulated as a regular internal PHY.
> 
> Since the regular PHY in the switch uses interrupt instead of polling the
> driver has to handle the SGMII interrupt indicating link on/off.
> 
> One issue for the 1000BaseT SFP is there is no link down interrupt, so
> the driver has to use polling to detect link down when the link is up.
> 
> Recent change in the DSA operation can setup the port before the PHY
> interrupt handling function is registered.  As the SGMII interrupt can
> be triggered after port setup there is extra code in the interrupt
> processing to handle this situation.  Otherwise a kernel fault can be
> triggered.
> 
> 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 the interrupt is not triggered prematurely.

Why not model this as a PCS? Russell has been converting all PCS like
things in DSA into try PCS drivers. So i suspect Russell will not like
this code, and would prefer a PCS driver.

	Andrew

Re: [PATCH net-next 4/4] net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Andrew Lunn]

On Fri, Aug 09, 2024 at 04:38:40PM -0700, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <tristram.ha@microchip.com>
> 
> The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> 
> SFP is typically used so the default is 1.  The driver can detect
> 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> has to be explicitly set to 0 as driver cannot detect that
> configuration.

Is 1 actually 1000BaseX? An SFP module using fibre would typically
want 1000BaseX, and only support one speed. An SFP module using copper
typically has a PHY in it, it performs auto-neg on the media side, and
then uses SGMII inband signalling to tell the MAC what data rate,
symbol duplication to do. And maybe mode 0 has in-band signalling
turned off, in which case 1000BaseX and SGMII become identical,
because it is the signalling which is different.

	Andrew

RE: [PATCH net-next 4/4] net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> On Fri, Aug 09, 2024 at 04:38:40PM -0700, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <tristram.ha@microchip.com>
> >
> > The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> >
> > SFP is typically used so the default is 1.  The driver can detect
> > 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> > has to be explicitly set to 0 as driver cannot detect that
> > configuration.
> 
> Is 1 actually 1000BaseX? An SFP module using fibre would typically
> want 1000BaseX, and only support one speed. An SFP module using copper
> typically has a PHY in it, it performs auto-neg on the media side, and
> then uses SGMII inband signalling to tell the MAC what data rate,
> symbol duplication to do. And maybe mode 0 has in-band signalling
> turned off, in which case 1000BaseX and SGMII become identical,
> because it is the signalling which is different.

There are 2 ways to program the hardware registers so that the SGMII
module can communicate with either 1000Base-T/LX/SX SFP or
10/100/1000Base-T SFP.  After a cable is plugged in to the SFP the link
interrupt is triggered.  For 10/100/1000Base-T SFP the connected speed
is also reported.  For fiber type SFP this information is not revealed
so the driver assumes 1000 speed.  In fact 100Base-FX fiber SFP is not
supported and does not work.

RE: [PATCH net-next 4/4] net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> On Fri, Aug 09, 2024 at 04:38:40PM -0700, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <tristram.ha@microchip.com>
> >
> > The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> >
> > SFP is typically used so the default is 1.  The driver can detect
> > 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> > has to be explicitly set to 0 as driver cannot detect that
> > configuration.
> >
> > The SGMII module can only report basic link status of the SFP, so it is
> > simulated as a regular internal PHY.
> >
> > Since the regular PHY in the switch uses interrupt instead of polling the
> > driver has to handle the SGMII interrupt indicating link on/off.
> >
> > One issue for the 1000BaseT SFP is there is no link down interrupt, so
> > the driver has to use polling to detect link down when the link is up.
> >
> > Recent change in the DSA operation can setup the port before the PHY
> > interrupt handling function is registered.  As the SGMII interrupt can
> > be triggered after port setup there is extra code in the interrupt
> > processing to handle this situation.  Otherwise a kernel fault can be
> > triggered.
> >
> > 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 the interrupt is not triggered prematurely.
> 
> Why not model this as a PCS? Russell has been converting all PCS like
> things in DSA into try PCS drivers. So i suspect Russell will not like
> this code, and would prefer a PCS driver.

I am not familiar with PCS.  It seems too complicated after looking at
the phylink_pcs structure and associated phylink_pcs_ops functions.

The SGMII module can only report link and does not even restart
auto-negotiation.  It is a set once and forget operation.

RE: [PATCH net-next 1/4] dt-bindings: 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: 0 for direct
> > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> >
> > SFP is typically used so the default is 1.  The driver can detect
> > 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> > has to be explicitly set to 0 as driver cannot detect that
> > configuration.
> 
> Could you explain this in more detail. Other SGMII blocks don't need
> this. Why is this block special?
> 
> Has this anything to do with in-band signalling?
 
There are 2 ways to program the hardware registers so that the SGMII
module can communicate with either 1000Base-T/LX/SX SFP or
10/100/1000Base-T SFP.  When a SFP is plugged in the driver can try to
detect which type and if it thinks 10/100/1000Base-T SFP is used it
changes the mode to 2 and program appropriately.

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Andrew Lunn]

On Tue, Aug 13, 2024 at 09:14:34PM +0000, Tristram.Ha@microchip.com wrote:
> > > From: Tristram Ha <tristram.ha@microchip.com>
> > >
> > > The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> > > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> > >
> > > SFP is typically used so the default is 1.  The driver can detect
> > > 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> > > has to be explicitly set to 0 as driver cannot detect that
> > > configuration.
> > 
> > Could you explain this in more detail. Other SGMII blocks don't need
> > this. Why is this block special?
> > 
> > Has this anything to do with in-band signalling?
>  
> There are 2 ways to program the hardware registers so that the SGMII
> module can communicate with either 1000Base-T/LX/SX SFP or
> 10/100/1000Base-T SFP.  When a SFP is plugged in the driver can try to
> detect which type and if it thinks 10/100/1000Base-T SFP is used it
> changes the mode to 2 and program appropriately.

What should happen here is that phylink will read the SFP EEPROM and
determine what mode should be used. It will then tell the MAC or PCS
how to configure itself, 1000BaseX, or SGMII. Look at the
mac_link_up() callback, parameter interface.

	Andrew

RE: [PATCH net-next 1/4] dt-bindings: 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: 0 for direct
> > > > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> > > >
> > > > SFP is typically used so the default is 1.  The driver can detect
> > > > 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> > > > has to be explicitly set to 0 as driver cannot detect that
> > > > configuration.
> > >
> > > Could you explain this in more detail. Other SGMII blocks don't need
> > > this. Why is this block special?
> > >
> > > Has this anything to do with in-band signalling?
> >
> > There are 2 ways to program the hardware registers so that the SGMII
> > module can communicate with either 1000Base-T/LX/SX SFP or
> > 10/100/1000Base-T SFP.  When a SFP is plugged in the driver can try to
> > detect which type and if it thinks 10/100/1000Base-T SFP is used it
> > changes the mode to 2 and program appropriately.
> 
> What should happen here is that phylink will read the SFP EEPROM and
> determine what mode should be used. It will then tell the MAC or PCS
> how to configure itself, 1000BaseX, or SGMII. Look at the
> mac_link_up() callback, parameter interface.
 
I am not sure the module can retrieve SFP EEPROM information.

RE: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts in KSZ9477/KSZ9893 switch families

[Tristram.Ha]

> > +static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
> > +                                        u8 *data)
> > +{
> > +     struct dsa_switch *ds = dev->ds;
> > +     struct phy_device *phydev;
> > +     int cnt = 0;
> > +
> > +     phydev = mdiobus_get_phy(ds->user_mii_bus, port);
> > +     if (*data & PORT_PHY_INT) {
> > +             /* Handle the interrupt if there is no PHY device or its
> > +              * interrupt is not registered yet.
> > +              */
> > +             if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
> > +                     u8 phy_status;
> > +
> > +                     ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
> > +                                &phy_status);
> > +                     if (phydev)
> > +                             phy_trigger_machine(phydev);
> > +                     ++cnt;
> > +                     *data &= ~PORT_PHY_INT;
> > +             }
> > +     }
> 
> This looks like a layering violation. Why is this needed? An interrupt
> controller generally has no idea what the individual interrupt is
> about. It just calls into the interrupt core to get the handler
> called, and then clears the interrupt. Why does that not work here?
> 
> What other DSA drivers do if they need to handle some of the
> interrupts is just request the interrupt like any other driver:
> 
> https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/pcs-
> 639x.c#L95

The PHY and ACL interrupt handling can be removed, but the SGMII
interrupt handling cannot as the SGMII port is simulated as having an
internal PHY but the regular PHY interrupt processing will not clear the
interrupt.

Furthermore, there will be a situation where the SGMII interrupt is
triggered before the PHY interrupt handling function is registered.

RE: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> On 10/08/2024 01:38, Tristram.Ha@microchip.com wrote:
> > From: Tristram Ha <tristram.ha@microchip.com>
> >
> > The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> 
> Binding should say it, not commit msg. But aren't you duplicating
> something like phy-connection-type?

The sgmii-mode parameter is just used internally.  I am not sure using
phy-connection-type or phy-mode is appropriate.

> > @@ -137,6 +144,7 @@ examples:
> >              compatible = "microchip,ksz9477";
> >              reg = <0>;
> >              reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
> > +            sgmii-mode = <1>;
> 
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.

Sorry, I missed the example part.

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Krzysztof Kozlowski]

On 14/08/2024 01:09, Tristram.Ha@microchip.com wrote:
>> On 10/08/2024 01:38, Tristram.Ha@microchip.com wrote:
>>> From: Tristram Ha <tristram.ha@microchip.com>
>>>
>>> The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
>>> connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
>>
>> Binding should say it, not commit msg. But aren't you duplicating
>> something like phy-connection-type?
> 
> The sgmii-mode parameter is just used internally.  I am not sure using

This does not matter.

> phy-connection-type or phy-mode is appropriate.

Depends on what this property expressed in terms of hardware. Looks like
you want to say which SGMII mode is being used?



Best regards,
Krzysztof

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Andrew Lunn]

On Tue, Aug 13, 2024 at 10:17:03PM +0000, Tristram.Ha@microchip.com wrote:
> > > > > From: Tristram Ha <tristram.ha@microchip.com>
> > > > >
> > > > > The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> > > > > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> > > > >
> > > > > SFP is typically used so the default is 1.  The driver can detect
> > > > > 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> > > > > has to be explicitly set to 0 as driver cannot detect that
> > > > > configuration.
> > > >
> > > > Could you explain this in more detail. Other SGMII blocks don't need
> > > > this. Why is this block special?
> > > >
> > > > Has this anything to do with in-band signalling?
> > >
> > > There are 2 ways to program the hardware registers so that the SGMII
> > > module can communicate with either 1000Base-T/LX/SX SFP or
> > > 10/100/1000Base-T SFP.  When a SFP is plugged in the driver can try to
> > > detect which type and if it thinks 10/100/1000Base-T SFP is used it
> > > changes the mode to 2 and program appropriately.
> > 
> > What should happen here is that phylink will read the SFP EEPROM and
> > determine what mode should be used. It will then tell the MAC or PCS
> > how to configure itself, 1000BaseX, or SGMII. Look at the
> > mac_link_up() callback, parameter interface.
>  
> I am not sure the module can retrieve SFP EEPROM information.

The board should be designed such that the I2C bus pins of the SFP
cage are connected to an I2C controller. There are also a few pins
which ideally should be connected to GPIOs, LOS, Tx disable etc. You
can then put a node in DT describing the SFP cage:

Documentation/devicetree/bindings/net/sff,sfp.yaml

    sfp2: sfp {
      compatible = "sff,sfp";
      i2c-bus = <&sfp_i2c>;
      los-gpios = <&cps_gpio1 28 GPIO_ACTIVE_HIGH>;
      mod-def0-gpios = <&cps_gpio1 27 GPIO_ACTIVE_LOW>;
      pinctrl-names = "default";
      pinctrl-0 = <&cps_sfpp0_pins>;
      tx-disable-gpios = <&cps_gpio1 29 GPIO_ACTIVE_HIGH>;
      tx-fault-gpios = <&cps_gpio1 26 GPIO_ACTIVE_HIGH>;
    };

and then the ethernet node has a link to it:

    ethernet {
      phy-names = "comphy";
      phys = <&cps_comphy5 0>;
      sfp = <&sfp1>;
    };

Phylink will then driver the SFP and tell the MAC what to do.

	Andrew

Re: [PATCH net-next 3/4] net: dsa: microchip: handle most interrupts in KSZ9477/KSZ9893 switch families

[Andrew Lunn]

On Tue, Aug 13, 2024 at 10:24:52PM +0000, Tristram.Ha@microchip.com wrote:
> > > +static irqreturn_t ksz9477_handle_port_irq(struct ksz_device *dev, u8 port,
> > > +                                        u8 *data)
> > > +{
> > > +     struct dsa_switch *ds = dev->ds;
> > > +     struct phy_device *phydev;
> > > +     int cnt = 0;
> > > +
> > > +     phydev = mdiobus_get_phy(ds->user_mii_bus, port);
> > > +     if (*data & PORT_PHY_INT) {
> > > +             /* Handle the interrupt if there is no PHY device or its
> > > +              * interrupt is not registered yet.
> > > +              */
> > > +             if (!phydev || phydev->interrupts != PHY_INTERRUPT_ENABLED) {
> > > +                     u8 phy_status;
> > > +
> > > +                     ksz_pread8(dev, port, REG_PORT_PHY_INT_STATUS,
> > > +                                &phy_status);
> > > +                     if (phydev)
> > > +                             phy_trigger_machine(phydev);
> > > +                     ++cnt;
> > > +                     *data &= ~PORT_PHY_INT;
> > > +             }
> > > +     }
> > 
> > This looks like a layering violation. Why is this needed? An interrupt
> > controller generally has no idea what the individual interrupt is
> > about. It just calls into the interrupt core to get the handler
> > called, and then clears the interrupt. Why does that not work here?
> > 
> > What other DSA drivers do if they need to handle some of the
> > interrupts is just request the interrupt like any other driver:
> > 
> > https://elixir.bootlin.com/linux/v6.10.3/source/drivers/net/dsa/mv88e6xxx/pcs-
> > 639x.c#L95
> 
> The PHY and ACL interrupt handling can be removed, but the SGMII
> interrupt handling cannot as the SGMII port is simulated as having an
> internal PHY but the regular PHY interrupt processing will not clear the
> interrupt.
> 
> Furthermore, there will be a situation where the SGMII interrupt is
> triggered before the PHY interrupt handling function is registered.

This is one of the reasons i suggested a PCS driver. Look at
drivers/net/dsa/mv88e6xxx/pcs-6185.c as an example, how it handles
interrupts from the PCS. And it is a similar setup, the switch has an
interrupt controller, and the PCS driver requests the interrupt. PCS
drivers do not need to be complex. pcs-6185.c has an empty AN restart
callback, pcs_config does nothing, etc.

	  Andrew

RE: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> On Tue, Aug 13, 2024 at 10:17:03PM +0000, Tristram.Ha@microchip.com wrote:
> > > > > > From: Tristram Ha <tristram.ha@microchip.com>
> > > > > >
> > > > > > The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> > > > > > connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> > > > > >
> > > > > > SFP is typically used so the default is 1.  The driver can detect
> > > > > > 10/100/1000 SFP and change the mode to 2.  For direct connect this mode
> > > > > > has to be explicitly set to 0 as driver cannot detect that
> > > > > > configuration.
> > > > >
> > > > > Could you explain this in more detail. Other SGMII blocks don't need
> > > > > this. Why is this block special?
> > > > >
> > > > > Has this anything to do with in-band signalling?
> > > >
> > > > There are 2 ways to program the hardware registers so that the SGMII
> > > > module can communicate with either 1000Base-T/LX/SX SFP or
> > > > 10/100/1000Base-T SFP.  When a SFP is plugged in the driver can try to
> > > > detect which type and if it thinks 10/100/1000Base-T SFP is used it
> > > > changes the mode to 2 and program appropriately.
> > >
> > > What should happen here is that phylink will read the SFP EEPROM and
> > > determine what mode should be used. It will then tell the MAC or PCS
> > > how to configure itself, 1000BaseX, or SGMII. Look at the
> > > mac_link_up() callback, parameter interface.
> >
> > I am not sure the module can retrieve SFP EEPROM information.
> 
> The board should be designed such that the I2C bus pins of the SFP
> cage are connected to an I2C controller. There are also a few pins
> which ideally should be connected to GPIOs, LOS, Tx disable etc. You
> can then put a node in DT describing the SFP cage:
> 
> Documentation/devicetree/bindings/net/sff,sfp.yaml
> 
>     sfp2: sfp {
>       compatible = "sff,sfp";
>       i2c-bus = <&sfp_i2c>;
>       los-gpios = <&cps_gpio1 28 GPIO_ACTIVE_HIGH>;
>       mod-def0-gpios = <&cps_gpio1 27 GPIO_ACTIVE_LOW>;
>       pinctrl-names = "default";
>       pinctrl-0 = <&cps_sfpp0_pins>;
>       tx-disable-gpios = <&cps_gpio1 29 GPIO_ACTIVE_HIGH>;
>       tx-fault-gpios = <&cps_gpio1 26 GPIO_ACTIVE_HIGH>;
>     };
> 
> and then the ethernet node has a link to it:
> 
>     ethernet {
>       phy-names = "comphy";
>       phys = <&cps_comphy5 0>;
>       sfp = <&sfp1>;
>     };
> 
> Phylink will then driver the SFP and tell the MAC what to do.

I do not think the KSZ9477 switch design allows I2C access to the SFP
EEPROM.  The SGMII module provides only 2 registers that work like PHY's
MMD operation to access vendor-specific registers.  One of such
registers controls how the module communicates with the SFP so that it
works with either 1000Base-T/SX/LX fiber-like SFP or 10/100/1000Base-T
copper SFP.

Actually the default setting works with 10/100/1000Base-T copper SFP
without any programming.  That register needs to be changed when
1000Base-T/SX/LX fiber SFP is used.

I will see if those vendor-specific registers contain the SFP EEPROM,
but I do not know how the sfp_bus structure allows mapping the register
access to probe for those information.

RE: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> On 14/08/2024 01:09, Tristram.Ha@microchip.com wrote:
> >> On 10/08/2024 01:38, Tristram.Ha@microchip.com wrote:
> >>> From: Tristram Ha <tristram.ha@microchip.com>
> >>>
> >>> The SGMII module of KSZ9477 switch can be setup in 3 ways: 0 for direct
> >>> connect, 1 for 1000BaseT SFP, and 2 for 10/100/1000 SFP.
> >>
> >> Binding should say it, not commit msg. But aren't you duplicating
> >> something like phy-connection-type?
> >
> > The sgmii-mode parameter is just used internally.  I am not sure using
> 
> This does not matter.
> 
> > phy-connection-type or phy-mode is appropriate.
> 
> Depends on what this property expressed in terms of hardware. Looks like
> you want to say which SGMII mode is being used?

The driver can detect whether 10/100/1000Base-T copper SFP is being
used.  So the main purpose of this device tree parameter is to indicate
the SGMII module is directly connected to another one without using any
SFP.  This is a very rare case.  In such case the device tree parameter
can be changed to a flag to just indicate SFP is not used.

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Andrew Lunn]

> > The board should be designed such that the I2C bus pins of the SFP
> > cage are connected to an I2C controller. There are also a few pins
> > which ideally should be connected to GPIOs, LOS, Tx disable etc. You
> > can then put a node in DT describing the SFP cage:
> > 
> > Documentation/devicetree/bindings/net/sff,sfp.yaml
> > 
> >     sfp2: sfp {
> >       compatible = "sff,sfp";
> >       i2c-bus = <&sfp_i2c>;
> >       los-gpios = <&cps_gpio1 28 GPIO_ACTIVE_HIGH>;
> >       mod-def0-gpios = <&cps_gpio1 27 GPIO_ACTIVE_LOW>;
> >       pinctrl-names = "default";
> >       pinctrl-0 = <&cps_sfpp0_pins>;
> >       tx-disable-gpios = <&cps_gpio1 29 GPIO_ACTIVE_HIGH>;
> >       tx-fault-gpios = <&cps_gpio1 26 GPIO_ACTIVE_HIGH>;
> >     };
> > 
> > and then the ethernet node has a link to it:
> > 
> >     ethernet {
> >       phy-names = "comphy";
> >       phys = <&cps_comphy5 0>;
> >       sfp = <&sfp1>;
> >     };
> > 
> > Phylink will then driver the SFP and tell the MAC what to do.
> 
> I do not think the KSZ9477 switch design allows I2C access to the SFP
> EEPROM.

This is not a switch design issue, it is a board design issue. Plenty
of Marvell switches have a PCR which do SGMII and 1000BaseX. Only the
SFP SERDES data lines are connected to the switch. The I2C bus and
other lines are connected to the SoC, not the switch.

Do you have the schematics for the board you are testing on? Is it
open? Can you give us a link?

	Andrew

RE: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Tristram.Ha]

> > > The board should be designed such that the I2C bus pins of the SFP
> > > cage are connected to an I2C controller. There are also a few pins
> > > which ideally should be connected to GPIOs, LOS, Tx disable etc. You
> > > can then put a node in DT describing the SFP cage:
> > >
> > > Documentation/devicetree/bindings/net/sff,sfp.yaml
> > >
> > >     sfp2: sfp {
> > >       compatible = "sff,sfp";
> > >       i2c-bus = <&sfp_i2c>;
> > >       los-gpios = <&cps_gpio1 28 GPIO_ACTIVE_HIGH>;
> > >       mod-def0-gpios = <&cps_gpio1 27 GPIO_ACTIVE_LOW>;
> > >       pinctrl-names = "default";
> > >       pinctrl-0 = <&cps_sfpp0_pins>;
> > >       tx-disable-gpios = <&cps_gpio1 29 GPIO_ACTIVE_HIGH>;
> > >       tx-fault-gpios = <&cps_gpio1 26 GPIO_ACTIVE_HIGH>;
> > >     };
> > >
> > > and then the ethernet node has a link to it:
> > >
> > >     ethernet {
> > >       phy-names = "comphy";
> > >       phys = <&cps_comphy5 0>;
> > >       sfp = <&sfp1>;
> > >     };
> > >
> > > Phylink will then driver the SFP and tell the MAC what to do.
> >
> > I do not think the KSZ9477 switch design allows I2C access to the SFP
> > EEPROM.
> 
> This is not a switch design issue, it is a board design issue. Plenty
> of Marvell switches have a PCR which do SGMII and 1000BaseX. Only the
> SFP SERDES data lines are connected to the switch. The I2C bus and
> other lines are connected to the SoC, not the switch.
> 
> Do you have the schematics for the board you are testing on? Is it
> open? Can you give us a link?

My KSZ9477 board does not have that I2C connection, so I cannot
implement the change as suggested.  I am getting a new design board that
needs verification of this connection.  After I make it work I will
re-submit the patch.

Thanks for your help.

Re: [PATCH net-next 1/4] dt-bindings: net: dsa: microchip: add SGMII port support to KSZ9477 switch

[Andrew Lunn]

> My KSZ9477 board does not have that I2C connection, so I cannot
> implement the change as suggested.

I would say any board without the I2C bus connected is fatally
broken. Most SFPs are buggy, and don't follow the standard. You need
to read the vendor and product ID so you can enable various quirks to
make them work correctly.

> I am getting a new design board that needs verification of this
> connection.  After I make it work I will re-submit the patch.

O.K, that sounds good.

	Andrew