gpio/omap: fix off-mode bug: clear debounce clock enable mask on disable

From: Kevin Hilman <[email protected]>

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 <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---
Applies on v3.7-rc2, targetted for v3.7.

 drivers/gpio/gpio-omap.c | 1 +
 1 file changed, 1 insertion(+)

drivers/gpio/gpio-omap.c +0 -0

index 94cbc84..dee2856 100644

Hi,

Quoted 19 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Quoted 3 lines

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

looks like this deserves a Cc: [email protected] tag.

Quoted 11 lines

---
Applies on v3.7-rc2, targetted for v3.7.

 drivers/gpio/gpio-omap.c | 1 +
 1 file changed, 1 insertion(+)

drivers/gpio/gpio-omap.c +0 -0

index 94cbc84..dee2856 100644

drivers/gpio/gpio-omap.c static inline void _gpio_dbck_disable(struct gpio_bank *bank) · line 190

Quoted 4 lines

drivers/gpio/gpio-omap.c +1 -0

@@ -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:

drivers/gpio/gpio-omap.c +1 -1

index 94cbc84..b3a39a7 100644
@@ -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.

Quoted 22 lines · Felipe Balbi

Hi,

Quoted 19 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Quoted 6 lines

Quoted 3 lines

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

looks like this deserves a Cc: stable at 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.

Quoted 33 lines

Quoted 11 lines

---
Applies on v3.7-rc2, targetted for v3.7.

 drivers/gpio/gpio-omap.c | 1 +
 1 file changed, 1 insertion(+)

drivers/gpio/gpio-omap.c +0 -0

index 94cbc84..dee2856 100644

drivers/gpio/gpio-omap.c static inline void _gpio_dbck_disable(struct gpio_bank *bank) · line 190

Quoted 4 lines

drivers/gpio/gpio-omap.c +1 -0

@@ -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:

drivers/gpio/gpio-omap.c +1 -1

index 94cbc84..b3a39a7 100644
@@ -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,

Quoted 2 lines

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.

Quoted 1 line

No strong feelings though.

Good. I'll take that as an Ack. :)

From: Kevin Hilman <[email protected]>

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 <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---

From: Kevin Hilman <[email protected]>

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 <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

Quoted 19 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Quoted 3 lines

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

Thanks! Applied with Felipe's and Santosh's ACKs.

From: Kevin Hilman <[email protected]>

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 <[email protected]>

Cc Igor Grinberg <[email protected]>

Signed-off-by Kevin Hilman <[email protected]>

---
Applies on v3.7-rc2, targetted for v3.7.

 drivers/gpio/gpio-omap.c | 1 +
 1 file changed, 1 insertion(+)

drivers/gpio/gpio-omap.c +1 -1

index 94cbc84..dee2856 100644
@@ -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 at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 7 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Quoted 7 lines

This was discovered when board code was doing

   gpio_request_one()
   gpio_set_debounce()
   gpio_free()

Quoted 5 lines

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.

Quoted 13 lines · Grazvydas Ignotas

Quoted 8 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Quoted 6 lines

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.

Quoted 7 lines

Quoted 7 lines

This was discovered when board code was doing

   gpio_request_one()
   gpio_set_debounce()
   gpio_free()

Quoted 5 lines

Quoted 5 lines

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.

Quoted 7 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Quoted 10 lines · Grazvydas Ignotas

Quoted 7 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

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.

Quoted 5 lines

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.