| Message ID | 1448644860-29323-2-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Nov 27, 2015 at 6:21 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > From: Sudeep Holla <Sudeep.Holla@arm.com> > > The IRQF_NO_SUSPEND flag is used to identify the interrupts that should > be left enabled so as to allow them to work as expected during the > suspend-resume cycle, but doesn't guarantee that it will wake the system > from a suspended state, enable_irq_wake is recommended to be used for > the wakeup. > > This patch removes the use of IRQF_NO_SUSPEND flags replacing it with > irq_set_irq_wake instead. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: linux-gpio@vger.kernel.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> I need Tony's ACK on this as well. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12/15 11:18, Grygorii Strashko wrote: > On 12/04/2015 12:54 PM, Sudeep Holla wrote: >> Hi Grygorii, >> >> On 04/12/15 10:44, Grygorii Strashko wrote: >>> On 12/03/2015 11:37 PM, Tony Lindgren wrote: >> >> [...] >> >>>> And these both need to be applied together when we have a fix for the >>>> above >>>> as otherwise we'll get the lock recursion Sudeep mentioned in patch 2/2. >>>> >>> >>> Most probably below diff will fix above issue: >>> >>> diff --git a/arch/arm/mach-omap2/prm_common.c >>> b/arch/arm/mach-omap2/prm_common.c >>> index 3fc2cbe..69cde67 100644 >>> --- a/arch/arm/mach-omap2/prm_common.c >>> +++ b/arch/arm/mach-omap2/prm_common.c >>> @@ -338,6 +338,7 @@ int omap_prcm_register_chain_handler(struct >>> omap_prcm_irq_setup *irq_setup) >>> ct->chip.irq_ack = irq_gc_ack_set_bit; >>> ct->chip.irq_mask = irq_gc_mask_clr_bit; >>> ct->chip.irq_unmask = irq_gc_mask_set_bit; >>> + ct->chip.flags = IRQCHIP_SKIP_SET_WAKE; >> >> Thanks for testing. > > Sry, I've not tested it yet - it's just fast assumption :( > OK, no worries. >> In that case without this hunk, we should get error >> from pcs_irq_set_wake in the suspend path. No ? May be driver is not >> checking the error value and entering suspend. >> > > Yep. Noone is checking return result from enable_irq_wake() in suspend path > (see dev_pm_arm_wake_irq()). > True, but one possible reason for the warning Tony posted. > Actually, return result of enable_irq_wake() is checked only in ~30% of > cases in kernel now :) > That's bad, but I admit that even I failed to add check in some of the patches I posted earlier. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12/15 15:40, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [151203 13:41]: >> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]: >>> >>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake >>> which ensures it's marked for wakeup. >> >> Hmm well see the error I pasted in this thread, maybe that provides >> more clues. > > The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not > look right to me as pcs_irq_set_wake toggles the irq_wake for each pin > separately, not for the whole controller. > OK, my understanding was that this driver supports multiple single pinmux with one main irq `pcs_soc->irq`. Hence I added the wakeup on that irq. I now think that understand is wrong. > I think all that can be left out with the snipped from Grygorii, and maybe > also the lock_class_key changes. OK -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tony, On 04/12/15 15:40, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [151203 13:41]: >> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]: >>> >>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake >>> which ensures it's marked for wakeup. >> >> Hmm well see the error I pasted in this thread, maybe that provides >> more clues. > > The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not > look right to me as pcs_irq_set_wake toggles the irq_wake for each pin > separately, not for the whole controller. > After thinking more about it we need some way to tell IRQ core that pcs_soc->irq is wakeup capable. Is that going to happen automatically via dev_pm_set_dedicated_wake_irq as you mentioned earlier ? > I think all that can be left out with the snipped from Grygorii, and maybe > also the lock_class_key changes. > If we not calling irq_set_irq_wake(pcs_soc->irq) in pcs_irq_set_wake, do you see possibility of lockdep recursion in any other paths. Otherwise we don't need this if we remove irq_set_irq_wake(pcs_soc->irq) from pcs_irq_set_wake -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12/15 16:19, Grygorii Strashko wrote: > On 12/04/2015 05:44 PM, Sudeep Holla wrote: >> >> >> On 04/12/15 15:40, Tony Lindgren wrote: >>> * Tony Lindgren <tony@atomide.com> [151203 13:41]: >>>> * Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]: >>>>> >>>>> I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake >>>>> which ensures it's marked for wakeup. >>>> >>>> Hmm well see the error I pasted in this thread, maybe that provides >>>> more clues. >>> >>> The irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake does not >>> look right to me as pcs_irq_set_wake toggles the irq_wake for each pin >>> separately, not for the whole controller. >>> >> >> OK, my understanding was that this driver supports multiple single >> pinmux with one main irq `pcs_soc->irq`. Hence I added the wakeup on >> that irq. I now think that understand is wrong. >> > > With this change, PCS parent IRQ will be marked as wake up source as many > times as many pins were requested as wake up IRQs (protected by counter). > Most of all GPIO IRQ chips work this way. > Of course, if we will look on pinctrl-single.c from only OMAP point of view > then Prent IRQ can be marked as wake up source from probe only once. > But, since this driver expected to be generic - this patch is more correct, > because other HW may require to perform some real HW re-configuration to > enable/disable wake up capabilities for Parent IRQ in Parent IRQ controller. > Thanks for the detailed explanation. I was bit confused if my understanding is correct or not. > Any way, in my opinion, it's right and more safe to manage all wakeup IRQs > through IRQ PM core and Device wakeirq framework. And this patch should just > go together with platform changes and not alone. > Agreed, since I don't have platform to test, I will leave it you guys to pick up these patches when ready and with any changes if required. -- Regards, Sudeep -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index 945a7d0f0704..a1e32c6b554a 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -1622,12 +1622,14 @@ static void pcs_irq_unmask(struct irq_data *d) */ static int pcs_irq_set_wake(struct irq_data *d, unsigned int state) { + struct pcs_soc_data *pcs_soc = irq_data_get_irq_chip_data(d); + if (state) pcs_irq_unmask(d); else pcs_irq_mask(d); - return 0; + return irq_set_irq_wake(pcs_soc->irq, state); } /** @@ -1763,8 +1765,7 @@ static int pcs_irq_init_chained_handler(struct pcs_device *pcs, int res; res = request_irq(pcs_soc->irq, pcs_irq_handler, - IRQF_SHARED | IRQF_NO_SUSPEND | - IRQF_NO_THREAD, + IRQF_SHARED | IRQF_NO_THREAD, name, pcs_soc); if (res) { pcs_soc->irq = -1;