Message-ID: <80e16de0-63e4-4ead-9577-4ebba9b1a02d@nvidia.com>
Date: Fri, 24 Oct 2025 18:52:51 +0530
From: Sumit Gupta <sumitg@...dia.com>
To: Ionela Voinescu <ionela.voinescu@....com>
Cc: rafael@...nel.org, viresh.kumar@...aro.org, lenb@...nel.org,
 robert.moore@...el.com, corbet@....net, pierre.gondois@....com,
 zhenglifeng1@...wei.com, rdunlap@...radead.org, ray.huang@....com,
 gautham.shenoy@....com, mario.limonciello@....com, perry.yuan@....com,
 linux-pm@...r.kernel.org, linux-acpi@...r.kernel.org,
 linux-doc@...r.kernel.org, acpica-devel@...ts.linux.dev,
 linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
 treding@...dia.com, jonathanh@...dia.com, vsethi@...dia.com,
 ksitaraman@...dia.com, sanjayc@...dia.com, bbasu@...dia.com,
 sumitg@...dia.com
Subject: Re: [PATCH v3 4/8] ACPI: CPPC: add APIs and sysfs interface for
 min/max_perf


On 22/10/25 16:28, Ionela Voinescu wrote:
> External email: Use caution opening links or attachments
>
>
> On Wednesday 01 Oct 2025 at 20:31:00 (+0530), Sumit Gupta wrote:
>> CPPC allows platforms to specify minimum and maximum performance
>> limits that constrain the operating range for CPU performance scaling
>> when Autonomous Selection is enabled. These limits can be dynamically
>> adjusted to implement power management policies or workload-specific
>> optimizations.
>>
>> Add cppc_get_min_perf() and cppc_set_min_perf() functions to read and
>> write the MIN_PERF register, allowing dynamic adjustment of the minimum
>> performance floor.
>>
>> Add cppc_get_max_perf() and cppc_set_max_perf() functions to read and
>> write the MAX_PERF register, enabling dynamic ceiling control for
>> maximum performance.
>>
>> Expose these capabilities through cpufreq sysfs attributes:
>> - /sys/.../cpufreq/policy*/min_perf: Read/write min performance limit
>> - /sys/.../cpufreq/policy*/max_perf: Read/write max performance limit
>>
>> Also update EPP constants for better clarity:
>> - Rename CPPC_ENERGY_PERF_MAX to CPPC_EPP_ENERGY_EFFICIENCY_PREF
>> - Add CPPC_EPP_PERFORMANCE_PREF for the performance-oriented setting
>>
>> Signed-off-by: Sumit Gupta <sumitg@...dia.com>
>> ---
>>   drivers/acpi/cppc_acpi.c       |  55 +++++++++++++++-
>>   drivers/cpufreq/cppc_cpufreq.c | 115 +++++++++++++++++++++++++++++++++
>>   include/acpi/cppc_acpi.h       |  23 ++++++-
>>   3 files changed, 191 insertions(+), 2 deletions(-)
>>
> [..]
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
>> index 732f35096991..864978674efc 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -23,6 +23,7 @@
>>   #include <uapi/linux/sched/types.h>
>>
>>   #include <linux/unaligned.h>
>> +#include <linux/cleanup.h>
>>
>>   #include <acpi/cppc_acpi.h>
>>
>> @@ -38,6 +39,8 @@ static enum {
>>   module_param(fie_disabled, int, 0444);
>>   MODULE_PARM_DESC(fie_disabled, "Disable Frequency Invariance Engine (FIE)");
>>
>> +static DEFINE_MUTEX(cppc_cpufreq_update_autosel_config_lock);
> Should this be under CONFIG_ACPI_CPPC_CPUFREQ_FIE? 

Thank you for catching. Will move it outside in v4.


>> +
>>   /* Frequency invariance support */
>>   struct cppc_freq_invariance {
>>        int cpu;
>> @@ -572,6 +575,70 @@ static void cppc_cpufreq_put_cpu_data(struct cpufreq_policy *policy)
>>        policy->driver_data = NULL;
>>   }
>>
>> +/**
>> + * cppc_cpufreq_set_mperf_limit - Generic function to set min/max performance limit
>> + * @policy: cpufreq policy
>> + * @val: performance value to set
>> + * @update_reg: whether to update hardware register
>> + * @update_policy: whether to update policy constraints
>> + * @is_min: true for min_perf, false for max_perf
>> + */
>> +static int cppc_cpufreq_set_mperf_limit(struct cpufreq_policy *policy, u64 val,
>> +                                     bool update_reg, bool update_policy, bool is_min)
>> +{
>> +     struct cppc_cpudata *cpu_data = policy->driver_data;
>> +     struct cppc_perf_caps *caps = &cpu_data->perf_caps;
>> +     unsigned int cpu = policy->cpu;
>> +     struct freq_qos_request *req;
>> +     unsigned int freq;
>> +     u32 perf;
>> +     int ret;
>> +
>> +     perf = clamp(val, caps->lowest_perf, caps->highest_perf);
>> +     freq = cppc_perf_to_khz(caps, perf);
>> +
>> +     pr_debug("cpu%d, %s_perf:%llu, update_reg:%d, update_policy:%d\n", cpu,
>> +              is_min ? "min" : "max", (u64)perf, update_reg, update_policy);
>> +
>> +     guard(mutex)(&cppc_cpufreq_update_autosel_config_lock);
>> +
>> +     if (update_reg) {
>> +             ret = is_min ? cppc_set_min_perf(cpu, perf) : cppc_set_max_perf(cpu, perf);
>> +             if (ret == -EOPNOTSUPP)
>> +                     return 0;
> Should we return success here? The user will have no feedback that
> setting min/max performance limits is not supported. 

Will do change in v4 to also return EOPNOTSUPP error code to the caller.


>> +             if (ret) {
>> +                     pr_warn("Failed to set %s_perf (%llu) on CPU%d (%d)\n",
>> +                             is_min ? "min" : "max", (u64)perf, cpu, ret);
>> +                     return ret;
>> +             }
>> +
>> +             /* Update cached value only on success */
>> +             if (is_min)
>> +                     cpu_data->perf_ctrls.min_perf = perf;
>> +             else
>> +                     cpu_data->perf_ctrls.max_perf = perf;
>> +     }
>> +
>> +     if (update_policy) {
>> +             req = is_min ? policy->min_freq_req : policy->max_freq_req;
>> +
>> +             ret = freq_qos_update_request(req, freq);
>> +             if (ret < 0) {
>> +                     pr_warn("Failed to update %s_freq constraint for CPU%d: %d\n",
>> +                             is_min ? "min" : "max", cpu, ret);
>> +                     return ret;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +#define cppc_cpufreq_set_min_perf(policy, val, update_reg, update_policy) \
>> +     cppc_cpufreq_set_mperf_limit(policy, val, update_reg, update_policy, true)
>> +
>> +#define cppc_cpufreq_set_max_perf(policy, val, update_reg, update_policy) \
>> +     cppc_cpufreq_set_mperf_limit(policy, val, update_reg, update_policy, false)
>> +
>>   static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
>>   {
>>        unsigned int cpu = policy->cpu;
>> @@ -873,16 +940,64 @@ static ssize_t store_energy_performance_preference_val(struct cpufreq_policy *po
>>        return cppc_cpufreq_sysfs_store_u64(buf, count, cppc_set_epp, policy->cpu);
>>   }
>>
>> +static ssize_t show_min_perf(struct cpufreq_policy *policy, char *buf)
>> +{
>> +     return cppc_cpufreq_sysfs_show_u64(policy->cpu, cppc_get_min_perf, buf);
>> +}
>> +
>> +static ssize_t store_min_perf(struct cpufreq_policy *policy, const char *buf, size_t count)
>> +{
>> +     struct cppc_cpudata *cpu_data = policy->driver_data;
>> +     u64 val;
>> +     int ret;
>> +
>> +     ret = kstrtou64(buf, 0, &val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = cppc_cpufreq_set_min_perf(policy, val, true, cpu_data->perf_caps.auto_sel);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +
>> +static ssize_t show_max_perf(struct cpufreq_policy *policy, char *buf)
>> +{
>> +     return cppc_cpufreq_sysfs_show_u64(policy->cpu, cppc_get_max_perf, buf);
>> +}
>> +
>> +static ssize_t store_max_perf(struct cpufreq_policy *policy, const char *buf, size_t count)
>> +{
>> +     struct cppc_cpudata *cpu_data = policy->driver_data;
>> +     u64 val;
>> +     int ret;
>> +
>> +     ret = kstrtou64(buf, 0, &val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = cppc_cpufreq_set_max_perf(policy, val, true, cpu_data->perf_caps.auto_sel);
> These "work" on the performance scale, so the values read are
> performance values and the values that should be provided should be
> performance values as well. How is the user supposed to know what that
> scale is and what is the range of values it can provide? All of the
> other sysfs interfaces work on the "frequency" scale and the lowest and
> highest performance values are never exposed to the user.
>
> Thanks,
> Ionela.

Can do the change in v4 for these nodes to get min/max freq value from 
user, convert
that to perf with cppc_khz_to_perf() before write to min/max_perf 
registers and vice-versa.
That will keep the scale same as other cpufreq sysfs interfaces.


>> +     if (ret)
>> +             return ret;
>> +
>> +     return count;
>> +}
>> +
>>   cpufreq_freq_attr_ro(freqdomain_cpus);
>>   cpufreq_freq_attr_rw(auto_select);
>>   cpufreq_freq_attr_rw(auto_act_window);
>>   cpufreq_freq_attr_rw(energy_performance_preference_val);
>> +cpufreq_freq_attr_rw(min_perf);
>> +cpufreq_freq_attr_rw(max_perf);
>>
>>   static struct freq_attr *cppc_cpufreq_attr[] = {
>>        &freqdomain_cpus,
>>        &auto_select,
>>        &auto_act_window,
>>        &energy_performance_preference_val,
>> +     &min_perf,
>> +     &max_perf,
>>        NULL,
>>   };
>>
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 3babc6d6e70a..fc7614eb9dcb 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -39,7 +39,8 @@
>>   /* CPPC_AUTO_ACT_WINDOW_MAX_SIG is 127, so 128 and 129 will decay to 127 when writing */
>>   #define CPPC_AUTO_ACT_WINDOW_SIG_CARRY_THRESH 129
>>
>> -#define CPPC_ENERGY_PERF_MAX (0xFF)
>> +#define CPPC_EPP_PERFORMANCE_PREF            0x00
>> +#define CPPC_EPP_ENERGY_EFFICIENCY_PREF              0xFF
>>
>>   /* Each register has the folowing format. */
>>   struct cpc_reg {
>> @@ -172,6 +173,10 @@ extern int cppc_get_auto_act_window(int cpu, u64 *auto_act_window);
>>   extern int cppc_set_auto_act_window(int cpu, u64 auto_act_window);
>>   extern int cppc_get_auto_sel(int cpu, bool *enable);
>>   extern int cppc_set_auto_sel(int cpu, bool enable);
>> +extern int cppc_get_min_perf(int cpu, u64 *min_perf);
>> +extern int cppc_set_min_perf(int cpu, u64 min_perf);
>> +extern int cppc_get_max_perf(int cpu, u64 *max_perf);
>> +extern int cppc_set_max_perf(int cpu, u64 max_perf);
>>   extern int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf);
>>   extern int amd_get_boost_ratio_numerator(unsigned int cpu, u64 *numerator);
>>   extern int amd_detect_prefcore(bool *detected);
>> @@ -264,6 +269,22 @@ static inline int cppc_set_auto_sel(int cpu, bool enable)
>>   {
>>        return -EOPNOTSUPP;
>>   }
>> +static inline int cppc_get_min_perf(int cpu, u64 *min_perf)
>> +{
>> +     return -EOPNOTSUPP;
>> +}
>> +static inline int cppc_set_min_perf(int cpu, u64 min_perf)
>> +{
>> +     return -EOPNOTSUPP;
>> +}
>> +static inline int cppc_get_max_perf(int cpu, u64 *max_perf)
>> +{
>> +     return -EOPNOTSUPP;
>> +}
>> +static inline int cppc_set_max_perf(int cpu, u64 max_perf)
>> +{
>> +     return -EOPNOTSUPP;
>> +}
>>   static inline int amd_get_highest_perf(unsigned int cpu, u32 *highest_perf)
>>   {
>>        return -ENODEV;
>> --
>> 2.34.1
>>
>>

Powered by blists - more mailing lists