[PATCHv2 0/2] arm64 topology updates

[PATCHv2 0/2] arm64 topology updates

[Zi Shen Lim]

In this updated series, I've added Mark Brown's review-by to PATCH 1/2.

I've addressed Mark's comments for PATCH 2/2.
In order not to lose any information from MPIDR bits, we map all
higher affinity levels into "cluster". This approach is similar to
how Mark is handling multi-level clusters in DT.

Many thanks to Mark for review and comments!

Link to initial posting of this series:
    https://lkml.org/lkml/2014/4/22/950

Zi Shen Lim (2):
  arm64, sched: Remove unused mc_capable() and smt_capable()
  arm64: topology: add MPIDR-based detection

 arch/arm64/include/asm/cputype.h  |  2 ++
 arch/arm64/include/asm/topology.h |  3 ---
 arch/arm64/kernel/topology.c      | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 3 deletions(-)

-- 
1.8.4.3

[PATCHv2 1/2] arm64, sched: Remove unused mc_capable() and smt_capable()

[Zi Shen Lim]

Remove unused and deprecated mc_capable() and smt_capable().

Both were added recently by f6e763b93a6c ("arm64: topology:
Implement basic CPU topology support"). Uses of both were removed
by 8e7fbcbc22c1 ("sched: Remove stale power aware scheduling
remnants and dysfunctional knobs").

Reviewed-by: Mark Brown <broonie@linaro.org>
Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
v1->v2: No changes.
 - Added Mark's review-by.

 arch/arm64/include/asm/topology.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm64/include/asm/topology.h b/arch/arm64/include/asm/topology.h
index 0172e6d..7ebcd31 100644
--- a/arch/arm64/include/asm/topology.h
+++ b/arch/arm64/include/asm/topology.h
@@ -20,9 +20,6 @@ extern struct cpu_topology cpu_topology[NR_CPUS];
 #define topology_core_cpumask(cpu)	(&cpu_topology[cpu].core_sibling)
 #define topology_thread_cpumask(cpu)	(&cpu_topology[cpu].thread_sibling)
 
-#define mc_capable()	(cpu_topology[0].cluster_id != -1)
-#define smt_capable()	(cpu_topology[0].thread_id != -1)
-
 void init_cpu_topology(void);
 void store_cpu_topology(unsigned int cpuid);
 const struct cpumask *cpu_coregroup_mask(int cpu);
-- 
1.8.4.3

[PATCHv2 2/2] arm64: topology: add MPIDR-based detection

[Zi Shen Lim]

Create cpu topology based on MPIDR. When hardware sets MPIDR to sane
values, this method will always work. Therefore it should also work well
as the fallback method. [1]

When we have multiple processing elements in the system, we create
the cpu topology by mapping each affinity level (from lowest to highest)
to threads (if they exist), cores, and clusters.

We combine data from all higher affinity levels into cluster_id
so we don't lose any information from MPIDR. [2]

[1] http://www.spinics.net/lists/arm-kernel/msg317445.html
[2] https://lkml.org/lkml/2014/4/23/703

Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
---
v1->v2: Addressed comments from Mark Brown.
 - Reduce noise. Use pr_debug instead of pr_info.
 - Don't ignore higher affinity levels.

 arch/arm64/include/asm/cputype.h |  2 ++
 arch/arm64/kernel/topology.c     | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
index c404fb0..7639e8b 100644
--- a/arch/arm64/include/asm/cputype.h
+++ b/arch/arm64/include/asm/cputype.h
@@ -18,6 +18,8 @@
 
 #define INVALID_HWID		ULONG_MAX
 
+#define MPIDR_UP_BITMASK	(0x1 << 30)
+#define MPIDR_MT_BITMASK	(0x1 << 24)
 #define MPIDR_HWID_BITMASK	0xff00ffffff
 
 #define MPIDR_LEVEL_BITS_SHIFT	3
diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
index 3e06b0b..7dbf981 100644
--- a/arch/arm64/kernel/topology.c
+++ b/arch/arm64/kernel/topology.c
@@ -19,6 +19,8 @@
 #include <linux/nodemask.h>
 #include <linux/sched.h>
 
+#include <asm/cputype.h>
+#include <asm/smp_plat.h>
 #include <asm/topology.h>
 
 /*
@@ -71,6 +73,38 @@ static void update_siblings_masks(unsigned int cpuid)
 
 void store_cpu_topology(unsigned int cpuid)
 {
+	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
+	u64 mpidr;
+
+	mpidr = read_cpuid_mpidr();
+
+	/* Create cpu topology mapping based on MPIDR. */
+	if (mpidr & MPIDR_UP_BITMASK) {
+		/* Uniprocessor system */
+		cpuid_topo->thread_id  = -1;
+		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+		cpuid_topo->cluster_id = -1;
+	} else if (mpidr & MPIDR_MT_BITMASK) {
+		/* Multiprocessor system : Multi-threads per core */
+		cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+		cpuid_topo->cluster_id =
+			MPIDR_AFFINITY_LEVEL(mpidr, 2) |
+			MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];
+	} else {
+		/* Multiprocessor system : Single-thread per core */
+		cpuid_topo->thread_id  = -1;
+		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+		cpuid_topo->cluster_id =
+			MPIDR_AFFINITY_LEVEL(mpidr, 1) |
+			MPIDR_AFFINITY_LEVEL(mpidr, 2) << mpidr_hash.shift_aff[2] |
+			MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];
+	}
+
+	pr_debug("CPU%u: cluster %d core %d thread %d mpidr %llx\n",
+		 cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
+		 cpuid_topo->thread_id, mpidr);
+
 	update_siblings_masks(cpuid);
 }
 
-- 
1.8.4.3

[PATCHv2 2/2] arm64: topology: add MPIDR-based detection

[Lorenzo Pieralisi]

On Fri, Apr 25, 2014 at 04:18:42AM +0100, Zi Shen Lim wrote:
> Create cpu topology based on MPIDR. When hardware sets MPIDR to sane
> values, this method will always work. Therefore it should also work well
> as the fallback method. [1]

It has to be implemented as fallback, so you have to rebase this patch
on top of Mark's series.

> When we have multiple processing elements in the system, we create
> the cpu topology by mapping each affinity level (from lowest to highest)
> to threads (if they exist), cores, and clusters.
> 
> We combine data from all higher affinity levels into cluster_id
> so we don't lose any information from MPIDR. [2]
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg317445.html
> [2] https://lkml.org/lkml/2014/4/23/703
> 
> Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
> ---
> v1->v2: Addressed comments from Mark Brown.
>  - Reduce noise. Use pr_debug instead of pr_info.
>  - Don't ignore higher affinity levels.
> 
>  arch/arm64/include/asm/cputype.h |  2 ++
>  arch/arm64/kernel/topology.c     | 34 ++++++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
> index c404fb0..7639e8b 100644
> --- a/arch/arm64/include/asm/cputype.h
> +++ b/arch/arm64/include/asm/cputype.h
> @@ -18,6 +18,8 @@
>  
>  #define INVALID_HWID		ULONG_MAX
>  
> +#define MPIDR_UP_BITMASK	(0x1 << 30)
> +#define MPIDR_MT_BITMASK	(0x1 << 24)
>  #define MPIDR_HWID_BITMASK	0xff00ffffff
>  
>  #define MPIDR_LEVEL_BITS_SHIFT	3
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index 3e06b0b..7dbf981 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -19,6 +19,8 @@
>  #include <linux/nodemask.h>
>  #include <linux/sched.h>
>  
> +#include <asm/cputype.h>
> +#include <asm/smp_plat.h>
>  #include <asm/topology.h>
>  
>  /*
> @@ -71,6 +73,38 @@ static void update_siblings_masks(unsigned int cpuid)
>  
>  void store_cpu_topology(unsigned int cpuid)
>  {
> +	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> +	u64 mpidr;
> +
> +	mpidr = read_cpuid_mpidr();
> +
> +	/* Create cpu topology mapping based on MPIDR. */
> +	if (mpidr & MPIDR_UP_BITMASK) {
> +		/* Uniprocessor system */
> +		cpuid_topo->thread_id  = -1;
> +		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +		cpuid_topo->cluster_id = -1;
> +	} else if (mpidr & MPIDR_MT_BITMASK) {
> +		/* Multiprocessor system : Multi-threads per core */
> +		cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +		cpuid_topo->cluster_id =
> +			MPIDR_AFFINITY_LEVEL(mpidr, 2) |
> +			MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];

That's probably not what you want, even though you still end up with a
unique cluster identifier (but insanely large) if you get lucky and it
does not overflow an int. The shift is the amount of bits the level must be
shift _right_ to create the hash value.

I am wondering whether it is time for me to add those as macros.

> +	} else {
> +		/* Multiprocessor system : Single-thread per core */
> +		cpuid_topo->thread_id  = -1;
> +		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +		cpuid_topo->cluster_id =
> +			MPIDR_AFFINITY_LEVEL(mpidr, 1) |
> +			MPIDR_AFFINITY_LEVEL(mpidr, 2) << mpidr_hash.shift_aff[2] |
> +			MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];

Ditto.

> +	}
> +
> +	pr_debug("CPU%u: cluster %d core %d thread %d mpidr %llx\n",
> +		 cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
> +		 cpuid_topo->thread_id, mpidr);
> +
>  	update_siblings_masks(cpuid);

That's why I object. With this implementation MPIDR_EL1 takes over DT,
and we do not want that. It has to work the other way around.

What you should do, in update_sibling_masks(), check if the topology has
been reset (ie it is not set-up), and parse the MPIDR if that's the case.

Thanks,
Lorenzo

[PATCHv2 2/2] arm64: topology: add MPIDR-based detection

[Zi Shen Lim]

Hi Lorenzo,

On Fri, Apr 25, 2014 at 12:18:43PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Apr 25, 2014 at 04:18:42AM +0100, Zi Shen Lim wrote:
> > Create cpu topology based on MPIDR. When hardware sets MPIDR to sane
> > values, this method will always work. Therefore it should also work well
> > as the fallback method. [1]
> 
> It has to be implemented as fallback, so you have to rebase this patch
> on top of Mark's series.

Ok, I'll rebase this patch :)

> 
> > When we have multiple processing elements in the system, we create
> > the cpu topology by mapping each affinity level (from lowest to highest)
> > to threads (if they exist), cores, and clusters.
> > 
> > We combine data from all higher affinity levels into cluster_id
> > so we don't lose any information from MPIDR. [2]
> > 
> > [1] http://www.spinics.net/lists/arm-kernel/msg317445.html
> > [2] https://lkml.org/lkml/2014/4/23/703
> > 
> > Signed-off-by: Zi Shen Lim <zlim@broadcom.com>
> > ---
> > v1->v2: Addressed comments from Mark Brown.
> >  - Reduce noise. Use pr_debug instead of pr_info.
> >  - Don't ignore higher affinity levels.
> > 
> >  arch/arm64/include/asm/cputype.h |  2 ++
> >  arch/arm64/kernel/topology.c     | 34 ++++++++++++++++++++++++++++++++++
> >  2 files changed, 36 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/cputype.h b/arch/arm64/include/asm/cputype.h
> > index c404fb0..7639e8b 100644
> > --- a/arch/arm64/include/asm/cputype.h
> > +++ b/arch/arm64/include/asm/cputype.h
> > @@ -18,6 +18,8 @@
> >  
> >  #define INVALID_HWID		ULONG_MAX
> >  
> > +#define MPIDR_UP_BITMASK	(0x1 << 30)
> > +#define MPIDR_MT_BITMASK	(0x1 << 24)
> >  #define MPIDR_HWID_BITMASK	0xff00ffffff
> >  
> >  #define MPIDR_LEVEL_BITS_SHIFT	3
> > diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> > index 3e06b0b..7dbf981 100644
> > --- a/arch/arm64/kernel/topology.c
> > +++ b/arch/arm64/kernel/topology.c
> > @@ -19,6 +19,8 @@
> >  #include <linux/nodemask.h>
> >  #include <linux/sched.h>
> >  
> > +#include <asm/cputype.h>
> > +#include <asm/smp_plat.h>
> >  #include <asm/topology.h>
> >  
> >  /*
> > @@ -71,6 +73,38 @@ static void update_siblings_masks(unsigned int cpuid)
> >  
> >  void store_cpu_topology(unsigned int cpuid)
> >  {
> > +	struct cpu_topology *cpuid_topo = &cpu_topology[cpuid];
> > +	u64 mpidr;
> > +
> > +	mpidr = read_cpuid_mpidr();
> > +
> > +	/* Create cpu topology mapping based on MPIDR. */
> > +	if (mpidr & MPIDR_UP_BITMASK) {
> > +		/* Uniprocessor system */
> > +		cpuid_topo->thread_id  = -1;
> > +		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > +		cpuid_topo->cluster_id = -1;
> > +	} else if (mpidr & MPIDR_MT_BITMASK) {
> > +		/* Multiprocessor system : Multi-threads per core */
> > +		cpuid_topo->thread_id  = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > +		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> > +		cpuid_topo->cluster_id =
> > +			MPIDR_AFFINITY_LEVEL(mpidr, 2) |
> > +			MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];
> 
> That's probably not what you want, even though you still end up with a
> unique cluster identifier (but insanely large) if you get lucky and it
> does not overflow an int. The shift is the amount of bits the level must be
> shift _right_ to create the hash value.

Oh oops, I should read the mpidr_hash code more carefully.
My intention of using your mpidr_hash values is to achieve something better than
the naive solution of (aff2 | (aff3 << 8)).

> 
> I am wondering whether it is time for me to add those as macros.

Sure :)

> 
> > +	} else {
> > +		/* Multiprocessor system : Single-thread per core */
> > +		cpuid_topo->thread_id  = -1;
> > +		cpuid_topo->core_id    = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> > +		cpuid_topo->cluster_id =
> > +			MPIDR_AFFINITY_LEVEL(mpidr, 1) |
> > +			MPIDR_AFFINITY_LEVEL(mpidr, 2) << mpidr_hash.shift_aff[2] |
> > +			MPIDR_AFFINITY_LEVEL(mpidr, 3) << mpidr_hash.shift_aff[3];
> 
> Ditto.
> 
> > +	}
> > +
> > +	pr_debug("CPU%u: cluster %d core %d thread %d mpidr %llx\n",
> > +		 cpuid, cpuid_topo->cluster_id, cpuid_topo->core_id,
> > +		 cpuid_topo->thread_id, mpidr);
> > +
> >  	update_siblings_masks(cpuid);
> 
> That's why I object. With this implementation MPIDR_EL1 takes over DT,
> and we do not want that. It has to work the other way around.
> 
> What you should do, in update_sibling_masks(), check if the topology has
> been reset (ie it is not set-up), and parse the MPIDR if that's the case.

Ok, point taken. MPIDR as fallback.
In the rebased patch, I'll take care of it.

> 
> Thanks,
> Lorenzo
>