Re: Runtime PM handling in gpio-zynq

[Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxx>]

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@xxxxxxxxxx> 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
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com