Re: [PATCH v14 5/5] cpufreq: Only disable boost during cpu online when using frequency tables

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/25/2024 06:55, Gautham R.Shenoy wrote:
> 
> Hello Mario,
> 
> Mario Limonciello <mario.limonciello@amd.com> writes:
> 
>> The behavior introduced in commit f37a4d6b4a2c ("cpufreq: Fix per-policy
>> boost behavior on SoCs using cpufreq_boost_set_sw()") sets up the boost
>> policy incorrectly when boost has been enabled by the platform firmware
>> initially even if a driver sets the policy up.
>>
>> This is because there are no discrete entries in the frequency table.
>> Update that code to only run when a frequency table is present.
> 
> Thanks for this fix.
> 
> 
> There are also drivers such as acpi-cpufreq which have a frequency
> table, but the boost Pstates are not advertised. Thus none of the table
> entries have CPUFREQ_BOOST_FREQ set.
> 
> 
>>
>> Fixes: f37a4d6b4a2c ("cpufreq: Fix per-policy boost behavior on SoCs using cpufreq_boost_set_sw()")
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> Cc: Sibi Sankar <quic_sibis@quicinc.com>
>> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Dhruva Gole <d-gole@ti.com>
>> Cc: Yipeng Zou <zouyipeng@huawei.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>   drivers/cpufreq/cpufreq.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 1fdabb660231..32c119614710 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1430,7 +1430,8 @@ static int cpufreq_online(unsigned int cpu)
>>   		}
>>   
>>   		/* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
>> -		policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
>> +		if (policy->freq_table)
>> +			policy->boost_enabled = cpufreq_boost_enabled() && policy_has_boost_freq(policy);
>>   
>>   		/*
>>   		 * The initialization has succeeded and the policy is
>>   		online.
> 
> 
> How about something like the following:
> 
> ---
> 
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 37f1cdf46d29..be5f4d3e9c1d 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -140,6 +140,7 @@ static int set_boost(struct cpufreq_policy *policy, int val)
>          pr_debug("CPU %*pbl: Core Boosting %s.\n",
>                   cpumask_pr_args(policy->cpus), str_enabled_disabled(val));
>   
> +       policy->boost_enabled = val;
>          return 0;
>   }
>   
> diff --git a/drivers/cpufreq/freq_table.c b/drivers/cpufreq/freq_table.c
> index 10e80d912b8d..f604389b9cd6 100644
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -18,6 +18,10 @@ bool policy_has_boost_freq(struct cpufreq_policy *policy)
>   {
>          struct cpufreq_frequency_table *pos, *table = policy->freq_table;
>   
> +       /* The driver has explicitly advertised the boost-capabilities */
> +       if (policy->boost_enabled)
> +               return true;
> +
>          if (!table)
>                  return false;
>   
> 
> 
> 
>> -- 
>> 2.43.0

Yeah I think this works too.  Let's see see what Viresh thinks.