Re: [PATCH 0/10] OPP layer and additional cleanups.

[Nishanth Menon <nm@ti.com>]

Dasgupta, Romit had written, on 12/31/2009 07:29 AM, the following:
> Hi,
>    The following set of patches apply on top of the Kevin's pm-wip-opp
> branch. What I have tried to do in this set of patches are:
> 
> (Not in patch-set order)
> * OPP layer internals have moved to list based implementation.

Is there a benefit of list based implementation?

> * The OPP layer APIs have been changed. The search APIs have been
> reduced to one instead of opp_find_{exact|floor|ceil}.

Could you let us know the benefit of merging this? the split is aligned 
in the community as such after the very first implementation which had 
all three merged as a single function, but was split after multiple 
review comments on readability aspects.

> * OPP book-keeping is now done inside OPP layer. We do not have to
> maintain pointers to {mpu|dsp|l3}_opp, outside this layer.
nice idea, BUT, you need some sort of domain reference mechanism, 
introducing a enum (as done in enum volt_rail - patch 6/10) is the same 
as providing named struct pointers, they perform the same function = 
identifying the voltage domain for the operation. what is the benefit of 
using enum?

> * removed omap_opp_def as this is very similar to omap_opp.
yes, but the original intent was that registration mechanism will use a 
structure that is visible to the caller, this allows for isolated 
modification of structure and handling mechanism keeping the overall 
system impact minimal.

> * Cleaned up the SRF framework to use new OPP APIs.
nice :) lets align on your opp improvement patches as the first go.

> * Removed VDD1,2 OPP resources and instead introduced voltage resources.
>    This results in leaner code.
good to hear this.
> * L3 frequency change now happens from cpufreq notifier mechanism.
> * cpufreq driver now honors the CPUFREQ{H|L} flags.
> * uv to vsel precision loss is handled cleanly.
:) thanks.

> 
> Verified this on zoom2 with and without lock debugging. 
zoom3/sdp3630?

> 
> Some output from cpufreq translation statistics.
> 
> #
> cat /sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table                    
>    From  :
> To                                                                
>           600000    550000    500000    250000 125000
> 600000:      0      6804      4536      4536    4535
> 
> 550000:   4536         0      6804      4536    4535
> 
> 500000:   4537      4536         0      6804    4535
> 
> 250000:   4536      4536      4536         0    6802
> 
> 125000:   6802      4535      4535      4535     0
> 
> 
> diffstat output!
> 
>  mach-omap2/pm.h              |   17 +
>  mach-omap2/pm34xx.c          |   79 ++++--
>  mach-omap2/resource34xx.c    |  542 ++++++++++++++-----------------------------
>  mach-omap2/resource34xx.h    |   62 ++--
>  mach-omap2/smartreflex.c     |  285 +++++++++++-----------
>  mach-omap2/smartreflex.h     |   16 -
>  plat-omap/common.c           |    6 
>  plat-omap/cpu-omap.c         |   73 +++++
>  plat-omap/include/plat/io.h  |    1 
>  plat-omap/include/plat/opp.h |  265 +++++----------------
>  plat-omap/omap-pm-noop.c     |   35 --
>  plat-omap/omap-pm-srf.c      |   38 ---
>  plat-omap/opp.c              |  497 +++++++++++++++++++++------------------
>  plat-omap/opp_twl_tps.c      |   11 
>  14 files changed, 851 insertions(+), 1076 deletions(-)
> 
> 
minor comment:
Might be good if your patches 1/10 to 9/10 had different subjects.

-- 
Regards,
Nishanth Menon