KSZ8463's interrupt scheme differs from the others KSZ swicthes. Its
global interrupt handling is done through an 'enable irq' register
instead of a 'mask irq' one, so the bit logic to enable/disable
interrupt is reversed. Also its interrupts registers are 16-bits
registers and don't have the same address.
Add ksz8463-specific global interrupt setup function that still relies
on the ksz_irq_common_setup().
Add a check on the device type in the irq_chip operations to adjust the
bit logic for KSZ8463
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
---
drivers/net/dsa/microchip/ksz_common.c | 46 ++++++++++++++++++++++++++++------
drivers/net/dsa/microchip/ksz_common.h | 3 +++
2 files changed, 42 insertions(+), 7 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index c517478cc47677544b6523faee113ece036c9ed9..8179415d9b3593d4a9d17661b4ed30007dea9958 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2817,14 +2817,20 @@ static void ksz_irq_mask(struct irq_data *d)
{
struct ksz_irq *kirq = irq_data_get_irq_chip_data(d);
- kirq->masked |= BIT(d->hwirq);
+ if (ksz_is_ksz8463(kirq->dev))
+ kirq->masked &= ~BIT(d->hwirq);
+ else
+ kirq->masked |= BIT(d->hwirq);
}
static void ksz_irq_unmask(struct irq_data *d)
{
struct ksz_irq *kirq = irq_data_get_irq_chip_data(d);
- kirq->masked &= ~BIT(d->hwirq);
+ if (ksz_is_ksz8463(kirq->dev))
+ kirq->masked |= BIT(d->hwirq);
+ else
+ kirq->masked &= ~BIT(d->hwirq);
}
static void ksz_irq_bus_lock(struct irq_data *d)
@@ -2840,7 +2846,10 @@ static void ksz_irq_bus_sync_unlock(struct irq_data *d)
struct ksz_device *dev = kirq->dev;
int ret;
- ret = ksz_write8(dev, kirq->reg_mask, kirq->masked);
+ if (ksz_is_ksz8463(dev))
+ ret = ksz_write16(dev, kirq->reg_mask, kirq->masked);
+ else
+ ret = ksz_write8(dev, kirq->reg_mask, kirq->masked);
if (ret)
dev_err(dev->dev, "failed to change IRQ mask\n");
@@ -2890,14 +2899,18 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id)
unsigned int nhandled = 0;
struct ksz_device *dev;
unsigned int sub_irq;
- u8 data;
+ u16 data;
int ret;
u8 n;
dev = kirq->dev;
- /* Read interrupt status register */
- ret = ksz_read8(dev, kirq->reg_status, &data);
+ /*
+ * Most of the KSZ switches have a 8-bits long interrupt status
+ * register, but the KSZ8463 has a 16-bits long one. The overread here
+ * is safe because we only iterate over kirq->nirqs in the below loop.
+ */
+ ret = ksz_read16(dev, kirq->reg_status, &data);
if (ret)
goto out;
@@ -2939,6 +2952,22 @@ static int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq)
return ret;
}
+static int ksz8463_girq_setup(struct dsa_switch *ds)
+{
+ struct ksz_device *dev = ds->priv;
+ struct ksz_irq *girq = &dev->girq;
+
+ girq->nirqs = 15;
+ girq->reg_mask = KSZ8463_REG_IER;
+ girq->reg_status = KSZ8463_REG_ISR;
+ girq->masked = 0;
+ snprintf(girq->name, sizeof(girq->name), "global_irq");
+
+ girq->irq_num = dev->irq;
+
+ return ksz_irq_common_setup(dev, girq);
+}
+
static int ksz_girq_setup(struct ksz_device *dev)
{
struct ksz_irq *girq = &dev->girq;
@@ -3044,7 +3073,10 @@ static int ksz_setup(struct dsa_switch *ds)
p->learning = true;
if (dev->irq > 0) {
- ret = ksz_girq_setup(dev);
+ if (ksz_is_ksz8463(dev))
+ ret = ksz8463_girq_setup(ds);
+ else
+ ret = ksz_girq_setup(dev);
if (ret)
return ret;
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 929aff4c55de5254defdc1afb52b224b3898233b..67a488a3b5787f93f9e2a9266ce04f6611b56bf8 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -839,6 +839,9 @@ static inline bool ksz_is_sgmii_port(struct ksz_device *dev, int port)
#define KSZ87XX_INT_PME_MASK BIT(4)
/* Interrupt */
+#define KSZ8463_REG_ISR 0x190
+#define KSZ8463_REG_IER 0x192
+
#define REG_SW_PORT_INT_STATUS__1 0x001B
#define REG_SW_PORT_INT_MASK__1 0x001F
--
2.53.0On Wed, Mar 04, 2026 at 11:18:52AM +0100, Bastien Curutchet (Schneider Electric) wrote: > @@ -2890,14 +2899,18 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id) > unsigned int nhandled = 0; > struct ksz_device *dev; > unsigned int sub_irq; > - u8 data; > + u16 data; > int ret; > u8 n; > > dev = kirq->dev; > > - /* Read interrupt status register */ > - ret = ksz_read8(dev, kirq->reg_status, &data); > + /* > + * Most of the KSZ switches have a 8-bits long interrupt status > + * register, but the KSZ8463 has a 16-bits long one. The overread here > + * is safe because we only iterate over kirq->nirqs in the below loop. FWIW, this isn't the only thing making an overread "safe". If the adjacent register also has "clear on read" semantics, that's not good. I can't tell whether that's the case here, though. There are just too many hardware variations to check for. I just wanted to point out that the reasoning is incomplete. > + */ > + ret = ksz_read16(dev, kirq->reg_status, &data); I guess I don't fully understand the KSZ_REGMAP_TABLE() layer, but I have a concern here. Take the normal ksz_girq_setup() case for example, where we set kirq->reg_status = REG_SW_PORT_INT_STATUS__1. With ksz_read8(), we read u8 data from address REG_SW_PORT_INT_STATUS__1. With ksz_read16(), how do we read u16 data? Don't we read data[15:8] from REG_SW_PORT_INT_STATUS__1 and data[7:0] from REG_SW_PORT_INT_STATUS__1 + 1? But then we still look at the 0..kirq->nirq bits, effectively from address REG_SW_PORT_INT_STATUS__1 + 1 now? Or are the registers at address i 16-bits wide without spilling over into address i+1? Because if that's the case, I don't understand the comment/concern about overreading. > if (ret) > goto out;
Hi Vladimir, On 3/5/26 10:56 AM, Vladimir Oltean wrote: > On Wed, Mar 04, 2026 at 11:18:52AM +0100, Bastien Curutchet (Schneider Electric) wrote: >> @@ -2890,14 +2899,18 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id) >> unsigned int nhandled = 0; >> struct ksz_device *dev; >> unsigned int sub_irq; >> - u8 data; >> + u16 data; >> int ret; >> u8 n; >> >> dev = kirq->dev; >> >> - /* Read interrupt status register */ >> - ret = ksz_read8(dev, kirq->reg_status, &data); >> + /* >> + * Most of the KSZ switches have a 8-bits long interrupt status >> + * register, but the KSZ8463 has a 16-bits long one. The overread here >> + * is safe because we only iterate over kirq->nirqs in the below loop. > > FWIW, this isn't the only thing making an overread "safe". > If the adjacent register also has "clear on read" semantics, that's not > good. > > I can't tell whether that's the case here, though. There are just too > many hardware variations to check for. I just wanted to point out that > the reasoning is incomplete. > You're right, I hadn't thought about the 'clear on read' case. I'll use ksz_read16() only for the KSZ8463 to be 100% sure it won't break anything for other switches. Best regards, Bastien
On Thu, Mar 05, 2026 at 01:39:29PM +0100, Bastien Curutchet wrote: > Hi Vladimir, > > On 3/5/26 10:56 AM, Vladimir Oltean wrote: > > On Wed, Mar 04, 2026 at 11:18:52AM +0100, Bastien Curutchet (Schneider Electric) wrote: > > > @@ -2890,14 +2899,18 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id) > > > unsigned int nhandled = 0; > > > struct ksz_device *dev; > > > unsigned int sub_irq; > > > - u8 data; > > > + u16 data; > > > int ret; > > > u8 n; > > > dev = kirq->dev; > > > - /* Read interrupt status register */ > > > - ret = ksz_read8(dev, kirq->reg_status, &data); > > > + /* > > > + * Most of the KSZ switches have a 8-bits long interrupt status > > > + * register, but the KSZ8463 has a 16-bits long one. The overread here > > > + * is safe because we only iterate over kirq->nirqs in the below loop. > > > > FWIW, this isn't the only thing making an overread "safe". > > If the adjacent register also has "clear on read" semantics, that's not > > good. > > > > I can't tell whether that's the case here, though. There are just too > > many hardware variations to check for. I just wanted to point out that > > the reasoning is incomplete. > > > You're right, I hadn't thought about the 'clear on read' case. > > I'll use ksz_read16() only for the KSZ8463 to be 100% sure it won't break > anything for other switches. I'm still on the fence on whether to say this or not, because I don't really want to get so involved in internal driver bookkeeping, but... this driver is just becoming a hell to review, even if I want to concentrate exclusively on correct API use, like I try to do. Linus Walleij has added a new ks8995 driver, which has some overlap with the common ksz driver for the KSZ8 family. Now he wants to remove the overlapping device support: https://lore.kernel.org/netdev/20260219-ks8995-fixups-v3-0-a7fc63fe1916@kernel.org/ Maybe we should go the other way around, migrate KSZ8 support to the ks8995 driver instead? The common ksz driver is becoming just extremely convoluted to handle all hardware variations. Would it help in any way to maintain cleaner code paths, what do you think?
On 3/5/26 1:51 PM, Vladimir Oltean wrote: > On Thu, Mar 05, 2026 at 01:39:29PM +0100, Bastien Curutchet wrote: >> Hi Vladimir, >> >> On 3/5/26 10:56 AM, Vladimir Oltean wrote: >>> On Wed, Mar 04, 2026 at 11:18:52AM +0100, Bastien Curutchet (Schneider Electric) wrote: >>>> @@ -2890,14 +2899,18 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id) >>>> unsigned int nhandled = 0; >>>> struct ksz_device *dev; >>>> unsigned int sub_irq; >>>> - u8 data; >>>> + u16 data; >>>> int ret; >>>> u8 n; >>>> dev = kirq->dev; >>>> - /* Read interrupt status register */ >>>> - ret = ksz_read8(dev, kirq->reg_status, &data); >>>> + /* >>>> + * Most of the KSZ switches have a 8-bits long interrupt status >>>> + * register, but the KSZ8463 has a 16-bits long one. The overread here >>>> + * is safe because we only iterate over kirq->nirqs in the below loop. >>> >>> FWIW, this isn't the only thing making an overread "safe". >>> If the adjacent register also has "clear on read" semantics, that's not >>> good. >>> >>> I can't tell whether that's the case here, though. There are just too >>> many hardware variations to check for. I just wanted to point out that >>> the reasoning is incomplete. >>> >> You're right, I hadn't thought about the 'clear on read' case. >> >> I'll use ksz_read16() only for the KSZ8463 to be 100% sure it won't break >> anything for other switches. > > I'm still on the fence on whether to say this or not, because I don't > really want to get so involved in internal driver bookkeeping, but... > this driver is just becoming a hell to review, even if I want to > concentrate exclusively on correct API use, like I try to do. > > Linus Walleij has added a new ks8995 driver, which has some overlap with > the common ksz driver for the KSZ8 family. Now he wants to remove the > overlapping device support: > https://lore.kernel.org/netdev/20260219-ks8995-fixups-v3-0-a7fc63fe1916@kernel.org/ > > Maybe we should go the other way around, migrate KSZ8 support to the > ks8995 driver instead? The common ksz driver is becoming just extremely > convoluted to handle all hardware variations. Would it help in any way > to maintain cleaner code paths, what do you think? I agree, this driver is extremely convoluted because of all the different hardware it handles. It wasn't easy to fit PTP support for the KSZ8463 into it. And I encountered the same kind of difficulties when adding periodic output support (I have another series ready for this once this PTP support will be merged). Regarding migrating the KSZ8 support into the ksz8995, I think we would quickly hit the same pain points than in ksz_common. Even inside the KSZ8 family we can find a quite big amount of differences between the switches. For instance, both KSZ8463 and KSZ8563 support PTP, they share lots of common registers but their interrupt scheme is very different. I've added Tristram in Cc:, who works at Microchip. Maybe, Tristram, you have some insights about which switches could share code if we decide to split the big ksz_common into several drivers ? Best regards, Bastien
© 2016 - 2026 Red Hat, Inc.