Re: [PATCH/RFC] gpio: rcar: Add Runtime PM handling for interrupts

[Linus Walleij]

On Tue, Feb 16, 2016 at 10:28 AM, Jon Hunter  wrote:

> As I mentioned I do plan to get back to the series for adding runtime-pm
> support for IRQ chips, in the next week or two.

It sounds like the two of you need to coordinate your work.

We're breaking new ground here so keep me, Ulf and tglx in the loop!

Yours,
Linus Walleij

Re: [PATCH/RFC] gpio: rcar: Add Runtime PM handling for interrupts

[Geert Uytterhoeven]

Hi Jon,

On Tue, Feb 16, 2016 at 10:28 AM, Jon Hunter  wrote:
> On 09/02/16 15:19, Geert Uytterhoeven wrote:
>> The R-Car GPIO driver handles Runtime PM for requested GPIOs only.
>>
>> When using a GPIO purely as an interrupt source, no Runtime PM handling
>> is done, and the GPIO module's clock may not be enabled.
>>
>> To fix this:
>>   - Add .irq_request_resources() and .irq_release_resources() callbacks
>> to handle Runtime PM when an interrupt is requested,
>
> I know that when I was looking at runtime-pm support for IRQ chips
> (which I have been meaning to get back too), the problem with
> irq_request_resources() is that it is called from the context of a
> spinlock (in __setup_irq()). You mentioned that you have not seen any
> reports of might_sleep_if(), but have you ensured that it is actually
> runtime resuming in your testing and you are not getting lucky?

It must be runtime-resuming, because without the call to pm_runtime_get_sync()
interrupts don't work.

> An alternative for you might be to use the
> irq_bus_lock/irq_bus_sync_unlock callbacks. See what Grygorii
> implemented for OMAP in commit aca82d1cbb49 ("gpio: omap: move pm
> runtime in irq_chip.irq_bus_lock/sync_unlock").

Thanks, that indeed looks interesting...

> As I mentioned I do plan to get back to the series for adding runtime-pm
> support for IRQ chips, in the next week or two.

Looking forward to it!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH/RFC] gpio: rcar: Add Runtime PM handling for interrupts

[Jon Hunter]

Hi Geert,

On 09/02/16 15:19, Geert Uytterhoeven wrote:
> The R-Car GPIO driver handles Runtime PM for requested GPIOs only.
> 
> When using a GPIO purely as an interrupt source, no Runtime PM handling
> is done, and the GPIO module's clock may not be enabled.
> 
> To fix this:
>   - Add .irq_request_resources() and .irq_release_resources() callbacks
> to handle Runtime PM when an interrupt is requested,

I know that when I was looking at runtime-pm support for IRQ chips
(which I have been meaning to get back too), the problem with
irq_request_resources() is that it is called from the context of a
spinlock (in __setup_irq()). You mentioned that you have not seen any
reports of might_sleep_if(), but have you ensured that it is actually
runtime resuming in your testing and you are not getting lucky?

An alternative for you might be to use the
irq_bus_lock/irq_bus_sync_unlock callbacks. See what Grygorii
implemented for OMAP in commit aca82d1cbb49 ("gpio: omap: move pm
runtime in irq_chip.irq_bus_lock/sync_unlock").

As I mentioned I do plan to get back to the series for adding runtime-pm
support for IRQ chips, in the next week or two.

Cheers
Jon

---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---

Re: [PATCH/RFC] gpio: rcar: Add Runtime PM handling for interrupts

[Linus Walleij]

On Tue, Feb 9, 2016 at 4:19 PM, Geert Uytterhoeven
 wrote:

Quoting in verbatim as we add new recipients.

I don't know about any runtime_pm_get():s from the irqchip functions:
Ulf and others are discussing with Thomas Gleixner about a more general
solution here.

But since it's a regression I guess we need quick decisions.

> The R-Car GPIO driver handles Runtime PM for requested GPIOs only.
>
> When using a GPIO purely as an interrupt source, no Runtime PM handling
> is done, and the GPIO module's clock may not be enabled.
>
> To fix this:
>   - Add .irq_request_resources() and .irq_release_resources() callbacks
> to handle Runtime PM when an interrupt is requested,

This is pretty OK since they are slowpath.

>   - Runtime-resume the device before writing to the GPIO module's
> registers for disabling/enabling an interrupt, or for configuring
> the interrupt type,

Will that have *any* effect now that .irq_request_resources has increased
usecount to 1?

Isn't it enough with the patches to .irq_request/release_resources to get
this working?

>   - Add checks to warn when the GPIO module's registers are accessed
> while the device is runtime-suspended.
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> This fixes ravb Ethernet on r8a7795/Salvator-X, which was "broken" by
> commit d5c3d84657db57bd ("net: phy: Avoid polling PHY with
> PHY_IGNORE_INTERRUPTS").
>
> Does it also fix the HDMI interrupt on r8a7791/Koelsch?
>
> Notes:
>   1. I assume gpio_rcar_irq_{dis,en}able() may be called from contexts
>  where pm_runtime_get_sync() may not be called.
>  However, so far I didn't see any reports from the might_sleep_if()
>  check in __pm_runtime_resume(),
>   2. gpio_rcar_irq_disable() is called from the IRQ core before
>  gpio_rcar_irq_set_type().
>  Else an alternative solution could be to just runtime-resume the
>  device from gpio_rcar_irq_set_type() when an interrupt is setup,
>  and never call pm_runtime_put() again. That would cause issues when
>  compiling gpio-rcar as a module, though.
> ---
>  drivers/gpio/gpio-rcar.c | 42 ++
>  1 file changed, 42 insertions(+)
>
> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c
> index cf41440aff91971e..63b679b3af5d6d16 100644
> --- a/drivers/gpio/gpio-rcar.c
> +++ b/drivers/gpio/gpio-rcar.c
> @@ -59,12 +59,18 @@ struct gpio_rcar_priv {
>
>  static inline u32 gpio_rcar_read(struct gpio_rcar_priv *p, int offs)
>  {
> +   WARN(pm_runtime_suspended(>pdev->dev),
> + "%s: %s is runtime-suspended\n", __func__,
> + dev_name(>pdev->dev));
> return ioread32(p->base + offs);
>  }
>
>  static inline void gpio_rcar_write(struct gpio_rcar_priv *p, int offs,
>u32 value)
>  {
> +   WARN(pm_runtime_suspended(>pdev->dev),
> + "%s: %s is runtime-suspended\n", __func__,
> + dev_name(>pdev->dev));
> iowrite32(value, p->base + offs);
>  }
>
> @@ -86,7 +92,11 @@ static void gpio_rcar_irq_disable(struct irq_data *d)
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct gpio_rcar_priv *p = gpiochip_get_data(gc);
>
> +   if (pm_runtime_get_sync(>pdev->dev) < 0)
> +   return;
> +
> gpio_rcar_write(p, INTMSK, ~BIT(irqd_to_hwirq(d)));
> +   pm_runtime_put(>pdev->dev);
>  }
>
>  static void gpio_rcar_irq_enable(struct irq_data *d)
> @@ -94,7 +104,11 @@ static void gpio_rcar_irq_enable(struct irq_data *d)
> struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> struct gpio_rcar_priv *p = gpiochip_get_data(gc);
>
> +   if (pm_runtime_get_sync(>pdev->dev) < 0)
> +   return;
> +
> gpio_rcar_write(p, MSKCLR, BIT(irqd_to_hwirq(d)));
> +   pm_runtime_put(>pdev->dev);
>  }
>
>  static void gpio_rcar_config_interrupt_input_mode(struct gpio_rcar_priv *p,
> @@ -105,6 +119,9 @@ static void gpio_rcar_config_interrupt_input_mode(struct 
> gpio_rcar_priv *p,
>  {
> unsigned long flags;
>
> +   if (pm_runtime_get_sync(>pdev->dev) < 0)
> +   return;
> +
> /* follow steps in the GPIO documentation for
>  * "Setting Edge-Sensitive Interrupt Input Mode" and
>  * "Setting Level-Sensitive Interrupt Input Mode"
> @@ -130,6 +147,8 @@ static void gpio_rcar_config_interrupt_input_mode(struct 
> gpio_rcar_priv *p,
> gpio_rcar_write(p, INTCLR, BIT(hwirq));
>
> spin_unlock_irqrestore(>lock, flags);
> +
> +   pm_runtime_put(>pdev->dev);
>  }
>
>  static int gpio_rcar_irq_set_type(struct irq_data *d, unsigned int type)
> @@ -196,6 +215,27 @@ static int gpio_rcar_irq_set_wake(struct irq_data *d, 
> unsigned int on)
> return 0;
>  }
>
> +static int gpio_rcar_irq_request_resources(struct irq_data *d)
> +{
> +