Re: [PATCH v1] power: support amd-pstate cpufreq driver

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

On 5/24/2023 7:46 PM, Thomas Monjalon wrote:
> 12/04/2023 11:52, Sivaprasad Tummala:
>> amd-pstate introduces a new CPU frequency control mechanism for AMD
>> processors using the ACPI Collaborative Performance Power Control
>> feature for a finer grained frequency management.
>>
>> Patch to add support for amd-pstate driver.
>>
>> Signed-off-by: Sivaprasad Tummala <Sivaprasad.Tummala@amd.com>
>> ---
>>   app/test/test_power.c                  |   1 +
>>   app/test/test_power_cpufreq.c          |   5 +-
>>   doc/guides/rel_notes/release_23_07.rst |   3 +
>>   examples/l3fwd-power/main.c            |   1 +
>>   lib/power/meson.build                  |   1 +
>>   lib/power/power_amd_pstate_cpufreq.c   | 698 +++++++++++++++++++++++++
>>   lib/power/power_amd_pstate_cpufreq.h   | 219 ++++++++
>>   lib/power/rte_power.c                  |  26 +
>>   lib/power/rte_power.h                  |   3 +-
>>   lib/power/rte_power_pmd_mgmt.c         |   6 +-
>>   10 files changed, 958 insertions(+), 5 deletions(-)
> 
> I'm not comfortable to merge this patch without a word from David Hunt.
> Given there is 0 review, what do we do?
> 
> 
>>        Also, make sure to start the actual text at the margin.
>>        =======================================================
>>   
>> +   * **Added amd-pstate driver support to power management library.**
>> +
>> +        Added support for amd-pstate driver which works on AMD Zen processors.
> 
> Looks like the indent is not correct.
> 
>>           'power_pstate_cpufreq.c',
>> +        'power_amd_pstate_cpufreq.c',
> 
> Can you say briefly why AMD has a different pstate?
> Does it mean power_pstate_cpufreq.c should be renamed
> power_intel_pstate_cpufreq.c?
> 
>> +++ b/lib/power/power_amd_pstate_cpufreq.c
>> @@ -0,0 +1,698 @@
>> +/* SPDX-License-Identifier: BSD-3-Clause
>> + * Copyright(c) 2010-2021 Intel Corporation
>> + * Copyright(c) 2021 Arm Limited
> 
> Why is there copyright for Intel and Arm?
> Does it mean you copied some code and did not try
> to keep common code in a common place?
> 

Yes, most of it appears to be copied from Intel pstate code, so the 
copyright would be correct here.

I don't know if we can merge these into one, to be honest - while a lot 
of it is shared (like, a lot), there are some subtle differences in code 
that might prevent us from sharing the code paths - joys of C.

-- 
Thanks,
Anatoly