On 06/04/2020 18:07, Lukasz Luba wrote: > > > On 4/6/20 3:58 PM, Daniel Lezcano wrote: >> >> Hi Lukasz, >> >> >> On 06/04/2020 15:29, Lukasz Luba wrote: >>> Hi Daniel, >>> >>> Thank you for the review. >>> >>> On 4/3/20 5:05 PM, Daniel Lezcano wrote: >>>> >>>> Hi Lukasz, >>>> >>>> >>>> On 18/03/2020 12:45, Lukasz Luba wrote: >>>>> Add support of other devices into the Energy Model framework not only >>>>> the >>>>> CPUs. Change the interface to be more unified which can handle other >>>>> devices as well. >>>> >>>> thanks for taking care of that. Overall I like the changes in this >>>> patch >>>> but it hard to review in details because the patch is too big :/ >>>> >>>> Could you split this patch into smaller ones? >>>> >>>> eg. (at your convenience) >>>> >>>> - One patch renaming s/cap/perf/ >>>> >>>> - One patch adding a new function: >>>> >>>> em_dev_register_perf_domain(struct device *dev, >>>> unsigned int nr_states, >>>> struct em_data_callback *cb); >>>> >>>> (+ EXPORT_SYMBOL_GPL) >>>> >>>> And em_register_perf_domain() using it. >>>> >>>> - One converting the em_register_perf_domain() user to >>>> em_dev_register_perf_domain >>>> >>>> - One adding the different new 'em' functions >>>> >>>> - And finally one removing em_register_perf_domain(). >>> >>> I agree and will do the split. I could also break the dependencies >>> for future easier merge. >>> >>>> >>>> >>>>> Acked-by: Quentin Perret <qperret@xxxxxxxxxx> >>>>> Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx> >>>>> --- >>>> >>>> [ ... ] >>>> >>>>> 2. Core APIs >>>>> @@ -70,14 +72,16 @@ CONFIG_ENERGY_MODEL must be enabled to use the EM >>>>> framework. >>>>> Drivers are expected to register performance domains into the EM >>>>> framework by >>>>> calling the following API:: >>>>> - int em_register_perf_domain(cpumask_t *span, unsigned int >>>>> nr_states, >>>>> - struct em_data_callback *cb); >>>>> + int em_register_perf_domain(struct device *dev, unsigned int >>>>> nr_states, >>>>> + struct em_data_callback *cb, cpumask_t *cpus); >>>> >>>> Isn't possible to get rid of this cpumask by using >>>> cpufreq_cpu_get() which returns the cpufreq's policy and from their get >>>> the related cpus ? >>> >>> We had similar thoughts with Quentin and I've checked this. >> >> Yeah, I suspected you already think about that :) >> >>> Unfortunately, if the policy is a 'new policy' [1] it gets >>> allocated and passed into cpufreq driver ->init(policy) [2]. >>> Then that policy is set into per_cpu pointer for each related_cpu [3]: >>> >>> for_each_cpu(j, policy->related_cpus) >>> per_cpu(cpufreq_cpu_data, j) = policy; >>> >>> Thus, any calls of functions (i.e. cpufreq_cpu_get()) which try to >>> take this ptr before [3] won't work. >>> >>> We are trying to register EM from cpufreq_driver->init(policy) and the >>> per_cpu policy is likely to be not populated at that phase. >> >> What is the problem of registering at the end of the cpufreq_online ? > > We want to enable driver developers to choose one of two options for the > registration of Energy Model: > 1. a simple one via dev_pm_opp_of_register_em(), which uses default > callback function calculating power based on: voltage, freq > and DT entry 'dynamic-power-coefficient' for each OPP > 2. a more sophisticated, when driver provides callback function, which > will be called from EM for each OPP to ask for related power; > This interface could also be used by devices which relay not only > on one source of 'voltage', i.e. manipulate body bias or have > other controlling voltage for gates in the new 3D transistors. They > might provide custom callback function in their cpufreq driver. > This is used i.e. in cpufreq drivers which use firmware to get power, > like scmi-cpufreq.c; > > To meet this requirement the registration of EM is moved into cpufreq > drivers, not in the framework i.e cpufreq_online(). If we could limit > the support for only option 1. then we could move the registration > call into cpufreq framework and clean the cpufreq drivers. I'm not sure to get your point but I think a series setting the scene by moving the dev_pm_opp_of_register_em() to cpufreq_online() and remove the cpumask may make sense. Can you send the split version of patch 1/5 as a series without the other changes ? So we can focus on first ? -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
