[PATCH 00/11] AMD Pstate Driver Fixes and Improvements

[PATCH 00/11] AMD Pstate Driver Fixes and Improvements

[Perry Yuan]

Hello everyone,

This patchset addresses critical issues and enhances performance settings for CPUs
with heterogeneous core types in the AMD pstate driver. 
Specifically, it resolves problems related to calculating the highest performance
and frequency on the latest CPUs with preferred cores. 
Additionally, the patchset includes documentation improvements in amd-pstate.rst,
offering a comprehensive guide covering topics such as recommended reboot requirements
during driver switching, debugging procedures for driver loading failures.

Your feedback and suggestions for improvement are highly appreciated. 
Please review the patches and provide your valuable input.

Thank you.

Best regards,
Perry.

Perry Yuan (11):
  cpufreq: amd-pstate: optimiza the initial frequency values
    verification
  cpufreq: amd-pstate: show CPPC debug message if CPPC is not supported
  cpufreq: amd-pstate: add debug message while CPPC is supported and
    disabled by SBIOS
  Documentation: PM: amd-pstate: introducing recommended reboot
    requirement during driver switch
  Documentation: PM: amd-pstate: add debugging section for driver
    loading failure
  Documentation: PM: amd-pstate: add guide mode to the Operation mode
  cpufreq: amd-pstate: switch boot_cpu_has() to cpu_feature_enabled()
  x86/cpufeatures: Add feature bits for AMD heterogeneous processor
  cpufreq: amd-pstate: implement heterogeneous core topology for highest
    performance initialization
  cpufreq: amd-pstate: fix the highest frequency issue which limit
    performance
  cpufreq: amd-pstate: automatically load pstate driver by default

 Documentation/admin-guide/pm/amd-pstate.rst |  19 +-
 arch/x86/include/asm/cpufeatures.h          |   1 +
 arch/x86/include/asm/processor.h            |   2 +
 arch/x86/kernel/cpu/amd.c                   |  19 ++
 arch/x86/kernel/cpu/scattered.c             |   1 +
 drivers/cpufreq/amd-pstate.c                | 196 ++++++++++++++------
 include/linux/amd-pstate.h                  |   8 +
 7 files changed, 185 insertions(+), 61 deletions(-)

-- 
2.34.1

[PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification

[Perry Yuan]

To enhance the debugging capability of the driver loading failure for
broken CPPC ACPI tables, we can optimize the expression by moving the
verification of `min_freq`, `nominal_freq`, and other dependency values
to the `amd_pstate_init_freq()` function where they are initialized.
If any of these values are incorrect, the `amd-pstate` driver will not be registered.

By ensuring that these values are correct before they are used, it will facilitate
the debugging process when encountering driver loading failures due to faulty CPPC
ACPI tables from BIOS

Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 2db095867d03..be7f2d3c86b6 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -873,6 +873,24 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
 	WRITE_ONCE(cpudata->max_freq, max_freq);
 
+	/**
+	 * Below values need to be initialized correctly, otherwise driver will be failed to load
+	 * max_freq is calculated accoreding to (nominal_freq * highest_perf)/nominal_perf
+	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
+	 * Check _CPC in ACPI table ojbects 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);
+		return -EINVAL;
+	}
+
+	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
+		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 * 1000);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -911,15 +929,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	max_freq = READ_ONCE(cpudata->max_freq);
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
 
-	if (min_freq <= 0 || max_freq <= 0 ||
-	    nominal_freq <= 0 || min_freq > max_freq) {
-		dev_err(dev,
-			"min_freq(%d) or max_freq(%d) or nominal_freq (%d) value is incorrect, check _CPC in ACPI tables\n",
-			min_freq, max_freq, nominal_freq);
-		ret = -EINVAL;
-		goto free_cpudata1;
-	}
-
 	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
 	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
 
@@ -1372,14 +1381,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	min_freq = READ_ONCE(cpudata->min_freq);
 	max_freq = READ_ONCE(cpudata->max_freq);
 	nominal_freq = READ_ONCE(cpudata->nominal_freq);
-	if (min_freq <= 0 || max_freq <= 0 ||
-	    nominal_freq <= 0 || min_freq > max_freq) {
-		dev_err(dev,
-			"min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect, check _CPC in ACPI tables\n",
-			min_freq, max_freq, nominal_freq);
-		ret = -EINVAL;
-		goto free_cpudata1;
-	}
 
 	policy->cpuinfo.min_freq = min_freq;
 	policy->cpuinfo.max_freq = max_freq;
-- 
2.34.1

Re: [PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification

[Mario Limonciello]

The title has a typo s/optimiza/optimize/

On 5/7/2024 02:15, Perry Yuan wrote:
> To enhance the debugging capability of the driver loading failure for
> broken CPPC ACPI tables, we can optimize the expression by moving the

Remove the "we" here.

> verification of `min_freq`, `nominal_freq`, and other dependency values
> to the `amd_pstate_init_freq()` function where they are initialized.
> If any of these values are incorrect, the `amd-pstate` driver will not be registered.
> 
> By ensuring that these values are correct before they are used, it will facilitate
> the debugging process when encountering driver loading failures due to faulty CPPC
> ACPI tables from BIOS
> 
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 35 ++++++++++++++++++-----------------
>   1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 2db095867d03..be7f2d3c86b6 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -873,6 +873,24 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
>   	WRITE_ONCE(cpudata->max_freq, max_freq);
>   
> +	/**
> +	 * Below values need to be initialized correctly, otherwise driver will be failed to load

"will fail to load"

> +	 * max_freq is calculated accoreding to (nominal_freq * highest_perf)/nominal_perf

according to

> +	 * lowest_nonlinear_freq is a value between [min_freq, nominal_freq]
> +	 * Check _CPC in ACPI table ojbects if any values are incorrect

objects

> +	 */
> +	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);
> +		return -EINVAL;
> +	}
> +
> +	if (lowest_nonlinear_freq <= min_freq || lowest_nonlinear_freq > nominal_freq * 1000) {
> +		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 * 1000);
> +		return -EINVAL;
> +	}
> +
>   	return 0;
>   }
>   
> @@ -911,15 +929,6 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>   	max_freq = READ_ONCE(cpudata->max_freq);
>   	nominal_freq = READ_ONCE(cpudata->nominal_freq);
>   
> -	if (min_freq <= 0 || max_freq <= 0 ||
> -	    nominal_freq <= 0 || min_freq > max_freq) {
> -		dev_err(dev,
> -			"min_freq(%d) or max_freq(%d) or nominal_freq (%d) value is incorrect, check _CPC in ACPI tables\n",
> -			min_freq, max_freq, nominal_freq);
> -		ret = -EINVAL;
> -		goto free_cpudata1;
> -	}
> -
>   	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
>   	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
>   
> @@ -1372,14 +1381,6 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>   	min_freq = READ_ONCE(cpudata->min_freq);
>   	max_freq = READ_ONCE(cpudata->max_freq);
>   	nominal_freq = READ_ONCE(cpudata->nominal_freq);
> -	if (min_freq <= 0 || max_freq <= 0 ||
> -	    nominal_freq <= 0 || min_freq > max_freq) {
> -		dev_err(dev,
> -			"min_freq(%d) or max_freq(%d) or nominal_freq(%d) value is incorrect, check _CPC in ACPI tables\n",
> -			min_freq, max_freq, nominal_freq);
> -		ret = -EINVAL;
> -		goto free_cpudata1;
> -	}
>   
>   	policy->cpuinfo.min_freq = min_freq;
>   	policy->cpuinfo.max_freq = max_freq;

Re: [PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification

[kernel test robot]

Hi Perry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge next-20240507]
[cannot apply to tip/x86/core linus/master v6.9-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Perry-Yuan/cpufreq-amd-pstate-optimiza-the-initial-frequency-values-verification/20240507-151930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/0049ad44052b051cf57d1059bf71b7ce227a5f21.1715065568.git.perry.yuan%40amd.com
patch subject: [PATCH 01/11] cpufreq: amd-pstate: optimiza the initial frequency values verification
config: x86_64-randconfig-102-20240507 (https://download.01.org/0day-ci/archive/20240508/202405080431.BPU6Yg9s-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080431.BPU6Yg9s-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405080431.BPU6Yg9s-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_cpu_init':
>> drivers/cpufreq/amd-pstate.c:899:33: warning: variable 'nominal_freq' set but not used [-Wunused-but-set-variable]
     899 |         int min_freq, max_freq, nominal_freq, ret;
         |                                 ^~~~~~~~~~~~
   drivers/cpufreq/amd-pstate.c: In function 'amd_pstate_epp_cpu_init':
   drivers/cpufreq/amd-pstate.c:1350:33: warning: variable 'nominal_freq' set but not used [-Wunused-but-set-variable]
    1350 |         int min_freq, max_freq, nominal_freq, ret;
         |                                 ^~~~~~~~~~~~


vim +/nominal_freq +899 drivers/cpufreq/amd-pstate.c

5547c0ebfc2efd Perry Yuan        2024-04-25  896  
ec437d71db77a1 Huang Rui         2021-12-24  897  static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
ec437d71db77a1 Huang Rui         2021-12-24  898  {
5c3fd1edaa8b4c Perry Yuan        2024-04-30 @899  	int min_freq, max_freq, nominal_freq, ret;
ec437d71db77a1 Huang Rui         2021-12-24  900  	struct device *dev;
ec437d71db77a1 Huang Rui         2021-12-24  901  	struct amd_cpudata *cpudata;
ec437d71db77a1 Huang Rui         2021-12-24  902  
919f4557696939 Wyes Karny        2022-11-17  903  	/*
919f4557696939 Wyes Karny        2022-11-17  904  	 * Resetting PERF_CTL_MSR will put the CPU in P0 frequency,
919f4557696939 Wyes Karny        2022-11-17  905  	 * which is ideal for initialization process.
919f4557696939 Wyes Karny        2022-11-17  906  	 */
919f4557696939 Wyes Karny        2022-11-17  907  	amd_perf_ctl_reset(policy->cpu);
ec437d71db77a1 Huang Rui         2021-12-24  908  	dev = get_cpu_device(policy->cpu);
ec437d71db77a1 Huang Rui         2021-12-24  909  	if (!dev)
ec437d71db77a1 Huang Rui         2021-12-24  910  		return -ENODEV;
ec437d71db77a1 Huang Rui         2021-12-24  911  
ec437d71db77a1 Huang Rui         2021-12-24  912  	cpudata = kzalloc(sizeof(*cpudata), GFP_KERNEL);
ec437d71db77a1 Huang Rui         2021-12-24  913  	if (!cpudata)
ec437d71db77a1 Huang Rui         2021-12-24  914  		return -ENOMEM;
ec437d71db77a1 Huang Rui         2021-12-24  915  
ec437d71db77a1 Huang Rui         2021-12-24  916  	cpudata->cpu = policy->cpu;
ec437d71db77a1 Huang Rui         2021-12-24  917  
f3a052391822b7 Meng Li           2024-01-19  918  	amd_pstate_init_prefcore(cpudata);
f3a052391822b7 Meng Li           2024-01-19  919  
ec437d71db77a1 Huang Rui         2021-12-24  920  	ret = amd_pstate_init_perf(cpudata);
ec437d71db77a1 Huang Rui         2021-12-24  921  	if (ret)
41271016dfa4a0 Huang Rui         2021-12-24  922  		goto free_cpudata1;
ec437d71db77a1 Huang Rui         2021-12-24  923  
5547c0ebfc2efd Perry Yuan        2024-04-25  924  	ret = amd_pstate_init_freq(cpudata);
5547c0ebfc2efd Perry Yuan        2024-04-25  925  	if (ret)
5547c0ebfc2efd Perry Yuan        2024-04-25  926  		goto free_cpudata1;
5547c0ebfc2efd Perry Yuan        2024-04-25  927  
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25  928  	min_freq = READ_ONCE(cpudata->min_freq);
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25  929  	max_freq = READ_ONCE(cpudata->max_freq);
3cbbe8871a2fb8 Gautham R. Shenoy 2024-04-25  930  	nominal_freq = READ_ONCE(cpudata->nominal_freq);
ec437d71db77a1 Huang Rui         2021-12-24  931  
069a2bb8c48c43 Perry Yuan        2024-04-25  932  	policy->cpuinfo.transition_latency = amd_pstate_get_transition_latency(policy->cpu);
069a2bb8c48c43 Perry Yuan        2024-04-25  933  	policy->transition_delay_us = amd_pstate_get_transition_delay_us(policy->cpu);
ec437d71db77a1 Huang Rui         2021-12-24  934  
ec437d71db77a1 Huang Rui         2021-12-24  935  	policy->min = min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  936  	policy->max = max_freq;
ec437d71db77a1 Huang Rui         2021-12-24  937  
ec437d71db77a1 Huang Rui         2021-12-24  938  	policy->cpuinfo.min_freq = min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  939  	policy->cpuinfo.max_freq = max_freq;
ec437d71db77a1 Huang Rui         2021-12-24  940  
ec437d71db77a1 Huang Rui         2021-12-24  941  	/* It will be updated by governor */
ec437d71db77a1 Huang Rui         2021-12-24  942  	policy->cur = policy->cpuinfo.min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  943  
e059c184da47e9 Huang Rui         2021-12-24  944  	if (boot_cpu_has(X86_FEATURE_CPPC))
1d215f0319c206 Huang Rui         2021-12-24  945  		policy->fast_switch_possible = true;
1d215f0319c206 Huang Rui         2021-12-24  946  
41271016dfa4a0 Huang Rui         2021-12-24  947  	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
41271016dfa4a0 Huang Rui         2021-12-24  948  				   FREQ_QOS_MIN, policy->cpuinfo.min_freq);
41271016dfa4a0 Huang Rui         2021-12-24  949  	if (ret < 0) {
41271016dfa4a0 Huang Rui         2021-12-24  950  		dev_err(dev, "Failed to add min-freq constraint (%d)\n", ret);
41271016dfa4a0 Huang Rui         2021-12-24  951  		goto free_cpudata1;
41271016dfa4a0 Huang Rui         2021-12-24  952  	}
41271016dfa4a0 Huang Rui         2021-12-24  953  
41271016dfa4a0 Huang Rui         2021-12-24  954  	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[1],
41271016dfa4a0 Huang Rui         2021-12-24  955  				   FREQ_QOS_MAX, policy->cpuinfo.max_freq);
41271016dfa4a0 Huang Rui         2021-12-24  956  	if (ret < 0) {
41271016dfa4a0 Huang Rui         2021-12-24  957  		dev_err(dev, "Failed to add max-freq constraint (%d)\n", ret);
41271016dfa4a0 Huang Rui         2021-12-24  958  		goto free_cpudata2;
41271016dfa4a0 Huang Rui         2021-12-24  959  	}
41271016dfa4a0 Huang Rui         2021-12-24  960  
febab20caebac9 Wyes Karny        2023-11-17  961  	cpudata->max_limit_freq = max_freq;
febab20caebac9 Wyes Karny        2023-11-17  962  	cpudata->min_limit_freq = min_freq;
ec437d71db77a1 Huang Rui         2021-12-24  963  
ec437d71db77a1 Huang Rui         2021-12-24  964  	policy->driver_data = cpudata;
ec437d71db77a1 Huang Rui         2021-12-24  965  
41271016dfa4a0 Huang Rui         2021-12-24  966  	amd_pstate_boost_init(cpudata);
abd61c08ef349a Perry Yuan        2023-01-31  967  	if (!current_pstate_driver->adjust_perf)
abd61c08ef349a Perry Yuan        2023-01-31  968  		current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
41271016dfa4a0 Huang Rui         2021-12-24  969  
ec437d71db77a1 Huang Rui         2021-12-24  970  	return 0;
ec437d71db77a1 Huang Rui         2021-12-24  971  
41271016dfa4a0 Huang Rui         2021-12-24  972  free_cpudata2:
41271016dfa4a0 Huang Rui         2021-12-24  973  	freq_qos_remove_request(&cpudata->req[0]);
41271016dfa4a0 Huang Rui         2021-12-24  974  free_cpudata1:
ec437d71db77a1 Huang Rui         2021-12-24  975  	kfree(cpudata);
ec437d71db77a1 Huang Rui         2021-12-24  976  	return ret;
ec437d71db77a1 Huang Rui         2021-12-24  977  }
ec437d71db77a1 Huang Rui         2021-12-24  978  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 02/11] cpufreq: amd-pstate: show CPPC debug message if CPPC is not supported

[Perry Yuan]

Add CPU ID checking in case the driver attempt to load on systems where
CPPC functionality is unavailable. And the warning message will not
be showed if CPPC is not supported.

It will also print debug message if the CPU has no CPPC support that
helps to debug the driver loading failure issue.

Closes: https://lore.kernel.org/linux-pm/CYYPR12MB8655D32EA18574C9497E888A9C122@CYYPR12MB8655.namprd12.prod.outlook.com/T/#t
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index be7f2d3c86b6..cb766c061c86 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1687,6 +1687,20 @@ static int __init amd_pstate_set_driver(int mode_idx)
 	return -EINVAL;
 }
 
+/**
+ * CPPC function is not supported for family ID 17H with model_ID ranging from 0x10 to 0x2F.
+ * show the debug message that helps to check if the CPU has CPPC support for loading issue.
+ */
+static bool amd_cppc_supported(void)
+{
+	if ((boot_cpu_data.x86 == 0x17) && (boot_cpu_data.x86_model < 0x30)) {
+		pr_debug_once("CPPC feature is not supported by the processor\n");
+		return false;
+	}
+
+	return true;
+}
+
 static int __init amd_pstate_init(void)
 {
 	struct device *dev_root;
@@ -1695,6 +1709,11 @@ static int __init amd_pstate_init(void)
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
 		return -ENODEV;
 
+	/* show debug message only if CPPC is not supported */
+	if (!amd_cppc_supported())
+		return -EOPNOTSUPP;
+
+	/* show warning message when BIOS broken or ACPI disabled */
 	if (!acpi_cpc_valid()) {
 		pr_warn_once("the _CPC object is not present in SBIOS or ACPI disabled\n");
 		return -ENODEV;
-- 
2.34.1

Re: [PATCH 02/11] cpufreq: amd-pstate: show CPPC debug message if CPPC is not supported

[Mario Limonciello]

On 5/7/2024 02:15, Perry Yuan wrote:
> Add CPU ID checking in case the driver attempt to load on systems where
> CPPC functionality is unavailable. And the warning message will not
> be showed if CPPC is not supported.

shown

> 
> It will also print debug message if the CPU has no CPPC support that
> helps to debug the driver loading failure issue.
> 

You should add a "Reported-by:" here for Paul too.

> Closes: https://lore.kernel.org/linux-pm/CYYPR12MB8655D32EA18574C9497E888A9C122@CYYPR12MB8655.namprd12.prod.outlook.com/T/#t
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index be7f2d3c86b6..cb766c061c86 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1687,6 +1687,20 @@ static int __init amd_pstate_set_driver(int mode_idx)
>   	return -EINVAL;
>   }
>   
> +/**
> + * CPPC function is not supported for family ID 17H with model_ID ranging from 0x10 to 0x2F.
> + * show the debug message that helps to check if the CPU has CPPC support for loading issue.
> + */
> +static bool amd_cppc_supported(void)
> +{
> +	if ((boot_cpu_data.x86 == 0x17) && (boot_cpu_data.x86_model < 0x30)) {
> +		pr_debug_once("CPPC feature is not supported by the processor\n");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
>   static int __init amd_pstate_init(void)
>   {
>   	struct device *dev_root;
> @@ -1695,6 +1709,11 @@ static int __init amd_pstate_init(void)
>   	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>   		return -ENODEV;
>   
> +	/* show debug message only if CPPC is not supported */
> +	if (!amd_cppc_supported())
> +		return -EOPNOTSUPP;
> +
> +	/* show warning message when BIOS broken or ACPI disabled */
>   	if (!acpi_cpc_valid()) {
>   		pr_warn_once("the _CPC object is not present in SBIOS or ACPI disabled\n");
>   		return -ENODEV;

[PATCH 03/11] cpufreq: amd-pstate: add debug message while CPPC is supported and disabled by SBIOS

[Perry Yuan]

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)) {
+		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;
 }
 
-- 
2.34.1

Re: [PATCH 03/11] cpufreq: amd-pstate: add debug message while CPPC is supported and disabled by SBIOS

[Mario Limonciello]

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

[PATCH 04/11] Documentation: PM: amd-pstate: introducing recommended reboot requirement during driver switch

[Perry Yuan]

It is highly recommended to reboot the system when switching between the 'amd-pstate'
and 'acpi-cpufreq' drivers to avoid potential unexpected issues.
Rebooting will ensure that the system properly applies the changes and allows for a
seamless transition between the drivers.

Reported-by : Reported-by: Artem S. Tashkinov <aros@gmx.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215801
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 Documentation/admin-guide/pm/amd-pstate.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index 1e0d101b020a..2695feec1baa 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -299,6 +299,10 @@ module which supports the new AMD P-States mechanism on most of the future AMD
 platforms. The AMD P-States mechanism is the more performance and energy
 efficiency frequency management method on AMD processors.
 
+When users attempt to switch from the acpi-cpufreq driver to the amd-pstate driver's working mode,
+it is recommended to reboot the system. This is to ensure that any low-level power management
+control states are properly switched to pstate control. By rebooting, the firmware can initialize
+with optimal power states, reducing the likelihood of unexpected issues.
 
 ``amd-pstate`` Driver Operation Modes
 ======================================
-- 
2.34.1

[PATCH 05/11] Documentation: PM: amd-pstate: add debugging section for driver loading failure

[Perry Yuan]

To address issues with the loading of the amd-pstate driver on certain platforms,
It needs to enable dynamic debugging to capture debug messages during the driver
loading process. By adding "amd_pstate.dyndbg=+p cppc_acpi.dyndbg=+p  loglevel=4 debug
amd_pstate=active" to the kernel command line, driver debug logging is enabled.

Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 Documentation/admin-guide/pm/amd-pstate.rst | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index 2695feec1baa..230e236796f7 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -476,6 +476,19 @@ operations for the new ``amd-pstate`` module with this tool. ::
 Diagnostics and Tuning
 =======================
 
+Debugging AMD P-State Driver Loading Issues
+------------------------------------------
+
+On some platforms, there may be issues with the loading of the amd-pstate driver.
+To capture debug messages for issue analysis, users can add below parameter,
+"amd_pstate.dyndbg=+p cppc_acpi.dyndbg=+p  loglevel=4 debug amd_pstate=active"
+to the kernel command line. This will enable dynamic debugging and allow better
+analysis and troubleshooting of the driver loading process.
+
+Please note that adding this parameter will only enable debug logging during the
+driver loading phase and may affect system behavior. Use this option with caution
+and only for debugging purposes.
+
 Trace Events
 --------------
 
-- 
2.34.1

Re: [PATCH 05/11] Documentation: PM: amd-pstate: add debugging section for driver loading failure

[Mario Limonciello]

On 5/7/2024 02:15, Perry Yuan wrote:
> To address issues with the loading of the amd-pstate driver on certain platforms,
> It needs to enable dynamic debugging to capture debug messages during the driver
> loading process. By adding "amd_pstate.dyndbg=+p cppc_acpi.dyndbg=+p  loglevel=4 debug
> amd_pstate=active" to the kernel command line, driver debug logging is enabled.

Here's an attempt at rewording this paragraph to consider.

For debugging issues reported by users with amd-pstate dynamic debugging 
output is often needed to aid in root cause.  amd-pstate does use a 
variety of CPPC functions that also provide dynamic debugging output.

Add documentation showing how to use dynamic debug for both.
> 
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   Documentation/admin-guide/pm/amd-pstate.rst | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 2695feec1baa..230e236796f7 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -476,6 +476,19 @@ operations for the new ``amd-pstate`` module with this tool. ::
>   Diagnostics and Tuning
>   =======================
>   
> +Debugging AMD P-State Driver Loading Issues
> +------------------------------------------
> +
> +On some platforms, there may be issues with the loading of the amd-pstate driver.
> +To capture debug messages for issue analysis, users can add below parameter,
> +"amd_pstate.dyndbg=+p cppc_acpi.dyndbg=+p  loglevel=4 debug amd_pstate=active"

Two things:

1. AFAICT you shouldn't need to set the loglevel, this should only 
affect console right?  Most users will share a kernel ring buffer or 
journal.

2. I don't think the debugging suggestion documentation should include 
explicitly setting the mode to active mode.  Active mode is the kernel 
default already.  Users or their distro may have set the mode to a 
different mode by default and this may change things unintentionally for 
them.

> +to the kernel command line. This will enable dynamic debugging and allow better
> +analysis and troubleshooting of the driver loading process.
> +
> +Please note that adding this parameter will only enable debug logging during the
> +driver loading phase and may affect system behavior. Use this option with caution
> +and only for debugging purposes.
> +
>   Trace Events
>   --------------
>   

[PATCH 06/11] Documentation: PM: amd-pstate: add guide mode to the Operation mode

[Perry Yuan]

the guided mode is also supported, so the Operation mode should include
that mode as well.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 Documentation/admin-guide/pm/amd-pstate.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
index 230e236796f7..9fc924930595 100644
--- a/Documentation/admin-guide/pm/amd-pstate.rst
+++ b/Documentation/admin-guide/pm/amd-pstate.rst
@@ -410,7 +410,7 @@ control its functionality at the system level.  They are located in the
 ``/sys/devices/system/cpu/amd_pstate/`` directory and affect all CPUs.
 
 ``status``
-	Operation mode of the driver: "active", "passive" or "disable".
+	Operation mode of the driver: "active", "passive", "guided" or "disable".
 
 	"active"
 		The driver is functional and in the ``active mode``
-- 
2.34.1

Re: [PATCH 06/11] Documentation: PM: amd-pstate: add guide mode to the Operation mode

[Mario Limonciello]

One nit for next version.  The title should be "guided" mode not "guide" 
mode.

On 5/7/2024 02:15, Perry Yuan wrote:
> the guided mode is also supported, so the Operation mode should include
> that mode as well.
> 
> Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   Documentation/admin-guide/pm/amd-pstate.rst | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/admin-guide/pm/amd-pstate.rst b/Documentation/admin-guide/pm/amd-pstate.rst
> index 230e236796f7..9fc924930595 100644
> --- a/Documentation/admin-guide/pm/amd-pstate.rst
> +++ b/Documentation/admin-guide/pm/amd-pstate.rst
> @@ -410,7 +410,7 @@ control its functionality at the system level.  They are located in the
>   ``/sys/devices/system/cpu/amd_pstate/`` directory and affect all CPUs.
>   
>   ``status``
> -	Operation mode of the driver: "active", "passive" or "disable".
> +	Operation mode of the driver: "active", "passive", "guided" or "disable".
>   
>   	"active"
>   		The driver is functional and in the ``active mode``

[PATCH 07/11] cpufreq: amd-pstate: switch boot_cpu_has() to cpu_feature_enabled()

[Perry Yuan]

replace the usage of the deprecated boot_cpu_has() function with
the modern cpu_feature_enabled() function. The switch to cpu_feature_enabled()
ensures compatibility with the latest CPU feature detection mechanisms and
improves code maintainability.

Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index e94b55a7bb59..7145248b38ec 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -124,7 +124,7 @@ static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id *dmi)
 	 * broken BIOS lack of nominal_freq and lowest_freq capabilities
 	 * definition in ACPI tables
 	 */
-	if (boot_cpu_has(X86_FEATURE_ZEN2)) {
+	if (cpu_feature_enabled(X86_FEATURE_ZEN2)) {
 		quirks = dmi->driver_data;
 		pr_info("Overriding nominal and lowest frequencies for %s\n", dmi->ident);
 		return 1;
@@ -166,7 +166,7 @@ static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
 	u64 epp;
 	int ret;
 
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		if (!cppc_req_cached) {
 			epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
 					&cppc_req_cached);
@@ -219,7 +219,7 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
 	int ret;
 	struct cppc_perf_ctrls perf_ctrls;
 
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		u64 value = READ_ONCE(cpudata->cppc_req_cached);
 
 		value &= ~GENMASK_ULL(31, 24);
@@ -705,7 +705,7 @@ static int amd_pstate_get_highest_perf(int cpu, u32 *highest_perf)
 {
 	int ret;
 
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		u64 cap1;
 
 		ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &cap1);
@@ -941,7 +941,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
 	/* It will be updated by governor */
 	policy->cur = policy->cpuinfo.min_freq;
 
-	if (boot_cpu_has(X86_FEATURE_CPPC))
+	if (cpu_feature_enabled(X86_FEATURE_CPPC))
 		policy->fast_switch_possible = true;
 
 	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
@@ -1174,7 +1174,7 @@ static int amd_pstate_change_mode_without_dvr_change(int mode)
 
 	cppc_state = mode;
 
-	if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
+	if (cpu_feature_enabled(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
 		return 0;
 
 	for_each_present_cpu(cpu) {
@@ -1404,7 +1404,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
 	else
 		policy->policy = CPUFREQ_POLICY_POWERSAVE;
 
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);
 		if (ret)
 			return ret;
@@ -1487,7 +1487,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 		epp = 0;
 
 	/* Set initial EPP value */
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		value &= ~GENMASK_ULL(31, 24);
 		value |= (u64)epp << 24;
 	}
@@ -1526,7 +1526,7 @@ static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
 	value = READ_ONCE(cpudata->cppc_req_cached);
 	max_perf = READ_ONCE(cpudata->highest_perf);
 
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
 	} else {
 		perf_ctrls.max_perf = max_perf;
@@ -1560,7 +1560,7 @@ static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
 	value = READ_ONCE(cpudata->cppc_req_cached);
 
 	mutex_lock(&amd_pstate_limits_lock);
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
 
 		/* Set max perf same as min perf */
@@ -1748,7 +1748,7 @@ static int __init amd_pstate_init(void)
 		 */
 		if (amd_pstate_acpi_pm_profile_undefined() ||
 		    amd_pstate_acpi_pm_profile_server() ||
-		    !boot_cpu_has(X86_FEATURE_CPPC)) {
+		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
 			pr_info("driver load is disabled, boot with specific mode to enable this\n");
 			return -ENODEV;
 		}
@@ -1767,7 +1767,7 @@ static int __init amd_pstate_init(void)
 	}
 
 	/* capability check */
-	if (boot_cpu_has(X86_FEATURE_CPPC)) {
+	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
 		pr_debug("AMD CPPC MSR based functionality is supported\n");
 		if (cppc_state != AMD_PSTATE_ACTIVE)
 			current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;
-- 
2.34.1

Re: [PATCH 07/11] cpufreq: amd-pstate: switch boot_cpu_has() to cpu_feature_enabled()

[Mario Limonciello]

On 5/7/2024 02:15, Perry Yuan wrote:
> replace the usage of the deprecated boot_cpu_has() function with

One nit.

Capitalize the "R" in replace.

> the modern cpu_feature_enabled() function. The switch to cpu_feature_enabled()
> ensures compatibility with the latest CPU feature detection mechanisms and
> improves code maintainability.
> 
> Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>

Acked-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/cpufreq/amd-pstate.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index e94b55a7bb59..7145248b38ec 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -124,7 +124,7 @@ static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id *dmi)
>   	 * broken BIOS lack of nominal_freq and lowest_freq capabilities
>   	 * definition in ACPI tables
>   	 */
> -	if (boot_cpu_has(X86_FEATURE_ZEN2)) {
> +	if (cpu_feature_enabled(X86_FEATURE_ZEN2)) {
>   		quirks = dmi->driver_data;
>   		pr_info("Overriding nominal and lowest frequencies for %s\n", dmi->ident);
>   		return 1;
> @@ -166,7 +166,7 @@ static s16 amd_pstate_get_epp(struct amd_cpudata *cpudata, u64 cppc_req_cached)
>   	u64 epp;
>   	int ret;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		if (!cppc_req_cached) {
>   			epp = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ,
>   					&cppc_req_cached);
> @@ -219,7 +219,7 @@ static int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
>   	int ret;
>   	struct cppc_perf_ctrls perf_ctrls;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		u64 value = READ_ONCE(cpudata->cppc_req_cached);
>   
>   		value &= ~GENMASK_ULL(31, 24);
> @@ -705,7 +705,7 @@ static int amd_pstate_get_highest_perf(int cpu, u32 *highest_perf)
>   {
>   	int ret;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		u64 cap1;
>   
>   		ret = rdmsrl_safe_on_cpu(cpu, MSR_AMD_CPPC_CAP1, &cap1);
> @@ -941,7 +941,7 @@ static int amd_pstate_cpu_init(struct cpufreq_policy *policy)
>   	/* It will be updated by governor */
>   	policy->cur = policy->cpuinfo.min_freq;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC))
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC))
>   		policy->fast_switch_possible = true;
>   
>   	ret = freq_qos_add_request(&policy->constraints, &cpudata->req[0],
> @@ -1174,7 +1174,7 @@ static int amd_pstate_change_mode_without_dvr_change(int mode)
>   
>   	cppc_state = mode;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC) || cppc_state == AMD_PSTATE_ACTIVE)
>   		return 0;
>   
>   	for_each_present_cpu(cpu) {
> @@ -1404,7 +1404,7 @@ static int amd_pstate_epp_cpu_init(struct cpufreq_policy *policy)
>   	else
>   		policy->policy = CPUFREQ_POLICY_POWERSAVE;
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		ret = rdmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, &value);
>   		if (ret)
>   			return ret;
> @@ -1487,7 +1487,7 @@ static void amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>   		epp = 0;
>   
>   	/* Set initial EPP value */
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		value &= ~GENMASK_ULL(31, 24);
>   		value |= (u64)epp << 24;
>   	}
> @@ -1526,7 +1526,7 @@ static void amd_pstate_epp_reenable(struct amd_cpudata *cpudata)
>   	value = READ_ONCE(cpudata->cppc_req_cached);
>   	max_perf = READ_ONCE(cpudata->highest_perf);
>   
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		wrmsrl_on_cpu(cpudata->cpu, MSR_AMD_CPPC_REQ, value);
>   	} else {
>   		perf_ctrls.max_perf = max_perf;
> @@ -1560,7 +1560,7 @@ static void amd_pstate_epp_offline(struct cpufreq_policy *policy)
>   	value = READ_ONCE(cpudata->cppc_req_cached);
>   
>   	mutex_lock(&amd_pstate_limits_lock);
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		cpudata->epp_policy = CPUFREQ_POLICY_UNKNOWN;
>   
>   		/* Set max perf same as min perf */
> @@ -1748,7 +1748,7 @@ static int __init amd_pstate_init(void)
>   		 */
>   		if (amd_pstate_acpi_pm_profile_undefined() ||
>   		    amd_pstate_acpi_pm_profile_server() ||
> -		    !boot_cpu_has(X86_FEATURE_CPPC)) {
> +		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   			pr_info("driver load is disabled, boot with specific mode to enable this\n");
>   			return -ENODEV;
>   		}
> @@ -1767,7 +1767,7 @@ static int __init amd_pstate_init(void)
>   	}
>   
>   	/* capability check */
> -	if (boot_cpu_has(X86_FEATURE_CPPC)) {
> +	if (cpu_feature_enabled(X86_FEATURE_CPPC)) {
>   		pr_debug("AMD CPPC MSR based functionality is supported\n");
>   		if (cppc_state != AMD_PSTATE_ACTIVE)
>   			current_pstate_driver->adjust_perf = amd_pstate_adjust_perf;

[PATCH 08/11] x86/cpufeatures: Add feature bits for AMD heterogeneous processor

[Perry Yuan]

CPUID leaf 0x80000026 advertises core types with different efficiency rankings

Bit 30 indicates the heterogeneous core topology feature, if the bit
set, it means not all instances at the current hierarchical level have
the same core topology.

For better utilization of feature words and help to identify core type,
X86_FEATURE_HETERO_CORE_TOPOLOGY is added as a few scattered feature bits.

Link: https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf
PDF p274
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/kernel/cpu/scattered.c    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 3c7434329661..39a92338c015 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -470,6 +470,7 @@
 #define X86_FEATURE_BHI_CTRL		(21*32+ 2) /* "" BHI_DIS_S HW control available */
 #define X86_FEATURE_CLEAR_BHB_HW	(21*32+ 3) /* "" BHI_DIS_S HW control enabled */
 #define X86_FEATURE_CLEAR_BHB_LOOP_ON_VMEXIT (21*32+ 4) /* "" Clear branch history at vmexit using SW loop */
+#define X86_FEATURE_HETERO_CORE_TOPOLOGY       (21*32+ 5) /* "" Heterogeneous Core Topology */
 
 /*
  * BUG word(s)
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index af5aa2c754c2..9e237a3daf7e 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -51,6 +51,7 @@ static const struct cpuid_bit cpuid_bits[] = {
 	{ X86_FEATURE_PERFMON_V2,	CPUID_EAX,  0, 0x80000022, 0 },
 	{ X86_FEATURE_AMD_LBR_V2,	CPUID_EAX,  1, 0x80000022, 0 },
 	{ X86_FEATURE_AMD_LBR_PMC_FREEZE,	CPUID_EAX,  2, 0x80000022, 0 },
+	{ X86_FEATURE_HETERO_CORE_TOPOLOGY,	CPUID_EAX,  30, 0x80000026, 0 },
 	{ 0, 0, 0, 0, 0 }
 };
 
-- 
2.34.1

[PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

[Perry Yuan]

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
and highest frequency initialized by power firmware, `amd_pstate` driver
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
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();
+}
+
+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);
 
 	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 */
-- 
2.34.1

Re: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

[Mario Limonciello]

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 */

RE: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

[Yuan, Perry]

[AMD Official Use Only - General]

 Hi Mario

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Tuesday, May 7, 2024 11:19 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> viresh.kumar@linaro.org; Huang, Ray <Ray.Huang@amd.com>; Shenoy,
> Gautham Ranjal <gautham.shenoy@amd.com>; Petkov, Borislav
> <Borislav.Petkov@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous
> core topology for highest performance initialization
>
> 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/p
> > rogrammer-references/24593.pdf
>
> Use the word "Link:" to prefix this link.
>
> > PDF p274
> > Signed-off-by: Perry Yuan <perry.yuan@amd.com>

Above comments have be addressed in V2.

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

amd_get_this_core_type() cannot be merged, it will be called by some other components, for example qemu.
It is better to user separate function to make codes clear and simple to maintain.

Perry.

>
> > +
> > +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.

Yes, you are right.
pstate ut codes will be modified later

perry.

>
> >
> >     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 */

Re: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

[kernel test robot]

Hi Perry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge next-20240507]
[cannot apply to tip/x86/core linus/master v6.9-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Perry-Yuan/cpufreq-amd-pstate-optimiza-the-initial-frequency-values-verification/20240507-151930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/731a28ea8dda4ca1db64f673c87770de4646290b.1715065568.git.perry.yuan%40amd.com
patch subject: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization
config: x86_64-buildonly-randconfig-001-20240507 (https://download.01.org/0day-ci/archive/20240508/202405080122.4pEEcR2C-lkp@intel.com/config)
compiler: clang version 18.1.4 (https://github.com/llvm/llvm-project e6c3289804a67ea0bb6a86fadbe454dd93b8d855)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080122.4pEEcR2C-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405080122.4pEEcR2C-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/x86/kernel/asm-offsets.c:9:
   In file included from include/linux/crypto.h:15:
   In file included from include/linux/completion.h:12:
   In file included from include/linux/swait.h:7:
   In file included from include/linux/spinlock.h:60:
   In file included from include/linux/thread_info.h:60:
   In file included from arch/x86/include/asm/thread_info.h:59:
   In file included from arch/x86/include/asm/cpufeature.h:5:
>> arch/x86/include/asm/processor.h:690:51: warning: non-void function does not return a value [-Wreturn-type]
     690 | static inline int amd_get_this_core_type(void)          { }
         |                                                           ^
   1 warning generated.
--
   In file included from drivers/usb/dwc2/hcd_ddma.c:12:
   In file included from include/linux/module.h:13:
   In file included from include/linux/stat.h:19:
   In file included from include/linux/time.h:60:
   In file included from include/linux/time32.h:13:
   In file included from include/linux/timex.h:67:
   In file included from arch/x86/include/asm/timex.h:5:
>> arch/x86/include/asm/processor.h:690:51: warning: non-void function does not return a value [-Wreturn-type]
     690 | static inline int amd_get_this_core_type(void)          { }
         |                                                           ^
   drivers/usb/dwc2/hcd_ddma.c:555:16: warning: variable 'n_desc' set but not used [-Wunused-but-set-variable]
     555 |         u16 idx, inc, n_desc = 0, ntd_max = 0;
         |                       ^
   2 warnings generated.
--
   In file included from arch/x86/kernel/asm-offsets.c:9:
   In file included from include/linux/crypto.h:15:
   In file included from include/linux/completion.h:12:
   In file included from include/linux/swait.h:7:
   In file included from include/linux/spinlock.h:60:
   In file included from include/linux/thread_info.h:60:
   In file included from arch/x86/include/asm/thread_info.h:59:
   In file included from arch/x86/include/asm/cpufeature.h:5:
>> arch/x86/include/asm/processor.h:690:51: warning: non-void function does not return a value [-Wreturn-type]
     690 | static inline int amd_get_this_core_type(void)          { }
         |                                                           ^
   1 warning generated.


vim +690 arch/x86/include/asm/processor.h

   680	
   681	#ifdef CONFIG_CPU_SUP_AMD
   682	extern u32 amd_get_highest_perf(void);
   683	extern void amd_clear_divider(void);
   684	extern void amd_check_microcode(void);
   685	extern int amd_get_this_core_type(void);
   686	#else
   687	static inline u32 amd_get_highest_perf(void)		{ return 0; }
   688	static inline void amd_clear_divider(void)		{ }
   689	static inline void amd_check_microcode(void)		{ }
 > 690	static inline int amd_get_this_core_type(void)		{ }
   691	#endif
   692	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization

[kernel test robot]

Hi Perry,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/bleeding-edge next-20240507]
[cannot apply to tip/x86/core linus/master v6.9-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Perry-Yuan/cpufreq-amd-pstate-optimiza-the-initial-frequency-values-verification/20240507-151930
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/731a28ea8dda4ca1db64f673c87770de4646290b.1715065568.git.perry.yuan%40amd.com
patch subject: [PATCH 09/11] cpufreq: amd-pstate: implement heterogeneous core topology for highest performance initialization
config: i386-buildonly-randconfig-005-20240507 (https://download.01.org/0day-ci/archive/20240508/202405080340.n0nVlmfY-lkp@intel.com/config)
compiler: gcc-11 (Ubuntu 11.4.0-4ubuntu1) 11.4.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240508/202405080340.n0nVlmfY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405080340.n0nVlmfY-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/x86/include/asm/cpufeature.h:5,
                    from arch/x86/include/asm/thread_info.h:59,
                    from include/linux/thread_info.h:60,
                    from include/linux/spinlock.h:60,
                    from include/linux/swait.h:7,
                    from include/linux/completion.h:12,
                    from include/linux/crypto.h:15,
                    from arch/x86/kernel/asm-offsets.c:9:
   arch/x86/include/asm/processor.h: In function 'amd_get_this_core_type':
>> arch/x86/include/asm/processor.h:690:1: warning: no return statement in function returning non-void [-Wreturn-type]
     690 | static inline int amd_get_this_core_type(void)          { }
         | ^~~~~~
--
   In file included from arch/x86/include/asm/cpufeature.h:5,
                    from arch/x86/include/asm/thread_info.h:59,
                    from include/linux/thread_info.h:60,
                    from include/linux/spinlock.h:60,
                    from include/linux/swait.h:7,
                    from include/linux/completion.h:12,
                    from include/linux/crypto.h:15,
                    from arch/x86/kernel/asm-offsets.c:9:
   arch/x86/include/asm/processor.h: In function 'amd_get_this_core_type':
>> arch/x86/include/asm/processor.h:690:1: warning: no return statement in function returning non-void [-Wreturn-type]
     690 | static inline int amd_get_this_core_type(void)          { }
         | ^~~~~~


vim +690 arch/x86/include/asm/processor.h

   680	
   681	#ifdef CONFIG_CPU_SUP_AMD
   682	extern u32 amd_get_highest_perf(void);
   683	extern void amd_clear_divider(void);
   684	extern void amd_check_microcode(void);
   685	extern int amd_get_this_core_type(void);
   686	#else
   687	static inline u32 amd_get_highest_perf(void)		{ return 0; }
   688	static inline void amd_clear_divider(void)		{ }
   689	static inline void amd_check_microcode(void)		{ }
 > 690	static inline int amd_get_this_core_type(void)		{ }
   691	#endif
   692	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 10/11] cpufreq: amd-pstate: fix the highest frequency issue which limit performance

[Perry Yuan]

To address the performance drop issue, an optimization has been implemented.
The incorrect highest performance value previously set by the low-level power
firmware for AMD CPUs with Family ID 0x19 and Model ID ranging from 0x70 to 0x7F
series has been identified as the cause.

To resolve this, a check has been implemented to accurately determine the CPU family
and model ID. The correct highest performance value is now set and the performance
drop caused by the incorrect highest performance value are eliminated.

Before the fix, the highest frequency was set to 4200MHz, now it is set
to 4971MHz which is correct.

CPU NODE SOCKET CORE L1d:L1i:L2:L3 ONLINE    MAXMHZ   MINMHZ       MHZ
  0    0      0    0 0:0:0:0          yes 4971.0000 400.0000  400.0000
  1    0      0    0 0:0:0:0          yes 4971.0000 400.0000  400.0000
  2    0      0    1 1:1:1:0          yes 4971.0000 400.0000 4865.8140
  3    0      0    1 1:1:1:0          yes 4971.0000 400.0000  400.0000

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218759
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 7fe8a8fc6227..3ff381c4edf7 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -348,6 +348,7 @@ static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
 {
 	u32 highest_perf;
 	int core_type;
+	struct cpuinfo_x86 *c = &cpu_data(0);
 
 	core_type = amd_pstate_get_cpu_type(cpudata->cpu);
 	pr_debug("core_type %d found\n", core_type);
@@ -355,6 +356,13 @@ static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
 	switch (core_type) {
 	case CPU_CORE_TYPE_NO_HETERO_SUP:
 		highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
+		/*
+		 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7F,
+		 * the highest performance level is set to 196.
+		 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
+		 */
+		if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <= 0x7F))
+			highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;
 		break;
 	case CPU_CORE_TYPE_PERFORMANCE:
 		highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;
-- 
2.34.1

Re: [PATCH 10/11] cpufreq: amd-pstate: fix the highest frequency issue which limit performance

[Mario Limonciello]

On 5/7/2024 02:15, Perry Yuan wrote:
> To address the performance drop issue, an optimization has been implemented.
> The incorrect highest performance value previously set by the low-level power
> firmware for AMD CPUs with Family ID 0x19 and Model ID ranging from 0x70 to 0x7F
> series has been identified as the cause.
> 
> To resolve this, a check has been implemented to accurately determine the CPU family
> and model ID. The correct highest performance value is now set and the performance
> drop caused by the incorrect highest performance value are eliminated.
> 
> Before the fix, the highest frequency was set to 4200MHz, now it is set
> to 4971MHz which is correct.
> 
> CPU NODE SOCKET CORE L1d:L1i:L2:L3 ONLINE    MAXMHZ   MINMHZ       MHZ
>    0    0      0    0 0:0:0:0          yes 4971.0000 400.0000  400.0000
>    1    0      0    0 0:0:0:0          yes 4971.0000 400.0000  400.0000
>    2    0      0    1 1:1:1:0          yes 4971.0000 400.0000 4865.8140
>    3    0      0    1 1:1:1:0          yes 4971.0000 400.0000  400.0000
> 
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218759
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 7fe8a8fc6227..3ff381c4edf7 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -348,6 +348,7 @@ static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
>   {
>   	u32 highest_perf;
>   	int core_type;
> +	struct cpuinfo_x86 *c = &cpu_data(0);
>   
>   	core_type = amd_pstate_get_cpu_type(cpudata->cpu);
>   	pr_debug("core_type %d found\n", core_type);
> @@ -355,6 +356,13 @@ static u32 amd_pstate_highest_perf_set(struct amd_cpudata *cpudata)
>   	switch (core_type) {
>   	case CPU_CORE_TYPE_NO_HETERO_SUP:
>   		highest_perf = CPPC_HIGHEST_PERF_DEFAULT;
> +		/*
> +		 * For AMD CPUs with Family ID 19H and Model ID range 0x70 to 0x7F,
> +		 * the highest performance level is set to 196.
> +		 * https://bugzilla.kernel.org/show_bug.cgi?id=218759
> +		 */
> +		if (c->x86 == 0x19 && (c->x86_model >= 0x70 && c->x86_model <= 0x7F))
> +			highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;

I agree this is the right type of change to make for the reported issue, 
but since it's actually a performance regression from 6.9, can you move 
this to the start of the series and add a Fixes tag and stable tag so we 
can get the regression fixed for 6.10 and 6.9.y?

This will of course mean you need to adjust patch 9 as well for such a 
change, but I think it's better this specific patch goes into 6.10 as a 
fix and the rest of the series can aim for 6.11.

>   		break;
>   	case CPU_CORE_TYPE_PERFORMANCE:
>   		highest_perf = CPPC_HIGHEST_PERF_PERFORMANCE;

[PATCH 11/11] cpufreq: amd-pstate: automatically load pstate driver by default

[Perry Yuan]

If the `amd-pstate` driver is not loaded automatically by default,
it is because the kernel command line parameter has not been added.
To resolve this issue, it is necessary to call the `amd_pstate_set_driver()`
function to enable the desired mode (passive/active/guided) before registering
the driver instance.
This ensures that the driver is loaded correctly without relying on the kernel
command line parameter.

[    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd
[    0.982579] amd_pstate: failed to register with return -22

Reported-by: Andrei Amuraritei <andamu@posteo.net>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
Signed-off-by: Perry Yuan <perry.yuan@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 3ff381c4edf7..f5368497ee79 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -66,7 +66,7 @@
 static struct cpufreq_driver *current_pstate_driver;
 static struct cpufreq_driver amd_pstate_driver;
 static struct cpufreq_driver amd_pstate_epp_driver;
-static int cppc_state = AMD_PSTATE_UNDEFINED;
+static int cppc_state;
 static bool cppc_enabled;
 static bool amd_pstate_prefcore = true;
 static struct quirk_entry *quirks;
@@ -1762,6 +1762,16 @@ static int __init amd_pstate_init(void)
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
 		return -ENODEV;
 
+	/* Disable on the following configs by default:
+	 * 1. Undefined platforms
+	 * 2. Server platforms
+	 */
+	if (amd_pstate_acpi_pm_profile_undefined() ||
+		amd_pstate_acpi_pm_profile_server()) {
+		pr_info("driver load is disabled for server or undefined platform\n");
+		return -ENODEV;
+	}
+
 	/* show debug message only if CPPC is not supported */
 	if (!amd_cppc_supported())
 		return -EOPNOTSUPP;
@@ -1781,28 +1791,21 @@ static int __init amd_pstate_init(void)
 	/* check if this machine need CPPC quirks */
 	dmi_check_system(amd_pstate_quirks_table);
 
+	/* get default driver mode for loading*/
+	cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
+	pr_debug("cppc working state set to mode:%d\n", cppc_state);
+
 	switch (cppc_state) {
-	case AMD_PSTATE_UNDEFINED:
-		/* Disable on the following configs by default:
-		 * 1. Undefined platforms
-		 * 2. Server platforms
-		 * 3. Shared memory designs
-		 */
-		if (amd_pstate_acpi_pm_profile_undefined() ||
-		    amd_pstate_acpi_pm_profile_server() ||
-		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
-			pr_info("driver load is disabled, boot with specific mode to enable this\n");
-			return -ENODEV;
-		}
-		ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
-		if (ret)
-			return ret;
-		break;
 	case AMD_PSTATE_DISABLE:
+		pr_info("driver load is disabled, boot with specific mode to enable this\n");
 		return -ENODEV;
+	case AMD_PSTATE_UNDEFINED:
 	case AMD_PSTATE_PASSIVE:
 	case AMD_PSTATE_ACTIVE:
 	case AMD_PSTATE_GUIDED:
+		ret = amd_pstate_set_driver(cppc_state);
+		if (ret)
+			return ret;
 		break;
 	default:
 		return -EINVAL;
@@ -1823,7 +1826,7 @@ static int __init amd_pstate_init(void)
 	/* enable amd pstate feature */
 	ret = amd_pstate_enable(true);
 	if (ret) {
-		pr_err("failed to enable with return %d\n", ret);
+		pr_err("failed to enable driver mode(%d) with return %d\n", cppc_state, ret);
 		return ret;
 	}
 
-- 
2.34.1

Re: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate driver by default

[Mario Limonciello]

On 5/7/2024 02:15, Perry Yuan wrote:
> If the `amd-pstate` driver is not loaded automatically by default,
> it is because the kernel command line parameter has not been added.
> To resolve this issue, it is necessary to call the `amd_pstate_set_driver()`
> function to enable the desired mode (passive/active/guided) before registering
> the driver instance.
> This ensures that the driver is loaded correctly without relying on the kernel
> command line parameter.
> 
> [    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd
> [    0.982579] amd_pstate: failed to register with return -22
> 
> Reported-by: Andrei Amuraritei <andamu@posteo.net>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
> Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> ---
>   drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++-----------------
>   1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 3ff381c4edf7..f5368497ee79 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -66,7 +66,7 @@
>   static struct cpufreq_driver *current_pstate_driver;
>   static struct cpufreq_driver amd_pstate_driver;
>   static struct cpufreq_driver amd_pstate_epp_driver;
> -static int cppc_state = AMD_PSTATE_UNDEFINED;
> +static int cppc_state;
>   static bool cppc_enabled;
>   static bool amd_pstate_prefcore = true;
>   static struct quirk_entry *quirks;
> @@ -1762,6 +1762,16 @@ static int __init amd_pstate_init(void)
>   	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
>   		return -ENODEV;
>   
> +	/* Disable on the following configs by default:
> +	 * 1. Undefined platforms
> +	 * 2. Server platforms
> +	 */
> +	if (amd_pstate_acpi_pm_profile_undefined() ||
> +		amd_pstate_acpi_pm_profile_server()) {
> +		pr_info("driver load is disabled for server or undefined platform\n");
> +		return -ENODEV;
> +	}
> +
>   	/* show debug message only if CPPC is not supported */
>   	if (!amd_cppc_supported())
>   		return -EOPNOTSUPP;
> @@ -1781,28 +1791,21 @@ static int __init amd_pstate_init(void)
>   	/* check if this machine need CPPC quirks */
>   	dmi_check_system(amd_pstate_quirks_table);
>   
> +	/* get default driver mode for loading*/
> +	cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;

Rather than setting it here within amd_pstate_init() I think you can 
just set it at line 67 to CONFIG_X86_AMD_PSTATE_DEFAULT_MODE.

> +	pr_debug("cppc working state set to mode:%d\n", cppc_state);

I think this debug line is going to be unnecessary when it's already 
hardcoded to kernel config.

> +
>   	switch (cppc_state) {
> -	case AMD_PSTATE_UNDEFINED:
> -		/* Disable on the following configs by default:
> -		 * 1. Undefined platforms
> -		 * 2. Server platforms
> -		 * 3. Shared memory designs
> -		 */
> -		if (amd_pstate_acpi_pm_profile_undefined() ||
> -		    amd_pstate_acpi_pm_profile_server() ||
> -		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
> -			pr_info("driver load is disabled, boot with specific mode to enable this\n");
> -			return -ENODEV;
> -		}
> -		ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
> -		if (ret)
> -			return ret;
> -		break;
>   	case AMD_PSTATE_DISABLE:
> +		pr_info("driver load is disabled, boot with specific mode to enable this\n");
>   		return -ENODEV;
> +	case AMD_PSTATE_UNDEFINED:

With the intent of this patch I no longer see a need for 
AMD_PSTATE_UNDEFINED in the rest of the driver (or in this case 
statement even).

I feel you can drop it from amd-pstate.h.

>   	case AMD_PSTATE_PASSIVE:
>   	case AMD_PSTATE_ACTIVE:
>   	case AMD_PSTATE_GUIDED:
> +		ret = amd_pstate_set_driver(cppc_state);
> +		if (ret)
> +			return ret;
>   		break;
>   	default:
>   		return -EINVAL;
> @@ -1823,7 +1826,7 @@ static int __init amd_pstate_init(void)
>   	/* enable amd pstate feature */
>   	ret = amd_pstate_enable(true);
>   	if (ret) {
> -		pr_err("failed to enable with return %d\n", ret);
> +		pr_err("failed to enable driver mode(%d) with return %d\n", cppc_state, ret);
>   		return ret;
>   	}
>   

Re: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate driver by default

[Oleksandr Natalenko]

[-- Attachment #1: Type: text/plain, Size: 4791 bytes --]

Hello.

On úterý 7. května 2024 16:41:29, SELČ Mario Limonciello wrote:
> On 5/7/2024 02:15, Perry Yuan wrote:
> > If the `amd-pstate` driver is not loaded automatically by default,
> > it is because the kernel command line parameter has not been added.
> > To resolve this issue, it is necessary to call the `amd_pstate_set_driver()`
> > function to enable the desired mode (passive/active/guided) before registering
> > the driver instance.
> > This ensures that the driver is loaded correctly without relying on the kernel
> > command line parameter.
> > 
> > [    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-fix-v1 xhci-hcd
> > [    0.982579] amd_pstate: failed to register with return -22
> > 
> > Reported-by: Andrei Amuraritei <andamu@posteo.net>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
> > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> > ---
> >   drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++-----------------
> >   1 file changed, 21 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> > index 3ff381c4edf7..f5368497ee79 100644
> > --- a/drivers/cpufreq/amd-pstate.c
> > +++ b/drivers/cpufreq/amd-pstate.c
> > @@ -66,7 +66,7 @@
> >   static struct cpufreq_driver *current_pstate_driver;
> >   static struct cpufreq_driver amd_pstate_driver;
> >   static struct cpufreq_driver amd_pstate_epp_driver;
> > -static int cppc_state = AMD_PSTATE_UNDEFINED;
> > +static int cppc_state;
> >   static bool cppc_enabled;
> >   static bool amd_pstate_prefcore = true;
> >   static struct quirk_entry *quirks;
> > @@ -1762,6 +1762,16 @@ static int __init amd_pstate_init(void)
> >   	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> >   		return -ENODEV;
> >   
> > +	/* Disable on the following configs by default:
> > +	 * 1. Undefined platforms
> > +	 * 2. Server platforms
> > +	 */
> > +	if (amd_pstate_acpi_pm_profile_undefined() ||
> > +		amd_pstate_acpi_pm_profile_server()) {
> > +		pr_info("driver load is disabled for server or undefined platform\n");
> > +		return -ENODEV;
> > +	}
> > +
> >   	/* show debug message only if CPPC is not supported */
> >   	if (!amd_cppc_supported())
> >   		return -EOPNOTSUPP;
> > @@ -1781,28 +1791,21 @@ static int __init amd_pstate_init(void)
> >   	/* check if this machine need CPPC quirks */
> >   	dmi_check_system(amd_pstate_quirks_table);
> >   
> > +	/* get default driver mode for loading*/
> > +	cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
> 
> Rather than setting it here within amd_pstate_init() I think you can 
> just set it at line 67 to CONFIG_X86_AMD_PSTATE_DEFAULT_MODE.

To me it seems like setting it here kills a possibility to set the mode via the kernel cmdline. Regardless of what will be set in `amd_pstate=`, `CONFIG_X86_AMD_PSTATE_DEFAULT_MODE` will be used instead.

> 
> > +	pr_debug("cppc working state set to mode:%d\n", cppc_state);
> 
> I think this debug line is going to be unnecessary when it's already 
> hardcoded to kernel config.
> 
> > +
> >   	switch (cppc_state) {
> > -	case AMD_PSTATE_UNDEFINED:
> > -		/* Disable on the following configs by default:
> > -		 * 1. Undefined platforms
> > -		 * 2. Server platforms
> > -		 * 3. Shared memory designs
> > -		 */
> > -		if (amd_pstate_acpi_pm_profile_undefined() ||
> > -		    amd_pstate_acpi_pm_profile_server() ||
> > -		    !cpu_feature_enabled(X86_FEATURE_CPPC)) {
> > -			pr_info("driver load is disabled, boot with specific mode to enable this\n");
> > -			return -ENODEV;
> > -		}
> > -		ret = amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
> > -		if (ret)
> > -			return ret;
> > -		break;
> >   	case AMD_PSTATE_DISABLE:
> > +		pr_info("driver load is disabled, boot with specific mode to enable this\n");
> >   		return -ENODEV;
> > +	case AMD_PSTATE_UNDEFINED:
> 
> With the intent of this patch I no longer see a need for 
> AMD_PSTATE_UNDEFINED in the rest of the driver (or in this case 
> statement even).
> 
> I feel you can drop it from amd-pstate.h.
> 
> >   	case AMD_PSTATE_PASSIVE:
> >   	case AMD_PSTATE_ACTIVE:
> >   	case AMD_PSTATE_GUIDED:
> > +		ret = amd_pstate_set_driver(cppc_state);
> > +		if (ret)
> > +			return ret;
> >   		break;
> >   	default:
> >   		return -EINVAL;
> > @@ -1823,7 +1826,7 @@ static int __init amd_pstate_init(void)
> >   	/* enable amd pstate feature */
> >   	ret = amd_pstate_enable(true);
> >   	if (ret) {
> > -		pr_err("failed to enable with return %d\n", ret);
> > +		pr_err("failed to enable driver mode(%d) with return %d\n", cppc_state, ret);
> >   		return ret;
> >   	}
> >   
> 
> 
> 


-- 
Oleksandr Natalenko (post-factum)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate driver by default

[Yuan, Perry]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Oleksandr Natalenko <oleksandr@natalenko.name>
> Sent: Wednesday, May 8, 2024 11:21 PM
> To: Yuan, Perry <Perry.Yuan@amd.com>; rafael.j.wysocki@intel.com;
> viresh.kumar@linaro.org; Huang, Ray <Ray.Huang@amd.com>; Shenoy,
> Gautham Ranjal <gautham.shenoy@amd.com>; Petkov, Borislav
> <Borislav.Petkov@amd.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Shimmer
> <Shimmer.Huang@amd.com>; Du, Xiaojian <Xiaojian.Du@amd.com>; Meng,
> Li (Jassmine) <Li.Meng@amd.com>; linux-pm@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 11/11] cpufreq: amd-pstate: automatically load pstate
> driver by default
>
> Hello.
>
> On úterý 7. května 2024 16:41:29, SELČ Mario Limonciello wrote:
> > On 5/7/2024 02:15, Perry Yuan wrote:
> > > If the `amd-pstate` driver is not loaded automatically by default,
> > > it is because the kernel command line parameter has not been added.
> > > To resolve this issue, it is necessary to call the
> > > `amd_pstate_set_driver()` function to enable the desired mode
> > > (passive/active/guided) before registering the driver instance.
> > > This ensures that the driver is loaded correctly without relying on
> > > the kernel command line parameter.
> > >
> > > [    0.917789] usb usb6: Manufacturer: Linux 6.9.0-rc6-amd-pstate-new-
> fix-v1 xhci-hcd
> > > [    0.982579] amd_pstate: failed to register with return -22
> > >
> > > Reported-by: Andrei Amuraritei <andamu@posteo.net>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218705
> > > Signed-off-by: Perry Yuan <perry.yuan@amd.com>
> > > ---
> > >   drivers/cpufreq/amd-pstate.c | 39 +++++++++++++++++++---------------
> --
> > >   1 file changed, 21 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/amd-pstate.c
> > > b/drivers/cpufreq/amd-pstate.c index 3ff381c4edf7..f5368497ee79
> > > 100644
> > > --- a/drivers/cpufreq/amd-pstate.c
> > > +++ b/drivers/cpufreq/amd-pstate.c
> > > @@ -66,7 +66,7 @@
> > >   static struct cpufreq_driver *current_pstate_driver;
> > >   static struct cpufreq_driver amd_pstate_driver;
> > >   static struct cpufreq_driver amd_pstate_epp_driver; -static int
> > > cppc_state = AMD_PSTATE_UNDEFINED;
> > > +static int cppc_state;
> > >   static bool cppc_enabled;
> > >   static bool amd_pstate_prefcore = true;
> > >   static struct quirk_entry *quirks; @@ -1762,6 +1762,16 @@ static
> > > int __init amd_pstate_init(void)
> > >           if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD)
> > >                   return -ENODEV;
> > >
> > > + /* Disable on the following configs by default:
> > > +  * 1. Undefined platforms
> > > +  * 2. Server platforms
> > > +  */
> > > + if (amd_pstate_acpi_pm_profile_undefined() ||
> > > +         amd_pstate_acpi_pm_profile_server()) {
> > > +         pr_info("driver load is disabled for server or undefined
> platform\n");
> > > +         return -ENODEV;
> > > + }
> > > +
> > >           /* show debug message only if CPPC is not supported */
> > >           if (!amd_cppc_supported())
> > >                   return -EOPNOTSUPP;
> > > @@ -1781,28 +1791,21 @@ static int __init amd_pstate_init(void)
> > >           /* check if this machine need CPPC quirks */
> > >           dmi_check_system(amd_pstate_quirks_table);
> > >
> > > + /* get default driver mode for loading*/
> > > + cppc_state = CONFIG_X86_AMD_PSTATE_DEFAULT_MODE;
> >
> > Rather than setting it here within amd_pstate_init() I think you can
> > just set it at line 67 to CONFIG_X86_AMD_PSTATE_DEFAULT_MODE.
>
> To me it seems like setting it here kills a possibility to set the mode via the
> kernel cmdline. Regardless of what will be set in `amd_pstate=`,
> `CONFIG_X86_AMD_PSTATE_DEFAULT_MODE` will be used instead.

You are right,  the kernel command line will be missed by this change,  will fix it in next version.
Thanks

Perry.

>
> >
> > > + pr_debug("cppc working state set to mode:%d\n", cppc_state);
> >
> > I think this debug line is going to be unnecessary when it's already
> > hardcoded to kernel config.
> >
> > > +
> > >           switch (cppc_state) {
> > > - case AMD_PSTATE_UNDEFINED:
> > > -         /* Disable on the following configs by default:
> > > -          * 1. Undefined platforms
> > > -          * 2. Server platforms
> > > -          * 3. Shared memory designs
> > > -          */
> > > -         if (amd_pstate_acpi_pm_profile_undefined() ||
> > > -             amd_pstate_acpi_pm_profile_server() ||
> > > -             !cpu_feature_enabled(X86_FEATURE_CPPC)) {
> > > -                 pr_info("driver load is disabled, boot with specific
> mode to enable this\n");
> > > -                 return -ENODEV;
> > > -         }
> > > -         ret =
> amd_pstate_set_driver(CONFIG_X86_AMD_PSTATE_DEFAULT_MODE);
> > > -         if (ret)
> > > -                 return ret;
> > > -         break;
> > >           case AMD_PSTATE_DISABLE:
> > > +         pr_info("driver load is disabled, boot with specific mode to
> > > +enable this\n");
> > >                   return -ENODEV;
> > > + case AMD_PSTATE_UNDEFINED:
> >
> > With the intent of this patch I no longer see a need for
> > AMD_PSTATE_UNDEFINED in the rest of the driver (or in this case
> > statement even).
> >
> > I feel you can drop it from amd-pstate.h.
> >
> > >           case AMD_PSTATE_PASSIVE:
> > >           case AMD_PSTATE_ACTIVE:
> > >           case AMD_PSTATE_GUIDED:
> > > +         ret = amd_pstate_set_driver(cppc_state);
> > > +         if (ret)
> > > +                 return ret;
> > >                   break;
> > >           default:
> > >                   return -EINVAL;
> > > @@ -1823,7 +1826,7 @@ static int __init amd_pstate_init(void)
> > >           /* enable amd pstate feature */
> > >           ret = amd_pstate_enable(true);
> > >           if (ret) {
> > > -         pr_err("failed to enable with return %d\n", ret);
> > > +         pr_err("failed to enable driver mode(%d) with return %d\n",
> > > +cppc_state, ret);
> > >                   return ret;
> > >           }
> > >
> >
> >
> >
>
>
> --
> Oleksandr Natalenko (post-factum)