> -----Original Message----- > From: Burakov, Anatoly <anatoly.bura...@intel.com> > Sent: Friday, April 23, 2021 7:38 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 11:02 AM, Richael Zhuang wrote: > > > > > >> -----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 > > > > Hi, > > FYI i've updated my refactor patch [1] so that you could base your work off it > and not have to do most of it yourself. Feel free to take over the patchset if > you like! > > [1] > http://patches.dpdk.org/project/dpdk/patch/83b1e89d14834251d4d7e72fcc > 19d82dfb52686d.1619175790.git.anatoly.bura...@intel.com/ > -- > Thanks, > Anatoly
Thanks. I will rework it and reduce some duplicated code in patch v2. Best Regards, Richael
