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 ? > [1] > https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1328 > > [2] > https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1350 > > [3] > https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq.c#L1374 > > > -- <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
