Runtime PM handling in gpio-zynq

Message ID 20190103165031.20a49185@windsurf
State New
Headers show
Series
  • Runtime PM handling in gpio-zynq
Related show

Commit Message

Thomas Petazzoni Jan. 3, 2019, 3:50 p.m.
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:

Comments

Thomas Petazzoni Jan. 7, 2019, 3:35 p.m. | #1
Hello,

On Thu, 3 Jan 2019 16:50:31 +0100, Thomas Petazzoni wrote:

> 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.

FWIW, off-list, Shubhrajyoti Datta <shubhraj@xilinx.com> pointed to me
the proposal that Michal did back in 2017 on this topic:

  https://patchwork.kernel.org/patch/9899079/

Since this is relevant, I will reply to this older thread, with some
testing results and comments.

Best regards,

Thomas

Patch

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,
 };