Re: [PATCH v5 2/2] cpufreq: CPPC: add autonomous mode boot parameter support

[Sumit Gupta <sumitg@nvidia.com>]

On 01/07/26 21:54, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
> On 6/23/26 10:06, Sumit Gupta wrote:
>> Add a kernel boot parameter 'cppc_cpufreq.auto_sel_mode' to enable
>> CPPC autonomous performance selection on all CPUs at system startup.
>> When autonomous mode is enabled, the hardware automatically adjusts
>> CPU performance based on workload demands using Energy Performance
>> Preference (EPP) hints.
>>
>> When the parameter is set:
>> - Configure all CPUs for autonomous operation on first init
>> - Use HW min/max_perf when available; otherwise initialize from caps
>> - Initialize desired_perf to max_perf as a starting hint
>> - Hardware controls frequency instead of the OS governor
>> - EPP behavior depends on parameter value:
>>    - performance (or 1):         override EPP to performance (0x0)
>>    - balance_performance (or 2): override EPP to balance_performance
>>                                  (0x80)
>>    - default_epp (or 3):         preserve EPP value programmed by
>>                                  BIOS/firmware
>>
>> Unset, "0"/"disabled", or an unrecognized value leaves autonomous
>> selection disabled.
>>
>> The boot parameter is applied only during first policy initialization.
>> Skip applying it on CPU hotplug to preserve runtime sysfs configuration.
>>
>> This relies on commit 8c83947c5dbb ("cpufreq: Use policy->min/max 
>> init as
>> QoS request") so that the policy->min/max set in cppc_cpufreq_cpu_init()
>> are used as the policy's QoS requests and not overridden by
>> cpufreq_set_policy() during init.
>>
>> Signed-off-by: Sumit Gupta<sumitg@nvidia.com>
>> ---
>>   .../admin-guide/kernel-parameters.txt         |  22 +++
>>   drivers/cpufreq/cppc_cpufreq.c                | 151 +++++++++++++++++-
>>   include/acpi/cppc_acpi.h                      |   1 +
>>   3 files changed, 169 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt 
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index b5493a7f8f22..88820d34d516 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -1019,6 +1019,28 @@ Kernel parameters
>>                       policy to use. This governor must be registered 
>> in the
>>                       kernel before the cpufreq driver probes.
>>
>> +     cppc_cpufreq.auto_sel_mode=
>> +                     [CPU_FREQ] Enable ACPI CPPC autonomous performance
>> +                     selection. When enabled, hardware automatically 
>> adjusts
>> +                     CPU frequency on all CPUs based on workload 
>> demands.
>> +                     In Autonomous mode, Energy Performance 
>> Preference (EPP)
>> +                     hints guide hardware toward performance (0x0) 
>> or energy
>> +                     efficiency (0xff).
>> +                     Requires ACPI CPPC autonomous selection register
>> +                     support.
>> +                     Accepts:
>> +                       disabled, 0:
>
> Just a question, but would it be worth only accepting
> strings ? If we want to have a thinner granularity later,
> it will be harder to introduce them if there are integer values
> already present.

Sure, will change to keep strings only for flexibility in future.

>
>> +                               cpufreq governors are used (auto_sel 
>> disabled)
>> +                       performance, 1:
>> +                               enable auto_sel + set EPP to 
>> performance (0x0)
>> +                       balance_performance, 2:
>> +                               enable auto_sel + set EPP to
>> +                               balance_performance (0x80)
>> +                       default_epp, 3:
>> +                               enable auto_sel, preserve EPP value 
>> programmed
>> +                               by BIOS/firmware
>> +                     Unset or an unrecognized value is treated as 
>> disabled.
>> +
>>       cpu_init_udelay=N
>>                       [X86,EARLY] Delay for N microsec between assert 
>> and de-assert
>>                       of APIC INIT to start processors.  This delay 
>> occurs
>> diff --git a/drivers/cpufreq/cppc_cpufreq.c 
>> b/drivers/cpufreq/cppc_cpufreq.c
>> index f7a47576717a..efa673e3830c 100644
>> --- a/drivers/cpufreq/cppc_cpufreq.c
>> +++ b/drivers/cpufreq/cppc_cpufreq.c
>> @@ -28,6 +28,55 @@
>>
>>   static struct cpufreq_driver cppc_cpufreq_driver;
>>
>> +/* Autonomous Selection boot parameter modes */
>> +enum {
>> +     AUTO_SEL_DISABLED = 0,
>> +     AUTO_SEL_PERFORMANCE = 1,
>> +     AUTO_SEL_BALANCE_PERFORMANCE = 2,
>> +     AUTO_SEL_DEFAULT_EPP = 3,
>> +};
>> +
>> +static int auto_sel_mode;
>> +
>> +static int auto_sel_mode_set(const char *val, const struct 
>> kernel_param *kp)
>> +{
>> +     int *mode = kp->arg;
>> +
>> +     *mode = AUTO_SEL_DISABLED;
>> +
>> +     if (sysfs_streq(val, "performance") || sysfs_streq(val, "1"))
>> +             *mode = AUTO_SEL_PERFORMANCE;
>> +     else if (sysfs_streq(val, "balance_performance") || 
>> sysfs_streq(val, "2"))
>> +             *mode = AUTO_SEL_BALANCE_PERFORMANCE;
>> +     else if (sysfs_streq(val, "default_epp") || sysfs_streq(val, "3"))
>> +             *mode = AUTO_SEL_DEFAULT_EPP;
>> +     else if (!sysfs_streq(val, "disabled") && !sysfs_streq(val, "0"))
>> +             pr_warn("Invalid auto_sel_mode \"%s\", disable auto 
>> select\n", val);
>> +
>> +     return 0;
>> +}
>> +
>> +static int auto_sel_mode_get(char *buffer, const struct kernel_param 
>> *kp)
>> +{
>> +     int *mode = kp->arg;
>> +
>> +     switch (*mode) {
>> +     case AUTO_SEL_PERFORMANCE:
>> +             return sysfs_emit(buffer, "performance\n");
>> +     case AUTO_SEL_BALANCE_PERFORMANCE:
>> +             return sysfs_emit(buffer, "balance_performance\n");
>> +     case AUTO_SEL_DEFAULT_EPP:
>> +             return sysfs_emit(buffer, "default_epp\n");
>> +     default:
>> +             return sysfs_emit(buffer, "disabled\n");
>> +     }
>> +}
>> +
>> +static const struct kernel_param_ops auto_sel_mode_ops = {
>> +     .set = auto_sel_mode_set,
>> +     .get = auto_sel_mode_get,
>> +};
>> +
>>   #ifdef CONFIG_ACPI_CPPC_CPUFREQ_FIE
>>   static enum {
>>       FIE_UNSET = -1,
>> @@ -645,7 +694,9 @@ static int cppc_cpufreq_cpu_init(struct 
>> cpufreq_policy *policy)
>>       unsigned int cpu = policy->cpu;
>>       struct cppc_cpudata *cpu_data;
>>       struct cppc_perf_caps *caps;
>> +     bool set_epp = true;
>>       int ret;
>> +     u32 epp;
>>
>>       cpu_data = cppc_cpufreq_get_cpu_data(cpu);
>>       if (!cpu_data) {
>> @@ -715,11 +766,87 @@ 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);
>> -     if (ret) {
>> -             pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
>> -                      caps->highest_perf, cpu, ret);
>> -             goto out;
>> +     /*
>> +      * Enable autonomous mode on first init if boot param is set.
>> +      * Check last_governor to detect first init and skip if auto_sel
>> +      * is already enabled.
>> +      */
>> +     if (auto_sel_mode && policy->last_governor[0] == '\0' &&
>> +         !cpu_data->perf_ctrls.auto_sel) {
>
> If an .online() callback is introduced, does it mean that we can
> remove the "policy->last_governor[0] == '\0'" check ?
>
> Also maybe it would be worth creating a function for this
> "if" block ?
>
> Also (bis), maybe we should check that auto_sel is supported before
> doing anything else.
>

Agreed. Will change in v6.

>
>> +             /* Init min/max_perf from caps if not already set by 
>> HW. */
>> +             if (!cpu_data->perf_ctrls.min_perf)
>> +                     cpu_data->perf_ctrls.min_perf = 
>> caps->lowest_nonlinear_perf;
>> +             if (!cpu_data->perf_ctrls.max_perf)
>> +                     cpu_data->perf_ctrls.max_perf = 
>> policy->boost_enabled ?
>> +                             caps->highest_perf : caps->nominal_perf;
>
> Is it necessary to do that ?
>
> - for min_perf, we are setting it to the lowest possible value
>
> - for max_perf, we are setting it to the highest available value.
> If boost is disabled and we enabled it later, I don't think max_perf is
> updated accordingly, so we would limit the freq. to caps->nominal_perf
>
> (If I m not missing something)
>

The min/max seed is still needed so the HW has valid min/max limits
when firmware leaves them unset.
Here, will keep max_perf capped to nominal while boost is off and
change the second place below to set policy->max = highest_perf so the
driver does not limit the max below highest, letting the core cap
to nominal when boost is off.
Enabling boost then raises the HW max_perf to highest via ->target(),
fixing this point too.

>> +
>> +             /*
>> +              * In autonomous mode desired_perf is only a hint; EPP and
>> +              * the platform drive actual selection within [min, max].
>> +              * Initialize it to max_perf so HW starts at the upper 
>> bound.
>> +              */
>> +             cpu_data->perf_ctrls.desired_perf = 
>> cpu_data->perf_ctrls.max_perf;
>> +
>> +             policy->cur = cppc_perf_to_khz(caps,
>> + cpu_data->perf_ctrls.desired_perf);
>> +
>> +             /*
>> +              * Set EPP per mode. 'default_epp' preserves the 
>> BIOS/firmware
>> +              * programmed EPP value. EPP is optional - some 
>> platforms may
>> +              * not support it.
>> +              */
>> +             switch (auto_sel_mode) {
>> +             case AUTO_SEL_PERFORMANCE:
>> +                     epp = CPPC_EPP_PERFORMANCE_PREF;
>> +                     break;
>> +             case AUTO_SEL_BALANCE_PERFORMANCE:
>> +                     epp = CPPC_EPP_BALANCE_PERFORMANCE_PREF;
>> +                     break;
>> +             default:
>> +                     set_epp = false;
>> +                     break;
>> +             }
>> +
>> +             if (set_epp) {
>> +                     ret = cppc_set_epp(cpu, epp);
>> +                     if (ret && ret != -EOPNOTSUPP)
>> +                             pr_warn("Failed to set EPP for CPU%d 
>> (%d)\n", cpu, ret);
>> +                     else if (!ret)
>> + cpu_data->perf_ctrls.energy_perf = epp;
>> +             }
>> +
>> +             /* Program min/max/desired into CPPC regs (non-fatal on 
>> failure). */
>> +             ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
>> +             if (ret)
>> +                     pr_warn("set_perf failed CPU%d (%d); using HW 
>> values\n",
>> +                             cpu, ret);
>> +
>> +             ret = cppc_set_auto_sel(cpu, true);
>> +             if (ret && ret != -EOPNOTSUPP)
>> +                     pr_warn("auto_sel CPU%d failed (%d); using OS 
>> mode\n",
>> +                             cpu, ret);
>> +             else if (!ret)
>> +                     cpu_data->perf_ctrls.auto_sel = true;
>> +     }
> (until here)
>> +
>> +     if (cpu_data->perf_ctrls.auto_sel) {
>> +             /* Sync policy limits from HW when autonomous mode is 
>> active */
>
> Similar comment as above, doesn't it fall into a similar case as:
> 521223d8b3ec ("cpufreq: Fix initialization of min and max frequency QoS
> request")
>
> ?
>

Answered above.

>
>> +             policy->min = cppc_perf_to_khz(caps,
>> + cpu_data->perf_ctrls.min_perf ?:
>> + caps->lowest_nonlinear_perf);
>> +             policy->max = cppc_perf_to_khz(caps,
>> + cpu_data->perf_ctrls.max_perf ?:
>> + (policy->boost_enabled ?
>> + caps->highest_perf :
>> + caps->nominal_perf));
>> +     } else {
>> +             /* Normal mode: governors control frequency */
>> +             ret = cppc_set_perf(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);
>> @@ -1066,10 +1193,24 @@ static int __init cppc_cpufreq_init(void)
>>
>>   static void __exit cppc_cpufreq_exit(void)
>>   {
>> +     unsigned int cpu;
>> +
>> +     for_each_present_cpu(cpu)
>> +             cppc_set_auto_sel(cpu, false);
>
> I saw that this is being changed in:
> [PATCH] cpufreq: CPPC: Preserve OSPM-set registers across hotplug and 
> unload
>
> But I think it would make more sense to have this patch comes before 
> these
> patches:
>
>  [1] ACPI: CPPC: Add ospm_nominal_perf support
> https://lore.kernel.org/lkml/20260615185934.2383514-1-sumitg@nvidia.com/
>  [2] cpufreq: CPPC: add autonomous mode boot parameter support
> https://lore.kernel.org/lkml/20260623080652.3353386-1-sumitg@nvidia.com/
>
> to avoid making changes that are corrected in this last patch of the 
> serie.
>

Sure, will reorder them.

Thanks,
Sumit

....