ARM: OMAP2+: PM: miscellaneous powerdomain-related improvements

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 <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Rajendra Nayak <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Tero Kristo <[email protected]>

---

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.

Quoted 4 lines · 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.

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

arch/arm/mach-omap2/omap-mpuss-lowpower.c +0 -2

index 1d5d010..bbabe1d 100644
@@ -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));

Quoted 4 lines · 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.

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

arch/arm/mach-omap2/omap-mpuss-lowpower.c +0 -0

index 1d5d010..bbabe1d 100644
@@ -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));

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

Hi

just a few thoughts.

Quoted 6 lines · Santosh

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.

Quoted 9 lines · Paul Walmsley

Quoted 6 lines · Santosh

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

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 17 lines · Santosh

Quoted 16 lines · Paul Walmsley

Quoted 6 lines · Santosh

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.

Quoted 2 lines

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.

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.

Quoted 5 lines · Santosh

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.

Quoted 4 lines · 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.

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.

Quoted 7 lines · Kevin Hilman

Quoted 4 lines · 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.

Quoted 4 lines

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?

Quoted 9 lines · Kevin Hilman

Quoted 7 lines · Kevin Hilman

Quoted 4 lines · 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.

Quoted 7 lines

Quoted 4 lines

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

Quoted 7 lines · Paul Walmsley

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.

Quoted 4 lines

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.

Hi

Quoted 9 lines · Santosh

Quoted 7 lines · Paul Walmsley

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.

Quoted 3 lines

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?

Quoted 2 lines

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.

Quoted 2 lines

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?

Quoted 3 lines

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

Quoted 6 lines

Quoted 4 lines

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

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.

Quoted 6 lines · Santosh

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

Quoted 8 lines

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.

Quoted 17 lines · Paul Walmsley

Quoted 6 lines · Santosh

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

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 30 lines · Santosh

Quoted 17 lines · Paul Walmsley

Quoted 6 lines · Santosh

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

Quoted 8 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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.

Quoted 12 lines · Kevin Hilman

Quoted 10 lines · Kevin Hilman

Quoted 4 lines · 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.

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

Quoted 4 lines

Quoted 1 line

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.

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 <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Rajendra Nayak <[email protected]>

Cc Santosh Shilimkar <[email protected]>

Cc Tero Kristo <[email protected]>

---

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.

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

Quoted 9 lines · 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 :-)

Quoted 1 line

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

Quoted 9 lines · 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 :-)

Quoted 1 line

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