[2/8] gpio: zynq: Wakeup gpio controller when it is used as IRQ controller

On 14.8.2017 15:53, Linus Walleij wrote:
> On Mon, Aug 7, 2017 at 1:01 PM, Michal Simek <michal.simek@xilinx.com> wrote:
> 
>> From: Borsodi Petr <Petr.Borsodi@i.cz>
>>
>> There is a problem with GPIO driver when used as IRQ controller.
>> It is not working because the module is sleeping (clock is disabled).
>> The patch enables clocks when IP is used as IRQ controller.
>>
>> Signed-off-by: Borsodi Petr <Petr.Borsodi@i.cz>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> 
> I'm a bit worried about this patch.
> 
>> +static int zynq_gpio_irq_request_resources(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +       int ret;
>> +
>> +       if (!try_module_get(chip->gpiodev->owner))
>> +               return -ENODEV;
> 
> You are poking around in gpiolib internals, I don't really like that.
> I prefer that you use accessors and try to make the core deal with
> this instead of fixing it up with a local hack in the driver.
> 
>> +       ret = pm_runtime_get_sync(chip->parent);
>> +       if (ret < 0) {
>> +               module_put(chip->gpiodev->owner);
>> +               return ret;
>> +       }
> 
> What you essentially do is disable runtime PM while IRQs are in
> use, the patch commit log should say this.
> 
>> +       if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>> +               chip_err(chip, "unable to lock HW IRQ %lu for IRQ\n", d->hwirq);
>> +               pm_runtime_put(chip->parent);
>> +               module_put(chip->gpiodev->owner);
>> +               return -EINVAL;
>> +       }
> 
> This is essentially a separate patch that should be done orthogonally.
> (I don't care super-much about that though.)
> 
>> +static void zynq_gpio_irq_release_resources(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +       gpiochip_unlock_as_irq(chip, d->hwirq);
>> +       pm_runtime_put(chip->parent);
>> +       module_put(chip->gpiodev->owner);
>> +}
> (...)
>> +       .irq_request_resources = zynq_gpio_irq_request_resources,
>> +       .irq_release_resources = zynq_gpio_irq_release_resources,
> 
> Look at this from gpiolib.c:
> 
> static int gpiochip_irq_reqres(struct irq_data *d)
> {
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> 
>         if (!try_module_get(chip->gpiodev->owner))
>                 return -ENODEV;
> 
>         if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>                 chip_err(chip,
>                         "unable to lock HW IRQ %lu for IRQ\n",
>                         d->hwirq);
>                 module_put(chip->gpiodev->owner);
>                 return -EINVAL;
>         }
>         return 0;
> }
> 
> static void gpiochip_irq_relres(struct irq_data *d)
> {
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> 
>         gpiochip_unlock_as_irq(chip, d->hwirq);
>         module_put(chip->gpiodev->owner);
> }
> 
> If you add pm_runtime_get_sync()/put to this and export
> the functions you have the same thing and you can just reuse this
> code instead of copying it.
> 
> Arguably the above should indeed have that runtime PM code
> (unless we know a better way to deal with IRQs).
> 
> So can we fix this in the core and reuse it from there?

I have checked 4.13-rc1 and none is doing anything with clock in these
irq routines.
It means it is a question if they have the same issue when device is
sleeping or we do something wrong.
It is not a problem to move these calls to core (patch is quite simple)
but validate that if this is correct on others SoC.
Do you know if we can validate this on different SoC?

Nava: Can you please validate this again on ZynqMP?

Thanks,
Michal

        }
@@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);

        gpiochip_unlock_as_irq(chip, d->hwirq);
+       pm_runtime_put(chip->parent);
        module_put(chip->gpiodev->owner);
 }


--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Aug 14, 2017 at 4:33 PM, Michal Simek <michal.simek@xilinx.com> wrote:

> I have checked 4.13-rc1 and none is doing anything with clock in these
> irq routines.
> It means it is a question if they have the same issue when device is
> sleeping or we do something wrong.

No but they may get in the future and new drivers may have
the issue.

> It is not a problem to move these calls to core (patch is quite simple)
> but validate that if this is correct on others SoC.
> Do you know if we can validate this on different SoC?

pm_runtime_get() etc are only utilized if the driver
explicitly enable runtime PM, and if they do, they should
have their semantics right for this or their code would be
broken severely.


> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 9568708a550b..a08a044fa4aa 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain
> *d, unsigned int irq)
>  static int gpiochip_irq_reqres(struct irq_data *d)
>  {
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +       int ret;
>
>         if (!try_module_get(chip->gpiodev->owner))
>                 return -ENODEV;
>
> +       ret = pm_runtime_get_sync(chip->parent);
> +       if (ret < 0) {
> +               module_put(chip->gpiodev->owner);
> +               return ret;
> +       }
> +
>         if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>                 chip_err(chip,
>                         "unable to lock HW IRQ %lu for IRQ\n",
>                         d->hwirq);
> +               pm_runtime_put(chip->parent);
>                 module_put(chip->gpiodev->owner);
>                 return -EINVAL;
>         }
> @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>
>         gpiochip_unlock_as_irq(chip, d->hwirq);
> +       pm_runtime_put(chip->parent);
>         module_put(chip->gpiodev->owner);

This looks fine, I'm happy to apply that early for v4.15 after the merge
window (now it is a bit late for radical changes).

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hello,

I am reviving this old thread, because the proposed patch (almost)
solves the problem I recently reported with the bad interaction of
runtime PM with the Zynq GPIO driver (see
https://www.spinics.net/lists/linux-gpio/msg35437.html).

On Mon, 14 Aug 2017 16:33:09 +0200, Michal Simek wrote:

> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 9568708a550b..a08a044fa4aa 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain
> *d, unsigned int irq)
>  static int gpiochip_irq_reqres(struct irq_data *d)
>  {
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +       int ret;
> 
>         if (!try_module_get(chip->gpiodev->owner))
>                 return -ENODEV;
> 
> +       ret = pm_runtime_get_sync(chip->parent);
> +       if (ret < 0) {
> +               module_put(chip->gpiodev->owner);
> +               return ret;
> +       }
> +
>         if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>                 chip_err(chip,
>                         "unable to lock HW IRQ %lu for IRQ\n",
>                         d->hwirq);
> +               pm_runtime_put(chip->parent);
>                 module_put(chip->gpiodev->owner);
>                 return -EINVAL;
>         }
> @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> 
>         gpiochip_unlock_as_irq(chip, d->hwirq);
> +       pm_runtime_put(chip->parent);
>         module_put(chip->gpiodev->owner);
>  }

This patch almost solves the problem. It doesn't work as-is, because it
assumes that runtime PM is used by all GPIO controllers, which is not
the case. When runtime PM is not enabled, pm_runtime_get_sync() fails
with -EACCES, and the whole gpiochip_irq_reqres() function aborts.

The following patch works fine in my case (a MMC card detect signal is
connected to a pin of a PCA GPIO expander over I2C, whose INT# pin is
itself connected to a GPIO pin of the Zynq SoC).

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 20887c62fbb3..bd9a81fc8d56 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -27,6 +27,7 @@
 #include <linux/kfifo.h>
 #include <linux/poll.h>
 #include <linux/timekeeping.h>
+#include <linux/pm_runtime.h>
 #include <uapi/linux/gpio.h>
 
 #include "gpiolib.h"
@@ -3540,12 +3541,23 @@ int gpiochip_reqres_irq(struct gpio_chip *chip, unsigned int offset)
        if (!try_module_get(chip->gpiodev->owner))
                return -ENODEV;
 
+       if (pm_runtime_enabled(chip->parent)) {
+               ret = pm_runtime_get_sync(chip->parent);
+               if (ret < 0) {
+                       module_put(chip->gpiodev->owner);
+                       return ret;
+               }
+       }
+
        ret = gpiochip_lock_as_irq(chip, offset);
        if (ret) {
                chip_err(chip, "unable to lock HW IRQ %u for IRQ\n", offset);
+               if (pm_runtime_enabled(chip->parent))
+                       pm_runtime_put(chip->parent);
                module_put(chip->gpiodev->owner);
                return ret;
        }
+
        return 0;
 }
 EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
@@ -3553,6 +3565,8 @@ EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
 void gpiochip_relres_irq(struct gpio_chip *chip, unsigned int offset)
 {
        gpiochip_unlock_as_irq(chip, offset);
+       if (pm_runtime_enabled(chip->parent))
+               pm_runtime_put(chip->parent);
        module_put(chip->gpiodev->owner);
 }
 EXPORT_SYMBOL_GPL(gpiochip_relres_irq);

However, I must say that from a design perspective, I am not a big fan
of this solution. Indeed for the normal GPIO ->request() and ->free()
hooks, it is currently the GPIO driver itself that is responsible for
runtime PM get/put, so it would be weird to have the runtime PM get/put
for the IRQ request/free be done by the GPIO core.

I believe that either the GPIO core should be in charge of the entire
runtime PM interaction, or it should entirely be the responsibility of
each GPIO controller driver. Having a mixed solution seems very
confusing.

Let me know which direction should be taken so that I can submit a
proper patch to hopefully resolve this issue.

Best regards,

Thomas

On 07. 01. 19 16:42, Thomas Petazzoni wrote:
> Hello,
> 
> I am reviving this old thread, because the proposed patch (almost)
> solves the problem I recently reported with the bad interaction of
> runtime PM with the Zynq GPIO driver (see
> https://www.spinics.net/lists/linux-gpio/msg35437.html).
> 
> On Mon, 14 Aug 2017 16:33:09 +0200, Michal Simek wrote:
> 
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 9568708a550b..a08a044fa4aa 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -1647,14 +1647,22 @@ static void gpiochip_irq_unmap(struct irq_domain
>> *d, unsigned int irq)
>>  static int gpiochip_irq_reqres(struct irq_data *d)
>>  {
>>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +       int ret;
>>
>>         if (!try_module_get(chip->gpiodev->owner))
>>                 return -ENODEV;
>>
>> +       ret = pm_runtime_get_sync(chip->parent);
>> +       if (ret < 0) {
>> +               module_put(chip->gpiodev->owner);
>> +               return ret;
>> +       }
>> +
>>         if (gpiochip_lock_as_irq(chip, d->hwirq)) {
>>                 chip_err(chip,
>>                         "unable to lock HW IRQ %lu for IRQ\n",
>>                         d->hwirq);
>> +               pm_runtime_put(chip->parent);
>>                 module_put(chip->gpiodev->owner);
>>                 return -EINVAL;
>>         }
>> @@ -1666,6 +1674,7 @@ static void gpiochip_irq_relres(struct irq_data *d)
>>         struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>>
>>         gpiochip_unlock_as_irq(chip, d->hwirq);
>> +       pm_runtime_put(chip->parent);
>>         module_put(chip->gpiodev->owner);
>>  }
> 
> This patch almost solves the problem. It doesn't work as-is, because it
> assumes that runtime PM is used by all GPIO controllers, which is not
> the case. When runtime PM is not enabled, pm_runtime_get_sync() fails
> with -EACCES, and the whole gpiochip_irq_reqres() function aborts.
> 
> The following patch works fine in my case (a MMC card detect signal is
> connected to a pin of a PCA GPIO expander over I2C, whose INT# pin is
> itself connected to a GPIO pin of the Zynq SoC).
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 20887c62fbb3..bd9a81fc8d56 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -27,6 +27,7 @@
>  #include <linux/kfifo.h>
>  #include <linux/poll.h>
>  #include <linux/timekeeping.h>
> +#include <linux/pm_runtime.h>
>  #include <uapi/linux/gpio.h>
>  
>  #include "gpiolib.h"
> @@ -3540,12 +3541,23 @@ int gpiochip_reqres_irq(struct gpio_chip *chip, unsigned int offset)
>         if (!try_module_get(chip->gpiodev->owner))
>                 return -ENODEV;
>  
> +       if (pm_runtime_enabled(chip->parent)) {
> +               ret = pm_runtime_get_sync(chip->parent);
> +               if (ret < 0) {
> +                       module_put(chip->gpiodev->owner);
> +                       return ret;
> +               }
> +       }
> +
>         ret = gpiochip_lock_as_irq(chip, offset);
>         if (ret) {
>                 chip_err(chip, "unable to lock HW IRQ %u for IRQ\n", offset);
> +               if (pm_runtime_enabled(chip->parent))
> +                       pm_runtime_put(chip->parent);
>                 module_put(chip->gpiodev->owner);
>                 return ret;
>         }
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
> @@ -3553,6 +3565,8 @@ EXPORT_SYMBOL_GPL(gpiochip_reqres_irq);
>  void gpiochip_relres_irq(struct gpio_chip *chip, unsigned int offset)
>  {
>         gpiochip_unlock_as_irq(chip, offset);
> +       if (pm_runtime_enabled(chip->parent))
> +               pm_runtime_put(chip->parent);
>         module_put(chip->gpiodev->owner);
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_relres_irq);
> 
> However, I must say that from a design perspective, I am not a big fan
> of this solution. Indeed for the normal GPIO ->request() and ->free()
> hooks, it is currently the GPIO driver itself that is responsible for
> runtime PM get/put, so it would be weird to have the runtime PM get/put
> for the IRQ request/free be done by the GPIO core.
> 
> I believe that either the GPIO core should be in charge of the entire
> runtime PM interaction, or it should entirely be the responsibility of
> each GPIO controller driver. Having a mixed solution seems very
> confusing.
> 
> Let me know which direction should be taken so that I can submit a
> proper patch to hopefully resolve this issue.

I think it is up to Linus to say which way he wants to go. We found that
way which omap is using.

In connection to this old patch. I think I have tested it later and
wasn't able to replicate it that's why I stop keep track on this.

Thanks,
Michal

On Mon, Jan 7, 2019 at 4:42 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:

> This patch almost solves the problem. It doesn't work as-is, because it
> assumes that runtime PM is used by all GPIO controllers, which is not
> the case. When runtime PM is not enabled, pm_runtime_get_sync() fails
> with -EACCES, and the whole gpiochip_irq_reqres() function aborts.
(...)
> However, I must say that from a design perspective, I am not a big fan
> of this solution. Indeed for the normal GPIO ->request() and ->free()
> hooks, it is currently the GPIO driver itself that is responsible for
> runtime PM get/put, so it would be weird to have the runtime PM get/put
> for the IRQ request/free be done by the GPIO core.
>
> I believe that either the GPIO core should be in charge of the entire
> runtime PM interaction, or it should entirely be the responsibility of
> each GPIO controller driver. Having a mixed solution seems very
> confusing.

My stance is that the driver is responsible of enabling and managing
runtime PM for its hardware block(s).

Runtime PM in the core should only be added if the core needs to
be aware about it, such as is the case when e.g. a block device
needs to drain its write buffer before going to runtime sleep.

I fail so see why the GPIO core need to be aware about this.

Yours,
Linus Walleij

Hello Linus,

On Fri, 11 Jan 2019 10:54:20 +0100, Linus Walleij wrote:

> My stance is that the driver is responsible of enabling and managing
> runtime PM for its hardware block(s).
> 
> Runtime PM in the core should only be added if the core needs to
> be aware about it, such as is the case when e.g. a block device
> needs to drain its write buffer before going to runtime sleep.
> 
> I fail so see why the GPIO core need to be aware about this.

In this very same thread at
https://www.spinics.net/lists/arm-kernel/msg600515.html, you kind of
proposed to handle this in the core in fact :-) Though indeed you said
that the core could provide helpers.

Thomas

On Fri, Jan 11, 2019 at 1:54 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
> On Fri, 11 Jan 2019 10:54:20 +0100, Linus Walleij wrote:
>
> > My stance is that the driver is responsible of enabling and managing
> > runtime PM for its hardware block(s).
> >
> > Runtime PM in the core should only be added if the core needs to
> > be aware about it, such as is the case when e.g. a block device
> > needs to drain its write buffer before going to runtime sleep.
> >
> > I fail so see why the GPIO core need to be aware about this.
>
> In this very same thread at
> https://www.spinics.net/lists/arm-kernel/msg600515.html, you kind of
> proposed to handle this in the core in fact :-) Though indeed you said
> that the core could provide helpers.

Yeah allright, I have never been good with consistency but what I guess
I would mean to say (today) is that the driver needs to be in the driver
seat (heh) and opting in to any runtime PM support.

This is in contrast with "midlayer" where all drivers are forced to behave
"as if" they had runtime PM (i.e. calls are done to the runtime PM helpers
even if the device doesn't really activate runtime PM).

Yours,
Linus Walleij

On Fri, Jan 11, 2019 at 8:26 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Jan 11, 2019 at 1:54 PM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> > On Fri, 11 Jan 2019 10:54:20 +0100, Linus Walleij wrote:
> >
> > > My stance is that the driver is responsible of enabling and managing
> > > runtime PM for its hardware block(s).
> > >
> > > Runtime PM in the core should only be added if the core needs to
> > > be aware about it, such as is the case when e.g. a block device
> > > needs to drain its write buffer before going to runtime sleep.
> > >
> > > I fail so see why the GPIO core need to be aware about this.
> >
> > In this very same thread at
> > https://www.spinics.net/lists/arm-kernel/msg600515.html,  you kind of

I was not abloe to open the link could you please let me  know if I am
missing something?