Runtime PM handling in gpio-zynq

[Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>]

Hello,

Today, I discovered that the runtime PM support in the gpio-zynq driver
was not entirely correct. Indeed, it does a pm_runtime_get_sync() in
the ->request() hook.

However, if the only GPIO used on the Zynq GPIO controller is used for
an interrupt (as is my case, where a PCA GPIO expander INT# signal is
connected to PS_MIO11 on the Zynq side), then ->request() is not
called. Due to this, pm_runtime_get_sync() is never called, and the
GPIO controller remains in PM runtime suspended state. This is
confirmed by looking
at /sys/bus/platform/devices/e000a000.gpio/power/runtime_status.

Forcing the GPIO controller on by writing "on"
to /sys/bus/platform/devices/e000a000.gpio/power/control makes it work
perfectly fine.

After discussing with Michal on IRC, I tried implementing the solution
used in gpio-omap, which consists in pm_runtime_get_sync() in
->irq_bus_lock() and pm_runtime_put() in ->irq_bus_sync_unlock(). While
this makes sure that the GPIO controller is runtime resumed when
masking/unmask interrupts, it remains runtime suspended otherwise, and
therefore never notices that interrupts are happening.

Looking at the gpio-omap implementation, I see that they do a
pm_runtime_get_sync() inside the GPIO controller IRQ handler. So it
seems like on OMAP the GPIO controller can fire its interrupt in a
runtime PM suspend state, and doing a pm_runtime_get_sync() in the
interrupt handler is needed to be able to read the GPIO controller
registers.

On Zynq, it seems like if the GPIO controller is not clocked, it will
not notice interrupts. So basically, as soon as one GPIO is used as an
interrupt on Zynq, the GPIO controller should not be runtime suspended.

What is the proper way of implementing this ?

As seen in aca82d1cbb49af34b69ecd4571a0fe48ad9247c1, doing the
pm_runtime_get_sync() in ->irq_set_type() is not the right thing to do.

Best regards,

Thomas

For reference, here is the change I did to gpio-zynq.c:

diff --git a/drivers/gpio/gpio-zynq.c b/drivers/gpio/gpio-zynq.c
index 3f5fcdd5a429..5462b8bcc234 100644
--- a/drivers/gpio/gpio-zynq.c
+++ b/drivers/gpio/gpio-zynq.c
@@ -533,6 +535,24 @@ static int zynq_gpio_set_wake(struct irq_data *data, unsigned int on)
        return 0;
 }
 
+static void zynq_gpio_irq_bus_lock(struct irq_data *data)
+{
+       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+       pr_err("%s: %d\n", __func__, __LINE__);
+
+       pm_runtime_get_sync(chip->parent);
+}
+
+static void zynq_gpio_irq_bus_sync_unlock(struct irq_data *data)
+{
+       struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+       pr_err("%s: %d\n", __func__, __LINE__);
+
+       pm_runtime_put(chip->parent);
+}
+
 /* irq chip descriptor */
 static struct irq_chip zynq_gpio_level_irqchip = {
        .name           = DRIVER_NAME,
@@ -542,6 +562,8 @@ static struct irq_chip zynq_gpio_level_irqchip = {
        .irq_unmask     = zynq_gpio_irq_unmask,
        .irq_set_type   = zynq_gpio_set_irq_type,
        .irq_set_wake   = zynq_gpio_set_wake,
+       .irq_bus_lock   = zynq_gpio_irq_bus_lock,
+       .irq_bus_sync_unlock = zynq_gpio_irq_bus_sync_unlock,
        .flags          = IRQCHIP_EOI_THREADED | IRQCHIP_EOI_IF_HANDLED |
                          IRQCHIP_MASK_ON_SUSPEND,
 };
@@ -554,6 +576,8 @@ static struct irq_chip zynq_gpio_edge_irqchip = {
        .irq_unmask     = zynq_gpio_irq_unmask,
        .irq_set_type   = zynq_gpio_set_irq_type,
        .irq_set_wake   = zynq_gpio_set_wake,
+       .irq_bus_lock   = zynq_gpio_irq_bus_lock,
+       .irq_bus_sync_unlock = zynq_gpio_irq_bus_sync_unlock,
        .flags          = IRQCHIP_MASK_ON_SUSPEND,
 };
 
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com