Received-SPF: pass (google.com: domain of linux-kernel+bounces-171680-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <9255af0c-4574-49fe-922f-56885ae2d55b@amd.com>
Date: Tue, 7 May 2024 10:19:10 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core
 topology for highest performance initialization
To: Perry Yuan <perry.yuan@amd.com>, rafael.j.wysocki@intel.com,
 viresh.kumar@linaro.org, Ray.Huang@amd.com, gautham.shenoy@amd.com,
 Borislav.Petkov@amd.com
Cc: 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
References: <cover.1715065568.git.perry.yuan@amd.com>
 <731a28ea8dda4ca1db64f673c87770de4646290b.1715065568.git.perry.yuan@amd.com>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <731a28ea8dda4ca1db64f673c87770de4646290b.1715065568.git.perry.yuan@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
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?Z1hoZlJGWUI1clQ3RWpkRlp5RW16VFdtSTVLVEtuOTB4V0w5Zmc5S2VIOUVQ?=
 =?utf-8?B?ck9lWFgvTWExNXFGZnFPc3VGcnNtOThaVWpERVhVVjdLV1puOW9RalNwWVZs?=
 =?utf-8?B?a3F6OFJsdEEvckhBYmhoWmRrUXpIR3hldlJPdDBacEI2U1pjZmdweEJvUlN6?=
 =?utf-8?B?WVh5TUlyV0xYeTNTQnVrZng4Z0pXMm9JdFZjUnRucVk0K3E5c2JvVGFSb0hp?=
 =?utf-8?B?dXFsYVllQjJqdVdOeEI3bng3b2Y0T1cwanRFaG85Vk14SThWcUJGTkVBY3Mw?=
 =?utf-8?B?TkZIcTM2cE11WEpQMmMxM1VEMldFZmxGOVBxUEVxK2p5RG1xcXNDcUFPcFBQ?=
 =?utf-8?B?cXdxWTVnaGY0alNhZUptMGswclU0VVROdUI3clFGT3h5VElpVGtRdFkzZ3ZH?=
 =?utf-8?B?Rzk3SWRaUGpNcDNGOHlMbzI5Y1NSSTNEUVJpZitKWUtDVHZ3dXZ4M3FHdktS?=
 =?utf-8?B?a1JDK0poUWxhUStDVlVHWlV5WFpuNHVaZjNHY0R5TThWTW53dUtMb21ralZp?=
 =?utf-8?B?NDEvK015VU5aRDU1YzFhaXUwZVh0ZWdCcktBWmcyMURnRlB6YjdqcTFQeTJl?=
 =?utf-8?B?R0VwZlgySm5hMVJEMVNBQVVMK0lYRTcxNHRieFhBcC9jMG45TXpPRVlqOENP?=
 =?utf-8?B?RDN0cFE0bTFnZ0pOOEpScjZxNzdzNzA1RGtrVlRPWU1vaGwrcGhxSk1udUcr?=
 =?utf-8?B?eko1dXZBcUVUK3YvdllXSm5iNFRmdjN1dURxRkV2MFRqWTQ3NHA4VVJEYnYw?=
 =?utf-8?B?WkRhUjlmVnVYSnhjY092OFhML2lXeTh6RmZTNHIvQnc2THI3cHlwOVczNEhn?=
 =?utf-8?B?WXhnR2Y4RDNYdTJCRmMvK2RZQ3VITVlGSjBMTUdnYlpMOStPbjBRTzlrVWJo?=
 =?utf-8?B?bUZVbXF3RmErckhuR1Ezai9jYjJJZ2ZXb0EybHRoTU1QN0UzVkxDQStzd2lS?=
 =?utf-8?B?dVFKWkJBT3VacUt4TzY4QjVJa1U3dTlLUW1QeTM3K1NpZ2VsS2t1TnhxN1g4?=
 =?utf-8?B?TWFEM2FzdTJoYmFtKzhOZjhIU25OWmp1SkFBZ3pqSFlIK1FFc1kxTVVxb0NW?=
 =?utf-8?B?Z3NjWWFJN0ppa1ZyMGNiOUNyOVRhNDF2aEFrKzFRMUh2Z1lINkFFWWlwSE4x?=
 =?utf-8?B?RGU2N3orUWlIREl2RmxpTDQ2S2lFcFZRM1RYb3E3b1o2cU9mUzhNQ2ZDQnZV?=
 =?utf-8?B?TVhudUhxSzM0WEk3Wmx5N2pUOFQzZkZWL3JOelpJZFJBYTIvdUpRMnByMmJX?=
 =?utf-8?B?b0MrS2ZNLzk5bjRucUNwYjdlQUVsdm1DNlpsOTVKblZYSWVRM09LOGhMTjBV?=
 =?utf-8?B?ZzBDd1M0Ly9tUUZoVnFZd3FvS09SSjVUU3M5c3htcmxhN29oK1R4UnhHSXFw?=
 =?utf-8?B?dlFiU3E0R2JweWI5dnRxUnBENy9SdUFGQUZtM3NOMGtyYWZrK0VYaEhqbnZy?=
 =?utf-8?B?VDFlWDlxZEhSalpyMjVuMWNTT1VtNFFnZFVKR1dUcmVqeHFjdm5GVFg5RFpZ?=
 =?utf-8?B?WkFyb1RDeStQcS8wSWFScTRRODNaVUJzQmR5ZHVrcnRhN2dweG1iYmNhb0ha?=
 =?utf-8?B?cUovWm9FN0FSb0FFUXVqR29JWC9GSzMyTXJNTldtN1pDclc2bzFCVzdRVDgx?=
 =?utf-8?B?WlE0Q1dFUWVGZjlSQ0Y0NVFpVDkxcHdLNGNwR0ZLd2ZkSmpmQnJsdWpJa3ZQ?=
 =?utf-8?B?WFovZTRxWVpXNmtXQUk2UEY1S1FuVEFSUWgvTHpBY2l0QWp1NGt3WDE3dTMx?=
 =?utf-8?B?RnJrZXZMRGsvU3FISVRUM3VwZjQ1UXdpTTY0WW15NmNlT2VvWEFRKzhPZSta?=
 =?utf-8?B?b0V5aFdvUXpBRWlTbmE5SGdxYndjREhDS2kyUWFzUXJPbjJFckhzRlloUklN?=
 =?utf-8?B?OC9zYmdUbzl4dXRLMDEzR1FSaWh6bWJua2x0RmphMlZFU09KSnpONkxXb0NV?=
 =?utf-8?B?ejJyQ2s4VWZKMURFWEtHZ1hWZDZBbGVIcnNDSXdxNnNYaXZKaVd4UG1Pd2ZP?=
 =?utf-8?B?cWsyZGZRTmh6bXZIencvd1NEVkxkYmVRa1BMWEVXbE9sSGtrRnRHbEJDU3Z4?=
 =?utf-8?B?Rk9DWjY5RldVanRiWHNDaURsZnpQakFpUW5jeTNPWXZFRHhuYWVqSjNyOGo0?=
 =?utf-8?Q?yHOj2DYWhOpM91HinfUX1WjAY?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 572ed7d4-23be-4e66-1f82-08dc6ea90cf2
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 15:19:12.7413
 (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: vVq5KQUjS0Mpn2VwGRx3ADZw3BYLt3KUMObcVEEqjZs/1dzfbpjKogQmzl9/lYTjI3RCwpTQRXcYS5J00yHC9w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR12MB6668

On 5/7/2024 02:15, Perry Yuan wrote:
> Introduces an optimization to the AMD-Pstate driver by implementing
> a heterogeneous core topology for the initialization of the highest
> performance value while driver loading.
> There are two type cores designed including performance core and
> efficiency Core. each core type has different highest performance value

Capitalize the "E" in each.

> and highest frequency initialized by power firmware, `amd_pstate` driver

Three things:

1. Rather than "power firmware" you should just say "platform".
2. I would use "configured" instead of "initialized"
3. This is a run on sentence.

Here's my proposed change.

Each core type has different highest performance and frequency values 
configured by the platform.  The `amd_pstate` driver needs to identify
the type of core to correctly set an appropriate highest perf value.

> need to identify the core types and set correct highest perf value.
> 
> X86_FEATURE_HETERO_CORE_TOPOLOGY is used to identify whether the
> processor support heterogeneous core type by reading CPUID leaf
> Fn_0x80000026_EAX and bit 30. if the bit is set as one, then amd_pstate
> driver will check EBX 30:28 bits to get the core type.
> 
> Value Description:
> 0h Performance Core.
> 1h Efficiency Core.
> 
> https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf

Use the word "Link:" to prefix this link.

> PDF p274
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   arch/x86/include/asm/processor.h |  2 ++
>   arch/x86/kernel/cpu/amd.c        | 19 ++++++++++
>   drivers/cpufreq/amd-pstate.c     | 62 ++++++++++++++++++++++++--------
>   include/linux/amd-pstate.h       |  8 +++++
>   4 files changed, 77 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index 811548f131f4..30d1900bb7e0 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -683,10 +683,12 @@ static inline u32 per_cpu_l2c_id(unsigned int cpu)
>   extern u32 amd_get_highest_perf(void);
>   extern void amd_clear_divider(void);
>   extern void amd_check_microcode(void);
> +extern int amd_get_this_core_type(void);
>   #else
>   static inline u32 amd_get_highest_perf(void)		{ return 0; }
>   static inline void amd_clear_divider(void)		{ }
>   static inline void amd_check_microcode(void)		{ }
> +static inline int amd_get_this_core_type(void)		{ }
>   #endif
>   
>   extern unsigned long arch_align_stack(unsigned long sp);
> diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
> index 307302af0aee..67966bdcde65 100644
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1219,3 +1219,22 @@ void noinstr amd_clear_divider(void)
>   		     :: "a" (0), "d" (0), "r" (1));
>   }
>   EXPORT_SYMBOL_GPL(amd_clear_divider);
> +
> +#define X86_CPU_TYPE_ID_SHIFT	28
> +
> +/**
> + * amd_get_this_core_type - Get the type of this heterogeneous CPU
> + *
> + * Returns the CPU type [31:28] (i.e., performance or efficient) of
> + * a CPU in the processor.
> + * If the processor has no core type support, returns -1.
> + */
> +
> +int amd_get_this_core_type(void)
> +{
> +	if (!cpu_feature_enabled(X86_FEATURE_HETERO_CORE_TOPOLOGY))
> +		return -1;
> +
> +	return cpuid_ebx(0x80000026) >> X86_CPU_TYPE_ID_SHIFT;
> +}
> +EXPORT_SYMBOL_GPL(amd_get_this_core_type);
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 7145248b38ec..7fe8a8fc6227 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -50,7 +50,9 @@
>   
>   #define AMD_PSTATE_TRANSITION_LATENCY	20000
>   #define AMD_PSTATE_TRANSITION_DELAY	1000
> -#define AMD_PSTATE_PREFCORE_THRESHOLD	166
> +#define CPPC_HIGHEST_PERF_EFFICIENT		132
> +#define CPPC_HIGHEST_PERF_PERFORMANCE		196
> +#define CPPC_HIGHEST_PERF_DEFAULT		166
>   
>   /*
>    * TODO: We need more time to fine tune processors with shared memory solution
> @@ -326,6 +328,49 @@ static inline int amd_pstate_enable(bool enable)
>   	return static_call(amd_pstate_enable)(enable);
>   }
>   
> +static void get_this_core_type(void *data)
> +{
> +	int *cpu_type = data;
> +
> +	*cpu_type = amd_get_this_core_type();
> +}

Does this really need a static function calling a static function just 
to set a variable?

I would think that you can simplify it something like this:

int amd_get_core_type(void *data)
{
	int *type = data;

	if (!type)
		return -EINVAL;
	if (!cpu_feature_enabled(X86_FEATURE_HETERO_CORE_TOPOLOGY)) {
		*type = -1;
		return -ENODEV;
	}
	*type = cpuid_ebx(0x80000026) >> X86_CPU_TYPE_ID_SHIFT;
}

And then amd_pstate_get_cpu_type() can call:

smp_call_function_single(cpu, amd_get_core_type, &cpu_type, 1);

> +
> +static int amd_pstate_get_cpu_type(int cpu)
> +{
> +	int cpu_type = 0;
> +
> +	smp_call_function_single(cpu, get_this_core_type, &cpu_type, 1);
> +
> +	return cpu_type;
> +}
> +
> +static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
> +{
> +	u32 highest_perf;
> +	int core_type;
> +
> +	core_type = amd_pstate_get_cpu_type(cpudata->cpu);
> +	pr_debug("core_type %d found\n", core_type);
> +
> +	switch (core_type) {
> +	case CPU_CORE_TYPE_NO_HETERO_SUP:
> +		highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
> +		break;
> +	case CPU_CORE_TYPE_PERFORMANCE:
> +		highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;
> +		break;
> +	case CPU_CORE_TYPE_EFFICIENCY:
> +		highest_perf = CPPC_HIGHEST_PERF_EFFICIENT;
> +		break;
> +	default:
> +		highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
> +		WARN_ONCE(true, "WARNING: Undefined core type found");
> +		break;
> +	}
> +
> +    return highest_perf;
> +}
> +
>   static int pstate_init_perf(struct amd_cpudata *cpudata)
>   {
>   	u64 cap1;
> @@ -336,15 +381,7 @@ static int pstate_init_perf(struct amd_cpudata *cpudata)
>   	if (ret)
>   		return ret;
>   
> -	/* For platforms that do not support the preferred core feature, the
> -	 * highest_pef may be configured with 166 or 255, to avoid max frequency
> -	 * calculated wrongly. we take the AMD_CPPC_HIGHEST_PERF(cap1) value as
> -	 * the default max perf.
> -	 */
> -	if (cpudata->hw_prefcore)
> -		highest_perf = AMD_PSTATE_PREFCORE_THRESHOLD;
> -	else
> -		highest_perf = AMD_CPPC_HIGHEST_PERF(cap1);
> +	highest_perf = amd_pstate_highest_perf_set(cpudata);

I think it would be a good idea to follow up later on validate the 
selections of this logic from amd-pstate-ut.c.

>   
>   	WRITE_ONCE(cpudata->highest_perf, highest_perf);
>   	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
> @@ -365,10 +402,7 @@ static int cppc_init_perf(struct amd_cpudata *cpudata)
>   	if (ret)
>   		return ret;
>   
> -	if (cpudata->hw_prefcore)
> -		highest_perf = AMD_PSTATE_PREFCORE_THRESHOLD;
> -	else
> -		highest_perf = cppc_perf.highest_perf;
> +	highest_perf = amd_pstate_highest_perf_set(cpudata);
>   
>   	WRITE_ONCE(cpudata->highest_perf, highest_perf);
>   	WRITE_ONCE(cpudata->max_limit_perf, highest_perf);
> diff --git a/include/linux/amd-pstate.h b/include/linux/amd-pstate.h
> index d58fc022ec46..869d916003f1 100644
> --- a/include/linux/amd-pstate.h
> +++ b/include/linux/amd-pstate.h
> @@ -134,4 +134,12 @@ struct quirk_entry {
>   	u32 lowest_freq;
>   };
>   
> +/* defined by CPUID_Fn80000026_EBX BIT [31:28] */
> +enum amd_core_type {
> +	CPU_CORE_TYPE_NO_HETERO_SUP = -1,
> +	CPU_CORE_TYPE_PERFORMANCE = 0,
> +	CPU_CORE_TYPE_EFFICIENCY = 1,
> +	CPU_CORE_TYPE_UNDEFINED = 2,
> +};
> +
>   #endif /* _LINUX_AMD_PSTATE_H */