[PATCH v5] gpio/omap: fix off-mode bug: clear debounce settings on free/reset

[PATCH v5] gpio/omap: fix off-mode bug: clear debounce settings on free/reset

[Jon Hunter]

This change was originally titled "gpio/omap: fix off-mode bug: clear debounce
clock enable mask on free/reset". The title has been updated slightly to
reflect (what should be) the final fix.

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.

Currently, the debounce settings are not cleared on a GPIO free or shutdown and
so during a context restore on subsequent off-mode transition, the previous
debounce values are restored 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 this, introduce a new function called _clear_gpio_debounce() to clear
any debounce settings when the GPIO is freed or shutdown. If this GPIO is the
last debounce-enabled GPIO in the bank, the debounce will also be cut.

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.

Special thanks to Kevin Hilman for root causing the bug. This fix is a
collaborative effort with inputs from Kevin Hilman, Grazvydas Ignotas and
Santosh Shilimkar.

Testing:
- 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.
- Kevin Hilman tested 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.

V5 changes:
- Corrected author

Reported-by: Paul Walmsley <paul@pwsan.com>
Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Grazvydas Ignotas <notasas@gmail.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Signed-off-by: Jon Hunter <jon-hunter@ti.com>
Reviewed-by: Kevin Hilman <khilman@ti.com>
Tested-by: Kevin Hilman <khilman@ti.com>
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/gpio/gpio-omap.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..d335af1 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -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,6 +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);
+	_clear_gpio_debounce(bank, gpio);
 }
 
 /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */
-- 
1.7.9.5

[PATCH v5] gpio/omap: fix off-mode bug: clear debounce settings on free/reset

[Kevin Hilman]

Jon Hunter <jon-hunter@ti.com> writes:

> This change was originally titled "gpio/omap: fix off-mode bug: clear debounce
> clock enable mask on free/reset". The title has been updated slightly to
> reflect (what should be) the final fix.

nit: this kind of thing should typically go after the '---' since it's
useful reviewers/maintainers but not necessarily in the permanant git
history.

Otherwise, thanks for taking this over, and hopefully this can still
get in for v3.7-rc.

Kevin

> 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.
>
> Currently, the debounce settings are not cleared on a GPIO free or shutdown and
> so during a context restore on subsequent off-mode transition, the previous
> debounce values are restored 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 this, introduce a new function called _clear_gpio_debounce() to clear
> any debounce settings when the GPIO is freed or shutdown. If this GPIO is the
> last debounce-enabled GPIO in the bank, the debounce will also be cut.
>
> 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.
>
> Special thanks to Kevin Hilman for root causing the bug. This fix is a
> collaborative effort with inputs from Kevin Hilman, Grazvydas Ignotas and
> Santosh Shilimkar.
>
> Testing:
> - 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.
> - Kevin Hilman tested 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.
>
> V5 changes:
> - Corrected author
>
> Reported-by: Paul Walmsley <paul@pwsan.com>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Grazvydas Ignotas <notasas@gmail.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 94cbc84..d335af1 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -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,6 +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);
> +	_clear_gpio_debounce(bank, gpio);
>  }
>  
>  /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */

[PATCH v5] gpio/omap: fix off-mode bug: clear debounce settings on free/reset

[Jon Hunter]

On 10/26/2012 04:19 PM, Kevin Hilman wrote:
> Jon Hunter <jon-hunter@ti.com> writes:
> 
>> This change was originally titled "gpio/omap: fix off-mode bug: clear debounce
>> clock enable mask on free/reset". The title has been updated slightly to
>> reflect (what should be) the final fix.
> 
> nit: this kind of thing should typically go after the '---' since it's
> useful reviewers/maintainers but not necessarily in the permanant git
> history.

Thanks for the tip.

Linus, let me know if I should resend.

Cheers
Jon

[PATCH v5] gpio/omap: fix off-mode bug: clear debounce settings on free/reset

[Linus Walleij]

On Fri, Oct 26, 2012 at 9:26 PM, Jon Hunter <jon-hunter@ti.com> wrote:

> This change was originally titled "gpio/omap: fix off-mode bug: clear debounce
> clock enable mask on free/reset". The title has been updated slightly to
> reflect (what should be) the final fix.
>
> 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.
>
> Currently, the debounce settings are not cleared on a GPIO free or shutdown and
> so during a context restore on subsequent off-mode transition, the previous
> debounce values are restored 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 this, introduce a new function called _clear_gpio_debounce() to clear
> any debounce settings when the GPIO is freed or shutdown. If this GPIO is the
> last debounce-enabled GPIO in the bank, the debounce will also be cut.
>
> 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.
>
> Special thanks to Kevin Hilman for root causing the bug. This fix is a
> collaborative effort with inputs from Kevin Hilman, Grazvydas Ignotas and
> Santosh Shilimkar.
>
> Testing:
> - 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.
> - Kevin Hilman tested 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.
>
> V5 changes:
> - Corrected author
>
> Reported-by: Paul Walmsley <paul@pwsan.com>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Grazvydas Ignotas <notasas@gmail.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Signed-off-by: Jon Hunter <jon-hunter@ti.com>
> Reviewed-by: Kevin Hilman <khilman@ti.com>
> Tested-by: Kevin Hilman <khilman@ti.com>
> Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Hey there is a version I requested, with ACKs and all.

You must be reading my mind :-D

OK patch applied for fixes.

Yours,
Linus Walleij