Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932452AbcCRLMA (ORCPT ); Fri, 18 Mar 2016 07:12:00 -0400 Received: from bear.ext.ti.com ([192.94.94.41]:57683 "EHLO bear.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932290AbcCRLLr (ORCPT ); Fri, 18 Mar 2016 07:11:47 -0400 Subject: Re: [PATCH 08/15] genirq: Add runtime power management support for IRQ chips To: Jon Hunter , Thomas Gleixner , Jason Cooper , Marc Zyngier , =?UTF-8?Q?Beno=c3=aet_Cousson?= , Tony Lindgren , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Stephen Warren , Thierry Reding References: <1458224359-32665-1-git-send-email-jonathanh@nvidia.com> <1458224359-32665-9-git-send-email-jonathanh@nvidia.com> CC: Kevin Hilman , Geert Uytterhoeven , Lars-Peter Clausen , Linus Walleij , , , , From: Grygorii Strashko Message-ID: <56EBE244.6070400@ti.com> Date: Fri, 18 Mar 2016 13:11:00 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <1458224359-32665-9-git-send-email-jonathanh@nvidia.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4873 Lines: 130 Hi Jon, On 03/17/2016 04:19 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 CONFIG_PM is selected in the kernel > configuration, then the pm_runtime_get/put APIs for this chip will be > called when an IRQ is requested/freed, respectively. > > When entering system suspend and each interrupt is disabled if there is > no wake-up set for that interrupt. For an IRQ chip that utilises runtime > power management, print a warning message for each active interrupt that > has no wake-up set because these interrupts may be unnecessarily keeping > the IRQ chip enabled during system suspend. > > Signed-off-by: Jon Hunter > --- > include/linux/irq.h | 5 +++++ > kernel/irq/chip.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ > kernel/irq/internals.h | 1 + > kernel/irq/manage.c | 14 +++++++++++--- > kernel/irq/pm.c | 3 +++ > 5 files changed, 72 insertions(+), 3 deletions(-) > > diff --git a/include/linux/irq.h b/include/linux/irq.h > index c4de62348ff2..82f36390048d 100644 > --- a/include/linux/irq.h > +++ b/include/linux/irq.h > @@ -315,6 +315,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > /** > * struct irq_chip - hardware interrupt chip descriptor > * > + * @parent: 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) > @@ -354,6 +355,7 @@ static inline irq_hw_number_t irqd_to_hwirq(struct irq_data *d) > * @flags: chip specific flags > */ [..] > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c > index cea1de0161f1..ab436119084f 100644 > --- a/kernel/irq/pm.c > +++ b/kernel/irq/pm.c > @@ -83,6 +83,9 @@ static bool suspend_device_irq(struct irq_desc *desc) > * suspend_device_irqs(). > */ > return true; > + } else if (!irq_chip_pm_suspended(&desc->irq_data)) { > + pr_warn("irq %d has no wakeup set and has not been freed!\n", > + desc->irq_data.irq); Sry. But I did not get this part of the patch :( static bool suspend_device_irq(struct irq_desc *desc) { if (!desc->action || irq_desc_is_chained(desc) || desc->no_suspend_depth) { pr_err("skip irq %d\n", irq_desc_get_irq(desc)); return false; } if (irqd_is_wakeup_set(&desc->irq_data)) { irqd_set(&desc->irq_data, IRQD_WAKEUP_ARMED); /* * We return true here to force the caller to issue * synchronize_irq(). We need to make sure that the * IRQD_WAKEUP_ARMED is visible before we return from * suspend_device_irqs(). */ pr_err("wakeup irq %d\n", irq_desc_get_irq(desc)); return true; } ^^^^ Here you've added a warning desc->istate |= IRQS_SUSPENDED; __disable_irq(desc); ^^^^ Here non wakeup IRQs will be disabled pr_err("%s __disable_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); /* * Hardware which has no wakeup source configuration facility * requires that the non wakeup interrupts are masked at the * chip level. The chip implementation indicates that with * IRQCHIP_MASK_ON_SUSPEND. */ if (irq_desc_get_chip(desc)->flags & IRQCHIP_MASK_ON_SUSPEND) { mask_irq(desc); pr_err("%s mask_irq irq %d\n", irq_desc_get_chip(desc)->name, irq_desc_get_irq(desc)); } return true; } As result, there should be a ton of warnings if one IRQ (for example in GPIO irqchip) is wakeup source, but all other are not. Also I'd like to note that: - it is not expected that any IRQs have to be freed on enter Suspend - Primary interrupt controller is expected to be suspended from syscore_suspend() - not Primary interrupt controllers may be Suspended from: -- dpm_suspend() or dpm_suspend_late() - usual case for external interrupt controllers GPIO expanders (I2C, SPI ..) -- dpm_suspend_noirq() - usual case for SoC peripherals like OMAP GPIO dpm_suspend_noirq |- suspend_device_irqs() |- device_suspend_noirq() [1] <-- OMAP GPIO do suspend here. -- as always, some arches/maches may require hacks in platform code. So, In my opinion, suspend has to be handled by each irqchip driver separately, most probably at suspend_noirq level [1], because only irqchip driver now sees a full picture and knows if it can suspend or not, and when, and how. (may require to use pm_runtime_force_suspend/resume()). I propose do not touch common/generic suspend code now. Any common code can be always refactored later once there will be real drivers updated to use irqchip RPM and which will support Suspend. -- regards, -grygorii