* [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
@ 2025-01-14 2:47 Tristram.Ha
2025-01-14 16:09 ` Vladimir Oltean
0 siblings, 1 reply; 35+ messages in thread
From: Tristram.Ha @ 2025-01-14 2:47 UTC (permalink / raw)
To: Woojung Huh, Andrew Lunn, Vladimir Oltean
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
UNGLinuxDriver, netdev, linux-kernel, 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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-14 2:47 Tristram.Ha
@ 2025-01-14 16:09 ` Vladimir Oltean
2025-01-14 16:53 ` Vladimir Oltean
` (2 more replies)
0 siblings, 3 replies; 35+ messages in thread
From: Vladimir Oltean @ 2025-01-14 16:09 UTC (permalink / raw)
To: Tristram.Ha
Cc: Woojung Huh, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, UNGLinuxDriver, netdev, linux-kernel
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
>
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-14 16:09 ` Vladimir Oltean
@ 2025-01-14 16:53 ` Vladimir Oltean
2025-01-15 10:10 ` Maxime Chevallier
2025-01-17 0:51 ` Tristram.Ha
2 siblings, 0 replies; 35+ messages in thread
From: Vladimir Oltean @ 2025-01-14 16:53 UTC (permalink / raw)
To: Tristram.Ha
Cc: Woojung Huh, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, UNGLinuxDriver, netdev, linux-kernel
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/
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-14 16:09 ` Vladimir Oltean
2025-01-14 16:53 ` Vladimir Oltean
@ 2025-01-15 10:10 ` Maxime Chevallier
2025-01-17 0:56 ` Tristram.Ha
2025-01-17 0:51 ` Tristram.Ha
2 siblings, 1 reply; 35+ messages in thread
From: Maxime Chevallier @ 2025-01-15 10:10 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Tristram.Ha, Woojung Huh, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, UNGLinuxDriver, netdev,
linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-14 16:09 ` Vladimir Oltean
2025-01-14 16:53 ` Vladimir Oltean
2025-01-15 10:10 ` Maxime Chevallier
@ 2025-01-17 0:51 ` Tristram.Ha
2 siblings, 0 replies; 35+ messages in thread
From: Tristram.Ha @ 2025-01-17 0:51 UTC (permalink / raw)
To: olteanv
Cc: Woojung.Huh, andrew, davem, edumazet, kuba, pabeni,
UNGLinuxDriver, netdev, linux-kernel
> 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-15 10:10 ` Maxime Chevallier
@ 2025-01-17 0:56 ` Tristram.Ha
2025-01-17 13:36 ` Andrew Lunn
2025-01-17 16:18 ` Vladimir Oltean
0 siblings, 2 replies; 35+ messages in thread
From: Tristram.Ha @ 2025-01-17 0:56 UTC (permalink / raw)
To: maxime.chevallier, olteanv
Cc: Woojung.Huh, andrew, davem, edumazet, kuba, pabeni,
UNGLinuxDriver, netdev, linux-kernel
> 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-17 0:56 ` Tristram.Ha
@ 2025-01-17 13:36 ` Andrew Lunn
2025-01-18 0:59 ` Tristram.Ha
2025-01-17 16:18 ` Vladimir Oltean
1 sibling, 1 reply; 35+ messages in thread
From: Andrew Lunn @ 2025-01-17 13:36 UTC (permalink / raw)
To: Tristram.Ha
Cc: maxime.chevallier, olteanv, Woojung.Huh, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-17 0:56 ` Tristram.Ha
2025-01-17 13:36 ` Andrew Lunn
@ 2025-01-17 16:18 ` Vladimir Oltean
1 sibling, 0 replies; 35+ messages in thread
From: Vladimir Oltean @ 2025-01-17 16:18 UTC (permalink / raw)
To: Tristram.Ha
Cc: maxime.chevallier, Woojung.Huh, andrew, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-17 13:36 ` Andrew Lunn
@ 2025-01-18 0:59 ` Tristram.Ha
2025-01-18 1:26 ` Vladimir Oltean
0 siblings, 1 reply; 35+ messages in thread
From: Tristram.Ha @ 2025-01-18 0:59 UTC (permalink / raw)
To: andrew
Cc: maxime.chevallier, olteanv, Woojung.Huh, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
> > 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-18 0:59 ` Tristram.Ha
@ 2025-01-18 1:26 ` Vladimir Oltean
2025-01-18 4:07 ` Tristram.Ha
0 siblings, 1 reply; 35+ messages in thread
From: Vladimir Oltean @ 2025-01-18 1:26 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, maxime.chevallier, Woojung.Huh, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-18 1:26 ` Vladimir Oltean
@ 2025-01-18 4:07 ` Tristram.Ha
2025-01-18 20:00 ` Andrew Lunn
0 siblings, 1 reply; 35+ messages in thread
From: Tristram.Ha @ 2025-01-18 4:07 UTC (permalink / raw)
To: olteanv
Cc: andrew, maxime.chevallier, Woojung.Huh, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
> 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-18 4:07 ` Tristram.Ha
@ 2025-01-18 20:00 ` Andrew Lunn
2025-01-21 0:28 ` Tristram.Ha
0 siblings, 1 reply; 35+ messages in thread
From: Andrew Lunn @ 2025-01-18 20:00 UTC (permalink / raw)
To: Tristram.Ha
Cc: olteanv, maxime.chevallier, Woojung.Huh, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
> 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
^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-18 20:00 ` Andrew Lunn
@ 2025-01-21 0:28 ` Tristram.Ha
2025-01-21 3:08 ` Tristram.Ha
0 siblings, 1 reply; 35+ messages in thread
From: Tristram.Ha @ 2025-01-21 0:28 UTC (permalink / raw)
To: andrew
Cc: olteanv, maxime.chevallier, Woojung.Huh, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
> > 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-01-21 0:28 ` Tristram.Ha
@ 2025-01-21 3:08 ` Tristram.Ha
0 siblings, 0 replies; 35+ messages in thread
From: Tristram.Ha @ 2025-01-21 3:08 UTC (permalink / raw)
To: andrew
Cc: olteanv, maxime.chevallier, Woojung.Huh, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel
> > > 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
@ 2025-05-07 0:09 Tristram.Ha
2025-05-07 7:44 ` Maxime Chevallier
2025-05-10 0:41 ` Jakub Kicinski
0 siblings, 2 replies; 35+ messages in thread
From: Tristram.Ha @ 2025-05-07 0:09 UTC (permalink / raw)
To: Andrew Lunn, Woojung Huh, Russell King, Vladimir Oltean
Cc: Heiner Kallweit, Maxime Chevallier, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, UNGLinuxDriver, netdev, linux-kernel,
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
^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-05-07 0:09 [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch Tristram.Ha
@ 2025-05-07 7:44 ` Maxime Chevallier
2025-05-07 8:31 ` Russell King (Oracle)
2025-05-10 0:41 ` Jakub Kicinski
1 sibling, 1 reply; 35+ messages in thread
From: Maxime Chevallier @ 2025-05-07 7:44 UTC (permalink / raw)
To: Tristram.Ha
Cc: Andrew Lunn, Woojung Huh, Russell King, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, UNGLinuxDriver, netdev, linux-kernel
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/
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-05-07 7:44 ` Maxime Chevallier
@ 2025-05-07 8:31 ` Russell King (Oracle)
2025-05-07 8:54 ` Maxime Chevallier
0 siblings, 1 reply; 35+ messages in thread
From: Russell King (Oracle) @ 2025-05-07 8:31 UTC (permalink / raw)
To: Maxime Chevallier
Cc: Tristram.Ha, Andrew Lunn, Woojung Huh, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, UNGLinuxDriver, netdev, linux-kernel
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!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-05-07 8:31 ` Russell King (Oracle)
@ 2025-05-07 8:54 ` Maxime Chevallier
2025-05-07 9:23 ` [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch) Russell King (Oracle)
2025-05-08 1:41 ` [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch Tristram.Ha
0 siblings, 2 replies; 35+ messages in thread
From: Maxime Chevallier @ 2025-05-07 8:54 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Tristram.Ha, Andrew Lunn, Woojung Huh, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, UNGLinuxDriver, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 8:54 ` Maxime Chevallier
@ 2025-05-07 9:23 ` Russell King (Oracle)
2025-05-07 9:59 ` Russell King (Oracle)
2025-05-08 1:41 ` [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch Tristram.Ha
1 sibling, 1 reply; 35+ messages in thread
From: Russell King (Oracle) @ 2025-05-07 9:23 UTC (permalink / raw)
To: Maxime Chevallier, Linus Torvalds
Cc: Andrew Lunn, Woojung Huh, Vladimir Oltean, Heiner Kallweit,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Dmitry Torokhov, linux-input, linux-kernel
[Sorry for going off topic here - changed the Cc list, added Linus,
changed the subject.]
On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> On Wed, 7 May 2025 09:31:48 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > [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...
I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
laptop I had previously (normally with ctrl-F keys). However, hitting
ctrl/shift/alt would stop it.
This is the first time I've seen the behaviour with the Carbon X1
laptop, but this was way more severe. No key would stop it. Trying to
move the focus using the trackpad/nipple had any effect. Meanwhile
the email was being deleted one character at a time. So I shut the
laptop lid causing it to suspend, and wondered what to do... on
re-opening the laptop, it didn't restart and is back to normal.
This suggests that the entire input subsystem in the software stack
collapsed just after the backspace key was pressed, and Xorg never
saw the key-release event. So Xorg duitifully did its key-repeat
processing, causing the email to be deleted one character at a time.
The problem is, not only did this destroy the email reply, but it
also destroyed my train of thought for the reply as well through
the panic of trying to stop the entire email being deleted.
I don't think this is a hardware issue - I think there's a problem
in the input handling somewhere in the stack of kernel, Xorg,
whatever multiple input libraries make up modern systems, and KDE.
I did check the logs. Nothing in the kernel messages that suggests
a problem. Nothing in Xorg's logs (which are difficult to tie up
because it doesn't use real timestamps that one can relate to real
time.) There's no longer any ~/.xsession-errors logfile for logging
the stuff below Xorg.
I'm running Debian Stable here - kernel 6.1.0-34-amd64, X.Org X Server
1.21.1.7, KDE Plasma (5.27.5, frameworks 5.103.0, QT 5.15.8).
Anyone else seeing this kind of behaviour - if so, what are you
using?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 9:23 ` [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch) Russell King (Oracle)
@ 2025-05-07 9:59 ` Russell King (Oracle)
2025-05-07 13:32 ` Maxime Chevallier
0 siblings, 1 reply; 35+ messages in thread
From: Russell King (Oracle) @ 2025-05-07 9:59 UTC (permalink / raw)
To: Maxime Chevallier, Linus Torvalds
Cc: Andrew Lunn, Woojung Huh, Vladimir Oltean, Heiner Kallweit,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Dmitry Torokhov, linux-input, linux-kernel
On Wed, May 07, 2025 at 10:23:17AM +0100, Russell King (Oracle) wrote:
> [Sorry for going off topic here - changed the Cc list, added Linus,
> changed the subject.]
>
> On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> > On Wed, 7 May 2025 09:31:48 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > [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...
>
> I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
> laptop I had previously (normally with ctrl-F keys). However, hitting
> ctrl/shift/alt would stop it.
>
> This is the first time I've seen the behaviour with the Carbon X1
> laptop, but this was way more severe. No key would stop it. Trying to
> move the focus using the trackpad/nipple had any effect. Meanwhile
> the email was being deleted one character at a time. So I shut the
> laptop lid causing it to suspend, and wondered what to do... on
> re-opening the laptop, it didn't restart and is back to normal.
>
> This suggests that the entire input subsystem in the software stack
> collapsed just after the backspace key was pressed, and Xorg never
> saw the key-release event. So Xorg duitifully did its key-repeat
> processing, causing the email to be deleted one character at a time.
>
> The problem is, not only did this destroy the email reply, but it
> also destroyed my train of thought for the reply as well through
> the panic of trying to stop the entire email being deleted.
>
> I don't think this is a hardware issue - I think there's a problem
> in the input handling somewhere in the stack of kernel, Xorg,
> whatever multiple input libraries make up modern systems, and KDE.
>
> I did check the logs. Nothing in the kernel messages that suggests
> a problem. Nothing in Xorg's logs (which are difficult to tie up
> because it doesn't use real timestamps that one can relate to real
> time.) There's no longer any ~/.xsession-errors logfile for logging
> the stuff below Xorg.
>
> I'm running Debian Stable here - kernel 6.1.0-34-amd64, X.Org X Server
> 1.21.1.7, KDE Plasma (5.27.5, frameworks 5.103.0, QT 5.15.8).
I'll also add that The Carbon X1, being a laptop, its built-in keyboard
uses the i8042:
[ 1.698156] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
[ 1.698543] i8042: Warning: Keylock active
[ 1.700170] serio: i8042 KBD port at 0x60,0x64 irq 1
[ 1.700174] serio: i8042 AUX port at 0x60,0x64 irq 12
[ 1.700271] mousedev: PS/2 mouse device common for all mice
[ 1.702951] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
I don't have the HP laptop with me to check what that was using.
The mysterious thing is "Keylock active" - clearly it isn't because I
can write this email typing on that very keyboard. However, I wonder
if it needs i8042_unlock=1 to set I8042_CTR_IGNKEYLOCK.
Unfortunately, it's probably going to take a year on the Carbon X1
to work out if this makes any difference.
> Anyone else seeing this kind of behaviour - if so, what are you
> using?
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 13:32 ` Maxime Chevallier
@ 2025-05-07 11:44 ` Russell King (Oracle)
2025-05-07 11:51 ` Maxime Chevallier
2025-05-07 20:46 ` [BUG] Stuck key syndrome Holger Hoffstätte
0 siblings, 2 replies; 35+ messages in thread
From: Russell King (Oracle) @ 2025-05-07 11:44 UTC (permalink / raw)
To: Maxime Chevallier
Cc: Linus Torvalds, Andrew Lunn, Woojung Huh, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Dmitry Torokhov, linux-input, linux-kernel
Hi Maxime,
On Wed, May 07, 2025 at 03:32:36PM +0200, Maxime Chevallier wrote:
> Hi Russell,
>
> On Wed, 7 May 2025 10:59:21 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>
> > On Wed, May 07, 2025 at 10:23:17AM +0100, Russell King (Oracle) wrote:
> > > [Sorry for going off topic here - changed the Cc list, added Linus,
> > > changed the subject.]
> > >
> > > On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> > > > On Wed, 7 May 2025 09:31:48 +0100
> > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > > [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...
> > >
> > > I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
> > > laptop I had previously (normally with ctrl-F keys). However, hitting
> > > ctrl/shift/alt would stop it.
> > >
> > > This is the first time I've seen the behaviour with the Carbon X1
> > > laptop, but this was way more severe. No key would stop it. Trying to
> > > move the focus using the trackpad/nipple had any effect. Meanwhile
> > > the email was being deleted one character at a time. So I shut the
> > > laptop lid causing it to suspend, and wondered what to do... on
> > > re-opening the laptop, it didn't restart and is back to normal.
> > >
> > > This suggests that the entire input subsystem in the software stack
> > > collapsed just after the backspace key was pressed, and Xorg never
> > > saw the key-release event. So Xorg duitifully did its key-repeat
> > > processing, causing the email to be deleted one character at a time.
> > >
> > > The problem is, not only did this destroy the email reply, but it
> > > also destroyed my train of thought for the reply as well through
> > > the panic of trying to stop the entire email being deleted.
> > >
> > > I don't think this is a hardware issue - I think there's a problem
> > > in the input handling somewhere in the stack of kernel, Xorg,
> > > whatever multiple input libraries make up modern systems, and KDE.
> > >
> > > I did check the logs. Nothing in the kernel messages that suggests
> > > a problem. Nothing in Xorg's logs (which are difficult to tie up
> > > because it doesn't use real timestamps that one can relate to real
> > > time.) There's no longer any ~/.xsession-errors logfile for logging
> > > the stuff below Xorg.
> > >
> > > I'm running Debian Stable here - kernel 6.1.0-34-amd64, X.Org X Server
> > > 1.21.1.7, KDE Plasma (5.27.5, frameworks 5.103.0, QT 5.15.8).
> >
> > I'll also add that The Carbon X1, being a laptop, its built-in keyboard
> > uses the i8042:
> >
> > [ 1.698156] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
> > [ 1.698543] i8042: Warning: Keylock active
> > [ 1.700170] serio: i8042 KBD port at 0x60,0x64 irq 1
> > [ 1.700174] serio: i8042 AUX port at 0x60,0x64 irq 12
> > [ 1.700271] mousedev: PS/2 mouse device common for all mice
> > [ 1.702951] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
> >
> > I don't have the HP laptop with me to check what that was using.
> >
> > The mysterious thing is "Keylock active" - clearly it isn't because I
> > can write this email typing on that very keyboard. However, I wonder
> > if it needs i8042_unlock=1 to set I8042_CTR_IGNKEYLOCK.
> >
> > Unfortunately, it's probably going to take a year on the Carbon X1
> > to work out if this makes any difference.
> >
> > > Anyone else seeing this kind of behaviour - if so, what are you
> > > using?
>
> It just happened to me as I was typing this very email (key 'd' got
> stuck, nothing could un-stick it, couldn't move the mouse cursor but
> mouse-click events did work, had to suspend/resume the laptop to fix
> that)
'd' can be quite disastrous if you're using vi and you're in command
mode!
> Got the same "Keylock active" warning at boot :
>
> [ 0.916750] i8042: PNP: PS/2 Controller [PNP0303:PS2K,PNP0f13:PS2M] at 0x60,0x64 irq 1,12
> [ 0.917210] i8042: Warning: Keylock active
> [ 0.920087] serio: i8042 KBD port at 0x60,0x64 irq 1
> [ 0.920090] serio: i8042 AUX port at 0x60,0x64 irq 12
>
> Nothing in the kernel logs when the key got stuck.
>
> Laptop is a Dell XPS 15 9510, Running Fedora 41 but I saw this issue
> before, kernel 6.14.4-200.fc41.x86_64, Wayland-based, Gnome 47.
>
> Hopefully this helps a bit narrowing this down, I have a fairly
> different userspace stack and kernel version, but we do have the same
> driver involved and same keylock warning...
Could you try booting with i8042_unlock=1 and see whether that makes any
difference please?
I've added that to my grub config in preparation for rebooting, but even
if I booted now, I suspect it'll be some time before I have any useful
result.
How often do you see the problem?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 11:44 ` Russell King (Oracle)
@ 2025-05-07 11:51 ` Maxime Chevallier
2025-05-07 14:46 ` Linus Torvalds
2025-05-07 17:45 ` Dmitry Torokhov
2025-05-07 20:46 ` [BUG] Stuck key syndrome Holger Hoffstätte
1 sibling, 2 replies; 35+ messages in thread
From: Maxime Chevallier @ 2025-05-07 11:51 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Linus Torvalds, Andrew Lunn, Woojung Huh, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Dmitry Torokhov, linux-input, linux-kernel
On Wed, 7 May 2025 12:44:24 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> Hi Maxime,
>
> On Wed, May 07, 2025 at 03:32:36PM +0200, Maxime Chevallier wrote:
> > Hi Russell,
> >
> > On Wed, 7 May 2025 10:59:21 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >
> > > On Wed, May 07, 2025 at 10:23:17AM +0100, Russell King (Oracle) wrote:
> > > > [Sorry for going off topic here - changed the Cc list, added Linus,
> > > > changed the subject.]
> > > >
> > > > On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> > > > > On Wed, 7 May 2025 09:31:48 +0100
> > > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > > > [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...
> > > >
> > > > I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
> > > > laptop I had previously (normally with ctrl-F keys). However, hitting
> > > > ctrl/shift/alt would stop it.
> > > >
> > > > This is the first time I've seen the behaviour with the Carbon X1
> > > > laptop, but this was way more severe. No key would stop it. Trying to
> > > > move the focus using the trackpad/nipple had any effect. Meanwhile
> > > > the email was being deleted one character at a time. So I shut the
> > > > laptop lid causing it to suspend, and wondered what to do... on
> > > > re-opening the laptop, it didn't restart and is back to normal.
> > > >
> > > > This suggests that the entire input subsystem in the software stack
> > > > collapsed just after the backspace key was pressed, and Xorg never
> > > > saw the key-release event. So Xorg duitifully did its key-repeat
> > > > processing, causing the email to be deleted one character at a time.
> > > >
> > > > The problem is, not only did this destroy the email reply, but it
> > > > also destroyed my train of thought for the reply as well through
> > > > the panic of trying to stop the entire email being deleted.
> > > >
> > > > I don't think this is a hardware issue - I think there's a problem
> > > > in the input handling somewhere in the stack of kernel, Xorg,
> > > > whatever multiple input libraries make up modern systems, and KDE.
> > > >
> > > > I did check the logs. Nothing in the kernel messages that suggests
> > > > a problem. Nothing in Xorg's logs (which are difficult to tie up
> > > > because it doesn't use real timestamps that one can relate to real
> > > > time.) There's no longer any ~/.xsession-errors logfile for logging
> > > > the stuff below Xorg.
> > > >
> > > > I'm running Debian Stable here - kernel 6.1.0-34-amd64, X.Org X Server
> > > > 1.21.1.7, KDE Plasma (5.27.5, frameworks 5.103.0, QT 5.15.8).
> > >
> > > I'll also add that The Carbon X1, being a laptop, its built-in keyboard
> > > uses the i8042:
> > >
> > > [ 1.698156] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
> > > [ 1.698543] i8042: Warning: Keylock active
> > > [ 1.700170] serio: i8042 KBD port at 0x60,0x64 irq 1
> > > [ 1.700174] serio: i8042 AUX port at 0x60,0x64 irq 12
> > > [ 1.700271] mousedev: PS/2 mouse device common for all mice
> > > [ 1.702951] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
> > >
> > > I don't have the HP laptop with me to check what that was using.
> > >
> > > The mysterious thing is "Keylock active" - clearly it isn't because I
> > > can write this email typing on that very keyboard. However, I wonder
> > > if it needs i8042_unlock=1 to set I8042_CTR_IGNKEYLOCK.
> > >
> > > Unfortunately, it's probably going to take a year on the Carbon X1
> > > to work out if this makes any difference.
> > >
> > > > Anyone else seeing this kind of behaviour - if so, what are you
> > > > using?
> >
> > It just happened to me as I was typing this very email (key 'd' got
> > stuck, nothing could un-stick it, couldn't move the mouse cursor but
> > mouse-click events did work, had to suspend/resume the laptop to fix
> > that)
>
> 'd' can be quite disastrous if you're using vi and you're in command
> mode!
>
> > Got the same "Keylock active" warning at boot :
> >
> > [ 0.916750] i8042: PNP: PS/2 Controller [PNP0303:PS2K,PNP0f13:PS2M] at 0x60,0x64 irq 1,12
> > [ 0.917210] i8042: Warning: Keylock active
> > [ 0.920087] serio: i8042 KBD port at 0x60,0x64 irq 1
> > [ 0.920090] serio: i8042 AUX port at 0x60,0x64 irq 12
> >
> > Nothing in the kernel logs when the key got stuck.
> >
> > Laptop is a Dell XPS 15 9510, Running Fedora 41 but I saw this issue
> > before, kernel 6.14.4-200.fc41.x86_64, Wayland-based, Gnome 47.
> >
> > Hopefully this helps a bit narrowing this down, I have a fairly
> > different userspace stack and kernel version, but we do have the same
> > driver involved and same keylock warning...
>
> Could you try booting with i8042_unlock=1 and see whether that makes any
> difference please?
I'll try this out indeed :)
> I've added that to my grub config in preparation for rebooting, but even
> if I booted now, I suspect it'll be some time before I have any useful
> result.
>
> How often do you see the problem?
It's very sporadic, sometimes I'll see this behaviour multiple times a
day, and then nothing for weeks... This laptop has been very quirky
ever since I got it, so I categorized that as a "yet another XPS 9510
broken behaviour", but this has been a recurrent thing for multiple
years...
So, same as you, it'll take a long time for me to say with some amount
of certainty that 'i8042_unlock=1' has a beneficial effect, of
course unless I see the problem happen again in the meantime.
Maxime
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 9:59 ` Russell King (Oracle)
@ 2025-05-07 13:32 ` Maxime Chevallier
2025-05-07 11:44 ` Russell King (Oracle)
0 siblings, 1 reply; 35+ messages in thread
From: Maxime Chevallier @ 2025-05-07 13:32 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Linus Torvalds, Andrew Lunn, Woojung Huh, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Dmitry Torokhov, linux-input, linux-kernel
Hi Russell,
On Wed, 7 May 2025 10:59:21 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Wed, May 07, 2025 at 10:23:17AM +0100, Russell King (Oracle) wrote:
> > [Sorry for going off topic here - changed the Cc list, added Linus,
> > changed the subject.]
> >
> > On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> > > On Wed, 7 May 2025 09:31:48 +0100
> > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > [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...
> >
> > I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
> > laptop I had previously (normally with ctrl-F keys). However, hitting
> > ctrl/shift/alt would stop it.
> >
> > This is the first time I've seen the behaviour with the Carbon X1
> > laptop, but this was way more severe. No key would stop it. Trying to
> > move the focus using the trackpad/nipple had any effect. Meanwhile
> > the email was being deleted one character at a time. So I shut the
> > laptop lid causing it to suspend, and wondered what to do... on
> > re-opening the laptop, it didn't restart and is back to normal.
> >
> > This suggests that the entire input subsystem in the software stack
> > collapsed just after the backspace key was pressed, and Xorg never
> > saw the key-release event. So Xorg duitifully did its key-repeat
> > processing, causing the email to be deleted one character at a time.
> >
> > The problem is, not only did this destroy the email reply, but it
> > also destroyed my train of thought for the reply as well through
> > the panic of trying to stop the entire email being deleted.
> >
> > I don't think this is a hardware issue - I think there's a problem
> > in the input handling somewhere in the stack of kernel, Xorg,
> > whatever multiple input libraries make up modern systems, and KDE.
> >
> > I did check the logs. Nothing in the kernel messages that suggests
> > a problem. Nothing in Xorg's logs (which are difficult to tie up
> > because it doesn't use real timestamps that one can relate to real
> > time.) There's no longer any ~/.xsession-errors logfile for logging
> > the stuff below Xorg.
> >
> > I'm running Debian Stable here - kernel 6.1.0-34-amd64, X.Org X Server
> > 1.21.1.7, KDE Plasma (5.27.5, frameworks 5.103.0, QT 5.15.8).
>
> I'll also add that The Carbon X1, being a laptop, its built-in keyboard
> uses the i8042:
>
> [ 1.698156] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
> [ 1.698543] i8042: Warning: Keylock active
> [ 1.700170] serio: i8042 KBD port at 0x60,0x64 irq 1
> [ 1.700174] serio: i8042 AUX port at 0x60,0x64 irq 12
> [ 1.700271] mousedev: PS/2 mouse device common for all mice
> [ 1.702951] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
>
> I don't have the HP laptop with me to check what that was using.
>
> The mysterious thing is "Keylock active" - clearly it isn't because I
> can write this email typing on that very keyboard. However, I wonder
> if it needs i8042_unlock=1 to set I8042_CTR_IGNKEYLOCK.
>
> Unfortunately, it's probably going to take a year on the Carbon X1
> to work out if this makes any difference.
>
> > Anyone else seeing this kind of behaviour - if so, what are you
> > using?
It just happened to me as I was typing this very email (key 'd' got
stuck, nothing could un-stick it, couldn't move the mouse cursor but
mouse-click events did work, had to suspend/resume the laptop to fix
that)
Got the same "Keylock active" warning at boot :
[ 0.916750] i8042: PNP: PS/2 Controller [PNP0303:PS2K,PNP0f13:PS2M] at 0x60,0x64 irq 1,12
[ 0.917210] i8042: Warning: Keylock active
[ 0.920087] serio: i8042 KBD port at 0x60,0x64 irq 1
[ 0.920090] serio: i8042 AUX port at 0x60,0x64 irq 12
Nothing in the kernel logs when the key got stuck.
Laptop is a Dell XPS 15 9510, Running Fedora 41 but I saw this issue
before, kernel 6.14.4-200.fc41.x86_64, Wayland-based, Gnome 47.
Hopefully this helps a bit narrowing this down, I have a fairly
different userspace stack and kernel version, but we do have the same
driver involved and same keylock warning...
Maxime
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 11:51 ` Maxime Chevallier
@ 2025-05-07 14:46 ` Linus Torvalds
2025-05-07 17:23 ` Dmitry Torokhov
2025-05-07 17:45 ` Dmitry Torokhov
1 sibling, 1 reply; 35+ messages in thread
From: Linus Torvalds @ 2025-05-07 14:46 UTC (permalink / raw)
To: Maxime Chevallier
Cc: Russell King (Oracle), Andrew Lunn, Woojung Huh, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Dmitry Torokhov, linux-input, linux-kernel
On Wed, 7 May 2025 at 04:51, Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> So, same as you, it'll take a long time for me to say with some amount
> of certainty that 'i8042_unlock=1' has a beneficial effect, of
> course unless I see the problem happen again in the meantime.
Christ. You'd expect that any i8042 issues had been fixed long ago,
but the problem is that the chip doesn't necessarily even exist in
modern platforms, and everybody just fakes it.
So the platform presumably still has hardware support for it, but it's
mostly in the form of "take a trap when accessing the legacy keyboard
ports, and fake it in firmware".
Although it doesn't help that there are literally decades of clone
chips and hacky real hardware that extended on the i8042 in various
more-or-less compatible ways.
Which makes all of these things almost entirely undebuggable.
I'm surprised the XPS9510 would be particularly troublesome - I've had
an XPS for years (older version, obviously) with no issues outside of
WiFi sometimes acting up. But random firmware...
I doubt it's "keylock active", but who knows. I get that on my xps
too, it's a random bit that doesn't really mean much. But - because of
all the reasons above - who knows...
One typical problem has been "the interrupt line is wired oddly", but
the fact that it apparently works *most* of the time means that that
is unlikely to be the issue here.
Linus
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 14:46 ` Linus Torvalds
@ 2025-05-07 17:23 ` Dmitry Torokhov
2025-05-07 17:46 ` Russell King (Oracle)
0 siblings, 1 reply; 35+ messages in thread
From: Dmitry Torokhov @ 2025-05-07 17:23 UTC (permalink / raw)
To: Linus Torvalds
Cc: Maxime Chevallier, Russell King (Oracle), Andrew Lunn,
Woojung Huh, Vladimir Oltean, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-input,
linux-kernel
On Wed, May 07, 2025 at 07:46:03AM -0700, Linus Torvalds wrote:
> On Wed, 7 May 2025 at 04:51, Maxime Chevallier
> <maxime.chevallier@bootlin.com> wrote:
> >
> > So, same as you, it'll take a long time for me to say with some amount
> > of certainty that 'i8042_unlock=1' has a beneficial effect, of
> > course unless I see the problem happen again in the meantime.
>
> Christ. You'd expect that any i8042 issues had been fixed long ago,
> but the problem is that the chip doesn't necessarily even exist in
> modern platforms, and everybody just fakes it.
It has not existed as a real chip for more than 20 years I believe. It's
all faked in firmware and embedded controllers that fake it in their
firmwares.
And newer firmware tend to implement less and less of it, just what OS
that devices that ship with it needs.
>
> So the platform presumably still has hardware support for it, but it's
> mostly in the form of "take a trap when accessing the legacy keyboard
> ports, and fake it in firmware".
>
> Although it doesn't help that there are literally decades of clone
> chips and hacky real hardware that extended on the i8042 in various
> more-or-less compatible ways.
>
> Which makes all of these things almost entirely undebuggable.
>
> I'm surprised the XPS9510 would be particularly troublesome - I've had
> an XPS for years (older version, obviously) with no issues outside of
> WiFi sometimes acting up. But random firmware...
>
> I doubt it's "keylock active", but who knows. I get that on my xps
> too, it's a random bit that doesn't really mean much. But - because of
> all the reasons above - who knows...
It is typically harmless and whats more trying to "unlock" 8042 when it
reports being locked might confuse 8042 emulation.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 11:51 ` Maxime Chevallier
2025-05-07 14:46 ` Linus Torvalds
@ 2025-05-07 17:45 ` Dmitry Torokhov
2025-05-07 18:18 ` Russell King (Oracle)
1 sibling, 1 reply; 35+ messages in thread
From: Dmitry Torokhov @ 2025-05-07 17:45 UTC (permalink / raw)
To: Maxime Chevallier
Cc: Russell King (Oracle), Linus Torvalds, Andrew Lunn, Woojung Huh,
Vladimir Oltean, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-input, linux-kernel,
Peter Hutterer, Benjamin Tissoires
On Wed, May 07, 2025 at 01:51:26PM +0200, Maxime Chevallier wrote:
> On Wed, 7 May 2025 12:44:24 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>
> > Hi Maxime,
> >
> > On Wed, May 07, 2025 at 03:32:36PM +0200, Maxime Chevallier wrote:
> > > Hi Russell,
> > >
> > > On Wed, 7 May 2025 10:59:21 +0100
> > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > >
> > > > On Wed, May 07, 2025 at 10:23:17AM +0100, Russell King (Oracle) wrote:
> > > > > [Sorry for going off topic here - changed the Cc list, added Linus,
> > > > > changed the subject.]
> > > > >
> > > > > On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> > > > > > On Wed, 7 May 2025 09:31:48 +0100
> > > > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > > > > [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...
> > > > >
> > > > > I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
> > > > > laptop I had previously (normally with ctrl-F keys). However, hitting
> > > > > ctrl/shift/alt would stop it.
> > > > >
> > > > > This is the first time I've seen the behaviour with the Carbon X1
> > > > > laptop, but this was way more severe. No key would stop it. Trying to
> > > > > move the focus using the trackpad/nipple had any effect. Meanwhile
> > > > > the email was being deleted one character at a time. So I shut the
If we indeed lost a key release event somewhere the way to "restore" it
is to hit the stuck key again. Then we should get press/release with
press most likely being ignored and release achieving the desired
result. Of course that will not help if embedded controller is confused.
> > > > > laptop lid causing it to suspend, and wondered what to do... on
> > > > > re-opening the laptop, it didn't restart and is back to normal.
> > > > >
> > > > > This suggests that the entire input subsystem in the software stack
> > > > > collapsed just after the backspace key was pressed, and Xorg never
> > > > > saw the key-release event. So Xorg duitifully did its key-repeat
> > > > > processing, causing the email to be deleted one character at a time.
> > > > >
> > > > > The problem is, not only did this destroy the email reply, but it
> > > > > also destroyed my train of thought for the reply as well through
> > > > > the panic of trying to stop the entire email being deleted.
> > > > >
> > > > > I don't think this is a hardware issue - I think there's a problem
> > > > > in the input handling somewhere in the stack of kernel, Xorg,
> > > > > whatever multiple input libraries make up modern systems, and KDE.
> > > > >
> > > > > I did check the logs. Nothing in the kernel messages that suggests
> > > > > a problem. Nothing in Xorg's logs (which are difficult to tie up
> > > > > because it doesn't use real timestamps that one can relate to real
> > > > > time.) There's no longer any ~/.xsession-errors logfile for logging
> > > > > the stuff below Xorg.
> > > > >
> > > > > I'm running Debian Stable here - kernel 6.1.0-34-amd64, X.Org X Server
> > > > > 1.21.1.7, KDE Plasma (5.27.5, frameworks 5.103.0, QT 5.15.8).
> > > >
> > > > I'll also add that The Carbon X1, being a laptop, its built-in keyboard
> > > > uses the i8042:
> > > >
> > > > [ 1.698156] i8042: PNP: PS/2 Controller [PNP0303:KBD,PNP0f13:MOU] at 0x60,0x64 irq 1,12
> > > > [ 1.698543] i8042: Warning: Keylock active
> > > > [ 1.700170] serio: i8042 KBD port at 0x60,0x64 irq 1
> > > > [ 1.700174] serio: i8042 AUX port at 0x60,0x64 irq 12
> > > > [ 1.700271] mousedev: PS/2 mouse device common for all mice
> > > > [ 1.702951] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
> > > >
> > > > I don't have the HP laptop with me to check what that was using.
> > > >
> > > > The mysterious thing is "Keylock active" - clearly it isn't because I
> > > > can write this email typing on that very keyboard. However, I wonder
> > > > if it needs i8042_unlock=1 to set I8042_CTR_IGNKEYLOCK.
Just ignore this message, it is harmless and trying to flip the bit
might confuse the emulation even more. Maybe we should lower the
severity of it to debug.
That said I do not see it on my Carbon (neither v5 nor v12, can't check
v9 because it is at home)... What version of Carbon do you have? Do you
have up-to-date BIOS/EC?
> > > >
> > > > Unfortunately, it's probably going to take a year on the Carbon X1
> > > > to work out if this makes any difference.
> > > >
> > > > > Anyone else seeing this kind of behaviour - if so, what are you
> > > > > using?
> > >
> > > It just happened to me as I was typing this very email (key 'd' got
> > > stuck, nothing could un-stick it, couldn't move the mouse cursor but
> > > mouse-click events did work, had to suspend/resume the laptop to fix
> > > that)
This is weird and suggests that the breakage happens up the stack from
the kernel (or down in the firmware). Mouse clicks and mouse movement is
delivered as part of a mouse packet, so if there are button clicks there
will also be movement, they are not separate. If the cursor is not
reacting that means desktop environment is not handling input properly.
> >
> > 'd' can be quite disastrous if you're using vi and you're in command
> > mode!
> >
> > > Got the same "Keylock active" warning at boot :
> > >
> > > [ 0.916750] i8042: PNP: PS/2 Controller [PNP0303:PS2K,PNP0f13:PS2M] at 0x60,0x64 irq 1,12
> > > [ 0.917210] i8042: Warning: Keylock active
> > > [ 0.920087] serio: i8042 KBD port at 0x60,0x64 irq 1
> > > [ 0.920090] serio: i8042 AUX port at 0x60,0x64 irq 12
> > >
> > > Nothing in the kernel logs when the key got stuck.
> > >
> > > Laptop is a Dell XPS 15 9510, Running Fedora 41 but I saw this issue
> > > before, kernel 6.14.4-200.fc41.x86_64, Wayland-based, Gnome 47.
> > >
> > > Hopefully this helps a bit narrowing this down, I have a fairly
> > > different userspace stack and kernel version, but we do have the same
> > > driver involved and same keylock warning...
> >
> > Could you try booting with i8042_unlock=1 and see whether that makes any
> > difference please?
>
> I'll try this out indeed :)
>
> > I've added that to my grub config in preparation for rebooting, but even
> > if I booted now, I suspect it'll be some time before I have any useful
> > result.
> >
> > How often do you see the problem?
>
> It's very sporadic, sometimes I'll see this behaviour multiple times a
> day, and then nothing for weeks... This laptop has been very quirky
> ever since I got it, so I categorized that as a "yet another XPS 9510
> broken behaviour", but this has been a recurrent thing for multiple
> years...
>
> So, same as you, it'll take a long time for me to say with some amount
> of certainty that 'i8042_unlock=1' has a beneficial effect, of
> course unless I see the problem happen again in the meantime.
The kernel does drop input events if userspace is unable to read buffers
quickly enough. It notifies userspace by queuing special
EV_SYN/SYN_DROPPED event and userspace is supposed to query the full
device state upon receiving it to figure out what to do. I doubt we are
running into this with keyboards, but maybe we should add some logging
there to rule this out.
I'll add Peter and Benjamin to this thread in case they have ideas.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 17:23 ` Dmitry Torokhov
@ 2025-05-07 17:46 ` Russell King (Oracle)
0 siblings, 0 replies; 35+ messages in thread
From: Russell King (Oracle) @ 2025-05-07 17:46 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linus Torvalds, Maxime Chevallier, Andrew Lunn, Woojung Huh,
Vladimir Oltean, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-input, linux-kernel
On Wed, May 07, 2025 at 10:23:37AM -0700, Dmitry Torokhov wrote:
> On Wed, May 07, 2025 at 07:46:03AM -0700, Linus Torvalds wrote:
> > On Wed, 7 May 2025 at 04:51, Maxime Chevallier
> > <maxime.chevallier@bootlin.com> wrote:
> > >
> > > So, same as you, it'll take a long time for me to say with some amount
> > > of certainty that 'i8042_unlock=1' has a beneficial effect, of
> > > course unless I see the problem happen again in the meantime.
> >
> > Christ. You'd expect that any i8042 issues had been fixed long ago,
> > but the problem is that the chip doesn't necessarily even exist in
> > modern platforms, and everybody just fakes it.
>
> It has not existed as a real chip for more than 20 years I believe. It's
> all faked in firmware and embedded controllers that fake it in their
> firmwares.
>
> And newer firmware tend to implement less and less of it, just what OS
> that devices that ship with it needs.
>
> >
> > So the platform presumably still has hardware support for it, but it's
> > mostly in the form of "take a trap when accessing the legacy keyboard
> > ports, and fake it in firmware".
> >
> > Although it doesn't help that there are literally decades of clone
> > chips and hacky real hardware that extended on the i8042 in various
> > more-or-less compatible ways.
> >
> > Which makes all of these things almost entirely undebuggable.
> >
> > I'm surprised the XPS9510 would be particularly troublesome - I've had
> > an XPS for years (older version, obviously) with no issues outside of
> > WiFi sometimes acting up. But random firmware...
> >
> > I doubt it's "keylock active", but who knows. I get that on my xps
> > too, it's a random bit that doesn't really mean much. But - because of
> > all the reasons above - who knows...
>
> It is typically harmless and whats more trying to "unlock" 8042 when it
> reports being locked might confuse 8042 emulation.
So I think the summary of this thread is... laptop keyboards are
unreliable, and it's a lottery whether any particular laptop works
or remains working over firmware updates.
Surely that essentially means, laptops are basically unreliable
devices?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 17:45 ` Dmitry Torokhov
@ 2025-05-07 18:18 ` Russell King (Oracle)
2025-05-08 5:10 ` Peter Hutterer
0 siblings, 1 reply; 35+ messages in thread
From: Russell King (Oracle) @ 2025-05-07 18:18 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Maxime Chevallier, Linus Torvalds, Andrew Lunn, Woojung Huh,
Vladimir Oltean, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, linux-input, linux-kernel,
Peter Hutterer, Benjamin Tissoires
On Wed, May 07, 2025 at 10:45:48AM -0700, Dmitry Torokhov wrote:
> On Wed, May 07, 2025 at 01:51:26PM +0200, Maxime Chevallier wrote:
> > On Wed, 7 May 2025 12:44:24 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >
> > > Hi Maxime,
> > >
> > > On Wed, May 07, 2025 at 03:32:36PM +0200, Maxime Chevallier wrote:
> > > > Hi Russell,
> > > >
> > > > On Wed, 7 May 2025 10:59:21 +0100
> > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > >
> > > > > On Wed, May 07, 2025 at 10:23:17AM +0100, Russell King (Oracle) wrote:
> > > > > > [Sorry for going off topic here - changed the Cc list, added Linus,
> > > > > > changed the subject.]
> > > > > >
> > > > > > On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> > > > > > > On Wed, 7 May 2025 09:31:48 +0100
> > > > > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > > > > > [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...
> > > > > >
> > > > > > I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
> > > > > > laptop I had previously (normally with ctrl-F keys). However, hitting
> > > > > > ctrl/shift/alt would stop it.
> > > > > >
> > > > > > This is the first time I've seen the behaviour with the Carbon X1
> > > > > > laptop, but this was way more severe. No key would stop it. Trying to
> > > > > > move the focus using the trackpad/nipple had any effect. Meanwhile
> > > > > > the email was being deleted one character at a time. So I shut the
>
> If we indeed lost a key release event somewhere the way to "restore" it
> is to hit the stuck key again. Then we should get press/release with
> press most likely being ignored and release achieving the desired
> result. Of course that will not help if embedded controller is confused.
I tried pressing every key...
> > > > > The mysterious thing is "Keylock active" - clearly it isn't because I
> > > > > can write this email typing on that very keyboard. However, I wonder
> > > > > if it needs i8042_unlock=1 to set I8042_CTR_IGNKEYLOCK.
>
> Just ignore this message, it is harmless and trying to flip the bit
> might confuse the emulation even more. Maybe we should lower the
> severity of it to debug.
>
> That said I do not see it on my Carbon (neither v5 nor v12, can't check
> v9 because it is at home)... What version of Carbon do you have? Do you
> have up-to-date BIOS/EC?
Neither did I see a problem until today, and I've been using the laptop
since October 2024, and this is the first time it's had an issue.
Looking at fwupd, it has an Intel ME update pending (1.32.2418 to
1.35.2557). I can't find a way to get any update history beyond
that out of fwupd and fwupd doesn't seem to log to journald what
it's doing.
> > > > It just happened to me as I was typing this very email (key 'd' got
> > > > stuck, nothing could un-stick it, couldn't move the mouse cursor but
> > > > mouse-click events did work, had to suspend/resume the laptop to fix
> > > > that)
>
> This is weird and suggests that the breakage happens up the stack from
> the kernel (or down in the firmware). Mouse clicks and mouse movement is
> delivered as part of a mouse packet, so if there are button clicks there
> will also be movement, they are not separate. If the cursor is not
> reacting that means desktop environment is not handling input properly.
So could we be looking at an Xorg bug?
> The kernel does drop input events if userspace is unable to read buffers
> quickly enough. It notifies userspace by queuing special
> EV_SYN/SYN_DROPPED event and userspace is supposed to query the full
> device state upon receiving it to figure out what to do. I doubt we are
> running into this with keyboards, but maybe we should add some logging
> there to rule this out.
>
> I'll add Peter and Benjamin to this thread in case they have ideas.
I'm thinking of leaving evtest running in a terminal, so its output
can be inspected if it happens again. One issue though is the
timestamps aren't readable, but I'm sure with a bit of perl
post-processing that could be fixed.
That would allow an answer to "is it kernel or firmware" vs
"userspace".
The problem is - if it's taken 7-ish months to show for me, it's
likely that evtest won't be running when it next happens (as there
will be needs to reboot for kernel upgrades/firmware upgrades in
that time.) Really, it needs to be something like an automatically
started at boot e.g. evtest inside a detached screen session.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome
2025-05-07 11:44 ` Russell King (Oracle)
2025-05-07 11:51 ` Maxime Chevallier
@ 2025-05-07 20:46 ` Holger Hoffstätte
2025-05-07 21:11 ` Dmitry Torokhov
2025-05-07 22:06 ` Russell King (Oracle)
1 sibling, 2 replies; 35+ messages in thread
From: Holger Hoffstätte @ 2025-05-07 20:46 UTC (permalink / raw)
To: Russell King (Oracle), Maxime Chevallier
Cc: Linus Torvalds, Andrew Lunn, Woojung Huh, Vladimir Oltean,
Heiner Kallweit, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Dmitry Torokhov, linux-input, linux-kernel
On 2025-05-07 13:44, Russell King (Oracle) wrote:
> Could you try booting with i8042_unlock=1 and see whether that makes any
> difference please?
It did not help - just had another runaway event with that setting,
on my ca. 2021 Thinkpad L14. Had the symptom for as long as I have
had this machine.
We've been tracking this problem in Gentoo since late 2022, see
https://bugs.gentoo.org/873163 and none of the suggested options
for i8042 really make a difference. In my case I almost always get
the stuck key events when using the cursor keys for scrolling in a
web browser. Sometimes once a month, sometimes twice a day.
Fwiw it's not necessary to reboot; suspend/resume fixes it,
as in close/reopen the lid if you have that configured.
thanks
Holger
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome
2025-05-07 20:46 ` [BUG] Stuck key syndrome Holger Hoffstätte
@ 2025-05-07 21:11 ` Dmitry Torokhov
2025-05-07 22:06 ` Russell King (Oracle)
1 sibling, 0 replies; 35+ messages in thread
From: Dmitry Torokhov @ 2025-05-07 21:11 UTC (permalink / raw)
To: Holger Hoffstätte
Cc: Russell King (Oracle), Maxime Chevallier, Linus Torvalds,
Andrew Lunn, Woojung Huh, Vladimir Oltean, Heiner Kallweit,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-input, linux-kernel
On Wed, May 07, 2025 at 10:46:35PM +0200, Holger Hoffstätte wrote:
> On 2025-05-07 13:44, Russell King (Oracle) wrote:
> > Could you try booting with i8042_unlock=1 and see whether that makes any
> > difference please?
>
> It did not help - just had another runaway event with that setting,
> on my ca. 2021 Thinkpad L14. Had the symptom for as long as I have
> had this machine.
>
> We've been tracking this problem in Gentoo since late 2022, see
> https://bugs.gentoo.org/873163 and none of the suggested options
> for i8042 really make a difference. In my case I almost always get
> the stuck key events when using the cursor keys for scrolling in a
> web browser. Sometimes once a month, sometimes twice a day.
>
> Fwiw it's not necessary to reboot; suspend/resume fixes it,
> as in close/reopen the lid if you have that configured.
So looking at your logs in gentoo bugzilla we see:
>>> It is around 1 second later that I realise the J key has died.
>>> Now I sit and watch for a few seconds before closing the lid.
Event: time 1664975487.559043, -------------- SYN_REPORT ------------
Event: time 1664975487.591980, type 4 (EV_MSC), code 4 (MSC_SCAN), value 24
Event: time 1664975487.591980, type 1 (EV_KEY), code 36 (KEY_J), value 2
Event: time 1664975487.591980, -------------- SYN_REPORT ------------
Event: time 1664975487.624955, type 4 (EV_MSC), code 4 (MSC_SCAN), value 24
Event: time 1664975487.624955, type 1 (EV_KEY), code 36 (KEY_J), value 2
Event: time 1664975487.624955, -------------- SYN_REPORT ------------
Event: time 1664975487.657800, type 4 (EV_MSC), code 4 (MSC_SCAN), value 24
Event: time 1664975487.657800, type 1 (EV_KEY), code 36 (KEY_J), value 2
Event: time 1664975487.657800, -------------- SYN_REPORT ------------
Because I see the MSC_SCAN events this means you are not using
atkbd.softrepeat for software-emulated autorepeat and is using the
hardware autorepeat function (which is the default). In this mode
keyboard controller repeatedly sends the scancode for the pressed key
and kernel reports it. We know that interrupts are working because we do
get scancodes from i8042 and from the kernel POV the key is still
pressed because [piece of software that emulates] i8042 tells it so. :(
...
Event: time 1664975493.812157, -------------- SYN_REPORT ------------
Event: time 1664975495.120717, type 1 (EV_KEY), code 36 (KEY_J), value 0
>>> It is around here that the computer resumes from suspend.
This is software-emulated key release that we do as part of suspend
process. And afterwards firmware gets jolted into its senses by suspend
and starts working...
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome
2025-05-07 20:46 ` [BUG] Stuck key syndrome Holger Hoffstätte
2025-05-07 21:11 ` Dmitry Torokhov
@ 2025-05-07 22:06 ` Russell King (Oracle)
2025-05-07 22:49 ` Linus Torvalds
1 sibling, 1 reply; 35+ messages in thread
From: Russell King (Oracle) @ 2025-05-07 22:06 UTC (permalink / raw)
To: Holger Hoffstätte
Cc: Maxime Chevallier, Linus Torvalds, Andrew Lunn, Woojung Huh,
Vladimir Oltean, Heiner Kallweit, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Dmitry Torokhov, linux-input,
linux-kernel
On Wed, May 07, 2025 at 10:46:35PM +0200, Holger Hoffstätte wrote:
> On 2025-05-07 13:44, Russell King (Oracle) wrote:
> > Could you try booting with i8042_unlock=1 and see whether that makes any
> > difference please?
>
> It did not help - just had another runaway event with that setting,
> on my ca. 2021 Thinkpad L14. Had the symptom for as long as I have
> had this machine.
>
> We've been tracking this problem in Gentoo since late 2022, see
> https://bugs.gentoo.org/873163 and none of the suggested options
> for i8042 really make a difference. In my case I almost always get
> the stuck key events when using the cursor keys for scrolling in a
> web browser. Sometimes once a month, sometimes twice a day.
>
> Fwiw it's not necessary to reboot; suspend/resume fixes it,
> as in close/reopen the lid if you have that configured.
Thanks - it's good to know that I'm not alone with this problem!
I wonder how common it is, I think we're now up to four people.
So it's interesting that Finn's system is AMD vs mine which have
both been Intel based systems, and we seem to have exactly the same
problem. Is it possible that both are using the same firmware for
emulating an i8042?
Also what seems to be interesting is that it afflicts specific keys.
On my old HP Pavilion, it was always Ctrl-F3 which would get stuck
down (which I use to switch to virtual desktop 3 which has my Konsoles
on.) In this case, pressing all of ctrl-shift-alt would clear it.
I've only had it once on the Lenovo Carbon X1 so far, so can't comment
if it's going to be the backspace key every time - there is an Intel ME
firmware update pending (it didn't get installed at the last reboot -
I'd accidentally left the laptop with sleep disabled but no external
power, so it drained the battery and shut itself down which probably
prevented the firmware update being installed.) I believe the Intel ME
deals with the keyboard.
Thanks for adding comment 24... I assume from what you've said above
that comment 23 has proven not to solve it completely, but has it
reduced the frequency at all for you?
What also crosses my mind is that if the i8042 is now emulated by
firmware, is there a replacement interface that the kernel should
instead be using? Surely if the i8042 emulation is as bad as this,
then e.g. under Windows these laptops would be getting very poor
write-ups due to keyboard problems afflicting Windows as well.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome
2025-05-07 22:06 ` Russell King (Oracle)
@ 2025-05-07 22:49 ` Linus Torvalds
0 siblings, 0 replies; 35+ messages in thread
From: Linus Torvalds @ 2025-05-07 22:49 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Holger Hoffstätte, Maxime Chevallier, Andrew Lunn,
Woojung Huh, Vladimir Oltean, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Dmitry Torokhov,
linux-input, linux-kernel
On Wed, 7 May 2025 at 15:07, Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> So it's interesting that Finn's system is AMD vs mine which have
> both been Intel based systems, and we seem to have exactly the same
> problem. Is it possible that both are using the same firmware for
> emulating an i8042?
Might be a BIOS vendor issue.
> Also what seems to be interesting is that it afflicts specific keys.
> On my old HP Pavilion, it was always Ctrl-F3 which would get stuck
> down (which I use to switch to virtual desktop 3 which has my Konsoles
> on.) In this case, pressing all of ctrl-shift-alt would clear it.
So multiple keys being pressed at once can result in confusion
depending on how the keyboard matrix is set up, and pressing multiple
keys then causes ghost reports.
Usually it requires three keys to be pressed simultaneously - and some
really cheap underlying hardware without some basic N-key rollover
protection.
But who knows what can confuse the firmware.
And honestly, it might also be a timing issue. So when you switch
virtual desktops, you end up doing more CPU work, changing timings,
and messing something up in the firmware in the process.
For example, I wouldn't expect firmware to be great about SMP. So
while the i8042 driver serializes everything with i8042_lock, who
knows *where* the firmware runs.
Maybe we could do something like tie irq1 (keyboard) and irq12 ("aux",
aka mouse) to the boot CPU in the hopes that there's less chance of
confusing some firmware that way.
I have no idea what Windows does, and - as usual - that's the case
that gets almost all the testing from vendors.
> What also crosses my mind is that if the i8042 is now emulated by
> firmware, is there a replacement interface that the kernel should
> instead be using?
I don't think there is any documentation - or necessarily commonality
- for the low-level hardware. I would guess that it's probably some
small i2c controller that ends up doing some keypad matrix thing. That
i2c thing *may* do native HID, but it might easily also be just some
custom GPIO expander thing.
I think the touchpad is usually some i2c device, and it is sometimes
accessible both ways. And there's a long history of keyboard problems
when the touchpad is looked at just the wrong way, so those things
most definitely can interact (because the firmware emulation emulates
both).
It's very hard to find hardware information at that level these days.
It's been decades since things like the keyboard matrix was documented
as such....
Linus
^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-05-07 8:54 ` Maxime Chevallier
2025-05-07 9:23 ` [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch) Russell King (Oracle)
@ 2025-05-08 1:41 ` Tristram.Ha
1 sibling, 0 replies; 35+ messages in thread
From: Tristram.Ha @ 2025-05-08 1:41 UTC (permalink / raw)
To: maxime.chevallier
Cc: andrew, Woojung.Huh, olteanv, hkallweit1, davem, edumazet, kuba,
pabeni, UNGLinuxDriver, netdev, linux-kernel, linux
> > > 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.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch)
2025-05-07 18:18 ` Russell King (Oracle)
@ 2025-05-08 5:10 ` Peter Hutterer
0 siblings, 0 replies; 35+ messages in thread
From: Peter Hutterer @ 2025-05-08 5:10 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Dmitry Torokhov, Maxime Chevallier, Linus Torvalds, Andrew Lunn,
Woojung Huh, Vladimir Oltean, Heiner Kallweit, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-input,
linux-kernel, Benjamin Tissoires
On Wed, May 07, 2025 at 07:18:37PM +0100, Russell King (Oracle) wrote:
> On Wed, May 07, 2025 at 10:45:48AM -0700, Dmitry Torokhov wrote:
> > On Wed, May 07, 2025 at 01:51:26PM +0200, Maxime Chevallier wrote:
> > > On Wed, 7 May 2025 12:44:24 +0100
> > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > >
> > > > Hi Maxime,
> > > >
> > > > On Wed, May 07, 2025 at 03:32:36PM +0200, Maxime Chevallier wrote:
> > > > > Hi Russell,
> > > > >
> > > > > On Wed, 7 May 2025 10:59:21 +0100
> > > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > > On Wed, May 07, 2025 at 10:23:17AM +0100, Russell King (Oracle) wrote:
> > > > > > > [Sorry for going off topic here - changed the Cc list, added Linus,
> > > > > > > changed the subject.]
> > > > > > >
> > > > > > > On Wed, May 07, 2025 at 10:54:57AM +0200, Maxime Chevallier wrote:
> > > > > > > > On Wed, 7 May 2025 09:31:48 +0100
> > > > > > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > > > > > > [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...
> > > > > > >
> > > > > > > I've had the "stuck key" behaviour with the HP Pavilion 15-au185sa
> > > > > > > laptop I had previously (normally with ctrl-F keys). However, hitting
> > > > > > > ctrl/shift/alt would stop it.
> > > > > > >
> > > > > > > This is the first time I've seen the behaviour with the Carbon X1
> > > > > > > laptop, but this was way more severe. No key would stop it. Trying to
> > > > > > > move the focus using the trackpad/nipple had any effect. Meanwhile
> > > > > > > the email was being deleted one character at a time. So I shut the
> >
> > If we indeed lost a key release event somewhere the way to "restore" it
> > is to hit the stuck key again. Then we should get press/release with
> > press most likely being ignored and release achieving the desired
> > result. Of course that will not help if embedded controller is confused.
>
> I tried pressing every key...
ftr, any key autorepeat in XKB (or libxkbcommon) is cancelled when
another key arrives so this does indeed hint at a firmware/controller
issue.
> > > > > > The mysterious thing is "Keylock active" - clearly it isn't because I
> > > > > > can write this email typing on that very keyboard. However, I wonder
> > > > > > if it needs i8042_unlock=1 to set I8042_CTR_IGNKEYLOCK.
> >
> > Just ignore this message, it is harmless and trying to flip the bit
> > might confuse the emulation even more. Maybe we should lower the
> > severity of it to debug.
> >
> > That said I do not see it on my Carbon (neither v5 nor v12, can't check
> > v9 because it is at home)... What version of Carbon do you have? Do you
> > have up-to-date BIOS/EC?
>
> Neither did I see a problem until today, and I've been using the laptop
> since October 2024, and this is the first time it's had an issue.
>
> Looking at fwupd, it has an Intel ME update pending (1.32.2418 to
> 1.35.2557). I can't find a way to get any update history beyond
> that out of fwupd and fwupd doesn't seem to log to journald what
> it's doing.
>
> > > > > It just happened to me as I was typing this very email (key 'd' got
> > > > > stuck, nothing could un-stick it, couldn't move the mouse cursor but
> > > > > mouse-click events did work, had to suspend/resume the laptop to fix
> > > > > that)
> >
> > This is weird and suggests that the breakage happens up the stack from
> > the kernel (or down in the firmware). Mouse clicks and mouse movement is
> > delivered as part of a mouse packet, so if there are button clicks there
> > will also be movement, they are not separate. If the cursor is not
> > reacting that means desktop environment is not handling input properly.
>
> So could we be looking at an Xorg bug?
>
> > The kernel does drop input events if userspace is unable to read buffers
> > quickly enough. It notifies userspace by queuing special
> > EV_SYN/SYN_DROPPED event and userspace is supposed to query the full
> > device state upon receiving it to figure out what to do. I doubt we are
> > running into this with keyboards, but maybe we should add some logging
> > there to rule this out.
> >
> > I'll add Peter and Benjamin to this thread in case they have ideas.
>
> I'm thinking of leaving evtest running in a terminal, so its output
> can be inspected if it happens again. One issue though is the
> timestamps aren't readable, but I'm sure with a bit of perl
> post-processing that could be fixed.
$ screen sudo libinput record --autorestart=5 -o keyboard.yml
Select the keyboard devce and off you go.
This will create a keyboard.yml.$DATETIME file and after 5 seconds of no
events it will rotate to the next file. This means you can leave this
running for months and when it happens you stop typing for 5 seconds and
then look at the most recent (usually) two files for epiphanies.
Bonus points for ssh-ing in and killing the process remotely because
then you don't have interference of whatever
If the last event you get is a key down event the issue is in the
firmware and userspace does the right thing. In that case your
logs will be full of EV_KEY KEY_FOO with a value of 2 (kernel repeat).
If the last event you get is a key up but the desktop still repeats
then the issue is in that part of the stack. In that case the log should
be empty after you stopped typing.
If you get a SYN_DROPPED and the desktop does the wrong thing the issue
is in libinput because it should handle that transparently (same with
the Xorg evdev driver if you're using that).
Optionally: --show-keycodes but adding this means you will leak
all your key presses into the logs (without this all alphanumeric keys
are recorded as KEY_A). So maybe only use that for entertainment value.
Optionally: provide the device node as arg (see below)
> That would allow an answer to "is it kernel or firmware" vs
> "userspace".
>
> The problem is - if it's taken 7-ish months to show for me, it's
> likely that evtest won't be running when it next happens (as there
> will be needs to reboot for kernel upgrades/firmware upgrades in
> that time.) Really, it needs to be something like an automatically
> started at boot e.g. evtest inside a detached screen session.
Well, that bit I leave to you to figure out because it'll also require
scripting to find the keyboard's device node automatically so you can
pass it to libinput record via the .unit file, or cron job script, or
whatever.
Either way, this should at least make it easier to catch the issue when
it occurs.
Cheers,
Peter
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch
2025-05-07 0:09 [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch Tristram.Ha
2025-05-07 7:44 ` Maxime Chevallier
@ 2025-05-10 0:41 ` Jakub Kicinski
1 sibling, 0 replies; 35+ messages in thread
From: Jakub Kicinski @ 2025-05-10 0:41 UTC (permalink / raw)
To: Tristram.Ha
Cc: Andrew Lunn, Woojung Huh, Russell King, Vladimir Oltean,
Heiner Kallweit, Maxime Chevallier, David S. Miller, Eric Dumazet,
Paolo Abeni, UNGLinuxDriver, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2025-05-10 0:41 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-07 0:09 [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch Tristram.Ha
2025-05-07 7:44 ` Maxime Chevallier
2025-05-07 8:31 ` Russell King (Oracle)
2025-05-07 8:54 ` Maxime Chevallier
2025-05-07 9:23 ` [BUG] Stuck key syndrome (was: Re: [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch) Russell King (Oracle)
2025-05-07 9:59 ` Russell King (Oracle)
2025-05-07 13:32 ` Maxime Chevallier
2025-05-07 11:44 ` Russell King (Oracle)
2025-05-07 11:51 ` Maxime Chevallier
2025-05-07 14:46 ` Linus Torvalds
2025-05-07 17:23 ` Dmitry Torokhov
2025-05-07 17:46 ` Russell King (Oracle)
2025-05-07 17:45 ` Dmitry Torokhov
2025-05-07 18:18 ` Russell King (Oracle)
2025-05-08 5:10 ` Peter Hutterer
2025-05-07 20:46 ` [BUG] Stuck key syndrome Holger Hoffstätte
2025-05-07 21:11 ` Dmitry Torokhov
2025-05-07 22:06 ` Russell King (Oracle)
2025-05-07 22:49 ` Linus Torvalds
2025-05-08 1:41 ` [PATCH net-next v2] net: dsa: microchip: Add SGMII port support to KSZ9477 switch Tristram.Ha
2025-05-10 0:41 ` Jakub Kicinski
-- strict thread matches above, loose matches on Subject: below --
2025-01-14 2:47 Tristram.Ha
2025-01-14 16:09 ` Vladimir Oltean
2025-01-14 16:53 ` Vladimir Oltean
2025-01-15 10:10 ` Maxime Chevallier
2025-01-17 0:56 ` Tristram.Ha
2025-01-17 13:36 ` Andrew Lunn
2025-01-18 0:59 ` Tristram.Ha
2025-01-18 1:26 ` Vladimir Oltean
2025-01-18 4:07 ` Tristram.Ha
2025-01-18 20:00 ` Andrew Lunn
2025-01-21 0:28 ` Tristram.Ha
2025-01-21 3:08 ` Tristram.Ha
2025-01-17 16:18 ` Vladimir Oltean
2025-01-17 0:51 ` Tristram.Ha
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.