| Message ID | 5345FDBD.9090908@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 10 Apr 2014 10:11:09 +0800 "Li, Aubrey" <aubrey.li@linux.intel.com> wrote: > When the wakeup attribute is set, GPIO button is supposed to set > irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters > the suspend sleep mode, the GPIO irq keeps enabled and is able to > wake the system up. > > The affected/tested machines include Dell Venue 11 Pro and Asus T100TA. > > Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com> Tested-by: Alan Cox <alan@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
ping... On 2014/4/10 18:48, One Thousand Gnomes wrote: > On Thu, 10 Apr 2014 10:11:09 +0800 > "Li, Aubrey" <aubrey.li@linux.intel.com> wrote: > >> When the wakeup attribute is set, GPIO button is supposed to set >> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters >> the suspend sleep mode, the GPIO irq keeps enabled and is able to >> wake the system up. >> >> The affected/tested machines include Dell Venue 11 Pro and Asus T100TA. >> >> Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com> > > Tested-by: Alan Cox <alan@linux.intel.com> > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: > ping... > > On 2014/4/10 18:48, One Thousand Gnomes wrote: >> On Thu, 10 Apr 2014 10:11:09 +0800 >> "Li, Aubrey" <aubrey.li@linux.intel.com> wrote: >> >>> When the wakeup attribute is set, GPIO button is supposed to set >>> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters >>> the suspend sleep mode, the GPIO irq keeps enabled and is able to >>> wake the system up. >>> I think when we say irq_wake_enable() then based on underlying HW, it should not turn off the irq if it is require for the wakeup. I mean it need to be handle in the hw specific callbacks to keep enabling the wakeup irq on suspend also. For me, I have key which is interrupt based from PMIC, not based on GPIO and on that if I set it to IRQF_EARLY_RESUME then it works fine. ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ----------------------------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/4/15 20:38, Laxman Dewangan wrote: > On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >> ping... >> >> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>> On Thu, 10 Apr 2014 10:11:09 +0800 >>> "Li, Aubrey" <aubrey.li@linux.intel.com> wrote: >>> >>>> When the wakeup attribute is set, GPIO button is supposed to set >>>> irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters >>>> the suspend sleep mode, the GPIO irq keeps enabled and is able to >>>> wake the system up. >>>> > > I think when we say irq_wake_enable() then based on underlying HW, it > should not turn off the irq if it is require for the wakeup. I mean it > need to be handle in the hw specific callbacks to keep enabling the > wakeup irq on suspend also. I failed to see why this can't be generic to all of the GPIO buttons for suspend wakeup. Do you see any cases broken by this proposal? > For me, I have key which is interrupt based from PMIC, not based on GPIO > and on that if I set it to IRQF_EARLY_RESUME then it works fine. > IRQF_NO_SUSPEND - Do not disable this IRQ during suspend IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device resume time. IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and why IRQF_EARLY_RESUME is better than IRQF_NO_SUSPEND? Thanks, -Aubrey > > > ----------------------------------------------------------------------------------- > > This email message is for the sole use of the intended recipient(s) and > may contain > confidential information. Any unauthorized review, use, disclosure or > distribution > is prohibited. If you are not the intended recipient, please contact > the sender by > reply email and destroy all copies of the original message. > ----------------------------------------------------------------------------------- > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: > On 2014/4/15 20:38, Laxman Dewangan wrote: >> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >>> ping... >>> >>> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>>> >> I think when we say irq_wake_enable() then based on underlying HW, it >> should not turn off the irq if it is require for the wakeup. I mean it >> need to be handle in the hw specific callbacks to keep enabling the >> wakeup irq on suspend also. > I failed to see why this can't be generic to all of the GPIO buttons for > suspend wakeup. Do you see any cases broken by this proposal? My point here is that if underlying HW needs to have irq enabled for wakup then it need to handle in centralized location i.e. the driver which is implementing it for the irq callbacks. Otherwise, we need to change this on multiple places who needs wakeups which is vast in nature like sd driver for sdcard insert/remove etc. almost all drivers which need wakeups through GPIOs. >> For me, I have key which is interrupt based from PMIC, not based on GPIO >> and on that if I set it to IRQF_EARLY_RESUME then it works fine. >> > IRQF_NO_SUSPEND - Do not disable this IRQ during suspend > IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > resume time. > > IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. > Can you please send your proposal/code to help me understand why this > has to hw specific and why IRQF_EARLY_RESUME is better than IRQF_NO_SUSPEND? IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt resume and so parent isr handler sees the irq flag enabled when it try to scan source of interrupt. Otherwise parent isr handler treat this as spurious interrupt and does not call handler as irq flag disabled for that. This only happen when on resume, parent inettrupt enabled before the child interrupt on irq resume. Because as soon as parent isr re-enabled on resume, its hadnler get called before actually child interrupt enabled. This is what I observed mainly on PMIC and its sub irq. Not observed on SoC level of interrupts. ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ----------------------------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/4/16 20:35, Laxman Dewangan wrote: > On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: >> On 2014/4/15 20:38, Laxman Dewangan wrote: >>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >>>> ping... >>>> >>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>>>> >>> I think when we say irq_wake_enable() then based on underlying HW, it >>> should not turn off the irq if it is require for the wakeup. I mean it >>> need to be handle in the hw specific callbacks to keep enabling the >>> wakeup irq on suspend also. >> I failed to see why this can't be generic to all of the GPIO buttons for >> suspend wakeup. Do you see any cases broken by this proposal? > > My point here is that if underlying HW needs to have irq enabled for > wakup then it need to handle in centralized location i.e. the driver > which is implementing it for the irq callbacks. > Otherwise, we need to change this on multiple places who needs wakeups > which is vast in nature like sd driver for sdcard insert/remove etc. > almost all drivers which need wakeups through GPIOs. I think we have to handle this driver by driver. I didn't see how can we make it in a centralized location. Looking forward to see your proposal. > >>> For me, I have key which is interrupt based from PMIC, not based on GPIO >>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. >>> >> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend >> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device >> resume time. >> >> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. >> Can you please send your proposal/code to help me understand why this >> has to hw specific and why IRQF_EARLY_RESUME is better than >> IRQF_NO_SUSPEND? > > IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt > resume and so parent isr handler sees the irq flag enabled when it try > to scan source of interrupt. Otherwise parent isr handler treat this as > spurious interrupt and does not call handler as irq flag disabled for that. > > This only happen when on resume, parent inettrupt enabled before the > child interrupt on irq resume. Because as soon as parent isr re-enabled > on resume, its hadnler get called before actually child interrupt > enabled. This is what I observed mainly on PMIC and its sub irq. Not > observed on SoC level of interrupts. > This is expected behavior. I think I still need IRQF_NO_SUSPEND here. What I want is, this IRQ is able to generate pm wakeup event to wake the system up. It's enough for my case. Did you see a failing case of my patch? Thanks, -Aubrey -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 17 April 2014 10:12 PM, Li, Aubrey wrote: > On 2014/4/16 20:35, Laxman Dewangan wrote: >> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: >>> On 2014/4/15 20:38, Laxman Dewangan wrote: >>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >>>>> ping... >>>>> >>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>>> I think when we say irq_wake_enable() then based on underlying HW, it >>>> should not turn off the irq if it is require for the wakeup. I mean it >>>> need to be handle in the hw specific callbacks to keep enabling the >>>> wakeup irq on suspend also. >>> I failed to see why this can't be generic to all of the GPIO buttons for >>> suspend wakeup. Do you see any cases broken by this proposal? >> My point here is that if underlying HW needs to have irq enabled for >> wakup then it need to handle in centralized location i.e. the driver >> which is implementing it for the irq callbacks. >> Otherwise, we need to change this on multiple places who needs wakeups >> which is vast in nature like sd driver for sdcard insert/remove etc. >> almost all drivers which need wakeups through GPIOs. > I think we have to handle this driver by driver. I didn't see how can we > make it in a centralized location. Looking forward to see your proposal. For Tegra SoC, we have implemented this such that we keep re-enabe interrupts when going to suspend. That's why my point is. May be your SoC ha implemented on different way and hence it is require NO_SUSPEND. I do not have any negative remark here, I jut kept my point here. > This is expected behavior. I think I still need IRQF_NO_SUSPEND here. > What I want is, this IRQ is able to generate pm wakeup event to wake the > system up. It's enough for my case. > > Did you see a failing case of my patch? Nop, I have not tested the patch and I think it will not break anything for me with your patch. ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. ----------------------------------------------------------------------------------- -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/4/18 1:18, Laxman Dewangan wrote: > On Thursday 17 April 2014 10:12 PM, Li, Aubrey wrote: >> On 2014/4/16 20:35, Laxman Dewangan wrote: >>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: >>>> On 2014/4/15 20:38, Laxman Dewangan wrote: >>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >>>>>> ping... >>>>>> >>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>>>> I think when we say irq_wake_enable() then based on underlying HW, it >>>>> should not turn off the irq if it is require for the wakeup. I mean it >>>>> need to be handle in the hw specific callbacks to keep enabling the >>>>> wakeup irq on suspend also. >>>> I failed to see why this can't be generic to all of the GPIO buttons >>>> for >>>> suspend wakeup. Do you see any cases broken by this proposal? >>> My point here is that if underlying HW needs to have irq enabled for >>> wakup then it need to handle in centralized location i.e. the driver >>> which is implementing it for the irq callbacks. >>> Otherwise, we need to change this on multiple places who needs wakeups >>> which is vast in nature like sd driver for sdcard insert/remove etc. >>> almost all drivers which need wakeups through GPIOs. >> I think we have to handle this driver by driver. I didn't see how can we >> make it in a centralized location. Looking forward to see your proposal. > > For Tegra SoC, we have implemented this such that we keep re-enabe > interrupts when going to suspend. That's why my point is. > May be your SoC ha implemented on different way and hence it is require > NO_SUSPEND. > > I do not have any negative remark here, I jut kept my point here. I see. thanks for your point. > >> This is expected behavior. I think I still need IRQF_NO_SUSPEND here. >> What I want is, this IRQ is able to generate pm wakeup event to wake the >> system up. It's enough for my case. >> >> Did you see a failing case of my patch? > > Nop, I have not tested the patch and I think it will not break anything > for me with your patch. Good to hear it. Thanks, -Aubrey > > > > ----------------------------------------------------------------------------------- > > This email message is for the sole use of the intended recipient(s) and > may contain > confidential information. Any unauthorized review, use, disclosure or > distribution > is prohibited. If you are not the intended recipient, please contact > the sender by > reply email and destroy all copies of the original message. > ----------------------------------------------------------------------------------- > > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Aubrey, On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote: > On 2014/4/16 20:35, Laxman Dewangan wrote: > > On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: > >> On 2014/4/15 20:38, Laxman Dewangan wrote: > >>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: > >>>> ping... > >>>> > >>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: > >>>>> > >>> I think when we say irq_wake_enable() then based on underlying HW, it > >>> should not turn off the irq if it is require for the wakeup. I mean it > >>> need to be handle in the hw specific callbacks to keep enabling the > >>> wakeup irq on suspend also. > >> I failed to see why this can't be generic to all of the GPIO buttons for > >> suspend wakeup. Do you see any cases broken by this proposal? > > > > My point here is that if underlying HW needs to have irq enabled for > > wakup then it need to handle in centralized location i.e. the driver > > which is implementing it for the irq callbacks. > > Otherwise, we need to change this on multiple places who needs wakeups > > which is vast in nature like sd driver for sdcard insert/remove etc. > > almost all drivers which need wakeups through GPIOs. > > I think we have to handle this driver by driver. I didn't see how can we > make it in a centralized location. Looking forward to see your proposal. > > > > >>> For me, I have key which is interrupt based from PMIC, not based on GPIO > >>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. > >>> > >> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend > >> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > >> resume time. > >> > >> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. > >> Can you please send your proposal/code to help me understand why this > >> has to hw specific and why IRQF_EARLY_RESUME is better than > >> IRQF_NO_SUSPEND? > > > > IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt > > resume and so parent isr handler sees the irq flag enabled when it try > > to scan source of interrupt. Otherwise parent isr handler treat this as > > spurious interrupt and does not call handler as irq flag disabled for that. > > > > This only happen when on resume, parent inettrupt enabled before the > > child interrupt on irq resume. Because as soon as parent isr re-enabled > > on resume, its hadnler get called before actually child interrupt > > enabled. This is what I observed mainly on PMIC and its sub irq. Not > > observed on SoC level of interrupts. > > > > This is expected behavior. I think I still need IRQF_NO_SUSPEND here. > What I want is, this IRQ is able to generate pm wakeup event to wake the > system up. It's enough for my case. The driver does call enable_irq_wake() in its suspend routine to prepare the interrupt in question to be used as a wakeup source. Why isn't it enough? It seems to me that your platform code should properly handle this case instead of relying on the driver to modify IRQ flags. Thanks.
On 2014/4/18 7:54, Dmitry Torokhov wrote: > Hi Aubrey, > > On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote: >> On 2014/4/16 20:35, Laxman Dewangan wrote: >>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: >>>> On 2014/4/15 20:38, Laxman Dewangan wrote: >>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >>>>>> ping... >>>>>> >>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>>>>>> >>>>> I think when we say irq_wake_enable() then based on underlying HW, it >>>>> should not turn off the irq if it is require for the wakeup. I mean it >>>>> need to be handle in the hw specific callbacks to keep enabling the >>>>> wakeup irq on suspend also. >>>> I failed to see why this can't be generic to all of the GPIO buttons for >>>> suspend wakeup. Do you see any cases broken by this proposal? >>> >>> My point here is that if underlying HW needs to have irq enabled for >>> wakup then it need to handle in centralized location i.e. the driver >>> which is implementing it for the irq callbacks. >>> Otherwise, we need to change this on multiple places who needs wakeups >>> which is vast in nature like sd driver for sdcard insert/remove etc. >>> almost all drivers which need wakeups through GPIOs. >> >> I think we have to handle this driver by driver. I didn't see how can we >> make it in a centralized location. Looking forward to see your proposal. >> >>> >>>>> For me, I have key which is interrupt based from PMIC, not based on GPIO >>>>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. >>>>> >>>> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend >>>> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device >>>> resume time. >>>> >>>> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. >>>> Can you please send your proposal/code to help me understand why this >>>> has to hw specific and why IRQF_EARLY_RESUME is better than >>>> IRQF_NO_SUSPEND? >>> >>> IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt >>> resume and so parent isr handler sees the irq flag enabled when it try >>> to scan source of interrupt. Otherwise parent isr handler treat this as >>> spurious interrupt and does not call handler as irq flag disabled for that. >>> >>> This only happen when on resume, parent inettrupt enabled before the >>> child interrupt on irq resume. Because as soon as parent isr re-enabled >>> on resume, its hadnler get called before actually child interrupt >>> enabled. This is what I observed mainly on PMIC and its sub irq. Not >>> observed on SoC level of interrupts. >>> >> >> This is expected behavior. I think I still need IRQF_NO_SUSPEND here. >> What I want is, this IRQ is able to generate pm wakeup event to wake the >> system up. It's enough for my case. > > The driver does call enable_irq_wake() in its suspend routine to prepare > the interrupt in question to be used as a wakeup source. Why isn't it > enough? It seems to me that your platform code should properly handle > this case instead of relying on the driver to modify IRQ flags. Yes, gpio_keys_suspend() does call enable_irq_wake() to enable the irq of the button. So when the button is pressed, hardware interrupt from this irq does occur. However, after gpio_keys_suspend(), irq_desc of this irq will be disabled if there is no IRQF_NO_SUSPEND flag. So when the hardware interrupt occurs, the irq handler won't call the action of the irq desc. That is, for this case, even if the driver call enable_irq_wake() during suspend, the irq handler in this driver won't be called because it's an action handler, not a irq handler. Does this make sense? Thanks, -Aubrey -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 18, 2014 at 01:23:11PM +0800, Li, Aubrey wrote: > On 2014/4/18 7:54, Dmitry Torokhov wrote: > > Hi Aubrey, > > > > On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote: > >> On 2014/4/16 20:35, Laxman Dewangan wrote: > >>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: > >>>> On 2014/4/15 20:38, Laxman Dewangan wrote: > >>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: > >>>>>> ping... > >>>>>> > >>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: > >>>>>>> > >>>>> I think when we say irq_wake_enable() then based on underlying HW, it > >>>>> should not turn off the irq if it is require for the wakeup. I mean it > >>>>> need to be handle in the hw specific callbacks to keep enabling the > >>>>> wakeup irq on suspend also. > >>>> I failed to see why this can't be generic to all of the GPIO buttons for > >>>> suspend wakeup. Do you see any cases broken by this proposal? > >>> > >>> My point here is that if underlying HW needs to have irq enabled for > >>> wakup then it need to handle in centralized location i.e. the driver > >>> which is implementing it for the irq callbacks. > >>> Otherwise, we need to change this on multiple places who needs wakeups > >>> which is vast in nature like sd driver for sdcard insert/remove etc. > >>> almost all drivers which need wakeups through GPIOs. > >> > >> I think we have to handle this driver by driver. I didn't see how can we > >> make it in a centralized location. Looking forward to see your proposal. > >> > >>> > >>>>> For me, I have key which is interrupt based from PMIC, not based on GPIO > >>>>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. > >>>>> > >>>> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend > >>>> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > >>>> resume time. > >>>> > >>>> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. > >>>> Can you please send your proposal/code to help me understand why this > >>>> has to hw specific and why IRQF_EARLY_RESUME is better than > >>>> IRQF_NO_SUSPEND? > >>> > >>> IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt > >>> resume and so parent isr handler sees the irq flag enabled when it try > >>> to scan source of interrupt. Otherwise parent isr handler treat this as > >>> spurious interrupt and does not call handler as irq flag disabled for that. > >>> > >>> This only happen when on resume, parent inettrupt enabled before the > >>> child interrupt on irq resume. Because as soon as parent isr re-enabled > >>> on resume, its hadnler get called before actually child interrupt > >>> enabled. This is what I observed mainly on PMIC and its sub irq. Not > >>> observed on SoC level of interrupts. > >>> > >> > >> This is expected behavior. I think I still need IRQF_NO_SUSPEND here. > >> What I want is, this IRQ is able to generate pm wakeup event to wake the > >> system up. It's enough for my case. > > > > The driver does call enable_irq_wake() in its suspend routine to prepare > > the interrupt in question to be used as a wakeup source. Why isn't it > > enough? It seems to me that your platform code should properly handle > > this case instead of relying on the driver to modify IRQ flags. > > Yes, gpio_keys_suspend() does call enable_irq_wake() to enable the irq > of the button. So when the button is pressed, hardware interrupt from > this irq does occur. > > However, after gpio_keys_suspend(), irq_desc of this irq will be > disabled if there is no IRQF_NO_SUSPEND flag. So when the hardware > interrupt occurs, the irq handler won't call the action of the irq desc. > That is, for this case, even if the driver call enable_irq_wake() during > suspend, the irq handler in this driver won't be called because it's an > action handler, not a irq handler. Right, so what I am saying is that enable_irq_wake() should really be taking care of that and ensuring that if device is marked as wakeup source it should prepare irq handler code to run all necessary parts instead of sprinkling random flags all over individual drivers. Thanks.
On 2014/4/21 3:39, Dmitry Torokhov wrote: > On Fri, Apr 18, 2014 at 01:23:11PM +0800, Li, Aubrey wrote: >> On 2014/4/18 7:54, Dmitry Torokhov wrote: >>> Hi Aubrey, >>> >>> On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote: >>>> On 2014/4/16 20:35, Laxman Dewangan wrote: >>>>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: >>>>>> On 2014/4/15 20:38, Laxman Dewangan wrote: >>>>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >>>>>>>> ping... >>>>>>>> >>>>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>>>>>>>> >>>>>>> I think when we say irq_wake_enable() then based on underlying HW, it >>>>>>> should not turn off the irq if it is require for the wakeup. I mean it >>>>>>> need to be handle in the hw specific callbacks to keep enabling the >>>>>>> wakeup irq on suspend also. >>>>>> I failed to see why this can't be generic to all of the GPIO buttons for >>>>>> suspend wakeup. Do you see any cases broken by this proposal? >>>>> >>>>> My point here is that if underlying HW needs to have irq enabled for >>>>> wakup then it need to handle in centralized location i.e. the driver >>>>> which is implementing it for the irq callbacks. >>>>> Otherwise, we need to change this on multiple places who needs wakeups >>>>> which is vast in nature like sd driver for sdcard insert/remove etc. >>>>> almost all drivers which need wakeups through GPIOs. >>>> >>>> I think we have to handle this driver by driver. I didn't see how can we >>>> make it in a centralized location. Looking forward to see your proposal. >>>> >>>>> >>>>>>> For me, I have key which is interrupt based from PMIC, not based on GPIO >>>>>>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. >>>>>>> >>>>>> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend >>>>>> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device >>>>>> resume time. >>>>>> >>>>>> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. >>>>>> Can you please send your proposal/code to help me understand why this >>>>>> has to hw specific and why IRQF_EARLY_RESUME is better than >>>>>> IRQF_NO_SUSPEND? >>>>> >>>>> IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt >>>>> resume and so parent isr handler sees the irq flag enabled when it try >>>>> to scan source of interrupt. Otherwise parent isr handler treat this as >>>>> spurious interrupt and does not call handler as irq flag disabled for that. >>>>> >>>>> This only happen when on resume, parent inettrupt enabled before the >>>>> child interrupt on irq resume. Because as soon as parent isr re-enabled >>>>> on resume, its hadnler get called before actually child interrupt >>>>> enabled. This is what I observed mainly on PMIC and its sub irq. Not >>>>> observed on SoC level of interrupts. >>>>> >>>> >>>> This is expected behavior. I think I still need IRQF_NO_SUSPEND here. >>>> What I want is, this IRQ is able to generate pm wakeup event to wake the >>>> system up. It's enough for my case. >>> >>> The driver does call enable_irq_wake() in its suspend routine to prepare >>> the interrupt in question to be used as a wakeup source. Why isn't it >>> enough? It seems to me that your platform code should properly handle >>> this case instead of relying on the driver to modify IRQ flags. >> >> Yes, gpio_keys_suspend() does call enable_irq_wake() to enable the irq >> of the button. So when the button is pressed, hardware interrupt from >> this irq does occur. >> >> However, after gpio_keys_suspend(), irq_desc of this irq will be >> disabled if there is no IRQF_NO_SUSPEND flag. So when the hardware >> interrupt occurs, the irq handler won't call the action of the irq desc. >> That is, for this case, even if the driver call enable_irq_wake() during >> suspend, the irq handler in this driver won't be called because it's an >> action handler, not a irq handler. > > Right, so what I am saying is that enable_irq_wake() should really be > taking care of that and ensuring that if device is marked as wakeup > source it should prepare irq handler code to run all necessary parts > instead of sprinkling random flags all over individual drivers. > If the IRQ is shared, how to handle the case of the one is marked as wakeup source and the other is not? Thanks, -Aubrey -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 21, 2014 at 09:34:20AM +0800, Li, Aubrey wrote: > On 2014/4/21 3:39, Dmitry Torokhov wrote: > > On Fri, Apr 18, 2014 at 01:23:11PM +0800, Li, Aubrey wrote: > >> On 2014/4/18 7:54, Dmitry Torokhov wrote: > >>> Hi Aubrey, > >>> > >>> On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote: > >>>> On 2014/4/16 20:35, Laxman Dewangan wrote: > >>>>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: > >>>>>> On 2014/4/15 20:38, Laxman Dewangan wrote: > >>>>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: > >>>>>>>> ping... > >>>>>>>> > >>>>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: > >>>>>>>>> > >>>>>>> I think when we say irq_wake_enable() then based on underlying HW, it > >>>>>>> should not turn off the irq if it is require for the wakeup. I mean it > >>>>>>> need to be handle in the hw specific callbacks to keep enabling the > >>>>>>> wakeup irq on suspend also. > >>>>>> I failed to see why this can't be generic to all of the GPIO buttons for > >>>>>> suspend wakeup. Do you see any cases broken by this proposal? > >>>>> > >>>>> My point here is that if underlying HW needs to have irq enabled for > >>>>> wakup then it need to handle in centralized location i.e. the driver > >>>>> which is implementing it for the irq callbacks. > >>>>> Otherwise, we need to change this on multiple places who needs wakeups > >>>>> which is vast in nature like sd driver for sdcard insert/remove etc. > >>>>> almost all drivers which need wakeups through GPIOs. > >>>> > >>>> I think we have to handle this driver by driver. I didn't see how can we > >>>> make it in a centralized location. Looking forward to see your proposal. > >>>> > >>>>> > >>>>>>> For me, I have key which is interrupt based from PMIC, not based on GPIO > >>>>>>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. > >>>>>>> > >>>>>> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend > >>>>>> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > >>>>>> resume time. > >>>>>> > >>>>>> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. > >>>>>> Can you please send your proposal/code to help me understand why this > >>>>>> has to hw specific and why IRQF_EARLY_RESUME is better than > >>>>>> IRQF_NO_SUSPEND? > >>>>> > >>>>> IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt > >>>>> resume and so parent isr handler sees the irq flag enabled when it try > >>>>> to scan source of interrupt. Otherwise parent isr handler treat this as > >>>>> spurious interrupt and does not call handler as irq flag disabled for that. > >>>>> > >>>>> This only happen when on resume, parent inettrupt enabled before the > >>>>> child interrupt on irq resume. Because as soon as parent isr re-enabled > >>>>> on resume, its hadnler get called before actually child interrupt > >>>>> enabled. This is what I observed mainly on PMIC and its sub irq. Not > >>>>> observed on SoC level of interrupts. > >>>>> > >>>> > >>>> This is expected behavior. I think I still need IRQF_NO_SUSPEND here. > >>>> What I want is, this IRQ is able to generate pm wakeup event to wake the > >>>> system up. It's enough for my case. > >>> > >>> The driver does call enable_irq_wake() in its suspend routine to prepare > >>> the interrupt in question to be used as a wakeup source. Why isn't it > >>> enough? It seems to me that your platform code should properly handle > >>> this case instead of relying on the driver to modify IRQ flags. > >> > >> Yes, gpio_keys_suspend() does call enable_irq_wake() to enable the irq > >> of the button. So when the button is pressed, hardware interrupt from > >> this irq does occur. > >> > >> However, after gpio_keys_suspend(), irq_desc of this irq will be > >> disabled if there is no IRQF_NO_SUSPEND flag. So when the hardware > >> interrupt occurs, the irq handler won't call the action of the irq desc. > >> That is, for this case, even if the driver call enable_irq_wake() during > >> suspend, the irq handler in this driver won't be called because it's an > >> action handler, not a irq handler. > > > > Right, so what I am saying is that enable_irq_wake() should really be > > taking care of that and ensuring that if device is marked as wakeup > > source it should prepare irq handler code to run all necessary parts > > instead of sprinkling random flags all over individual drivers. > > > > If the IRQ is shared, how to handle the case of the one is marked as > wakeup source and the other is not? How do we handle this now? You can either make enable_irq_wake() fail or, like with other shared interrupts, have it runi both handlers and let drivers sort it out. Thanks.
On 2014/4/21 23:55, Dmitry Torokhov wrote: > On Mon, Apr 21, 2014 at 09:34:20AM +0800, Li, Aubrey wrote: >> On 2014/4/21 3:39, Dmitry Torokhov wrote: >>> On Fri, Apr 18, 2014 at 01:23:11PM +0800, Li, Aubrey wrote: >>>> On 2014/4/18 7:54, Dmitry Torokhov wrote: >>>>> Hi Aubrey, >>>>> >>>>> On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote: >>>>>> On 2014/4/16 20:35, Laxman Dewangan wrote: >>>>>>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: >>>>>>>> On 2014/4/15 20:38, Laxman Dewangan wrote: >>>>>>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: >>>>>>>>>> ping... >>>>>>>>>> >>>>>>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: >>>>>>>>>>> >>>>>>>>> I think when we say irq_wake_enable() then based on underlying HW, it >>>>>>>>> should not turn off the irq if it is require for the wakeup. I mean it >>>>>>>>> need to be handle in the hw specific callbacks to keep enabling the >>>>>>>>> wakeup irq on suspend also. >>>>>>>> I failed to see why this can't be generic to all of the GPIO buttons for >>>>>>>> suspend wakeup. Do you see any cases broken by this proposal? >>>>>>> >>>>>>> My point here is that if underlying HW needs to have irq enabled for >>>>>>> wakup then it need to handle in centralized location i.e. the driver >>>>>>> which is implementing it for the irq callbacks. >>>>>>> Otherwise, we need to change this on multiple places who needs wakeups >>>>>>> which is vast in nature like sd driver for sdcard insert/remove etc. >>>>>>> almost all drivers which need wakeups through GPIOs. >>>>>> >>>>>> I think we have to handle this driver by driver. I didn't see how can we >>>>>> make it in a centralized location. Looking forward to see your proposal. >>>>>> >>>>>>> >>>>>>>>> For me, I have key which is interrupt based from PMIC, not based on GPIO >>>>>>>>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. >>>>>>>>> >>>>>>>> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend >>>>>>>> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device >>>>>>>> resume time. >>>>>>>> >>>>>>>> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. >>>>>>>> Can you please send your proposal/code to help me understand why this >>>>>>>> has to hw specific and why IRQF_EARLY_RESUME is better than >>>>>>>> IRQF_NO_SUSPEND? >>>>>>> >>>>>>> IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt >>>>>>> resume and so parent isr handler sees the irq flag enabled when it try >>>>>>> to scan source of interrupt. Otherwise parent isr handler treat this as >>>>>>> spurious interrupt and does not call handler as irq flag disabled for that. >>>>>>> >>>>>>> This only happen when on resume, parent inettrupt enabled before the >>>>>>> child interrupt on irq resume. Because as soon as parent isr re-enabled >>>>>>> on resume, its hadnler get called before actually child interrupt >>>>>>> enabled. This is what I observed mainly on PMIC and its sub irq. Not >>>>>>> observed on SoC level of interrupts. >>>>>>> >>>>>> >>>>>> This is expected behavior. I think I still need IRQF_NO_SUSPEND here. >>>>>> What I want is, this IRQ is able to generate pm wakeup event to wake the >>>>>> system up. It's enough for my case. >>>>> >>>>> The driver does call enable_irq_wake() in its suspend routine to prepare >>>>> the interrupt in question to be used as a wakeup source. Why isn't it >>>>> enough? It seems to me that your platform code should properly handle >>>>> this case instead of relying on the driver to modify IRQ flags. >>>> >>>> Yes, gpio_keys_suspend() does call enable_irq_wake() to enable the irq >>>> of the button. So when the button is pressed, hardware interrupt from >>>> this irq does occur. >>>> >>>> However, after gpio_keys_suspend(), irq_desc of this irq will be >>>> disabled if there is no IRQF_NO_SUSPEND flag. So when the hardware >>>> interrupt occurs, the irq handler won't call the action of the irq desc. >>>> That is, for this case, even if the driver call enable_irq_wake() during >>>> suspend, the irq handler in this driver won't be called because it's an >>>> action handler, not a irq handler. >>> >>> Right, so what I am saying is that enable_irq_wake() should really be >>> taking care of that and ensuring that if device is marked as wakeup >>> source it should prepare irq handler code to run all necessary parts >>> instead of sprinkling random flags all over individual drivers. >>> >> >> If the IRQ is shared, how to handle the case of the one is marked as >> wakeup source and the other is not? > > How do we handle this now? You can either make enable_irq_wake() fail > or, like with other shared interrupts, have it runi both handlers and > let drivers sort it out. > Hi Dmitry, The current implementation is, setup the irq with the desired/correct flags, so the irq and the corresponding irq_desc handler will be enabled or disabled properly during suspend and resume. My patch is exactly following this way and the impact is gpio key driver only. What you suggest seems not to be a small change and need a widely testing because the change is supposed to cover all of the drivers. I'm looking forward to seeing your patch or if you want me to do this change, please let me know. Thanks, -Aubrey > Thanks. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 22, 2014 at 12:16:52AM +0800, Li, Aubrey wrote: > On 2014/4/21 23:55, Dmitry Torokhov wrote: > > On Mon, Apr 21, 2014 at 09:34:20AM +0800, Li, Aubrey wrote: > >> On 2014/4/21 3:39, Dmitry Torokhov wrote: > >>> On Fri, Apr 18, 2014 at 01:23:11PM +0800, Li, Aubrey wrote: > >>>> On 2014/4/18 7:54, Dmitry Torokhov wrote: > >>>>> Hi Aubrey, > >>>>> > >>>>> On Fri, Apr 18, 2014 at 12:42:24AM +0800, Li, Aubrey wrote: > >>>>>> On 2014/4/16 20:35, Laxman Dewangan wrote: > >>>>>>> On Tuesday 15 April 2014 09:48 PM, Li, Aubrey wrote: > >>>>>>>> On 2014/4/15 20:38, Laxman Dewangan wrote: > >>>>>>>>> On Monday 14 April 2014 09:12 PM, Li, Aubrey wrote: > >>>>>>>>>> ping... > >>>>>>>>>> > >>>>>>>>>> On 2014/4/10 18:48, One Thousand Gnomes wrote: > >>>>>>>>>>> > >>>>>>>>> I think when we say irq_wake_enable() then based on underlying HW, it > >>>>>>>>> should not turn off the irq if it is require for the wakeup. I mean it > >>>>>>>>> need to be handle in the hw specific callbacks to keep enabling the > >>>>>>>>> wakeup irq on suspend also. > >>>>>>>> I failed to see why this can't be generic to all of the GPIO buttons for > >>>>>>>> suspend wakeup. Do you see any cases broken by this proposal? > >>>>>>> > >>>>>>> My point here is that if underlying HW needs to have irq enabled for > >>>>>>> wakup then it need to handle in centralized location i.e. the driver > >>>>>>> which is implementing it for the irq callbacks. > >>>>>>> Otherwise, we need to change this on multiple places who needs wakeups > >>>>>>> which is vast in nature like sd driver for sdcard insert/remove etc. > >>>>>>> almost all drivers which need wakeups through GPIOs. > >>>>>> > >>>>>> I think we have to handle this driver by driver. I didn't see how can we > >>>>>> make it in a centralized location. Looking forward to see your proposal. > >>>>>> > >>>>>>> > >>>>>>>>> For me, I have key which is interrupt based from PMIC, not based on GPIO > >>>>>>>>> and on that if I set it to IRQF_EARLY_RESUME then it works fine. > >>>>>>>>> > >>>>>>>> IRQF_NO_SUSPEND - Do not disable this IRQ during suspend > >>>>>>>> IRQF_EARLY_RESUME - Resume IRQ early during syscore instead of at device > >>>>>>>> resume time. > >>>>>>>> > >>>>>>>> IRQF_NO_SUSPEND is exactly what I want, instead of IRQF_EARLY_RESUME. > >>>>>>>> Can you please send your proposal/code to help me understand why this > >>>>>>>> has to hw specific and why IRQF_EARLY_RESUME is better than > >>>>>>>> IRQF_NO_SUSPEND? > >>>>>>> > >>>>>>> IRQF_EARLY_RESUME helps to re-enable mask or irq before parent interrupt > >>>>>>> resume and so parent isr handler sees the irq flag enabled when it try > >>>>>>> to scan source of interrupt. Otherwise parent isr handler treat this as > >>>>>>> spurious interrupt and does not call handler as irq flag disabled for that. > >>>>>>> > >>>>>>> This only happen when on resume, parent inettrupt enabled before the > >>>>>>> child interrupt on irq resume. Because as soon as parent isr re-enabled > >>>>>>> on resume, its hadnler get called before actually child interrupt > >>>>>>> enabled. This is what I observed mainly on PMIC and its sub irq. Not > >>>>>>> observed on SoC level of interrupts. > >>>>>>> > >>>>>> > >>>>>> This is expected behavior. I think I still need IRQF_NO_SUSPEND here. > >>>>>> What I want is, this IRQ is able to generate pm wakeup event to wake the > >>>>>> system up. It's enough for my case. > >>>>> > >>>>> The driver does call enable_irq_wake() in its suspend routine to prepare > >>>>> the interrupt in question to be used as a wakeup source. Why isn't it > >>>>> enough? It seems to me that your platform code should properly handle > >>>>> this case instead of relying on the driver to modify IRQ flags. > >>>> > >>>> Yes, gpio_keys_suspend() does call enable_irq_wake() to enable the irq > >>>> of the button. So when the button is pressed, hardware interrupt from > >>>> this irq does occur. > >>>> > >>>> However, after gpio_keys_suspend(), irq_desc of this irq will be > >>>> disabled if there is no IRQF_NO_SUSPEND flag. So when the hardware > >>>> interrupt occurs, the irq handler won't call the action of the irq desc. > >>>> That is, for this case, even if the driver call enable_irq_wake() during > >>>> suspend, the irq handler in this driver won't be called because it's an > >>>> action handler, not a irq handler. > >>> > >>> Right, so what I am saying is that enable_irq_wake() should really be > >>> taking care of that and ensuring that if device is marked as wakeup > >>> source it should prepare irq handler code to run all necessary parts > >>> instead of sprinkling random flags all over individual drivers. > >>> > >> > >> If the IRQ is shared, how to handle the case of the one is marked as > >> wakeup source and the other is not? > > > > How do we handle this now? You can either make enable_irq_wake() fail > > or, like with other shared interrupts, have it runi both handlers and > > let drivers sort it out. > > > Hi Dmitry, > > The current implementation is, setup the irq with the desired/correct > flags, so the irq and the corresponding irq_desc handler will be enabled > or disabled properly during suspend and resume. > > My patch is exactly following this way and the impact is gpio key driver > only. What you suggest seems not to be a small change and need a widely > testing because the change is supposed to cover all of the drivers. Right, you are working around issue in IRQ core by adjusting the driver. Once the core is fixed you (and others) won't be needing to change drivers at all. > > I'm looking forward to seeing your patch or if you want me to do this > change, please let me know. Yes please ;) Thanks.
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 2db1324..40f3963 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -503,6 +503,9 @@ static int gpio_keys_setup_key(struct platform_device *pdev, if (!button->can_disable) irqflags |= IRQF_SHARED; + if (button->wakeup) + irqflags |= IRQF_NO_SUSPEND; + error = request_any_context_irq(bdata->irq, isr, irqflags, desc, bdata); if (error < 0) { dev_err(dev, "Unable to claim irq %d; error %d\n",
When the wakeup attribute is set, GPIO button is supposed to set irqflag - IRQF_NO_SUSPEND to request irq. So when the system enters the suspend sleep mode, the GPIO irq keeps enabled and is able to wake the system up. The affected/tested machines include Dell Venue 11 Pro and Asus T100TA. Signed-off-by: Aubrey Li <aubrey.li@linux.intel.com> --- drivers/input/keyboard/gpio_keys.c | 3 +++ 1 file changed, 3 insertions(+)