Re: [PATCH v4 3/8] ACPI: CPPC: extend APIs to support auto_sel and epp

[Sumit Gupta <sumitg@nvidia.com>]

On 27/11/25 20:24, Pierre Gondois wrote:
> External email: Use caution opening links or attachments
>
>
> Hello Sumit, Mario,
>
> On 11/5/25 12:38, Sumit Gupta wrote:
>> - Add auto_sel read support in cppc_get_perf_caps().
>> - Add write of both auto_sel and energy_perf in cppc_set_epp_perf().
>> - Remove redundant energy_perf field from 'struct cppc_perf_caps' as
>>    the same is available in 'struct cppc_perf_ctrls' which is used.
>>
>> Signed-off-by: Sumit Gupta<sumitg@nvidia.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 42 ++++++++++++++++++++++++++++++++--------
>>   include/acpi/cppc_acpi.h |  1 -
>>   2 files changed, 34 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index 05672c30187c..757e8ce87e9b 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1344,8 +1344,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;
>
> I am not sure this is really useful to get the auto_sel value register
> in this function as:
> - All of the other registers read are read-only
> - The name of the function doesn't match: the autonomous selection is
> not really
> related to perf. capabilities
>
> I assume this change comes from the presence of the auto_sel register 
> in the
> 'struct cppc_perf_caps', but IMO this register should be placed in
> another structure.
>
> I assume this is ok to let it in 'struct cppc_perf_caps' for now, but I
> think we should not
> fetch the value with all the other perf. capabilities values.
>

Yes, reading it here in cppc_get_perf_caps() as it was part of
'struct cppc_perf_caps' in the existing code which i agree looks wrong.
I am adding cppc_get_perf() api to read performance controls in 'patch 
2/8'.
Additionally, I can move the 'auto_sel' field from 'struct cppc_perf_caps'
to 'struct cppc_perf_ctrls' and read it within cppc_get_perf() with other
performance controls.


>
>>       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;
>> @@ -1362,11 +1362,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;
>> @@ -1414,6 +1415,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)
>> @@ -1555,6 +1559,8 @@ int cppc_set_epp_perf(int cpu, struct 
>> cppc_perf_ctrls *perf_ctrls, bool enable)
>>       struct cpc_register_resource *auto_sel_reg;
>>       struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>       struct cppc_pcc_data *pcc_ss_data = NULL;
>> +     bool autosel_support_in_ffh_or_sysmem;
>> +     bool epp_support_in_ffh_or_sysmem;
>>       int ret;
>>
>>       if (!cpc_desc) {
>> @@ -1565,6 +1571,11 @@ int cppc_set_epp_perf(int cpu, struct 
>> cppc_perf_ctrls *perf_ctrls, bool enable)
>>       auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>>       epp_set_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
>>
>> +     epp_support_in_ffh_or_sysmem = CPC_SUPPORTED(epp_set_reg) &&
>> +                             (CPC_IN_FFH(epp_set_reg) || 
>> CPC_IN_SYSTEM_MEMORY(epp_set_reg));
>> +     autosel_support_in_ffh_or_sysmem = CPC_SUPPORTED(auto_sel_reg) &&
>> +                             (CPC_IN_FFH(auto_sel_reg) || 
>> CPC_IN_SYSTEM_MEMORY(auto_sel_reg));
>> +
>>       if (CPC_IN_PCC(epp_set_reg) || CPC_IN_PCC(auto_sel_reg)) {
>>               if (pcc_ss_id < 0) {
>>                       pr_debug("Invalid pcc_ss_id forCPU:%d\n", cpu);
>> @@ -1589,14 +1600,29 @@ int cppc_set_epp_perf(int cpu, struct 
>> cppc_perf_ctrls *perf_ctrls, 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 if (osc_cpc_flexible_adr_space_confirmed &&
>> -                CPC_SUPPORTED(epp_set_reg) && 
>> CPC_IN_FFH(epp_set_reg)) {
>> -             ret = cpc_write(cpu, epp_set_reg, 
>> perf_ctrls->energy_perf);
>
> I think this is a bit out of the scope of this patchset, but I'm not
> sure this is necessary
> to check the value of "osc_cpc_flexible_adr_space_confirmed" here.
> Indeed, acpi_cppc_processor_probe() already loops over the CPPC fields
> and should detect when a field is using an address space that is not
> allowed by "osc_cpc_flexible_adr_space_confirmed".
>
> From what I understand:
>
> - osc_cpc_flexible_adr_space_confirmed was introduced to check that CPPC
> registers are in the correct address space
>
> - this broke some amd platforms that didn't configure the _OSC method
> correctly
>
> - 8b356e536e69 ("ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is
> supported") introduced cpc_supported_by_cpu() to bypass the check of
> osc_cpc_flexible_adr_space_confirmed. Indeed, the broken amd platforms
> don't configure the _OSC method, but it is possible to check if there is
> CPPC support by reading an MSR register.
>
> - an amd platform failed to set the EPP register. This seems to be due
> to the EPP register being located in FFH and not in PCC. However the
> handler only supported PCC at that time: 7bc1fcd39901 ("ACPI: CPPC: Add
> AMD pstate energy performance preference cppc control") The bug report
> thread: bugzilla.kernel.org/show_bug.cgi?id=218686
>
> - to allow setting the EPP value when it is located in FFH, the
> following patch was done: aaf21ac93909 ("ACPI: CPPC: Add support for
> setting EPP register in FFH") This patch seems to have added a check
> over the _OSC flexible bit value due to this comment:
> https://bugzilla.kernel.org/show_bug.cgi?id=218686#c83 However the CPPC
> registers are always allowed to be located in the FFH and PCC address
> space. Cf: 0651ab90e4ad ("ACPI: CPPC: Check _OSC for flexible address
> space")
>
> ------------
>
> Just to summarize, I think the check over
> osc_cpc_flexible_adr_space_confirmed could/should be removed. Ideally in
> a separate patch.
>
> If Mario could confirm this is correct this would be nice.
>

Can send a separate patch with this change.

Thank you,
Sumit Gupta


>
>> +     } else if (osc_cpc_flexible_adr_space_confirmed) {
>> +             if (!epp_support_in_ffh_or_sysmem && 
>> !autosel_support_in_ffh_or_sysmem) {
>> +                     ret = -EOPNOTSUPP;
>> +             } else {
>> +                     if (autosel_support_in_ffh_or_sysmem) {
>> +                             ret = cpc_write(cpu, auto_sel_reg, 
>> enable);
>> +                             if (ret)
>> +                                     return ret;
>> +                     }
>> +
>> +                     if (epp_support_in_ffh_or_sysmem) {
>> +                             ret = cpc_write(cpu, epp_set_reg, 
>> perf_ctrls->energy_perf);
>> +                             if (ret)
>> +                                     return ret;
>> +                     }
>> +             }
>>       } else {
>> -             ret = -ENOTSUPP;
>> -             pr_debug("_CPC in PCC and _CPC in FFH are not 
>> supported\n");
>> +             ret = -EOPNOTSUPP;
>>       }
>>
>> +     if (ret == -EOPNOTSUPP)
>> +             pr_debug("_CPC in PCC and _CPC in FFH are not 
>> supported\n");
>> +
>>       return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 7190afeead8b..42e37a84cac9 100644
>> --- a/include/acpi/cppc_acpi.h
>> +++ b/include/acpi/cppc_acpi.h
>> @@ -119,7 +119,6 @@ struct cppc_perf_caps {
>>       u32 lowest_nonlinear_perf;
>>       u32 lowest_freq;
>>       u32 nominal_freq;
>> -     u32 energy_perf;
>>       bool auto_sel;
>>   };
>>