gpio/omap: fix debounce clock handling

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

---
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(-)

drivers/gpio/gpio-omap.c +25 -6

index fa6c9c5..85e9c2a 100644
@@ -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

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

+	}
 }
 
 /*

@@ -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);

drivers/gpio/gpio-omap.c +6 -7

 		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;

Quoted 1 line · Tarun Kanti DebBarma

Currently debounce clock state is not tracked in the system.

??

bank->dbck_enable_mask ?

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

In _set_gpio_debounce, enable debounce clock before accessing registers.

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

Quoted 85 lines

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
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(-)

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

index fa6c9c5..85e9c2a 100644
@@ -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

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

@@ -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);
+ ? ? }

Quoted 10 lines

? ? ? spin_lock_irqsave(&bank->lock, flags);
? ? ? /* Set trigger to none. You need to enable the desired trigger with

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

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

Quoted 16 lines

+ ? ? }
?}

?/*

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

@@ -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;
+ ? ? ? ? ? ? }
 ? ? ? }
 

drivers/gpio/gpio-omap.c static int omap_gpio_suspend(struct device *dev) · line 1113

Quoted 7 lines

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

 ? ? ? 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.

Quoted 8 lines

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

 ? ? ? }
 
 ? ? ? return 0;

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

drivers/gpio/gpio-omap.c static int omap_gpio_resume(struct device *dev) · line 1130

Quoted 1 line

drivers/gpio/gpio-omap.c +1 -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.

Quoted 47 lines

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

 ? ? ? ? ? ? ? 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

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

@@ -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);
+ ? ? ? ? ? ? }
? ? ? }
?}

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

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

drivers/gpio/gpio-omap.c void omap2_gpio_resume_after_idle(void) · line 1205

Quoted 10 lines

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

- ? ? ? ? ? ? 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

Quoted 7 lines · Kevin Hilman

Quoted 1 line · Tarun Kanti DebBarma

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 85 lines

Quoted 85 lines

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
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(-)

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

index fa6c9c5..85e9c2a 100644
@@ -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

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

@@ -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);
+ ? ? }

Quoted 12 lines

Quoted 10 lines

? ? ? spin_lock_irqsave(&bank->lock, flags);
? ? ? /* Set trigger to none. You need to enable the desired trigger with

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

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

Quoted 16 lines

Quoted 16 lines

+ ? ? }
?}

?/*

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

@@ -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;
+ ? ? ? ? ? ? }
 ? ? ? }
 

Quoted 10 lines

drivers/gpio/gpio-omap.c static int omap_gpio_suspend(struct device *dev) · line 1113

Quoted 7 lines

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

 ? ? ? 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.

Quoted 8 lines

Quoted 8 lines

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

 ? ? ? }
 
 ? ? ? return 0;

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

Quoted 4 lines

drivers/gpio/gpio-omap.c static int omap_gpio_resume(struct device *dev) · line 1130

Quoted 1 line

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

+ ? ? ? ? ? ? _gpio_dbck_enable(bank);

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

Right.

Quoted 3 lines

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

Yes.

Quoted 60 lines

Quoted 47 lines

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

 ? ? ? ? ? ? ? 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

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

@@ -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);
+ ? ? ? ? ? ? }
? ? ? }
?}

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

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

drivers/gpio/gpio-omap.c void omap2_gpio_resume_after_idle(void) · line 1205

Quoted 10 lines

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

- ? ? ? ? ? ? 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

Quoted 179 lines · Tarun Kanti

Quoted 7 lines · Kevin Hilman

Quoted 1 line · Tarun Kanti DebBarma

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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 6 lines

Quoted 3 lines

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.

Quoted 85 lines

Quoted 85 lines

Signed-off-by Tarun Kanti DebBarma <[email protected]>

---
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(-)

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

index fa6c9c5..85e9c2a 100644
@@ -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

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

@@ -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);
+ ? ? }

Quoted 12 lines

Quoted 10 lines

? ? ? spin_lock_irqsave(&bank->lock, flags);
? ? ? /* Set trigger to none. You need to enable the desired trigger with

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

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

Quoted 16 lines

Quoted 16 lines

+ ? ? }
?}

?/*

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

@@ -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;
+ ? ? ? ? ? ? }
 ? ? ? }
 

Quoted 10 lines

drivers/gpio/gpio-omap.c static int omap_gpio_suspend(struct device *dev) · line 1113

Quoted 7 lines

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

 ? ? ? 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.

Quoted 8 lines

Quoted 8 lines

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

 ? ? ? }
 
 ? ? ? return 0;

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

Quoted 4 lines

drivers/gpio/gpio-omap.c static int omap_gpio_resume(struct device *dev) · line 1130

Quoted 1 line

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

+ ? ? ? ? ? ? _gpio_dbck_enable(bank);

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

Quoted 6 lines

Right.

Quoted 3 lines

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?

Quoted 60 lines

Quoted 60 lines

Quoted 47 lines

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

 ? ? ? ? ? ? ? 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

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

@@ -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);
+ ? ? ? ? ? ? }
? ? ? }
?}

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

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

drivers/gpio/gpio-omap.c void omap2_gpio_resume_after_idle(void) · line 1205

Quoted 10 lines

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

- ? ? ? ? ? ? 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