Am Montag, den 29.10.2012, 12:17 +0530 schrieb Santosh Shilimkar:
> + 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@xxxxxxxxxxxxx>
> > ---
> > 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
cpuidle? I set 'CPU_IDLE=n' in my .config..
> > 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 ?
Nothing special, just applied the patch to a 3.4.14 kernel and used one
gpio to show the status of my spi communication.
> > 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.
Hu, i thought pm_runtime_get_sync(bank->dev) handles the use counting
per bank?
> > @@ -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.
Yes, you are right, if something frees the gpio it should suspend
immediately.
> > }
> >
> > /*
> > @@ -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?
My main goal isn't to have a good power-saving, i need a small latency.
The runtime-pm disturbed that. I tested to disable CONFIG_PM_RUNTIME,
but than the gptimer couldn't be initialized.
My scenario: gptimer triggers every 250µs and starts an async_spi
transfer while it sets one gpio line to high. The spi_complete then puts
this gpio to low again.
Every second interrupt was lost and i wanted to know why so i removed
the spi stuff and just turned a gpio on and off in the timer-isr. It
turned out that the system wasn't able to tick with 4kHz. I then
disabled the CONFIG_PM_RUNTIME and fiddled the gptimer to start without
runtime-pm. Then i was able to use gpio with up to 100kHz. I enabled
CONFIG_PM_RUNTIME again and disabled the gpio runtime_pm via sysfs (echo
on > /sys/...) which got me same results.
I have to admit, i didn't completely understand all of this.. :( It's
even possible that my testresult is only a nice side effect of this
patch.
> 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.
I first had also some problems with the mod_usage change, but after
asking Felipe again, i thought i understood it. Maybe Felipe can clear
this up as this was his idea?
--
Tim Niemeyer
Corscience GmbH & Co. KG
Henkestr. 91
D-91052 Erlangen
Germany
e-mail: tim.niemeyer@xxxxxxxxxxxxx
Internet: www.corscience.de
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
