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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
