ARM: introducing DT topology

In order to set up a proper logical to per-cpu interrupt controller IF mapping, the GIC interrupt controller device tree bindings must be enhanced to define the CPU IF id for all present CPUs.
GIC CPU IF ids are needed to send interprocessor IPIs and to set affinity levels. Since the way CPU IF ids are wired depends on the specific system design, they cannot be extrapolated or probed in HW by the boot CPU, so a binding to define them is needed to set-up the system properly.

This patch adds a logical map of per-cpu interrupt controller identifiers. The newly introduced per-cpu IF map has to be initialized by the GIC driver so that interprocessor interrupts and affinity levels can be set accordingly in an SMP system, with a proper 1:1 relation between per-cpu IF ids and logical cpu indexes.

This patch adds a function that parses the device tree properties and initializes the cpu interfaces ids properly according to the latest GIC device tree bindings.

If CONFIG_OF is not enabled, per-cpu CPU IF mappings are defined as cpu_logical_map(), leaving the current functionality unchanged.

The GIC device tree bindings documentation is updated by the patch accordingly.

Signed-off-by Lorenzo Pieralisi <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Will Deacon <[email protected]>

Cc Russell King <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

Cc Grant Likely <[email protected]>

Cc Rob Herring <[email protected]>

Cc Vincent Guittot <[email protected]>

---
 Documentation/devicetree/bindings/arm/gic.txt |   69 ++++++++++++++++++
 arch/arm/common/gic.c                         |   94 +++++++++++++++++++++++-
 2 files changed, 159 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/arm/gic.txt +69 -0

index 9b4b82a..da2f6a8 100644
@@ -57,3 +57,72 @@ Example:
 		      <0xfff10100 0x100>;
 	};
 
+* ARM Generic Interrupt Controller CPU Interfaces
+
+ARM GIC device tree nodes contain subnodes representing its CPU interfaces.
+
+The main properties required by CPU interface nodes are:
+
+- compatible	: "arm,gic-cpuif"
+- cpuif-id	: specifies the CPU IF HW identifier
+- cpu		: a phandle to the respective CPU node
+
+Example:
+
+	cpus {
+		#size-cells = <0>;
+		#address-cells = <1>;
+
+		CPU0: cpu at 0x0 {
+			device_type = "cpu";
+			reg = <0x0>;
+		};
+
+		CPU1: cpu at 0x1 {
+			device_type = "cpu";
+			reg = <0x1>;
+		};
+
+		CPU2: cpu at 0x100 {
+			device_type = "cpu";
+			reg = <0x100>;
+		};
+
+		CPU3: cpu at 0x101 {
+			device_type = "cpu";
+			reg = <0x101>;
+		};
+	};
+
+	intc: interrupt-controller at fff11000 {
+		compatible = "arm,cortex-a9-gic";
+		#interrupt-cells = <3>;
+		#address-cells = <1>;
+		interrupt-controller;
+		reg = <0xfff11000 0x1000>,
+		      <0xfff10100 0x100>;
+
+			gic-cpuif at 0x0 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x0>;
+				cpu = <&CPU0>;
+			};
+
+			gic-cpuif at 0x1 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x1>;
+				cpu = <&CPU1>;
+			};
+
+			gic-cpuif at 0x2 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x2>;
+				cpu = <&CPU2>;
+			};
+
+			gic-cpuif at 0x3 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x3>;
+				cpu = <&CPU3>;
+			};
+	};

arch/arm/common/gic.c +90 -4

index c47d619..4be754d 100644
@@ -140,6 +140,83 @@ static inline unsigned int gic_irq(struct irq_data *d)
 	return d->hwirq;
 }
 
+#ifdef CONFIG_OF
+static u32 gic_cpuif_logical_map[NR_CPUS];
+#define cpuif_logical_map(cpu)	gic_cpuif_logical_map[cpu]
+
+/*
+ * Create a mapping of GIC CPU IF numbers to logical cpus through the device
+ * tree. GIC CPU IF are linked to the respective cpu nodes through the "cpu"
+ * phandle.
+ */
+static void __init gic_init_if_maps(struct gic_chip_data *gic)
+{
+	struct device_node *ncpu, *gic_cpuif;
+	struct irq_domain *domain = &gic->domain;
+	int i;
+
+	if (WARN_ON(!domain || !domain->of_node))
+		return;
+
+	for_each_child_of_node(domain->of_node, gic_cpuif) {
+		const u32 *cpuif_hwid, *mpidr;
+		int len;
+
+		if (!of_device_is_compatible(gic_cpuif, "arm,gic-cpuif"))
+			continue;
+
+		pr_debug("  * %s...\n", gic_cpuif->full_name);
+
+		ncpu = of_parse_phandle(gic_cpuif, "cpu", 0);
+
+		if (!ncpu) {
+			pr_err("  * %s missing cpu phandle\n",
+						gic_cpuif->full_name);
+			continue;
+		}
+
+		mpidr = of_get_property(ncpu, "reg", &len);
+
+		if (!mpidr || len != 4) {
+			pr_err("  * %s missing reg property\n",
+					ncpu->full_name);
+			continue;
+		}
+
+		cpuif_hwid = of_get_property(gic_cpuif, "cpuif-id", &len);
+
+		if (!cpuif_hwid || len != 4) {
+			pr_err("  * %s missing cpuif-id property\n",
+						gic_cpuif->full_name);
+			continue;
+		}
+
+		/*
+		 * Do the logical enumeration once in arm_dt_init_cpu_maps and
+		 * use it again here to avoid logical numbering mix-ups between
+		 * cpu and interrupt controller ids
+		 */
+
+		i = get_logical_index(be32_to_cpup(mpidr));
+
+		if (i < 0) {
+			pr_err("  * %s mpidr mismatch\n",
+						ncpu->full_name);
+			continue;
+		}
+
+		if (!i)
+			printk(KERN_INFO "Booting Linux on GIC CPU IF 0x%x\n",
+					be32_to_cpup(cpuif_hwid));
+
+		cpuif_logical_map(i) = be32_to_cpup(cpuif_hwid);
+	}
+}
+#else
+static inline void gic_init_if_maps(struct gic_chip_data *gic) {}
+#define cpuif_logical_map(cpu)	cpu_logical_map(cpu)
+#endif
+
 /*
  * Routines to acknowledge, disable and enable interrupts
  */

@@ -245,7 +322,7 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 		return -EINVAL;
 
 	mask = 0xff << shift;
-	bit = 1 << (cpu_logical_map(cpu) + shift);
+	bit = 1 << (cpuif_logical_map(cpu) + shift);
 
 	raw_spin_lock(&irq_controller_lock);
 	val = readl_relaxed(reg) & ~mask;

@@ -353,7 +430,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	unsigned int gic_irqs = gic->gic_irqs;
 	struct irq_domain *domain = &gic->domain;
 	void __iomem *base = gic_data_dist_base(gic);
-	u32 cpu = cpu_logical_map(smp_processor_id());
+	u32 cpu = cpuif_logical_map(smp_processor_id());
 
 	cpumask = 1 << cpu;
 	cpumask |= cpumask << 8;

@@ -659,6 +736,14 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 
 	gic = &gic_data[gic_nr];
 	domain = &gic->domain;
+
+	/*
+	 * create the logical/physical mapping just for the
+	 * primary GIC
+	 */
+	if (gic_nr == 0)
+		gic_init_if_maps(gic);
+
 #ifdef CONFIG_GIC_NON_BANKED
 	if (percpu_offset) { /* Frankein-GIC without banked registers... */
 		unsigned int cpu;

@@ -673,7 +758,8 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 		}
 
 		for_each_possible_cpu(cpu) {
-			unsigned long offset = percpu_offset * cpu_logical_map(cpu);
+			unsigned long offset =
+				percpu_offset * cpuif_logical_map(cpu);
 			*per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
 			*per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
 		}

@@ -746,7 +832,7 @@ void gic_raise_softirq(const struct cpumask *mask, unsigned int irq)
 
 	/* Convert our logical CPU mask into a physical one. */
 	for_each_cpu(cpu, mask)
-		map |= 1 << cpu_logical_map(cpu);
+		map |= 1 << cpuif_logical_map(cpu);
 
 	/*
 	 * Ensure that stores to Normal memory are visible to the

Quoted 18 lines · Lorenzo Pieralisi

In order to set up a proper logical to per-cpu interrupt controller IF mapping, the GIC interrupt controller device tree bindings must be enhanced to define the CPU IF id for all present CPUs.
GIC CPU IF ids are needed to send interprocessor IPIs and to set affinity levels. Since the way CPU IF ids are wired depends on the specific system design, they cannot be extrapolated or probed in HW by the boot CPU, so a binding to define them is needed to set-up the system properly.

This patch adds a logical map of per-cpu interrupt controller identifiers. The newly introduced per-cpu IF map has to be initialized by the GIC driver so that interprocessor interrupts and affinity levels can be set accordingly in an SMP system, with a proper 1:1 relation between per-cpu IF ids and logical cpu indexes.

This patch adds a function that parses the device tree properties and initializes the cpu interfaces ids properly according to the latest GIC device tree bindings.

Quoted 3 lines

If CONFIG_OF is not enabled, per-cpu CPU IF mappings are defined as cpu_logical_map(), leaving the current functionality unchanged.

What if CONFIG_OF is enabled, but the DTB is not updated? That needs to work.

I really don't get this. Furthermore, DT bindings are not supposed to evolve. They need to be complete enough that they don't need frequent updates for existing h/w. We discussed GIC bindings at length for 3+ months. The binding had sub nodes for cpu interfaces at one point, but got rid of them. Then support for cpu interfaces at different addresses was added. Those times were the time to bring-up issues like this. We can't keep changing the binding.

The introduction of multi-cluster ARM systems in SoC designs requires the kernel to become cluster aware, so that it can be booted on every CPU in the system and it can build an appropriate representation of topology levels.

Current code in the kernel, in particular the boot sequence, hinges upon a sequential mapping of MPIDR values for cpus and related interrupt controller CPU interfaces to logical cpu indexing.
This hypothesis is not valid when the concept of cluster is introduced since the MPIDR cannot be represented as a single index and interrupt controller CPU interfaces might be wired with a numbering scheme following per-SoC design parameters which cannot be extrapolated easily through generic functions by the primary CPU.

Furthermore, relying on the MPIDR to be wired according to real topology levels might turn out to be an unreliable solution, hence a SW representation is needed to override possibly incorrect MPIDR register values.

Through the device tree and relative bindings, the kernel is provided with HW values for MPIDR registers, interrupt controller CPU interfaces and with a topology representation of hierarchy levels and connection between cores.

The device tree bindings allow to boot the Linux kernel on a multi-cluster system without relying on platform specific hook to identify the number of CPUs and interrupt controller wiring definition.

The patchset has been tested against:

git://git.kernel.org/pub/scm/linux/kernel/git/cmarinas/linux-arm-arch.git master

to boot an 8-core dual-cluster system on every possible CPU and to describe the relative cpu topology according to the affinity levels.

Compile tested against linux-next, with a dependency on the following patch:

http://www.spinics.net/lists/arm-kernel/msg155873.html

Documentation of new device tree bindings is provided in the corresponding patches and provided for discussion.

Lorenzo Pieralisi (5):
ARM: kernel: add device tree init map function
ARM: kernel: add cpu logical map DT init in setup_arch
ARM: kernel: add logical mappings look-up
ARM: gic: add cpuif topology description
ARM: kernel: build CPU topology from DT

 Documentation/devicetree/bindings/arm/cpus.txt     |   42 +++++
 Documentation/devicetree/bindings/arm/gic.txt      |   69 ++++++++
 Documentation/devicetree/bindings/arm/topology.txt |  167 ++++++++++++++++++++
 arch/arm/common/gic.c                              |   94 +++++++++++-
 arch/arm/include/asm/prom.h                        |    2 +
 arch/arm/include/asm/smp_plat.h                    |   12 ++
 arch/arm/kernel/devtree.c                          |   40 +++++
 arch/arm/kernel/setup.c                            |    1 +
 arch/arm/kernel/topology.c                         |  110 +++++++++++---
 9 files changed, 513 insertions(+), 24 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/cpus.txt
 create mode 100644 Documentation/devicetree/bindings/arm/topology.txt

Quoted 13 lines · Lorenzo Pieralisi

The introduction of multi-cluster ARM systems in SoC designs requires the kernel to become cluster aware, so that it can be booted on every CPU in the system and it can build an appropriate representation of topology levels.

Current code in the kernel, in particular the boot sequence, hinges upon a sequential mapping of MPIDR values for cpus and related interrupt controller CPU interfaces to logical cpu indexing.
This hypothesis is not valid when the concept of cluster is introduced since the MPIDR cannot be represented as a single index and interrupt controller CPU interfaces might be wired with a numbering scheme following per-SoC design parameters which cannot be extrapolated easily through generic functions by the primary CPU.

Quoted 4 lines

Furthermore, relying on the MPIDR to be wired according to real topology levels might turn out to be an unreliable solution, hence a SW representation is needed to override possibly incorrect MPIDR register values.

"might be" is used several times. Is this a real problem? Wouldn't a more simple solution be providing properties to override the MPIDR register value if it is unreliable?

Current code in the kernel, in particular the boot sequence, hinges upon a sequential mapping of MPIDR values for cpus and related interrupt controller CPU interfaces to logical cpu indexing.

This hypothesis is not valid when the concept of cluster is introduced since the MPIDR cannot be represented as a single index and interrupt controller CPU interfaces might be wired with a numbering scheme following per-SoC design parameters which cannot be extrapolated easily through generic functions by the primary CPU.

Furthermore, relying on the MPIDR to be wired according to real topology levels might turn out to be an unreliable solution, hence a SW representation is needed to override possibly incorrect MPIDR register values.

Quoted 3 lines · Lorenzo Pieralisi

Current code in the kernel, in particular the boot sequence, hinges upon a sequential mapping of MPIDR values for cpus and related interrupt controller CPU interfaces to logical cpu indexing.

I don't believe it does. What it does rely upon is having cpu_logical_map() correctly setup for the logical-to-hardware mapping - which, if it's different, should be done in smp_init_cpus(), taking note that logical CPU 0 is _always_ the boot CPU. (That's a restriction caused by the way suspend/resume unplugs all but the lowest numbered logical online CPU.)

So, with the code today, there's nothing in the code which prevents this from happening:

a) you boot on h/w CPU 1, which becomes logical CPU 0.
b) you boot h/w CPU 3 as logical CPU 1.
c) you boot h/w CPU 0 as logical CPU 2.
d) you boot h/w CPU 2 as logical CPU 3.

You just need to ensure that cpu_logical_map() contains the array {1, 3, 0, 2} instead of the default (because we _have_ to have a default) of {1, 0, 2, 3}.

Quoted 5 lines

This hypothesis is not valid when the concept of cluster is introduced since the MPIDR cannot be represented as a single index and interrupt controller CPU interfaces might be wired with a numbering scheme following per-SoC design parameters which cannot be extrapolated easily through generic functions by the primary CPU.

So what you're saying is that the GIC CPU index may not be the CPU number given by MPIDR?

Quoted 4 lines

Furthermore, relying on the MPIDR to be wired according to real topology levels might turn out to be an unreliable solution, hence a SW representation is needed to override possibly incorrect MPIDR register values.

This sounds like you're saying that the contents of MPIDR might be buggy sometime in the future? Do we actually know of any situations where the information in there is currently wrong (outside of the development lab)? If not, it's not something we should cater for until it's actually happened, and then the pain should be felt by those silly enough to allow the chip to go out the door.

I agree with you Russell, that's 100% valid on a single cluster. But on a multi-cluster (eg dual-cluster) the MPIDR might be wired like this:

MPIDR[15:8] - cluster id
MPIDR[7:0] - core id

no hyperthreading

* CLUSTER 0 *

                       MPIDR[23:16]   MPIDR[15:8]    MPIDR[7:0]

HWCPU0: MPIDR=0x0 0x0 0x0 0x0
HWCPU1: MPIDR=0x1 0x0 0x0 0x1

* CLUSTER 1 *

HWCPU2: MPIDR=0x100 0x0 0x1 0x0
HWCPU3: MPIDR=0x101 0x0 0x1 0x1

MPIDR is not a sequential index anymore, that's what I am going on about.

And yes, code like cpu_resume, that relies on MPIDR[7:0] to be unique needs patching, since that just takes into account the first affinity level, which can have same values for different CPUs in the system if they belong to different clusters.

Quoted 8 lines

Quoted 5 lines

This hypothesis is not valid when the concept of cluster is introduced since the MPIDR cannot be represented as a single index and interrupt controller CPU interfaces might be wired with a numbering scheme following per-SoC design parameters which cannot be extrapolated easily through generic functions by the primary CPU.

So what you're saying is that the GIC CPU index may not be the CPU number given by MPIDR?

Yes, that's correct. Taking the same example as above:

MPIDR[15:8] - cluster id
MPIDR[7:0] - core id

no hyperthreading

* CLUSTER 0 *

                         MPIDR[15:8]    MPIDR[7:0]  GIC-CPU-ID

HWCPU0: MPIDR=0x0 0x0 0x0 0x0
HWCPU1: MPIDR=0x1 0x0 0x1 0x1

* CLUSTER 1 *

HWCPU2: MPIDR=0x100 0x1 0x0 0x2
HWCPU3: MPIDR=0x101 0x1 0x1 0x3

There is just one GIC distributor shared across all clusters.

Quoted 5 lines

Quoted 4 lines

Furthermore, relying on the MPIDR to be wired according to real topology levels might turn out to be an unreliable solution, hence a SW representation is needed to override possibly incorrect MPIDR register values.

Quoted 6 lines

This sounds like you're saying that the contents of MPIDR might be buggy sometime in the future? Do we actually know of any situations where the information in there is currently wrong (outside of the development lab)? If not, it's not something we should cater for until it's actually happened, and then the pain should be felt by those silly enough to allow the chip to go out the door.

I share your view Russell. Having said that: MPIDR is IMPLEMENTATION DEFINED.

Quoted 18 lines · Lorenzo Pieralisi

I agree with you Russell, that's 100% valid on a single cluster. But on a multi-cluster (eg dual-cluster) the MPIDR might be wired like this:

MPIDR[15:8] - cluster id
MPIDR[7:0] - core id

no hyperthreading

* CLUSTER 0 *

                       MPIDR[23:16]   MPIDR[15:8]    MPIDR[7:0]

HWCPU0: MPIDR=0x0 0x0 0x0 0x0
HWCPU1: MPIDR=0x1 0x0 0x0 0x1

* CLUSTER 1 *

HWCPU2: MPIDR=0x100 0x0 0x1 0x0
HWCPU3: MPIDR=0x101 0x0 0x1 0x1

Quoted 1 line

MPIDR is not a sequential index anymore, that's what I am going on about.

And that's no problem to the boot code. Assuming cluster 0 CPU 0 is the boot CPU, then:

cpu_logical_map() would need to be initialized as {0x000, 0x001, 0x100, 0x101}.

Quoted 4 lines

And yes, code like cpu_resume, that relies on MPIDR[7:0] to be unique needs patching, since that just takes into account the first affinity level, which can have same values for different CPUs in the system if they belong to different clusters.

That's going to be very painful to deal with, because of the restricted environment we have there. I guess we need to build some kind of reverse table in memory somewhere which contains MPIDR mask+MPIDR value+ CPU array index. Not nice at all.

Quoted 27 lines

Quoted 8 lines

Quoted 5 lines

This hypothesis is not valid when the concept of cluster is introduced since the MPIDR cannot be represented as a single index and interrupt controller CPU interfaces might be wired with a numbering scheme following per-SoC design parameters which cannot be extrapolated easily through generic functions by the primary CPU.

So what you're saying is that the GIC CPU index may not be the CPU number given by MPIDR?

Yes, that's correct. Taking the same example as above:

MPIDR[15:8] - cluster id
MPIDR[7:0] - core id

no hyperthreading

* CLUSTER 0 *

                         MPIDR[15:8]    MPIDR[7:0]  GIC-CPU-ID

HWCPU0: MPIDR=0x0 0x0 0x0 0x0
HWCPU1: MPIDR=0x1 0x0 0x1 0x1

* CLUSTER 1 *

HWCPU2: MPIDR=0x100 0x1 0x0 0x2
HWCPU3: MPIDR=0x101 0x1 0x1 0x3

Quoted 1 line

There is just one GIC distributor shared across all clusters.

Right, so what we need is a kernel logical CPU id to GIC id mapping, which you have in your patch. You call it cpuif_logical_map() but as it's specific to GIC, I wonder if it would be better called gic_logical_map().

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Furthermore, relying on the MPIDR to be wired according to real topology levels might turn out to be an unreliable solution, hence a SW representation is needed to override possibly incorrect MPIDR register values.

Quoted 8 lines

Quoted 6 lines

This sounds like you're saying that the contents of MPIDR might be buggy sometime in the future? Do we actually know of any situations where the information in there is currently wrong (outside of the development lab)? If not, it's not something we should cater for until it's actually happened, and then the pain should be felt by those silly enough to allow the chip to go out the door.

I share your view Russell. Having said that: MPIDR is IMPLEMENTATION DEFINED.

I'll assume you mean that it's left to the implementation to set MPIDR appropriately, and you're expecting implementations to make mistakes with it.

Tell me: would you tolerate people making mistakes when they implement an ARM CPU which results in the ISA almost being followed except some op-codes having bits switched?

If not, then why would you tolerate wrong MPIDR values? That's precisely the same kind of bug. And if we go adding work-arounds now, before there's a problem, there will be no incentive for people to fix the hardware bugs during their initial implementation testing.

Quoted 7 lines · Lorenzo Pieralisi

Quoted 6 lines

This sounds like you're saying that the contents of MPIDR might be buggy sometime in the future? Do we actually know of any situations where the information in there is currently wrong (outside of the development lab)? If not, it's not something we should cater for until it's actually happened, and then the pain should be felt by those silly enough to allow the chip to go out the door.

Quoted 1 line

I share your view Russell. Having said that: MPIDR is IMPLEMENTATION DEFINED.

I'll assume you mean that it's left to the implementation to set MPIDR appropriately, and you're expecting implementations to make mistakes with it.

Quoted 8 lines · Russell King - ARM Linux

Quoted 7 lines · Lorenzo Pieralisi

Quoted 6 lines

This sounds like you're saying that the contents of MPIDR might be buggy sometime in the future? Do we actually know of any situations where the information in there is currently wrong (outside of the development lab)? If not, it's not something we should cater for until it's actually happened, and then the pain should be felt by those silly enough to allow the chip to go out the door.

Quoted 5 lines

Quoted 1 line

I share your view Russell. Having said that: MPIDR is IMPLEMENTATION DEFINED.

I'll assume you mean that it's left to the implementation to set MPIDR appropriately, and you're expecting implementations to make mistakes with it.

I think what Lorenzo means is that MPIDR is indeed a mandated register with certain bits specified by the ARM ARM. However, the affinity bits are left to the implementation. The ARM ARM makes recommendations and gives some examples but they are not mandatory. An implementer is under no legal obligation to follow the examples. It is not that unlikely that someone in the future may use a different cluster/processor counting scheme than ours. Also in the context of virtualisation, the lower affinity field may be used for virtual CPUs counting).