Received-SPF: pass (google.com: domain of linux-kernel+bounces-171632-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <66bfb2f2-074c-4d13-a3d7-56c6558f7010@amd.com>
Date: Tue, 7 May 2024 09:55:32 -0500
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 03/11] cpufreq: amd-pstate: add debug message while CPPC
 is supported and disabled by SBIOS
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>
 <0ec1b203bc83c1acdaf1c5a2f3e51031b4374da2.1715065568.git.perry.yuan@amd.com>
Content-Language: en-US
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <0ec1b203bc83c1acdaf1c5a2f3e51031b4374da2.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?a2ZSTmNadFVjWWJiNjJPWVN3b1ZIUEdrblRLeFhxczROSUc1cjFHSm5TY1dG?=
 =?utf-8?B?ZkVZeVZHYmNYdmhMWnp1WGx6dk0xbVN4a0pMZ0hpWlNLTGNKZEs0S1BZZFJU?=
 =?utf-8?B?K01WeStVWjRrYkk2UUpncDFvVjJhdW1aZDlORHhMZUI1VjRWWWJhWWN6OTNZ?=
 =?utf-8?B?STMxczNTNTd6N2JHVGRBMWxMTkhKUUNHZysvU0QxOW1ySEIxb2dWUU0zOEM0?=
 =?utf-8?B?cEM3OFZwT09YN3JxOENWT3hzYjNSLy9kajA1alNRZGI5ZDk1Tm5kQU9yRUZt?=
 =?utf-8?B?dnpqUVZMaUJ0cU9melc4dndFSzlKcmhqM3ZadnhJUDExa0ZzTjdqUXkzQVFE?=
 =?utf-8?B?ZWNHQkFGMEIwMDdNWU02RHVTbEdtQWdPaTZha1IzTWpxVzBhRHVLVmZBcWE2?=
 =?utf-8?B?dlI2cHNwSHJ5TUtYcEZTUi9meTRlbWpzU28vbVFQc1hFMVgzd0tqcWdLdjJT?=
 =?utf-8?B?Wkx0dWhoNXhLNHJHQktnTU1FaVJEMTNwNlVTVXZ3Z1ppcTNiOWdQeVN6ZXBh?=
 =?utf-8?B?aE9tVkE4dEUvcndRc0FzRSt4UkxYRU50a3dRZHpuU3lNTlpLTHgwTUZPelJG?=
 =?utf-8?B?VStqSGRyMmJJbzlYMTZiZlQrSXNpU1BIVVpuV0RvbHBaWndCb0x5V0RuNWhw?=
 =?utf-8?B?ejZCWWNnQjVqYWpFOXhmcnZ0RWo2VUN1Q2FxaEdTYzU4WHpmSE54MnJIV1kx?=
 =?utf-8?B?d2h0VTR4MXVEbWFta2VjbjhZSkNiaEhWVmpRejhVQVRyUFhPUXJsZk4rUlFC?=
 =?utf-8?B?MEZhZTBoYlJocy92R1JPUG16YTY4MmU2M0RETE5NbEUwWU1UNURWakwrSjFJ?=
 =?utf-8?B?enNwOU5oQ2dPTitEUXh0UFIrOWdJUmFQNUdrdi9RenVkZUZOV3VOaEF0UU94?=
 =?utf-8?B?Ui9wT0padnN6WXdnR3FFRmNYaFBNUzgyZlJXcEEwenVzSm4wakZXa3dwakFZ?=
 =?utf-8?B?WDJ6ZTdpR2ZWOUw3NnBLa1h0MEhTdVpmTElzUlpQTzg2NHMrdE1BbXFPMTlP?=
 =?utf-8?B?ZHBRV1hEUmhMNWNQSTdxdG5HWjRja3pDbW9ZVlMyTFpFdzd0d0p5ZnNtMEM0?=
 =?utf-8?B?dFpiYVVhcGx6Zm1VdDZ1VEdEVzlwbEVaM21PeUN5bHJNMjkrVHdqKzVvSzNq?=
 =?utf-8?B?SFFEekREcHl3MEZkWUZxZVcrZjFhZDhlYTM4clhWRmNNeHpXMU9NMEZKK2ti?=
 =?utf-8?B?dWZOOHVGNDZXSWM3RjQvYUd0Z0NTU0dQeFI3aUNHZDhWcWJEZUFVNVUydkla?=
 =?utf-8?B?cktzaGFZQ2VyNjFwNFlRVG5IY3ZDaGxUUlhqUEI4TXRaYm43TTVvcStHMWJn?=
 =?utf-8?B?UXlnRUphcW1sWWJMZmZ2dGV6M3k0dTFVaTR4Unp6U0tFUWpJMUNUSk9rbTZH?=
 =?utf-8?B?U2kxL2RyNWQ0Q25uRnZiZmNHYm1weXhiYjFXb3NIYmk3OFFRZkdkeGNHWFla?=
 =?utf-8?B?ZTdCMjVsdms0VnJCRjdCam5JZS9MQkZ2V2NSeXRnd292ZmJTaHdzb29wbjJi?=
 =?utf-8?B?cnowekNyWldtNjkremF6c2VweVBQOHpGVGE0Z2dsSi80T3Fmc0dyRm9tSTRV?=
 =?utf-8?B?NERSamxBRk1SMEthc2JsaTBkUUxadlAvTy9WWDJYWng5Znl0UVZmeG9iQXhw?=
 =?utf-8?B?c28vNS9tWGJPdlFuT2lxZWVOdlQ4UUFwZ0d3WGlWZGtsMDdrdllaYnpodDRP?=
 =?utf-8?B?aDV2RzBLZ2xEQmtUTXdlWTF3WGlwNHJTalVBR2dmQmlla2hYNm4wU2lVVmU5?=
 =?utf-8?B?OHBUeUl6ME54YjFueXMvdFVUSXd5TXF3MmNHRkl5OEpoR0VWNmZEUU5sekVq?=
 =?utf-8?B?YWJNQlVVY3JLVDRUd1pFK0MyMmVQT3NNb2hxUldZVWZyM0VqTWlUMjhJT0F0?=
 =?utf-8?B?QTV5RVlDMjhxdWF4YUxrUVErUGx1MS9HWlJnQ2VkcWgrNkZXczA4bG1Nc1RZ?=
 =?utf-8?B?TjlxdXZVbTRWYTRMRC9DbFpLRzdXYkExT1BZY2dSSUhwYTl3eW5uenRRYUFZ?=
 =?utf-8?B?eDE5L0RsaEJFeHUwL3hwOUxYa2ZqOWhWcHVrNS91U3lzRTNaaDFVVWxyYm9H?=
 =?utf-8?B?VldGamhKTFMvQ0JGeUl1cFliNkpXMEYvdDExT0NaajFyMDNiVlN0M2lkK1Fx?=
 =?utf-8?Q?ZQoIaswcgkuGrZu1GovuFzCgE?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 818996e9-fa75-47c4-49fa-08dc6ea5bfad
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 May 2024 14:55:34.5706
 (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: ZMBWvxAaKTFl0xqTD5c6MZHz+w219vCaxQO9vkcNb0GX1BQDADNRrMbluDqu4FvW292lxSj8TlnMVR4i/2WHFg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB7024

On 5/7/2024 02:15, Perry Yuan wrote:
> If CPPC feature is supported by the CPU however the CPUID flag bit is not
> set by SBIOS, the `amd_pstate` will be failed to load while system
> booting.
> So adding one more debug message to inform user to check the SBIOS setting,
> The change also can help maintainers to debug why amd_pstate driver failed
> to be loaded at system booting if the processor support CPPC.
> 
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218686
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index cb766c061c86..e94b55a7bb59 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1698,6 +1698,17 @@ static bool amd_cppc_supported(void)
>   		return false;
>   	}
>   
> +	/*
> +	 * If the CPPC flag is disabled in the BIOS for processors that support MSR-based CPPC
> +	 * the AMD Pstate driver may not function correctly.
> +	 */
> +	if ((boot_cpu_data.x86 >= 0x19) && (boot_cpu_data.x86_model >= 0x40) &&
> +			!cpu_feature_enabled(X86_FEATURE_CPPC)) {

I don't think this if statement is correct.  The intent behind is is 
family 0x19 but models 0x40 to 0x4f AFAICT and then family 0x1a all 
models right?  This message will miss any models that are 0x00 through 
0x39 in family 0x1a.

I'll give you two ideas:

What do you think about instead doing the inverse like this:

If (boot_cpu_data.x86 == 0x19) && (boot_cpu_data.x86_model < 0x40)
	return true;

if (!cpu_feature_enabled(X86_FEATURE_CPPC)) {
	pr_debug_once();
	return false;
}

return true;

Another idea can be to look at X86_FEATURE_ZEN# to decide what to do. 
For example all Zen4 and Zen5 should have the architectural MSR.

if (!cpu_feature_enabled(X86_FEATURE_CPPC)) {
	if (cpu_feature_enabled(X86_FEATURE_ZEN5) {
		// do stuff
	} else if (cpu_feature_enabled(X86_FEATURE_ZEN4) {
		// do stuff
	} else if (cpu_feature_enabled(X86_FEATURE_ZEN3) {
		// do stuff
	}
}

> +		pr_debug_once("The CPPC feature is supported but disabled by the BIOS. "
> +						"Please enable it if your BIOS has the CPPC option.\n");
> +		return false;
> +	}
> +
>   	return true;
>   }
>