[PATCH V2 0/9] cpufreq: Auto-register with energy model

[PATCH V2 0/9] cpufreq: Auto-register with energy model

[Viresh Kumar]

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 new callback, which will be called, if present, by the cpufreq
core at the right moment (more on that in the code's comment). Also
provide a generic implementation that uses dev_pm_opp_of_register_em().

This also allows us to register with the EM at a later point of time,
compared to ->init(), from where the EM core can access cpufreq policy
directly using cpufreq_cpu_get() type of helpers and perform other work,
like marking few frequencies inefficient, this will be done separately.

This is build/boot tested by the bot for a couple of boards.

https://gitlab.com/vireshk/pmko/-/pipelines/351525873

Note that I haven't picked any of the Reviewed-by tags from the first version
since the idea is very much changed here.

V1->V2:
- Add a callback instead of flag.
- Register before governor is initialized.
- Update scmi driver as well.
- Don't unregister from the EM core.

--
Viresh

Viresh Kumar (9):
  cpufreq: Auto-register with energy model if asked
  cpufreq: dt: Use auto-registration for energy model
  cpufreq: imx6q: Use auto-registration for energy model
  cpufreq: mediatek: Use auto-registration for energy model
  cpufreq: omap: Use auto-registration for energy model
  cpufreq: qcom-cpufreq-hw: Use auto-registration for energy model
  cpufreq: scpi: Use auto-registration for energy model
  cpufreq: vexpress: Use auto-registration for energy model
  cpufreq: scmi: Use .register_em() callback

 drivers/cpufreq/cpufreq-dt.c           |  3 +-
 drivers/cpufreq/cpufreq.c              | 12 ++++++
 drivers/cpufreq/imx6q-cpufreq.c        |  2 +-
 drivers/cpufreq/mediatek-cpufreq.c     |  3 +-
 drivers/cpufreq/omap-cpufreq.c         |  2 +-
 drivers/cpufreq/qcom-cpufreq-hw.c      |  3 +-
 drivers/cpufreq/scmi-cpufreq.c         | 55 +++++++++++++++-----------
 drivers/cpufreq/scpi-cpufreq.c         |  3 +-
 drivers/cpufreq/vexpress-spc-cpufreq.c |  3 +-
 include/linux/cpufreq.h                | 14 +++++++
 10 files changed, 65 insertions(+), 35 deletions(-)

-- 
2.31.1.272.g89b43f80a514

[PATCH V2 1/9] cpufreq: Auto-register with energy model if asked

[Viresh Kumar]

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 new callback, which will be called, if present, by the cpufreq
core at the right moment (more on that in the code's comment). Also
provide a generic implementation that uses dev_pm_opp_of_register_em().

This also allows us to register with the EM at a later point of time,
compared to ->init(), from where the EM core can access cpufreq policy
directly using cpufreq_cpu_get() type of helpers and perform other work,
like marking few frequencies inefficient, this will be done separately.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 12 ++++++++++++
 include/linux/cpufreq.h   | 14 ++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 06c526d66dd3..75974e7d6cc5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1493,6 +1493,18 @@ static int cpufreq_online(unsigned int cpu)
 		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
 
+	/*
+	 * Register with the energy model before sched_cpufreq_governor_change()
+	 * is called, which will result in rebuilding of the sched domains,
+	 * which should only be done once the energy model is properly
+	 * initialized for the policy first.
+	 *
+	 * Also, this should be called before the policy is registered with
+	 * cooling framework.
+	 */
+	if (cpufreq_driver->register_em)
+		cpufreq_driver->register_em(policy);
+
 	ret = cpufreq_init_policy(policy);
 	if (ret) {
 		pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n",
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 9fd719475fcd..1295621f6c28 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -9,10 +9,12 @@
 #define _LINUX_CPUFREQ_H
 
 #include <linux/clk.h>
+#include <linux/cpu.h>
 #include <linux/cpumask.h>
 #include <linux/completion.h>
 #include <linux/kobject.h>
 #include <linux/notifier.h>
+#include <linux/pm_opp.h>
 #include <linux/pm_qos.h>
 #include <linux/spinlock.h>
 #include <linux/sysfs.h>
@@ -373,6 +375,12 @@ struct cpufreq_driver {
 	/* platform specific boost support code */
 	bool		boost_enabled;
 	int		(*set_boost)(struct cpufreq_policy *policy, int state);
+
+	/*
+	 * Set by drivers that want the core to automatically register the
+	 * policy's devices with Energy Model.
+	 */
+	void		(*register_em)(struct cpufreq_policy *policy);
 };
 
 /* flags */
@@ -1046,4 +1054,10 @@ unsigned int cpufreq_generic_get(unsigned int cpu);
 void cpufreq_generic_init(struct cpufreq_policy *policy,
 		struct cpufreq_frequency_table *table,
 		unsigned int transition_latency);
+
+static inline void cpufreq_register_em_with_opp(struct cpufreq_policy *policy)
+{
+	dev_pm_opp_of_register_em(get_cpu_device(policy->cpu),
+				  policy->related_cpus);
+}
 #endif /* _LINUX_CPUFREQ_H */
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 1/9] cpufreq: Auto-register with energy model if asked

[Quentin Perret]

On Wednesday 11 Aug 2021 at 17:28:39 (+0530), 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 new callback, which will be called, if present, by the cpufreq
> core at the right moment (more on that in the code's comment). Also
> provide a generic implementation that uses dev_pm_opp_of_register_em().
> 
> This also allows us to register with the EM at a later point of time,
> compared to ->init(), from where the EM core can access cpufreq policy
> directly using cpufreq_cpu_get() type of helpers and perform other work,
> like marking few frequencies inefficient, this will be done separately.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 12 ++++++++++++
>  include/linux/cpufreq.h   | 14 ++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 06c526d66dd3..75974e7d6cc5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1493,6 +1493,18 @@ static int cpufreq_online(unsigned int cpu)
>  		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>  	}
>  
> +	/*
> +	 * Register with the energy model before sched_cpufreq_governor_change()
> +	 * is called, which will result in rebuilding of the sched domains,
> +	 * which should only be done once the energy model is properly
> +	 * initialized for the policy first.
> +	 *
> +	 * Also, this should be called before the policy is registered with
> +	 * cooling framework.
> +	 */
> +	if (cpufreq_driver->register_em)
> +		cpufreq_driver->register_em(policy);

Maybe move that to the 'if (new_policy)' block above? There is currently
no need to re-register the EM on CPU hotplug.

>  	ret = cpufreq_init_policy(policy);
>  	if (ret) {
>  		pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n",
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9fd719475fcd..1295621f6c28 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -9,10 +9,12 @@
>  #define _LINUX_CPUFREQ_H
>  
>  #include <linux/clk.h>
> +#include <linux/cpu.h>
>  #include <linux/cpumask.h>
>  #include <linux/completion.h>
>  #include <linux/kobject.h>
>  #include <linux/notifier.h>
> +#include <linux/pm_opp.h>
>  #include <linux/pm_qos.h>
>  #include <linux/spinlock.h>
>  #include <linux/sysfs.h>
> @@ -373,6 +375,12 @@ struct cpufreq_driver {
>  	/* platform specific boost support code */
>  	bool		boost_enabled;
>  	int		(*set_boost)(struct cpufreq_policy *policy, int state);
> +
> +	/*
> +	 * Set by drivers that want the core to automatically register the
> +	 * policy's devices with Energy Model.
> +	 */
> +	void		(*register_em)(struct cpufreq_policy *policy);
>  };
>  
>  /* flags */
> @@ -1046,4 +1054,10 @@ unsigned int cpufreq_generic_get(unsigned int cpu);
>  void cpufreq_generic_init(struct cpufreq_policy *policy,
>  		struct cpufreq_frequency_table *table,
>  		unsigned int transition_latency);
> +
> +static inline void cpufreq_register_em_with_opp(struct cpufreq_policy *policy)
> +{
> +	dev_pm_opp_of_register_em(get_cpu_device(policy->cpu),
> +				  policy->related_cpus);
> +}

I was thinking this could go in pm_opp.h instead, but it doesn't really
matter.

So, with the first comment above fixed:

Reviewed-by: Quentin Perret <qperret@google.com>

Thanks,
Quentin

Re: [PATCH V2 1/9] cpufreq: Auto-register with energy model if asked

[Lukasz Luba]

On 8/11/21 12:58 PM, 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 new callback, which will be called, if present, by the cpufreq
> core at the right moment (more on that in the code's comment). Also
> provide a generic implementation that uses dev_pm_opp_of_register_em().
> 
> This also allows us to register with the EM at a later point of time,
> compared to ->init(), from where the EM core can access cpufreq policy
> directly using cpufreq_cpu_get() type of helpers and perform other work,
> like marking few frequencies inefficient, this will be done separately.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/cpufreq.c | 12 ++++++++++++
>   include/linux/cpufreq.h   | 14 ++++++++++++++
>   2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 06c526d66dd3..75974e7d6cc5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1493,6 +1493,18 @@ static int cpufreq_online(unsigned int cpu)
>   		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
>   	}
>   
> +	/*
> +	 * Register with the energy model before sched_cpufreq_governor_change()
> +	 * is called, which will result in rebuilding of the sched domains,
> +	 * which should only be done once the energy model is properly
> +	 * initialized for the policy first.
> +	 *
> +	 * Also, this should be called before the policy is registered with
> +	 * cooling framework.
> +	 */
> +	if (cpufreq_driver->register_em)
> +		cpufreq_driver->register_em(policy);
> +
>   	ret = cpufreq_init_policy(policy);
>   	if (ret) {
>   		pr_err("%s: Failed to initialize policy for cpu: %d (%d)\n",
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 9fd719475fcd..1295621f6c28 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -9,10 +9,12 @@
>   #define _LINUX_CPUFREQ_H
>   
>   #include <linux/clk.h>
> +#include <linux/cpu.h>
>   #include <linux/cpumask.h>
>   #include <linux/completion.h>
>   #include <linux/kobject.h>
>   #include <linux/notifier.h>
> +#include <linux/pm_opp.h>
>   #include <linux/pm_qos.h>
>   #include <linux/spinlock.h>
>   #include <linux/sysfs.h>
> @@ -373,6 +375,12 @@ struct cpufreq_driver {
>   	/* platform specific boost support code */
>   	bool		boost_enabled;
>   	int		(*set_boost)(struct cpufreq_policy *policy, int state);
> +
> +	/*
> +	 * Set by drivers that want the core to automatically register the
> +	 * policy's devices with Energy Model.

It covers one use case. I would add also something about customized EM
setup, which might be provided by drivers and implemented then in this
callback. It won't be only for automatic registration.


> +	 */
> +	void		(*register_em)(struct cpufreq_policy *policy);
>   };
>   
>   /* flags */
> @@ -1046,4 +1054,10 @@ unsigned int cpufreq_generic_get(unsigned int cpu);
>   void cpufreq_generic_init(struct cpufreq_policy *policy,
>   		struct cpufreq_frequency_table *table,
>   		unsigned int transition_latency);
> +
> +static inline void cpufreq_register_em_with_opp(struct cpufreq_policy *policy)
> +{
> +	dev_pm_opp_of_register_em(get_cpu_device(policy->cpu),
> +				  policy->related_cpus);
> +}
>   #endif /* _LINUX_CPUFREQ_H */
> 

There rest is OK,

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

[PATCH V2 2/9] cpufreq: dt: Use auto-registration for energy model

[Viresh Kumar]

Set the newly added .register_em() callback with
cpufreq_register_em_with_opp() to automatically register with the EM
core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq-dt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq-dt.c b/drivers/cpufreq/cpufreq-dt.c
index ece52863ba62..8fcaba541539 100644
--- a/drivers/cpufreq/cpufreq-dt.c
+++ b/drivers/cpufreq/cpufreq-dt.c
@@ -143,8 +143,6 @@ static int cpufreq_init(struct cpufreq_policy *policy)
 		cpufreq_dt_attr[1] = &cpufreq_freq_attr_scaling_boost_freqs;
 	}
 
-	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
-
 	return 0;
 
 out_clk_put:
@@ -184,6 +182,7 @@ static struct cpufreq_driver dt_cpufreq_driver = {
 	.exit = cpufreq_exit,
 	.online = cpufreq_online,
 	.offline = cpufreq_offline,
+	.register_em = cpufreq_register_em_with_opp,
 	.name = "cpufreq-dt",
 	.attr = cpufreq_dt_attr,
 	.suspend = cpufreq_generic_suspend,
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 3/9] cpufreq: imx6q: Use auto-registration for energy model

[Viresh Kumar]

Set the newly added .register_em() callback with
cpufreq_register_em_with_opp() to automatically register with the EM
core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/imx6q-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 5bf5fc759881..90beb26ed34e 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -192,7 +192,6 @@ static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
 	policy->clk = clks[ARM].clk;
 	cpufreq_generic_init(policy, freq_table, transition_latency);
 	policy->suspend_freq = max_freq;
-	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
 
 	return 0;
 }
@@ -204,6 +203,7 @@ static struct cpufreq_driver imx6q_cpufreq_driver = {
 	.target_index = imx6q_set_target,
 	.get = cpufreq_generic_get,
 	.init = imx6q_cpufreq_init,
+	.register_em = cpufreq_register_em_with_opp,
 	.name = "imx6q-cpufreq",
 	.attr = cpufreq_generic_attr,
 	.suspend = cpufreq_generic_suspend,
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 4/9] cpufreq: mediatek: Use auto-registration for energy model

[Viresh Kumar]

Set the newly added .register_em() callback with
cpufreq_register_em_with_opp() to automatically register with the EM
core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/mediatek-cpufreq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/mediatek-cpufreq.c b/drivers/cpufreq/mediatek-cpufreq.c
index 87019d5a9547..866163883b48 100644
--- a/drivers/cpufreq/mediatek-cpufreq.c
+++ b/drivers/cpufreq/mediatek-cpufreq.c
@@ -448,8 +448,6 @@ static int mtk_cpufreq_init(struct cpufreq_policy *policy)
 	policy->driver_data = info;
 	policy->clk = info->cpu_clk;
 
-	dev_pm_opp_of_register_em(info->cpu_dev, policy->cpus);
-
 	return 0;
 }
 
@@ -471,6 +469,7 @@ static struct cpufreq_driver mtk_cpufreq_driver = {
 	.get = cpufreq_generic_get,
 	.init = mtk_cpufreq_init,
 	.exit = mtk_cpufreq_exit,
+	.register_em = cpufreq_register_em_with_opp,
 	.name = "mtk-cpufreq",
 	.attr = cpufreq_generic_attr,
 };
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 5/9] cpufreq: omap: Use auto-registration for energy model

[Viresh Kumar]

Set the newly added .register_em() callback with
cpufreq_register_em_with_opp() to automatically register with the EM
core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/omap-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index e035ee216b0f..1b50df06c6bc 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -131,7 +131,6 @@ static int omap_cpu_init(struct cpufreq_policy *policy)
 
 	/* FIXME: what's the actual transition time? */
 	cpufreq_generic_init(policy, freq_table, 300 * 1000);
-	dev_pm_opp_of_register_em(mpu_dev, policy->cpus);
 
 	return 0;
 }
@@ -150,6 +149,7 @@ static struct cpufreq_driver omap_driver = {
 	.get		= cpufreq_generic_get,
 	.init		= omap_cpu_init,
 	.exit		= omap_cpu_exit,
+	.register_em	= cpufreq_register_em_with_opp,
 	.name		= "omap",
 	.attr		= cpufreq_generic_attr,
 };
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 6/9] cpufreq: qcom-cpufreq-hw: Use auto-registration for energy model

[Viresh Kumar]

Set the newly added .register_em() callback with
cpufreq_register_em_with_opp() to automatically register with the EM
core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index f86859bf76f1..c2e71c430fbf 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -362,8 +362,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 		goto error;
 	}
 
-	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
-
 	if (policy_has_boost_freq(policy)) {
 		ret = cpufreq_enable_boost_support();
 		if (ret)
@@ -412,6 +410,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 	.get		= qcom_cpufreq_hw_get,
 	.init		= qcom_cpufreq_hw_cpu_init,
 	.exit		= qcom_cpufreq_hw_cpu_exit,
+	.register_em	= cpufreq_register_em_with_opp,
 	.fast_switch    = qcom_cpufreq_hw_fast_switch,
 	.name		= "qcom-cpufreq-hw",
 	.attr		= qcom_cpufreq_hw_attr,
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 7/9] cpufreq: scpi: Use auto-registration for energy model

[Viresh Kumar]

Set the newly added .register_em() callback with
cpufreq_register_em_with_opp() to automatically register with the EM
core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/scpi-cpufreq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/scpi-cpufreq.c b/drivers/cpufreq/scpi-cpufreq.c
index d6a698a1b5d1..bda3e7d42964 100644
--- a/drivers/cpufreq/scpi-cpufreq.c
+++ b/drivers/cpufreq/scpi-cpufreq.c
@@ -163,8 +163,6 @@ static int scpi_cpufreq_init(struct cpufreq_policy *policy)
 
 	policy->fast_switch_possible = false;
 
-	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
-
 	return 0;
 
 out_free_cpufreq_table:
@@ -200,6 +198,7 @@ static struct cpufreq_driver scpi_cpufreq_driver = {
 	.init	= scpi_cpufreq_init,
 	.exit	= scpi_cpufreq_exit,
 	.target_index	= scpi_cpufreq_set_target,
+	.register_em	= cpufreq_register_em_with_opp,
 };
 
 static int scpi_cpufreq_probe(struct platform_device *pdev)
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 8/9] cpufreq: vexpress: Use auto-registration for energy model

[Viresh Kumar]

Set the newly added .register_em() callback with
cpufreq_register_em_with_opp() to automatically register with the EM
core.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/vexpress-spc-cpufreq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/vexpress-spc-cpufreq.c b/drivers/cpufreq/vexpress-spc-cpufreq.c
index ab56813b7256..284b6bd040b1 100644
--- a/drivers/cpufreq/vexpress-spc-cpufreq.c
+++ b/drivers/cpufreq/vexpress-spc-cpufreq.c
@@ -440,8 +440,6 @@ static int ve_spc_cpufreq_init(struct cpufreq_policy *policy)
 	policy->freq_table = freq_table[cur_cluster];
 	policy->cpuinfo.transition_latency = 1000000; /* 1 ms */
 
-	dev_pm_opp_of_register_em(cpu_dev, policy->cpus);
-
 	if (is_bL_switching_enabled())
 		per_cpu(cpu_last_req_freq, policy->cpu) =
 						clk_get_cpu_rate(policy->cpu);
@@ -475,6 +473,7 @@ static struct cpufreq_driver ve_spc_cpufreq_driver = {
 	.get			= ve_spc_cpufreq_get_rate,
 	.init			= ve_spc_cpufreq_init,
 	.exit			= ve_spc_cpufreq_exit,
+	.register_em		= cpufreq_register_em_with_opp,
 	.attr			= cpufreq_generic_attr,
 };
 
-- 
2.31.1.272.g89b43f80a514

[PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Viresh Kumar]

Set the newly added .register_em() callback to register with the EM
after the cpufreq policy is properly initialized.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
 1 file changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index 75f818d04b48..b916c9e22921 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -22,7 +22,9 @@
 
 struct scmi_data {
 	int domain_id;
+	int nr_opp;
 	struct device *cpu_dev;
+	cpumask_var_t opp_shared_cpus;
 };
 
 static struct scmi_protocol_handle *ph;
@@ -123,9 +125,6 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	struct device *cpu_dev;
 	struct scmi_data *priv;
 	struct cpufreq_frequency_table *freq_table;
-	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
-	cpumask_var_t opp_shared_cpus;
-	bool power_scale_mw;
 
 	cpu_dev = get_cpu_device(policy->cpu);
 	if (!cpu_dev) {
@@ -133,9 +132,15 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 		return -ENODEV;
 	}
 
-	if (!zalloc_cpumask_var(&opp_shared_cpus, GFP_KERNEL))
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
 
+	if (!zalloc_cpumask_var(&priv->opp_shared_cpus, GFP_KERNEL)) {
+		ret = -ENOMEM;
+		goto out_free_priv;
+	}
+
 	/* Obtain CPUs that share SCMI performance controls */
 	ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus);
 	if (ret) {
@@ -148,14 +153,14 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	 * The OPP 'sharing cpus' info may come from DT through an empty opp
 	 * table and opp-shared.
 	 */
-	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, opp_shared_cpus);
-	if (ret || !cpumask_weight(opp_shared_cpus)) {
+	ret = dev_pm_opp_of_get_sharing_cpus(cpu_dev, priv->opp_shared_cpus);
+	if (ret || !cpumask_weight(priv->opp_shared_cpus)) {
 		/*
 		 * Either opp-table is not set or no opp-shared was found.
 		 * Use the CPU mask from SCMI to designate CPUs sharing an OPP
 		 * table.
 		 */
-		cpumask_copy(opp_shared_cpus, policy->cpus);
+		cpumask_copy(priv->opp_shared_cpus, policy->cpus);
 	}
 
 	 /*
@@ -180,7 +185,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 			goto out_free_opp;
 		}
 
-		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, opp_shared_cpus);
+		ret = dev_pm_opp_set_sharing_cpus(cpu_dev, priv->opp_shared_cpus);
 		if (ret) {
 			dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
 				__func__, ret);
@@ -188,21 +193,13 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 			goto out_free_opp;
 		}
 
-		power_scale_mw = perf_ops->power_scale_mw_get(ph);
-		em_dev_register_perf_domain(cpu_dev, nr_opp, &em_cb,
-					    opp_shared_cpus, power_scale_mw);
-	}
-
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		ret = -ENOMEM;
-		goto out_free_opp;
+		priv->nr_opp = nr_opp;
 	}
 
 	ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
 	if (ret) {
 		dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
-		goto out_free_priv;
+		goto out_free_opp;
 	}
 
 	priv->cpu_dev = cpu_dev;
@@ -223,17 +220,16 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy)
 	policy->fast_switch_possible =
 		perf_ops->fast_switch_possible(ph, cpu_dev);
 
-	free_cpumask_var(opp_shared_cpus);
 	return 0;
 
-out_free_priv:
-	kfree(priv);
-
 out_free_opp:
 	dev_pm_opp_remove_all_dynamic(cpu_dev);
 
 out_free_cpumask:
-	free_cpumask_var(opp_shared_cpus);
+	free_cpumask_var(priv->opp_shared_cpus);
+
+out_free_priv:
+	kfree(priv);
 
 	return ret;
 }
@@ -244,11 +240,23 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy)
 
 	dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table);
 	dev_pm_opp_remove_all_dynamic(priv->cpu_dev);
+	free_cpumask_var(priv->opp_shared_cpus);
 	kfree(priv);
 
 	return 0;
 }
 
+static void scmi_cpufreq_register_em(struct cpufreq_policy *policy)
+{
+	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
+	bool power_scale_mw = perf_ops->power_scale_mw_get(ph);
+	struct scmi_data *priv = policy->driver_data;
+
+	em_dev_register_perf_domain(get_cpu_device(policy->cpu), priv->nr_opp,
+				    &em_cb, priv->opp_shared_cpus,
+				    power_scale_mw);
+}
+
 static struct cpufreq_driver scmi_cpufreq_driver = {
 	.name	= "scmi",
 	.flags	= CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
@@ -261,6 +269,7 @@ static struct cpufreq_driver scmi_cpufreq_driver = {
 	.get	= scmi_cpufreq_get_rate,
 	.init	= scmi_cpufreq_init,
 	.exit	= scmi_cpufreq_exit,
+	.register_em	= scmi_cpufreq_register_em,
 };
 
 static int scmi_cpufreq_probe(struct scmi_device *sdev)
-- 
2.31.1.272.g89b43f80a514

Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Quentin Perret]

On Wednesday 11 Aug 2021 at 17:28:47 (+0530), Viresh Kumar wrote:
> Set the newly added .register_em() callback to register with the EM
> after the cpufreq policy is properly initialized.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
>  1 file changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index 75f818d04b48..b916c9e22921 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -22,7 +22,9 @@
>  
>  struct scmi_data {
>  	int domain_id;
> +	int nr_opp;
>  	struct device *cpu_dev;
> +	cpumask_var_t opp_shared_cpus;

Can we use policy->related_cpus and friends directly in the callback
instead? That should simplify the patch a bit.

Also, we can probably afford calling dev_pm_opp_get_opp_count() from the
em_register callback as it is not a hot path, which would avoid wasting
some 'resident' memory here that is only used during init.

Thanks,
Quentin

Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Lukasz Luba]

On 8/11/21 2:17 PM, Quentin Perret wrote:
> On Wednesday 11 Aug 2021 at 17:28:47 (+0530), Viresh Kumar wrote:
>> Set the newly added .register_em() callback to register with the EM
>> after the cpufreq policy is properly initialized.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>   drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
>>   1 file changed, 32 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>> index 75f818d04b48..b916c9e22921 100644
>> --- a/drivers/cpufreq/scmi-cpufreq.c
>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>> @@ -22,7 +22,9 @@
>>   
>>   struct scmi_data {
>>   	int domain_id;
>> +	int nr_opp;
>>   	struct device *cpu_dev;
>> +	cpumask_var_t opp_shared_cpus;
> 
> Can we use policy->related_cpus and friends directly in the callback

Unfortunately not. This tricky setup code was introduced because we may
have a platform with per-CPU policy, so single bit set in
policy->related_cpus, but we want EAS to be still working on set
of CPUs. That's why we construct temporary cpumask and pass it to EM.

> instead? That should simplify the patch a bit.
> 
> Also, we can probably afford calling dev_pm_opp_get_opp_count() from the
> em_register callback as it is not a hot path, which would avoid wasting
> some 'resident' memory here that is only used during init.
> 
> Thanks,
> Quentin
> 

Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Quentin Perret]

On Wednesday 11 Aug 2021 at 15:09:13 (+0100), Lukasz Luba wrote:
> 
> 
> On 8/11/21 2:17 PM, Quentin Perret wrote:
> > On Wednesday 11 Aug 2021 at 17:28:47 (+0530), Viresh Kumar wrote:
> > > Set the newly added .register_em() callback to register with the EM
> > > after the cpufreq policy is properly initialized.
> > > 
> > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > > ---
> > >   drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
> > >   1 file changed, 32 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> > > index 75f818d04b48..b916c9e22921 100644
> > > --- a/drivers/cpufreq/scmi-cpufreq.c
> > > +++ b/drivers/cpufreq/scmi-cpufreq.c
> > > @@ -22,7 +22,9 @@
> > >   struct scmi_data {
> > >   	int domain_id;
> > > +	int nr_opp;
> > >   	struct device *cpu_dev;
> > > +	cpumask_var_t opp_shared_cpus;
> > 
> > Can we use policy->related_cpus and friends directly in the callback
> 
> Unfortunately not. This tricky setup code was introduced because we may
> have a platform with per-CPU policy, so single bit set in
> policy->related_cpus, but we want EAS to be still working on set
> of CPUs. That's why we construct temporary cpumask and pass it to EM.

Aha, I see this now. Hmm, those platforms better have AMUs then,
otherwise PELT signals will be wonky ...

I was going to suggest using dev_pm_opp_get_sharing_cpus() from the
callback instead, but maybe that's overkill as we'd need to allocate a
temporary cpumask and all. So n/m this patch should be fine as is.

Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Lukasz Luba]

On 8/11/21 3:39 PM, Quentin Perret wrote:
> On Wednesday 11 Aug 2021 at 15:09:13 (+0100), Lukasz Luba wrote:
>>
>>
>> On 8/11/21 2:17 PM, Quentin Perret wrote:
>>> On Wednesday 11 Aug 2021 at 17:28:47 (+0530), Viresh Kumar wrote:
>>>> Set the newly added .register_em() callback to register with the EM
>>>> after the cpufreq policy is properly initialized.
>>>>
>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> ---
>>>>    drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
>>>>    1 file changed, 32 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
>>>> index 75f818d04b48..b916c9e22921 100644
>>>> --- a/drivers/cpufreq/scmi-cpufreq.c
>>>> +++ b/drivers/cpufreq/scmi-cpufreq.c
>>>> @@ -22,7 +22,9 @@
>>>>    struct scmi_data {
>>>>    	int domain_id;
>>>> +	int nr_opp;
>>>>    	struct device *cpu_dev;
>>>> +	cpumask_var_t opp_shared_cpus;
>>>
>>> Can we use policy->related_cpus and friends directly in the callback
>>
>> Unfortunately not. This tricky setup code was introduced because we may
>> have a platform with per-CPU policy, so single bit set in
>> policy->related_cpus, but we want EAS to be still working on set
>> of CPUs. That's why we construct temporary cpumask and pass it to EM.
> 
> Aha, I see this now. Hmm, those platforms better have AMUs then,
> otherwise PELT signals will be wonky ...

That's the plan, to have the AMUs. We suggest that, but reality would
tell... ;)

> 
> I was going to suggest using dev_pm_opp_get_sharing_cpus() from the
> callback instead, but maybe that's overkill as we'd need to allocate a
> temporary cpumask and all. So n/m this patch should be fine as is.
> 

Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Viresh Kumar]

On 11-08-21, 14:17, Quentin Perret wrote:
> Also, we can probably afford calling dev_pm_opp_get_opp_count() from the
> em_register callback as it is not a hot path, which would avoid wasting
> some 'resident' memory here that is only used during init.

We also need to make sure that OPPs are available in init(), else we
fail. So, we can't really move that out.

-- 
viresh

Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Lukasz Luba]

On 8/11/21 12:58 PM, Viresh Kumar wrote:
> Set the newly added .register_em() callback to register with the EM
> after the cpufreq policy is properly initialized.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>   drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
>   1 file changed, 32 insertions(+), 23 deletions(-)

>   
> +static void scmi_cpufreq_register_em(struct cpufreq_policy *policy)
> +{
> +	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
> +	bool power_scale_mw = perf_ops->power_scale_mw_get(ph);
> +	struct scmi_data *priv = policy->driver_data;
> +
> +	em_dev_register_perf_domain(get_cpu_device(policy->cpu), priv->nr_opp,
> +				    &em_cb, priv->opp_shared_cpus,
> +				    power_scale_mw);

I would free the priv->opp_shared_cpus mask here, since we don't
need it anymore and memory can be reclaimed. Don't forget this
setup would be called N CPUs times, on this per-CPU policy platform.

If freed here, then also there wouldn't be a need to free it in
scmi_cpufreq_exit() so you can remove it from there.

> +}
> +
>   static struct cpufreq_driver scmi_cpufreq_driver = {
>   	.name	= "scmi",
>   	.flags	= CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
> @@ -261,6 +269,7 @@ static struct cpufreq_driver scmi_cpufreq_driver = {
>   	.get	= scmi_cpufreq_get_rate,
>   	.init	= scmi_cpufreq_init,
>   	.exit	= scmi_cpufreq_exit,
> +	.register_em	= scmi_cpufreq_register_em,
>   };
>   
>   static int scmi_cpufreq_probe(struct scmi_device *sdev)
> 

I will test&review this patch on Monday when I re-flash custom FW to my
Juno (just to be sure that this per-CPU cpufreq + shared EM/EAS works).

Re: [PATCH V2 9/9] cpufreq: scmi: Use .register_em() callback

[Viresh Kumar]

On 11-08-21, 17:32, Lukasz Luba wrote:
> 
> 
> On 8/11/21 12:58 PM, Viresh Kumar wrote:
> > Set the newly added .register_em() callback to register with the EM
> > after the cpufreq policy is properly initialized.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >   drivers/cpufreq/scmi-cpufreq.c | 55 ++++++++++++++++++++--------------
> >   1 file changed, 32 insertions(+), 23 deletions(-)
> 
> > +static void scmi_cpufreq_register_em(struct cpufreq_policy *policy)
> > +{
> > +	struct em_data_callback em_cb = EM_DATA_CB(scmi_get_cpu_power);
> > +	bool power_scale_mw = perf_ops->power_scale_mw_get(ph);
> > +	struct scmi_data *priv = policy->driver_data;
> > +
> > +	em_dev_register_perf_domain(get_cpu_device(policy->cpu), priv->nr_opp,
> > +				    &em_cb, priv->opp_shared_cpus,
> > +				    power_scale_mw);
> 
> I would free the priv->opp_shared_cpus mask here, since we don't
> need it anymore and memory can be reclaimed.

Yes, we don't need it anymore, but this isn't a good place to undo
what init() has done. Moreover, it is possible that register_em() may
not get called at all, if some error has occurred after init() has
successfully returned. It is always better to use exit() for such
things. It won't hurt a lot to keep this around anyway.

> Don't forget this
> setup would be called N CPUs times, on this per-CPU policy platform.

Yes, but EM will just ignore this call. Though I have made a change
here now to check for non-zero nr_opp to avoid the unnecessary call.

> If freed here, then also there wouldn't be a need to free it in
> scmi_cpufreq_exit() so you can remove it from there.

-- 
viresh