[PATCH] OMAP3: PM: Modify suspend wakeup timer to use milliseconds instead of seconds

[PATCH] OMAP3: PM: Modify suspend wakeup timer to use milliseconds instead of seconds

[Ari Kauppi]

Millisecond resolution is possible and there are use cases for it
(automatic testing).

Seconds-based interface is preserved for compatibility.

Signed-off-by: Ari Kauppi <Ext-Ari.Kauppi@nokia.com>
---
 arch/arm/mach-omap2/pm-debug.c |   11 +++++++++++
 arch/arm/mach-omap2/pm.h       |    2 +-
 arch/arm/mach-omap2/pm34xx.c   |   16 ++++++++--------
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index 8aafd71..0cf2a03 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -37,6 +37,7 @@
 #include "prm-regbits-34xx.h"
 
 int omap2_pm_debug;
+static u32 wakeup_timer_seconds;
 
 #define DUMP_PRM_MOD_REG(mod, reg)    \
 	regs[reg_count].name = #mod "." #reg; \
@@ -552,6 +553,12 @@ static int option_set(void *data, u64 val)
 
 	if (option == &enable_off_mode)
 		omap3_pm_off_mode_enable(val);
+
+	if (option == &wakeup_timer_seconds)
+		wakeup_timer_milliseconds = val * 1000;
+	if (option == &wakeup_timer_milliseconds)
+		wakeup_timer_seconds = roundup((u32)val, 1000) / 1000;
+
 	return 0;
 }
 
@@ -605,6 +612,10 @@ static int __init pm_dbg_init(void)
 				   &sleep_while_idle, &pm_dbg_option_fops);
 	(void) debugfs_create_file("wakeup_timer_seconds", S_IRUGO | S_IWUGO, d,
 				   &wakeup_timer_seconds, &pm_dbg_option_fops);
+	(void) debugfs_create_file("wakeup_timer_milliseconds",
+				   S_IRUGO | S_IWUGO, d,
+				   &wakeup_timer_milliseconds,
+				   &pm_dbg_option_fops);
 
 	/* Only enable for >= ES2.1 . Going to 0V on anything under
 	 * ES2.1 will eventually cause a crash */
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index b761be5..423395a 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -68,7 +68,7 @@ static inline void omap3_pm_init_vc(struct prm_setup_vc *setup_vc)
 extern int omap3_pm_get_suspend_state(struct powerdomain *pwrdm);
 extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
 
-extern u32 wakeup_timer_seconds;
+extern u32 wakeup_timer_milliseconds;
 extern struct omap_dm_timer *gptimer_wakeup;
 
 #ifdef CONFIG_PM_DEBUG
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 3868c76..cd55968 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -73,7 +73,7 @@ static inline bool is_suspending(void)
 
 u32 enable_off_mode;
 u32 sleep_while_idle;
-u32 wakeup_timer_seconds;
+u32 wakeup_timer_milliseconds;
 u32 voltage_off_while_idle;
 
 struct power_state {
@@ -640,20 +640,20 @@ out:
 }
 
 #ifdef CONFIG_SUSPEND
-static void omap2_pm_wakeup_on_timer(u32 seconds)
+static void omap2_pm_wakeup_on_timer(u32 milliseconds)
 {
 	u32 tick_rate, cycles;
 
-	if (!seconds)
+	if (!milliseconds)
 		return;
 
 	tick_rate = clk_get_rate(omap_dm_timer_get_fclk(gptimer_wakeup));
-	cycles = tick_rate * seconds;
+	cycles = tick_rate * milliseconds / 1000;
 	omap_dm_timer_stop(gptimer_wakeup);
 	omap_dm_timer_set_load_start(gptimer_wakeup, 0, 0xffffffff - cycles);
 
-	pr_info("PM: Resume timer in %d secs (%d ticks at %d ticks/sec.)\n",
-		seconds, cycles, tick_rate);
+	pr_info("PM: Resume timer in %u ms (%u ticks at %u ticks/sec.)\n",
+		milliseconds, cycles, tick_rate);
 }
 
 static int omap3_pm_prepare(void)
@@ -667,8 +667,8 @@ static int omap3_pm_suspend(void)
 	struct power_state *pwrst;
 	int state, ret = 0;
 
-	if (wakeup_timer_seconds)
-		omap2_pm_wakeup_on_timer(wakeup_timer_seconds);
+	if (wakeup_timer_milliseconds)
+		omap2_pm_wakeup_on_timer(wakeup_timer_milliseconds);
 
 	/* Read current next_pwrsts */
 	list_for_each_entry(pwrst, &pwrst_list, node)
-- 
1.6.4.2

Re: [PATCH] OMAP3: PM: Modify suspend wakeup timer to use milliseconds instead of seconds

[Phil Carmody]

On 22/03/10 09:10 +0100, Kauppi Ari (EXT-Ixonos/Oulu) wrote:
> Millisecond resolution is possible and there are use cases for it
> (automatic testing).
> 
> Seconds-based interface is preserved for compatibility.
> 
> Signed-off-by: Ari Kauppi <Ext-Ari.Kauppi@nokia.com>
> ---
...
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 3868c76..cd55968 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
...
> @@ -640,20 +640,20 @@ out:
>  }
>  
>  #ifdef CONFIG_SUSPEND
> -static void omap2_pm_wakeup_on_timer(u32 seconds)
> +static void omap2_pm_wakeup_on_timer(u32 milliseconds)
>  {
>  	u32 tick_rate, cycles;
>  
> -	if (!seconds)
> +	if (!milliseconds)
>  		return;
>  
>  	tick_rate = clk_get_rate(omap_dm_timer_get_fclk(gptimer_wakeup));
> -	cycles = tick_rate * seconds;
> +	cycles = tick_rate * milliseconds / 1000;

Ari has pointed me here regarding clarification of whether the above
is an overflow risk? At 32768 Hz, the above intermediate will overflow 
a u32 if milliseconds >= 131072, e.g. 2 minutes 10+ seconds. 

If there is an overflow risk, then something like the following would
work around it:

	u32 seconds = milliseconds / 1000;
	milliseconds -= seconds * 1000;
	cycles = tick_rate * seconds + tick_rate * milliseconds / 1000;

(Alternatively, a u64 could be used for the intermediate, and we could
just let gcc manage the 64 bit / constant 32 bit division.)

Unfortunately, as tick_rate is not a constant, there's no trivial fast-path 
check for overflow before it happens. Then again, this looks like it's
only used in debug code, so hopefully that's not an issue.

Phil

[PATCH v2] OMAP3: PM: Add milliseconds interface to suspend wakeup timer

[Ari Kauppi]

Millisecond resolution is possible and there are use cases for it
(automatic testing).

Seconds-based interface is preserved for compatibility.

Signed-off-by: Ari Kauppi <Ext-Ari.Kauppi@nokia.com>

---
v2: Keep seconds and milliseconds interfaces strictly separate:
 - Consistent interface, setting/getting seconds and milliseconds
   is always accurate
 - Fixes potential overflow issues in omap2_pm_wakeup_on_timer
 - Cleaner patch
---
 arch/arm/mach-omap2/pm-debug.c |    7 +++++++
 arch/arm/mach-omap2/pm.h       |    1 +
 arch/arm/mach-omap2/pm34xx.c   |   17 ++++++++++-------
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index 8aafd71..83acaa2 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -548,6 +548,9 @@ static int option_set(void *data, u64 val)
 {
 	u32 *option = data;
 
+	if (option == &wakeup_timer_milliseconds && val >= 1000)
+		return -EINVAL;
+
 	*option = val;
 
 	if (option == &enable_off_mode)
@@ -605,6 +608,10 @@ static int __init pm_dbg_init(void)
 				   &sleep_while_idle, &pm_dbg_option_fops);
 	(void) debugfs_create_file("wakeup_timer_seconds", S_IRUGO | S_IWUGO, d,
 				   &wakeup_timer_seconds, &pm_dbg_option_fops);
+	(void) debugfs_create_file("wakeup_timer_milliseconds",
+				   S_IRUGO | S_IWUGO, d,
+				   &wakeup_timer_milliseconds,
+				   &pm_dbg_option_fops);
 
 	/* Only enable for >= ES2.1 . Going to 0V on anything under
 	 * ES2.1 will eventually cause a crash */
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index b761be5..b3594a9 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -69,6 +69,7 @@ extern int omap3_pm_get_suspend_state(struct powerdomain *pwrdm);
 extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
 
 extern u32 wakeup_timer_seconds;
+extern u32 wakeup_timer_milliseconds;
 extern struct omap_dm_timer *gptimer_wakeup;
 
 #ifdef CONFIG_PM_DEBUG
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 3868c76..d7922a5 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -74,6 +74,7 @@ static inline bool is_suspending(void)
 u32 enable_off_mode;
 u32 sleep_while_idle;
 u32 wakeup_timer_seconds;
+u32 wakeup_timer_milliseconds;
 u32 voltage_off_while_idle;
 
 struct power_state {
@@ -640,20 +641,21 @@ out:
 }
 
 #ifdef CONFIG_SUSPEND
-static void omap2_pm_wakeup_on_timer(u32 seconds)
+static void omap2_pm_wakeup_on_timer(u32 seconds, u32 milliseconds)
 {
 	u32 tick_rate, cycles;
 
-	if (!seconds)
+	if (!seconds && !milliseconds)
 		return;
 
 	tick_rate = clk_get_rate(omap_dm_timer_get_fclk(gptimer_wakeup));
-	cycles = tick_rate * seconds;
+	cycles = tick_rate * seconds + tick_rate * milliseconds / 1000;
 	omap_dm_timer_stop(gptimer_wakeup);
 	omap_dm_timer_set_load_start(gptimer_wakeup, 0, 0xffffffff - cycles);
 
-	pr_info("PM: Resume timer in %d secs (%d ticks at %d ticks/sec.)\n",
-		seconds, cycles, tick_rate);
+	pr_info("PM: Resume timer in %u.%03u secs"
+		" (%d ticks at %d ticks/sec.)\n",
+		seconds, milliseconds, cycles, tick_rate);
 }
 
 static int omap3_pm_prepare(void)
@@ -667,8 +669,9 @@ static int omap3_pm_suspend(void)
 	struct power_state *pwrst;
 	int state, ret = 0;
 
-	if (wakeup_timer_seconds)
-		omap2_pm_wakeup_on_timer(wakeup_timer_seconds);
+	if (wakeup_timer_seconds || wakeup_timer_milliseconds)
+		omap2_pm_wakeup_on_timer(wakeup_timer_seconds,
+					 wakeup_timer_milliseconds);
 
 	/* Read current next_pwrsts */
 	list_for_each_entry(pwrst, &pwrst_list, node)
-- 
1.6.4.2

Re: [PATCH v2] OMAP3: PM: Add milliseconds interface to suspend wakeup timer

[Phil Carmody]

On 23/03/10 08:04 +0100, Kauppi Ari (EXT-Ixonos/Oulu) wrote:
> Millisecond resolution is possible and there are use cases for it
> (automatic testing).
> 
> Seconds-based interface is preserved for compatibility.
> 
> Signed-off-by: Ari Kauppi <Ext-Ari.Kauppi@nokia.com>
>
> ---
> v2: Keep seconds and milliseconds interfaces strictly separate:
>  - Consistent interface, setting/getting seconds and milliseconds
>    is always accurate
>  - Fixes potential overflow issues in omap2_pm_wakeup_on_timer
>  - Cleaner patch

Nice one. Just what we need, nothing more, nothing less.

Reviewed-by: Phil Carmody <ext-phil.2.carmody@nokia.com>

Phil

> ---
>  arch/arm/mach-omap2/pm-debug.c |    7 +++++++
>  arch/arm/mach-omap2/pm.h       |    1 +
>  arch/arm/mach-omap2/pm34xx.c   |   17 ++++++++++-------
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index 8aafd71..83acaa2 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -548,6 +548,9 @@ static int option_set(void *data, u64 val)
>  {
>  	u32 *option = data;
>  
> +	if (option == &wakeup_timer_milliseconds && val >= 1000)
> +		return -EINVAL;
> +
>  	*option = val;
>  
>  	if (option == &enable_off_mode)
> @@ -605,6 +608,10 @@ static int __init pm_dbg_init(void)
>  				   &sleep_while_idle, &pm_dbg_option_fops);
>  	(void) debugfs_create_file("wakeup_timer_seconds", S_IRUGO | S_IWUGO, d,
>  				   &wakeup_timer_seconds, &pm_dbg_option_fops);
> +	(void) debugfs_create_file("wakeup_timer_milliseconds",
> +				   S_IRUGO | S_IWUGO, d,
> +				   &wakeup_timer_milliseconds,
> +				   &pm_dbg_option_fops);
>  
>  	/* Only enable for >= ES2.1 . Going to 0V on anything under
>  	 * ES2.1 will eventually cause a crash */
> diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
> index b761be5..b3594a9 100644
> --- a/arch/arm/mach-omap2/pm.h
> +++ b/arch/arm/mach-omap2/pm.h
> @@ -69,6 +69,7 @@ extern int omap3_pm_get_suspend_state(struct powerdomain *pwrdm);
>  extern int omap3_pm_set_suspend_state(struct powerdomain *pwrdm, int state);
>  
>  extern u32 wakeup_timer_seconds;
> +extern u32 wakeup_timer_milliseconds;
>  extern struct omap_dm_timer *gptimer_wakeup;
>  
>  #ifdef CONFIG_PM_DEBUG
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 3868c76..d7922a5 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -74,6 +74,7 @@ static inline bool is_suspending(void)
>  u32 enable_off_mode;
>  u32 sleep_while_idle;
>  u32 wakeup_timer_seconds;
> +u32 wakeup_timer_milliseconds;
>  u32 voltage_off_while_idle;
>  
>  struct power_state {
> @@ -640,20 +641,21 @@ out:
>  }
>  
>  #ifdef CONFIG_SUSPEND
> -static void omap2_pm_wakeup_on_timer(u32 seconds)
> +static void omap2_pm_wakeup_on_timer(u32 seconds, u32 milliseconds)
>  {
>  	u32 tick_rate, cycles;
>  
> -	if (!seconds)
> +	if (!seconds && !milliseconds)
>  		return;
>  
>  	tick_rate = clk_get_rate(omap_dm_timer_get_fclk(gptimer_wakeup));
> -	cycles = tick_rate * seconds;
> +	cycles = tick_rate * seconds + tick_rate * milliseconds / 1000;
>  	omap_dm_timer_stop(gptimer_wakeup);
>  	omap_dm_timer_set_load_start(gptimer_wakeup, 0, 0xffffffff - cycles);
>  
> -	pr_info("PM: Resume timer in %d secs (%d ticks at %d ticks/sec.)\n",
> -		seconds, cycles, tick_rate);
> +	pr_info("PM: Resume timer in %u.%03u secs"
> +		" (%d ticks at %d ticks/sec.)\n",
> +		seconds, milliseconds, cycles, tick_rate);
>  }
>  
>  static int omap3_pm_prepare(void)
> @@ -667,8 +669,9 @@ static int omap3_pm_suspend(void)
>  	struct power_state *pwrst;
>  	int state, ret = 0;
>  
> -	if (wakeup_timer_seconds)
> -		omap2_pm_wakeup_on_timer(wakeup_timer_seconds);
> +	if (wakeup_timer_seconds || wakeup_timer_milliseconds)
> +		omap2_pm_wakeup_on_timer(wakeup_timer_seconds,
> +					 wakeup_timer_milliseconds);
>  
>  	/* Read current next_pwrsts */
>  	list_for_each_entry(pwrst, &pwrst_list, node)
> -- 
> 1.6.4.2

Re: [PATCH v2] OMAP3: PM: Add milliseconds interface to suspend wakeup timer

[Kevin Hilman]

Phil Carmody <ext-phil.2.carmody@nokia.com> writes:

> On 23/03/10 08:04 +0100, Kauppi Ari (EXT-Ixonos/Oulu) wrote:
>> Millisecond resolution is possible and there are use cases for it
>> (automatic testing).
>> 
>> Seconds-based interface is preserved for compatibility.
>> 
>> Signed-off-by: Ari Kauppi <Ext-Ari.Kauppi@nokia.com>
>>
>> ---
>> v2: Keep seconds and milliseconds interfaces strictly separate:
>>  - Consistent interface, setting/getting seconds and milliseconds
>>    is always accurate
>>  - Fixes potential overflow issues in omap2_pm_wakeup_on_timer
>>  - Cleaner patch
>
> Nice one. Just what we need, nothing more, nothing less.
>
> Reviewed-by: Phil Carmody <ext-phil.2.carmody@nokia.com>
>

Thanks, tested on omap3evm.

Pulling into PM branch queueing for 2.6.35.

Kevin