linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  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).