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 ?