Messages in this thread | | | Date | Tue, 17 Apr 2018 11:24:24 +0100 | From | Lorenzo Pieralisi <> | Subject | Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver |
| |
On Tue, Apr 17, 2018 at 09:17:36AM +0200, Daniel Lezcano wrote:
[...]
> >>>> Actually there is no impact with the change Sudeep is referring to. It > >>>> is for ACPI, we are DT based. Confirmed with Jeremy. > >>>> > >>>> So AFAICT, it is not a problem. > >>> > >>> It is a problem - DT or ACPI alike. Sudeep was referring to the notion > >>> of "cluster" that has no architectural meaning whatsoever and using > >>> topology_physical_package_id() to detect a "cluster" was/is/will always > >>> be the wrong thing to do. The notion of cluster must not appear in the > >>> kernel at all, it has no architectural meaning. I understand you need > >>> to group CPUs but that has to be done in a different way, through > >>> cooling devices, thermal domains or power domains DT/ACPI bindings but > >>> not by using topology masks. > >> > >> I don't get it. What is the cluster concept defined in the ARM > >> documentation? > >> > >> ARM Cortex-A53 MPCore Processor Technical Reference Manual > >> > >> 4.5.2. Multiprocessor Affinity Register > >> > >> I see the documentation says: > >> > >> A cluster with two cores, three cores, ... > >> > >> How the kernel can represent that if you kill the > >> topology_physical_package_id() ? > > > > From an Arm ARM perspective (ARM v8 reference manual), the MPIDR_EL1 has > > no notion of cluster which means that a cluster is not architecturally > > defined on Arm systems. > > Sorry, I'm lost :/ You say the MPIDR_EL1 has no notion of cluster but > the documentation describing this register is all talking about cluster. > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0500g/BABHBJCI.html
I pointed you at the documentation I am referring to. You are referring to A53 TRM, I am referring to the Arm architecture reference manual that is the reference for all Arm cores.
> > Currently, as Morten explained today, topology_physical_package_id() > > is supposed to represent a "cluster" and that's completely wrong > > because a "cluster" cannot be defined from an architectural perspective. > > > > It was a bodge used as a shortcut, wrongly. We should have never used > > that API for that purpose and there must be no code in the kernel > > relying on: > > > > topology_physical_package_id() > > > > to define a cluster; the information you require to group CPUs must > > come from something else, which is firmware bindings(DT or ACPI) as > > I mentioned. > > Why not ?
I explained why not :). A cluster is not defined architecturally on Arm - it is as simple as that and you can't rely on a given MPIDR_EL1 subfield to define what a cluster id is.
> diff --git a/arch/arm64/include/asm/topology.h > b/arch/arm64/include/asm/topology.h > index c4f2d50..ac0776d 100644 > --- a/arch/arm64/include/asm/topology.h > +++ b/arch/arm64/include/asm/topology.h > @@ -14,7 +14,8 @@ struct cpu_topology { > > extern struct cpu_topology cpu_topology[NR_CPUS]; > > -#define topology_physical_package_id(cpu) > (cpu_topology[cpu].cluster_id) > +#define topology_physical_package_id(cpu) (0) > +#define topology_physical_cluster_id(cpu)
There is no such a thing (and there is no architecturally defined package id on Arm either).
> (cpu_topology[cpu].cluster_id) > #define topology_core_id(cpu) (cpu_topology[cpu].core_id) > #define topology_core_cpumask(cpu) (&cpu_topology[cpu].core_sibling) > #define topology_sibling_cpumask(cpu) (&cpu_topology[cpu].thread_sibling) > > > > Please speak to Sudeep who will fill you on the reasoning above. > > Yes, Sudeep is next to me but I would prefer to keep the discussion on > the mailing list so everyone can get the reasoning.
It is not a reasoning - it is the Arm architecture. There is no architecturally defined cluster id on Arm. The affinity bits in MPIDR_EL1 must be treated as a unique number that represents a given core/thread, how the bits are allocated across affinity levels is not something that you can rely on architecturally - that's why DT/ACPI topology bindings exist to group cpus in a hierarchical topology.
HTH, Lorenzo
| |