[PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Wei Xu]

Invoke acpi_gpiochip_request_interrupts after the acpi data has been
attached to the pl061 acpi node to register interruption.

Otherwise it will be failed to register interruption for the ACPI case.
Because in the gpiochip_add_data_with_key, acpi_gpiochip_add is invoked
after gpiochip_add_irqchip but at that time the acpi data has not been
attached yet.

Tested with below steps:

	qemu-system-aarch64 \
	-machine virt,gic-version=3 -cpu cortex-a57 \
	-m 1G,maxmem=4G,slots=4 \
	-kernel Image -initrd rootfs.cpio.gz \
	-net none -nographic  \
	-bios QEMU_EFI.fd  \
	-append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"

The pl061 interruption is missed and the following output is not in the
/proc/interrupts on the v5.3-rc4 compared with the v5.2.0-rc7.

	 43:          0  ARMH0061:00   3 Edge      ACPI:Event

Fixes: 04ce935c6b2a ("gpio: pl061: Pass irqchip when adding gpiochip")
Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
---
v2 -> v3:
* addressed the comments of Andy to show only affected output of
/proc/interrupts and drop the whole log of v5.2.0-rc7

v1- > v2:
* rebased on https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=devel
* attached the log based on QEMU v3.0.0 and Linux kernel v5.2.0-rc7
---
 drivers/gpio/gpio-pl061.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpio/gpio-pl061.c b/drivers/gpio/gpio-pl061.c
index 722ce5c..e1a434e 100644
--- a/drivers/gpio/gpio-pl061.c
+++ b/drivers/gpio/gpio-pl061.c
@@ -8,6 +8,7 @@
  *
  * Data sheet: ARM DDI 0190B, September 2000
  */
+#include <linux/acpi.h>
 #include <linux/spinlock.h>
 #include <linux/errno.h>
 #include <linux/init.h>
@@ -24,6 +25,9 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/pm.h>
 
+#include "gpiolib.h"
+#include "gpiolib-acpi.h"
+
 #define GPIODIR 0x400
 #define GPIOIS  0x404
 #define GPIOIBE 0x408
@@ -345,6 +349,9 @@ static int pl061_probe(struct amba_device *adev, const struct amba_id *id)
 	if (ret)
 		return ret;
 
+	if (has_acpi_companion(dev))
+		acpi_gpiochip_request_interrupts(&pl061->gc);
+
 	amba_set_drvdata(adev, pl061);
 	dev_info(dev, "PL061 GPIO chip registered\n");
 
-- 
2.8.1

Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Andy Shevchenko]

On Mon, Aug 19, 2019 at 4:29 PM Wei Xu <xuwei5@hisilicon.com> wrote:
>
> Invoke acpi_gpiochip_request_interrupts after the acpi data has been
> attached to the pl061 acpi node to register interruption.
>
> Otherwise it will be failed to register interruption for the ACPI case.
> Because in the gpiochip_add_data_with_key, acpi_gpiochip_add is invoked
> after gpiochip_add_irqchip but at that time the acpi data has not been
> attached yet.
>
> Tested with below steps:
>
>         qemu-system-aarch64 \
>         -machine virt,gic-version=3 -cpu cortex-a57 \
>         -m 1G,maxmem=4G,slots=4 \
>         -kernel Image -initrd rootfs.cpio.gz \
>         -net none -nographic  \
>         -bios QEMU_EFI.fd  \
>         -append "console=ttyAMA0 acpi=force earlycon=pl011,0x9000000"
>
> The pl061 interruption is missed and the following output is not in the
> /proc/interrupts on the v5.3-rc4 compared with the v5.2.0-rc7.
>
>          43:          0  ARMH0061:00   3 Edge      ACPI:Event

The proper fix is to revert the culprit since we call
acpi_gpiochip_request_interrupts() for all controllers.
Linus, please re-do the approach with IRQ handling, it seems broadly
regress with ACPI enabled platforms.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Linus Walleij]

On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> The proper fix is to revert the culprit since we call
> acpi_gpiochip_request_interrupts() for all controllers.
> Linus, please re-do the approach with IRQ handling,

Exactly what do you refer to when you want me to
"re-do the approach for IRQ handling"? Do you mean
this driver or are you referring to:

commit e0d89728981393b7d694bd3419b7794b9882c92d
Author: Thierry Reding <treding@nvidia.com>
Date:   Tue Nov 7 19:15:54 2017 +0100

    gpio: Implement tighter IRQ chip integration

    Currently GPIO drivers are required to add the GPIO chip and its
    corresponding IRQ chip separately, which can result in a lot of
    boilerplate. Use the newly introduced struct gpio_irq_chip, embedded in
    struct gpio_chip, that drivers can fill in if they want the GPIO core
    to automatically register the IRQ chip associated with a GPIO chip.

    Signed-off-by: Thierry Reding <treding@nvidia.com>
    Acked-by: Grygorii Strashko <grygorii.strashko@ti.com>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

The new API introduced by this patch is what I am trying to switch
everything over to, because the forked paths inside of gpiolib
is causing me a maintenance headache and also increasing
the footprint of the library.

>  it seems broadly
> regress with ACPI enabled platforms.

It only becomes a problem if the platform uses ACPI right?
But it's a problem if I can't really tell if a driver is using
ACPI or not, there is no sign in the pl061 driver that it would
be used on ACPI systems until now, so how do I design
for it?

The problem comes from the problem/mess I am trying to
clean up in the first place. So if the new way of registering GPIO
irqchips is not working for ACPI, then we have to fix that instead
of reverting all attempts to use the new API IMO.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Linus Walleij]

On Mon, Aug 19, 2019 at 3:29 PM Wei Xu <xuwei5@hisilicon.com> wrote:

> Invoke acpi_gpiochip_request_interrupts after the acpi data has been
> attached to the pl061 acpi node to register interruption.
>
> Otherwise it will be failed to register interruption for the ACPI case.
> Because in the gpiochip_add_data_with_key, acpi_gpiochip_add is invoked
> after gpiochip_add_irqchip but at that time the acpi data has not been
> attached yet.

We need to fix this problem in gpiochip_add_data_with_key() instead.

Yours,
Linus Walleij

Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Andy Shevchenko]

On Tue, Aug 20, 2019 at 10:12 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
> > The proper fix is to revert the culprit since we call
> > acpi_gpiochip_request_interrupts() for all controllers.
> > Linus, please re-do the approach with IRQ handling,
>
> Exactly what do you refer to when you want me to
> "re-do the approach for IRQ handling"? Do you mean
> this driver or are you referring to:
>
> commit e0d89728981393b7d694bd3419b7794b9882c92d
> Author: Thierry Reding <treding@nvidia.com>
> Date:   Tue Nov 7 19:15:54 2017 +0100
>
>     gpio: Implement tighter IRQ chip integration
>
>     Currently GPIO drivers are required to add the GPIO chip and its
>     corresponding IRQ chip separately, which can result in a lot of
>     boilerplate. Use the newly introduced struct gpio_irq_chip, embedded in
>     struct gpio_chip, that drivers can fill in if they want the GPIO core
>     to automatically register the IRQ chip associated with a GPIO chip.
>
>     Signed-off-by: Thierry Reding <treding@nvidia.com>
>     Acked-by: Grygorii Strashko <grygorii.strashko@ti.com>
>     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> The new API introduced by this patch is what I am trying to switch
> everything over to, because the forked paths inside of gpiolib
> is causing me a maintenance headache and also increasing
> the footprint of the library.
>
> >  it seems broadly
> > regress with ACPI enabled platforms.
>
> It only becomes a problem if the platform uses ACPI right?
> But it's a problem if I can't really tell if a driver is using
> ACPI or not, there is no sign in the pl061 driver that it would
> be used on ACPI systems until now, so how do I design
> for it?
>
> The problem comes from the problem/mess I am trying to
> clean up in the first place. So if the new way of registering GPIO
> irqchips is not working for ACPI, then we have to fix that instead
> of reverting all attempts to use the new API IMO.
>
> Yours,
> Linus Walleij



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Andy Shevchenko]

On Tue, Aug 20, 2019 at 10:12 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
> > The proper fix is to revert the culprit since we call
> > acpi_gpiochip_request_interrupts() for all controllers.
> > Linus, please re-do the approach with IRQ handling,
>
> Exactly what do you refer to when you want me to
> "re-do the approach for IRQ handling"? Do you mean
> this driver or are you referring to:
>
> commit e0d89728981393b7d694bd3419b7794b9882c92d
> Author: Thierry Reding <treding@nvidia.com>
> Date:   Tue Nov 7 19:15:54 2017 +0100
>
>     gpio: Implement tighter IRQ chip integration
>
>     Currently GPIO drivers are required to add the GPIO chip and its
>     corresponding IRQ chip separately, which can result in a lot of
>     boilerplate. Use the newly introduced struct gpio_irq_chip, embedded in
>     struct gpio_chip, that drivers can fill in if they want the GPIO core
>     to automatically register the IRQ chip associated with a GPIO chip.
>
>     Signed-off-by: Thierry Reding <treding@nvidia.com>
>     Acked-by: Grygorii Strashko <grygorii.strashko@ti.com>
>     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Yes.

> The new API introduced by this patch is what I am trying to switch
> everything over to, because the forked paths inside of gpiolib
> is causing me a maintenance headache and also increasing
> the footprint of the library.

Yes, I understand.

> >  it seems broadly
> > regress with ACPI enabled platforms.
>
> It only becomes a problem if the platform uses ACPI right?

Unfortunately yes. Though in this case it was working and stopped working.

> But it's a problem if I can't really tell if a driver is using
> ACPI or not, there is no sign in the pl061 driver that it would
> be used on ACPI systems until now, so how do I design
> for it?

It's hidden under amba_driver_register() which works for all
registered thru drivers/acpi/acpi_amba.c.
I agree this is not straightforward.

> The problem comes from the problem/mess I am trying to
> clean up in the first place. So if the new way of registering GPIO
> irqchips is not working for ACPI, then we have to fix that instead
> of reverting all attempts to use the new API IMO.

Sorry for me being impatient and asking for a groundless requests.
I'll help you with cleaning this.

-- 
With Best Regards,
Andy Shevchenko

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Wei Xu]

Hi Linus,

On 2019/8/20 16:01, Linus Walleij wrote:
> On Mon, Aug 19, 2019 at 3:29 PM Wei Xu <xuwei5@hisilicon.com> wrote:
>
>> Invoke acpi_gpiochip_request_interrupts after the acpi data has been
>> attached to the pl061 acpi node to register interruption.
>>
>> Otherwise it will be failed to register interruption for the ACPI case.
>> Because in the gpiochip_add_data_with_key, acpi_gpiochip_add is invoked
>> after gpiochip_add_irqchip but at that time the acpi data has not been
>> attached yet.
> We need to fix this problem in gpiochip_add_data_with_key() instead.
Thanks!

Best Regards,
Wei

> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>

Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion

[Linus Walleij]

On Tue, Aug 20, 2019 at 10:51 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 20, 2019 at 10:12 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:

> > Exactly what do you refer to when you want me to
> > "re-do the approach for IRQ handling"? Do you mean
> > this driver or are you referring to:
> >
> > commit e0d89728981393b7d694bd3419b7794b9882c92d
> > Author: Thierry Reding <treding@nvidia.com>
> > Date:   Tue Nov 7 19:15:54 2017 +0100
> >
> >     gpio: Implement tighter IRQ chip integration
> >
> >     Currently GPIO drivers are required to add the GPIO chip and its
> >     corresponding IRQ chip separately, which can result in a lot of
> >     boilerplate. Use the newly introduced struct gpio_irq_chip, embedded in
> >     struct gpio_chip, that drivers can fill in if they want the GPIO core
> >     to automatically register the IRQ chip associated with a GPIO chip.
> >
> >     Signed-off-by: Thierry Reding <treding@nvidia.com>
> >     Acked-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Yes.

OK let's fix this mess, it shouldn't be too hard, I've sent a first
few patches.

> > The problem comes from the problem/mess I am trying to
> > clean up in the first place. So if the new way of registering GPIO
> > irqchips is not working for ACPI, then we have to fix that instead
> > of reverting all attempts to use the new API IMO.
>
> Sorry for me being impatient and asking for a groundless requests.
> I'll help you with cleaning this.

Sorry if I sounded harsh :( I just have a bit of panic.

I am sure we can fix this, I only recently realized what a headache
the new API is going to be if I can't straighten it out and have to
keep the old stuff around forever in parallel.

Yours,
Linus Walleij