Re: [PATCH] gpio: omap-gpio: add support for pm_runtime autosuspend

[Santosh Shilimkar <santosh.shilimkar@ti.com>]

+ Jon,

On Friday 26 October 2012 06:49 PM, Tim Niemeyer wrote:
> Adds support for configuring the omap-gpio driver use autosuspend for
> runtime power management. This can reduce the latency in using it by
> not suspending the device immediately on idle. If another access takes
> place before the autosuspend timeout (2 secs), the call to resume the
> device can return immediately saving some save/ restore cycles.
>
> This removes also the bank->mod_usage counter, because this is already
> handled in pm_runtime.
>
> I use a gpio to monitor a spi transfer which occurs every 250µs. The
> suspend overhead is to high, so almost every second transfer is lost.
> This patch fixes that.
>
> Signed-off-by: Tim Niemeyer <tim.niemeyer@corscience.de>
> ---
>   drivers/gpio/gpio-omap.c |   81 ++++++++++++++++++++++++---------------------
>   1 files changed, 43 insertions(+), 38 deletions(-)
>
 From patch it appears your main motive is to delay the idle kicking in
with a timeout to avoid GPIO on cpuidle path. Some comments

> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 94cbc84..708d5a9 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -31,6 +31,7 @@
>   #include <asm/mach/irq.h>
>
>   #define OFF_MODE	1
> +#define GPIO_AUTOSUSPEND_TIMEOUT                2000
>
>   static LIST_HEAD(omap_gpio_list);
>
> @@ -64,7 +65,6 @@ struct gpio_bank {
>   	spinlock_t lock;
>   	struct gpio_chip chip;
>   	struct clk *dbck;
> -	u32 mod_usage;
How have you tested 'mod_suage' change ?

>   	u32 dbck_enable_mask;
>   	bool dbck_enabled;
>   	struct device *dev;
> @@ -557,10 +557,9 @@ 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.
> +	 * resume the bank module.
Since you removing bank notion, "If this is the first gpio_request
for the bank," becomes irrelevant from code perspective.

[..]

> @@ -608,28 +594,15 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
>   			__raw_readl(bank->base + bank->regs->wkup_en);
>   	}
>
> -	bank->mod_usage &= ~(1 << offset);
> -
> -	if (bank->regs->ctrl && !bank->mod_usage) {
> -		void __iomem *reg = bank->base + bank->regs->ctrl;
> -		u32 ctrl;
> -
> -		ctrl = __raw_readl(reg);
> -		/* Module is disabled, clocks are gated */
> -		ctrl |= GPIO_MOD_CTRL_BIT;
> -		__raw_writel(ctrl, reg);
> -		bank->context.ctrl = ctrl;
> -	}
> -
>   	_reset_gpio(bank, bank->chip.base + offset);
>   	spin_unlock_irqrestore(&bank->lock, flags);
>
>   	/*
>   	 * If this is the last gpio to be freed in the bank,
> -	 * disable the bank module.
> +	 * put the bank module into suspend.
>   	 */
> -	if (!bank->mod_usage)
> -		pm_runtime_put(bank->dev);
> +	pm_runtime_mark_last_busy(bank->dev);
> +	pm_runtime_put_autosuspend(bank->dev);
Waiting for 2 seconds timeout even on GPIO free
seems to be wrong.

>   }
>
>   /*
> @@ -715,7 +688,8 @@ static void gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
>   exit:
>   	if (!unmasked)
>   		chained_irq_exit(chip, desc);
> -	pm_runtime_put(bank->dev);
> +	pm_runtime_mark_last_busy(bank->dev);
> +	pm_runtime_put_autosuspend(bank->dev);
This is what is the main change from this patch which helps your
case.
>   }
>
>   static void gpio_irq_shutdown(struct irq_data *d)
> @@ -1132,6 +1106,8 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>
>   	platform_set_drvdata(pdev, bank);
>
> +	pm_runtime_use_autosuspend(bank->dev);
> +	pm_runtime_set_autosuspend_delay(bank->dev, GPIO_AUTOSUSPEND_TIMEOUT);

Can you please report how have you tested this change ? What other PM
tests you have done?

Removing mod usage might just break this driver because now individual
banks which can idle before, can no longer idle.

Just to expand a bit, Out of 6 GPIO banks, GPIO1 bank is in always ON
domain where as remaing 5 are in peripheral domain. Letting individual
banks idle allowed you let the clock domain idle than keeping all the
6 banks and hence respective clock/power domain in ON state.

So the adding timeout might be reasonable but I am not sure about
the mod_usage change here.

Jon, What you say ?



--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html