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