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

Gautham R. Shenoy
2025-02-17T06:39:12Z

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 ?

> 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);

--
Thanks and Regards
gautham.