[Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Sumit Gupta]

This patchset supports the Autonomous Performance Level Selection mode
in the cppc_cpufreq driver. The feature is part of the existing CPPC
specification and already present in Intel and AMD specific pstate
cpufreq drivers. The patchset adds the support in generic acpi cppc
cpufreq driver.

It adds a new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver
for supporting the Autonomous Performance Level Selection and Energy
Performance Preference (EPP).
Autonomous selection will get enabled during boot if 'cppc_auto_sel'
boot argument is passed or the 'Autonomous Selection Enable' register
is already set before kernel boot. When enabled, the hardware is
allowed to autonomously select the CPU frequency within the min and
max perf boundaries using the Engergy Performance Preference hints.
The EPP values range from '0x0'(performance preference) to '0xFF'
(energy efficiency preference).

It also exposes the acpi_cppc sysfs nodes to update the epp, auto_sel
and {min|max_perf} registers for changing the hints to hardware for
Autonomous selection.

In a followup patch, plan to add support to dynamically switch the
cpufreq driver instance from 'cppc_cpufreq_epp' to 'cppc_cpufreq' and
vice-versa without reboot.

The patches are divided into below groups:
- Patch [1-2]: Improvements. Can be applied independently.
- Patch [3-4]: sysfs store nodes for Auto mode. Depend on Patch [1-2].
- Patch [5]: Support for 'cppc_cpufreq_epp'. Uses a macro from [3].

Sumit Gupta (5):
  ACPI: CPPC: add read perf ctrls api and rename few existing
  ACPI: CPPC: expand macro to create store acpi_cppc sysfs node
  ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from
    sysfs
  Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt
  cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode

 Documentation/admin-guide/acpi/cppc_sysfs.rst |  28 ++
 .../admin-guide/kernel-parameters.txt         |  11 +
 drivers/acpi/cppc_acpi.c                      | 311 ++++++++++++++++--
 drivers/cpufreq/cppc_cpufreq.c                | 260 ++++++++++++++-
 include/acpi/cppc_acpi.h                      |  19 +-
 5 files changed, 572 insertions(+), 57 deletions(-)

-- 
2.25.1

[Patch 1/5] ACPI: CPPC: add read perf ctrls api and rename few existing

[Sumit Gupta]

Add new API cppc_get_perf_ctrls() to read the performance controls.
Rename the following existing API's for more clarity.
- cppc_set_perf() to cppc_set_perf_ctrls().
- cppc_get_perf_ctrs() to cppc_get_perf_fb_ctrs().
- cppc_get_perf_ctrs_sample() to cppc_get_perf_fb_ctrs_sample().

Also, remove redundant energy_perf field from 'struct cppc_perf_caps'.
It is also present in 'struct cppc_perf_ctrls' which is being used.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/acpi/cppc_acpi.c       | 91 +++++++++++++++++++++++++++++-----
 drivers/cpufreq/cppc_cpufreq.c | 26 +++++-----
 include/acpi/cppc_acpi.h       | 14 ++++--
 3 files changed, 101 insertions(+), 30 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index f193e713825a..297e689f8214 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -59,7 +59,7 @@ struct cppc_pcc_data {
 	/*
 	 * Lock to provide controlled access to the PCC channel.
 	 *
-	 * For performance critical usecases(currently cppc_set_perf)
+	 * For performance critical usecases(currently cppc_set_perf_ctrls)
 	 *	We need to take read_lock and check if channel belongs to OSPM
 	 * before reading or writing to PCC subspace
 	 *	We need to take write_lock before transferring the channel
@@ -169,8 +169,8 @@ show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, guaranteed_perf);
 show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_freq);
 show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
 
-show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, reference_perf);
-show_cppc_data(cppc_get_perf_ctrs, cppc_perf_fb_ctrs, wraparound_time);
+show_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, reference_perf);
+show_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, wraparound_time);
 
 /* Check for valid access_width, otherwise, fallback to using bit_width */
 #define GET_BIT_WIDTH(reg) ((reg)->access_width ? (8 << ((reg)->access_width - 1)) : (reg)->bit_width)
@@ -189,7 +189,7 @@ static ssize_t show_feedback_ctrs(struct kobject *kobj,
 	struct cppc_perf_fb_ctrs fb_ctrs = {0};
 	int ret;
 
-	ret = cppc_get_perf_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
+	ret = cppc_get_perf_fb_ctrs(cpc_ptr->cpu_id, &fb_ctrs);
 	if (ret)
 		return ret;
 
@@ -1360,7 +1360,7 @@ EXPORT_SYMBOL_GPL(cppc_get_perf_caps);
  *
  * CPPC has flexibility about how CPU performance counters are accessed.
  * One of the choices is PCC regions, which can have a high access latency. This
- * routine allows callers of cppc_get_perf_ctrs() to know this ahead of time.
+ * routine allows callers of cppc_get_perf_fb_ctrs() to know this ahead of time.
  *
  * Return: true if any of the counters are in PCC regions, false otherwise
  */
@@ -1398,13 +1398,13 @@ bool cppc_perf_ctrs_in_pcc(void)
 EXPORT_SYMBOL_GPL(cppc_perf_ctrs_in_pcc);
 
 /**
- * cppc_get_perf_ctrs - Read a CPU's performance feedback counters.
+ * cppc_get_perf_fb_ctrs - Read a CPU's performance feedback counters.
  * @cpunum: CPU from which to read counters.
  * @perf_fb_ctrs: ptr to cppc_perf_fb_ctrs. See cppc_acpi.h
  *
  * Return: 0 for success with perf_fb_ctrs populated else -ERRNO.
  */
-int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
+int cppc_get_perf_fb_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 {
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
 	struct cpc_register_resource *delivered_reg, *reference_reg,
@@ -1475,7 +1475,7 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 		up_write(&pcc_ss_data->pcc_lock);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
+EXPORT_SYMBOL_GPL(cppc_get_perf_fb_ctrs);
 
 /*
  * Set Energy Performance Preference Register value through
@@ -1674,15 +1674,82 @@ int cppc_set_enable(int cpu, bool enable)
 	return cpc_write(cpu, enable_reg, enable);
 }
 EXPORT_SYMBOL_GPL(cppc_set_enable);
+/**
+ * cppc_get_perf - Get a CPU's performance controls.
+ * @cpu: CPU for which to get performance controls.
+ * @perf_ctrls: ptr to cppc_perf_ctrls. See cppc_acpi.h
+ *
+ * Return: 0 for success with perf_ctrls, -ERRNO otherwise.
+ */
+int cppc_get_perf_ctrls(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+{
+	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
+	struct cpc_register_resource *desired_perf_reg, *min_perf_reg, *max_perf_reg,
+				     *energy_perf_reg;
+	u64 max, min, desired_perf, energy_perf;
+	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
+	struct cppc_pcc_data *pcc_ss_data = NULL;
+	int ret = 0, regs_in_pcc = 0;
+
+	if (!cpc_desc) {
+		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
+		return -ENODEV;
+	}
+
+	desired_perf_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
+	min_perf_reg = &cpc_desc->cpc_regs[MIN_PERF];
+	max_perf_reg = &cpc_desc->cpc_regs[MAX_PERF];
+	energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
+
+	/* Are any of the regs PCC ?*/
+	if (CPC_IN_PCC(desired_perf_reg) || CPC_IN_PCC(min_perf_reg) ||
+	    CPC_IN_PCC(max_perf_reg) || CPC_IN_PCC(energy_perf_reg)) {
+		if (pcc_ss_id < 0) {
+			pr_debug("Invalid pcc_ss_id\n");
+			return -ENODEV;
+		}
+		pcc_ss_data = pcc_data[pcc_ss_id];
+		regs_in_pcc = 1;
+		down_write(&pcc_ss_data->pcc_lock);
+		/* Ring doorbell once to update PCC subspace */
+		if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
+			ret = -EIO;
+			goto out_err;
+		}
+	}
+
+	/* Read optional elements if present */
+	if (CPC_SUPPORTED(max_perf_reg))
+		cpc_read(cpu, max_perf_reg, &max);
+	perf_ctrls->max_perf = max;
+
+	if (CPC_SUPPORTED(min_perf_reg))
+		cpc_read(cpu, min_perf_reg, &min);
+	perf_ctrls->min_perf = min;
+
+	if (CPC_SUPPORTED(desired_perf_reg))
+		cpc_read(cpu, desired_perf_reg, &desired_perf);
+	perf_ctrls->desired_perf = desired_perf;
+
+	if (CPC_SUPPORTED(energy_perf_reg))
+		cpc_read(cpu, energy_perf_reg, &energy_perf);
+	perf_ctrls->energy_perf = energy_perf;
+
+out_err:
+	if (regs_in_pcc)
+		up_write(&pcc_ss_data->pcc_lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(cppc_get_perf_ctrls);
 
 /**
- * cppc_set_perf - Set a CPU's performance controls.
+ * cppc_set_perf_ctrls - Set a CPU's performance controls.
  * @cpu: CPU for which to set performance controls.
  * @perf_ctrls: ptr to cppc_perf_ctrls. See cppc_acpi.h
  *
  * Return: 0 for success, -ERRNO otherwise.
  */
-int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+int cppc_set_perf_ctrls(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 {
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
 	struct cpc_register_resource *desired_reg, *min_perf_reg, *max_perf_reg;
@@ -1746,7 +1813,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	/*
 	 * This is Phase-II where we transfer the ownership of PCC to Platform
 	 *
-	 * Short Summary: Basically if we think of a group of cppc_set_perf
+	 * Short Summary: Basically if we think of a group of cppc_set_perf_ctrls
 	 * requests that happened in short overlapping interval. The last CPU to
 	 * come out of Phase-I will enter Phase-II and ring the doorbell.
 	 *
@@ -1805,7 +1872,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	}
 	return ret;
 }
-EXPORT_SYMBOL_GPL(cppc_set_perf);
+EXPORT_SYMBOL_GPL(cppc_set_perf_ctrls);
 
 /**
  * cppc_get_transition_latency - returns frequency transition latency in ns
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index b3d74f9adcf0..17c49653a3c4 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -90,7 +90,7 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
 	cppc_fi = container_of(work, struct cppc_freq_invariance, work);
 	cpu_data = cppc_fi->cpu_data;
 
-	if (cppc_get_perf_ctrs(cppc_fi->cpu, &fb_ctrs)) {
+	if (cppc_get_perf_fb_ctrs(cppc_fi->cpu, &fb_ctrs)) {
 		pr_warn("%s: failed to read perf counters\n", __func__);
 		return;
 	}
@@ -125,7 +125,7 @@ static void cppc_scale_freq_tick(void)
 	struct cppc_freq_invariance *cppc_fi = &per_cpu(cppc_freq_inv, smp_processor_id());
 
 	/*
-	 * cppc_get_perf_ctrs() can potentially sleep, call that from the right
+	 * cppc_get_perf_fb_ctrs() can potentially sleep, call that from the right
 	 * context.
 	 */
 	irq_work_queue(&cppc_fi->irq_work);
@@ -151,7 +151,7 @@ static void cppc_cpufreq_cpu_fie_init(struct cpufreq_policy *policy)
 		kthread_init_work(&cppc_fi->work, cppc_scale_freq_workfn);
 		init_irq_work(&cppc_fi->irq_work, cppc_irq_work);
 
-		ret = cppc_get_perf_ctrs(cpu, &cppc_fi->prev_perf_fb_ctrs);
+		ret = cppc_get_perf_fb_ctrs(cpu, &cppc_fi->prev_perf_fb_ctrs);
 		if (ret) {
 			pr_warn("%s: failed to read perf counters for cpu:%d: %d\n",
 				__func__, cpu, ret);
@@ -281,7 +281,7 @@ static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 	freqs.new = target_freq;
 
 	cpufreq_freq_transition_begin(policy, &freqs);
-	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
+	ret = cppc_set_perf_ctrls(cpu, &cpu_data->perf_ctrls);
 	cpufreq_freq_transition_end(policy, &freqs, ret != 0);
 
 	if (ret)
@@ -301,7 +301,7 @@ static unsigned int cppc_cpufreq_fast_switch(struct cpufreq_policy *policy,
 
 	desired_perf = cppc_khz_to_perf(&cpu_data->perf_caps, target_freq);
 	cpu_data->perf_ctrls.desired_perf = desired_perf;
-	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
+	ret = cppc_set_perf_ctrls(cpu, &cpu_data->perf_ctrls);
 
 	if (ret) {
 		pr_debug("Failed to set target on CPU:%d. ret:%d\n",
@@ -657,7 +657,7 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
 	cpu_data->perf_ctrls.desired_perf =  caps->highest_perf;
 
-	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
+	ret = cppc_set_perf_ctrls(cpu, &cpu_data->perf_ctrls);
 	if (ret) {
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 			 caps->highest_perf, cpu, ret);
@@ -683,7 +683,7 @@ static void cppc_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 
 	cpu_data->perf_ctrls.desired_perf = caps->lowest_perf;
 
-	ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
+	ret = cppc_set_perf_ctrls(cpu, &cpu_data->perf_ctrls);
 	if (ret)
 		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
 			 caps->lowest_perf, cpu, ret);
@@ -723,19 +723,19 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
 	return (reference_perf * delta_delivered) / delta_reference;
 }
 
-static int cppc_get_perf_ctrs_sample(int cpu,
-				     struct cppc_perf_fb_ctrs *fb_ctrs_t0,
-				     struct cppc_perf_fb_ctrs *fb_ctrs_t1)
+static int cppc_get_perf_fb_ctrs_sample(int cpu,
+					struct cppc_perf_fb_ctrs *fb_ctrs_t0,
+					struct cppc_perf_fb_ctrs *fb_ctrs_t1)
 {
 	int ret;
 
-	ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
+	ret = cppc_get_perf_fb_ctrs(cpu, fb_ctrs_t0);
 	if (ret)
 		return ret;
 
 	udelay(2); /* 2usec delay between sampling */
 
-	return cppc_get_perf_ctrs(cpu, fb_ctrs_t1);
+	return cppc_get_perf_fb_ctrs(cpu, fb_ctrs_t1);
 }
 
 static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
@@ -753,7 +753,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
 
 	cpufreq_cpu_put(policy);
 
-	ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
+	ret = cppc_get_perf_fb_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
 	if (ret) {
 		if (ret == -EFAULT)
 			/* Any of the associated CPPC regs is 0. */
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 62d368bcd9ec..31f4fd288b65 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -110,7 +110,6 @@ struct cppc_perf_caps {
 	u32 lowest_nonlinear_perf;
 	u32 lowest_freq;
 	u32 nominal_freq;
-	u32 energy_perf;
 	bool auto_sel;
 };
 
@@ -142,8 +141,9 @@ struct cppc_cpudata {
 extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
 extern int cppc_get_nominal_perf(int cpunum, u64 *nominal_perf);
 extern int cppc_get_highest_perf(int cpunum, u64 *highest_perf);
-extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
-extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
+extern int cppc_get_perf_fb_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
+extern int cppc_get_perf_ctrls(int cpu, struct cppc_perf_ctrls *perf_ctrls);
+extern int cppc_set_perf_ctrls(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_set_enable(int cpu, bool enable);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
 extern bool cppc_perf_ctrs_in_pcc(void);
@@ -177,11 +177,15 @@ static inline int cppc_get_highest_perf(int cpunum, u64 *highest_perf)
 {
 	return -EOPNOTSUPP;
 }
-static inline int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
+static inline int cppc_get_perf_fb_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
+{
+	return -EOPNOTSUPP;
+}
+static inline int cppc_get_perf_ctrls(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 {
 	return -EOPNOTSUPP;
 }
-static inline int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
+static inline int cppc_set_perf_ctrls(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 {
 	return -EOPNOTSUPP;
 }
-- 
2.25.1

Re: [Patch 1/5] ACPI: CPPC: add read perf ctrls api and rename few existing

[kernel test robot]

Hi Sumit,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20250210]
[also build test ERROR on linus/master v6.14-rc2]
[cannot apply to rafael-pm/linux-next rafael-pm/bleeding-edge v6.14-rc2 v6.14-rc1 v6.13]
[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/Sumit-Gupta/ACPI-CPPC-add-read-perf-ctrls-api-and-rename-few-existing/20250211-184154
base:   next-20250210
patch link:    https://lore.kernel.org/r/20250211103737.447704-2-sumitg%40nvidia.com
patch subject: [Patch 1/5] ACPI: CPPC: add read perf ctrls api and rename few existing
config: x86_64-buildonly-randconfig-005-20250212 (https://download.01.org/0day-ci/archive/20250212/202502121512.r83JqnGm-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250212/202502121512.r83JqnGm-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/202502121512.r83JqnGm-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/cpufreq/amd-pstate.c: In function 'shmem_cppc_enable':
>> drivers/cpufreq/amd-pstate.c:395:31: error: implicit declaration of function 'cppc_set_perf'; did you mean 'cppc_set_epp_perf'? [-Werror=implicit-function-declaration]
     395 |                         ret = cppc_set_perf(cpu, &perf_ctrls);
         |                               ^~~~~~~~~~~~~
         |                               cppc_set_epp_perf
   cc1: some warnings being treated as errors


vim +395 drivers/cpufreq/amd-pstate.c

ec437d71db77a1 Huang Rui         2021-12-24  377  
7fb463aac84577 Dhananjay Ugwekar 2024-10-23  378  static int shmem_cppc_enable(bool enable)
e059c184da47e9 Huang Rui         2021-12-24  379  {
e059c184da47e9 Huang Rui         2021-12-24  380  	int cpu, ret = 0;
ffa5096a7c3386 Perry Yuan        2023-01-31  381  	struct cppc_perf_ctrls perf_ctrls;
e059c184da47e9 Huang Rui         2021-12-24  382  
217e67784eab30 Wyes Karny        2023-05-30  383  	if (enable == cppc_enabled)
217e67784eab30 Wyes Karny        2023-05-30  384  		return 0;
217e67784eab30 Wyes Karny        2023-05-30  385  
e059c184da47e9 Huang Rui         2021-12-24  386  	for_each_present_cpu(cpu) {
e059c184da47e9 Huang Rui         2021-12-24  387  		ret = cppc_set_enable(cpu, enable);
e059c184da47e9 Huang Rui         2021-12-24  388  		if (ret)
e059c184da47e9 Huang Rui         2021-12-24  389  			return ret;
ffa5096a7c3386 Perry Yuan        2023-01-31  390  
ffa5096a7c3386 Perry Yuan        2023-01-31  391  		/* Enable autonomous mode for EPP */
ffa5096a7c3386 Perry Yuan        2023-01-31  392  		if (cppc_state == AMD_PSTATE_ACTIVE) {
ffa5096a7c3386 Perry Yuan        2023-01-31  393  			/* Set desired perf as zero to allow EPP firmware control */
ffa5096a7c3386 Perry Yuan        2023-01-31  394  			perf_ctrls.desired_perf = 0;
ffa5096a7c3386 Perry Yuan        2023-01-31 @395  			ret = cppc_set_perf(cpu, &perf_ctrls);
ffa5096a7c3386 Perry Yuan        2023-01-31  396  			if (ret)
ffa5096a7c3386 Perry Yuan        2023-01-31  397  				return ret;
ffa5096a7c3386 Perry Yuan        2023-01-31  398  		}
e059c184da47e9 Huang Rui         2021-12-24  399  	}
e059c184da47e9 Huang Rui         2021-12-24  400  
217e67784eab30 Wyes Karny        2023-05-30  401  	cppc_enabled = enable;
e059c184da47e9 Huang Rui         2021-12-24  402  	return ret;
e059c184da47e9 Huang Rui         2021-12-24  403  }
e059c184da47e9 Huang Rui         2021-12-24  404  

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

Re: [Patch 1/5] ACPI: CPPC: add read perf ctrls api and rename few existing

[kernel test robot]

Hi Sumit,

kernel test robot noticed the following build errors:

[auto build test ERROR on next-20250210]
[also build test ERROR on linus/master v6.14-rc2]
[cannot apply to rafael-pm/linux-next rafael-pm/bleeding-edge v6.14-rc2 v6.14-rc1 v6.13]
[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/Sumit-Gupta/ACPI-CPPC-add-read-perf-ctrls-api-and-rename-few-existing/20250211-184154
base:   next-20250210
patch link:    https://lore.kernel.org/r/20250211103737.447704-2-sumitg%40nvidia.com
patch subject: [Patch 1/5] ACPI: CPPC: add read perf ctrls api and rename few existing
config: x86_64-buildonly-randconfig-002-20250212 (https://download.01.org/0day-ci/archive/20250212/202502121658.IcwbwB0y-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250212/202502121658.IcwbwB0y-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/202502121658.IcwbwB0y-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/cpufreq/amd-pstate.c:395:10: error: call to undeclared function 'cppc_set_perf'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     395 |                         ret = cppc_set_perf(cpu, &perf_ctrls);
         |                               ^
   drivers/cpufreq/amd-pstate.c:395:10: note: did you mean 'cppc_set_epp_perf'?
   include/acpi/cppc_acpi.h:161:12: note: 'cppc_set_epp_perf' declared here
     161 | extern int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable);
         |            ^
   drivers/cpufreq/amd-pstate.c:498:9: error: call to undeclared function 'cppc_set_perf'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
     498 |         return cppc_set_perf(cpudata->cpu, &perf_ctrls);
         |                ^
   2 errors generated.
--
>> drivers/acpi/cppc_acpi.c:1734:6: warning: variable 'energy_perf' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1734 |         if (CPC_SUPPORTED(energy_perf_reg))
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:129:28: note: expanded from macro 'CPC_SUPPORTED'
     129 | #define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?          \
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     130 |                                 !!(cpc)->cpc_entry.int_value :          \
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     131 |                                 !IS_NULL_REG(&(cpc)->cpc_entry.reg))
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:1736:28: note: uninitialized use occurs here
    1736 |         perf_ctrls->energy_perf = energy_perf;
         |                                   ^~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:1734:2: note: remove the 'if' if its condition is always true
    1734 |         if (CPC_SUPPORTED(energy_perf_reg))
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1735 |                 cpc_read(cpu, energy_perf_reg, &energy_perf);
   drivers/acpi/cppc_acpi.c:1689:41: note: initialize the variable 'energy_perf' to silence this warning
    1689 |         u64 max, min, desired_perf, energy_perf;
         |                                                ^
         |                                                 = 0
>> drivers/acpi/cppc_acpi.c:1730:6: warning: variable 'desired_perf' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1730 |         if (CPC_SUPPORTED(desired_perf_reg))
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:129:28: note: expanded from macro 'CPC_SUPPORTED'
     129 | #define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?          \
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     130 |                                 !!(cpc)->cpc_entry.int_value :          \
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     131 |                                 !IS_NULL_REG(&(cpc)->cpc_entry.reg))
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:1732:29: note: uninitialized use occurs here
    1732 |         perf_ctrls->desired_perf = desired_perf;
         |                                    ^~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:1730:2: note: remove the 'if' if its condition is always true
    1730 |         if (CPC_SUPPORTED(desired_perf_reg))
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1731 |                 cpc_read(cpu, desired_perf_reg, &desired_perf);
   drivers/acpi/cppc_acpi.c:1689:28: note: initialize the variable 'desired_perf' to silence this warning
    1689 |         u64 max, min, desired_perf, energy_perf;
         |                                   ^
         |                                    = 0
>> drivers/acpi/cppc_acpi.c:1726:6: warning: variable 'min' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1726 |         if (CPC_SUPPORTED(min_perf_reg))
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:129:28: note: expanded from macro 'CPC_SUPPORTED'
     129 | #define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?          \
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     130 |                                 !!(cpc)->cpc_entry.int_value :          \
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     131 |                                 !IS_NULL_REG(&(cpc)->cpc_entry.reg))
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:1728:25: note: uninitialized use occurs here
    1728 |         perf_ctrls->min_perf = min;
         |                                ^~~
   drivers/acpi/cppc_acpi.c:1726:2: note: remove the 'if' if its condition is always true
    1726 |         if (CPC_SUPPORTED(min_perf_reg))
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1727 |                 cpc_read(cpu, min_perf_reg, &min);
   drivers/acpi/cppc_acpi.c:1689:14: note: initialize the variable 'min' to silence this warning
    1689 |         u64 max, min, desired_perf, energy_perf;
         |                     ^
         |                      = 0
>> drivers/acpi/cppc_acpi.c:1722:6: warning: variable 'max' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1722 |         if (CPC_SUPPORTED(max_perf_reg))
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:129:28: note: expanded from macro 'CPC_SUPPORTED'
     129 | #define CPC_SUPPORTED(cpc) ((cpc)->type == ACPI_TYPE_INTEGER ?          \
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     130 |                                 !!(cpc)->cpc_entry.int_value :          \
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     131 |                                 !IS_NULL_REG(&(cpc)->cpc_entry.reg))
         |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/acpi/cppc_acpi.c:1724:25: note: uninitialized use occurs here
    1724 |         perf_ctrls->max_perf = max;
         |                                ^~~
   drivers/acpi/cppc_acpi.c:1722:2: note: remove the 'if' if its condition is always true
    1722 |         if (CPC_SUPPORTED(max_perf_reg))
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1723 |                 cpc_read(cpu, max_perf_reg, &max);
   drivers/acpi/cppc_acpi.c:1689:9: note: initialize the variable 'max' to silence this warning
    1689 |         u64 max, min, desired_perf, energy_perf;
         |                ^
         |                 = 0
   4 warnings generated.
--
>> drivers/acpi/cppc_acpi.c:1685: warning: expecting prototype for cppc_get_perf(). Prototype was for cppc_get_perf_ctrls() instead


vim +/cppc_set_perf +395 drivers/cpufreq/amd-pstate.c

ec437d71db77a1 Huang Rui         2021-12-24  377  
7fb463aac84577 Dhananjay Ugwekar 2024-10-23  378  static int shmem_cppc_enable(bool enable)
e059c184da47e9 Huang Rui         2021-12-24  379  {
e059c184da47e9 Huang Rui         2021-12-24  380  	int cpu, ret = 0;
ffa5096a7c3386 Perry Yuan        2023-01-31  381  	struct cppc_perf_ctrls perf_ctrls;
e059c184da47e9 Huang Rui         2021-12-24  382  
217e67784eab30 Wyes Karny        2023-05-30  383  	if (enable == cppc_enabled)
217e67784eab30 Wyes Karny        2023-05-30  384  		return 0;
217e67784eab30 Wyes Karny        2023-05-30  385  
e059c184da47e9 Huang Rui         2021-12-24  386  	for_each_present_cpu(cpu) {
e059c184da47e9 Huang Rui         2021-12-24  387  		ret = cppc_set_enable(cpu, enable);
e059c184da47e9 Huang Rui         2021-12-24  388  		if (ret)
e059c184da47e9 Huang Rui         2021-12-24  389  			return ret;
ffa5096a7c3386 Perry Yuan        2023-01-31  390  
ffa5096a7c3386 Perry Yuan        2023-01-31  391  		/* Enable autonomous mode for EPP */
ffa5096a7c3386 Perry Yuan        2023-01-31  392  		if (cppc_state == AMD_PSTATE_ACTIVE) {
ffa5096a7c3386 Perry Yuan        2023-01-31  393  			/* Set desired perf as zero to allow EPP firmware control */
ffa5096a7c3386 Perry Yuan        2023-01-31  394  			perf_ctrls.desired_perf = 0;
ffa5096a7c3386 Perry Yuan        2023-01-31 @395  			ret = cppc_set_perf(cpu, &perf_ctrls);
ffa5096a7c3386 Perry Yuan        2023-01-31  396  			if (ret)
ffa5096a7c3386 Perry Yuan        2023-01-31  397  				return ret;
ffa5096a7c3386 Perry Yuan        2023-01-31  398  		}
e059c184da47e9 Huang Rui         2021-12-24  399  	}
e059c184da47e9 Huang Rui         2021-12-24  400  
217e67784eab30 Wyes Karny        2023-05-30  401  	cppc_enabled = enable;
e059c184da47e9 Huang Rui         2021-12-24  402  	return ret;
e059c184da47e9 Huang Rui         2021-12-24  403  }
e059c184da47e9 Huang Rui         2021-12-24  404  

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

[Patch 2/5] ACPI: CPPC: expand macro to create store acpi_cppc sysfs node

[Sumit Gupta]

Rename show_cppc_data() macro to sysfs_cppc_data() and expand
it to enable creating the acpi_cppc sysfs store node.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/acpi/cppc_acpi.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 297e689f8214..cc2bf958e84f 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -139,12 +139,21 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
 #define OVER_16BTS_MASK ~0xFFFFULL
 
 #define define_one_cppc_ro(_name)		\
-static struct kobj_attribute _name =		\
+	static struct kobj_attribute _name =	\
 __ATTR(_name, 0444, show_##_name, NULL)
 
+#define define_one_cppc_rw(_name)		\
+	static struct kobj_attribute _name =	\
+__ATTR(_name, 0644, show_##_name, store_##_name)
+
 #define to_cpc_desc(a) container_of(a, struct cpc_desc, kobj)
 
-#define show_cppc_data(access_fn, struct_name, member_name)		\
+#define define_one_cppc(member_name, mode)       define_one_cppc_attr(mode, member_name)
+#define define_one_cppc_attr(mode, member_name)  define_one_cppc_attr_##mode(member_name)
+#define define_one_cppc_attr_ro(member_name)     define_one_cppc_ro(member_name)
+#define define_one_cppc_attr_rw(member_name)     define_one_cppc_rw(member_name)
+
+#define sysfs_cppc_data(access_fn, struct_name, member_name, mode)	\
 	static ssize_t show_##member_name(struct kobject *kobj,		\
 				struct kobj_attribute *attr, char *buf)	\
 	{								\
@@ -159,18 +168,18 @@ __ATTR(_name, 0444, show_##_name, NULL)
 		return sysfs_emit(buf, "%llu\n",		\
 				(u64)st_name.member_name);		\
 	}								\
-	define_one_cppc_ro(member_name)
-
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, highest_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_nonlinear_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, guaranteed_perf);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_freq);
-show_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq);
-
-show_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, reference_perf);
-show_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, wraparound_time);
+	define_one_cppc(member_name, mode)
+
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, highest_perf, ro);
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_perf, ro);
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_perf, ro);
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_nonlinear_perf, ro);
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, guaranteed_perf, ro);
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_freq, ro);
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq, ro);
+
+sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, reference_perf, ro);
+sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, wraparound_time, ro);
 
 /* Check for valid access_width, otherwise, fallback to using bit_width */
 #define GET_BIT_WIDTH(reg) ((reg)->access_width ? (8 << ((reg)->access_width - 1)) : (reg)->bit_width)
-- 
2.25.1

[Patch 3/5] ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from sysfs

[Sumit Gupta]

Add support to update the CPC registers used for Autonomous
Performance Level Selection from acpi_cppc sysfs store nodes.
Registers supported for updation are:
- Engergy Performance Preference (EPP): energy_perf
- Autonomous Selection: auto_sel
- Maximum Performance: max_perf
- Minimum Performance: min_perf

Also, enable show nodes to read of the following CPC registers:
- Performance Limited: perf_limited
- Autonomous Activity Window: auto_activity_window

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 drivers/acpi/cppc_acpi.c | 191 ++++++++++++++++++++++++++++++++++++---
 include/acpi/cppc_acpi.h |   5 +
 2 files changed, 183 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index cc2bf958e84f..c60ad66ece85 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -170,6 +170,133 @@ __ATTR(_name, 0644, show_##_name, store_##_name)
 	}								\
 	define_one_cppc(member_name, mode)
 
+static ssize_t store_min_perf(struct kobject *kobj, struct kobj_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	struct cpufreq_policy *policy;
+	struct cppc_cpudata *cpu_data;
+	u32 min_perf, input;
+	int ret;
+
+	policy = cpufreq_cpu_get(cpc_ptr->cpu_id);
+	cpu_data = policy->driver_data;
+
+	if (kstrtouint(buf, 10, &input))
+		return -EINVAL;
+
+	if (input > cpu_data->perf_ctrls.max_perf)
+		return -EINVAL;
+
+	input = clamp(input, cpu_data->perf_caps.lowest_perf, cpu_data->perf_caps.highest_perf);
+
+	min_perf = cpu_data->perf_ctrls.min_perf;
+	cpu_data->perf_ctrls.min_perf = input;
+
+	ret = cppc_set_perf_ctrls(cpc_ptr->cpu_id, &cpu_data->perf_ctrls);
+	if (ret) {
+		pr_debug("Err writing CPU%d perf ctrls: ret:%d\n", cpc_ptr->cpu_id, ret);
+		cpu_data->perf_ctrls.min_perf = min_perf;
+		return ret;
+	}
+	cpufreq_cpu_put(policy);
+
+	return count;
+}
+
+static ssize_t store_max_perf(struct kobject *kobj, struct kobj_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	struct cpufreq_policy *policy;
+	struct cppc_cpudata *cpu_data;
+	u32 max_perf, input;
+	int ret;
+
+	policy = cpufreq_cpu_get(cpc_ptr->cpu_id);
+	cpu_data = policy->driver_data;
+
+	if (kstrtouint(buf, 10, &input))
+		return -EINVAL;
+
+	if (input < cpu_data->perf_ctrls.min_perf)
+		return -EINVAL;
+
+	input = clamp(input, cpu_data->perf_caps.lowest_perf, cpu_data->perf_caps.highest_perf);
+
+	max_perf = cpu_data->perf_ctrls.max_perf;
+	cpu_data->perf_ctrls.max_perf = input;
+
+	ret = cppc_set_perf_ctrls(cpc_ptr->cpu_id, &cpu_data->perf_ctrls);
+	if (ret) {
+		pr_debug("Err writing CPU%d perf ctrls: ret:%d\n", cpc_ptr->cpu_id, ret);
+		cpu_data->perf_ctrls.max_perf = max_perf;
+		return ret;
+	}
+	cpufreq_cpu_put(policy);
+
+	return count;
+}
+
+static ssize_t store_energy_perf(struct kobject *kobj, struct kobj_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	struct cpufreq_policy *policy;
+	struct cppc_cpudata *cpu_data;
+	u64 epp, input;
+	int ret;
+
+	policy = cpufreq_cpu_get(cpc_ptr->cpu_id);
+	cpu_data = policy->driver_data;
+
+	if (kstrtou64(buf, 10, &input))
+		return -EINVAL;
+
+	input = clamp(input, CPPC_EPP_PERFORMANCE_PREF, CPPC_EPP_ENERGY_EFFICIENCY_PREF);
+
+	epp = cpu_data->perf_ctrls.energy_perf;
+	cpu_data->perf_ctrls.energy_perf = input;
+
+	ret = cppc_set_epp_perf(cpc_ptr->cpu_id, &cpu_data->perf_ctrls,
+				cpu_data->perf_caps.auto_sel);
+	if (ret) {
+		pr_debug("failed to set energy perf value (%d)\n", ret);
+		cpu_data->perf_ctrls.energy_perf = epp;
+		return ret;
+	}
+	cpufreq_cpu_put(policy);
+
+	return count;
+}
+
+static ssize_t store_auto_sel(struct kobject *kobj, struct kobj_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct cpc_desc *cpc_ptr = to_cpc_desc(kobj);
+	struct cpufreq_policy *policy;
+	struct cppc_cpudata *cpu_data;
+	bool input = false;
+	int ret;
+
+	policy = cpufreq_cpu_get(cpc_ptr->cpu_id);
+	cpu_data = policy->driver_data;
+
+	if (kstrtobool(buf, &input))
+		return -EINVAL;
+
+	ret = cppc_set_auto_sel(cpc_ptr->cpu_id, input);
+	if (ret) {
+		pr_info("failed to set autonomous selection (%d)\n", ret);
+		return ret;
+	}
+	cpu_data->perf_caps.auto_sel = input;
+
+	cpufreq_cpu_put(policy);
+
+	return count;
+}
+
 sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, highest_perf, ro);
 sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_perf, ro);
 sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_perf, ro);
@@ -177,9 +304,16 @@ sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_nonlinear_perf, ro);
 sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, guaranteed_perf, ro);
 sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_freq, ro);
 sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq, ro);
+sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, auto_sel, rw);
 
 sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, reference_perf, ro);
 sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, wraparound_time, ro);
+sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, perf_limited, ro);
+
+sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, min_perf, rw);
+sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, max_perf, rw);
+sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, energy_perf, rw);
+sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, auto_activity_window, ro);
 
 /* Check for valid access_width, otherwise, fallback to using bit_width */
 #define GET_BIT_WIDTH(reg) ((reg)->access_width ? (8 << ((reg)->access_width - 1)) : (reg)->bit_width)
@@ -218,6 +352,12 @@ static struct attribute *cppc_attrs[] = {
 	&nominal_perf.attr,
 	&nominal_freq.attr,
 	&lowest_freq.attr,
+	&auto_sel.attr,
+	&max_perf.attr,
+	&min_perf.attr,
+	&perf_limited.attr,
+	&auto_activity_window.attr,
+	&energy_perf.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(cppc);
@@ -1286,8 +1426,8 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
 	struct cpc_register_resource *highest_reg, *lowest_reg,
 		*lowest_non_linear_reg, *nominal_reg, *guaranteed_reg,
-		*low_freq_reg = NULL, *nom_freq_reg = NULL;
-	u64 high, low, guaranteed, nom, min_nonlinear, low_f = 0, nom_f = 0;
+		*low_freq_reg = NULL, *nom_freq_reg = NULL, *auto_sel_reg = NULL;
+	u64 high, low, guaranteed, nom, min_nonlinear, low_f = 0, nom_f = 0, auto_sel = 0;
 	int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
 	struct cppc_pcc_data *pcc_ss_data = NULL;
 	int ret = 0, regs_in_pcc = 0;
@@ -1304,11 +1444,12 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	low_freq_reg = &cpc_desc->cpc_regs[LOWEST_FREQ];
 	nom_freq_reg = &cpc_desc->cpc_regs[NOMINAL_FREQ];
 	guaranteed_reg = &cpc_desc->cpc_regs[GUARANTEED_PERF];
+	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
 
 	/* Are any of the regs PCC ?*/
 	if (CPC_IN_PCC(highest_reg) || CPC_IN_PCC(lowest_reg) ||
 		CPC_IN_PCC(lowest_non_linear_reg) || CPC_IN_PCC(nominal_reg) ||
-		CPC_IN_PCC(low_freq_reg) || CPC_IN_PCC(nom_freq_reg)) {
+		CPC_IN_PCC(low_freq_reg) || CPC_IN_PCC(nom_freq_reg) || CPC_IN_PCC(auto_sel_reg)) {
 		if (pcc_ss_id < 0) {
 			pr_debug("Invalid pcc_ss_id\n");
 			return -ENODEV;
@@ -1356,6 +1497,9 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	perf_caps->lowest_freq = low_f;
 	perf_caps->nominal_freq = nom_f;
 
+	if (CPC_SUPPORTED(auto_sel_reg))
+		cpc_read(cpunum, auto_sel_reg, &auto_sel);
+	perf_caps->auto_sel = (bool)auto_sel;
 
 out_err:
 	if (regs_in_pcc)
@@ -1535,8 +1679,22 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
 		   CPC_SUPPORTED(epp_set_reg) && CPC_IN_FFH(epp_set_reg)) {
 		ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
 	} else {
-		ret = -ENOTSUPP;
-		pr_debug("_CPC in PCC and _CPC in FFH are not supported\n");
+		if (CPC_SUPPORTED(auto_sel_reg) && CPC_SUPPORTED(epp_set_reg)) {
+			ret = cpc_write(cpu, auto_sel_reg, enable);
+			if (ret) {
+				pr_debug("Error in writing auto_sel for CPU:%d\n", cpu);
+				return ret;
+			}
+
+			ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
+			if (ret) {
+				pr_debug("Error in writing energy_perf for CPU:%d\n", cpu);
+				return ret;
+			}
+		} else {
+			ret = -EOPNOTSUPP;
+			pr_debug("_CPC in PCC and _CPC in FFH are not supported\n");
+		}
 	}
 
 	return ret;
@@ -1553,6 +1711,7 @@ int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
 	struct cpc_register_resource *auto_sel_reg;
 	u64  auto_sel;
+	int ret = 0;
 
 	if (!cpc_desc) {
 		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
@@ -1561,13 +1720,9 @@ int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 
 	auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
 
-	if (!CPC_SUPPORTED(auto_sel_reg))
-		pr_warn_once("Autonomous mode is not unsupported!\n");
-
 	if (CPC_IN_PCC(auto_sel_reg)) {
 		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
 		struct cppc_pcc_data *pcc_ss_data = NULL;
-		int ret = 0;
 
 		if (pcc_ss_id < 0)
 			return -ENODEV;
@@ -1588,7 +1743,15 @@ int cppc_get_auto_sel_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 		return ret;
 	}
 
-	return 0;
+	if (CPC_SUPPORTED(auto_sel_reg)) {
+		cpc_read(cpunum, auto_sel_reg, &auto_sel);
+	} else {
+		pr_debug("Autonomous mode is not unsupported!\n");
+		ret = -EOPNOTSUPP;
+	}
+	perf_caps->auto_sel = (bool)auto_sel;
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_get_auto_sel_caps);
 
@@ -1630,11 +1793,13 @@ int cppc_set_auto_sel(int cpu, bool enable)
 		/* after writing CPC, transfer the ownership of PCC to platform */
 		ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
 		up_write(&pcc_ss_data->pcc_lock);
-	} else {
-		ret = -ENOTSUPP;
-		pr_debug("_CPC in PCC is not supported\n");
+
+		return ret;
 	}
 
+	if (CPC_SUPPORTED(auto_sel_reg))
+		ret = cpc_write(cpu, auto_sel_reg, enable);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(cppc_set_auto_sel);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 31f4fd288b65..b072ef11f128 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -32,6 +32,9 @@
 #define	CMD_READ 0
 #define	CMD_WRITE 1
 
+#define CPPC_EPP_PERFORMANCE_PREF			0x00
+#define CPPC_EPP_ENERGY_EFFICIENCY_PREF			0xFF
+
 /* Each register has the folowing format. */
 struct cpc_reg {
 	u8 descriptor;
@@ -118,6 +121,7 @@ struct cppc_perf_ctrls {
 	u32 min_perf;
 	u32 desired_perf;
 	u32 energy_perf;
+	u32 auto_activity_window;
 };
 
 struct cppc_perf_fb_ctrs {
@@ -125,6 +129,7 @@ struct cppc_perf_fb_ctrs {
 	u64 delivered;
 	u64 reference_perf;
 	u64 wraparound_time;
+	u32 perf_limited;
 };
 
 /* Per CPU container for runtime CPPC management. */
-- 
2.25.1

Re: [Patch 3/5] ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from sysfs

[Pierre Gondois]

Hello Sumit,

On 2/11/25 11:37, Sumit Gupta wrote:
> Add support to update the CPC registers used for Autonomous
> Performance Level Selection from acpi_cppc sysfs store nodes.
> Registers supported for updation are:
> - Engergy Performance Preference (EPP): energy_perf
> - Autonomous Selection: auto_sel
> - Maximum Performance: max_perf
> - Minimum Performance: min_perf
> 
> Also, enable show nodes to read of the following CPC registers:
> - Performance Limited: perf_limited
> - Autonomous Activity Window: auto_activity_window
> 
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
>   drivers/acpi/cppc_acpi.c | 191 ++++++++++++++++++++++++++++++++++++---
>   include/acpi/cppc_acpi.h |   5 +
>   2 files changed, 183 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index cc2bf958e84f..c60ad66ece85 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c

[...]

>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, highest_perf, ro);
>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_perf, ro);
>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_perf, ro);
> @@ -177,9 +304,16 @@ sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_nonlinear_perf, ro);
>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, guaranteed_perf, ro);
>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_freq, ro);
>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq, ro);
> +sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, auto_sel, rw);
>   
>   sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, reference_perf, ro);
>   sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, wraparound_time, ro);
> +sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, perf_limited, ro);
> +
> +sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, min_perf, rw);
> +sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, max_perf, rw);

IIUC, this means that users can modify the min/max performance levels of the CPU
without having the cpufreq framework notified. Meaning that if a user modifies these
levels, the frequency selection will be done using the initial min/max performance
level.
I think it would be better not allow users to modifies these values directly. Reliying
on existing scaling_min_freq/scaling_max_freq files would be better IMO.

Regards,
Pierre

Re: [Patch 3/5] ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from sysfs

[Sumit Gupta]

On 24/02/25 15:54, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hello Sumit,
> 
> On 2/11/25 11:37, Sumit Gupta wrote:
>> Add support to update the CPC registers used for Autonomous
>> Performance Level Selection from acpi_cppc sysfs store nodes.
>> Registers supported for updation are:
>> - Engergy Performance Preference (EPP): energy_perf
>> - Autonomous Selection: auto_sel
>> - Maximum Performance: max_perf
>> - Minimum Performance: min_perf
>>
>> Also, enable show nodes to read of the following CPC registers:
>> - Performance Limited: perf_limited
>> - Autonomous Activity Window: auto_activity_window
>>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 191 ++++++++++++++++++++++++++++++++++++---
>>   include/acpi/cppc_acpi.h |   5 +
>>   2 files changed, 183 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index cc2bf958e84f..c60ad66ece85 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
> 
> [...]
> 
>>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, highest_perf, ro);
>>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_perf, ro);
>>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_perf, ro);
>> @@ -177,9 +304,16 @@ sysfs_cppc_data(cppc_get_perf_caps, 
>> cppc_perf_caps, lowest_nonlinear_perf, ro);
>>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, guaranteed_perf, 
>> ro);
>>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, lowest_freq, ro);
>>   sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, nominal_freq, ro);
>> +sysfs_cppc_data(cppc_get_perf_caps, cppc_perf_caps, auto_sel, rw);
>>
>>   sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, 
>> reference_perf, ro);
>>   sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, 
>> wraparound_time, ro);
>> +sysfs_cppc_data(cppc_get_perf_fb_ctrs, cppc_perf_fb_ctrs, 
>> perf_limited, ro);
>> +
>> +sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, min_perf, rw);
>> +sysfs_cppc_data(cppc_get_perf_ctrls, cppc_perf_ctrls, max_perf, rw);
> 
> IIUC, this means that users can modify the min/max performance levels of 
> the CPU
> without having the cpufreq framework notified. Meaning that if a user 
> modifies these
> levels, the frequency selection will be done using the initial min/max 
> performance
> level.
> I think it would be better not allow users to modifies these values 
> directly. Reliying
> on existing scaling_min_freq/scaling_max_freq files would be better IMO.
> 
> Regards,
> Pierre


Hi Pierre,

Sorry to get back late on this.

Providing the option to modify these registers from sysfs will help to
easily tune for Power and Performance.
When min/max_perf are updated or auto_sel is enabled from sysfs, the
policy min/max can also be updated with those values.
When auto_sel is disabled for a CPU from sysfs, then its policy min/max
can be updated back to the nominal_perf and lowest_non_linear_perf.
Was thinking of doing this in v2 after conclusion of [1].

[1] 
https://lore.kernel.org/lkml/f0f1b31b-a0fc-4d21-8b79-c896833dae35@nvidia.com/

Best Regards,
Sumit Gupta

[Patch 4/5] Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt

[Sumit Gupta]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="yes", Size: 4117 bytes --]

Add information about the CPC registers used during Autonomous
Performance Level Selection mode. Also, add information about other
regsiters like Guaranteed performance and Performance limited.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 Documentation/admin-guide/acpi/cppc_sysfs.rst | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/admin-guide/acpi/cppc_sysfs.rst b/Documentation/admin-guide/acpi/cppc_sysfs.rst
index 36981c667823..ff3f48d95bb7 100644
--- a/Documentation/admin-guide/acpi/cppc_sysfs.rst
+++ b/Documentation/admin-guide/acpi/cppc_sysfs.rst
@@ -27,22 +27,33 @@ for each cpu X::
   $ ls -lR  /sys/devices/system/cpu/cpu0/acpi_cppc/
   /sys/devices/system/cpu/cpu0/acpi_cppc/:
   total 0
+  -r--r--r-- 1 root root 65536 Mar  5 19:38 auto_activity_window
+  -rw-r--r-- 1 root root 65536 Mar  5 19:38 auto_sel
+  -rw-r--r-- 1 root root 65536 Mar  5 19:38 energy_perf
   -r--r--r-- 1 root root 65536 Mar  5 19:38 feedback_ctrs
+  -r--r--r-- 1 root root 65536 Mar  5 19:38 guaranteed_perf
   -r--r--r-- 1 root root 65536 Mar  5 19:38 highest_perf
   -r--r--r-- 1 root root 65536 Mar  5 19:38 lowest_freq
   -r--r--r-- 1 root root 65536 Mar  5 19:38 lowest_nonlinear_perf
   -r--r--r-- 1 root root 65536 Mar  5 19:38 lowest_perf
+  -rw-r--r-- 1 root root 65536 Mar  5 19:38 max_perf
+  -rw-r--r-- 1 root root 65536 Mar  5 19:38 min_perf
   -r--r--r-- 1 root root 65536 Mar  5 19:38 nominal_freq
   -r--r--r-- 1 root root 65536 Mar  5 19:38 nominal_perf
+  -r--r--r-- 1 root root 65536 Mar  5 19:38 per_limited
   -r--r--r-- 1 root root 65536 Mar  5 19:38 reference_perf
   -r--r--r-- 1 root root 65536 Mar  5 19:38 wraparound_time
 
+Performance Capabilities / Thresholds:
 * highest_perf : Highest performance of this processor (abstract scale).
 * nominal_perf : Highest sustained performance of this processor
   (abstract scale).
 * lowest_nonlinear_perf : Lowest performance of this processor with nonlinear
   power savings (abstract scale).
 * lowest_perf : Lowest performance of this processor (abstract scale).
+* guaranteed_perf : Current maximum sustained performance level of a processor,
+  taking into account all known external constraints. All processors are expected
+  to be able to sustain their guaranteed performance levels simultaneously.
 
 * lowest_freq : CPU frequency corresponding to lowest_perf (in MHz).
 * nominal_freq : CPU frequency corresponding to nominal_perf (in MHz).
@@ -50,6 +61,7 @@ for each cpu X::
   frequency instead of abstract scale. These values should not be used for any
   functional decisions.
 
+Performance Feedback:
 * feedback_ctrs : Includes both Reference and delivered performance counter.
   Reference counter ticks up proportional to processor's reference performance.
   Delivered counter ticks up proportional to processor's delivered performance.
@@ -57,6 +69,22 @@ for each cpu X::
   (seconds).
 * reference_perf : Performance level at which reference performance counter
   accumulates (abstract scale).
+* perf_limited : Set when Delivered Performance has been constrained due to an
+  unpredictable event. It is not utilized when Autonomous Selection is enabled.
+
+Performance Controls:
+* max_perf : Maximum performance level at which the platform may run in the
+  range [Lowest Performance, Highest Performance], inclusive.
+* min_perf : Minimum performance level at which the platform may run in the
+  range [Lowest Performance, Highest Performance], inclusive but must be set
+  to a value that is less than or equal to that specified by the max_perf.
+* auto_sel : Enable Autonomous Performance Level Selection on this processor.
+* auto_activity_window : Indicates a moving utilization sensitivity window to
+  the platform’s autonomous selection policy.
+* energy_perf: Provides a value ranging from 0 (performance preference) to
+  0xFF (energy efficiency preference) that influences the rate of performance
+  increase /decrease and the result of the hardware's energy efficiency and
+  performance optimization policies.
 
 
 Computing Average Delivered Performance
-- 
2.25.1

[Patch 5/5] cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode

[Sumit Gupta]

Add new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver for
supporting the Autonomous Performance Level Selection and Engergy
Performance Preference (EPP) as per the ACPI specification.
Autonomous selection will get enabled during boot if 'cppc_auto_sel'
bootarg is present or the 'Autonomous Selection Enable' register is
set before kernel boot.

When Autonomous selection capability is enabled, then the hardware is
allowed to autonomously select the CPU frequency within the min and
max perf boundaries according to EPP hints.

EPP values range from '0x0'(performance preference) to '0xFF'(energy
efficiency preference). It influences the rate of performance
increase/decrease and the result of the hardware's energy efficiency
and performance optimization policies.

Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---
 .../admin-guide/kernel-parameters.txt         |  11 +
 drivers/cpufreq/cppc_cpufreq.c                | 234 +++++++++++++++++-
 2 files changed, 241 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index aa7447f8837c..8777970e6e35 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -904,6 +904,17 @@
 			Format:
 			<first_slot>,<last_slot>,<port>,<enum_bit>[,<debug>]
 
+	cppc_auto_sel=  [CPU_FREQ] Autonomous Performance Level Selection.
+			When Autonomous selection is enabled, then the hardware is
+			allowed to autonomously select the CPU frequency.
+			In Autonomous mode, Engergy Performance Preference(EPP)
+			provides input to the hardware to favour performance (0x0)
+			or energy efficiency (0xff).
+			Format: <bool>
+			Default: disabled.
+			0: force disabled
+			1: force enabled
+
 	cpuidle.off=1	[CPU_IDLE]
 			disable the cpuidle sub-system
 
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 17c49653a3c4..a4fa46cc2a6f 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -34,7 +34,10 @@
  */
 static LIST_HEAD(cpu_data_list);
 
-static struct cpufreq_driver cppc_cpufreq_driver;
+/* Autonomous Selection */
+static bool auto_sel_mode;
+
+static struct cpufreq_driver *current_cppc_cpufreq_driver;
 
 #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
 static enum {
@@ -519,7 +522,7 @@ static int populate_efficiency_class(void)
 		}
 		index++;
 	}
-	cppc_cpufreq_driver.register_em = cppc_cpufreq_register_em;
+	current_cppc_cpufreq_driver->register_em = cppc_cpufreq_register_em;
 
 	return 0;
 }
@@ -567,6 +570,12 @@ static struct cppc_cpudata *cppc_cpufreq_get_cpu_data(unsigned int cpu)
 		goto free_mask;
 	}
 
+	ret = cppc_get_perf_ctrls(cpu, &cpu_data->perf_ctrls);
+	if (ret) {
+		pr_debug("Err reading CPU%d perf ctrls: ret:%d\n", cpu, ret);
+		goto free_mask;
+	}
+
 	list_add(&cpu_data->node, &cpu_data_list);
 
 	return cpu_data;
@@ -672,6 +681,171 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	return ret;
 }
 
+static int cppc_cpufreq_epp_cpu_init(struct cpufreq_policy *policy)
+{
+	unsigned int cpu = policy->cpu;
+	struct cppc_cpudata *cpu_data;
+	struct cppc_perf_ctrls *ctrls;
+	struct cppc_perf_caps *caps;
+	int ret;
+
+	cpu_data = cppc_cpufreq_get_cpu_data(cpu);
+	if (!cpu_data) {
+		pr_err("Error in acquiring _CPC/_PSD data for CPU%d.\n", cpu);
+		return -ENODEV;
+	}
+	caps = &cpu_data->perf_caps;
+	ctrls = &cpu_data->perf_ctrls;
+	policy->driver_data = cpu_data;
+
+	policy->min = cppc_perf_to_khz(caps, ctrls->min_perf);
+	policy->max = cppc_perf_to_khz(caps, ctrls->max_perf);
+
+	/*
+	 * Set cpuinfo.min_freq to Lowest to make the full range of performance
+	 * available if userspace wants to use any perf between lowest & minimum perf
+	 */
+	policy->cpuinfo.min_freq = cppc_perf_to_khz(caps, caps->lowest_perf);
+	policy->cpuinfo.max_freq = cppc_perf_to_khz(caps, caps->highest_perf);
+
+	policy->transition_delay_us = cppc_cpufreq_get_transition_delay_us(cpu);
+	policy->shared_type = cpu_data->shared_type;
+
+	switch (policy->shared_type) {
+	case CPUFREQ_SHARED_TYPE_HW:
+	case CPUFREQ_SHARED_TYPE_NONE:
+		/* Nothing to be done - we'll have a policy for each CPU */
+		break;
+	case CPUFREQ_SHARED_TYPE_ANY:
+		/*
+		 * All CPUs in the domain will share a policy and all cpufreq
+		 * operations will use a single cppc_cpudata structure stored
+		 * in policy->driver_data.
+		 */
+		cpumask_copy(policy->cpus, cpu_data->shared_cpu_map);
+		break;
+	default:
+		pr_debug("Unsupported CPU co-ord type: %d\n",
+			 policy->shared_type);
+		ret = -EFAULT;
+		goto out;
+	}
+
+	/* Set policy->cur to max now. The governors will adjust later. */
+	policy->cur = cppc_perf_to_khz(caps, caps->highest_perf);
+
+	ret = cppc_set_perf_ctrls(cpu, &cpu_data->perf_ctrls);
+	if (ret) {
+		pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
+			 caps->highest_perf, cpu, ret);
+		goto out;
+	}
+
+	cppc_cpufreq_cpu_fie_init(policy);
+	return 0;
+
+out:
+	cppc_cpufreq_put_cpu_data(policy);
+	return ret;
+}
+
+static int cppc_cpufreq_epp_update_perf_ctrls(struct cpufreq_policy *policy,
+					      u32 highest_perf, u32 lowest_perf)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+	int ret;
+
+	pr_debug("cpu%d, curr max_perf:%u, curr min_perf:%u, highest_perf:%u, lowest_perf:%u\n",
+		 policy->cpu, cpu_data->perf_ctrls.max_perf, cpu_data->perf_ctrls.min_perf,
+		 highest_perf, lowest_perf);
+
+	cpu_data->perf_ctrls.max_perf = highest_perf;
+	cpu_data->perf_ctrls.min_perf = lowest_perf;
+
+	ret = cppc_set_perf_ctrls(policy->cpu, &cpu_data->perf_ctrls);
+	if (ret) {
+		pr_debug("Failed to set perf_ctrls on CPU:%d. ret:%d\n", policy->cpu, ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int cppc_cpufreq_epp_update_auto_mode(struct cpufreq_policy *policy, int auto_sel, u32 epp)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+	int ret, curr_epp;
+
+	curr_epp = cpu_data->perf_ctrls.energy_perf;
+	pr_debug("cpu%d, curr epp:%u, new epp:%u, curr mode:%u, new mode:%u\n",
+		 curr_epp, epp, cpu_data->perf_caps.auto_sel, auto_sel);
+
+	/* set Performance preference as default */
+	cpu_data->perf_ctrls.energy_perf = epp;
+	ret = cppc_set_epp_perf(policy->cpu, &cpu_data->perf_ctrls, auto_sel);
+	if (ret < 0) {
+		pr_err("failed to set energy perf value (%d)\n", ret);
+		cpu_data->perf_ctrls.energy_perf = curr_epp;
+		return ret;
+	}
+	cpu_data->perf_caps.auto_sel = auto_sel;
+
+	return ret;
+}
+
+static int cppc_cpufreq_epp_update_perf(struct cpufreq_policy *policy, int auto_sel, u32 epp,
+					u32 highest_perf, u32 lowest_perf)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+	int ret;
+
+	ret = cppc_cpufreq_epp_update_perf_ctrls(policy, highest_perf, lowest_perf);
+	if (ret)
+		return ret;
+
+	ret = cppc_cpufreq_epp_update_auto_mode(policy, auto_sel, epp);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static int cppc_cpufreq_epp_set_policy(struct cpufreq_policy *policy)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+	int ret;
+
+	if (!cpu_data)
+		return -ENODEV;
+
+	ret = cppc_cpufreq_epp_update_perf(policy, true, CPPC_EPP_PERFORMANCE_PREF,
+					   cpu_data->perf_caps.highest_perf,
+					   cpu_data->perf_caps.lowest_perf);
+	if (ret)
+		return ret;
+
+	return ret;
+}
+
+static void cppc_cpufreq_epp_cpu_exit(struct cpufreq_policy *policy)
+{
+	struct cppc_cpudata *cpu_data = policy->driver_data;
+	int ret;
+
+	cppc_cpufreq_cpu_fie_exit(policy);
+
+	cpu_data->perf_ctrls.desired_perf = cpu_data->perf_caps.lowest_perf;
+
+	cppc_cpufreq_epp_update_perf_ctrls(policy, cpu_data->perf_caps.highest_perf,
+					   cpu_data->perf_caps.lowest_perf);
+
+	ret = cppc_set_auto_sel(policy->cpu, false);
+	if (ret)
+		pr_debug("failed to disable autonomous selection (%d)\n", ret);
+
+	cppc_cpufreq_put_cpu_data(policy);
+}
+
 static void cppc_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 {
 	struct cppc_cpudata *cpu_data = policy->driver_data;
@@ -828,17 +1002,57 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
 	.name = "cppc_cpufreq",
 };
 
+static struct cpufreq_driver cppc_cpufreq_epp_driver = {
+	.flags = CPUFREQ_CONST_LOOPS,
+	.verify = cppc_verify_policy,
+	.setpolicy = cppc_cpufreq_epp_set_policy,
+	.get = cppc_cpufreq_get_rate,
+	.init = cppc_cpufreq_epp_cpu_init,
+	.exit = cppc_cpufreq_epp_cpu_exit,
+	.attr = cppc_cpufreq_attr,
+	.name = "cppc_cpufreq_epp",
+};
+
+static int cppc_cpufreq_auto_sel_enable(bool auto_sel_mode)
+{
+	struct cppc_cpudata *cpu_data;
+	int cpu, pref, ret = 0;
+
+	if (auto_sel_mode) {
+		for_each_present_cpu(cpu) {
+			/* Enable autonomous mode */
+			ret = cppc_set_auto_sel(cpu, true);
+			if (ret)
+				pr_debug("failed to enable autonomous selection (%d)\n", ret);
+		}
+	}
+
+	return ret;
+}
+
 static int __init cppc_cpufreq_init(void)
 {
+	struct cppc_perf_caps caps;
 	int ret;
 
 	if (!acpi_cpc_valid())
 		return -ENODEV;
 
+	cppc_get_auto_sel_caps(0, &caps);
+	if (auto_sel_mode || caps.auto_sel) {
+		ret = cppc_cpufreq_auto_sel_enable(true);
+		if (ret)
+			return ret;
+
+		current_cppc_cpufreq_driver = &cppc_cpufreq_epp_driver;
+	} else {
+		current_cppc_cpufreq_driver = &cppc_cpufreq_driver;
+	}
+
 	cppc_freq_invariance_init();
 	populate_efficiency_class();
 
-	ret = cpufreq_register_driver(&cppc_cpufreq_driver);
+	ret = cpufreq_register_driver(current_cppc_cpufreq_driver);
 	if (ret)
 		cppc_freq_invariance_exit();
 
@@ -859,12 +1073,24 @@ static inline void free_cpu_data(void)
 
 static void __exit cppc_cpufreq_exit(void)
 {
-	cpufreq_unregister_driver(&cppc_cpufreq_driver);
+	cpufreq_unregister_driver(current_cppc_cpufreq_driver);
 	cppc_freq_invariance_exit();
 
 	free_cpu_data();
 }
 
+static int __init cppc_auto_sel_setup(char *buf)
+{
+	int ret = 0;
+
+	ret = kstrtobool(buf, &auto_sel_mode);
+	if (!ret)
+		pr_err("Wrong autonomous selection param\n");
+
+	return ret;
+}
+__setup("cppc_auto_sel=", cppc_auto_sel_setup);
+
 module_exit(cppc_cpufreq_exit);
 MODULE_AUTHOR("Ashwin Chaugule");
 MODULE_DESCRIPTION("CPUFreq driver based on the ACPI CPPC v5.0+ spec");
-- 
2.25.1

Re: [Patch 5/5] cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode

[kernel test robot]

Hi Sumit,

kernel test robot noticed the following build warnings:

[auto build test WARNING on next-20250210]
[cannot apply to rafael-pm/linux-next rafael-pm/bleeding-edge v6.14-rc2 v6.14-rc1 v6.13 linus/master v6.14-rc2]
[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/Sumit-Gupta/ACPI-CPPC-add-read-perf-ctrls-api-and-rename-few-existing/20250211-184154
base:   next-20250210
patch link:    https://lore.kernel.org/r/20250211103737.447704-6-sumitg%40nvidia.com
patch subject: [Patch 5/5] cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode
config: riscv-randconfig-001-20250212 (https://download.01.org/0day-ci/archive/20250212/202502121734.xMnvqs6o-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 6807164500e9920638e2ab0cdb4bf8321d24f8eb)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250212/202502121734.xMnvqs6o-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/202502121734.xMnvqs6o-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/cpufreq/cppc_cpufreq.c:780:68: warning: more '%' conversions than data arguments [-Wformat-insufficient-args]
     780 |         pr_debug("cpu%d, curr epp:%u, new epp:%u, curr mode:%u, new mode:%u\n",
         |                                                                          ~^
   include/linux/printk.h:631:30: note: expanded from macro 'pr_debug'
     631 |         no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
         |                                     ^~~
   drivers/cpufreq/cppc_cpufreq.c:11:37: note: expanded from macro 'pr_fmt'
      11 | #define pr_fmt(fmt)     "CPPC Cpufreq:" fmt
         |                                         ^~~
   include/linux/printk.h:135:11: note: expanded from macro 'no_printk'
     135 |                 _printk(fmt, ##__VA_ARGS__);            \
         |                         ^~~
>> drivers/cpufreq/cppc_cpufreq.c:799:23: warning: unused variable 'cpu_data' [-Wunused-variable]
     799 |         struct cppc_cpudata *cpu_data = policy->driver_data;
         |                              ^~~~~~~~
   drivers/cpufreq/cppc_cpufreq.c:1018:23: warning: unused variable 'cpu_data' [-Wunused-variable]
    1018 |         struct cppc_cpudata *cpu_data;
         |                              ^~~~~~~~
>> drivers/cpufreq/cppc_cpufreq.c:1019:11: warning: unused variable 'pref' [-Wunused-variable]
    1019 |         int cpu, pref, ret = 0;
         |                  ^~~~
   4 warnings generated.


vim +780 drivers/cpufreq/cppc_cpufreq.c

   773	
   774	static int cppc_cpufreq_epp_update_auto_mode(struct cpufreq_policy *policy, int auto_sel, u32 epp)
   775	{
   776		struct cppc_cpudata *cpu_data = policy->driver_data;
   777		int ret, curr_epp;
   778	
   779		curr_epp = cpu_data->perf_ctrls.energy_perf;
 > 780		pr_debug("cpu%d, curr epp:%u, new epp:%u, curr mode:%u, new mode:%u\n",
   781			 curr_epp, epp, cpu_data->perf_caps.auto_sel, auto_sel);
   782	
   783		/* set Performance preference as default */
   784		cpu_data->perf_ctrls.energy_perf = epp;
   785		ret = cppc_set_epp_perf(policy->cpu, &cpu_data->perf_ctrls, auto_sel);
   786		if (ret < 0) {
   787			pr_err("failed to set energy perf value (%d)\n", ret);
   788			cpu_data->perf_ctrls.energy_perf = curr_epp;
   789			return ret;
   790		}
   791		cpu_data->perf_caps.auto_sel = auto_sel;
   792	
   793		return ret;
   794	}
   795	
   796	static int cppc_cpufreq_epp_update_perf(struct cpufreq_policy *policy, int auto_sel, u32 epp,
   797						u32 highest_perf, u32 lowest_perf)
   798	{
 > 799		struct cppc_cpudata *cpu_data = policy->driver_data;
   800		int ret;
   801	
   802		ret = cppc_cpufreq_epp_update_perf_ctrls(policy, highest_perf, lowest_perf);
   803		if (ret)
   804			return ret;
   805	
   806		ret = cppc_cpufreq_epp_update_auto_mode(policy, auto_sel, epp);
   807		if (ret)
   808			return ret;
   809	
   810		return ret;
   811	}
   812	

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

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Viresh Kumar]

On 11-02-25, 16:07, Sumit Gupta wrote:
> This patchset supports the Autonomous Performance Level Selection mode
> in the cppc_cpufreq driver. The feature is part of the existing CPPC
> specification and already present in Intel and AMD specific pstate
> cpufreq drivers. The patchset adds the support in generic acpi cppc
> cpufreq driver.

Is there an overlap with:

https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/

?

> It adds a new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver
> for supporting the Autonomous Performance Level Selection and Energy
> Performance Preference (EPP).
> Autonomous selection will get enabled during boot if 'cppc_auto_sel'
> boot argument is passed or the 'Autonomous Selection Enable' register
> is already set before kernel boot. When enabled, the hardware is
> allowed to autonomously select the CPU frequency within the min and
> max perf boundaries using the Engergy Performance Preference hints.
> The EPP values range from '0x0'(performance preference) to '0xFF'
> (energy efficiency preference).
> 
> It also exposes the acpi_cppc sysfs nodes to update the epp, auto_sel
> and {min|max_perf} registers for changing the hints to hardware for
> Autonomous selection.
> 
> In a followup patch, plan to add support to dynamically switch the
> cpufreq driver instance from 'cppc_cpufreq_epp' to 'cppc_cpufreq' and
> vice-versa without reboot.
> 
> The patches are divided into below groups:
> - Patch [1-2]: Improvements. Can be applied independently.
> - Patch [3-4]: sysfs store nodes for Auto mode. Depend on Patch [1-2].
> - Patch [5]: Support for 'cppc_cpufreq_epp'. Uses a macro from [3].
> 
> Sumit Gupta (5):
>   ACPI: CPPC: add read perf ctrls api and rename few existing
>   ACPI: CPPC: expand macro to create store acpi_cppc sysfs node
>   ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from
>     sysfs
>   Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt
>   cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode
> 
>  Documentation/admin-guide/acpi/cppc_sysfs.rst |  28 ++
>  .../admin-guide/kernel-parameters.txt         |  11 +
>  drivers/acpi/cppc_acpi.c                      | 311 ++++++++++++++++--
>  drivers/cpufreq/cppc_cpufreq.c                | 260 ++++++++++++++-
>  include/acpi/cppc_acpi.h                      |  19 +-
>  5 files changed, 572 insertions(+), 57 deletions(-)

-- 
viresh

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[zhenglifeng (A)]

On 2025/2/11 18:44, Viresh Kumar wrote:
> On 11-02-25, 16:07, Sumit Gupta wrote:
>> This patchset supports the Autonomous Performance Level Selection mode
>> in the cppc_cpufreq driver. The feature is part of the existing CPPC
>> specification and already present in Intel and AMD specific pstate
>> cpufreq drivers. The patchset adds the support in generic acpi cppc
>> cpufreq driver.
> 
> Is there an overlap with:
> 
> https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> 
> ?

Ha, it looks like we're doing something very similar.

> 
>> It adds a new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver
>> for supporting the Autonomous Performance Level Selection and Energy
>> Performance Preference (EPP).
>> Autonomous selection will get enabled during boot if 'cppc_auto_sel'
>> boot argument is passed or the 'Autonomous Selection Enable' register
>> is already set before kernel boot. When enabled, the hardware is
>> allowed to autonomously select the CPU frequency within the min and
>> max perf boundaries using the Engergy Performance Preference hints.
>> The EPP values range from '0x0'(performance preference) to '0xFF'
>> (energy efficiency preference).
>>
>> It also exposes the acpi_cppc sysfs nodes to update the epp, auto_sel
>> and {min|max_perf} registers for changing the hints to hardware for
>> Autonomous selection.
>>
>> In a followup patch, plan to add support to dynamically switch the
>> cpufreq driver instance from 'cppc_cpufreq_epp' to 'cppc_cpufreq' and
>> vice-versa without reboot.
>>
>> The patches are divided into below groups:
>> - Patch [1-2]: Improvements. Can be applied independently.
>> - Patch [3-4]: sysfs store nodes for Auto mode. Depend on Patch [1-2].
>> - Patch [5]: Support for 'cppc_cpufreq_epp'. Uses a macro from [3].
>>
>> Sumit Gupta (5):
>>   ACPI: CPPC: add read perf ctrls api and rename few existing
>>   ACPI: CPPC: expand macro to create store acpi_cppc sysfs node
>>   ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from
>>     sysfs
>>   Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt
>>   cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode
>>
>>  Documentation/admin-guide/acpi/cppc_sysfs.rst |  28 ++
>>  .../admin-guide/kernel-parameters.txt         |  11 +
>>  drivers/acpi/cppc_acpi.c                      | 311 ++++++++++++++++--
>>  drivers/cpufreq/cppc_cpufreq.c                | 260 ++++++++++++++-
>>  include/acpi/cppc_acpi.h                      |  19 +-
>>  5 files changed, 572 insertions(+), 57 deletions(-)
> 

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Sumit Gupta]

> 
> On 2025/2/11 18:44, Viresh Kumar wrote:
>> On 11-02-25, 16:07, Sumit Gupta wrote:
>>> This patchset supports the Autonomous Performance Level Selection mode
>>> in the cppc_cpufreq driver. The feature is part of the existing CPPC
>>> specification and already present in Intel and AMD specific pstate
>>> cpufreq drivers. The patchset adds the support in generic acpi cppc
>>> cpufreq driver.
>>
>> Is there an overlap with:
>>
>> https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>
>> ?
> 
> Ha, it looks like we're doing something very similar.
> 

Hi Viresh,

Thank you for pointing to [1].

There seems to be some common points about updating the 'energy_perf'
and 'auto_sel' registers for autonomous mode but the current patchset
has more comprehensive changes to support Autonomous mode with the
cppc_cpufreq driver.

The patches in [1]:
1) Make the cpc register read/write API’s generic and improves error
    handling for 'CPC_IN_PCC'.
2) Expose sysfs under 'cppc_cpufreq_attr' to update 'auto_select',
    'auto_act_window' and 'epp' registers.

The current patch series:
1) Exposes sysfs under 'cppc_attrs' to keep CPC registers together.
2) Updates existing API’s to use new registers and creates new API
    with similar semantics to get all perf_ctrls.
3) Renames some existing API’s for clarity.
4) Use these existing API’s from acpi_cppc sysfs to update the CPC
    registers used in Autonomous mode:
    'auto_select', 'epp', 'min_perf', 'max_perf' registers.
5) Add separate 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq'
    driver to apply different limit and policy for Autonomous mode.
    Having it separate will avoid confusion between SW and HW mode.
    Also, it will be easy to scale and add new features in future
    without interference. Similar approach is used in Intel and AMD
    pstate drivers.

Please share inputs about the preferred approach.

Best Regards,
Sumit Gupta

[1] 
https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/


>>
>>> It adds a new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver
>>> for supporting the Autonomous Performance Level Selection and Energy
>>> Performance Preference (EPP).
>>> Autonomous selection will get enabled during boot if 'cppc_auto_sel'
>>> boot argument is passed or the 'Autonomous Selection Enable' register
>>> is already set before kernel boot. When enabled, the hardware is
>>> allowed to autonomously select the CPU frequency within the min and
>>> max perf boundaries using the Engergy Performance Preference hints.
>>> The EPP values range from '0x0'(performance preference) to '0xFF'
>>> (energy efficiency preference).
>>>
>>> It also exposes the acpi_cppc sysfs nodes to update the epp, auto_sel
>>> and {min|max_perf} registers for changing the hints to hardware for
>>> Autonomous selection.
>>>
>>> In a followup patch, plan to add support to dynamically switch the
>>> cpufreq driver instance from 'cppc_cpufreq_epp' to 'cppc_cpufreq' and
>>> vice-versa without reboot.
>>>
>>> The patches are divided into below groups:
>>> - Patch [1-2]: Improvements. Can be applied independently.
>>> - Patch [3-4]: sysfs store nodes for Auto mode. Depend on Patch [1-2].
>>> - Patch [5]: Support for 'cppc_cpufreq_epp'. Uses a macro from [3].
>>>
>>> Sumit Gupta (5):
>>>    ACPI: CPPC: add read perf ctrls api and rename few existing
>>>    ACPI: CPPC: expand macro to create store acpi_cppc sysfs node
>>>    ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from
>>>      sysfs
>>>    Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt
>>>    cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode
>>>
>>>   Documentation/admin-guide/acpi/cppc_sysfs.rst |  28 ++
>>>   .../admin-guide/kernel-parameters.txt         |  11 +
>>>   drivers/acpi/cppc_acpi.c                      | 311 ++++++++++++++++--
>>>   drivers/cpufreq/cppc_cpufreq.c                | 260 ++++++++++++++-
>>>   include/acpi/cppc_acpi.h                      |  19 +-
>>>   5 files changed, 572 insertions(+), 57 deletions(-)
>>
> 

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[zhenglifeng (A)]

On 2025/2/11 22:08, Sumit Gupta wrote:
> 
> 
>>
>> On 2025/2/11 18:44, Viresh Kumar wrote:
>>> On 11-02-25, 16:07, Sumit Gupta wrote:
>>>> This patchset supports the Autonomous Performance Level Selection mode
>>>> in the cppc_cpufreq driver. The feature is part of the existing CPPC
>>>> specification and already present in Intel and AMD specific pstate
>>>> cpufreq drivers. The patchset adds the support in generic acpi cppc
>>>> cpufreq driver.
>>>
>>> Is there an overlap with:
>>>
>>> https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>>
>>> ?
>>
>> Ha, it looks like we're doing something very similar.
>>
> 
> Hi Viresh,
> 
> Thank you for pointing to [1].
> 
> There seems to be some common points about updating the 'energy_perf'
> and 'auto_sel' registers for autonomous mode but the current patchset
> has more comprehensive changes to support Autonomous mode with the
> cppc_cpufreq driver.
> 
> The patches in [1]:
> 1) Make the cpc register read/write API’s generic and improves error
>    handling for 'CPC_IN_PCC'.
> 2) Expose sysfs under 'cppc_cpufreq_attr' to update 'auto_select',
>    'auto_act_window' and 'epp' registers.
> 
> The current patch series:
> 1) Exposes sysfs under 'cppc_attrs' to keep CPC registers together.
> 2) Updates existing API’s to use new registers and creates new API
>    with similar semantics to get all perf_ctrls.
> 3) Renames some existing API’s for clarity.
> 4) Use these existing API’s from acpi_cppc sysfs to update the CPC
>    registers used in Autonomous mode:
>    'auto_select', 'epp', 'min_perf', 'max_perf' registers.
> 5) Add separate 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq'
>    driver to apply different limit and policy for Autonomous mode.
>    Having it separate will avoid confusion between SW and HW mode.
>    Also, it will be easy to scale and add new features in future
>    without interference. Similar approach is used in Intel and AMD
>    pstate drivers.
> 
> Please share inputs about the preferred approach.
> 
> Best Regards,
> Sumit Gupta
> 
> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> 
> 

Hi Sumit,

Thanks for upstreaming this.

I think the changes to cppc_acpi in this patchset is inappropriate.

1) cppc_attrs are common sysfs for any system that supports CPPC. That
means, these interfaces would appear even if the cpufreq driver has already
managing it, e.g. amd-pstate and cppc_cpufreq. This would create multiple
interfaces to modify the same CPPC regs, which may probably introduce
concurrency and data consistency issues. Instead, exposing the interfaces
under cppc_cpufreq_attr decouples the write access to CPPC regs.

2) It's inappropriate to call cpufreq_cpu_get() in cppc_acpi. This file
currently provides interfaces for cpufreq drivers to use. It has no ABI
dependency on cpufreq at the moment.

Apart from the changes to cppc_acpi, I think the whole patchset in [1] can
be independent to this patchset. In other words, adding the
cppc_cpufreq_epp_driver could be standalone to discuss. I think combining
the use of ->setpolicy() and setting EPP could be a use case? Could you
explain more on the motivation of adding a new cppc_cpufreq_epp_driver?

[1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/

Regards,
Lifeng

>>>
>>>> It adds a new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver
>>>> for supporting the Autonomous Performance Level Selection and Energy
>>>> Performance Preference (EPP).
>>>> Autonomous selection will get enabled during boot if 'cppc_auto_sel'
>>>> boot argument is passed or the 'Autonomous Selection Enable' register
>>>> is already set before kernel boot. When enabled, the hardware is
>>>> allowed to autonomously select the CPU frequency within the min and
>>>> max perf boundaries using the Engergy Performance Preference hints.
>>>> The EPP values range from '0x0'(performance preference) to '0xFF'
>>>> (energy efficiency preference).
>>>>
>>>> It also exposes the acpi_cppc sysfs nodes to update the epp, auto_sel
>>>> and {min|max_perf} registers for changing the hints to hardware for
>>>> Autonomous selection.
>>>>
>>>> In a followup patch, plan to add support to dynamically switch the
>>>> cpufreq driver instance from 'cppc_cpufreq_epp' to 'cppc_cpufreq' and
>>>> vice-versa without reboot.
>>>>
>>>> The patches are divided into below groups:
>>>> - Patch [1-2]: Improvements. Can be applied independently.
>>>> - Patch [3-4]: sysfs store nodes for Auto mode. Depend on Patch [1-2].
>>>> - Patch [5]: Support for 'cppc_cpufreq_epp'. Uses a macro from [3].
>>>>
>>>> Sumit Gupta (5):
>>>>    ACPI: CPPC: add read perf ctrls api and rename few existing
>>>>    ACPI: CPPC: expand macro to create store acpi_cppc sysfs node
>>>>    ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from
>>>>      sysfs
>>>>    Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt
>>>>    cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode
>>>>
>>>>   Documentation/admin-guide/acpi/cppc_sysfs.rst |  28 ++
>>>>   .../admin-guide/kernel-parameters.txt         |  11 +
>>>>   drivers/acpi/cppc_acpi.c                      | 311 ++++++++++++++++--
>>>>   drivers/cpufreq/cppc_cpufreq.c                | 260 ++++++++++++++-
>>>>   include/acpi/cppc_acpi.h                      |  19 +-
>>>>   5 files changed, 572 insertions(+), 57 deletions(-)
>>>
>>

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Sumit Gupta]

On 12/02/25 16:22, zhenglifeng (A) wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 2025/2/11 22:08, Sumit Gupta wrote:
>>
>>
>>>
>>> On 2025/2/11 18:44, Viresh Kumar wrote:
>>>> On 11-02-25, 16:07, Sumit Gupta wrote:
>>>>> This patchset supports the Autonomous Performance Level Selection mode
>>>>> in the cppc_cpufreq driver. The feature is part of the existing CPPC
>>>>> specification and already present in Intel and AMD specific pstate
>>>>> cpufreq drivers. The patchset adds the support in generic acpi cppc
>>>>> cpufreq driver.
>>>>
>>>> Is there an overlap with:
>>>>
>>>> https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>>>
>>>> ?
>>>
>>> Ha, it looks like we're doing something very similar.
>>>
>>
>> Hi Viresh,
>>
>> Thank you for pointing to [1].
>>
>> There seems to be some common points about updating the 'energy_perf'
>> and 'auto_sel' registers for autonomous mode but the current patchset
>> has more comprehensive changes to support Autonomous mode with the
>> cppc_cpufreq driver.
>>
>> The patches in [1]:
>> 1) Make the cpc register read/write API’s generic and improves error
>>     handling for 'CPC_IN_PCC'.
>> 2) Expose sysfs under 'cppc_cpufreq_attr' to update 'auto_select',
>>     'auto_act_window' and 'epp' registers.
>>
>> The current patch series:
>> 1) Exposes sysfs under 'cppc_attrs' to keep CPC registers together.
>> 2) Updates existing API’s to use new registers and creates new API
>>     with similar semantics to get all perf_ctrls.
>> 3) Renames some existing API’s for clarity.
>> 4) Use these existing API’s from acpi_cppc sysfs to update the CPC
>>     registers used in Autonomous mode:
>>     'auto_select', 'epp', 'min_perf', 'max_perf' registers.
>> 5) Add separate 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq'
>>     driver to apply different limit and policy for Autonomous mode.
>>     Having it separate will avoid confusion between SW and HW mode.
>>     Also, it will be easy to scale and add new features in future
>>     without interference. Similar approach is used in Intel and AMD
>>     pstate drivers.
>>
>> Please share inputs about the preferred approach.
>>
>> Best Regards,
>> Sumit Gupta
>>
>> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>
>>
> 
> Hi Sumit,
> 
> Thanks for upstreaming this.
> 
> I think the changes to cppc_acpi in this patchset is inappropriate.
> 
> 1) cppc_attrs are common sysfs for any system that supports CPPC. That
> means, these interfaces would appear even if the cpufreq driver has already
> managing it, e.g. amd-pstate and cppc_cpufreq. This would create multiple
> interfaces to modify the same CPPC regs, which may probably introduce
> concurrency and data consistency issues. Instead, exposing the interfaces
> under cppc_cpufreq_attr decouples the write access to CPPC regs.
> 

Hi Lifeng,

I think its more appropriate to keep all the CPC registers together
instead of splitting the read only registers to the acpi_cppc sysfs
and read/write registers to the cpufreq sysfs.

Only the EPP register is written from Intel and AMD.
  $ grep cpufreq_freq_attr_rw drivers/cpufreq/* | grep -v scaling
  drivers/cpufreq/acpi-cpufreq.c:cpufreq_freq_attr_rw(cpb);
 
drivers/cpufreq/amd-pstate.c:cpufreq_freq_attr_rw(energy_performance_preference);
 
drivers/cpufreq/intel_pstate.c:cpufreq_freq_attr_rw(energy_performance_preference);

We are currently updating four registers and there can be more in
future like 'auto_act_window' update attribute in [1].
Changed to make this conditional with 'ifdef CONFIG_ACPI_CPPC_CPUFREQ'
to not create attributes for Intel/AMD.

  +++ b/drivers/acpi/cppc_acpi.c
  @@ static struct attribute *cppc_attrs[] = {
          &lowest_freq.attr,
  +#ifdef CONFIG_ACPI_CPPC_CPUFREQ
          &max_perf.attr,
          &min_perf.attr,
          &perf_limited.attr,
          &auto_activity_window.attr,
          &energy_perf.attr,
  +#endif

> 2) It's inappropriate to call cpufreq_cpu_get() in cppc_acpi. This file
> currently provides interfaces for cpufreq drivers to use. It has no ABI
> dependency on cpufreq at the moment.
> 

cpufreq_cpu_get() is already used by multiple non-cpufreq drivers.
So, don't think its a problem.
  $ grep -inr "= cpufreq_cpu_get(.*;" drivers/*| grep -v "cpufreq/"|wc -l
  10

> Apart from the changes to cppc_acpi, I think the whole patchset in [1] can
> be independent to this patchset. In other words, adding the
> cppc_cpufreq_epp_driver could be standalone to discuss. I think combining
> the use of ->setpolicy() and setting EPP could be a use case? Could you
> explain more on the motivation of adding a new cppc_cpufreq_epp_driver?
> 

With 'cppc_cpufreq_epp_driver', we provide an easy option to boot all
CPU's in auto mode with right epp and policy min/max equivalent of
{min|max}_perf. The mode can be found clearly with scaling_driver node. 
Separating the HW and SW mode based on driver instance also
makes it easy to scale later.
Advanced users can program sysfs to switch individual CPU's in and out
of the HW mode. We can update policy min/max values accordingly.
In this case, there can be some CPU's in SW mode with epp driver 
instance. But a separate instance will be more convenient for the
users who want all CPU's either in HW mode or in SW mode than having
to explicitly set all the values correctly.

Regards,
Sumit Gupta

> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> 
> Regards,
> Lifeng
> 
>>>>
>>>>> It adds a new 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq' driver
>>>>> for supporting the Autonomous Performance Level Selection and Energy
>>>>> Performance Preference (EPP).
>>>>> Autonomous selection will get enabled during boot if 'cppc_auto_sel'
>>>>> boot argument is passed or the 'Autonomous Selection Enable' register
>>>>> is already set before kernel boot. When enabled, the hardware is
>>>>> allowed to autonomously select the CPU frequency within the min and
>>>>> max perf boundaries using the Engergy Performance Preference hints.
>>>>> The EPP values range from '0x0'(performance preference) to '0xFF'
>>>>> (energy efficiency preference).
>>>>>
>>>>> It also exposes the acpi_cppc sysfs nodes to update the epp, auto_sel
>>>>> and {min|max_perf} registers for changing the hints to hardware for
>>>>> Autonomous selection.
>>>>>
>>>>> In a followup patch, plan to add support to dynamically switch the
>>>>> cpufreq driver instance from 'cppc_cpufreq_epp' to 'cppc_cpufreq' and
>>>>> vice-versa without reboot.
>>>>>
>>>>> The patches are divided into below groups:
>>>>> - Patch [1-2]: Improvements. Can be applied independently.
>>>>> - Patch [3-4]: sysfs store nodes for Auto mode. Depend on Patch [1-2].
>>>>> - Patch [5]: Support for 'cppc_cpufreq_epp'. Uses a macro from [3].
>>>>>
>>>>> Sumit Gupta (5):
>>>>>     ACPI: CPPC: add read perf ctrls api and rename few existing
>>>>>     ACPI: CPPC: expand macro to create store acpi_cppc sysfs node
>>>>>     ACPI: CPPC: support updating epp, auto_sel and {min|max_perf} from
>>>>>       sysfs
>>>>>     Documentation: ACPI: add autonomous mode ctrls info in cppc_sysfs.txt
>>>>>     cpufreq: CPPC: Add cppc_cpufreq_epp instance for Autonomous mode
>>>>>
>>>>>    Documentation/admin-guide/acpi/cppc_sysfs.rst |  28 ++
>>>>>    .../admin-guide/kernel-parameters.txt         |  11 +
>>>>>    drivers/acpi/cppc_acpi.c                      | 311 ++++++++++++++++--
>>>>>    drivers/cpufreq/cppc_cpufreq.c                | 260 ++++++++++++++-
>>>>>    include/acpi/cppc_acpi.h                      |  19 +-
>>>>>    5 files changed, 572 insertions(+), 57 deletions(-)
>>>>
>>>
> 

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Rafael J. Wysocki]

On Fri, Feb 14, 2025 at 8:09 AM Sumit Gupta <sumitg@nvidia.com> wrote:
>
>
>
> On 12/02/25 16:22, zhenglifeng (A) wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > On 2025/2/11 22:08, Sumit Gupta wrote:
> >>
> >>
> >>>
> >>> On 2025/2/11 18:44, Viresh Kumar wrote:
> >>>> On 11-02-25, 16:07, Sumit Gupta wrote:
> >>>>> This patchset supports the Autonomous Performance Level Selection mode
> >>>>> in the cppc_cpufreq driver. The feature is part of the existing CPPC
> >>>>> specification and already present in Intel and AMD specific pstate
> >>>>> cpufreq drivers. The patchset adds the support in generic acpi cppc
> >>>>> cpufreq driver.
> >>>>
> >>>> Is there an overlap with:
> >>>>
> >>>> https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> >>>>
> >>>> ?
> >>>
> >>> Ha, it looks like we're doing something very similar.
> >>>
> >>
> >> Hi Viresh,
> >>
> >> Thank you for pointing to [1].
> >>
> >> There seems to be some common points about updating the 'energy_perf'
> >> and 'auto_sel' registers for autonomous mode but the current patchset
> >> has more comprehensive changes to support Autonomous mode with the
> >> cppc_cpufreq driver.
> >>
> >> The patches in [1]:
> >> 1) Make the cpc register read/write API’s generic and improves error
> >>     handling for 'CPC_IN_PCC'.
> >> 2) Expose sysfs under 'cppc_cpufreq_attr' to update 'auto_select',
> >>     'auto_act_window' and 'epp' registers.
> >>
> >> The current patch series:
> >> 1) Exposes sysfs under 'cppc_attrs' to keep CPC registers together.
> >> 2) Updates existing API’s to use new registers and creates new API
> >>     with similar semantics to get all perf_ctrls.
> >> 3) Renames some existing API’s for clarity.
> >> 4) Use these existing API’s from acpi_cppc sysfs to update the CPC
> >>     registers used in Autonomous mode:
> >>     'auto_select', 'epp', 'min_perf', 'max_perf' registers.
> >> 5) Add separate 'cppc_cpufreq_epp' instance of the 'cppc_cpufreq'
> >>     driver to apply different limit and policy for Autonomous mode.
> >>     Having it separate will avoid confusion between SW and HW mode.
> >>     Also, it will be easy to scale and add new features in future
> >>     without interference. Similar approach is used in Intel and AMD
> >>     pstate drivers.
> >>
> >> Please share inputs about the preferred approach.
> >>
> >> Best Regards,
> >> Sumit Gupta
> >>
> >> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> >>
> >>
> >
> > Hi Sumit,
> >
> > Thanks for upstreaming this.
> >
> > I think the changes to cppc_acpi in this patchset is inappropriate.
> >
> > 1) cppc_attrs are common sysfs for any system that supports CPPC. That
> > means, these interfaces would appear even if the cpufreq driver has already
> > managing it, e.g. amd-pstate and cppc_cpufreq. This would create multiple
> > interfaces to modify the same CPPC regs, which may probably introduce
> > concurrency and data consistency issues. Instead, exposing the interfaces
> > under cppc_cpufreq_attr decouples the write access to CPPC regs.
> >
>
> Hi Lifeng,
>
> I think its more appropriate to keep all the CPC registers together
> instead of splitting the read only registers to the acpi_cppc sysfs
> and read/write registers to the cpufreq sysfs.
>
> Only the EPP register is written from Intel and AMD.
>   $ grep cpufreq_freq_attr_rw drivers/cpufreq/* | grep -v scaling
>   drivers/cpufreq/acpi-cpufreq.c:cpufreq_freq_attr_rw(cpb);
>
> drivers/cpufreq/amd-pstate.c:cpufreq_freq_attr_rw(energy_performance_preference);
>
> drivers/cpufreq/intel_pstate.c:cpufreq_freq_attr_rw(energy_performance_preference);
>
> We are currently updating four registers and there can be more in
> future like 'auto_act_window' update attribute in [1].
> Changed to make this conditional with 'ifdef CONFIG_ACPI_CPPC_CPUFREQ'
> to not create attributes for Intel/AMD.
>
>   +++ b/drivers/acpi/cppc_acpi.c
>   @@ static struct attribute *cppc_attrs[] = {
>           &lowest_freq.attr,
>   +#ifdef CONFIG_ACPI_CPPC_CPUFREQ
>           &max_perf.attr,
>           &min_perf.attr,
>           &perf_limited.attr,
>           &auto_activity_window.attr,
>           &energy_perf.attr,
>   +#endif
>
> > 2) It's inappropriate to call cpufreq_cpu_get() in cppc_acpi. This file
> > currently provides interfaces for cpufreq drivers to use. It has no ABI
> > dependency on cpufreq at the moment.
> >
>
> cpufreq_cpu_get() is already used by multiple non-cpufreq drivers.
> So, don't think its a problem.
>   $ grep -inr "= cpufreq_cpu_get(.*;" drivers/*| grep -v "cpufreq/"|wc -l
>   10
>
> > Apart from the changes to cppc_acpi, I think the whole patchset in [1] can
> > be independent to this patchset. In other words, adding the
> > cppc_cpufreq_epp_driver could be standalone to discuss. I think combining
> > the use of ->setpolicy() and setting EPP could be a use case? Could you
> > explain more on the motivation of adding a new cppc_cpufreq_epp_driver?
> >
>
> With 'cppc_cpufreq_epp_driver', we provide an easy option to boot all
> CPU's in auto mode with right epp and policy min/max equivalent of
> {min|max}_perf. The mode can be found clearly with scaling_driver node.
> Separating the HW and SW mode based on driver instance also
> makes it easy to scale later.
> Advanced users can program sysfs to switch individual CPU's in and out
> of the HW mode. We can update policy min/max values accordingly.
> In this case, there can be some CPU's in SW mode with epp driver
> instance. But a separate instance will be more convenient for the
> users who want all CPU's either in HW mode or in SW mode than having
> to explicitly set all the values correctly.

There seems to be some quite fundamental disagreement on how this
should be done, so I'm afraid I cannot do much about it ATM.

Please agree on a common approach and come back to me when you are ready.

Sending two concurrent patchsets under confusingly similar names again
and again isn't particularly helpful.

Thanks!

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Sumit Gupta]

On 19/02/25 00:53, Rafael J. Wysocki wrote:
> 
> There seems to be some quite fundamental disagreement on how this
> should be done, so I'm afraid I cannot do much about it ATM.
> 
> Please agree on a common approach and come back to me when you are ready.
> 
> Sending two concurrent patchsets under confusingly similar names again
> and again isn't particularly helpful.
> 
> Thanks!

Hi Rafael,

Thank you for looking into this.

Hi Lifeng,

As per the discussion, we can make the driver future extensible and
also can optimize the register read/write access.

I gave some thought and below is my proposal.

1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
    to read/write a cpc register.

2) Pick my patches in [2]:
    - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
                 Also, update existing API's to read/write regs in batch.
    - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
      all CPU's in Auto mode and set registers with right values.
      They can be updated after boot from sysfs to change hints to HW.
      I can use the optimized API's from [1] where required in [2].

Let me know if you are okay with this proposal.
I can also send an updated patch series with all the patches combined?

[1] 
https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
[2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/

Regards,
Sumit Gupta

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[zhenglifeng (A)]

On 2025/2/21 21:14, Sumit Gupta wrote:
> 
> 
> On 19/02/25 00:53, Rafael J. Wysocki wrote:
>>
>> There seems to be some quite fundamental disagreement on how this
>> should be done, so I'm afraid I cannot do much about it ATM.
>>
>> Please agree on a common approach and come back to me when you are ready.
>>
>> Sending two concurrent patchsets under confusingly similar names again
>> and again isn't particularly helpful.
>>
>> Thanks!
> 
> Hi Rafael,
> 
> Thank you for looking into this.
> 
> Hi Lifeng,
> 
> As per the discussion, we can make the driver future extensible and
> also can optimize the register read/write access.
> 
> I gave some thought and below is my proposal.
> 
> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>    to read/write a cpc register.

'patch 1-7' in [1] doesn't conflicts with [2], so can be reviewed and
applied separately. I would follow this up in that series.

> 
> 2) Pick my patches in [2]:
>    - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>                 Also, update existing API's to read/write regs in batch.
>    - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>      all CPU's in Auto mode and set registers with right values.
>      They can be updated after boot from sysfs to change hints to HW.
>      I can use the optimized API's from [1] where required in [2].
> 
> Let me know if you are okay with this proposal.
> I can also send an updated patch series with all the patches combined?

As mentioned above, 'patch 1-7' in [1] can be reviewed and applied
separately. No need to be combined with other patches.

About how to support auto selection mode in cppc_cpufreq, I think we need
to sort out usecases, scenarios, and requirements from both of us before we
disscus and agree on a design to implement. I am currently working on it
and will sent out my thoughts later.

Regards,
Lifeng

> 
> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
> 
> Regards,
> Sumit Gupta
> 

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[zhenglifeng (A)]

On 2025/2/21 21:14, Sumit Gupta wrote:
> 
> 
> On 19/02/25 00:53, Rafael J. Wysocki wrote:
>>
>> There seems to be some quite fundamental disagreement on how this
>> should be done, so I'm afraid I cannot do much about it ATM.
>>
>> Please agree on a common approach and come back to me when you are ready.
>>
>> Sending two concurrent patchsets under confusingly similar names again
>> and again isn't particularly helpful.
>>
>> Thanks!
> 
> Hi Rafael,
> 
> Thank you for looking into this.
> 
> Hi Lifeng,
> 
> As per the discussion, we can make the driver future extensible and
> also can optimize the register read/write access.
> 
> I gave some thought and below is my proposal.
> 
> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>    to read/write a cpc register.
> 
> 2) Pick my patches in [2]:
>    - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>                 Also, update existing API's to read/write regs in batch.
>    - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>      all CPU's in Auto mode and set registers with right values.
>      They can be updated after boot from sysfs to change hints to HW.
>      I can use the optimized API's from [1] where required in [2].
> 
> Let me know if you are okay with this proposal.
> I can also send an updated patch series with all the patches combined?
> 
> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
> 
> Regards,
> Sumit Gupta
> 

Hi Sumit,

Over the past few days, I've been thinking about your proposal and
scenario.

I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so
the rest of the discussion focuses on the differences between [2] and the
PATCH 8 in [1].

We both tried to support autonomous selection mode in cppc_cpufreq but on
different ways. I think the differences between these two approaches can be
summarized into three questions:

1. Which sysfs files to expose? I think this is not a problem, we can keep
all of them.

2. Where to expose these sysfs files? I understand your willing to keep all
cpc registers together under acpi_cppc sysfs. But in my opinion, it is more
suitable to expose them under cppc_cpufreq_attr, for these reasons:

  1) It may probably introduce concurrency and data consistency issues, as 
I mentioned before.

  2) The store functions call cpufreq_cpu_get() to get policy and update
the driver_data which is a cppc_cpudata. Only the driver_data in 
cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate
in cppc_acpi. This file currently provides interfaces for cpufreq drivers
to use. Reverse calls might mess up call relationships, break code
structures, and cause problems that are hard to pinpoint the root cause!

  3) Difficult to extend. Different cpufreq drivers may have different
processing logic when reading from and writing to these CPC registers.
Limiting all sysfs here makes it difficult for each cpufreq driver to
extend. I think this is why there are only read-only interfaces under
cppc_attrs before.

Adding a 'ifdef' is not a good way to solve these problems. Defining this
config does not necessarily mean that the cpufreq driver is cppc_cpufreq.

3. Is it necessary to add a new driver instance? [1] exposed the sysfs
files to support users dynamically change the auto selection mode of each
policy. Each policy can be operated seperately. It seems to me that if you
want to boot all CPUs in auto mode, it should be sufficient to set all
relevant registers to the correct values at boot time. I can't see why the
new instance is necessary unless you explain it further. Could you explain
more about why you add a new instance starting from answer these questions:

For a specific CPU, what is the difference between using the two instances
when auto_sel is 1? And what is the difference when auto_sel is 0?

If it turns out that the new instance is necessary, I think we can reach a
common approach by adding this new cpufreq driver instance and place the
attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did.

What do you think?

Regards,
Lifeng

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Sumit Gupta]

>>>
>>> There seems to be some quite fundamental disagreement on how this
>>> should be done, so I'm afraid I cannot do much about it ATM.
>>>
>>> Please agree on a common approach and come back to me when you are ready.
>>>
>>> Sending two concurrent patchsets under confusingly similar names again
>>> and again isn't particularly helpful.
>>>
>>> Thanks!
>>
>> Hi Rafael,
>>
>> Thank you for looking into this.
>>
>> Hi Lifeng,
>>
>> As per the discussion, we can make the driver future extensible and
>> also can optimize the register read/write access.
>>
>> I gave some thought and below is my proposal.
>>
>> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>>     to read/write a cpc register.
>>
>> 2) Pick my patches in [2]:
>>     - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>>                  Also, update existing API's to read/write regs in batch.
>>     - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>>       all CPU's in Auto mode and set registers with right values.
>>       They can be updated after boot from sysfs to change hints to HW.
>>       I can use the optimized API's from [1] where required in [2].
>>
>> Let me know if you are okay with this proposal.
>> I can also send an updated patch series with all the patches combined?
>>
>> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
>>
>> Regards,
>> Sumit Gupta
>>
> 
> Hi Sumit,
> 
> Over the past few days, I've been thinking about your proposal and
> scenario.
> 
> I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so
> the rest of the discussion focuses on the differences between [2] and the
> PATCH 8 in [1].
> 
> We both tried to support autonomous selection mode in cppc_cpufreq but on
> different ways. I think the differences between these two approaches can be
> summarized into three questions:
> 
> 1. Which sysfs files to expose? I think this is not a problem, we can keep
> all of them.
> 
> 2. Where to expose these sysfs files? I understand your willing to keep all
> cpc registers together under acpi_cppc sysfs. But in my opinion, it is more
> suitable to expose them under cppc_cpufreq_attr, for these reasons:
> 
>    1) It may probably introduce concurrency and data consistency issues, as
> I mentioned before.
> 

As explained in previous reply, this will be solved with the ifdef
check to enable the attributes for only those CPUFREQ drivers which want
to use the generic nodes.
  e.g: '#ifdef CONFIG_ACPI_CPPC_CPUFREQ' for 'cppc_cpufreq'.
			
These CPC register read/write sysfs nodes are generic as per the ACPI
specification and without any vendor specific logic.
So, adding them in the lib file 'cppc_acpi.c'(CONFIG_ACPI_CPPC_LIB) will
avoid code duplication if a different or new ACPI based CPUFREQ driver
also wants to use them just by adding their macro check. Such ifdef 
check is also used in other places for attributes creation like below.
So, don't look like a problem.
  $ grep -A4 "acpi_cpufreq_attr\[" drivers/cpufreq/acpi-cpufreq.c
  static struct freq_attr *acpi_cpufreq_attr[] = {
	&freqdomain_cpus,
  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
	&cpb,
  #endif

>    2) The store functions call cpufreq_cpu_get() to get policy and update
> the driver_data which is a cppc_cpudata. Only the driver_data in
> cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate
> in cppc_acpi. This file currently provides interfaces for cpufreq drivers
> to use. Reverse calls might mess up call relationships, break code
> structures, and cause problems that are hard to pinpoint the root cause!
> 

If we don't want to update the cpufreq policy from 'cppc_acpi.c' and 
only update it from within the cpufreq,	then this could be one valid
point to not add the write syfs nodes in 'cppc_acpi.c' lib file.

@Rafael, @Viresh : Do you have any comments on this?

>    3) Difficult to extend. Different cpufreq drivers may have different
> processing logic when reading from and writing to these CPC registers.
> Limiting all sysfs here makes it difficult for each cpufreq driver to
> extend. I think this is why there are only read-only interfaces under
> cppc_attrs before.
> 

We are updating the CPC registers as per the generic ACPI specification.
So, any ACPI based CPUFREQ driver can use these generic nodes to
read/write reg's until they have a vendor specific requirement or
implementation.
As explained above, If someone wants to update in different way and use
their own CPUFREQ driver then these generic attributes won't be created
due to the CPUFREQ driver macro check.
I think AMD and Intel are doing more than just reading/updating the 
registers. That's why they needed their driver specific implementations.

> Adding a 'ifdef' is not a good way to solve these problems. Defining this
> config does not necessarily mean that the cpufreq driver is cppc_cpufreq.
> 

It means that only.
  ./drivers/cpufreq/Makefile:obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += 
cppc_cpufreq.o

> 3. Is it necessary to add a new driver instance? [1] exposed the sysfs
> files to support users dynamically change the auto selection mode of each
> policy. Each policy can be operated seperately. It seems to me that if you
> want to boot all CPUs in auto mode, it should be sufficient to set all
> relevant registers to the correct values at boot time. I can't see why the
> new instance is necessary unless you explain it further. Could you explain
> more about why you add a new instance starting from answer these questions:
> 
> For a specific CPU, what is the difference between using the two instances
> when auto_sel is 1? And what is the difference when auto_sel is 0?
> 

Explained this in previous reply. Let me elaborate more.

For hundred's of CPU's, we don't need to explicitly set multiple sysfs
after boot to enable and configure Auto mode with right params. That's 
why an easy option is to pass boot argument or module param for enabling
and configuration.
A separate instance 'cppc_cpufreq_epp' of the 'cppc_cpufreq' driver is
added because policy min/max need to be updated to the min/max_perf
and not nominal/lowest nonlinear perf which is done by the default
init hook. Min_perf value can be lower than lowest nonlinear perf and 
Max_perf can be higher than nominal perf.
If some CPU is booted with epp instance and later the auto mode is 
disabled or min/max_perf is changed from sysfs then also the policy
min/max need to be updated accordingly.

Another is that in Autonomous mode the freq selection and setting is
done by HW. So, cpufreq_driver->target() hook is not needed.
These are few reasons which I am aware of as of now.
I think in future there can be more. Having a separate instance
reflecting a HW based Autonomous frequency selection will make it easy
for any future changes.

> If it turns out that the new instance is necessary, I think we can reach a
> common approach by adding this new cpufreq driver instance and place the
> attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did.
> 
> What do you think?

I initially thought about this but there was a problem.
What if we boot with non-epp instance which doesn't have these 
attributes and later want to enable Auto mode for few CPU's from sysfs.


Best Regards,
Sumit Gupta

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[zhenglifeng (A)]

Sorry for the delay.

On 2025/3/14 20:48, Sumit Gupta wrote:
> 
> 
>>>>
>>>> There seems to be some quite fundamental disagreement on how this
>>>> should be done, so I'm afraid I cannot do much about it ATM.
>>>>
>>>> Please agree on a common approach and come back to me when you are ready.
>>>>
>>>> Sending two concurrent patchsets under confusingly similar names again
>>>> and again isn't particularly helpful.
>>>>
>>>> Thanks!
>>>
>>> Hi Rafael,
>>>
>>> Thank you for looking into this.
>>>
>>> Hi Lifeng,
>>>
>>> As per the discussion, we can make the driver future extensible and
>>> also can optimize the register read/write access.
>>>
>>> I gave some thought and below is my proposal.
>>>
>>> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>>>     to read/write a cpc register.
>>>
>>> 2) Pick my patches in [2]:
>>>     - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>>>                  Also, update existing API's to read/write regs in batch.
>>>     - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>>>       all CPU's in Auto mode and set registers with right values.
>>>       They can be updated after boot from sysfs to change hints to HW.
>>>       I can use the optimized API's from [1] where required in [2].
>>>
>>> Let me know if you are okay with this proposal.
>>> I can also send an updated patch series with all the patches combined?
>>>
>>> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
>>>
>>> Regards,
>>> Sumit Gupta
>>>
>>
>> Hi Sumit,
>>
>> Over the past few days, I've been thinking about your proposal and
>> scenario.
>>
>> I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so
>> the rest of the discussion focuses on the differences between [2] and the
>> PATCH 8 in [1].
>>
>> We both tried to support autonomous selection mode in cppc_cpufreq but on
>> different ways. I think the differences between these two approaches can be
>> summarized into three questions:
>>
>> 1. Which sysfs files to expose? I think this is not a problem, we can keep
>> all of them.
>>
>> 2. Where to expose these sysfs files? I understand your willing to keep all
>> cpc registers together under acpi_cppc sysfs. But in my opinion, it is more
>> suitable to expose them under cppc_cpufreq_attr, for these reasons:
>>
>>    1) It may probably introduce concurrency and data consistency issues, as
>> I mentioned before.
>>
> 
> As explained in previous reply, this will be solved with the ifdef
> check to enable the attributes for only those CPUFREQ drivers which want
> to use the generic nodes.
>  e.g: '#ifdef CONFIG_ACPI_CPPC_CPUFREQ' for 'cppc_cpufreq'.
>            
> These CPC register read/write sysfs nodes are generic as per the ACPI
> specification and without any vendor specific logic.
> So, adding them in the lib file 'cppc_acpi.c'(CONFIG_ACPI_CPPC_LIB) will
> avoid code duplication if a different or new ACPI based CPUFREQ driver
> also wants to use them just by adding their macro check. Such ifdef check is also used in other places for attributes creation like below.
> So, don't look like a problem.
>  $ grep -A4 "acpi_cpufreq_attr\[" drivers/cpufreq/acpi-cpufreq.c
>  static struct freq_attr *acpi_cpufreq_attr[] = {
>     &freqdomain_cpus,
>  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>     &cpb,
>  #endif

So in the future, we will see:

static struct attribute *cppc_attrs[] = {
	...
#ifdef CONFIG_XXX
	&xxx.attr,
	&xxx.attr,
#endif
#ifdef CONFIG_XXX
	&xxx.attr,
#endif
#ifdef CONFIG_XXX
	&xxx.attr,
	...
};

I think you are making things more complicated.

> 
>>    2) The store functions call cpufreq_cpu_get() to get policy and update
>> the driver_data which is a cppc_cpudata. Only the driver_data in
>> cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate
>> in cppc_acpi. This file currently provides interfaces for cpufreq drivers
>> to use. Reverse calls might mess up call relationships, break code
>> structures, and cause problems that are hard to pinpoint the root cause!
>>
> 
> If we don't want to update the cpufreq policy from 'cppc_acpi.c' and only update it from within the cpufreq,    then this could be one valid
> point to not add the write syfs nodes in 'cppc_acpi.c' lib file.
> 
> @Rafael, @Viresh : Do you have any comments on this?

I think updating cpufreq policy from 'cppc_acpi.c' should be forbidden.

> 
>>    3) Difficult to extend. Different cpufreq drivers may have different
>> processing logic when reading from and writing to these CPC registers.
>> Limiting all sysfs here makes it difficult for each cpufreq driver to
>> extend. I think this is why there are only read-only interfaces under
>> cppc_attrs before.
>>
> 
> We are updating the CPC registers as per the generic ACPI specification.
> So, any ACPI based CPUFREQ driver can use these generic nodes to
> read/write reg's until they have a vendor specific requirement or
> implementation.
> As explained above, If someone wants to update in different way and use
> their own CPUFREQ driver then these generic attributes won't be created
> due to the CPUFREQ driver macro check.
> I think AMD and Intel are doing more than just reading/updating the registers. That's why they needed their driver specific implementations.
> 
>> Adding a 'ifdef' is not a good way to solve these problems. Defining this
>> config does not necessarily mean that the cpufreq driver is cppc_cpufreq.
>>
> 
> It means that only.
>  ./drivers/cpufreq/Makefile:obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o

Compile this file does not mean that the cpufreq driver is cppc_cpufreq.
Driver registration may fail, and the actually loaded driver may be
another. It'll be dangerous to expose these sysfs files for users to update
registers' value in this case.

> 
>> 3. Is it necessary to add a new driver instance? [1] exposed the sysfs
>> files to support users dynamically change the auto selection mode of each
>> policy. Each policy can be operated seperately. It seems to me that if you
>> want to boot all CPUs in auto mode, it should be sufficient to set all
>> relevant registers to the correct values at boot time. I can't see why the
>> new instance is necessary unless you explain it further. Could you explain
>> more about why you add a new instance starting from answer these questions:
>>
>> For a specific CPU, what is the difference between using the two instances
>> when auto_sel is 1? And what is the difference when auto_sel is 0?
>>
> 
> Explained this in previous reply. Let me elaborate more.
> 
> For hundred's of CPU's, we don't need to explicitly set multiple sysfs
> after boot to enable and configure Auto mode with right params. That's why an easy option is to pass boot argument or module param for enabling
> and configuration.
> A separate instance 'cppc_cpufreq_epp' of the 'cppc_cpufreq' driver is
> added because policy min/max need to be updated to the min/max_perf
> and not nominal/lowest nonlinear perf which is done by the default
> init hook. Min_perf value can be lower than lowest nonlinear perf and Max_perf can be higher than nominal perf.
> If some CPU is booted with epp instance and later the auto mode is disabled or min/max_perf is changed from sysfs then also the policy
> min/max need to be updated accordingly.
> 
> Another is that in Autonomous mode the freq selection and setting is
> done by HW. So, cpufreq_driver->target() hook is not needed.
> These are few reasons which I am aware of as of now.
> I think in future there can be more. Having a separate instance
> reflecting a HW based Autonomous frequency selection will make it easy
> for any future changes.

So CPUs will act totally differently under these two instance. But what if
I want part of the CPUs in HW mode and others in SW mode? Should I boot on
HW mode and set some policies' auto_set to false or the other way? It seems
like the effects of theses two approaches are completely different. In my
opinion, this new instance is more like a completely different driver than
cppc_cpufreq.

> 
>> If it turns out that the new instance is necessary, I think we can reach a
>> common approach by adding this new cpufreq driver instance and place the
>> attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did.
>>
>> What do you think?
> 
> I initially thought about this but there was a problem.
> What if we boot with non-epp instance which doesn't have these attributes and later want to enable Auto mode for few CPU's from sysfs.

That's the problem. CPUs can be set to Auto mode with or without this new
instance. So what's the point of it?

> 
> 
> Best Regards,
> Sumit Gupta

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[zhenglifeng (A)]

Hi Sumit,

May I resend the patch 8 in [1] first? Because I really need this new
feature.

After that patch being merged, you can resend this series base on that,
change the paths of the sysfs files, add a new cppc_cpufreq instance or do
anything in that series. Then we can continue this discussion.

Is that all right?

On 2025/4/1 21:56, zhenglifeng (A) wrote:

> Sorry for the delay.
> 
> On 2025/3/14 20:48, Sumit Gupta wrote:
>>
>>
>>>>>
>>>>> There seems to be some quite fundamental disagreement on how this
>>>>> should be done, so I'm afraid I cannot do much about it ATM.
>>>>>
>>>>> Please agree on a common approach and come back to me when you are ready.
>>>>>
>>>>> Sending two concurrent patchsets under confusingly similar names again
>>>>> and again isn't particularly helpful.
>>>>>
>>>>> Thanks!
>>>>
>>>> Hi Rafael,
>>>>
>>>> Thank you for looking into this.
>>>>
>>>> Hi Lifeng,
>>>>
>>>> As per the discussion, we can make the driver future extensible and
>>>> also can optimize the register read/write access.
>>>>
>>>> I gave some thought and below is my proposal.
>>>>
>>>> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>>>>     to read/write a cpc register.
>>>>
>>>> 2) Pick my patches in [2]:
>>>>     - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>>>>                  Also, update existing API's to read/write regs in batch.
>>>>     - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>>>>       all CPU's in Auto mode and set registers with right values.
>>>>       They can be updated after boot from sysfs to change hints to HW.
>>>>       I can use the optimized API's from [1] where required in [2].
>>>>
>>>> Let me know if you are okay with this proposal.
>>>> I can also send an updated patch series with all the patches combined?
>>>>
>>>> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>>> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
>>>>
>>>> Regards,
>>>> Sumit Gupta
>>>>
>>>
>>> Hi Sumit,
>>>
>>> Over the past few days, I've been thinking about your proposal and
>>> scenario.
>>>
>>> I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so
>>> the rest of the discussion focuses on the differences between [2] and the
>>> PATCH 8 in [1].
>>>
>>> We both tried to support autonomous selection mode in cppc_cpufreq but on
>>> different ways. I think the differences between these two approaches can be
>>> summarized into three questions:
>>>
>>> 1. Which sysfs files to expose? I think this is not a problem, we can keep
>>> all of them.
>>>
>>> 2. Where to expose these sysfs files? I understand your willing to keep all
>>> cpc registers together under acpi_cppc sysfs. But in my opinion, it is more
>>> suitable to expose them under cppc_cpufreq_attr, for these reasons:
>>>
>>>    1) It may probably introduce concurrency and data consistency issues, as
>>> I mentioned before.
>>>
>>
>> As explained in previous reply, this will be solved with the ifdef
>> check to enable the attributes for only those CPUFREQ drivers which want
>> to use the generic nodes.
>>  e.g: '#ifdef CONFIG_ACPI_CPPC_CPUFREQ' for 'cppc_cpufreq'.
>>            
>> These CPC register read/write sysfs nodes are generic as per the ACPI
>> specification and without any vendor specific logic.
>> So, adding them in the lib file 'cppc_acpi.c'(CONFIG_ACPI_CPPC_LIB) will
>> avoid code duplication if a different or new ACPI based CPUFREQ driver
>> also wants to use them just by adding their macro check. Such ifdef check is also used in other places for attributes creation like below.
>> So, don't look like a problem.
>>  $ grep -A4 "acpi_cpufreq_attr\[" drivers/cpufreq/acpi-cpufreq.c
>>  static struct freq_attr *acpi_cpufreq_attr[] = {
>>     &freqdomain_cpus,
>>  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>>     &cpb,
>>  #endif
> 
> So in the future, we will see:
> 
> static struct attribute *cppc_attrs[] = {
> 	...
> #ifdef CONFIG_XXX
> 	&xxx.attr,
> 	&xxx.attr,
> #endif
> #ifdef CONFIG_XXX
> 	&xxx.attr,
> #endif
> #ifdef CONFIG_XXX
> 	&xxx.attr,
> 	...
> };
> 
> I think you are making things more complicated.
> 
>>
>>>    2) The store functions call cpufreq_cpu_get() to get policy and update
>>> the driver_data which is a cppc_cpudata. Only the driver_data in
>>> cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate
>>> in cppc_acpi. This file currently provides interfaces for cpufreq drivers
>>> to use. Reverse calls might mess up call relationships, break code
>>> structures, and cause problems that are hard to pinpoint the root cause!
>>>
>>
>> If we don't want to update the cpufreq policy from 'cppc_acpi.c' and only update it from within the cpufreq,    then this could be one valid
>> point to not add the write syfs nodes in 'cppc_acpi.c' lib file.
>>
>> @Rafael, @Viresh : Do you have any comments on this?
> 
> I think updating cpufreq policy from 'cppc_acpi.c' should be forbidden.
> 
>>
>>>    3) Difficult to extend. Different cpufreq drivers may have different
>>> processing logic when reading from and writing to these CPC registers.
>>> Limiting all sysfs here makes it difficult for each cpufreq driver to
>>> extend. I think this is why there are only read-only interfaces under
>>> cppc_attrs before.
>>>
>>
>> We are updating the CPC registers as per the generic ACPI specification.
>> So, any ACPI based CPUFREQ driver can use these generic nodes to
>> read/write reg's until they have a vendor specific requirement or
>> implementation.
>> As explained above, If someone wants to update in different way and use
>> their own CPUFREQ driver then these generic attributes won't be created
>> due to the CPUFREQ driver macro check.
>> I think AMD and Intel are doing more than just reading/updating the registers. That's why they needed their driver specific implementations.
>>
>>> Adding a 'ifdef' is not a good way to solve these problems. Defining this
>>> config does not necessarily mean that the cpufreq driver is cppc_cpufreq.
>>>
>>
>> It means that only.
>>  ./drivers/cpufreq/Makefile:obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
> 
> Compile this file does not mean that the cpufreq driver is cppc_cpufreq.
> Driver registration may fail, and the actually loaded driver may be
> another. It'll be dangerous to expose these sysfs files for users to update
> registers' value in this case.
> 
>>
>>> 3. Is it necessary to add a new driver instance? [1] exposed the sysfs
>>> files to support users dynamically change the auto selection mode of each
>>> policy. Each policy can be operated seperately. It seems to me that if you
>>> want to boot all CPUs in auto mode, it should be sufficient to set all
>>> relevant registers to the correct values at boot time. I can't see why the
>>> new instance is necessary unless you explain it further. Could you explain
>>> more about why you add a new instance starting from answer these questions:
>>>
>>> For a specific CPU, what is the difference between using the two instances
>>> when auto_sel is 1? And what is the difference when auto_sel is 0?
>>>
>>
>> Explained this in previous reply. Let me elaborate more.
>>
>> For hundred's of CPU's, we don't need to explicitly set multiple sysfs
>> after boot to enable and configure Auto mode with right params. That's why an easy option is to pass boot argument or module param for enabling
>> and configuration.
>> A separate instance 'cppc_cpufreq_epp' of the 'cppc_cpufreq' driver is
>> added because policy min/max need to be updated to the min/max_perf
>> and not nominal/lowest nonlinear perf which is done by the default
>> init hook. Min_perf value can be lower than lowest nonlinear perf and Max_perf can be higher than nominal perf.
>> If some CPU is booted with epp instance and later the auto mode is disabled or min/max_perf is changed from sysfs then also the policy
>> min/max need to be updated accordingly.
>>
>> Another is that in Autonomous mode the freq selection and setting is
>> done by HW. So, cpufreq_driver->target() hook is not needed.
>> These are few reasons which I am aware of as of now.
>> I think in future there can be more. Having a separate instance
>> reflecting a HW based Autonomous frequency selection will make it easy
>> for any future changes.
> 
> So CPUs will act totally differently under these two instance. But what if
> I want part of the CPUs in HW mode and others in SW mode? Should I boot on
> HW mode and set some policies' auto_set to false or the other way? It seems
> like the effects of theses two approaches are completely different. In my
> opinion, this new instance is more like a completely different driver than
> cppc_cpufreq.
> 
>>
>>> If it turns out that the new instance is necessary, I think we can reach a
>>> common approach by adding this new cpufreq driver instance and place the
>>> attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did.
>>>
>>> What do you think?
>>
>> I initially thought about this but there was a problem.
>> What if we boot with non-epp instance which doesn't have these attributes and later want to enable Auto mode for few CPU's from sysfs.
> 
> That's the problem. CPUs can be set to Auto mode with or without this new
> instance. So what's the point of it?
> 
>>
>>
>> Best Regards,
>> Sumit Gupta
> 
> 
> 

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[zhenglifeng (A)]

On 2025/4/19 15:44, zhenglifeng (A) wrote:

> Hi Sumit,
> 
> May I resend the patch 8 in [1] first? Because I really need this new
> feature.
> 
> After that patch being merged, you can resend this series base on that,
> change the paths of the sysfs files, add a new cppc_cpufreq instance or do
> anything in that series. Then we can continue this discussion.
> 
> Is that all right?

Hi Sumit,

Please let me know if you are OK with it.

> 
> On 2025/4/1 21:56, zhenglifeng (A) wrote:
> 
>> Sorry for the delay.
>>
>> On 2025/3/14 20:48, Sumit Gupta wrote:
>>>
>>>
>>>>>>
>>>>>> There seems to be some quite fundamental disagreement on how this
>>>>>> should be done, so I'm afraid I cannot do much about it ATM.
>>>>>>
>>>>>> Please agree on a common approach and come back to me when you are ready.
>>>>>>
>>>>>> Sending two concurrent patchsets under confusingly similar names again
>>>>>> and again isn't particularly helpful.
>>>>>>
>>>>>> Thanks!
>>>>>
>>>>> Hi Rafael,
>>>>>
>>>>> Thank you for looking into this.
>>>>>
>>>>> Hi Lifeng,
>>>>>
>>>>> As per the discussion, we can make the driver future extensible and
>>>>> also can optimize the register read/write access.
>>>>>
>>>>> I gave some thought and below is my proposal.
>>>>>
>>>>> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>>>>>     to read/write a cpc register.
>>>>>
>>>>> 2) Pick my patches in [2]:
>>>>>     - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>>>>>                  Also, update existing API's to read/write regs in batch.
>>>>>     - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>>>>>       all CPU's in Auto mode and set registers with right values.
>>>>>       They can be updated after boot from sysfs to change hints to HW.
>>>>>       I can use the optimized API's from [1] where required in [2].
>>>>>
>>>>> Let me know if you are okay with this proposal.
>>>>> I can also send an updated patch series with all the patches combined?
>>>>>
>>>>> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>>>> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
>>>>>
>>>>> Regards,
>>>>> Sumit Gupta
>>>>>
>>>>
>>>> Hi Sumit,
>>>>
>>>> Over the past few days, I've been thinking about your proposal and
>>>> scenario.
>>>>
>>>> I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so
>>>> the rest of the discussion focuses on the differences between [2] and the
>>>> PATCH 8 in [1].
>>>>
>>>> We both tried to support autonomous selection mode in cppc_cpufreq but on
>>>> different ways. I think the differences between these two approaches can be
>>>> summarized into three questions:
>>>>
>>>> 1. Which sysfs files to expose? I think this is not a problem, we can keep
>>>> all of them.
>>>>
>>>> 2. Where to expose these sysfs files? I understand your willing to keep all
>>>> cpc registers together under acpi_cppc sysfs. But in my opinion, it is more
>>>> suitable to expose them under cppc_cpufreq_attr, for these reasons:
>>>>
>>>>    1) It may probably introduce concurrency and data consistency issues, as
>>>> I mentioned before.
>>>>
>>>
>>> As explained in previous reply, this will be solved with the ifdef
>>> check to enable the attributes for only those CPUFREQ drivers which want
>>> to use the generic nodes.
>>>  e.g: '#ifdef CONFIG_ACPI_CPPC_CPUFREQ' for 'cppc_cpufreq'.
>>>            
>>> These CPC register read/write sysfs nodes are generic as per the ACPI
>>> specification and without any vendor specific logic.
>>> So, adding them in the lib file 'cppc_acpi.c'(CONFIG_ACPI_CPPC_LIB) will
>>> avoid code duplication if a different or new ACPI based CPUFREQ driver
>>> also wants to use them just by adding their macro check. Such ifdef check is also used in other places for attributes creation like below.
>>> So, don't look like a problem.
>>>  $ grep -A4 "acpi_cpufreq_attr\[" drivers/cpufreq/acpi-cpufreq.c
>>>  static struct freq_attr *acpi_cpufreq_attr[] = {
>>>     &freqdomain_cpus,
>>>  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>>>     &cpb,
>>>  #endif
>>
>> So in the future, we will see:
>>
>> static struct attribute *cppc_attrs[] = {
>> 	...
>> #ifdef CONFIG_XXX
>> 	&xxx.attr,
>> 	&xxx.attr,
>> #endif
>> #ifdef CONFIG_XXX
>> 	&xxx.attr,
>> #endif
>> #ifdef CONFIG_XXX
>> 	&xxx.attr,
>> 	...
>> };
>>
>> I think you are making things more complicated.
>>
>>>
>>>>    2) The store functions call cpufreq_cpu_get() to get policy and update
>>>> the driver_data which is a cppc_cpudata. Only the driver_data in
>>>> cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate
>>>> in cppc_acpi. This file currently provides interfaces for cpufreq drivers
>>>> to use. Reverse calls might mess up call relationships, break code
>>>> structures, and cause problems that are hard to pinpoint the root cause!
>>>>
>>>
>>> If we don't want to update the cpufreq policy from 'cppc_acpi.c' and only update it from within the cpufreq,    then this could be one valid
>>> point to not add the write syfs nodes in 'cppc_acpi.c' lib file.
>>>
>>> @Rafael, @Viresh : Do you have any comments on this?
>>
>> I think updating cpufreq policy from 'cppc_acpi.c' should be forbidden.
>>
>>>
>>>>    3) Difficult to extend. Different cpufreq drivers may have different
>>>> processing logic when reading from and writing to these CPC registers.
>>>> Limiting all sysfs here makes it difficult for each cpufreq driver to
>>>> extend. I think this is why there are only read-only interfaces under
>>>> cppc_attrs before.
>>>>
>>>
>>> We are updating the CPC registers as per the generic ACPI specification.
>>> So, any ACPI based CPUFREQ driver can use these generic nodes to
>>> read/write reg's until they have a vendor specific requirement or
>>> implementation.
>>> As explained above, If someone wants to update in different way and use
>>> their own CPUFREQ driver then these generic attributes won't be created
>>> due to the CPUFREQ driver macro check.
>>> I think AMD and Intel are doing more than just reading/updating the registers. That's why they needed their driver specific implementations.
>>>
>>>> Adding a 'ifdef' is not a good way to solve these problems. Defining this
>>>> config does not necessarily mean that the cpufreq driver is cppc_cpufreq.
>>>>
>>>
>>> It means that only.
>>>  ./drivers/cpufreq/Makefile:obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
>>
>> Compile this file does not mean that the cpufreq driver is cppc_cpufreq.
>> Driver registration may fail, and the actually loaded driver may be
>> another. It'll be dangerous to expose these sysfs files for users to update
>> registers' value in this case.
>>
>>>
>>>> 3. Is it necessary to add a new driver instance? [1] exposed the sysfs
>>>> files to support users dynamically change the auto selection mode of each
>>>> policy. Each policy can be operated seperately. It seems to me that if you
>>>> want to boot all CPUs in auto mode, it should be sufficient to set all
>>>> relevant registers to the correct values at boot time. I can't see why the
>>>> new instance is necessary unless you explain it further. Could you explain
>>>> more about why you add a new instance starting from answer these questions:
>>>>
>>>> For a specific CPU, what is the difference between using the two instances
>>>> when auto_sel is 1? And what is the difference when auto_sel is 0?
>>>>
>>>
>>> Explained this in previous reply. Let me elaborate more.
>>>
>>> For hundred's of CPU's, we don't need to explicitly set multiple sysfs
>>> after boot to enable and configure Auto mode with right params. That's why an easy option is to pass boot argument or module param for enabling
>>> and configuration.
>>> A separate instance 'cppc_cpufreq_epp' of the 'cppc_cpufreq' driver is
>>> added because policy min/max need to be updated to the min/max_perf
>>> and not nominal/lowest nonlinear perf which is done by the default
>>> init hook. Min_perf value can be lower than lowest nonlinear perf and Max_perf can be higher than nominal perf.
>>> If some CPU is booted with epp instance and later the auto mode is disabled or min/max_perf is changed from sysfs then also the policy
>>> min/max need to be updated accordingly.
>>>
>>> Another is that in Autonomous mode the freq selection and setting is
>>> done by HW. So, cpufreq_driver->target() hook is not needed.
>>> These are few reasons which I am aware of as of now.
>>> I think in future there can be more. Having a separate instance
>>> reflecting a HW based Autonomous frequency selection will make it easy
>>> for any future changes.
>>
>> So CPUs will act totally differently under these two instance. But what if
>> I want part of the CPUs in HW mode and others in SW mode? Should I boot on
>> HW mode and set some policies' auto_set to false or the other way? It seems
>> like the effects of theses two approaches are completely different. In my
>> opinion, this new instance is more like a completely different driver than
>> cppc_cpufreq.
>>
>>>
>>>> If it turns out that the new instance is necessary, I think we can reach a
>>>> common approach by adding this new cpufreq driver instance and place the
>>>> attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did.
>>>>
>>>> What do you think?
>>>
>>> I initially thought about this but there was a problem.
>>> What if we boot with non-epp instance which doesn't have these attributes and later want to enable Auto mode for few CPU's from sysfs.
>>
>> That's the problem. CPUs can be set to Auto mode with or without this new
>> instance. So what's the point of it?
>>
>>>
>>>
>>> Best Regards,
>>> Sumit Gupta
>>
>>
>>
> 
> 

Re: [Patch 0/5] Support Autonomous Selection mode in cppc_cpufreq

[Sumit Gupta]

> 
>> Hi Sumit,
>>
>> May I resend the patch 8 in [1] first? Because I really need this new
>> feature.
>>
>> After that patch being merged, you can resend this series base on that,
>> change the paths of the sysfs files, add a new cppc_cpufreq instance or do
>> anything in that series. Then we can continue this discussion.
>>
>> Is that all right?
> 
> Hi Sumit,
> 
> Please let me know if you are OK with it.
> 

Hi Zhenglifeng,

Both the ways do the same job i.e. set CPC registers.

To move this work forward, I am Ok with adding sysfs entries under
cpufreq syfs node and not under acpi_cppc if the maintainers are fine.

I will later send my updated patch to add more entries under cpufreq
sysfs for updating more CPC registers as done in patch 8 in [1].

Thank you,
Sumit Gupta

>>
>> On 2025/4/1 21:56, zhenglifeng (A) wrote:
>>
>>> Sorry for the delay.
>>>
>>> On 2025/3/14 20:48, Sumit Gupta wrote:
>>>>
>>>>
>>>>>>>
>>>>>>> There seems to be some quite fundamental disagreement on how this
>>>>>>> should be done, so I'm afraid I cannot do much about it ATM.
>>>>>>>
>>>>>>> Please agree on a common approach and come back to me when you are ready.
>>>>>>>
>>>>>>> Sending two concurrent patchsets under confusingly similar names again
>>>>>>> and again isn't particularly helpful.
>>>>>>>
>>>>>>> Thanks!
>>>>>>
>>>>>> Hi Rafael,
>>>>>>
>>>>>> Thank you for looking into this.
>>>>>>
>>>>>> Hi Lifeng,
>>>>>>
>>>>>> As per the discussion, we can make the driver future extensible and
>>>>>> also can optimize the register read/write access.
>>>>>>
>>>>>> I gave some thought and below is my proposal.
>>>>>>
>>>>>> 1) Pick 'Patch 1-7' from your patch series [1] which optimize API's
>>>>>>      to read/write a cpc register.
>>>>>>
>>>>>> 2) Pick my patches in [2]:
>>>>>>      - Patch 1-4: Keep all cpc registers together under acpi_cppc sysfs.
>>>>>>                   Also, update existing API's to read/write regs in batch.
>>>>>>      - Patch 5: Creates 'cppc_cpufreq_epp_driver' instance for booting
>>>>>>        all CPU's in Auto mode and set registers with right values.
>>>>>>        They can be updated after boot from sysfs to change hints to HW.
>>>>>>        I can use the optimized API's from [1] where required in [2].
>>>>>>
>>>>>> Let me know if you are okay with this proposal.
>>>>>> I can also send an updated patch series with all the patches combined?
>>>>>>
>>>>>> [1] https://lore.kernel.org/all/20250206131428.3261578-1-zhenglifeng1@huawei.com/
>>>>>> [2] https://lore.kernel.org/lkml/20250211103737.447704-1-sumitg@nvidia.com/
>>>>>>
>>>>>> Regards,
>>>>>> Sumit Gupta
>>>>>>
>>>>>
>>>>> Hi Sumit,
>>>>>
>>>>> Over the past few days, I've been thinking about your proposal and
>>>>> scenario.
>>>>>
>>>>> I think we both agree that PATCH 1-7 in [1] doesn't conflicts with [2], so
>>>>> the rest of the discussion focuses on the differences between [2] and the
>>>>> PATCH 8 in [1].
>>>>>
>>>>> We both tried to support autonomous selection mode in cppc_cpufreq but on
>>>>> different ways. I think the differences between these two approaches can be
>>>>> summarized into three questions:
>>>>>
>>>>> 1. Which sysfs files to expose? I think this is not a problem, we can keep
>>>>> all of them.
>>>>>
>>>>> 2. Where to expose these sysfs files? I understand your willing to keep all
>>>>> cpc registers together under acpi_cppc sysfs. But in my opinion, it is more
>>>>> suitable to expose them under cppc_cpufreq_attr, for these reasons:
>>>>>
>>>>>     1) It may probably introduce concurrency and data consistency issues, as
>>>>> I mentioned before.
>>>>>
>>>>
>>>> As explained in previous reply, this will be solved with the ifdef
>>>> check to enable the attributes for only those CPUFREQ drivers which want
>>>> to use the generic nodes.
>>>>   e.g: '#ifdef CONFIG_ACPI_CPPC_CPUFREQ' for 'cppc_cpufreq'.
>>>>
>>>> These CPC register read/write sysfs nodes are generic as per the ACPI
>>>> specification and without any vendor specific logic.
>>>> So, adding them in the lib file 'cppc_acpi.c'(CONFIG_ACPI_CPPC_LIB) will
>>>> avoid code duplication if a different or new ACPI based CPUFREQ driver
>>>> also wants to use them just by adding their macro check. Such ifdef check is also used in other places for attributes creation like below.
>>>> So, don't look like a problem.
>>>>   $ grep -A4 "acpi_cpufreq_attr\[" drivers/cpufreq/acpi-cpufreq.c
>>>>   static struct freq_attr *acpi_cpufreq_attr[] = {
>>>>      &freqdomain_cpus,
>>>>   #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>>>>      &cpb,
>>>>   #endif
>>>
>>> So in the future, we will see:
>>>
>>> static struct attribute *cppc_attrs[] = {
>>>       ...
>>> #ifdef CONFIG_XXX
>>>       &xxx.attr,
>>>       &xxx.attr,
>>> #endif
>>> #ifdef CONFIG_XXX
>>>       &xxx.attr,
>>> #endif
>>> #ifdef CONFIG_XXX
>>>       &xxx.attr,
>>>       ...
>>> };
>>>
>>> I think you are making things more complicated.
>>>
>>>>
>>>>>     2) The store functions call cpufreq_cpu_get() to get policy and update
>>>>> the driver_data which is a cppc_cpudata. Only the driver_data in
>>>>> cppc_cpufreq's policy is a cppc_cpudata! These operations are inappropriate
>>>>> in cppc_acpi. This file currently provides interfaces for cpufreq drivers
>>>>> to use. Reverse calls might mess up call relationships, break code
>>>>> structures, and cause problems that are hard to pinpoint the root cause!
>>>>>
>>>>
>>>> If we don't want to update the cpufreq policy from 'cppc_acpi.c' and only update it from within the cpufreq,    then this could be one valid
>>>> point to not add the write syfs nodes in 'cppc_acpi.c' lib file.
>>>>
>>>> @Rafael, @Viresh : Do you have any comments on this?
>>>
>>> I think updating cpufreq policy from 'cppc_acpi.c' should be forbidden.
>>>
>>>>
>>>>>     3) Difficult to extend. Different cpufreq drivers may have different
>>>>> processing logic when reading from and writing to these CPC registers.
>>>>> Limiting all sysfs here makes it difficult for each cpufreq driver to
>>>>> extend. I think this is why there are only read-only interfaces under
>>>>> cppc_attrs before.
>>>>>
>>>>
>>>> We are updating the CPC registers as per the generic ACPI specification.
>>>> So, any ACPI based CPUFREQ driver can use these generic nodes to
>>>> read/write reg's until they have a vendor specific requirement or
>>>> implementation.
>>>> As explained above, If someone wants to update in different way and use
>>>> their own CPUFREQ driver then these generic attributes won't be created
>>>> due to the CPUFREQ driver macro check.
>>>> I think AMD and Intel are doing more than just reading/updating the registers. That's why they needed their driver specific implementations.
>>>>
>>>>> Adding a 'ifdef' is not a good way to solve these problems. Defining this
>>>>> config does not necessarily mean that the cpufreq driver is cppc_cpufreq.
>>>>>
>>>>
>>>> It means that only.
>>>>   ./drivers/cpufreq/Makefile:obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
>>>
>>> Compile this file does not mean that the cpufreq driver is cppc_cpufreq.
>>> Driver registration may fail, and the actually loaded driver may be
>>> another. It'll be dangerous to expose these sysfs files for users to update
>>> registers' value in this case.
>>>
>>>>
>>>>> 3. Is it necessary to add a new driver instance? [1] exposed the sysfs
>>>>> files to support users dynamically change the auto selection mode of each
>>>>> policy. Each policy can be operated seperately. It seems to me that if you
>>>>> want to boot all CPUs in auto mode, it should be sufficient to set all
>>>>> relevant registers to the correct values at boot time. I can't see why the
>>>>> new instance is necessary unless you explain it further. Could you explain
>>>>> more about why you add a new instance starting from answer these questions:
>>>>>
>>>>> For a specific CPU, what is the difference between using the two instances
>>>>> when auto_sel is 1? And what is the difference when auto_sel is 0?
>>>>>
>>>>
>>>> Explained this in previous reply. Let me elaborate more.
>>>>
>>>> For hundred's of CPU's, we don't need to explicitly set multiple sysfs
>>>> after boot to enable and configure Auto mode with right params. That's why an easy option is to pass boot argument or module param for enabling
>>>> and configuration.
>>>> A separate instance 'cppc_cpufreq_epp' of the 'cppc_cpufreq' driver is
>>>> added because policy min/max need to be updated to the min/max_perf
>>>> and not nominal/lowest nonlinear perf which is done by the default
>>>> init hook. Min_perf value can be lower than lowest nonlinear perf and Max_perf can be higher than nominal perf.
>>>> If some CPU is booted with epp instance and later the auto mode is disabled or min/max_perf is changed from sysfs then also the policy
>>>> min/max need to be updated accordingly.
>>>>
>>>> Another is that in Autonomous mode the freq selection and setting is
>>>> done by HW. So, cpufreq_driver->target() hook is not needed.
>>>> These are few reasons which I am aware of as of now.
>>>> I think in future there can be more. Having a separate instance
>>>> reflecting a HW based Autonomous frequency selection will make it easy
>>>> for any future changes.
>>>
>>> So CPUs will act totally differently under these two instance. But what if
>>> I want part of the CPUs in HW mode and others in SW mode? Should I boot on
>>> HW mode and set some policies' auto_set to false or the other way? It seems
>>> like the effects of theses two approaches are completely different. In my
>>> opinion, this new instance is more like a completely different driver than
>>> cppc_cpufreq.
>>>
>>>>
>>>>> If it turns out that the new instance is necessary, I think we can reach a
>>>>> common approach by adding this new cpufreq driver instance and place the
>>>>> attributes in 'cppc_cpufreq_epp_attr', like amd-pstate did.
>>>>>
>>>>> What do you think?
>>>>
>>>> I initially thought about this but there was a problem.
>>>> What if we boot with non-epp instance which doesn't have these attributes and later want to enable Auto mode for few CPU's from sysfs.
>>>
>>> That's the problem. CPUs can be set to Auto mode with or without this new
>>> instance. So what's the point of it?
>>>
>>>>
>>>>
>>>> Best Regards,
>>>> Sumit Gupta
>>>
>>>
>>>
>>
>>
>