Re: [PATCH 1/3] ACPI: CPPC: Refactor register get and set ABIs

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

Hello Lifeng,

On 11/14/24 09:48, Lifeng Zheng wrote:
> Refactor register get and set ABIs using cppc_get_reg() and cppc_set_reg().
> 
> Rename cppc_get_perf() to cppc_get_reg() as a generic function to read cppc
> registers, with two changes:
> 
> 1. Change the error kind to "no such device" when pcc_ss_id < 0, which
> means that this cpu cannot get a valid pcc_ss_id.
> 
> 2. Add a check to verify if the register is a cpc supported one before
> using it.
> 
> Add cppc_set_reg() as a generic function for setting cppc registers. Unlike
> other set reg ABIs, this function checks CPC_SUPPORTED right after getting
> the register, because the rest of the operations are meaningless if this
> register is not a cpc supported one.
> 
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> ---
>   drivers/acpi/cppc_acpi.c | 191 +++++++++++++++------------------------
>   1 file changed, 72 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index c1f3568d0c50..306ced9c3376 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1179,10 +1179,13 @@ static int cpc_write(int cpu, struct cpc_register_resource *reg_res, u64 val)
>   	return ret_val;
>   }
>   
> -static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
> +static int cppc_get_reg(int cpunum, enum cppc_regs reg_idx, u64 *val)
>   {
>   	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpunum);
> +	struct cppc_pcc_data *pcc_ss_data = NULL;
>   	struct cpc_register_resource *reg;
> +	int pcc_ss_id;
> +	int ret = 0;

NIT: Might not be necessary if we save the value returned by cpc_read(),
cf. other comment below.

>   
>   	if (!cpc_desc) {
>   		pr_debug("No CPC descriptor for CPU:%d\n", cpunum);
> @@ -1191,20 +1194,23 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>   
>   	reg = &cpc_desc->cpc_regs[reg_idx];
>   
> +	if (!CPC_SUPPORTED(reg)) {
> +		pr_debug("CPC register (reg_idx=%u) is not supported\n", reg_idx);
> +		return -EOPNOTSUPP;
> +	}
> +
>   	if (CPC_IN_PCC(reg)) {
> -		int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
> -		struct cppc_pcc_data *pcc_ss_data = NULL;
> -		int ret = 0;
> +		pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpunum);
>   
>   		if (pcc_ss_id < 0)
> -			return -EIO;
> +			return -ENODEV;

NIT: Could add here:
   pr_debug("Invalid pcc_ss_id\n");
just as you did in cppc_set_reg()

>   
>   		pcc_ss_data = pcc_data[pcc_ss_id];
>   
>   		down_write(&pcc_ss_data->pcc_lock);
>   
>   		if (send_pcc_cmd(pcc_ss_id, CMD_READ) >= 0)
> -			cpc_read(cpunum, reg, perf);
> +			cpc_read(cpunum, reg, val);

This was not introduced by your patch, but cpc_read() return a value.
Shouldn't we return it instead of 0 ?

>   		else
>   			ret = -EIO;
>   
> @@ -1213,21 +1219,65 @@ static int cppc_get_perf(int cpunum, enum cppc_regs reg_idx, u64 *perf)
>   		return ret;
>   	}
>   
> -	cpc_read(cpunum, reg, perf);
> +	cpc_read(cpunum, reg, val);

Same comment as above

>   
>   	return 0;
>   }
>   
> +static int cppc_set_reg(int cpu, enum cppc_regs reg_idx, u64 val)

Just to have similar functions, maybe 'cpu' should be renamed to 'cpunum' ?
Or the other way around.

> +{
> +	struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> +	struct cppc_pcc_data *pcc_ss_data = NULL;
> +	struct cpc_register_resource *reg;
> +	int pcc_ss_id;
> +	int ret;
> +
> +	if (!cpc_desc) {
> +		pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> +		return -ENODEV;
> +	}
> +
> +	reg = &cpc_desc->cpc_regs[reg_idx];
> +
> +	if (!CPC_SUPPORTED(reg)) {
> +		pr_debug("CPC register (reg_idx=%u) is not supported\n", reg_idx);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (CPC_IN_PCC(reg)) {
> +		pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> +
> +		if (pcc_ss_id < 0) {
> +			pr_debug("Invalid pcc_ss_id\n");
> +			return -ENODEV;
> +		}
> +
> +		ret = cpc_write(cpu, reg, val);
> +		if (ret)
> +			return ret;
> +
> +		pcc_ss_data = pcc_data[pcc_ss_id];
> +
> +		down_write(&pcc_ss_data->pcc_lock);
> +		/* 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);
> +		return ret;
> +	}
> +
> +	return cpc_write(cpu, reg, val);
> +}
> +

[snip]