Re: [PATCH v5 04/11] ACPI: CPPC: Extend cppc_set_epp_perf() to support auto_sel and epp

[Sumit Gupta <sumitg@nvidia.com>]

On 25/12/25 09:26, zhenglifeng (A) wrote:
> External email: Use caution opening links or attachments
>
>
> On 2025/12/23 20:13, Sumit Gupta wrote:
>> Extend cppc_set_epp_perf() to write both auto_sel and energy_perf
>> registers when they are in FFH or SystemMemory address space.
>>
>> This keeps the behavior consistent with PCC case where both registers
>> are already updated together, but was missing for FFH/SystemMemory.
>>
>> Also update EPP constants for better clarity:
>> - Add CPPC_EPP_PERFORMANCE_PREF (0x00) for performance preference
>> - Add CPPC_EPP_ENERGY_EFFICIENCY_PREF (0xFF) for energy efficiency
>>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>   drivers/acpi/cppc_acpi.c | 35 +++++++++++++++++++++++++++++------
>>   include/acpi/cppc_acpi.h |  3 ++-
>>   2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>> index a4e89fe6aab5..403ee988a8c6 100644
>> --- a/drivers/acpi/cppc_acpi.c
>> +++ b/drivers/acpi/cppc_acpi.c
>> @@ -1556,6 +1556,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_ffh_sysmem;
>> +     bool epp_ffh_sysmem;
>>        int ret;
>>
>>        if (!cpc_desc) {
>> @@ -1566,6 +1568,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_ffh_sysmem = CPC_SUPPORTED(epp_set_reg) &&
>> +             (CPC_IN_FFH(epp_set_reg) || CPC_IN_SYSTEM_MEMORY(epp_set_reg));
>> +     autosel_ffh_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);
>> @@ -1590,14 +1597,30 @@ 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_ffh_sysmem && !autosel_ffh_sysmem) {
>> +                     ret = -EOPNOTSUPP;
>> +             } else {
> I think this can do the same thing:
>
>          } else if (osc_cpc_flexible_adr_space_confirmed &&
>                     (epp_ffh_sysmem || autosel_ffh_sysmem)) {
>
> and reduce the levels of indentation.

Thanks for suggesting. Will change in v6.

>> +                     if (autosel_ffh_sysmem) {
>> +                             ret = cpc_write(cpu, auto_sel_reg, enable);
>> +                             if (ret)
>> +                                     return ret;
>> +                     }
>> +
>> +                     if (epp_ffh_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("CPU%d: _CPC not in PCC/FFH/SystemMemory\n", cpu);
>> +
>>        return ret;
>>   }
>>   EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
> The code below is unrelated to the code above and should ideally be
> separated into a separate patch I think.

Will move it to a separate patch in v6.

Thank you,
Sumit Gupta

>
>> @@ -1609,7 +1632,7 @@ EXPORT_SYMBOL_GPL(cppc_set_epp_perf);
>>    */
>>   int cppc_set_epp(int cpu, u64 epp_val)
>>   {
>> -     if (epp_val > CPPC_ENERGY_PERF_MAX)
>> +     if (epp_val > CPPC_EPP_ENERGY_EFFICIENCY_PREF)
>>                return -EINVAL;
>>
>>        return cppc_set_reg_val(cpu, ENERGY_PERF, epp_val);
>> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
>> index 12a1dc31bf2a..2860a0252313 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 {