Re: [PATCH v2 2/2] of: property: Add fw_devlink support for interrupts

[Marc Zyngier]

Hi Saravana,

On Mon, 15 Feb 2021 08:29:53 +0000,
Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
> 
> On Sun, Feb 14, 2021 at 7:58 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> >
> > On 2/14/21 1:12 PM, Saravana Kannan wrote:
> > [ ... ]
> > >
> > > Can you please give me the following details:
> > > * The DTS file for the board (not the SoC).
> >
> > The devicetree file extracted from the running system is attached.
> > Hope it helps.
> 
> Hi Guenter,
> 
> Thanks for the DTS file and logs. That helps a lot.
> 
> Looking at the attachment and this line from the earlier email:
> [   14.084606][   T11] pci 0005:01:00.0: probe deferral - wait for
> supplier interrupt-controller@0
> 
> It's clear the PCI node is waiting on:
>         interrupt-controller@0 {
>                 #address-cells = <0x00>;
>                 device_type = "PowerPC-Interrupt-Source-Controller";
>                 compatible = "ibm,opal-xive-vc\0IBM,opal-xics";
>                 #interrupt-cells = <0x02>;
>                 reg = <0x00 0x00 0x00 0x00>;
>                 phandle = <0x804b>;
>                 interrupt-controller;
>         };
> 
> If I grep for "ibm,opal-xive-vc", I see only one instance of it in the
> code. And that eventually ends up getting called like this:
> irq_find_matching_fwspec() -> xive_irq_domain_match() -> xive_native_match()
> 
> static bool xive_native_match(struct device_node *node)
> {
>         return of_device_is_compatible(node, "ibm,opal-xive-vc");
> }
> 
> However, when the IRQ domain are first registered, in xive_init_host()
> the "np" passed in is NOT the same node that xive_native_match() would
> match.
> static void __init xive_init_host(struct device_node *np)
> {
>         xive_irq_domain = irq_domain_add_nomap(np, XIVE_MAX_IRQ,
>                                                &xive_irq_domain_ops, NULL);
>         if (WARN_ON(xive_irq_domain == NULL))
>                 return;
>         irq_set_default_host(xive_irq_domain);
> }
> 
> Instead, the "np" here is:
>         interrupt-controller@6030203180000 {
>                 ibm,xive-provision-page-size = <0x10000>;
>                 ibm,xive-eq-sizes = <0x0c 0x10 0x15 0x18>;
>                 single-escalation-support;
>                 ibm,xive-provision-chips = <0x00>;
>                 ibm,xive-#priorities = <0x08>;
>                 compatible = "ibm,opal-xive-pe\0ibm,opal-intc";
>                 reg = <0x60302 0x3180000 0x00 0x10000 0x60302
> 0x3190000 0x00 0x10000 0x60302 0x31a0000 0x00 0x10000 0x60302
> 0x31b0000 0x00 0x10000>;
>                 phandle = <0x8051>;
>         };
> 
> There are many ways to fix this, but I first want to make sure this is
> a valid way to register irqdomains before trying to fix it. I just
> find it weird that the node that's registered is unrelated (not a
> parent/child) of the node that matches.
> 
> Marc,
> 
> Is this a valid way to register irqdomains? Just registering
> interrupt-controller@6030203180000 DT node where there are multiple
> interrupt controllers?

Absolutely.

The node is only one of the many possible ways to retrieve a
domain. In general, what you pass as the of_node/fwnode_handle can be
anything you want. It doesn't have to represent anything in the system
(we even create then ex-nihilo in some cases), and the match/select
callbacks are authoritative when they exist.

There is also the use of a default domain, which is used as a fallback
when no domain is found via the normal matching procedure.

PPC has established a way of dealing with domains long before ARM did,
closer to the board files of old than what we would do today (code
driven rather than data structure driven).

Strictly mapping domains onto HW blocks is a desirable property, but
that is all it is. That doesn't affect the very purpose of the IRQ
domains, which is to translate numbers from one context into another.

I'd be all for rationalising this, but it is pretty hard to introduce
semantic where there is none.

Thanks,

	M.

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