[PATCH v5 0/4] Per-parent domains for realtek-rtl IRQ driver

[PATCH v5 0/4] Per-parent domains for realtek-rtl IRQ driver

[Sander Vanheule]

The original implementation for this interrupt controller/router used
an interrupt-map parser to determine which parent interrupts were
present. However, this controller is not transparent, so a list of
parent interrupts seems more appropriate, while also getting rid of the
assumed routing to parent interrupts.

Additionally, N real cascaded interrupts are implemented, instead of
handling all input interrupts with one cascaded interrupt. Otherwise it
is possible that the priority of the parent interrupts is not respected.

Changes since v4:
Link: https://lore.kernel.org/all/cover.1644165421.git.sander@svanheule.net/

- Add Rob's Reviewed-by
- Use irq_domain_add_linear instead of irq_domain_add_simple
- Drop 'inline' specifiers from static functions
- Drop WARN in intc_select() to only warn once for old bindings

Changes since v3:
Link: https://lore.kernel.org/all/cover.1641739718.git.sander@svanheule.net/

- Patches with fixes were merged, so these are no longer included.
- Update the devicetree changes to more clearly indicate the controller
  is not transparent.

Changes since v2 (RFC):
Link: https://lore.kernel.org/all/cover.1640548009.git.sander@svanheule.net/

- Define new, two-part compatibles for devicetree bindings. The existing format
  is kept for the old one-part compatible, but deprecated. New compatibles will
  require a different way of specifying parent interrupts and interrupt routing.
- Add change to handle all pending SoC interrupts in one go.

Changes since v1 (RFC):
Link: https://lore.kernel.org/all/cover.1640261161.git.sander@svanheule.net/

- Split some of the changes to limit the patch scope to one issue.
- Dropped some small (spurious or unneeded) changes
- Instead of dropping/replacing interrupt-map, the last patches now provide an
  implementation that amends the current situtation.

Sander Vanheule (4):
  irqchip/realtek-rtl: use irq_domain_add_linear
  dt-bindings: interrupt-controller: realtek,rtl-intc: require parents
  irqchip/realtek-rtl: use parent interrupts
  irqchip/realtek-rtl: use per-parent domains

 .../realtek,rtl-intc.yaml                     |  82 +++++--
 drivers/irqchip/irq-realtek-rtl.c             | 231 ++++++++++++------
 2 files changed, 221 insertions(+), 92 deletions(-)

-- 
2.35.1

[PATCH v5 1/4] irqchip/realtek-rtl: use irq_domain_add_linear

[Sander Vanheule]

When using an offset of 0, irq_domain_add_simple is identical to
irq_domain_add_linear on DT-based systems, so use the latter instead.

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

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 50a56820c99b..14e049eed0f2 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -171,8 +171,7 @@ static int __init realtek_rtl_of_init(struct device_node *node, struct device_no
 	/* Disable all cascaded interrupts */
 	writel(0, REG(RTL_ICTL_GIMR));
 
-	domain = irq_domain_add_simple(node, 32, 0,
-				       &irq_domain_ops, NULL);
+	domain = irq_domain_add_linear(node, 32, &irq_domain_ops, NULL);
 
 	ret = map_interrupts(node, domain);
 	if (ret) {
-- 
2.35.1

[PATCH v5 2/4] dt-bindings: interrupt-controller: realtek,rtl-intc: require parents

[Sander Vanheule]

The interrupt router has 32 inputs, and up to 15 outputs connected to
the MIPS CPU's interrupts. The way these are mapped to each other is
runtime configurable. This controller can also mask individual interrupt
sources, and has a status register to indicate pending interrupts. This
means the controller is not transparent, and the use of "interrupt-map"
inappropriate. Instead, a list of parent interrupts should be specified.

Two-part compatibles are introduced to be able to require "interrupts"
for new devicetrees. The relevant descriptions are extended or added to
more clearly describe the functionality of this controller. The old
compatible, with "interrupt-map" and "#address-cells", is deprecated.
Interrupt specifiers for new compatibles will require two cells, to
indicate the output selection.

To prevent spurious changes to the binding when more SoCs are added,
"allOf" is used with one "if", and the compatible enum only has one
item.

The example is updated to provide a correct example for RTL8380 SoCs.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes in v5:
- Add Rob's Reviewed-by

Changes in v4:
- Indicate more clearly that the controller is not transparent.

 .../realtek,rtl-intc.yaml                     | 82 ++++++++++++++-----
 1 file changed, 62 insertions(+), 20 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..f212962afd62 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
@@ -6,6 +6,14 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 
 title: Realtek RTL SoC interrupt controller devicetree bindings
 
+description:
+  Interrupt controller and router for Realtek MIPS SoCs, allowing each SoC
+  interrupt to be routed to one parent CPU (hardware) interrupt, or left
+  disconnected.
+  All connected input lines from SoC peripherals can be masked individually,
+  and an interrupt status register is present to indicate which interrupts are
+  pending.
+
 maintainers:
   - Birger Koblitz <mail@birger-koblitz.de>
   - Bert Vermeulen <bert@biot.com>
@@ -13,45 +21,79 @@ maintainers:
 
 properties:
   compatible:
-    const: realtek,rtl-intc
+    oneOf:
+      - items:
+          - enum:
+              - realtek,rtl8380-intc
+          - const: realtek,rtl-intc
+      - const: realtek,rtl-intc
+        deprecated: true
 
-  "#interrupt-cells":
-    const: 1
+  "#interrupt-cells": true
 
   reg:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 15
+    description:
+      List of parent interrupts, in the order that they are connected to this
+      interrupt router's outputs.
 
   interrupt-controller: true
 
-  "#address-cells":
-    const: 0
-
-  interrupt-map:
-    description: Describes mapping from SoC interrupts to CPU interrupts
-
 required:
   - compatible
   - reg
   - "#interrupt-cells"
   - interrupt-controller
-  - "#address-cells"
-  - interrupt-map
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          const: realtek,rtl-intc
+    then:
+      properties:
+        "#interrupt-cells":
+          const: 1
+
+        "#address-cells":
+          const: 0
+
+        interrupt-map: true
+      required:
+        - "#address-cells"
+        - interrupt-map
+    else:
+      properties:
+        "#interrupt-cells":
+          description:
+            Two cells to specify which line to connect to, and which output it should
+            be routed to. Both cells use a zero-based index.
+          const: 2
+      required:
+        - interrupts
 
 additionalProperties: false
 
 examples:
   - |
     intc: interrupt-controller@3000 {
-      compatible = "realtek,rtl-intc";
-      #interrupt-cells = <1>;
+      compatible = "realtek,rtl8380-intc", "realtek,rtl-intc";
+      #interrupt-cells = <2>;
       interrupt-controller;
-      reg = <0x3000 0x20>;
-      #address-cells = <0>;
-      interrupt-map =
-              <31 &cpuintc 2>,
-              <30 &cpuintc 1>,
-              <29 &cpuintc 5>;
+      reg = <0x3000 0x18>;
+
+      interrupt-parent = <&cpuintc>;
+      interrupts = <2>, <3>, <4>, <5>, <6>;
+    };
+
+    irq-consumer@0 {
+      reg = <0 4>;
+      interrupt-parent = <&intc>;
+      interrupts =
+        <19 3>, /* IRQ 19, routed to output 3 (cpuintc 5) */
+        <18 4>; /* IRQ 18, routed to output 4 (cpuintc 6) */
     };
-- 
2.35.1

[PATCH v5 3/4] irqchip/realtek-rtl: use parent interrupts

[Sander Vanheule]

The interrupt-map property for "realtek,rtl-intc" has been deprecated in
favor of a list of parent interrupts. Drop the open-coded parser for
interrupt-map, and use the first parent interrupt instead. If no parent
was provided, the driver will assume that this is the first hardware
interrupt of the SoC's MIPS CPU.

All SoC interrupts were treated equally, independent of which output
they were actually routed to. This means the driver might as well route
all interrupts to the first output, and achieve the same behaviour.

Without the interrupt-map property, interrupt routing information is no
longer available at initialisation. Routing setup will now happen later,
when a hardware interrupt is mapped by the subsystem.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
Changes since v4:
- Use irq_domain_add_linear instead of irq_domain_add_simple
- Drop 'inline' specifiers from static functions

 drivers/irqchip/irq-realtek-rtl.c | 133 ++++++++++++++----------------
 1 file changed, 61 insertions(+), 72 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 14e049eed0f2..2a349082af81 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -21,11 +21,33 @@
 #define RTL_ICTL_IRR2		0x10
 #define RTL_ICTL_IRR3		0x14
 
+#define RTL_ICTL_NUM_INPUTS	32
+
 #define REG(x)		(realtek_ictl_base + x)
 
 static DEFINE_RAW_SPINLOCK(irq_lock);
 static void __iomem *realtek_ictl_base;
 
+/*
+ * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted numbering,
+ * placing IRQ 31 in the first four bits. A routing value of '0' means the
+ * interrupt is left disconnected. Routing values {1..15} connect to output
+ * lines {0..14}.
+ */
+#define IRR_OFFSET(idx)		(4 * (3 - (idx * 4) / 32))
+#define IRR_SHIFT(idx)		((idx * 4) % 32)
+
+static 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)
 {
 	unsigned long flags;
@@ -62,8 +84,14 @@ static struct irq_chip realtek_ictl_irq = {
 
 static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
+	unsigned long flags;
+
 	irq_set_chip_and_handler(irq, &realtek_ictl_irq, handle_level_irq);
 
+	raw_spin_lock_irqsave(&irq_lock, flags);
+	write_irr(REG(RTL_ICTL_IRR0), hw, 1);
+	raw_spin_unlock_irqrestore(&irq_lock, flags);
+
 	return 0;
 }
 
@@ -95,89 +123,50 @@ static void realtek_irq_dispatch(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
-/*
- * 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. A routing value of '0' means the interrupt is left
- * disconnected. Routing values {1..15} connect to output lines {0..14}.
- */
-static int __init map_interrupts(struct device_node *node, struct irq_domain *domain)
-{
-	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;
-
-	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;
-
-	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)
-			return -EINVAL;
-
-		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);
-
-		cpu_int = be32_to_cpup(imap + 2);
-		if (cpu_int > 7 || cpu_int < 2)
-			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);
-		}
-
-		/* Use routing values (1..6) for CPU interrupts (2..7) */
-		regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
-		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 of_phandle_args oirq;
 	struct irq_domain *domain;
-	int ret;
+	unsigned int soc_irq;
+	int parent_irq;
 
 	realtek_ictl_base = of_iomap(node, 0);
 	if (!realtek_ictl_base)
 		return -ENXIO;
 
-	/* Disable all cascaded interrupts */
+	/* Disable all cascaded interrupts and clear routing */
 	writel(0, REG(RTL_ICTL_GIMR));
+	for (soc_irq = 0; soc_irq < RTL_ICTL_NUM_INPUTS; soc_irq++)
+		write_irr(REG(RTL_ICTL_IRR0), soc_irq, 0);
+
+	if (WARN_ON(!of_irq_count(node))) {
+		/*
+		 * If DT contains no parent interrupts, assume MIPS CPU IRQ 2
+		 * (HW0) is connected to the first output. This is the case for
+		 * all known hardware anyway. "interrupt-map" is deprecated, so
+		 * don't bother trying to parse that.
+		 */
+		oirq.np = of_find_compatible_node(NULL, NULL, "mti,cpu-interrupt-controller");
+		oirq.args_count = 1;
+		oirq.args[0] = 2;
+
+		parent_irq = irq_create_of_mapping(&oirq);
+
+		of_node_put(oirq.np);
+	} else {
+		parent_irq = of_irq_get(node, 0);
+	}
 
-	domain = irq_domain_add_linear(node, 32, &irq_domain_ops, NULL);
+	if (parent_irq < 0)
+		return parent_irq;
+	else if (!parent_irq)
+		return -ENODEV;
 
-	ret = map_interrupts(node, domain);
-	if (ret) {
-		pr_err("invalid interrupt map\n");
-		return ret;
-	}
+	domain = irq_domain_add_linear(node, RTL_ICTL_NUM_INPUTS, &irq_domain_ops, NULL);
+	if (!domain)
+		return -ENOMEM;
+
+	irq_set_chained_handler_and_data(parent_irq, realtek_irq_dispatch, domain);
 
 	return 0;
 }
-- 
2.35.1

[PATCH v5 4/4] irqchip/realtek-rtl: use per-parent domains

[Sander Vanheule]

The interrupt router can connect each of its inputs to one of the parent
interrupts. These parent interrupts may be handled differently by their
interrupt controller. SoC interrupts should be treated per-parent, to
maintain this expected behaviour for routed child interrupts.

For example, it is possible that both networking interrupts and the
system event timer interrupts are routed through this controller. Even
under high network load, event timer interrupts should take precedence,
which can be ensured by routing them to a higher priority parent.

Rework the driver to use a separate domain for each output, using all
available parents interrupts (as specified in the devicetree). A
per-parent mask of child interrupts is used to keep track of which
domain should handle which interrupts.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
Changes since v4:
- Use irq_domain_add_linear instead of irq_domain_add_simple
- Drop 'inline' specifiers from static functions
- Drop WARN in intc_select() to only warn once for old bindings

 drivers/irqchip/irq-realtek-rtl.c | 149 +++++++++++++++++++++++++-----
 1 file changed, 124 insertions(+), 25 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 2a349082af81..0cd25e2cb8cc 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -22,12 +22,22 @@
 #define RTL_ICTL_IRR3		0x14
 
 #define RTL_ICTL_NUM_INPUTS	32
+#define RTL_ICTL_NUM_OUTPUTS	15
 
 #define REG(x)		(realtek_ictl_base + x)
 
 static DEFINE_RAW_SPINLOCK(irq_lock);
 static void __iomem *realtek_ictl_base;
 
+struct realtek_ictl_output {
+	/* IRQ controller data */
+	struct fwnode_handle *fwnode;
+	/* Output specific data */
+	unsigned int output_index;
+	struct irq_domain *domain;
+	u32 child_mask;
+};
+
 /*
  * IRR0-IRR3 store 4 bits per interrupt, but Realtek uses inverted numbering,
  * placing IRQ 31 in the first four bits. A routing value of '0' means the
@@ -37,6 +47,11 @@ static void __iomem *realtek_ictl_base;
 #define IRR_OFFSET(idx)		(4 * (3 - (idx * 4) / 32))
 #define IRR_SHIFT(idx)		((idx * 4) % 32)
 
+static u32 read_irr(void __iomem *irr0, int idx)
+{
+	return (readl(irr0 + IRR_OFFSET(idx)) >> IRR_SHIFT(idx)) & 0xf;
+}
+
 static void write_irr(void __iomem *irr0, int idx, u32 value)
 {
 	unsigned int offset = IRR_OFFSET(idx);
@@ -84,51 +99,128 @@ static struct irq_chip realtek_ictl_irq = {
 
 static int intc_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
 {
+	struct realtek_ictl_output *output = d->host_data;
 	unsigned long flags;
+	u32 routing_old;
+	int err = 0;
+
+	raw_spin_lock_irqsave(&irq_lock, flags);
+
+	/*
+	 * Inputs can only be routed to one output, so they shouldn't end up in
+	 * multiple domains. Perform this check in the same atomic context as
+	 * configuring the routing to prevent races.
+	 */
+	routing_old = read_irr(REG(RTL_ICTL_IRR0), hw);
+	if (routing_old && output->output_index != routing_old - 1) {
+		pr_err("int %ld already routed to output %d\n",
+			hw, routing_old - 1);
+		err = -EINVAL;
+		goto out;
+	}
+
+	output->child_mask |= BIT(hw);
+	write_irr(REG(RTL_ICTL_IRR0), hw, output->output_index + 1);
 
 	irq_set_chip_and_handler(irq, &realtek_ictl_irq, handle_level_irq);
 
-	raw_spin_lock_irqsave(&irq_lock, flags);
-	write_irr(REG(RTL_ICTL_IRR0), hw, 1);
+out:
 	raw_spin_unlock_irqrestore(&irq_lock, flags);
 
-	return 0;
+	return err;
+}
+
+static int intc_select(struct irq_domain *d, struct irq_fwspec *fwspec,
+	enum irq_domain_bus_token bus_token)
+{
+	struct realtek_ictl_output *output = d->host_data;
+
+	if (fwspec->fwnode != output->fwnode)
+		return false;
+
+	/* Original specifiers only had one parameter */
+	if (fwspec->param_count < 2)
+		return true;
+
+	return fwspec->param[1] == output->output_index;
 }
 
 static const struct irq_domain_ops irq_domain_ops = {
 	.map = intc_map,
+	.select = intc_select,
 	.xlate = irq_domain_xlate_onecell,
 };
 
 static void realtek_irq_dispatch(struct irq_desc *desc)
 {
+	struct realtek_ictl_output *output = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct irq_domain *domain;
 	unsigned long pending;
 	unsigned int soc_int;
 
 	chained_irq_enter(chip, desc);
-	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR));
+	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
+		& output->child_mask;
 
 	if (unlikely(!pending)) {
 		spurious_interrupt();
 		goto out;
 	}
 
-	domain = irq_desc_get_handler_data(desc);
-	for_each_set_bit(soc_int, &pending, 32)
-		generic_handle_domain_irq(domain, soc_int);
+	for_each_set_bit(soc_int, &pending, RTL_ICTL_NUM_INPUTS)
+		generic_handle_domain_irq(output->domain, soc_int);
 
 out:
 	chained_irq_exit(chip, desc);
 }
 
+static int __init setup_parent_interrupts(struct device_node *node, int *parents,
+	unsigned int num_parents)
+{
+	struct realtek_ictl_output *outputs;
+	struct realtek_ictl_output *output;
+	struct irq_domain *domain;
+	unsigned int p;
+
+	outputs = kcalloc(num_parents, sizeof(*outputs), GFP_KERNEL);
+	if (!outputs)
+		return -ENOMEM;
+
+	for (p = 0; p < num_parents; p++) {
+		output = outputs + p;
+
+		domain = irq_domain_add_linear(node, RTL_ICTL_NUM_INPUTS,
+						&irq_domain_ops, output);
+		if (!domain)
+			goto domain_err;
+
+		output->fwnode = of_node_to_fwnode(node);
+		output->output_index = p;
+		output->domain = domain;
+
+		irq_set_chained_handler_and_data(parents[p], realtek_irq_dispatch, output);
+	}
+
+	return 0;
+
+domain_err:
+	while (p--) {
+		irq_set_chained_handler_and_data(parents[p], NULL, NULL);
+		irq_domain_remove(outputs[p].domain);
+	}
+
+	kfree(outputs);
+
+	return -ENOMEM;
+}
+
 static int __init realtek_rtl_of_init(struct device_node *node, struct device_node *parent)
 {
+	int parent_irqs[RTL_ICTL_NUM_OUTPUTS];
 	struct of_phandle_args oirq;
-	struct irq_domain *domain;
+	unsigned int num_parents;
 	unsigned int soc_irq;
-	int parent_irq;
+	unsigned int p;
 
 	realtek_ictl_base = of_iomap(node, 0);
 	if (!realtek_ictl_base)
@@ -139,36 +231,43 @@ static int __init realtek_rtl_of_init(struct device_node *node, struct device_no
 	for (soc_irq = 0; soc_irq < RTL_ICTL_NUM_INPUTS; soc_irq++)
 		write_irr(REG(RTL_ICTL_IRR0), soc_irq, 0);
 
-	if (WARN_ON(!of_irq_count(node))) {
+	num_parents = of_irq_count(node);
+	if (num_parents > RTL_ICTL_NUM_OUTPUTS) {
+		pr_err("too many parent interrupts\n");
+		return -EINVAL;
+	}
+
+	for (p = 0; p < num_parents; p++)
+		parent_irqs[p] = of_irq_get(node, p);
+
+	if (WARN_ON(!num_parents)) {
 		/*
 		 * If DT contains no parent interrupts, assume MIPS CPU IRQ 2
 		 * (HW0) is connected to the first output. This is the case for
 		 * all known hardware anyway. "interrupt-map" is deprecated, so
 		 * don't bother trying to parse that.
+		 * Since this is to account for old devicetrees with one-cell
+		 * interrupt specifiers, only one output domain is needed.
 		 */
 		oirq.np = of_find_compatible_node(NULL, NULL, "mti,cpu-interrupt-controller");
 		oirq.args_count = 1;
 		oirq.args[0] = 2;
 
-		parent_irq = irq_create_of_mapping(&oirq);
+		parent_irqs[0] = irq_create_of_mapping(&oirq);
+		num_parents = 1;
 
 		of_node_put(oirq.np);
-	} else {
-		parent_irq = of_irq_get(node, 0);
 	}
 
-	if (parent_irq < 0)
-		return parent_irq;
-	else if (!parent_irq)
-		return -ENODEV;
-
-	domain = irq_domain_add_linear(node, RTL_ICTL_NUM_INPUTS, &irq_domain_ops, NULL);
-	if (!domain)
-		return -ENOMEM;
-
-	irq_set_chained_handler_and_data(parent_irq, realtek_irq_dispatch, domain);
+	/* Ensure we haven't collected any errors before proceeding */
+	for (p = 0; p < num_parents; p++) {
+		if (parent_irqs[p] < 0)
+			return parent_irqs[p];
+		if (!parent_irqs[p])
+			return -ENODEV;
+	}
 
-	return 0;
+	return setup_parent_interrupts(node, parent_irqs, num_parents);
 }
 
 IRQCHIP_DECLARE(realtek_rtl_intc, "realtek,rtl-intc", realtek_rtl_of_init);
-- 
2.35.1

Re: [PATCH v5 0/4] Per-parent domains for realtek-rtl IRQ driver

[Marc Zyngier]

On Mon, 14 Feb 2022 18:56:57 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
> 
> The original implementation for this interrupt controller/router used
> an interrupt-map parser to determine which parent interrupts were
> present. However, this controller is not transparent, so a list of
> parent interrupts seems more appropriate, while also getting rid of the
> assumed routing to parent interrupts.
> 
> Additionally, N real cascaded interrupts are implemented, instead of
> handling all input interrupts with one cascaded interrupt. Otherwise it
> is possible that the priority of the parent interrupts is not respected.

My original question[1] still stands. An old kernel breaks with a new
DT. I am not convinced that this is an acceptable outcome.

	M.

[1] https://lore.kernel.org/all/874k585efy.wl-maz@kernel.org

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

Re: [PATCH v5 0/4] Per-parent domains for realtek-rtl IRQ driver

[Sander Vanheule]

Hi Marc,

On Tue, 2022-02-15 at 12:09 +0000, Marc Zyngier wrote:
> On Mon, 14 Feb 2022 18:56:57 +0000,
> Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > The original implementation for this interrupt controller/router used
> > an interrupt-map parser to determine which parent interrupts were
> > present. However, this controller is not transparent, so a list of
> > parent interrupts seems more appropriate, while also getting rid of the
> > assumed routing to parent interrupts.
> > 
> > Additionally, N real cascaded interrupts are implemented, instead of
> > handling all input interrupts with one cascaded interrupt. Otherwise it
> > is possible that the priority of the parent interrupts is not respected.
> 
> My original question[1] still stands. An old kernel breaks with a new
> DT. I am not convinced that this is an acceptable outcome.
> 
>         M.
> 
> [1] https://lore.kernel.org/all/874k585efy.wl-maz@kernel.org


My apologies for the delay in replying, although I suppose the lack of response
from others perhaps indicates that there is little interest maintaining old
kernel/new DT compatibility for this hardware. John has previously argued in
favour of breaking compatibility [2].

Chances of someone running a vanilla kernel build on this hardware are close to
zero at this moment. The most important part, the internal ethernet switch, is
only supported with out-of-tree patches. If patches can be included on an old
(LTS) kernel to provide networking support, then patches can be included to be
compatible with a new DT specification for the interrupts as well. OpenWrt does
exactly this: use an old (5.10) kernel with new upstream features backported.

The binding could be adjusted to allow (but deprecate) interrupt-map for the new
two-part compatibles. This would require a new DT to both specify two-cell
interrupt specifiers, and an equivalent interrupt-map definition to ensure
perfect two-way compatibility. This duplicated info would need to be maintained
for years however, as LTS kernels stay around for a long time. In my opinion,
breaking compatibility with old kernels would allow us to move forward with a
cleaner driver and binding.

Best,
Sander

[2]
https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/