[PATCH v8 REPOST 17/24] gpio/omap: fix debounce clock handling

[PATCH v8 REPOST 17/24] gpio/omap: fix debounce clock handling

[Tarun Kanti DebBarma]

Currently debounce clock state is not tracked in the system. The bank->dbck
is enabled/disabled in suspend/idle paths irrespective of whether debounce
interval has been set or not. Ideally, it should be handled only for those
gpio banks where the debounce is enabled. In _set_gpio_debounce, enable
debounce clock before accessing registers.

Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
---
During further internal testing it was found that image was crashing within
_set_gpio_debounce(). It is observed that we are trying to access registers
without enabling debounce clock. This patch incorporates the change whereby
the debounce clock is enabled before accessing registers and disabled at the
end of the function.

 drivers/gpio/gpio-omap.c |   60 ++++++++++++++++++++++++++++++++-------------
 1 files changed, 42 insertions(+), 18 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index fa6c9c5..85e9c2a 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -65,6 +65,7 @@ struct gpio_bank {
 	struct clk *dbck;
 	u32 mod_usage;
 	u32 dbck_enable_mask;
+	bool dbck_enabled;
 	struct device *dev;
 	bool is_mpuio;
 	bool dbck_flag;
@@ -156,6 +157,22 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
 	__raw_writel(l, base + reg);
 }
 
+static inline void _gpio_dbck_enable(struct gpio_bank *bank)
+{
+	if (bank->dbck_enable_mask && !bank->dbck_enabled) {
+		clk_enable(bank->dbck);
+		bank->dbck_enabled = true;
+	}
+}
+
+static inline void _gpio_dbck_disable(struct gpio_bank *bank)
+{
+	if (bank->dbck_enable_mask && bank->dbck_enabled) {
+		clk_disable(bank->dbck);
+		bank->dbck_enabled = false;
+	}
+}
+
 /**
  * _set_gpio_debounce - low level gpio debounce time
  * @bank: the gpio bank we're acting upon
@@ -184,22 +201,22 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
 
 	l = GPIO_BIT(bank, gpio);
 
+	clk_enable(bank->dbck);
 	reg = bank->base + bank->regs->debounce;
 	__raw_writel(debounce, reg);
 
 	reg = bank->base + bank->regs->debounce_en;
 	val = __raw_readl(reg);
 
-	if (debounce) {
+	if (debounce)
 		val |= l;
-		clk_enable(bank->dbck);
-	} else {
+	else
 		val &= ~l;
-		clk_disable(bank->dbck);
-	}
+
 	bank->dbck_enable_mask = val;
 
 	__raw_writel(val, reg);
+	clk_disable(bank->dbck);
 }
 
 static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
@@ -485,8 +502,10 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 	 * If this is the first gpio_request for the bank,
 	 * enable the bank module.
 	 */
-	if (!bank->mod_usage)
+	if (!bank->mod_usage) {
+		_gpio_dbck_enable(bank);
 		pm_runtime_get_sync(bank->dev);
+	}
 
 	spin_lock_irqsave(&bank->lock, flags);
 	/* Set trigger to none. You need to enable the desired trigger with
@@ -549,8 +568,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) {
 		pm_runtime_put_sync(bank->dev);
+		_gpio_dbck_disable(bank);
+	}
 }
 
 /*
@@ -829,8 +850,10 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
 
 	if (!bank->dbck) {
 		bank->dbck = clk_get(bank->dev, "dbclk");
-		if (IS_ERR(bank->dbck))
+		if (IS_ERR(bank->dbck)) {
 			dev_err(bank->dev, "Could not get gpio dbck\n");
+			return -EINVAL;
+		}
 	}
 
 	spin_lock_irqsave(&bank->lock, flags);
@@ -1086,6 +1109,8 @@ static int omap_gpio_suspend(struct device *dev)
 		bank->saved_wakeup = __raw_readl(wake_status);
 		_gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
 		spin_unlock_irqrestore(&bank->lock, flags);
+
+		_gpio_dbck_disable(bank);
 	}
 
 	return 0;
@@ -1102,6 +1127,8 @@ static int omap_gpio_resume(struct device *dev)
 		if (!bank->regs->wkup_en)
 			return 0;
 
+		_gpio_dbck_enable(bank);
+
 		spin_lock_irqsave(&bank->lock, flags);
 		_gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
 		spin_unlock_irqrestore(&bank->lock, flags);
@@ -1120,16 +1147,14 @@ void omap2_gpio_prepare_for_idle(int off_mode)
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
 		u32 l1 = 0, l2 = 0;
-		int j;
 
 		if (!bank->loses_context)
 			continue;
 
-		for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
-			clk_disable(bank->dbck);
-
-		if (!off_mode)
+		if (!off_mode) {
+			_gpio_dbck_disable(bank);
 			continue;
+		}
 
 		/* If going to OFF, remove triggering for all
 		 * non-wakeup GPIOs.  Otherwise spurious IRQs will be
@@ -1151,15 +1176,16 @@ void omap2_gpio_prepare_for_idle(int off_mode)
 		__raw_writel(l2, bank->base + bank->regs->risingdetect);
 
 save_gpio_context:
-
 		if (bank->get_context_loss_count)
 			bank->context_loss_count =
 				bank->get_context_loss_count(bank->dev);
 
 		omap_gpio_save_context(bank);
 
-		if (!pm_runtime_suspended(bank->dev))
+		if (!pm_runtime_suspended(bank->dev)) {
 			pm_runtime_put_sync(bank->dev);
+			_gpio_dbck_disable(bank);
+		}
 	}
 }
 
@@ -1170,13 +1196,11 @@ void omap2_gpio_resume_after_idle(void)
 	list_for_each_entry(bank, &omap_gpio_list, node) {
 		u32 context_lost_cnt_after;
 		u32 l = 0, gen, gen0, gen1;
-		int j;
 
 		if (!bank->loses_context)
 			continue;
 
-		for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
-			clk_enable(bank->dbck);
+		_gpio_dbck_enable(bank);
 
 		if (pm_runtime_suspended(bank->dev))
 			pm_runtime_get_sync(bank->dev);
-- 
1.7.0.4

[PATCH v8 REPOST 17/24] gpio/omap: fix debounce clock handling

[Kevin Hilman]

Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:

> Currently debounce clock state is not tracked in the system. 

?? 

bank->dbck_enable_mask ?

> The bank->dbck
> is enabled/disabled in suspend/idle paths irrespective of whether debounce
> interval has been set or not. 

No.  It's conditional based on bank->dbck_enable_mask, which is based on
whether or not debounce has been enabled.

> Ideally, it should be handled only for those
> gpio banks where the debounce is enabled. 

AFAICT, it is.  Please explain more what is actually happening in the
patch, and why.

> In _set_gpio_debounce, enable debounce clock before accessing
> registers.

This is a separate issue/bug and wants its own patch with descriptive
changelog.

> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> ---
> During further internal testing it was found that image was crashing within
> _set_gpio_debounce(). It is observed that we are trying to access registers
> without enabling debounce clock. This patch incorporates the change whereby
> the debounce clock is enabled before accessing registers and disabled at the
> end of the function.
>
>  drivers/gpio/gpio-omap.c |   60 ++++++++++++++++++++++++++++++++-------------
>  1 files changed, 42 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index fa6c9c5..85e9c2a 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -65,6 +65,7 @@ struct gpio_bank {
>  	struct clk *dbck;
>  	u32 mod_usage;
>  	u32 dbck_enable_mask;
> +	bool dbck_enabled;
>  	struct device *dev;
>  	bool is_mpuio;
>  	bool dbck_flag;
> @@ -156,6 +157,22 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>  	__raw_writel(l, base + reg);
>  }
>  
> +static inline void _gpio_dbck_enable(struct gpio_bank *bank)
> +{
> +	if (bank->dbck_enable_mask && !bank->dbck_enabled) {
> +		clk_enable(bank->dbck);
> +		bank->dbck_enabled = true;
> +	}
> +}
> +
> +static inline void _gpio_dbck_disable(struct gpio_bank *bank)
> +{
> +	if (bank->dbck_enable_mask && bank->dbck_enabled) {
> +		clk_disable(bank->dbck);
> +		bank->dbck_enabled = false;
> +	}
> +}
> +
>  /**
>   * _set_gpio_debounce - low level gpio debounce time
>   * @bank: the gpio bank we're acting upon
> @@ -184,22 +201,22 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>  
>  	l = GPIO_BIT(bank, gpio);
>  
> +	clk_enable(bank->dbck);
>  	reg = bank->base + bank->regs->debounce;
>  	__raw_writel(debounce, reg);
>  
>  	reg = bank->base + bank->regs->debounce_en;
>  	val = __raw_readl(reg);
>  
> -	if (debounce) {
> +	if (debounce)
>  		val |= l;
> -		clk_enable(bank->dbck);
> -	} else {
> +	else
>  		val &= ~l;
> -		clk_disable(bank->dbck);
> -	}
> +
>  	bank->dbck_enable_mask = val;
>  
>  	__raw_writel(val, reg);
> +	clk_disable(bank->dbck);
>  }
>  
>  static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
> @@ -485,8 +502,10 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>  	 * If this is the first gpio_request for the bank,
>  	 * enable the bank module.
>  	 */
> -	if (!bank->mod_usage)
> +	if (!bank->mod_usage) {
> +		_gpio_dbck_enable(bank);
>  		pm_runtime_get_sync(bank->dev);
> +	}
>  
>  	spin_lock_irqsave(&bank->lock, flags);
>  	/* Set trigger to none. You need to enable the desired trigger with
> @@ -549,8 +568,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) {
>  		pm_runtime_put_sync(bank->dev);
> +		_gpio_dbck_disable(bank);

Why not add this to the ->runtime_suspend() callback?

> +	}
>  }
>  
>  /*
> @@ -829,8 +850,10 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
>  
>  	if (!bank->dbck) {
>  		bank->dbck = clk_get(bank->dev, "dbclk");
> -		if (IS_ERR(bank->dbck))
> +		if (IS_ERR(bank->dbck)) {
>  			dev_err(bank->dev, "Could not get gpio dbck\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	spin_lock_irqsave(&bank->lock, flags);
> @@ -1086,6 +1109,8 @@ static int omap_gpio_suspend(struct device *dev)
>  		bank->saved_wakeup = __raw_readl(wake_status);
>  		_gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
>  		spin_unlock_irqrestore(&bank->lock, flags);
> +
> +		_gpio_dbck_disable(bank);

If this call was in the ->runtime_suspend() callback, you wouldn't need
it here.

>  	}
>  
>  	return 0;
> @@ -1102,6 +1127,8 @@ static int omap_gpio_resume(struct device *dev)
>  		if (!bank->regs->wkup_en)
>  			return 0;
>  
> +		_gpio_dbck_enable(bank);

Similarily, this call should be in the ->runtime_resume() callback and
it wouldn't be needed here.  

Using the runtime PM callbacks, all the _gpio_dbck_* calls below would
not be needed.

>  		spin_lock_irqsave(&bank->lock, flags);
>  		_gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
>  		spin_unlock_irqrestore(&bank->lock, flags);
> @@ -1120,16 +1147,14 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>  
>  	list_for_each_entry(bank, &omap_gpio_list, node) {
>  		u32 l1 = 0, l2 = 0;
> -		int j;
>  
>  		if (!bank->loses_context)
>  			continue;
>  
> -		for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
> -			clk_disable(bank->dbck);
> -
> -		if (!off_mode)
> +		if (!off_mode) {
> +			_gpio_dbck_disable(bank);
>  			continue;
> +		}
>  
>  		/* If going to OFF, remove triggering for all
>  		 * non-wakeup GPIOs.  Otherwise spurious IRQs will be
> @@ -1151,15 +1176,16 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>  		__raw_writel(l2, bank->base + bank->regs->risingdetect);
>  
>  save_gpio_context:
> -
>  		if (bank->get_context_loss_count)
>  			bank->context_loss_count =
>  				bank->get_context_loss_count(bank->dev);
>  
>  		omap_gpio_save_context(bank);
>  
> -		if (!pm_runtime_suspended(bank->dev))
> +		if (!pm_runtime_suspended(bank->dev)) {
>  			pm_runtime_put_sync(bank->dev);
> +			_gpio_dbck_disable(bank);
> +		}
>  	}
>  }
>  
> @@ -1170,13 +1196,11 @@ void omap2_gpio_resume_after_idle(void)
>  	list_for_each_entry(bank, &omap_gpio_list, node) {
>  		u32 context_lost_cnt_after;
>  		u32 l = 0, gen, gen0, gen1;
> -		int j;
>  
>  		if (!bank->loses_context)
>  			continue;
>  
> -		for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
> -			clk_enable(bank->dbck);
> +		_gpio_dbck_enable(bank);
>  		if (pm_runtime_suspended(bank->dev))
>  			pm_runtime_get_sync(bank->dev);

Kevin

[PATCH v8 REPOST 17/24] gpio/omap: fix debounce clock handling

[DebBarma, Tarun Kanti]

On Fri, Nov 4, 2011 at 10:10 PM, Kevin Hilman <khilman@ti.com> wrote:
> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>
>> Currently debounce clock state is not tracked in the system.
>
> ??
>
> bank->dbck_enable_mask ?
As I understand, this only tells which all GPIOs have debounce timeout enabled.
But, during system operation as debounce clocks are enabled and disabled I need
additional flag to keep track of current state (enabled/disabled).
This is what I meant.

>
>> The bank->dbck
>> is enabled/disabled in suspend/idle paths irrespective of whether debounce
>> interval has been set or not.
>
> No. ?It's conditional based on bank->dbck_enable_mask, which is based on
> whether or not debounce has been enabled.
You are right. I need to rephrase my description.

>
>> Ideally, it should be handled only for those
>> gpio banks where the debounce is enabled.
>
> AFAICT, it is. ?Please explain more what is actually happening in the
> patch, and why.
Yes, as I explained above, it is more about the tracking the debounce clock
enabled/disabled state for those GPIOs whose debounce timeouts are enabled.
I will modify the patch description.

>
>> In _set_gpio_debounce, enable debounce clock before accessing
>> registers.
>
> This is a separate issue/bug and wants its own patch with descriptive
> changelog.
OK.

>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>> ---
>> During further internal testing it was found that image was crashing within
>> _set_gpio_debounce(). It is observed that we are trying to access registers
>> without enabling debounce clock. This patch incorporates the change whereby
>> the debounce clock is enabled before accessing registers and disabled at the
>> end of the function.
>>
>> ?drivers/gpio/gpio-omap.c | ? 60 ++++++++++++++++++++++++++++++++-------------
>> ?1 files changed, 42 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index fa6c9c5..85e9c2a 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -65,6 +65,7 @@ struct gpio_bank {
>> ? ? ? struct clk *dbck;
>> ? ? ? u32 mod_usage;
>> ? ? ? u32 dbck_enable_mask;
>> + ? ? bool dbck_enabled;
>> ? ? ? struct device *dev;
>> ? ? ? bool is_mpuio;
>> ? ? ? bool dbck_flag;
>> @@ -156,6 +157,22 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>> ? ? ? __raw_writel(l, base + reg);
>> ?}
>>
>> +static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>> +{
>> + ? ? if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>> + ? ? ? ? ? ? clk_enable(bank->dbck);
>> + ? ? ? ? ? ? bank->dbck_enabled = true;
>> + ? ? }
>> +}
>> +
>> +static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>> +{
>> + ? ? if (bank->dbck_enable_mask && bank->dbck_enabled) {
>> + ? ? ? ? ? ? clk_disable(bank->dbck);
>> + ? ? ? ? ? ? bank->dbck_enabled = false;
>> + ? ? }
>> +}
>> +
>> ?/**
>> ? * _set_gpio_debounce - low level gpio debounce time
>> ? * @bank: the gpio bank we're acting upon
>> @@ -184,22 +201,22 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>
>> ? ? ? l = GPIO_BIT(bank, gpio);
>>
>> + ? ? clk_enable(bank->dbck);
>> ? ? ? reg = bank->base + bank->regs->debounce;
>> ? ? ? __raw_writel(debounce, reg);
>>
>> ? ? ? reg = bank->base + bank->regs->debounce_en;
>> ? ? ? val = __raw_readl(reg);
>>
>> - ? ? if (debounce) {
>> + ? ? if (debounce)
>> ? ? ? ? ? ? ? val |= l;
>> - ? ? ? ? ? ? clk_enable(bank->dbck);
>> - ? ? } else {
>> + ? ? else
>> ? ? ? ? ? ? ? val &= ~l;
>> - ? ? ? ? ? ? clk_disable(bank->dbck);
>> - ? ? }
>> +
>> ? ? ? bank->dbck_enable_mask = val;
>>
>> ? ? ? __raw_writel(val, reg);
>> + ? ? clk_disable(bank->dbck);
>> ?}
>>
>> ?static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
>> @@ -485,8 +502,10 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>> ? ? ? ?* If this is the first gpio_request for the bank,
>> ? ? ? ?* enable the bank module.
>> ? ? ? ?*/
>> - ? ? if (!bank->mod_usage)
>> + ? ? if (!bank->mod_usage) {
>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>> ? ? ? ? ? ? ? pm_runtime_get_sync(bank->dev);
>> + ? ? }
>>
>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>> ? ? ? /* Set trigger to none. You need to enable the desired trigger with
>> @@ -549,8 +568,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) {
>> ? ? ? ? ? ? ? pm_runtime_put_sync(bank->dev);
>> + ? ? ? ? ? ? _gpio_dbck_disable(bank);
>
> Why not add this to the ->runtime_suspend() callback?
Yes, I can move there and test.

>
>> + ? ? }
>> ?}
>>
>> ?/*
>> @@ -829,8 +850,10 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
>>
>> ? ? ? if (!bank->dbck) {
>> ? ? ? ? ? ? ? bank->dbck = clk_get(bank->dev, "dbclk");
>> - ? ? ? ? ? ? if (IS_ERR(bank->dbck))
>> + ? ? ? ? ? ? if (IS_ERR(bank->dbck)) {
>> ? ? ? ? ? ? ? ? ? ? ? dev_err(bank->dev, "Could not get gpio dbck\n");
>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>> + ? ? ? ? ? ? }
>> ? ? ? }
>>
>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>> @@ -1086,6 +1109,8 @@ static int omap_gpio_suspend(struct device *dev)
>> ? ? ? ? ? ? ? bank->saved_wakeup = __raw_readl(wake_status);
>> ? ? ? ? ? ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>> +
>> + ? ? ? ? ? ? _gpio_dbck_disable(bank);
>
> If this call was in the ->runtime_suspend() callback, you wouldn't need
> it here.
Yes.

>
>> ? ? ? }
>>
>> ? ? ? return 0;
>> @@ -1102,6 +1127,8 @@ static int omap_gpio_resume(struct device *dev)
>> ? ? ? ? ? ? ? if (!bank->regs->wkup_en)
>> ? ? ? ? ? ? ? ? ? ? ? return 0;
>>
>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>
> Similarily, this call should be in the ->runtime_resume() callback and
> it wouldn't be needed here.
Right.

>
> Using the runtime PM callbacks, all the _gpio_dbck_* calls below would
> not be needed.
Yes.
--
Tarun
>
>> ? ? ? ? ? ? ? spin_lock_irqsave(&bank->lock, flags);
>> ? ? ? ? ? ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>> @@ -1120,16 +1147,14 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>>
>> ? ? ? list_for_each_entry(bank, &omap_gpio_list, node) {
>> ? ? ? ? ? ? ? u32 l1 = 0, l2 = 0;
>> - ? ? ? ? ? ? int j;
>>
>> ? ? ? ? ? ? ? if (!bank->loses_context)
>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>
>> - ? ? ? ? ? ? for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
>> - ? ? ? ? ? ? ? ? ? ? clk_disable(bank->dbck);
>> -
>> - ? ? ? ? ? ? if (!off_mode)
>> + ? ? ? ? ? ? if (!off_mode) {
>> + ? ? ? ? ? ? ? ? ? ? _gpio_dbck_disable(bank);
>> ? ? ? ? ? ? ? ? ? ? ? continue;
>> + ? ? ? ? ? ? }
>>
>> ? ? ? ? ? ? ? /* If going to OFF, remove triggering for all
>> ? ? ? ? ? ? ? ?* non-wakeup GPIOs. ?Otherwise spurious IRQs will be
>> @@ -1151,15 +1176,16 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>> ? ? ? ? ? ? ? __raw_writel(l2, bank->base + bank->regs->risingdetect);
>>
>> ?save_gpio_context:
>> -
>> ? ? ? ? ? ? ? if (bank->get_context_loss_count)
>> ? ? ? ? ? ? ? ? ? ? ? bank->context_loss_count =
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>>
>> ? ? ? ? ? ? ? omap_gpio_save_context(bank);
>>
>> - ? ? ? ? ? ? if (!pm_runtime_suspended(bank->dev))
>> + ? ? ? ? ? ? if (!pm_runtime_suspended(bank->dev)) {
>> ? ? ? ? ? ? ? ? ? ? ? pm_runtime_put_sync(bank->dev);
>> + ? ? ? ? ? ? ? ? ? ? _gpio_dbck_disable(bank);
>> + ? ? ? ? ? ? }
>> ? ? ? }
>> ?}
>>
>> @@ -1170,13 +1196,11 @@ void omap2_gpio_resume_after_idle(void)
>> ? ? ? list_for_each_entry(bank, &omap_gpio_list, node) {
>> ? ? ? ? ? ? ? u32 context_lost_cnt_after;
>> ? ? ? ? ? ? ? u32 l = 0, gen, gen0, gen1;
>> - ? ? ? ? ? ? int j;
>>
>> ? ? ? ? ? ? ? if (!bank->loses_context)
>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>
>> - ? ? ? ? ? ? for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
>> - ? ? ? ? ? ? ? ? ? ? clk_enable(bank->dbck);
>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>> ? ? ? ? ? ? ? if (pm_runtime_suspended(bank->dev))
>> ? ? ? ? ? ? ? ? ? ? ? pm_runtime_get_sync(bank->dev);
>
> Kevin
>

[PATCH v8 REPOST 17/24] gpio/omap: fix debounce clock handling

[DebBarma, Tarun Kanti]

On Mon, Nov 7, 2011 at 7:19 PM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Fri, Nov 4, 2011 at 10:10 PM, Kevin Hilman <khilman@ti.com> wrote:
>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>
>>> Currently debounce clock state is not tracked in the system.
>>
>> ??
>>
>> bank->dbck_enable_mask ?
> As I understand, this only tells which all GPIOs have debounce timeout enabled.
> But, during system operation as debounce clocks are enabled and disabled I need
> additional flag to keep track of current state (enabled/disabled).
> This is what I meant.
>
>>
>>> The bank->dbck
>>> is enabled/disabled in suspend/idle paths irrespective of whether debounce
>>> interval has been set or not.
>>
>> No. ?It's conditional based on bank->dbck_enable_mask, which is based on
>> whether or not debounce has been enabled.
> You are right. I need to rephrase my description.
>
>>
>>> Ideally, it should be handled only for those
>>> gpio banks where the debounce is enabled.
>>
>> AFAICT, it is. ?Please explain more what is actually happening in the
>> patch, and why.
> Yes, as I explained above, it is more about the tracking the debounce clock
> enabled/disabled state for those GPIOs whose debounce timeouts are enabled.
> I will modify the patch description.
>
>>
>>> In _set_gpio_debounce, enable debounce clock before accessing
>>> registers.
>>
>> This is a separate issue/bug and wants its own patch with descriptive
>> changelog.
> OK.
>
>>
>>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>>> ---
>>> During further internal testing it was found that image was crashing within
>>> _set_gpio_debounce(). It is observed that we are trying to access registers
>>> without enabling debounce clock. This patch incorporates the change whereby
>>> the debounce clock is enabled before accessing registers and disabled at the
>>> end of the function.
>>>
>>> ?drivers/gpio/gpio-omap.c | ? 60 ++++++++++++++++++++++++++++++++-------------
>>> ?1 files changed, 42 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>> index fa6c9c5..85e9c2a 100644
>>> --- a/drivers/gpio/gpio-omap.c
>>> +++ b/drivers/gpio/gpio-omap.c
>>> @@ -65,6 +65,7 @@ struct gpio_bank {
>>> ? ? ? struct clk *dbck;
>>> ? ? ? u32 mod_usage;
>>> ? ? ? u32 dbck_enable_mask;
>>> + ? ? bool dbck_enabled;
>>> ? ? ? struct device *dev;
>>> ? ? ? bool is_mpuio;
>>> ? ? ? bool dbck_flag;
>>> @@ -156,6 +157,22 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>>> ? ? ? __raw_writel(l, base + reg);
>>> ?}
>>>
>>> +static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>>> +{
>>> + ? ? if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>>> + ? ? ? ? ? ? clk_enable(bank->dbck);
>>> + ? ? ? ? ? ? bank->dbck_enabled = true;
>>> + ? ? }
>>> +}
>>> +
>>> +static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>>> +{
>>> + ? ? if (bank->dbck_enable_mask && bank->dbck_enabled) {
>>> + ? ? ? ? ? ? clk_disable(bank->dbck);
>>> + ? ? ? ? ? ? bank->dbck_enabled = false;
>>> + ? ? }
>>> +}
>>> +
>>> ?/**
>>> ? * _set_gpio_debounce - low level gpio debounce time
>>> ? * @bank: the gpio bank we're acting upon
>>> @@ -184,22 +201,22 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>
>>> ? ? ? l = GPIO_BIT(bank, gpio);
>>>
>>> + ? ? clk_enable(bank->dbck);
>>> ? ? ? reg = bank->base + bank->regs->debounce;
>>> ? ? ? __raw_writel(debounce, reg);
>>>
>>> ? ? ? reg = bank->base + bank->regs->debounce_en;
>>> ? ? ? val = __raw_readl(reg);
>>>
>>> - ? ? if (debounce) {
>>> + ? ? if (debounce)
>>> ? ? ? ? ? ? ? val |= l;
>>> - ? ? ? ? ? ? clk_enable(bank->dbck);
>>> - ? ? } else {
>>> + ? ? else
>>> ? ? ? ? ? ? ? val &= ~l;
>>> - ? ? ? ? ? ? clk_disable(bank->dbck);
>>> - ? ? }
>>> +
>>> ? ? ? bank->dbck_enable_mask = val;
>>>
>>> ? ? ? __raw_writel(val, reg);
>>> + ? ? clk_disable(bank->dbck);
>>> ?}
>>>
>>> ?static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
>>> @@ -485,8 +502,10 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>>> ? ? ? ?* If this is the first gpio_request for the bank,
>>> ? ? ? ?* enable the bank module.
>>> ? ? ? ?*/
>>> - ? ? if (!bank->mod_usage)
>>> + ? ? if (!bank->mod_usage) {
>>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>>> ? ? ? ? ? ? ? pm_runtime_get_sync(bank->dev);
>>> + ? ? }
>>>
>>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>> ? ? ? /* Set trigger to none. You need to enable the desired trigger with
>>> @@ -549,8 +568,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) {
>>> ? ? ? ? ? ? ? pm_runtime_put_sync(bank->dev);
>>> + ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>
>> Why not add this to the ->runtime_suspend() callback?
> Yes, I can move there and test.
>
>>
>>> + ? ? }
>>> ?}
>>>
>>> ?/*
>>> @@ -829,8 +850,10 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
>>>
>>> ? ? ? if (!bank->dbck) {
>>> ? ? ? ? ? ? ? bank->dbck = clk_get(bank->dev, "dbclk");
>>> - ? ? ? ? ? ? if (IS_ERR(bank->dbck))
>>> + ? ? ? ? ? ? if (IS_ERR(bank->dbck)) {
>>> ? ? ? ? ? ? ? ? ? ? ? dev_err(bank->dev, "Could not get gpio dbck\n");
>>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>>> + ? ? ? ? ? ? }
>>> ? ? ? }
>>>
>>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>> @@ -1086,6 +1109,8 @@ static int omap_gpio_suspend(struct device *dev)
>>> ? ? ? ? ? ? ? bank->saved_wakeup = __raw_readl(wake_status);
>>> ? ? ? ? ? ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
>>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>> +
>>> + ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>
>> If this call was in the ->runtime_suspend() callback, you wouldn't need
>> it here.
> Yes.
>
>>
>>> ? ? ? }
>>>
>>> ? ? ? return 0;
>>> @@ -1102,6 +1127,8 @@ static int omap_gpio_resume(struct device *dev)
>>> ? ? ? ? ? ? ? if (!bank->regs->wkup_en)
>>> ? ? ? ? ? ? ? ? ? ? ? return 0;
>>>
>>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>>
>> Similarily, this call should be in the ->runtime_resume() callback and
>> it wouldn't be needed here.
> Right.
>
>>
>> Using the runtime PM callbacks, all the _gpio_dbck_* calls below would
>> not be needed.
> Yes.
I removed the  _gpio_dbck_disable/enable calls are removed from
*_prepare_for_idle()
and *_resume_after_idle(). But I am not seeing runtime callbacks being
called and
hence the  _gpio_dbck_* calls are not getting called.

I have also tried making explicit pm_runtime_put() and pm_runtime_get_sync() in
*_prepare_for_idle() and *_resume_after_idle(). Still, I do not see
the runtime callbacks
getting called.

Is there anything that I could be missing?
--
Tarun
>>
>>> ? ? ? ? ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>> ? ? ? ? ? ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
>>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>> @@ -1120,16 +1147,14 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>>>
>>> ? ? ? list_for_each_entry(bank, &omap_gpio_list, node) {
>>> ? ? ? ? ? ? ? u32 l1 = 0, l2 = 0;
>>> - ? ? ? ? ? ? int j;
>>>
>>> ? ? ? ? ? ? ? if (!bank->loses_context)
>>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>>
>>> - ? ? ? ? ? ? for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
>>> - ? ? ? ? ? ? ? ? ? ? clk_disable(bank->dbck);
>>> -
>>> - ? ? ? ? ? ? if (!off_mode)
>>> + ? ? ? ? ? ? if (!off_mode) {
>>> + ? ? ? ? ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>> + ? ? ? ? ? ? }
>>>
>>> ? ? ? ? ? ? ? /* If going to OFF, remove triggering for all
>>> ? ? ? ? ? ? ? ?* non-wakeup GPIOs. ?Otherwise spurious IRQs will be
>>> @@ -1151,15 +1176,16 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>>> ? ? ? ? ? ? ? __raw_writel(l2, bank->base + bank->regs->risingdetect);
>>>
>>> ?save_gpio_context:
>>> -
>>> ? ? ? ? ? ? ? if (bank->get_context_loss_count)
>>> ? ? ? ? ? ? ? ? ? ? ? bank->context_loss_count =
>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>>>
>>> ? ? ? ? ? ? ? omap_gpio_save_context(bank);
>>>
>>> - ? ? ? ? ? ? if (!pm_runtime_suspended(bank->dev))
>>> + ? ? ? ? ? ? if (!pm_runtime_suspended(bank->dev)) {
>>> ? ? ? ? ? ? ? ? ? ? ? pm_runtime_put_sync(bank->dev);
>>> + ? ? ? ? ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>> + ? ? ? ? ? ? }
>>> ? ? ? }
>>> ?}
>>>
>>> @@ -1170,13 +1196,11 @@ void omap2_gpio_resume_after_idle(void)
>>> ? ? ? list_for_each_entry(bank, &omap_gpio_list, node) {
>>> ? ? ? ? ? ? ? u32 context_lost_cnt_after;
>>> ? ? ? ? ? ? ? u32 l = 0, gen, gen0, gen1;
>>> - ? ? ? ? ? ? int j;
>>>
>>> ? ? ? ? ? ? ? if (!bank->loses_context)
>>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>>
>>> - ? ? ? ? ? ? for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
>>> - ? ? ? ? ? ? ? ? ? ? clk_enable(bank->dbck);
>>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>>> ? ? ? ? ? ? ? if (pm_runtime_suspended(bank->dev))
>>> ? ? ? ? ? ? ? ? ? ? ? pm_runtime_get_sync(bank->dev);
>>
>> Kevin
>>

[PATCH v8 REPOST 17/24] gpio/omap: fix debounce clock handling

[DebBarma, Tarun Kanti]

On Mon, Dec 5, 2011 at 7:27 PM, DebBarma, Tarun Kanti
<tarun.kanti@ti.com> wrote:
> On Mon, Nov 7, 2011 at 7:19 PM, DebBarma, Tarun Kanti
> <tarun.kanti@ti.com> wrote:
>> On Fri, Nov 4, 2011 at 10:10 PM, Kevin Hilman <khilman@ti.com> wrote:
>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes:
>>>
>>>> Currently debounce clock state is not tracked in the system.
>>>
>>> ??
>>>
>>> bank->dbck_enable_mask ?
>> As I understand, this only tells which all GPIOs have debounce timeout enabled.
>> But, during system operation as debounce clocks are enabled and disabled I need
>> additional flag to keep track of current state (enabled/disabled).
>> This is what I meant.
>>
>>>
>>>> The bank->dbck
>>>> is enabled/disabled in suspend/idle paths irrespective of whether debounce
>>>> interval has been set or not.
>>>
>>> No. ?It's conditional based on bank->dbck_enable_mask, which is based on
>>> whether or not debounce has been enabled.
>> You are right. I need to rephrase my description.
>>
>>>
>>>> Ideally, it should be handled only for those
>>>> gpio banks where the debounce is enabled.
>>>
>>> AFAICT, it is. ?Please explain more what is actually happening in the
>>> patch, and why.
>> Yes, as I explained above, it is more about the tracking the debounce clock
>> enabled/disabled state for those GPIOs whose debounce timeouts are enabled.
>> I will modify the patch description.
>>
>>>
>>>> In _set_gpio_debounce, enable debounce clock before accessing
>>>> registers.
>>>
>>> This is a separate issue/bug and wants its own patch with descriptive
>>> changelog.
>> OK.
>>
>>>
>>>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>
>>>> ---
>>>> During further internal testing it was found that image was crashing within
>>>> _set_gpio_debounce(). It is observed that we are trying to access registers
>>>> without enabling debounce clock. This patch incorporates the change whereby
>>>> the debounce clock is enabled before accessing registers and disabled at the
>>>> end of the function.
>>>>
>>>> ?drivers/gpio/gpio-omap.c | ? 60 ++++++++++++++++++++++++++++++++-------------
>>>> ?1 files changed, 42 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>>>> index fa6c9c5..85e9c2a 100644
>>>> --- a/drivers/gpio/gpio-omap.c
>>>> +++ b/drivers/gpio/gpio-omap.c
>>>> @@ -65,6 +65,7 @@ struct gpio_bank {
>>>> ? ? ? struct clk *dbck;
>>>> ? ? ? u32 mod_usage;
>>>> ? ? ? u32 dbck_enable_mask;
>>>> + ? ? bool dbck_enabled;
>>>> ? ? ? struct device *dev;
>>>> ? ? ? bool is_mpuio;
>>>> ? ? ? bool dbck_flag;
>>>> @@ -156,6 +157,22 @@ static inline void _gpio_rmw(void __iomem *base, u32 reg, u32 mask, bool set)
>>>> ? ? ? __raw_writel(l, base + reg);
>>>> ?}
>>>>
>>>> +static inline void _gpio_dbck_enable(struct gpio_bank *bank)
>>>> +{
>>>> + ? ? if (bank->dbck_enable_mask && !bank->dbck_enabled) {
>>>> + ? ? ? ? ? ? clk_enable(bank->dbck);
>>>> + ? ? ? ? ? ? bank->dbck_enabled = true;
>>>> + ? ? }
>>>> +}
>>>> +
>>>> +static inline void _gpio_dbck_disable(struct gpio_bank *bank)
>>>> +{
>>>> + ? ? if (bank->dbck_enable_mask && bank->dbck_enabled) {
>>>> + ? ? ? ? ? ? clk_disable(bank->dbck);
>>>> + ? ? ? ? ? ? bank->dbck_enabled = false;
>>>> + ? ? }
>>>> +}
>>>> +
>>>> ?/**
>>>> ? * _set_gpio_debounce - low level gpio debounce time
>>>> ? * @bank: the gpio bank we're acting upon
>>>> @@ -184,22 +201,22 @@ static void _set_gpio_debounce(struct gpio_bank *bank, unsigned gpio,
>>>>
>>>> ? ? ? l = GPIO_BIT(bank, gpio);
>>>>
>>>> + ? ? clk_enable(bank->dbck);
>>>> ? ? ? reg = bank->base + bank->regs->debounce;
>>>> ? ? ? __raw_writel(debounce, reg);
>>>>
>>>> ? ? ? reg = bank->base + bank->regs->debounce_en;
>>>> ? ? ? val = __raw_readl(reg);
>>>>
>>>> - ? ? if (debounce) {
>>>> + ? ? if (debounce)
>>>> ? ? ? ? ? ? ? val |= l;
>>>> - ? ? ? ? ? ? clk_enable(bank->dbck);
>>>> - ? ? } else {
>>>> + ? ? else
>>>> ? ? ? ? ? ? ? val &= ~l;
>>>> - ? ? ? ? ? ? clk_disable(bank->dbck);
>>>> - ? ? }
>>>> +
>>>> ? ? ? bank->dbck_enable_mask = val;
>>>>
>>>> ? ? ? __raw_writel(val, reg);
>>>> + ? ? clk_disable(bank->dbck);
>>>> ?}
>>>>
>>>> ?static inline void set_gpio_trigger(struct gpio_bank *bank, int gpio,
>>>> @@ -485,8 +502,10 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
>>>> ? ? ? ?* If this is the first gpio_request for the bank,
>>>> ? ? ? ?* enable the bank module.
>>>> ? ? ? ?*/
>>>> - ? ? if (!bank->mod_usage)
>>>> + ? ? if (!bank->mod_usage) {
>>>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>>>> ? ? ? ? ? ? ? pm_runtime_get_sync(bank->dev);
>>>> + ? ? }
>>>>
>>>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>>> ? ? ? /* Set trigger to none. You need to enable the desired trigger with
>>>> @@ -549,8 +568,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) {
>>>> ? ? ? ? ? ? ? pm_runtime_put_sync(bank->dev);
>>>> + ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>>
>>> Why not add this to the ->runtime_suspend() callback?
>> Yes, I can move there and test.
>>
>>>
>>>> + ? ? }
>>>> ?}
>>>>
>>>> ?/*
>>>> @@ -829,8 +850,10 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
>>>>
>>>> ? ? ? if (!bank->dbck) {
>>>> ? ? ? ? ? ? ? bank->dbck = clk_get(bank->dev, "dbclk");
>>>> - ? ? ? ? ? ? if (IS_ERR(bank->dbck))
>>>> + ? ? ? ? ? ? if (IS_ERR(bank->dbck)) {
>>>> ? ? ? ? ? ? ? ? ? ? ? dev_err(bank->dev, "Could not get gpio dbck\n");
>>>> + ? ? ? ? ? ? ? ? ? ? return -EINVAL;
>>>> + ? ? ? ? ? ? }
>>>> ? ? ? }
>>>>
>>>> ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>>> @@ -1086,6 +1109,8 @@ static int omap_gpio_suspend(struct device *dev)
>>>> ? ? ? ? ? ? ? bank->saved_wakeup = __raw_readl(wake_status);
>>>> ? ? ? ? ? ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1);
>>>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>>> +
>>>> + ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>>
>>> If this call was in the ->runtime_suspend() callback, you wouldn't need
>>> it here.
>> Yes.
>>
>>>
>>>> ? ? ? }
>>>>
>>>> ? ? ? return 0;
>>>> @@ -1102,6 +1127,8 @@ static int omap_gpio_resume(struct device *dev)
>>>> ? ? ? ? ? ? ? if (!bank->regs->wkup_en)
>>>> ? ? ? ? ? ? ? ? ? ? ? return 0;
>>>>
>>>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>>>
>>> Similarily, this call should be in the ->runtime_resume() callback and
>>> it wouldn't be needed here.
>> Right.
>>
>>>
>>> Using the runtime PM callbacks, all the _gpio_dbck_* calls below would
>>> not be needed.
>> Yes.
> I removed the ?_gpio_dbck_disable/enable calls are removed from
> *_prepare_for_idle()
> and *_resume_after_idle(). But I am not seeing runtime callbacks being
> called and
> hence the ?_gpio_dbck_* calls are not getting called.
>
> I have also tried making explicit pm_runtime_put() and pm_runtime_get_sync() in
> *_prepare_for_idle() and *_resume_after_idle(). Still, I do not see
> the runtime callbacks
> getting called.
>
> Is there anything that I could be missing?
Sorry for the silly mistake.
I used !off_mode check before calling pm_runtime_put_sync_suspend()
in *_prepare_for_idle(). This resulted in use count mismatch with
*_get_sync() call in *_resume_after_idle() which is called both for
offmode and retention. I have removed the off_mode and made it unused.


void omap2_gpio_prepare_for_idle(int off_mode)
{
 	struct gpio_bank *bank;

 	list_for_each_entry(bank, &omap_gpio_list, node) {
 		if (!bank->mod_usage || !off_mode)
 			continue;

		pm_runtime_put_sync_suspend(bank->dev);
 	}
 }

void omap2_gpio_resume_after_idle(void)
 	struct gpio_bank *bank;

 	list_for_each_entry(bank, &omap_gpio_list, node) {
 		if (!bank->mod_usage)
 			continue;

		pm_runtime_get_sync(bank->dev);
 	}
 }

--
Tarun

>>>
>>>> ? ? ? ? ? ? ? spin_lock_irqsave(&bank->lock, flags);
>>>> ? ? ? ? ? ? ? _gpio_rmw(base, bank->regs->wkup_en, bank->saved_wakeup, 1);
>>>> ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>>> @@ -1120,16 +1147,14 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>>>>
>>>> ? ? ? list_for_each_entry(bank, &omap_gpio_list, node) {
>>>> ? ? ? ? ? ? ? u32 l1 = 0, l2 = 0;
>>>> - ? ? ? ? ? ? int j;
>>>>
>>>> ? ? ? ? ? ? ? if (!bank->loses_context)
>>>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>>>
>>>> - ? ? ? ? ? ? for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
>>>> - ? ? ? ? ? ? ? ? ? ? clk_disable(bank->dbck);
>>>> -
>>>> - ? ? ? ? ? ? if (!off_mode)
>>>> + ? ? ? ? ? ? if (!off_mode) {
>>>> + ? ? ? ? ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>>> + ? ? ? ? ? ? }
>>>>
>>>> ? ? ? ? ? ? ? /* If going to OFF, remove triggering for all
>>>> ? ? ? ? ? ? ? ?* non-wakeup GPIOs. ?Otherwise spurious IRQs will be
>>>> @@ -1151,15 +1176,16 @@ void omap2_gpio_prepare_for_idle(int off_mode)
>>>> ? ? ? ? ? ? ? __raw_writel(l2, bank->base + bank->regs->risingdetect);
>>>>
>>>> ?save_gpio_context:
>>>> -
>>>> ? ? ? ? ? ? ? if (bank->get_context_loss_count)
>>>> ? ? ? ? ? ? ? ? ? ? ? bank->context_loss_count =
>>>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>>>>
>>>> ? ? ? ? ? ? ? omap_gpio_save_context(bank);
>>>>
>>>> - ? ? ? ? ? ? if (!pm_runtime_suspended(bank->dev))
>>>> + ? ? ? ? ? ? if (!pm_runtime_suspended(bank->dev)) {
>>>> ? ? ? ? ? ? ? ? ? ? ? pm_runtime_put_sync(bank->dev);
>>>> + ? ? ? ? ? ? ? ? ? ? _gpio_dbck_disable(bank);
>>>> + ? ? ? ? ? ? }
>>>> ? ? ? }
>>>> ?}
>>>>
>>>> @@ -1170,13 +1196,11 @@ void omap2_gpio_resume_after_idle(void)
>>>> ? ? ? list_for_each_entry(bank, &omap_gpio_list, node) {
>>>> ? ? ? ? ? ? ? u32 context_lost_cnt_after;
>>>> ? ? ? ? ? ? ? u32 l = 0, gen, gen0, gen1;
>>>> - ? ? ? ? ? ? int j;
>>>>
>>>> ? ? ? ? ? ? ? if (!bank->loses_context)
>>>> ? ? ? ? ? ? ? ? ? ? ? continue;
>>>>
>>>> - ? ? ? ? ? ? for (j = 0; j < hweight_long(bank->dbck_enable_mask); j++)
>>>> - ? ? ? ? ? ? ? ? ? ? clk_enable(bank->dbck);
>>>> + ? ? ? ? ? ? _gpio_dbck_enable(bank);
>>>> ? ? ? ? ? ? ? if (pm_runtime_suspended(bank->dev))
>>>> ? ? ? ? ? ? ? ? ? ? ? pm_runtime_get_sync(bank->dev);
>>>
>>> Kevin
>>>