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

PATCH Superseded Kevin Hilman 4 participants 6 messages Oct 25, 2012 – Oct 27, 2012

Subsystems (2)

GPIO OMAP GPIO driver

lore

Share & tools

lore

Kevin Hilman

author

2012-10-25 16:34 UTC #0 lore raw

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

From: Kevin Hilman <[email protected]>

When a GPIO is freed or shutdown, ensure that the proper bit in dbck_enable_mask is cleared also. Otherwise, context restore on subsequent off-mode transition will restore previous debounce values 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 right bit in bank->dbck_enable_mask is cleared when a GPIO is freed/shutdown so debounce state doesn't persist after free/reset. If this GPIO is the last debounce-enabled GPIO in the bank, the debounce will also be cut.

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

And, special thanks to Jon Hunter for pointing out a bug in the second version which was mistakenly clearing all debounce bits on reset instead of individual GPIOs, as well as suggesting cutting the debounce clock after all debounce bits are cleared.

Tesed on 37xx/EVM board which configures GPIO debounce for the ads7846 touchscreen in its board file using the above sequence, and so was failing off-mode tests in dynamic idle. Verified that off-mode tests are passing with this patch.

Reported-by Paul Walmsley <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Grazvydas Ignotas <[email protected]>

Cc Jon Hunter <[email protected]>

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

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

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

index 94cbc84..ce1da19 100644
@@ -539,6 +539,8 @@ 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 &= ~(GPIO_BIT(bank, gpio));
+	_gpio_dbck_disable(bank);
 }
 
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */

Signature · 1 line

1.8.0

Jon Hunter

2012-10-25 22:52 UTC #1 lore raw

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

Hi Kevin,

Quoted 24 lines · Kevin Hilman

From: Kevin Hilman <[email protected]>
When a GPIO is freed or shutdown, ensure that the proper bit in dbck_enable_mask is cleared also. Otherwise, context restore on subsequent off-mode transition will restore previous debounce values 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 right bit in bank->dbck_enable_mask is cleared when a GPIO is freed/shutdown so debounce state doesn't persist after free/reset. If this GPIO is the last debounce-enabled GPIO in the bank, the debounce will also be cut.

Quoted 8 lines

Special thanks to Grazvydas Ignotas for pointing out a bug in the first version that would've disabled debounce on any runtime PM transition.
And, special thanks to Jon Hunter for pointing out a bug in the second version which was mistakenly clearing all debounce bits on reset instead of individual GPIOs, as well as suggesting cutting the debounce clock after all debounce bits are cleared.

... and for introducing yet another bug :-(

Quoted 19 lines

Tesed on 37xx/EVM board which configures GPIO debounce for the ads7846 touchscreen in its board file using the above sequence, and so was failing off-mode tests in dynamic idle. Verified that off-mode tests are passing with this patch.
Reported-by Paul Walmsley <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Grazvydas Ignotas <[email protected]>
Cc Jon Hunter <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
---
 drivers/gpio/gpio-omap.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpio/gpio-omap.c +0 -0
index 94cbc84..ce1da19 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 +2 -0

@@ -539,6 +539,8 @@ 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 &= ~(GPIO_BIT(bank, gpio));
+	_gpio_dbck_disable(bank);

We can't use _gpio_dbck_disable() here. This has been specifically implemented for the suspend path and is designed to disable the debounce clock while debounce is enabled (which makes sense). Yes this needs to be cleaned up.

I have implemented the following and unit tested. Care to test on your 37xx board? Sorry I would do it myself I had one.

Also not sure if you just wish to squash your patch and mine together. This is based on top of yours.

Cheers
Jon

Quoted 1 line

From 33812f3bd4f7aab1154e7194b7f11fba700a5086 Mon Sep 17 00:00:00 2001

From: Jon Hunter <[email protected]>
Date: Thu, 25 Oct 2012 16:00:51 -0500
Subject: [PATCH] gpio/omap: fix clearing of debounce settings on gpio
free/reset

When a GPIO is freed or shutdown, we need to ensure that any debounce settings are cleared and if the GPIO is the only GPIO in the bank that is currently using debounce, then disable the debounce clock as well to save power.

Therefore, introduce a new function called _clear_gpio_debounce() to clear any debounce settings when the GPIO is freed or shutdown.

Please note that we cannot use _gpio_dbck_disable() to disable the debounce clock because this has been specifically created for the gpio suspend path and is intended to shutdown the debounce clock while debounce is enabled.

This has been unit tested on an OMAP3430 Beagle board, by requesting a gpio, enabling debounce and then freeing the gpio and checking the register contents, the saved register context and the debounce clock state.

Signed-off-by Jon Hunter <[email protected]>

---
 drivers/gpio/gpio-omap.c |   37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

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

index ce1da19..d335af1 100644
@@ -251,6 +251,40 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
 	}
 }
 
+/**
+ * _clear_gpio_debounce - clear debounce settings for a gpio
+ * @bank: the gpio bank we're acting upon
+ * @gpio: the gpio number on this @gpio
+ *
+ * If a gpio is using debounce, then clear the debounce enable bit and if
+ * this is the only gpio in this bank using debounce, then clear the debounce
+ * time too. The debounce clock will also be disabled when calling this function
+ * if this is the only gpio in the bank using debounce.
+ */
+static void _clear_gpio_debounce(struct gpio_bank *bank, unsigned gpio)
+{
+	u32 gpio_bit = GPIO_BIT(bank, gpio);
+
+	if (!bank->dbck_flag)
+		return;
+
+	if (!(bank->dbck_enable_mask & gpio_bit))
+		return;
+
+	bank->dbck_enable_mask &= ~gpio_bit;
+	bank->context.debounce_en &= ~gpio_bit;
+	__raw_writel(bank->context.debounce_en,
+		     bank->base + bank->regs->debounce_en);
+
+	if (!bank->dbck_enable_mask) {
+		bank->context.debounce = 0;
+		__raw_writel(bank->context.debounce, bank->base +
+			     bank->regs->debounce);
+		clk_disable(bank->dbck);
+		bank->dbck_enabled = false;
+	}
+}
+
 static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
 						unsigned trigger)
 {

@@ -539,8 +573,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 &= ~(GPIO_BIT(bank, gpio));
-	_gpio_dbck_disable(bank);
+	_clear_gpio_debounce(bank, gpio);
 }
 
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */

Signature · 1 line

1.7.9.5

Kevin Hilman

author

2012-10-25 23:56 UTC #2 lore raw

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

Quoted 52 lines · Jon Hunter

Hi Kevin,
Quoted 24 lines · Kevin Hilman
From: Kevin Hilman <[email protected]>
When a GPIO is freed or shutdown, ensure that the proper bit in dbck_enable_mask is cleared also. Otherwise, context restore on subsequent off-mode transition will restore previous debounce values 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 right bit in bank->dbck_enable_mask is cleared when a GPIO is freed/shutdown so debounce state doesn't persist after free/reset. If this GPIO is the last debounce-enabled GPIO in the bank, the debounce will also be cut.
Quoted 8 lines
Special thanks to Grazvydas Ignotas for pointing out a bug in the first version that would've disabled debounce on any runtime PM transition.
And, special thanks to Jon Hunter for pointing out a bug in the second version which was mistakenly clearing all debounce bits on reset instead of individual GPIOs, as well as suggesting cutting the debounce clock after all debounce bits are cleared.
... and for introducing yet another bug :-(
Quoted 14 lines
Tesed on 37xx/EVM board which configures GPIO debounce for the ads7846 touchscreen in its board file using the above sequence, and so was failing off-mode tests in dynamic idle. Verified that off-mode tests are passing with this patch.
Reported-by Paul Walmsley <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Grazvydas Ignotas <[email protected]>
Cc Jon Hunter <[email protected]>
Signed-off-by Kevin Hilman <[email protected]>
---
 drivers/gpio/gpio-omap.c | 2 ++
 1 file changed, 2 insertions(+)

Quoted 14 lines

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, int gpio) · line 543
Quoted 10 lines
drivers/gpio/gpio-omap.c +2 -0
index 94cbc84..ce1da19 100644
@@ -539,6 +539,8 @@ 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 &= ~(GPIO_BIT(bank, gpio));
+	_gpio_dbck_disable(bank);
We can't use _gpio_dbck_disable() here. This has been specifically implemented for the suspend path and is designed to disable the debounce clock while debounce is enabled (which makes sense).

I agree, it makes sense. It reminds me that this driver could use some comments, since each time I come back to it I forget why some of this stuff is there.

Quoted 1 line

Yes this needs to be cleaned up.

Most certainly agree.

Quoted 2 lines

I have implemented the following and unit tested. Care to test on your 37xx board? Sorry I would do it myself I had one.

Yes, it fixes the off-mode dynamic idle problem on my 37xx EVM.

Quoted 2 lines

Also not sure if you just wish to squash your patch and mine together. This is based on top of yours.

Let's just drop mine and you can take this forward.

Quoted 9 lines

Cheers
Jon
From 33812f3bd4f7aab1154e7194b7f11fba700a5086 Mon Sep 17 00:00:00 2001
From: Jon Hunter <[email protected]>
Date: Thu, 25 Oct 2012 16:00:51 -0500
Subject: [PATCH] gpio/omap: fix clearing of debounce settings on gpio
free/reset

Quoted 3 lines

When a GPIO is freed or shutdown, we need to ensure that any debounce settings are cleared and if the GPIO is the only GPIO in the bank that is currently using debounce, then disable the debounce clock as well to save power.

Since this is a fix needed for v3.7-rc, you should add a bit more here describing what was broken (bogus context restore, etc.) Basically answering "why" to your the "we need to ensure that..." statement.

Quoted 7 lines

Therefore, introduce a new function called _clear_gpio_debounce() to clear any debounce settings when the GPIO is freed or shutdown.
Please note that we cannot use _gpio_dbck_disable() to disable the debounce clock because this has been specifically created for the gpio suspend path and is intended to shutdown the debounce clock while debounce is enabled.

Quoted 5 lines

This has been unit tested on an OMAP3430 Beagle board, by requesting a gpio, enabling debounce and then freeing the gpio and checking the register contents, the saved register context and the debounce clock state.
Signed-off-by Jon Hunter <[email protected]>

Reviewed-by Kevin Hilman <[email protected]>

(though not that that matters now that I've shown a high-level of
incompetence with this patch) ;)

Also,

Tested-by Kevin Hilman <[email protected]>

When reposting for Linus W., you might make this 'v4' so that it's clear that it superceeds the other bungling attempts at the same fix.

Thanks for following this through,

Kevin

Santosh Shilimkar

2012-10-26 06:01 UTC #3 lore raw

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

In reply to Jon Hunter

Jon,

Quoted 5 lines · Jon Hunter

Hi Kevin,
Quoted 2 lines · Kevin Hilman
From: Kevin Hilman <[email protected]>

[...]

Quoted 6 lines

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

index 94cbc84..ce1da19 100644
@@ -539,6 +539,8 @@ static void _reset_gpio(struct gpio_bank *bank, int gpio)
  	_set_gpio_irqenable(bank, gpio, 0);

Quoted 9 lines

drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, int gpio)
Quoted 4 lines
drivers/gpio/gpio-omap.c +2 -0
  	_clear_gpio_irqstatus(bank, gpio);
  	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+	bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
+	_gpio_dbck_disable(bank);
We can't use _gpio_dbck_disable() here. This has been specifically implemented for the suspend path and is designed to disable the debounce clock while debounce is enabled (which makes sense). Yes this needs to be cleaned up.

I thought this bit was clear on v2 discussion list that debounce clock disable needs to be conditional based on debounce mask.

Quoted 17 lines

From 33812f3bd4f7aab1154e7194b7f11fba700a5086 Mon Sep 17 00:00:00 2001
From: Jon Hunter <[email protected]>
Date: Thu, 25 Oct 2012 16:00:51 -0500
Subject: [PATCH] gpio/omap: fix clearing of debounce settings on gpio
free/reset
When a GPIO is freed or shutdown, we need to ensure that any debounce settings are cleared and if the GPIO is the only GPIO in the bank that is currently using debounce, then disable the debounce clock as well to save power.
Therefore, introduce a new function called _clear_gpio_debounce() to clear any debounce settings when the GPIO is freed or shutdown.
Please note that we cannot use _gpio_dbck_disable() to disable the debounce clock because this has been specifically created for the gpio suspend path and is intended to shutdown the debounce clock while debounce is enabled.

Quoted 6 lines

This has been unit tested on an OMAP3430 Beagle board, by requesting a gpio, enabling debounce and then freeing the gpio and checking the register contents, the saved register context and the debounce clock state.
Signed-off-by Jon Hunter <[email protected]>
---

Now that we are aligned, so we can take this patch forward. Feel free to add my ack in case you plan to refresh it.

Acked-by Santosh Shilimkar <[email protected]>

Jon Hunter

2012-10-26 19:14 UTC #4 lore raw

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

Quoted 28 lines · Santosh Shilimkar

Jon,
Quoted 5 lines · Jon Hunter
Hi Kevin,
Quoted 2 lines · Kevin Hilman
From: Kevin Hilman <[email protected]>
[...]
Quoted 8 lines
Quoted 8 lines
drivers/gpio/gpio-omap.c +0 -0
index 94cbc84..ce1da19 100644
@@ -539,6 +539,8 @@ static void _reset_gpio(struct gpio_bank *bank,
 int gpio)
      _set_gpio_irqenable(bank, gpio, 0);
      _clear_gpio_irqstatus(bank, gpio);
Quoted 9 lines
drivers/gpio/gpio-omap.c static void _reset_gpio(struct gpio_bank *bank, · line 544
Quoted 3 lines
drivers/gpio/gpio-omap.c +2 -0
      _set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
+    bank->dbck_enable_mask &= ~(GPIO_BIT(bank, gpio));
+    _gpio_dbck_disable(bank);
We can't use _gpio_dbck_disable() here. This has been specifically implemented
for the suspend path and is designed to disable the debounce clock while debounce is enabled (which makes sense). Yes this needs to be cleaned up.

Quoted 2 lines

I thought this bit was clear on v2 discussion list that debounce clock disable needs to be conditional based on debounce mask.

Yes that part was crystal clear ;-)

However, what I had overlooked was how _gpio_dbck_disable() was implemented and is intended to work. Hence, I do not call this new in the _gpio_clear_debounce() function.

Quoted 28 lines

Quoted 28 lines
From 33812f3bd4f7aab1154e7194b7f11fba700a5086 Mon Sep 17 00:00:00 2001
From: Jon Hunter <[email protected]>
Date: Thu, 25 Oct 2012 16:00:51 -0500
Subject: [PATCH] gpio/omap: fix clearing of debounce settings on gpio
free/reset
When a GPIO is freed or shutdown, we need to ensure that any debounce settings
are cleared and if the GPIO is the only GPIO in the bank that is currently
using debounce, then disable the debounce clock as well to save power.
Therefore, introduce a new function called _clear_gpio_debounce() to clear
any debounce settings when the GPIO is freed or shutdown.
Please note that we cannot use _gpio_dbck_disable() to disable the debounce
clock because this has been specifically created for the gpio suspend path
and is intended to shutdown the debounce clock while debounce is enabled.
This has been unit tested on an OMAP3430 Beagle board, by requesting a gpio,
enabling debounce and then freeing the gpio and checking the register contents,
the saved register context and the debounce clock state.

Quoted 6 lines

Quoted 2 lines
Signed-off-by Jon Hunter <[email protected]>
---
Now that we are aligned, so we can take this patch forward. Feel free to add my ack in case you plan to refresh it.
Acked-by Santosh Shilimkar <[email protected]>

Thanks!

Jon

Linus Walleij

2012-10-27 16:23 UTC #5 lore raw

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

In reply to Jon Hunter

Quoted 14 lines · Jon Hunter

Subject: [PATCH] gpio/omap: fix clearing of debounce settings on gpio
free/reset
When a GPIO is freed or shutdown, we need to ensure that any debounce settings are cleared and if the GPIO is the only GPIO in the bank that is currently using debounce, then disable the debounce clock as well to save power.
Therefore, introduce a new function called _clear_gpio_debounce() to clear any debounce settings when the GPIO is freed or shutdown.
Please note that we cannot use _gpio_dbck_disable() to disable the debounce clock because this has been specifically created for the gpio suspend path and is intended to shutdown the debounce clock while debounce is enabled.

Quoted 5 lines

This has been unit tested on an OMAP3430 Beagle board, by requesting a gpio, enabling debounce and then freeing the gpio and checking the register contents, the saved register context and the debounce clock state.
Signed-off-by Jon Hunter <[email protected]>

I have come to the conclusion that there is consensus to merge this patch.

However it does *not* apply to my GPIO tree, which for the OMAP driver is just a clean v3.7-rc1 and nothing else.

I previously reverted Kevin's patch, is this meant to be applied on top of that one or what?

Jon, can you please submit a patch made against my GPIO tree or linux-next or v3.7-rc2 or whatever and include the ACKs you've recieved?

Thanks!
Linus Walleij