Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1293758 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH V2 3/8] genirq: Add runtime power management support for IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2015-12-17 12:00 +0100
    Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Linus Walleij <linus.walleij@linaro.org> - 2015-12-17 14:20 +0100
      Re: [RFC PATCH V2 3/8] genirq: Add runtime power management support  for IRQ chips Jon Hunter <jonathanh@nvidia.com> - 2015-12-18 11:30 +0100

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

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>
---
 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
  */
 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)
+{
+	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;
+}
+
 #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)
 		}
 	}
 
+	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;
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web