[PATCH V2] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[PATCH V2] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Chao Xu]

From: Felipe Balbi <balbi@ti.com>

try to keep gpio block suspended as much as possible.

Tested with pandaboard and a sysfs exported gpio.

Signed-off-by: Felipe Balbi <balbi at ti.com>

[caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added revision check to enable aggressive pm_runtime on OMAP4-only. Because am33xx_gpio_sysc.idlemodes seems to be wrongly marked as SIDLE_SMART_WKUP, which might cause missed interrupts with this patch. Tested on Pandaboard rev A2.]

Signed-off-by: Chao Xu <caesarxuchao@gmail.com>
---
According to Mr. Felipe Balbi, the original patch was not accepted because interrupts would be missed when GPIO was used as IRQ source. But in my tests with pandaboard, interrupts were not missed. This is because _idle_sysc() sets the idlemode of gpio module to smart-idle-wakeup, and according to OMAP4430 TRM, under this idlemode, the gpio can generate an asynchronous wakeup request to the PRCM. And after GPIO is awake, the wake-up request is reflected into the interrupt status registers. 

A change made on the original patch is only applying the aggressive runtime pm scheme on OMAP4, because I don’t have HW to test OMAP3 or am33xx devices. According to the respective TRMs, their GPIO modules also can generate wake-up request if the idlemode is set to smart-idle or smart-idle-wakeup. So the patch should work for them, too. But I suspect a potential SW bug may cause missed interrupts: the am33xx_gpio_sysc.idlemodes is marked as capable of SIDLE_SMART_WKUP in omap_hwmod_33xx.c. But according to AM335x TRM, _only_ gpio0 is capable of this mode. This may make GPIO stuck in force-idle mode and unable to generate wakeup request. And thus interrupt will be missed. Again, I don’t have the HW to verify whether this is a bug or not :(

 drivers/gpio/gpio-omap.c                |  103 ++++++++++++++++++++++++++-----
 include/linux/platform_data/gpio-omap.h |    1 +
 2 files changed, 87 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 89675f8..90661f2 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -76,6 +76,7 @@ struct gpio_bank {
 	int context_loss_count;
 	int power_mode;
 	bool workaround_enabled;
+	bool is_aggressive_pm;
 
 	void (*set_dataout)(struct gpio_bank *bank, int gpio, int enable);
 	int (*get_context_loss_count)(struct device *dev);
@@ -473,8 +474,15 @@ static void _disable_gpio_module(struct gpio_bank *bank, unsigned offset)
 static int gpio_is_input(struct gpio_bank *bank, int mask)
 {
 	void __iomem *reg = bank->base + bank->regs->direction;
+	u32 val;
 
-	return __raw_readl(reg) & mask;
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
+	val = __raw_readl(reg) & mask;
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
+
+	return val;
 }
 
 static int gpio_irq_type(struct irq_data *d, unsigned type)
@@ -485,7 +493,7 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 	unsigned long flags;
 	unsigned offset;
 
-	if (!BANK_USED(bank))
+	if (!BANK_USED(bank) && !bank->is_aggressive_pm)
 		pm_runtime_get_sync(bank->dev);
 
 #ifdef CONFIG_ARCH_OMAP1
@@ -503,6 +511,8 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
 		return -EINVAL;
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 	offset = GPIO_INDEX(bank, gpio);
 	retval = _set_gpio_triggering(bank, offset, type);
@@ -511,11 +521,16 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 		_set_gpio_direction(bank, offset, 1);
 	} else if (!gpio_is_input(bank, 1 << offset)) {
 		spin_unlock_irqrestore(&bank->lock, flags);
+		if (bank->is_aggressive_pm)
+			pm_runtime_put(bank->dev);
+
 		return -EINVAL;
 	}
 
 	bank->irq_usage |= 1 << GPIO_INDEX(bank, gpio);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
 
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
 		__irq_set_handler_locked(d->irq, handle_level_irq);
@@ -668,10 +683,11 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 	unsigned long flags;
 
 	/*
-	 * If this is the first gpio_request for the bank,
-	 * enable the bank module.
+	 * if aggressive runtime pm is supported, enable the bank module
+	 * for each gpio_request. Otherwise enable the bank module if this
+	 * is the first gpio_request for the bank.
 	 */
-	if (!BANK_USED(bank))
+	if (bank->is_aggressive_pm || !BANK_USED(bank))
 		pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
@@ -685,7 +701,8 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 	}
 	bank->mod_usage |= 1 << offset;
 	spin_unlock_irqrestore(&bank->lock, flags);
-
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
 	return 0;
 }
 
@@ -694,6 +711,9 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
 	unsigned long flags;
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
+
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->mod_usage &= ~(1 << offset);
 	_disable_gpio_module(bank, offset);
@@ -701,10 +721,11 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	/*
-	 * If this is the last gpio to be freed in the bank,
-	 * disable the bank module.
+	 * if aggressive runtime pm is supported, disable the bank module
+	 * for each gpio_free. Otherwise disable the bank module if this
+	 * is the last gpio to be freed in the bank.
 	 */
-	if (!BANK_USED(bank))
+	if (bank->is_aggressive_pm || !BANK_USED(bank))
 		pm_runtime_put(bank->dev);
 }
 
@@ -796,17 +817,19 @@ static void gpio_irq_shutdown(struct irq_data *d)
 	unsigned long flags;
 	unsigned offset = GPIO_INDEX(bank, gpio);
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->irq_usage &= ~(1 << offset);
 	_disable_gpio_module(bank, offset);
 	_reset_gpio(bank, gpio);
 	spin_unlock_irqrestore(&bank->lock, flags);
-
 	/*
-	 * If this is the last IRQ to be freed in the bank,
-	 * disable the bank module.
+	 * if aggressive runtime pm is supported, disable the bank module
+	 * for each irq_shutdown. Otherwise disable the bank module if this
+	 * is the last IRQ to be freed in the bank.
 	 */
-	if (!BANK_USED(bank))
+	if (bank->is_aggressive_pm || !BANK_USED(bank))
 		pm_runtime_put(bank->dev);
 }
 
@@ -815,7 +838,11 @@ static void gpio_ack_irq(struct irq_data *d)
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	_clear_gpio_irqstatus(bank, gpio);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
 }
 
 static void gpio_mask_irq(struct irq_data *d)
@@ -824,10 +851,14 @@ static void gpio_mask_irq(struct irq_data *d)
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_irqenable(bank, gpio, 0);
 	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
 }
 
 static void gpio_unmask_irq(struct irq_data *d)
@@ -838,6 +869,8 @@ static void gpio_unmask_irq(struct irq_data *d)
 	u32 trigger = irqd_get_trigger_type(d);
 	unsigned long flags;
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 	if (trigger)
 		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
@@ -851,6 +884,8 @@ static void gpio_unmask_irq(struct irq_data *d)
 
 	_set_gpio_irqenable(bank, gpio, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
 }
 
 static struct irq_chip gpio_irq_chip = {
@@ -933,9 +968,15 @@ static int gpio_input(struct gpio_chip *chip, unsigned offset)
 	unsigned long flags;
 
 	bank = container_of(chip, struct gpio_bank, chip);
+
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_direction(bank, offset, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
+
 	return 0;
 }
 
@@ -944,13 +985,20 @@ static int gpio_get(struct gpio_chip *chip, unsigned offset)
 	struct gpio_bank *bank;
 	u32 mask;
 
+	int val;
 	bank = container_of(chip, struct gpio_bank, chip);
 	mask = (1 << offset);
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	if (gpio_is_input(bank, mask))
-		return _get_gpio_datain(bank, offset);
+		val = _get_gpio_datain(bank, offset);
 	else
-		return _get_gpio_dataout(bank, offset);
+		val = _get_gpio_dataout(bank, offset);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
+
+	return val;
 }
 
 static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
@@ -960,6 +1008,9 @@ static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
 	int retval = 0;
 
 	bank = container_of(chip, struct gpio_bank, chip);
+
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	if (LINE_USED(bank->irq_usage, offset)) {
@@ -972,6 +1023,9 @@ static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
 
 exit:
 	spin_unlock_irqrestore(&bank->lock, flags);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
+
 	return retval;
 }
 
@@ -983,9 +1037,13 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
 
 	bank = container_of(chip, struct gpio_bank, chip);
 
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_debounce(bank, offset, debounce);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
 
 	return 0;
 }
@@ -996,9 +1054,14 @@ static void gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	unsigned long flags;
 
 	bank = container_of(chip, struct gpio_bank, chip);
+
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->set_dataout(bank, offset, value);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
 }
 
 /*---------------------------------------------------------------------*/
@@ -1168,6 +1231,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	bank->is_mpuio = pdata->is_mpuio;
 	bank->non_wakeup_gpios = pdata->non_wakeup_gpios;
 	bank->regs = pdata->regs;
+	bank->is_aggressive_pm = pdata->is_aggressive_pm;
 #ifdef CONFIG_OF_GPIO
 	bank->chip.of_node = of_node_get(node);
 #endif
@@ -1449,7 +1513,8 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
 
 		bank->power_mode = pwr_mode;
 
-		pm_runtime_put_sync_suspend(bank->dev);
+		if (!pm_runtime_suspended(bank->dev))
+			pm_runtime_suspend(bank->dev);
 	}
 }
 
@@ -1461,7 +1526,8 @@ void omap2_gpio_resume_after_idle(void)
 		if (!BANK_USED(bank) || !bank->loses_context)
 			continue;
 
-		pm_runtime_get_sync(bank->dev);
+		if (pm_runtime_suspended(bank->dev))
+			pm_runtime_resume(bank->dev);
 	}
 }
 
@@ -1585,18 +1651,21 @@ static const struct omap_gpio_platform_data omap2_pdata = {
 	.regs = &omap2_gpio_regs,
 	.bank_width = 32,
 	.dbck_flag = false,
+	.is_aggressive_pm = false,
 };
 
 static const struct omap_gpio_platform_data omap3_pdata = {
 	.regs = &omap2_gpio_regs,
 	.bank_width = 32,
 	.dbck_flag = true,
+	.is_aggressive_pm = false,
 };
 
 static const struct omap_gpio_platform_data omap4_pdata = {
 	.regs = &omap4_gpio_regs,
 	.bank_width = 32,
 	.dbck_flag = true,
+	.is_aggressive_pm = true,
 };
 
 static const struct of_device_id omap_gpio_match[] = {
diff --git a/include/linux/platform_data/gpio-omap.h b/include/linux/platform_data/gpio-omap.h
index 5d50b25..bb033b1 100644
--- a/include/linux/platform_data/gpio-omap.h
+++ b/include/linux/platform_data/gpio-omap.h
@@ -200,6 +200,7 @@ struct omap_gpio_platform_data {
 	bool dbck_flag;		/* dbck required or not - True for OMAP3&4 */
 	bool loses_context;	/* whether the bank would ever lose context */
 	bool is_mpuio;		/* whether the bank is of type MPUIO */
+	bool is_aggressive_pm; /* whether aggressive runtime pm is supported*/
 	u32 non_wakeup_gpios;
 
 	struct omap_gpio_reg_offs *regs;
-- 
1.7.9.5

Re: [PATCH V2] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 780 bytes --]

Hi,

On Tue, Nov 26, 2013 at 04:46:44PM -0600, Chao Xu wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> try to keep gpio block suspended as much as possible.
> 
> Tested with pandaboard and a sysfs exported gpio.
> 
> Signed-off-by: Felipe Balbi <balbi at ti.com>
> 
> [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added
> revision check to enable aggressive pm_runtime on OMAP4-only. Because
> am33xx_gpio_sysc.idlemodes seems to be wrongly marked as
> SIDLE_SMART_WKUP, which might cause missed interrupts with this patch.
> Tested on Pandaboard rev A2.]

Based on a quick look, it looks good to my eyes. Didn't test on any HW
though.

Signed-of-by: Felipe Balbi <balbi@ti.com>

> Signed-off-by: Chao Xu <caesarxuchao@gmail.com>

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V2] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Linus Walleij]

On Tue, Nov 26, 2013 at 11:46 PM, Chao Xu <caesarxuchao@gmail.com> wrote:

> From: Felipe Balbi <balbi@ti.com>
>
> try to keep gpio block suspended as much as possible.
>
> Tested with pandaboard and a sysfs exported gpio.
>
> Signed-off-by: Felipe Balbi <balbi at ti.com>
>
> [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added revision check to enable aggressive pm_runtime on OMAP4-only. Because am33xx_gpio_sysc.idlemodes seems to be wrongly marked as SIDLE_SMART_WKUP, which might cause missed interrupts with this patch. Tested on Pandaboard rev A2.]
>
> Signed-off-by: Chao Xu <caesarxuchao@gmail.com>

I'd like Tony and/or Kevin to have a look at this from a PM point
of view so I get some confirmation from the core maintainers
that this is the way forward.

Yours,
Linus Walleij

Re: [PATCH V2] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Santosh Shilimkar]

Adding Kevin's Linaro id, + Nishant,

On Tuesday 26 November 2013 05:46 PM, Chao Xu wrote:
> From: Felipe Balbi <balbi@ti.com>
> 
> try to keep gpio block suspended as much as possible.
> 
> Tested with pandaboard and a sysfs exported gpio.
> 
> Signed-off-by: Felipe Balbi <balbi at ti.com>
> 
> [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added revision check to enable aggressive pm_runtime on OMAP4-only. Because am33xx_gpio_sysc.idlemodes seems to be wrongly marked as SIDLE_SMART_WKUP, which might cause missed interrupts with this patch. Tested on Pandaboard rev A2.]
> 
Please format the text and limit it to 80 char rule.

> Signed-off-by: Chao Xu <caesarxuchao@gmail.com>
> ---
> According to Mr. Felipe Balbi, the original patch was not accepted because interrupts would be missed when GPIO was used as IRQ source. But in my tests with pandaboard, interrupts were not missed. This is because _idle_sysc() sets the idlemode of gpio module to smart-idle-wakeup, and according to OMAP4430 TRM, under this idlemode, the gpio can generate an asynchronous wakeup request to the PRCM. And after GPIO is awake, the wake-up request is reflected into the interrupt status registers. 
> 
> A change made on the original patch is only applying the aggressive runtime pm scheme on OMAP4, because I don’t have HW to test OMAP3 or am33xx devices. According to the respective TRMs, their GPIO modules also can generate wake-up request if the idlemode is set to smart-idle or smart-idle-wakeup. So the patch should work for them, too. But I suspect a potential SW bug may cause missed interrupts: the am33xx_gpio_sysc.idlemodes is marked as capable of SIDLE_SMART_WKUP in omap_hwmod_33xx.c. But according to AM335x TRM, _only_ gpio0 is capable of this mode. This may make GPIO stuck in force-idle mode and unable to generate wakeup request. And thus interrupt will be missed. Again, I don’t have the HW to verify whether this is a bug or not :(
> 
In general I am not against this idea but patch has many assumptions which
are not entirely correct.

- SMART_WAKEUP will take care of waking IP etc not always true to power
domain getting into idle.

- If we want to go on this path then I would like to see we address
omap2_gpio_[prepare/resumne]_for_idle()

- GPIO though sound simple is one of the most notorious IP block
on PM front so this needs lot of testing. This patch not seems be
tested at all so I would have much liked it to be in RFC/RFT
state.

>  drivers/gpio/gpio-omap.c                |  103 ++++++++++++++++++++++++++-----
>  include/linux/platform_data/gpio-omap.h |    1 +
>  2 files changed, 87 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 89675f8..90661f2 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -76,6 +76,7 @@ struct gpio_bank {
>  	int context_loss_count;
>  	int power_mode;
>  	bool workaround_enabled;
> +	bool is_aggressive_pm;
>  
>  	void (*set_dataout)(struct gpio_bank *bank, int gpio, int enable);
>  	int (*get_context_loss_count)(struct device *dev);
> @@ -473,8 +474,15 @@ static void _disable_gpio_module(struct gpio_bank *bank, unsigned offset)
>  static int gpio_is_input(struct gpio_bank *bank, int mask)
>  {
>  	void __iomem *reg = bank->base + bank->regs->direction;
> +	u32 val;
>  
> -	return __raw_readl(reg) & mask;
> +	if (bank->is_aggressive_pm)
> +		pm_runtime_get_sync(bank->dev);
> +	val = __raw_readl(reg) & mask;
> +	if (bank->is_aggressive_pm)
> +		pm_runtime_put(bank->dev);
> +
Move above in some wrapper instead of sprinkling the
'is_aggressive_pm' check everywhere.
  
> @@ -1585,18 +1651,21 @@ static const struct omap_gpio_platform_data omap2_pdata = {
>  	.regs = &omap2_gpio_regs,
>  	.bank_width = 32,
>  	.dbck_flag = false,
> +	.is_aggressive_pm = false,
>  };
>  
>  static const struct omap_gpio_platform_data omap3_pdata = {
>  	.regs = &omap2_gpio_regs,
>  	.bank_width = 32,
>  	.dbck_flag = true,
> +	.is_aggressive_pm = false,
>  };
>  
>  static const struct omap_gpio_platform_data omap4_pdata = {
>  	.regs = &omap4_gpio_regs,
>  	.bank_width = 32,
>  	.dbck_flag = true,
> +	.is_aggressive_pm = true,
>  };
>  
Well OMAP IP shouldn't have different behavior on OMAP3 and
OMAP4 at least so something you can enable for OMAP4, you
should be able to do it for OMAP3 as well.

Kevin might have some more concerns to add.

Regards,
Santosh

Re: [PATCH V2] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Chao Xu]

Sorry for the late reply.

On Fri, Nov 29, 2013 at 11:04 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> Adding Kevin's Linaro id, + Nishant,
>
> On Tuesday 26 November 2013 05:46 PM, Chao Xu wrote:
>> From: Felipe Balbi <balbi@ti.com>
>>
>> try to keep gpio block suspended as much as possible.
>>
>> Tested with pandaboard and a sysfs exported gpio.
>>
>> Signed-off-by: Felipe Balbi <balbi at ti.com>
>>
>> [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added revision check to enable aggressive pm_runtime on OMAP4-only. Because am33xx_gpio_sysc.idlemodes seems to be wrongly marked as SIDLE_SMART_WKUP, which might cause missed interrupts with this patch. Tested on Pandaboard rev A2.]
>>
> Please format the text and limit it to 80 char rule.
>
Sorry. It's my first time submitting a patch. I will fix it and resubmit.
>> Signed-off-by: Chao Xu <caesarxuchao@gmail.com>
>> ---
>> According to Mr. Felipe Balbi, the original patch was not accepted because interrupts would be missed when GPIO was used as IRQ source. But in my tests with pandaboard, interrupts were not missed. This is because _idle_sysc() sets the idlemode of gpio module to smart-idle-wakeup, and according to OMAP4430 TRM, under this idlemode, the gpio can generate an asynchronous wakeup request to the PRCM. And after GPIO is awake, the wake-up request is reflected into the interrupt status registers.
>>
>> A change made on the original patch is only applying the aggressive runtime pm scheme on OMAP4, because I don’t have HW to test OMAP3 or am33xx devices. According to the respective TRMs, their GPIO modules also can generate wake-up request if the idlemode is set to smart-idle or smart-idle-wakeup. So the patch should work for them, too. But I suspect a potential SW bug may cause missed interrupts: the am33xx_gpio_sysc.idlemodes is marked as capable of SIDLE_SMART_WKUP in omap_hwmod_33xx.c. But according to AM335x TRM, _only_ gpio0 is capable of this mode. This may make GPIO stuck in force-idle mode and unable to generate wakeup request. And thus interrupt will be missed. Again, I don’t have the HW to verify whether this is a bug or not :(
>>
> In general I am not against this idea but patch has many assumptions which
> are not entirely correct.
>
> - SMART_WAKEUP will take care of waking IP etc not always true to power
> domain getting into idle.
>
I agree that if the power domain goes to idle, SMART_WAKEUP
won't be effective. But, correct me if i'm wrong, even if we don't call
runtime_put(), the power domain can still go to idle. Because the
modulemode of GPIO is set to HW_AUTO in this case, which won't
prevent the power domain goes to retention. So this patch doesn't
make the situation worse.
> - If we want to go on this path then I would like to see we address
> omap2_gpio_[prepare/resumne]_for_idle()
>
I did a quick google search but didn't find the problem with the two
functions. Could you give me a pointer here?
> - GPIO though sound simple is one of the most notorious IP block
> on PM front so this needs lot of testing. This patch not seems be
> tested at all so I would have much liked it to be in RFC/RFT
> state.
I tested using a gpio pin as interrupt source after applying the patch.
What are the other tests that I should do? Is there a formal way of
reporting test results here?
>
>>  drivers/gpio/gpio-omap.c                |  103 ++++++++++++++++++++++++++-----
>>  include/linux/platform_data/gpio-omap.h |    1 +
>>  2 files changed, 87 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index 89675f8..90661f2 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -76,6 +76,7 @@ struct gpio_bank {
>>       int context_loss_count;
>>       int power_mode;
>>       bool workaround_enabled;
>> +     bool is_aggressive_pm;
>>
>>       void (*set_dataout)(struct gpio_bank *bank, int gpio, int enable);
>>       int (*get_context_loss_count)(struct device *dev);
>> @@ -473,8 +474,15 @@ static void _disable_gpio_module(struct gpio_bank *bank, unsigned offset)
>>  static int gpio_is_input(struct gpio_bank *bank, int mask)
>>  {
>>       void __iomem *reg = bank->base + bank->regs->direction;
>> +     u32 val;
>>
>> -     return __raw_readl(reg) & mask;
>> +     if (bank->is_aggressive_pm)
>> +             pm_runtime_get_sync(bank->dev);
>> +     val = __raw_readl(reg) & mask;
>> +     if (bank->is_aggressive_pm)
>> +             pm_runtime_put(bank->dev);
>> +
> Move above in some wrapper instead of sprinkling the
> 'is_aggressive_pm' check everywhere.
>
Will do.
>> @@ -1585,18 +1651,21 @@ static const struct omap_gpio_platform_data omap2_pdata = {
>>       .regs = &omap2_gpio_regs,
>>       .bank_width = 32,
>>       .dbck_flag = false,
>> +     .is_aggressive_pm = false,
>>  };
>>
>>  static const struct omap_gpio_platform_data omap3_pdata = {
>>       .regs = &omap2_gpio_regs,
>>       .bank_width = 32,
>>       .dbck_flag = true,
>> +     .is_aggressive_pm = false,
>>  };
>>
>>  static const struct omap_gpio_platform_data omap4_pdata = {
>>       .regs = &omap4_gpio_regs,
>>       .bank_width = 32,
>>       .dbck_flag = true,
>> +     .is_aggressive_pm = true,
>>  };
>>
> Well OMAP IP shouldn't have different behavior on OMAP3 and
> OMAP4 at least so something you can enable for OMAP4, you
> should be able to do it for OMAP3 as well.
>
the am33xx_gpio_sysc.idlemodes is marked as capable of
SMART_WKUP in omap_hwmod_33xx.c. But according to TRM,
only gpio0 is capable of this. So i suspect the patch won't work
for omap3. I don't have the hardware to verify this. Could someone
verify it? Thank you.
> Kevin might have some more concerns to add.
>
> Regards,
> Santosh
>



-- 
Regards,
Chao Xu

[RFC/RFT/PATCH V3] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Chao Xu]

From: Felipe Balbi <balbi@ti.com>

try to keep gpio block suspended as much as possible.

Tested with pandaboard and a sysfs exported gpio.

Signed-off-by: Felipe Balbi <balbi at ti.com>

[caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added
revision check to enable aggressive pm_runtime on OMAP4-only. Because
am33xx_gpio_sysc.idlemodes seems to be wrongly marked as
SIDLE_SMART_WKUP, which might cause missed interrupts with this patch.
Tested on Pandaboard rev A2.]
Signed-off-by: Chao Xu <caesarxuchao@gmail.com>
---
changes since v2:
*add wrapper function to avoid 'is_aggressive_pm' check everywhere, as
suggested by Santosh Shilimkar 
*fix format issue in commit log

 drivers/gpio/gpio-omap.c                |   90 +++++++++++++++++++++++++------
 include/linux/platform_data/gpio-omap.h |    1 +
 2 files changed, 74 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 89675f8..fc5318b 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -76,6 +76,7 @@ struct gpio_bank {
 	int context_loss_count;
 	int power_mode;
 	bool workaround_enabled;
+	bool is_aggressive_pm;
 
 	void (*set_dataout)(struct gpio_bank *bank, int gpio, int enable);
 	int (*get_context_loss_count)(struct device *dev);
@@ -90,6 +91,18 @@ struct gpio_bank {
 #define BANK_USED(bank) (bank->mod_usage || bank->irq_usage)
 #define LINE_USED(line, offset) (line & (1 << offset))
 
+static void _aggressive_pm_runtime_get_sync(struct gpio_bank *bank)
+{
+	if (bank->is_aggressive_pm)
+		pm_runtime_get_sync(bank->dev);
+}
+
+static void _aggressive_pm_runtime_put(struct gpio_bank *bank)
+{
+	if (bank->is_aggressive_pm)
+		pm_runtime_put(bank->dev);
+}
+
 static int irq_to_gpio(struct gpio_bank *bank, unsigned int gpio_irq)
 {
 	return bank->chip.base + gpio_irq;
@@ -473,8 +486,13 @@ static void _disable_gpio_module(struct gpio_bank *bank, unsigned offset)
 static int gpio_is_input(struct gpio_bank *bank, int mask)
 {
 	void __iomem *reg = bank->base + bank->regs->direction;
+	u32 val;
 
-	return __raw_readl(reg) & mask;
+	_aggressive_pm_runtime_get_sync(bank);
+	val = __raw_readl(reg) & mask;
+	_aggressive_pm_runtime_put(bank);
+
+	return val;
 }
 
 static int gpio_irq_type(struct irq_data *d, unsigned type)
@@ -485,7 +503,7 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 	unsigned long flags;
 	unsigned offset;
 
-	if (!BANK_USED(bank))
+	if (!BANK_USED(bank) && !bank->is_aggressive_pm)
 		pm_runtime_get_sync(bank->dev);
 
 #ifdef CONFIG_ARCH_OMAP1
@@ -503,6 +521,7 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 		(type & (IRQ_TYPE_LEVEL_LOW|IRQ_TYPE_LEVEL_HIGH)))
 		return -EINVAL;
 
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 	offset = GPIO_INDEX(bank, gpio);
 	retval = _set_gpio_triggering(bank, offset, type);
@@ -511,11 +530,13 @@ static int gpio_irq_type(struct irq_data *d, unsigned type)
 		_set_gpio_direction(bank, offset, 1);
 	} else if (!gpio_is_input(bank, 1 << offset)) {
 		spin_unlock_irqrestore(&bank->lock, flags);
+		_aggressive_pm_runtime_put(bank);
 		return -EINVAL;
 	}
 
 	bank->irq_usage |= 1 << GPIO_INDEX(bank, gpio);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	_aggressive_pm_runtime_put(bank);
 
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
 		__irq_set_handler_locked(d->irq, handle_level_irq);
@@ -668,10 +689,11 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 	unsigned long flags;
 
 	/*
-	 * If this is the first gpio_request for the bank,
-	 * enable the bank module.
+	 * if aggressive runtime pm is supported, enable the bank module
+	 * for each gpio_request. Otherwise enable the bank module if this
+	 * is the first gpio_request for the bank.
 	 */
-	if (!BANK_USED(bank))
+	if (bank->is_aggressive_pm || !BANK_USED(bank))
 		pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
@@ -685,7 +707,7 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 	}
 	bank->mod_usage |= 1 << offset;
 	spin_unlock_irqrestore(&bank->lock, flags);
-
+	_aggressive_pm_runtime_put(bank);
 	return 0;
 }
 
@@ -694,6 +716,8 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 	struct gpio_bank *bank = container_of(chip, struct gpio_bank, chip);
 	unsigned long flags;
 
+	_aggressive_pm_runtime_get_sync(bank);
+
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->mod_usage &= ~(1 << offset);
 	_disable_gpio_module(bank, offset);
@@ -701,10 +725,11 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	/*
-	 * If this is the last gpio to be freed in the bank,
-	 * disable the bank module.
+	 * if aggressive runtime pm is supported, disable the bank module
+	 * for each gpio_free. Otherwise disable the bank module if this
+	 * is the last gpio to be freed in the bank.
 	 */
-	if (!BANK_USED(bank))
+	if (bank->is_aggressive_pm || !BANK_USED(bank))
 		pm_runtime_put(bank->dev);
 }
 
@@ -796,17 +821,18 @@ static void gpio_irq_shutdown(struct irq_data *d)
 	unsigned long flags;
 	unsigned offset = GPIO_INDEX(bank, gpio);
 
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->irq_usage &= ~(1 << offset);
 	_disable_gpio_module(bank, offset);
 	_reset_gpio(bank, gpio);
 	spin_unlock_irqrestore(&bank->lock, flags);
-
 	/*
-	 * If this is the last IRQ to be freed in the bank,
-	 * disable the bank module.
+	 * if aggressive runtime pm is supported, disable the bank module
+	 * for each irq_shutdown. Otherwise disable the bank module if this
+	 * is the last IRQ to be freed in the bank.
 	 */
-	if (!BANK_USED(bank))
+	if (bank->is_aggressive_pm || !BANK_USED(bank))
 		pm_runtime_put(bank->dev);
 }
 
@@ -815,7 +841,9 @@ static void gpio_ack_irq(struct irq_data *d)
 	struct gpio_bank *bank = irq_data_get_irq_chip_data(d);
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 
+	_aggressive_pm_runtime_get_sync(bank);
 	_clear_gpio_irqstatus(bank, gpio);
+	_aggressive_pm_runtime_put(bank);
 }
 
 static void gpio_mask_irq(struct irq_data *d)
@@ -824,10 +852,12 @@ static void gpio_mask_irq(struct irq_data *d)
 	unsigned int gpio = irq_to_gpio(bank, d->hwirq);
 	unsigned long flags;
 
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_irqenable(bank, gpio, 0);
 	_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), IRQ_TYPE_NONE);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	_aggressive_pm_runtime_put(bank);
 }
 
 static void gpio_unmask_irq(struct irq_data *d)
@@ -838,6 +868,7 @@ static void gpio_unmask_irq(struct irq_data *d)
 	u32 trigger = irqd_get_trigger_type(d);
 	unsigned long flags;
 
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 	if (trigger)
 		_set_gpio_triggering(bank, GPIO_INDEX(bank, gpio), trigger);
@@ -851,6 +882,7 @@ static void gpio_unmask_irq(struct irq_data *d)
 
 	_set_gpio_irqenable(bank, gpio, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	_aggressive_pm_runtime_put(bank);
 }
 
 static struct irq_chip gpio_irq_chip = {
@@ -933,9 +965,13 @@ static int gpio_input(struct gpio_chip *chip, unsigned offset)
 	unsigned long flags;
 
 	bank = container_of(chip, struct gpio_bank, chip);
+
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_direction(bank, offset, 1);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	_aggressive_pm_runtime_put(bank);
+
 	return 0;
 }
 
@@ -944,13 +980,18 @@ static int gpio_get(struct gpio_chip *chip, unsigned offset)
 	struct gpio_bank *bank;
 	u32 mask;
 
+	int val;
 	bank = container_of(chip, struct gpio_bank, chip);
 	mask = (1 << offset);
 
+	_aggressive_pm_runtime_get_sync(bank);
 	if (gpio_is_input(bank, mask))
-		return _get_gpio_datain(bank, offset);
+		val = _get_gpio_datain(bank, offset);
 	else
-		return _get_gpio_dataout(bank, offset);
+		val = _get_gpio_dataout(bank, offset);
+	_aggressive_pm_runtime_put(bank);
+
+	return val;
 }
 
 static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
@@ -960,6 +1001,8 @@ static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
 	int retval = 0;
 
 	bank = container_of(chip, struct gpio_bank, chip);
+
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 
 	if (LINE_USED(bank->irq_usage, offset)) {
@@ -972,6 +1015,8 @@ static int gpio_output(struct gpio_chip *chip, unsigned offset, int value)
 
 exit:
 	spin_unlock_irqrestore(&bank->lock, flags);
+	_aggressive_pm_runtime_put(bank);
+
 	return retval;
 }
 
@@ -983,9 +1028,11 @@ static int gpio_debounce(struct gpio_chip *chip, unsigned offset,
 
 	bank = container_of(chip, struct gpio_bank, chip);
 
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 	_set_gpio_debounce(bank, offset, debounce);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	_aggressive_pm_runtime_put(bank);
 
 	return 0;
 }
@@ -996,9 +1043,12 @@ static void gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	unsigned long flags;
 
 	bank = container_of(chip, struct gpio_bank, chip);
+
+	_aggressive_pm_runtime_get_sync(bank);
 	spin_lock_irqsave(&bank->lock, flags);
 	bank->set_dataout(bank, offset, value);
 	spin_unlock_irqrestore(&bank->lock, flags);
+	_aggressive_pm_runtime_put(bank);
 }
 
 /*---------------------------------------------------------------------*/
@@ -1168,6 +1218,7 @@ static int omap_gpio_probe(struct platform_device *pdev)
 	bank->is_mpuio = pdata->is_mpuio;
 	bank->non_wakeup_gpios = pdata->non_wakeup_gpios;
 	bank->regs = pdata->regs;
+	bank->is_aggressive_pm = pdata->is_aggressive_pm;
 #ifdef CONFIG_OF_GPIO
 	bank->chip.of_node = of_node_get(node);
 #endif
@@ -1449,7 +1500,8 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
 
 		bank->power_mode = pwr_mode;
 
-		pm_runtime_put_sync_suspend(bank->dev);
+		if (!pm_runtime_suspended(bank->dev))
+			pm_runtime_suspend(bank->dev);
 	}
 }
 
@@ -1461,7 +1513,8 @@ void omap2_gpio_resume_after_idle(void)
 		if (!BANK_USED(bank) || !bank->loses_context)
 			continue;
 
-		pm_runtime_get_sync(bank->dev);
+		if (pm_runtime_suspended(bank->dev))
+			pm_runtime_resume(bank->dev);
 	}
 }
 
@@ -1585,18 +1638,21 @@ static const struct omap_gpio_platform_data omap2_pdata = {
 	.regs = &omap2_gpio_regs,
 	.bank_width = 32,
 	.dbck_flag = false,
+	.is_aggressive_pm = false,
 };
 
 static const struct omap_gpio_platform_data omap3_pdata = {
 	.regs = &omap2_gpio_regs,
 	.bank_width = 32,
 	.dbck_flag = true,
+	.is_aggressive_pm = false,
 };
 
 static const struct omap_gpio_platform_data omap4_pdata = {
 	.regs = &omap4_gpio_regs,
 	.bank_width = 32,
 	.dbck_flag = true,
+	.is_aggressive_pm = true,
 };
 
 static const struct of_device_id omap_gpio_match[] = {
diff --git a/include/linux/platform_data/gpio-omap.h b/include/linux/platform_data/gpio-omap.h
index 5d50b25..bb033b1 100644
--- a/include/linux/platform_data/gpio-omap.h
+++ b/include/linux/platform_data/gpio-omap.h
@@ -200,6 +200,7 @@ struct omap_gpio_platform_data {
 	bool dbck_flag;		/* dbck required or not - True for OMAP3&4 */
 	bool loses_context;	/* whether the bank would ever lose context */
 	bool is_mpuio;		/* whether the bank is of type MPUIO */
+	bool is_aggressive_pm; /* whether aggressive runtime pm is supported*/
 	u32 non_wakeup_gpios;
 
 	struct omap_gpio_reg_offs *regs;
-- 
1.7.9.5

Re: [RFC/RFT/PATCH V3] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Linus Walleij]

On Sun, Dec 8, 2013 at 5:40 AM, Chao Xu <caesarxuchao@gmail.com> wrote:

> From: Felipe Balbi <balbi@ti.com>
>
> try to keep gpio block suspended as much as possible.
>
> Tested with pandaboard and a sysfs exported gpio.
>
> Signed-off-by: Felipe Balbi <balbi at ti.com>
>
> [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added
> revision check to enable aggressive pm_runtime on OMAP4-only. Because
> am33xx_gpio_sysc.idlemodes seems to be wrongly marked as
> SIDLE_SMART_WKUP, which might cause missed interrupts with this patch.
> Tested on Pandaboard rev A2.]
> Signed-off-by: Chao Xu <caesarxuchao@gmail.com>

Kevin/Santosh: you're maintainers for this driver, can you ACK/NACK
this patch?

Yours,
Linus Walleij

Re: [RFC/RFT/PATCH V3] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Kevin Hilman]

Chao Xu <caesarxuchao@gmail.com> writes:

> From: Felipe Balbi <balbi@ti.com>
>
> try to keep gpio block suspended as much as possible.

> Tested with pandaboard and a sysfs exported gpio.
>
> Signed-off-by: Felipe Balbi <balbi at ti.com>
>
> [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added
> revision check to enable aggressive pm_runtime on OMAP4-only. Because
> am33xx_gpio_sysc.idlemodes seems to be wrongly marked as
> SIDLE_SMART_WKUP, which might cause missed interrupts with this patch.
> Tested on Pandaboard rev A2.]
> Signed-off-by: Chao Xu <caesarxuchao@gmail.com>

I have several problems with this patch.

First, the changelog is missing a lot of information.  In particular, nowhere
is it described what problem is this patch addressing  and how this
patch addresses that problem.

Second, I don't see any mention of off-mode, and I suspect there are
issues with off-mode here that are not being addressed.

Also, I *really* don't like any approach that is targetted at a single
SoC.

Kevin

Re: [RFC/RFT/PATCH V3] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Linus Walleij]

On Sun, Dec 8, 2013 at 5:40 AM, Chao Xu <caesarxuchao@gmail.com> wrote:

> From: Felipe Balbi <balbi@ti.com>
>
> try to keep gpio block suspended as much as possible.
>
> Tested with pandaboard and a sysfs exported gpio.
>
> Signed-off-by: Felipe Balbi <balbi at ti.com>
>
> [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added
> revision check to enable aggressive pm_runtime on OMAP4-only. Because
> am33xx_gpio_sysc.idlemodes seems to be wrongly marked as
> SIDLE_SMART_WKUP, which might cause missed interrupts with this patch.
> Tested on Pandaboard rev A2.]
> Signed-off-by: Chao Xu <caesarxuchao@gmail.com>
> ---
> changes since v2:
> *add wrapper function to avoid 'is_aggressive_pm' check everywhere, as
> suggested by Santosh Shilimkar
> *fix format issue in commit log

I'm dropping this until you convince Kevin/Tony that this is
the way to go.

One thing caught my eye, you add:

> +static void _aggressive_pm_runtime_get_sync(struct gpio_bank *bank)
> +static void _aggressive_pm_runtime_put(struct gpio_bank *bank)
(..)

Then everywhere:

> +       _aggressive_pm_runtime_get_sync(bank);
(...)
> +       _aggressive_pm_runtime_put(bank);

Aggressive, argh, runtime PM is agressive by definition. If you
want to switch this on and off use the compile option
to enable/disable runtime PM altogether and do not wrap it
like this.

Yours,
Linus Walleij

Re: [RFC/RFT/PATCH V3] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

On Thu, Dec 12, 2013 at 07:19:35PM +0100, Linus Walleij wrote:
> On Sun, Dec 8, 2013 at 5:40 AM, Chao Xu <caesarxuchao@gmail.com> wrote:
> 
> > From: Felipe Balbi <balbi@ti.com>
> >
> > try to keep gpio block suspended as much as possible.
> >
> > Tested with pandaboard and a sysfs exported gpio.
> >
> > Signed-off-by: Felipe Balbi <balbi at ti.com>
> >
> > [caesarxuchao@gmail.com : Refreshed against v3.12-rc5, and added
> > revision check to enable aggressive pm_runtime on OMAP4-only. Because
> > am33xx_gpio_sysc.idlemodes seems to be wrongly marked as
> > SIDLE_SMART_WKUP, which might cause missed interrupts with this patch.
> > Tested on Pandaboard rev A2.]
> > Signed-off-by: Chao Xu <caesarxuchao@gmail.com>
> > ---
> > changes since v2:
> > *add wrapper function to avoid 'is_aggressive_pm' check everywhere, as
> > suggested by Santosh Shilimkar
> > *fix format issue in commit log
> 
> I'm dropping this until you convince Kevin/Tony that this is
> the way to go.
> 
> One thing caught my eye, you add:
> 
> > +static void _aggressive_pm_runtime_get_sync(struct gpio_bank *bank)
> > +static void _aggressive_pm_runtime_put(struct gpio_bank *bank)
> (..)
> 
> Then everywhere:
> 
> > +       _aggressive_pm_runtime_get_sync(bank);
> (...)
> > +       _aggressive_pm_runtime_put(bank);
> 
> Aggressive, argh, runtime PM is agressive by definition. If you
> want to switch this on and off use the compile option
> to enable/disable runtime PM altogether and do not wrap it
> like this.

heh, OMAP doesn't work without pm_runtime.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC/RFT/PATCH V3] gpio: omap: refresh patch "be more aggressive with pm_runtime" against v3.12-rc5

[Linus Walleij]

On Thu, Dec 12, 2013 at 7:28 PM, Felipe Balbi <balbi@ti.com> wrote:
> On Thu, Dec 12, 2013 at 07:19:35PM +0100, Linus Walleij wrote:

>> One thing caught my eye, you add:
>>
>> > +static void _aggressive_pm_runtime_get_sync(struct gpio_bank *bank)
>> > +static void _aggressive_pm_runtime_put(struct gpio_bank *bank)
>> (..)
>>
>> Then everywhere:
>>
>> > +       _aggressive_pm_runtime_get_sync(bank);
>> (...)
>> > +       _aggressive_pm_runtime_put(bank);
>>
>> Aggressive, argh, runtime PM is agressive by definition. If you
>> want to switch this on and off use the compile option
>> to enable/disable runtime PM altogether and do not wrap it
>> like this.
>
> heh, OMAP doesn't work without pm_runtime.

Hm then maybe that needs to be fixed ... or the runtime PM
people need to be convinced to support different levels of
aggressiveness in the core?

Yours,
Linus Walleij