Rafael J. Wysocki had written, on 09/17/2010 06:07 PM, the following: > On Friday, September 17, 2010, Nishanth Menon wrote: >> Andrew Morton had written, on 09/17/2010 02:19 PM, the following: >>> On Thu, 16 Sep 2010 20:29:33 -0500 >>> Nishanth Menon <nm@xxxxxx> wrote: >>> >> [...] >>>> Documentation/power/00-INDEX | 2 + >>>> include/linux/opp.h | 136 +++++++++++++ >>>> kernel/power/Kconfig | 14 ++ >>>> lib/Makefile | 2 + >>>> lib/opp.c | 440 ++++++++++++++++++++++++++++++++++++++++++ >>> ./lib/ is an unusual place to put a driver-like thing such as this. >>> The lib/ directory is mainly for generic kernel-wide support things. >>> I'd suggest that ./drivers/opp/ would be a better place. >> we had an interesting debate on this topic on the the thread starting >> http://marc.info/?l=linux-arm-kernel&m=128465710624421&w=2 >> >> It really does not provide any driver like feature here. it is just a >> bunch of helper functions which any SOC framework can use to build up to >> implement either: >> a) cpufreq implementation with various OPPs >> b) bootup in a specific opp in a set of supported OPPs and stay there >> (e.g. mpurate support on OMAP platforms) > > Generally, some files under drivers/base/power contain helper functions of > similar nature. You can put it in there, as far as I'm concerned. Ack. thanks for the suggestion. it looks like a good place to me too. will do it as part of v2. > >> I had considered putting it in drivers/power, but it looks to contain >> mostly regulator stuff, the other alternative would be to include it in >> kernel/power or as Kevin H, Linus W and you mention drivers/opp >> >> Personally, I dont have a strong feeling about it as long as it is >> reusable across SOCs and am hoping to hear some alignment :). >> >>>> ... >>>> >>>> +/* >>>> + * Initialization wrapper used to define an OPP. >>>> + * To point at the end of a terminator of a list of OPPs, >>>> + * use OPP_DEF(0, 0, 0) >>>> + */ >>>> +#define OPP_DEF(_enabled, _freq, _uv) \ >>>> +{ \ >>>> + .enabled = _enabled, \ >>>> + .freq = _freq, \ >>>> + .u_volt = _uv, \ >>>> +} >>> OPP_DEF is a somewhat atypical name. OPP_INITIALIZER would be more >>> conventional. >> Thanks. will incorporate this in v2 of my patchset. >> >>> However OPP_DEF has no usage in this patch so perhaps this can be >>> removed? >> there were a few OMAP followon patches from >> http://marc.info/?l=linux-omap&m=128152135801634&w=2 >> which will actually use this for OMAP3 platform. > > Still, the $subject patch doesn't use it. So perhaps add it in a separate patch? was hoping to align on the approach with v2, the follow on OMAP patch https://patchwork.kernel.org/patch/183742/ will post with needed changes be part of Kevin's thread for OMAP3 if we are ok with it, or Do we want to see it in this series as reference? > >>>> +static LIST_HEAD(dev_opp_list); >>> There's no locking for this list. That's OK under some circumstances, >>> but I do think there should be a comment here explaining this apparent >>> bug: why is no locking needed, what are the lifetime rules for entries >>> on this list. >> Locking: >> arrgh.. my bad.. I had documented it in the missing and later on posted >> opp.txt http://marc.info/?l=linux-arm-kernel&m=128473931626114&w=2 >> >> The OPP layer implementation expects to be used in multiple contexts >> (including calls from interrupt locked context) based on SOC framework >> implementation. It is recommended to use appropriate locking schemes >> within the SOC framework itself. > > That should be stated directly in the kerneldoc comments. The requirement > that the caller is supposed to ensure suitable locking is by no means > a trivial one. Agreed. thanks for pointing this out. I have done this as part of v2 of my patch. will post in a few hrs. > > Apart from this, it might be a good idea to help callers a bit and actually > introduce some sort of locking into the framework. in OMAP implementation we have need to use these apis from irq_locked contexts as well.. lock implementation will prevent thier necessary usage. instead we have implemented lock mechanisms in higher layers and ensured that the opp table does not change once created - the rest of them are query functions - the risk is on opp_enable/disable. > >> In terms of the lifetime rules on the nodes in the list: >> The list is expected to be maintained once created, entries are expected >> to be added optimally and not expected to be destroyed, the choice of >> list implementation was for reducing the complexity of the code itself >> and not yet meant as a mechanism to dynamically add and delete nodes on >> the fly.. Essentially, it was intended for the SOC framework to ensure >> it plugs in the OPP entries optimally and not create a humongous list of >> all possible OPPs for all families of the vendor SOCs - even though it >> is possible to use the OPP layer so - it just wont be smart to do so >> considering list scan latencies on hot paths such as cpufreq transitions >> or idle transitions. > > If the list nodes are not supposed to be added and removed dynamically, > it probably would make sense to create data structures containing > the "available" OPPs only, once they are known, and simply free the object > representing the other ones. I covered the usage in my reply here: http://marc.info/?l=linux-arm-kernel&m=128476570300466&w=2 but to repeat, the list is dynamic during initialization but remains static after initialization based on SOC framework implementation - this is best implemented with a list (we had started with an original array implementation which evolved to the current list implementation http://marc.info/?l=linux-omap&m=125912217718770&w=2) -- Regards, Nishanth Menon _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
