Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion
On Tue, Aug 20, 2019 at 10:51 AM Andy Shevchenko wrote: > On Tue, Aug 20, 2019 at 10:12 AM Linus Walleij > wrote: > > On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko > > 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 > > 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 > > Acked-by: Grygorii Strashko > > Signed-off-by: Linus Walleij > > 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
Re: [PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion
Hi Linus, On 2019/8/20 16:01, Linus Walleij wrote: On Mon, Aug 19, 2019 at 3:29 PM Wei Xu 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-ker...@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
On Tue, Aug 20, 2019 at 10:12 AM Linus Walleij wrote: > On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko > 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 > 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 > Acked-by: Grygorii Strashko > Signed-off-by: Linus Walleij 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
On Tue, Aug 20, 2019 at 10:12 AM Linus Walleij wrote: > > On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko > 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 > 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 > Acked-by: Grygorii Strashko > Signed-off-by: Linus Walleij > > 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
On Mon, Aug 19, 2019 at 3:29 PM Wei Xu 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
On Mon, Aug 19, 2019 at 5:07 PM Andy Shevchenko 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 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 Acked-by: Grygorii Strashko Signed-off-by: Linus Walleij 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
On Mon, Aug 19, 2019 at 4:29 PM Wei Xu 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,0x900" > > 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
[PATCH v3] gpio: pl061: Fix the issue failed to register the ACPI interrtupion
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,0x900"
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
---
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
#include
#include
#include
@@ -24,6 +25,9 @@
#include
#include
+#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(>gc);
+
amba_set_drvdata(adev, pl061);
dev_info(dev, "PL061 GPIO chip registered\n");
--
2.8.1
