GPIO button is supposed to wake the system up if the wakeup attribute is set
Subsystems (2)
PATCH 1/1 GPIO button is supposed to wake the system up if the wakeup attribute is set
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.
--- drivers/input/keyboard/gpio_keys.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/input/keyboard/gpio_keys.c +3 -0
@@ -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",
Signature · 1 line
1.7.10.4
Quoted 8 lines · Aubrey
When the wakeup attribute is set, GPIO button is supposed to set irqflag -
IRQF_NO_SUSPENDto 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 <[email protected]>
ping...
Quoted 19 lines · One Thousand Gnomes
Quoted 8 lines · Aubrey
When the wakeup attribute is set, GPIO button is supposed to set irqflag -
IRQF_NO_SUSPENDto 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 <[email protected]>Tested-by Alan Cox <[email protected]>--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · Aubrey
ping...
Quoted 8 lines · One Thousand Gnomes
Quoted 5 lines · Aubrey
When the wakeup attribute is set, GPIO button is supposed to set irqflag -
IRQF_NO_SUSPENDto 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.
-----------------------------------------------------------------------------------
Quoted 13 lines · Laxman Dewangan
Quoted 11 lines · Aubrey
ping...
Quoted 8 lines · One Thousand Gnomes
Quoted 5 lines · Aubrey
When the wakeup attribute is set, GPIO button is supposed to set irqflag -
IRQF_NO_SUSPENDto request irq. So when the system enters the suspend sleep mode, the GPIO irq keeps enabled and is able to wake the system up.
Quoted 4 lines
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?
Quoted 3 lines
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_RESUMEthen 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
Quoted 19 lines
-----------------------------------------------------------------------------------
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.
-----------------------------------------------------------------------------------Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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.
Quoted 10 lines
Quoted 3 lines
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_RESUMEthen it works fine.
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_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.
-----------------------------------------------------------------------------------
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 18 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.
Quoted 6 lines
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
Quoted 13 lines · Aubrey
Quoted 12 lines · Laxman Dewangan
Quoted 11 lines · Aubrey
Quoted 8 lines · Laxman Dewangan
Quoted 3 lines · Aubrey
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?
Quoted 8 lines
Quoted 6 lines
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.
Quoted 5 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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.
-----------------------------------------------------------------------------------
Quoted 24 lines · Laxman Dewangan
Quoted 14 lines · Aubrey
Quoted 13 lines · Laxman Dewangan
Quoted 12 lines · Aubrey
Quoted 8 lines · Laxman Dewangan
Quoted 3 lines · Aubrey
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?Quoted 8 lines
Quoted 6 lines
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.
Quoted 6 lines
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.
Quoted 9 lines
Quoted 6 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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
Quoted 16 lines
-----------------------------------------------------------------------------------
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.
-----------------------------------------------------------------------------------
Dmitry Torokhov
maintainerHi Aubrey,
Quoted 50 lines · Aubrey
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 18 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.Quoted 6 lines
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.
Quoted 3 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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.
Signature · 1 line
Dmitry
Quoted 53 lines · Dmitry Torokhov
Hi Aubrey,
Quoted 50 lines · Aubrey
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 24 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.
Quoted 8 lines
Quoted 3 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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
Dmitry Torokhov
maintainerQuoted 67 lines · Aubrey
Quoted 53 lines · Dmitry Torokhov
Hi Aubrey,
Quoted 50 lines · Aubrey
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 24 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.
Quoted 8 lines
Quoted 3 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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 callenable_irq_wake()to enable the irq of the button. So when the button is pressed, hardware interrupt from this irq does occur.
Quoted 6 lines
However, after
gpio_keys_suspend(),irq_descof this irq will be disabled if there is noIRQF_NO_SUSPENDflag. 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 callenable_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.
Signature · 1 line
Dmitry
Quoted 75 lines · Dmitry Torokhov
Quoted 67 lines · Aubrey
Quoted 53 lines · Dmitry Torokhov
Hi Aubrey,
Quoted 50 lines · Aubrey
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 24 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.
Quoted 8 lines
Quoted 3 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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 callenable_irq_wake()to enable the irq of the button. So when the button is pressed, hardware interrupt from this irq does occur.Quoted 6 lines
However, after
gpio_keys_suspend(),irq_descof this irq will be disabled if there is noIRQF_NO_SUSPENDflag. 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 callenable_irq_wake()during suspend, the irq handler in this driver won't be called because it's an action handler, not a irq handler.
Quoted 5 lines
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
Dmitry Torokhov
maintainerQuoted 76 lines · Aubrey
Quoted 75 lines · Dmitry Torokhov
Quoted 73 lines · Aubrey
Quoted 53 lines · Dmitry Torokhov
Hi Aubrey,
Quoted 50 lines · Aubrey
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 24 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.
Quoted 8 lines
Quoted 3 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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 callenable_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_descof this irq will be disabled if there is noIRQF_NO_SUSPENDflag. 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 callenable_irq_wake()during suspend, the irq handler in this driver won't be called because it's an action handler, not a irq handler.
Quoted 8 lines
Quoted 5 lines
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.
Signature · 1 line
Dmitry
Quoted 83 lines · Dmitry Torokhov
Quoted 82 lines · Aubrey
Quoted 80 lines · Dmitry Torokhov
Quoted 67 lines · Aubrey
Quoted 53 lines · Dmitry Torokhov
Hi Aubrey,
Quoted 50 lines · Aubrey
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 24 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.
Quoted 8 lines
Quoted 3 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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 callenable_irq_wake()to enable the irq of the button. So when the button is pressed, hardware interrupt from this irq does occur.Quoted 6 lines
However, after
gpio_keys_suspend(),irq_descof this irq will be disabled if there is noIRQF_NO_SUSPENDflag. 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 callenable_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.
Quoted 7 lines
Quoted 2 lines
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
Quoted 2 lines
Thanks.
Dmitry Torokhov
maintainerQuoted 93 lines · Aubrey
Quoted 83 lines · Dmitry Torokhov
Quoted 82 lines · Aubrey
Quoted 80 lines · Dmitry Torokhov
Quoted 67 lines · Aubrey
Quoted 53 lines · Dmitry Torokhov
Hi Aubrey,
Quoted 50 lines · Aubrey
Quoted 14 lines · Laxman Dewangan
Quoted 4 lines · Aubrey
Quoted 3 lines · Laxman Dewangan
Quoted 2 lines · Aubrey
ping...
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines · One Thousand Gnomes
Quoted 1 line
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?
Quoted 6 lines
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.
Quoted 24 lines
Quoted 4 lines
Quoted 4 lines
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_RESUMEthen it works fine.Quoted 8 lines
IRQF_NO_SUSPEND- Do not disable this IRQ during suspendIRQF_EARLY_RESUME- Resume IRQ early during syscore instead of at device resume time.
IRQF_NO_SUSPENDis exactly what I want, instead ofIRQF_EARLY_RESUME. Can you please send your proposal/code to help me understand why this has to hw specific and whyIRQF_EARLY_RESUMEis better thanIRQF_NO_SUSPEND?
IRQF_EARLY_RESUMEhelps 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.
Quoted 8 lines
Quoted 3 lines
This is expected behavior. I think I still need
IRQF_NO_SUSPENDhere. 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 callenable_irq_wake()to enable the irq of the button. So when the button is pressed, hardware interrupt from this irq does occur.Quoted 6 lines
However, after
gpio_keys_suspend(),irq_descof this irq will be disabled if there is noIRQF_NO_SUSPENDflag. 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 callenable_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.Quoted 7 lines
Quoted 2 lines
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,
Quoted 7 lines
The current implementation is, setup the irq with the desired/correct flags, so the irq and the corresponding
irq_deschandler 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.
Quoted 3 lines
I'm looking forward to seeing your patch or if you want me to do this change, please let me know.
Yes please ;)
Thanks.
Signature · 1 line
Dmitry