Re: [dpdk-dev] [PATCH v4 1/2] power: add support for cppc cpufreq

[David Hunt <david.hunt@intel.com>]

Hi Richael

On 23/6/2021 4:55 AM, Richael Zhuang wrote:
> Currently in DPDK only acpi_cpufreq and pstate_cpufreq drivers are
> supported, which are both not available on arm64 platforms. Add
> support for cppc_cpufreq driver which works on most arm64 platforms.
>
> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
> ---
>   app/test/test_power.c          |   3 +-
>   app/test/test_power_cpufreq.c  |   3 +-
>   lib/power/meson.build          |   1 +
>   lib/power/power_cppc_cpufreq.c | 632 +++++++++++++++++++++++++++++++++
>   lib/power/power_cppc_cpufreq.h | 229 ++++++++++++
>   lib/power/rte_power.c          |  26 ++
>   lib/power/rte_power.h          |   2 +-
>   7 files changed, 893 insertions(+), 3 deletions(-)
>   create mode 100644 lib/power/power_cppc_cpufreq.c
>   create mode 100644 lib/power/power_cppc_cpufreq.h
>
> diff --git a/app/test/test_power.c b/app/test/test_power.c
> index da1d67c0a..b7b556134 100644
> --- a/app/test/test_power.c
> +++ b/app/test/test_power.c
> @@ -133,7 +133,8 @@ test_power(void)
>   	/* Perform tests for valid environments.*/
>   	const enum power_management_env envs[] = {PM_ENV_ACPI_CPUFREQ,
>   			PM_ENV_KVM_VM,
> -			PM_ENV_PSTATE_CPUFREQ};
> +			PM_ENV_PSTATE_CPUFREQ,
> +			PM_ENV_CPPC_CPUFREQ};
>   
>   	unsigned int i;
>   	for (i = 0; i < RTE_DIM(envs); ++i) {
> diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
> index 0c3adc5f3..8516df4ca 100644
> --- a/app/test/test_power_cpufreq.c
> +++ b/app/test/test_power_cpufreq.c
> @@ -496,7 +496,8 @@ test_power_cpufreq(void)
>   
>   	/* Test environment configuration */
>   	env = rte_power_get_env();
> -	if ((env != PM_ENV_ACPI_CPUFREQ) && (env != PM_ENV_PSTATE_CPUFREQ)) {
> +	if ((env != PM_ENV_ACPI_CPUFREQ) && (env != PM_ENV_PSTATE_CPUFREQ) &&
> +			(env != PM_ENV_CPPC_CPUFREQ)) {
>   		printf("Unexpectedly got an environment other than ACPI/PSTATE\n");
>   		goto fail_all;
>   	}
> diff --git a/lib/power/meson.build b/lib/power/meson.build
> index 74c5f3a29..4a5b07292 100644
> --- a/lib/power/meson.build
> +++ b/lib/power/meson.build
> @@ -21,6 +21,7 @@ sources = files(
>           'rte_power.c',
>           'rte_power_empty_poll.c',
>           'rte_power_pmd_mgmt.c',
> +	'power_cppc_cpufreq.c',
>   )
>   headers = files(
>           'rte_power.h',
> diff --git a/lib/power/power_cppc_cpufreq.c b/lib/power/power_cppc_cpufreq.c
> new file mode 100644
> index 000000000..fd4483e52
> --- /dev/null
> +++ b/lib/power/power_cppc_cpufreq.c
> @@ -0,0 +1,632 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2021 Arm Limited


Since this is a port of the acpi or p-state code for the cppc driver, I 
think you need to keep the Intel line in there, and then add in the Arm 
Line. Suggest adding in:

  * Copyright(c) 2010-2021 Intel Corporation


Also, (I know you're looking at this already), the FOPS* and FOPEN* 
macros need to be removed to bring in line with the latest (v6) patch 
for the cleanup work, a few people have been wanting to remove those 
macros for a while now :)

Once that's done, this patch looks good to me, although I can't give it 
a run as I don't have access to a suitable system. But visually, I've 
read through the patch and it seems fine to me.

Reviewed-by: David Hunt <david.hunt@intel.com>


--snip--