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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

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/83b1e89d14834251d4d7e72fcc19d82dfb52686d.1619175790.git.anatoly.burakov@intel.com/
-- 
Thanks,
Anatoly