Received-SPF: pass (google.com: domain of linux-kernel+bounces-98658-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <1aecf2fc-2ea4-46ec-aaf2-0dbbb11b5f8b@amd.com>
Date: Mon, 11 Mar 2024 15:28:40 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 2/6] cpufreq:amd-pstate: fix the nominal freq value set
Content-Language: en-US
To: Perry Yuan <perry.yuan@amd.com>
Cc: rafael.j.wysocki@intel.com, Mario.Limonciello@amd.com,
 viresh.kumar@linaro.org, Ray.Huang@amd.com, Borislav.Petkov@amd.com,
 Alexander.Deucher@amd.com, Xinmei.Huang@amd.com, Xiaojian.Du@amd.com,
 Li.Meng@amd.com, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
 "Gautham R. Shenoy" <gautham.shenoy@amd.com>
References: <cover.1707363758.git.perry.yuan@amd.com>
 <ebbe3741501a31b1ffcea208393cdd8929df6455.1707363758.git.perry.yuan@amd.com>
 <ZcRvoYZKdUEjBUHp@BLR-5CG11610CF.amd.com>
From: "Ugwekar, Dhananjay" <Dhananjay.Ugwekar@amd.com>
In-Reply-To: <ZcRvoYZKdUEjBUHp@BLR-5CG11610CF.amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?aXUzb1VGektoNEFSaDZ0RVgyRjIzdnp3aGFkRDVIeldiclNaNUNrbTZQZmxE?=
 =?utf-8?B?Y0NWelhJN1YvTmZTaFJCdFh2cUtyT3lVTFpGR0NNc1k1SGMwUUFqVU9IOTgr?=
 =?utf-8?B?c1dZL1ZBYUxGOURScEkwZmFLS0dsbGZGVXZ0MXI0R2JUSFFMWFl3UjZBYVRz?=
 =?utf-8?B?QlN5bnpSTzFLTHl5ZkFtK2E1N0Qxbm82ZEN4ZUhnL1NFU0xSSGVxb0h5SXJk?=
 =?utf-8?B?QTdBSU1kZlVpY0tIOFhLMEhzMDNlNjB4S05oaU02bUtYaUlUUXB3UTBHTHZH?=
 =?utf-8?B?ekt5RWhtMTdMcXJHVVV6UkpacURQa0FCWEE3OVhPWFluNndzSGFWM0dxaThU?=
 =?utf-8?B?N0tvd0F4ZnphbEhRYWpiK0NHTDFSMFE2UDhwVmtoMnc3RTl0UFM3STVBOTEx?=
 =?utf-8?B?eW1zc3liWThQOFBQMGVVOUpRaVQ5T3Z6ckVQL2l5OHRrU1NvN1E0S1V3blQ4?=
 =?utf-8?B?d2RaK3BXS1NVSVQ3QmVZOThodEhldU1qeEZPbWVKRUlHeU5DM1VSaVJwSFk5?=
 =?utf-8?B?YzdkUXlqNXZqT2JiL3huRm8yVnM5R3JPeEFVcmxPaUY5bEM1d3pSSzBtR0N1?=
 =?utf-8?B?R05xZkw4ZWUvb21COWdOYUZHYmoyMU51ZHpPUGtKbDNnRnJzVElCRDZQTklp?=
 =?utf-8?B?MFB6eGFJOXFoNjNuUldjZmVMQlRheUtjcUZuQTVlQmVERm8xNWE0NkdmMTZt?=
 =?utf-8?B?dmlwWUVMSGI0ZzlXWGMxQjhnQWRickN4cDgyVkpSdkdFOUxNZzNIODB6eDNw?=
 =?utf-8?B?Z2VrSktiTk4raXJHOHZ0cDlyR3BOWk1kNFVVbS9yVHlXLzBFdXBZN093Z3Rv?=
 =?utf-8?B?YXlmbWlhTUhKK0orVlQ4ZWtoRTFSbDdsemhQdHFWMXl3K0g4UkVhZXI5NTZ3?=
 =?utf-8?B?OU5BWUFtUkZEZWZTOTBhVW45VEd6VnBYenQ0TFJFVzBUTmk5UDJWUU5oS3RC?=
 =?utf-8?B?NUpzM05ZU0pzNWdVTmlMV2Q4OFhQOWtlZXpQOEQzMzMzc2VURnVLS29Za0ho?=
 =?utf-8?B?UHZ4NWx2NUNzS0kxMVF5UGJTVUtyT0NYS21nVm9WWUlEUG9iVDhYREVWbVhp?=
 =?utf-8?B?L3NZTGZpaDZOVXZRbG5SUTR3WlhKSHhlOEUxcEl2V1lZVU9zNGFKU0VXWmV0?=
 =?utf-8?B?RmpHVjJJZ3p0RHNqTm43Q252bXdRbmR1a1plQUJhYWw1Undoc05aT2Y0bWo4?=
 =?utf-8?B?YkpraTE4UFNIeG9zY0t3Uzh0VWx6b2syMk95SEVqUEtyNVU3aXNWVklYMm4z?=
 =?utf-8?B?Z3IvcytVS3lGcnN6RkRsWVh5azIzdStRWElUOXdJS2ZoSnJwRy90bTZTT0lB?=
 =?utf-8?B?a1A5REFzT1p1RWdXcHg4WE5YRDViQklGRFFieEJpMnVFZEVPcUFvbTd0Z2J5?=
 =?utf-8?B?UHV0RWZOZG5RdUlxZWZiYzBsNXpCSmNQcFh6a1lVTkoveW80NTk4VytFbk9x?=
 =?utf-8?B?SGNCU0VWdXMxdExrRndkdGhBWEFZblpTeUUwanlLS0hsbHdjU2lDcXFOTzhJ?=
 =?utf-8?B?SWlwYXEwdXRqNmw2a0M1MTYzSHZtTHh2c1RtOWRGVktxMHA3SzgyaGYyeU9X?=
 =?utf-8?B?VERxdVR6M1NVYXFSYjgvWGNhdVFPZE9oTW9NRFIyZVFrbDdjZkROS1BWZXpv?=
 =?utf-8?B?WjFNZGhBZUVLRW5GbDRFM1RNM0ZhT1pFcHRJTnlUSmMrNnRrSVJkc0RERGsz?=
 =?utf-8?B?ZXRNTXp4MHFPellraFRHekxGOWVPamEybU9IWXMwZ0Z1ajBZTjRDdXgzOXdU?=
 =?utf-8?B?K1N5VWFtOEhCYTczMFhRekdTamJod1hYUFlMc2tKdHAxWjZjdFVFOUM2Z2Y2?=
 =?utf-8?B?ZnIvV09rKyswTXFPS1JMZ0N5T0JoT2dkSXliRW9uaHFSQlQ4K214ZzhUT3Fk?=
 =?utf-8?B?OTM0Qk9ySGxnbmNqQUNxQTZrc0pxMHFHTExWUmRpbVBjVTZWOXdVd04ybkhu?=
 =?utf-8?B?dEtxVmd3TEJucXl4eitwQ2FQazE2NVcvRUl1ZEQwU3NxVWR0eXp3dmxoVmhC?=
 =?utf-8?B?SWF1WUVRMW1NbE9XWVZXb04rUUJzVW1ZVTMwWjlOSmtRZmltOEpaWW9vVzNY?=
 =?utf-8?B?L0paTUxvdDhaTEJtaXREU081dzVvQXpTbkNrNWxscGcwN3JUMWtHQnRhT0Qw?=
 =?utf-8?Q?TElj8FxpAUgx4+akMB+Pbxwhl?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f1c84a1e-1f18-4707-fe7d-08dc41b1da2f
X-MS-Exchange-CrossTenant-AuthSource: LV8PR12MB9207.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2024 09:58:50.8413
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: FyP5lgX0gaSA6RcLJlLVleIZaoSHRZRWirL/rwNtPrrdO1N7TzHZ6DcPbt4p5FW87Pf2roKRekVzy07cAcQvog==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR12MB5874


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@vger.kernel.org
>> Fixes: ec437d71db7 ("cpufreq: amd-pstate: Introduce a new AMD P-State driver to support future processors")
>> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
>> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
>> ---
>>  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
>>