| Message ID | 20190103165031.20a49185@windsurf |
|---|---|
| State | New |
| Headers | show |
| Series | Runtime PM handling in gpio-zynq | expand |
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
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, };