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

Mario Limonciello
2024-06-25T12:33:00Z

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.