Re: [PATCH v9 03/13] cpufreq: intel_pstate: use common macro definition for Energy Preference Performance(EPP)

From: Limonciello, Mario
Date: Tue Jan 03 2023 - 19:31:46 EST


On 12/25/2022 10:34, Perry Yuan wrote:
make the energy preference performance strings and profiles using one
common header for intel_pstate driver, then the amd_pstate epp driver can
use the common header as well. This will simpify the intel_pstate and
amd_pstate driver.

Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
---
drivers/cpufreq/Kconfig.x86 | 2 +-
drivers/cpufreq/intel_pstate.c | 13 +++----------
include/linux/cpufreq.h | 10 ++++++++++
3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/Kconfig.x86 b/drivers/cpufreq/Kconfig.x86
index 00476e94db90..f64aef1e093d 100644
--- a/drivers/cpufreq/Kconfig.x86
+++ b/drivers/cpufreq/Kconfig.x86
@@ -36,7 +36,7 @@ config X86_PCC_CPUFREQ
config X86_AMD_PSTATE
bool "AMD Processor P-State driver"
- depends on X86 && ACPI
+ depends on X86 && ACPI && X86_INTEL_PSTATE

This doesn't seem right to me. What if someone didn't compile in Intel x86 support for their kernel? They wouldn't be able to pick X86_AMD_PSTATE.

select ACPI_PROCESSOR
select ACPI_CPPC_LIB if X86_64
select CPU_FREQ_GOV_SCHEDUTIL if SMP
diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index ad9be31753b6..93a60fdac0fc 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -640,15 +640,7 @@ static int intel_pstate_set_epb(int cpu, s16 pref)
* 4 power
*/
-enum energy_perf_value_index {
- EPP_INDEX_DEFAULT = 0,
- EPP_INDEX_PERFORMANCE,
- EPP_INDEX_BALANCE_PERFORMANCE,
- EPP_INDEX_BALANCE_POWERSAVE,
- EPP_INDEX_POWERSAVE,
-};
-
-static const char * const energy_perf_strings[] = {
+const char * const energy_perf_strings[] = {
[EPP_INDEX_DEFAULT] = "default",
[EPP_INDEX_PERFORMANCE] = "performance",
[EPP_INDEX_BALANCE_PERFORMANCE] = "balance_performance",
@@ -656,7 +648,8 @@ static const char * const energy_perf_strings[] = {
[EPP_INDEX_POWERSAVE] = "power",
NULL
};
-static unsigned int epp_values[] = {
+
+unsigned int epp_values[] = {
[EPP_INDEX_DEFAULT] = 0, /* Unused index */
[EPP_INDEX_PERFORMANCE] = HWP_EPP_PERFORMANCE,
[EPP_INDEX_BALANCE_PERFORMANCE] = HWP_EPP_BALANCE_PERFORMANCE,
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d5595d57f4e5..0693269fb775 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -185,6 +185,16 @@ struct cpufreq_freqs {
u8 flags; /* flags of cpufreq_driver, see below. */
};
+enum energy_perf_value_index {
+ EPP_INDEX_DEFAULT = 0,
+ EPP_INDEX_PERFORMANCE,
+ EPP_INDEX_BALANCE_PERFORMANCE,
+ EPP_INDEX_BALANCE_POWERSAVE,
+ EPP_INDEX_POWERSAVE,
+};
+extern const char * const energy_perf_strings[];
+extern unsigned int epp_values[];
+
/* Only for ACPI */
#define CPUFREQ_SHARED_TYPE_NONE (0) /* None */
#define CPUFREQ_SHARED_TYPE_HW (1) /* HW does needed coordination */

I think the right place for these variables and strings is in the cppc library source file that is common across CPPC implementations.