[RFC 0/4] OMAP Cpuidle/Suspend Cleanups

[RFC 0/4] OMAP Cpuidle/Suspend Cleanups

[Rajendra Nayak]

Hi,

Here are some CPUidle/Suspend cleanup patches done by adding
some more functionality in the OMAP Powerdomain framework.
They mostly cleanup OMAP3 but the same ideas can be used/
applied for OMAP4 and beyond.

The series is based off Teros' series [1] to add usecounting
support within the OMAP Powerdomain framework.

The patches are tested with RET/OFF in CPUidle and Suspend
on 3630 beagle Xm and 3430 SDP.

regards,
Rajendra

[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg72163.html

Rajendra Nayak (4):
  ARM: OMAP3: cpuidle: Remove unused MPU OSWR support code
  ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle
  ARM: OMAP: powerdomain: Add .power_on/.power_down hooks for
    powerdomains
  ARM: OMAP3: PM: Use .power_on/.power_down to clean omap_sram_idle

 arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 +-
 arch/arm/mach-omap2/pm34xx.c              |  158 ++++++++++++++---------------
 arch/arm/mach-omap2/powerdomain.c         |   40 ++++----
 arch/arm/mach-omap2/powerdomain.h         |    8 +-
 arch/arm/mach-omap2/sleep34xx.S           |    4 +-
 5 files changed, 104 insertions(+), 110 deletions(-)

[RFC 1/4] ARM: OMAP3: cpuidle: Remove unused MPU OSWR support code

[Rajendra Nayak]

We do not support MPU OSWR on OMAP3. Get rid of the complex/multiple
save_state handling in omap_sram_idle() and just use 2 save_state
definitions

save_state = 1, all logic and memory lost, MPU hits OFF
save_state = 0, nothing lost, MPU hits CSWR or shallower state

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/pm34xx.c    |   35 +++++++++--------------------------
 arch/arm/mach-omap2/sleep34xx.S |    4 +---
 2 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 7c7b173..8d96b1f 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -256,19 +256,16 @@ static void omap34xx_save_context(u32 *save)
 
 static int omap34xx_do_sram_idle(unsigned long save_state)
 {
+	/*
+	 * save_state = 1 indicates all logic and memory/cache lost
+	 * save_state = 0 indicates nothing lost
+	 */
 	omap34xx_cpu_suspend(save_state);
 	return 0;
 }
 
 void omap_sram_idle(void)
 {
-	/* Variable to tell what needs to be saved and restored
-	 * in omap_sram_idle*/
-	/* save_state = 0 => Nothing to save and restored */
-	/* save_state = 1 => Only L1 and logic lost */
-	/* save_state = 2 => Only L2 lost */
-	/* save_state = 3 => L1, L2 and logic lost */
-	int save_state = 0;
 	int mpu_next_state = PWRDM_POWER_ON;
 	int per_next_state = PWRDM_POWER_ON;
 	int core_next_state = PWRDM_POWER_ON;
@@ -277,20 +274,6 @@ void omap_sram_idle(void)
 	u32 sdrc_pwr = 0;
 
 	mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
-	switch (mpu_next_state) {
-	case PWRDM_POWER_ON:
-	case PWRDM_POWER_RET:
-		/* No need to save context */
-		save_state = 0;
-		break;
-	case PWRDM_POWER_OFF:
-		save_state = 3;
-		break;
-	default:
-		/* Invalid state */
-		pr_err("Invalid mpu state in sram_idle\n");
-		return;
-	}
 
 	/* NEON control */
 	if (pwrdm_read_pwrst(neon_pwrdm) == PWRDM_POWER_ON)
@@ -342,12 +325,12 @@ void omap_sram_idle(void)
 	 * get saved. The rest is placed on the stack, and restored
 	 * from there before resuming.
 	 */
-	if (save_state)
+	if (mpu_next_state == PWRDM_POWER_OFF) {
 		omap34xx_save_context(omap3_arm_context);
-	if (save_state == 1 || save_state == 3)
-		cpu_suspend(save_state, omap34xx_do_sram_idle);
-	else
-		omap34xx_do_sram_idle(save_state);
+		cpu_suspend(1, omap34xx_do_sram_idle);
+	} else {
+		omap34xx_do_sram_idle(0);
+	}
 
 	/* Restore normal SDRC POWER settings */
 	if (cpu_is_omap3430() && omap_rev() >= OMAP3430_REV_ES3_0 &&
diff --git a/arch/arm/mach-omap2/sleep34xx.S b/arch/arm/mach-omap2/sleep34xx.S
index 1f62f23..a4d04a4 100644
--- a/arch/arm/mach-omap2/sleep34xx.S
+++ b/arch/arm/mach-omap2/sleep34xx.S
@@ -157,9 +157,7 @@ ENTRY(omap34xx_cpu_suspend)
 	/*
 	 * r0 contains information about saving context:
 	 *   0 - No context lost
-	 *   1 - Only L1 and logic lost
-	 *   2 - Only L2 lost (Even L1 is retained we clean it along with L2)
-	 *   3 - Both L1 and L2 lost and logic lost
+	 *   1 - Both L1 and L2 lost and logic lost
 	 */
 
 	/*
-- 
1.7.1

[RFC 1/4] ARM: OMAP3: cpuidle: Remove unused MPU OSWR support code

[Shilimkar, Santosh]

On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> We do not support MPU OSWR on OMAP3. Get rid of the complex/multiple
> save_state handling in omap_sram_idle() and just use 2 save_state
> definitions
>
With current mainline code, this is indeed true.

> save_state = 1, all logic and memory lost, MPU hits OFF
> save_state = 0, nothing lost, MPU hits CSWR or shallower state
>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  arch/arm/mach-omap2/pm34xx.c    |   35 +++++++++--------------------------
>  arch/arm/mach-omap2/sleep34xx.S |    4 +---
>  2 files changed, 10 insertions(+), 29 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 7c7b173..8d96b1f 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -256,19 +256,16 @@ static void omap34xx_save_context(u32 *save)
>
>  static int omap34xx_do_sram_idle(unsigned long save_state)
>  {
> +       /*
> +        * save_state = 1 indicates all logic and memory/cache lost
> +        * save_state = 0 indicates nothing lost
> +        */
Keep the same comment with mention of the MPU
state like
save_state = 1, all logic and memory lost, MPU hits OFF
save_state = 0, nothing lost, MPU hits CSWR or shallower state

Rest of the code looks good.

Regards
santosh

[RFC 1/4] ARM: OMAP3: cpuidle: Remove unused MPU OSWR support code

[Paul Walmsley]

On Fri, 20 Jul 2012, Rajendra Nayak wrote:

> We do not support MPU OSWR on OMAP3. Get rid of the complex/multiple
> save_state handling in omap_sram_idle() and just use 2 save_state
> definitions
> 
> save_state = 1, all logic and memory lost, MPU hits OFF
> save_state = 0, nothing lost, MPU hits CSWR or shallower state
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>

The code is certainly simpler, but I recall seeing patchsets in the past 
that implemented OSWR on OMAP3.  (Not sure which powerdomains it was 
implemented for.)  Do you know what the status of those patchsets are?

- Paul

[RFC 1/4] ARM: OMAP3: cpuidle: Remove unused MPU OSWR support code

[Rajendra Nayak]

Hi Paul,

On Friday 20 July 2012 11:55 PM, Paul Walmsley wrote:
> On Fri, 20 Jul 2012, Rajendra Nayak wrote:
>
>> We do not support MPU OSWR on OMAP3. Get rid of the complex/multiple
>> save_state handling in omap_sram_idle() and just use 2 save_state
>> definitions
>>
>> save_state = 1, all logic and memory lost, MPU hits OFF
>> save_state = 0, nothing lost, MPU hits CSWR or shallower state
>>
>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>
> The code is certainly simpler, but I recall seeing patchsets in the past
> that implemented OSWR on OMAP3.  (Not sure which powerdomains it was
> implemented for.)  Do you know what the status of those patchsets are?

OMAP3 supports OSWR for MPU/IVA/CORE and PER powerdomains. Though we did
have some OSWR implementations in our internal trees, we never ended up
using them effectively, mainly because the latencies for OSWR as
compared to OFF were very close on OMAP3 and the power savings from OFF
(given we could hit 0v) significantly higher.

OSWR on OMAP4 was hence reworked to keep much more retained in OSWR and
make it, in latency terms, something _in_between_ a CSWR and OFF.

Someone at some point (I don't recall and it could have even been me)
would have tried to upstream the OSWR support from our internal trees,
but as of now I don't know if anyone is trying to push or interested in
OSWR support for OMAP3 anymore.

regards,
Rajendra

>
> - Paul

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Rajendra Nayak]

pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
operations done within cpuidle to do Powerdomain level book-keeping to know
what state transitions for different Powerdomains have been triggered.
This is also useful to do a restore-on-demand in some cases when we know
the context for the given Powerdomain was lost etc.

Now that we have definitive entry/exit points (thanks to the Powerdomain
level usecounting) for Powerdomain transitions, these book-keeping functions
can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
clkdm_disable() functions.

Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
and get rid of the original ones which iterate over all powerdomains.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 ++--
 arch/arm/mach-omap2/pm34xx.c              |    4 ++--
 arch/arm/mach-omap2/powerdomain.c         |   28 ++++++++--------------------
 arch/arm/mach-omap2/powerdomain.h         |    4 ++--
 4 files changed, 14 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
index 13670aa..ea19439 100644
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -255,7 +255,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
 		return -ENXIO;
 	}
 
-	pwrdm_pre_transition();
+	pwrdm_cpu_idle();
 
 	/*
 	 * Check MPUSS next state and save interrupt controller if needed.
@@ -287,7 +287,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
 	wakeup_cpu = smp_processor_id();
 	set_cpu_next_pwrst(wakeup_cpu, PWRDM_POWER_ON);
 
-	pwrdm_post_transition();
+	pwrdm_cpu_wakeup();
 
 	return 0;
 }
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 8d96b1f..9bdf53c 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -290,7 +290,7 @@ void omap_sram_idle(void)
 			omap3_enable_io_chain();
 	}
 
-	pwrdm_pre_transition();
+	pwrdm_cpu_idle();
 
 	/* PER */
 	if (per_next_state < PWRDM_POWER_ON) {
@@ -355,7 +355,7 @@ void omap_sram_idle(void)
 	}
 	omap3_intc_resume_idle();
 
-	pwrdm_post_transition();
+	pwrdm_cpu_wakeup();
 
 	/* PER */
 	if (per_next_state < PWRDM_POWER_ON)
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 39a45a9..f435819 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -187,14 +187,14 @@ static int _pwrdm_state_switch(struct powerdomain *pwrdm, int flag)
 	return 0;
 }
 
-static int _pwrdm_pre_transition_cb(struct powerdomain *pwrdm, void *unused)
+int pwrdm_pre_transition(struct powerdomain *pwrdm)
 {
 	pwrdm_clear_all_prev_pwrst(pwrdm);
 	_pwrdm_state_switch(pwrdm, PWRDM_STATE_NOW);
 	return 0;
 }
 
-static int _pwrdm_post_transition_cb(struct powerdomain *pwrdm, void *unused)
+int pwrdm_post_transition(struct powerdomain *pwrdm)
 {
 	_pwrdm_state_switch(pwrdm, PWRDM_STATE_PREV);
 	return 0;
@@ -995,8 +995,10 @@ void pwrdm_clkdm_enable(struct powerdomain *pwrdm)
 	if (!pwrdm)
 		return;
 
-	if (atomic_inc_return(&pwrdm->usecount) == 1)
+	if (atomic_inc_return(&pwrdm->usecount) == 1) {
+		pwrdm_post_transition(pwrdm);
 		voltdm_pwrdm_enable(pwrdm->voltdm.ptr);
+	}
 }
 
 /**
@@ -1015,28 +1017,14 @@ void pwrdm_clkdm_disable(struct powerdomain *pwrdm)
 
 	val = atomic_dec_return(&pwrdm->usecount);
 
-	if (!val)
+	if (!val) {
 		voltdm_pwrdm_disable(pwrdm->voltdm.ptr);
+		pwrdm_pre_transition(pwrdm);
+	}
 
 	BUG_ON(val < 0);
 }
 
-int pwrdm_pre_transition(void)
-{
-	pwrdm_for_each(_pwrdm_pre_transition_cb, NULL);
-	/* Decrease mpu / core usecounts to indicate we are entering idle */
-	pwrdm_cpu_idle();
-	return 0;
-}
-
-int pwrdm_post_transition(void)
-{
-	/* Increase mpu / core usecounts to indicate we are leaving idle */
-	pwrdm_cpu_wakeup();
-	pwrdm_for_each(_pwrdm_post_transition_cb, NULL);
-	return 0;
-}
-
 /**
  * pwrdm_get_context_loss_count - get powerdomain's context loss count
  * @pwrdm: struct powerdomain * to wait for
diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
index ecf7d3d..52a99cf 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -214,8 +214,8 @@ bool pwrdm_has_hdwr_sar(struct powerdomain *pwrdm);
 int pwrdm_wait_transition(struct powerdomain *pwrdm);
 
 int pwrdm_state_switch(struct powerdomain *pwrdm);
-int pwrdm_pre_transition(void);
-int pwrdm_post_transition(void);
+int pwrdm_pre_transition(struct powerdomain *pwrdm);
+int pwrdm_post_transition(struct powerdomain *pwrdm);
 
 void pwrdm_clkdm_enable(struct powerdomain *pwrdm);
 void pwrdm_clkdm_disable(struct powerdomain *pwrdm);
-- 
1.7.1

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Shilimkar, Santosh]

On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
> operations done within cpuidle to do Powerdomain level book-keeping to know
> what state transitions for different Powerdomains have been triggered.
> This is also useful to do a restore-on-demand in some cases when we know
> the context for the given Powerdomain was lost etc.
>
> Now that we have definitive entry/exit points (thanks to the Powerdomain
> level usecounting) for Powerdomain transitions, these book-keeping functions
> can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
> clkdm_disable() functions.
>
> Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
> and get rid of the original ones which iterate over all powerdomains.
>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 ++--
>  arch/arm/mach-omap2/pm34xx.c              |    4 ++--
>  arch/arm/mach-omap2/powerdomain.c         |   28 ++++++++--------------------
>  arch/arm/mach-omap2/powerdomain.h         |    4 ++--
>  4 files changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> index 13670aa..ea19439 100644
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -255,7 +255,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
>                 return -ENXIO;
>         }
>
> -       pwrdm_pre_transition();
> +       pwrdm_cpu_idle();
>
Glad to see this is getting optimized.
I haven't seen how "pwrdm_cpu_[idle/wakeup]()" is
implemented but will those work on SMP system ?
I mean OMAP4, any CPU can make this call ?

Regards
Santosh

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Rajendra Nayak]

On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
> On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>  wrote:
>> pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
>> operations done within cpuidle to do Powerdomain level book-keeping to know
>> what state transitions for different Powerdomains have been triggered.
>> This is also useful to do a restore-on-demand in some cases when we know
>> the context for the given Powerdomain was lost etc.
>>
>> Now that we have definitive entry/exit points (thanks to the Powerdomain
>> level usecounting) for Powerdomain transitions, these book-keeping functions
>> can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
>> clkdm_disable() functions.
>>
>> Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
>> and get rid of the original ones which iterate over all powerdomains.
>>
>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>> ---
>>   arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 ++--
>>   arch/arm/mach-omap2/pm34xx.c              |    4 ++--
>>   arch/arm/mach-omap2/powerdomain.c         |   28 ++++++++--------------------
>>   arch/arm/mach-omap2/powerdomain.h         |    4 ++--
>>   4 files changed, 14 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
>> index 13670aa..ea19439 100644
>> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
>> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
>> @@ -255,7 +255,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
>>                  return -ENXIO;
>>          }
>>
>> -       pwrdm_pre_transition();
>> +       pwrdm_cpu_idle();
>>
> Glad to see this is getting optimized.
> I haven't seen how "pwrdm_cpu_[idle/wakeup]()" is
> implemented but will those work on SMP system ?
> I mean OMAP4, any CPU can make this call ?

Thats a good question. I think Tero did this so he can kick in
voltage transitions at the right time in idle/suspend.
Given that these deal with incrementing/decrementing the MPU and CORE
pwrdm usecounts alone, maybe on OMAP4 (SMP systems) this needs to also
increment/decrement the specific CPU usecounts on the CPUs these calls
are made.

>
> Regards
> Santosh

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Tero Kristo]

On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
> On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
> > On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>  wrote:
> >> pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
> >> operations done within cpuidle to do Powerdomain level book-keeping to know
> >> what state transitions for different Powerdomains have been triggered.
> >> This is also useful to do a restore-on-demand in some cases when we know
> >> the context for the given Powerdomain was lost etc.
> >>
> >> Now that we have definitive entry/exit points (thanks to the Powerdomain
> >> level usecounting) for Powerdomain transitions, these book-keeping functions
> >> can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
> >> clkdm_disable() functions.
> >>
> >> Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
> >> and get rid of the original ones which iterate over all powerdomains.
> >>
> >> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> >> ---
> >>   arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 ++--
> >>   arch/arm/mach-omap2/pm34xx.c              |    4 ++--
> >>   arch/arm/mach-omap2/powerdomain.c         |   28 ++++++++--------------------
> >>   arch/arm/mach-omap2/powerdomain.h         |    4 ++--
> >>   4 files changed, 14 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> >> index 13670aa..ea19439 100644
> >> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> >> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> >> @@ -255,7 +255,7 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
> >>                  return -ENXIO;
> >>          }
> >>
> >> -       pwrdm_pre_transition();
> >> +       pwrdm_cpu_idle();
> >>
> > Glad to see this is getting optimized.
> > I haven't seen how "pwrdm_cpu_[idle/wakeup]()" is
> > implemented but will those work on SMP system ?
> > I mean OMAP4, any CPU can make this call ?
> 
> Thats a good question. I think Tero did this so he can kick in
> voltage transitions at the right time in idle/suspend.
> Given that these deal with incrementing/decrementing the MPU and CORE
> pwrdm usecounts alone, maybe on OMAP4 (SMP systems) this needs to also
> increment/decrement the specific CPU usecounts on the CPUs these calls
> are made.

Yeah, you should keep the usecounts valid by each cpu separately calling
these functions. My current set only sets these usecounts based on cpu0
activity, as cpu1 is statically controlled through cpu online / offline.
Once per-cpu cpuidle is in, these should be changed so that each
individual cpu increases the usecounts when they are brought up,
decrease/increase during idle, and decrease when they are brought down.
The usecount should always reflect the number of CPUs active on MPU
domain.

-Tero

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Shilimkar, Santosh]

On Fri, Jul 20, 2012 at 2:21 PM, Tero Kristo <t-kristo@ti.com> wrote:
>
> On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
> > On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
> > > On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>
> > > wrote:
> > >> pwrdm_pre_transition()/pwrdm_post_transition() have always been high
> > >> latency
> > >> operations done within cpuidle to do Powerdomain level book-keeping
> > >> to know
> > >> what state transitions for different Powerdomains have been
> > >> triggered.
> > >> This is also useful to do a restore-on-demand in some cases when we
> > >> know
> > >> the context for the given Powerdomain was lost etc.
> > >>
> > >> Now that we have definitive entry/exit points (thanks to the
> > >> Powerdomain
> > >> level usecounting) for Powerdomain transitions, these book-keeping
> > >> functions
> > >> can very well be moved from within CPUidle into
> > >> pwrdm_clkdm_enable()/pwrdm_
> > >> clkdm_disable() functions.
> > >>
> > >> Also rename _pwrdm_pre/post_transition_cb() to
> > >> pwrdm_pre/post_transition()
> > >> and get rid of the original ones which iterate over all powerdomains.
> > >>
> > >> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> > >> ---
> > >>   arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 ++--
> > >>   arch/arm/mach-omap2/pm34xx.c              |    4 ++--
> > >>   arch/arm/mach-omap2/powerdomain.c         |   28
> > >> ++++++++--------------------
> > >>   arch/arm/mach-omap2/powerdomain.h         |    4 ++--
> > >>   4 files changed, 14 insertions(+), 26 deletions(-)
> > >>
> > >> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> > >> b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> > >> index 13670aa..ea19439 100644
> > >> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> > >> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> > >> @@ -255,7 +255,7 @@ int omap4_enter_lowpower(unsigned int cpu,
> > >> unsigned int power_state)
> > >>                  return -ENXIO;
> > >>          }
> > >>
> > >> -       pwrdm_pre_transition();
> > >> +       pwrdm_cpu_idle();
> > >>
> > > Glad to see this is getting optimized.
> > > I haven't seen how "pwrdm_cpu_[idle/wakeup]()" is
> > > implemented but will those work on SMP system ?
> > > I mean OMAP4, any CPU can make this call ?
> >
> > Thats a good question. I think Tero did this so he can kick in
> > voltage transitions at the right time in idle/suspend.
> > Given that these deal with incrementing/decrementing the MPU and CORE
> > pwrdm usecounts alone, maybe on OMAP4 (SMP systems) this needs to also
> > increment/decrement the specific CPU usecounts on the CPUs these calls
> > are made.
>
> Yeah, you should keep the usecounts valid by each cpu separately calling
> these functions. My current set only sets these usecounts based on cpu0
> activity, as cpu1 is statically controlled through cpu online / offline.
> Once per-cpu cpuidle is in, these should be changed so that each
> individual cpu increases the usecounts when they are brought up,
> decrease/increase during idle, and decrease when they are brought down.
> The usecount should always reflect the number of CPUs active on MPU
> domain.
>
Sounds good to me !!

Regards
santosh

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Kevin Hilman]

Tero Kristo <t-kristo@ti.com> writes:

> On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
>> On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
>> > On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>  wrote:
>> >> pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
>> >> operations done within cpuidle to do Powerdomain level book-keeping to know
>> >> what state transitions for different Powerdomains have been triggered.
>> >> This is also useful to do a restore-on-demand in some cases when we know
>> >> the context for the given Powerdomain was lost etc.
>> >>
>> >> Now that we have definitive entry/exit points (thanks to the Powerdomain
>> >> level usecounting) for Powerdomain transitions, these book-keeping functions
>> >> can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
>> >> clkdm_disable() functions.
>> >>
>> >> Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
>> >> and get rid of the original ones which iterate over all powerdomains.
>> >>
>> >> Signed-off-by: Rajendra Nayak<rnayak@ti.com>

This is excellent!   Thanks for working on this.

However, it needs a rebase against mainline though because I merged a
set of optimizations[1] to this code already that only calls pre/post
per-pwrdm.

[...]

>> > Glad to see this is getting optimized.
>> > I haven't seen how "pwrdm_cpu_[idle/wakeup]()" is
>> > implemented but will those work on SMP system ?
>> > I mean OMAP4, any CPU can make this call ?
>> 
>> Thats a good question. I think Tero did this so he can kick in
>> voltage transitions at the right time in idle/suspend.
>> Given that these deal with incrementing/decrementing the MPU and CORE
>> pwrdm usecounts alone, maybe on OMAP4 (SMP systems) this needs to also
>> increment/decrement the specific CPU usecounts on the CPUs these calls
>> are made.
>
> Yeah, you should keep the usecounts valid by each cpu separately calling
> these functions. My current set only sets these usecounts based on cpu0
> activity, as cpu1 is statically controlled through cpu online / offline.
> Once per-cpu cpuidle is in, these should be changed so that each
> individual cpu increases the usecounts when they are brought up,
> decrease/increase during idle, and decrease when they are brought down.
> The usecount should always reflect the number of CPUs active on MPU
> domain.

Coupled CPUidle is merging for v3.6 (hopefully OMAP support too), so
this should be addressed sooner rather than later.

Kevin

[1] Specifically, see:
commit 58f0829b7186150318c79515f0e0850c5e7a9c89
Author: Kevin Hilman <khilman@ti.com>
Date:   Fri May 11 15:47:17 2012 -0700

    ARM: OMAP3: PM: call pre/post transition per powerdomain
    
    We only need to call the pre/post transtion methods when we know the
    power state is changing.  First, split up the pre/post transition
    calls to be per-powerdomain, and then make them conditional on whether
    the power domain is actually changing states.
    
    Reviewed-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
    Tested-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
    Tested-by: Grazvydas Ignotas <notasas@gmail.com>
    Signed-off-by: Kevin Hilman <khilman@ti.com>

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Rajendra Nayak]

On Thursday 26 July 2012 04:13 AM, Kevin Hilman wrote:
> Tero Kristo<t-kristo@ti.com>  writes:
>
>> >  On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
>>> >>  On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
>>>> >>  >  On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>   wrote:
>>>>> >>  >>  pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
>>>>> >>  >>  operations done within cpuidle to do Powerdomain level book-keeping to know
>>>>> >>  >>  what state transitions for different Powerdomains have been triggered.
>>>>> >>  >>  This is also useful to do a restore-on-demand in some cases when we know
>>>>> >>  >>  the context for the given Powerdomain was lost etc.
>>>>> >>  >>
>>>>> >>  >>  Now that we have definitive entry/exit points (thanks to the Powerdomain
>>>>> >>  >>  level usecounting) for Powerdomain transitions, these book-keeping functions
>>>>> >>  >>  can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
>>>>> >>  >>  clkdm_disable() functions.
>>>>> >>  >>
>>>>> >>  >>  Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
>>>>> >>  >>  and get rid of the original ones which iterate over all powerdomains.
>>>>> >>  >>
>>>>> >>  >>  Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> This is excellent!   Thanks for working on this.
>
> However, it needs a rebase against mainline though because I merged a
> set of optimizations[1] to this code already that only calls pre/post
> per-pwrdm.

Sure Kevin, I'll repost this series once 3.6-rc1 is out.

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Rajendra Nayak]

On Thursday 26 July 2012 04:13 AM, Kevin Hilman wrote:
> Tero Kristo<t-kristo@ti.com>  writes:
>
>> On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
>>> On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
>>>> On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>   wrote:
>>>>> pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
>>>>> operations done within cpuidle to do Powerdomain level book-keeping to know
>>>>> what state transitions for different Powerdomains have been triggered.
>>>>> This is also useful to do a restore-on-demand in some cases when we know
>>>>> the context for the given Powerdomain was lost etc.
>>>>>
>>>>> Now that we have definitive entry/exit points (thanks to the Powerdomain
>>>>> level usecounting) for Powerdomain transitions, these book-keeping functions
>>>>> can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
>>>>> clkdm_disable() functions.
>>>>>
>>>>> Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
>>>>> and get rid of the original ones which iterate over all powerdomains.
>>>>>
>>>>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>
> This is excellent!   Thanks for working on this.
>
> However, it needs a rebase against mainline though because I merged a
> set of optimizations[1] to this code already that only calls pre/post
> per-pwrdm.
>

Hi Kevin,

I thought some more on this patch, and I think this way of collecting
stats and knowing what state transitions the powerdomains been through
will not work on OMAP3, mainly because of the autodeps. Might work on
OMAP4 and beyond which do not need any autodeps.

Here why I think so,
Lets assume a Powerdomain X with a last module Y active, once Y disables
the last clock (lets assume no hardware controlled clocks for
simplicity), we clear the last power state register for X. However
due to autodeps X does not transition to a target state immediately.
It only does so when the MPU (and IVA) go down, and because
of the wakeup dependency (autodeps set a sleep and a wakeup dep with
both MPU and IVA) is also woken up every time MPU or IVA are up.
So its quite possible that X transitions in and out of sleep multiple
times before Y decides to re-enable its clocks, which is when we end up
looking for the last power state entered.
Lets say X entered OFF 3 times in between Y disabling and re-enabling
its clocks. Though we end up updating the counter only once (instead of
3) we still know and can tell Y that it lost context.
The problem however arises if for some reason X entered OFF
twice and happened to stay ON the third time the dependencies were met.
When Y re-enables its clocks, we end up telling it that it *did not*
lose context because we see the previous power state was ON.

I think as long as we have autodeps, the only way on OMAP3 to accurately
do this is to do it for all dependent domains in CPUIdle :(

regards,
Rajendra

PS: In theory maybe there is still a possibility to miss some domain 
transitions even with the current way of doing it in CPUidle. Its quite
possible that while MPU is asleep, IVA goes in and out of sleep multiple
times causing all dependent domains to also wakeup and sleep :-)

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Kevin Hilman]

Rajendra Nayak <rnayak@ti.com> writes:

> On Thursday 26 July 2012 04:13 AM, Kevin Hilman wrote:
>> Tero Kristo<t-kristo@ti.com>  writes:
>>
>>> On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
>>>> On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
>>>>> On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>   wrote:
>>>>>> pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
>>>>>> operations done within cpuidle to do Powerdomain level book-keeping to know
>>>>>> what state transitions for different Powerdomains have been triggered.
>>>>>> This is also useful to do a restore-on-demand in some cases when we know
>>>>>> the context for the given Powerdomain was lost etc.
>>>>>>
>>>>>> Now that we have definitive entry/exit points (thanks to the Powerdomain
>>>>>> level usecounting) for Powerdomain transitions, these book-keeping functions
>>>>>> can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
>>>>>> clkdm_disable() functions.
>>>>>>
>>>>>> Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
>>>>>> and get rid of the original ones which iterate over all powerdomains.
>>>>>>
>>>>>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
>>
>> This is excellent!   Thanks for working on this.
>>
>> However, it needs a rebase against mainline though because I merged a
>> set of optimizations[1] to this code already that only calls pre/post
>> per-pwrdm.
>>
>
> Hi Kevin,
>
> I thought some more on this patch, and I think this way of collecting
> stats and knowing what state transitions the powerdomains been through
> will not work on OMAP3, mainly because of the autodeps. Might work on
> OMAP4 and beyond which do not need any autodeps.
>
> Here why I think so,
> Lets assume a Powerdomain X with a last module Y active, once Y disables
> the last clock (lets assume no hardware controlled clocks for
> simplicity), we clear the last power state register for X. However
> due to autodeps X does not transition to a target state immediately.
> It only does so when the MPU (and IVA) go down, and because
> of the wakeup dependency (autodeps set a sleep and a wakeup dep with
> both MPU and IVA) is also woken up every time MPU or IVA are up.
> So its quite possible that X transitions in and out of sleep multiple
> times before Y decides to re-enable its clocks, which is when we end up
> looking for the last power state entered.
> Lets say X entered OFF 3 times in between Y disabling and re-enabling
> its clocks. Though we end up updating the counter only once (instead of
> 3) we still know and can tell Y that it lost context.
> The problem however arises if for some reason X entered OFF
> twice and happened to stay ON the third time the dependencies were met.
> When Y re-enables its clocks, we end up telling it that it *did not*
> lose context because we see the previous power state was ON.

Yeah, this is definitely a problem.

As long as we have autodeps, everything is centralized around CPU
transitions anyways, so it makes sense to keep the accounting
centralized too.

> I think as long as we have autodeps, the only way on OMAP3 to accurately
> do this is to do it for all dependent domains in CPUIdle :(

Or, to get rid of autodeps. ;)

Kevin

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Tero Kristo]

On Thu, 2012-07-26 at 10:44 -0700, Kevin Hilman wrote:
> Rajendra Nayak <rnayak@ti.com> writes:
> 
> > On Thursday 26 July 2012 04:13 AM, Kevin Hilman wrote:
> >> Tero Kristo<t-kristo@ti.com>  writes:
> >>
> >>> On Fri, 2012-07-20 at 13:38 +0530, Rajendra Nayak wrote:
> >>>> On Friday 20 July 2012 12:55 PM, Shilimkar, Santosh wrote:
> >>>>> On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak<rnayak@ti.com>   wrote:
> >>>>>> pwrdm_pre_transition()/pwrdm_post_transition() have always been high latency
> >>>>>> operations done within cpuidle to do Powerdomain level book-keeping to know
> >>>>>> what state transitions for different Powerdomains have been triggered.
> >>>>>> This is also useful to do a restore-on-demand in some cases when we know
> >>>>>> the context for the given Powerdomain was lost etc.
> >>>>>>
> >>>>>> Now that we have definitive entry/exit points (thanks to the Powerdomain
> >>>>>> level usecounting) for Powerdomain transitions, these book-keeping functions
> >>>>>> can very well be moved from within CPUidle into pwrdm_clkdm_enable()/pwrdm_
> >>>>>> clkdm_disable() functions.
> >>>>>>
> >>>>>> Also rename _pwrdm_pre/post_transition_cb() to pwrdm_pre/post_transition()
> >>>>>> and get rid of the original ones which iterate over all powerdomains.
> >>>>>>
> >>>>>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
> >>
> >> This is excellent!   Thanks for working on this.
> >>
> >> However, it needs a rebase against mainline though because I merged a
> >> set of optimizations[1] to this code already that only calls pre/post
> >> per-pwrdm.
> >>
> >
> > Hi Kevin,
> >
> > I thought some more on this patch, and I think this way of collecting
> > stats and knowing what state transitions the powerdomains been through
> > will not work on OMAP3, mainly because of the autodeps. Might work on
> > OMAP4 and beyond which do not need any autodeps.
> >
> > Here why I think so,
> > Lets assume a Powerdomain X with a last module Y active, once Y disables
> > the last clock (lets assume no hardware controlled clocks for
> > simplicity), we clear the last power state register for X. However
> > due to autodeps X does not transition to a target state immediately.
> > It only does so when the MPU (and IVA) go down, and because
> > of the wakeup dependency (autodeps set a sleep and a wakeup dep with
> > both MPU and IVA) is also woken up every time MPU or IVA are up.
> > So its quite possible that X transitions in and out of sleep multiple
> > times before Y decides to re-enable its clocks, which is when we end up
> > looking for the last power state entered.
> > Lets say X entered OFF 3 times in between Y disabling and re-enabling
> > its clocks. Though we end up updating the counter only once (instead of
> > 3) we still know and can tell Y that it lost context.
> > The problem however arises if for some reason X entered OFF
> > twice and happened to stay ON the third time the dependencies were met.
> > When Y re-enables its clocks, we end up telling it that it *did not*
> > lose context because we see the previous power state was ON.
> 
> Yeah, this is definitely a problem.
> 
> As long as we have autodeps, everything is centralized around CPU
> transitions anyways, so it makes sense to keep the accounting
> centralized too.
> 
> > I think as long as we have autodeps, the only way on OMAP3 to accurately
> > do this is to do it for all dependent domains in CPUIdle :(
> 
> Or, to get rid of autodeps. ;)

Whats the reason for having them anyway? Some of the wakedeps make sense
(per domain due to hw bugs) but sleepdeps...?

-Tero

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Paul Walmsley]

On Thu, 26 Jul 2012, Tero Kristo wrote:

> On Thu, 2012-07-26 at 10:44 -0700, Kevin Hilman wrote:
> 
> > Or, to get rid of autodeps. ;)
> 
> Whats the reason for having them anyway?

No one has yet posted tested patches to convert OMAP2/3 to use the IP 
block-based clockdomain enable sequence.  A few folks posted some patches 
for this in 2010 but the patches didn't boot.

It should be a little easier to try this now since we have more data 
defined now than we did in the past.

> Some of the wakedeps make sense (per domain due to hw bugs) but 
> sleepdeps...?

That sounds like a different issue.


- Paul

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Rajendra Nayak]

Hi Tero,

On Thursday 26 July 2012 11:57 PM, Tero Kristo wrote:
>> Yeah, this is definitely a problem.
>> >
>> >  As long as we have autodeps, everything is centralized around CPU
>> >  transitions anyways, so it makes sense to keep the accounting
>> >  centralized too.
>> >
>>> >  >  I think as long as we have autodeps, the only way on OMAP3 to accurately
>>> >  >  do this is to do it for all dependent domains in CPUIdle:(
>> >
>> >  Or, to get rid of autodeps.;)
> Whats the reason for having them anyway? Some of the wakedeps make sense
> (per domain due to hw bugs) but sleepdeps...?
>

FWIK, the main problem is with modules with clocks under hardware
control. Once a slave module isn't functional and there are no
outstanding OCP accesses pending, the module when sent an IDLEREQ
by PRCM responds with an IDLEACK causing the clkdm to transition
to INACTIVE. A driver which is active can still in the meantime
cause a register access to the module, but the register access
does not act as an event which makes PRCM de-assert IDLEREQ causing
the module to unidle. This causes problems. So as long as there is
a possibility of some code doing a register access on the module
we need to keep it from idling. IIRC the issues we saw on OMAP3
were mostly around PER domain and I think with GPIO in particular.

The problem might be limited to modules with _only_ hardware controlled
clocks (iclks) like GPIO. For others which have an iclk and fclk, we can
always keep the autodeps while fclks are requested and get rid of them
when fclks are disabled. This is exactly what clkdm_clk_enable/disable
functions do, but given in the current mainline even iclks account
for usecounting, a clkdms usecount would never hit 0 causing 
clkdm_clk_disable never to be called. (On clkdms which have atleast one
iclk under hardware control).
hwmod keeps all iclks always enabled and under hardware control unless
the OCP interface has a 'OCPIF_SWSUP_IDLE' flag set.

But now with your series, which does not account iclks for usecounting,
I would expect this to be fixed. I was expecting for modules with both
fclk and iclk, as long as the driver has done a pm_runtime_get_sync()
or equivalent the autodeps would be set, and once the driver does a
pm_runtime_put_sync() the autodeps would be removed. This would also
be the same time we clear the Powerdomains previous power state register
and the Powerdomain should ideally immediately transition on not go in
and out with every MPU transition.
So this kind of rules out the problems that my theory above was
suggesting we would have with the $subject patch.
We still have to deal with the iclk only modules like GPIO though.

However a quick test with just your latest usecounting series (without
any of my RFC patches) seems to make me think I am still missing
something.

If you see the counts below for usbhost and dss, they both seem to
go in and out of RET with every MPU transition. Which means the
dependencies are still set.

# cat /debug/pm_debug/count
usbhost_pwrdm 
(ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
core_pwrdm 
(ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
per_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
dss_pwrdm (ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
neon_pwrdm (ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0
mpu_pwrdm (ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
iva2_pwrdm 
(RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RET-MEMBANK4-OFF:0

However if I look at the dss registers, I don;t see any fclks are
enabled.

# ./readmem 0x48004E00
0x00000000 <- All FCLK disabled.

# ./readmem 0x48004E10
0x00000001 <- ICLK enabled

# ./readmem 0x48004E44
0x00000006 <- dependencies are set with MPU and IVA

# ./readmem 0x48004E48
0x00000003 <- clkdm is under HWSUP.

Any idea why this could be happening?

regards,
Rajendra

[RFC 2/4] ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle

[Rajendra Nayak]

Hi Tero,

On Friday 27 July 2012 12:16 PM, Rajendra Nayak wrote:
> However a quick test with just your latest usecounting series (without
> any of my RFC patches) seems to make me think I am still missing
> something.
>
> If you see the counts below for usbhost and dss, they both seem to
> go in and out of RET with every MPU transition. Which means the
> dependencies are still set.
>
> # cat /debug/pm_debug/count
> usbhost_pwrdm
> (ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
> sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
> core_pwrdm
> (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0
>
> per_pwrdm (ON),OFF:0,RET:0,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
> dss_pwrdm
> (ON),OFF:0,RET:138,INA:0,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
> cam_pwrdm (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
> neon_pwrdm (ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0
> mpu_pwrdm
> (ON),OFF:0,RET:132,INA:6,ON:139,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0
> iva2_pwrdm
> (RET),OFF:0,RET:1,INA:0,ON:1,RET-LOGIC-OFF:0,RET-MEMBANK1-OFF:0,RET-MEMBANK2-OFF:0,RET-MEMBANK3-OFF:0,RET-MEMBANK4-OFF:0
>
>
> However if I look at the dss registers, I don;t see any fclks are
> enabled.
>
> # ./readmem 0x48004E00
> 0x00000000 <- All FCLK disabled.
>
> # ./readmem 0x48004E10
> 0x00000001 <- ICLK enabled
>
> # ./readmem 0x48004E44
> 0x00000006 <- dependencies are set with MPU and IVA
>
> # ./readmem 0x48004E48
> 0x00000003 <- clkdm is under HWSUP.
>
> Any idea why this could be happening?

I think I know what the problem is now.

omap3_pm_init calls clkdm_allow_idle for all clkdms, while autoidle on
all iclks is still disabled. This causes autodeps to be set as the iclks
are accounted for in the usecount.
(hwmod would have done a clk_enable() on the iclks and left them enabled
as OCPIF_SWSUP_IDLE isn't set)

static void omap3_clkdm_allow_idle(struct clockdomain *clkdm)
{
         if (atomic_read(&clkdm->usecount) > 0)
                 _clkdm_add_autodeps(clkdm);

         omap3xxx_cm_clkdm_enable_hwsup(clkdm->pwrdm.ptr->prcm_offs,
                                 clkdm->clktrctrl_mask);
}

A little later after omap3_pm_init, we enable autoidle for all iclks.
omap2_clkt_iclk_allow_idle decrements the usecount, but leaves the
autodeps still set.

This seems to be causing the dss and usb to also transition along with
MPU.

We will need some way to also clear and set autodeps in
omap2_clkt_iclk_allow_idle/deny_idle.

regards,
Rajendra

[RFC 3/4] ARM: OMAP: powerdomain: Add .power_on/.power_down hooks for powerdomains

[Rajendra Nayak]

With usecounting added to Powerdomains, its now possible to track Powerdomain
transitions, including when a Powerdomain is turned on and turned down.
Add support for hooking up callbacks for such events, which can be used to
do things like context save/restore etc. which today is mostly stuffed within
CPUidle and Suspend code.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/powerdomain.c |   12 +++++++++++-
 arch/arm/mach-omap2/powerdomain.h |    4 ++++
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index f435819..d2c096c 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -992,10 +992,16 @@ int pwrdm_state_switch(struct powerdomain *pwrdm)
  */
 void pwrdm_clkdm_enable(struct powerdomain *pwrdm)
 {
+	int prev_state;
+
 	if (!pwrdm)
 		return;
 
 	if (atomic_inc_return(&pwrdm->usecount) == 1) {
+		if (pwrdm->power_on) {
+			prev_state = pwrdm_read_prev_pwrst(pwrdm);
+			pwrdm->power_on(pwrdm, prev_state);
+		}
 		pwrdm_post_transition(pwrdm);
 		voltdm_pwrdm_enable(pwrdm->voltdm.ptr);
 	}
@@ -1010,7 +1016,7 @@ void pwrdm_clkdm_enable(struct powerdomain *pwrdm)
  */
 void pwrdm_clkdm_disable(struct powerdomain *pwrdm)
 {
-	int val;
+	int val, next_state;
 
 	if (!pwrdm)
 		return;
@@ -1020,6 +1026,10 @@ void pwrdm_clkdm_disable(struct powerdomain *pwrdm)
 	if (!val) {
 		voltdm_pwrdm_disable(pwrdm->voltdm.ptr);
 		pwrdm_pre_transition(pwrdm);
+		if (pwrdm->power_down) {
+			next_state = pwrdm_read_next_pwrst(pwrdm);
+			pwrdm->power_down(pwrdm, next_state);
+		}
 	}
 
 	BUG_ON(val < 0);
diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
index 52a99cf..a6e09cf 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -92,6 +92,8 @@ struct powerdomain;
  * @pwrdm_clkdms: Clockdomains in this powerdomain
  * @node: list_head linking all powerdomains
  * @voltdm_node: list_head linking all powerdomains in a voltagedomain
+ * @power_on: callback function before powerdomain is turned on
+ * @power_down: callback function after powerdomain is turned down
  * @state:
  * @state_counter:
  * @timer:
@@ -121,6 +123,8 @@ struct powerdomain {
 	unsigned ret_logic_off_counter;
 	unsigned ret_mem_off_counter[PWRDM_MAX_MEM_BANKS];
 	atomic_t usecount;
+	int (*power_on)(struct powerdomain *pwrdm, int prev_state);
+	int (*power_down)(struct powerdomain *pwrdm, int next_state);
 
 #ifdef CONFIG_PM_DEBUG
 	s64 timer;
-- 
1.7.1

[RFC 3/4] ARM: OMAP: powerdomain: Add .power_on/.power_down hooks for powerdomains

[Shilimkar, Santosh]

On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> With usecounting added to Powerdomains, its now possible to track Powerdomain
> transitions, including when a Powerdomain is turned on and turned down.
> Add support for hooking up callbacks for such events, which can be used to
> do things like context save/restore etc. which today is mostly stuffed within
> CPUidle and Suspend code.
>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
Agree.
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[RFC 4/4] ARM: OMAP3: PM: Use .power_on/.power_down to clean omap_sram_idle

[Rajendra Nayak]

With .power_on/.power_down hooks now available within the powerdomain
framework, its possible to clean a lot of mpu/core power_on/power_down
related code within omap_sram_idle into respective callbacks for these
powerdomains.

This should atleast also optimise the core domain context save
in CPUidle when attempting deeper C states, to prevent a core
domain context save when a device in core is still active.
The current code in such cases does a blind context save and
tries to put core domain down, even while devices are active.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 arch/arm/mach-omap2/pm34xx.c |  127 +++++++++++++++++++++++-------------------
 1 files changed, 69 insertions(+), 58 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 9bdf53c..f64bcc6 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -266,22 +266,16 @@ static int omap34xx_do_sram_idle(unsigned long save_state)
 
 void omap_sram_idle(void)
 {
-	int mpu_next_state = PWRDM_POWER_ON;
-	int per_next_state = PWRDM_POWER_ON;
-	int core_next_state = PWRDM_POWER_ON;
+	int mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
+	int per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
+	int core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
 	int per_going_off;
-	int core_prev_state;
-	u32 sdrc_pwr = 0;
-
-	mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
 
 	/* NEON control */
 	if (pwrdm_read_pwrst(neon_pwrdm) == PWRDM_POWER_ON)
 		pwrdm_set_next_pwrst(neon_pwrdm, mpu_next_state);
 
 	/* Enable IO-PAD and IO-CHAIN wakeups */
-	per_next_state = pwrdm_read_next_pwrst(per_pwrdm);
-	core_next_state = pwrdm_read_next_pwrst(core_pwrdm);
 	if (omap3_has_io_wakeup() &&
 	    (per_next_state < PWRDM_POWER_ON ||
 	     core_next_state < PWRDM_POWER_ON)) {
@@ -298,62 +292,15 @@ void omap_sram_idle(void)
 		omap2_gpio_prepare_for_idle(per_going_off);
 	}
 
-	/* CORE */
-	if (core_next_state < PWRDM_POWER_ON) {
-		if (core_next_state == PWRDM_POWER_OFF) {
-			omap3_core_save_context();
-			omap3_cm_save_context();
-		}
-	}
-
-	omap3_intc_prepare_idle();
-
-	/*
-	 * On EMU/HS devices ROM code restores a SRDC value
-	 * from scratchpad which has automatic self refresh on timeout
-	 * of AUTO_CNT = 1 enabled. This takes care of erratum ID i443.
-	 * Hence store/restore the SDRC_POWER register here.
-	 */
-	if (cpu_is_omap3430() && omap_rev() >= OMAP3430_REV_ES3_0 &&
-	    (omap_type() == OMAP2_DEVICE_TYPE_EMU ||
-	     omap_type() == OMAP2_DEVICE_TYPE_SEC) &&
-	    core_next_state == PWRDM_POWER_OFF)
-		sdrc_pwr = sdrc_read_reg(SDRC_POWER);
-
 	/*
 	 * omap3_arm_context is the location where some ARM context
 	 * get saved. The rest is placed on the stack, and restored
 	 * from there before resuming.
 	 */
-	if (mpu_next_state == PWRDM_POWER_OFF) {
-		omap34xx_save_context(omap3_arm_context);
+	if (mpu_next_state == PWRDM_POWER_OFF)
 		cpu_suspend(1, omap34xx_do_sram_idle);
-	} else {
+	else
 		omap34xx_do_sram_idle(0);
-	}
-
-	/* Restore normal SDRC POWER settings */
-	if (cpu_is_omap3430() && omap_rev() >= OMAP3430_REV_ES3_0 &&
-	    (omap_type() == OMAP2_DEVICE_TYPE_EMU ||
-	     omap_type() == OMAP2_DEVICE_TYPE_SEC) &&
-	    core_next_state == PWRDM_POWER_OFF)
-		sdrc_write_reg(sdrc_pwr, SDRC_POWER);
-
-	/* CORE */
-	if (core_next_state < PWRDM_POWER_ON) {
-		core_prev_state = pwrdm_read_prev_pwrst(core_pwrdm);
-		if (core_prev_state == PWRDM_POWER_OFF) {
-			omap3_core_restore_context();
-			omap3_cm_restore_context();
-			omap3_sram_restore_context();
-			omap2_sms_restore_context();
-		}
-		if (core_next_state == PWRDM_POWER_OFF)
-			omap2_prm_clear_mod_reg_bits(OMAP3430_AUTO_OFF_MASK,
-					       OMAP3430_GR_MOD,
-					       OMAP3_PRM_VOLTCTRL_OFFSET);
-	}
-	omap3_intc_resume_idle();
 
 	pwrdm_cpu_wakeup();
 
@@ -374,6 +321,64 @@ void omap_sram_idle(void)
 	clkdm_allow_idle(mpu_pwrdm->pwrdm_clkdms[0]);
 }
 
+int mpu_power_down(struct powerdomain *pwrdm, int next_state)
+{
+	omap3_intc_prepare_idle();
+	if (next_state == PWRDM_POWER_OFF)
+		omap34xx_save_context(omap3_arm_context);
+	return 0;
+}
+
+int mpu_power_on(struct powerdomain *pwrdm, int prev_state)
+{
+	omap3_intc_resume_idle();
+	return 0;
+}
+
+u32 sdrc_pwr;
+int core_attempted_state;
+int core_power_down(struct powerdomain *pwrdm, int next_state)
+{
+	if (next_state == PWRDM_POWER_OFF) {
+		omap3_core_save_context();
+		omap3_cm_save_context();
+
+		/*
+		 * On EMU/HS devices ROM code restores a SRDC value
+		 * from scratchpad which has automatic self refresh on timeout
+		 * of AUTO_CNT = 1 enabled. This takes care of erratum ID i443.
+		 * Hence store/restore the SDRC_POWER register here.
+		 */
+		if (cpu_is_omap3430() && omap_rev() >= OMAP3430_REV_ES3_0 &&
+		    (omap_type() == OMAP2_DEVICE_TYPE_EMU ||
+		     omap_type() == OMAP2_DEVICE_TYPE_SEC))
+			sdrc_pwr = sdrc_read_reg(SDRC_POWER);
+	}
+	core_attempted_state = next_state;
+	return 0;
+}
+
+int core_power_on(struct powerdomain *pwrdm, int prev_state)
+{
+	if (prev_state == PWRDM_POWER_OFF) {
+		omap3_core_restore_context();
+		omap3_cm_restore_context();
+		omap3_sram_restore_context();
+		omap2_sms_restore_context();
+	}
+	if (core_attempted_state == PWRDM_POWER_OFF) {
+		/* Restore normal SDRC POWER settings */
+		if (cpu_is_omap3430() && omap_rev() >= OMAP3430_REV_ES3_0 &&
+		    (omap_type() == OMAP2_DEVICE_TYPE_EMU ||
+		     omap_type() == OMAP2_DEVICE_TYPE_SEC))
+			sdrc_write_reg(sdrc_pwr, SDRC_POWER);
+		omap2_prm_clear_mod_reg_bits(OMAP3430_AUTO_OFF_MASK,
+				       OMAP3430_GR_MOD,
+				       OMAP3_PRM_VOLTCTRL_OFFSET);
+	}
+	return 0;
+}
+
 static void omap3_pm_idle(void)
 {
 	local_fiq_disable();
@@ -734,6 +739,12 @@ int __init omap3_pm_init(void)
 	core_pwrdm = pwrdm_lookup("core_pwrdm");
 	cam_pwrdm = pwrdm_lookup("cam_pwrdm");
 
+	mpu_pwrdm->power_on = mpu_power_on;
+	mpu_pwrdm->power_down = mpu_power_down;
+
+	core_pwrdm->power_on = core_power_on;
+	core_pwrdm->power_down = core_power_down;
+
 	neon_clkdm = clkdm_lookup("neon_clkdm");
 	mpu_clkdm = clkdm_lookup("mpu_clkdm");
 
-- 
1.7.1

[RFC 4/4] ARM: OMAP3: PM: Use .power_on/.power_down to clean omap_sram_idle

[Shilimkar, Santosh]

On Fri, Jul 20, 2012 at 11:34 AM, Rajendra Nayak <rnayak@ti.com> wrote:
> With .power_on/.power_down hooks now available within the powerdomain
> framework, its possible to clean a lot of mpu/core power_on/power_down
> related code within omap_sram_idle into respective callbacks for these
> powerdomains.
>
> This should atleast also optimise the core domain context save
> in CPUidle when attempting deeper C states, to prevent a core
> domain context save when a device in core is still active.
> The current code in such cases does a blind context save and
> tries to put core domain down, even while devices are active.
>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> ---
Looks more modular now. Thanks Rajendra and this series is
a good step towards decoupling CORE IDLE with CPUIDLE.

Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

Regards
Santosh

[RFC 0/4] OMAP Cpuidle/Suspend Cleanups

[Tero Kristo]

On Fri, 2012-07-20 at 11:34 +0530, Rajendra Nayak wrote:
> Hi,
> 
> Here are some CPUidle/Suspend cleanup patches done by adding
> some more functionality in the OMAP Powerdomain framework.
> They mostly cleanup OMAP3 but the same ideas can be used/
> applied for OMAP4 and beyond.
> 
> The series is based off Teros' series [1] to add usecounting
> support within the OMAP Powerdomain framework.
> 
> The patches are tested with RET/OFF in CPUidle and Suspend
> on 3630 beagle Xm and 3430 SDP.

This set looks good to me, just requires careful testing that you don't
break anything as you are moving lots of stuff around. For debugging
purposes (maybe also permanently?), it might be good to keep some sort
of failsafe mechanism in place that prevents actual core / per off
transitions in case there is a problem with the usecounting, and you
accidentally hit core / per off even though the usecounting doesn't
expect it (some autoidle mechanism is overlooked for example.)

Maybe add some caching for the programmed next state, and only program
the HW register once we get to the callbacks, and restore the next state
to ON after leaving...? This should get rid of any unexpected hangs.

-Tero

> 
> regards,
> Rajendra
> 
> [1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg72163.html
> 
> Rajendra Nayak (4):
>   ARM: OMAP3: cpuidle: Remove unused MPU OSWR support code
>   ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle
>   ARM: OMAP: powerdomain: Add .power_on/.power_down hooks for
>     powerdomains
>   ARM: OMAP3: PM: Use .power_on/.power_down to clean omap_sram_idle
> 
>  arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 +-
>  arch/arm/mach-omap2/pm34xx.c              |  158 ++++++++++++++---------------
>  arch/arm/mach-omap2/powerdomain.c         |   40 ++++----
>  arch/arm/mach-omap2/powerdomain.h         |    8 +-
>  arch/arm/mach-omap2/sleep34xx.S           |    4 +-
>  5 files changed, 104 insertions(+), 110 deletions(-)
> 

[RFC 0/4] OMAP Cpuidle/Suspend Cleanups

[Rajendra Nayak]

On Friday 20 July 2012 02:29 PM, Tero Kristo wrote:
> On Fri, 2012-07-20 at 11:34 +0530, Rajendra Nayak wrote:
>> Hi,
>>
>> Here are some CPUidle/Suspend cleanup patches done by adding
>> some more functionality in the OMAP Powerdomain framework.
>> They mostly cleanup OMAP3 but the same ideas can be used/
>> applied for OMAP4 and beyond.
>>
>> The series is based off Teros' series [1] to add usecounting
>> support within the OMAP Powerdomain framework.
>>
>> The patches are tested with RET/OFF in CPUidle and Suspend
>> on 3630 beagle Xm and 3430 SDP.
>
> This set looks good to me, just requires careful testing that you don't
> break anything as you are moving lots of stuff around. For debugging
> purposes (maybe also permanently?), it might be good to keep some sort
> of failsafe mechanism in place that prevents actual core / per off
> transitions in case there is a problem with the usecounting, and you
> accidentally hit core / per off even though the usecounting doesn't
> expect it (some autoidle mechanism is overlooked for example.)
>
> Maybe add some caching for the programmed next state, and only program
> the HW register once we get to the callbacks, and restore the next state
> to ON after leaving...? This should get rid of any unexpected hangs.ly

Thats exactly what I have been thinking too. The caching is turning out
to be a little tricky though. But something like that would certainly be
needed, and should also get rid of unwanted (multiple) target state
programming.

>
> -Tero
>
>>
>> regards,
>> Rajendra
>>
>> [1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg72163.html
>>
>> Rajendra Nayak (4):
>>    ARM: OMAP3: cpuidle: Remove unused MPU OSWR support code
>>    ARM: OMAP: PM: Get rid of Powerdomain book-keeping from cpuidle
>>    ARM: OMAP: powerdomain: Add .power_on/.power_down hooks for
>>      powerdomains
>>    ARM: OMAP3: PM: Use .power_on/.power_down to clean omap_sram_idle
>>
>>   arch/arm/mach-omap2/omap-mpuss-lowpower.c |    4 +-
>>   arch/arm/mach-omap2/pm34xx.c              |  158 ++++++++++++++---------------
>>   arch/arm/mach-omap2/powerdomain.c         |   40 ++++----
>>   arch/arm/mach-omap2/powerdomain.h         |    8 +-
>>   arch/arm/mach-omap2/sleep34xx.S           |    4 +-
>>   5 files changed, 104 insertions(+), 110 deletions(-)
>>
>
>