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

[Ruifeng Zhang]

Dietmar Eggemann <dietmar.eggemann@xxxxxxx> 于2021年4月16日周五 上午4:10写道：
>
> On 15/04/2021 20:09, Valentin Schneider wrote:
> > On 14/04/21 20:23, Ruifeng Zhang wrote:
> >> From: Ruifeng Zhang <ruifeng.zhang1@xxxxxxxxxx>
> >>
> >> In Unisoc, the sc9863a SoC which using cortex-a55, it has two software
> >> version, one of them is the kernel running on EL1 using aarch32.
> >>                 user(EL0)             kernel(EL1)
> >> sc9863a_go      aarch32               aarch32
> >> sc9863a         aarch64               aarch64
> >>
> >> When kernel runs on EL1 using aarch32, the topology will parse wrong.
> >> For example,
> >> The MPIDR has been written to the chip register in armv8.2 format.
> >> For example,
> >> core0: 0000000080000000
> >> core1: 0000000080000100
> >> core2: 0000000080000200
> >> ...
> >>
> >> It will parse to:
> >> |       | aff2 | packageid | coreid |
> >> |-------+------+-----------+--------|
> >> | Core0 |    0 |         0 |    0   |
> >> | Core1 |    0 |         1 |    0   |
> >> | Core2 |    0 |         2 |    0   |
> >> |  ...  |      |           |        |
> >>
> >> The wrong topology is that all of the coreid are 0 and unexpected
> >> packageid.
> >>
> >> The reason is the MPIDR format is different between armv7 and armv8.2.
> >> armv7 (A7) mpidr is:
> >> [11:8]      [7:2]       [1:0]
> >> cluster     reserved    cpu
> >> The cortex-a7 spec DDI0464F 4.3.5
> >> https://developer.arm.com/documentation/ddi0464/f/?lang=en
> >>
> >> armv8.2 (A55) mpidr is:
> >> [23:16]     [15:8]      [7:0]
> >> cluster     cpu         thread
> >>
> >
> > What I had understood from our conversation was that there *isn't* a format
> > difference (at least for the bottom 32 bits) - arm64/kernel/topopology.c
> > would parse it the same, except that MPIDR parsing has been deprecated for
> > arm64.

I agree, I should change my description.

> >
> > The problem is that those MPIDR values don't match the actual topology. If
> > they had the MT bit set, i.e.
> >
> >   core0: 0000000081000000
> >   core1: 0000000081000100
> >   core2: 0000000081000200
> >
> > then it would be parsed as:
> >
> >   |       | package_id | core_id | thread_id |
> >   |-------+------------+---------+-----------|
> >   | Core0 |          0 |       0 |         0 |
> >   | Core1 |          0 |       1 |         0 |
> >   | Core2 |          0 |       2 |         0 |
> >
> > which would make more sense (wrt the actual, physical topology).
>
> ... and this would be in sync with
> https://developer.arm.com/documentation/100442/0200/register-descriptions/aarch32-system-registers/mpidr--multiprocessor-affinity-register
>
> MT, [24]
>
>    0b1 ...
>
> There is no 0b0 for MT.
>
As you said, the MT must be 0b1, so the {aff1} means coreid for A55.
It's no problem for parsing coreid.

For more requirements, if all cores in one physical cluster, the
{aff2} of all cores are the same value.
i.e. the sc9863a,
core0: 0000000081000000
core1: 0000000081000100
core2: 0000000081000200
core3: 0000000081000300
core4: 0000000081000400
core5: 0000000081000500
core6: 0000000081000600
core7: 0000000081000700

According to MPIDR all cores will parse to the one cluster, but it's
the big.LITTLE system, it's need two logic cluster for schedule or
cpufreq.
So I think it's better to add the logic of parse topology from DT.