[PATCH] irqchip/ath79-cpu: Flush every write buffer of a shared interrupt

[PATCH] irqchip/ath79-cpu: Flush every write buffer of a shared interrupt

[Rosen Penev]

On AR934x and QCA953x, the WMAC and the PCIe root complex share CPU
interrupt 2, but each writes to DDR through its own write buffer, and
both have to be flushed before their handlers can see the data. The
driver keeps one write buffer per interrupt, so the last one listed in
the device tree wins.

Keep a mask of write buffers per interrupt instead, and flush all of
them. A flush is a few accesses to the DDR controller, which costs much
less than splitting the interrupt by source, as OpenWrt does with a
cascaded interrupt controller.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/irqchip/irq-ath79-cpu.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-ath79-cpu.c b/drivers/irqchip/irq-ath79-cpu.c
index 9b7273a7f8ce..6bbbdf7ff26e 100644
--- a/drivers/irqchip/irq-ath79-cpu.c
+++ b/drivers/irqchip/irq-ath79-cpu.c
@@ -10,6 +10,7 @@
  *  Parts of this file are based on Atheros' 2.6.15/2.6.31 BSP
  */
 
+#include <linux/bitops.h>
 #include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/of.h>
@@ -24,17 +25,17 @@
  * Issue a flush in the handlers to ensure that the driver sees
  * the update.
  *
- * This array map the interrupt lines to the DDR write buffer channels.
+ * This array maps the interrupt lines to a mask of the DDR write buffer
+ * channels to flush. A line shared by several devices, like the WMAC and
+ * the PCIe root complex on AR934x, flushes the channels of all of them.
  */
 
-static unsigned irq_wb_chan[8] = {
-	-1, -1, -1, -1, -1, -1, -1, -1,
-};
+static u32 irq_wb_chans[8];
 
 asmlinkage void plat_irq_dispatch(void)
 {
-	unsigned long pending;
-	int irq;
+	unsigned long pending, chans;
+	int irq, chan;
 
 	pending = read_c0_status() & read_c0_cause() & ST0_IM;
 
@@ -46,8 +47,11 @@ asmlinkage void plat_irq_dispatch(void)
 	pending >>= CAUSEB_IP;
 	while (pending) {
 		irq = fls(pending) - 1;
-		if (irq < ARRAY_SIZE(irq_wb_chan) && irq_wb_chan[irq] != -1)
-			ath79_ddr_wb_flush(irq_wb_chan[irq]);
+		if (irq < ARRAY_SIZE(irq_wb_chans)) {
+			chans = irq_wb_chans[irq];
+			for_each_set_bit(chan, &chans, 32)
+				ath79_ddr_wb_flush(chan);
+		}
 		do_IRQ(MIPS_CPU_IRQ_BASE + irq);
 		pending &= ~BIT(irq);
 	}
@@ -58,7 +62,7 @@ static int __init ar79_cpu_intc_of_init(
 {
 	int err, i, count;
 
-	/* Fill the irq_wb_chan table */
+	/* Fill the irq_wb_chans table */
 	count = of_count_phandle_with_args(
 		node, "qca,ddr-wb-channels", "#qca,ddr-wb-channel-cells");
 
@@ -68,7 +72,7 @@ static int __init ar79_cpu_intc_of_init(
 
 		of_property_read_u32_index(
 			node, "qca,ddr-wb-channel-interrupts", i, &irq);
-		if (irq >= ARRAY_SIZE(irq_wb_chan))
+		if (irq >= ARRAY_SIZE(irq_wb_chans))
 			continue;
 
 		err = of_parse_phandle_with_args(
@@ -78,7 +82,10 @@ static int __init ar79_cpu_intc_of_init(
 		if (err)
 			return err;
 
-		irq_wb_chan[irq] = args.args[0];
+		if (args.args[0] >= 32)
+			return -EINVAL;
+
+		irq_wb_chans[irq] |= BIT(args.args[0]);
 	}
 
 	return mips_cpu_irq_of_init(node, parent);
-- 
2.56.0

Re: [PATCH] irqchip/ath79-cpu: Flush every write buffer of a shared interrupt

[Thomas Gleixner]

On Fri, Nov 06 2026 at 14:12, Rosen Penev wrote:
>  
> -static unsigned irq_wb_chan[8] = {
> -	-1, -1, -1, -1, -1, -1, -1, -1,
> -};
> +static u32 irq_wb_chans[8];
>  
>  asmlinkage void plat_irq_dispatch(void)
>  {
> -	unsigned long pending;
> -	int irq;
> +	unsigned long pending, chans;
> +	int irq, chan;
>  
>  	pending = read_c0_status() & read_c0_cause() & ST0_IM;
>  
> @@ -46,8 +47,11 @@ asmlinkage void plat_irq_dispatch(void)
>  	pending >>= CAUSEB_IP;
>  	while (pending) {
>  		irq = fls(pending) - 1;
> -		if (irq < ARRAY_SIZE(irq_wb_chan) && irq_wb_chan[irq] != -1)
> -			ath79_ddr_wb_flush(irq_wb_chan[irq]);
> +		if (irq < ARRAY_SIZE(irq_wb_chans)) {
> +			chans = irq_wb_chans[irq];
> +			for_each_set_bit(chan, &chans, 32)
> +				ath79_ddr_wb_flush(chan);

So the CPU provides 8 interrupts and 32 sources, where each source has
its own specific flush register. There is some distribution mechanism
(firmware configured or fixed in hardware) which maps a interrupt source
to a CPU interrupt. When two sources are mapped to the same CPU
interrupt then they share the interrupt line, i.e. the handlers are
requested with IRQF_SHARED, _and_ both flush registers have to be
written to so that the interrupt handler can observe the data and figure
out whether it should handle it or not. Correct?

I kinda decoded this from your change log and tried to map it to the
code. It looks correct, but for heavens sake this needs a bit fat
comment explaining the magic behind all of this. Even for your own sake
as you will have forgotten the gory details three month down the road.

Btw, as this is the interrupt hotpath, this for_each_set_bit() loop is
pretty inefficient when there is more than one interrupt source
associated with a CPU interrupt.

What you want (as a patch on top) is:

ath79_ddr_wb_flush_many(unsigned long sources)
{
	__ath79_ddr_wb_flush_many(sources);
        // Document WHY this needs to be done twice
        // The existing comment is useless...
	__ath79_ddr_wb_flush_many(sources);
}

static __ath79_ddr_wb_flush_many(unsigned long sources)
{
	u32 __iomem *base = ath79_ddr_wb_flush_base;

	for_each_set_bit(idx, &sources, 32)
        	__raw_writel(0x1, base + idx)

	for_each_set_bit(idx, &sources, 32) {
        	while (__raw_readl(base + idx) & 0x1)
                	;
	}
}

or something like that. For a single interrupt it's exactly the same
outcome, but for multiple interrupts mapped to one CPU interrupt there
is an obvious speedup potential, no?

Thanks,

        tglx

Re: [PATCH] irqchip/ath79-cpu: Flush every write buffer of a shared interrupt

[Rosen Penev]

On Sun, Nov 8, 2026 at 12:41 PM Thomas Gleixner <tglx@kernel.org> wrote:
>
> On Fri, Nov 06 2026 at 14:12, Rosen Penev wrote:
> >
> > -static unsigned irq_wb_chan[8] = {
> > -     -1, -1, -1, -1, -1, -1, -1, -1,
> > -};
> > +static u32 irq_wb_chans[8];
> >
> >  asmlinkage void plat_irq_dispatch(void)
> >  {
> > -     unsigned long pending;
> > -     int irq;
> > +     unsigned long pending, chans;
> > +     int irq, chan;
> >
> >       pending = read_c0_status() & read_c0_cause() & ST0_IM;
> >
> > @@ -46,8 +47,11 @@ asmlinkage void plat_irq_dispatch(void)
> >       pending >>= CAUSEB_IP;
> >       while (pending) {
> >               irq = fls(pending) - 1;
> > -             if (irq < ARRAY_SIZE(irq_wb_chan) && irq_wb_chan[irq] != -1)
> > -                     ath79_ddr_wb_flush(irq_wb_chan[irq]);
> > +             if (irq < ARRAY_SIZE(irq_wb_chans)) {
> > +                     chans = irq_wb_chans[irq];
> > +                     for_each_set_bit(chan, &chans, 32)
> > +                             ath79_ddr_wb_flush(chan);
>
> So the CPU provides 8 interrupts and 32 sources, where each source has
> its own specific flush register. There is some distribution mechanism
> (firmware configured or fixed in hardware) which maps a interrupt source
> to a CPU interrupt. When two sources are mapped to the same CPU
> interrupt then they share the interrupt line, i.e. the handlers are
> requested with IRQF_SHARED, _and_ both flush registers have to be
> written to so that the interrupt handler can observe the data and figure
> out whether it should handle it or not. Correct?
Almost. The 32 is only the width of the mask. The flush registers
belong to the DMA masters, not to interrupt sources: each master (GE0,
GE1, USB, PCIe, WMAC) writes to DDR through its own write buffer in the
DDR controller, and each buffer has its own flush register. AR934x has
five of them.

The wiring of the devices to IP2-IP6 is fixed in hardware; the DT only
describes it. IP0/IP1 are the software interrupts and IP7 is the timer.

The flush is done in plat_irq_dispatch() before do_IRQ(), not by the
handlers. So for the shared IP2 on AR934x, both the WMAC and the PCIe
buffers are flushed, then ath9k and the PCIe controller driver (both
IRQF_SHARED) each check their own status and return IRQ_NONE if it was
not theirs. Otherwise, yes, that's it.

>
> I kinda decoded this from your change log and tried to map it to the
> code. It looks correct, but for heavens sake this needs a bit fat
> comment explaining the magic behind all of this. Even for your own sake
> as you will have forgotten the gory details three month down the road.
Will add.
>
> Btw, as this is the interrupt hotpath, this for_each_set_bit() loop is
> pretty inefficient when there is more than one interrupt source
> associated with a CPU interrupt.
>
> What you want (as a patch on top) is:
>
> ath79_ddr_wb_flush_many(unsigned long sources)
> {
>         __ath79_ddr_wb_flush_many(sources);
>         // Document WHY this needs to be done twice
>         // The existing comment is useless...
>         __ath79_ddr_wb_flush_many(sources);
> }
>
> static __ath79_ddr_wb_flush_many(unsigned long sources)
> {
>         u32 __iomem *base = ath79_ddr_wb_flush_base;
>
>         for_each_set_bit(idx, &sources, 32)
>                 __raw_writel(0x1, base + idx)
>
>         for_each_set_bit(idx, &sources, 32) {
>                 while (__raw_readl(base + idx) & 0x1)
>                         ;
>         }
> }
>
> or something like that. For a single interrupt it's exactly the same
> outcome, but for multiple interrupts mapped to one CPU interrupt there
> is an obvious speedup potential, no?
This gets hairy now. arch/ code gets involved. I'll sent as a series.
>
> Thanks,
>
>         tglx