Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Linus Walleij]

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> Add a driver for the Tegra-AGIC interrupt controller which is compatible
> with the ARM GIC-400 interrupt controller.
(...)
> +static const struct dev_pm_ops gic_pm_ops = {
> +       SET_RUNTIME_PM_OPS(gic_runtime_suspend,
> +                          gic_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
> +};

Now you do what I commented on in the earlier patch: assign
the runtime PM functions to normal suspend/resume.

This will have the effect of inhibiting any IRQs marked for
wakeup on the GIC, even if you just want to go to sleep until
something happens, will it not?

You should turn on the alarm clock before going to bed, not
turn it off, as figure of speak ...

Yours,
Linus Walleij

Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Jon Hunter]

On 17/12/15 13:32, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> 
>> Add a driver for the Tegra-AGIC interrupt controller which is compatible
>> with the ARM GIC-400 interrupt controller.
> (...)
>> +static const struct dev_pm_ops gic_pm_ops = {
>> +       SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>> +                          gic_runtime_resume, NULL)
>> +       SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
>> +};
> 
> Now you do what I commented on in the earlier patch: assign
> the runtime PM functions to normal suspend/resume.
> 
> This will have the effect of inhibiting any IRQs marked for
> wakeup on the GIC, even if you just want to go to sleep until
> something happens, will it not?
> 
> You should turn on the alarm clock before going to bed, not
> turn it off, as figure of speak ...

Yes I am alway having problems with my alarm, may be this is why ;-)

I see what you are saying, so if there are any wake-ups enabled then we
should not suspend the chip. Right?

Cheers
Jon

Re: [RFC PATCH V2 8/8] irqchip/gic: Add support for tegra AGIC interrupt controller

[Linus Walleij]

On Fri, Dec 18, 2015 at 11:44 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On 17/12/15 13:32, Linus Walleij wrote:
>> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>
>>> Add a driver for the Tegra-AGIC interrupt controller which is compatible
>>> with the ARM GIC-400 interrupt controller.
>> (...)
>>> +static const struct dev_pm_ops gic_pm_ops = {
>>> +       SET_RUNTIME_PM_OPS(gic_runtime_suspend,
>>> +                          gic_runtime_resume, NULL)
>>> +       SET_SYSTEM_SLEEP_PM_OPS(gic_suspend, gic_resume)
>>> +};
>>
>> Now you do what I commented on in the earlier patch: assign
>> the runtime PM functions to normal suspend/resume.
>>
>> This will have the effect of inhibiting any IRQs marked for
>> wakeup on the GIC, even if you just want to go to sleep until
>> something happens, will it not?
>>
>> You should turn on the alarm clock before going to bed, not
>> turn it off, as figure of speak ...
>
> Yes I am alway having problems with my alarm, may be this is why ;-)
>
> I see what you are saying, so if there are any wake-ups enabled then we
> should not suspend the chip. Right?

Yep, so I think you should just wrap the normal suspend functions
into some function that check if we have some wakeup active
and then just return 0, else if everything is clear, call the same
runtime PM hooks, and it will work nicely I think. IIUC the wakeup
flag is only for the suspend/resume/sleep case so this should
be enough.

Yours,
Linus Walleij

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Linus Walleij]

On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:

(Adding Rafael and linux-pm to To: list)

> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.
>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

Overall I like what you're trying to do. This will enable e.g. I2C
GPIO supplying expanders to power down if none of its lines are
used for IRQs. (Read below on the suspend() case for even
better stuff we can do!)

(...)
> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> + * @dev:               pointer to associated device
(...)
>  struct irq_chip {
> +       struct device   *dev;

In struct gpio_chip I just this merge window have to merge a gigantic
patch renaming this from "dev" to "parent" because we need to add
a *real* struct device dev; to gpio_chip.

So for the advent that we may in the future need a real struct device
inside irq_chip, name this .parent already today, please.

> +/* Inline functions for support of irq chips that require runtime pm */
> +static inline int chip_pm_get(struct irq_desc *desc)
> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;
> +}

That is boiling all PM upward into the platform_device or whatever
it is containing this. But we're not just in it for runtime_pm_suspend()
and runtime_pm_resume(). We also have regular suspend() and
resume(). And ideally that should be handled by the same
callbacks.

First: what if the device contain any wakeup-flagged IRQs?
I think there is something missing here. The suspend() usecase
is not handled by this patch, but we need to think about that
here as well. I think irqchips on GPIO expanders (for example)
should be powered down on suspend() *unless* one or more of
its IRQs is flagged as wakeup, and in that case it should
*not* be powered down, instead it should just mask all
non-wakeup IRQs and restore them on resume().

Second: it's soo easy to get something wrong here. It'd be good
if the kernel was helpful. What about something like:

if (desc->irq_data.chip->dev) {
    if (desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
           retval = pm_runtime_get_sync(desc->irq_data.chip->dev)
    else if (pm_runtime_enabled(desc->irq_data.chip->dev))
           dev_warn_once(desc->irq_data.chip->dev, "irqchip not
flagged for RPM but has runtime PM enabled! weird.\n");
}

As I see it, a device that supplies an irqchip, has runtime PM but
is *NOT* setting IRQCHIP_HAS_RPM just *have* to be looked
at in detail, and deserve to have this littering its dmesg so we can
fix it. It just makes no real sense. It more sounds like a recepie for
missing interrupts otherwise.

Yours,
Linus Walleij

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

On 17/12/15 13:19, Linus Walleij wrote:
> On Thu, Dec 17, 2015 at 11:48 AM, Jon Hunter <jonathanh@nvidia.com> wrote:
> 
> (Adding Rafael and linux-pm to To: list)
> 
>> Some IRQ chips may be located in a power domain outside of the CPU
>> subsystem and hence will require device specific runtime power management.
>> In order to support such IRQ chips, add a pointer for a device structure
>> to the irq_chip structure, and if this pointer is populated by the IRQ
>> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
>> APIs for this chip will be called when an IRQ is requested/freed,
>> respectively.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> Overall I like what you're trying to do. This will enable e.g. I2C
> GPIO supplying expanders to power down if none of its lines are
> used for IRQs. (Read below on the suspend() case for even
> better stuff we can do!)
> 
> (...)
>> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> + * @dev:               pointer to associated device
> (...)
>>  struct irq_chip {
>> +       struct device   *dev;
> 
> In struct gpio_chip I just this merge window have to merge a gigantic
> patch renaming this from "dev" to "parent" because we need to add
> a *real* struct device dev; to gpio_chip.
> 
> So for the advent that we may in the future need a real struct device
> inside irq_chip, name this .parent already today, please.

Ok, fine with me.

>> +/* Inline functions for support of irq chips that require runtime pm */
>> +static inline int chip_pm_get(struct irq_desc *desc)
>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
>> +}
> 
> That is boiling all PM upward into the platform_device or whatever
> it is containing this. But we're not just in it for runtime_pm_suspend()
> and runtime_pm_resume(). We also have regular suspend() and
> resume(). And ideally that should be handled by the same
> callbacks.

Yes, I have purposely not tried to handle suspend here as I have left it
to be handle by suspend_device_irqs() called during system suspend.

I don't follow why we need to handle regular suspend here? Or is this
for chips that do not support runtime-pm?

> First: what if the device contain any wakeup-flagged IRQs?

So today with this patch, the IRQ chip would only be runtime suspended
if all IRQs are freed. So it should not impact wakeups. Unless I am
missing something?

> I think there is something missing here. The suspend() usecase
> is not handled by this patch, but we need to think about that
> here as well. I think irqchips on GPIO expanders (for example)
> should be powered down on suspend() *unless* one or more of
> its IRQs is flagged as wakeup, and in that case it should
> *not* be powered down, instead it should just mask all
> non-wakeup IRQs and restore them on resume().
> 
> Second: it's soo easy to get something wrong here. It'd be good
> if the kernel was helpful. What about something like:
> 
> if (desc->irq_data.chip->dev) {
>     if (desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>            retval = pm_runtime_get_sync(desc->irq_data.chip->dev)
>     else if (pm_runtime_enabled(desc->irq_data.chip->dev))
>            dev_warn_once(desc->irq_data.chip->dev, "irqchip not
> flagged for RPM but has runtime PM enabled! weird.\n");
> }
>
> As I see it, a device that supplies an irqchip, has runtime PM but
> is *NOT* setting IRQCHIP_HAS_RPM just *have* to be looked
> at in detail, and deserve to have this littering its dmesg so we can
> fix it. It just makes no real sense. It more sounds like a recepie for
> missing interrupts otherwise.

Yes, I agree, additional checking such as the above would be helpful.
Thanks.

Cheers
Jon

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Grygorii Strashko]

Hi Jon,

On 12/17/2015 12:48 PM, Jon Hunter wrote:
> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>

I've tried to test these patches with OMAP GPIO and I see it works, in general.
"In general" - because OMAP GPIO has some code which is expected to be used
very late during suspend or when entering deep CPUIdle states, so I can't use
this approach "out-of-the-box" until i find the way to sort it out.

Hope some one else can try to test it with GPIO. Soren?

> ---
>   include/linux/irq.h    |  4 ++++
>   kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>   kernel/irq/manage.c    |  7 +++++++
>   3 files changed, 35 insertions(+)
> 
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 3c1c96786248..7a61a7f76177 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h

...

>   
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2a429b061171..8a96e4f1e985 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>   	if (!try_module_get(desc->owner))
>   		return -ENODEV;
>   
> +	ret = chip_pm_get(desc);
> +	if (ret < 0)
> +		return ret;
> +
>   	new->irq = irq;
>   
>   	/*
> @@ -1400,6 +1404,7 @@ out_thread:
>   		put_task_struct(t);
>   	}
>   out_mput:
> +	chip_pm_put(desc);
>   	module_put(desc->owner);
>   	return ret;
>   }

Here I still think, that for this solution to be complete It might be good to add additional
API to request/free chained IRQs. This is not usual case for GPIO drivers,
but with AGIC it seems possible. If no objection I can do rfc.


> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>   		}
>   	}
>   
> +	chip_pm_put(desc);
>   	module_put(desc->owner);
>   	kfree(action->secondary);
>   	return action;
> @@ -1799,6 +1805,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
>   
>   	unregister_handler_proc(irq, action);
>   
> +	chip_pm_put(desc);
>   	module_put(desc->owner);
>   	return action;
>   
> 


-- 
regards,
-grygorii

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Sören Brinkmann]

On Tue, 2016-01-12 at 08:40PM +0200, Grygorii Strashko wrote:
> Hi Jon,
> 
> On 12/17/2015 12:48 PM, Jon Hunter wrote:
> > Some IRQ chips may be located in a power domain outside of the CPU
> > subsystem and hence will require device specific runtime power management.
> > In order to support such IRQ chips, add a pointer for a device structure
> > to the irq_chip structure, and if this pointer is populated by the IRQ
> > chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> > APIs for this chip will be called when an IRQ is requested/freed,
> > respectively.
> > 
> > Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> 
> I've tried to test these patches with OMAP GPIO and I see it works, in general.
> "In general" - because OMAP GPIO has some code which is expected to be used
> very late during suspend or when entering deep CPUIdle states, so I can't use
> this approach "out-of-the-box" until i find the way to sort it out.
> 
> Hope some one else can try to test it with GPIO. Soren?

I try to find the person who had the failing test that triggered my
involvement. That would basically just cover the case that the GPIO
controller is used as IRQ controller and needs to be enabled even if no
GPIO has been requested.
Suspend/resume may be more difficult. We have some out-of-tree code for
that. But unfortunately not 100% stable. I'll check what happens and
report back.

	Thanks,
	Sören

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Ulf Hansson]

+linux-pm, Rafael

On 17 December 2015 at 11:48, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Some IRQ chips may be located in a power domain outside of the CPU
> subsystem and hence will require device specific runtime power management.
> In order to support such IRQ chips, add a pointer for a device structure
> to the irq_chip structure, and if this pointer is populated by the IRQ
> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
> APIs for this chip will be called when an IRQ is requested/freed,
> respectively.

Overall I like the idea of this patch(set), as it will allow us to
save power for "unused" irqchips.

>
> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  include/linux/irq.h    |  4 ++++
>  kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>  kernel/irq/manage.c    |  7 +++++++
>  3 files changed, 35 insertions(+)
>
> diff --git a/include/linux/irq.h b/include/linux/irq.h
> index 3c1c96786248..7a61a7f76177 100644
> --- a/include/linux/irq.h
> +++ b/include/linux/irq.h
> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>  /**
>   * struct irq_chip - hardware interrupt chip descriptor
>   *
> + * @dev:               pointer to associated device
>   * @name:              name for /proc/interrupts
>   * @irq_startup:       start up the interrupt (defaults to ->enable if NULL)
>   * @irq_shutdown:      shut down the interrupt (defaults to ->disable if NULL)
> @@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>   * @flags:             chip specific flags
>   */
>  struct irq_chip {
> +       struct device   *dev;
>         const char      *name;
>         unsigned int    (*irq_startup)(struct irq_data *data);
>         void            (*irq_shutdown)(struct irq_data *data);
> @@ -399,6 +401,7 @@ struct irq_chip {
>   * IRQCHIP_SKIP_SET_WAKE:      Skip chip.irq_set_wake(), for this irq chip
>   * IRQCHIP_ONESHOT_SAFE:       One shot does not require mask/unmask
>   * IRQCHIP_EOI_THREADED:       Chip requires eoi() on unmask in threaded mode
> + * IRQCHIP_HAS_PM:             Chip requires runtime power management

Perhaps we don't need to add a specific flag for this, but instead
just check if the ->dev pointer has been assigned and then perform
runtime PM management?

>   */
>  enum {
>         IRQCHIP_SET_TYPE_MASKED         = (1 <<  0),
> @@ -408,6 +411,7 @@ enum {
>         IRQCHIP_SKIP_SET_WAKE           = (1 <<  4),
>         IRQCHIP_ONESHOT_SAFE            = (1 <<  5),
>         IRQCHIP_EOI_THREADED            = (1 <<  6),
> +       IRQCHIP_HAS_RPM                 = (1 <<  7),
>  };
>
>  #include <linux/irqdesc.h>
> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
> index fcab63c66905..30a2add7cae6 100644
> --- a/kernel/irq/internals.h
> +++ b/kernel/irq/internals.h
> @@ -7,6 +7,7 @@
>   */
>  #include <linux/irqdesc.h>
>  #include <linux/kernel_stat.h>
> +#include <linux/pm_runtime.h>
>
>  #ifdef CONFIG_SPARSE_IRQ
>  # define IRQ_BITMAP_BITS       (NR_IRQS + 8196)
> @@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
>                 desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
>  }
>
> +/* Inline functions for support of irq chips that require runtime pm */
> +static inline int chip_pm_get(struct irq_desc *desc)

Why does these new get/put functions need to be inline functions and
thus defined in the header file? Perhaps move them to manage.c are
better?

> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;
> +}
> +
> +static inline int chip_pm_put(struct irq_desc *desc)
> +{
> +       int retval = 0;
> +
> +       if (desc->irq_data.chip->dev &&
> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
> +               retval = pm_runtime_put(desc->irq_data.chip->dev);
> +
> +       return (retval < 0) ? retval : 0;

This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
would return -ENOSYS. In such cases I guess you would like to ignore
the error!?

> +}
> +
>  #define _IRQ_DESC_CHECK                (1 << 0)
>  #define _IRQ_DESC_PERCPU       (1 << 1)
>
> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
> index 2a429b061171..8a96e4f1e985 100644
> --- a/kernel/irq/manage.c
> +++ b/kernel/irq/manage.c
> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>         if (!try_module_get(desc->owner))
>                 return -ENODEV;
>
> +       ret = chip_pm_get(desc);
> +       if (ret < 0)
> +               return ret;
> +
>         new->irq = irq;
>
>         /*
> @@ -1400,6 +1404,7 @@ out_thread:
>                 put_task_struct(t);
>         }
>  out_mput:
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         return ret;
>  }
> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>                 }
>         }

I don't think using __free_irq() is the correct place to decrease the
runtime PM usage count. It will keep the irqchip runtime resumed even
if there are no irqs enabled for it.

Instead I would rather allow the irqchip to be runtime suspended, when
there are no irqs enabled on it.

Therefore you should rather use __enable|disable_irq() from where you
increase/decrease the runtime PM usage count.

Although, I realize that may become a bit troublesome as in some of
the execution paths where these functions are invoked, are done while
holding a spinlock with irqs disabled. Invoking pm_runtime_get_sync()
thus leads to that the irqchip's runtime PM callbacks needs to be
irqsafe. Another option is to somehow make use the asynchronous API;
pm_runtime_get() instead.

>
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         kfree(action->secondary);
>         return action;
> @@ -1799,6 +1805,7 @@ static struct irqaction *__free_percpu_irq(unsigned int irq, void __percpu *dev_
>
>         unregister_handler_proc(irq, action);
>
> +       chip_pm_put(desc);
>         module_put(desc->owner);
>         return action;

Kind regards
Uffe

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

On 18/01/16 14:47, Ulf Hansson wrote:
> +linux-pm, Rafael
> 
> On 17 December 2015 at 11:48, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Some IRQ chips may be located in a power domain outside of the CPU
>> subsystem and hence will require device specific runtime power management.
>> In order to support such IRQ chips, add a pointer for a device structure
>> to the irq_chip structure, and if this pointer is populated by the IRQ
>> chip driver and the flag CHIP_HAS_RPM is set, then the pm_runtime_get/put
>> APIs for this chip will be called when an IRQ is requested/freed,
>> respectively.
> 
> Overall I like the idea of this patch(set), as it will allow us to
> save power for "unused" irqchips.

Great, thanks.

>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  include/linux/irq.h    |  4 ++++
>>  kernel/irq/internals.h | 24 ++++++++++++++++++++++++
>>  kernel/irq/manage.c    |  7 +++++++
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/include/linux/irq.h b/include/linux/irq.h
>> index 3c1c96786248..7a61a7f76177 100644
>> --- a/include/linux/irq.h
>> +++ b/include/linux/irq.h
>> @@ -307,6 +307,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>  /**
>>   * struct irq_chip - hardware interrupt chip descriptor
>>   *
>> + * @dev:               pointer to associated device
>>   * @name:              name for /proc/interrupts
>>   * @irq_startup:       start up the interrupt (defaults to ->enable if NULL)
>>   * @irq_shutdown:      shut down the interrupt (defaults to ->disable if NULL)
>> @@ -344,6 +345,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d)
>>   * @flags:             chip specific flags
>>   */
>>  struct irq_chip {
>> +       struct device   *dev;
>>         const char      *name;
>>         unsigned int    (*irq_startup)(struct irq_data *data);
>>         void            (*irq_shutdown)(struct irq_data *data);
>> @@ -399,6 +401,7 @@ struct irq_chip {
>>   * IRQCHIP_SKIP_SET_WAKE:      Skip chip.irq_set_wake(), for this irq chip
>>   * IRQCHIP_ONESHOT_SAFE:       One shot does not require mask/unmask
>>   * IRQCHIP_EOI_THREADED:       Chip requires eoi() on unmask in threaded mode
>> + * IRQCHIP_HAS_PM:             Chip requires runtime power management
> 
> Perhaps we don't need to add a specific flag for this, but instead
> just check if the ->dev pointer has been assigned and then perform
> runtime PM management?

Yes it may not be necessary. However, I was not sure if someone would
make use of the dev structure but not use RPM. For now we could drop it.

>>   */
>>  enum {
>>         IRQCHIP_SET_TYPE_MASKED         = (1 <<  0),
>> @@ -408,6 +411,7 @@ enum {
>>         IRQCHIP_SKIP_SET_WAKE           = (1 <<  4),
>>         IRQCHIP_ONESHOT_SAFE            = (1 <<  5),
>>         IRQCHIP_EOI_THREADED            = (1 <<  6),
>> +       IRQCHIP_HAS_RPM                 = (1 <<  7),
>>  };
>>
>>  #include <linux/irqdesc.h>
>> diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
>> index fcab63c66905..30a2add7cae6 100644
>> --- a/kernel/irq/internals.h
>> +++ b/kernel/irq/internals.h
>> @@ -7,6 +7,7 @@
>>   */
>>  #include <linux/irqdesc.h>
>>  #include <linux/kernel_stat.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #ifdef CONFIG_SPARSE_IRQ
>>  # define IRQ_BITMAP_BITS       (NR_IRQS + 8196)
>> @@ -125,6 +126,29 @@ static inline void chip_bus_sync_unlock(struct irq_desc *desc)
>>                 desc->irq_data.chip->irq_bus_sync_unlock(&desc->irq_data);
>>  }
>>
>> +/* Inline functions for support of irq chips that require runtime pm */
>> +static inline int chip_pm_get(struct irq_desc *desc)
> 
> Why does these new get/put functions need to be inline functions and
> thus defined in the header file? Perhaps move them to manage.c are
> better?

They don't have to be, and so I can move them.

>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_get_sync(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
>> +}
>> +
>> +static inline int chip_pm_put(struct irq_desc *desc)
>> +{
>> +       int retval = 0;
>> +
>> +       if (desc->irq_data.chip->dev &&
>> +           desc->irq_data.chip->flags & IRQCHIP_HAS_RPM)
>> +               retval = pm_runtime_put(desc->irq_data.chip->dev);
>> +
>> +       return (retval < 0) ? retval : 0;
> 
> This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
> would return -ENOSYS. In such cases I guess you would like to ignore
> the error!?

Ok, yes good point.

>> +}
>> +
>>  #define _IRQ_DESC_CHECK                (1 << 0)
>>  #define _IRQ_DESC_PERCPU       (1 << 1)
>>
>> diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
>> index 2a429b061171..8a96e4f1e985 100644
>> --- a/kernel/irq/manage.c
>> +++ b/kernel/irq/manage.c
>> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>         if (!try_module_get(desc->owner))
>>                 return -ENODEV;
>>
>> +       ret = chip_pm_get(desc);
>> +       if (ret < 0)
>> +               return ret;
>> +
>>         new->irq = irq;
>>
>>         /*
>> @@ -1400,6 +1404,7 @@ out_thread:
>>                 put_task_struct(t);
>>         }
>>  out_mput:
>> +       chip_pm_put(desc);
>>         module_put(desc->owner);
>>         return ret;
>>  }
>> @@ -1513,6 +1518,7 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>>                 }
>>         }
> 
> I don't think using __free_irq() is the correct place to decrease the
> runtime PM usage count. It will keep the irqchip runtime resumed even
> if there are no irqs enabled for it.
> 
> Instead I would rather allow the irqchip to be runtime suspended, when
> there are no irqs enabled on it.
> 
> Therefore you should rather use __enable|disable_irq() from where you
> increase/decrease the runtime PM usage count.
> 
> Although, I realize that may become a bit troublesome as in some of
> the execution paths where these functions are invoked, are done while
> holding a spinlock with irqs disabled. Invoking pm_runtime_get_sync()
> thus leads to that the irqchip's runtime PM callbacks needs to be
> irqsafe. Another option is to somehow make use the asynchronous API;
> pm_runtime_get() instead.

So that would be ideal, however, I don't think it is that trivial and
hence this is why I have not done this for now.

I don't think that the async callbacks will really help here because if
you call enable_irq() and the chip is runtime suspended, you need to
wait for the chip to resume before you can enable the IRQ. I think that
the disable path would be ok, but not the enable. Plus there could be
some "hot" paths where enable/disable are used and I did not want to
make any assumptions about these.

This may appear ugly, but for something like this, we may need to have a
separate enable/disable API, such as
enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
suspend/resume the chip and must not be used in critical sections.

I was hoping that we could get some initially functionality in place to
allow some basic support for these chips and then enhance it later.

Cheers
Jon

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Thomas Gleixner]

On Tue, 19 Jan 2016, Jon Hunter wrote:
> On 18/01/16 14:47, Ulf Hansson wrote:
> >> +/* Inline functions for support of irq chips that require runtime pm */
> >> +static inline int chip_pm_get(struct irq_desc *desc)
> > 
> > Why does these new get/put functions need to be inline functions and
> > thus defined in the header file? Perhaps move them to manage.c are
> > better?
> 
> They don't have to be, and so I can move them.

Yes, please make them proper functions. The proper place for them is chip.c
 
> > This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
> > would return -ENOSYS. In such cases I guess you would like to ignore
> > the error!?
> 
> Ok, yes good point.

So you need a CONFIG_PM variant and stubs which return 0 for the !PM case.
 
> >> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
> >>         if (!try_module_get(desc->owner))
> >>                 return -ENODEV;
> >>
> >> +       ret = chip_pm_get(desc);
> >> +       if (ret < 0)
> >> +               return ret;

That leaks the module refcount.

> > I don't think using __free_irq() is the correct place to decrease the
> > runtime PM usage count. It will keep the irqchip runtime resumed even
> > if there are no irqs enabled for it.
> >
> > Instead I would rather allow the irqchip to be runtime suspended, when
> > there are no irqs enabled on it.

Which is a no no, as you might lose interrupts that way. We disable interrupts
lazy, i.e. we do not mask them. So no, you cannot do that from
enable/disable_irq().
 
> This may appear ugly, but for something like this, we may need to have a
> separate enable/disable API, such as
> enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
> suspend/resume the chip and must not be used in critical sections.

enable_irq_lazy is a misnomer. enable_irq_pm or such might be acceptable.

But before we go there I really want to see a proper use case for such
functions.

Thanks,

	tglx

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Jon Hunter]

On 20/01/16 15:30, Thomas Gleixner wrote:
> On Tue, 19 Jan 2016, Jon Hunter wrote:
>> On 18/01/16 14:47, Ulf Hansson wrote:
>>>> +/* Inline functions for support of irq chips that require runtime pm */
>>>> +static inline int chip_pm_get(struct irq_desc *desc)
>>>
>>> Why does these new get/put functions need to be inline functions and
>>> thus defined in the header file? Perhaps move them to manage.c are
>>> better?
>>
>> They don't have to be, and so I can move them.
> 
> Yes, please make them proper functions. The proper place for them is chip.c
>  
>>> This won't play nicely when CONFIG_PM is unset, as pm_runtime_put()
>>> would return -ENOSYS. In such cases I guess you would like to ignore
>>> the error!?
>>
>> Ok, yes good point.
> 
> So you need a CONFIG_PM variant and stubs which return 0 for the !PM case.
>  
>>>> @@ -1116,6 +1116,10 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
>>>>         if (!try_module_get(desc->owner))
>>>>                 return -ENODEV;
>>>>
>>>> +       ret = chip_pm_get(desc);
>>>> +       if (ret < 0)
>>>> +               return ret;
> 
> That leaks the module refcount.

Ok, I will fix that.

>>> I don't think using __free_irq() is the correct place to decrease the
>>> runtime PM usage count. It will keep the irqchip runtime resumed even
>>> if there are no irqs enabled for it.
>>>
>>> Instead I would rather allow the irqchip to be runtime suspended, when
>>> there are no irqs enabled on it.
> 
> Which is a no no, as you might lose interrupts that way. We disable interrupts
> lazy, i.e. we do not mask them. So no, you cannot do that from
> enable/disable_irq().
>  
>> This may appear ugly, but for something like this, we may need to have a
>> separate enable/disable API, such as
>> enable_irq_lazy()/disable_irq_lazy() which could be used to runtime
>> suspend/resume the chip and must not be used in critical sections.
> 
> enable_irq_lazy is a misnomer. enable_irq_pm or such might be acceptable.

That's fine with me.

> But before we go there I really want to see a proper use case for such
> functions.

Ok, that makes sense.

Cheers
Jon

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Ulf Hansson]

[...]

>> > I don't think using __free_irq() is the correct place to decrease the
>> > runtime PM usage count. It will keep the irqchip runtime resumed even
>> > if there are no irqs enabled for it.
>> >
>> > Instead I would rather allow the irqchip to be runtime suspended, when
>> > there are no irqs enabled on it.
>
> Which is a no no, as you might lose interrupts that way. We disable interrupts
> lazy, i.e. we do not mask them. So no, you cannot do that from
> enable/disable_irq().

Thanks for the input!

My main point around the approach suggested in $subject patch, is that
I don't think it's *enough* fine grained.

>From a runtime PM perspective, we should allow the irqchip to enter a
low power state when there are no IRQs enabled for it.

As enable|disable_irq() isn't the place to do runtime PM reference
counting from, perhaps there is another option? Maybe the irqchip
driver itself is better suited to take these decisions?

[...]

Kind regards
Uffe

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Thomas Gleixner]

On Thu, 21 Jan 2016, Ulf Hansson wrote:
> >> > I don't think using __free_irq() is the correct place to decrease the
> >> > runtime PM usage count. It will keep the irqchip runtime resumed even
> >> > if there are no irqs enabled for it.
> >> >
> >> > Instead I would rather allow the irqchip to be runtime suspended, when
> >> > there are no irqs enabled on it.
> >
> > Which is a no no, as you might lose interrupts that way. We disable interrupts
> > lazy, i.e. we do not mask them. So no, you cannot do that from
> > enable/disable_irq().
> 
> Thanks for the input!
> 
> My main point around the approach suggested in $subject patch, is that
> I don't think it's *enough* fine grained.
> 
> From a runtime PM perspective, we should allow the irqchip to enter a
> low power state when there are no IRQs enabled for it.
> 
> As enable|disable_irq() isn't the place to do runtime PM reference
> counting from, perhaps there is another option? Maybe the irqchip
> driver itself is better suited to take these decisions?

The irq chip driver does not have more information than the core code,
actually it has less.

Let's look at disable_irq(). It's normaly used for short periods of time where
a driver needs to make sure that the interrupt is not handled. That's hardly a
point to do power management, because its going to be reenabled right
away. That's one of the reasons for doing the lazy masking, though the main
reason is to prevent losing interrupt on edge driven irq lines.

So as long as an interrupt handler is installed, there is no sane way that we
can decide to power down the irq chip, unless that chip has the magic ability
to relay incoming interrupts while powered down :)

There is also the issue with shared interrupts....

So the only sane way to power down the irq chip is to remove the handlers when
the device is not in use. Actually some of the subsystems/drivers do that
already.

Though there might be a case where the device driver is still in use, but it
has brought down the device into a quiescent state, which guarantees that no
interrupts happen unless its powered up again. That would be an opportunity to
shut down the interrupt and do power management as well. So currently we only
have the option of freeing the interrupt and requesting it again.

It might be conveniant to avoid that by having extra functions which are less
heavyweight. Something like irq_[de]activate(irq, void *dev_id), which would
mask/unmask the interrupt and do power management on the irq chip.

If you can come up with a simple comparision of such an approach with the
free/request_irq() method which shows that it is superior, I'm surely not in
the way. No need to code that core part, just show how a few drivers would use
those functions and why this is less intrusive and simpler to use for the
developer that going for the free/request() solution.

Thanks,

	tglx

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Ulf Hansson]

On 21 January 2016 at 20:51, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> On Thu, 21 Jan 2016, Ulf Hansson wrote:
>> >> > I don't think using __free_irq() is the correct place to decrease the
>> >> > runtime PM usage count. It will keep the irqchip runtime resumed even
>> >> > if there are no irqs enabled for it.
>> >> >
>> >> > Instead I would rather allow the irqchip to be runtime suspended, when
>> >> > there are no irqs enabled on it.
>> >
>> > Which is a no no, as you might lose interrupts that way. We disable interrupts
>> > lazy, i.e. we do not mask them. So no, you cannot do that from
>> > enable/disable_irq().
>>
>> Thanks for the input!
>>
>> My main point around the approach suggested in $subject patch, is that
>> I don't think it's *enough* fine grained.
>>
>> From a runtime PM perspective, we should allow the irqchip to enter a
>> low power state when there are no IRQs enabled for it.
>>
>> As enable|disable_irq() isn't the place to do runtime PM reference
>> counting from, perhaps there is another option? Maybe the irqchip
>> driver itself is better suited to take these decisions?
>
> The irq chip driver does not have more information than the core code,
> actually it has less.
>
> Let's look at disable_irq(). It's normaly used for short periods of time where
> a driver needs to make sure that the interrupt is not handled. That's hardly a
> point to do power management, because its going to be reenabled right
> away. That's one of the reasons for doing the lazy masking, though the main
> reason is to prevent losing interrupt on edge driven irq lines.

I didn't think of the edge driven irq case.

I totally agree, disable|enable_irq() can't be used for power management.

>
> So as long as an interrupt handler is installed, there is no sane way that we
> can decide to power down the irq chip, unless that chip has the magic ability
> to relay incoming interrupts while powered down :)
>
> There is also the issue with shared interrupts....
>
> So the only sane way to power down the irq chip is to remove the handlers when
> the device is not in use. Actually some of the subsystems/drivers do that
> already.
>
> Though there might be a case where the device driver is still in use, but it
> has brought down the device into a quiescent state, which guarantees that no
> interrupts happen unless its powered up again. That would be an opportunity to
> shut down the interrupt and do power management as well. So currently we only
> have the option of freeing the interrupt and requesting it again.

I agree, this is exactly the case I have been thinking of.

Closely related to this topic.

Currently there are several cases where subsystems/drivers don't
protects themselves from receiving an IRQ, when the device is in a
quiescent state.
This may lead to hangs at device register accesses etc, especially
when the IRQ handler expects the device to be functional and not in a
low power state.

This is a different issue, but the solution I had in mind was to use
disable|enable_irq() as protection. I now realize that
freeing/re-requesting the IRQ handler is better, as that would
potentially allow power management for the irqchip as well.

>
> It might be conveniant to avoid that by having extra functions which are less
> heavyweight. Something like irq_[de]activate(irq, void *dev_id), which would
> mask/unmask the interrupt and do power management on the irq chip.

I like that idea!

We may also want to distinguish between the PM case and by just having
an IRQ handler registered, for whatever reason.

More importantly, we don't want to introduce unnecessary latencies
when bringing devices back to full power from a quiescent power state.

Perhaps the functions should be named something with "pm" to better
reflect their purpose?

>
> If you can come up with a simple comparision of such an approach with the
> free/request_irq() method which shows that it is superior, I'm surely not in
> the way. No need to code that core part, just show how a few drivers would use
> those functions and why this is less intrusive and simpler to use for the
> developer that going for the free/request() solution.

Here's a small collection of drivers that I easily picked up as
candidates for using these new APIs.
In principle, they would invoke these new APIs from their runtime PM callbacks.

drivers/spi/spi-atmel.c
drivers/spi/spi-pl022.c
drivers/i2c/busses/i2c-omap.c
drivers/i2c/busses/i2c-nomadik.c
drivers/i2c/busses/i2c-sh_mobile.c
drivers/mmc/host/mtk-sd.c
drivers/mmc/host/mmci.c

Kind regards
Uffe

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Thomas Gleixner]

On Fri, 22 Jan 2016, Ulf Hansson wrote:
> Here's a small collection of drivers that I easily picked up as
> candidates for using these new APIs.
> In principle, they would invoke these new APIs from their runtime PM callbacks.
> 
> drivers/spi/spi-atmel.c
> drivers/spi/spi-pl022.c
> drivers/i2c/busses/i2c-omap.c
> drivers/i2c/busses/i2c-nomadik.c
> drivers/i2c/busses/i2c-sh_mobile.c
> drivers/mmc/host/mtk-sd.c
> drivers/mmc/host/mmci.c

Instead of adding those calls to each driver, we can be smart and flag the
interrupt as AUTO_RUNTIME_SUSPEND or such.

So the runtime_pm core can handle it when invoking the
dev_pm_ops->runtime_suspend()/resume() callbacks.

Unfortunately the devres stuff is exceptionally bad to be used for this, but
with some surgery it should be doable.

Thanks,

	tglx

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Linus Walleij]

On Thu, Jan 21, 2016 at 8:51 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:

> So as long as an interrupt handler is installed, there is no sane way that we
> can decide to power down the irq chip, unless that chip has the magic ability
> to relay incoming interrupts while powered down :)

Actually isn't that exactly what almost every SoC that supports
deepsleep does?

They power off the primary interrupt controller and arm the padring
of the SoC with an asynchronous edge detector to wake up as soon
as something happens on a few select lines, like a keypad button
or whatnot.

The asynchronous edge detector is handled by the ROM or some
power-management microcontroller, which wakes up the system
and restores power to the CPU and primary interrupt controller.
That is the magic ability right there.

Of course as the wakeup signal may be deasserted at the
point the system actually comes back up, so the magic ROM
power management unit then needs to latch
any latent IRQs from some shadow register to the primary interrupt
controller, which as far as I've seen is done by out-of-tree hacks
similar to the irq_[get/set]_irqchip_state() implemented
by Marc Zyngier, albeit for virtualization.

I've not seen it on any non-primary interrupt controller though.

Yours,
Linus Walleij

Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips

[Grygorii Strashko]

On 02/05/2016 04:37 PM, Linus Walleij wrote:
> On Thu, Jan 21, 2016 at 8:51 PM, Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org> wrote:
> 
>> So as long as an interrupt handler is installed, there is no sane way that we
>> can decide to power down the irq chip, unless that chip has the magic ability
>> to relay incoming interrupts while powered down :)
> 
> Actually isn't that exactly what almost every SoC that supports
> deepsleep does?
> 
> They power off the primary interrupt controller and arm the padring
> of the SoC with an asynchronous edge detector to wake up as soon
> as something happens on a few select lines, like a keypad button
> or whatnot.
> 
> The asynchronous edge detector is handled by the ROM or some
> power-management microcontroller, which wakes up the system
> and restores power to the CPU and primary interrupt controller.
> That is the magic ability right there.
> 
> Of course as the wakeup signal may be deasserted at the
> point the system actually comes back up, so the magic ROM
> power management unit then needs to latch
> any latent IRQs from some shadow register to the primary interrupt
> controller, which as far as I've seen is done by out-of-tree hacks
> similar to the irq_[get/set]_irqchip_state() implemented
> by Marc Zyngier, albeit for virtualization.
> 
> I've not seen it on any non-primary interrupt controller though.
> 

OMAP gpio can lose context(Power) even if there
GPIO IRQs configured as wakeup sources (wakeup happen through the SoC specific HW
responsible for wakeup in this case) - pcs.

It also injects IRQs if their triggering type is not fully compatible with PM HW.  

-- 
regards,
-grygorii