Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210412070819.23493-1-ruifeng.zhang0110@gmail.com>
 <87y2dnn3gw.mognet@arm.com> <CAG7+-3MsjuChoEOj11VAMX9W61UY6MmphkxWDF=-_R1A8sfvpA@mail.gmail.com>
 <87tuobmsba.mognet@arm.com> <CAG7+-3OMYVTc+ja9CK+94X8XtKq3hbRb+XOEv7xOeuXgNm0BHQ@mail.gmail.com>
 <87o8eimmyw.mognet@arm.com> <CAG7+-3NaUAvjv9=9HZ4jQU=DVcZW6gRKZg9ZjutL3aKnnC4FLw@mail.gmail.com>
 <74b3ff57-473f-4d5a-daf8-ecbb0761abb2@arm.com>
In-Reply-To: <74b3ff57-473f-4d5a-daf8-ecbb0761abb2@arm.com>
From:   Ruifeng Zhang <ruifeng.zhang0110@gmail.com>
Date:   Wed, 14 Apr 2021 19:26:48 +0800
Message-ID: <CAG7+-3P6FYhqmt3p6g8b5ZXzaFW2pfx-vOZJYwuLCKXVqfFMVw@mail.gmail.com>
Subject: Re: [PATCH 1/1] arm: topology: parse the topology from the dt
To:     Dietmar Eggemann <dietmar.eggemann@arm.com>
Cc:     Valentin Schneider <valentin.schneider@arm.com>,
        linux@armlinux.org.uk, sudeep.holla@arm.com,
        Greg KH <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>, a.p.zijlstra@chello.nl,
        mingo@kernel.org, ruifeng.zhang1@unisoc.com, nianfu.bai@unisoc.com,
        linux-arm-kernel@lists.infradead.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Dietmar Eggemann <dietmar.eggemann@arm.com> =E4=BA=8E2021=E5=B9=B44=E6=9C=
=8814=E6=97=A5=E5=91=A8=E4=B8=89 =E4=B8=8B=E5=8D=885:43=E5=86=99=E9=81=93=
=EF=BC=9A
>
> 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@arm.com> =E4=BA=8E2021=E5=B9=B44=
=E6=9C=8813=E6=97=A5=E5=91=A8=E4=BA=8C =E4=B8=8B=E5=8D=887:40=E5=86=99=E9=
=81=93=EF=BC=9A
> >>
> >> On 13/04/21 14:13, Ruifeng Zhang wrote:
> >>> Valentin Schneider <valentin.schneider@arm.com> =E4=BA=8E2021=E5=B9=
=B44=E6=9C=8812=E6=97=A5=E5=91=A8=E4=B8=80 =E4=B8=8B=E5=8D=8811:33=E5=86=99=
=E9=81=93=EF=BC=9A
> >>>> 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 mapp=
ed to
> >>>>   AArch64 System register MPIDR_EL1[31:0].
> >>>>   """
> >>>>
> >>>> Peeking at some armv7 doc and arm/kernel/topology.c the layout reall=
y 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=3Den
> >>>
> >>
> >> Ah, so that's where the core_id=3Dbit[1:0] comes from. That does still
> >> conform to the MPIDR format, and as you point out below that's being p=
arsed
> >> the same (aff2, aff1, aff0) =3D=3D mpidr([23:16][15:8][7:0])
> >>
> >>> The current arch/arm/kernel/topology code parse the MPIDR with a armv=
7 format.
> >>> the parse code is:
> >>> void store_cpu_topology(unsigned int cpuid)
> >>> {
> >>>     ...
> >>>     cpuid_topo->thread_id =3D -1;
> >>>     cpuid_topo->core_id =3D MPIDR_AFFINITY_LEVEL(mpidr, 0);
> >>>     cpuid_topo->package_id =3D 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 anymor=
e since
> >>>>
> >>>>   3102bc0e6ac7 ("arm64: topology: Stop using MPIDR for topology info=
rmation")
> >>>>
> >>>> 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 inform=
ation")
> >>
> >> 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 entr=
y. I
> >> don't see any obvious problem with adding support for it, so if you ca=
n 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()?

Yes
>
> 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 =3D <1024>` for each CPU.

Yes, for capacity, the DT should have a capacity-dmips-MHz entry or a
clock-frequency entry (for A7 and A15 only).
The sc9863a dts is a vendor file,  in my opinion is not appropriate to
be update with this series.
What do you think if I independently update the sc9863a dts file later?