diff mbox series

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

Message ID 20190425102020.21533-3-ard.biesheuvel@linaro.org
State New
Headers show
Series synquacer: implement ACPI gpio/interrupt support | expand

Commit Message

Ard Biesheuvel April 25, 2019, 10:20 a.m. UTC 
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@linaro.org>

---
 drivers/irqchip/irq-sni-exiu.c | 82 +++++++++++++++++---
 1 file changed, 73 insertions(+), 9 deletions(-)

-- 
2.20.1

Comments

Linus Walleij April 25, 2019, 1:14 p.m. UTC | #1 
Hi Ard!

Thanks for your patch!

As it involves ACPI I suggest to include Mika Westerberg on review since
he's doing the core gpiolib ACPI stuff.

On Thu, Apr 25, 2019 at 12:20 PM Ard Biesheuvel
<ard.biesheuvel@linaro.org> 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@linaro.org>


(...)

> +#include <linux/gpio/driver.h>


So now this is starting to go from being a pure irqchip driver
to a combined irq+gpio driver.

> +       gc->irq.domain = domain;

> +       gc->to_irq = exiu_acpi_gpio_to_irq;


And there is some gpiochip involved.

> +EXPORT_SYMBOL(exiu_acpi_init);


Including exporting functions.

What about we just move this driver over to drivers/gpio and
start working out a combined GPIO+irqchip driver there? I am
working on adding generic hierarchical irqchip helpers in the
gpiolib core and then it's gonna be nice to have all combined
drivers under the drivers/gpio folder.

Yours,
Linus Walleij
Marc Zyngier April 25, 2019, 3:33 p.m. UTC | #2 
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@linaro.org>

> ---

>  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?

I appreciate there is a dependency between the two, but that's something
we should be able to solve (probe deferral?).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
Ard Biesheuvel April 26, 2019, 8:24 a.m. UTC | #3 
On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> 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@linaro.org>

> > ---

> >  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.

> 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.
Marc Zyngier April 26, 2019, 8:44 a.m. UTC | #4 
On 26/04/2019 09:24, Ard Biesheuvel wrote:
> On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> 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@linaro.org>

>>> ---

>>>  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?

> 

>> 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...).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
Ard Biesheuvel April 26, 2019, 11:45 a.m. UTC | #5 
On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:
>

> On 26/04/2019 09:24, Ard Biesheuvel wrote:

> > On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> 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@linaro.org>

> >>> ---

> >>>  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.
Ard Biesheuvel April 26, 2019, 10:27 p.m. UTC | #6 
On Fri, 26 Apr 2019 at 13:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:

> >

> > On 26/04/2019 09:24, Ard Biesheuvel wrote:

> > > On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> 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@linaro.org>

> > >>> ---

> > >>>  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.

>


Unfortunately, this doesn't work either. The GED device can respond to
GSIVs only, and so going via the EXIU doesn't work.

I have attempted to hack it up via AML, but the GED driver uses a
threaded interrupt, and so the interrupt is re-enabled at the GIC
before the AML handler is executed (which clears it in the EXIU)

For the RAS case, I guess we could let the firmware pick an arbitrary
unused SPI and signal that directly into the GIC, but for the power
button (which is physically wired to the EXIU), we have to model the
EXIU either has a separate pseudo-GPIO controller, or as part of the
real GPIO block.
Ard Biesheuvel April 29, 2019, 9:09 a.m. UTC | #7 
On Sat, 27 Apr 2019 at 00:27, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>

> On Fri, 26 Apr 2019 at 13:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> >

> > On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:

> > >

> > > On 26/04/2019 09:24, Ard Biesheuvel wrote:

> > > > On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> 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@linaro.org>

> > > >>> ---

> > > >>>  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.

> >

>

> Unfortunately, this doesn't work either. The GED device can respond to

> GSIVs only, and so going via the EXIU doesn't work.

>

> I have attempted to hack it up via AML, but the GED driver uses a

> threaded interrupt, and so the interrupt is re-enabled at the GIC

> before the AML handler is executed (which clears it in the EXIU)

>

> For the RAS case, I guess we could let the firmware pick an arbitrary

> unused SPI and signal that directly into the GIC, but for the power

> button (which is physically wired to the EXIU), we have to model the

> EXIU either has a separate pseudo-GPIO controller, or as part of the

> real GPIO block.


(+ Mika)

I made some progress describing the EXIU and GPIO controllers as follow.

    Device (EXIU) {
      Name (_HID, "SCX0008")
      Name (_UID, Zero)
      Name (_CRS, ResourceTemplate () {
        Memory32Fixed (ReadWrite, SYNQUACER_EXIU_BASE, SYNQUACER_EXIU_SIZE)
        Interrupt (ResourceConsumer, Level, ActiveHigh, ExclusiveAndWake) {
          144, 145, 146, 147, 148, 149, 150, 151,
          152, 153, 154, 155, 156, 157, 158, 159,
          160, 161, 162, 163, 164, 165, 166, 167,
          168, 169, 170, 171, 172, 173, 174, 175,
        }
      })
      Name (_DSD, Package () {
        ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
          Package () { "socionext,spi-base", 112 },
        }
      })
    }

and

    Device (GPIO) {
      Name (_HID, "SCX0007")
      Name (_UID, Zero)
      Name (_CRS, ResourceTemplate () {
        Memory32Fixed (ReadWrite, SYNQUACER_GPIO_BASE, SYNQUACER_GPIO_SIZE)
        Interrupt (ResourceConsumer, Edge, ActiveLow,
ExclusiveAndWake, 0, "\\_SB.EXIU") {
          7,
        }
      })
      Name (_AEI, ResourceTemplate () {
        GpioInt (Edge, ActiveHigh, ExclusiveAndWake, PullDefault, 0,
"\\_SB.GPIO")
        {
          7
        }
      })
      Method (_E07) {
        Notify (\_SB.PWRB, 0x80)
      }
    }

This is actually a fairly accurate depiction of reality: the EXIU is a
separate unit, and only some of the GPIOs are routed through it.

In the GPIO controller's to_irq() callback, I return the Linux IRQ
number based on the platform IRQ resources claimed by the GPIO device,
and I end up with something like

 46:          0 ... 0      EXIU   7 Edge      ACPI:Event

which looks correct to me. debugfs tells me it is mapped as

handler:  handle_fasteoi_irq
device:   (null)
status:   0x00000001
istate:   0x00000020
            IRQS_ONESHOT
ddepth:   0
wdepth:   1
dstate:   0x03404201
            IRQ_TYPE_EDGE_RISING
            IRQD_ACTIVATED
            IRQD_IRQ_STARTED
            IRQD_SINGLE_TARGET
            IRQD_WAKEUP_STATE
node:     0
affinity: 0-23
effectiv: 0
domain:  \_SB_.EXIU
 hwirq:   0x7
 chip:    EXIU
  flags:   0x55
             IRQCHIP_SET_TYPE_MASKED
             IRQCHIP_MASK_ON_SUSPEND
             IRQCHIP_SKIP_SET_WAKE
             IRQCHIP_EOI_THREADED
 parent:
    domain:  irqchip@(____ptrval____)-1
     hwirq:   0x97
     chip:    GICv3
      flags:   0x15
                 IRQCHIP_SET_TYPE_MASKED
                 IRQCHIP_MASK_ON_SUSPEND
                 IRQCHIP_SKIP_SET_WAKE

The EXIU domain is described as

name:   \_SB_.EXIU
 size:   32
 mapped: 1
 flags:  0x00000041
 parent: irqchip@(____ptrval____)-1
    name:   irqchip@(____ptrval____)-1
     size:   0
     mapped: 55
     flags:  0x00000041

Unfortunately, the system locks up entirely as soon as the interrupt
is triggered (I use a DIP switch in this case). So while the
description is accurate and the correct interrupt does get mapped,
something is still not working entirely as expected.

Any thoughts?

Thanks,
Ard.
Marc Zyngier April 29, 2019, 9:35 a.m. UTC | #8 
On 29/04/2019 10:09, Ard Biesheuvel wrote:
> On Sat, 27 Apr 2019 at 00:27, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>

>> On Fri, 26 Apr 2019 at 13:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>>>

>>> On Fri, 26 Apr 2019 at 10:44, Marc Zyngier <marc.zyngier@arm.com> wrote:

>>>>

>>>> On 26/04/2019 09:24, Ard Biesheuvel wrote:

>>>>> On Thu, 25 Apr 2019 at 17:33, Marc Zyngier <marc.zyngier@arm.com> 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@linaro.org>

>>>>>>> ---

>>>>>>>  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.

>>>

>>

>> Unfortunately, this doesn't work either. The GED device can respond to

>> GSIVs only, and so going via the EXIU doesn't work.

>>

>> I have attempted to hack it up via AML, but the GED driver uses a

>> threaded interrupt, and so the interrupt is re-enabled at the GIC

>> before the AML handler is executed (which clears it in the EXIU)

>>

>> For the RAS case, I guess we could let the firmware pick an arbitrary

>> unused SPI and signal that directly into the GIC, but for the power

>> button (which is physically wired to the EXIU), we have to model the

>> EXIU either has a separate pseudo-GPIO controller, or as part of the

>> real GPIO block.

> 

> (+ Mika)

> 

> I made some progress describing the EXIU and GPIO controllers as follow.

> 

>     Device (EXIU) {

>       Name (_HID, "SCX0008")

>       Name (_UID, Zero)

>       Name (_CRS, ResourceTemplate () {

>         Memory32Fixed (ReadWrite, SYNQUACER_EXIU_BASE, SYNQUACER_EXIU_SIZE)

>         Interrupt (ResourceConsumer, Level, ActiveHigh, ExclusiveAndWake) {

>           144, 145, 146, 147, 148, 149, 150, 151,

>           152, 153, 154, 155, 156, 157, 158, 159,

>           160, 161, 162, 163, 164, 165, 166, 167,

>           168, 169, 170, 171, 172, 173, 174, 175,

>         }

>       })

>       Name (_DSD, Package () {

>         ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),

>         Package () {

>           Package () { "socionext,spi-base", 112 },

>         }

>       })

>     }

> 

> and

> 

>     Device (GPIO) {

>       Name (_HID, "SCX0007")

>       Name (_UID, Zero)

>       Name (_CRS, ResourceTemplate () {

>         Memory32Fixed (ReadWrite, SYNQUACER_GPIO_BASE, SYNQUACER_GPIO_SIZE)

>         Interrupt (ResourceConsumer, Edge, ActiveLow,

> ExclusiveAndWake, 0, "\\_SB.EXIU") {

>           7,

>         }

>       })

>       Name (_AEI, ResourceTemplate () {

>         GpioInt (Edge, ActiveHigh, ExclusiveAndWake, PullDefault, 0,

> "\\_SB.GPIO")

>         {

>           7

>         }

>       })

>       Method (_E07) {

>         Notify (\_SB.PWRB, 0x80)

>       }

>     }

> 

> This is actually a fairly accurate depiction of reality: the EXIU is a

> separate unit, and only some of the GPIOs are routed through it.

> 

> In the GPIO controller's to_irq() callback, I return the Linux IRQ

> number based on the platform IRQ resources claimed by the GPIO device,

> and I end up with something like

> 

>  46:          0 ... 0      EXIU   7 Edge      ACPI:Event

> 

> which looks correct to me. debugfs tells me it is mapped as

> 

> handler:  handle_fasteoi_irq

> device:   (null)

> status:   0x00000001

> istate:   0x00000020

>             IRQS_ONESHOT

> ddepth:   0

> wdepth:   1

> dstate:   0x03404201

>             IRQ_TYPE_EDGE_RISING

>             IRQD_ACTIVATED

>             IRQD_IRQ_STARTED

>             IRQD_SINGLE_TARGET

>             IRQD_WAKEUP_STATE

> node:     0

> affinity: 0-23

> effectiv: 0

> domain:  \_SB_.EXIU

>  hwirq:   0x7

>  chip:    EXIU

>   flags:   0x55

>              IRQCHIP_SET_TYPE_MASKED

>              IRQCHIP_MASK_ON_SUSPEND

>              IRQCHIP_SKIP_SET_WAKE

>              IRQCHIP_EOI_THREADED

>  parent:

>     domain:  irqchip@(____ptrval____)-1

>      hwirq:   0x97

>      chip:    GICv3

>       flags:   0x15

>                  IRQCHIP_SET_TYPE_MASKED

>                  IRQCHIP_MASK_ON_SUSPEND

>                  IRQCHIP_SKIP_SET_WAKE

> 

> The EXIU domain is described as

> 

> name:   \_SB_.EXIU

>  size:   32

>  mapped: 1

>  flags:  0x00000041

>  parent: irqchip@(____ptrval____)-1

>     name:   irqchip@(____ptrval____)-1

>      size:   0

>      mapped: 55

>      flags:  0x00000041

> 

> Unfortunately, the system locks up entirely as soon as the interrupt

> is triggered (I use a DIP switch in this case). So while the

> description is accurate and the correct interrupt does get mapped,

> something is still not working entirely as expected.

> 

> Any thoughts?


It feels like an interrupt storm with an edge vs level misconfiguration.
Can you check that the IRQ_TYPE_EDGE_RISING is correctly propagated
across the hierarchy?

The EXIU block has:

Interrupt (ResourceConsumer, Level, ActiveHigh, ExclusiveAndWake) {

while the GPIO block has:

Interrupt (ResourceConsumer, Edge, ActiveLow, ExclusiveAndWake, 0,
"\\_SB.EXIU")

and the interrupt is configured for yet another trigger
(IRQ_TYPE_EDGE_RISING).

Hope this helps,

	M.
-- 
Jazz is not dead. It just smells funny...
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-sni-exiu.c b/drivers/irqchip/irq-sni-exiu.c
index 52ce662334d4..99351cf997d9 100644
--- a/drivers/irqchip/irq-sni-exiu.c
+++ b/drivers/irqchip/irq-sni-exiu.c
@@ -12,6 +12,7 @@ 
  * published by the Free Software Foundation.
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/irq.h>
@@ -20,6 +21,7 @@ 
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/platform_device.h>
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 
@@ -134,9 +136,13 @@  static int exiu_domain_translate(struct irq_domain *domain,
 
 		*hwirq = fwspec->param[1] - info->spi_base;
 		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
-		return 0;
+	} else {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+		*hwirq = fwspec->param[0];
+		*type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK;
 	}
-	return -EINVAL;
+	return 0;
 }
 
 static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
@@ -147,16 +153,23 @@  static int exiu_domain_alloc(struct irq_domain *dom, unsigned int virq,
 	struct exiu_irq_data *info = dom->host_data;
 	irq_hw_number_t hwirq;
 
-	if (fwspec->param_count != 3)
-		return -EINVAL;	/* Not GIC compliant */
-	if (fwspec->param[0] != GIC_SPI)
-		return -EINVAL;	/* No PPI should point to this domain */
-
+	parent_fwspec = *fwspec;
+	if (is_of_node(dom->parent->fwnode)) {
+		if (fwspec->param_count != 3)
+			return -EINVAL;	/* Not GIC compliant */
+		if (fwspec->param[0] != GIC_SPI)
+			return -EINVAL;	/* No PPI should point to this domain */
+
+		hwirq = fwspec->param[1] - info->spi_base;
+	} else if (is_fwnode_irqchip(dom->parent->fwnode)) {
+		hwirq = fwspec->param[0];
+		parent_fwspec.param[0] = hwirq + info->spi_base + 32;
+	} else {
+		return -EINVAL;
+	}
 	WARN_ON(nr_irqs != 1);
-	hwirq = fwspec->param[1] - info->spi_base;
 	irq_domain_set_hwirq_and_chip(dom, virq, hwirq, &exiu_irq_chip, info);
 
-	parent_fwspec = *fwspec;
 	parent_fwspec.fwnode = dom->parent->fwnode;
 	return irq_domain_alloc_irqs_parent(dom, virq, nr_irqs, &parent_fwspec);
 }
@@ -244,3 +257,54 @@  static int __init exiu_dt_init(struct device_node *node,
 	return 0;
 }
 IRQCHIP_DECLARE(exiu, "socionext,synquacer-exiu", exiu_dt_init);
+
+#ifdef CONFIG_ACPI
+static int exiu_acpi_gpio_to_irq(struct gpio_chip *gc, u32 gpio)
+{
+	struct irq_fwspec fwspec;
+
+	fwspec.fwnode		= gc->parent->fwnode;
+	fwspec.param_count	= 2;
+	fwspec.param[0]		= gpio;
+	fwspec.param[1]		= IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
+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