[PATCH v3] of/irq: Use interrupts-extended to find parent

[PATCH v3] of/irq: Use interrupts-extended to find parent

[Samuel Holland]

The RISC-V PLIC binding uses interrupts-extended to specify its parent
domain(s). That binding does not allow the interrupt-parent property to
appear in the irqchip node. This prevents of_irq_init from properly
detecting the irqchip hierarchy.

If no interrupt-parent property is present in the enclosing bus or root
node, then desc->interrupt_parent will be NULL for both the per-CPU
RISC-V INTC (the actual root domain) and the RISC-V PLIC. Similarly, if
the bus or root node specifies `interrupt-parent = <&plic>`, then
of_irq_init will hit the `desc->interrupt_parent == np` check, and again
all parents will be NULL. So things happen to work today for some boards
due to Makefile ordering.

However, things break when another irqchip ("foo") is stacked on top of
the PLIC. The bus or root node will have `interrupt-parent = <&foo>`,
since that is what all of the other peripherals need. When of_irq_init
runs, it will try to find the PLIC's parent domain. of_irq_find_parent
will fall back to using the interrupt-parent property of the PLIC's
parent node (i.e. the bus or root node), and of_irq_init will see "foo"
as the PLIC's parent domain. But this is wrong, because "foo" is
actually the PLIC's child domain!

So of_irq_init wrongly attempts to init the stacked irqchip before the
PLIC. This fails and breaks booting.

Fix this by using the first node referenced by interrupts-extended as
the parent when that property is present. This allows of_irq_init to see
the relationship between the PLIC and the per-CPU RISC-V INTC, and thus
only the RISC-V INTC is (correctly) considered a root domain.

Signed-off-by: Samuel Holland <samuel@sholland.org>
---

Changes in v3:
 - Move the check into of_irq_init. Do not touch of_irq_find_parent.

Changes in v2:
 - Add comments noting the assumptions made here

 drivers/of/irq.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index 2b07677a386b..aa485eb312ac 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -550,9 +550,18 @@ void __init of_irq_init(const struct of_device_id *matches)
 
 		desc->irq_init_cb = match->data;
 		desc->dev = of_node_get(np);
-		desc->interrupt_parent = of_irq_find_parent(np);
-		if (desc->interrupt_parent == np)
+		/*
+		 * interrupts-extended can reference multiple parent domains.
+		 * Arbitrarily pick the first one; assume any other parents
+		 * are the same distance away from the root irq controller.
+		 */
+		desc->interrupt_parent = of_parse_phandle(np, "interrupts-extended", 0);
+		if (!desc->interrupt_parent)
+			desc->interrupt_parent = of_irq_find_parent(np);
+		if (desc->interrupt_parent == np) {
+			of_node_put(desc->interrupt_parent);
 			desc->interrupt_parent = NULL;
+		}
 		list_add_tail(&desc->list, &intc_desc_list);
 	}
 
-- 
2.35.1

Re: [PATCH v3] of/irq: Use interrupts-extended to find parent

[Rob Herring]

On Tue, 12 Apr 2022 00:15:28 -0500, Samuel Holland wrote:
> The RISC-V PLIC binding uses interrupts-extended to specify its parent
> domain(s). That binding does not allow the interrupt-parent property to
> appear in the irqchip node. This prevents of_irq_init from properly
> detecting the irqchip hierarchy.
> 
> If no interrupt-parent property is present in the enclosing bus or root
> node, then desc->interrupt_parent will be NULL for both the per-CPU
> RISC-V INTC (the actual root domain) and the RISC-V PLIC. Similarly, if
> the bus or root node specifies `interrupt-parent = <&plic>`, then
> of_irq_init will hit the `desc->interrupt_parent == np` check, and again
> all parents will be NULL. So things happen to work today for some boards
> due to Makefile ordering.
> 
> However, things break when another irqchip ("foo") is stacked on top of
> the PLIC. The bus or root node will have `interrupt-parent = <&foo>`,
> since that is what all of the other peripherals need. When of_irq_init
> runs, it will try to find the PLIC's parent domain. of_irq_find_parent
> will fall back to using the interrupt-parent property of the PLIC's
> parent node (i.e. the bus or root node), and of_irq_init will see "foo"
> as the PLIC's parent domain. But this is wrong, because "foo" is
> actually the PLIC's child domain!
> 
> So of_irq_init wrongly attempts to init the stacked irqchip before the
> PLIC. This fails and breaks booting.
> 
> Fix this by using the first node referenced by interrupts-extended as
> the parent when that property is present. This allows of_irq_init to see
> the relationship between the PLIC and the per-CPU RISC-V INTC, and thus
> only the RISC-V INTC is (correctly) considered a root domain.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
> Changes in v3:
>  - Move the check into of_irq_init. Do not touch of_irq_find_parent.
> 
> Changes in v2:
>  - Add comments noting the assumptions made here
> 
>  drivers/of/irq.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 

Applied, thanks!