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

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

[Romit Dasgupta]

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.
* The OPP layer APIs have been changed. The search APIs have been
reduced to one instead of opp_find_{exact|floor|ceil}.
* OPP book-keeping is now done inside OPP layer. We do not have to
maintain pointers to {mpu|dsp|l3}_opp, outside this layer.
* removed omap_opp_def as this is very similar to omap_opp.
* Cleaned up the SRF framework to use new OPP APIs.
* Removed VDD1,2 OPP resources and instead introduced voltage resources.
   This results in leaner code.
* 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.

Verified this on zoom2 with and without lock debugging. 

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

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

[Nishanth Menon]

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

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

[Romit Dasgupta]

>> * OPP layer internals have moved to list based implementation.
> 
> Is there a benefit of list based implementation?

Actually, this is a question I have asked myself several times. The motivation
behind list based implementation is to accommodate introduction and revocation
of OPPs.(Not just enabling and disabling). Today's CPUFREQ layer and OPP layer
are disjoint (meaning we prepare the OPPs at boot time and then cpufreq copies
them to its own internal arrays). I want this to be united.

Only point I see that may disfavor list based implementation is the fact that we
do not expect high number of OPPs.

Having said this, I have tried to encapsulate the implementation within the OPP
layer. So moving to array/list/any other fancy data structure should be hidden
within OPP.  So the patchset is not only about moving to a list based
implementation. It also to change the semantics of the OPP layer APIs with a
deliberate intent to hide/encapsulate the implementation details.

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

Actually I wanted to minimize the number of interfaces to OPP Layer. What was
shouting at me was the fact that OPP layer at the heart of it is a in memory
data list. So all we need to poke about OPP is to add/delete/enable/disable/search.
for search I thought only a single interface like
'find_opp_by_freq' is enough. The flags passed to this function should dictate
the mode of the search.
> 
>> * 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?

The introduction of enum volt_rail is a totally different thing. It is to make
the voltage scaling function generic. On the other hand identifying the OPP list
is also enum based (like MPU_OPP, DSP_OPP, L3_OPP). This is to identify the opp
list I am interested in. Note that doing this enables me to get away from
maintaining struct omap_opp *.
> 
>> * 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.

Moving to struct omap_opp reduces one more data structure. I am sorry, I did not
follow the later part of your above comment.
>> Verified this on zoom2 with and without lock debugging. 
> zoom3/sdp3630?
Not yet verified. Any help on this from anyone in the list is appreciated.
>>
>>
> minor comment:
> Might be good if your patches 1/10 to 9/10 had different subjects.
Yes, Santosh pointed the same to me few days back. I agree this can be done.

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

[Nishanth Menon]

Dasgupta, Romit had written, on 01/07/2010 02:24 AM, the following:
>>> * OPP layer internals have moved to list based implementation.
>> Is there a benefit of list based implementation?
> 
> Actually, this is a question I have asked myself several times. The motivation
> behind list based implementation is to accommodate introduction and revocation
> of OPPs.(Not just enabling and disabling). Today's CPUFREQ layer and OPP layer
> are disjoint (meaning we prepare the OPPs at boot time and then cpufreq copies
> them to its own internal arrays). I want this to be united.
> 
> Only point I see that may disfavor list based implementation is the fact that we
> do not expect high number of OPPs.
yes + overhead of CPU cycles walking thru the list Vs indexing off an array.

> 
> Having said this, I have tried to encapsulate the implementation within the OPP
> layer. So moving to array/list/any other fancy data structure should be hidden
> within OPP.  So the patchset is not only about moving to a list based
> implementation. It also to change the semantics of the OPP layer APIs with a
> deliberate intent to hide/encapsulate the implementation details.
opp.h:
+struct omap_opp {
+       struct list_head opp_node;
+       unsigned int enabled:1;
+       unsigned long rate;
+       unsigned long uvolt;
+};
this is exactly what we have been trying to avoid in the first place 
(see numerous discussions in the last few months in l-o ML). This allows 
for users of opp.h to write their own direct handling mechanisms and we 
want to prevent it by forcing callers to use only accessor apis. opp.h 
is meant as in include file for all users of opp layer and it's inner 
workings/ inner structures should be isolated to opp.c OR a private 
header file alone.

> 
>>> * 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.
> 
> Actually I wanted to minimize the number of interfaces to OPP Layer. What was
> shouting at me was the fact that OPP layer at the heart of it is a in memory
> data list. So all we need to poke about OPP is to add/delete/enable/disable/search.
> for search I thought only a single interface like
> 'find_opp_by_freq' is enough. The flags passed to this function should dictate
> the mode of the search.

yes, I am aware of this(my first series was motivated by the same 
though), but the alignment with the community is for:
split search into search_exact, search_ceil, search_floor for 
readability purposes. I dont deny that this is a data list and the basic 
APIs u mentioned are what is enough, but functionally, search is split 
as the above instead of taking params to denote the variations in search 
techniques - hence the community consensus.

>>> * 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?
> 
> The introduction of enum volt_rail is a totally different thing. It is to make
> the voltage scaling function generic. On the other hand identifying the OPP list
> is also enum based (like MPU_OPP, DSP_OPP, L3_OPP). This is to identify the opp
> list I am interested in. Note that doing this enables me to get away from
> maintaining struct omap_opp *.
Sigh.. more description below.

>>> * 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.
> 
> Moving to struct omap_opp reduces one more data structure. I am sorry, I did not
> follow the later part of your above comment.

I am hoping we are getting the thought across: providing a predefined 
supported OPP information to the OPP layer (a.k.a registration for a 
specific CPU type) should be decoupled with the OPP 
query/operation/search: that is the purpose of introducing APIs in the 
first place.

the initial intent was:
struct omap_opp_def is exposed to the world (a.k.a files which would 
like to register the predefined tables) for registration

struct omap_opp is provided for the files which would like to 
query/operate etc on OPPs.
For this: a) you have introduced enum with an array - which IMHO causes 
CPU specific dependencies - OMAP3 has MPU,DSP,L3 rails, while OMAP4 and 
future OMAP5 generations possibly will have different rails.
b) my approach was a generic struct omap_opp * which is CPU independent.
	NOTE: opp.h
956f872d        (Kevin Hilman   2009-12-16 14:29:39 -0800 
16)extern struct omap_opp *mpu_opps;
this is not intended to be final, but it is ok for the 
timebeing(considering OMAP1,2,3 ONLY). the reason why I would prefer 
[mpu|dsp|l3]_opps NOT to be defined in opp.h is because opp.h is 
supposed to be CPU independent - we should keep it that way. and 
introduce the specific opp list into the cpu specific file.

I really dont care if struct omap_opp * or enum is used (they are both 
32bit and have to be dereferenced at the end of the day) to refer to a 
voltage domain. In fact using enum might allow us to do cross opp 
dependency queries too if such an infrastructure is being introduced, 
but that can be done also with struct omap_opp albiet in an obtuse way.



>>> Verified this on zoom2 with and without lock debugging. 
>> zoom3/sdp3630?
> Not yet verified. Any help on this from anyone in the list is appreciated.
lets have an aligned version before we go ahead with major tests IMHO, 
just was curious in my question. but for the next patch series, please 
flag it so, so that we know if we need to pitch in ;).

>>>
>> minor comment:
>> Might be good if your patches 1/10 to 9/10 had different subjects.
> Yes, Santosh pointed the same to me few days back. I agree this can be done.
> 
thanks.

-- 
Regards,
Nishanth Menon

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

[Romit Dasgupta]

>>
>> Only point I see that may disfavor list based implementation is the fact that we
>> do not expect high number of OPPs.
> yes + overhead of CPU cycles walking thru the list Vs indexing off an array.
True there is overhead but the downside of an array is reallocing (as is the
case in the previous patch).
> 
>> Having said this, I have tried to encapsulate the implementation within the OPP
>> layer. So moving to array/list/any other fancy data structure should be hidden
>> within OPP.  So the patchset is not only about moving to a list based
>> implementation. It also to change the semantics of the OPP layer APIs with a
>> deliberate intent to hide/encapsulate the implementation details.
> opp.h:
> +struct omap_opp {
> +       struct list_head opp_node;
> +       unsigned int enabled:1;
> +       unsigned long rate;
> +       unsigned long uvolt;
> +};
> this is exactly what we have been trying to avoid in the first place 
> (see numerous discussions in the last few months in l-o ML). This allows 
> for users of opp.h to write their own direct handling mechanisms and we 
> want to prevent it by forcing callers to use only accessor apis. opp.h 
> is meant as in include file for all users of opp layer and it's inner 
> workings/ inner structures should be isolated to opp.c OR a private 
> header file alone.

I am not sure what you say is correct. If you see the opp.h file in my patches
you will find that it has accessor APIs. One does not have to do any
dereferencing to access any of the struct omap_opp members.
On the other hand if you look into opp.c you will find a struct opp_list. This
is the internal details that users do not want to care about and thus I have put
it in opp.c. OTOH when you define the opp lists (e.g. for 3630, 3430) we do it
in an array of struct omap_opp. Hence we need to define this in opp.h So I think
your concern is taken care of.
> 
>>>> * 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.
>> Actually I wanted to minimize the number of interfaces to OPP Layer. What was
>> shouting at me was the fact that OPP layer at the heart of it is a in memory
>> data list. So all we need to poke about OPP is to add/delete/enable/disable/search.
>> for search I thought only a single interface like
>> 'find_opp_by_freq' is enough. The flags passed to this function should dictate
>> the mode of the search.
> 
> yes, I am aware of this(my first series was motivated by the same 
> though), but the alignment with the community is for:
> split search into search_exact, search_ceil, search_floor for 
> readability purposes. I dont deny that this is a data list and the basic 
> APIs u mentioned are what is enough, but functionally, search is split 
> as the above instead of taking params to denote the variations in search 
> techniques - hence the community consensus.
> 

I wanted to reduce the interfaces. Imagine designing a car with two steerings
(one for going for driving back and the other for going forward). Instead it is
better to have one steering with a control to decide if you want to go forward
or backward.

> I really dont care if struct omap_opp * or enum is used (they are both 
> 32bit and have to be dereferenced at the end of the day) to refer to a 
> voltage domain. In fact using enum might allow us to do cross opp 
> dependency queries too if such an infrastructure is being introduced, 
> but that can be done also with struct omap_opp albiet in an obtuse way.

Slight difference than what you say. When you maintain a pointer to the head of
your data structure (be it an array or a list) and expect the APIs to pass it
around, it is different from passing an enum to identify which list you want to
search. You do not need to store a handle to the initiliazed list anymore.

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

[Nishanth Menon]

Dasgupta, Romit had written, on 01/08/2010 01:10 AM, the following:
>>> Only point I see that may disfavor list based implementation is the fact that we
>>> do not expect high number of OPPs.
>> yes + overhead of CPU cycles walking thru the list Vs indexing off an array.
> True there is overhead but the downside of an array is reallocing (as is the
> case in the previous patch).
lets see this series of list implementation as a seperate series.

>>> Having said this, I have tried to encapsulate the implementation within the OPP
>>> layer. So moving to array/list/any other fancy data structure should be hidden
>>> within OPP.  So the patchset is not only about moving to a list based
>>> implementation. It also to change the semantics of the OPP layer APIs with a
>>> deliberate intent to hide/encapsulate the implementation details.
>> opp.h:
>> +struct omap_opp {
>> +       struct list_head opp_node;
>> +       unsigned int enabled:1;
>> +       unsigned long rate;
>> +       unsigned long uvolt;
>> +};
>> this is exactly what we have been trying to avoid in the first place 
>> (see numerous discussions in the last few months in l-o ML). This allows 
>> for users of opp.h to write their own direct handling mechanisms and we 
>> want to prevent it by forcing callers to use only accessor apis. opp.h 
>> is meant as in include file for all users of opp layer and it's inner 
>> workings/ inner structures should be isolated to opp.c OR a private 
>> header file alone.
> 
> I am not sure what you say is correct. If you see the opp.h file in my patches
> you will find that it has accessor APIs. One does not have to do any
> dereferencing to access any of the struct omap_opp members.
> On the other hand if you look into opp.c you will find a struct opp_list. This
> is the internal details that users do not want to care about and thus I have put
> it in opp.c. OTOH when you define the opp lists (e.g. for 3630, 3430) we do it
> in an array of struct omap_opp. Hence we need to define this in opp.h So I think
> your concern is taken care of.
>>>>> * 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.
>>> Actually I wanted to minimize the number of interfaces to OPP Layer. What was
>>> shouting at me was the fact that OPP layer at the heart of it is a in memory
>>> data list. So all we need to poke about OPP is to add/delete/enable/disable/search.
>>> for search I thought only a single interface like
>>> 'find_opp_by_freq' is enough. The flags passed to this function should dictate
>>> the mode of the search.
>> yes, I am aware of this(my first series was motivated by the same 
>> though), but the alignment with the community is for:
>> split search into search_exact, search_ceil, search_floor for 
>> readability purposes. I dont deny that this is a data list and the basic 
>> APIs u mentioned are what is enough, but functionally, search is split 
>> as the above instead of taking params to denote the variations in search 
>> techniques - hence the community consensus.
>>
> 
> I wanted to reduce the interfaces. Imagine designing a car with two steerings
> (one for going for driving back and the other for going forward). Instead it is
> better to have one steering with a control to decide if you want to go forward
> or backward.

lets make the list implementation as a seperate series and discuss this. 
I am guessing that there could be wrapper apis which would could 
optimize the implementation while maintaining the overall APIs allowing 
other dependent users to continue. I will reserve my comments till we 
see the series on this.


> 
>> I really dont care if struct omap_opp * or enum is used (they are both 
>> 32bit and have to be dereferenced at the end of the day) to refer to a 
>> voltage domain. In fact using enum might allow us to do cross opp 
>> dependency queries too if such an infrastructure is being introduced, 
>> but that can be done also with struct omap_opp albiet in an obtuse way.
> 
> Slight difference than what you say. When you maintain a pointer to the head of
> your data structure (be it an array or a list) and expect the APIs to pass it
> around, it is different from passing an enum to identify which list you want to
> search. You do not need to store a handle to the initiliazed list anymore.
Enum referencing is better that way, ack. looking forward to seeing a 
seperate series with this.


-- 
Regards,
Nishanth Menon

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

[Romit Dasgupta]

> 
> lets make the list implementation as a seperate series and discuss this. 
> I am guessing that there could be wrapper apis which would could 
> optimize the implementation while maintaining the overall APIs allowing 
> other dependent users to continue. I will reserve my comments till we 
> see the series on this.
> 
Based on the  OPP nano-summit!! we had last Friday I will post two sets of
patches to start with

1. Miscellaneous changes that was part of the earlier patch.
2. Patch that will remove maintaining pointers to the beginning of the array
(OPP array).


Thanks,
-Romit