[RFC PATCH 0/4] ARM: multi-cluster aware boot protocol

[RFC PATCH 0/4] ARM: multi-cluster aware boot protocol

[Lorenzo Pieralisi]

This series is an updated version of a previous posting available here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/080868.html

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 HW-to-logical cpu map.

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 can be wired with a numbering scheme following per-SoC
design parameters which can be only detected through probing or device
tree representation.

Through the device tree and "cpu" nodes bindings, the kernel is provided
with HW values for MPIDR registers that allow the kernel to identify the
HW CPU ids that are present in the platform.

The GIC code has been extended to allow automatic detection of GIC CPU IF ids
at boot. IPIs are broadcast to all possible CPUs, and every time a secondary
CPU is booted, it initializes its own mask and clears itself from the mask of
all other logical CPUs.

The device tree bindings and GIC probing allow to boot the Linux kernel on any
CPU of a multi-cluster system without relying on a platform specific hook to
identify the number of CPUs and hypothesis on the sequential pattern of MPIDRs
and relative GIC CPU IF ids.

Pen release code for all converted platforms will need patching to extend the
current MPIDR range check; this will be done as soon as the bindings and
code for the multi-cluster boot protocol will be reviewed and accepted.

The patchset has been tested on:

- TC2 testchip

to boot a 5-core dual-cluster system on every possible CPU.

Lorenzo Pieralisi (3):
  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

Nicolas Pitre (1):
  ARM: gic: use a private mapping for CPU target interfaces

 Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
 arch/arm/common/gic.c                          | 42 ++++++++++++++++++-----
 arch/arm/include/asm/prom.h                    |  2 ++
 arch/arm/include/asm/smp_plat.h                | 17 ++++++++++
 arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
 arch/arm/kernel/setup.c                        |  1 +
 6 files changed, 143 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/cpus.txt

-- 
1.7.12

[RFC PATCH 0/4] ARM: multi-cluster aware boot protocol

[Lorenzo Pieralisi]

This series is an updated version of a previous posting available here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2012-January/080868.html

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 HW-to-logical cpu map.

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 can be wired with a numbering scheme following per-SoC
design parameters which can be only detected through probing or device
tree representation.

Through the device tree and "cpu" nodes bindings, the kernel is provided
with HW values for MPIDR registers that allow the kernel to identify the
HW CPU ids that are present in the platform.

The GIC code has been extended to allow automatic detection of GIC CPU IF ids
at boot. IPIs are broadcast to all possible CPUs, and every time a secondary
CPU is booted, it initializes its own mask and clears itself from the mask of
all other logical CPUs.

The device tree bindings and GIC probing allow to boot the Linux kernel on any
CPU of a multi-cluster system without relying on a platform specific hook to
identify the number of CPUs and hypothesis on the sequential pattern of MPIDRs
and relative GIC CPU IF ids.

Pen release code for all converted platforms will need patching to extend the
current MPIDR range check; this will be done as soon as the bindings and
code for the multi-cluster boot protocol will be reviewed and accepted.

The patchset has been tested on:

- TC2 testchip

to boot a 5-core dual-cluster system on every possible CPU.

Lorenzo Pieralisi (3):
  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

Nicolas Pitre (1):
  ARM: gic: use a private mapping for CPU target interfaces

 Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
 arch/arm/common/gic.c                          | 42 ++++++++++++++++++-----
 arch/arm/include/asm/prom.h                    |  2 ++
 arch/arm/include/asm/smp_plat.h                | 17 ++++++++++
 arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
 arch/arm/kernel/setup.c                        |  1 +
 6 files changed, 143 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/cpus.txt

-- 
1.7.12

[RFC PATCH 1/4] 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>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
 arch/arm/include/asm/prom.h                    |  2 ++
 arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 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..897f3b4
--- /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@0 {
+			device_type = "cpu";
+			reg = <0x0>;
+		};
+
+		CPU1: cpu@1 {
+			device_type = "cpu";
+			reg = <0x1>;
+		};
+
+		CPU2: cpu@100 {
+			device_type = "cpu";
+			reg = <0x100>;
+		};
+
+		CPU3: cpu@101 {
+			device_type = "cpu";
+			reg = <0x101>;
+		};
+	};
diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
index aeae9c6..8dd51dc 100644
--- a/arch/arm/include/asm/prom.h
+++ b/arch/arm/include/asm/prom.h
@@ -15,6 +15,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 */
 
@@ -24,6 +25,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..c86e414 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,51 @@ 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;
+		}
+		/*
+		 * We want to assign the boot CPU logical id 0.
+		 * Boot CPU checks its own MPIDR and if matches the current
+		 * cpu node "reg" value it sets the logical cpu index to 0
+		 * and stores the physical id accordingly.
+		 * If MPIDR does not match, assign sequential cpu logical
+		 * id (starting from 1) and increments it.
+		 */
+		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.12

[RFC PATCH 1/4] 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>
---
 Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
 arch/arm/include/asm/prom.h                    |  2 ++
 arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
 3 files changed, 91 insertions(+)
 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..897f3b4
--- /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 at 0 {
+			device_type = "cpu";
+			reg = <0x0>;
+		};
+
+		CPU1: cpu at 1 {
+			device_type = "cpu";
+			reg = <0x1>;
+		};
+
+		CPU2: cpu at 100 {
+			device_type = "cpu";
+			reg = <0x100>;
+		};
+
+		CPU3: cpu at 101 {
+			device_type = "cpu";
+			reg = <0x101>;
+		};
+	};
diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
index aeae9c6..8dd51dc 100644
--- a/arch/arm/include/asm/prom.h
+++ b/arch/arm/include/asm/prom.h
@@ -15,6 +15,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 */
 
@@ -24,6 +25,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..c86e414 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,51 @@ 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;
+		}
+		/*
+		 * We want to assign the boot CPU logical id 0.
+		 * Boot CPU checks its own MPIDR and if matches the current
+		 * cpu node "reg" value it sets the logical cpu index to 0
+		 * and stores the physical id accordingly.
+		 * If MPIDR does not match, assign sequential cpu logical
+		 * id (starting from 1) and increments it.
+		 */
+		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.12

[RFC PATCH 2/4] 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>
---
 arch/arm/kernel/setup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index da1d1aa..20c530b 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -758,6 +758,7 @@ void __init setup_arch(char **cmdline_p)
 
 	unflatten_device_tree();
 
+	arm_dt_init_cpu_maps();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
 		smp_set_ops(mdesc->smp);
-- 
1.7.12

[RFC PATCH 2/4] 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>
---
 arch/arm/kernel/setup.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index da1d1aa..20c530b 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -758,6 +758,7 @@ void __init setup_arch(char **cmdline_p)
 
 	unflatten_device_tree();
 
+	arm_dt_init_cpu_maps();
 #ifdef CONFIG_SMP
 	if (is_smp()) {
 		smp_set_ops(mdesc->smp);
-- 
1.7.12

[RFC PATCH 3/4] 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[23:0] so that 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>
---
 arch/arm/include/asm/smp_plat.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
index 558d6c8..aaa61b6 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>
 
 /*
@@ -48,5 +51,19 @@ static inline int cache_ops_need_broadcast(void)
  */
 extern int __cpu_logical_map[];
 #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
+/*
+ * Retrieve logical cpu index corresponding to a given MPIDR[23:0]
+ *  - mpidr: MPIDR[23:0] to be used for the look-up
+ *
+ * Returns the cpu logical index or -EINVAL on look-up error
+ */
+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.12

[RFC PATCH 3/4] 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[23:0] so that 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>
---
 arch/arm/include/asm/smp_plat.h | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
index 558d6c8..aaa61b6 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>
 
 /*
@@ -48,5 +51,19 @@ static inline int cache_ops_need_broadcast(void)
  */
 extern int __cpu_logical_map[];
 #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
+/*
+ * Retrieve logical cpu index corresponding to a given MPIDR[23:0]
+ *  - mpidr: MPIDR[23:0] to be used for the look-up
+ *
+ * Returns the cpu logical index or -EINVAL on look-up error
+ */
+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.12

[RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Lorenzo Pieralisi]

From: Nicolas Pitre <nicolas.pitre@linaro.org>

The GIC interface numbering does not necessarily follow the logical
CPU numbering, especially for complex topologies such as multi-cluster
systems.

Fortunately we can easily probe the GIC to create a mapping as the
Interrupt Processor Targets Registers for the first 32 interrupts are
read-only, and each field returns a value that always corresponds to
the processor reading the register.

Initially all mappings target all CPUs in case an IPI is required to
boot secondary CPUs.  It is refined as those CPUs discover what their
actual mapping is.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index aa52699..1338a55 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -70,6 +70,13 @@ struct gic_chip_data {
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 
 /*
+ * The GIC mapping of CPU interfaces does not necessarily match
+ * the logical CPU numbering.  Let's use a mapping as returned
+ * by the GIC itself.
+ */
+static u8 gic_cpu_map[8] __read_mostly;
+
+/*
  * Supported arch specific GIC irq extension.
  * Default make them NULL.
  */
@@ -242,7 +249,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 = gic_cpu_map[cpu] << shift;
 
 	raw_spin_lock(&irq_controller_lock);
 	val = readl_relaxed(reg) & ~mask;
@@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	u32 cpumask;
 	unsigned int gic_irqs = gic->gic_irqs;
 	void __iomem *base = gic_data_dist_base(gic);
-	u32 cpu = cpu_logical_map(smp_processor_id());
-
-	cpumask = 1 << cpu;
-	cpumask |= cpumask << 8;
-	cpumask |= cpumask << 16;
 
 	writel_relaxed(0, base + GIC_DIST_CTRL);
 
@@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	/*
 	 * Set all global interrupts to this CPU only.
 	 */
+	cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0);
 	for (i = 32; i < gic_irqs; i += 4)
 		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
 
@@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
 {
 	void __iomem *dist_base = gic_data_dist_base(gic);
 	void __iomem *base = gic_data_cpu_base(gic);
+	unsigned int cpu_mask, cpu = smp_processor_id();
 	int i;
 
 	/*
+	 * Get what the GIC says our CPU mask is.
+	 */
+	BUG_ON(cpu >= 8);
+	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
+	gic_cpu_map[cpu] = cpu_mask;
+
+	/*
+	 * Clear our mask from the other map entries in case they're
+	 * still undefined.
+	 */
+	for (i = 0; i < 8; i++)
+		if (i != cpu)
+			gic_cpu_map[i] &= ~cpu_mask;
+
+	/*
 	 * Deal with the banked PPI and SGI interrupts - disable all
 	 * PPI interrupts, ensure all SGI interrupts are enabled.
 	 */
@@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 {
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
-	int gic_irqs, irq_base;
+	int gic_irqs, irq_base, i;
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
@@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 	}
 
 	/*
+	 * Initialize the CPU interface map to all CPUs.
+	 * It will be refined as each CPU probes its ID.
+	 */
+	for (i = 0; i < 8; i++)
+		gic_cpu_map[i] = 0xff;
+
+	/*
 	 * For primary GICs, skip over SGIs.
 	 * For secondary GICs, skip over PPIs, too.
 	 */
@@ -737,7 +763,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 |= gic_cpu_map[cpu];
 
 	/*
 	 * Ensure that stores to Normal memory are visible to the
-- 
1.7.12

[RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Lorenzo Pieralisi]

From: Nicolas Pitre <nicolas.pitre@linaro.org>

The GIC interface numbering does not necessarily follow the logical
CPU numbering, especially for complex topologies such as multi-cluster
systems.

Fortunately we can easily probe the GIC to create a mapping as the
Interrupt Processor Targets Registers for the first 32 interrupts are
read-only, and each field returns a value that always corresponds to
the processor reading the register.

Initially all mappings target all CPUs in case an IPI is required to
boot secondary CPUs.  It is refined as those CPUs discover what their
actual mapping is.

Signed-off-by: Nicolas Pitre <nico@linaro.org>
---
 arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index aa52699..1338a55 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -70,6 +70,13 @@ struct gic_chip_data {
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
 
 /*
+ * The GIC mapping of CPU interfaces does not necessarily match
+ * the logical CPU numbering.  Let's use a mapping as returned
+ * by the GIC itself.
+ */
+static u8 gic_cpu_map[8] __read_mostly;
+
+/*
  * Supported arch specific GIC irq extension.
  * Default make them NULL.
  */
@@ -242,7 +249,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 = gic_cpu_map[cpu] << shift;
 
 	raw_spin_lock(&irq_controller_lock);
 	val = readl_relaxed(reg) & ~mask;
@@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	u32 cpumask;
 	unsigned int gic_irqs = gic->gic_irqs;
 	void __iomem *base = gic_data_dist_base(gic);
-	u32 cpu = cpu_logical_map(smp_processor_id());
-
-	cpumask = 1 << cpu;
-	cpumask |= cpumask << 8;
-	cpumask |= cpumask << 16;
 
 	writel_relaxed(0, base + GIC_DIST_CTRL);
 
@@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
 	/*
 	 * Set all global interrupts to this CPU only.
 	 */
+	cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0);
 	for (i = 32; i < gic_irqs; i += 4)
 		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
 
@@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
 {
 	void __iomem *dist_base = gic_data_dist_base(gic);
 	void __iomem *base = gic_data_cpu_base(gic);
+	unsigned int cpu_mask, cpu = smp_processor_id();
 	int i;
 
 	/*
+	 * Get what the GIC says our CPU mask is.
+	 */
+	BUG_ON(cpu >= 8);
+	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
+	gic_cpu_map[cpu] = cpu_mask;
+
+	/*
+	 * Clear our mask from the other map entries in case they're
+	 * still undefined.
+	 */
+	for (i = 0; i < 8; i++)
+		if (i != cpu)
+			gic_cpu_map[i] &= ~cpu_mask;
+
+	/*
 	 * Deal with the banked PPI and SGI interrupts - disable all
 	 * PPI interrupts, ensure all SGI interrupts are enabled.
 	 */
@@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 {
 	irq_hw_number_t hwirq_base;
 	struct gic_chip_data *gic;
-	int gic_irqs, irq_base;
+	int gic_irqs, irq_base, i;
 
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
@@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 	}
 
 	/*
+	 * Initialize the CPU interface map to all CPUs.
+	 * It will be refined as each CPU probes its ID.
+	 */
+	for (i = 0; i < 8; i++)
+		gic_cpu_map[i] = 0xff;
+
+	/*
 	 * For primary GICs, skip over SGIs.
 	 * For secondary GICs, skip over PPIs, too.
 	 */
@@ -737,7 +763,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 |= gic_cpu_map[cpu];
 
 	/*
 	 * Ensure that stores to Normal memory are visible to the
-- 
1.7.12

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

[Rob Herring]

On 10/16/2012 08:21 AM, Lorenzo Pieralisi wrote:
> 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-5wv7dgnIgG8@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
>  arch/arm/include/asm/prom.h                    |  2 ++
>  arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>  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..897f3b4
> --- /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

defining or matching the MPIDR?

> +
> +Every cpu node is required to set its device_type to "cpu".

This is a bit questionable as device_type is deprecated for FDT.
However, since the ePAPR defines using it

You should add a compatible property for the cpu model.

> +
> +Example:
> +
> +	cpus {
> +		#size-cells = <0>;
> +		#address-cells = <1>;
> +
> +		CPU0: cpu@0 {
> +			device_type = "cpu";
> +			reg = <0x0>;
> +		};
> +
> +		CPU1: cpu@1 {
> +			device_type = "cpu";
> +			reg = <0x1>;
> +		};
> +
> +		CPU2: cpu@100 {
> +			device_type = "cpu";
> +			reg = <0x100>;
> +		};
> +
> +		CPU3: cpu@101 {
> +			device_type = "cpu";
> +			reg = <0x101>;
> +		};
> +	};
> diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
> index aeae9c6..8dd51dc 100644
> --- a/arch/arm/include/asm/prom.h
> +++ b/arch/arm/include/asm/prom.h
> @@ -15,6 +15,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 */
>  
> @@ -24,6 +25,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..c86e414 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,51 @@ 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) {

I think all /cpu nodes would have the right type. You could use
for_each_child_of_node here.

> +		const u32 *hwid;
> +		int len;
> +
> +		pr_debug("  * %s...\n", dn->full_name);
> +
> +		hwid = of_get_property(dn, "reg", &len);
> +

Use of_property_read_u32.

> +		if (!hwid || len != 4) {
> +			pr_err("  * %s missing reg property\n", dn->full_name);
> +			continue;
> +		}
> +		/*
> +		 * We want to assign the boot CPU logical id 0.
> +		 * Boot CPU checks its own MPIDR and if matches the current
> +		 * cpu node "reg" value it sets the logical cpu index to 0
> +		 * and stores the physical id accordingly.
> +		 * If MPIDR does not match, assign sequential cpu logical
> +		 * id (starting from 1) and increments it.
> +		 */
> +		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
> 

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

[Rob Herring]

On 10/16/2012 08:21 AM, Lorenzo Pieralisi wrote:
> 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>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
>  arch/arm/include/asm/prom.h                    |  2 ++
>  arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
>  3 files changed, 91 insertions(+)
>  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..897f3b4
> --- /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

defining or matching the MPIDR?

> +
> +Every cpu node is required to set its device_type to "cpu".

This is a bit questionable as device_type is deprecated for FDT.
However, since the ePAPR defines using it

You should add a compatible property for the cpu model.

> +
> +Example:
> +
> +	cpus {
> +		#size-cells = <0>;
> +		#address-cells = <1>;
> +
> +		CPU0: cpu at 0 {
> +			device_type = "cpu";
> +			reg = <0x0>;
> +		};
> +
> +		CPU1: cpu at 1 {
> +			device_type = "cpu";
> +			reg = <0x1>;
> +		};
> +
> +		CPU2: cpu at 100 {
> +			device_type = "cpu";
> +			reg = <0x100>;
> +		};
> +
> +		CPU3: cpu at 101 {
> +			device_type = "cpu";
> +			reg = <0x101>;
> +		};
> +	};
> diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
> index aeae9c6..8dd51dc 100644
> --- a/arch/arm/include/asm/prom.h
> +++ b/arch/arm/include/asm/prom.h
> @@ -15,6 +15,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 */
>  
> @@ -24,6 +25,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..c86e414 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,51 @@ 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) {

I think all /cpu nodes would have the right type. You could use
for_each_child_of_node here.

> +		const u32 *hwid;
> +		int len;
> +
> +		pr_debug("  * %s...\n", dn->full_name);
> +
> +		hwid = of_get_property(dn, "reg", &len);
> +

Use of_property_read_u32.

> +		if (!hwid || len != 4) {
> +			pr_err("  * %s missing reg property\n", dn->full_name);
> +			continue;
> +		}
> +		/*
> +		 * We want to assign the boot CPU logical id 0.
> +		 * Boot CPU checks its own MPIDR and if matches the current
> +		 * cpu node "reg" value it sets the logical cpu index to 0
> +		 * and stores the physical id accordingly.
> +		 * If MPIDR does not match, assign sequential cpu logical
> +		 * id (starting from 1) and increments it.
> +		 */
> +		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
> 

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

[Lorenzo Pieralisi]

Hi Rob,

thanks for having a look.

On Tue, Oct 16, 2012 at 09:42:43PM +0100, Rob Herring wrote:
> On 10/16/2012 08:21 AM, Lorenzo Pieralisi wrote:
> > 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>
> > ---
> >  Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
> >  arch/arm/include/asm/prom.h                    |  2 ++
> >  arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
> >  3 files changed, 91 insertions(+)
> >  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..897f3b4
> > --- /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
> 
> defining or matching the MPIDR?

I would say matching, otherwise it reads as if the MPIDR were writable.

> > +
> > +Every cpu node is required to set its device_type to "cpu".
> 
> This is a bit questionable as device_type is deprecated for FDT.
> However, since the ePAPR defines using it
> 
> You should add a compatible property for the cpu model.

Ok, I will.

> > +
> > +Example:
> > +
> > +	cpus {
> > +		#size-cells = <0>;
> > +		#address-cells = <1>;
> > +
> > +		CPU0: cpu@0 {
> > +			device_type = "cpu";
> > +			reg = <0x0>;
> > +		};
> > +
> > +		CPU1: cpu@1 {
> > +			device_type = "cpu";
> > +			reg = <0x1>;
> > +		};
> > +
> > +		CPU2: cpu@100 {
> > +			device_type = "cpu";
> > +			reg = <0x100>;
> > +		};
> > +
> > +		CPU3: cpu@101 {
> > +			device_type = "cpu";
> > +			reg = <0x101>;
> > +		};
> > +	};
> > diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
> > index aeae9c6..8dd51dc 100644
> > --- a/arch/arm/include/asm/prom.h
> > +++ b/arch/arm/include/asm/prom.h
> > @@ -15,6 +15,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 */
> >  
> > @@ -24,6 +25,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..c86e414 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,51 @@ 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) {
> 
> I think all /cpu nodes would have the right type. You could use
> for_each_child_of_node here.

Starting from /cpus, looked-up by path, right ?

> > +		const u32 *hwid;
> > +		int len;
> > +
> > +		pr_debug("  * %s...\n", dn->full_name);
> > +
> > +		hwid = of_get_property(dn, "reg", &len);
> > +
> 
> Use of_property_read_u32.

Ok.

Thanks,
Lorenzo

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

[Lorenzo Pieralisi]

Hi Rob,

thanks for having a look.

On Tue, Oct 16, 2012 at 09:42:43PM +0100, Rob Herring wrote:
> On 10/16/2012 08:21 AM, Lorenzo Pieralisi wrote:
> > 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>
> > ---
> >  Documentation/devicetree/bindings/arm/cpus.txt | 42 +++++++++++++++++++++++
> >  arch/arm/include/asm/prom.h                    |  2 ++
> >  arch/arm/kernel/devtree.c                      | 47 ++++++++++++++++++++++++++
> >  3 files changed, 91 insertions(+)
> >  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..897f3b4
> > --- /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
> 
> defining or matching the MPIDR?

I would say matching, otherwise it reads as if the MPIDR were writable.

> > +
> > +Every cpu node is required to set its device_type to "cpu".
> 
> This is a bit questionable as device_type is deprecated for FDT.
> However, since the ePAPR defines using it
> 
> You should add a compatible property for the cpu model.

Ok, I will.

> > +
> > +Example:
> > +
> > +	cpus {
> > +		#size-cells = <0>;
> > +		#address-cells = <1>;
> > +
> > +		CPU0: cpu at 0 {
> > +			device_type = "cpu";
> > +			reg = <0x0>;
> > +		};
> > +
> > +		CPU1: cpu at 1 {
> > +			device_type = "cpu";
> > +			reg = <0x1>;
> > +		};
> > +
> > +		CPU2: cpu at 100 {
> > +			device_type = "cpu";
> > +			reg = <0x100>;
> > +		};
> > +
> > +		CPU3: cpu at 101 {
> > +			device_type = "cpu";
> > +			reg = <0x101>;
> > +		};
> > +	};
> > diff --git a/arch/arm/include/asm/prom.h b/arch/arm/include/asm/prom.h
> > index aeae9c6..8dd51dc 100644
> > --- a/arch/arm/include/asm/prom.h
> > +++ b/arch/arm/include/asm/prom.h
> > @@ -15,6 +15,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 */
> >  
> > @@ -24,6 +25,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..c86e414 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,51 @@ 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) {
> 
> I think all /cpu nodes would have the right type. You could use
> for_each_child_of_node here.

Starting from /cpus, looked-up by path, right ?

> > +		const u32 *hwid;
> > +		int len;
> > +
> > +		pr_debug("  * %s...\n", dn->full_name);
> > +
> > +		hwid = of_get_property(dn, "reg", &len);
> > +
> 
> Use of_property_read_u32.

Ok.

Thanks,
Lorenzo

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

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:45PM +0100, Lorenzo Pieralisi wrote:
> 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-5wv7dgnIgG8@public.gmane.org>

[...]

> +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;
> +		}
> +		/*
> +		 * We want to assign the boot CPU logical id 0.
> +		 * Boot CPU checks its own MPIDR and if matches the current
> +		 * cpu node "reg" value it sets the logical cpu index to 0
> +		 * and stores the physical id accordingly.
> +		 * If MPIDR does not match, assign sequential cpu logical
> +		 * id (starting from 1) and increments it.
> +		 */
> +		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));

I don't think we should bother with this print -- we already print something
in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
the full mpidr, we could change that code to include it as well.

We might also want some sanity checking that we do indeed end up with
logical id 0 for the boot CPU. If not, I think we should scream and fall
back on the simple mapping created earlier.

Will

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

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:45PM +0100, Lorenzo Pieralisi wrote:
> 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>

[...]

> +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;
> +		}
> +		/*
> +		 * We want to assign the boot CPU logical id 0.
> +		 * Boot CPU checks its own MPIDR and if matches the current
> +		 * cpu node "reg" value it sets the logical cpu index to 0
> +		 * and stores the physical id accordingly.
> +		 * If MPIDR does not match, assign sequential cpu logical
> +		 * id (starting from 1) and increments it.
> +		 */
> +		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));

I don't think we should bother with this print -- we already print something
in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
the full mpidr, we could change that code to include it as well.

We might also want some sanity checking that we do indeed end up with
logical id 0 for the boot CPU. If not, I think we should scream and fall
back on the simple mapping created earlier.

Will

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

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:46PM +0100, Lorenzo Pieralisi wrote:
> 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-5wv7dgnIgG8@public.gmane.org>
> ---
>  arch/arm/kernel/setup.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index da1d1aa..20c530b 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -758,6 +758,7 @@ void __init setup_arch(char **cmdline_p)
>  
>  	unflatten_device_tree();
>  
> +	arm_dt_init_cpu_maps();
>  #ifdef CONFIG_SMP
>  	if (is_smp()) {
>  		smp_set_ops(mdesc->smp);
> -- 
> 1.7.12

Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Will

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

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:46PM +0100, Lorenzo Pieralisi wrote:
> 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>
> ---
>  arch/arm/kernel/setup.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index da1d1aa..20c530b 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -758,6 +758,7 @@ void __init setup_arch(char **cmdline_p)
>  
>  	unflatten_device_tree();
>  
> +	arm_dt_init_cpu_maps();
>  #ifdef CONFIG_SMP
>  	if (is_smp()) {
>  		smp_set_ops(mdesc->smp);
> -- 
> 1.7.12

Acked-by: Will Deacon <will.deacon@arm.com>

Will

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

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:47PM +0100, Lorenzo Pieralisi wrote:
> 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[23:0] so that 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-5wv7dgnIgG8@public.gmane.org>
> ---
>  arch/arm/include/asm/smp_plat.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
> index 558d6c8..aaa61b6 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>
>  
>  /*
> @@ -48,5 +51,19 @@ static inline int cache_ops_need_broadcast(void)
>   */
>  extern int __cpu_logical_map[];
>  #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
> +/*
> + * Retrieve logical cpu index corresponding to a given MPIDR[23:0]
> + *  - mpidr: MPIDR[23:0] to be used for the look-up
> + *
> + * Returns the cpu logical index or -EINVAL on look-up error
> + */
> +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

Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Will

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

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:47PM +0100, Lorenzo Pieralisi wrote:
> 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[23:0] so that 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>
> ---
>  arch/arm/include/asm/smp_plat.h | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
> index 558d6c8..aaa61b6 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>
>  
>  /*
> @@ -48,5 +51,19 @@ static inline int cache_ops_need_broadcast(void)
>   */
>  extern int __cpu_logical_map[];
>  #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
> +/*
> + * Retrieve logical cpu index corresponding to a given MPIDR[23:0]
> + *  - mpidr: MPIDR[23:0] to be used for the look-up
> + *
> + * Returns the cpu logical index or -EINVAL on look-up error
> + */
> +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

Acked-by: Will Deacon <will.deacon@arm.com>

Will

Re: [RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:48PM +0100, Lorenzo Pieralisi wrote:
> From: Nicolas Pitre <nicolas.pitre-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> The GIC interface numbering does not necessarily follow the logical
> CPU numbering, especially for complex topologies such as multi-cluster
> systems.
> 
> Fortunately we can easily probe the GIC to create a mapping as the
> Interrupt Processor Targets Registers for the first 32 interrupts are
> read-only, and each field returns a value that always corresponds to
> the processor reading the register.
> 
> Initially all mappings target all CPUs in case an IPI is required to
> boot secondary CPUs.  It is refined as those CPUs discover what their
> actual mapping is.
> 
> Signed-off-by: Nicolas Pitre <nico-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

This is a really neat idea!

> ---
>  arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index aa52699..1338a55 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -70,6 +70,13 @@ struct gic_chip_data {
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  
>  /*
> + * The GIC mapping of CPU interfaces does not necessarily match
> + * the logical CPU numbering.  Let's use a mapping as returned
> + * by the GIC itself.
> + */
> +static u8 gic_cpu_map[8] __read_mostly;

Can we have a #define for the number CPUs supported by the GIC? It gets
used a fair amount in this patch for loop bounds etc.

> +/*
>   * Supported arch specific GIC irq extension.
>   * Default make them NULL.
>   */
> @@ -242,7 +249,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 = gic_cpu_map[cpu] << shift;
>  
>  	raw_spin_lock(&irq_controller_lock);
>  	val = readl_relaxed(reg) & ~mask;
> @@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>  	u32 cpumask;
>  	unsigned int gic_irqs = gic->gic_irqs;
>  	void __iomem *base = gic_data_dist_base(gic);
> -	u32 cpu = cpu_logical_map(smp_processor_id());
> -
> -	cpumask = 1 << cpu;
> -	cpumask |= cpumask << 8;
> -	cpumask |= cpumask << 16;
>  
>  	writel_relaxed(0, base + GIC_DIST_CTRL);
>  
> @@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>  	/*
>  	 * Set all global interrupts to this CPU only.
>  	 */
> +	cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0);
>  	for (i = 32; i < gic_irqs; i += 4)
>  		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
>  
> @@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
>  {
>  	void __iomem *dist_base = gic_data_dist_base(gic);
>  	void __iomem *base = gic_data_cpu_base(gic);
> +	unsigned int cpu_mask, cpu = smp_processor_id();
>  	int i;
>  
>  	/*
> +	 * Get what the GIC says our CPU mask is.
> +	 */
> +	BUG_ON(cpu >= 8);
> +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);

Making the mask a u8 and using readb_relaxed here makes this bit of code
clearer to me (and the GIC apparently allows such an access to this
register).

> +	gic_cpu_map[cpu] = cpu_mask;
> +
> +	/*
> +	 * Clear our mask from the other map entries in case they're
> +	 * still undefined.
> +	 */
> +	for (i = 0; i < 8; i++)
> +		if (i != cpu)
> +			gic_cpu_map[i] &= ~cpu_mask;
> +
> +	/*
>  	 * Deal with the banked PPI and SGI interrupts - disable all
>  	 * PPI interrupts, ensure all SGI interrupts are enabled.
>  	 */
> @@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  {
>  	irq_hw_number_t hwirq_base;
>  	struct gic_chip_data *gic;
> -	int gic_irqs, irq_base;
> +	int gic_irqs, irq_base, i;
>  
>  	BUG_ON(gic_nr >= MAX_GIC_NR);
>  
> @@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  	}
>  
>  	/*
> +	 * Initialize the CPU interface map to all CPUs.
> +	 * It will be refined as each CPU probes its ID.
> +	 */
> +	for (i = 0; i < 8; i++)
> +		gic_cpu_map[i] = 0xff;
> +
> +	/*
>  	 * For primary GICs, skip over SGIs.
>  	 * For secondary GICs, skip over PPIs, too.
>  	 */
> @@ -737,7 +763,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 |= gic_cpu_map[cpu];
>  
>  	/*
>  	 * Ensure that stores to Normal memory are visible to the

With those changes:

Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>

Will

[RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Will Deacon]

On Tue, Oct 16, 2012 at 02:21:48PM +0100, Lorenzo Pieralisi wrote:
> From: Nicolas Pitre <nicolas.pitre@linaro.org>
> 
> The GIC interface numbering does not necessarily follow the logical
> CPU numbering, especially for complex topologies such as multi-cluster
> systems.
> 
> Fortunately we can easily probe the GIC to create a mapping as the
> Interrupt Processor Targets Registers for the first 32 interrupts are
> read-only, and each field returns a value that always corresponds to
> the processor reading the register.
> 
> Initially all mappings target all CPUs in case an IPI is required to
> boot secondary CPUs.  It is refined as those CPUs discover what their
> actual mapping is.
> 
> Signed-off-by: Nicolas Pitre <nico@linaro.org>

This is a really neat idea!

> ---
>  arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 34 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> index aa52699..1338a55 100644
> --- a/arch/arm/common/gic.c
> +++ b/arch/arm/common/gic.c
> @@ -70,6 +70,13 @@ struct gic_chip_data {
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>  
>  /*
> + * The GIC mapping of CPU interfaces does not necessarily match
> + * the logical CPU numbering.  Let's use a mapping as returned
> + * by the GIC itself.
> + */
> +static u8 gic_cpu_map[8] __read_mostly;

Can we have a #define for the number CPUs supported by the GIC? It gets
used a fair amount in this patch for loop bounds etc.

> +/*
>   * Supported arch specific GIC irq extension.
>   * Default make them NULL.
>   */
> @@ -242,7 +249,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 = gic_cpu_map[cpu] << shift;
>  
>  	raw_spin_lock(&irq_controller_lock);
>  	val = readl_relaxed(reg) & ~mask;
> @@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>  	u32 cpumask;
>  	unsigned int gic_irqs = gic->gic_irqs;
>  	void __iomem *base = gic_data_dist_base(gic);
> -	u32 cpu = cpu_logical_map(smp_processor_id());
> -
> -	cpumask = 1 << cpu;
> -	cpumask |= cpumask << 8;
> -	cpumask |= cpumask << 16;
>  
>  	writel_relaxed(0, base + GIC_DIST_CTRL);
>  
> @@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
>  	/*
>  	 * Set all global interrupts to this CPU only.
>  	 */
> +	cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0);
>  	for (i = 32; i < gic_irqs; i += 4)
>  		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
>  
> @@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
>  {
>  	void __iomem *dist_base = gic_data_dist_base(gic);
>  	void __iomem *base = gic_data_cpu_base(gic);
> +	unsigned int cpu_mask, cpu = smp_processor_id();
>  	int i;
>  
>  	/*
> +	 * Get what the GIC says our CPU mask is.
> +	 */
> +	BUG_ON(cpu >= 8);
> +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);

Making the mask a u8 and using readb_relaxed here makes this bit of code
clearer to me (and the GIC apparently allows such an access to this
register).

> +	gic_cpu_map[cpu] = cpu_mask;
> +
> +	/*
> +	 * Clear our mask from the other map entries in case they're
> +	 * still undefined.
> +	 */
> +	for (i = 0; i < 8; i++)
> +		if (i != cpu)
> +			gic_cpu_map[i] &= ~cpu_mask;
> +
> +	/*
>  	 * Deal with the banked PPI and SGI interrupts - disable all
>  	 * PPI interrupts, ensure all SGI interrupts are enabled.
>  	 */
> @@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  {
>  	irq_hw_number_t hwirq_base;
>  	struct gic_chip_data *gic;
> -	int gic_irqs, irq_base;
> +	int gic_irqs, irq_base, i;
>  
>  	BUG_ON(gic_nr >= MAX_GIC_NR);
>  
> @@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>  	}
>  
>  	/*
> +	 * Initialize the CPU interface map to all CPUs.
> +	 * It will be refined as each CPU probes its ID.
> +	 */
> +	for (i = 0; i < 8; i++)
> +		gic_cpu_map[i] = 0xff;
> +
> +	/*
>  	 * For primary GICs, skip over SGIs.
>  	 * For secondary GICs, skip over PPIs, too.
>  	 */
> @@ -737,7 +763,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 |= gic_cpu_map[cpu];
>  
>  	/*
>  	 * Ensure that stores to Normal memory are visible to the

With those changes:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

Re: [RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Nicolas Pitre]

On Tue, 6 Nov 2012, Will Deacon wrote:

> On Tue, Oct 16, 2012 at 02:21:48PM +0100, Lorenzo Pieralisi wrote:
> > From: Nicolas Pitre <nicolas.pitre-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > 
> > The GIC interface numbering does not necessarily follow the logical
> > CPU numbering, especially for complex topologies such as multi-cluster
> > systems.
> > 
> > Fortunately we can easily probe the GIC to create a mapping as the
> > Interrupt Processor Targets Registers for the first 32 interrupts are
> > read-only, and each field returns a value that always corresponds to
> > the processor reading the register.
> > 
> > Initially all mappings target all CPUs in case an IPI is required to
> > boot secondary CPUs.  It is refined as those CPUs discover what their
> > actual mapping is.
> > 
> > Signed-off-by: Nicolas Pitre <nico-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> This is a really neat idea!
> 
> > ---
> >  arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 34 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> > index aa52699..1338a55 100644
> > --- a/arch/arm/common/gic.c
> > +++ b/arch/arm/common/gic.c
> > @@ -70,6 +70,13 @@ struct gic_chip_data {
> >  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> >  
> >  /*
> > + * The GIC mapping of CPU interfaces does not necessarily match
> > + * the logical CPU numbering.  Let's use a mapping as returned
> > + * by the GIC itself.
> > + */
> > +static u8 gic_cpu_map[8] __read_mostly;
> 
> Can we have a #define for the number CPUs supported by the GIC? It gets
> used a fair amount in this patch for loop bounds etc.

Sure.  I'll respin the patch.

> 
> > +/*
> >   * Supported arch specific GIC irq extension.
> >   * Default make them NULL.
> >   */
> > @@ -242,7 +249,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 = gic_cpu_map[cpu] << shift;
> >  
> >  	raw_spin_lock(&irq_controller_lock);
> >  	val = readl_relaxed(reg) & ~mask;
> > @@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
> >  	u32 cpumask;
> >  	unsigned int gic_irqs = gic->gic_irqs;
> >  	void __iomem *base = gic_data_dist_base(gic);
> > -	u32 cpu = cpu_logical_map(smp_processor_id());
> > -
> > -	cpumask = 1 << cpu;
> > -	cpumask |= cpumask << 8;
> > -	cpumask |= cpumask << 16;
> >  
> >  	writel_relaxed(0, base + GIC_DIST_CTRL);
> >  
> > @@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
> >  	/*
> >  	 * Set all global interrupts to this CPU only.
> >  	 */
> > +	cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0);
> >  	for (i = 32; i < gic_irqs; i += 4)
> >  		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
> >  
> > @@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
> >  {
> >  	void __iomem *dist_base = gic_data_dist_base(gic);
> >  	void __iomem *base = gic_data_cpu_base(gic);
> > +	unsigned int cpu_mask, cpu = smp_processor_id();
> >  	int i;
> >  
> >  	/*
> > +	 * Get what the GIC says our CPU mask is.
> > +	 */
> > +	BUG_ON(cpu >= 8);
> > +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
> 
> Making the mask a u8 and using readb_relaxed here makes this bit of code
> clearer to me (and the GIC apparently allows such an access to this
> register).

Not always.  At least RTSM throws an exception if you do so.
Been there.

> > +	gic_cpu_map[cpu] = cpu_mask;
> > +
> > +	/*
> > +	 * Clear our mask from the other map entries in case they're
> > +	 * still undefined.
> > +	 */
> > +	for (i = 0; i < 8; i++)
> > +		if (i != cpu)
> > +			gic_cpu_map[i] &= ~cpu_mask;
> > +
> > +	/*
> >  	 * Deal with the banked PPI and SGI interrupts - disable all
> >  	 * PPI interrupts, ensure all SGI interrupts are enabled.
> >  	 */
> > @@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >  {
> >  	irq_hw_number_t hwirq_base;
> >  	struct gic_chip_data *gic;
> > -	int gic_irqs, irq_base;
> > +	int gic_irqs, irq_base, i;
> >  
> >  	BUG_ON(gic_nr >= MAX_GIC_NR);
> >  
> > @@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >  	}
> >  
> >  	/*
> > +	 * Initialize the CPU interface map to all CPUs.
> > +	 * It will be refined as each CPU probes its ID.
> > +	 */
> > +	for (i = 0; i < 8; i++)
> > +		gic_cpu_map[i] = 0xff;
> > +
> > +	/*
> >  	 * For primary GICs, skip over SGIs.
> >  	 * For secondary GICs, skip over PPIs, too.
> >  	 */
> > @@ -737,7 +763,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 |= gic_cpu_map[cpu];
> >  
> >  	/*
> >  	 * Ensure that stores to Normal memory are visible to the
> 
> With those changes:
> 
> Acked-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> 
> Will
> 

[RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Nicolas Pitre]

On Tue, 6 Nov 2012, Will Deacon wrote:

> On Tue, Oct 16, 2012 at 02:21:48PM +0100, Lorenzo Pieralisi wrote:
> > From: Nicolas Pitre <nicolas.pitre@linaro.org>
> > 
> > The GIC interface numbering does not necessarily follow the logical
> > CPU numbering, especially for complex topologies such as multi-cluster
> > systems.
> > 
> > Fortunately we can easily probe the GIC to create a mapping as the
> > Interrupt Processor Targets Registers for the first 32 interrupts are
> > read-only, and each field returns a value that always corresponds to
> > the processor reading the register.
> > 
> > Initially all mappings target all CPUs in case an IPI is required to
> > boot secondary CPUs.  It is refined as those CPUs discover what their
> > actual mapping is.
> > 
> > Signed-off-by: Nicolas Pitre <nico@linaro.org>
> 
> This is a really neat idea!
> 
> > ---
> >  arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 34 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> > index aa52699..1338a55 100644
> > --- a/arch/arm/common/gic.c
> > +++ b/arch/arm/common/gic.c
> > @@ -70,6 +70,13 @@ struct gic_chip_data {
> >  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> >  
> >  /*
> > + * The GIC mapping of CPU interfaces does not necessarily match
> > + * the logical CPU numbering.  Let's use a mapping as returned
> > + * by the GIC itself.
> > + */
> > +static u8 gic_cpu_map[8] __read_mostly;
> 
> Can we have a #define for the number CPUs supported by the GIC? It gets
> used a fair amount in this patch for loop bounds etc.

Sure.  I'll respin the patch.

> 
> > +/*
> >   * Supported arch specific GIC irq extension.
> >   * Default make them NULL.
> >   */
> > @@ -242,7 +249,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 = gic_cpu_map[cpu] << shift;
> >  
> >  	raw_spin_lock(&irq_controller_lock);
> >  	val = readl_relaxed(reg) & ~mask;
> > @@ -349,11 +356,6 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
> >  	u32 cpumask;
> >  	unsigned int gic_irqs = gic->gic_irqs;
> >  	void __iomem *base = gic_data_dist_base(gic);
> > -	u32 cpu = cpu_logical_map(smp_processor_id());
> > -
> > -	cpumask = 1 << cpu;
> > -	cpumask |= cpumask << 8;
> > -	cpumask |= cpumask << 16;
> >  
> >  	writel_relaxed(0, base + GIC_DIST_CTRL);
> >  
> > @@ -366,6 +368,7 @@ static void __init gic_dist_init(struct gic_chip_data *gic)
> >  	/*
> >  	 * Set all global interrupts to this CPU only.
> >  	 */
> > +	cpumask = readl_relaxed(base + GIC_DIST_TARGET + 0);
> >  	for (i = 32; i < gic_irqs; i += 4)
> >  		writel_relaxed(cpumask, base + GIC_DIST_TARGET + i * 4 / 4);
> >  
> > @@ -389,9 +392,25 @@ static void __cpuinit gic_cpu_init(struct gic_chip_data *gic)
> >  {
> >  	void __iomem *dist_base = gic_data_dist_base(gic);
> >  	void __iomem *base = gic_data_cpu_base(gic);
> > +	unsigned int cpu_mask, cpu = smp_processor_id();
> >  	int i;
> >  
> >  	/*
> > +	 * Get what the GIC says our CPU mask is.
> > +	 */
> > +	BUG_ON(cpu >= 8);
> > +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
> 
> Making the mask a u8 and using readb_relaxed here makes this bit of code
> clearer to me (and the GIC apparently allows such an access to this
> register).

Not always.  At least RTSM throws an exception if you do so.
Been there.

> > +	gic_cpu_map[cpu] = cpu_mask;
> > +
> > +	/*
> > +	 * Clear our mask from the other map entries in case they're
> > +	 * still undefined.
> > +	 */
> > +	for (i = 0; i < 8; i++)
> > +		if (i != cpu)
> > +			gic_cpu_map[i] &= ~cpu_mask;
> > +
> > +	/*
> >  	 * Deal with the banked PPI and SGI interrupts - disable all
> >  	 * PPI interrupts, ensure all SGI interrupts are enabled.
> >  	 */
> > @@ -646,7 +665,7 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >  {
> >  	irq_hw_number_t hwirq_base;
> >  	struct gic_chip_data *gic;
> > -	int gic_irqs, irq_base;
> > +	int gic_irqs, irq_base, i;
> >  
> >  	BUG_ON(gic_nr >= MAX_GIC_NR);
> >  
> > @@ -683,6 +702,13 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
> >  	}
> >  
> >  	/*
> > +	 * Initialize the CPU interface map to all CPUs.
> > +	 * It will be refined as each CPU probes its ID.
> > +	 */
> > +	for (i = 0; i < 8; i++)
> > +		gic_cpu_map[i] = 0xff;
> > +
> > +	/*
> >  	 * For primary GICs, skip over SGIs.
> >  	 * For secondary GICs, skip over PPIs, too.
> >  	 */
> > @@ -737,7 +763,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 |= gic_cpu_map[cpu];
> >  
> >  	/*
> >  	 * Ensure that stores to Normal memory are visible to the
> 
> With those changes:
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> Will
> 

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

[Lorenzo Pieralisi]

Hi Will,

thanks for the review on the series.

On Tue, Nov 06, 2012 at 09:50:44PM +0000, Will Deacon wrote:
> On Tue, Oct 16, 2012 at 02:21:45PM +0100, Lorenzo Pieralisi wrote:
> > 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>
> 
> [...]
> 
> > +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;
> > +		}
> > +		/*
> > +		 * We want to assign the boot CPU logical id 0.
> > +		 * Boot CPU checks its own MPIDR and if matches the current
> > +		 * cpu node "reg" value it sets the logical cpu index to 0
> > +		 * and stores the physical id accordingly.
> > +		 * If MPIDR does not match, assign sequential cpu logical
> > +		 * id (starting from 1) and increments it.
> > +		 */
> > +		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));
> 
> I don't think we should bother with this print -- we already print something
> in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
> the full mpidr, we could change that code to include it as well.

Yes, it is duplicate code, I will remove it. Extending the printk in
smp_setup_processor_id() to include the entire MPIDR should be done
though, otherwise we might have printk aliases on system with multiple
clusters.

> 
> We might also want some sanity checking that we do indeed end up with
> logical id 0 for the boot CPU. If not, I think we should scream and fall
> back on the simple mapping created earlier.

Well, this basically means that we have to make sure this function is
executed on the boot CPU (and it is as the code stands now), right ?

Since we are reading the MPIDR of the CPU parsing the tree and assign logical
cpu 0 accordingly I think we have this check carried out automatically,
unless for any given reason someone calls this function on a CPU that is
not the boot one (Why ?).

Basically I could add a check like:

if (WARN_ON(smp_processor_id())
	return;

to fall back to the previous mapping, but that's overkill IMHO.

Thanks,
Lorenzo

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

[Lorenzo Pieralisi]

Hi Will,

thanks for the review on the series.

On Tue, Nov 06, 2012 at 09:50:44PM +0000, Will Deacon wrote:
> On Tue, Oct 16, 2012 at 02:21:45PM +0100, Lorenzo Pieralisi wrote:
> > 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>
> 
> [...]
> 
> > +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;
> > +		}
> > +		/*
> > +		 * We want to assign the boot CPU logical id 0.
> > +		 * Boot CPU checks its own MPIDR and if matches the current
> > +		 * cpu node "reg" value it sets the logical cpu index to 0
> > +		 * and stores the physical id accordingly.
> > +		 * If MPIDR does not match, assign sequential cpu logical
> > +		 * id (starting from 1) and increments it.
> > +		 */
> > +		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));
> 
> I don't think we should bother with this print -- we already print something
> in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
> the full mpidr, we could change that code to include it as well.

Yes, it is duplicate code, I will remove it. Extending the printk in
smp_setup_processor_id() to include the entire MPIDR should be done
though, otherwise we might have printk aliases on system with multiple
clusters.

> 
> We might also want some sanity checking that we do indeed end up with
> logical id 0 for the boot CPU. If not, I think we should scream and fall
> back on the simple mapping created earlier.

Well, this basically means that we have to make sure this function is
executed on the boot CPU (and it is as the code stands now), right ?

Since we are reading the MPIDR of the CPU parsing the tree and assign logical
cpu 0 accordingly I think we have this check carried out automatically,
unless for any given reason someone calls this function on a CPU that is
not the boot one (Why ?).

Basically I could add a check like:

if (WARN_ON(smp_processor_id())
	return;

to fall back to the previous mapping, but that's overkill IMHO.

Thanks,
Lorenzo

Re: [RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Will Deacon]

On Tue, Nov 06, 2012 at 10:59:35PM +0000, Nicolas Pitre wrote:
> On Tue, 6 Nov 2012, Will Deacon wrote:
> > >  arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 34 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> > > index aa52699..1338a55 100644
> > > --- a/arch/arm/common/gic.c
> > > +++ b/arch/arm/common/gic.c
> > > @@ -70,6 +70,13 @@ struct gic_chip_data {
> > >  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> > >  
> > >  /*
> > > + * The GIC mapping of CPU interfaces does not necessarily match
> > > + * the logical CPU numbering.  Let's use a mapping as returned
> > > + * by the GIC itself.
> > > + */
> > > +static u8 gic_cpu_map[8] __read_mostly;
> > 
> > Can we have a #define for the number CPUs supported by the GIC? It gets
> > used a fair amount in this patch for loop bounds etc.
> 
> Sure.  I'll respin the patch.

Cheers Nicolas.

> > >  	/*
> > > +	 * Get what the GIC says our CPU mask is.
> > > +	 */
> > > +	BUG_ON(cpu >= 8);
> > > +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
> > 
> > Making the mask a u8 and using readb_relaxed here makes this bit of code
> > clearer to me (and the GIC apparently allows such an access to this
> > register).
> 
> Not always.  At least RTSM throws an exception if you do so.
> Been there.

That would be a bug in the RTSM then. Have you reported it to support? (if
not, I can chase this one up). I'd rather we just fix the model than work
around it in Linux.

Will

[RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Will Deacon]

On Tue, Nov 06, 2012 at 10:59:35PM +0000, Nicolas Pitre wrote:
> On Tue, 6 Nov 2012, Will Deacon wrote:
> > >  arch/arm/common/gic.c | 42 ++++++++++++++++++++++++++++++++++--------
> > >  1 file changed, 34 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
> > > index aa52699..1338a55 100644
> > > --- a/arch/arm/common/gic.c
> > > +++ b/arch/arm/common/gic.c
> > > @@ -70,6 +70,13 @@ struct gic_chip_data {
> > >  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> > >  
> > >  /*
> > > + * The GIC mapping of CPU interfaces does not necessarily match
> > > + * the logical CPU numbering.  Let's use a mapping as returned
> > > + * by the GIC itself.
> > > + */
> > > +static u8 gic_cpu_map[8] __read_mostly;
> > 
> > Can we have a #define for the number CPUs supported by the GIC? It gets
> > used a fair amount in this patch for loop bounds etc.
> 
> Sure.  I'll respin the patch.

Cheers Nicolas.

> > >  	/*
> > > +	 * Get what the GIC says our CPU mask is.
> > > +	 */
> > > +	BUG_ON(cpu >= 8);
> > > +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
> > 
> > Making the mask a u8 and using readb_relaxed here makes this bit of code
> > clearer to me (and the GIC apparently allows such an access to this
> > register).
> 
> Not always.  At least RTSM throws an exception if you do so.
> Been there.

That would be a bug in the RTSM then. Have you reported it to support? (if
not, I can chase this one up). I'd rather we just fix the model than work
around it in Linux.

Will

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

[Will Deacon]

On Wed, Nov 07, 2012 at 10:23:49AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 06, 2012 at 09:50:44PM +0000, Will Deacon wrote:
> > > +	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;
> > > +		}
> > > +		/*
> > > +		 * We want to assign the boot CPU logical id 0.
> > > +		 * Boot CPU checks its own MPIDR and if matches the current
> > > +		 * cpu node "reg" value it sets the logical cpu index to 0
> > > +		 * and stores the physical id accordingly.
> > > +		 * If MPIDR does not match, assign sequential cpu logical
> > > +		 * id (starting from 1) and increments it.
> > > +		 */
> > > +		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));
> > 
> > I don't think we should bother with this print -- we already print something
> > in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
> > the full mpidr, we could change that code to include it as well.
> 
> Yes, it is duplicate code, I will remove it. Extending the printk in
> smp_setup_processor_id() to include the entire MPIDR should be done
> though, otherwise we might have printk aliases on system with multiple
> clusters.

Feel free to make that change. You could also replace NR_CPUS in
smp_setup_processor_id with nr_cpu_ids for consistency (they'll be the same
this early on).

> > 
> > We might also want some sanity checking that we do indeed end up with
> > logical id 0 for the boot CPU. If not, I think we should scream and fall
> > back on the simple mapping created earlier.
> 
> Well, this basically means that we have to make sure this function is
> executed on the boot CPU (and it is as the code stands now), right ?

Yes, smp is not up yet which is why we're allowed to play with the logical
map.

> Since we are reading the MPIDR of the CPU parsing the tree and assign logical
> cpu 0 accordingly I think we have this check carried out automatically,
> unless for any given reason someone calls this function on a CPU that is
> not the boot one (Why ?).
> 
> Basically I could add a check like:
> 
> if (WARN_ON(smp_processor_id())
> 	return;
> 
> to fall back to the previous mapping, but that's overkill IMHO.

No, I was thinking about what happens if the devicetree doesn't contain an
mpidr property that matches the boot cpu. In this case, we will fail to
assign logical ID 0, right? If this happens, we should complain about an
invalid devicetree and try to fall back on the logical_map that was
generated earlier on.

Will

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

[Will Deacon]

On Wed, Nov 07, 2012 at 10:23:49AM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 06, 2012 at 09:50:44PM +0000, Will Deacon wrote:
> > > +	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;
> > > +		}
> > > +		/*
> > > +		 * We want to assign the boot CPU logical id 0.
> > > +		 * Boot CPU checks its own MPIDR and if matches the current
> > > +		 * cpu node "reg" value it sets the logical cpu index to 0
> > > +		 * and stores the physical id accordingly.
> > > +		 * If MPIDR does not match, assign sequential cpu logical
> > > +		 * id (starting from 1) and increments it.
> > > +		 */
> > > +		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));
> > 
> > I don't think we should bother with this print -- we already print something
> > in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
> > the full mpidr, we could change that code to include it as well.
> 
> Yes, it is duplicate code, I will remove it. Extending the printk in
> smp_setup_processor_id() to include the entire MPIDR should be done
> though, otherwise we might have printk aliases on system with multiple
> clusters.

Feel free to make that change. You could also replace NR_CPUS in
smp_setup_processor_id with nr_cpu_ids for consistency (they'll be the same
this early on).

> > 
> > We might also want some sanity checking that we do indeed end up with
> > logical id 0 for the boot CPU. If not, I think we should scream and fall
> > back on the simple mapping created earlier.
> 
> Well, this basically means that we have to make sure this function is
> executed on the boot CPU (and it is as the code stands now), right ?

Yes, smp is not up yet which is why we're allowed to play with the logical
map.

> Since we are reading the MPIDR of the CPU parsing the tree and assign logical
> cpu 0 accordingly I think we have this check carried out automatically,
> unless for any given reason someone calls this function on a CPU that is
> not the boot one (Why ?).
> 
> Basically I could add a check like:
> 
> if (WARN_ON(smp_processor_id())
> 	return;
> 
> to fall back to the previous mapping, but that's overkill IMHO.

No, I was thinking about what happens if the devicetree doesn't contain an
mpidr property that matches the boot cpu. In this case, we will fail to
assign logical ID 0, right? If this happens, we should complain about an
invalid devicetree and try to fall back on the logical_map that was
generated earlier on.

Will

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

[Lorenzo Pieralisi]

On Wed, Nov 07, 2012 at 11:05:42AM +0000, Will Deacon wrote:
> On Wed, Nov 07, 2012 at 10:23:49AM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Nov 06, 2012 at 09:50:44PM +0000, Will Deacon wrote:
> > > > +	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;
> > > > +		}
> > > > +		/*
> > > > +		 * We want to assign the boot CPU logical id 0.
> > > > +		 * Boot CPU checks its own MPIDR and if matches the current
> > > > +		 * cpu node "reg" value it sets the logical cpu index to 0
> > > > +		 * and stores the physical id accordingly.
> > > > +		 * If MPIDR does not match, assign sequential cpu logical
> > > > +		 * id (starting from 1) and increments it.
> > > > +		 */
> > > > +		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));
> > > 
> > > I don't think we should bother with this print -- we already print something
> > > in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
> > > the full mpidr, we could change that code to include it as well.
> > 
> > Yes, it is duplicate code, I will remove it. Extending the printk in
> > smp_setup_processor_id() to include the entire MPIDR should be done
> > though, otherwise we might have printk aliases on system with multiple
> > clusters.
> 
> Feel free to make that change. You could also replace NR_CPUS in
> smp_setup_processor_id with nr_cpu_ids for consistency (they'll be the same
> this early on).

Ok.

> > > We might also want some sanity checking that we do indeed end up with
> > > logical id 0 for the boot CPU. If not, I think we should scream and fall
> > > back on the simple mapping created earlier.
> > 
> > Well, this basically means that we have to make sure this function is
> > executed on the boot CPU (and it is as the code stands now), right ?
> 
> Yes, smp is not up yet which is why we're allowed to play with the logical
> map.
> 
> > Since we are reading the MPIDR of the CPU parsing the tree and assign logical
> > cpu 0 accordingly I think we have this check carried out automatically,
> > unless for any given reason someone calls this function on a CPU that is
> > not the boot one (Why ?).
> > 
> > Basically I could add a check like:
> > 
> > if (WARN_ON(smp_processor_id())
> > 	return;
> > 
> > to fall back to the previous mapping, but that's overkill IMHO.
> 
> No, I was thinking about what happens if the devicetree doesn't contain an
> mpidr property that matches the boot cpu. In this case, we will fail to
> assign logical ID 0, right? If this happens, we should complain about an
> invalid devicetree and try to fall back on the logical_map that was
> generated earlier on.

Good point. What I could do, I can assign the MPIDR of the boot CPU to
the logical index 0 before even starting to parse the DT (that's what it
is done in smp_setup_processor_id(), with a couple of twists). Then, if I
find a node that matches the boot CPU mpidr I just skip over it. This
way the boot CPU MPIDR will always be correct the only difference with
the current approach will be that instead of generating the secondaries
MPIDRs we will read them from DT.

The problem with this approach is that if we need a pointer (phandle) to the
boot CPU DT node through the MPIDR and the boot CPU node is botched or missing
we still behave as if the DT CPU nodes were ok.

I think I'd better stick a warning condition in there if the boot CPU
node is not present or botched (from a MPIDR perspective at least).

Thoughts ?
Lorenzo

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

[Lorenzo Pieralisi]

On Wed, Nov 07, 2012 at 11:05:42AM +0000, Will Deacon wrote:
> On Wed, Nov 07, 2012 at 10:23:49AM +0000, Lorenzo Pieralisi wrote:
> > On Tue, Nov 06, 2012 at 09:50:44PM +0000, Will Deacon wrote:
> > > > +	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;
> > > > +		}
> > > > +		/*
> > > > +		 * We want to assign the boot CPU logical id 0.
> > > > +		 * Boot CPU checks its own MPIDR and if matches the current
> > > > +		 * cpu node "reg" value it sets the logical cpu index to 0
> > > > +		 * and stores the physical id accordingly.
> > > > +		 * If MPIDR does not match, assign sequential cpu logical
> > > > +		 * id (starting from 1) and increments it.
> > > > +		 */
> > > > +		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));
> > > 
> > > I don't think we should bother with this print -- we already print something
> > > in smp_setup_processor_id, which cannot differ for the boot CPU, If you want
> > > the full mpidr, we could change that code to include it as well.
> > 
> > Yes, it is duplicate code, I will remove it. Extending the printk in
> > smp_setup_processor_id() to include the entire MPIDR should be done
> > though, otherwise we might have printk aliases on system with multiple
> > clusters.
> 
> Feel free to make that change. You could also replace NR_CPUS in
> smp_setup_processor_id with nr_cpu_ids for consistency (they'll be the same
> this early on).

Ok.

> > > We might also want some sanity checking that we do indeed end up with
> > > logical id 0 for the boot CPU. If not, I think we should scream and fall
> > > back on the simple mapping created earlier.
> > 
> > Well, this basically means that we have to make sure this function is
> > executed on the boot CPU (and it is as the code stands now), right ?
> 
> Yes, smp is not up yet which is why we're allowed to play with the logical
> map.
> 
> > Since we are reading the MPIDR of the CPU parsing the tree and assign logical
> > cpu 0 accordingly I think we have this check carried out automatically,
> > unless for any given reason someone calls this function on a CPU that is
> > not the boot one (Why ?).
> > 
> > Basically I could add a check like:
> > 
> > if (WARN_ON(smp_processor_id())
> > 	return;
> > 
> > to fall back to the previous mapping, but that's overkill IMHO.
> 
> No, I was thinking about what happens if the devicetree doesn't contain an
> mpidr property that matches the boot cpu. In this case, we will fail to
> assign logical ID 0, right? If this happens, we should complain about an
> invalid devicetree and try to fall back on the logical_map that was
> generated earlier on.

Good point. What I could do, I can assign the MPIDR of the boot CPU to
the logical index 0 before even starting to parse the DT (that's what it
is done in smp_setup_processor_id(), with a couple of twists). Then, if I
find a node that matches the boot CPU mpidr I just skip over it. This
way the boot CPU MPIDR will always be correct the only difference with
the current approach will be that instead of generating the secondaries
MPIDRs we will read them from DT.

The problem with this approach is that if we need a pointer (phandle) to the
boot CPU DT node through the MPIDR and the boot CPU node is botched or missing
we still behave as if the DT CPU nodes were ok.

I think I'd better stick a warning condition in there if the boot CPU
node is not present or botched (from a MPIDR perspective at least).

Thoughts ?
Lorenzo

Re: [RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Nicolas Pitre]

On Wed, 7 Nov 2012, Will Deacon wrote:

> On Tue, Nov 06, 2012 at 10:59:35PM +0000, Nicolas Pitre wrote:
> > On Tue, 6 Nov 2012, Will Deacon wrote:
> > > >  	/*
> > > > +	 * Get what the GIC says our CPU mask is.
> > > > +	 */
> > > > +	BUG_ON(cpu >= 8);
> > > > +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
> > > 
> > > Making the mask a u8 and using readb_relaxed here makes this bit of code
> > > clearer to me (and the GIC apparently allows such an access to this
> > > register).
> > 
> > Not always.  At least RTSM throws an exception if you do so.
> > Been there.
> 
> That would be a bug in the RTSM then. Have you reported it to support? (if
> not, I can chase this one up). I'd rather we just fix the model than work
> around it in Linux.

I have no problem with you chasing it down with the support people.

I don't want to wait for fixed RTSM versions to be released and the 
whole world to migrate to them though.

While the readl is maybe marginally unintuitive compared to a readb 
here, the code is always using readl everywhere else already, even using 
bit masking and shifting when a readb/writeb could have made the code 
much simpler (see gic_set_affinity() for example).  I therefore much 
prefer to stick to a proven 32-bit access than risking regression on 
some possible implementation where the 8-bit access wasn't properly 
implemented as the doc says it should and never exercised before.

In other words, I prefer erring on the safe side here.


Nicolas

[RFC PATCH 4/4] ARM: gic: use a private mapping for CPU target interfaces

[Nicolas Pitre]

On Wed, 7 Nov 2012, Will Deacon wrote:

> On Tue, Nov 06, 2012 at 10:59:35PM +0000, Nicolas Pitre wrote:
> > On Tue, 6 Nov 2012, Will Deacon wrote:
> > > >  	/*
> > > > +	 * Get what the GIC says our CPU mask is.
> > > > +	 */
> > > > +	BUG_ON(cpu >= 8);
> > > > +	cpu_mask = readl_relaxed(dist_base + GIC_DIST_TARGET + 0);
> > > 
> > > Making the mask a u8 and using readb_relaxed here makes this bit of code
> > > clearer to me (and the GIC apparently allows such an access to this
> > > register).
> > 
> > Not always.  At least RTSM throws an exception if you do so.
> > Been there.
> 
> That would be a bug in the RTSM then. Have you reported it to support? (if
> not, I can chase this one up). I'd rather we just fix the model than work
> around it in Linux.

I have no problem with you chasing it down with the support people.

I don't want to wait for fixed RTSM versions to be released and the 
whole world to migrate to them though.

While the readl is maybe marginally unintuitive compared to a readb 
here, the code is always using readl everywhere else already, even using 
bit masking and shifting when a readb/writeb could have made the code 
much simpler (see gic_set_affinity() for example).  I therefore much 
prefer to stick to a proven 32-bit access than risking regression on 
some possible implementation where the 8-bit access wasn't properly 
implemented as the doc says it should and never exercised before.

In other words, I prefer erring on the safe side here.


Nicolas

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

[Will Deacon]

On Wed, Nov 07, 2012 at 12:00:52PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Nov 07, 2012 at 11:05:42AM +0000, Will Deacon wrote:
> > No, I was thinking about what happens if the devicetree doesn't contain an
> > mpidr property that matches the boot cpu. In this case, we will fail to
> > assign logical ID 0, right? If this happens, we should complain about an
> > invalid devicetree and try to fall back on the logical_map that was
> > generated earlier on.
> 
> Good point. What I could do, I can assign the MPIDR of the boot CPU to
> the logical index 0 before even starting to parse the DT (that's what it
> is done in smp_setup_processor_id(), with a couple of twists). Then, if I
> find a node that matches the boot CPU mpidr I just skip over it. This
> way the boot CPU MPIDR will always be correct the only difference with
> the current approach will be that instead of generating the secondaries
> MPIDRs we will read them from DT.

That should work, although I'm not sure why you can't just give up
altogether and use the initial mapping from smp_setup_processor_id?

> The problem with this approach is that if we need a pointer (phandle) to the
> boot CPU DT node through the MPIDR and the boot CPU node is botched or missing
> we still behave as if the DT CPU nodes were ok.

Does any code do this? Wouldn't it be much better to lookup logical CPU 0 if
you want to find anything out about the boot CPU?

> I think I'd better stick a warning condition in there if the boot CPU
> node is not present or botched (from a MPIDR perspective at least).

Definitely!

Will

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

[Will Deacon]

On Wed, Nov 07, 2012 at 12:00:52PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Nov 07, 2012 at 11:05:42AM +0000, Will Deacon wrote:
> > No, I was thinking about what happens if the devicetree doesn't contain an
> > mpidr property that matches the boot cpu. In this case, we will fail to
> > assign logical ID 0, right? If this happens, we should complain about an
> > invalid devicetree and try to fall back on the logical_map that was
> > generated earlier on.
> 
> Good point. What I could do, I can assign the MPIDR of the boot CPU to
> the logical index 0 before even starting to parse the DT (that's what it
> is done in smp_setup_processor_id(), with a couple of twists). Then, if I
> find a node that matches the boot CPU mpidr I just skip over it. This
> way the boot CPU MPIDR will always be correct the only difference with
> the current approach will be that instead of generating the secondaries
> MPIDRs we will read them from DT.

That should work, although I'm not sure why you can't just give up
altogether and use the initial mapping from smp_setup_processor_id?

> The problem with this approach is that if we need a pointer (phandle) to the
> boot CPU DT node through the MPIDR and the boot CPU node is botched or missing
> we still behave as if the DT CPU nodes were ok.

Does any code do this? Wouldn't it be much better to lookup logical CPU 0 if
you want to find anything out about the boot CPU?

> I think I'd better stick a warning condition in there if the boot CPU
> node is not present or botched (from a MPIDR perspective at least).

Definitely!

Will

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

[Lorenzo Pieralisi]

On Wed, Nov 07, 2012 at 03:35:30PM +0000, Will Deacon wrote:
> On Wed, Nov 07, 2012 at 12:00:52PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Nov 07, 2012 at 11:05:42AM +0000, Will Deacon wrote:
> > > No, I was thinking about what happens if the devicetree doesn't contain an
> > > mpidr property that matches the boot cpu. In this case, we will fail to
> > > assign logical ID 0, right? If this happens, we should complain about an
> > > invalid devicetree and try to fall back on the logical_map that was
> > > generated earlier on.
> > 
> > Good point. What I could do, I can assign the MPIDR of the boot CPU to
> > the logical index 0 before even starting to parse the DT (that's what it
> > is done in smp_setup_processor_id(), with a couple of twists). Then, if I
> > find a node that matches the boot CPU mpidr I just skip over it. This
> > way the boot CPU MPIDR will always be correct the only difference with
> > the current approach will be that instead of generating the secondaries
> > MPIDRs we will read them from DT.
> 
> That should work, although I'm not sure why you can't just give up
> altogether and use the initial mapping from smp_setup_processor_id?

Since I need to either stash the values to avoid reparsing the tree or
at first I just parse to check for correctness, second pass I update the map.

I will stash the reg values, and if the boot CPU MPIDR is correct I will
copy the stashed map to the cpu_logical_map. If that's unacceptable we
will change it.

> > The problem with this approach is that if we need a pointer (phandle) to the
> > boot CPU DT node through the MPIDR and the boot CPU node is botched or missing
> > we still behave as if the DT CPU nodes were ok.
> 
> Does any code do this? Wouldn't it be much better to lookup logical CPU 0 if
> you want to find anything out about the boot CPU?

Formulated my point in a horrible way sorry.

In order to retrieve the logical id of a CPU (eg IRQ affinity list) we need
its MPIDR for the reverse look-up and for that to work the reg property in the
/cpu nodes must be correct. Let's gloss over this for now.

> > I think I'd better stick a warning condition in there if the boot CPU
> > node is not present or botched (from a MPIDR perspective at least).

Done, IMHO I wrote some code that is too convoluted, I will post it anyway for
review to get further feeback.

Thanks !!
Lorenzo

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

[Lorenzo Pieralisi]

On Wed, Nov 07, 2012 at 03:35:30PM +0000, Will Deacon wrote:
> On Wed, Nov 07, 2012 at 12:00:52PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Nov 07, 2012 at 11:05:42AM +0000, Will Deacon wrote:
> > > No, I was thinking about what happens if the devicetree doesn't contain an
> > > mpidr property that matches the boot cpu. In this case, we will fail to
> > > assign logical ID 0, right? If this happens, we should complain about an
> > > invalid devicetree and try to fall back on the logical_map that was
> > > generated earlier on.
> > 
> > Good point. What I could do, I can assign the MPIDR of the boot CPU to
> > the logical index 0 before even starting to parse the DT (that's what it
> > is done in smp_setup_processor_id(), with a couple of twists). Then, if I
> > find a node that matches the boot CPU mpidr I just skip over it. This
> > way the boot CPU MPIDR will always be correct the only difference with
> > the current approach will be that instead of generating the secondaries
> > MPIDRs we will read them from DT.
> 
> That should work, although I'm not sure why you can't just give up
> altogether and use the initial mapping from smp_setup_processor_id?

Since I need to either stash the values to avoid reparsing the tree or
at first I just parse to check for correctness, second pass I update the map.

I will stash the reg values, and if the boot CPU MPIDR is correct I will
copy the stashed map to the cpu_logical_map. If that's unacceptable we
will change it.

> > The problem with this approach is that if we need a pointer (phandle) to the
> > boot CPU DT node through the MPIDR and the boot CPU node is botched or missing
> > we still behave as if the DT CPU nodes were ok.
> 
> Does any code do this? Wouldn't it be much better to lookup logical CPU 0 if
> you want to find anything out about the boot CPU?

Formulated my point in a horrible way sorry.

In order to retrieve the logical id of a CPU (eg IRQ affinity list) we need
its MPIDR for the reverse look-up and for that to work the reg property in the
/cpu nodes must be correct. Let's gloss over this for now.

> > I think I'd better stick a warning condition in there if the boot CPU
> > node is not present or botched (from a MPIDR perspective at least).

Done, IMHO I wrote some code that is too convoluted, I will post it anyway for
review to get further feeback.

Thanks !!
Lorenzo