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