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

[Richael Zhuang <Richael.Zhuang@arm.com>]

> -----Original Message-----
> From: David Hunt <david.hunt@intel.com>
> Sent: Wednesday, June 23, 2021 10:10 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> Cc: yux.jiang@intel.com
> Subject: Re: [PATCH v4 1/2] power: add support for cppc cpufreq
> 
> 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 :)
> 
Hi David,
Thanks! I will submit a new version soon.

> 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--