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

[Ionela Voinescu <ionela.voinescu@arm.com>]

Hi,

A small nit that applies to multiple places: let's keep the line length
under 80 characters - the lines seem easy to split.

On Wednesday 05 Nov 2025 at 17:08:39 (+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>
> ---
>  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;
>  	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 for CPU:%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);
> +	} 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;
> +			}

Wouldn't it be more clear to have separate functions for setting auto-sel
and EPP? I think this is functionally correct, but somewhat unclear, given
the signature of the function. But I do acknowledge that the function was
like this to begin with.

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

This message needs updating.

Thank you,
Ionela.
> +
>  	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;
>  };
>  
> -- 
> 2.34.1
>