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

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

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Friday, April 23, 2021 7:38 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; David Hunt <david.hunt@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.burakov@intel.com>
> >> Sent: Thursday, April 22, 2021 6:00 PM
> >> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> >> Cc: nd <nd@arm.com>; David Hunt <david.hunt@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.burakov@intel.com>
> >>>> Sent: Thursday, April 22, 2021 5:06 PM
> >>>> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> >>>> Cc: nd <nd@arm.com>; David Hunt <david.hunt@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.zhuang@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.burakov@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.burakov@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.burakov@intel.com/
> --
> Thanks,
> Anatoly

Thanks. I will rework it and reduce some duplicated code in patch v2.

Best Regards,
Richael