[RFC PATCH 0/5] ARM: introducing DT topology

[RFC PATCH 0/5] ARM: introducing DT topology

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

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

-- 
1.7.4.4

[RFC PATCH 1/5] ARM: kernel: add device tree init map function

[Lorenzo Pieralisi]

When booting through a device tree, the kernel cpu logical id map can be
initialized using device tree data passed by FW or through an embedded blob.

This patch adds a function that parses device tree "cpu" nodes and
retrieves the corresponding CPUs hardware identifiers (MPIDR).
It sets the possible cpus and the cpu logical map values according to
the number of CPUs defined in the device tree and respective properties.

The primary CPU is assigned cpu logical number 0 to keep the current
convention valid.

Current bindings documentation is included in the patch:

Documentation/devicetree/bindings/arm/cpus.txt

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
---
 Documentation/devicetree/bindings/arm/cpus.txt |   42 ++++++++++++++++++++++++
 arch/arm/include/asm/prom.h                    |    2 +
 arch/arm/kernel/devtree.c                      |   40 ++++++++++++++++++++++
 3 files changed, 84 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/cpus.txt

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
new file mode 100644
index 0000000..28f2b8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -0,0 +1,42 @@
+* ARM CPUs binding description
+
+The device tree allows to describe the layout of CPUs in a system through
+the "cpus" node, which in turn contains a number of subnodes (ie "cpu")
+defining properties for every cpu.
+
+Bindings for CPU nodes follow the ePAPR standard, available from:
+
+http://devicetree.org
+
+For the ARM architecture every CPU node must contain the following property:
+
+- reg : property defining the CPU MPIDR[23:0] register bits
+
+Every cpu node is required to set its device_type to "cpu".
+
+Example:
+
+	cpus {
+		#size-cells = <0>;
+		#address-cells = <1>;
+
+		CPU0: cpu@0x0 {
+			device_type = "cpu";
+			reg = <0x0>;
+		};
+
+		CPU1: cpu@0x1 {
+			device_type = "cpu";
+			reg = <0x1>;
+		};
+
+		CPU2: cpu@0x100 {
+			device_type = "cpu";
+			reg = <0x100>;
+		};
+
+		CPU3: cpu@0x101 {
+			device_type = "cpu";
+			reg = <0x101>;
+		};
+	};
diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
index 6f65ca8..2ab0e07 100644
--- a/arch/arm/include/asm/prom.h
+++ b/arch/arm/include/asm/prom.h
@@ -18,6 +18,7 @@
 
 extern struct machine_desc *setup_machine_fdt(unsigned int dt_phys);
 extern void arm_dt_memblock_reserve(void);
+extern void __init arm_dt_init_cpu_maps(void);
 
 #else /* CONFIG_OF */
 
@@ -27,6 +28,7 @@ static inline struct machine_desc *setup_machine_fdt(unsigned int dt_phys)
 }
 
 static inline void arm_dt_memblock_reserve(void) { }
+static inline void arm_dt_init_cpu_maps(void) { }
 
 #endif /* CONFIG_OF */
 #endif /* ASMARM_PROM_H */
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index bee7f9d..a4662b1 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -19,8 +19,10 @@
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 
+#include <asm/cputype.h>
 #include <asm/setup.h>
 #include <asm/page.h>
+#include <asm/smp_plat.h>
 #include <asm/mach/arch.h>
 #include <asm/mach-types.h>
 
@@ -61,6 +63,44 @@ void __init arm_dt_memblock_reserve(void)
 	}
 }
 
+/*
+ * arm_dt_init_cpu_maps - Function retrieves cpu nodes from the device tree
+ * and builds the cpu logical map array containing MPIDR values related to
+ * logical cpus
+ *
+ * Updates the cpu possible mask with the number of parsed cpu nodes
+ */
+void __init arm_dt_init_cpu_maps(void)
+{
+	struct device_node *dn = NULL;
+	int i, cpu = 1;
+
+	while ((dn = of_find_node_by_type(dn, "cpu")) && cpu <= nr_cpu_ids) {
+		const u32 *hwid;
+		int len;
+
+		pr_debug("  * %s...\n", dn->full_name);
+
+		hwid = of_get_property(dn, "reg", &len);
+
+		if (!hwid || len != 4) {
+			pr_err("  * %s missing reg property\n", dn->full_name);
+			continue;
+		}
+
+		i = (be32_to_cpup(hwid) == (read_cpuid_mpidr() & 0xffffff))
+							? 0 : cpu++;
+
+		if (!i)
+			printk(KERN_INFO "Booting Linux on CPU HWID 0x%x\n",
+					be32_to_cpup(hwid));
+
+		cpu_logical_map(i) = be32_to_cpup(hwid);
+
+		set_cpu_possible(i, true);
+	}
+}
+
 /**
  * setup_machine_fdt - Machine setup when an dtb was passed to the kernel
  * @dt_phys: physical address of dt blob
-- 
1.7.4.4

[RFC PATCH 2/5] ARM: kernel: add cpu logical map DT init in setup_arch

[Lorenzo Pieralisi]

As soon as the device tree is unflattened the cpu logical to physical
mapping is carried out in setup_arch to build a proper array of MPIDR and
corresponding logical indexes.

The mapping could have been carried out using the flattened DT blob and
related primitives, but since the mapping is not needed by early boot
code it can safely be executed when the device tree has been uncompressed to
its tree data structure.

This patch adds the arm_dt_init_cpu maps() function call in setup_arch().

If the kernel is not compiled with DT support the function is empty and
no logical mapping takes place through it; the mapping carried out in
smp_setup_processor_id() is left unchanged.
If DT is supported the mapping created in smp_setup_processor_id() is overriden.
The DT mapping also sets the possible cpus mask, hence platform
code need not set it again in the respective smp_init_cpus() functions.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
---
 arch/arm/kernel/setup.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index eedf7f0..efd1ea5 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -952,6 +952,7 @@ void __init setup_arch(char **cmdline_p)
 
 	unflatten_device_tree();
 
+	arm_dt_init_cpu_maps();
 #ifdef CONFIG_SMP
 	if (is_smp())
 		smp_init_cpus();
-- 
1.7.4.4

[RFC PATCH 3/5] ARM: kernel: add logical mappings look-up

[Lorenzo Pieralisi]

In ARM SMP systems the MPIDR register ([23:0] bits) is used to uniquely
identify CPUs.

In order to retrieve the logical CPU index corresponding to a given
MPIDR value and guarantee a consistent translation throughout the kernel, this
patch adds a look-up based on the MPIDR so that irq controller drivers and
other kernel subsystems can use it whenever the logical cpu index corresponding
to a given MPIDR value is needed.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
---
 arch/arm/include/asm/smp_plat.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
index 558d6c8..deabc78 100644
--- a/arch/arm/include/asm/smp_plat.h
+++ b/arch/arm/include/asm/smp_plat.h
@@ -5,6 +5,9 @@
 #ifndef __ASMARM_SMP_PLAT_H
 #define __ASMARM_SMP_PLAT_H
 
+#include <linux/cpumask.h>
+#include <linux/err.h>
+
 #include <asm/cputype.h>
 
 /*
@@ -49,4 +52,13 @@ static inline int cache_ops_need_broadcast(void)
 extern int __cpu_logical_map[];
 #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
 
+static inline int get_logical_index(u32 mpidr)
+{
+	int cpu;
+	for (cpu = 0; cpu < nr_cpu_ids; cpu++)
+		if (cpu_logical_map(cpu) == mpidr)
+			return cpu;
+	return -EINVAL;
+}
+
 #endif
-- 
1.7.4.4

[RFC PATCH 4/5] ARM: gic: add cpuif topology description

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

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 <lorenzo.pieralisi@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
---
 Documentation/devicetree/bindings/arm/gic.txt |   69 ++++++++++++++++++
 arch/arm/common/gic.c                         |   94 +++++++++++++++++++++++-
 2 files changed, 159 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 9b4b82a..da2f6a8 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -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@0x0 {
+			device_type = "cpu";
+			reg = <0x0>;
+		};
+
+		CPU1: cpu@0x1 {
+			device_type = "cpu";
+			reg = <0x1>;
+		};
+
+		CPU2: cpu@0x100 {
+			device_type = "cpu";
+			reg = <0x100>;
+		};
+
+		CPU3: cpu@0x101 {
+			device_type = "cpu";
+			reg = <0x101>;
+		};
+	};
+
+	intc: interrupt-controller@fff11000 {
+		compatible = "arm,cortex-a9-gic";
+		#interrupt-cells = <3>;
+		#address-cells = <1>;
+		interrupt-controller;
+		reg = <0xfff11000 0x1000>,
+		      <0xfff10100 0x100>;
+
+			gic-cpuif@0x0 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x0>;
+				cpu = <&CPU0>;
+			};
+
+			gic-cpuif@0x1 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x1>;
+				cpu = <&CPU1>;
+			};
+
+			gic-cpuif@0x2 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x2>;
+				cpu = <&CPU2>;
+			};
+
+			gic-cpuif@0x3 {
+				compatible = "arm,gic-cpuif";
+				cpuif-id = <0x3>;
+				cpu = <&CPU3>;
+			};
+	};
diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index c47d619..4be754d 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -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
-- 
1.7.4.4

Re: [RFC PATCH 4/5] ARM: gic: add cpuif topology description

[Rob Herring]

On 01/18/2012 08:36 AM, Lorenzo Pieralisi wrote:
> 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.
> 

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.

Rob


> The GIC device tree bindings documentation is updated by the patch
> accordingly.
> 
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> Cc: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Cc: Russell King <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Cc: Vincent Guittot <vincent.guittot-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt |   69 ++++++++++++++++++
>  arch/arm/common/gic.c                         |   94 +++++++++++++++++++++++-
>  2 files changed, 159 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 9b4b82a..da2f6a8 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -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@0x0 {
> +			device_type = "cpu";
> +			reg = <0x0>;
> +		};
> +
> +		CPU1: cpu@0x1 {
> +			device_type = "cpu";
> +			reg = <0x1>;
> +		};
> +
> +		CPU2: cpu@0x100 {
> +			device_type = "cpu";
> +			reg = <0x100>;
> +		};
> +
> +		CPU3: cpu@0x101 {
> +			device_type = "cpu";
> +			reg = <0x101>;
> +		};
> +	};
> +
> +	intc: interrupt-controller@fff11000 {
> +		compatible = "arm,cortex-a9-gic";
> +		#interrupt-cells = <3>;
> +		#address-cells = <1>;
> +		interrupt-controller;
> +		reg = <0xfff11000 0x1000>,
> +		      <0xfff10100 0x100>;
> +
> +			gic-cpuif@0x0 {
> +				compatible = "arm,gic-cpuif";
> +				cpuif-id = <0x0>;
> +				cpu = <&CPU0>;
> +			};
> +
> +			gic-cpuif@0x1 {
> +				compatible = "arm,gic-cpuif";
> +				cpuif-id = <0x1>;
> +				cpu = <&CPU1>;
> +			};
> +
> +			gic-cpuif@0x2 {
> +				compatible = "arm,gic-cpuif";
> +				cpuif-id = <0x2>;
> +				cpu = <&CPU2>;
> +			};
> +
> +			gic-cpuif@0x3 {
> +				compatible = "arm,gic-cpuif";
> +				cpuif-id = <0x3>;
> +				cpu = <&CPU3>;
> +			};
> +	};
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index c47d619..4be754d 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -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

Re: [RFC PATCH 4/5] ARM: gic: add cpuif topology description

[Lorenzo Pieralisi]

On Wed, Jan 18, 2012 at 03:54:12PM +0000, Rob Herring wrote:
> On 01/18/2012 08:36 AM, Lorenzo Pieralisi wrote:
> > 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.
> > 
> 
> What if CONFIG_OF is enabled, but the DTB is not updated? That needs to
> work.
> 

That's a fair point, I have to cater for that, point taken, I will rework it.  

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

We are updating it, not changing it and we can change it if it does not 
describe all HW features we need, I hope.
This patch does not re-introduce GIC CPU IF addresses. It just adds a
GIC CPU IF id which is required to associate software generated IRQs to a 
given CPU. ARM mandates that those CPU ids should be wired sequentially,
but it depends on design parameters, so it is important to have the capability  
to define them through DT.

Lorenzo

[RFC PATCH 5/5] ARM: kernel: build CPU topology from DT

[Lorenzo Pieralisi]

Current ARM SMP systems can be described as a hierarchy of CPU layers,
each one comprised of resources (ie caches) shared between CPUs
belonging to a given hierarchy level. In order to optimize scheduling
and provide power management code with a description of CPUs topology, a
topology description must be retrieved either from SW or by probing HW.
The current solution relies on the MPIDR register to build CPU topology,
which assumes the MPIDR is wired with a reliable representation of
affinity levels for the system.

Since this hypothesis might not hold, this patch adds code allowing the CPU
topology to be parsed and built from a device tree representation (ie
bindings) provided via a device tree blob.

Topology is parsed from DT and built once for all by the boot CPU;
common code creating sibling maps has been factored out in a function

void update_siblings_masks(unsigned int);

that is executed for DT and non-DT initialization.

Documentation for the bindings is provided in the patchset.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>
---
 Documentation/devicetree/bindings/arm/topology.txt |  167 ++++++++++++++++++++
 arch/arm/kernel/topology.c                         |  110 +++++++++++---
 2 files changed, 257 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/topology.txt

diff --git a/Documentation/devicetree/bindings/arm/topology.txt b/Documentation/devicetree/bindings/arm/topology.txt
new file mode 100644
index 0000000..071cd18
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/topology.txt
@@ -0,0 +1,167 @@
+* ARM topology binding description
+
+The device tree notion of CPUs in a system must be extended to cater for
+systems comprised of clusters and hyper-threaded (SMT) cpus.
+
+In an ARM system CPUs hierarchy is defined through three levels (in descending
+hierarchy order):
+
+(1) cluster
+(2) core
+(3) thread
+
+The cpus node represents the lowest hierarchy level for CPUs in a system.
+For instance in a system where CPUs support SMT, "cpu" nodes represent all
+HW threads ("thread" - level (3) above) existing in the systems.
+In systems where SMT is not supported "cpu" nodes just represent a "core"
+level (level (2) above).
+
+Every "cpu" node must define two phandles:
+
+- cluster: phandle pointing to the cluster the given thread/core belongs to
+- core: phandle pointing to the core the given thread/core belongs to
+
+In an SMT system a "cpu" node must also define
+
+- thread-id: property defining the thread-id for the cpu node in question
+
+A missing "thread-id" means that the cpu does not support SMT.
+
+The topology requires the definition of a "clusters" node.
+The "clusters" node does not represent a HW device; it is a container where
+all cluster child nodes ("cluster") are defined.
+
+Each "cluster" node must contain:
+
+- reg: property defining cluster id
+- it must provide a label (eg CLUSTER0: cluster@0x0) to be pointed to by a
+  phandle
+
+The topology requires the definition of a "cores" node.
+The "cores" node does not represent a HW device; it is a container where all
+core child nodes ("core") are defined.
+
+Each core node must contain:
+
+- reg: property defining core id
+- it must provide a label (eg CORE0: core@0x0) to be pointed to by a
+  phandle
+
+Example (8-core system, dual-cluster, no SMT):
+
+clusters {
+
+	#size-cells = <0>;
+	#address-cells = <1>;
+
+	CLUSTER0: cluster@0x0 {
+		reg = <0x0>;
+		cores {
+
+			#size-cells = <0>;
+			#address-cells = <1>;
+
+			CORE0: core@0x0 {
+				reg = <0x0>;
+			};
+
+			CORE1: core@0x1 {
+				reg = <0x1>;
+			};
+
+			CORE2: core@0x2 {
+				reg = <0x2>;
+			};
+
+			CORE3: core@0x3 {
+				reg = <0x3>;
+			};
+		};
+	};
+
+	CLUSTER1: cluster@0x1 {
+		reg = <0x1>;
+		cores {
+
+			#size-cells = <0>;
+			#address-cells = <1>;
+
+			CORE4: core@0x0 {
+				reg = <0x0>;
+			};
+
+			CORE5: core@0x1 {
+				reg = <0x1>;
+			};
+
+			CORE6: core@0x2 {
+				reg = <0x2>;
+			};
+
+			CORE7: core@0x3 {
+				reg = <0x3>;
+			};
+		};
+	};
+};
+
+cpus {
+	#size-cells = <0>;
+	#address-cells = <1>;
+
+	CPU0: cpu@0x0 {
+		device_type = "cpu";
+		reg = <0x0>;
+		cluster = <&CLUSTER0>;
+		core = <&CORE0>;
+	};
+
+	CPU1: cpu@0x1 {
+		device_type = "cpu";
+		reg = <0x1>;
+		cluster = <&CLUSTER0>;
+		core = <&CORE1>;
+	};
+
+	CPU2: cpu@0x2 {
+		device_type = "cpu";
+		reg = <0x2>;
+		cluster = <&CLUSTER0>;
+		core = <&CORE2>;
+	};
+
+	CPU3: cpu@0x3 {
+		device_type = "cpu";
+		reg = <0x3>;
+		cluster = <&CLUSTER0>;
+		core = <&CORE3>;
+	};
+
+	CPU4: cpu@0x100 {
+		device_type = "cpu";
+		reg = <0x100>;
+		cluster = <&CLUSTER1>;
+		core = <&CORE4>;
+	};
+
+	CPU5: cpu@0x101 {
+		device_type = "cpu";
+		reg = <0x101>;
+		cluster = <&CLUSTER1>;
+		core = <&CORE5>;
+	};
+
+	CPU6: cpu@0x102 {
+		device_type = "cpu";
+		reg = <0x102>;
+		cluster = <&CLUSTER1>;
+		core = <&CORE6>;
+	};
+
+	CPU7: cpu@0x103 {
+		device_type = "cpu";
+		reg = <0x103>;
+		cluster = <&CLUSTER1>;
+		core = <&CORE7>;
+	};
+};
diff --git a/arch/arm/kernel/topology.c b/arch/arm/kernel/topology.c
index 8200dea..f58fd20 100644
--- a/arch/arm/kernel/topology.c
+++ b/arch/arm/kernel/topology.c
@@ -17,9 +17,11 @@
 #include <linux/percpu.h>
 #include <linux/node.h>
 #include <linux/nodemask.h>
+#include <linux/of.h>
 #include <linux/sched.h>
 
 #include <asm/cputype.h>
+#include <asm/smp_plat.h>
 #include <asm/topology.h>
 
 #define MPIDR_SMP_BITMASK (0x3 << 30)
@@ -48,6 +50,30 @@ const struct cpumask *cpu_coregroup_mask(int cpu)
 	return &cpu_topology[cpu].core_sibling;
 }
 
+void update_siblings_masks(unsigned int cpuid)
+{
+	struct cputopo_arm *cpu_topo, *cpuid_topo = &cpu_topology[cpuid];
+	int cpu;
+	/* update core and thread sibling masks */
+	for_each_possible_cpu(cpu) {
+		cpu_topo = &cpu_topology[cpu];
+
+		if (cpuid_topo->socket_id == cpu_topo->socket_id) {
+			cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
+			if (cpu != cpuid)
+				cpumask_set_cpu(cpu, &cpuid_topo->core_sibling);
+
+			if (cpuid_topo->core_id == cpu_topo->core_id) {
+				cpumask_set_cpu(cpuid,
+					&cpu_topo->thread_sibling);
+				if (cpu != cpuid)
+					cpumask_set_cpu(cpu,
+						&cpuid_topo->thread_sibling);
+			}
+		}
+	}
+}
+
 /*
  * store_cpu_topology is called at boot when only one cpu is running
  * and with the mutex cpu_hotplug.lock locked, when several cpus have booted,
@@ -57,7 +83,6 @@ void store_cpu_topology(unsigned int cpuid)
 {
 	struct cputopo_arm *cpuid_topo = &cpu_topology[cpuid];
 	unsigned int mpidr;
-	unsigned int cpu;
 
 	/* If the cpu topology has been already set, just return */
 	if (cpuid_topo->core_id != -1)
@@ -99,25 +124,7 @@ void store_cpu_topology(unsigned int cpuid)
 		cpuid_topo->socket_id = -1;
 	}
 
-	/* update core and thread sibling masks */
-	for_each_possible_cpu(cpu) {
-		struct cputopo_arm *cpu_topo = &cpu_topology[cpu];
-
-		if (cpuid_topo->socket_id == cpu_topo->socket_id) {
-			cpumask_set_cpu(cpuid, &cpu_topo->core_sibling);
-			if (cpu != cpuid)
-				cpumask_set_cpu(cpu,
-					&cpuid_topo->core_sibling);
-
-			if (cpuid_topo->core_id == cpu_topo->core_id) {
-				cpumask_set_cpu(cpuid,
-					&cpu_topo->thread_sibling);
-				if (cpu != cpuid)
-					cpumask_set_cpu(cpu,
-						&cpuid_topo->thread_sibling);
-			}
-		}
-	}
+	update_siblings_masks(cpuid);
 	smp_wmb();
 
 	printk(KERN_INFO "CPU%u: thread %d, cpu %d, socket %d, mpidr %x\n",
@@ -126,6 +133,67 @@ void store_cpu_topology(unsigned int cpuid)
 		cpu_topology[cpuid].socket_id, mpidr);
 }
 
+#ifdef CONFIG_OF
+static void __init parse_dt_topology(void)
+{
+	struct cputopo_arm *cpu_info;
+	struct device_node *dn, *cn = NULL;
+	int cpu;
+
+	while ((cn = of_find_node_by_type(cn, "cpu"))) {
+		const u32 *hwid;
+		int len;
+
+		pr_debug("  * %s...\n", cn->full_name);
+
+		hwid = of_get_property(cn, "reg", &len);
+
+		if (!hwid || len != 4) {
+			pr_err("  * %s missing reg property\n", cn->full_name);
+			continue;
+		}
+
+		cpu = get_logical_index(be32_to_cpup(hwid));
+
+		if (cpu < 0) {
+			pr_err("  * %s wrong logical index %d\n", cn->full_name,
+							cpu);
+			continue;
+		}
+
+		cpu_info = &cpu_topology[cpu];
+
+		hwid = of_get_property(cn, "thread-id", &len);
+
+		if (hwid && len == 4)
+			cpu_info->thread_id = be32_to_cpup(hwid);
+
+		dn = of_parse_phandle(cn, "core", 0);
+
+		hwid = of_get_property(dn, "reg", &len);
+
+		if (hwid && len == 4)
+			cpu_info->core_id = be32_to_cpup(hwid);
+
+		dn = of_parse_phandle(cn, "cluster", 0);
+
+		hwid = of_get_property(dn, "reg", &len);
+
+		if (hwid && len == 4)
+			cpu_info->socket_id = be32_to_cpup(hwid);
+
+		update_siblings_masks(cpu);
+
+		printk(KERN_INFO "CPU%u: thread %d, core %d, socket %d\n",
+			cpu, cpu_topology[cpu].thread_id,
+			cpu_topology[cpu].core_id,
+			cpu_topology[cpu].socket_id);
+	}
+}
+#else
+static inline void parse_dt_topology(void) {}
+#endif
+
 /*
  * init_cpu_topology is called at boot when only one cpu is running
  * which prevent simultaneous write access to cpu_topology array
@@ -145,4 +213,6 @@ void init_cpu_topology(void)
 		cpumask_clear(&cpu_topo->thread_sibling);
 	}
 	smp_wmb();
+
+	parse_dt_topology();
 }
-- 
1.7.4.4

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Rob Herring]

On 01/18/2012 08:36 AM, Lorenzo Pieralisi wrote:
> 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.
> 

"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?

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

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Lorenzo Pieralisi]

On Wed, Jan 18, 2012 at 03:38:54PM +0000, Rob Herring wrote:
> On 01/18/2012 08:36 AM, Lorenzo Pieralisi wrote:
> > 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.
> > 
> 
> "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?

Thanks for having a look. 
We cannot control how it is wired up, so yes, it might be a problem.
The strict requirement is that it must be a unique identifier, and the 
bindings I wrote provide the MPIDR through "cpu" nodes "reg" properties, 
but those values must correspond to the real HW value, which has to be unique 
[ie pen release], but it *might not* represent the affinity levels properly.

Thanks,
Lorenzo

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Russell King - ARM Linux]

On Wed, Jan 18, 2012 at 02:36:43PM +0000, Lorenzo Pieralisi wrote:
> 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}.

> 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?

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

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Lorenzo Pieralisi]

On Wed, Jan 18, 2012 at 04:24:23PM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 18, 2012 at 02:36:43PM +0000, Lorenzo Pieralisi wrote:
> > 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}.

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.

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

> > 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 share your view Russell. Having said that: MPIDR is IMPLEMENTATION DEFINED.

There are three possibilities:

1- MPIDR is not unique (there are CPUs with duplicated values) 
2- MPIDR is unique but affinity levels do not represent hierarchy
   properly (cluster level, core level)
3- MPIDR is unique and affinity levels are properly set

IMHO:

case 1) We should not care and designers must get their act together
case 2) We might use DT to build the topology properly and rely on
        uniqueness to have a properly running system
case 3) We are home and dry	

Thoughts ?

Thanks a lot for your feedback,
Lorenzo

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Russell King - ARM Linux]

On Wed, Jan 18, 2012 at 05:50:28PM +0000, Lorenzo Pieralisi wrote:
> 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 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}.

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

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

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

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

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Lorenzo Pieralisi]

On Wed, Jan 18, 2012 at 06:04:53PM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 18, 2012 at 05:50:28PM +0000, Lorenzo Pieralisi wrote:
> > 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 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}.

Correct. Now the question is how to get those values in the array.
There are two ways:

a) the boot CPU generates them
b) the boot CPU parses them from DT

case a) For this to work, and to be able to boot on every CPU on every
        cluster we need to have a way to detect the number of clusters
	and the number of CPUs per cluster
case b) Just parse the tree and retrieve the "reg" property, patch 1 of
        this series

Now, as you know ways better than I do, to boot an SMP system (with a
GIC) we need a pair {mpidr, GIC CPU IF id} and the values must be linked
(ie they have to uniquely represent a CPU), you correctly stated that in
your reply.

Hence, as you said, we need to retrieve the GIC CPU IF related to a given
MPIDR. We cannot generate it, as the current code assumes.

Example (dual-cluster system, each cluster made up of two CPUS, no SMT)

* CLUSTER 0

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

* CLUSTER 1

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

Even if we knew the number of cpus per cluster and the number of clusters 
at boot

MPIDR = 0x101 does NOT imply GIC CPU IF id=0x3

The following formula does not work all the time
(GIC CPU IF id = MPIDR[7:0] + nb_cpus_in_lower_cluster_indexes)

NB: nb_cpus_in_lower_cluster_indexes must be computed by the boot CPU.

ARM mandate that numbering, but it is not a strict requirement (GICs
deviating from that numbering will be the exception, not the rule, but
we have to cater for them).

Hence we need a way to link GIC CPU IF id to MPIDR and that's patch 4 or
we could do that by using static data, I see no other solution.

But if we accept we can use DT to create that link, we can also use DT
to init cpu_logical_map instead of generating its values (since the linkage in
DT is done by using "cpu" nodes).

I have no strong opinion on this, just would like us to agree on a clean and
proper way forward, so any comment more than welcome.

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

That's the first thing I thought when I noticed the MPIDR config for 
multi-clusters. 

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

That's agreed.

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

I agree Russell, nothing to object, but you should concede that having
op-codes with bits switched is easier to detect :) (even though if we
decide to "generate" the cpu_logical_map with a bunch of code, the pen 
release code will easily go pop if MPIDR is botched).

PATCH 5 can also be used to count the number of CPUs per cluster and the 
number of clusters in the system if we cannot rely on a HW mechanisms to probe 
that.

I will keep it on my tree in case we deem it necessary.

Thank you very much,
Lorenzo

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Catalin Marinas]

On Wed, Jan 18, 2012 at 06:04:53PM +0000, Russell King - ARM Linux wrote:
> On Wed, Jan 18, 2012 at 05:50:28PM +0000, Lorenzo Pieralisi wrote:
> > > 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.

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

-- 
Catalin

Re: [RFC PATCH 0/5] ARM: introducing DT topology

[Russell King - ARM Linux]

On Thu, Jan 19, 2012 at 12:18:32PM +0000, Catalin Marinas wrote:
> On Wed, Jan 18, 2012 at 06:04:53PM +0000, Russell King - ARM Linux wrote:
> > On Wed, Jan 18, 2012 at 05:50:28PM +0000, Lorenzo Pieralisi wrote:
> > > > 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.
> 
> 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).

So what you're saying is that the definition of this register has become
soo loose that it's completely useless.  Oh what a really great move.

Your designers really need their heads bashed against a brick wall to
knock some sense into them over stuff like this.  It's exactly this
kind of idiotic 'infinite flexibility' that Linus had a moan about
last year.

What it means is that we need _more_ code to support whatever
manufacturers come up in their scheme of things - rather than having
a standard way to represent these details, which software can parse
in a consistent way.

Instead, we have something that's effectively mandated to be present
but is basically useless because no software can parse it properly
without specific information from the manufacturer, and even then
its not guaranteed.

Well done ARM Ltd.  Again.

Stop doing this.