Re: [PATCH v2 04/21] ACPI: CPPC: add cppc enable register function

From: Fontenot, Nathan
Date: Tue Sep 28 2021 - 13:06:36 EST


On 9/26/2021 4:05 AM, Huang Rui wrote:
> From: Jinzhou Su <Jinzhou.Su@xxxxxxx>
>
> Add a new function to enable CPPC feature. This function
> will write Continuous Performance Control package
> EnableRegister field on the processor.
>
> Signed-off-by: Jinzhou Su <Jinzhou.Su@xxxxxxx>
> Signed-off-by: Huang Rui <ray.huang@xxxxxxx>
> ---
> drivers/acpi/cppc_acpi.c | 48 ++++++++++++++++++++++++++++++++++++++++
> include/acpi/cppc_acpi.h | 5 +++++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
> index 2efe2ba97d96..b285960c35e7 100644
> --- a/drivers/acpi/cppc_acpi.c
> +++ b/drivers/acpi/cppc_acpi.c
> @@ -1220,6 +1220,54 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
> }
> EXPORT_SYMBOL_GPL(cppc_get_perf_ctrs);
>
> +/**
> + * cppc_set_enable - Set to enable CPPC on the processor by writing the
> + * Continuous Performance Control package EnableRegister feild.
> + * @cpu: CPU for which to enable CPPC register.
> + * @enable: 0 - disable, 1 - enable CPPC feature on the processor.
> + *
> + * Return: 0 for success, -ERRNO or -EIO otherwise.
> + */
> +int cppc_set_enable(int cpu, u32 enable)

This should take a bool instead of u32 for enable, you can only enable
or diable cppc. The only caller I see is in patch 7/21 in which the
enable arg is already a bool that's converted to a u32. This also allows
for the removal of the enable value check.

You should consider merging this patch with patch 7/21. This patch adds
the cppc_set_enable() routine but has no users. The only caller I find is
in patch 7/21.

-Nathan

> +{
> + int pcc_ss_id = per_cpu(cpu_pcc_subspace_idx, cpu);
> + struct cpc_register_resource *enable_reg;
> + struct cpc_desc *cpc_desc = per_cpu(cpc_desc_ptr, cpu);
> + struct cppc_pcc_data *pcc_ss_data = NULL;
> + int ret = -1;
> +
> + /* check the input value*/
> + if (cpu < 0 || cpu > num_possible_cpus() - 1 || enable > 1)
> + return -ENODEV;
> +
> + if (!cpc_desc) {
> + pr_debug("No CPC descriptor for CPU:%d\n", cpu);
> + return -ENODEV;
> + }
> +
> + enable_reg = &cpc_desc->cpc_regs[ENABLE];
> +
> + if (CPC_IN_PCC(enable_reg)) {
> +
> + if (pcc_ss_id < 0)
> + return -EIO;
> +
> + ret = cpc_write(cpu, enable_reg, enable);
> + 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 platfrom */
> + ret = send_pcc_cmd(pcc_ss_id, CMD_WRITE);
> + up_write(&pcc_ss_data->pcc_lock);
> + }
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(cppc_set_enable);
> +
> /**
> * cppc_set_perf - Set a CPU's performance controls.
> * @cpu: CPU for which to set performance controls.
> diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
> index 9f4985b4d64d..3fdae40a75fc 100644
> --- a/include/acpi/cppc_acpi.h
> +++ b/include/acpi/cppc_acpi.h
> @@ -137,6 +137,7 @@ struct cppc_cpudata {
> extern int cppc_get_desired_perf(int cpunum, u64 *desired_perf);
> extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
> extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
> +extern int cppc_set_enable(int cpu, u32 enable);
> extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
> extern bool acpi_cpc_valid(void);
> extern int acpi_get_psd_map(unsigned int cpu, struct cppc_cpudata *cpu_data);
> @@ -157,6 +158,10 @@ static inline int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
> {
> return -ENOTSUPP;
> }
> +static inline int cppc_set_enable(int cpu, u32 enable)
> +{
> + return -ENOTSUPP;
> +}
> static inline int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps)
> {
> return -ENOTSUPP;
> --
> 2.25.1
>