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

[Mario Limonciello <superm1@kernel.org>]

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