> -----Original Message----- > From: Burakov, Anatoly <anatoly.bura...@intel.com> > Sent: Thursday, April 22, 2021 6:00 PM > To: Richael Zhuang <richael.zhu...@arm.com>; dev@dpdk.org > Cc: nd <n...@arm.com>; David Hunt <david.h...@intel.com> > Subject: Re: [dpdk-dev] [PATCH v1 1/1] power: add support for cppc cpufreq > > On 22-Apr-21 10:29 AM, Richael Zhuang wrote: > > > > > >> -----Original Message----- > >> From: Burakov, Anatoly <anatoly.bura...@intel.com> > >> Sent: Thursday, April 22, 2021 5:06 PM > >> To: Richael Zhuang <richael.zhu...@arm.com>; dev@dpdk.org > >> Cc: nd <n...@arm.com>; David Hunt <david.h...@intel.com> > >> Subject: Re: [dpdk-dev] [PATCH v1 1/1] power: add support for cppc > >> cpufreq > >> > >> On 22-Apr-21 7:15 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.zhu...@arm.com> > >>> --- > >> > >> Just a general note: this looks like a copy-paste of pstate code. > >> Which is perfectly fine, except that we can do better than copying > >> some faults of the pstate code to other drivers. I've submitted a > >> patch [1] attempting to fix some of the pressing issues and code > >> duplication in pstate driver, but i'm sure with a fresh driver, you > >> can do even better :) > >> > >> [1] > >> http://patches.dpdk.org/project/dpdk/patch/20210402092701.258316-1- > >> anatoly.bura...@intel.com/ > >> > >> -- > >> Thanks, > >> Anatoly > > > > For CPPC is defined in acpi v5.0+ spec, I reused most code in acpi_cpufreq > to get a quick workable version on our platform with only cppc driver. I have > verified its basic functions. If you find some problems please help to point > out thus I can rework it. Thanks . > > > > Best Regards, > > Richael > > > > Well, pstate code was copied from ACPI so it does share the same flaws: > > - Lots of code duplication (e.g. snprintf for filename, fopen sequences, > etc.) > - Confusing and bug-prone error handling (e.g. return macros in the middle > of a function) > - Mixing power management logic and gory details of string handling > > Good examples of the above are in your `power_check_turbo()` function - > lots of string handling code interspersed with file opens, and actual logic of > power management. > > Please see the patch i linked earlier [1] to understand what kind of changes > i'm suggesting. Perhaps you could do even better :) > > [1] > http://patches.dpdk.org/project/dpdk/patch/20210402092701.258316-1- > anatoly.bura...@intel.com/ > > -- > Thanks, > Anatoly Thanks. I'll rework it to make it look better.
Best Regards, Richael
