[RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

[RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

[Sander Vanheule]

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 (patch 1)
- routing has an off-by-one error. Routing values (1..6) correspond to
  MIPS CAUSEF(2..7) (patch 2)

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. (patch 3)
- Get rid of assumed routing to parent interrupts of the original
  implementation (patch 4, 5)

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

Still an RFC. Mainly since I don't like the open coding in the last
patch, but also since I still have a question about the chained IRQ
handlers.

- 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 (5):
  irqchip/realtek-rtl: map control data to virq
  irqchip/realtek-rtl: fix off-by-one in routing
  irqchip/realtek-rtl: use per-parent irq handling
  dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines
  irqchip/realtek-rtl: add explicit output routing

 .../realtek,rtl-intc.yaml                     |  38 ++-
 drivers/irqchip/irq-realtek-rtl.c             | 232 ++++++++++++++----
 2 files changed, 218 insertions(+), 52 deletions(-)

-- 
2.33.1

[RFC PATCH v2 1/5] irqchip/realtek-rtl: map control data to virq

[Sander Vanheule]

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

[RFC PATCH v2 2/5] irqchip/realtek-rtl: fix off-by-one in routing

[Sander Vanheule]

There is an offset between routing values (1..6) and the connected MIPS
CPU interrupts (2..7), but no distinction was made between these two
values.

This issue was previously hidden during testing, because an interrupt
mapping was used where for each required interrupt another (unused)
routing was configured, with an offset of +1.

Offset the CPU IRQ numbers by -1 to retrieve the correct routing value.

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 | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index d6788dd93c7b..568614edd88f 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *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.
+ * 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)
 {
@@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
 		of_node_put(cpu_ictl);
 
 		cpu_int = be32_to_cpup(imap + 2);
-		if (cpu_int > 7)
+		if (cpu_int > 7 || cpu_int < 2)
 			return -EINVAL;
 
 		if (!(mips_irqs_set & BIT(cpu_int))) {
@@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
 			mips_irqs_set |= BIT(cpu_int);
 		}
 
-		regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
+		/* Use routing values (1..6) for CPU interrupts (2..7) */
+		regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
 		imap += 3;
 	}
 
-- 
2.33.1

[RFC PATCH v2 3/5] irqchip/realtek-rtl: use per-parent irq handling

[Sander Vanheule]

The driver handled all SoC interrupts equally, independent of which
parent interrupt it is routed to. Between all configured inputs, the use
of __ffs actually gives higher priority to lower input lines,
effectively bypassing any priority there might be among the parent
interrupts.

Rework the driver to use a separate handler for each parent interrupt,
to respect the order in which those parents interrupt are handled.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
With switching back to chained handlers, it became impossible to route a
SoC interrupt to the timer interrupt (CPU IRQ 7) on systems using the
CEVT-R4K timer. If a chained handler is already set for the timer
interrupt, the timer cannot request it anymore (due to IRQ_NOREQUEST)
and the system hangs. It is probably a terrible idea to also run e.g.
the console on the timer interrupt, but it is possible. If there are no
solutions to this, I can live with it though; there are still 5 other
interrupts.

Changes since v1:
- Limit scope to per-parent handling
- Replace the "priority" naming with the more generic "output"
- Don't request interrupts, but stick to chained handlers
---
 drivers/irqchip/irq-realtek-rtl.c | 109 ++++++++++++++++++++----------
 1 file changed, 74 insertions(+), 35 deletions(-)

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 568614edd88f..1f8f21a0bd1a 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,45 @@
 #define RTL_ICTL_IRR2		0x10
 #define RTL_ICTL_IRR3		0x14
 
+#define RTL_ICTL_NUM_OUTPUTS	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_output {
+	unsigned int routing_value;
+	u32 child_mask;
+};
+
+static struct realtek_ictl_output realtek_ictl_outputs[RTL_ICTL_NUM_OUTPUTS];
+
+/*
+ * 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 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)
 {
@@ -74,42 +110,45 @@ static const struct irq_domain_ops irq_domain_ops = {
 
 static void realtek_irq_dispatch(struct irq_desc *desc)
 {
+	struct realtek_ictl_output *parent = irq_desc_get_handler_data(desc);
 	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));
+	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
+		& parent->child_mask;
+
 	if (unlikely(!pending)) {
 		spurious_interrupt();
 		goto out;
 	}
-	domain = irq_desc_get_handler_data(desc);
-	generic_handle_domain_irq(domain, __ffs(pending));
+	generic_handle_domain_irq(realtek_ictl_domain, __ffs(pending));
 
 out:
 	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)
+static void __init set_routing(struct realtek_ictl_output *output, unsigned int soc_int)
 {
+	unsigned int routing_old;
+
+	routing_old = read_irr(REG(RTL_ICTL_IRR0), soc_int);
+	if (routing_old) {
+		pr_warn("int %d already routed to %d, not updating\n", soc_int, routing_old);
+		return;
+	}
+
+	output->child_mask |= BIT(soc_int);
+	write_irr(REG(RTL_ICTL_IRR0), soc_int, output->routing_value);
+}
+
+static int __init map_interrupts(struct device_node *node)
+{
+	struct realtek_ictl_output *output;
 	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, cpu_int, tmp;
+	int ret, i;
 
 	ret = of_property_read_u32(node, "#address-cells", &tmp);
 	if (ret || tmp)
@@ -119,8 +158,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)
@@ -138,39 +175,41 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
 		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);
+		output = &realtek_ictl_outputs[cpu_int - 2];
+
+		if (!output->routing_value) {
+			irq_set_chained_handler_and_data(cpu_int, realtek_irq_dispatch, output);
+			/* Use routing values (1..6) for CPU interrupts (2..7) */
+			output->routing_value = cpu_int - 1;
 		}
 
-		/* Use routing values (1..6) for CPU interrupts (2..7) */
-		regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
+		set_routing(output, 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(&realtek_ictl_outputs, 0, sizeof(realtek_ictl_outputs));
+
 	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

[RFC PATCH v2 4/5] dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines

[Sander Vanheule]

Amend the binding to also require a list of parent interrupts, and an
optional mask to specify which parent is mapped to which output.

Without this information, any driver would have to make an assumption on
which parent interrupt is connected to which output.

Additionally, extend (or add) the relevant descriptions to more clearly
describe the inputs and outputs of this router.

Signed-off-by: Sander Vanheule <sander@svanheule.net>
---
Since it does not properly describe the hardware, I would still really
rather get rid of "interrupt-map", even though that would mean breaking
ABI for this binding. As we've argued before [1], that is our prefered
solution, and would enable us to not carry more (hacky) code because of
a mistake with the initial submission.

Vendors don't ship independent DT blobs for devices with this hardware,
so the independent devicetree/kernel upgrades issue is really rather
theoretical here. Realtek isn't driving the development of the bindings
and associated drivers for this platform. They have their SDK and seem
to care very little about proper kernel integration.

Furthermore, there are currently no device descriptions in the kernel
using this binding. There are in OpenWrt, but OpenWrt firmware images
for this platform always contain both the kernel and the appended DTB,
so there's also no breakage to worry about.

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

Differences with v1:
- Don't drop the "interrupt-map" property
- Add the "realtek,output-valid-mask" property
---
 .../realtek,rtl-intc.yaml                     | 38 ++++++++++++++++---
 1 file changed, 33 insertions(+), 5 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..29014673c34e 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
@@ -6,6 +6,10 @@ $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.
+
 maintainers:
   - Birger Koblitz <mail@birger-koblitz.de>
   - Bert Vermeulen <bert@biot.com>
@@ -22,7 +26,11 @@ properties:
     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
 
@@ -30,7 +38,21 @@ properties:
     const: 0
 
   interrupt-map:
-    description: Describes mapping from SoC interrupts to CPU interrupts
+    description:
+      List of <soc_int parent_phandle parent_args ...> tuples, where "soc_int"
+      is the interrupt input line number as provided by this controller.
+      "parent_phandle" and "parent_args" specify which parent interrupt this
+      line should be routed to. Note that interrupt specifiers should be
+      identical to the parents specified in the "interrupts" property.
+
+  realtek,output-valid-mask:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Optional bit mask indicating which outputs are connected to the parent
+      interrupts. The lowest set bit indicates which output line the first
+      interrupt from "interrupts" is connected to, the second lowest set bit
+      for the second interrupt, etc. If not provided, parent interrupts will be
+      assigned sequentially to the outputs.
 
 required:
   - compatible
@@ -39,6 +61,7 @@ required:
   - interrupt-controller
   - "#address-cells"
   - interrupt-map
+  - interrupts
 
 additionalProperties: false
 
@@ -49,9 +72,14 @@ examples:
       #interrupt-cells = <1>;
       interrupt-controller;
       reg = <0x3000 0x20>;
+
+      interrupt-parent = <&cpuintc>;
+      interrupts = <1>, <2>, <5>;
+      realtek,output-valid-mask = <0x13>;
+
       #address-cells = <0>;
       interrupt-map =
-              <31 &cpuintc 2>,
-              <30 &cpuintc 1>,
-              <29 &cpuintc 5>;
+              <31 &cpuintc 2>, /* connect to cpuintc 2 via output 1 */
+              <30 &cpuintc 1>, /* connect to cpuintc 1 via output 0 */
+              <29 &cpuintc 5>; /* connect to cpuintc 5 via output 4 */
     };
-- 
2.33.1

[RFC PATCH v2 5/5] irqchip/realtek-rtl: add explicit output routing

[Sander Vanheule]

Use the list of parent interrupts, and the optional output validity
mask, to build the list of routed output interrupts. This enables us to
remove the assumption that interrupt outputs (1..6) are always connected
to MIPS CPU interrupts (2..7).

Since the use of interrupt-map is non-standard, extra logic is required
to resolve the specified interrupts to one of the parent interrupts.

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

diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
index 1f8f21a0bd1a..1b9c1108e945 100644
--- a/drivers/irqchip/irq-realtek-rtl.c
+++ b/drivers/irqchip/irq-realtek-rtl.c
@@ -22,6 +22,8 @@
 #define RTL_ICTL_IRR2		0x10
 #define RTL_ICTL_IRR3		0x14
 
+/* Support up to 6 active outputs for now */
+#define RTL_ICTL_OUTPUT_MASK	GENMASK(14, 0)
 #define RTL_ICTL_NUM_OUTPUTS	6
 
 #define REG(x)		(realtek_ictl_base + x)
@@ -31,6 +33,7 @@ static void __iomem *realtek_ictl_base;
 static struct irq_domain *realtek_ictl_domain;
 
 struct realtek_ictl_output {
+	int parent;
 	unsigned int routing_value;
 	u32 child_mask;
 };
@@ -142,10 +145,58 @@ static void __init set_routing(struct realtek_ictl_output *output, unsigned int
 	write_irr(REG(RTL_ICTL_IRR0), soc_int, output->routing_value);
 }
 
+static int __init resolve_parent(struct device_node *node, const __be32 *table,
+	unsigned int table_len, int *parent)
+{
+	struct of_phandle_args parent_irq;
+	struct device_node *parent_ictl;
+	unsigned int parent_cell_count;
+	unsigned int table_len_left;
+	int cell;
+	int ret;
+
+	if (!table_len)
+		return -EINVAL;
+
+	parent_ictl = of_find_node_by_phandle(be32_to_cpup(table++));
+	table_len_left = table_len - 1;
+
+	if (!parent_ictl)
+		return -EINVAL;
+
+	parent_irq.np = parent_ictl;
+
+	parent_cell_count = 0;
+	ret = of_property_read_u32(parent_ictl, "#interrupt-cells", &parent_cell_count);
+	if (ret)
+		goto out;
+
+	if (table_len_left < parent_cell_count) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	parent_irq.args_count = parent_cell_count;
+
+	for (cell = 0; cell < parent_cell_count; cell++)
+		parent_irq.args[cell] = be32_to_cpup(table++);
+
+	table_len_left -= parent_cell_count;
+
+	ret = of_irq_parse_raw(of_get_property(node, "reg", NULL), &parent_irq);
+	if (ret < 0)
+		goto out;
+
+	*parent = irq_create_of_mapping(&parent_irq);
+	ret = table_len - table_len_left;
+out:
+	of_node_put(parent_ictl);
+	return ret;
+}
+
 static int __init map_interrupts(struct device_node *node)
 {
 	struct realtek_ictl_output *output;
-	struct device_node *cpu_ictl;
 	const __be32 *imap;
 	u32 imaplen, soc_int, cpu_int, tmp;
 	int ret, i;
@@ -158,34 +209,76 @@ static int __init map_interrupts(struct device_node *node)
 	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;
+	imaplen /= sizeof(*imap);
+	while (imaplen > 1) {
+		soc_int = be32_to_cpup(imap++);
+		imaplen--;
 
-		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)
+		if (soc_int > 31)
 			return -EINVAL;
-		of_node_put(cpu_ictl);
 
-		cpu_int = be32_to_cpup(imap + 2);
-		if (cpu_int > 7 || cpu_int < 2)
-			return -EINVAL;
+		cpu_int = 0;
+		ret = resolve_parent(node, imap, imaplen, &cpu_int);
+		if (ret < 0)
+			return ret;
+		imaplen -= ret;
+		imap += ret;
 
-		output = &realtek_ictl_outputs[cpu_int - 2];
+		i = 0;
+		output = &realtek_ictl_outputs[0];
 
-		if (!output->routing_value) {
-			irq_set_chained_handler_and_data(cpu_int, realtek_irq_dispatch, output);
-			/* Use routing values (1..6) for CPU interrupts (2..7) */
-			output->routing_value = cpu_int - 1;
+		while (i < RTL_ICTL_NUM_OUTPUTS && output->parent != cpu_int) {
+			output++;
+			i++;
 		}
 
+		if (i == RTL_ICTL_NUM_OUTPUTS)
+			return -EINVAL;
+
 		set_routing(output, soc_int);
+	}
+
+	return 0;
+}
 
-		imap += 3;
+static int __init route_parent_interrupts(struct device_node *node)
+{
+	struct realtek_ictl_output *output;
+	unsigned int current_output;
+	unsigned int num_parents;
+	unsigned int parent;
+	u32 output_mask;
+	int parent_irq;
+
+	output_mask = RTL_ICTL_OUTPUT_MASK;
+	of_property_read_u32(node, "realtek,output-valid-mask", &output_mask);
+	if (output_mask & ~RTL_ICTL_OUTPUT_MASK) {
+		pr_warn("realtek,output-valid-mask contains unsupported outputs\n");
+		output_mask &= RTL_ICTL_OUTPUT_MASK;
+	}
+
+	num_parents = of_irq_count(node);
+	if (num_parents > RTL_ICTL_NUM_OUTPUTS) {
+		pr_err("too many parent interrupts\n");
+		return -EINVAL;
+	}
+
+	for (parent = 0; output_mask && parent < num_parents; parent++) {
+		current_output = __ffs(output_mask);
+
+		parent_irq = of_irq_get(node, parent);
+		if (parent_irq < 0)
+			return parent_irq;
+		else if (!parent_irq)
+			return -ENODEV;
+
+		output = &realtek_ictl_outputs[parent];
+		output->parent = parent_irq;
+		output->routing_value = current_output + 1;
+
+		irq_set_chained_handler_and_data(parent_irq, realtek_irq_dispatch, output);
+
+		output_mask &= ~BIT(current_output);
 	}
 
 	return 0;
@@ -209,6 +302,10 @@ 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 = route_parent_interrupts(node);
+	if (ret)
+		return ret;
+
 	ret = map_interrupts(node);
 	if (ret) {
 		pr_err("invalid interrupt map\n");
-- 
2.33.1

Re: [RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

[Birger Koblitz]

Hi,

I don't think the IRQ routing has an off-by one error. This was chosen
by John to correspond to Realtek's own "documentation" and to
take account of the special meaning of IRQs 0, 1 for VSMP and 6 and 7
for the Realtek SoCs. In any case it would break the ABI as the meaning
of these values changes and I don't think the change in range actually
gives any additional functionality.

With regards to the RTL8390, that SoC actually has two IRQ controllers
to allow VSMP. The changes in parent routing have a good chance of breaking
VSMP on the RTL8390 targets. Did you stress test this new logic under VSMP?

Cheers,
   Birger


On 26/12/2021 20:59, Sander Vanheule wrote:
> 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 (patch 1)
> - routing has an off-by-one error. Routing values (1..6) correspond to
>    MIPS CAUSEF(2..7) (patch 2)
> 
> 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. (patch 3)
> - Get rid of assumed routing to parent interrupts of the original
>    implementation (patch 4, 5)
> 
> Changes since v1:
> Link: https://lore.kernel.org/all/cover.1640261161.git.sander@svanheule.net/
> 
> Still an RFC. Mainly since I don't like the open coding in the last
> patch, but also since I still have a question about the chained IRQ
> handlers.
> 
> - 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 (5):
>    irqchip/realtek-rtl: map control data to virq
>    irqchip/realtek-rtl: fix off-by-one in routing
>    irqchip/realtek-rtl: use per-parent irq handling
>    dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines
>    irqchip/realtek-rtl: add explicit output routing
> 
>   .../realtek,rtl-intc.yaml                     |  38 ++-
>   drivers/irqchip/irq-realtek-rtl.c             | 232 ++++++++++++++----
>   2 files changed, 218 insertions(+), 52 deletions(-)
> 

Re: [RFC PATCH v2 2/5] irqchip/realtek-rtl: fix off-by-one in routing

[Marc Zyngier]

On Sun, 26 Dec 2021 19:59:25 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
> 
> There is an offset between routing values (1..6) and the connected MIPS
> CPU interrupts (2..7), but no distinction was made between these two
> values.
> 
> This issue was previously hidden during testing, because an interrupt
> mapping was used where for each required interrupt another (unused)
> routing was configured, with an offset of +1.

Where does this 'other routing' come from?

> 
> Offset the CPU IRQ numbers by -1 to retrieve the correct routing value.
> 
> 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
> index d6788dd93c7b..568614edd88f 100644
> --- a/drivers/irqchip/irq-realtek-rtl.c
> +++ b/drivers/irqchip/irq-realtek-rtl.c
> @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *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.
> + * 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)
>  {
> @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
>  		of_node_put(cpu_ictl);
>  
>  		cpu_int = be32_to_cpup(imap + 2);
> -		if (cpu_int > 7)
> +		if (cpu_int > 7 || cpu_int < 2)

How many output lines do you have? The comment above says something
about having 15 output lines, but you limit it to 7...

>  			return -EINVAL;
>  
>  		if (!(mips_irqs_set & BIT(cpu_int))) {
> @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
>  			mips_irqs_set |= BIT(cpu_int);
>  		}
>  
> -		regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
> +		/* Use routing values (1..6) for CPU interrupts (2..7) */
> +		regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
>  		imap += 3;
>  	}
>  

What I don't understand is how this worked so far if all mappings were
off my one. Or the mapping really doesn't matter, because this is all
under SW control?

	M.

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

Re: [RFC PATCH v2 3/5] irqchip/realtek-rtl: use per-parent irq handling

[Marc Zyngier]

On Sun, 26 Dec 2021 19:59:26 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
> 
> The driver handled all SoC interrupts equally, independent of which
> parent interrupt it is routed to. Between all configured inputs, the use
> of __ffs actually gives higher priority to lower input lines,
> effectively bypassing any priority there might be among the parent
> interrupts.
> 
> Rework the driver to use a separate handler for each parent interrupt,
> to respect the order in which those parents interrupt are handled.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
> With switching back to chained handlers, it became impossible to route a
> SoC interrupt to the timer interrupt (CPU IRQ 7) on systems using the
> CEVT-R4K timer. If a chained handler is already set for the timer
> interrupt, the timer cannot request it anymore (due to IRQ_NOREQUEST)
> and the system hangs. It is probably a terrible idea to also run e.g.
> the console on the timer interrupt, but it is possible. If there are no
> solutions to this, I can live with it though; there are still 5 other
> interrupts.

Shared interrupts and chaining don't mix. You can look at it any way
you want, there is always something that breaks eventually.

> 
> Changes since v1:
> - Limit scope to per-parent handling
> - Replace the "priority" naming with the more generic "output"
> - Don't request interrupts, but stick to chained handlers
> ---
>  drivers/irqchip/irq-realtek-rtl.c | 109 ++++++++++++++++++++----------
>  1 file changed, 74 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
> index 568614edd88f..1f8f21a0bd1a 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,45 @@
>  #define RTL_ICTL_IRR2		0x10
>  #define RTL_ICTL_IRR3		0x14
>  
> +#define RTL_ICTL_NUM_OUTPUTS	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_output {
> +	unsigned int routing_value;
> +	u32 child_mask;
> +};
> +
> +static struct realtek_ictl_output realtek_ictl_outputs[RTL_ICTL_NUM_OUTPUTS];
> +
> +/*
> + * 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 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)
>  {
> @@ -74,42 +110,45 @@ static const struct irq_domain_ops irq_domain_ops = {
>  
>  static void realtek_irq_dispatch(struct irq_desc *desc)
>  {
> +	struct realtek_ictl_output *parent = irq_desc_get_handler_data(desc);
>  	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));
> +	pending = readl(REG(RTL_ICTL_GIMR)) & readl(REG(RTL_ICTL_GISR))
> +		& parent->child_mask;
> +
>  	if (unlikely(!pending)) {
>  		spurious_interrupt();
>  		goto out;
>  	}
> -	domain = irq_desc_get_handler_data(desc);
> -	generic_handle_domain_irq(domain, __ffs(pending));
> +	generic_handle_domain_irq(realtek_ictl_domain, __ffs(pending));

You were complaining about the use of __ffs() creating artificial
priorities. And yet you keep using it, recreating the same issue for a
smaller set of interrupts. Why do we need all the complexity of
registering multiple handlers when a simple loop on the pending bits
would ensure some level of fairness?

It looks to me that you are solving a different problem, where you'd
deliver interrupts that have may not yet been signalled to the CPU
yet.  And you definitely should consider consuming all the pending
bits before exiting.

>
>  out:
>  	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)
> +static void __init set_routing(struct realtek_ictl_output *output, unsigned int soc_int)
>  {
> +	unsigned int routing_old;
> +
> +	routing_old = read_irr(REG(RTL_ICTL_IRR0), soc_int);
> +	if (routing_old) {
> +		pr_warn("int %d already routed to %d, not updating\n", soc_int, routing_old);
> +		return;
> +	}
> +
> +	output->child_mask |= BIT(soc_int);
> +	write_irr(REG(RTL_ICTL_IRR0), soc_int, output->routing_value);
> +}
> +
> +static int __init map_interrupts(struct device_node *node)
> +{
> +	struct realtek_ictl_output *output;
>  	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, cpu_int, tmp;
> +	int ret, i;
>  
>  	ret = of_property_read_u32(node, "#address-cells", &tmp);
>  	if (ret || tmp)
> @@ -119,8 +158,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)
> @@ -138,39 +175,41 @@ static int __init map_interrupts(struct device_node *node, struct irq_domain *do
>  		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);
> +		output = &realtek_ictl_outputs[cpu_int - 2];
> +
> +		if (!output->routing_value) {
> +			irq_set_chained_handler_and_data(cpu_int, realtek_irq_dispatch, output);
> +			/* Use routing values (1..6) for CPU interrupts (2..7) */
> +			output->routing_value = cpu_int - 1;

Why do you keep this routing_value around? Its only purpose is to be
read by set_routing(), which already checks for a programmed value.

	M.

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

Re: [RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

[Sander Vanheule]

Hi Birger,

On Monday, 27 December 2021, Birger Koblitz wrote:
> Hi,
> 
> I don't think the IRQ routing has an off-by one error. This was chosen
> by John to correspond to Realtek's own "documentation" and to
> take account of the special meaning of IRQs 0, 1 for VSMP and 6 and 7
> for the Realtek SoCs. In any case it would break the ABI as the meaning
> of these values changes and I don't think the change in range actually
> gives any additional functionality.

Realtek's SDK provides routing register values. I would have to check to see what CPU IRQs it then binds to, to service those interrupts. The binding wouldn't have to change, but we could fix the driver and devicetrees.

The binding specifies that interrupt-map provides a mapping of interrupt inputs to parent interrupt. The driver then takes these values, but doesn't check what the parent interrupt controller actually is, and finally assigns a chained handler to a hardware IRQ index (instead of a VIRQ).

You can try limiting the interrupt-map to only the mapping for UART0 with the current driver, and you will find that you end up with a broken system.

CPU IRQs 0 and 1 are indeed special (IPI for VSMP), yet we have interrupt mappings that contain <... &cpuintc 1>. Furthermore, if you specify a mapping of <... &cpuint 6> for an active interrupt source, you will get spurious timer (CPU IRQ 7) interrupts. This can't be correct. 
 
> With regards to the RTL8390, that SoC actually has two IRQ controllers
> to allow VSMP. The changes in parent routing have a good chance of breaking
> VSMP on the RTL8390 targets. Did you stress test this new logic under VSMP?

I haven't tested this with VSMP, because it is out of scope for this series. For the binding, I expect that would only require N register ranges instead of one; one per CPU. I think the driver should then be able to perform the IRQ balancing based on that information alone, given that the parent IRQs are available at each CPU.

Best,
Sander

Re: [RFC PATCH v2 4/5] dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines

[Marc Zyngier]

On Sun, 26 Dec 2021 19:59:27 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
> 
> Amend the binding to also require a list of parent interrupts, and an
> optional mask to specify which parent is mapped to which output.
> 
> Without this information, any driver would have to make an assumption on
> which parent interrupt is connected to which output.

Why should an endpoint driver care at all?

> 
> Additionally, extend (or add) the relevant descriptions to more clearly
> describe the inputs and outputs of this router.
> 
> Signed-off-by: Sander Vanheule <sander@svanheule.net>
> ---
> Since it does not properly describe the hardware, I would still really
> rather get rid of "interrupt-map", even though that would mean breaking
> ABI for this binding. As we've argued before [1], that is our prefered
> solution, and would enable us to not carry more (hacky) code because of
> a mistake with the initial submission.

Again, this is too late. Broken bindings live forever.

> 
> Vendors don't ship independent DT blobs for devices with this hardware,
> so the independent devicetree/kernel upgrades issue is really rather
> theoretical here. Realtek isn't driving the development of the bindings
> and associated drivers for this platform. They have their SDK and seem
> to care very little about proper kernel integration.

Any vendor can do whatever they want. You can do the same thing if you
really want to.

> 
> Furthermore, there are currently no device descriptions in the kernel
> using this binding. There are in OpenWrt, but OpenWrt firmware images
> for this platform always contain both the kernel and the appended DTB,
> so there's also no breakage to worry about.

That's just one use case. Who knows who is using this stuff in a
different context? Nobody can tell.

> 
> [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/
> 
> Differences with v1:
> - Don't drop the "interrupt-map" property
> - Add the "realtek,output-valid-mask" property
> ---
>  .../realtek,rtl-intc.yaml                     | 38 ++++++++++++++++---
>  1 file changed, 33 insertions(+), 5 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..29014673c34e 100644
> --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> @@ -6,6 +6,10 @@ $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.
> +
>  maintainers:
>    - Birger Koblitz <mail@birger-koblitz.de>
>    - Bert Vermeulen <bert@biot.com>
> @@ -22,7 +26,11 @@ properties:
>      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.

Is that to support multiple SoCs? I'd expect a given SoC to have a
fixed number of output interrupts.

>  
>    interrupt-controller: true
>  
> @@ -30,7 +38,21 @@ properties:
>      const: 0
>  
>    interrupt-map:
> -    description: Describes mapping from SoC interrupts to CPU interrupts
> +    description:
> +      List of <soc_int parent_phandle parent_args ...> tuples, where "soc_int"
> +      is the interrupt input line number as provided by this controller.
> +      "parent_phandle" and "parent_args" specify which parent interrupt this
> +      line should be routed to. Note that interrupt specifiers should be
> +      identical to the parents specified in the "interrupts" property.
> +
> +  realtek,output-valid-mask:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Optional bit mask indicating which outputs are connected to the parent
> +      interrupts. The lowest set bit indicates which output line the first
> +      interrupt from "interrupts" is connected to, the second lowest set bit
> +      for the second interrupt, etc. If not provided, parent interrupts will be
> +      assigned sequentially to the outputs.
>  
>  required:
>    - compatible
> @@ -39,6 +61,7 @@ required:
>    - interrupt-controller
>    - "#address-cells"
>    - interrupt-map
> +  - interrupts
>
>  additionalProperties: false
>  
> @@ -49,9 +72,14 @@ examples:
>        #interrupt-cells = <1>;
>        interrupt-controller;
>        reg = <0x3000 0x20>;
> +
> +      interrupt-parent = <&cpuintc>;
> +      interrupts = <1>, <2>, <5>;
> +      realtek,output-valid-mask = <0x13>;

What additional information does this bring? From the description
above, this is all SW configurable, so why should this be described in
the DT?

> +
>        #address-cells = <0>;
>        interrupt-map =
> -              <31 &cpuintc 2>,
> -              <30 &cpuintc 1>,
> -              <29 &cpuintc 5>;
> +              <31 &cpuintc 2>, /* connect to cpuintc 2 via output 1 */
> +              <30 &cpuintc 1>, /* connect to cpuintc 1 via output 0 */
> +              <29 &cpuintc 5>; /* connect to cpuintc 5 via output 4 */
>      };

My conclusion here is that, as I stated in my initial review of this
series, you could completely ignore the 3rd field of the map, and let
the driver decide on the mapping without any extra information.

We already have plenty of crossbar-type drivers in the tree that can
mux a number of input to a number of outputs and route them
accordingly to a set of parent interrupts. None of this requires to be
described in DT.

	M.

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

Re: [RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

[Birger Koblitz]

Hi Sander,
> I haven't tested this with VSMP, because it is out of scope for this series. For the binding, I expect that would only require N register ranges instead of one; one per CPU. I think the driver should then be able to perform the IRQ balancing based on that information alone, given that the parent IRQs are available at each CPU.

whether this is out of the scope of this series is not the point. In my experience you only see issues with locking and race conditions with the IRQ driver if you test with VSMP enabled,
because only with VSMP you can be in the IRQ code multiple times at the same time. Since you want to change routing logic and hierarchies I would believe it to be a very good idea
to test that. The present code passes that test.

Cheers,
   Birger

Re: [RFC PATCH v2 2/5] irqchip/realtek-rtl: fix off-by-one in routing

[Sander Vanheule]

Hi Marc,

On Mon, 2021-12-27 at 10:16 +0000, Marc Zyngier wrote:
> On Sun, 26 Dec 2021 19:59:25 +0000,
> Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > There is an offset between routing values (1..6) and the connected MIPS
> > CPU interrupts (2..7), but no distinction was made between these two
> > values.
> > 
> > This issue was previously hidden during testing, because an interrupt
> > mapping was used where for each required interrupt another (unused)
> > routing was configured, with an offset of +1.
> 
> Where does this 'other routing' come from?

When I reported the interrupt-map issue with this binding/driver, I tried to reduce the
mapping/routing to something as small as possible, but I couldn't get away with anything
less than the following:

	interrupt-map =
		<31 &cpuintc 2>, /* UART0 */
		<20 &cpuintc 3>, /* SWCORE */
		<19 &cpuintc 4>, /* WDT IP1 */
		<18 &cpuintc 5>; /* WDT IP2 */

The UART0 and WDT_IP1 mappings were required. These would cause the driver to assign
chained handlers to <&cpuint 2> and <&cpuint 4>, and also write values "2" and "4" to the
respective routing registers.

The SWCORE mapping was not required for any configured features, but it was required to
get the console to work. This is because a routing register value of "2", actually causes
that interrupt input to be (electrically) cascaded into to <&cpuintc 3>. But <&cpuintc 3>
would only be assigned a chained handler because of the SWCORE mapping.

By then assigning the same handler to all parent interrupts, and not caring about which
parent interrupt caused the handler to be called, this ended up actually working.

> > 
> > Offset the CPU IRQ numbers by -1 to retrieve the correct routing value.
> > 
> > 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 | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
> > index d6788dd93c7b..568614edd88f 100644
> > --- a/drivers/irqchip/irq-realtek-rtl.c
> > +++ b/drivers/irqchip/irq-realtek-rtl.c
> > @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *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.
> > + * 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)
> >  {
> > @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct
> > irq_domain *do
> >                 of_node_put(cpu_ictl);
> >  
> >                 cpu_int = be32_to_cpup(imap + 2);
> > -               if (cpu_int > 7)
> > +               if (cpu_int > 7 || cpu_int < 2)
> 
> How many output lines do you have? The comment above says something
> about having 15 output lines, but you limit it to 7...

The SoCs we are using with this interrupt controller, connect their output lines to CPU
IRQ 2..7. If "interrupt-map" specifies parent HW IRQ numbers, the driver needs to verify
those numbers are valid. If they are, it can derive the routing register values from that.

On the other hand, routing register values have four bits. "0" appears to disconnect an
input interrupt, so that leaves 15 potential interrupt outputs (in theory, we don't have
actual hardware descriptions). Only 6 outputs are used here, but that could be an
implementation detail for these SoCs, rather than a limitation of the interrupt router.

> >                         return -EINVAL;
> >  
> >                 if (!(mips_irqs_set & BIT(cpu_int))) {
> > @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct
> > irq_domain *do
> >                         mips_irqs_set |= BIT(cpu_int);
> >                 }
> >  
> > -               regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
> > +               /* Use routing values (1..6) for CPU interrupts (2..7) */
> > +               regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
> >                 imap += 3;
> >         }
> >  
> 
> What I don't understand is how this worked so far if all mappings were
> off my one. Or the mapping really doesn't matter, because this is all
> under SW control?

The reason this worked, was due to a number of issues compensating for each other, like I
tried to explain above.

The mapping is indeed arbitrary, and not designed into the hardware. So it could (should?)
just be put in the driver, instead of the devicetree.

Best,
Sander

Re: [RFC PATCH v2 2/5] irqchip/realtek-rtl: fix off-by-one in routing

[Marc Zyngier]

On Tue, 28 Dec 2021 10:13:26 +0000,
Sander Vanheule <sander@svanheule.net> wrote:
> 
> Hi Marc,
> 
> On Mon, 2021-12-27 at 10:16 +0000, Marc Zyngier wrote:
> > On Sun, 26 Dec 2021 19:59:25 +0000,
> > Sander Vanheule <sander@svanheule.net> wrote:
> > > 
> > > There is an offset between routing values (1..6) and the connected MIPS
> > > CPU interrupts (2..7), but no distinction was made between these two
> > > values.
> > > 
> > > This issue was previously hidden during testing, because an interrupt
> > > mapping was used where for each required interrupt another (unused)
> > > routing was configured, with an offset of +1.
> > 
> > Where does this 'other routing' come from?
> 
> When I reported the interrupt-map issue with this binding/driver, I
> tried to reduce the mapping/routing to something as small as
> possible, but I couldn't get away with anything less than the
> following:
> 
> 	interrupt-map =
> 		<31 &cpuintc 2>, /* UART0 */
> 		<20 &cpuintc 3>, /* SWCORE */
> 		<19 &cpuintc 4>, /* WDT IP1 */
> 		<18 &cpuintc 5>; /* WDT IP2 */
> 
> The UART0 and WDT_IP1 mappings were required. These would cause the
> driver to assign chained handlers to <&cpuint 2> and <&cpuint 4>,
> and also write values "2" and "4" to the respective routing
> registers.
> 
> The SWCORE mapping was not required for any configured features, but
> it was required to get the console to work. This is because a
> routing register value of "2", actually causes that interrupt input
> to be (electrically) cascaded into to <&cpuintc 3>. But <&cpuintc 3>
> would only be assigned a chained handler because of the SWCORE
> mapping.
> 
> By then assigning the same handler to all parent interrupts, and not
> caring about which parent interrupt caused the handler to be called,
> this ended up actually working.

Urgh... Pure luck, then.

> 
> > > 
> > > Offset the CPU IRQ numbers by -1 to retrieve the correct routing value.
> > > 
> > > 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 | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
> > > index d6788dd93c7b..568614edd88f 100644
> > > --- a/drivers/irqchip/irq-realtek-rtl.c
> > > +++ b/drivers/irqchip/irq-realtek-rtl.c
> > > @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *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.
> > > + * 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)
> > >  {
> > > @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node, struct
> > > irq_domain *do
> > >                 of_node_put(cpu_ictl);
> > >  
> > >                 cpu_int = be32_to_cpup(imap + 2);
> > > -               if (cpu_int > 7)
> > > +               if (cpu_int > 7 || cpu_int < 2)
> > 
> > How many output lines do you have? The comment above says something
> > about having 15 output lines, but you limit it to 7...
> 
> The SoCs we are using with this interrupt controller, connect their
> output lines to CPU IRQ 2..7. If "interrupt-map" specifies parent HW
> IRQ numbers, the driver needs to verify those numbers are valid. If
> they are, it can derive the routing register values from that.
> 
> On the other hand, routing register values have four bits. "0"
> appears to disconnect an input interrupt, so that leaves 15
> potential interrupt outputs (in theory, we don't have actual
> hardware descriptions). Only 6 outputs are used here, but that could
> be an implementation detail for these SoCs, rather than a limitation
> of the interrupt router.

It would be good to find out if there are more CPU interrupts that can
be targeted. My impression is that this is a copy/paste effect from
the original BSP, and that nobody actually checked. But maybe that's
the wrong impression.

> 
> > >                         return -EINVAL;
> > >  
> > >                 if (!(mips_irqs_set & BIT(cpu_int))) {
> > > @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node, struct
> > > irq_domain *do
> > >                         mips_irqs_set |= BIT(cpu_int);
> > >                 }
> > >  
> > > -               regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
> > > +               /* Use routing values (1..6) for CPU interrupts (2..7) */
> > > +               regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
> > >                 imap += 3;
> > >         }
> > >  
> > 
> > What I don't understand is how this worked so far if all mappings were
> > off my one. Or the mapping really doesn't matter, because this is all
> > under SW control?
> 
> The reason this worked, was due to a number of issues compensating
> for each other, like I tried to explain above.
> 
> The mapping is indeed arbitrary, and not designed into the
> hardware. So it could (should?)  just be put in the driver, instead
> of the devicetree.

Indeed. Which is what I was advocating for the first place. What I
understand from the HW is that it is able to freely route any input to
any output (muxing them as required), and to map each output to any
CPU interrupt line.

If my understanding is correct, the only thing you need from the DT is
a description of which input an endpoint is targeting (the interrupt
specifier), and a list of CPU interrupt lines. Everything else can be
decided at runtime.

Anyway, I'll probably end-up queuing the first two patches for 5.17,
and a Cc: to stable. The rest we can discuss ad-nauseam.

	M.

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

Re: [RFC PATCH v2 2/5] irqchip/realtek-rtl: fix off-by-one in routing

[Sander Vanheule]

On Tue, 2021-12-28 at 10:59 +0000, Marc Zyngier wrote:
> On Tue, 28 Dec 2021 10:13:26 +0000,
> Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > Hi Marc,
> > 
> > On Mon, 2021-12-27 at 10:16 +0000, Marc Zyngier wrote:
> > > On Sun, 26 Dec 2021 19:59:25 +0000,
> > > Sander Vanheule <sander@svanheule.net> wrote:
> > > > 
> > > > There is an offset between routing values (1..6) and the connected MIPS
> > > > CPU interrupts (2..7), but no distinction was made between these two
> > > > values.
> > > > 
> > > > This issue was previously hidden during testing, because an interrupt
> > > > mapping was used where for each required interrupt another (unused)
> > > > routing was configured, with an offset of +1.
> > > 
> > > Where does this 'other routing' come from?
> > 
> > When I reported the interrupt-map issue with this binding/driver, I
> > tried to reduce the mapping/routing to something as small as
> > possible, but I couldn't get away with anything less than the
> > following:
> > 
> >         interrupt-map =
> >                 <31 &cpuintc 2>, /* UART0 */
> >                 <20 &cpuintc 3>, /* SWCORE */
> >                 <19 &cpuintc 4>, /* WDT IP1 */
> >                 <18 &cpuintc 5>; /* WDT IP2 */
> > 
> > The UART0 and WDT_IP1 mappings were required. These would cause the
> > driver to assign chained handlers to <&cpuint 2> and <&cpuint 4>,
> > and also write values "2" and "4" to the respective routing
> > registers.
> > 
> > The SWCORE mapping was not required for any configured features, but
> > it was required to get the console to work. This is because a
> > routing register value of "2", actually causes that interrupt input
> > to be (electrically) cascaded into to <&cpuintc 3>. But <&cpuintc 3>
> > would only be assigned a chained handler because of the SWCORE
> > mapping.
> > 
> > By then assigning the same handler to all parent interrupts, and not
> > caring about which parent interrupt caused the handler to be called,
> > this ended up actually working.
> 
> Urgh... Pure luck, then.
> 
> > 
> > > > 
> > > > Offset the CPU IRQ numbers by -1 to retrieve the correct routing value.
> > > > 
> > > > 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 | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/irqchip/irq-realtek-rtl.c b/drivers/irqchip/irq-realtek-rtl.c
> > > > index d6788dd93c7b..568614edd88f 100644
> > > > --- a/drivers/irqchip/irq-realtek-rtl.c
> > > > +++ b/drivers/irqchip/irq-realtek-rtl.c
> > > > @@ -95,7 +95,8 @@ static void realtek_irq_dispatch(struct irq_desc *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.
> > > > + * 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)
> > > >  {
> > > > @@ -134,7 +135,7 @@ static int __init map_interrupts(struct device_node *node,
> > > > struct
> > > > irq_domain *do
> > > >                 of_node_put(cpu_ictl);
> > > >  
> > > >                 cpu_int = be32_to_cpup(imap + 2);
> > > > -               if (cpu_int > 7)
> > > > +               if (cpu_int > 7 || cpu_int < 2)
> > > 
> > > How many output lines do you have? The comment above says something
> > > about having 15 output lines, but you limit it to 7...
> > 
> > The SoCs we are using with this interrupt controller, connect their
> > output lines to CPU IRQ 2..7. If "interrupt-map" specifies parent HW
> > IRQ numbers, the driver needs to verify those numbers are valid. If
> > they are, it can derive the routing register values from that.
> > 
> > On the other hand, routing register values have four bits. "0"
> > appears to disconnect an input interrupt, so that leaves 15
> > potential interrupt outputs (in theory, we don't have actual
> > hardware descriptions). Only 6 outputs are used here, but that could
> > be an implementation detail for these SoCs, rather than a limitation
> > of the interrupt router.
> 
> It would be good to find out if there are more CPU interrupts that can
> be targeted. My impression is that this is a copy/paste effect from
> the original BSP, and that nobody actually checked. But maybe that's
> the wrong impression.

The BSP doesn't seems to refer to anything but the 6 CPU HW IRQs, so it appears to be a
standard mti,cpu-interrupt-controller. Those only have 6 HW (2-7) and 2 SW (0-1) IRQs.

> > 
> > > >                         return -EINVAL;
> > > >  
> > > >                 if (!(mips_irqs_set & BIT(cpu_int))) {
> > > > @@ -143,7 +144,8 @@ static int __init map_interrupts(struct device_node *node,
> > > > struct
> > > > irq_domain *do
> > > >                         mips_irqs_set |= BIT(cpu_int);
> > > >                 }
> > > >  
> > > > -               regs[(soc_int * 4) / 32] |= cpu_int << (soc_int * 4) % 32;
> > > > +               /* Use routing values (1..6) for CPU interrupts (2..7) */
> > > > +               regs[(soc_int * 4) / 32] |= (cpu_int - 1) << (soc_int * 4) % 32;
> > > >                 imap += 3;
> > > >         }
> > > >  
> > > 
> > > What I don't understand is how this worked so far if all mappings were
> > > off my one. Or the mapping really doesn't matter, because this is all
> > > under SW control?
> > 
> > The reason this worked, was due to a number of issues compensating
> > for each other, like I tried to explain above.
> > 
> > The mapping is indeed arbitrary, and not designed into the
> > hardware. So it could (should?)  just be put in the driver, instead
> > of the devicetree.
> 
> Indeed. Which is what I was advocating for the first place. What I
> understand from the HW is that it is able to freely route any input to
> any output (muxing them as required), and to map each output to any
> CPU interrupt line.

Indeed input-to-output is runtime configurable, but output-to-parent is hard-wired AFAICT.
If the output-to-parent mapping is known, an input-to-parent mapping can be used (i.e.
"interrupt-map") to select the outputs.

> If my understanding is correct, the only thing you need from the DT is
> a description of which input an endpoint is targeting (the interrupt
> specifier), and a list of CPU interrupt lines. Everything else can be
> decided at runtime.

I think this hardware is similar to 'fsl,imx-intmux' (irq-imx-intmux.c). See also my
comments on the bindings patch.


> Anyway, I'll probably end-up queuing the first two patches for 5.17,
> and a Cc: to stable. The rest we can discuss ad-nauseam.

Would you like me to submit the first two separately or will you take them as they are
from this series?

Best,
Sander

Re: [RFC PATCH v2 4/5] dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines

[Sander Vanheule]

On Mon, 2021-12-27 at 11:17 +0000, Marc Zyngier wrote:
> On Sun, 26 Dec 2021 19:59:27 +0000,
> Sander Vanheule <sander@svanheule.net> wrote:
> > 
> > Amend the binding to also require a list of parent interrupts, and an
> > optional mask to specify which parent is mapped to which output.
> > 
> > Without this information, any driver would have to make an assumption on
> > which parent interrupt is connected to which output.
> 
> Why should an endpoint driver care at all?

Interrupt inputs to interrupt outputs are SW configurable, but outputs to parent
interrupts are hard-wired and cannot be modified. "interrupt-map" defines an input to
parent interrupt mapping, so it seems a piece of information is missing. This is currently
provided as an assumption in the driver ("CPU IRQs (2..7) are connected to outputs
(1..6)").

Input-to-output is SW configurable, so that can be put in the driver. Output-to-parent is
hardware configuration, 


> > 
> > Additionally, extend (or add) the relevant descriptions to more clearly
> > describe the inputs and outputs of this router.
> > 
> > Signed-off-by: Sander Vanheule <sander@svanheule.net>
> > ---
> > Since it does not properly describe the hardware, I would still really
> > rather get rid of "interrupt-map", even though that would mean breaking
> > ABI for this binding. As we've argued before [1], that is our prefered
> > solution, and would enable us to not carry more (hacky) code because of
> > a mistake with the initial submission.
> 
> Again, this is too late. Broken bindings live forever.
> 
> > 
> > Vendors don't ship independent DT blobs for devices with this hardware,
> > so the independent devicetree/kernel upgrades issue is really rather
> > theoretical here. Realtek isn't driving the development of the bindings
> > and associated drivers for this platform. They have their SDK and seem
> > to care very little about proper kernel integration.
> 
> Any vendor can do whatever they want. You can do the same thing if you
> really want to.
> 
> > 
> > Furthermore, there are currently no device descriptions in the kernel
> > using this binding. There are in OpenWrt, but OpenWrt firmware images
> > for this platform always contain both the kernel and the appended DTB,
> > so there's also no breakage to worry about.
> 
> That's just one use case. Who knows who is using this stuff in a
> different context? Nobody can tell.
> 
> > 
> > [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/
> > 
> > Differences with v1:
> > - Don't drop the "interrupt-map" property
> > - Add the "realtek,output-valid-mask" property
> > ---
> >  .../realtek,rtl-intc.yaml                     | 38 ++++++++++++++++---
> >  1 file changed, 33 insertions(+), 5 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..29014673c34e 100644
> > --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
> > @@ -6,6 +6,10 @@ $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.
> > +
> >  maintainers:
> >    - Birger Koblitz <mail@birger-koblitz.de>
> >    - Bert Vermeulen <bert@biot.com>
> > @@ -22,7 +26,11 @@ properties:
> >      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.
> 
> Is that to support multiple SoCs? I'd expect a given SoC to have a
> fixed number of output interrupts.

It is, and they do AFAICT. But all values from 1 to 15 can be written to the routing
registers, so I wanted this definition to be as broad as possible.

The SoCs I'm working with only connect to the six CPU HW interrupts, but I don't know what
the actual limit of this interrupt hardware is, or if the outputs always connect to the
MIPS CPU HW interrupts.

> >  
> >    interrupt-controller: true
> >  
> > @@ -30,7 +38,21 @@ properties:
> >      const: 0
> >  
> >    interrupt-map:
> > -    description: Describes mapping from SoC interrupts to CPU interrupts
> > +    description:
> > +      List of <soc_int parent_phandle parent_args ...> tuples, where "soc_int"
> > +      is the interrupt input line number as provided by this controller.
> > +      "parent_phandle" and "parent_args" specify which parent interrupt this
> > +      line should be routed to. Note that interrupt specifiers should be
> > +      identical to the parents specified in the "interrupts" property.
> > +
> > +  realtek,output-valid-mask:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Optional bit mask indicating which outputs are connected to the parent
> > +      interrupts. The lowest set bit indicates which output line the first
> > +      interrupt from "interrupts" is connected to, the second lowest set bit
> > +      for the second interrupt, etc. If not provided, parent interrupts will be
> > +      assigned sequentially to the outputs.
> >  
> >  required:
> >    - compatible
> > @@ -39,6 +61,7 @@ required:
> >    - interrupt-controller
> >    - "#address-cells"
> >    - interrupt-map
> > +  - interrupts
> > 
> >  additionalProperties: false
> >  
> > @@ -49,9 +72,14 @@ examples:
> >        #interrupt-cells = <1>;
> >        interrupt-controller;
> >        reg = <0x3000 0x20>;
> > +
> > +      interrupt-parent = <&cpuintc>;
> > +      interrupts = <1>, <2>, <5>;
> > +      realtek,output-valid-mask = <0x13>;
> 
> What additional information does this bring? From the description
> above, this is all SW configurable, so why should this be described in
> the DT?

The hardware I currently have, has a single contiguous range of outputs hard-wired to
parent interrupts, starting at the first output.

I wanted to provide maximum flexibility for output connections, which I think should
support sparse output connections. However, since this would be an optional property, and
is currently not needed for any hardware, I suppose it could be added later when needed.

Adding "interrupts" as a required property is also a no-go, I assume, since that would
invalidate currently-valid DT-s.

> > +
> >        #address-cells = <0>;
> >        interrupt-map =
> > -              <31 &cpuintc 2>,
> > -              <30 &cpuintc 1>,
> > -              <29 &cpuintc 5>;
> > +              <31 &cpuintc 2>, /* connect to cpuintc 2 via output 1 */
> > +              <30 &cpuintc 1>, /* connect to cpuintc 1 via output 0 */
> > +              <29 &cpuintc 5>; /* connect to cpuintc 5 via output 4 */
> >      };
> 
> My conclusion here is that, as I stated in my initial review of this
> series, you could completely ignore the 3rd field of the map, and let
> the driver decide on the mapping without any extra information.
> 
> We already have plenty of crossbar-type drivers in the tree that can
> mux a number of input to a number of outputs and route them
> accordingly to a set of parent interrupts. None of this requires to be
> described in DT.

I had another look and "fsl,imx-intmux" looks like what I had in mind.

If I understand correctly, changing "#interrupt-cells" (to add the routing info) and the
required properties (to add "interrupts"), would require a new set of compatibles, in
addition to some fall-back behaviour if only the original compatible is provided.

Let me give this another spin, see what I can come up with.

Best,
Sander

Re: [RFC PATCH v2 4/5] dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines

[Birger Koblitz]

On 28/12/2021 17:21, Sander Vanheule wrote:
> On Mon, 2021-12-27 at 11:17 +0000, Marc Zyngier wrote:
>> On Sun, 26 Dec 2021 19:59:27 +0000,
>> Sander Vanheule <sander@svanheule.net> wrote:
>>>
>>> Amend the binding to also require a list of parent interrupts, and an
>>> optional mask to specify which parent is mapped to which output.
>>>
>>> Without this information, any driver would have to make an assumption on
>>> which parent interrupt is connected to which output.
>>
>> Why should an endpoint driver care at all?
> 
> Interrupt inputs to interrupt outputs are SW configurable, but outputs to parent
> interrupts are hard-wired and cannot be modified. "interrupt-map" defines an input to
> parent interrupt mapping, so it seems a piece of information is missing. This is currently
> provided as an assumption in the driver ("CPU IRQs (2..7) are connected to outputs
> (1..6)").
> 
> Input-to-output is SW configurable, so that can be put in the driver. Output-to-parent is
> hardware configuration,
> 
> 
>>>
>>> Additionally, extend (or add) the relevant descriptions to more clearly
>>> describe the inputs and outputs of this router.
>>>
>>> Signed-off-by: Sander Vanheule <sander@svanheule.net>
>>> ---
>>> Since it does not properly describe the hardware, I would still really
>>> rather get rid of "interrupt-map", even though that would mean breaking
>>> ABI for this binding. As we've argued before [1], that is our prefered
>>> solution, and would enable us to not carry more (hacky) code because of
>>> a mistake with the initial submission.
>>
>> Again, this is too late. Broken bindings live forever.
>>
>>>
>>> Vendors don't ship independent DT blobs for devices with this hardware,
>>> so the independent devicetree/kernel upgrades issue is really rather
>>> theoretical here. Realtek isn't driving the development of the bindings
>>> and associated drivers for this platform. They have their SDK and seem
>>> to care very little about proper kernel integration.
>>
>> Any vendor can do whatever they want. You can do the same thing if you
>> really want to.
>>
>>>
>>> Furthermore, there are currently no device descriptions in the kernel
>>> using this binding. There are in OpenWrt, but OpenWrt firmware images
>>> for this platform always contain both the kernel and the appended DTB,
>>> so there's also no breakage to worry about.
>>
>> That's just one use case. Who knows who is using this stuff in a
>> different context? Nobody can tell.
>>
>>>
>>> [1] https://lore.kernel.org/all/9c169aad-3c7b-2ffb-90a2-1ca791a3f411@phrozen.org/
>>>
>>> Differences with v1:
>>> - Don't drop the "interrupt-map" property
>>> - Add the "realtek,output-valid-mask" property
>>> ---
>>>   .../realtek,rtl-intc.yaml                     | 38 ++++++++++++++++---
>>>   1 file changed, 33 insertions(+), 5 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..29014673c34e 100644
>>> --- a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtl-intc.yaml
>>> @@ -6,6 +6,10 @@ $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.
>>> +
>>>   maintainers:
>>>     - Birger Koblitz <mail@birger-koblitz.de>
>>>     - Bert Vermeulen <bert@biot.com>
>>> @@ -22,7 +26,11 @@ properties:
>>>       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.
>>
>> Is that to support multiple SoCs? I'd expect a given SoC to have a
>> fixed number of output interrupts.
> 
> It is, and they do AFAICT. But all values from 1 to 15 can be written to the routing
> registers, so I wanted this definition to be as broad as possible.
> 
> The SoCs I'm working with only connect to the six CPU HW interrupts, but I don't know what
> the actual limit of this interrupt hardware is, or if the outputs always connect to the
> MIPS CPU HW interrupts.
> 
 From what I know, the IRQ controller is used solely by Realtek in the RTL838x, RTL839x and
RTL930x SoC families, all of them MIPS 4KEc or 34Kc with the standard 7 CPU IRQ lines.
In their final RTL931x series they abandoned their custom IRQ controller and went for
an InterAptiv core with a standard MIPS GIC.

Multiple SoCs are supported by these SoCs via direct register access for configuration either
via SPI or I2C, for which the SoCs have HW support as a slave and master (only RTL93xx).
Optionally, a GPIO pin can be used for raising an IRQ between SoCs. Fast control of the switch
hardware in the SoCs is done by proprietary Ethernet frames. At least for OpenWRT, only the original
7 IRQ outputs are used. Adding support for 15 does not seem to hurt, though and if necessary
the driver can do additional error checking.

Cheers,
   Birger

Re: [RFC PATCH v2 4/5] dt-bindings: interrupt-controller: realtek,rtl-intc: map output lines

[Sander Vanheule]

Hi Birger,

On Tue, 2021-12-28 at 17:53 +0100, Birger Koblitz wrote:
> On 28/12/2021 17:21, Sander Vanheule wrote:
> > On Mon, 2021-12-27 at 11:17 +0000, Marc Zyngier wrote:
> > > On Sun, 26 Dec 2021 19:59:27 +0000,
> > > Sander Vanheule <sander@svanheule.net> wrote:  
> > > >     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.
> > > 
> > > Is that to support multiple SoCs? I'd expect a given SoC to have a
> > > fixed number of output interrupts.
> > 
> > It is, and they do AFAICT. But all values from 1 to 15 can be written to the routing
> > registers, so I wanted this definition to be as broad as possible.
> > 
> > The SoCs I'm working with only connect to the six CPU HW interrupts, but I don't know
> > what
> > the actual limit of this interrupt hardware is, or if the outputs always connect to
> > the
> > MIPS CPU HW interrupts.
> > 
>  From what I know, the IRQ controller is used solely by Realtek in the RTL838x, RTL839x
> and
> RTL930x SoC families, all of them MIPS 4KEc or 34Kc with the standard 7 CPU IRQ lines.
> In their final RTL931x series they abandoned their custom IRQ controller and went for
> an InterAptiv core with a standard MIPS GIC.

There is some code floating around [1] to support a few Wi-Fi SoCs (RTL8196E, RTL8197D,
and RTL8197F) which appear to use the same interrupt controller. Not that it's very likely
these will ever be supported property, because they contain Lexra MIPS cores.

That code claims these cores can have 16 CPU interrupts, althought the non-standard
interrupts are apparently not implemented in that driver. There is also mention of 64 SoC
interrupts, but it looks like this can be implemented by just instantiating this driver
once for each register range (given the code would be modified to get rid of some static
variables).

Anyway, a mostly theoretical problem. The SoCs we're targetting (RTL8380x, RTL839x, and
RTL930x) use the /6/ MIPS CPU HW interrupts (the two software interrupts are not used
AFAICT).

[1]
https://github.com/ggbruno/openwrt/blob/Realtek/target/linux/realtek/files-4.14/arch/mips/realtek/irq.c

Best,
Sander

Re: [RFC PATCH v2 0/5] Rework realtek-rtl IRQ driver

[Sander Vanheule]

Hi Birger,

On Tue, 2021-12-28 at 09:09 +0100, Birger Koblitz wrote:
> Hi Sander,
> > I haven't tested this with VSMP, because it is out of scope for this series. For the
> > binding, I expect that would only require N register ranges instead of one; one per
> > CPU. I think the driver should then be able to perform the IRQ balancing based on that
> > information alone, given that the parent IRQs are available at each CPU.
> 
> whether this is out of the scope of this series is not the point. In my experience you
> only see issues with locking and race conditions with the IRQ driver if you test with
> VSMP enabled,
> because only with VSMP you can be in the IRQ code multiple times at the same time. Since
> you want to change routing logic and hierarchies I would believe it to be a very good
> idea
> to test that. The present code passes that test.

Implementing CPU affinity is a separate issue for after these patches IMHO. The current
problems have to be fixed anyway. Otherwise you're just compounding (potential) issues,
only making further development harder.

FWIW, the driver with these (reworked) patches runs fine on my RTL839x (Zyxel GS1900-48)
with SMP enabled. That's without implementing CPU affinity support on this driver, so all
SoC interrupts just go to CPU0. If any issues with lock-ups show up later, we can fix them
when they appear.

Best,
Sander