| 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
* Linus Walleij <linus.walleij@linaro.org> [151201 06:07]: > 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. At least on omaps, this controller is always powered and we never want to suspend it as it handles wake-up events for all the IO pins. And that usecase sounds exactly like what you're describing above. I don't quite follow what your suggested alternative for an interrupt controller is? At least we need to have the alternative patched in with this chage before just removing IRQF_NO_SUSPEND. The enable_irq_wake is naturally used for the consumer drivers of this interrupt controller and actually mostly done automatically now with the dev_pm_set_dedicated_wake_irq. Regards, Tony -- 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 12/03/2015 08:13 PM, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [151201 06:07]: >> 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. > > At least on omaps, this controller is always powered and we never want to > suspend it as it handles wake-up events for all the IO pins. And that > usecase sounds exactly like what you're describing above. > > I don't quite follow what your suggested alternative for an interrupt > controller is? > > At least we need to have the alternative patched in with this chage before > just removing IRQF_NO_SUSPEND. > > The enable_irq_wake is naturally used for the consumer drivers of this > interrupt controller and actually mostly done automatically now with the > dev_pm_set_dedicated_wake_irq. > I think, this patch should not break our wake-up functionality. It will just change the moment when pcs_irq_handler() will be called: before this change: - suspend_enter() .... - arch_suspend_enable_irqs(); - ^ right here after this change: - suspend_enter() .... dpm_resume_noirq() - resume_device_irqs() ^ here Correct? And as for me this is more safe.
On 03/12/15 18:13, Tony Lindgren wrote: > * Linus Walleij <linus.walleij@linaro.org> [151201 06:07]: >> 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. > > At least on omaps, this controller is always powered and we never want to > suspend it as it handles wake-up events for all the IO pins. And that > usecase sounds exactly like what you're describing above. > Understood, but I assume this is a generic driver that can be used by any pinmux. > I don't quite follow what your suggested alternative for an interrupt > controller is? > Why can't we use enable_irq_wake even for parent/interrupt controller as they can be considered as parent wakeup irq. I agree the interrupt controller may not be powered down, but still it's part of wakeup and the irq core needs to identify that. By just marking IRQF_NO_SUSPEND, you are saying that you can handle interrupt in the suspend path but not informing that it's a wakeup interrupt. With this change, the wakeup handler (including the parent handler) is called when it's safe as the irq core maintains the state machine. > At least we need to have the alternative patched in with this chage before > just removing IRQF_NO_SUSPEND. > I have added irq_set_irq_wake(pcs_soc->irq, state) in pcs_irq_set_wake which ensures it's marked for wakeup. > The enable_irq_wake is naturally used for the consumer drivers of this > interrupt controller and actually mostly done automatically now with the > dev_pm_set_dedicated_wake_irq. > Agreed, no doubt on that.
* Grygorii Strashko <grygorii.strashko@ti.com> [151203 10:36]: > > I think, this patch should not break our wake-up functionality. > It will just change the moment when pcs_irq_handler() will be called: > > before this change: > - suspend_enter() > .... > - arch_suspend_enable_irqs(); > - ^ right here > > after this change: > - suspend_enter() > .... > dpm_resume_noirq() > - resume_device_irqs() > ^ here > > Correct? And as for me this is more safe. I think there's more to it though. With both applied, it produces this on coming back up from suspend: PM: noirq resume of devices complete after 18.127 msecs ------------[ cut here ]------------ WARNING: CPU: 0 PID: 123 at kernel/irq/manage.c:605 irq_set_irq_wake+0xbc/0xfc() Unbalanced IRQ 375 wake disable Modules linked in: ledtrig_default_on leds_gpio led_class rtc_twl twl4030_wdt CPU: 0 PID: 123 Comm: bash Tainted: G W 4.4.0-rc3-dirty #2682 Hardware name: Generic OMAP36xx (Flattened Device Tree) [<c0017df0>] (unwind_backtrace) from [<c0014084>] (show_stack+0x10/0x14) <c0014084>] (show_stack) from [<c03492d0>] (dump_stack+0x84/0x9c) [<c03492d0>] (dump_stack) from [<c003ca2c>] (warn_slowpath_common+0x7c/0xb8) [<c003ca2c>] (warn_slowpath_common) from [<c003ca98>] (warn_slowpath_fmt+0x30/0x40) [<c003ca98>] (warn_slowpath_fmt) from [<c009b66c>] (irq_set_irq_wake+0xbc/0xfc) [<c009b66c>] (irq_set_irq_wake) from [<c03f0f1c>] (device_wakeup_disarm_wake_irqs+0x70/0x12c) [<c03f0f1c>] (device_wakeup_disarm_wake_irqs) from [<c03ee4ac>] (dpm_resume_noirq+0x20c/0x2e4) [<c03ee4ac>] (dpm_resume_noirq) from [<c0095e94>] (suspend_devices_and_enter+0x1e4/0x6bc) [<c0095e94>] (suspend_devices_and_enter) from [<c00966c4>] (pm_suspend+0x358/0x4b8) [<c00966c4>] (pm_suspend) from [<c0094fdc>] (state_store+0x64/0xb8) [<c0094fdc>] (state_store) from [<c034b46c>] (kobj_attr_store+0x14/0x20) [<c034b46c>] (kobj_attr_store) from [<c01ea4d8>] (sysfs_kf_write+0x4c/0x50) [<c01ea4d8>] (sysfs_kf_write) from [<c01e9afc>] (kernfs_fop_write+0xbc/0x1cc) [<c01e9afc>] (kernfs_fop_write) from [<c0171c7c>] (__vfs_write+0x24/0xd8) [<c0171c7c>] (__vfs_write) from [<c0172520>] (vfs_write+0x94/0x154) [<c0172520>] (vfs_write) from [<c0172d1c>] (SyS_write+0x40/0x94) [<c0172d1c>] (SyS_write) from [<c000f760>] (ret_fast_syscall+0x0/0x1c) ---[ end trace 321b51565e161bee ]--- 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. Regards, Tony -- 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
* Sudeep Holla <sudeep.holla@arm.com> [151203 11:00]: > On 03/12/15 18:13, Tony Lindgren wrote: > >At least on omaps, this controller is always powered and we never want to > >suspend it as it handles wake-up events for all the IO pins. And that > >usecase sounds exactly like what you're describing above. > > > > Understood, but I assume this is a generic driver that can be used by > any pinmux. Right no question about that, but we need to keep things working. I just pasted the output to this thread what happens coming back up from suspend. > >I don't quite follow what your suggested alternative for an interrupt > >controller is? > > Why can't we use enable_irq_wake even for parent/interrupt controller as > they can be considered as parent wakeup irq. I agree the interrupt > controller may not be powered down, but still it's part of wakeup and > the irq core needs to identify that. By just marking IRQF_NO_SUSPEND, > you are saying that you can handle interrupt in the suspend path but not > informing that it's a wakeup interrupt. > > With this change, the wakeup handler (including the parent handler) is > called when it's safe as the irq core maintains the state machine. Maybe paste a suggested patch and I can try it. I guess you mean call that from pinctrl-single.c. > >At least we need to have the alternative patched in with this chage before > >just removing IRQF_NO_SUSPEND. > > > > 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. Regards, Tony -- 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
* 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. I think all that can be left out with the snipped from Grygorii, and maybe also the lock_class_key changes. Regards, Tony -- 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
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
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. 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.
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.
* Sudeep Holla <sudeep.holla@arm.com> [151204 08:27]: > > > 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. OK yeah if it's a counter then it makes sense to me. > 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. Yeah probably best that Grygorii tries to sort it out :) Regards, Tony -- 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
* Sudeep Holla <sudeep.holla@arm.com> [151204 08:16]: > 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 I think Grygorii is right here and this is correct as it's a counter once the other issues are sorted out and we have figured out what all needs to be patched together. Regards, Tony -- 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;