Re: [PATCH v6 2/6] cpufreq:amd-pstate: fix the nominal freq value set

[Ugwekar, Dhananjay]

On 2/8/2024 11:37 AM, Gautham R. Shenoy wrote:
> On Thu, Feb 08, 2024 at 11:46:29AM +0800, Perry Yuan wrote:
>> Address an untested error where the nominal_freq was returned in KHz
>> instead of the correct MHz units, this oversight led to a wrong
>> nominal_freq set and resued, it will cause the max frequency of core to
>> be initialized with a wrong frequency value.
> 
> The _CPC object on my Zen2 EPYC server which has CPPC v3, I see :
>             0x00000190,			//LowestFrequency 
>             0x000007D0			//NominalFrequency
>         })
>     }
>  
> where 0x7D0 is 2000 Mhz. Thus the value returend by cppc_perf.nominal_freq is in Mhz.
> 
> In this patch, we are changing making amd_get_nominal_freq() return
> 2000 (Mhz) instead of 2000000 (Khz) as it was doing previously.
> 
> amd_get_min_freq(), amd_get_max_freq(),
> amd_get_lowest_nonlinear_perf() -all these functions return the
> frequency in Khz units.
> 
> These functions are used the initialize the value of
> cpudata->max_freq, cpu_data->min_freq, .... all of which are in Khz
> units.
> 
> In, amd_pstate_set_boost(), we have:
>  
>  
> 	if (state)
> 		policy->cpuinfo.max_freq = cpudata->max_freq;  <---- In Khz
> 	else
> 		policy->cpuinfo.max_freq = cpudata->nominal_freq; <--- Now in Mhz
> 
> Since policy->cpuinfo.max_freq is expected to be in Khz, isn't this
> patch introducing an error ? Or perhaps this patch series is based on
> another patchset ?
> 
> --
> Thanks and Regards
> gautham.
> 
>
I can confirm on Gautham's comment, when boost is disabled with the scaling 
driver set as amd-pstate, the max frequency reported is incorrect, as it 
shows the frequency in MHz.

I tested this patch on Zen4 Genoa server platform, below are the 
inconsistent min and max frequencies I observed.

[cpufreq]# grep . *freq                                                                                                                                                                                                                                                     
amd_pstate_lowest_nonlinear_freq:1804000                                                                                                                                                                                                                                                  
amd_pstate_max_freq:3514000
cpuinfo_max_freq:2151                                                                                                                                                                                                                                                                     
cpuinfo_min_freq:400000                                                                                                                                                                                                                                                                   
scaling_cur_freq:2151                                                                                                                                                                                                                                                                     
scaling_max_freq:2151                                                                                                                                                                                                                                                                     
scaling_min_freq:2151                                                                                                                                                                                                                                                                     
[cpufreq]# pwd        
/sys/devices/system/cpu/cpu0/cpufreq

According to Documentation/admin-guide/pm/cpufreq.rst, all the frequency 
values need to be in KHz.

Thanks,
Dhananjay

>>
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State driver to support future processors")
>> Reviewed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
>> Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
>> ---
>>  drivers/cpufreq/amd-pstate.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 08e112444c27..ac7faa98a450 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -640,8 +640,7 @@ static int amd_get_nominal_freq(struct amd_cpudata *cpudata)
>>  	if (ret)
>>  		return ret;
>>  
>> -	/* Switch to khz */
>> -	return cppc_perf.nominal_freq * 1000;
>> +	return cppc_perf.nominal_freq;
>>  }
>>  
>>  static int amd_get_lowest_nonlinear_freq(struct amd_cpudata *cpudata)
>> -- 
>> 2.34.1
>>