Re: [PATCH 0/10] OPP layer and additional cleanups.
> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/10] OPP layer and additional cleanups.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/10] OPP layer and additional cleanups.
>>
>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/10] OPP layer and additional cleanups.
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.
Re: [PATCH 0/10] OPP layer and additional cleanups.
>> * 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/10] OPP layer and additional cleanups.
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
60555025 125000
60: 0 6804 4536 45364535
55: 4536 0 6804 45364535
50: 4537 4536 0 68044535
25: 4536 4536 4536 06802
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/10] 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
60555025 125000
60: 0 6804 4536 45364535
55: 4536 0 6804 45364535
50: 4537 4536 0 68044535
25: 4536 4536 4536 06802
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(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
