Message-ID: <8fb77549-ce33-4c89-959b-57113eb716b6@arm.com>
Date: Thu, 27 Nov 2025 15:54:04 +0100
From: Pierre Gondois <pierre.gondois@....com>
To: Sumit Gupta <sumitg@...dia.com>, mario.limonciello@....com
Cc: linux-kernel@...r.kernel.org, acpica-devel@...ts.linux.dev,
 linux-doc@...r.kernel.org, linux-acpi@...r.kernel.org,
 linux-pm@...r.kernel.org, zhanjie9@...ilicon.com, ionela.voinescu@....com,
 perry.yuan@....com, gautham.shenoy@....com, ray.huang@....com,
 zhenglifeng1@...wei.com, corbet@....net, lenb@...nel.org,
 robert.moore@...el.com, viresh.kumar@...aro.org, rafael@...nel.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, rdunlap@...radead.org
Subject: Re: [PATCH v4 3/8] ACPI: CPPC: extend APIs to support auto_sel and
 epp

Hello Sumit, Mario,

On 11/5/25 12:38, 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@...dia.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;

I am not sure this is really useful to get the auto_sel value register 
in this function as:
- All of the other registers read are read-only
- The name of the function doesn't match: the autonomous selection is 
not really
related to perf. capabilities

I assume this change comes from the presence of the auto_sel register in the
'struct cppc_perf_caps', but IMO this register should be placed in 
another structure.

I assume this is ok to let it in 'struct cppc_perf_caps' for now, but I 
think we should not
fetch the value with all the other perf. capabilities values.


>   	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 forCPU:%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);

I think this is a bit out of the scope of this patchset, but I'm not 
sure this is necessary
to check the value of "osc_cpc_flexible_adr_space_confirmed" here. 
Indeed, acpi_cppc_processor_probe() already loops over the CPPC fields 
and should detect when a field is using an address space that is not 
allowed by "osc_cpc_flexible_adr_space_confirmed".

 From what I understand:

- osc_cpc_flexible_adr_space_confirmed was introduced to check that CPPC 
registers are in the correct address space

- this broke some amd platforms that didn't configure the _OSC method 
correctly

- 8b356e536e69 ("ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is 
supported") introduced cpc_supported_by_cpu() to bypass the check of 
osc_cpc_flexible_adr_space_confirmed. Indeed, the broken amd platforms 
don't configure the _OSC method, but it is possible to check if there is 
CPPC support by reading an MSR register.

- an amd platform failed to set the EPP register. This seems to be due 
to the EPP register being located in FFH and not in PCC. However the 
handler only supported PCC at that time: 7bc1fcd39901 ("ACPI: CPPC: Add 
AMD pstate energy performance preference cppc control") The bug report 
thread: bugzilla.kernel.org/show_bug.cgi?id=218686

- to allow setting the EPP value when it is located in FFH, the 
following patch was done: aaf21ac93909 ("ACPI: CPPC: Add support for 
setting EPP register in FFH") This patch seems to have added a check 
over the _OSC flexible bit value due to this comment: 
https://bugzilla.kernel.org/show_bug.cgi?id=218686#c83 However the CPPC 
registers are always allowed to be located in the FFH and PCC address 
space. Cf: 0651ab90e4ad ("ACPI: CPPC: Check _OSC for flexible address 
space")

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

Just to summarize, I think the check over 
osc_cpc_flexible_adr_space_confirmed could/should be removed. Ideally in 
a separate patch.

If Mario could confirm this is correct this would be nice.


> +	} 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;
> +			}
> +		}
>   	} 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");
> +
>   	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;
>   };
>   

Powered by blists - more mailing lists