* [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
@ 2012-10-23 18:09 Kevin Hilman
2012-10-23 19:09 ` Felipe Balbi
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Kevin Hilman @ 2012-10-23 18:09 UTC (permalink / raw)
To: Linus Walleij, Felipe Balbi, linux-omap
Cc: Paul Walmsley, linux-arm-kernel, Igor Grinberg
From: Kevin Hilman <khilman@ti.com>
When debounce clocks are disabled, ensure that the banks
dbck_enable_mask is cleared also. Otherwise, context restore on
subsequent off-mode transition will restore previous value from the
shadow copies (bank->context.debounce*) leading to mismatch state
between driver state and hardware state.
This was discovered when board code was doing
gpio_request_one()
gpio_set_debounce()
gpio_free()
which was leaving the GPIO debounce settings in a confused state.
Then, enabling off mode causing bogus state to be restored, leaving
GPIO debounce enabled which then prevented the CORE powerdomain from
transitioning.
Reported-by: Paul Walmsley <paul@pwsan.com>
Cc: Igor Grinberg <grinberg@compulab.co.il>
Signed-off-by: Kevin Hilman <khilman@ti.com>
---
Applies on v3.7-rc2, targetted for v3.7.
drivers/gpio/gpio-omap.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..dee2856 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -187,6 +187,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
* to detect events and generate interrupts at least on OMAP3.
*/
__raw_writel(0, bank->base + bank->regs->debounce_en);
+ bank->dbck_enable_mask = 0;
clk_disable(bank->dbck);
bank->dbck_enabled = false;
--
1.8.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-23 18:09 [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable Kevin Hilman
@ 2012-10-23 19:09 ` Felipe Balbi
2012-10-23 22:00 ` Kevin Hilman
2012-10-24 7:33 ` Santosh Shilimkar
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Felipe Balbi @ 2012-10-23 19:09 UTC (permalink / raw)
To: Kevin Hilman
Cc: Linus Walleij, Felipe Balbi, linux-omap, Paul Walmsley,
linux-arm-kernel, Igor Grinberg
[-- Attachment #1: Type: text/plain, Size: 2354 bytes --]
Hi,
On Tue, Oct 23, 2012 at 11:09:31AM -0700, Kevin Hilman wrote:
> From: Kevin Hilman <khilman@ti.com>
>
> When debounce clocks are disabled, ensure that the banks
> dbck_enable_mask is cleared also. Otherwise, context restore on
> subsequent off-mode transition will restore previous value from the
> shadow copies (bank->context.debounce*) leading to mismatch state
> between driver state and hardware state.
>
> This was discovered when board code was doing
>
> gpio_request_one()
> gpio_set_debounce()
> gpio_free()
>
> which was leaving the GPIO debounce settings in a confused state.
> Then, enabling off mode causing bogus state to be restored, leaving
> GPIO debounce enabled which then prevented the CORE powerdomain from
> transitioning.
>
> Reported-by: Paul Walmsley <paul@pwsan.com>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
looks like this deserves a Cc: stable@vger.kernel.org tag.
> ---
> Applies on v3.7-rc2, targetted for v3.7.
>
> drivers/gpio/gpio-omap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 94cbc84..dee2856 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -187,6 +187,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
> * to detect events and generate interrupts at least on OMAP3.
> */
> __raw_writel(0, bank->base + bank->regs->debounce_en);
> + bank->dbck_enable_mask = 0;
shouldn't omap_gpio_restore_context() check for dbck_enabled instead of
the mask ? I mean:
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..b3a39a7 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1371,7 +1371,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
bank->base + bank->regs->dataout);
__raw_writel(bank->context.oe, bank->base + bank->regs->direction);
- if (bank->dbck_enable_mask) {
+ if (bank->dbck_enabled) {
__raw_writel(bank->context.debounce, bank->base +
bank->regs->debounce);
__raw_writel(bank->context.debounce_en,
the outcome would be the same, so it doesn't really matter. Just that,
at least to me, it would look better.
No strong feelings though.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-23 19:09 ` Felipe Balbi
@ 2012-10-23 22:00 ` Kevin Hilman
2012-10-24 7:39 ` Felipe Balbi
0 siblings, 1 reply; 13+ messages in thread
From: Kevin Hilman @ 2012-10-23 22:00 UTC (permalink / raw)
To: balbi
Cc: Linus Walleij, linux-omap, Paul Walmsley, linux-arm-kernel,
Igor Grinberg
Felipe Balbi <balbi@ti.com> writes:
> Hi,
>
> On Tue, Oct 23, 2012 at 11:09:31AM -0700, Kevin Hilman wrote:
>> From: Kevin Hilman <khilman@ti.com>
>>
>> When debounce clocks are disabled, ensure that the banks
>> dbck_enable_mask is cleared also. Otherwise, context restore on
>> subsequent off-mode transition will restore previous value from the
>> shadow copies (bank->context.debounce*) leading to mismatch state
>> between driver state and hardware state.
>>
>> This was discovered when board code was doing
>>
>> gpio_request_one()
>> gpio_set_debounce()
>> gpio_free()
>>
>> which was leaving the GPIO debounce settings in a confused state.
>> Then, enabling off mode causing bogus state to be restored, leaving
>> GPIO debounce enabled which then prevented the CORE powerdomain from
>> transitioning.
>>
>> Reported-by: Paul Walmsley <paul@pwsan.com>
>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>
> looks like this deserves a Cc: stable@vger.kernel.org tag.
>
Agreed. I think this goes all the way back to v3.5, but would've only
been seen on boards using a request/gpio_set_debounce/free sequence
combined with off-mode.
Linus, feel free to add the Cc: stable when commiting. Thanks.
>> ---
>> Applies on v3.7-rc2, targetted for v3.7.
>>
>> drivers/gpio/gpio-omap.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 94cbc84..dee2856 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -187,6 +187,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>> * to detect events and generate interrupts at least on OMAP3.
>> */
>> __raw_writel(0, bank->base + bank->regs->debounce_en);
>> + bank->dbck_enable_mask = 0;
>
> shouldn't omap_gpio_restore_context() check for dbck_enabled instead of
> the mask ? I mean:
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 94cbc84..b3a39a7 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1371,7 +1371,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
> bank->base + bank->regs->dataout);
> __raw_writel(bank->context.oe, bank->base + bank->regs->direction);
>
> - if (bank->dbck_enable_mask) {
> + if (bank->dbck_enabled) {
> __raw_writel(bank->context.debounce, bank->base +
> bank->regs->debounce);
> __raw_writel(bank->context.debounce_en,
>
> the outcome would be the same, so it doesn't really matter. Just that,
> at least to me, it would look better.
I tried your version, and unfortunately, the outcome is not the same,
but don't plan to look into why. $SUBJECT version is targetted and
tested. If you want to cleanup the cosmetics here, please do in a
subsequent patch. This driver could certainly benefit from more
readability cleanups.
> No strong feelings though.
Good. I'll take that as an Ack. :)
Kevin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-23 18:09 [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable Kevin Hilman
2012-10-23 19:09 ` Felipe Balbi
@ 2012-10-24 7:33 ` Santosh Shilimkar
2012-10-24 8:16 ` Linus Walleij
2012-10-24 12:02 ` Grazvydas Ignotas
3 siblings, 0 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2012-10-24 7:33 UTC (permalink / raw)
To: Kevin Hilman
Cc: Linus Walleij, Felipe Balbi, linux-omap, Paul Walmsley,
Igor Grinberg, linux-arm-kernel
On Tuesday 23 October 2012 11:39 PM, Kevin Hilman wrote:
> From: Kevin Hilman <khilman@ti.com>
>
> When debounce clocks are disabled, ensure that the banks
> dbck_enable_mask is cleared also. Otherwise, context restore on
> subsequent off-mode transition will restore previous value from the
> shadow copies (bank->context.debounce*) leading to mismatch state
> between driver state and hardware state.
>
> This was discovered when board code was doing
>
> gpio_request_one()
> gpio_set_debounce()
> gpio_free()
>
> which was leaving the GPIO debounce settings in a confused state.
> Then, enabling off mode causing bogus state to be restored, leaving
> GPIO debounce enabled which then prevented the CORE powerdomain from
> transitioning.
>
> Reported-by: Paul Walmsley <paul@pwsan.com>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-23 22:00 ` Kevin Hilman
@ 2012-10-24 7:39 ` Felipe Balbi
0 siblings, 0 replies; 13+ messages in thread
From: Felipe Balbi @ 2012-10-24 7:39 UTC (permalink / raw)
To: Kevin Hilman
Cc: balbi, Linus Walleij, linux-omap, Paul Walmsley,
linux-arm-kernel, Igor Grinberg
[-- Attachment #1: Type: text/plain, Size: 3337 bytes --]
On Tue, Oct 23, 2012 at 03:00:09PM -0700, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
>
> > Hi,
> >
> > On Tue, Oct 23, 2012 at 11:09:31AM -0700, Kevin Hilman wrote:
> >> From: Kevin Hilman <khilman@ti.com>
> >>
> >> When debounce clocks are disabled, ensure that the banks
> >> dbck_enable_mask is cleared also. Otherwise, context restore on
> >> subsequent off-mode transition will restore previous value from the
> >> shadow copies (bank->context.debounce*) leading to mismatch state
> >> between driver state and hardware state.
> >>
> >> This was discovered when board code was doing
> >>
> >> gpio_request_one()
> >> gpio_set_debounce()
> >> gpio_free()
> >>
> >> which was leaving the GPIO debounce settings in a confused state.
> >> Then, enabling off mode causing bogus state to be restored, leaving
> >> GPIO debounce enabled which then prevented the CORE powerdomain from
> >> transitioning.
> >>
> >> Reported-by: Paul Walmsley <paul@pwsan.com>
> >> Cc: Igor Grinberg <grinberg@compulab.co.il>
> >> Signed-off-by: Kevin Hilman <khilman@ti.com>
> >
> > looks like this deserves a Cc: stable@vger.kernel.org tag.
> >
>
> Agreed. I think this goes all the way back to v3.5, but would've only
> been seen on boards using a request/gpio_set_debounce/free sequence
> combined with off-mode.
>
> Linus, feel free to add the Cc: stable when commiting. Thanks.
>
> >> ---
> >> Applies on v3.7-rc2, targetted for v3.7.
> >>
> >> drivers/gpio/gpio-omap.c | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> >> index 94cbc84..dee2856 100644
> >> --- a/drivers/gpio/gpio-omap.c
> >> +++ b/drivers/gpio/gpio-omap.c
> >> @@ -187,6 +187,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
> >> * to detect events and generate interrupts at least on OMAP3.
> >> */
> >> __raw_writel(0, bank->base + bank->regs->debounce_en);
> >> + bank->dbck_enable_mask = 0;
> >
> > shouldn't omap_gpio_restore_context() check for dbck_enabled instead of
> > the mask ? I mean:
> >
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index 94cbc84..b3a39a7 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -1371,7 +1371,7 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
> > bank->base + bank->regs->dataout);
> > __raw_writel(bank->context.oe, bank->base + bank->regs->direction);
> >
> > - if (bank->dbck_enable_mask) {
> > + if (bank->dbck_enabled) {
> > __raw_writel(bank->context.debounce, bank->base +
> > bank->regs->debounce);
> > __raw_writel(bank->context.debounce_en,
> >
> > the outcome would be the same, so it doesn't really matter. Just that,
> > at least to me, it would look better.
>
> I tried your version, and unfortunately, the outcome is not the same,
> but don't plan to look into why. $SUBJECT version is targetted and
> tested. If you want to cleanup the cosmetics here, please do in a
> subsequent patch. This driver could certainly benefit from more
> readability cleanups.
>
> > No strong feelings though.
>
> Good. I'll take that as an Ack. :)
please do:
Acked-by: Felipe Balbi <balbi@ti.com>
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-23 18:09 [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable Kevin Hilman
2012-10-23 19:09 ` Felipe Balbi
2012-10-24 7:33 ` Santosh Shilimkar
@ 2012-10-24 8:16 ` Linus Walleij
2012-10-24 8:56 ` Igor Grinberg
2012-10-24 12:02 ` Grazvydas Ignotas
3 siblings, 1 reply; 13+ messages in thread
From: Linus Walleij @ 2012-10-24 8:16 UTC (permalink / raw)
To: Kevin Hilman
Cc: Felipe Balbi, linux-omap, Paul Walmsley, linux-arm-kernel, Igor Grinberg
On Tue, Oct 23, 2012 at 8:09 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> From: Kevin Hilman <khilman@ti.com>
>
> When debounce clocks are disabled, ensure that the banks
> dbck_enable_mask is cleared also. Otherwise, context restore on
> subsequent off-mode transition will restore previous value from the
> shadow copies (bank->context.debounce*) leading to mismatch state
> between driver state and hardware state.
>
> This was discovered when board code was doing
>
> gpio_request_one()
> gpio_set_debounce()
> gpio_free()
>
> which was leaving the GPIO debounce settings in a confused state.
> Then, enabling off mode causing bogus state to be restored, leaving
> GPIO debounce enabled which then prevented the CORE powerdomain from
> transitioning.
>
> Reported-by: Paul Walmsley <paul@pwsan.com>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
Thanks! Applied with Felipe's and Santosh's ACKs.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-24 8:16 ` Linus Walleij
@ 2012-10-24 8:56 ` Igor Grinberg
0 siblings, 0 replies; 13+ messages in thread
From: Igor Grinberg @ 2012-10-24 8:56 UTC (permalink / raw)
To: Linus Walleij
Cc: Kevin Hilman, Paul Walmsley, linux-omap, linux-arm-kernel, Felipe Balbi
On 10/24/12 10:16, Linus Walleij wrote:
> On Tue, Oct 23, 2012 at 8:09 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>
>> From: Kevin Hilman <khilman@ti.com>
>>
>> When debounce clocks are disabled, ensure that the banks
>> dbck_enable_mask is cleared also. Otherwise, context restore on
>> subsequent off-mode transition will restore previous value from the
>> shadow copies (bank->context.debounce*) leading to mismatch state
>> between driver state and hardware state.
>>
>> This was discovered when board code was doing
>>
>> gpio_request_one()
>> gpio_set_debounce()
>> gpio_free()
>>
>> which was leaving the GPIO debounce settings in a confused state.
>> Then, enabling off mode causing bogus state to be restored, leaving
>> GPIO debounce enabled which then prevented the CORE powerdomain from
>> transitioning.
>>
>> Reported-by: Paul Walmsley <paul@pwsan.com>
>> Cc: Igor Grinberg <grinberg@compulab.co.il>
>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>
> Thanks! Applied with Felipe's and Santosh's ACKs.
If not too late:
Acked-by: Igor Grinberg <grinberg@compulab.co.il>
Kevin, thanks for the patch and sorry I could not respond on time.
--
Regards,
Igor.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-23 18:09 [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable Kevin Hilman
` (2 preceding siblings ...)
2012-10-24 8:16 ` Linus Walleij
@ 2012-10-24 12:02 ` Grazvydas Ignotas
2012-10-24 12:49 ` Santosh Shilimkar
2012-10-24 14:19 ` Kevin Hilman
3 siblings, 2 replies; 13+ messages in thread
From: Grazvydas Ignotas @ 2012-10-24 12:02 UTC (permalink / raw)
To: Kevin Hilman
Cc: Linus Walleij, Felipe Balbi, linux-omap, Paul Walmsley,
Igor Grinberg, linux-arm-kernel
On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> From: Kevin Hilman <khilman@ti.com>
>
> When debounce clocks are disabled, ensure that the banks
> dbck_enable_mask is cleared also. Otherwise, context restore on
> subsequent off-mode transition will restore previous value from the
> shadow copies (bank->context.debounce*) leading to mismatch state
> between driver state and hardware state.
This doesn't look right to me, aren't you effectively disabling
debounce forever here? _gpio_dbck_disable is called from
omap_gpio_runtime_suspend() and nothing will ever restore
dbck_enable_mask back to what it was set by _set_gpio_debounce and
debounce functionality is lost.
>
> This was discovered when board code was doing
>
> gpio_request_one()
> gpio_set_debounce()
> gpio_free()
>
> which was leaving the GPIO debounce settings in a confused state.
> Then, enabling off mode causing bogus state to be restored, leaving
> GPIO debounce enabled which then prevented the CORE powerdomain from
> transitioning.
>
> Reported-by: Paul Walmsley <paul@pwsan.com>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
> Applies on v3.7-rc2, targetted for v3.7.
>
> drivers/gpio/gpio-omap.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 94cbc84..dee2856 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -187,6 +187,7 @@ static inline void _gpio_dbck_disable(struct gpio_bank *bank)
> * to detect events and generate interrupts at least on OMAP3.
> */
> __raw_writel(0, bank->base + bank->regs->debounce_en);
> + bank->dbck_enable_mask = 0;
>
> clk_disable(bank->dbck);
> bank->dbck_enabled = false;
> --
> 1.8.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-24 12:02 ` Grazvydas Ignotas
@ 2012-10-24 12:49 ` Santosh Shilimkar
2012-10-24 13:40 ` Grazvydas Ignotas
2012-10-24 14:19 ` Kevin Hilman
1 sibling, 1 reply; 13+ messages in thread
From: Santosh Shilimkar @ 2012-10-24 12:49 UTC (permalink / raw)
To: Grazvydas Ignotas
Cc: Kevin Hilman, Paul Walmsley, Linus Walleij, Felipe Balbi,
Igor Grinberg, linux-omap, linux-arm-kernel
On Wednesday 24 October 2012 05:32 PM, Grazvydas Ignotas wrote:
> On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> From: Kevin Hilman <khilman@ti.com>
>>
>> When debounce clocks are disabled, ensure that the banks
>> dbck_enable_mask is cleared also. Otherwise, context restore on
>> subsequent off-mode transition will restore previous value from the
>> shadow copies (bank->context.debounce*) leading to mismatch state
>> between driver state and hardware state.
>
> This doesn't look right to me, aren't you effectively disabling
> debounce forever here? _gpio_dbck_disable is called from
> omap_gpio_runtime_suspend() and nothing will ever restore
> dbck_enable_mask back to what it was set by _set_gpio_debounce and
> debounce functionality is lost.
>
As per commit log, the issue seen with request->debounce->free()
sequence and hence on free, debounce state needs to be cleared.
Next gpio_set_debounce() should set the debounce mask right so
the patch seems to be right.
>>
>> This was discovered when board code was doing
>>
>> gpio_request_one()
>> gpio_set_debounce()
>> gpio_free()
>>
>> which was leaving the GPIO debounce settings in a confused state.
>> Then, enabling off mode causing bogus state to be restored, leaving
>> GPIO debounce enabled which then prevented the CORE powerdomain from
>> transitioning.
>>
But there is one more case which might break because of this patch.
As part of idle, we might end up with below without gpio_free()
omap2_gpio_prepare_for_idle()
->pm_runtime_put_sync_suspend()
-> omap_gpio_runtime_suspend()
->_gpio_dbck_disable()
And last call will clear the debounce mask state which will lost and
hence the debounce clock won't be enabled on resume.
I let Kevin comment whether this is the valid case or not.
Regards
Santosh
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-24 12:49 ` Santosh Shilimkar
@ 2012-10-24 13:40 ` Grazvydas Ignotas
0 siblings, 0 replies; 13+ messages in thread
From: Grazvydas Ignotas @ 2012-10-24 13:40 UTC (permalink / raw)
To: Santosh Shilimkar
Cc: Kevin Hilman, Paul Walmsley, Linus Walleij, Felipe Balbi,
Igor Grinberg, linux-omap, linux-arm-kernel
On Wed, Oct 24, 2012 at 3:49 PM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> On Wednesday 24 October 2012 05:32 PM, Grazvydas Ignotas wrote:
>>
>> On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>>>
>>> From: Kevin Hilman <khilman@ti.com>
>>>
>>> When debounce clocks are disabled, ensure that the banks
>>> dbck_enable_mask is cleared also. Otherwise, context restore on
>>> subsequent off-mode transition will restore previous value from the
>>> shadow copies (bank->context.debounce*) leading to mismatch state
>>> between driver state and hardware state.
>>
>>
>> This doesn't look right to me, aren't you effectively disabling
>> debounce forever here? _gpio_dbck_disable is called from
>> omap_gpio_runtime_suspend() and nothing will ever restore
>> dbck_enable_mask back to what it was set by _set_gpio_debounce and
>> debounce functionality is lost.
>>
> As per commit log, the issue seen with request->debounce->free()
> sequence and hence on free, debounce state needs to be cleared.
> Next gpio_set_debounce() should set the debounce mask right so
> the patch seems to be right.
>
>
>>>
>>> This was discovered when board code was doing
>>>
>>> gpio_request_one()
>>> gpio_set_debounce()
>>> gpio_free()
>>>
>>> which was leaving the GPIO debounce settings in a confused state.
>>> Then, enabling off mode causing bogus state to be restored, leaving
>>> GPIO debounce enabled which then prevented the CORE powerdomain from
>>> transitioning.
>>>
>
> But there is one more case which might break because of this patch.
> As part of idle, we might end up with below without gpio_free()
> omap2_gpio_prepare_for_idle()
> ->pm_runtime_put_sync_suspend()
> -> omap_gpio_runtime_suspend()
> ->_gpio_dbck_disable()
>
> And last call will clear the debounce mask state which will lost and
> hence the debounce clock won't be enabled on resume.
>
> I let Kevin comment whether this is the valid case or not.
That's exactly what I had in mind - we lose debounce functionality on
first runtime suspend.
--
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-24 12:02 ` Grazvydas Ignotas
2012-10-24 12:49 ` Santosh Shilimkar
@ 2012-10-24 14:19 ` Kevin Hilman
2012-10-24 14:38 ` Santosh Shilimkar
2012-10-26 7:21 ` Linus Walleij
1 sibling, 2 replies; 13+ messages in thread
From: Kevin Hilman @ 2012-10-24 14:19 UTC (permalink / raw)
To: Grazvydas Ignotas
Cc: Linus Walleij, Felipe Balbi, linux-omap, Paul Walmsley,
Igor Grinberg, linux-arm-kernel
Grazvydas Ignotas <notasas@gmail.com> writes:
> On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman
> <khilman@deeprootsystems.com> wrote:
>> From: Kevin Hilman <khilman@ti.com>
>>
>> When debounce clocks are disabled, ensure that the banks
>> dbck_enable_mask is cleared also. Otherwise, context restore on
>> subsequent off-mode transition will restore previous value from the
>> shadow copies (bank->context.debounce*) leading to mismatch state
>> between driver state and hardware state.
>
> This doesn't look right to me, aren't you effectively disabling
> debounce forever here? _gpio_dbck_disable is called from
> omap_gpio_runtime_suspend() and nothing will ever restore
> dbck_enable_mask back to what it was set by _set_gpio_debounce and
> debounce functionality is lost.
Yes, you're right. Good catch.
I need a fix that's more targetted to the free/reset path.
Linus, please revert if it's not too late, and I'll come up with a more
targetted fix.
Kevin
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-24 14:19 ` Kevin Hilman
@ 2012-10-24 14:38 ` Santosh Shilimkar
2012-10-26 7:21 ` Linus Walleij
1 sibling, 0 replies; 13+ messages in thread
From: Santosh Shilimkar @ 2012-10-24 14:38 UTC (permalink / raw)
To: Kevin Hilman
Cc: Grazvydas Ignotas, Paul Walmsley, Linus Walleij, Felipe Balbi,
Igor Grinberg, linux-omap, linux-arm-kernel
On Wednesday 24 October 2012 07:49 PM, Kevin Hilman wrote:
> Grazvydas Ignotas <notasas@gmail.com> writes:
>
>> On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman
>> <khilman@deeprootsystems.com> wrote:
>>> From: Kevin Hilman <khilman@ti.com>
>>>
>>> When debounce clocks are disabled, ensure that the banks
>>> dbck_enable_mask is cleared also. Otherwise, context restore on
>>> subsequent off-mode transition will restore previous value from the
>>> shadow copies (bank->context.debounce*) leading to mismatch state
>>> between driver state and hardware state.
>>
>> This doesn't look right to me, aren't you effectively disabling
>> debounce forever here? _gpio_dbck_disable is called from
>> omap_gpio_runtime_suspend() and nothing will ever restore
>> dbck_enable_mask back to what it was set by _set_gpio_debounce and
>> debounce functionality is lost.
>
> Yes, you're right. Good catch.
>
> I need a fix that's more targetted to the free/reset path.
>
Right. Just clearing the debounce mask in omap_gpio_free()
should do the trick.
Regards
Santosh
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
2012-10-24 14:19 ` Kevin Hilman
2012-10-24 14:38 ` Santosh Shilimkar
@ 2012-10-26 7:21 ` Linus Walleij
1 sibling, 0 replies; 13+ messages in thread
From: Linus Walleij @ 2012-10-26 7:21 UTC (permalink / raw)
To: Kevin Hilman
Cc: Grazvydas Ignotas, Felipe Balbi, linux-omap, Paul Walmsley,
Igor Grinberg, linux-arm-kernel
On Wed, Oct 24, 2012 at 4:19 PM, Kevin Hilman
<khilman@deeprootsystems.com> wrote:
> Linus, please revert if it's not too late, and I'll come up with a more
> targetted fix.
OK I ditched it, no big deal.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2012-10-26 7:21 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-23 18:09 [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable Kevin Hilman
2012-10-23 19:09 ` Felipe Balbi
2012-10-23 22:00 ` Kevin Hilman
2012-10-24 7:39 ` Felipe Balbi
2012-10-24 7:33 ` Santosh Shilimkar
2012-10-24 8:16 ` Linus Walleij
2012-10-24 8:56 ` Igor Grinberg
2012-10-24 12:02 ` Grazvydas Ignotas
2012-10-24 12:49 ` Santosh Shilimkar
2012-10-24 13:40 ` Grazvydas Ignotas
2012-10-24 14:19 ` Kevin Hilman
2012-10-24 14:38 ` Santosh Shilimkar
2012-10-26 7:21 ` Linus Walleij
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).