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

[Sumit Gupta <sumitg@nvidia.com>]

Hi Ionela,

On 12/11/25 20:32, Ionela Voinescu wrote:
> External email: Use caution opening links or attachments
>
>
> Hi,
>
> A small nit that applies to multiple places: let's keep the line length
> under 80 characters - the lines seem easy to split.
Wrapped line length to 100 as per the max limit.
Will change to 80 chars if that's preferred.

   $ grep "max_line_length =" scripts/checkpatch.pl
   my $max_line_length = 100;


>
> On Wednesday 05 Nov 2025 at 17:08:39 (+0530), 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;
>>        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 for CPU:%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);
>> +     } 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;
>> +                     }
> Wouldn't it be more clear to have separate functions for setting auto-sel
> and EPP? I think this is functionally correct, but somewhat unclear, given
> the signature of the function. But I do acknowledge that the function was
> like this to begin with.


In v2, changed names of some functions for better clarity. One of those
was rename cppc_set_epp_perf() to cppc_set_epp_and_autosel() to reflect
that the API updates both. Reverted those changes after suggestion from
Rafael in [1] to avoid renaming.
There are existing separate API's cppc_set_auto_sel() & cppc_set_epp()
which set the register directly.
This change keeps the behavior of cppc_set_epp_perf() consistent to update
both together as that is already happening for PCC but not for other cases.

[1] 
https://lore.kernel.org/lkml/CAJZ5v0ipt=zDDORjuHzuerfLryZO5NE1RpPfnADeWEqi6kyv6Q@mail.gmail.com/


>> +             }
>>        } 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");
> This message needs updating.
>
> Thank you,
> Ionela.

Sure.

Thank you,
Sumit Gupta

>> +
>>        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;
>>   };
>>
>> --
>> 2.34.1
>>