Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption
@ 2019-08-12 11:28 Wei Xu
  2019-08-14  9:04 ` Linus Walleij
  2019-08-15 13:10 ` Andy Shevchenko
  0 siblings, 2 replies; 5+ messages in thread
From: Wei Xu @ 2019-08-12 11:28 UTC (permalink / raw)
  To: linux-gpio, linux-kernel, linux-arm-kernel, linus.walleij
  Cc: Linuxarm, xuwei5, Shameerali Kolothum Thodi, Jonathan Cameron,
	John Garry, Salil Mehta, Shiju Jose, jinying, Zhangyi ac,
	Liguozhu (Kenneth),
	Tangkunshan, huangdaode

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 on QEMU v4.1.0-rc3 and Linux kernel v5.3-rc4,
and found pl061 interruption is missed in the /proc/interrupts:
1.
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"

2. cat /proc/interrupts in the guest console:
estuary:/$ cat /proc/interrupts
CPU0
2:       3228     GICv3  27 Level     arch_timer
4:         15     GICv3  33 Level     uart-pl011
42:          0     GICv3  23 Level     arm-pmu
IPI0:         0       Rescheduling interrupts
IPI1:         0       Function call interrupts
IPI2:         0       CPU stop interrupts
IPI3:         0       CPU stop (for crash dump) interrupts
IPI4:         0       Timer broadcast interrupts
IPI5:         0       IRQ work interrupts
IPI6:         0       CPU wake-up interrupts
Err:          0

Fixes: 04ce935c6b2a ("gpio: pl061: Pass irqchip when adding gpiochip")
Signed-off-by: Wei Xu <xuwei5@hisilicon.com>
---
  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


.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption
  2019-08-12 11:28 [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption Wei Xu
@ 2019-08-14  9:04 ` Linus Walleij
  2019-08-16  8:41   ` Wei Xu
  2019-08-15 13:10 ` Andy Shevchenko
  1 sibling, 1 reply; 5+ messages in thread
From: Linus Walleij @ 2019-08-14  9:04 UTC (permalink / raw)
  To: Wei Xu
  Cc: open list:GPIO SUBSYSTEM, linux-kernel, linux-arm-kernel,
	Linuxarm, Shameerali Kolothum Thodi, Jonathan Cameron,
	John Garry, Salil Mehta, Shiju Jose, jinying, Zhangyi ac,
	Liguozhu (Kenneth),
	Tangkunshan, huangdaode

Hi Wei,

thanks for your patch!

This doesn't apply for my "devel" branch, can you rebase
on this:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=devel

We have moved some ACPI headers around recently.

On Mon, Aug 12, 2019 at 1:28 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.

Makes sense.

> Fixes: 04ce935c6b2a ("gpio: pl061: Pass irqchip when adding gpiochip")

I doubt this is a regression since I haven't seen anyone use this
gpiochip with ACPI before.

Please rename the patch "gpio: pl061: Add ACPI support" unless
you can convince me it worked without changes before.

Please include some ACPI people on review of this. From
MAINTAINERS:
ACPI
M:      "Rafael J. Wysocki" <rjw@rjwysocki.net>
M:      Len Brown <lenb@kernel.org>
L:      linux-acpi@vger.kernel.org

I would also include Andy Shevchenko and Mika Westerberg for
the GPIO aspects.

Thanks!
Linus Walleij

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption
  2019-08-12 11:28 [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption Wei Xu
  2019-08-14  9:04 ` Linus Walleij
@ 2019-08-15 13:10 ` Andy Shevchenko
  2019-08-15 13:39   ` Linus Walleij
  1 sibling, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2019-08-15 13:10 UTC (permalink / raw)
  To: Wei Xu, Mika Westerberg
  Cc: open list:GPIO SUBSYSTEM, Linux Kernel Mailing List,
	linux-arm-kernel, Linus Walleij, Linuxarm,
	Shameerali Kolothum Thodi, Jonathan Cameron, John Garry,
	Salil Mehta, Shiju Jose, jinying, Zhangyi ac, Liguozhu (Kenneth),
	Tangkunshan, huangdaode

On Mon, Aug 12, 2019 at 2:30 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 on QEMU v4.1.0-rc3 and Linux kernel v5.3-rc4,
> and found pl061 interruption is missed in the /proc/interrupts:
> 1.
> 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"
>
> 2. cat /proc/interrupts in the guest console:
> estuary:/$ cat /proc/interrupts
> CPU0
> 2:       3228     GICv3  27 Level     arch_timer
> 4:         15     GICv3  33 Level     uart-pl011
> 42:          0     GICv3  23 Level     arm-pmu
> IPI0:         0       Rescheduling interrupts
> IPI1:         0       Function call interrupts
> IPI2:         0       CPU stop interrupts
> IPI3:         0       CPU stop (for crash dump) interrupts
> IPI4:         0       Timer broadcast interrupts
> IPI5:         0       IRQ work interrupts
> IPI6:         0       CPU wake-up interrupts
> Err:          0
>
> Fixes: 04ce935c6b2a ("gpio: pl061: Pass irqchip when adding gpiochip")

Linus, I'm wondering if we can do this for all inside the GPIO library.
Thoughts?

--
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption
  2019-08-15 13:10 ` Andy Shevchenko
@ 2019-08-15 13:39   ` Linus Walleij
  0 siblings, 0 replies; 5+ messages in thread
From: Linus Walleij @ 2019-08-15 13:39 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wei Xu, Mika Westerberg, open list:GPIO SUBSYSTEM,
	Linux Kernel Mailing List, linux-arm-kernel, Linuxarm,
	Shameerali Kolothum Thodi, Jonathan Cameron, John Garry,
	Salil Mehta, Shiju Jose, jinying, Zhangyi ac, Liguozhu (Kenneth),
	Tangkunshan, huangdaode

On Thu, Aug 15, 2019 at 3:10 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Mon, Aug 12, 2019 at 2:30 PM Wei Xu <xuwei5@hisilicon.com> wrote:

> Linus, I'm wondering if we can do this for all inside the GPIO library.
> Thoughts?

If it's supposed to happen exactly the same way on all ACPI-enabled
gpiochips, I think it is more or less mandatory :D

We need to be sure we're not gonna have to quirks for misc variants
of GPIO controllers down in the gpiolib-acpi.c as a result because
some ACPI-thing is "standard not quite standard" though.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption
  2019-08-14  9:04 ` Linus Walleij
@ 2019-08-16  8:41   ` Wei Xu
  0 siblings, 0 replies; 5+ messages in thread
From: Wei Xu @ 2019-08-16  8:41 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM, linux-kernel, linux-arm-kernel,
	Linuxarm, Shameerali Kolothum Thodi, Jonathan Cameron,
	John Garry, Salil Mehta, Shiju Jose, jinying, Zhangyi ac,
	Liguozhu (Kenneth),
	Tangkunshan, huangdaode

Hi Linus,

On 2019/8/14 17:04, Linus Walleij wrote:
> Hi Wei,
>
> thanks for your patch!
>
> This doesn't apply for my "devel" branch, can you rebase
> on this:
> https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=devel
>
> We have moved some ACPI headers around recently.

Thanks to review!
I just sent out the v2 based on that.

> On Mon, Aug 12, 2019 at 1:28 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.
> Makes sense.
>
>> Fixes: 04ce935c6b2a ("gpio: pl061: Pass irqchip when adding gpiochip")
> I doubt this is a regression since I haven't seen anyone use this
> gpiochip with ACPI before.
>
> Please rename the patch "gpio: pl061: Add ACPI support" unless
> you can convince me it worked without changes before.

In the v2, I attached the log on QEMU v3.0.0 and Linux kernel v5.2.0-rc7 
and
the pl061 driver can register ACPI interruption.
Based on that, I did not rename the patch but I am OK to rename it if 
you have
any doubt.

> Please include some ACPI people on review of this. From
> MAINTAINERS:
> ACPI
> M:      "Rafael J. Wysocki" <rjw@rjwysocki.net>
> M:      Len Brown <lenb@kernel.org>
> L:      linux-acpi@vger.kernel.org
>
> I would also include Andy Shevchenko and Mika Westerberg for
> the GPIO aspects.
Copied to all of them in the v2.
Thanks!

Best Regards,
Wei

> Thanks!
> Linus Walleij
>
> .
>



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-12 11:28 [PATCH] gpio: pl061: Fix the issue failed to register the ACPI interruption Wei Xu
2019-08-14  9:04 ` Linus Walleij
2019-08-16  8:41   ` Wei Xu
2019-08-15 13:10 ` Andy Shevchenko
2019-08-15 13:39   ` Linus Walleij

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org linux-gpio@archiver.kernel.org
	public-inbox-index linux-gpio


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/ public-inbox