Re: [PATCH 1/1] arm: topology: parse the topology from the dt

[Dietmar Eggemann]

On 13/04/2021 15:26, Ruifeng Zhang wrote:
> Thanks for your review. Patch-v2 that solve the capacity issue will be
> uploaded as soon as possible. : )
> 
> Valentin Schneider <valentin.schneider@xxxxxxx> 于2021年4月13日周二 下午7:40写道：
>>
>> On 13/04/21 14:13, Ruifeng Zhang wrote:
>>> Valentin Schneider <valentin.schneider@xxxxxxx> 于2021年4月12日周一 下午11:33写道：
>>>> I'm not fluent at all in armv7 (or most aarch32 compat mode stuff), but
>>>> I couldn't find anything about MPIDR format differences:
>>>>
>>>>   DDI 0487G.a G8.2.113
>>>>   """
>>>>   AArch32 System register MPIDR bits [31:0] are architecturally mapped to
>>>>   AArch64 System register MPIDR_EL1[31:0].
>>>>   """
>>>>
>>>> Peeking at some armv7 doc and arm/kernel/topology.c the layout really looks
>>>> just the same, i.e. for both of them, with your example of:
>>>
>>> The cortex-a7 spec DDI0464F 4.3.5
>>> https://developer.arm.com/documentation/ddi0464/f/?lang=en
>>>
>>
>> Ah, so that's where the core_id=bit[1:0] comes from. That does still
>> conform to the MPIDR format, and as you point out below that's being parsed
>> the same (aff2, aff1, aff0) == mpidr([23:16][15:8][7:0])
>>
>>> The current arch/arm/kernel/topology code parse the MPIDR with a armv7 format.
>>> the parse code is:
>>> void store_cpu_topology(unsigned int cpuid)
>>> {
>>>     ...
>>>     cpuid_topo->thread_id = -1;
>>>     cpuid_topo->core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>>     cpuid_topo->package_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>>     ...
>>> }
>>>>
>>>>   core0: 0000000080000000
>>>>   core1: 0000000080000100
>>>>   core2: 0000000080000200
>>>>   ...
>>>>
>>>> we'll get:
>>>>
>>>>   |       | aff2 | aff1 | aff0 |
>>>>   |-------+------+------+------|
>>>>   | Core0 |    0 |    0 |    0 |
>>>>   | Core1 |    0 |    1 |    0 |
>>>>   | Core2 |    0 |    2 |    0 |
>>>>       ...
>>>>
>>>> Now, arm64 doesn't fallback to MPIDR for topology information anymore since
>>>>
>>>>   3102bc0e6ac7 ("arm64: topology: Stop using MPIDR for topology information")
>>>>
>>>> so without DT we would get:
>>>>   |       | package_id | core_id |
>>>>   |-------+------------+---------|
>>>>   | Core0 |          0 |       0 |
>>>>   | Core1 |          0 |       1 |
>>>>   | Core2 |          0 |       2 |
>>>>
>>>> Whereas with an arm kernel we'll end up parsing MPIDR as:
>>>>   |       | package_id | core_id |
>>>>   |-------+------------+---------|
>>>>   | Core0 |          0 |       0 |
>>>>   | Core1 |          1 |       0 |
>>>>   | Core2 |          2 |       0 |
>>>>
>>>> Did I get this right? Is this what you're observing?
>>>
>>> Yes, this is a problem if an armv8.2 or above cpu is running a 32-bit
>>> kernel on EL1.
>>
>>
>> With the above MPIDR(_EL1) values, you would have the same problem in
>> aarch64 mode on any kernel predating
>>
>>   3102bc0e6ac7 ("arm64: topology: Stop using MPIDR for topology information")
>>
>> since all Aff0 values are 0. Arguably those MPIDR(_EL1) values don't
>> make much sense (cores in the same cluster should have different Aff0
>> values, unless SMT), but in arm64 that's usually "corrected" by DT.
>>
>> As you pointed out, arm doesn't currently leverage the cpu-map DT entry. I
>> don't see any obvious problem with adding support for it, so if you can fix
>> the capacity issue Dietmar reported, I think we could consider it.

Coming back to your original patch. You want to use parse_dt_topology()
from drivers/base/arch_topology.c to be able detect a cpu-map in dt and
so bypassing the read of mpidr in store_cpu_topology()?

Looks like sc9863a has two frequency domains (1.6 and 1.2GHz). So
technically it's a big.LITTLE system (based only on max CPU frequency
(not on uarch) differences).
But the dts file doesn't contain any `capacity-dmips-mhz` entries? So
asymmetric CPU capacity (even only based on max CPU frequency) detection
won't kick in. Since you don't have any uarch diffs, you would have to
specify `capacity-dmips-mhz = <1024>` for each CPU.