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

[Marc Zyngier <marc.zyngier@xxxxxxx>]

On 29/04/2019 10:09, Ard Biesheuvel wrote:
> On Sat, 27 Apr 2019 at 00:27, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>>
>> On Fri, 26 Apr 2019 at 13:45, Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> wrote:
>>>
>>> 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.
>>>
>>
>> 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...