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

[Saravana Kannan <saravanak@google.com>]

On Mon, Feb 15, 2021 at 1:09 AM Marc Zyngier <maz@kernel.org> wrote:
>
> Hi Saravana,
>
> On Mon, 15 Feb 2021 08:29:53 +0000,
> Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Sun, Feb 14, 2021 at 7:58 PM Guenter Roeck <linux@roeck-us.net> 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.

Ok, I'm going to disable parsing "interrupts" for PPC. It doesn't look
like any of the irq drivers are even remotely ready to be converted to
a proper device driver anyway.

And if this continues for other properties, I'll just disable
fw_devlink for PPC entirely.

-Saravana