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

[Sumit Gupta <sumitg@nvidia.com>]

Hi Ionela,

Thank you for the comments.


On 22/10/25 14:42, Ionela Voinescu wrote:
> External email: Use caution opening links or attachments
>
>
> Hi Sumit,
>
> On Wednesday 01 Oct 2025 at 20:30:59 (+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>
>> ---
> [..]
>> @@ -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);
>> @@ -1590,8 +1601,19 @@ int cppc_set_epp_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls, bool enable)
>>                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)) {
>> +                epp_support_in_ffh_or_sysmem && autosel_support_in_ffh_or_sysmem) {
> Isn't this problematic for when auto-select is an integer set to 1 or it's
> not present at all? In those cases the EPP register won't be written and
> -ENOTSUPP will be returned.
>
> I suppose for the case when auto-select is not present at all in _CPC
> (it's an optional attribute) it's not very clear what one should do
> regarding writing the EPP register. The specification mentions that
> "Writes to this register only have meaning when Autonomous Selection
> is enabled.". From my perspective the OS should not block writes to the
> EPP register for this case, as autonomous selection might still be
> enabled but not exposed to the OS.
>
> Thanks,
> Ionela.

Will change in v4 to write them independently as below.

-------------
         } else if (osc_cpc_flexible_adr_space_confirmed) {
                 if (!epp_support_in_ffh_or_sysmem && 
!autosel_support_in_ffh_or_sysmem) {
                         ret = -ENOTSUPP;
                 } 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;
         }

         if (ret == -ENOTSUPP)
                 pr_debug("_CPC in PCC and _CPC in FFH are not 
supported\n");

-------------

Thank you,
Sumit Gupta


>> +             ret = cpc_write(cpu, auto_sel_reg, enable);
>> +             if (ret) {
>> +                     pr_debug("Failed to write auto_sel=%d for CPU:%d\n", enable, cpu);
>> +                     return ret;
>> +             }
>> +
>>                ret = cpc_write(cpu, epp_set_reg, perf_ctrls->energy_perf);
>> +             if (ret) {
>> +                     pr_debug("Failed to write energy_perf=%u for CPU:%d\n",
>> +                              perf_ctrls->energy_perf, cpu);
>> +                     return ret;
>> +             }
>>        } else {
>>                ret = -ENOTSUPP;
>>                pr_debug("_CPC in PCC and _CPC in FFH are not supported\n");
> [..]