diff mbox series

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

Message ID 87fecd84e3f6ff6f153be14b0d53de93c0b04ae6.1628579170.git.viresh.kumar@linaro.org
State New
Headers show
Series cpufreq: Auto-register with energy model | expand

Commit Message

Viresh Kumar Aug. 10, 2021, 7:36 a.m. UTC 
Use the CPUFREQ_REGISTER_WITH_EM flag to allow cpufreq core to
automatically register with the energy model.

This allows removal of boiler plate code from the driver and fixes the
unregistration part as well.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
 drivers/cpufreq/vexpress-spc-cpufreq.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

-- 
2.31.1.272.g89b43f80a514

Comments

Lukasz Luba Aug. 10, 2021, 10:05 a.m. UTC | #1 
On 8/10/21 8:36 AM, Viresh Kumar wrote:
> Use the CPUFREQ_REGISTER_WITH_EM flag to allow cpufreq core to

> automatically register with the energy model.

> 

> This allows removal of boiler plate code from the driver and fixes the

> unregistration part as well.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>   drivers/cpufreq/vexpress-spc-cpufreq.c | 5 ++---

>   1 file changed, 2 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/cpufreq/vexpress-spc-cpufreq.c b/drivers/cpufreq/vexpress-spc-cpufreq.c

> index 51dfa9ae6cf5..28c4c3254337 100644

> --- a/drivers/cpufreq/vexpress-spc-cpufreq.c

> +++ b/drivers/cpufreq/vexpress-spc-cpufreq.c

> @@ -442,8 +442,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);

> @@ -487,7 +485,8 @@ static void ve_spc_cpufreq_ready(struct cpufreq_policy *policy)

>   static struct cpufreq_driver ve_spc_cpufreq_driver = {

>   	.name			= "vexpress-spc",

>   	.flags			= CPUFREQ_HAVE_GOVERNOR_PER_POLICY |

> -					CPUFREQ_NEED_INITIAL_FREQ_CHECK,

> +				  CPUFREQ_NEED_INITIAL_FREQ_CHECK |

> +				  CPUFREQ_REGISTER_WITH_EM,

>   	.verify			= cpufreq_generic_frequency_table_verify,

>   	.target_index		= ve_spc_cpufreq_set_target,

>   	.get			= ve_spc_cpufreq_get_rate,

> 


I can see that this driver calls explicitly the
of_cpufreq_cooling_register()
It does this in the cpufreq_driver->ready() callback
implementation: ve_spc_cpufreq_ready()

With that in mind, the new code in the patch 1/8, which
registers the EM, should be called even earlier, above:
---------------------8<---------------------------------
/* Callback for handling stuff after policy is ready */
	if (cpufreq_driver->ready)
		cpufreq_driver->ready(policy);
------------------->8----------------------------------

This also triggered a question:
If this new flag can be set in the cpufreq driver which hasn't set
CPUFREQ_IS_COOLING_DEV
?
I can only see one driver (this one in the patch) which has such
configuration.
Viresh Kumar Aug. 10, 2021, 10:06 a.m. UTC | #2 
On 10-08-21, 11:05, Lukasz Luba wrote:
> I can see that this driver calls explicitly the

> of_cpufreq_cooling_register()

> It does this in the cpufreq_driver->ready() callback

> implementation: ve_spc_cpufreq_ready()

> 

> With that in mind, the new code in the patch 1/8, which

> registers the EM, should be called even earlier, above:

> ---------------------8<---------------------------------

> /* Callback for handling stuff after policy is ready */

> 	if (cpufreq_driver->ready)

> 		cpufreq_driver->ready(policy);

> ------------------->8----------------------------------


Thanks. I will look at this sequencing issue again.

> This also triggered a question:

> If this new flag can be set in the cpufreq driver which hasn't set

> CPUFREQ_IS_COOLING_DEV

> ?


Why not ?

> I can only see one driver (this one in the patch) which has such

> configuration.


-- 
viresh
Lukasz Luba Aug. 10, 2021, 10:11 a.m. UTC | #3 
On 8/10/21 11:06 AM, Viresh Kumar wrote:
> On 10-08-21, 11:05, Lukasz Luba wrote:

>> I can see that this driver calls explicitly the

>> of_cpufreq_cooling_register()

>> It does this in the cpufreq_driver->ready() callback

>> implementation: ve_spc_cpufreq_ready()

>>

>> With that in mind, the new code in the patch 1/8, which

>> registers the EM, should be called even earlier, above:

>> ---------------------8<---------------------------------

>> /* Callback for handling stuff after policy is ready */

>> 	if (cpufreq_driver->ready)

>> 		cpufreq_driver->ready(policy);

>> ------------------->8----------------------------------

> 

> Thanks. I will look at this sequencing issue again.

> 

>> This also triggered a question:

>> If this new flag can be set in the cpufreq driver which hasn't set

>> CPUFREQ_IS_COOLING_DEV

>> ?

> 

> Why not ?


I thought someone could try to call cpufreq_cooling_register()
from the cpufreq driver init function, but it's not possible. I have
just checked that, so should be good with these two flags being
independent and working fine.

> 

>> I can only see one driver (this one in the patch) which has such

>> configuration.

>
Viresh Kumar Aug. 10, 2021, 10:12 a.m. UTC | #4 
On 10-08-21, 11:11, Lukasz Luba wrote:
> 

> 

> On 8/10/21 11:06 AM, Viresh Kumar wrote:

> > On 10-08-21, 11:05, Lukasz Luba wrote:

> > > I can see that this driver calls explicitly the

> > > of_cpufreq_cooling_register()

> > > It does this in the cpufreq_driver->ready() callback

> > > implementation: ve_spc_cpufreq_ready()

> > > 

> > > With that in mind, the new code in the patch 1/8, which

> > > registers the EM, should be called even earlier, above:

> > > ---------------------8<---------------------------------

> > > /* Callback for handling stuff after policy is ready */

> > > 	if (cpufreq_driver->ready)

> > > 		cpufreq_driver->ready(policy);

> > > ------------------->8----------------------------------

> > 

> > Thanks. I will look at this sequencing issue again.

> > 

> > > This also triggered a question:

> > > If this new flag can be set in the cpufreq driver which hasn't set

> > > CPUFREQ_IS_COOLING_DEV

> > > ?

> > 

> > Why not ?

> 

> I thought someone could try to call cpufreq_cooling_register()

> from the cpufreq driver init function, but it's not possible. I have

> just checked that, so should be good with these two flags being

> independent and working fine.


Ahh, I see. Great.

-- 
viresh
Lukasz Luba Aug. 10, 2021, 10:30 a.m. UTC | #5 
On 8/10/21 8:36 AM, Viresh Kumar wrote:
> Use the CPUFREQ_REGISTER_WITH_EM flag to allow cpufreq core to

> automatically register with the energy model.

> 

> This allows removal of boiler plate code from the driver and fixes the

> unregistration part as well.

> 

> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

> ---

>   drivers/cpufreq/vexpress-spc-cpufreq.c | 5 ++---

>   1 file changed, 2 insertions(+), 3 deletions(-)

> 

> diff --git a/drivers/cpufreq/vexpress-spc-cpufreq.c b/drivers/cpufreq/vexpress-spc-cpufreq.c

> index 51dfa9ae6cf5..28c4c3254337 100644

> --- a/drivers/cpufreq/vexpress-spc-cpufreq.c

> +++ b/drivers/cpufreq/vexpress-spc-cpufreq.c

> @@ -442,8 +442,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);

> @@ -487,7 +485,8 @@ static void ve_spc_cpufreq_ready(struct cpufreq_policy *policy)

>   static struct cpufreq_driver ve_spc_cpufreq_driver = {

>   	.name			= "vexpress-spc",

>   	.flags			= CPUFREQ_HAVE_GOVERNOR_PER_POLICY |

> -					CPUFREQ_NEED_INITIAL_FREQ_CHECK,

> +				  CPUFREQ_NEED_INITIAL_FREQ_CHECK |

> +				  CPUFREQ_REGISTER_WITH_EM,

>   	.verify			= cpufreq_generic_frequency_table_verify,

>   	.target_index		= ve_spc_cpufreq_set_target,

>   	.get			= ve_spc_cpufreq_get_rate,

> 


With the change for patch 1/8 the we discussed below this patch 8/8,
it LGTM

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
Sudeep Holla Aug. 11, 2021, 2:40 a.m. UTC | #6 
On Tue, Aug 10, 2021 at 01:06:55PM +0530, Viresh Kumar wrote:
> Use the CPUFREQ_REGISTER_WITH_EM flag to allow cpufreq core to

> automatically register with the energy model.

> 

> This allows removal of boiler plate code from the driver and fixes the

> unregistration part as well.

> 


Acked-by: Sudeep Holla <sudeep.holla@arm.com>


-- 
Regards,
Sudeep
diff mbox series

Patch

diff --git a/drivers/cpufreq/vexpress-spc-cpufreq.c b/drivers/cpufreq/vexpress-spc-cpufreq.c
index 51dfa9ae6cf5..28c4c3254337 100644
--- a/drivers/cpufreq/vexpress-spc-cpufreq.c
+++ b/drivers/cpufreq/vexpress-spc-cpufreq.c
@@ -442,8 +442,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);
@@ -487,7 +485,8 @@  static void ve_spc_cpufreq_ready(struct cpufreq_policy *policy)
 static struct cpufreq_driver ve_spc_cpufreq_driver = {
 	.name			= "vexpress-spc",
 	.flags			= CPUFREQ_HAVE_GOVERNOR_PER_POLICY |
-					CPUFREQ_NEED_INITIAL_FREQ_CHECK,
+				  CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+				  CPUFREQ_REGISTER_WITH_EM,
 	.verify			= cpufreq_generic_frequency_table_verify,
 	.target_index		= ve_spc_cpufreq_set_target,
 	.get			= ve_spc_cpufreq_get_rate,