Re: [PATCH v2 02/17] cpufreq/amd-pstate: Drop min and max cached frequencies

Mario Limonciello
2025-02-17T21:06:58Z

On 2/17/2025 00:38, Gautham R. Shenoy wrote:
> Hello Mario,
>
> On Fri, Feb 14, 2025 at 06:52:29PM -0600, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> Use the perf_to_freq helpers to calculate this on the fly.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> --
>> v2:
>> * Keep cached limits
>> ---
>
> [..snip..]
>
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -717,7 +717,7 @@ static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
>> int ret = 0;
>>
>> nominal_freq = READ_ONCE(cpudata->nominal_freq);
>> - max_freq = READ_ONCE(cpudata->max_freq);
>> + max_freq = perf_to_freq(cpudata, READ_ONCE(cpudata->highest_perf));
>>
>> if (on)
>> policy->cpuinfo.max_freq = max_freq;
>> @@ -901,35 +901,25 @@ static u32 amd_pstate_get_transition_latency(unsigned int cpu)
>> static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>> {
>> int ret;
>> - u32 min_freq, max_freq;
>> - u32 nominal_freq, lowest_nonlinear_freq;
>> + u32 min_freq, nominal_freq, lowest_nonlinear_freq;
>> struct cppc_perf_caps cppc_perf;
>>
>> ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
>> if (ret)
>> return ret;
>>
>> - if (quirks && quirks->lowest_freq)
>> - min_freq = quirks->lowest_freq;
>> - else
>> - min_freq = cppc_perf.lowest_freq;
>> -
>> if (quirks && quirks->nominal_freq)
>> nominal_freq = quirks->nominal_freq;
>> else
>> nominal_freq = cppc_perf.nominal_freq;
>>
>> - min_freq *= 1000;
>> nominal_freq *= 1000;
>> -
>> WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
>
> So cpudata->nominal_freq will be in KHz here.
>
>
>> - WRITE_ONCE(cpudata->min_freq, min_freq);
>> -
>> - max_freq = perf_to_freq(cpudata, cpudata->highest_perf);
>> - lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
>>
>> - WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
>> - WRITE_ONCE(cpudata->max_freq, max_freq);
>> + if (quirks && quirks->lowest_freq) {
>> + min_freq = quirks->lowest_freq;
>> + } else
>> + min_freq = cppc_perf.lowest_freq;
>>
>
> Since cppc_perf exposes the frequency values in MHz, min_freq is in MHz.
>
>> /**
>> * Below values need to be initialized correctly, otherwise driver will fail to load
>> @@ -937,12 +927,15 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>> * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
>> * Check _CPC in ACPI table objects if any values are incorrect
>> */
>> - if (min_freq <= 0 || max_freq <= 0 || nominal_freq <= 0 || min_freq > max_freq) {
>> - pr_err("min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect\n",
>> - min_freq, max_freq, nominal_freq);
>> + if (nominal_freq <= 0) {
>> + pr_err("nominal_freq(%d) value is incorrect\n",
>> + nominal_freq);
>> return -EINVAL;
>> }
>>
>> + lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
>
> Since lowest_nonlinear_freq will be computed using cpudata->nominal_freq, the former will be in KHz.
>
>> + WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
>> +
>> if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq) {
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> And thus since lowest_nonlinear_freq is in KHz and min_freq is in MHz, this check will always be true.
>
> Shouldn't the min_freq be multiplied by 1000 ?
>

Yup, I think you're right. Great catch!
I'll spin this for v3.

>
>> pr_err("lowest_nonlinear_freq(%d) value is out of range [min_freq(%d), nominal_freq(%d)]\n",
>> lowest_nonlinear_freq, min_freq, nominal_freq);
>
>