[RFC PATCH v1 0/4] Rework realtek-rtl IRQ driver

After seeing some use and with more devices tested, the current implementation
for the Realtek SoC interrupt controller was found to contain a few flaws.

The driver requires the following fixes:
- irq_domain_ops::map should map the virq, not the hwirq
- routing has an off-by-one error. Values (1..6) correspond to MIPS CAUSEF(2..7)

The following improvements should also be made:
- Use N real cascaded interrupts with an interrupt-specific mask of child irq lines.
  Otherwise a high-priority interrupt may cause a low-priority interrupt to be
  handled first.
- Get rid of custom interrupt-map interpretation, replace by realtek,interrupt-routing.

These patches are some initial changes, and since is this my first real work on
an interrupt driver, I'm submitting this as an RFC first. Please see the notes
on the patches for some extra remarks.

I've also currently opted to stay with cascased interrupts, as I'm not
sure if (and how) this should be implemented as a hierarchical
controller.

Best,
Sander

Sander Vanheule (4):
  irqchip: realtek-rtl: map control data to virq
  irqchip: realtek-rtl: use per-parent irq handling
  dt-bindings: interrupt-controller: realtek,rtl-intc: replace irq
    mapping
  irqchip: realtek-rtl: replace custom interrupt-map

 .../realtek,rtl-intc.yaml                     |  49 +++--
 drivers/irqchip/irq-realtek-rtl.c             | 202 +++++++++++-------
 2 files changed, 163 insertions(+), 88 deletions(-)

-- 
2.33.1

The interrupt router controller is used to route 32 SoC interrupts to up
to 6 MIPS CPU interrupts. This means that the SoC interrupts inherit the
priority of to the target CPU interrupt.

Currently the driver handles all SoC interrupts equally, independent of
which CPU interrupt it is routed to. The use of __ffs actually gives
higher priority to lower IRQ lines, effectively bypassing the CPU
interrupt priority.

Additionally, this indiscriminate handling of SoC interrupts masked
another issue. There is an actually an offset between routing values
(1..6) and CPU interrupts (2..7), but the current mapping makes no
distinction between these two values. This issue was also hidden during
testing, because an interrupt mapping was used where for each required
interrupt another (unused) routing was configured, with an offset of +1.

Rework the driver to use a separate handler for each used CPU interrupt,
and use the correct routing values. Instead of assuming that the parent
interrupt controller is the MIPS CPU interrupt controller
("mti,cpu-interrupt-controller"), this is now checked explicitly to
correctly handle the timer interrupt.

Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt controller")
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---

This patch makes a few changes at the same time, and introduces the
*_irr functions, which aren't strictly required. This allows the last
patch to be a bit smaller, and seeks to add some clarity to the code.

Please let me know if this should be split into separate patches with
more incremental changes (in addition to other likely comments).
---
 drivers/irqchip/irq-realtek-rtl.c | 153 +++++++++++++++++++++---------
 1 file changed, 108 insertions(+), 45 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index d6788dd93c7b..71366f1cf721 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c

@@ -7,6 +7,7 @@
 
 #include <linux/of_irq.h>
 #include <linux/irqchip.h>
+#include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/of_address.h>
 #include <linux/irqchip/chained_irq.h>

@@ -21,10 +22,43 @@
 #define RTL_ICTL_IRR2		0x10
 #define RTL_ICTL_IRR3		0x14
 
-#define REG(x)		(realtek_ictl_base + x)
+#define RTL_ICTL_NUM_PRIO	6
+
+#define REG(x)			(realtek_ictl_base + x)
 
 static DEFINE_RAW_SPINLOCK(irq_lock);
 static void __iomem *realtek_ictl_base;
+static struct irq_domain *realtek_ictl_domain;
+
+struct realtek_ictl_priority {
+	unsigned int routing_value;
+	u32 child_mask;
+};
+
+static struct realtek_ictl_priority priorities[RTL_ICTL_NUM_PRIO];
+
+/*
+ * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted
+ * numbering, placing IRQ 31 in the first four bits.
+ */
+#define IRR_OFFSET(idx)		(4 * (3 - (idx * 4) / 32))
+#define IRR_SHIFT(idx)		((idx * 4) % 32)
+
+static inline u32 read_irr(void __iomem *irr0, int idx)
+{
+	return (readl(irr0 + IRR_OFFSET(idx)) >> IRR_SHIFT(idx)) & 0xf;
+}
+
+static inline void write_irr(void __iomem *irr0, int idx, u32 value)
+{
+	unsigned int offset = IRR_OFFSET(idx);
+	unsigned int shift = IRR_SHIFT(idx);
+	u32 irr;
+
+	irr = readl(irr0 + offset) & ~(0xf << shift);
+	irr |= (value & 0xf) << shift;
+	writel(irr, irr0 + offset);
+}
 
 static void realtek_ictl_unmask_irq(struct irq_data *i)
 {

@@ -72,43 +106,67 @@ static const struct irq_domain_ops irq_domain_ops = {
 	.xlate = irq_domain_xlate_onecell,
 };
 
-static void realtek_irq_dispatch(struct irq_desc *desc)
+static irqreturn_t realtek_irq_dispatch(int irq, void *devid)
 {
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct irq_domain *domain;
-	unsigned int pending;
-
-	chained_irq_enter(chip, desc);
-	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR));
-	if (unlikely(!pending)) {
-		spurious_interrupt();
-		goto out;
+	struct realtek_ictl_priority *priority = devid;
+	unsigned long pending;
+	int soc_irq;
+	int ret = 0;
+
+	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
+		& priority->child_mask;
+
+	for_each_set_bit(soc_irq, &pending, BITS_PER_LONG) {
+		generic_handle_domain_irq(realtek_ictl_domain, soc_irq);
+		ret = 1;
 	}
-	domain = irq_desc_get_handler_data(desc);
-	generic_handle_domain_irq(domain, __ffs(pending));
 
-out:
-	chained_irq_exit(chip, desc);
+	return IRQ_RETVAL(ret);
 }
 
-/*
- * SoC interrupts are cascaded to MIPS CPU interrupts according to the
- * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for
- * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts
- * thus go into 4 IRRs.
- */
-static int __init map_interrupts(struct device_node *node, struct irq_domain *domain)
+static void __init set_routing(struct realtek_ictl_priority *priority, unsigned int soc_int)
 {
+	unsigned int priority_old;
+
+	priority_old = read_irr(REG(RTL_ICTL_IRR0), soc_int);
+	if (priority_old) {
+		pr_warn("int %d already routed to %d, not updating\n", soc_int, priority_old);
+		return;
+	}
+
+	priority->child_mask |= BIT(soc_int);
+	write_irr(REG(RTL_ICTL_IRR0), soc_int, priority->routing_value);
+}
+
+static int __init setup_parent_interrupt(struct realtek_ictl_priority *prio_ctl, int parent)
+{
+	struct device_node *parent_node;
+	struct irq_data *irqd;
+	unsigned int flags;
+	int parent_hwirq;
+
+	irqd = irq_get_irq_data(parent);
+	if (!irqd)
+		return -ENOENT;
+
+	parent_node = to_of_node(irqd->domain->fwnode);
+	parent_hwirq = irqd_to_hwirq(irqd);
+
+	flags = IRQF_PERCPU | IRQF_SHARED;
+	if (of_device_is_compatible(parent_node, "mti,cpu-interrupt-controller")
+		&& parent_hwirq == 7)
+		flags |= IRQF_TIMER;
+
+	return request_irq(parent, realtek_irq_dispatch, flags, "rtl-intc", prio_ctl);
+}
+
+static int __init map_interrupts(struct device_node *node)
+{
+	struct realtek_ictl_priority *prio_ctl;
 	struct device_node *cpu_ictl;
 	const __be32 *imap;
-	u32 imaplen, soc_int, cpu_int, tmp, regs[4];
-	int ret, i, irr_regs[] = {
-		RTL_ICTL_IRR3,
-		RTL_ICTL_IRR2,
-		RTL_ICTL_IRR1,
-		RTL_ICTL_IRR0,
-	};
-	u8 mips_irqs_set;
+	u32 imaplen, soc_int, priority, tmp;
+	int ret, i;
 
 	ret = of_property_read_u32(node, "#address-cells", &tmp);
 	if (ret || tmp)

@@ -118,8 +176,6 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
 	if (!imap || imaplen % 3)
 		return -EINVAL;
 
-	mips_irqs_set = 0;
-	memset(regs, 0, sizeof(regs));
 	for (i = 0; i < imaplen; i += 3 * sizeof(u32)) {
 		soc_int = be32_to_cpup(imap);
 		if (soc_int > 31)

@@ -133,42 +189,49 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
 			return -EINVAL;
 		of_node_put(cpu_ictl);
 
-		cpu_int = be32_to_cpup(imap + 2);
-		if (cpu_int > 7)
+		/* Map priority (1..6) to MIPS CPU interrupt (2..7) */
+		priority = be32_to_cpup(imap + 2);
+		if (priority > 6 || priority < 1)
 			return -EINVAL;
 
-		if (!(mips_irqs_set & BIT(cpu_int))) {
-			irq_set_chained_handler_and_data(cpu_int, realtek_irq_dispatch,
-							 domain);
-			mips_irqs_set |= BIT(cpu_int);
+		prio_ctl = &priorities[priority - 1];
+
+		if (!prio_ctl->routing_value) {
+			ret = setup_parent_interrupt(prio_ctl, priority + 1);
+			if (ret)
+				return ret;
+
+			prio_ctl->routing_value = priority;
 		}
 
-		regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
+		set_routing(prio_ctl, soc_int);
+
 		imap += 3;
 	}
 
-	for (i = 0; i < 4; i++)
-		writel(regs[i], REG(irr_regs[i]));
 
 	return 0;
 }
 
 static int __init realtek_rtl_of_init(struct device_node *node, struct device_node *parent)
 {
-	struct irq_domain *domain;
+	unsigned int soc_irq;
 	int ret;
 
+	memset(&priorities, 0, sizeof(priorities));
+
 	realtek_ictl_base = of_iomap(node, 0);
 	if (!realtek_ictl_base)
 		return -ENXIO;
 
 	/* Disable all cascaded interrupts */
 	writel(0, REG(RTL_ICTL_GIMR));
+	for (soc_irq = 0; soc_irq < 32; soc_irq++)
+		write_irr(REG(RTL_ICTL_IRR0), soc_irq, 0);
 
-	domain = irq_domain_add_simple(node, 32, 0,
-				       &irq_domain_ops, NULL);
+	realtek_ictl_domain = irq_domain_add_simple(node, 32, 0, &irq_domain_ops, NULL);
 
-	ret = map_interrupts(node, domain);
+	ret = map_interrupts(node);
 	if (ret) {
 		pr_err("invalid interrupt map\n");
 		return ret;

-- 
2.33.1

The binding incorrectly specified the "interrupt-map" property should be
used, although the use is non-standard. A quirk had to be introduced in
commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
definition of interrupt-map") to allow the driver to function again.

Update the binding to require a list of parent interrupts instead, and
replace the "interrupt-map" property by the custom
"realtek,interrupt-routing" property.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---

The registers for this interrupt controller have 4 bits per SoC
interrupt. This means that, in theory, 15 output interrupts could be
wired up (a value of 0 means 'disconnected'), but we have only ever seen
this router being used to map to the six MIPS CPU hardware interrupts.
---
 .../realtek,rtl-intc.yaml                     | 49 +++++++++++++------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
index 9e76fff20323..4f7f30111a8e 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml

@@ -6,6 +6,11 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: Realtek RTL SoC interrupt controller devicetree bindings
 
+description:
+  Interrupt router for Realtek MIPS SoCs, allowing up to 32 SoC interrupts to
+  be routed to one of up to 15 parent interrupts, or left disconnected. Most
+  commonly, the CPU's six hardware interrupts are used as parent interrupts.
+
 maintainers:
   - Birger Koblitz <mail@birger-koblitz.de>
   - Bert Vermeulen <bert@biot.com>

@@ -15,30 +20,40 @@ properties:
   compatible:
     const: realtek,rtl-intc
 
-  "#interrupt-cells":
-    const: 1
-
   reg:
     maxItems: 1
 
-  interrupts:
-    maxItems: 1
+  "#interrupt-cells":
+    const: 1
 
   interrupt-controller: true
 
-  "#address-cells":
-    const: 0
+  interrupts:
+    minItems: 1
+    maxItems: 15
+    description:
+      List of interrupts where SoC interrupts inputs can be routed to. Must be
+      provided in the same order as the output lines. The first interrupt is
+      thus selected via routing value 0, etc.
 
-  interrupt-map:
-    description: Describes mapping from SoC interrupts to CPU interrupts
+  realtek,interrupt-routing:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    description:
+      List of <soc_int irq_idx> pairs, where "soc_int" is the interrupt line
+      number as provided by this controller. "irq_idx" is the index of the
+      interrupt in the list as specified the interrupts property.
+    items:
+      items:
+        - description: SoC interrupt index
+        - description: parent interrupt index
 
 required:
   - compatible
   - reg
   - "#interrupt-cells"
   - interrupt-controller
-  - "#address-cells"
-  - interrupt-map
+  - interrupts
+  - realtek,interrupt-routing
 
 additionalProperties: false
 

@@ -49,9 +64,11 @@ examples:
       #interrupt-cells = <1>;
       interrupt-controller;
       reg = <0x3000 0x20>;
-      #address-cells = <0>;
-      interrupt-map =
-              <31 &cpuintc 2>,
-              <30 &cpuintc 1>,
-              <29 &cpuintc 5>;
+
+      interrupt-parent = <&cpu_intc>;
+      interrupts = <2>, <3>, <4>;
+      realtek,interrupt-routing =
+              <31 0>, /* route to cpu_intc interrupt 2 */
+              <30 1>, /* route to cpu_intc interrupt 3 */
+              <29 2>; /* route to cpu_intc interrupt 4 */
     };

-- 
2.33.1

The driver assigned the irqchip and irq handler to the hardware irq,
instead of the virq. This is incorrect, and only worked because these
irq numbers happened to be the same on the devices used for testing the
original driver.

Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt controller")
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/irqchip/irq-realtek-rtl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index fd9f275592d2..d6788dd93c7b 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c

@@ -62,7 +62,7 @@ static struct irq_chip realtek_ictl_irq = {
 
 static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
-	irq_set_chip_and_handler(hw, &realtek_ictl_irq, handle_level_irq);
+	irq_set_chip_and_handler(irq, &realtek_ictl_irq, handle_level_irq);
 
 	return 0;
 }

-- 
2.33.1

To match the updated realtek,rtl-intc devicetree binding, replace the
interrupt routing parsing.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
 drivers/irqchip/irq-realtek-rtl.c | 77 +++++++++++++++----------------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 71366f1cf721..d80fbc5e651b 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c

@@ -160,55 +160,50 @@ static int __init setup_parent_interrupt(struct realtek_ictl_priority *prio_ctl,
 	return request_irq(parent, realtek_irq_dispatch, flags, "rtl-intc", prio_ctl);
 }
 
-static int __init map_interrupts(struct device_node *node)
+static int __init route_interrupts(struct device_node *node)
 {
 	struct realtek_ictl_priority *prio_ctl;
-	struct device_node *cpu_ictl;
-	const __be32 *imap;
-	u32 imaplen, soc_int, priority, tmp;
-	int ret, i;
-
-	ret = of_property_read_u32(node, "#address-cells", &tmp);
-	if (ret || tmp)
-		return -EINVAL;
-
-	imap = of_get_property(node, "interrupt-map", &imaplen);
-	if (!imap || imaplen % 3)
-		return -EINVAL;
-
-	for (i = 0; i < imaplen; i += 3 * sizeof(u32)) {
-		soc_int = be32_to_cpup(imap);
-		if (soc_int > 31)
-			return -EINVAL;
+	unsigned int num_prio, parent_idx;
+	const __be32 *routing_table;
+	int table_len;
+	u32 hw_irq;
+	int ret;
 
-		cpu_ictl = of_find_node_by_phandle(be32_to_cpup(imap + 1));
-		if (!cpu_ictl)
-			return -EINVAL;
-		ret = of_property_read_u32(cpu_ictl, "#interrupt-cells", &tmp);
-		if (ret || tmp != 1)
-			return -EINVAL;
-		of_node_put(cpu_ictl);
+	num_prio = of_irq_count(node);
+	if (num_prio > RTL_ICTL_NUM_PRIO) {
+		pr_err("too many parent interrupts\n");
+		return -ENODEV;
+	}
 
-		/* Map priority (1..6) to MIPS CPU interrupt (2..7) */
-		priority = be32_to_cpup(imap + 2);
-		if (priority > 6 || priority < 1)
-			return -EINVAL;
+	for (parent_idx = 0; parent_idx < num_prio; parent_idx++) {
+		prio_ctl = &priorities[parent_idx];
 
-		prio_ctl = &priorities[priority - 1];
+		ret = irq_of_parse_and_map(node, parent_idx);
+		if (ret < 0)
+			return ret;
 
-		if (!prio_ctl->routing_value) {
-			ret = setup_parent_interrupt(prio_ctl, priority + 1);
-			if (ret)
-				return ret;
+		ret = setup_parent_interrupt(prio_ctl, ret);
+		if (ret)
+			return ret;
 
-			prio_ctl->routing_value = priority;
-		}
+		prio_ctl->routing_value = parent_idx + 1;
+	}
 
-		set_routing(prio_ctl, soc_int);
+	routing_table = of_get_property(node, "realtek,interrupt-routing", &table_len);
+	if (!routing_table)
+		return -ENOENT;
 
-		imap += 3;
-	}
+	for (table_len /= sizeof(*routing_table); table_len >= 2; table_len -= 2) {
+		hw_irq = be32_to_cpup(routing_table++);
+		if (hw_irq > 31)
+			return -EINVAL;
 
+		parent_idx = be32_to_cpup(routing_table++);
+		if (parent_idx >= num_prio)
+			return -EINVAL;
+
+		set_routing(&priorities[parent_idx], hw_irq);
+	}
 
 	return 0;
 }

@@ -231,9 +226,9 @@ static int __init realtek_rtl_of_init(struct device_node *node, struct device_no
 
 	realtek_ictl_domain = irq_domain_add_simple(node, 32, 0, &irq_domain_ops, NULL);
 
-	ret = map_interrupts(node);
+	ret = route_interrupts(node);
 	if (ret) {
-		pr_err("invalid interrupt map\n");
+		pr_err("invalid interrupt routing\n");
 		return ret;
 	}
 

-- 
2.33.1

Hi Sander,

nit: please check the way the irqchip patches have their title
formatted, and make sure you follow these rules. In this case, it
should read:

irqchip/realtek-rtl: Use per-parent...

On Thu, 23 Dec 2021 12:08:32 +0000,
Sander Vanheule [off-list ref] wrote:

Spurious change?

 static DEFINE_RAW_SPINLOCK(irq_lock);
 static void __iomem *realtek_ictl_base;
+static struct irq_domain *realtek_ictl_domain;
+
+struct realtek_ictl_priority {
+	unsigned int routing_value;
+	u32 child_mask;
+};
+
+static struct realtek_ictl_priority priorities[RTL_ICTL_NUM_PRIO];
+
+/*
+ * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted
+ * numbering, placing IRQ 31 in the first four bits.
+ */
+#define IRR_OFFSET(idx)		(4 * (3 - (idx * 4) / 32))
+#define IRR_SHIFT(idx)		((idx * 4) % 32)
+
+static inline u32 read_irr(void __iomem *irr0, int idx)
+{
+	return (readl(irr0 + IRR_OFFSET(idx)) >> IRR_SHIFT(idx)) & 0xf;
+}
+
+static inline void write_irr(void __iomem *irr0, int idx, u32 value)
+{
+	unsigned int offset = IRR_OFFSET(idx);
+	unsigned int shift = IRR_SHIFT(idx);
+	u32 irr;
+
+	irr = readl(irr0 + offset) & ~(0xf << shift);
+	irr |= (value & 0xf) << shift;
+	writel(irr, irr0 + offset);

Are you always in a situation where this doesn't need any locking?

No, that's definitely not on. Interrupt handlers are not chained
handlers. They have different guarantees, and they aren't
interchangeable. It is only in limited circumstances that you can do
this change (threaded interrupts).

 {
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct irq_domain *domain;
-	unsigned int pending;
-
-	chained_irq_enter(chip, desc);
-	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR));
-	if (unlikely(!pending)) {
-		spurious_interrupt();
-		goto out;
+	struct realtek_ictl_priority *priority = devid;

So this is *why* you made this change. We have per-interrupt data that
you can use, and get rid of this horrible hack.

+	unsigned long pending;
+	int soc_irq;
+	int ret = 0;
+
+	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
+		& priority->child_mask;
+
+	for_each_set_bit(soc_irq, &pending, BITS_PER_LONG) {
+		generic_handle_domain_irq(realtek_ictl_domain, soc_irq);
+		ret = 1;
 	}
-	domain = irq_desc_get_handler_data(desc);
-	generic_handle_domain_irq(domain, __ffs(pending));
 
-out:
-	chained_irq_exit(chip, desc);
+	return IRQ_RETVAL(ret);
 }
 
-/*
- * SoC interrupts are cascaded to MIPS CPU interrupts according to the
- * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for
- * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts
- * thus go into 4 IRRs.
- */
-static int __init map_interrupts(struct device_node *node, struct irq_domain *domain)
+static void __init set_routing(struct realtek_ictl_priority *priority, unsigned int soc_int)
 {
+	unsigned int priority_old;
+
+	priority_old = read_irr(REG(RTL_ICTL_IRR0), soc_int);
+	if (priority_old) {
+		pr_warn("int %d already routed to %d, not updating\n", soc_int, priority_old);
+		return;
+	}
+
+	priority->child_mask |= BIT(soc_int);
+	write_irr(REG(RTL_ICTL_IRR0), soc_int, priority->routing_value);
+}
+
+static int __init setup_parent_interrupt(struct realtek_ictl_priority *prio_ctl, int parent)
+{
+	struct device_node *parent_node;
+	struct irq_data *irqd;
+	unsigned int flags;
+	int parent_hwirq;
+
+	irqd = irq_get_irq_data(parent);
+	if (!irqd)
+		return -ENOENT;
+
+	parent_node = to_of_node(irqd->domain->fwnode);
+	parent_hwirq = irqd_to_hwirq(irqd);
+
+	flags = IRQF_PERCPU | IRQF_SHARED;
+	if (of_device_is_compatible(parent_node, "mti,cpu-interrupt-controller")
+		&& parent_hwirq == 7)
+		flags |= IRQF_TIMER;
+
+	return request_irq(parent, realtek_irq_dispatch, flags, "rtl-intc", prio_ctl);

This really is mixing two different things. Why aren't the flags on
the actual endpoint interrupt? This really is the business of the
driver requesting the interrupt, and not the irqchip.

I don't understand this. As far as the binding describes it, this is
the target interrupt, and not a priority. Either the binding is wrong,
and it needs fixing, or this is wrong. What gives?

If the binding is really describing a priority, how is the interrupt
priority actually mapped to the CPU interrupt? Why can't you just
ignore the DT-provided priority and simply have a flat priority
scheme, allocating mapping input lines to output lines as they get
allocated?

	M.

-- 
Without deviation from the norm, progress is not possible.

On Thu, 23 Dec 2021 12:08:33 +0000,
Sander Vanheule [off-list ref] wrote:

The binding incorrectly specified the "interrupt-map" property should be
used, although the use is non-standard. A quirk had to be introduced in
commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
definition of interrupt-map") to allow the driver to function again.

That's too late. We have released a kernel with this binding, and it
will live on forever until we totally remove the platform from the
tree.

DT is an ABI, and only time travel can fix this blunder.

	M.

-- 
Without deviation from the norm, progress is not possible.

Hi Mark,

Thanks for your feedback.

On Thu, 2021-12-23 at 17:57 +0000, Marc Zyngier wrote:

Hi Sander,

nit: please check the way the irqchip patches have their title
formatted, and make sure you follow these rules. In this case, it
should read:

irqchip/realtek-rtl: Use per-parent...

I'll update the titles.

On Thu, 23 Dec 2021 12:08:32 +0000,
Sander Vanheule [off-list ref] wrote:

quoted

The interrupt router controller is used to route 32 SoC interrupts to up
to 6 MIPS CPU interrupts. This means that the SoC interrupts inherit the
priority of to the target CPU interrupt.

Currently the driver handles all SoC interrupts equally, independent of
which CPU interrupt it is routed to. The use of __ffs actually gives
higher priority to lower IRQ lines, effectively bypassing the CPU
interrupt priority.

Additionally, this indiscriminate handling of SoC interrupts masked
another issue. There is an actually an offset between routing values
(1..6) and CPU interrupts (2..7), but the current mapping makes no
distinction between these two values. This issue was also hidden during
testing, because an interrupt mapping was used where for each required
interrupt another (unused) routing was configured, with an offset of +1.

Rework the driver to use a separate handler for each used CPU interrupt,
and use the correct routing values. Instead of assuming that the parent
interrupt controller is the MIPS CPU interrupt controller
("mti,cpu-interrupt-controller"), this is now checked explicitly to
correctly handle the timer interrupt.

Fixes: 9f3a0f34b84a ("irqchip: Add support for Realtek RTL838x/RTL839x interrupt
controller")
Signed-off-by: Sander Vanheule <sander@svanheule.net>
---

This patch makes a few changes at the same time, and introduces the
*_irr functions, which aren't strictly required. This allows the last
patch to be a bit smaller, and seeks to add some clarity to the code.

Please let me know if this should be split into separate patches with
more incremental changes (in addition to other likely comments).
---
 drivers/irqchip/irq-realtek-rtl.c | 153 +++++++++++++++++++++---------
 1 file changed, 108 insertions(+), 45 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index d6788dd93c7b..71366f1cf721 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c

@@ -7,6 +7,7 @@

 
 #include <linux/of_irq.h>
 #include <linux/irqchip.h>
+#include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/of_address.h>
 #include <linux/irqchip/chained_irq.h>

@@ -21,10 +22,43 @@

 #define RTL_ICTL_IRR2          0x10
 #define RTL_ICTL_IRR3          0x14
 
-#define REG(x)         (realtek_ictl_base + x)
+#define RTL_ICTL_NUM_PRIO      6
+
+#define REG(x)                 (realtek_ictl_base + x)

Spurious change?

The indentation didn't match with the other defines, but I can leave out this change here.

quoted

 static DEFINE_RAW_SPINLOCK(irq_lock);
 static void __iomem *realtek_ictl_base;
+static struct irq_domain *realtek_ictl_domain;
+
+struct realtek_ictl_priority {
+       unsigned int routing_value;
+       u32 child_mask;
+};
+
+static struct realtek_ictl_priority priorities[RTL_ICTL_NUM_PRIO];
+
+/*
+ * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted
+ * numbering, placing IRQ 31 in the first four bits.
+ */
+#define IRR_OFFSET(idx)                (4 * (3 - (idx * 4) / 32))
+#define IRR_SHIFT(idx)         ((idx * 4) % 32)
+
+static inline u32 read_irr(void __iomem *irr0, int idx)
+{
+       return (readl(irr0 + IRR_OFFSET(idx)) >> IRR_SHIFT(idx)) & 0xf;
+}
+
+static inline void write_irr(void __iomem *irr0, int idx, u32 value)
+{
+       unsigned int offset = IRR_OFFSET(idx);
+       unsigned int shift = IRR_SHIFT(idx);
+       u32 irr;
+
+       irr = readl(irr0 + offset) & ~(0xf << shift);
+       irr |= (value & 0xf) << shift;
+       writel(irr, irr0 + offset);

Are you always in a situation where this doesn't need any locking?

These are currently only used on initialisation, so that's before any interrupts should be
active.

quoted

+}
 
 static void realtek_ictl_unmask_irq(struct irq_data *i)
 {

@@ -72,43 +106,67 @@ static const struct irq_domain_ops irq_domain_ops = {

        .xlate = irq_domain_xlate_onecell,
 };
 
-static void realtek_irq_dispatch(struct irq_desc *desc)
+static irqreturn_t realtek_irq_dispatch(int irq, void *devid)

No, that's definitely not on. Interrupt handlers are not chained
handlers. They have different guarantees, and they aren't
interchangeable. It is only in limited circumstances that you can do
this change (threaded interrupts).

quoted

 {
-       struct irq_chip *chip = irq_desc_get_chip(desc);
-       struct irq_domain *domain;
-       unsigned int pending;
-
-       chained_irq_enter(chip, desc);
-       pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR));
-       if (unlikely(!pending)) {
-               spurious_interrupt();
-               goto out;
+       struct realtek_ictl_priority *priority = devid;

So this is *why* you made this change. We have per-interrupt data that
you can use, and get rid of this horrible hack.

Aha, I'll need to have another look then. I'll make sure to switch back to chained
handlers for the next version.

quoted

+       unsigned long pending;
+       int soc_irq;
+       int ret = 0;
+
+       pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
+               & priority->child_mask;
+
+       for_each_set_bit(soc_irq, &pending, BITS_PER_LONG) {
+               generic_handle_domain_irq(realtek_ictl_domain, soc_irq);
+               ret = 1;
        }
-       domain = irq_desc_get_handler_data(desc);
-       generic_handle_domain_irq(domain, __ffs(pending));
 
-out:
-       chained_irq_exit(chip, desc);
+       return IRQ_RETVAL(ret);
 }
 
-/*
- * SoC interrupts are cascaded to MIPS CPU interrupts according to the
- * interrupt-map in the device tree. Each SoC interrupt gets 4 bits for
- * the CPU interrupt in an Interrupt Routing Register. Max 32 SoC interrupts
- * thus go into 4 IRRs.
- */
-static int __init map_interrupts(struct device_node *node, struct irq_domain *domain)
+static void __init set_routing(struct realtek_ictl_priority *priority, unsigned int
soc_int)
 {
+       unsigned int priority_old;
+
+       priority_old = read_irr(REG(RTL_ICTL_IRR0), soc_int);
+       if (priority_old) {
+               pr_warn("int %d already routed to %d, not updating\n", soc_int,
priority_old);
+               return;
+       }
+
+       priority->child_mask |= BIT(soc_int);
+       write_irr(REG(RTL_ICTL_IRR0), soc_int, priority->routing_value);
+}
+
+static int __init setup_parent_interrupt(struct realtek_ictl_priority *prio_ctl, int
parent)
+{
+       struct device_node *parent_node;
+       struct irq_data *irqd;
+       unsigned int flags;
+       int parent_hwirq;
+
+       irqd = irq_get_irq_data(parent);
+       if (!irqd)
+               return -ENOENT;
+
+       parent_node = to_of_node(irqd->domain->fwnode);
+       parent_hwirq = irqd_to_hwirq(irqd);
+
+       flags = IRQF_PERCPU | IRQF_SHARED;
+       if (of_device_is_compatible(parent_node, "mti,cpu-interrupt-controller")
+               && parent_hwirq == 7)
+               flags |= IRQF_TIMER;
+
+       return request_irq(parent, realtek_irq_dispatch, flags, "rtl-intc", prio_ctl);

This really is mixing two different things. Why aren't the flags on
the actual endpoint interrupt? This really is the business of the
driver requesting the interrupt, and not the irqchip.

I needed to set the flags, because otherwise the CEVT-R4K timer couldn't use the timer
interrupt any more. But I assume that's a side effect of using an interrupt handler
instead of a chained handler, and this will disappear when I use the correct handler type.

quoted

+}
+
+static int __init map_interrupts(struct device_node *node)
+{
+       struct realtek_ictl_priority *prio_ctl;
        struct device_node *cpu_ictl;
        const __be32 *imap;
-       u32 imaplen, soc_int, cpu_int, tmp, regs[4];
-       int ret, i, irr_regs[] = {
-               RTL_ICTL_IRR3,
-               RTL_ICTL_IRR2,
-               RTL_ICTL_IRR1,
-               RTL_ICTL_IRR0,
-       };
-       u8 mips_irqs_set;
+       u32 imaplen, soc_int, priority, tmp;
+       int ret, i;
 
        ret = of_property_read_u32(node, "#address-cells", &tmp);
        if (ret || tmp)

@@ -118,8 +176,6 @@ static int __init map_interrupts(struct device_node *node, struct

irq_domain *do
        if (!imap || imaplen % 3)
                return -EINVAL;
 
-       mips_irqs_set = 0;
-       memset(regs, 0, sizeof(regs));
        for (i = 0; i < imaplen; i += 3 * sizeof(u32)) {
                soc_int = be32_to_cpup(imap);
                if (soc_int > 31)

@@ -133,42 +189,49 @@ static int __init map_interrupts(struct device_node *node,

struct irq_domain *do
                        return -EINVAL;
                of_node_put(cpu_ictl);
 
-               cpu_int = be32_to_cpup(imap + 2);
-               if (cpu_int > 7)
+               /* Map priority (1..6) to MIPS CPU interrupt (2..7) */
+               priority = be32_to_cpup(imap + 2);

I don't understand this. As far as the binding describes it, this is
the target interrupt, and not a priority. Either the binding is wrong,
and it needs fixing, or this is wrong. What gives?

The "interrupt-map" values that my (and OpenWrt's) DTS-s have could also be amended to
provide correct <soc_int &parent_node parent_hwirq> tuples, instead of the current (wrong)
<soc_int &parent_node routing_value> tuples. That would probably make it possible to limit
this patch only to the per-parent chained handlers.

However, that would require me to keep the assumption that output lines (1..6) are mapped
to CPU interrupts (2..7). This is what I wanted to get rid of by using "interrupt-parent"
+ "realtek,interrupt-routing". Maybe this will be clearer if I can isolate these two
issues in a v2.

If the binding is really describing a priority, how is the interrupt
priority actually mapped to the CPU interrupt? Why can't you just
ignore the DT-provided priority and simply have a flat priority
scheme, allocating mapping input lines to output lines as they get
allocated?

I updated the bindings quite late while cleaning up my changes, the priority naming is
wrong. Because the SoC interrupts are always routed to MIPS CPU interrupts (in my
hardware), they "inherit" a priority, and this is what I had in mind when I started on
these patches.

While updating the binding, I realised that there doesn't need to be a priority to the
output interrupts of this controller. We don't have any real documentation on the
hardware, so for all we know they could maybe even be routed to different interrupt
controllers. I'll update the naming to something else throughout the patch.


Best,
Sander

On Thu, 2021-12-23 at 18:00 +0000, Marc Zyngier wrote:

On Thu, 23 Dec 2021 12:08:33 +0000,
Sander Vanheule [off-list ref] wrote:

quoted

The binding incorrectly specified the "interrupt-map" property should be
used, although the use is non-standard. A quirk had to be introduced in
commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
definition of interrupt-map") to allow the driver to function again.

That's too late. We have released a kernel with this binding, and it
will live on forever until we totally remove the platform from the
tree.

DT is an ABI, and only time travel can fix this blunder.

Taking into account your comments on the previous patch, this change wouldn't even be
required if I correct the mappings for my devices. But that wouldn't get rid of the
assumed mapping between output lines and parent interrupts.

To what extent can the binding be updated to get rid of this assumption? Or would that
require a completely new binding?


Best,
Sander

On Thu, 23 Dec 2021 19:29:23 +0000,
Sander Vanheule [off-list ref] wrote:

On Thu, 2021-12-23 at 18:00 +0000, Marc Zyngier wrote:

quoted

On Thu, 23 Dec 2021 12:08:33 +0000,
Sander Vanheule [off-list ref] wrote:

quoted

The binding incorrectly specified the "interrupt-map" property should be
used, although the use is non-standard. A quirk had to be introduced in
commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
definition of interrupt-map") to allow the driver to function again.

That's too late. We have released a kernel with this binding, and it
will live on forever until we totally remove the platform from the
tree.

DT is an ABI, and only time travel can fix this blunder.

Taking into account your comments on the previous patch, this change
wouldn't even be required if I correct the mappings for my
devices. But that wouldn't get rid of the assumed mapping between
output lines and parent interrupts.

A driver can always ignore some information from the DT and do its own
thing. No sure if that addresses your problem though.

To what extent can the binding be updated to get rid of this
assumption? Or would that require a completely new binding?

You can only extend a binding in a two-way fashion: old kernel works
with new DT, new kernel works old DT. Which means that in practice,
you can only *add* information to the DT, and have reasonable defaults
in the driver when you don't find it.

	M.

-- 
Without deviation from the norm, progress is not possible.

Hi Marc,

On Monday, 27 December 2021, Marc Zyngier wrote:

On Thu, 23 Dec 2021 19:29:23 +0000,
Sander Vanheule [off-list ref] wrote:

quoted

On Thu, 2021-12-23 at 18:00 +0000, Marc Zyngier wrote:

quoted

On Thu, 23 Dec 2021 12:08:33 +0000,
Sander Vanheule [off-list ref] wrote:

quoted

The binding incorrectly specified the "interrupt-map" property should be
used, although the use is non-standard. A quirk had to be introduced in
commit de4adddcbcc2 ("of/irq: Add a quirk for controllers with their own
definition of interrupt-map") to allow the driver to function again.

That's too late. We have released a kernel with this binding, and it
will live on forever until we totally remove the platform from the
tree.

DT is an ABI, and only time travel can fix this blunder.

Taking into account your comments on the previous patch, this change
wouldn't even be required if I correct the mappings for my
devices. But that wouldn't get rid of the assumed mapping between
output lines and parent interrupts.

A driver can always ignore some information from the DT and do its own
thing. No sure if that addresses your problem though.

quoted

To what extent can the binding be updated to get rid of this
assumption? Or would that require a completely new binding?

You can only extend a binding in a two-way fashion: old kernel works
with new DT, new kernel works old DT. Which means that in practice,
you can only *add* information to the DT, and have reasonable defaults
in the driver when you don't find it.

Thanks for clarifying. In that case I don't think it is possible to get rid of the output-to-parent assumption entirely, since the driver would always need to accommodate for the original binding, where there is no output mapping specified in the binding. There are no SoC-specific compatibles (where a mapping could be assumed), and I don't know on how many MIPS platforms Realtek has used this interrupt router/controller.

I don't have much time anymore today, but I'll break my head over it again tomorrow.

Best,
Sander