[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3097e4dd-a895-4c55-96c7-433ae1d046f9@arm.com>
Date: Mon, 26 Jan 2026 12:20:31 +0100
From: Pierre Gondois <pierre.gondois@....com>
To: Sumit Gupta <sumitg@...dia.com>, "zhenglifeng (A)"
<zhenglifeng1@...wei.com>
Cc: rafael@...nel.org, viresh.kumar@...aro.org, ionela.voinescu@....com,
lenb@...nel.org, robert.moore@...el.com, corbet@....net,
rdunlap@...radead.org, ray.huang@....com, gautham.shenoy@....com,
mario.limonciello@....com, perry.yuan@....com, zhanjie9@...ilicon.com,
linux-pm@...r.kernel.org, linux-acpi@...r.kernel.org,
linux-doc@...r.kernel.org, acpica-devel@...ts.linux.dev,
linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
treding@...dia.com, jonathanh@...dia.com, vsethi@...dia.com,
ksitaraman@...dia.com, sanjayc@...dia.com, nhartman@...dia.com,
bbasu@...dia.com
Subject: Re: [PATCH v6 4/9] ACPI: CPPC: Add cppc_get_perf() API to read
performance controls
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@...dia.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
Powered by blists - more mailing lists