Messages in this thread | | | From | Ruifeng Zhang <> | Date | Wed, 14 Apr 2021 19:26:48 +0800 | Subject | Re: [PATCH 1/1] arm: topology: parse the topology from the dt |
| |
Dietmar Eggemann <dietmar.eggemann@arm.com> 于2021年4月14日周三 下午5:43写道: > > 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> 于2021年4月13日周二 下午7:40写道: > >> > >> On 13/04/21 14:13, Ruifeng Zhang wrote: > >>> Valentin Schneider <valentin.schneider@arm.com> 于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()?
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 = <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?
| |