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

[Mario Limonciello]

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@xxxxxxx>
> >
> > Use the perf_to_freq helpers to calculate this on the fly.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > --
> > 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);