On Fri, Aug 16, 2019 at 12:18 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Linus,
>
> On 12-08-19 15:53, Linus Walleij wrote:
> > We need to convert all old gpio irqchips to pass the irqchip
> > setup along when adding the gpio_chip. For more info see
> > drivers/gpio/TODO.
> >
> > For chained irqchips this is a pretty straight-forward
> > conversion.
> >
> > Cc: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
> > Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
> > Cc: Darren Hart <dvhart@xxxxxxxxxxxxx>
> > Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> > Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> > Andy please merge this into your platform tree when you
> > feel happy with the patch, would be great of someone
> > can test it on hardware as well.
>
> So I've just tested this on a Cherry Trail machine and
> the interrupt storm, fixing which is the reason the
> intel_int0002_vgpio.c driver was introduced, is back:
Hans, thanks for testing.
I postpone this one.
>
> [root@localhost ~]# cat /proc/interrupts | grep INT0002
> 9: 0 23429420 0 0 IO-APIC 9-fasteoi acpi, INT0002
>
> 23 million interrupts and counting, normally this is 0
> at boot low 10s after a suspend/resume with wakeup by
> USB keyboard.
>
> Notice that the driver has attached itself as shared irq-handler
> to the ACPI IRQ, but it seems something is going wrong with the
> registration of its own IRQ and/or for some reason the ACPI
> subsys is no longer attaching the ACPI event handler for the
> child IRQ to it, here is a the same command on a working
> kernel:
>
> [root@localhost ~]# cat /proc/interrupts | grep INT0002
> 9: 0 0 0 0 IO-APIC 9-fasteoi acpi, INT0002
> 123: 0 0 0 0 INT0002 Virtual GPIO 2 ACPI:Event
>
> Do I need any patches on top of 5.3-rc4 to test this patch?
>
> Note that it is important that the single irq on the chip is
> advertised as irq number 2 (so the third irq) because that
> is what the ACPI event code expects:
>
> Device (GPED)
> {
> Name (_ADR, Zero) // _ADR: Address
> Name (_HID, "INT0002" /* Virtual GPIO Controller */) // _HID: Hardw
> Name (_CID, "INT0002" /* Virtual GPIO Controller */) // _CID: Compa
> Name (_DDN, "Virtual GPIO controller") // _DDN: DOS Device Name
> ...
> Method (_AEI, 0, NotSerialized) // _AEI: ACPI Event Interrupts
> {
> Name (RBUF, ResourceTemplate ()
> {
> GpioInt (Level, ActiveHigh, ExclusiveAndWake, PullDown, 0x00
> "\\_SB.GPED", 0x00, ResourceConsumer, ,
> )
> { // Pin list
> 0x0002
> }
> })
> Return (RBUF) /* \_SB_.GPED._AEI.RBUF */
> }
>
> Method (_L02, 0, NotSerialized) // _Lxx: Level-Triggered GPE
> {
> ...
> }
> }
>
> Anyways, this will need to be fixed before we can merge this.
>
> Regards,
>
> Hans
>
>
>
>
> > ---
> > drivers/platform/x86/intel_int0002_vgpio.c | 29 +++++++++-------------
> > 1 file changed, 12 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
> > index d9542c661ddc..493a97ce0b08 100644
> > --- a/drivers/platform/x86/intel_int0002_vgpio.c
> > +++ b/drivers/platform/x86/intel_int0002_vgpio.c
> > @@ -156,8 +156,8 @@ static int int0002_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > const struct x86_cpu_id *cpu_id;
> > - struct irq_chip *irq_chip;
> > struct gpio_chip *chip;
> > + struct gpio_irq_chip *girq;
> > int irq, ret;
> >
> > /* Menlow has a different INT0002 device? <sigh> */
> > @@ -186,17 +186,9 @@ static int int0002_probe(struct platform_device *pdev)
> > chip->ngpio = GPE0A_PME_B0_VIRT_GPIO_PIN + 1;
> > chip->irq.need_valid_mask = true;
> >
> > - ret = devm_gpiochip_add_data(&pdev->dev, chip, NULL);
> > - if (ret) {
> > - dev_err(dev, "Error adding gpio chip: %d\n", ret);
> > - return ret;
> > - }
> > -
> > - bitmap_clear(chip->irq.valid_mask, 0, GPE0A_PME_B0_VIRT_GPIO_PIN);
> > -
> > /*
> > - * We manually request the irq here instead of passing a flow-handler
> > - * to gpiochip_set_chained_irqchip, because the irq is shared.
> > + * We directly request the irq here instead of passing a flow-handler
> > + * to the gpio irqchip, because the irq is shared.
> > */
> > ret = devm_request_irq(dev, irq, int0002_irq,
> > IRQF_SHARED, "INT0002", chip);
> > @@ -204,17 +196,20 @@ static int int0002_probe(struct platform_device *pdev)
> > dev_err(dev, "Error requesting IRQ %d: %d\n", irq, ret);
> > return ret;
> > }
> > + girq = &chip->irq;
> > + girq->chip = (struct irq_chip *)cpu_id->driver_data;
> > + girq->parent_handler = NULL;
> > + girq->num_parents = 0;
> > + girq->default_type = IRQ_TYPE_NONE;
> > + girq->handler = handle_edge_irq;
> >
> > - irq_chip = (struct irq_chip *)cpu_id->driver_data;
> > -
> > - ret = gpiochip_irqchip_add(chip, irq_chip, 0, handle_edge_irq,
> > - IRQ_TYPE_NONE);
> > + ret = devm_gpiochip_add_data(&pdev->dev, chip, NULL);
> > if (ret) {
> > - dev_err(dev, "Error adding irqchip: %d\n", ret);
> > + dev_err(dev, "Error adding gpio chip: %d\n", ret);
> > return ret;
> > }
> >
> > - gpiochip_set_chained_irqchip(chip, irq_chip, irq, NULL);
> > + bitmap_clear(chip->irq.valid_mask, 0, GPE0A_PME_B0_VIRT_GPIO_PIN);
> >
> > return 0;
> > }
> >
--
With Best Regards,
Andy Shevchenko
