[PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements

[PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements

[Paul Walmsley]

This series optimizes some of the powerdomain-related code in
arch/arm/mach-omap2/pm*, and fixes a bug or two.  These were noticed while
working on the functional powerstate code.

Rajendra and Santosh, if you have a spare moment, could you please
peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch?  It
makes sense to me in theory, but you both would probably know better
than I :-)

Kevin, want to take this series (assuming folks are happy with it) ?

Tested dynamic idle with and without CPUIdle, with and without
off-mode, on a Beagleboard 35xx.

This series is available via git at git://git.pwsan.com/linux-2.6 in
the branch "pm_cleanup_3.4".


- Paul

---

pm_cleanup_3.4
   text	   data	    bss	    dec	    hex	filename
6592771	 678492	5590684	12861947	 c441fb	vmlinux.orig
6592771	 678492	5590684	12861947	 c441fb	vmlinux.patched

Paul Walmsley (2):
      ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()
      ARM: OMAP2+: PM: clean up omap_set_pwrdm_state()


 arch/arm/mach-omap2/pm.c     |   39 +++++++++++++++++----------------------
 arch/arm/mach-omap2/pm34xx.c |    5 -----
 2 files changed, 17 insertions(+), 27 deletions(-)

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
pwrdm_pre_transition(), which appears a few lines after these calls,
invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
need to do it twice.

N.B.: some of us have observed that accesses to the previous
powerstate registers seem to be quite slow.  Although the writes
removed by this patch should be buffered by the write buffer, there is
a read to a PRM register immediately afterwards.  That will block the
OMAP3 MPU until all of those writes complete.  So this patch should
result in a minor performance improvement during idle entry.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/pm34xx.c |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index fc69875..463eb6c 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -290,11 +290,6 @@ void omap_sram_idle(void)
 	int core_prev_state, per_prev_state;
 	u32 sdrc_pwr = 0;
 
-	pwrdm_clear_all_prev_pwrst(mpu_pwrdm);
-	pwrdm_clear_all_prev_pwrst(neon_pwrdm);
-	pwrdm_clear_all_prev_pwrst(core_pwrdm);
-	pwrdm_clear_all_prev_pwrst(per_pwrdm);
-
 	mpu_next_state = pwrdm_read_next_pwrst(mpu_pwrdm);
 	switch (mpu_next_state) {
 	case PWRDM_POWER_ON:

[PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state()

[Paul Walmsley]

Clean up a few different parts of omap_set_pwrdm_state():

- Remove a superfluous call to pwrdm_state_switch().  Not needed
  unless LOWPOWERSTATECHANGE is used, because the state switch code is
  called by either clkdm_sleep() or clkdm_allow_idle().

- Add code to wait for the power state transition in the OMAP4+ low
  power state change.  This is speculative, so I would particularly
  appreciate feedback on this part.

- Remove a superfluous call to pwrdm_read_pwrst().

- Update variable names to be more meaningful (hopefully) and precise.

- Fix an error path bug that would not place the clockdomain back into
  hardware-supervised idle or sleep mode if the power state could not
  be programmed.

The documentation for this function still needs major improvements;
that's left for a later patch.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>
Cc: Rajendra Nayak <rnayak@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Tero Kristo <t-kristo@ti.com>
---
 arch/arm/mach-omap2/pm.c |   39 +++++++++++++++++----------------------
 1 files changed, 17 insertions(+), 22 deletions(-)

diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index 1881fe9..137d2d9 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -72,28 +72,27 @@ static void omap2_init_processor_devices(void)
  * This sets pwrdm state (other than mpu & core. Currently only ON &
  * RET are supported.
  */
-int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
+int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 pwrst)
 {
-	u32 cur_state;
-	int sleep_switch = -1;
-	int ret = 0;
-	int hwsup = 0;
+	u8 curr_pwrst, next_pwrst;
+	int sleep_switch = -1, ret = 0, hwsup = 0;
 
-	if (pwrdm == NULL || IS_ERR(pwrdm))
+	if (!pwrdm || IS_ERR(pwrdm))
 		return -EINVAL;
 
-	while (!(pwrdm->pwrsts & (1 << state))) {
-		if (state == PWRDM_POWER_OFF)
+	while (!(pwrdm->pwrsts & (1 << pwrst))) {
+		if (pwrst == PWRDM_POWER_OFF)
 			return ret;
-		state--;
+		pwrst--;
 	}
 
-	cur_state = pwrdm_read_next_pwrst(pwrdm);
-	if (cur_state == state)
+	next_pwrst = pwrdm_read_next_pwrst(pwrdm);
+	if (next_pwrst == pwrst)
 		return ret;
 
-	if (pwrdm_read_pwrst(pwrdm) < PWRDM_POWER_ON) {
-		if ((pwrdm_read_pwrst(pwrdm) > state) &&
+	curr_pwrst = pwrdm_read_pwrst(pwrdm);
+	if (curr_pwrst < PWRDM_POWER_ON) {
+		if ((curr_pwrst > pwrst) &&
 			(pwrdm->flags & PWRDM_HAS_LOWPOWERSTATECHANGE)) {
 			sleep_switch = LOWPOWERSTATE_SWITCH;
 		} else {
@@ -103,12 +102,10 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
 		}
 	}
 
-	ret = pwrdm_set_next_pwrst(pwrdm, state);
-	if (ret) {
-		pr_err("%s: unable to set state of powerdomain: %s\n",
+	ret = pwrdm_set_next_pwrst(pwrdm, pwrst);
+	if (ret)
+		pr_err("%s: unable to set power state of powerdomain: %s\n",
 		       __func__, pwrdm->name);
-		goto err;
-	}
 
 	switch (sleep_switch) {
 	case FORCEWAKEUP_SWITCH:
@@ -119,13 +116,11 @@ int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 state)
 		break;
 	case LOWPOWERSTATE_SWITCH:
 		pwrdm_set_lowpwrstchange(pwrdm);
+		pwrdm_wait_transition(pwrdm);
+		pwrdm_state_switch(pwrdm);
 		break;
-	default:
-		return ret;
 	}
 
-	pwrdm_state_switch(pwrdm);
-err:
 	return ret;
 }
 

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Mon, Jan 30, 2012 at 3:13 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
> pwrdm_pre_transition(), which appears a few lines after these calls,
> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
> need to do it twice.
>
> N.B.: some of us have observed that accesses to the previous
> powerstate registers seem to be quite slow. ?Although the writes
> removed by this patch should be buffered by the write buffer, there is
> a read to a PRM register immediately afterwards. ?That will block the
> OMAP3 MPU until all of those writes complete. ?So this patch should
> result in a minor performance improvement during idle entry.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> ---
Nice clean-up
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state()

[Shilimkar, Santosh]

On Mon, Jan 30, 2012 at 3:13 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Clean up a few different parts of omap_set_pwrdm_state():
>
> - Remove a superfluous call to pwrdm_state_switch(). ?Not needed
> ?unless LOWPOWERSTATECHANGE is used, because the state switch code is
> ?called by either clkdm_sleep() or clkdm_allow_idle().
>
Indeed

> - Add code to wait for the power state transition in the OMAP4+ low
> ?power state change. ?This is speculative, so I would particularly
> ?appreciate feedback on this part.
>
> - Remove a superfluous call to pwrdm_read_pwrst().
>
> - Update variable names to be more meaningful (hopefully) and precise.
>
> - Fix an error path bug that would not place the clockdomain back into
> ?hardware-supervised idle or sleep mode if the power state could not
> ?be programmed.
>
> The documentation for this function still needs major improvements;
> that's left for a later patch.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Cc: Rajendra Nayak <rnayak@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Tero Kristo <t-kristo@ti.com>
> ---

All the changes look fine to me from OMAP4
perspective. Would be good if Tero can try out this
patch and test CORE RET on OMAP4.

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

Regards
santosh

[PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> This series optimizes some of the powerdomain-related code in
> arch/arm/mach-omap2/pm*, and fixes a bug or two.  These were noticed while
> working on the functional powerstate code.
>
> Rajendra and Santosh, if you have a spare moment, could you please
> peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch?  It
> makes sense to me in theory, but you both would probably know better
> than I :-)
>
> Kevin, want to take this series (assuming folks are happy with it) ?

Yes, I'll take this and add the Ack from Santosh.  Thanks for the nice
cleanup.   

Will submit after seeing a Tested-by from someone who can test with CORE
retention/off on OMAP4 (hint, hint,  Tero :)

Kevin

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
> pwrdm_pre_transition(), which appears a few lines after these calls,
> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
> need to do it twice.

It looks like these two for OMAP4 are surpurfluous since the immediately
follow a call to pwrdm_pre_transition() as well.

Santosh/Rajendra, please confirm/ack.

Kevin


diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
index 1d5d010..bbabe1d 100644
--- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
+++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
@@ -263,12 +263,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
 	 * In MPUSS OSWR or device OFF, interrupt controller  contest is lost.
 	 */
 	mpuss_clear_prev_logic_pwrst();
-	pwrdm_clear_all_prev_pwrst(mpuss_pd);
 	if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) &&
 		(pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF))
 		save_state = 2;
 
-	clear_cpu_prev_pwrst(cpu);
 	cpu_clear_prev_logic_pwrst(cpu);
 	set_cpu_next_pwrst(cpu, power_state);
 	set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume));

[PATCH 2/2] ARM: OMAP2+: PM: clean up omap_set_pwrdm_state()

[Rajendra Nayak]

Hi Paul,

On Monday 30 January 2012 03:13 PM, Paul Walmsley wrote:
> Clean up a few different parts of omap_set_pwrdm_state():
>
> - Remove a superfluous call to pwrdm_state_switch().  Not needed
>    unless LOWPOWERSTATECHANGE is used, because the state switch code is
>    called by either clkdm_sleep() or clkdm_allow_idle().
>
> - Add code to wait for the power state transition in the OMAP4+ low
>    power state change.  This is speculative, so I would particularly
>    appreciate feedback on this part.
>
> - Remove a superfluous call to pwrdm_read_pwrst().
>
> - Update variable names to be more meaningful (hopefully) and precise.
>
> - Fix an error path bug that would not place the clockdomain back into
>    hardware-supervised idle or sleep mode if the power state could not
>    be programmed.

All the changes look good. Thanks.
Acked-by: Rajendra Nayak <rnayak@ti.com>

regards,
Rajendra

>

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Rajendra Nayak]

On Tuesday 31 January 2012 05:44 AM, Kevin Hilman wrote:
> Paul Walmsley<paul@pwsan.com>  writes:
>
>> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
>> pwrdm_pre_transition(), which appears a few lines after these calls,
>> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
>> need to do it twice.
>
> It looks like these two for OMAP4 are surpurfluous since the immediately
> follow a call to pwrdm_pre_transition() as well.
>
> Santosh/Rajendra, please confirm/ack.

I agree, looks like they should be removed.
Acked-by: Rajendra Nayak <rnayak@ti.com>

>
> Kevin
>
>
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> index 1d5d010..bbabe1d 100644
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -263,12 +263,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
>   	 * In MPUSS OSWR or device OFF, interrupt controller  contest is lost.
>   	 */
>   	mpuss_clear_prev_logic_pwrst();
> -	pwrdm_clear_all_prev_pwrst(mpuss_pd);
>   	if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET)&&
>   		(pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF))
>   		save_state = 2;
>
> -	clear_cpu_prev_pwrst(cpu);
>   	cpu_clear_prev_logic_pwrst(cpu);
>   	set_cpu_next_pwrst(cpu, power_state);
>   	set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume));

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Tue, Jan 31, 2012 at 5:44 AM, Kevin Hilman <khilman@ti.com> wrote:
> Paul Walmsley <paul@pwsan.com> writes:
>
>> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
>> pwrdm_pre_transition(), which appears a few lines after these calls,
>> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
>> need to do it twice.
>
> It looks like these two for OMAP4 are surpurfluous since the immediately
> follow a call to pwrdm_pre_transition() as well.
>
> Santosh/Rajendra, please confirm/ack.
>
> Kevin
>
>
> diff --git a/arch/arm/mach-omap2/omap-mpuss-lowpower.c b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> index 1d5d010..bbabe1d 100644
> --- a/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> +++ b/arch/arm/mach-omap2/omap-mpuss-lowpower.c
> @@ -263,12 +263,10 @@ int omap4_enter_lowpower(unsigned int cpu, unsigned int power_state)
> ? ? ? ? * In MPUSS OSWR or device OFF, interrupt controller ?contest is lost.
> ? ? ? ? */
> ? ? ? ?mpuss_clear_prev_logic_pwrst();
> - ? ? ? pwrdm_clear_all_prev_pwrst(mpuss_pd);
> ? ? ? ?if ((pwrdm_read_next_pwrst(mpuss_pd) == PWRDM_POWER_RET) &&
> ? ? ? ? ? ? ? ?(pwrdm_read_logic_retst(mpuss_pd) == PWRDM_POWER_OFF))
> ? ? ? ? ? ? ? ?save_state = 2;
>
> - ? ? ? clear_cpu_prev_pwrst(cpu);
> ? ? ? ?cpu_clear_prev_logic_pwrst(cpu);
> ? ? ? ?set_cpu_next_pwrst(cpu, power_state);
> ? ? ? ?set_cpu_wakeup_addr(cpu, virt_to_phys(omap4_cpu_resume));

The power domain pre-transition and post transition calls were kept
under the CONFIG_PM_DEBUG in the first few versions of the patches.
These pre and post
transition use to take lot of time once in while when measurement were
done and the only
useful thing they were doing was the counter updates. Later I removed
the PM_DEBUG as
per your suggestion.

As per Pauls comment they are optimised, so that should be good.

In this code the need is to clear only CPU and MPUPD, and hence they are
explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG
in production kernels.

But as you stated in current mainline kernel they are superfluous since the
pre/post calls are not under PM debug. So I am ok either way

Regards
Santosh

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

Hi

just a few thoughts.

On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:

> In this code the need is to clear only CPU and MPUPD, and hence they are
> explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG
> in production kernels.
> 
> But as you stated in current mainline kernel they are superfluous since the
> pre/post calls are not under PM debug. So I am ok either way

We're using the PM counters for the context restore skip optimization now 
in pwrdm_get_context_loss_count(), so they've suddenly become needed even 
when PM_DEBUG=n.  But those counters are only needed when there are 
devices in the CPU* powerdomains to track that can lose logic context.  I 
don't think that's the case on OMAP4, but you would probably know better 
than I.

Another aspect is that previous powerstate accesses for the CPU* 
powerdomains would theoretically go to the MPU local PRM rather than the 
system PRCM.  So they may actually return quickly.  Haven't tested this.

The MPUSS powerdomain might be another case, though.  There are a bunch of 
devices in there: the local timers, APB debug devices, etc.  We probably 
have to worry about restoring context for some of those at some point.


- Paul

[PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements

[Tero Kristo]

On Mon, 2012-01-30 at 13:27 -0800, Kevin Hilman wrote:
> Paul Walmsley <paul@pwsan.com> writes:
> 
> > This series optimizes some of the powerdomain-related code in
> > arch/arm/mach-omap2/pm*, and fixes a bug or two.  These were noticed while
> > working on the functional powerstate code.
> >
> > Rajendra and Santosh, if you have a spare moment, could you please
> > peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch?  It
> > makes sense to me in theory, but you both would probably know better
> > than I :-)
> >
> > Kevin, want to take this series (assuming folks are happy with it) ?
> 
> Yes, I'll take this and add the Ack from Santosh.  Thanks for the nice
> cleanup.   
> 
> Will submit after seeing a Tested-by from someone who can test with CORE
> retention/off on OMAP4 (hint, hint,  Tero :)

Yeah, I can test these out hopefully tomorrow / today, I am busy today
creating a release but lets see. :) The patches seem okay to me also at
least.

-Tero

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Tue, Jan 31, 2012 at 12:45 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Hi
>
> just a few thoughts.
>
> On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:
>
>> In this code the need is to clear only CPU and MPUPD, and hence they are
>> explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG
>> in production kernels.
>>
>> But as you stated in current mainline kernel they are superfluous since the
>> pre/post calls are not under PM debug. So I am ok either way
>
> We're using the PM counters for the context restore skip optimization now
> in pwrdm_get_context_loss_count(), so they've suddenly become needed even
> when PM_DEBUG=n. ?But those counters are only needed when there are
> devices in the CPU* powerdomains to track that can lose logic context. ?I
> don't think that's the case on OMAP4, but you would probably know better
> than I.
>
Good point so the pre/post calls are must then. We should kill those extra calls
form OMAP4 code then.

> Another aspect is that previous powerstate accesses for the CPU*
> powerdomains would theoretically go to the MPU local PRM rather than the
> system PRCM. ?So they may actually return quickly. ?Haven't tested this.
>
You are correct. They do.

> The MPUSS powerdomain might be another case, though. ?There are a bunch of
> devices in there: the local timers, APB debug devices, etc. ?We probably
> have to worry about restoring context for some of those at some point.
>
MPUSS PD is at the global PRCM level but that one doesn't take time either.
You need few more cycles to reach there.
IIRC, other power domains use to take time because of the wait_transition
calls we had. Mostly it was because of the BUGs you fixed. When get some
time I will profile these calls and see if we still have that issue.

Regards
Santosh

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:

> On Tue, Jan 31, 2012 at 12:45 PM, Paul Walmsley <paul@pwsan.com> wrote:
> > On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:
> >
> >> In this code the need is to clear only CPU and MPUPD, and hence they are
> >> explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG
> >> in production kernels.
> >>
> >> But as you stated in current mainline kernel they are superfluous since the
> >> pre/post calls are not under PM debug. So I am ok either way
> >
> > We're using the PM counters for the context restore skip optimization now
> > in pwrdm_get_context_loss_count(), so they've suddenly become needed even
> > when PM_DEBUG=n. ?But those counters are only needed when there are
> > devices in the CPU* powerdomains to track that can lose logic context. ?I
> > don't think that's the case on OMAP4, but you would probably know better
> > than I.
> >
> Good point so the pre/post calls are must then. We should kill those extra calls
> form OMAP4 code then.

I wonder if we should track how many hwmods are present in a powerdomain?  
We could check that counter to determine if we could skip the access.

> > Another aspect is that previous powerstate accesses for the CPU*
> > powerdomains would theoretically go to the MPU local PRM rather than the
> > system PRCM. ?So they may actually return quickly. ?Haven't tested this.
> >
> You are correct. They do.

Cool.

> > The MPUSS powerdomain might be another case, though. ?There are a bunch of
> > devices in there: the local timers, APB debug devices, etc. ?We probably
> > have to worry about restoring context for some of those at some point.
> >
> MPUSS PD is at the global PRCM level but that one doesn't take time either.
> You need few more cycles to reach there.
> IIRC, other power domains use to take time because of the wait_transition
> calls we had. Mostly it was because of the BUGs you fixed. When get some
> time I will profile these calls and see if we still have that issue.

That would be interesting.  I was guessing that the previous powerstate 
registers in the global PRCM might be located in some clockdomain that was 
clocked at sys_clk or the 32KiHZ clock.  But if it's really just due to 
the wait_transition(), that might be something we can optimize...


- Paul

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Tue, Jan 31, 2012 at 12:57 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:
>
>> On Tue, Jan 31, 2012 at 12:45 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> > On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:
>> >
>> >> In this code the need is to clear only CPU and MPUPD, and hence they are
>> >> explicitly cleared since the pre/post transition calls can be moved to PM_DEBUG
>> >> in production kernels.
>> >>
>> >> But as you stated in current mainline kernel they are superfluous since the
>> >> pre/post calls are not under PM debug. So I am ok either way
>> >
>> > We're using the PM counters for the context restore skip optimization now
>> > in pwrdm_get_context_loss_count(), so they've suddenly become needed even
>> > when PM_DEBUG=n. ?But those counters are only needed when there are
>> > devices in the CPU* powerdomains to track that can lose logic context. ?I
>> > don't think that's the case on OMAP4, but you would probably know better
>> > than I.
>> >
>> Good point so the pre/post calls are must then. We should kill those extra calls
>> form OMAP4 code then.
>
> I wonder if we should track how many hwmods are present in a powerdomain?
> We could check that counter to determine if we could skip the access.
>
That's good idea since these calls are in critical patha nd faster we can do
better it is..

A week back I was discussing with Benoit and Tony about having some
infrastructure like "unused clocks" so that we can shutdown those
modules, and if possible some power domains. That way they get
removed at one single place and they the power domain calls
need not iterate over the once which are no longer needed.

Regards
Santosh

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:

> A week back I was discussing with Benoit and Tony about having some
> infrastructure like "unused clocks" so that we can shutdown those
> modules, and if possible some power domains. That way they get
> removed at one single place and they the power domain calls
> need not iterate over the once which are no longer needed.

Sure, if you have some ideas, that sounds promising.  I'd assume the main 
performance issues would be related to global PRCM register reads, rather 
than raw CPU cycles.  So that's probably where it would make sense to 
focus the effort. 

We should also be able to optimize out a fair number of the powerdomain 
register reads if we cache them in the struct powerdomain.  The current 
powerstate and next powerstate seem to be hit a lot, and those should both 
be cacheable.  Will probably do some of this as part of the functional 
powerstate code.


- Paul

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Tue, Jan 31, 2012 at 1:19 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Tue, 31 Jan 2012, Shilimkar, Santosh wrote:
>
>> A week back I was discussing with Benoit and Tony about having some
>> infrastructure like "unused clocks" so that we can shutdown those
>> modules, and if possible some power domains. That way they get
>> removed at one single place and they the power domain calls
>> need not iterate over the once which are no longer needed.
>
> Sure, if you have some ideas, that sounds promising. ?I'd assume the main
> performance issues would be related to global PRCM register reads, rather
> than raw CPU cycles. ?So that's probably where it would make sense to
> focus the effort.
>
I fully agree that PRCM register read time is the main issue.
Would discuss this with Benoit and Rajendra bit more on what we
can be done.

> We should also be able to optimize out a fair number of the powerdomain
> register reads if we cache them in the struct powerdomain. ?The current
> powerstate and next powerstate seem to be hit a lot, and those should both
> be cacheable. ?Will probably do some of this as part of the functional
> powerstate code.
>
PD register caching seems to be a good optimization.

Regards
Santosh

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Kevin Hilman]

Paul,

Kevin Hilman <khilman@ti.com> writes:

> Paul Walmsley <paul@pwsan.com> writes:
>
>> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
>> pwrdm_pre_transition(), which appears a few lines after these calls,
>> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
>> need to do it twice.
>
> It looks like these two for OMAP4 are surpurfluous since the immediately
> follow a call to pwrdm_pre_transition() as well.
>
> Santosh/Rajendra, please confirm/ack.

So after the discussion, do you want to fold this into the original
patch, or do you want a separate patch?

Kevin

[PATCH 0/2] ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements

[Tero Kristo]

On Mon, 2012-01-30 at 13:27 -0800, Kevin Hilman wrote:
> Paul Walmsley <paul@pwsan.com> writes:
> 
> > This series optimizes some of the powerdomain-related code in
> > arch/arm/mach-omap2/pm*, and fixes a bug or two.  These were noticed while
> > working on the functional powerstate code.
> >
> > Rajendra and Santosh, if you have a spare moment, could you please
> > peek at the OMAP4 LOWPOWERSTATECHANGE part of the second patch?  It
> > makes sense to me in theory, but you both would probably know better
> > than I :-)
> >
> > Kevin, want to take this series (assuming folks are happy with it) ?
> 
> Yes, I'll take this and add the Ack from Santosh.  Thanks for the nice
> cleanup.   
> 
> Will submit after seeing a Tested-by from someone who can test with CORE
> retention/off on OMAP4 (hint, hint,  Tero :)

The patches appear to behave nicely on omap4, there are still a couple
of quirks in the basic omap4 support though. l3init pd state is not
updated properly once we wake-up from device-off (it remains active for
a short while during wake-up before switching to idle with hwauto), and
cpu1 pwrdm state is not updated properly during hotplug, but these are
minor issues and should be fixed elsewhere.

You can add:

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

> 
> Kevin

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

On Tue, 31 Jan 2012, Kevin Hilman wrote:

> Kevin Hilman <khilman@ti.com> writes:
> 
> > Paul Walmsley <paul@pwsan.com> writes:
> >
> >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
> >> pwrdm_pre_transition(), which appears a few lines after these calls,
> >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
> >> need to do it twice.
> >
> > It looks like these two for OMAP4 are surpurfluous since the immediately
> > follow a call to pwrdm_pre_transition() as well.
> >
> > Santosh/Rajendra, please confirm/ack.
> 
> So after the discussion, do you want to fold this into the original
> patch, or do you want a separate patch?

Your changes make sense to me.  I am fine with you adding them into the 
original patch and adding some credit for you into the commit message.  
Or you can create a separate patch.

N.B., I haven't looked at this file before.  There are a few other things 
that don't look right that hopefully someone can take the initiative to 
fix.  For example, those calls to mpuss_clear_prev_logic_pwrst() and 
cpu_clear_prev_logic_pwrst() should be removed as well.  That should be 
done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c.  
Currently it's not, so that needs to be patched.

Also all of the open-coded powerdomain register accesses in 
omap-mpuss-lowpower.c should be removed - those should all go through 
pwrdm_*() functions...



- Paul

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Thu, Feb 2, 2012 at 12:57 AM, Paul Walmsley <paul@pwsan.com> wrote:
> On Tue, 31 Jan 2012, Kevin Hilman wrote:
>
>> Kevin Hilman <khilman@ti.com> writes:
>>
>> > Paul Walmsley <paul@pwsan.com> writes:
>> >
>> >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
>> >> pwrdm_pre_transition(), which appears a few lines after these calls,
>> >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
>> >> need to do it twice.
>> >
>> > It looks like these two for OMAP4 are surpurfluous since the immediately
>> > follow a call to pwrdm_pre_transition() as well.
>> >
>> > Santosh/Rajendra, please confirm/ack.
>>
>> So after the discussion, do you want to fold this into the original
>> patch, or do you want a separate patch?
>
> Your changes make sense to me. ?I am fine with you adding them into the
> original patch and adding some credit for you into the commit message.
> Or you can create a separate patch.
>
> N.B., I haven't looked at this file before. ?There are a few other things
> that don't look right that hopefully someone can take the initiative to
> fix. ?For example, those calls to mpuss_clear_prev_logic_pwrst() and
> cpu_clear_prev_logic_pwrst() should be removed as well. ?That should be
> done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c.
> Currently it's not, so that needs to be patched.
>
We(rajendra and myself) discussed this some time back with Benoit and we agreed
that for CPU and MPU power domains which are needed very early in the PM power
up sequence, we can do direct APIs..
These calls were in critical path and the PD API's proposed for context clear
and logic clear were use to iterate over all power domains.
It is only recently we got clear context etc some reasonable form
even though it still iterates over all hwmod etc which is really overhead.
Also you need to create 'pdev' etc for context-loss kind of APIs and
we all thought that that makes things un-ncessary complicated and
lengthy.


> Also all of the open-coded powerdomain register accesses in
> omap-mpuss-lowpower.c should be removed - those should all go through
> pwrdm_*() functions...
>
 I will have another look at the code with your recent power domain clean-ups
and see what all can be moved to generic APIs.

Regards
Santosh

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

Hi

On Thu, 2 Feb 2012, Shilimkar, Santosh wrote:

> On Thu, Feb 2, 2012 at 12:57 AM, Paul Walmsley <paul@pwsan.com> wrote:
>
> > N.B., I haven't looked at this file before. ?There are a few other things
> > that don't look right that hopefully someone can take the initiative to
> > fix. ?For example, those calls to mpuss_clear_prev_logic_pwrst() and
> > cpu_clear_prev_logic_pwrst() should be removed as well. ?That should be
> > done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c.
> > Currently it's not, so that needs to be patched.
> >
> We(rajendra and myself) discussed this some time back with Benoit and we agreed
> that for CPU and MPU power domains which are needed very early in the PM power
> up sequence, we can do direct APIs..

Looking at the call sites, don't these calls occur quite late in boot, 
after everything is initialized?  I see calls from a late_initcall() and a 
suspend entry function.  Am I missing something?

> These calls were in critical path and the PD API's proposed for context clear
> and logic clear were use to iterate over all power domains.

The code iterates over all powerdomains anyway a few lines above those 
calls, right?

If the code only needs to iterate over a subset, let's discuss what you 
need and a different iterator can be implemented in the powerdomain code.

> It is only recently we got clear context etc some reasonable form
> even though it still iterates over all hwmod etc which is really overhead.

Which mainline PM calls iterate over all hwmods?

> Also you need to create 'pdev' etc for context-loss kind of APIs and
> we all thought that that makes things un-ncessary complicated and
> lengthy.

Could you point out the code that you are referring to?  I seem to be 
missing it :-(

If the existing powerdomain API is missing something you need, there's no 
problem to add it.  It makes things easier to debug and more portable in 
the long term if there is a clean, standard interface.  Another hope is 
that more of the PM code can be shared.

In terms of performance, it seems pretty unlikely that one would be able 
to measure a meaningful performance difference, unless we are missing an 
API call that was needed?  As we were discussing before, device reads are 
likely to dominate the profile.

Flipping through the code, I see that code is getting duplicated again.  
We now have three identical copies of clkdms_setup().  The OMAP4 
pwrdms_setup() is doing string comparisons to skip powerdomains -- that's 
also pretty fragile; that should be controlled through powerdomain flags 
or some similar mechanism.  Once that's fixed, it looks to me like that 
function could be shared with the OMAP34xx pwrdms_setup()?  Looks like we 
could also share omap{3,4}_pm_suspend() and omap{2,3,4}_pm_{begin,end}?

It would be good if we can keep working towards making as much of this 
code as common as possible.  If already-tested code can be reused, that 
should cut down on bugs and maintainer workload.  Also, we can avoid 
adding unnecessary lines of diff; we are trying to cut that down too..

> > Also all of the open-coded powerdomain register accesses in
> > omap-mpuss-lowpower.c should be removed - those should all go through
> > pwrdm_*() functions...
> >
>  I will have another look at the code with your recent power domain 
> clean-ups and see what all can be moved to generic APIs.

That would be great!


- Paul

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Thu, Feb 2, 2012 at 2:03 PM, Paul Walmsley <paul@pwsan.com> wrote:
> Hi
>
> On Thu, 2 Feb 2012, Shilimkar, Santosh wrote:
>
>> On Thu, Feb 2, 2012 at 12:57 AM, Paul Walmsley <paul@pwsan.com> wrote:
>>
>> > N.B., I haven't looked at this file before. ?There are a few other things
>> > that don't look right that hopefully someone can take the initiative to
>> > fix. ?For example, those calls to mpuss_clear_prev_logic_pwrst() and
>> > cpu_clear_prev_logic_pwrst() should be removed as well. ?That should be
>> > done by pwrdm_clear_all_prev_pwrst() in mach-omap2/powerdomain44xx.c.
>> > Currently it's not, so that needs to be patched.
>> >
>> We(rajendra and myself) discussed this some time back with Benoit and we agreed
>> that for CPU and MPU power domains which are needed very early in the PM power
>> up sequence, we can do direct APIs..
>
> Looking at the call sites, don't these calls occur quite late in boot,
> after everything is initialized? ?I see calls from a late_initcall() and a
> suspend entry function. ?Am I missing something?
>
The code has changed now  because of the recent ARM suspend code and CPU PM idle
notifiers consolidation. O.w before we had to check the last power
state as early as MMU is
with identity mapping and we use to check the CPU last power state.

Then for the interrupt controller enable/disable etc as well we use to
check MPUSS PD
context lost state etc.

Thanks to the consolidation, some of those situations have reduced now.

>> These calls were in critical path and the PD API's proposed for context clear
>> and logic clear were use to iterate over all power domains.
>
> The code iterates over all powerdomains anyway a few lines above those
> calls, right?
>
As I said in other thread, that code was under PM debug.

> If the code only needs to iterate over a subset, let's discuss what you
> need and a different iterator can be implemented in the powerdomain code.
>
>> It is only recently we got clear context etc some reasonable form
>> even though it still iterates over all hwmod etc which is really overhead.
>
> Which mainline PM calls iterate over all hwmods?
>
>> Also you need to create 'pdev' etc for context-loss kind of APIs and
>> we all thought that that makes things un-ncessary complicated and
>> lengthy.
>
> Could you point out the code that you are referring to? ?I seem to be
> missing it :-(
>
> If the existing powerdomain API is missing something you need, there's no
> problem to add it. ?It makes things easier to debug and more portable in
> the long term if there is a clean, standard interface. ?Another hope is
> that more of the PM code can be shared.
>
Yes, we do have issue with below APIs for OMAP4 and onwards design
because of the PRCM change.

pwrdm_clear_all_prev_pwrst
pwrdm_*_mem_*

There use to be a single power state and memory state register till OMAP3
at power domain level which can give you the logic and memory last test
which is needed for OSWR.

On OMAP4, we have registers per modules and it becomes difficult to model
this difference in APIs. Initially we tried to fix that with [1] and
then later realized
that's not going to work on OMAP4 + devices because of mentioned issue.

Regards
Santosh
[1] http://permalink.gmane.org/gmane.linux.ports.arm.omap/42564




> In terms of performance, it seems pretty unlikely that one would be able
> to measure a meaningful performance difference, unless we are missing an
> API call that was needed? ?As we were discussing before, device reads are
> likely to dominate the profile.
>
> Flipping through the code, I see that code is getting duplicated again.
> We now have three identical copies of clkdms_setup(). ?The OMAP4
> pwrdms_setup() is doing string comparisons to skip powerdomains -- that's
> also pretty fragile; that should be controlled through powerdomain flags
> or some similar mechanism. ?Once that's fixed, it looks to me like that
> function could be shared with the OMAP34xx pwrdms_setup()? ?Looks like we
> could also share omap{3,4}_pm_suspend() and omap{2,3,4}_pm_{begin,end}?
>
> It would be good if we can keep working towards making as much of this
> code as common as possible. ?If already-tested code can be reused, that
> should cut down on bugs and maintainer workload. ?Also, we can avoid
> adding unnecessary lines of diff; we are trying to cut that down too..
>
>> > Also all of the open-coded powerdomain register accesses in
>> > omap-mpuss-lowpower.c should be removed - those should all go through
>> > pwrdm_*() functions...
>> >
>> ?I will have another look at the code with your recent power domain
>> clean-ups and see what all can be moved to generic APIs.
>
> That would be great!
>
>
> - Paul

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

On Thu, 2 Feb 2012, Shilimkar, Santosh wrote:

> Yes, we do have issue with below APIs for OMAP4 and onwards design
> because of the PRCM change.
> 
> pwrdm_clear_all_prev_pwrst
> pwrdm_*_mem_*
> 
> There use to be a single power state and memory state register till OMAP3
> at power domain level which can give you the logic and memory last test
> which is needed for OSWR.
> 
> On OMAP4, we have registers per modules and it becomes difficult to model
> this difference in APIs. Initially we tried to fix that with [1] and
> then later realized
> that's not going to work on OMAP4 + devices because of mentioned issue.

If the registers are per-module, it seems like the best place for those 
would be the hwmod layer.  Do you think that makes sense?  So, something 
like omap_hwmod_clear_all_prev_pwrst(), etc.?  Seems like that should be 
pretty efficient.


- Paul

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

On Thu, Feb 2, 2012 at 3:35 PM, Paul Walmsley <paul@pwsan.com> wrote:
> On Thu, 2 Feb 2012, Shilimkar, Santosh wrote:
>
>> Yes, we do have issue with below APIs for OMAP4 and onwards design
>> because of the PRCM change.
>>
>> pwrdm_clear_all_prev_pwrst
>> pwrdm_*_mem_*
>>
>> There use to be a single power state and memory state register till OMAP3
>> at power domain level which can give you the logic and memory last test
>> which is needed for OSWR.
>>
>> On OMAP4, we have registers per modules and it becomes difficult to model
>> this difference in APIs. Initially we tried to fix that with [1] and
>> then later realized
>> that's not going to work on OMAP4 + devices because of mentioned issue.
>
> If the registers are per-module, it seems like the best place for those
> would be the hwmod layer. ?Do you think that makes sense? ?So, something
> like omap_hwmod_clear_all_prev_pwrst(), etc.? ?Seems like that should be
> pretty efficient.
>
But all these are power domain registers after all. Rajendra did one version of
" pwrdm_clear_all_prev_pwrst"  API and inside used hwmod. But then there he
has iterate over all the modules belongs to that power domain.

And if you use hwmod or omap_device kind of API, then  you need to
build those devices in init or some where.

All of that was not looking so elegant and hence the other path was chosen.
The mess is, the registers are still part of power domains though they
are specific
to module. And Fundamentally power domain is stil the central entity
decides the fate of all the modules within that PD, in terms of context
loss/ret etc.

Regards
santosh

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Shilimkar, Santosh]

Paul,

On Thu, Feb 2, 2012 at 3:47 PM, Shilimkar, Santosh
<santosh.shilimkar@ti.com> wrote:
> On Thu, Feb 2, 2012 at 3:35 PM, Paul Walmsley <paul@pwsan.com> wrote:
>> On Thu, 2 Feb 2012, Shilimkar, Santosh wrote:
>>
>>> Yes, we do have issue with below APIs for OMAP4 and onwards design
>>> because of the PRCM change.
>>>
>>> pwrdm_clear_all_prev_pwrst
>>> pwrdm_*_mem_*
>>>
>>> There use to be a single power state and memory state register till OMAP3
>>> at power domain level which can give you the logic and memory last test
>>> which is needed for OSWR.
>>>
>>> On OMAP4, we have registers per modules and it becomes difficult to model
>>> this difference in APIs. Initially we tried to fix that with [1] and
>>> then later realized
>>> that's not going to work on OMAP4 + devices because of mentioned issue.
>>
>> If the registers are per-module, it seems like the best place for those
>> would be the hwmod layer. ?Do you think that makes sense? ?So, something
>> like omap_hwmod_clear_all_prev_pwrst(), etc.? ?Seems like that should be
>> pretty efficient.
>>
> But all these are power domain registers after all. Rajendra did one version of
> " pwrdm_clear_all_prev_pwrst" ?API and inside used hwmod. But then there he
> has iterate over all the modules belongs to that power domain.
>
> And if you use hwmod or omap_device kind of API, then ?you need to
> build those devices in init or some where.
>
> All of that was not looking so elegant and hence the other path was chosen.
> The mess is, the registers are still part of power domains though they
> are specific
> to module. And Fundamentally power domain is stil the central entity
> decides the fate of all the modules within that PD, in terms of context
> loss/ret etc.
>
I looked at the MPUSS file. There are 3 functions which access
power domain registers directly.

- mpuss_clear_prev_logic_pwrst
- cpu_clear_prev_logic_pwrst
- omap4_mpuss_read_prev_context_state

All of these functions access the context registers which
we don't have support today at power domian APIs for
mentioned issue. This file is using power domain APIs
in all possible scenario's except the OSWR scenario
which needs the context register accesses.

if the context clear and read can be handled part of
PD APIs, then we can certainly kill this functions.

Regards
santosh

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> On Tue, 31 Jan 2012, Kevin Hilman wrote:
>
>> Kevin Hilman <khilman@ti.com> writes:
>> 
>> > Paul Walmsley <paul@pwsan.com> writes:
>> >
>> >> Remove some superfluous calls to pwrdm_clear_all_prev_pwrst().
>> >> pwrdm_pre_transition(), which appears a few lines after these calls,
>> >> invokes pwrdm_clear_all_prev_pwrst() on each powerdomain -- there's no
>> >> need to do it twice.
>> >
>> > It looks like these two for OMAP4 are surpurfluous since the immediately
>> > follow a call to pwrdm_pre_transition() as well.
>> >
>> > Santosh/Rajendra, please confirm/ack.
>> 
>> So after the discussion, do you want to fold this into the original
>> patch, or do you want a separate patch?
>
> Your changes make sense to me.  I am fine with you adding them into the 
> original patch and adding some credit for you into the commit message.  
> Or you can create a separate patch.

OK, I'll fold it in.

Thanks,

Kevin

[PATCH 1/2] ARM: OMAP3: PM: remove superfluous calls to pwrdm_clear_all_prev_pwrst()

[Paul Walmsley]

Hi Santosh

On Thu, 2 Feb 2012, Shilimkar, Santosh wrote:

> On Thu, Feb 2, 2012 at 3:47 PM, Shilimkar, Santosh
> <santosh.shilimkar@ti.com> wrote:
> > On Thu, Feb 2, 2012 at 3:35 PM, Paul Walmsley <paul@pwsan.com> wrote:
> >> On Thu, 2 Feb 2012, Shilimkar, Santosh wrote:
> >>
> >>> Yes, we do have issue with below APIs for OMAP4 and onwards design
> >>> because of the PRCM change.
> >>>
> >>> pwrdm_clear_all_prev_pwrst
> >>> pwrdm_*_mem_*
> >>>
> >>> There use to be a single power state and memory state register till OMAP3
> >>> at power domain level which can give you the logic and memory last test
> >>> which is needed for OSWR.
> >>>
> >>> On OMAP4, we have registers per modules and it becomes difficult to model
> >>> this difference in APIs. Initially we tried to fix that with [1] and
> >>> then later realized
> >>> that's not going to work on OMAP4 + devices because of mentioned issue.
> >>
> >> If the registers are per-module, it seems like the best place for those
> >> would be the hwmod layer. ?Do you think that makes sense? ?So, something
> >> like omap_hwmod_clear_all_prev_pwrst(), etc.? ?Seems like that should be
> >> pretty efficient.
> >>
> > But all these are power domain registers after all. Rajendra did one version of
> > " pwrdm_clear_all_prev_pwrst" ?API and inside used hwmod. But then there he
> > has iterate over all the modules belongs to that power domain.
> >
> > And if you use hwmod or omap_device kind of API, then ?you need to
> > build those devices in init or some where.
> >
> > All of that was not looking so elegant and hence the other path was chosen.
> > The mess is, the registers are still part of power domains though they
> > are specific
> > to module. And Fundamentally power domain is stil the central entity
> > decides the fate of all the modules within that PD, in terms of context
> > loss/ret etc.
> >
> I looked at the MPUSS file. There are 3 functions which access
> power domain registers directly.
> 
> - mpuss_clear_prev_logic_pwrst
> - cpu_clear_prev_logic_pwrst
> - omap4_mpuss_read_prev_context_state
> 
> All of these functions access the context registers which
> we don't have support today at power domian APIs for
> mentioned issue. This file is using power domain APIs
> in all possible scenario's except the OSWR scenario
> which needs the context register accesses.
> 
> if the context clear and read can be handled part of
> PD APIs, then we can certainly kill this functions.

That sounds great.  Maybe we can add powerdomain functions for these 
purposes that take both a powerdomain pointer and a hwmod pointer.  That 
should hopefully work.


- Paul