[PATCH 0/4] Fixes for DT CPU topology specification on Exynos

[PATCH 0/4] Fixes for DT CPU topology specification on Exynos

[Tomasz Figa]

Due to some parts of Exynos SoC support designed originally in a non-scalable
way, relying on 1:1 mapping between value returned by cpu_logical_map() and
CPU IDs as seen by PMU and GIC, trying to specify CPU topology in device tree
caused various boot-up issues on Exynos SoCs, ranging from CPUs other than 0
failing to boot to crashes due to GIC driver accessing registers out of range.

This series attempts to fix aforementioned issues by removing incorrect
assumptions from Exynos SoC core code and GIC driver and then adding CPU
topology data to device tree sources of Exynos4.

[On Exynos4210-based TRATS and Exynos4412-based TRATS2 board]
Tested-by: Tomasz Figa <t.figa@samsung.com>

Tomasz Figa (4):
  ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers
  ARM: EXYNOS: Fix core ID used by platsmp and hotplug code
  irqchip: gic: Add support for per CPU bank offset specification in DT
  ARM: dts: exynos4: Add CPU topology data

 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 arch/arm/boot/dts/exynos4210.dtsi              | 19 ++++++
 arch/arm/boot/dts/exynos4212.dtsi              | 19 ++++++
 arch/arm/boot/dts/exynos4412.dtsi              | 37 ++++++++--
 arch/arm/mach-exynos/hotplug.c                 | 10 +--
 arch/arm/mach-exynos/platsmp.c                 | 31 +++++----
 arch/arm/mach-exynos/regs-pmu.h                |  4 +-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 9 files changed, 202 insertions(+), 53 deletions(-)

-- 
1.9.2

[PATCH 1/4] ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers

[Tomasz Figa]

On SoCs with more than 2 cores there are more than 2 S5P_ARM_CORE_*
registers that can be addressed with fixed stride of 0x80. This patch
renames the definitions of S5P_ARM_CORE1_* registers to be S5P_ARM_CORE_*
and make them take physical core ID as argument to calculate register
address.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/mach-exynos/hotplug.c  | 2 +-
 arch/arm/mach-exynos/platsmp.c  | 6 +++---
 arch/arm/mach-exynos/regs-pmu.h | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 5eead53..7e0f31a 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -96,7 +96,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 
 		/* make cpu1 to be turned off at next WFI command */
 		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));
 
 		/*
 		 * here's the WFI
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 03e5e9f..7b7de4b 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -107,14 +107,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 */
 	write_pen_release(phys_cpu);
 
-	if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN)) {
+	if (!(__raw_readl(S5P_ARM_CORE_STATUS(1)) & S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(1));
 
 		timeout = 10;
 
 		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(1))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 4f6a256..d94bbc8 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -105,8 +105,8 @@
 #define S5P_GPS_LOWPWR				S5P_PMUREG(0x139C)
 #define S5P_GPS_ALIVE_LOWPWR			S5P_PMUREG(0x13A0)
 
-#define S5P_ARM_CORE1_CONFIGURATION		S5P_PMUREG(0x2080)
-#define S5P_ARM_CORE1_STATUS			S5P_PMUREG(0x2084)
+#define S5P_ARM_CORE_CONFIGURATION(n)		S5P_PMUREG(0x2000 + (n) * 0x80)
+#define S5P_ARM_CORE_STATUS(n)			S5P_PMUREG(0x2004 + (n) * 0x80)
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
-- 
1.9.2

[PATCH 2/4] ARM: EXYNOS: Fix core ID used by platsmp and hotplug code

[Tomasz Figa]

When CPU topology is specified in device tree, cpu_logical_map() does
not return core ID anymore, but rather full MPIDR value. This breaks
existing calculation of PMU register offsets on Exynos SoCs.

This patch fixes the problem by adjusting the code to use only core ID
bits of the value returned by cpu_logical_map() to allow CPU topology to
be specified in device tree on Exynos SoCs.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/mach-exynos/hotplug.c | 10 ++++++----
 arch/arm/mach-exynos/platsmp.c | 31 ++++++++++++++++++-------------
 2 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 7e0f31a..8a5f07d 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -92,11 +92,13 @@ static inline void cpu_leave_lowpower(void)
 
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
+	u32 mpidr = cpu_logical_map(cpu);
+	u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+
 	for (;;) {
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));
+		/* Turn the CPU off on next WFI instruction. */
+		__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(core_id));
 
 		/*
 		 * here's the WFI
@@ -106,7 +108,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 		    :
 		    : "memory", "cc");
 
-		if (pen_release == cpu_logical_map(cpu)) {
+		if (pen_release == core_id) {
 			/*
 			 * OK, proper wakeup, we're done
 			 */
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 7b7de4b..e08b2c5 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -89,7 +89,9 @@ static void exynos_secondary_init(unsigned int cpu)
 static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
-	unsigned long phys_cpu = cpu_logical_map(cpu);
+	u32 mpidr = cpu_logical_map(cpu);
+	u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	u32 reg;
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -102,19 +104,20 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 	 * the holding pen - release it, then wait for it to flag
 	 * that it has been released by resetting pen_release.
 	 *
-	 * Note that "pen_release" is the hardware CPU ID, whereas
+	 * Note that "pen_release" is the hardware CPU core ID, whereas
 	 * "cpu" is Linux's internal ID.
 	 */
-	write_pen_release(phys_cpu);
+	write_pen_release(core_id);
 
-	if (!(__raw_readl(S5P_ARM_CORE_STATUS(1)) & S5P_CORE_LOCAL_PWR_EN)) {
+	reg = __raw_readl(S5P_ARM_CORE_STATUS(core_id));
+	if (!(reg & S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE_CONFIGURATION(1));
+			     S5P_ARM_CORE_CONFIGURATION(core_id));
 
 		timeout = 10;
 
 		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE_STATUS(1))
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(core_id))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -146,10 +149,10 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		 * Try to set boot address using firmware first
 		 * and fall back to boot register if it fails.
 		 */
-		if (call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr))
-			__raw_writel(boot_addr, cpu_boot_reg(phys_cpu));
+		if (call_firmware_op(set_cpu_boot_addr, core_id, boot_addr))
+			__raw_writel(boot_addr, cpu_boot_reg(core_id));
 
-		call_firmware_op(cpu_boot, phys_cpu);
+		call_firmware_op(cpu_boot, core_id);
 
 		arch_send_wakeup_ipi_mask(cpumask_of(cpu));
 
@@ -215,14 +218,16 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
 	 * boot register if it fails.
 	 */
 	for (i = 1; i < max_cpus; ++i) {
-		unsigned long phys_cpu;
 		unsigned long boot_addr;
+		u32 mpidr;
+		u32 core_id;
 
-		phys_cpu = cpu_logical_map(i);
+		mpidr = cpu_logical_map(i);
+		core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
 		boot_addr = virt_to_phys(exynos4_secondary_startup);
 
-		if (call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr))
-			__raw_writel(boot_addr, cpu_boot_reg(phys_cpu));
+		if (call_firmware_op(set_cpu_boot_addr, core_id, boot_addr))
+			__raw_writel(boot_addr, cpu_boot_reg(core_id));
 	}
 }
 
-- 
1.9.2

[PATCH 3/4] irqchip: gic: Add support for per CPU bank offset specification in DT

[Tomasz Figa]

On most platforms GIC registers are banked, so each CPU can access its
registers at the same address. However there is a small number of SoCs
on which the banking is not implemented and each CPU has its GIC
register set at different offset from GIC base address.

Originally the driver used simple maths to calculate the address, i.e.
multiplying constant percpu_offset by cpu_logical_map(cpu). However this
assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1,
but if CPU topology is specified via DT, this changes to full ID in
the same format as MPIDR register and thus breaks the assumption.

This patch adds support for per CPU GIC bank offset specification
through device tree to separate SoC-internal core wiring from CPU
multi-processor IDs.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
 Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
 drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
 3 files changed, 105 insertions(+), 30 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
index 333f4ae..47654e6 100644
--- a/Documentation/devicetree/bindings/arm/cpus.txt
+++ b/Documentation/devicetree/bindings/arm/cpus.txt
@@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
 		Value type: <phandle>
 		Definition: Specifies the ACC[2] node associated with this CPU.
 
+	- gic-offset
+		Usage: required for systems that have non-banked GIC
+		       implementation that requires each CPU to use different
+		       offset to access its set of GIC registers
+		Value type: <u32>
+		Definition: Specifies the offset of GIC registers specific to
+			    this CPU.
 
 Example 1 (dual-cluster big.LITTLE system 32-bit):
 
diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 5573c08..2bd03406 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -48,7 +48,7 @@ Optional
 
 - cpu-offset	: per-cpu offset within the distributor and cpu interface
   regions, used when the GIC doesn't have banked registers. The offset is
-  cpu-offset * cpu-nr.
+  cpu-offset * cpu-nr. (DEPRECATED, see per-CPU 'gic-offset' property.)
 
 - arm,routable-irqs : Total number of gic irq inputs which are not directly
 		  connected from the peripherals, but are routed dynamically
@@ -67,6 +67,38 @@ Example:
 		      <0xfff10100 0x100>;
 	};
 
+* Per-CPU register offset specification for non-banked GIC
+
+On most platforms GIC registers are banked, so each CPU can access its
+registers at the same address. However there is a small number of SoCs
+on which the banking is not implemented and each CPU has its GIC
+register set at different offset from GIC base address. These offsets
+need to be be provided from device tree, as described below.
+
+Optional properties in node of each CPU in the system:
+
+ - gic-offset : A single u32 value that needs to be added to GIC base
+   address to calculate address of GIC registers for that CPU.
+
+See [1] for more details about ARM CPU bindings.
+
+Example:
+
+	cpus {
+		/* ... */
+
+		cpu at a00 {
+			/* ... */
+			gic-offset = <0x0000>;
+		};
+
+		cpu at a01 {
+			/* ... */
+			gic-offset = <0x8000>;
+		};
+	};
+
+[1] Documentation/devicetree/bindings/arm/cpus.txt
 
 * GIC virtualization extensions (VGIC)
 
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4300b66..ad6f4fe 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -924,6 +924,69 @@ const struct irq_domain_ops gic_default_routable_irq_domain_ops = {
 const struct irq_domain_ops *gic_routable_irq_domain_ops =
 					&gic_default_routable_irq_domain_ops;
 
+static int gic_setup_bases(struct gic_chip_data *gic, void __iomem *dist_base,
+			   void __iomem *cpu_base, u32 percpu_offset)
+{
+	bool use_cpu_nodes = true;
+	u32 offset;
+	unsigned int cpu;
+
+	for_each_possible_cpu(cpu) {
+		struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
+
+		if (!cpu_node
+		    || of_property_read_u32(cpu_node, "gic-offset", &offset)) {
+			use_cpu_nodes = false;
+			break;
+		}
+	}
+
+	if (!(percpu_offset || use_cpu_nodes)
+	    || !IS_ENABLED(CONFIG_GIC_NON_BANKED)) {
+		/* Normal, sane GIC... (or non-banked unsupported) */
+		WARN(percpu_offset || use_cpu_nodes,
+		     "GIC_NON_BANKED not enabled, ignoring %08x offset!",
+		     percpu_offset);
+
+		gic->dist_base.common_base = dist_base;
+		gic->cpu_base.common_base = cpu_base;
+		gic_set_base_accessor(gic, gic_get_common_base);
+
+		return 0;
+	}
+
+	/* Frankein-GIC without banked registers... */
+	gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
+	gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
+	if (WARN_ON(!gic->dist_base.percpu_base ||
+			!gic->cpu_base.percpu_base)) {
+		free_percpu(gic->dist_base.percpu_base);
+		free_percpu(gic->cpu_base.percpu_base);
+
+		return -ENOMEM;
+	}
+
+	for_each_possible_cpu(cpu) {
+		if (use_cpu_nodes) {
+			struct device_node *cpu_node =
+						of_get_cpu_node(cpu, NULL);
+
+			of_property_read_u32(cpu_node, "gic-offset", &offset);
+		} else {
+			offset = percpu_offset * cpu_logical_map(cpu);
+		}
+
+		*per_cpu_ptr(gic->dist_base.percpu_base, cpu) =
+							dist_base + offset;
+		*per_cpu_ptr(gic->cpu_base.percpu_base, cpu) =
+							cpu_base + offset;
+	}
+
+	gic_set_base_accessor(gic, gic_get_percpu_base);
+
+	return 0;
+}
+
 void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 			   void __iomem *dist_base, void __iomem *cpu_base,
 			   u32 percpu_offset, struct device_node *node)
@@ -936,36 +999,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
 	BUG_ON(gic_nr >= MAX_GIC_NR);
 
 	gic = &gic_data[gic_nr];
-#ifdef CONFIG_GIC_NON_BANKED
-	if (percpu_offset) { /* Frankein-GIC without banked registers... */
-		unsigned int cpu;
-
-		gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
-		gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
-		if (WARN_ON(!gic->dist_base.percpu_base ||
-			    !gic->cpu_base.percpu_base)) {
-			free_percpu(gic->dist_base.percpu_base);
-			free_percpu(gic->cpu_base.percpu_base);
-			return;
-		}
 
-		for_each_possible_cpu(cpu) {
-			unsigned long offset = percpu_offset * cpu_logical_map(cpu);
-			*per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
-			*per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
-		}
-
-		gic_set_base_accessor(gic, gic_get_percpu_base);
-	} else
-#endif
-	{			/* Normal, sane GIC... */
-		WARN(percpu_offset,
-		     "GIC_NON_BANKED not enabled, ignoring %08x offset!",
-		     percpu_offset);
-		gic->dist_base.common_base = dist_base;
-		gic->cpu_base.common_base = cpu_base;
-		gic_set_base_accessor(gic, gic_get_common_base);
-	}
+	if (gic_setup_bases(gic, dist_base, cpu_base, percpu_offset))
+		return;
 
 	/*
 	 * Initialize the CPU interface map to all CPUs.
-- 
1.9.2

[PATCH 4/4] ARM: dts: exynos4: Add CPU topology data

[Tomasz Figa]

After fixing non-banked GIC support in GIC driver, CPU nodes can be
safely added to Exynos4 device tree sources, along with appropriate
gic-offset properties.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
---
 arch/arm/boot/dts/exynos4210.dtsi | 19 +++++++++++++++++++
 arch/arm/boot/dts/exynos4212.dtsi | 19 +++++++++++++++++++
 arch/arm/boot/dts/exynos4412.dtsi | 37 +++++++++++++++++++++++++++++++++----
 3 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index cacf614..e463361 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -25,6 +25,25 @@
 / {
 	compatible = "samsung,exynos4210", "samsung,exynos4";
 
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu at 900 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0x900>;
+			gic-offset = <0x0000>;
+		};
+
+		cpu at 901 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0x901>;
+			gic-offset = <0x8000>;
+		};
+	};
+
 	aliases {
 		pinctrl0 = &pinctrl_0;
 		pinctrl1 = &pinctrl_1;
diff --git a/arch/arm/boot/dts/exynos4212.dtsi b/arch/arm/boot/dts/exynos4212.dtsi
index 3c00e6e..211e129 100644
--- a/arch/arm/boot/dts/exynos4212.dtsi
+++ b/arch/arm/boot/dts/exynos4212.dtsi
@@ -22,6 +22,25 @@
 / {
 	compatible = "samsung,exynos4212", "samsung,exynos4";
 
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu at a00 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0xa00>;
+			gic-offset = <0x0000>;
+		};
+
+		cpu at a01 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0xa01>;
+			gic-offset = <0x8000>;
+		};
+	};
+
 	combiner: interrupt-controller at 10440000 {
 		samsung,combiner-nr = <18>;
 	};
diff --git a/arch/arm/boot/dts/exynos4412.dtsi b/arch/arm/boot/dts/exynos4412.dtsi
index 15d3c0a..04530ef 100644
--- a/arch/arm/boot/dts/exynos4412.dtsi
+++ b/arch/arm/boot/dts/exynos4412.dtsi
@@ -22,11 +22,40 @@
 / {
 	compatible = "samsung,exynos4412", "samsung,exynos4";
 
-	combiner: interrupt-controller at 10440000 {
-		samsung,combiner-nr = <20>;
+	cpus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		cpu at a00 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0xa00>;
+			gic-offset = <0x0000>;
+		};
+
+		cpu at a01 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0xa01>;
+			gic-offset = <0x4000>;
+		};
+
+		cpu at a02 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0xa02>;
+			gic-offset = <0x8000>;
+		};
+
+		cpu at a03 {
+			compatible = "arm,cortex-a9";
+			device_type = "cpu";
+			reg = <0xa03>;
+			gic-offset = <0xc000>;
+		};
 	};
 
-	gic: interrupt-controller at 10490000 {
-		cpu-offset = <0x4000>;
+	combiner: interrupt-controller at 10440000 {
+		samsung,combiner-nr = <20>;
 	};
 };
-- 
1.9.2

[PATCH 1/4] ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers

[Chanwoo Choi]

Hi Tomasz,

On Fri, Apr 18, 2014 at 11:42 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On SoCs with more than 2 cores there are more than 2 S5P_ARM_CORE_*
> registers that can be addressed with fixed stride of 0x80. This patch
> renames the definitions of S5P_ARM_CORE1_* registers to be S5P_ARM_CORE_*
> and make them take physical core ID as argument to calculate register
> address.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> ---
>  arch/arm/mach-exynos/hotplug.c  | 2 +-
>  arch/arm/mach-exynos/platsmp.c  | 6 +++---
>  arch/arm/mach-exynos/regs-pmu.h | 4 ++--
>  3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 5eead53..7e0f31a 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -96,7 +96,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>
>                 /* make cpu1 to be turned off at next WFI command */
>                 if (cpu == 1)
> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> +                       __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));

Exynos4412 has quad cores. If turn off third/fourth core, this patch
could not turn off third/fourth cores.

Why didn't you use cpu number for argument of
S5P_ARM_CORE_CONFIGURATION() as following?
- __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpu));

>
>                 /*
>                  * here's the WFI
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index 03e5e9f..7b7de4b 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -107,14 +107,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>          */
>         write_pen_release(phys_cpu);
>
> -       if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN)) {
> +       if (!(__raw_readl(S5P_ARM_CORE_STATUS(1)) & S5P_CORE_LOCAL_PWR_EN)) {
>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
> -                            S5P_ARM_CORE1_CONFIGURATION);
> +                            S5P_ARM_CORE_CONFIGURATION(1));

ditto.

>
>                 timeout = 10;
>
>                 /* wait max 10 ms until cpu1 is on */
> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(1))

ditto.

Best regards,
Chanwoo Choi

[PATCH 1/4] ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers

[Tomasz Figa]

Hi Chanwoo,

On 19.04.2014 09:47, Chanwoo Choi wrote:
> Hi Tomasz,
>
> On Fri, Apr 18, 2014 at 11:42 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> On SoCs with more than 2 cores there are more than 2 S5P_ARM_CORE_*
>> registers that can be addressed with fixed stride of 0x80. This patch
>> renames the definitions of S5P_ARM_CORE1_* registers to be S5P_ARM_CORE_*
>> and make them take physical core ID as argument to calculate register
>> address.
>>
>> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>> ---
>>   arch/arm/mach-exynos/hotplug.c  | 2 +-
>>   arch/arm/mach-exynos/platsmp.c  | 6 +++---
>>   arch/arm/mach-exynos/regs-pmu.h | 4 ++--
>>   3 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
>> index 5eead53..7e0f31a 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -96,7 +96,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>
>>                  /* make cpu1 to be turned off at next WFI command */
>>                  if (cpu == 1)
>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> +                       __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));
>
> Exynos4412 has quad cores. If turn off third/fourth core, this patch
> could not turn off third/fourth cores.
>
> Why didn't you use cpu number for argument of
> S5P_ARM_CORE_CONFIGURATION() as following?
> - __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpu));
>
>>
>>                  /*
>>                   * here's the WFI
>> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
>> index 03e5e9f..7b7de4b 100644
>> --- a/arch/arm/mach-exynos/platsmp.c
>> +++ b/arch/arm/mach-exynos/platsmp.c
>> @@ -107,14 +107,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>>           */
>>          write_pen_release(phys_cpu);
>>
>> -       if (!(__raw_readl(S5P_ARM_CORE1_STATUS) & S5P_CORE_LOCAL_PWR_EN)) {
>> +       if (!(__raw_readl(S5P_ARM_CORE_STATUS(1)) & S5P_CORE_LOCAL_PWR_EN)) {
>>                  __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>> -                            S5P_ARM_CORE1_CONFIGURATION);
>> +                            S5P_ARM_CORE_CONFIGURATION(1));
>
> ditto.
>
>>
>>                  timeout = 10;
>>
>>                  /* wait max 10 ms until cpu1 is on */
>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(1))
>
> ditto.

You are absolutely correct, this needs to be fixed, but this series just 
keeps current behavior for now to just make CPU topology in DT not break 
things that are working.

I have further patches that clean up platsmp and hotplug code and they 
will add proper support for arbitrary number of cores.

Best regards,
Tomasz

[PATCH 2/4] ARM: EXYNOS: Fix core ID used by platsmp and hotplug code

[Chander Kashyap]

Hi Tomasz,


On 18 April 2014 20:12, Tomasz Figa <t.figa@samsung.com> wrote:
> When CPU topology is specified in device tree, cpu_logical_map() does
> not return core ID anymore, but rather full MPIDR value. This breaks
> existing calculation of PMU register offsets on Exynos SoCs.
>
> This patch fixes the problem by adjusting the code to use only core ID
> bits of the value returned by cpu_logical_map() to allow CPU topology to
> be specified in device tree on Exynos SoCs.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> ---
>  arch/arm/mach-exynos/hotplug.c | 10 ++++++----
>  arch/arm/mach-exynos/platsmp.c | 31 ++++++++++++++++++-------------
>  2 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 7e0f31a..8a5f07d 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -92,11 +92,13 @@ static inline void cpu_leave_lowpower(void)
>
>  static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>  {
> +       u32 mpidr = cpu_logical_map(cpu);
> +       u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +
>         for (;;) {
>
> -               /* make cpu1 to be turned off at next WFI command */
> -               if (cpu == 1)
> -                       __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));

I think this macro is not yet in ML code.

> +               /* Turn the CPU off on next WFI instruction. */
> +               __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(core_id));
>
>                 /*
>                  * here's the WFI
> @@ -106,7 +108,7 @@ static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>                     :
>                     : "memory", "cc");
>
> -               if (pen_release == cpu_logical_map(cpu)) {
> +               if (pen_release == core_id) {
>                         /*
>                          * OK, proper wakeup, we're done
>                          */
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index 7b7de4b..e08b2c5 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -89,7 +89,9 @@ static void exynos_secondary_init(unsigned int cpu)
>  static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>  {
>         unsigned long timeout;
> -       unsigned long phys_cpu = cpu_logical_map(cpu);
> +       u32 mpidr = cpu_logical_map(cpu);
> +       u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +       u32 reg;
>
>         /*
>          * Set synchronisation state between this boot processor
> @@ -102,19 +104,20 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>          * the holding pen - release it, then wait for it to flag
>          * that it has been released by resetting pen_release.
>          *
> -        * Note that "pen_release" is the hardware CPU ID, whereas
> +        * Note that "pen_release" is the hardware CPU core ID, whereas
>          * "cpu" is Linux's internal ID.
>          */
> -       write_pen_release(phys_cpu);
> +       write_pen_release(core_id);
>
> -       if (!(__raw_readl(S5P_ARM_CORE_STATUS(1)) & S5P_CORE_LOCAL_PWR_EN)) {
> +       reg = __raw_readl(S5P_ARM_CORE_STATUS(core_id));
> +       if (!(reg & S5P_CORE_LOCAL_PWR_EN)) {
>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
> -                            S5P_ARM_CORE_CONFIGURATION(1));
> +                            S5P_ARM_CORE_CONFIGURATION(core_id));
>
>                 timeout = 10;
>
>                 /* wait max 10 ms until cpu1 is on */
> -               while ((__raw_readl(S5P_ARM_CORE_STATUS(1))
> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(core_id))
>                         & S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
>                         if (timeout-- == 0)
>                                 break;
> @@ -146,10 +149,10 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>                  * Try to set boot address using firmware first
>                  * and fall back to boot register if it fails.
>                  */
> -               if (call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr))
> -                       __raw_writel(boot_addr, cpu_boot_reg(phys_cpu));
> +               if (call_firmware_op(set_cpu_boot_addr, core_id, boot_addr))
> +                       __raw_writel(boot_addr, cpu_boot_reg(core_id));
>
> -               call_firmware_op(cpu_boot, phys_cpu);
> +               call_firmware_op(cpu_boot, core_id);
>
>                 arch_send_wakeup_ipi_mask(cpumask_of(cpu));
>
> @@ -215,14 +218,16 @@ static void __init exynos_smp_prepare_cpus(unsigned int max_cpus)
>          * boot register if it fails.
>          */
>         for (i = 1; i < max_cpus; ++i) {
> -               unsigned long phys_cpu;
>                 unsigned long boot_addr;
> +               u32 mpidr;
> +               u32 core_id;
>
> -               phys_cpu = cpu_logical_map(i);
> +               mpidr = cpu_logical_map(i);
> +               core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>                 boot_addr = virt_to_phys(exynos4_secondary_startup);
>
> -               if (call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr))
> -                       __raw_writel(boot_addr, cpu_boot_reg(phys_cpu));
> +               if (call_firmware_op(set_cpu_boot_addr, core_id, boot_addr))
> +                       __raw_writel(boot_addr, cpu_boot_reg(core_id));
>         }
>  }
>
> --
> 1.9.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
with warm regards,
Chander Kashyap

[PATCH 2/4] ARM: EXYNOS: Fix core ID used by platsmp and hotplug code

[Tomasz Figa]

Hi Chander,

On 20.04.2014 09:23, Chander Kashyap wrote:
> Hi Tomasz,
>
>
> On 18 April 2014 20:12, Tomasz Figa <t.figa@samsung.com> wrote:
>> When CPU topology is specified in device tree, cpu_logical_map() does
>> not return core ID anymore, but rather full MPIDR value. This breaks
>> existing calculation of PMU register offsets on Exynos SoCs.
>>
>> This patch fixes the problem by adjusting the code to use only core ID
>> bits of the value returned by cpu_logical_map() to allow CPU topology to
>> be specified in device tree on Exynos SoCs.
>>
>> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>> ---
>>   arch/arm/mach-exynos/hotplug.c | 10 ++++++----
>>   arch/arm/mach-exynos/platsmp.c | 31 ++++++++++++++++++-------------
>>   2 files changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
>> index 7e0f31a..8a5f07d 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -92,11 +92,13 @@ static inline void cpu_leave_lowpower(void)
>>
>>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   {
>> +       u32 mpidr = cpu_logical_map(cpu);
>> +       u32 core_id = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> +
>>          for (;;) {
>>
>> -               /* make cpu1 to be turned off at next WFI command */
>> -               if (cpu == 1)
>> -                       __raw_writel(0, S5P_ARM_CORE_CONFIGURATION(1));
>
> I think this macro is not yet in ML code.

What do you mean? The S5P_ARM_CORE_CONFIGURATION() macro is being added 
by patch 1/4.

Best regards,
Tomasz

[PATCH 0/4] Fixes for DT CPU topology specification on Exynos

[Tomasz Figa]

Hi,

On 18.04.2014 16:42, Tomasz Figa wrote:
> Due to some parts of Exynos SoC support designed originally in a non-scalable
> way, relying on 1:1 mapping between value returned by cpu_logical_map() and
> CPU IDs as seen by PMU and GIC, trying to specify CPU topology in device tree
> caused various boot-up issues on Exynos SoCs, ranging from CPUs other than 0
> failing to boot to crashes due to GIC driver accessing registers out of range.
> 
> This series attempts to fix aforementioned issues by removing incorrect
> assumptions from Exynos SoC core code and GIC driver and then adding CPU
> topology data to device tree sources of Exynos4.
> 
> [On Exynos4210-based TRATS and Exynos4412-based TRATS2 board]
> Tested-by: Tomasz Figa <t.figa@samsung.com>
> 
> Tomasz Figa (4):
>   ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers
>   ARM: EXYNOS: Fix core ID used by platsmp and hotplug code
>   irqchip: gic: Add support for per CPU bank offset specification in DT
>   ARM: dts: exynos4: Add CPU topology data
> 
>  Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
>  Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
>  arch/arm/boot/dts/exynos4210.dtsi              | 19 ++++++
>  arch/arm/boot/dts/exynos4212.dtsi              | 19 ++++++
>  arch/arm/boot/dts/exynos4412.dtsi              | 37 ++++++++--
>  arch/arm/mach-exynos/hotplug.c                 | 10 +--
>  arch/arm/mach-exynos/platsmp.c                 | 31 +++++----
>  arch/arm/mach-exynos/regs-pmu.h                |  4 +-
>  drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
>  9 files changed, 202 insertions(+), 53 deletions(-)
> 

Any comments for this series? I'm especially interested in hearing some
opinions about patch 3/4, which extends DT bindings of ARM GIC.

Best regards,
Tomasz

[PATCH 3/4] irqchip: gic: Add support for per CPU bank offset specification in DT

[Rob Herring]

On Fri, Apr 18, 2014 at 9:43 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> On most platforms GIC registers are banked, so each CPU can access its
> registers at the same address. However there is a small number of SoCs
> on which the banking is not implemented and each CPU has its GIC
> register set at different offset from GIC base address.
>
> Originally the driver used simple maths to calculate the address, i.e.
> multiplying constant percpu_offset by cpu_logical_map(cpu). However this
> assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1,
> but if CPU topology is specified via DT, this changes to full ID in
> the same format as MPIDR register and thus breaks the assumption.
>
> This patch adds support for per CPU GIC bank offset specification
> through device tree to separate SoC-internal core wiring from CPU
> multi-processor IDs.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> ---
>  Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
>  Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
>  drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
>  3 files changed, 105 insertions(+), 30 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
> index 333f4ae..47654e6 100644
> --- a/Documentation/devicetree/bindings/arm/cpus.txt
> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
> @@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
>                 Value type: <phandle>
>                 Definition: Specifies the ACC[2] node associated with this CPU.
>
> +       - gic-offset
> +               Usage: required for systems that have non-banked GIC
> +                      implementation that requires each CPU to use different
> +                      offset to access its set of GIC registers
> +               Value type: <u32>
> +               Definition: Specifies the offset of GIC registers specific to
> +                           this CPU.

What if you have 1 distributor address and a per cpu address which is
allowed in the gicv2 spec IIRC.

I think I would rather see this stay contained within the gic node and
use reg property.

Rob

>
>  Example 1 (dual-cluster big.LITTLE system 32-bit):
>
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 5573c08..2bd03406 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -48,7 +48,7 @@ Optional
>
>  - cpu-offset   : per-cpu offset within the distributor and cpu interface
>    regions, used when the GIC doesn't have banked registers. The offset is
> -  cpu-offset * cpu-nr.
> +  cpu-offset * cpu-nr. (DEPRECATED, see per-CPU 'gic-offset' property.)
>
>  - arm,routable-irqs : Total number of gic irq inputs which are not directly
>                   connected from the peripherals, but are routed dynamically
> @@ -67,6 +67,38 @@ Example:
>                       <0xfff10100 0x100>;
>         };
>
> +* Per-CPU register offset specification for non-banked GIC
> +
> +On most platforms GIC registers are banked, so each CPU can access its
> +registers at the same address. However there is a small number of SoCs
> +on which the banking is not implemented and each CPU has its GIC
> +register set at different offset from GIC base address. These offsets
> +need to be be provided from device tree, as described below.
> +
> +Optional properties in node of each CPU in the system:
> +
> + - gic-offset : A single u32 value that needs to be added to GIC base
> +   address to calculate address of GIC registers for that CPU.
> +
> +See [1] for more details about ARM CPU bindings.
> +
> +Example:
> +
> +       cpus {
> +               /* ... */
> +
> +               cpu at a00 {
> +                       /* ... */
> +                       gic-offset = <0x0000>;
> +               };
> +
> +               cpu at a01 {
> +                       /* ... */
> +                       gic-offset = <0x8000>;
> +               };
> +       };
> +
> +[1] Documentation/devicetree/bindings/arm/cpus.txt
>
>  * GIC virtualization extensions (VGIC)
>
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 4300b66..ad6f4fe 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -924,6 +924,69 @@ const struct irq_domain_ops gic_default_routable_irq_domain_ops = {
>  const struct irq_domain_ops *gic_routable_irq_domain_ops =
>                                         &gic_default_routable_irq_domain_ops;
>
> +static int gic_setup_bases(struct gic_chip_data *gic, void __iomem *dist_base,
> +                          void __iomem *cpu_base, u32 percpu_offset)
> +{
> +       bool use_cpu_nodes = true;
> +       u32 offset;
> +       unsigned int cpu;
> +
> +       for_each_possible_cpu(cpu) {
> +               struct device_node *cpu_node = of_get_cpu_node(cpu, NULL);
> +
> +               if (!cpu_node
> +                   || of_property_read_u32(cpu_node, "gic-offset", &offset)) {
> +                       use_cpu_nodes = false;
> +                       break;
> +               }
> +       }
> +
> +       if (!(percpu_offset || use_cpu_nodes)
> +           || !IS_ENABLED(CONFIG_GIC_NON_BANKED)) {
> +               /* Normal, sane GIC... (or non-banked unsupported) */
> +               WARN(percpu_offset || use_cpu_nodes,
> +                    "GIC_NON_BANKED not enabled, ignoring %08x offset!",
> +                    percpu_offset);
> +
> +               gic->dist_base.common_base = dist_base;
> +               gic->cpu_base.common_base = cpu_base;
> +               gic_set_base_accessor(gic, gic_get_common_base);
> +
> +               return 0;
> +       }
> +
> +       /* Frankein-GIC without banked registers... */
> +       gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
> +       gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
> +       if (WARN_ON(!gic->dist_base.percpu_base ||
> +                       !gic->cpu_base.percpu_base)) {
> +               free_percpu(gic->dist_base.percpu_base);
> +               free_percpu(gic->cpu_base.percpu_base);
> +
> +               return -ENOMEM;
> +       }
> +
> +       for_each_possible_cpu(cpu) {
> +               if (use_cpu_nodes) {
> +                       struct device_node *cpu_node =
> +                                               of_get_cpu_node(cpu, NULL);
> +
> +                       of_property_read_u32(cpu_node, "gic-offset", &offset);
> +               } else {
> +                       offset = percpu_offset * cpu_logical_map(cpu);
> +               }
> +
> +               *per_cpu_ptr(gic->dist_base.percpu_base, cpu) =
> +                                                       dist_base + offset;
> +               *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) =
> +                                                       cpu_base + offset;
> +       }
> +
> +       gic_set_base_accessor(gic, gic_get_percpu_base);
> +
> +       return 0;
> +}
> +
>  void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>                            void __iomem *dist_base, void __iomem *cpu_base,
>                            u32 percpu_offset, struct device_node *node)
> @@ -936,36 +999,9 @@ void __init gic_init_bases(unsigned int gic_nr, int irq_start,
>         BUG_ON(gic_nr >= MAX_GIC_NR);
>
>         gic = &gic_data[gic_nr];
> -#ifdef CONFIG_GIC_NON_BANKED
> -       if (percpu_offset) { /* Frankein-GIC without banked registers... */
> -               unsigned int cpu;
> -
> -               gic->dist_base.percpu_base = alloc_percpu(void __iomem *);
> -               gic->cpu_base.percpu_base = alloc_percpu(void __iomem *);
> -               if (WARN_ON(!gic->dist_base.percpu_base ||
> -                           !gic->cpu_base.percpu_base)) {
> -                       free_percpu(gic->dist_base.percpu_base);
> -                       free_percpu(gic->cpu_base.percpu_base);
> -                       return;
> -               }
>
> -               for_each_possible_cpu(cpu) {
> -                       unsigned long offset = percpu_offset * cpu_logical_map(cpu);
> -                       *per_cpu_ptr(gic->dist_base.percpu_base, cpu) = dist_base + offset;
> -                       *per_cpu_ptr(gic->cpu_base.percpu_base, cpu) = cpu_base + offset;
> -               }
> -
> -               gic_set_base_accessor(gic, gic_get_percpu_base);
> -       } else
> -#endif
> -       {                       /* Normal, sane GIC... */
> -               WARN(percpu_offset,
> -                    "GIC_NON_BANKED not enabled, ignoring %08x offset!",
> -                    percpu_offset);
> -               gic->dist_base.common_base = dist_base;
> -               gic->cpu_base.common_base = cpu_base;
> -               gic_set_base_accessor(gic, gic_get_common_base);
> -       }
> +       if (gic_setup_bases(gic, dist_base, cpu_base, percpu_offset))
> +               return;
>
>         /*
>          * Initialize the CPU interface map to all CPUs.
> --
> 1.9.2
>

[PATCH 3/4] irqchip: gic: Add support for per CPU bank offset specification in DT

[Tomasz Figa]

On 08.05.2014 19:04, Rob Herring wrote:
> On Fri, Apr 18, 2014 at 9:43 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> On most platforms GIC registers are banked, so each CPU can access its
>> registers at the same address. However there is a small number of SoCs
>> on which the banking is not implemented and each CPU has its GIC
>> register set at different offset from GIC base address.
>>
>> Originally the driver used simple maths to calculate the address, i.e.
>> multiplying constant percpu_offset by cpu_logical_map(cpu). However this
>> assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1,
>> but if CPU topology is specified via DT, this changes to full ID in
>> the same format as MPIDR register and thus breaks the assumption.
>>
>> This patch adds support for per CPU GIC bank offset specification
>> through device tree to separate SoC-internal core wiring from CPU
>> multi-processor IDs.
>>
>> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>> ---
>>  Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
>>  Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
>>  drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
>>  3 files changed, 105 insertions(+), 30 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>> index 333f4ae..47654e6 100644
>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>> @@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
>>                 Value type: <phandle>
>>                 Definition: Specifies the ACC[2] node associated with this CPU.
>>
>> +       - gic-offset
>> +               Usage: required for systems that have non-banked GIC
>> +                      implementation that requires each CPU to use different
>> +                      offset to access its set of GIC registers
>> +               Value type: <u32>
>> +               Definition: Specifies the offset of GIC registers specific to
>> +                           this CPU.
> 
> What if you have 1 distributor address and a per cpu address which is
> allowed in the gicv2 spec IIRC.

Hmm, I need to take a look at GIC v2 spec... but I think my proposed
binding would still cover this, as the implementation (if modified to
support this) would simply ignore the offset for distributor in this case.

> 
> I think I would rather see this stay contained within the gic node and
> use reg property.

How do we match reg entries with CPUs then? The first idea that comes to
my mind would be adding arm,cpu-map property that would list MPIDR
values of CPUs in the same order as register banks are listed in reg
property but I'm not sure this is a good idea.

Best regards,
Tomasz

[PATCH 3/4] irqchip: gic: Add support for per CPU bank offset specification in DT

[Rob Herring]

On Thu, May 8, 2014 at 12:09 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On 08.05.2014 19:04, Rob Herring wrote:
>> On Fri, Apr 18, 2014 at 9:43 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>>> On most platforms GIC registers are banked, so each CPU can access its
>>> registers at the same address. However there is a small number of SoCs
>>> on which the banking is not implemented and each CPU has its GIC
>>> register set at different offset from GIC base address.
>>>
>>> Originally the driver used simple maths to calculate the address, i.e.
>>> multiplying constant percpu_offset by cpu_logical_map(cpu). However this
>>> assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1,
>>> but if CPU topology is specified via DT, this changes to full ID in
>>> the same format as MPIDR register and thus breaks the assumption.
>>>
>>> This patch adds support for per CPU GIC bank offset specification
>>> through device tree to separate SoC-internal core wiring from CPU
>>> multi-processor IDs.
>>>
>>> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>>> ---
>>>  Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
>>>  Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
>>>  drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
>>>  3 files changed, 105 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>>> index 333f4ae..47654e6 100644
>>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>>> @@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
>>>                 Value type: <phandle>
>>>                 Definition: Specifies the ACC[2] node associated with this CPU.
>>>
>>> +       - gic-offset
>>> +               Usage: required for systems that have non-banked GIC
>>> +                      implementation that requires each CPU to use different
>>> +                      offset to access its set of GIC registers
>>> +               Value type: <u32>
>>> +               Definition: Specifies the offset of GIC registers specific to
>>> +                           this CPU.
>>
>> What if you have 1 distributor address and a per cpu address which is
>> allowed in the gicv2 spec IIRC.
>
> Hmm, I need to take a look at GIC v2 spec... but I think my proposed
> binding would still cover this, as the implementation (if modified to
> support this) would simply ignore the offset for distributor in this case.

How does the implementation know whether to ignore the offset for the
distributor or not?

>> I think I would rather see this stay contained within the gic node and
>> use reg property.
>
> How do we match reg entries with CPUs then? The first idea that comes to
> my mind would be adding arm,cpu-map property that would list MPIDR
> values of CPUs in the same order as register banks are listed in reg
> property but I'm not sure this is a good idea.

How do you do it currently?

The issue with putting properties in the cpu node is it does not
scale. What if you had 10 per cpu properties on 10 different blocks.

Using the MPIDR directly is a bit specific. I would perhaps just do
something like this:

gic-cpu-map = <&CPU0 &CPU1>;

Rob

[PATCH 3/4] irqchip: gic: Add support for per CPU bank offset specification in DT

[Tomasz Figa]

On 08.05.2014 20:04, Rob Herring wrote:
> On Thu, May 8, 2014 at 12:09 PM, Tomasz Figa <t.figa@samsung.com> wrote:
>> On 08.05.2014 19:04, Rob Herring wrote:
>>> On Fri, Apr 18, 2014 at 9:43 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>>>> On most platforms GIC registers are banked, so each CPU can access its
>>>> registers at the same address. However there is a small number of SoCs
>>>> on which the banking is not implemented and each CPU has its GIC
>>>> register set at different offset from GIC base address.
>>>>
>>>> Originally the driver used simple maths to calculate the address, i.e.
>>>> multiplying constant percpu_offset by cpu_logical_map(cpu). However this
>>>> assumed the namespace of cpu_logical_map() to be from 0 to num_cpus-1,
>>>> but if CPU topology is specified via DT, this changes to full ID in
>>>> the same format as MPIDR register and thus breaks the assumption.
>>>>
>>>> This patch adds support for per CPU GIC bank offset specification
>>>> through device tree to separate SoC-internal core wiring from CPU
>>>> multi-processor IDs.
>>>>
>>>> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>>>> ---
>>>>  Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
>>>>  Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
>>>>  drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
>>>>  3 files changed, 105 insertions(+), 30 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/cpus.txt b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> index 333f4ae..47654e6 100644
>>>> --- a/Documentation/devicetree/bindings/arm/cpus.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/cpus.txt
>>>> @@ -209,6 +209,13 @@ nodes to be present and contain the properties described below.
>>>>                 Value type: <phandle>
>>>>                 Definition: Specifies the ACC[2] node associated with this CPU.
>>>>
>>>> +       - gic-offset
>>>> +               Usage: required for systems that have non-banked GIC
>>>> +                      implementation that requires each CPU to use different
>>>> +                      offset to access its set of GIC registers
>>>> +               Value type: <u32>
>>>> +               Definition: Specifies the offset of GIC registers specific to
>>>> +                           this CPU.
>>>
>>> What if you have 1 distributor address and a per cpu address which is
>>> allowed in the gicv2 spec IIRC.
>>
>> Hmm, I need to take a look at GIC v2 spec... but I think my proposed
>> binding would still cover this, as the implementation (if modified to
>> support this) would simply ignore the offset for distributor in this case.
> 
> How does the implementation know whether to ignore the offset for the
> distributor or not?

Hmm, if GIC v2 spec allows both cases, then this solution is not enough
indeed. I need to find some time to familiarize with necessary
documentation...

>>> I think I would rather see this stay contained within the gic node and
>>> use reg property.
>>
>> How do we match reg entries with CPUs then? The first idea that comes to
>> my mind would be adding arm,cpu-map property that would list MPIDR
>> values of CPUs in the same order as register banks are listed in reg
>> property but I'm not sure this is a good idea.
> 
> How do you do it currently?

We don't. As I described in commit message, the driver expects
cpu_logical_map() to return a value from [0...N-1] range where N is the
number of CPUs and that respective per-CPU bank is located at gic_base +
cpu_offset * cpu_logical_map(). There are two problems with this:

a) after adding CPU topology data to DT, cpu_logical_map() starts
returning lower 24 bits of MPIDR and certain SoCs have non-zero cluster
ID, even if there is only a single cluster present,

b) obviously for multi-cluster SoCs the above assumption doesn't stand.

> 
> The issue with putting properties in the cpu node is it does not
> scale. What if you had 10 per cpu properties on 10 different blocks.
> 
> Using the MPIDR directly is a bit specific. I would perhaps just do
> something like this:
> 
> gic-cpu-map = <&CPU0 &CPU1>;

It sounds quite good. I'll try to get this implemented in next version,
but after looking through GIC v2 spec first.

Best regards,
Tomasz

[PATCH 0/4] Fixes for DT CPU topology specification on Exynos

[Tomasz Figa]

Hi Kukjin,

On 18.04.2014 16:42, Tomasz Figa wrote:
> Due to some parts of Exynos SoC support designed originally in a non-scalable
> way, relying on 1:1 mapping between value returned by cpu_logical_map() and
> CPU IDs as seen by PMU and GIC, trying to specify CPU topology in device tree
> caused various boot-up issues on Exynos SoCs, ranging from CPUs other than 0
> failing to boot to crashes due to GIC driver accessing registers out of range.
> 
> This series attempts to fix aforementioned issues by removing incorrect
> assumptions from Exynos SoC core code and GIC driver and then adding CPU
> topology data to device tree sources of Exynos4.
> 
> [On Exynos4210-based TRATS and Exynos4412-based TRATS2 board]
> Tested-by: Tomasz Figa <t.figa@samsung.com>
> 
> Tomasz Figa (4):
>   ARM: EXYNOS: Fix definitions of S5P_ARM_CORE_* registers
>   ARM: EXYNOS: Fix core ID used by platsmp and hotplug code
>   irqchip: gic: Add support for per CPU bank offset specification in DT
>   ARM: dts: exynos4: Add CPU topology data
> 
>  Documentation/devicetree/bindings/arm/cpus.txt |  7 ++
>  Documentation/devicetree/bindings/arm/gic.txt  | 34 +++++++++-
>  arch/arm/boot/dts/exynos4210.dtsi              | 19 ++++++
>  arch/arm/boot/dts/exynos4212.dtsi              | 19 ++++++
>  arch/arm/boot/dts/exynos4412.dtsi              | 37 ++++++++--
>  arch/arm/mach-exynos/hotplug.c                 | 10 +--
>  arch/arm/mach-exynos/platsmp.c                 | 31 +++++----
>  arch/arm/mach-exynos/regs-pmu.h                |  4 +-
>  drivers/irqchip/irq-gic.c                      | 94 ++++++++++++++++++--------
>  9 files changed, 202 insertions(+), 53 deletions(-)
> 

Please drop patches 3 and 4 from this series, as patch 3 needs a bit
more work and 4 depends on 3.

Having patches 1 and 2 applied regardless would be nice, though.

Best regards,
Tomasz