[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