[PATCH v2 0/3] ath79: flush every write buffer of a shared CPU interrupt

[PATCH v2 0/3] ath79: flush every write buffer of a shared CPU 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 in the
DDR controller. The ath79 CPU interrupt controller flushes one write
buffer per interrupt before running its handlers, so only the buffer
listed last in the device tree gets flushed, and the other device can
read stale descriptors.

Patch 1 documents in the binding that a CPU interrupt may be listed
once per write buffer. Patch 2 keeps a mask of write buffers per CPU
interrupt and flushes all of them, and documents how the interrupt
lines, the write buffers and the flush fit together. Patch 3, suggested
by Thomas, starts the flushes of all the buffers of an interrupt before
waiting for any of them, so they drain in parallel, and replaces the
"It must be run twice" comment with what the AR7100 datasheet says
about it.

Patch 3 changes the signature of ath79_ddr_wb_flush() and its call site
in the irqchip driver, so patches 2 and 3 should go through the same
tree.

Changes since v1:
- Add dt-binding change.
- Add a comment describing the interrupt wiring and the write buffers
  (Thomas)
- New patch 3: flush the write buffers of an interrupt in parallel, and
  document the double flush from the AR7100 datasheet (Thomas)

Build tested with ath79_defconfig (LLVM, W=1).

Claude Code (Claude Opus 5.5) helped write the comments, patch 3 and
the commit messages, and searched the Atheros datasheets for the
double-flush requirement. I reviewed all the changes.

Rosen Penev (3):
  dt-bindings: interrupt-controller: qca,ar7100-cpu-intc: allow shared
    write buffer interrupts
  irqchip/ath79-cpu: flush every write buffer of a shared interrupt
  MIPS: ath79: flush the DDR write buffers of an interrupt in parallel

 .../qca,ar7100-cpu-intc.yaml                  |  5 +-
 arch/mips/ath79/common.c                      | 51 ++++++++++++++-----
 arch/mips/include/asm/mach-ath79/ath79.h      |  2 +-
 drivers/irqchip/irq-ath79-cpu.c               | 44 ++++++++++------
 4 files changed, 73 insertions(+), 29 deletions(-)

-- 
2.56.0

[PATCH v2 1/3] dt-bindings: interrupt-controller: qca,ar7100-cpu-intc: allow shared write buffer interrupts

[Rosen Penev]

On AR934x and QCA953x, the WMAC and the PCIe root complex share CPU
interrupt 2, but write to DDR through different write buffers. Document
that such an interrupt is listed once for each write buffer, and that
all of them are flushed.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 .../bindings/interrupt-controller/qca,ar7100-cpu-intc.yaml   | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/qca,ar7100-cpu-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/qca,ar7100-cpu-intc.yaml
index ab32a91af4c4..becec88061d7 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qca,ar7100-cpu-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/qca,ar7100-cpu-intc.yaml
@@ -29,7 +29,10 @@ properties:
     const: 1
 
   qca,ddr-wb-channel-interrupts:
-    description: List of interrupts needing a write buffer flush
+    description:
+      List of interrupts needing a write buffer flush. An interrupt shared by
+      several devices that use different write buffers is listed once for
+      each of them, and all of their write buffers are flushed.
     $ref: /schemas/types.yaml#/definitions/uint32-array
 
   qca,ddr-wb-channels:
-- 
2.56.0

[PATCH v2 2/3] 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.

Document the interrupt wiring, the write buffers and why a shared line
flushes all of them.

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

diff --git a/drivers/irqchip/irq-ath79-cpu.c b/drivers/irqchip/irq-ath79-cpu.c
index 9b7273a7f8ce..ec39c37895c0 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>
@@ -18,23 +19,33 @@
 #include <asm/mach-ath79/ath79.h>
 
 /*
- * The IP2/IP3 lines are tied to a PCI/WMAC/USB device. Drivers for
- * these devices typically allocate coherent DMA memory, however the
- * DMA controller may still have some unsynchronized data in the FIFO.
- * Issue a flush in the handlers to ensure that the driver sees
- * the update.
+ * The MIPS core has 8 interrupt lines: IP0/IP1 are software interrupts,
+ * IP7 is the CP0 timer, and IP2-IP6 are wired by the SoC to its devices.
+ * The wiring is fixed in hardware. Several devices can share a line, like
+ * the WMAC and the PCIe root complex on IP2 of AR934x, and their drivers
+ * then request the interrupt with IRQF_SHARED.
  *
- * This array map the interrupt lines to the DDR write buffer channels.
+ * Each DMA master writes to DDR through its own write buffer in the DDR
+ * controller, and each write buffer has its own flush register. When a
+ * device raises its interrupt, its last DMA writes may still sit in its
+ * write buffer, so the handler could read stale descriptors or data.
+ *
+ * The CPU interrupt says nothing about which device on a shared line
+ * raised it. So before running the handlers of a line, flush the write
+ * buffers of every device wired to it. Each handler then sees its
+ * device's data, checks its own status, and returns IRQ_NONE if it was
+ * not the source.
+ *
+ * irq_wb_chans maps each CPU interrupt line to the mask of the write
+ * buffer channels to flush, from the qca,ddr-wb-channel-interrupts and
+ * qca,ddr-wb-channels properties.
  */
-
-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 +57,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 +72,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 +82,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 +92,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

[PATCH v2 3/3] MIPS: ath79: flush the DDR write buffers of an interrupt in parallel

[Rosen Penev]

When several devices share a CPU interrupt, plat_irq_dispatch() flushes
their DDR write buffers one after the other, waiting for each to drain
before starting the next.

Make ath79_ddr_wb_flush() take a mask of channels, and start the flush
of every channel before waiting for any of them, so that the write
buffers drain in parallel. For a single channel the sequence is
unchanged. Its only user is the built-in irqchip, so drop the export.

Keep flushing twice, as the AR7100 datasheet asks, and quote it instead
of the comment that it must be run twice.

Suggested-by: Thomas Gleixner <tglx@kernel.org>
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 arch/mips/ath79/common.c                 | 51 ++++++++++++++++++------
 arch/mips/include/asm/mach-ath79/ath79.h |  2 +-
 drivers/irqchip/irq-ath79-cpu.c          | 13 +++---
 3 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/arch/mips/ath79/common.c b/arch/mips/ath79/common.c
index 137abbc65c60..9c5e9cd6931a 100644
--- a/arch/mips/ath79/common.c
+++ b/arch/mips/ath79/common.c
@@ -9,6 +9,7 @@
  *  Parts of this file are based on Atheros' 2.6.15/2.6.31 BSP
  */
 
+#include <linux/bitops.h>
 #include <linux/kernel.h>
 #include <linux/export.h>
 #include <linux/types.h>
@@ -53,21 +54,47 @@ void ath79_ddr_ctrl_init(void)
 }
 EXPORT_SYMBOL_GPL(ath79_ddr_ctrl_init);
 
-void ath79_ddr_wb_flush(u32 reg)
+static void __ath79_ddr_wb_flush(unsigned long chans)
 {
-	void __iomem *flush_reg = ath79_ddr_wb_flush_base + (reg * 4);
-
-	/* Flush the DDR write buffer. */
-	__raw_writel(0x1, flush_reg);
-	while (__raw_readl(flush_reg) & 0x1)
-		;
+	u32 __iomem *base = ath79_ddr_wb_flush_base;
+	unsigned int chan;
+
+	/*
+	 * Start every flush before waiting for any of them, so that the
+	 * write buffers drain in parallel instead of one after the other.
+	 */
+	for_each_set_bit(chan, &chans, 32)
+		__raw_writel(0x1, base + chan);
+
+	for_each_set_bit(chan, &chans, 32)
+		while (__raw_readl(base + chan) & 0x1)
+			;
+}
 
-	/* It must be run twice. */
-	__raw_writel(0x1, flush_reg);
-	while (__raw_readl(flush_reg) & 0x1)
-		;
+/**
+ * ath79_ddr_wb_flush - flush DDR write buffers
+ * @chans: mask of the write buffer channels to flush
+ *
+ * Every DMA master of the SoC (the Ethernet MACs, USB, PCIe, WMAC) writes
+ * to DDR through its own write buffer in the DDR controller. The buffer
+ * may still hold data when the device raises its interrupt, so it has to
+ * be flushed before the interrupt handler looks at the DMA'd memory.
+ * Setting bit 0 of the channel's flush register starts the flush, and the
+ * controller clears it once the buffer is empty.
+ */
+void ath79_ddr_wb_flush(unsigned long chans)
+{
+	/*
+	 * The AR7100 datasheet says to flush twice, after reading the
+	 * interrupt status of the device, to ensure that its descriptor
+	 * updates have reached memory before software reads them. It does
+	 * not say why a single flush is not enough. The datasheets of the
+	 * later SoCs drop the note, but nothing says the need went away, so
+	 * flush twice on all of them.
+	 */
+	__ath79_ddr_wb_flush(chans);
+	__ath79_ddr_wb_flush(chans);
 }
-EXPORT_SYMBOL_GPL(ath79_ddr_wb_flush);
 
 void ath79_ddr_set_pci_windows(void)
 {
diff --git a/arch/mips/include/asm/mach-ath79/ath79.h b/arch/mips/include/asm/mach-ath79/ath79.h
index 70cda74494a4..15897d0f570d 100644
--- a/arch/mips/include/asm/mach-ath79/ath79.h
+++ b/arch/mips/include/asm/mach-ath79/ath79.h
@@ -145,7 +145,7 @@ static inline int soc_is_qca956x(void)
 	return soc_is_qca9561() || soc_is_qca9563();
 }
 
-void ath79_ddr_wb_flush(unsigned int reg);
+void ath79_ddr_wb_flush(unsigned long chans);
 void ath79_ddr_set_pci_windows(void);
 
 extern void __iomem *ath79_pll_base;
diff --git a/drivers/irqchip/irq-ath79-cpu.c b/drivers/irqchip/irq-ath79-cpu.c
index ec39c37895c0..64272b15f4ed 100644
--- a/drivers/irqchip/irq-ath79-cpu.c
+++ b/drivers/irqchip/irq-ath79-cpu.c
@@ -10,7 +10,7 @@
  *  Parts of this file are based on Atheros' 2.6.15/2.6.31 BSP
  */
 
-#include <linux/bitops.h>
+#include <linux/bits.h>
 #include <linux/interrupt.h>
 #include <linux/irqchip.h>
 #include <linux/of.h>
@@ -44,8 +44,8 @@ static u32 irq_wb_chans[8];
 
 asmlinkage void plat_irq_dispatch(void)
 {
-	unsigned long pending, chans;
-	int irq, chan;
+	unsigned long pending;
+	int irq;
 
 	pending = read_c0_status() & read_c0_cause() & ST0_IM;
 
@@ -57,11 +57,8 @@ asmlinkage void plat_irq_dispatch(void)
 	pending >>= CAUSEB_IP;
 	while (pending) {
 		irq = fls(pending) - 1;
-		if (irq < ARRAY_SIZE(irq_wb_chans)) {
-			chans = irq_wb_chans[irq];
-			for_each_set_bit(chan, &chans, 32)
-				ath79_ddr_wb_flush(chan);
-		}
+		if (irq < ARRAY_SIZE(irq_wb_chans) && irq_wb_chans[irq])
+			ath79_ddr_wb_flush(irq_wb_chans[irq]);
 		do_IRQ(MIPS_CPU_IRQ_BASE + irq);
 		pending &= ~BIT(irq);
 	}
-- 
2.56.0