OPP layer and additional cleanups.
cover OPP layer and additional cleanups.
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(-)
Dasgupta, Romit had written, on 12/31/2009 07:29 AM, the following:
Quoted 6 lines
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?
Quoted 2 lines
* 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.
Quoted 2 lines
* 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?
Quoted 1 line
* removed
omap_opp_defas this is very similar toomap_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.
Quoted 1 line
* Cleaned up the SRF framework to use new OPP APIs.
nice :) lets align on your opp improvement patches as the first go.
Quoted 2 lines
* Removed VDD1,2 OPP resources and instead introduced voltage resources. This results in leaner code.
good to hear this.
Quoted 3 lines
* 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.
Quoted 2 lines
Verified this on zoom2 with and without lock debugging.
zoom3/sdp3630?
Quoted 21 lines
Some output from cpufreq translation statistics.
#
cat/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table
From :
To600000 550000 500000 250000 125000 600000: 0 6804 4536 4536 4535550000: 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!
Quoted 17 lines
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.
Signature · 2 lines
Regards, Nishanth Menon
Quoted 3 lines
Quoted 1 line
* 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.
Quoted 8 lines
Quoted 3 lines
* 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.
Quoted 8 lines
Quoted 3 lines
* 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 *.
Quoted 6 lines
Quoted 2 lines
* removed
omap_opp_defas this is very similar toomap_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.
Quoted 2 lines
Quoted 1 line
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.
Quoted 4 lines
Quoted 2 lines
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.
Dasgupta, Romit had written, on 01/07/2010 02:24 AM, the following:
Quoted 11 lines
Quoted 2 lines
Quoted 1 line
* 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.
Quoted 6 lines
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.
Quoted 11 lines
Quoted 4 lines
Quoted 3 lines
* 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.
Quoted 9 lines
Quoted 3 lines
Quoted 2 lines
* 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_railis 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 (likeMPU_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 structomap_opp*.
Sigh.. more description below.
Quoted 5 lines
Quoted 2 lines
Quoted 1 line
* removed
omap_opp_defas this is very similar toomap_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_oppreduces 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.h956f872d (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.
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
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 ;).
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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.
Signature · 2 lines
Regards, Nishanth Menon
Quoted 4 lines
Quoted 3 lines
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).
Quoted 13 lines
Quoted 6 lines
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:(quoted diff) +6
-0+struct omap_opp { + struct list_head opp_node; + unsigned int enabled:1; + unsigned long rate; + unsigned long uvolt; +};
Quoted 7 lines
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.hto write their own direct handling mechanisms and we want to prevent it by forcing callers to use only accessor apis.opp.his meant as in include file for all users of opp layer and it's inner workings/ inner structures should be isolated toopp.cOR 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.
Quoted 14 lines
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
* 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.
Quoted 6 lines
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.
Quoted 8 lines
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 intosearch_exact,search_ceil,search_floorfor 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.
Quoted 5 lines
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 structomap_oppalbiet 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.
Dasgupta, Romit had written, on 01/08/2010 01:10 AM, the following:
Quoted 5 lines
Quoted 3 lines
Quoted 2 lines
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.
Quoted 31 lines
Quoted 12 lines
Quoted 5 lines
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:(quoted diff) +6
-0+struct omap_opp { + struct list_head opp_node; + unsigned int enabled:1; + unsigned long rate; + unsigned long uvolt; +};Quoted 1 line
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.hto write their own direct handling mechanisms and we want to prevent it by forcing callers to use only accessor apis.opp.his meant as in include file for all users of opp layer and it's inner workings/ inner structures should be isolated toopp.cOR a private header file alone.I am not sure what you say is correct. If you see the
opp.hfile in my patches you will find that it has accessor APIs. One does not have to do any dereferencing to access any of the structomap_oppmembers.
On the other hand if you look intoopp.cyou will find a structopp_list. This is the internal details that users do not want to care about and thus I have put it inopp.c. OTOH when you define the opp lists (e.g. for 3630, 3430) we do it in an array of structomap_opp. Hence we need to define this inopp.hSo I think your concern is taken care of.Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
Quoted 2 lines
* 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.
Quoted 8 lines
Quoted 3 lines
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 intosearch_exact,search_ceil,search_floorfor 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.
Quoted 7 lines
Quoted 2 lines
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 structomap_oppalbiet 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.
Signature · 2 lines
Regards, Nishanth Menon
Quoted 7 lines
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