On Tue, Feb 15, 2022 at 08:20:39PM -0600, Samuel Holland 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.
>
> 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!
>
> 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).
>
> Acked-by: Marc Zyngier <maz@xxxxxxxxxx>
> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
> ---
>
> Changes in v2:
> - Add comments noting the assumptions made here
>
> drivers/of/irq.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/irq.c b/drivers/of/irq.c
> index 2b07677a386b..c7d14f5c4660 100644
> --- a/drivers/of/irq.c
> +++ b/drivers/of/irq.c
> @@ -60,7 +60,12 @@ struct device_node *of_irq_find_parent(struct device_node *child)
> return NULL;
>
> do {
> - if (of_property_read_u32(child, "interrupt-parent", &parent)) {
> + /*
> + * interrupts-extended can reference multiple parent domains.
> + * This only returns the first one.
> + */
> + if (of_property_read_u32(child, "interrupt-parent", &parent) &&
> + of_property_read_u32(child, "interrupts-extended", &parent)) {
> p = of_get_parent(child);
of_irq_find_parent() fundamentally works with interrupt-parent.
'Finding' the parent just doesn't make sense for 'interrupts-extended'
because it is explicit. Other than the comment, what gets returned in
the case of 'interrupts-extended' is ambiguous.
Also, this will walk parent nodes to find 'interrupts-extended'. While
that's somewhat unlikely to occur, it is not what you want.
Instead, just check 'interrupts-extended' within of_irq_init() and
then fallback to calling of_irq_find_parent(). Then the ambiguous
nature of only looking at the 1st entry is in one place. (And more
easily fixed if we ever need all the parents.)
Rob
