| Message ID | b48e2c944db072c220a1b0ae0c3d94eb1c4da7ab.1628579170.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | cpufreq: Auto-register with energy model | expand |
On 8/10/21 8:36 AM, Viresh Kumar wrote: > Many cpufreq drivers register with the energy model for each policy and > do exactly the same thing. Follow the footsteps of thermal-cooling, to > get it done from the cpufreq core itself. > > Provide a cpufreq driver flag so drivers can ask the cpufreq core to > register with the EM core on their behalf. This allows us to get rid of > duplicated code in the drivers. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > drivers/cpufreq/cpufreq.c | 9 +++++++++ > include/linux/cpufreq.h | 6 ++++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 06c526d66dd3..a060dc2aa2f2 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -23,6 +23,7 @@ > #include <linux/kernel_stat.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/pm_opp.h> > #include <linux/pm_qos.h> > #include <linux/slab.h> > #include <linux/suspend.h> > @@ -1511,6 +1512,11 @@ static int cpufreq_online(unsigned int cpu) > if (cpufreq_thermal_control_enabled(cpufreq_driver)) > policy->cdev = of_cpufreq_cooling_register(policy); The of_cpufreq_cooling_register() should be called after the EM is present for the CPU device. When you check that function, you will see that we call em_cpu_get(policy->cpu) to get the EM pointer. Otherwise IPA might fail. > > + if (cpufreq_driver->flags & CPUFREQ_REGISTER_WITH_EM) { > + dev_pm_opp_of_register_em(get_cpu_device(policy->cpu), > + policy->related_cpus); > + } > + So please move these new code above the thermal registration few lines above. > pr_debug("initialization complete\n"); > > return 0; > @@ -1602,6 +1608,9 @@ static int cpufreq_offline(unsigned int cpu) > goto unlock; > } > > + if (cpufreq_driver->flags & CPUFREQ_REGISTER_WITH_EM) > + dev_pm_opp_of_unregister_em(get_cpu_device(cpu)); > + Here is similar situation. Move the EM unregister after the thermal is done. For consistency it's OK, the real EM struct won't be freed for CPUs (due to scheduler reasons), though. > if (cpufreq_thermal_control_enabled(cpufreq_driver)) { > cpufreq_cooling_unregister(policy->cdev); > policy->cdev = NULL; > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index 9fd719475fcd..f11723cd4cca 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -424,6 +424,12 @@ struct cpufreq_driver { > */ > #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING BIT(6) > > +/* > + * Set by drivers that want the core to automatically register the CPU device > + * with Energy Model. > + */ > +#define CPUFREQ_REGISTER_WITH_EM BIT(7) > + > int cpufreq_register_driver(struct cpufreq_driver *driver_data); > int cpufreq_unregister_driver(struct cpufreq_driver *driver_data); > >
On 10-08-21, 10:36, Lukasz Luba wrote: > The of_cpufreq_cooling_register() should be called after the EM > is present for the CPU device. When you check that function, > you will see that we call > em_cpu_get(policy->cpu) > to get the EM pointer. Otherwise IPA might fail. Good point. -- viresh
On 8/10/21 10:38 AM, Viresh Kumar wrote: > On 10-08-21, 10:36, Lukasz Luba wrote: >> The of_cpufreq_cooling_register() should be called after the EM >> is present for the CPU device. When you check that function, >> you will see that we call >> em_cpu_get(policy->cpu) >> to get the EM pointer. Otherwise IPA might fail. > > Good point. > In other patch set I had a discussion with Quentin and I've checked the Performance Domains setup code. There is a code triggering the rebuilding perf domains with EM from governor. We cannot call EM registration so late in this cpufreq_online(), not after cpufreq_init_policy() call. So this dev_pm_opp_of_unregister_em() must be called before the policy is initialized. I'm not sure if you still would like to push forward this patch set in this case.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 06c526d66dd3..a060dc2aa2f2 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -23,6 +23,7 @@ #include <linux/kernel_stat.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/pm_opp.h> #include <linux/pm_qos.h> #include <linux/slab.h> #include <linux/suspend.h> @@ -1511,6 +1512,11 @@ static int cpufreq_online(unsigned int cpu) if (cpufreq_thermal_control_enabled(cpufreq_driver)) policy->cdev = of_cpufreq_cooling_register(policy); + if (cpufreq_driver->flags & CPUFREQ_REGISTER_WITH_EM) { + dev_pm_opp_of_register_em(get_cpu_device(policy->cpu), + policy->related_cpus); + } + pr_debug("initialization complete\n"); return 0; @@ -1602,6 +1608,9 @@ static int cpufreq_offline(unsigned int cpu) goto unlock; } + if (cpufreq_driver->flags & CPUFREQ_REGISTER_WITH_EM) + dev_pm_opp_of_unregister_em(get_cpu_device(cpu)); + if (cpufreq_thermal_control_enabled(cpufreq_driver)) { cpufreq_cooling_unregister(policy->cdev); policy->cdev = NULL; diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index 9fd719475fcd..f11723cd4cca 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -424,6 +424,12 @@ struct cpufreq_driver { */ #define CPUFREQ_NO_AUTO_DYNAMIC_SWITCHING BIT(6) +/* + * Set by drivers that want the core to automatically register the CPU device + * with Energy Model. + */ +#define CPUFREQ_REGISTER_WITH_EM BIT(7) + int cpufreq_register_driver(struct cpufreq_driver *driver_data); int cpufreq_unregister_driver(struct cpufreq_driver *driver_data);
Many cpufreq drivers register with the energy model for each policy and do exactly the same thing. Follow the footsteps of thermal-cooling, to get it done from the cpufreq core itself. Provide a cpufreq driver flag so drivers can ask the cpufreq core to register with the EM core on their behalf. This allows us to get rid of duplicated code in the drivers. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpufreq/cpufreq.c | 9 +++++++++ include/linux/cpufreq.h | 6 ++++++ 2 files changed, 15 insertions(+) -- 2.31.1.272.g89b43f80a514