Re: [RFT/RFC PATCH v3 3/5] cpu-topology: Move cpu topology code to common code.

[Atish Patra <atish.patra@wdc.com>]

On 4/16/19 6:23 AM, Sudeep Holla wrote:
> On Mon, Apr 15, 2019 at 03:08:45PM -0700, Atish Patra wrote:
>> On 4/15/19 8:27 AM, Sudeep Holla wrote:
>>> Hi Atish,
>>>
>>> Thanks again for doing this. Overall changes look good except a couple
>>> of minor nit, see below.
>>>
>>> On Wed, Mar 20, 2019 at 04:48:04PM -0700, Atish Patra wrote:
>>>> Both RISC-V & ARM64 are using cpu-map device tree to describe
>>>> their cpu topology. It's better to move the relevant code to
>>>> a common place instead of duplicate code.
>>>>
>>>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>>> Tested-by: Jeffrey Hugo <jhugo@codeaurora.org>
>>>> ---
>>>>    arch/arm64/include/asm/topology.h |  23 ---
>>>>    arch/arm64/kernel/topology.c      | 303 +-----------------------------
>>>>    drivers/base/arch_topology.c      | 298 ++++++++++++++++++++++++++++-
>>>>    drivers/base/topology.c           |   1 +
>>>>    include/linux/arch_topology.h     |  28 +++
>>>>    5 files changed, 330 insertions(+), 323 deletions(-)
>>>>
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>>>> index edfcf8d9..6cc6a860 100644
>>>> --- a/drivers/base/arch_topology.c
>>>> +++ b/drivers/base/arch_topology.c
>>>> @@ -6,8 +6,8 @@
>>>>     * Written by: Juri Lelli, ARM Ltd.
>>>>     */
>>>> -#include <linux/acpi.h>
>>>>    #include <linux/arch_topology.h>
>>>> +#include <linux/acpi.h>
>>>>    #include <linux/cpu.h>
>>>>    #include <linux/cpufreq.h>
>>>>    #include <linux/device.h>
>>>> @@ -16,6 +16,11 @@
>>>>    #include <linux/string.h>
>>>>    #include <linux/sched/topology.h>
>>>>    #include <linux/cpuset.h>
>>>> +#include <linux/cpumask.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/percpu.h>
>>>> +#include <linux/sched.h>
>>>> +#include <linux/smp.h>
>>>>    DEFINE_PER_CPU(unsigned long, freq_scale) = SCHED_CAPACITY_SCALE;
>>>> @@ -278,3 +283,294 @@ static void parsing_done_workfn(struct work_struct *work)
>>>>    #else
>>>>    core_initcall(free_raw_capacity);
>>>>    #endif
>>>> +
>>>> +#if defined(CONFIG_ARM64) || defined(CONFIG_RISCV)
>>>
>>> Why can't the above one be just GENERIC_ARCH_TOPOLOGY ?
>>> I may be missing to find it myself, but would like to know.
>>>
>> GENERIC_ARCH_TOPOLOGY is now used for both RISCV, ARM & ARM64.
>> The below functions under this #ifdef have different implementation for ARM
>> and ARM64.
>>
>> parse_dt_topology
>> cpu_coregroup_mask
>> update_siblings_masks
>>
>> While we can combine the later two functions and move them to common code as
>> well, parse_dt_topology is significantly different.
>>
> 
> Sure, had a quick glance and indeed they may look different, but won't
> it defeat the purpose of this binding consolidation ?
> 
I didn't want change too much at first go.

>> That's why we need some kind of #ifdef or renaming of parse_dt_topology for
>> ARM32 code.
>>
> 
> I am fine if we want to take this up later to keep the impact minimum.
> But cpu_coregroup_mask and update_siblings_masks can and must be unified.

Sure. I will just leave parse_dt_topology as it is for now and unify 
other two functions.

I think we should unify parse_dt_topology in separate series.

Regards,
Atish
> In fact the existing generic version must work on ARM32 too.
> 
>> Thanks for the review!!
>>
> 
> You are welcome.
> 
> --
> Regards,
> Sudeep
>