diff mbox series

[06/15] cpufreq: cppc: Set policy->boost_supported

Message ID c744751c8f61cae509959270176ebdef77326ba2.1737707712.git.viresh.kumar@linaro.org (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series cpufreq: simplify boost handling | expand

Commit Message

Viresh Kumar Jan. 24, 2025, 8:58 a.m. UTC
With a later commit, the cpufreq core will call the ->set_boost()
callback only if the policy supports boost frequency. The
boost_supported flag is set by the cpufreq core if policy->freq_table is
set and one or more boost frequencies are present.

For other drivers, the flag must be set explicitly.

With this, the local variable boost_supported isn't required anymore.

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

Comments

Lifeng Zheng Feb. 6, 2025, 3:58 a.m. UTC | #1
On 2025/1/24 16:58, Viresh Kumar wrote:

> With a later commit, the cpufreq core will call the ->set_boost()
> callback only if the policy supports boost frequency. The
> boost_supported flag is set by the cpufreq core if policy->freq_table is
> set and one or more boost frequencies are present.
> 
> For other drivers, the flag must be set explicitly.
> 
> With this, the local variable boost_supported isn't required anymore.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 7fa89b601d2a..08117fb9c1eb 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -34,8 +34,6 @@
>   */
>  static LIST_HEAD(cpu_data_list);
>  
> -static bool boost_supported;
> -
>  static struct cpufreq_driver cppc_cpufreq_driver;
>  
>  #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
> @@ -653,7 +651,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  	 * is supported.
>  	 */
>  	if (caps->highest_perf > caps->nominal_perf)
> -		boost_supported = true;
> +		policy->boost_supported = true;
>  
>  	/* Set policy->cur to max now. The governors will adjust later. */
>  	policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> @@ -791,11 +789,6 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>  	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
>  	int ret;
>  
> -	if (!boost_supported) {
> -		pr_err("BOOST not supported by CPU or firmware\n");
> -		return -EINVAL;
> -	}
> -
>  	if (state)
>  		policy->max = cppc_perf_to_khz(caps, caps->highest_perf);
>  	else

I have a little question. With the old boost_supported flag as false, it
will fail when you operate the global boost flag. But if you replace it
with the per-policy boost_supported flag, the global boost_enabled flag can
be set to true without any error, even no policy's boost_enabled flag
changed. Is this interface behavior change OK?
Viresh Kumar Feb. 6, 2025, 5:27 a.m. UTC | #2
On 06-02-25, 11:58, zhenglifeng (A) wrote:
> On 2025/1/24 16:58, Viresh Kumar wrote:
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index 7fa89b601d2a..08117fb9c1eb 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -34,8 +34,6 @@
> >   */
> >  static LIST_HEAD(cpu_data_list);
> >  
> > -static bool boost_supported;
> > -
> >  static struct cpufreq_driver cppc_cpufreq_driver;
> >  
> >  #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
> > @@ -653,7 +651,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >  	 * is supported.
> >  	 */
> >  	if (caps->highest_perf > caps->nominal_perf)
> > -		boost_supported = true;
> > +		policy->boost_supported = true;
> >  
> >  	/* Set policy->cur to max now. The governors will adjust later. */
> >  	policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> > @@ -791,11 +789,6 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> >  	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
> >  	int ret;
> >  
> > -	if (!boost_supported) {
> > -		pr_err("BOOST not supported by CPU or firmware\n");
> > -		return -EINVAL;
> > -	}
> > -
> >  	if (state)
> >  		policy->max = cppc_perf_to_khz(caps, caps->highest_perf);
> >  	else
> 
> I have a little question. With the old boost_supported flag as false, it
> will fail when you operate the global boost flag. But if you replace it
> with the per-policy boost_supported flag, the global boost_enabled flag can
> be set to true without any error, even no policy's boost_enabled flag
> changed. Is this interface behavior change OK?

Right, so earlier even if a single policy didn't support boost, the code disabled
boost for all of them. Or it was rather racy, as the last policy to be
initialized will decide if boost will be supported or not. This is surely
incorrect.

The global boost flag can be enabled disabled without worrying about any
individual policy. If the policy supports boost, it will follow the global boost
here, else it shouldn't take part in the change.

So yes, the new interface does the right thing here.
Lifeng Zheng Feb. 6, 2025, 6:27 a.m. UTC | #3
On 2025/1/24 16:58, Viresh Kumar wrote:

> With a later commit, the cpufreq core will call the ->set_boost()
> callback only if the policy supports boost frequency. The
> boost_supported flag is set by the cpufreq core if policy->freq_table is
> set and one or more boost frequencies are present.
> 
> For other drivers, the flag must be set explicitly.
> 
> With this, the local variable boost_supported isn't required anymore.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cppc_cpufreq.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index 7fa89b601d2a..08117fb9c1eb 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -34,8 +34,6 @@
>   */
>  static LIST_HEAD(cpu_data_list);
>  
> -static bool boost_supported;
> -
>  static struct cpufreq_driver cppc_cpufreq_driver;
>  
>  #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
> @@ -653,7 +651,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  	 * is supported.
>  	 */
>  	if (caps->highest_perf > caps->nominal_perf)
> -		boost_supported = true;
> +		policy->boost_supported = true;
>  
>  	/* Set policy->cur to max now. The governors will adjust later. */
>  	policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
> @@ -791,11 +789,6 @@ static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
>  	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
>  	int ret;
>  
> -	if (!boost_supported) {
> -		pr_err("BOOST not supported by CPU or firmware\n");
> -		return -EINVAL;
> -	}
> -
>  	if (state)
>  		policy->max = cppc_perf_to_khz(caps, caps->highest_perf);
>  	else

Reviewed-by: Lifeng Zheng <zhenglifeng1@huawei.com>
diff mbox series

Patch

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 7fa89b601d2a..08117fb9c1eb 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -34,8 +34,6 @@ 
  */
 static LIST_HEAD(cpu_data_list);
 
-static bool boost_supported;
-
 static struct cpufreq_driver cppc_cpufreq_driver;
 
 #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
@@ -653,7 +651,7 @@  static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	 * is supported.
 	 */
 	if (caps->highest_perf > caps->nominal_perf)
-		boost_supported = true;
+		policy->boost_supported = true;
 
 	/* Set policy->cur to max now. The governors will adjust later. */
 	policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
@@ -791,11 +789,6 @@  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
 	struct cppc_perf_caps *caps = &cpu_data->perf_caps;
 	int ret;
 
-	if (!boost_supported) {
-		pr_err("BOOST not supported by CPU or firmware\n");
-		return -EINVAL;
-	}
-
 	if (state)
 		policy->max = cppc_perf_to_khz(caps, caps->highest_perf);
 	else