linux-omap.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).