From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> To: Daniel Lezcano <daniel.lezcano@linaro.org> Cc: Viresh Kumar <viresh.kumar@linaro.org>, Sudeep Holla <sudeep.holla@arm.com>, edubezval@gmail.com, kevin.wangtao@linaro.org, leo.yan@linaro.org, vincent.guittot@linaro.org, linux-kernel@vger.kernel.org, javi.merino@kernel.org, rui.zhang@intel.com, daniel.thompson@linaro.org, linux-pm@vger.kernel.org, Amit Daniel Kachhap <amit.kachhap@gmail.com> Subject: Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver Date: Tue, 17 Apr 2018 11:24:24 +0100 [thread overview] Message-ID: <20180417102424.GA27279@e107981-ln.cambridge.arm.com> (raw) In-Reply-To: <845fafa3-f0ab-2050-fe32-1780dc61b8a8@linaro.org> 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
next prev parent reply other threads:[~2018-04-17 10:24 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-05 16:16 [PATCH v3 0/7] CPU cooling device new strategies Daniel Lezcano 2018-04-05 16:16 ` [PATCH v3 1/7] thermal/drivers/cpu_cooling: Fixup the header and copyright Daniel Lezcano [not found] ` <20180411061514.GL7671@vireshk-i7> 2018-04-11 8:56 ` Daniel Lezcano 2018-04-05 16:16 ` [PATCH v3 2/7] thermal/drivers/cpu_cooling: Add Software Package Data Exchange (SPDX) Daniel Lezcano 2018-04-05 16:16 ` [PATCH v3 3/7] thermal/drivers/cpu_cooling: Remove pointless field Daniel Lezcano 2018-04-05 16:16 ` [PATCH v3 4/7] thermal/drivers/Kconfig: Convert the CPU cooling device to a choice Daniel Lezcano [not found] ` <20180411061851.GM7671@vireshk-i7> 2018-04-11 8:58 ` Daniel Lezcano 2018-04-05 16:16 ` [PATCH v3 5/7] thermal/drivers/cpu_cooling: Add idle cooling device documentation Daniel Lezcano 2018-04-05 16:16 ` [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver Daniel Lezcano 2018-04-11 8:51 ` Viresh Kumar 2018-04-11 9:29 ` Daniel Lezcano 2018-04-13 11:23 ` Sudeep Holla 2018-04-13 11:47 ` Daniel Lezcano 2018-04-16 7:37 ` Viresh Kumar 2018-04-16 7:44 ` Daniel Lezcano 2018-04-16 9:34 ` Sudeep Holla 2018-04-16 9:37 ` Viresh Kumar 2018-04-16 9:45 ` Daniel Lezcano 2018-04-16 9:50 ` Viresh Kumar 2018-04-16 10:03 ` Daniel Lezcano 2018-04-16 10:10 ` Viresh Kumar 2018-04-16 12:10 ` Daniel Lezcano 2018-04-16 12:30 ` Lorenzo Pieralisi 2018-04-16 13:57 ` Daniel Lezcano 2018-04-16 14:22 ` Lorenzo Pieralisi 2018-04-17 7:17 ` Daniel Lezcano 2018-04-17 10:24 ` Lorenzo Pieralisi [this message] 2018-04-16 12:31 ` Sudeep Holla 2018-04-16 12:49 ` Daniel Lezcano 2018-04-16 13:03 ` Sudeep Holla 2018-04-16 12:29 ` Sudeep Holla 2018-04-13 11:38 ` Daniel Thompson 2018-04-13 11:46 ` Daniel Lezcano 2019-08-05 5:11 ` Martin Kepplinger 2019-08-05 6:53 ` Martin Kepplinger 2019-08-05 7:39 ` Daniel Lezcano 2019-08-05 7:42 ` Martin Kepplinger 2019-08-05 7:58 ` Daniel Lezcano 2019-10-25 11:22 ` Martin Kepplinger 2019-10-25 14:45 ` Daniel Lezcano 2019-10-26 18:23 ` Martin Kepplinger 2019-10-28 15:16 ` Daniel Lezcano 2019-08-05 7:37 ` Daniel Lezcano 2019-08-05 7:40 ` Martin Kepplinger 2018-04-05 16:16 ` [PATCH v3 7/7] cpuidle/drivers/cpuidle-arm: Register the cooling device Daniel Lezcano 2018-04-11 8:51 ` Viresh Kumar
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180417102424.GA27279@e107981-ln.cambridge.arm.com \ --to=lorenzo.pieralisi@arm.com \ --cc=amit.kachhap@gmail.com \ --cc=daniel.lezcano@linaro.org \ --cc=daniel.thompson@linaro.org \ --cc=edubezval@gmail.com \ --cc=javi.merino@kernel.org \ --cc=kevin.wangtao@linaro.org \ --cc=leo.yan@linaro.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=rui.zhang@intel.com \ --cc=sudeep.holla@arm.com \ --cc=vincent.guittot@linaro.org \ --cc=viresh.kumar@linaro.org \ --subject='Re: [PATCH v3 6/7] thermal/drivers/cpu_cooling: Introduce the cpu idle cooling driver' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).