Re: [PATCH v6 4/9] ACPI: CPPC: Add cppc_get_perf() API to read performance controls

[Pierre Gondois <pierre.gondois@arm.com>]

On 1/24/26 21:19, Sumit Gupta wrote:
>
> On 25/01/26 01:35, Sumit Gupta wrote:
>>
>> On 22/01/26 14:26, zhenglifeng (A) wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On 2026/1/20 22:56, Sumit Gupta wrote:
>>>> Add cppc_get_perf() function to read values of performance control
>>>> registers including desired_perf, min_perf, max_perf, energy_perf,
>>>> and auto_sel.
>>>>
>>>> This provides a read interface to complement the existing
>>>> cppc_set_perf() write interface for performance control registers.
>>>>
>>>> Note that auto_sel is read by cppc_get_perf() but not written by
>>>> cppc_set_perf() to avoid unintended mode changes during performance
>>>> updates. It can be updated with existing dedicated cppc_set_auto_sel()
>>>> API.
>>>>
>>>> Use cppc_get_perf() in cppc_cpufreq_get_cpu_data() to initialize
>>>> perf_ctrls with current hardware register values during cpufreq
>>>> policy initialization.
>>>>
>>>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>>>> ---
>>>>   drivers/acpi/cppc_acpi.c       | 80 
>>>> ++++++++++++++++++++++++++++++++++
>>>>   drivers/cpufreq/cppc_cpufreq.c |  6 +++
>>>>   include/acpi/cppc_acpi.h       |  5 +++
>>>>   3 files changed, 91 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index a09bdabaa804..de35aeb07833 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -1739,6 +1739,86 @@ int cppc_set_enable(int cpu, bool enable)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(cppc_set_enable);
>>>>
>>>> +/**
>>>> + * cppc_get_perf - Get a CPU's performance controls.
>>>> + * @cpu: CPU for which to get performance controls.
>>>> + * @perf_ctrls: ptr to cppc_perf_ctrls. See cppc_acpi.h
>>>> + *
>>>> + * Return: 0 for success with perf_ctrls, -ERRNO otherwise.
>>>> + */
>>>> +int cppc_get_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
>>>> +{
>>>> +     struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
>>>> +     struct cpc_register_resource *desired_perf_reg,
>>>> +                                  *min_perf_reg, *max_perf_reg,
>>>> +                                  *energy_perf_reg, *auto_sel_reg;
>>>> +     u64 desired_perf = 0, min = 0, max = 0, energy_perf = 0, 
>>>> auto_sel = 0;
>>>> +     int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
>>>> +     struct cppc_pcc_data *pcc_ss_data = NULL;
>>>> +     int ret = 0, regs_in_pcc = 0;
>>>> +
>>>> +     if (!cpc_desc) {
>>>> +             pr_debug("No CPC descriptor for CPU:%d\n", cpu);
>>>> +             return -ENODEV;
>>>> +     }
>>>> +
>>>> +     if (!perf_ctrls) {
>>>> +             pr_debug("Invalid perf_ctrls pointer\n");
>>>> +             return -EINVAL;
>>>> +     }
>>>> +
>>>> +     desired_perf_reg = &cpc_desc->cpc_regs[DESIRED_PERF];
>>>> +     min_perf_reg = &cpc_desc->cpc_regs[MIN_PERF];
>>>> +     max_perf_reg = &cpc_desc->cpc_regs[MAX_PERF];
>>>> +     energy_perf_reg = &cpc_desc->cpc_regs[ENERGY_PERF];
>>>> +     auto_sel_reg = &cpc_desc->cpc_regs[AUTO_SEL_ENABLE];
>>>> +
>>>> +     /* Are any of the regs PCC ?*/
>>>> +     if (CPC_IN_PCC(desired_perf_reg) || CPC_IN_PCC(min_perf_reg) ||
>>>> +         CPC_IN_PCC(max_perf_reg) || CPC_IN_PCC(energy_perf_reg) ||
>>>> +         CPC_IN_PCC(auto_sel_reg)) {
>>>> +             if (pcc_ss_id < 0) {
>>>> +                     pr_debug("Invalid pcc_ss_id for CPU:%d\n", cpu);
>>>> +                     return -ENODEV;
>>>> +             }
>>>> +             pcc_ss_data = pcc_data[pcc_ss_id];
>>>> +             regs_in_pcc = 1;
>>>> +             down_write(&pcc_ss_data->pcc_lock);
>>>> +             /* Ring doorbell once to update PCC subspace */
>>>> +             if (send_pcc_cmd(pcc_ss_id, CMD_READ) < 0) {
>>>> +                     ret = -EIO;
>>>> +                     goto out_err;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     /* Read optional elements if present */
>>>> +     if (CPC_SUPPORTED(max_perf_reg))
>>>> +             cpc_read(cpu, max_perf_reg, &max);
>>>> +     perf_ctrls->max_perf = max;
>>>> +
>>>> +     if (CPC_SUPPORTED(min_perf_reg))
>>>> +             cpc_read(cpu, min_perf_reg, &min);
>>>> +     perf_ctrls->min_perf = min;
>>>> +
>>>> +     if (CPC_SUPPORTED(desired_perf_reg))
>>>> +             cpc_read(cpu, desired_perf_reg, &desired_perf);
>>>> +     perf_ctrls->desired_perf = desired_perf;
>>> desired_perf_reg is not an optional one, so it has to be supported.
>
> Please ignore my previous reply on this. Had some problem with my email
> client and both mails got mixed.
>
> The register is optional when Autonomous mode is enabled.
> Seems the comment here is already resolved with Pierre's reply.
> We discussed this during v4 [1] also.
> [1] 
> https://lore.kernel.org/lkml/ccd45c1b-2f69-4725-918f-18063f00a864@nvidia.com/
>
> Thank you,
> Sumit Gupta
>
>
As suggested at:

https://lore.kernel.org/all/5afea521-7d80-4e72-8809-77af60b0d957@arm.com/

Maybe it would be useful to add a patch checking this:


diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c

index e3796b520e473..7db74e19425e6 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -854,6 +854,12 @@ int acpi_cppc_processor_probe(struct acpi_processor 
*pr)
         }
         per_cpu(cpu_pcc_subspace_idx, pr->id) = pcc_subspace_id;

+       if (!CPC_SUPPORTED(&cpc_ptr->cpc_regs[DESIRED_PERF]) &&
+           (!osc_sb_cppc2_support_acked ||
+ !CPC_SUPPORTED(&cpc_ptr->cpc_regs[AUTO_SEL_ENABLE])))
+               pr_warn("Desired perf. register is mandatory if CPPCV2 
is not supported "
+                       "or autonomous selection is disabled.\n");
+
         /*
          * Initialize the remaining cpc_regs as unsupported.
          * Example: In case FW exposes CPPC v2, the below loop will 
initialize