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

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

[Tim Niemeyer]

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.

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 |   23 ++++++++++++++++++-----
 1 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 94cbc84..92f48cb 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);
 
@@ -628,8 +629,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)
-		pm_runtime_put(bank->dev);
+	if (!bank->mod_usage) {
+		pm_runtime_mark_last_busy(bank->dev);
+		pm_runtime_put_autosuspend(bank->dev);
+	}
 }
 
 /*
@@ -715,7 +718,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);
 }
 
 static void gpio_irq_shutdown(struct irq_data *d)
@@ -1132,6 +1136,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);
 	pm_runtime_enable(bank->dev);
 	pm_runtime_irq_safe(bank->dev);
 	pm_runtime_get_sync(bank->dev);
@@ -1146,7 +1152,8 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	if (bank->loses_context)
 		bank->get_context_loss_count = pdata->get_context_loss_count;
 
-	pm_runtime_put(bank->dev);
+	pm_runtime_mark_last_busy(bank->dev);
+	pm_runtime_put_autosuspend(bank->dev);
 
 	list_add_tail(&bank->node, &omap_gpio_list);
 
@@ -1333,7 +1340,13 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
 
 		bank->power_mode = pwr_mode;
 
-		pm_runtime_put_sync_suspend(bank->dev);
+		/* direct pm_runtime on pwroff */
+		if (pwr_mode)
+			pm_runtime_put_sync_suspend(bank->dev);
+		else {
+			pm_runtime_mark_last_busy(bank->dev);
+			pm_runtime_put_sync_autosuspend(bank->dev);
+		}
 	}
 }
 
-- 
1.7.2.5

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

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

[Felipe Balbi]

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

Hi,

On Fri, Oct 26, 2012 at 09:55:30AM +0200, 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.
> 
> 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 |   23 ++++++++++++++++++-----
>  1 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 94cbc84..92f48cb 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);
>  
> @@ -628,8 +629,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)
> -		pm_runtime_put(bank->dev);
> +	if (!bank->mod_usage) {

while at that I would drop this bank->mod_usage nonsense and let
power core handle reference counting.

> +		pm_runtime_mark_last_busy(bank->dev);
> +		pm_runtime_put_autosuspend(bank->dev);
> +	}
>  }
>  
>  /*
> @@ -715,7 +718,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);
>  }
>  
>  static void gpio_irq_shutdown(struct irq_data *d)
> @@ -1132,6 +1136,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);
>  	pm_runtime_enable(bank->dev);
>  	pm_runtime_irq_safe(bank->dev);
>  	pm_runtime_get_sync(bank->dev);
> @@ -1146,7 +1152,8 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>  	if (bank->loses_context)
>  		bank->get_context_loss_count = pdata->get_context_loss_count;
>  
> -	pm_runtime_put(bank->dev);
> +	pm_runtime_mark_last_busy(bank->dev);
> +	pm_runtime_put_autosuspend(bank->dev);
>  
>  	list_add_tail(&bank->node, &omap_gpio_list);
>  
> @@ -1333,7 +1340,13 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
>  
>  		bank->power_mode = pwr_mode;
>  
> -		pm_runtime_put_sync_suspend(bank->dev);
> +		/* direct pm_runtime on pwroff */
> +		if (pwr_mode)

you also need braces here.

> +			pm_runtime_put_sync_suspend(bank->dev);
> +		else {
> +			pm_runtime_mark_last_busy(bank->dev);
> +			pm_runtime_put_sync_autosuspend(bank->dev);
> +		}
>  	}
>  }
>  
> -- 
> 1.7.2.5
> 
> --
> 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

-- 
balbi

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

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

[Tim Niemeyer]

Am Freitag, den 26.10.2012, 11:03 +0300 schrieb Felipe Balbi:
> Hi,
> 
> On Fri, Oct 26, 2012 at 09:55:30AM +0200, 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.
> > 
> > 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 |   23 ++++++++++++++++++-----
> >  1 files changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index 94cbc84..92f48cb 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);
> >  
> > @@ -628,8 +629,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)
> > -		pm_runtime_put(bank->dev);
> > +	if (!bank->mod_usage) {
> 
> while at that I would drop this bank->mod_usage nonsense and let
> power core handle reference counting.
I looked at it, but i'm unsure about the GPIO_MOD_CTRL_BIT. The
bank->mod_usage counter prevents omap_gpio_free() to disable the hole
bank while another gpio is still in use.

> > +		pm_runtime_mark_last_busy(bank->dev);
> > +		pm_runtime_put_autosuspend(bank->dev);
> > +	}
> >  }
> >  
> >  /*
> > @@ -715,7 +718,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);
> >  }
> >  
> >  static void gpio_irq_shutdown(struct irq_data *d)
> > @@ -1132,6 +1136,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);
> >  	pm_runtime_enable(bank->dev);
> >  	pm_runtime_irq_safe(bank->dev);
> >  	pm_runtime_get_sync(bank->dev);
> > @@ -1146,7 +1152,8 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
> >  	if (bank->loses_context)
> >  		bank->get_context_loss_count = pdata->get_context_loss_count;
> >  
> > -	pm_runtime_put(bank->dev);
> > +	pm_runtime_mark_last_busy(bank->dev);
> > +	pm_runtime_put_autosuspend(bank->dev);
> >  
> >  	list_add_tail(&bank->node, &omap_gpio_list);
> >  
> > @@ -1333,7 +1340,13 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
> >  
> >  		bank->power_mode = pwr_mode;
> >  
> > -		pm_runtime_put_sync_suspend(bank->dev);
> > +		/* direct pm_runtime on pwroff */
> > +		if (pwr_mode)
> 
> you also need braces here.
Yes. I resend this later.

> > +			pm_runtime_put_sync_suspend(bank->dev);
> > +		else {
> > +			pm_runtime_mark_last_busy(bank->dev);
> > +			pm_runtime_put_sync_autosuspend(bank->dev);
> > +		}
> >  	}
> >  }
> >  
> > -- 
> > 1.7.2.5
> > 
> > --
> > 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
> 

-- 
Tim Niemeyer

Corscience GmbH & Co. KG
Henkestr. 91
D-91052 Erlangen
Germany

e-mail: tim.niemeyer@corscience.de
Internet: www.corscience.de

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

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

[Felipe Balbi]

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

On Fri, Oct 26, 2012 at 12:42:35PM +0200, Tim Niemeyer wrote:
> Am Freitag, den 26.10.2012, 11:03 +0300 schrieb Felipe Balbi:
> > Hi,
> > 
> > On Fri, Oct 26, 2012 at 09:55:30AM +0200, 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.
> > > 
> > > 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 |   23 ++++++++++++++++++-----
> > >  1 files changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > > index 94cbc84..92f48cb 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);
> > >  
> > > @@ -628,8 +629,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)
> > > -		pm_runtime_put(bank->dev);
> > > +	if (!bank->mod_usage) {
> > 
> > while at that I would drop this bank->mod_usage nonsense and let
> > power core handle reference counting.
> I looked at it, but i'm unsure about the GPIO_MOD_CTRL_BIT. The
> bank->mod_usage counter prevents omap_gpio_free() to disable the hole
> bank while another gpio is still in use.

pm core's usage count will do the same thing. If you request 6 gpios,
you will have 6 pm_runtime_get(). If you free 5 of those, you will have
5 pm_runtime_put_autosuspend() which will only decrement the counter and
do nothing else.

-- 
balbi

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

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

[Tim Niemeyer]

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

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;
 	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.
 	 */
-	if (!bank->mod_usage)
-		pm_runtime_get_sync(bank->dev);
+	pm_runtime_get_sync(bank->dev);
 
 	spin_lock_irqsave(&bank->lock, flags);
 	/* Set trigger to none. You need to enable the desired trigger with
@@ -575,19 +574,6 @@ static int omap_gpio_request(struct gpio_chip *chip, unsigned offset)
 		__raw_writel(__raw_readl(reg) | (1 << offset), reg);
 	}
 
-	if (bank->regs->ctrl && !bank->mod_usage) {
-		void __iomem *reg = bank->base + bank->regs->ctrl;
-		u32 ctrl;
-
-		ctrl = __raw_readl(reg);
-		/* Module is enabled, clocks are not gated */
-		ctrl &= ~GPIO_MOD_CTRL_BIT;
-		__raw_writel(ctrl, reg);
-		bank->context.ctrl = ctrl;
-	}
-
-	bank->mod_usage |= 1 << offset;
-
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -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);
 }
 
 /*
@@ -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);
 }
 
 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);
 	pm_runtime_enable(bank->dev);
 	pm_runtime_irq_safe(bank->dev);
 	pm_runtime_get_sync(bank->dev);
@@ -1146,7 +1122,8 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
 	if (bank->loses_context)
 		bank->get_context_loss_count = pdata->get_context_loss_count;
 
-	pm_runtime_put(bank->dev);
+	pm_runtime_mark_last_busy(bank->dev);
+	pm_runtime_put_autosuspend(bank->dev);
 
 	list_add_tail(&bank->node, &omap_gpio_list);
 
@@ -1168,6 +1145,17 @@ static int omap_gpio_runtime_suspend(struct device *dev)
 
 	spin_lock_irqsave(&bank->lock, flags);
 
+	if (bank->regs->ctrl) {
+		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;
+	}
+
 	/*
 	 * Only edges can generate a wakeup event to the PRCM.
 	 *
@@ -1316,6 +1304,17 @@ static int omap_gpio_runtime_resume(struct device *dev)
 		__raw_writel(old1, bank->base + bank->regs->leveldetect1);
 	}
 
+	if (bank->regs->ctrl) {
+		void __iomem *reg = bank->base + bank->regs->ctrl;
+		u32 ctrl;
+
+		ctrl = __raw_readl(reg);
+		/* Module is enabled, clocks are not gated */
+		ctrl &= ~GPIO_MOD_CTRL_BIT;
+		__raw_writel(ctrl, reg);
+		bank->context.ctrl = ctrl;
+	}
+
 	bank->workaround_enabled = false;
 	spin_unlock_irqrestore(&bank->lock, flags);
 
@@ -1328,12 +1327,18 @@ void omap2_gpio_prepare_for_idle(int pwr_mode)
 	struct gpio_bank *bank;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
-		if (!bank->mod_usage || !bank->loses_context)
+		if (!bank->loses_context)
 			continue;
 
 		bank->power_mode = pwr_mode;
 
-		pm_runtime_put_sync_suspend(bank->dev);
+		/* direct pm_runtime on pwroff */
+		if (pwr_mode) {
+			pm_runtime_put_sync_suspend(bank->dev);
+		} else {
+			pm_runtime_mark_last_busy(bank->dev);
+			pm_runtime_put_sync_autosuspend(bank->dev);
+		}
 	}
 }
 
@@ -1342,7 +1347,7 @@ void omap2_gpio_resume_after_idle(void)
 	struct gpio_bank *bank;
 
 	list_for_each_entry(bank, &omap_gpio_list, node) {
-		if (!bank->mod_usage || !bank->loses_context)
+		if (!bank->loses_context)
 			continue;
 
 		pm_runtime_get_sync(bank->dev);
-- 
1.7.2.5

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

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

[Felipe Balbi]

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

Hi,

On Fri, Oct 26, 2012 at 03:19:13PM +0200, 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(-)
> 
> 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

something just hit me... If you keep timeout at 2000 ms and you hook
this up to an IRQ line, it's very unlikely GPIO will ever sleep.

Why did you choose 2000 ms ? Arbitrary value ?

-- 
balbi

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

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

[Jon Hunter]

On 10/26/2012 03:01 PM, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Oct 26, 2012 at 03:19:13PM +0200, 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(-)
>>
>> 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
> 
> something just hit me... If you keep timeout at 2000 ms and you hook
> this up to an IRQ line, it's very unlikely GPIO will ever sleep.
> 
> Why did you choose 2000 ms ? Arbitrary value ?

It does seem quite large. I wonder if the default timeout should be
something much smaller and then users can set the timeout needed for
their specific application via the sysfs.

By the way, it appears that I keep getting unsubscribed from linux-omap
mailing list and I only saw this because you copied me. Thanks Felipe!
Can you bounce the thread to me?

Also if anyone knows why I could be getting unsubscribed let me know.
May be my emails have a bad signal to noise ratio :-(

Cheers
Jon



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

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

[Santosh Shilimkar]

On Saturday 27 October 2012 03:09 AM, Jon Hunter wrote:
>
> On 10/26/2012 03:01 PM, Felipe Balbi wrote:
>> Hi,
>>
>> On Fri, Oct 26, 2012 at 03:19:13PM +0200, 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(-)
>>>
>>> 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
>>
>> something just hit me... If you keep timeout at 2000 ms and you hook
>> this up to an IRQ line, it's very unlikely GPIO will ever sleep.
>>
>> Why did you choose 2000 ms ? Arbitrary value ?
>
> It does seem quite large. I wonder if the default timeout should be
> something much smaller and then users can set the timeout needed for
> their specific application via the sysfs.
>
> By the way, it appears that I keep getting unsubscribed from linux-omap
> mailing list and I only saw this because you copied me. Thanks Felipe!
> Can you bounce the thread to me?
>
I didn't get this email either though am getting other LOML emails.
Please bounce the thread.

Am curious to see the implementation, since GPIO IP as such doesn't have
any state machine of any sort.

Regards
Santosh
--
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

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

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

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

[Felipe Balbi]

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

On Mon, Oct 29, 2012 at 12:17:08PM +0530, Santosh Shilimkar wrote:
> + 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.

why's that ?

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

IMHO that whole mod_usage is broken. I remember sending a big series of
patches getting rid of that long ago. I _did_ break a few things but
just because of omap_gpio_prepare_for_idle() /
omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough.

I still think mod_usage needs to go, so does
omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it
looks like that needs to be done on ->prepare()/->complete() callbacks
of system suspend and the gpio driver needs to learn proper runtime
suspend.

The way it is today (get() on gpio_request() and put() on gpio_free())
is just wrong and non-optimal.

cheers

-- 
balbi

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

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

[Santosh Shilimkar]

On Monday 29 October 2012 01:35 PM, Felipe Balbi wrote:
> On Mon, Oct 29, 2012 at 12:17:08PM +0530, Santosh Shilimkar wrote:
>> + 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.
>
> why's that ?
>
>> 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.
>
> IMHO that whole mod_usage is broken. I remember sending a big series of
> patches getting rid of that long ago. I _did_ break a few things but
> just because of omap_gpio_prepare_for_idle() /
> omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough.
>
Well so far I haven't seen/come across a patch/proposal which fixes it.

> I still think mod_usage needs to go, so does
> omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it
> looks like that needs to be done on ->prepare()/->complete() callbacks
> of system suspend and the gpio driver needs to learn proper runtime
> suspend.
>
I am not saying it shouldn't go :-)
The $subject patch isn't fixing it correctly is what I said.

Don't get hung up on suspend case because thats the easiest
way to address it. The issue is with idle where GPIO can prevent
SOC idle if it isn't taken care. And since its just an IO, its not
easy to implement something like inactivity timer towards
autosupend.

Co-processor also makes use of GPIO via syslink proxy and thats
make things even harder.

Regards
Santosh


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

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

[Tim Niemeyer]

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@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
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@corscience.de
Internet: www.corscience.de

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

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

[Tim Niemeyer]

Hello

Am Freitag, den 26.10.2012, 23:01 +0300 schrieb Felipe Balbi:
> Hi,
> 
> On Fri, Oct 26, 2012 at 03:19:13PM +0200, 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(-)
> > 
> > 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
> 
> something just hit me... If you keep timeout at 2000 ms and you hook
> this up to an IRQ line, it's very unlikely GPIO will ever sleep.
> 
> Why did you choose 2000 ms ? Arbitrary value ?
Got it from the spi driver (27b5284cfbe187732ebb184b03ea693f44837f9d).
I have no problem with reducing this. As i don't know how much
power-overhead the suspend needs, what do you think is a good value in
terms of power-saving?

-- 
Tim Niemeyer

Corscience GmbH & Co. KG
Henkestr. 91
D-91052 Erlangen
Germany

e-mail: tim.niemeyer@corscience.de
Internet: www.corscience.de

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

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

[Felipe Balbi]

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

Hi,

On Mon, Oct 29, 2012 at 01:53:37PM +0530, Santosh Shilimkar wrote:
> >>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.
> >
> >IMHO that whole mod_usage is broken. I remember sending a big series of
> >patches getting rid of that long ago. I _did_ break a few things but
> >just because of omap_gpio_prepare_for_idle() /
> >omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough.
> >
> Well so far I haven't seen/come across a patch/proposal which fixes it.

fair point

> >I still think mod_usage needs to go, so does
> >omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it
> >looks like that needs to be done on ->prepare()/->complete() callbacks
> >of system suspend and the gpio driver needs to learn proper runtime
> >suspend.
> >
> I am not saying it shouldn't go :-)
> The $subject patch isn't fixing it correctly is what I said.
> 
> Don't get hung up on suspend case because thats the easiest
> way to address it. The issue is with idle where GPIO can prevent
> SOC idle if it isn't taken care. And since its just an IO, its not
> easy to implement something like inactivity timer towards
> autosupend.

I don't see the relation here. Care to expand a bit ?

> Co-processor also makes use of GPIO via syslink proxy and thats
> make things even harder.

that's supposed to be solved with hwspinlock, isn't it ?

cheers

-- 
balbi

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

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

[Santosh Shilimkar]

On Tuesday 30 October 2012 01:33 AM, Felipe Balbi wrote:
> Hi,
>
> On Mon, Oct 29, 2012 at 01:53:37PM +0530, Santosh Shilimkar wrote:
>>>> 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.
>>>
>>> IMHO that whole mod_usage is broken. I remember sending a big series of
>>> patches getting rid of that long ago. I _did_ break a few things but
>>> just because of omap_gpio_prepare_for_idle() /
>>> omap_gpio_resume_from_idle() hackery to get GPIO suspended early enough.
>>>
>> Well so far I haven't seen/come across a patch/proposal which fixes it.
>
> fair point
>
>>> I still think mod_usage needs to go, so does
>>> omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it
>>> looks like that needs to be done on ->prepare()/->complete() callbacks
>>> of system suspend and the gpio driver needs to learn proper runtime
>>> suspend.
>>>
>> I am not saying it shouldn't go :-)
>> The $subject patch isn't fixing it correctly is what I said.
>>
>> Don't get hung up on suspend case because thats the easiest
>> way to address it. The issue is with idle where GPIO can prevent
>> SOC idle if it isn't taken care. And since its just an IO, its not
>> easy to implement something like inactivity timer towards
>> autosupend.
>
> I don't see the relation here. Care to expand a bit ?
>
Let me try again...

Before I answer you, a closer look at code, the multiple bank should
not be a problem since we create PIO device per bank so the runtime
PM layer can indeed manage the use-counting. The issue is with SOC
idle.

System wide suspend, we suspend all the devices and hence all the
GPIO devices(banks) will be suspended letting the SOC to hit suspend
state. Hence suspend case is no problem as such.

Devices like UART/USB can implement inactivity timers since they
do have state machine and after the timeout, those devices can be
suspended. GPIO doesn't fall exactly in that category and hence
its bit of an issue to take care. How do you ensure that GPIO
does idle on SOC idle C-state attempts in such cases. Today that
job is done by omap_gpio_[prepare/resume]_for_idle.

Idle if we had generic idle notifiers, that would been easy
but it doesn't exist today.

In case we get rid of "mod_usage" which is doable, we need to
see how to handle the idle since the runtime callback will be
in that case controlled by runtime PM layer which is unaware
of idle. You can do put on all banks to trigger callback but
that will highly UN-optimal. See summary in the end.


>> Co-processor also makes use of GPIO via syslink proxy and thats
>> make things even harder.
>
> that's supposed to be solved with hwspinlock, isn't it ?
>
Spin locks are needed when same devices are shared across. That
is not the main concern. The PM is centralized on Linux side for
GPIO where as the users are outside Linux SW. They may use of
syslink proxy to request/free GPIO.

Just to summaries, there are 3 things we are talking here.

1. Delaying the idle with a timeout which $subject patch is
trying to do to reduce latency for interrupts. That itself
is reasonable.

2. Removal of the bank "mod_usage" which is also clubbed
in $subject path. Ofcourse that break the current driver
for idle. So that change needs to be made with better thought
and in a separate patch. This is doable.

3. Removing omap_gpio_[prepare/resume]_for_idle() with soome
thing better. For this one though, so far I have not come
across a good solution. Ideas/Solution is welcome !!

Regards
Santosh

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

[Felipe Balbi]

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

Hi,

On Tue, Oct 30, 2012 at 12:02:17PM +0530, Santosh Shilimkar wrote:
> >>>I still think mod_usage needs to go, so does
> >>>omap_gpio_prepare_for_idle() and omap_gpio_resume_from_idle(). To me, it
> >>>looks like that needs to be done on ->prepare()/->complete() callbacks
> >>>of system suspend and the gpio driver needs to learn proper runtime
> >>>suspend.
> >>>
> >>I am not saying it shouldn't go :-)
> >>The $subject patch isn't fixing it correctly is what I said.
> >>
> >>Don't get hung up on suspend case because thats the easiest
> >>way to address it. The issue is with idle where GPIO can prevent
> >>SOC idle if it isn't taken care. And since its just an IO, its not
> >>easy to implement something like inactivity timer towards
> >>autosupend.
> >
> >I don't see the relation here. Care to expand a bit ?
> >
> Let me try again...
> 
> Before I answer you, a closer look at code, the multiple bank should
> not be a problem since we create PIO device per bank so the runtime
> PM layer can indeed manage the use-counting. The issue is with SOC
> idle.
> 
> System wide suspend, we suspend all the devices and hence all the
> GPIO devices(banks) will be suspended letting the SOC to hit suspend
> state. Hence suspend case is no problem as such.
> 
> Devices like UART/USB can implement inactivity timers since they
> do have state machine and after the timeout, those devices can be
> suspended. GPIO doesn't fall exactly in that category and hence

idling the device has nothing to do with their internal state machines,
though :-)

We just assume that if device hasn't been used for X ms, it's unlikely
it will continue to be used. Likewise, If device has been used Y ms ago,
it's likely it will continue to be used.

> its bit of an issue to take care. How do you ensure that GPIO
> does idle on SOC idle C-state attempts in such cases. Today that
> job is done by omap_gpio_[prepare/resume]_for_idle.

that's only there because we pm_runtime_get_sync() on gpio_request() and
pm_runtime_put_sync() only on gpio_free().

That's the problem IMHO. And that's easy enough to 'fix':

call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend();
also on gpio_request() and pm_runtime_get_sync() on gpio_free().

> Idle if we had generic idle notifiers, that would been easy
> but it doesn't exist today.
> 
> In case we get rid of "mod_usage" which is doable, we need to
> see how to handle the idle since the runtime callback will be
> in that case controlled by runtime PM layer which is unaware
> of idle. You can do put on all banks to trigger callback but

I think we might have a clash of concepts here. What are you calling
idle ? We _do_ have ->runtime_idle() callback, are we not using that ?

I can see omap_device.c implements it properly and I can see that
->runtime_idle() will be called whenever pm_usage counter is decremented
but is not zero. Looks like we can turn that into something useful.

> that will highly UN-optimal. See summary in the end.
> 
> >>Co-processor also makes use of GPIO via syslink proxy and thats
> >>make things even harder.
> >
> >that's supposed to be solved with hwspinlock, isn't it ?
> >
> Spin locks are needed when same devices are shared across. That
> is not the main concern. The PM is centralized on Linux side for
> GPIO where as the users are outside Linux SW. They may use of
> syslink proxy to request/free GPIO.

fair enough, that's even better.

> Just to summaries, there are 3 things we are talking here.
> 
> 1. Delaying the idle with a timeout which $subject patch is
> trying to do to reduce latency for interrupts. That itself
> is reasonable.
> 
> 2. Removal of the bank "mod_usage" which is also clubbed
> in $subject path. Ofcourse that break the current driver
> for idle. So that change needs to be made with better thought
> and in a separate patch. This is doable.
> 
> 3. Removing omap_gpio_[prepare/resume]_for_idle() with soome
> thing better. For this one though, so far I have not come
> across a good solution. Ideas/Solution is welcome !!

make pm_runtime a bit more aggressive. You don't need to keep GPIO
bank's clocks on just because a GPIO in that bank is requested. If
context save & restore isn't buggy, you can disable clocks no problem.

The difficult part, IMHO, is to figure out what's a good autosuspend
timeout to use. Some GPIO lines are used as IRQ lines on some devices,
that means that the GPIO will be periodically triggered and, depending
on our timeout, we will either loose IRQs or prevent power domain from
idling. We could figure out a way to let board code to choose what it
wants on a per-bank basis (maybe some extra DT attribute).

-- 
balbi

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

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

[Jon Hunter]

Hi Felipe,

On 10/30/2012 02:09 AM, Felipe Balbi wrote:

...

>> its bit of an issue to take care. How do you ensure that GPIO
>> does idle on SOC idle C-state attempts in such cases. Today that
>> job is done by omap_gpio_[prepare/resume]_for_idle.
> 
> that's only there because we pm_runtime_get_sync() on gpio_request() and
> pm_runtime_put_sync() only on gpio_free().
> 
> That's the problem IMHO. And that's easy enough to 'fix':
> 
> call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend();
> also on gpio_request() and pm_runtime_get_sync() on gpio_free().

Sounds like a good approach. I have been discussing with Kevin and I
need to look more at the resume handler as we are working around some
old issues in there and with this approach the resume following idle
will be delayed and we were not sure if there could be any implications
for omap2. I am hoping not, but we need to look into this.

So I am wondering if we should just take Tim's original proposal for now
and then I will look into improving this long term. I really need to
clean-up the suspend/resume stuff for gpio and so may be we can make
that a separate change. What do you think?

...

> The difficult part, IMHO, is to figure out what's a good autosuspend
> timeout to use. Some GPIO lines are used as IRQ lines on some devices,
> that means that the GPIO will be periodically triggered and, depending
> on our timeout, we will either loose IRQs or prevent power domain from
> idling. We could figure out a way to let board code to choose what it
> wants on a per-bank basis (maybe some extra DT attribute).

I have also been bending Kevin's ear on this, this week and we were
wondering if we should make the default 0 for now as this will have the
same behaviour that we have today but would allow Tim to customise via
the sysfs for his specific app.

Cheers
Jon

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

[Felipe Balbi]

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

Hi,

On Tue, Oct 30, 2012 at 09:16:34AM -0500, Jon Hunter wrote:
> Hi Felipe,
> 
> On 10/30/2012 02:09 AM, Felipe Balbi wrote:
> 
> ...
> 
> >> its bit of an issue to take care. How do you ensure that GPIO
> >> does idle on SOC idle C-state attempts in such cases. Today that
> >> job is done by omap_gpio_[prepare/resume]_for_idle.
> > 
> > that's only there because we pm_runtime_get_sync() on gpio_request() and
> > pm_runtime_put_sync() only on gpio_free().
> > 
> > That's the problem IMHO. And that's easy enough to 'fix':
> > 
> > call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend();
> > also on gpio_request() and pm_runtime_get_sync() on gpio_free().
> 
> Sounds like a good approach. I have been discussing with Kevin and I
> need to look more at the resume handler as we are working around some
> old issues in there and with this approach the resume following idle
> will be delayed and we were not sure if there could be any implications
> for omap2. I am hoping not, but we need to look into this.
> 
> So I am wondering if we should just take Tim's original proposal for now
> and then I will look into improving this long term. I really need to
> clean-up the suspend/resume stuff for gpio and so may be we can make
> that a separate change. What do you think?

that'll cause a regression right ?

> > The difficult part, IMHO, is to figure out what's a good autosuspend
> > timeout to use. Some GPIO lines are used as IRQ lines on some devices,
> > that means that the GPIO will be periodically triggered and, depending
> > on our timeout, we will either loose IRQs or prevent power domain from
> > idling. We could figure out a way to let board code to choose what it
> > wants on a per-bank basis (maybe some extra DT attribute).
> 
> I have also been bending Kevin's ear on this, this week and we were
> wondering if we should make the default 0 for now as this will have the

I believe you mean -1 here ;-)

> same behaviour that we have today but would allow Tim to customise via
> the sysfs for his specific app.

sysfs might be too late for his platform. What if he needs NFS root
(just wondering, not sure about his use case) ?

-- 
balbi

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

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

[Jon Hunter]

On 10/30/2012 10:10 AM, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Oct 30, 2012 at 09:16:34AM -0500, Jon Hunter wrote:
>> Hi Felipe,
>>
>> On 10/30/2012 02:09 AM, Felipe Balbi wrote:
>>
>> ...
>>
>>>> its bit of an issue to take care. How do you ensure that GPIO
>>>> does idle on SOC idle C-state attempts in such cases. Today that
>>>> job is done by omap_gpio_[prepare/resume]_for_idle.
>>>
>>> that's only there because we pm_runtime_get_sync() on gpio_request() and
>>> pm_runtime_put_sync() only on gpio_free().
>>>
>>> That's the problem IMHO. And that's easy enough to 'fix':
>>>
>>> call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend();
>>> also on gpio_request() and pm_runtime_get_sync() on gpio_free().
>>
>> Sounds like a good approach. I have been discussing with Kevin and I
>> need to look more at the resume handler as we are working around some
>> old issues in there and with this approach the resume following idle
>> will be delayed and we were not sure if there could be any implications
>> for omap2. I am hoping not, but we need to look into this.
>>
>> So I am wondering if we should just take Tim's original proposal for now
>> and then I will look into improving this long term. I really need to
>> clean-up the suspend/resume stuff for gpio and so may be we can make
>> that a separate change. What do you think?
> 
> that'll cause a regression right ?

Sorry, not sure I follow.

>>> The difficult part, IMHO, is to figure out what's a good autosuspend
>>> timeout to use. Some GPIO lines are used as IRQ lines on some devices,
>>> that means that the GPIO will be periodically triggered and, depending
>>> on our timeout, we will either loose IRQs or prevent power domain from
>>> idling. We could figure out a way to let board code to choose what it
>>> wants on a per-bank basis (maybe some extra DT attribute).
>>
>> I have also been bending Kevin's ear on this, this week and we were
>> wondering if we should make the default 0 for now as this will have the
> 
> I believe you mean -1 here ;-)

I did mean 0, so that it will autosuspend right away. Basically, it will
behave like today, however, will allow people to change the timeout. I
did not wish to make it -1 as then suspend/resume would not be exercised
and so people would need to change it via the sysfs to exercise deep
power states on the device.

>> same behaviour that we have today but would allow Tim to customise via
>> the sysfs for his specific app.
> 
> sysfs might be too late for his platform. What if he needs NFS root
> (just wondering, not sure about his use case) ?

His use case was for SPI (see the original changelog). That's a good
point, but I am wondering if we can live with that for now.

Cheers
Jon

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

[Felipe Balbi]

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

Hi,

On Wed, Oct 31, 2012 at 05:15:02AM -0500, Jon Hunter wrote:
> >>>> its bit of an issue to take care. How do you ensure that GPIO
> >>>> does idle on SOC idle C-state attempts in such cases. Today that
> >>>> job is done by omap_gpio_[prepare/resume]_for_idle.
> >>>
> >>> that's only there because we pm_runtime_get_sync() on gpio_request() and
> >>> pm_runtime_put_sync() only on gpio_free().
> >>>
> >>> That's the problem IMHO. And that's easy enough to 'fix':
> >>>
> >>> call pm_runtime_mark_last_busy(); pm_runtime_put_sync_autosuspend();
> >>> also on gpio_request() and pm_runtime_get_sync() on gpio_free().
> >>
> >> Sounds like a good approach. I have been discussing with Kevin and I
> >> need to look more at the resume handler as we are working around some
> >> old issues in there and with this approach the resume following idle
> >> will be delayed and we were not sure if there could be any implications
> >> for omap2. I am hoping not, but we need to look into this.
> >>
> >> So I am wondering if we should just take Tim's original proposal for now
> >> and then I will look into improving this long term. I really need to
> >> clean-up the suspend/resume stuff for gpio and so may be we can make
> >> that a separate change. What do you think?
> > 
> > that'll cause a regression right ?
> 
> Sorry, not sure I follow.

$SUBJECT will prevent core idle.

> >>> The difficult part, IMHO, is to figure out what's a good autosuspend
> >>> timeout to use. Some GPIO lines are used as IRQ lines on some devices,
> >>> that means that the GPIO will be periodically triggered and, depending
> >>> on our timeout, we will either loose IRQs or prevent power domain from
> >>> idling. We could figure out a way to let board code to choose what it
> >>> wants on a per-bank basis (maybe some extra DT attribute).
> >>
> >> I have also been bending Kevin's ear on this, this week and we were
> >> wondering if we should make the default 0 for now as this will have the
> > 
> > I believe you mean -1 here ;-)
> 
> I did mean 0, so that it will autosuspend right away. Basically, it will
> behave like today, however, will allow people to change the timeout. I

yeah, that's the -1 timeout, it disables pm timer and all
pm_runtime_*_autosuspend() will act as pm_runtime_*().

> did not wish to make it -1 as then suspend/resume would not be exercised
> and so people would need to change it via the sysfs to exercise deep
> power states on the device.

why won't be exercised ?

> >> same behaviour that we have today but would allow Tim to customise via
> >> the sysfs for his specific app.
> > 
> > sysfs might be too late for his platform. What if he needs NFS root
> > (just wondering, not sure about his use case) ?
> 
> His use case was for SPI (see the original changelog). That's a good
> point, but I am wondering if we can live with that for now.

fair enough, but it's possible we will cause regressions. At least with
current version of $SUBJECT, I guess.

-- 
balbi

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

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

[Kevin Hilman]

Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

[...]

> Just to summaries, there are 3 things we are talking here.

Santosh, thanks for the summary.  You are right on.

> 1. Delaying the idle with a timeout which $subject patch is
> trying to do to reduce latency for interrupts. That itself
> is reasonable.

I agree, this is reasonable.   IMO, adding autosuspend should be done as
an isolated patch and kept separate from any other cleanup or changes.

Also, as Jon mentioned, I think this should be done with a default
timeout of zero so that current behaviour is unchanged.  Tim can then
set the timeout from userspace for his usecase and everyone is happy.

> 2. Removal of the bank "mod_usage" which is also clubbed
> in $subject path. Ofcourse that break the current driver
> for idle. So that change needs to be made with better thought
> and in a separate patch. This is doable.

I had this discussion with Charu/Tarun during the last round of cleanup
because I didn't like this mod_usage either.  The alternative is to do a
'get' for every GPIO in the bank since runtime PM is doing the
usecounting already.  As Santosh said, this is fine for suspend, but for
idle it means calling 'put' for every enabled GPIO in the bank instead
of just forcing things with a single 'put.'

> 3. Removing omap_gpio_[prepare/resume]_for_idle() with soome thing
> better. For this one though, so far I have not come across a good
> solution. Ideas/Solution is welcome !!

I agree that the prepare/resume idle hooks a are ugly and should be
removed, but this needs quite a bit more thought.  In particular,
the 'remove triggering' stuff that is done needs pretty close
examination.

I'm not saying it can't be done, but patches to do this should be
separate, well described and well tested, especially with off-mode.

Kevin

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

[Santosh Shilimkar]

On Wednesday 31 October 2012 04:07 PM, Kevin Hilman wrote:
> Santosh Shilimkar <santosh.shilimkar@ti.com> writes:
>
> [...]
>
>> Just to summaries, there are 3 things we are talking here.
>
> Santosh, thanks for the summary.  You are right on.
>
>> 1. Delaying the idle with a timeout which $subject patch is
>> trying to do to reduce latency for interrupts. That itself
>> is reasonable.
>
> I agree, this is reasonable.   IMO, adding autosuspend should be done as
> an isolated patch and kept separate from any other cleanup or changes.
>
> Also, as Jon mentioned, I think this should be done with a default
> timeout of zero so that current behaviour is unchanged.  Tim can then
> set the timeout from userspace for his usecase and everyone is happy.
>
Default 0 sounds good to me as well.

>> 2. Removal of the bank "mod_usage" which is also clubbed
>> in $subject path. Ofcourse that break the current driver
>> for idle. So that change needs to be made with better thought
>> and in a separate patch. This is doable.
>
> I had this discussion with Charu/Tarun during the last round of cleanup
> because I didn't like this mod_usage either.  The alternative is to do a
> 'get' for every GPIO in the bank since runtime PM is doing the
> usecounting already.  As Santosh said, this is fine for suspend, but for
> idle it means calling 'put' for every enabled GPIO in the bank instead
> of just forcing things with a single 'put.'
>
Exactly. The oherhead becomes too much when if you need to call put like 
6 Banks * 32 GPIOS times.
Actually even though there are 32 GPIOs in a bank, from PM point of
view, it only needs to care about a bank level control. Because
clocks and register set is per bank. And we model a GPIO device
per bank as well.

>> 3. Removing omap_gpio_[prepare/resume]_for_idle() with soome thing
>> better. For this one though, so far I have not come across a good
>> solution. Ideas/Solution is welcome !!
>
> I agree that the prepare/resume idle hooks a are ugly and should be
> removed, but this needs quite a bit more thought.  In particular,
> the 'remove triggering' stuff that is done needs pretty close
> examination.
>
> I'm not saying it can't be done, but patches to do this should be
> separate, well described and well tested, especially with off-mode.
>
Couldn't agree more. Thanks for confirming the points.

Regards
santosh