Re: [RFC PATCH 2/3] irqchip/exiu: implement ACPI gpiolib/irqchip support

[Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>]

On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
>
> On 26/04/2019 09:24, Ard Biesheuvel wrote:
> > On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> >>
> >> Hi Ard,
> >>
> >> On 25/04/2019 11:20, Ard Biesheuvel wrote:
> >>> Expose the existing EXIU hierarchical irqchip domain code to permit
> >>> the interrupt controller to be used as the irqchip component of a
> >>> GPIO controller on ACPI systems.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> >>> ---
> >>>  drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
> >>>  1 file changed, 73 insertions(+), 9 deletions(-)
> >>>
> >>
> >> [...]
> >>
> >>> +int exiu_acpi_init(struct platform_device *pdev, struct gpio_chip *gc)
> >>> +{
> >>> +     struct irq_domain *parent_domain = NULL, *domain;
> >>> +     struct resource *res;
> >>> +     int irq;
> >>> +
> >>> +     irq = platform_get_irq(pdev, 0);
> >>> +     if (irq > 0)
> >>> +             parent_domain = irq_get_irq_data(irq)->domain;
> >>> +
> >>> +     if (!parent_domain) {
> >>> +             dev_err(&pdev->dev, "unable to obtain parent domain\n");
> >>> +             return -ENODEV;
> >>> +     }
> >>> +
> >>> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >>> +     if (!res) {
> >>> +             dev_err(&pdev->dev, "failed to parse memory resource\n");
> >>> +             return -ENXIO;
> >>> +     }
> >>> +
> >>> +     domain = exiu_init(parent_domain, dev_fwnode(&pdev->dev), res);
> >>> +     if (IS_ERR(domain)) {
> >>> +             dev_err(&pdev->dev, "failed to create IRQ domain (%ld)\n",
> >>> +                     PTR_ERR(domain));
> >>> +             return PTR_ERR(domain);
> >>> +     }
> >>> +
> >>> +     gc->irq.domain = domain;
> >>> +     gc->to_irq = exiu_acpi_gpio_to_irq;
> >>> +
> >>> +     dev_info(&pdev->dev, "%d interrupts forwarded\n", NUM_IRQS);
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +EXPORT_SYMBOL(exiu_acpi_init);
> >>> +#endif
> >>>
> >>
> >> I must say I'm not overly keen on this function. Why can't this be
> >> probed as an ACPI device, creating the corresponding domain, and leaving
> >> to the GPIO driver the task of doing the GPIO stuff?
> >>
> >
> > Because ACPI only permits 'system' interrupts or GPIO interrupts, it
> > does not allow arbitrary device objects in the ACPI namespace to be
> > targeted as interrupt controllers.
>
> Hmmm. We already have at least one driver (irq-mbigen.c) that does
> exactly that. It uses interrupts from the GIC (it is notionally behind
> the ITS), and offers a bunch of interrupt lines itself. Why is it different?
>

You are right, it isn't. It turns out that there is another way to
signal ACPI events based on interrupts, and it involves the ACPI0013
GED device. I will try to model it that way instead.

> >
> >> I appreciate there is a dependency between the two, but that's something
> >> we should be able to solve (probe deferral?).
> >>
> >
> > Perhaps it would be better to just clone the irqchip part into the
> > GPIO driver? Everything else needs to be modified anyway, including
> > the domain alloc/translate methods.
> >
> > That still leaves the question how to deal with the first four signal
> > lines, which are not shared between the EXIU and the GPIO controller.
> > but personally, I'm happy to just categorize that as "don't do that"
> > territory, and just ignore it for the time being.
> >
>
> Maybe. But I'd really like to understand why the above doesn't work in
> your case (as you can tell, my ACPI-foo is pretty basic...).
>

As is mine. I am just trying to make sense of all of this so we can
report non-fatal RAS errors via ACPI events.