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

From: Kevin Hilman <[email protected]>

When a GPIO bank is freed or shutdown, 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. If that GPIO bank is subsequently used with off-mode enabled, bogus state would be restored, leaving GPIO debounce enabled which then prevented the CORE powerdomain from transitioning.

To fix, ensure that bank->dbck_enable_mask is cleared when the bank is freed/shutdown so debounce state doesn't persist.

Special thanks to Grazvydas Ignotas for pointing out a bug in an earlier version that would've disabled debounce on any runtime PM transition.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

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

---
v2: only clear mask in free/shutdown, not in runtime PM paths,
clarified changelog
Applies on v3.7-rc2.

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

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

index 94cbc84..113b167 100644

Quoted 14 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

When a GPIO bank is freed or shutdown, 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()

Quoted 8 lines

which was leaving the GPIO debounce settings in a confused state. If that GPIO bank is subsequently used with off-mode enabled, bogus state would be restored, leaving GPIO debounce enabled which then prevented the CORE powerdomain from transitioning.

To fix, ensure that bank->dbck_enable_mask is cleared when the bank is freed/shutdown so debounce state doesn't persist.

Quoted 21 lines

Special thanks to Grazvydas Ignotas for pointing out a bug in an earlier version that would've disabled debounce on any runtime PM transition.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

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

---
v2: only clear mask in free/shutdown, not in runtime PM paths,
clarified changelog
Applies on v3.7-rc2.

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

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

index 94cbc84..113b167 100644

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, int gpio) · line 543

Quoted 5 lines

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

@@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank, int gpio)
  	_set_gpio_irqenable(bank, gpio, 0);
  	_clear_gpio_irqstatus(bank, gpio);
  	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+	bank->dbck_enable_mask = 0;
  }

Does this need to be ...

There could be more than one gpio using debounce and so we should only clear the appropriate bit. Also after clearing a bit we could see if we can disable the debounce clock too.

Quoted 21 lines · Jon Hunter

Quoted 19 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

When a GPIO bank is freed or shutdown, 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. If that GPIO bank is subsequently used with off-mode enabled, bogus state would be restored, leaving GPIO debounce enabled which then prevented the CORE powerdomain from transitioning.

Quoted 2 lines

Quoted 2 lines

To fix, ensure that bank->dbck_enable_mask is cleared when the bank is freed/shutdown so debounce state doesn't persist.

The freed part is fine but I don't understand why it needs to be done on _a_ gpio irq shutdown callback where IRQs related configuration on that one GPIO needs to be cleared. De-bounce clock is surely not IRQ related configuration.

Quoted 31 lines

Quoted 21 lines

Special thanks to Grazvydas Ignotas for pointing out a bug in an earlier version that would've disabled debounce on any runtime PM transition.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

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

---
v2: only clear mask in free/shutdown, not in runtime PM paths,
clarified changelog
Applies on v3.7-rc2.

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

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

index 94cbc84..113b167 100644

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, · line 544

Quoted 6 lines

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

@@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank,
 int gpio)
      _set_gpio_irqenable(bank, gpio, 0);
      _clear_gpio_irqstatus(bank, gpio);
      _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+    bank->dbck_enable_mask = 0;
  }

Does this need to be ...

Review

Quoted 2 lines

(quoted diff) +2 -0

+    bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
+    _gpio_dbck_disable(bank);

Yes, its a per bank clock. There is an alternate. See below.

Quoted 4 lines

There could be more than one gpio using debounce and so we should only clear the appropriate bit. Also after clearing a bit we could see if we can disable the debounce clock too.

When I mentioned the clearing in gpio_free() path will do trick, I had something like below in mind.

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

index dee2856..8574105 100644

Quoted 24 lines · Santosh Shilimkar

Quoted 23 lines · Jon Hunter

Quoted 21 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

When a GPIO bank is freed or shutdown, 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. If that GPIO bank is subsequently used with off-mode enabled, bogus state would be restored, leaving GPIO debounce enabled which then prevented the CORE powerdomain from transitioning.

To fix, ensure that bank->dbck_enable_mask is cleared when the bank is freed/shutdown so debounce state doesn't persist.

Quoted 4 lines

The freed part is fine but I don't understand why it needs to be done on _a_ gpio irq shutdown callback where IRQs related configuration on that one GPIO needs to be cleared. De-bounce clock is surely not IRQ related configuration.

If we are freeing the IRQs related to gpio and resetting the gpio, then I don't see why we should not. We should not leave the debounce clock on if these gpios are no longer being used.

Quoted 53 lines

Quoted 31 lines

Quoted 21 lines

Special thanks to Grazvydas Ignotas for pointing out a bug in an earlier version that would've disabled debounce on any runtime PM transition.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

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

---
v2: only clear mask in free/shutdown, not in runtime PM paths,
clarified changelog
Applies on v3.7-rc2.

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

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

index 94cbc84..113b167 100644

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, · line 544

Quoted 6 lines

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

@@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank,
 int gpio)
       _set_gpio_irqenable(bank, gpio, 0);
       _clear_gpio_irqstatus(bank, gpio);
       _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+    bank->dbck_enable_mask = 0;
   }

Does this need to be ...

Review

Quoted 2 lines

(quoted diff) +2 -0

+    bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
+    _gpio_dbck_disable(bank);

Yes, its a per bank clock. There is an alternate. See below.

Quoted 4 lines

There could be more than one gpio using debounce and so we should only clear the appropriate bit. Also after clearing a bit we could see if we can disable the debounce clock too.

When I mentioned the clearing in gpio_free() path will do trick, I had something like below in mind.

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

index dee2856..8574105 100644
@@ -629,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
 unsigned offset)
       * If this is the last gpio to be freed in the bank,
       * disable the bank module.
       */
-    if (!bank->mod_usage)
+    if (!bank->mod_usage) {

drivers/gpio/gpio-omap.c static void omap_gpio_free(struct gpio_chip *chip, · line 636

Quoted 3 lines

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

+        bank->dbck_enable_mask = 0;
          pm_runtime_put(bank->dev);
+    }

However, with this we could be leaving the debounce clock on longer than needed. I think we need to call _gpio_dbck_disable() each time we free a gpio and this function will determine if it can turn off the debounce clock.

In fact, there appears to be another bug in the current driver, that we do not clear the debounce_en register when freeing the gpio. Your patch addresses this, but I think that debounce should be disabled when a gpio is freed and not just when the last one is freed.

Also, with the above change, can't we still run into the original problem? In other words, if a gpio is freed, but there is still another one active in the back that is not using debounce, then we could restore a incorrect debounce context because we have not clean-up the debounce settings?

So may be we need to add a _clear_gpio_debounce() function and call this when freeing a gpio.

Quoted 22 lines · Santosh Shilimkar

Quoted 21 lines · Jon Hunter

Quoted 19 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

When a GPIO bank is freed or shutdown, 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. If that GPIO bank is subsequently used with off-mode enabled, bogus state would be restored, leaving GPIO debounce enabled which then prevented the CORE powerdomain from transitioning.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

To fix, ensure that bank->dbck_enable_mask is cleared when the bank is freed/shutdown so debounce state doesn't persist.

The freed part is fine but I don't understand why it needs to be done on _a_ gpio irq shutdown callback where IRQs related configuration on that one GPIO needs to be cleared. De-bounce clock is surely not IRQ related configuration.

If we are freeing the IRQs related to gpio and resetting the gpio, then I don't see why we should not. We should not leave the debounce clock on if these gpios are no longer being used.

Quoted 47 lines

Quoted 31 lines

Quoted 21 lines

Special thanks to Grazvydas Ignotas for pointing out a bug in an earlier version that would've disabled debounce on any runtime PM transition.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

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

---
v2: only clear mask in free/shutdown, not in runtime PM paths,
clarified changelog
Applies on v3.7-rc2.

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

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

index 94cbc84..113b167 100644

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, · line 544

Quoted 6 lines

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

@@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank,
 int gpio)
      _set_gpio_irqenable(bank, gpio, 0);
      _clear_gpio_irqstatus(bank, gpio);
      _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+    bank->dbck_enable_mask = 0;
  }

Does this need to be ...

Review

Quoted 2 lines

(quoted diff) +2 -0

+    bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
+    _gpio_dbck_disable(bank);

Yes, its a per bank clock. There is an alternate. See below.

Quoted 4 lines

There could be more than one gpio using debounce and so we should only clear the appropriate bit. Also after clearing a bit we could see if we can disable the debounce clock too.

When I mentioned the clearing in gpio_free() path will do trick, I had something like below in mind.

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

index dee2856..8574105 100644

drivers/gpio/gpio-omap.c static void omap_gpio_free(struct gpio_chip *chip, · line 636

Quoted 9 lines

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

@@ -629,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
 unsigned offset)
      * If this is the last gpio to be freed in the bank,
      * disable the bank module.
      */
-    if (!bank->mod_usage)
+    if (!bank->mod_usage) {
+        bank->dbck_enable_mask = 0;
         pm_runtime_put(bank->dev);
+    }

However, with this we could be leaving the debounce clock on longer than needed. I think we need to call _gpio_dbck_disable() each time we free a gpio and this function will determine if it can turn off the debounce clock.

In fact, there appears to be another bug in the current driver, that we do not clear the debounce_en register when freeing the gpio. Your patch addresses this, but I think that debounce should be disabled when a gpio is freed and not just when the last one is freed.

Also, with the above change, can't we still run into the original problem? In other words, if a gpio is freed, but there is still another one active in the back that is not using debounce, then we could restore a incorrect debounce context because we have not clean-up the debounce settings?

So may be we need to add a _clear_gpio_debounce() function and call this when freeing a gpio.

Quoted 107 lines · Jon Hunter

Quoted 22 lines · Santosh Shilimkar

Quoted 21 lines · Jon Hunter

Quoted 19 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

When a GPIO bank is freed or shutdown, 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. If that GPIO bank is subsequently used with off-mode enabled, bogus state would be restored, leaving GPIO debounce enabled which then prevented the CORE powerdomain from transitioning.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

To fix, ensure that bank->dbck_enable_mask is cleared when the bank is freed/shutdown so debounce state doesn't persist.

The freed part is fine but I don't understand why it needs to be done on _a_ gpio irq shutdown callback where IRQs related configuration on that one GPIO needs to be cleared. De-bounce clock is surely not IRQ related configuration.

If we are freeing the IRQs related to gpio and resetting the gpio, then I don't see why we should not. We should not leave the debounce clock on if these gpios are no longer being used.

Quoted 47 lines

Quoted 31 lines

Quoted 21 lines

Special thanks to Grazvydas Ignotas for pointing out a bug in an earlier version that would've disabled debounce on any runtime PM transition.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

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

---
v2: only clear mask in free/shutdown, not in runtime PM paths,
clarified changelog
Applies on v3.7-rc2.

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

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

index 94cbc84..113b167 100644

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, · line 544

Quoted 6 lines

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

@@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank,
 int gpio)
      _set_gpio_irqenable(bank, gpio, 0);
      _clear_gpio_irqstatus(bank, gpio);
      _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+    bank->dbck_enable_mask = 0;
  }

Does this need to be ...

Review

Quoted 2 lines

(quoted diff) +2 -0

+    bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
+    _gpio_dbck_disable(bank);

Yes, its a per bank clock. There is an alternate. See below.

Quoted 4 lines

There could be more than one gpio using debounce and so we should only clear the appropriate bit. Also after clearing a bit we could see if we can disable the debounce clock too.

When I mentioned the clearing in gpio_free() path will do trick, I had something like below in mind.

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

index dee2856..8574105 100644

drivers/gpio/gpio-omap.c static void omap_gpio_free(struct gpio_chip *chip, · line 636

Quoted 9 lines

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

@@ -629,8 +629,10 @@ static void omap_gpio_free(struct gpio_chip *chip,
 unsigned offset)
      * If this is the last gpio to be freed in the bank,
      * disable the bank module.
      */
-    if (!bank->mod_usage)
+    if (!bank->mod_usage) {
+        bank->dbck_enable_mask = 0;
         pm_runtime_put(bank->dev);
+    }

However, with this we could be leaving the debounce clock on longer than needed. I think we need to call _gpio_dbck_disable() each time we free a gpio and this function will determine if it can turn off the debounce clock.

In fact, there appears to be another bug in the current driver, that we do not clear the debounce_en register when freeing the gpio. Your patch addresses this, but I think that debounce should be disabled when a gpio is freed and not just when the last one is freed.

Also, with the above change, can't we still run into the original problem? In other words, if a gpio is freed, but there is still another one active in the back that is not using debounce, then we could restore a incorrect debounce context because we have not clean-up the debounce settings?

Quoted 2 lines

So may be we need to add a _clear_gpio_debounce() function and call this when freeing a gpio.

Care to cook up a patch for this, on top of v3 of $SUBJECT patch?

Quoted 43 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>

When a GPIO bank is freed or shutdown, 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. If that GPIO bank is subsequently used with off-mode enabled, bogus state would be restored, leaving GPIO debounce enabled which then prevented the CORE powerdomain from transitioning.

To fix, ensure that bank->dbck_enable_mask is cleared when the bank is freed/shutdown so debounce state doesn't persist.

Special thanks to Grazvydas Ignotas for pointing out a bug in an earlier version that would've disabled debounce on any runtime PM transition.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

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

---
v2: only clear mask in free/shutdown, not in runtime PM paths,
clarified changelog
Applies on v3.7-rc2.

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

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

index 94cbc84..113b167 100644

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, int gpio) · line 543

Quoted 5 lines

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

@@ -539,6 +539,7 @@ static void _reset_gpio(struct gpio_bank *bank, int gpio)
 	_set_gpio_irqenable(bank, gpio, 0);
 	_clear_gpio_irqstatus(bank, gpio);
 	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+	bank->dbck_enable_mask = 0;
 }

Does this need to be ...

(quoted diff) +2 -0

+	bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
+	_gpio_dbck_disable(bank);

There could be more than one gpio using debounce and so we should only clear the appropriate bit. Also after clearing a bit we could see if we can disable the debounce clock too.