On Mon, 14 Feb 2022 05:13:17 +0000,
Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
>
> Some OF irqchips, such as the RISC-V PLIC, use interrupts-extended to
> specify their parent domain(s). That binding does not allow using the
> interrupt-parent property in the irqchip node, which prevents
> of_irq_init from properly detecting the irqchip hierarchy.
Is this because there is more than a single interrupt parent?
>
> 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 INTCs (the actual root domains) 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/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. But because
> of_irq_find_parent ignores interrupts-extended, it will fall back to
> using the interrupt-parent property of the PLIC's parent node (i.e. the
> bus or root node), and see "foo" as the PLIC's parent domain. But this
> is wrong, because "foo" is actually the PLIC's child domain!
Let me see if I parsed this correctly. You have:
int-parent int-extended
foo -----------> PLIC --------------> root-irqchip
Is that correct?
>
> So of_irq_init wrongly attempts to init the stacked irqchip before the
> PLIC. This fails and breaks boot.
>
> Fix this by having of_irq_find_parent return the first node referenced
> by interrupts-extended when that property is present. Even if the
> property references multiple different IRQ domains, this will still work
> reliably in of_irq_init as long as all referenced domains are the same
> distance away from some root domain (e.g. the RISC-V INTCs referenced by
> the PLIC's interrupts-extended are always all root domains).
I'm a bit worried that the distance assumption may not always hold.
Maybe it isn't something we need to deal with right now, but a comment
wouldn't hurt to make this assumption clear.
>
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---
>
> drivers/of/irq.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 2b07677a386b..0c20e22b91f5 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -60,7 +60,8 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> return NULL;
>
> do {
> - if (of_property_read_u32(child, "interrupt-parent", &parent)) {
> + if (of_property_read_u32(child, "interrupt-parent", &parent) &&
> + of_property_read_u32(child, "interrupts-extended", &parent)) {
> p = of_get_parent(child);
> } else {
> if (of_irq_workarounds & OF_IMAP_NO_PHANDLE)
With the comment added:
Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
M.
--
Without deviation from the norm, progress is not possible.
