Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Wed, Oct 24, 2012 at 4:19 PM, Kevin Hilman 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Wednesday 24 October 2012 07:49 PM, Kevin Hilman wrote: Grazvydas Ignotas writes: On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman wrote: From: Kevin Hilman 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
Grazvydas Ignotas writes: > On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman > wrote: >> From: Kevin Hilman >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Wed, Oct 24, 2012 at 3:49 PM, Santosh Shilimkar wrote: > On Wednesday 24 October 2012 05:32 PM, Grazvydas Ignotas wrote: >> >> On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman >> wrote: >>> >>> From: Kevin Hilman >>> >>> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Wednesday 24 October 2012 05:32 PM, Grazvydas Ignotas wrote: On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman wrote: From: Kevin Hilman 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Tue, Oct 23, 2012 at 9:09 PM, Kevin Hilman wrote: > From: Kevin Hilman > > 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 > Cc: Igor Grinberg > Signed-off-by: Kevin Hilman > --- > 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-ker...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On 10/24/12 10:16, Linus Walleij wrote: > On Tue, Oct 23, 2012 at 8:09 PM, Kevin Hilman > wrote: > >> From: Kevin Hilman >> >> 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 >> Cc: Igor Grinberg >> Signed-off-by: Kevin Hilman > > Thanks! Applied with Felipe's and Santosh's ACKs. If not too late: Acked-by: Igor Grinberg Kevin, thanks for the patch and sorry I could not respond on time. -- Regards, Igor. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Tue, Oct 23, 2012 at 8:09 PM, Kevin Hilman wrote: > From: Kevin Hilman > > 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 > Cc: Igor Grinberg > Signed-off-by: Kevin Hilman Thanks! Applied with Felipe's and Santosh's ACKs. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Tue, Oct 23, 2012 at 03:00:09PM -0700, Kevin Hilman wrote:
> Felipe Balbi writes:
>
> > Hi,
> >
> > On Tue, Oct 23, 2012 at 11:09:31AM -0700, Kevin Hilman wrote:
> >> From: Kevin Hilman
> >>
> >> 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
> >> Cc: Igor Grinberg
> >> Signed-off-by: Kevin Hilman
> >
> > looks like this deserves a Cc: sta...@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
signature.asc
Description: Digital signature
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
On Tuesday 23 October 2012 11:39 PM, Kevin Hilman wrote: From: Kevin Hilman 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 Cc: Igor Grinberg Signed-off-by: Kevin Hilman --- Acked-by: Santosh Shilimkar -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
Felipe Balbi writes:
> Hi,
>
> On Tue, Oct 23, 2012 at 11:09:31AM -0700, Kevin Hilman wrote:
>> From: Kevin Hilman
>>
>> 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
>> Cc: Igor Grinberg
>> Signed-off-by: Kevin Hilman
>
> looks like this deserves a Cc: sta...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
Hi,
On Tue, Oct 23, 2012 at 11:09:31AM -0700, Kevin Hilman wrote:
> From: Kevin Hilman
>
> 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
> Cc: Igor Grinberg
> Signed-off-by: Kevin Hilman
looks like this deserves a Cc: sta...@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
signature.asc
Description: Digital signature
[PATCH] gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable
From: Kevin Hilman 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 Cc: Igor Grinberg Signed-off-by: Kevin Hilman --- 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
