ARM: Initial support for Tegra 114 SoC

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/platsmp.c +1 -0

index 1b926df..68e76ef 100644
@@ -23,6 +23,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
+#include <asm/cputype.h>
 
 #include <mach/powergate.h>
 

Quoted 28 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/platsmp.c +1 -0

index 1b926df..68e76ef 100644
@@ -23,6 +23,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
+#include <asm/cputype.h>
 
 #include <mach/powergate.h>
 

@@ -34,9 +35,13 @@
 #include "common.h"
 #include "iomap.h"
 

arch/arm/mach-tegra/platsmp.c line 40

Quoted 3 lines

arch/arm/mach-tegra/platsmp.c +3 -0

+#define CPU_MASK		0xff0ffff0
+#define CPU_CORTEX_A9		0x410fc090
+#define CPU_CORTEX_A15		0x410fc0f0

NAK. There's some patches around to make this stuff generic, we don't need more ifdefs springing up. We need to get those generic patches in.

Russell King - ARM Linux <[email protected]> wrote @ Tue, 8 Jan 2013 15:26:51 +0100:

Quoted 29 lines · Hiroshi Doyu

Quoted 28 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/platsmp.c +1 -0

index 1b926df..68e76ef 100644
@@ -23,6 +23,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
+#include <asm/cputype.h>
 
 #include <mach/powergate.h>
 

@@ -34,9 +35,13 @@
 #include "common.h"
 #include "iomap.h"
 

Quoted 7 lines

arch/arm/mach-tegra/platsmp.c line 40

Quoted 3 lines

arch/arm/mach-tegra/platsmp.c +3 -0

+#define CPU_MASK		0xff0ffff0
+#define CPU_CORTEX_A9		0x410fc090
+#define CPU_CORTEX_A15		0x410fc0f0

NAK. There's some patches around to make this stuff generic, we don't need more ifdefs springing up. We need to get those generic patches in.

Does anyone give some pointers to the above patches?

Quoted 39 lines

Russell King - ARM Linux <[email protected]> wrote @ Tue, 8 Jan 2013 15:26:51 +0100:

Quoted 36 lines · Hiroshi Doyu

Quoted 25 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/platsmp.c +1 -0

index 1b926df..68e76ef 100644
@@ -23,6 +23,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
+#include <asm/cputype.h>
 
 #include <mach/powergate.h>
 

arch/arm/mach-tegra/platsmp.c line 40

Quoted 6 lines

arch/arm/mach-tegra/platsmp.c +3 -0

@@ -34,9 +35,13 @@
 #include "common.h"
 #include "iomap.h"
 
+#define CPU_MASK		0xff0ffff0
+#define CPU_CORTEX_A9		0x410fc090
+#define CPU_CORTEX_A15		0x410fc0f0

NAK. There's some patches around to make this stuff generic, we don't need more ifdefs springing up. We need to get those generic patches in.

Quoted 1 line

Does anyone give some pointers to the above patches?

Hi Hiroshi,

Please check "arch/arm/include/asm/cputype.h" in next-20130109. It just be merged.

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/platsmp.c +6 -2

index 1b926df..68e76ef 100644
@@ -23,6 +23,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
+#include <asm/cputype.h>
 
 #include <mach/powergate.h>
 

@@ -34,9 +35,13 @@
 #include "common.h"
 #include "iomap.h"
 
+#define CPU_MASK		0xff0ffff0
+#define CPU_CORTEX_A9		0x410fc090
+#define CPU_CORTEX_A15		0x410fc0f0
+
 extern void tegra_secondary_startup(void);
 
-static void __iomem *scu_base = IO_ADDRESS(TEGRA_ARM_PERIF_BASE);
+static void __iomem *scu_base;
 
 #define EVP_CPU_RESET_VECTOR \
 	(IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)

@@ -149,7 +154,26 @@ done:
  */
 static void __init tegra_smp_init_cpus(void)
 {
-	unsigned int i, ncores = scu_get_core_count(scu_base);

Hello,

Quoted 54 lines · Hiroshi Doyu

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/platsmp.c +16 -2

index 1b926df..68e76ef 100644
@@ -23,6 +23,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
+#include <asm/cputype.h>
 
 #include <mach/powergate.h>
 

@@ -34,9 +35,13 @@
 #include "common.h"
 #include "iomap.h"
 
+#define CPU_MASK		0xff0ffff0
+#define CPU_CORTEX_A9		0x410fc090
+#define CPU_CORTEX_A15		0x410fc0f0
+
 extern void tegra_secondary_startup(void);
 
-static void __iomem *scu_base = IO_ADDRESS(TEGRA_ARM_PERIF_BASE);
+static void __iomem *scu_base;
 
 #define EVP_CPU_RESET_VECTOR \
 	(IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)

@@ -149,7 +154,26 @@ done:
  */
 static void __init tegra_smp_init_cpus(void)
 {
-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

As mentioned last time [1], you should get this information from the dt instead.

Hi Mark,

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 60 lines

Hello,

Quoted 54 lines · Hiroshi Doyu

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

arch/arm/mach-tegra/platsmp.c +16 -2

index 1b926df..68e76ef 100644
@@ -23,6 +23,7 @@
 #include <asm/hardware/gic.h>
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
+#include <asm/cputype.h>
 
 #include <mach/powergate.h>
 

@@ -34,9 +35,13 @@
 #include "common.h"
 #include "iomap.h"
 
+#define CPU_MASK		0xff0ffff0
+#define CPU_CORTEX_A9		0x410fc090
+#define CPU_CORTEX_A15		0x410fc0f0
+
 extern void tegra_secondary_startup(void);
 
-static void __iomem *scu_base = IO_ADDRESS(TEGRA_ARM_PERIF_BASE);
+static void __iomem *scu_base;
 
 #define EVP_CPU_RESET_VECTOR \
 	(IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)

@@ -149,7 +154,26 @@ done:
  */
 static void __init tegra_smp_init_cpus(void)
 {
-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

On Tue, Jan 08, 2013 at 02:53:42PM +0000, Hiroshi Doyu wrote:

Quoted 16 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

arch/arm/mach-tegra/platsmp.c +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 6 lines

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Maybe someone else has a better idea?

Quoted 1 line · Hiroshi Doyu

[...]

Quoted 35 lines

Quoted 16 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

(quoted diff) +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 6 lines

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Quoted 1 line

Maybe someone else has a better idea?

Have a look at mach-vexpress/platsmp.c (keeping in mind that the GENERIC_SCU case will be refactored/removed since arm_dt_init_cpu_maps() is doing most of the required steps), please let me know what you think.

Hi Lorenzo,

Lorenzo Pieralisi <[email protected]> wrote @ Tue, 8 Jan 2013 18:11:03 +0100:

Quoted 40 lines · Mark Rutland

Quoted 1 line · Hiroshi Doyu

[...]

Quoted 35 lines

Quoted 16 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

(quoted diff) +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 6 lines

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Quoted 5 lines

Quoted 1 line

Maybe someone else has a better idea?

Have a look at mach-vexpress/platsmp.c (keeping in mind that the GENERIC_SCU case will be refactored/removed since arm_dt_init_cpu_maps() is doing most of the required steps), please let me know what you think.

In tegra, we don't support non-DT. What about falling back SCU/MRC based detection only when arm_dt_init_cpu_maps() fails?

arch/arm/mach-tegra/platsmp.c +1 -1

index 68e76ef..51d24ae 100644
@@ -152,7 +152,7 @@ done:
  * Initialise the CPU possible map early - this describes the CPUs
  * which may be present or become present in the system.
  */
-static void __init tegra_smp_init_cpus(void)
+static void __init tegra_smp_detect_cores(void)
 {
 	unsigned int i, cpu_id, ncores;
 	u32 l2ctlr;

Quoted 15 lines

arch/arm/mach-tegra/platsmp.c +1 -1

index 68e76ef..51d24ae 100644
@@ -152,7 +152,7 @@ done:
  * Initialise the CPU possible map early - this describes the CPUs
  * which may be present or become present in the system.
  */
-static void __init tegra_smp_init_cpus(void)
+static void __init tegra_smp_detect_cores(void)
 {
 	unsigned int i, cpu_id, ncores;
 	u32 l2ctlr;

@@ -183,6 +183,12 @@ static void __init tegra_smp_init_cpus(void)
 

arch/arm/mach-tegra/platsmp.c static void __init tegra_smp_init_cpus(void) · line 191

Quoted 8 lines

arch/arm/mach-tegra/platsmp.c +6 -0

 	for (i = 0; i < ncores; i++)
 		set_cpu_possible(i, true);
+}
+
+static void __init tegra_smp_init_cpus(void)
+{
+	if (!num_possible_cpus())
+		tegra_smp_detect_cores();

Thought about this, I would prefer writing an inline function that provides this information (DT missing /cpu information or parsing failure) instead of relying on num_possible_cpus to be == 0 as a way to check that.

From f5f2a43952ce75fe061b3808b994c3ceb07f1af1 Mon Sep 17 00:00:00 2001

Lorenzo Pieralisi <[email protected]> wrote @ Wed, 9 Jan 2013 16:17:00 +0100:

....

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-tegra/platsmp.c +1 -1

index 68e76ef..51d24ae 100644
@@ -152,7 +152,7 @@ done:
  * Initialise the CPU possible map early - this describes the CPUs
  * which may be present or become present in the system.
  */
-static void __init tegra_smp_init_cpus(void)
+static void __init tegra_smp_detect_cores(void)
 {
 	unsigned int i, cpu_id, ncores;
 	u32 l2ctlr;

@@ -183,6 +183,12 @@ static void __init tegra_smp_init_cpus(void)
 

Quoted 12 lines

arch/arm/mach-tegra/platsmp.c static void __init tegra_smp_init_cpus(void) · line 191

Quoted 8 lines

arch/arm/mach-tegra/platsmp.c +6 -0

 	for (i = 0; i < ncores; i++)
 		set_cpu_possible(i, true);
+}
+
+static void __init tegra_smp_init_cpus(void)
+{
+	if (!num_possible_cpus())
+		tegra_smp_detect_cores();

Thought about this, I would prefer writing an inline function that provides this information (DT missing /cpu information or parsing failure) instead of relying on num_possible_cpus to be == 0 as a way to check that.

With your "[PATCH] ARM: kernel: DT cpu map validity check helper function", my original patch gets as below. I think that the following "smp_detect_ncores()" function may be so generic that it could be moved in some common place than platform?

From f5f2a43952ce75fe061b3808b994c3ceb07f1af1 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <[email protected]>
Date: Mon, 26 Nov 2012 12:25:14 +0200
Subject: [PATCH 1/1] ARM: tegra: # of CPU cores detection w/ & w/o
HAVE_ARM_SCU

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   54 ++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 11 deletions(-)

arch/arm/mach-tegra/platsmp.c +5 -1

index 6867030..9a1324a 100644
@@ -24,6 +24,8 @@
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
 #include <asm/smp_plat.h>
+#include <asm/cputype.h>
+#include <asm/prom.h>
 
 #include <mach/powergate.h>
 

@@ -38,7 +40,7 @@
 extern void tegra_secondary_startup(void);
 
 static cpumask_t tegra_cpu_init_mask;
-static void __iomem *scu_base = IO_ADDRESS(TEGRA_ARM_PERIF_BASE);
+static void __iomem *scu_base;
 
 #define EVP_CPU_RESET_VECTOR \
 	(IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)

@@ -177,23 +179,52 @@ done:
 	return status;
 }
 
+static int __init smp_detect_ncores(void)
+{

From A15/A7 onwards DT-only cpu map initialization is the way to go.

arch/arm/mach-tegra/platsmp.c +28 -10

+	case ARM_CPU_PART_CORTEX_A9:
+		/* Get SCU physical base */
+		asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa));
+		scu_base = IO_ADDRESS(pa);
+		ncores = scu_get_core_count(scu_base);
+		break;
+	default:
+		pr_warn("Unsupported mpidr\n");
+		ncores = 1;
+		break;
+	}
+
+	return ncores;
+}
+
 /*
  * Initialise the CPU possible map early - this describes the CPUs
  * which may be present or become present in the system.
  */
 static void __init tegra_smp_init_cpus(void)
 {
-	unsigned int i, ncores = scu_get_core_count(scu_base);
-
-	if (ncores > nr_cpu_ids) {
-		pr_warn("SMP: %u cores greater than maximum (%u), clipping\n",
-			ncores, nr_cpu_ids);
-		ncores = nr_cpu_ids;
+	if (!arm_dt_cpu_map_valid()) {
+		unsigned int i, ncores;
+
+		ncores = smp_detect_ncores();
+		if (ncores > nr_cpu_ids) {
+			pr_warn("SMP: %u cores greater than maximum (%u), clipping\n",
+				ncores, nr_cpu_ids);
+			ncores = nr_cpu_ids;
+		}
+		for (i = 0; i < ncores; i++)
+			set_cpu_possible(i, true);
 	}
 
-	for (i = 0; i < ncores; i++)
-		set_cpu_possible(i, true);
-
 	set_smp_cross_call(gic_raise_softirq);
 }

@@ -202,7 +233,8 @@ static void __init tegra_smp_prepare_cpus(unsigned int max_cpus)
 	/* Always mark the boot CPU (CPU0) as initialized. */
 	cpumask_set_cpu(0, &tegra_cpu_init_mask);
 
-	scu_enable(scu_base);
+	if (scu_base)
+		scu_enable(scu_base);
 }
 
 struct smp_operations tegra_smp_ops __initdata = {

Signature · 1 line

1.7.9.5

Quoted 32 lines

Lorenzo Pieralisi <[email protected]> wrote @ Wed, 9 Jan 2013 16:17:00 +0100:

....

Quoted 15 lines

Quoted 15 lines

arch/arm/mach-tegra/platsmp.c +1 -1

index 68e76ef..51d24ae 100644
@@ -152,7 +152,7 @@ done:
  * Initialise the CPU possible map early - this describes the CPUs
  * which may be present or become present in the system.
  */
-static void __init tegra_smp_init_cpus(void)
+static void __init tegra_smp_detect_cores(void)
 {
 	unsigned int i, cpu_id, ncores;
 	u32 l2ctlr;

@@ -183,6 +183,12 @@ static void __init tegra_smp_init_cpus(void)
 

Quoted 12 lines

arch/arm/mach-tegra/platsmp.c static void __init tegra_smp_init_cpus(void) · line 191

Quoted 8 lines

arch/arm/mach-tegra/platsmp.c +6 -0

 	for (i = 0; i < ncores; i++)
 		set_cpu_possible(i, true);
+}
+
+static void __init tegra_smp_init_cpus(void)
+{
+	if (!num_possible_cpus())
+		tegra_smp_detect_cores();

Thought about this, I would prefer writing an inline function that provides this information (DT missing /cpu information or parsing failure) instead of relying on num_possible_cpus to be == 0 as a way to check that.

Quoted 4 lines

With your "[PATCH] ARM: kernel: DT cpu map validity check helper function", my original patch gets as below. I think that the following "smp_detect_ncores()" function may be so generic that it could be moved in some common place than platform?

Possibly, but I have still some comments to add.

Quoted 47 lines

From f5f2a43952ce75fe061b3808b994c3ceb07f1af1 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <[email protected]>
Date: Mon, 26 Nov 2012 12:25:14 +0200
Subject: [PATCH 1/1] ARM: tegra: # of CPU cores detection w/ & w/o
HAVE_ARM_SCU

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 arch/arm/mach-tegra/platsmp.c |   54 ++++++++++++++++++++++++++++++++---------
 1 file changed, 43 insertions(+), 11 deletions(-)

arch/arm/mach-tegra/platsmp.c +5 -1

index 6867030..9a1324a 100644
@@ -24,6 +24,8 @@
 #include <asm/mach-types.h>
 #include <asm/smp_scu.h>
 #include <asm/smp_plat.h>
+#include <asm/cputype.h>
+#include <asm/prom.h>
 
 #include <mach/powergate.h>
 

@@ -38,7 +40,7 @@
 extern void tegra_secondary_startup(void);
 
 static cpumask_t tegra_cpu_init_mask;
-static void __iomem *scu_base = IO_ADDRESS(TEGRA_ARM_PERIF_BASE);
+static void __iomem *scu_base;
 
 #define EVP_CPU_RESET_VECTOR \
 	(IO_ADDRESS(TEGRA_EXCEPTION_VECTORS_BASE) + 0x100)

@@ -177,23 +179,52 @@ done:
 	return status;
 }
 
+static int __init smp_detect_ncores(void)
+{

arch/arm/mach-tegra/platsmp.c done: · line 193

Quoted 10 lines

arch/arm/mach-tegra/platsmp.c +10 -0

+	unsigned int ncores, mpidr;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	mpidr = read_cpuid_mpidr();
+	switch (mpidr) {
+	case ARM_CPU_PART_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

I do not want to see the case above merged. We keep the existing legacy methods there for legacy reasons and as a fall-back mechanism but I am not keen on adding new HW probing code to detect the number of cores.

From A15/A7 onwards DT-only cpu map initialization is the way to go.

Lorenzo Pieralisi <[email protected]> wrote @ Thu, 10 Jan 2013 14:47:23 +0100:

Quoted 17 lines

arch/arm/mach-tegra/platsmp.c done: · line 193

Quoted 12 lines

arch/arm/mach-tegra/platsmp.c +12 -0

+static int __init smp_detect_ncores(void)
+{
+	unsigned int ncores, mpidr;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	mpidr = read_cpuid_mpidr();
+	switch (mpidr) {
+	case ARM_CPU_PART_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

I do not want to see the case above merged. We keep the existing legacy methods there for legacy reasons and as a fall-back mechanism but I am not keen on adding new HW probing code to detect the number of cores.

Quoted 1 line

From A15/A7 onwards DT-only cpu map initialization is the way to go.

Ok, then, smp_detect_ncores(void) could be...

static int __init smp_detect_ncores(void)

{
	unsigned int ncores, mpidr;
	phys_addr_t pa;

	mpidr = read_cpuid_mpidr();

	switch (mpidr) {
	case ARM_CPU_PART_CORTEX_A9:
		/* Get SCU physical base */
		asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa));
		scu_base = IO_ADDRESS(pa);
		ncores = scu_get_core_count(scu_base);
		break;
	case ARM_CPU_PART_CORTEX_A15:

	default:
		pr_warn("Unsupported MPIDR %x\n", mpidr);
		ncores = 1;
		break;

Quoted 1 line

Lorenzo Pieralisi <[email protected]> wrote @ Thu, 10 Jan 2013 14:47:23 +0100:

[...]

Quoted 19 lines

Quoted 17 lines

arch/arm/mach-tegra/platsmp.c done: · line 193

Quoted 12 lines

arch/arm/mach-tegra/platsmp.c +12 -0

+static int __init smp_detect_ncores(void)
+{
+	unsigned int ncores, mpidr;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	mpidr = read_cpuid_mpidr();
+	switch (mpidr) {
+	case ARM_CPU_PART_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

I do not want to see the case above merged. We keep the existing legacy methods there for legacy reasons and as a fall-back mechanism but I am not keen on adding new HW probing code to detect the number of cores.

Quoted 1 line

From A15/A7 onwards DT-only cpu map initialization is the way to go.

Quoted 8 lines

Ok, then, smp_detect_ncores(void) could be...

static int __init smp_detect_ncores(void)

{
	unsigned int ncores, mpidr;
	phys_addr_t pa;

	mpidr = read_cpuid_mpidr();

Careful, MPIDR is not at all what you want. MIDR is.

Quoted 8 lines

	switch (mpidr) {
	case ARM_CPU_PART_CORTEX_A9:
		/* Get SCU physical base */
		asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa));
		scu_base = IO_ADDRESS(pa);
		ncores = scu_get_core_count(scu_base);
		break;
	case ARM_CPU_PART_CORTEX_A15:

Remove the ARM_CPU_PART_CORTEX_A15 case, I do not see how it helps.

And be careful with the case matching, if I am not mistaken read_cpuid_id() (which is the function you have to use) will return the full MIDR, you need to mask the return value.

Quoted 4 lines

	default:
		pr_warn("Unsupported MPIDR %x\n", mpidr);
		ncores = 1;
		break;

This is reasonable.

Lorenzo Pieralisi <[email protected]> wrote @ Wed, 9 Jan 2013 16:17:00 +0100:

With your "[PATCH] ARM: kernel: DT cpu map validity check helper function", my original patch gets as below. I think that the following "smp_detect_ncores()" function may be so generic that it could be moved in some common place than platform?

From f5f2a43952ce75fe061b3808b994c3ceb07f1af1 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <[email protected]>
Date: Mon, 26 Nov 2012 12:25:14 +0200
Subject: [PATCH 1/1] ARM: tegra: # of CPU cores detection w/ & w/o
HAVE_ARM_SCU

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

arch/arm/mach-tegra/platsmp.c +16 -0

 
+static int __init smp_detect_ncores(void)
+{
+	unsigned int ncores, mpidr;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	mpidr = read_cpuid_mpidr();
+	switch (mpidr) {
+	case ARM_CPU_PART_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;
+	case ARM_CPU_PART_CORTEX_A9:
+		/* Get SCU physical base */
+		asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa));
+		scu_base = IO_ADDRESS(pa);

/*
* Initialise the CPU possible map early - this describes the CPUs
* which may be present or become present in the system. */
static void __init tegra_smp_init_cpus(void)
{

Quoted 1 line · Hiroshi Doyu

Lorenzo Pieralisi <[email protected]> wrote @ Wed, 9 Jan 2013 16:17:00 +0100:

...

Quoted 36 lines

With your "[PATCH] ARM: kernel: DT cpu map validity check helper function", my original patch gets as below. I think that the following "smp_detect_ncores()" function may be so generic that it could be moved in some common place than platform?

From f5f2a43952ce75fe061b3808b994c3ceb07f1af1 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <[email protected]>
Date: Mon, 26 Nov 2012 12:25:14 +0200
Subject: [PATCH 1/1] ARM: tegra: # of CPU cores detection w/ & w/o
HAVE_ARM_SCU

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

arch/arm/mach-tegra/platsmp.c +16 -0

 
+static int __init smp_detect_ncores(void)
+{
+	unsigned int ncores, mpidr;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	mpidr = read_cpuid_mpidr();
+	switch (mpidr) {
+	case ARM_CPU_PART_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;
+	case ARM_CPU_PART_CORTEX_A9:
+		/* Get SCU physical base */
+		asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa));
+		scu_base = IO_ADDRESS(pa);

arch/arm/mach-tegra/platsmp.c done: · line 207

Quoted 10 lines

arch/arm/mach-tegra/platsmp.c +10 -0

+		ncores = scu_get_core_count(scu_base);
+		break;
+	default:
+		pr_warn("Unsupported mpidr\n");
+		ncores = 1;
+		break;
+	}
+
+	return ncores;
+}

Why not just remove that function...

Quoted 6 lines

/*
* Initialise the CPU possible map early - this describes the CPUs
* which may be present or become present in the system. */
static void __init tegra_smp_init_cpus(void)
{

arch/arm/mach-tegra/platsmp.c done: · line 218

Quoted 10 lines

arch/arm/mach-tegra/platsmp.c +4 -6

-	unsigned int i, ncores = scu_get_core_count(scu_base);
-
-	if (ncores > nr_cpu_ids) {
-		pr_warn("SMP: %u cores greater than maximum (%u), clipping\n",
-			ncores, nr_cpu_ids);
-		ncores = nr_cpu_ids;
+	if (!arm_dt_cpu_map_valid()) {
+		unsigned int i, ncores;
+
+		ncores = smp_detect_ncores();

And just say "ncores = 1" here?

For Tegra, it's trivial to simply put the patch adding the required DT nodes before this patch, and then there's no need for any backwards-compatibility, since the nodes are guaranteed to be there.

Quoted 71 lines · Hiroshi Doyu

Quoted 1 line

Lorenzo Pieralisi <[email protected]> wrote @ Wed, 9 Jan 2013 16:17:00 +0100:

...

Quoted 36 lines

With your "[PATCH] ARM: kernel: DT cpu map validity check helper function", my original patch gets as below. I think that the following "smp_detect_ncores()" function may be so generic that it could be moved in some common place than platform?

From f5f2a43952ce75fe061b3808b994c3ceb07f1af1 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <[email protected]>
Date: Mon, 26 Nov 2012 12:25:14 +0200
Subject: [PATCH 1/1] ARM: tegra: # of CPU cores detection w/ & w/o
HAVE_ARM_SCU

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

arch/arm/mach-tegra/platsmp.c +16 -0

 
+static int __init smp_detect_ncores(void)
+{
+	unsigned int ncores, mpidr;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	mpidr = read_cpuid_mpidr();
+	switch (mpidr) {
+	case ARM_CPU_PART_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;
+	case ARM_CPU_PART_CORTEX_A9:
+		/* Get SCU physical base */
+		asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa));
+		scu_base = IO_ADDRESS(pa);

arch/arm/mach-tegra/platsmp.c done: · line 207

Quoted 10 lines

arch/arm/mach-tegra/platsmp.c +10 -0

+		ncores = scu_get_core_count(scu_base);
+		break;
+	default:
+		pr_warn("Unsupported mpidr\n");
+		ncores = 1;
+		break;
+	}
+
+	return ncores;
+}

Why not just remove that function...

Quoted 6 lines

/*
* Initialise the CPU possible map early - this describes the CPUs
* which may be present or become present in the system. */
static void __init tegra_smp_init_cpus(void)
{

arch/arm/mach-tegra/platsmp.c done: · line 218

Quoted 10 lines

arch/arm/mach-tegra/platsmp.c +4 -6

-	unsigned int i, ncores = scu_get_core_count(scu_base);
-
-	if (ncores > nr_cpu_ids) {
-		pr_warn("SMP: %u cores greater than maximum (%u), clipping\n",
-			ncores, nr_cpu_ids);
-		ncores = nr_cpu_ids;
+	if (!arm_dt_cpu_map_valid()) {
+		unsigned int i, ncores;
+
+		ncores = smp_detect_ncores();

And just say "ncores = 1" here?

Quoted 3 lines

For Tegra, it's trivial to simply put the patch adding the required DT nodes before this patch, and then there's no need for any backwards-compatibility, since the nodes are guaranteed to be there.

If you are willing to accept that your code proposal is perfectly fine by me.

Quoted 8 lines · Hiroshi Doyu

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 7 lines · Hiroshi Doyu

Quoted 6 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

Quoted 16 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

arch/arm/mach-tegra/platsmp.c +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 6 lines

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Quoted 9 lines · Hiroshi Doyu

Quoted 8 lines

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 7 lines · Hiroshi Doyu

Quoted 6 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

...

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

arch/arm/mach-tegra/platsmp.c +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

Quoted 5 lines

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

Quoted 6 lines

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Tegra doesn't have any non-DT support now.

If we're going to make Tegra114 read the CPU information from DT, then I'd rather just switch over all Tegra platforms to requiring CPU nodes in the DT. We have few enough SoC variants that it should be easy to switch everything to DT before adding Tegra114 support without causing any problems.

But that all said, I'm not convinced it's a good idea to force the information to be present in DT when it's just duplicating stuff that can be runtime-probed from the HW...

Hi,

Stephen Warren <[email protected]> wrote @ Tue, 8 Jan 2013 20:32:32 +0100:

Quoted 34 lines · Mark Rutland

Quoted 9 lines · Hiroshi Doyu

Quoted 8 lines

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 7 lines · Hiroshi Doyu

Quoted 6 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

...

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

arch/arm/mach-tegra/platsmp.c +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

Quoted 5 lines

Quoted 5 lines

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

True.

Quoted 7 lines

Quoted 6 lines

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Quoted 1 line

Tegra doesn't have any non-DT support now.

What about falling down to SCU/MRC hack only when DT detection fails or no /cpus entry in DT?

Can arm_dt_init_cpu_maps() return if it suceeds or not?

Quoted 7 lines

If we're going to make Tegra114 read the CPU information from DT, then I'd rather just switch over all Tegra platforms to requiring CPU nodes in the DT. We have few enough SoC variants that it should be easy to switch everything to DT before adding Tegra114 support without causing any problems.

Possible.

Quoted 3 lines

But that all said, I'm not convinced it's a good idea to force the information to be present in DT when it's just duplicating stuff that can be runtime-probed from the HW...

That's also my concern somewhat.

The main purpose to DT CPU core detection is for multiple CPU clusters. SCU/MRC cannot know anything outside of their own clusters. In tegra, we have 4 Cortex-A9/A15 core plus 1 shadow core. Switching those clusters is done transparently. The last A9/A15 core is transparently switched back and forth to the shadow one. So in this case, SCU/MRC detection seems to work fine.

Quoted 55 lines · Hiroshi Doyu

Hi,

Stephen Warren <[email protected]> wrote @ Tue, 8 Jan 2013 20:32:32 +0100:

Quoted 34 lines · Mark Rutland

Quoted 9 lines · Hiroshi Doyu

Quoted 8 lines

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 7 lines · Hiroshi Doyu

Quoted 6 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

...

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

(quoted diff) +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

Quoted 5 lines

Quoted 5 lines

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

True.

Quoted 8 lines

Quoted 6 lines

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Tegra doesn't have any non-DT support now.

Quoted 4 lines

What about falling down to SCU/MRC hack only when DT detection fails or no /cpus entry in DT?

Can arm_dt_init_cpu_maps() return if it suceeds or not?

I think the best solution to this problem consists in /me adding a function, say, arm_dt_nb_cores(), that returns an error if DT probing failed and the number of cores if it succeeded.

Quoted 61 lines · Lorenzo Pieralisi

Quoted 59 lines · Hiroshi Doyu

Hi,

Stephen Warren <[email protected]> wrote @ Tue, 8 Jan 2013 20:32:32 +0100:

Quoted 34 lines · Mark Rutland

Quoted 9 lines · Hiroshi Doyu

Quoted 8 lines

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 7 lines · Hiroshi Doyu

Quoted 6 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

...

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

(quoted diff) +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

Quoted 5 lines

Quoted 5 lines

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

True.

Quoted 8 lines

Quoted 6 lines

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Tegra doesn't have any non-DT support now.

What about falling down to SCU/MRC hack only when DT detection fails or no /cpus entry in DT?

Can arm_dt_init_cpu_maps() return if it suceeds or not?

Quoted 3 lines

I think the best solution to this problem consists in /me adding a function, say, arm_dt_nb_cores(), that returns an error if DT probing failed and the number of cores if it succeeded.

Well, if the primary mechanism needs to be DT-based going forward, I'd say just require the DT nodes to be present to use the new function, and outright fail if they aren't. That way, everything always works one way. It shouldn't be hard to add the CPU nodes for Tegra, right?

Quoted 62 lines · Lorenzo Pieralisi

Quoted 61 lines · Hiroshi Doyu

Quoted 59 lines

Hi,

Stephen Warren <[email protected]> wrote @ Tue, 8 Jan 2013 20:32:32 +0100:

Quoted 34 lines · Mark Rutland

Quoted 9 lines · Hiroshi Doyu

Quoted 8 lines

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 7 lines · Hiroshi Doyu

Quoted 6 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

...

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

(quoted diff) +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

Quoted 5 lines

Quoted 5 lines

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

True.

Quoted 8 lines

Quoted 6 lines

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Tegra doesn't have any non-DT support now.

What about falling down to SCU/MRC hack only when DT detection fails or no /cpus entry in DT?

Can arm_dt_init_cpu_maps() return if it suceeds or not?

Quoted 8 lines

Quoted 3 lines

I think the best solution to this problem consists in /me adding a function, say, arm_dt_nb_cores(), that returns an error if DT probing failed and the number of cores if it succeeded.

Well, if the primary mechanism needs to be DT-based going forward, I'd say just require the DT nodes to be present to use the new function, and outright fail if they aren't. That way, everything always works one way. It shouldn't be hard to add the CPU nodes for Tegra, right?

Adding /cpu nodes to DT is trivial, but I would like to keep a fall back mechanism in place for existing platforms to carry out the transition as smoothly as possible (eg people using new kernels with old DTS).

/cpu nodes will have priority over HW based probing for cores counting.

As discussed with Hiroshi I will post a patch to provide platforms with a DT cpu map validity check so that the fall back mechanism can be triggered properly.

Quoted 53 lines

Hi,

Stephen Warren <[email protected]> wrote @ Tue, 8 Jan 2013 20:32:32 +0100:

Quoted 34 lines · Mark Rutland

Quoted 9 lines · Hiroshi Doyu

Quoted 8 lines

Mark Rutland <[email protected]> wrote @ Tue, 8 Jan 2013 15:28:28 +0100:

Quoted 7 lines · Hiroshi Doyu

Quoted 6 lines

The method to detect the number of CPU cores on Cortex-A9 MPCore and Cortex-A15 MPCore is different. On Cortex-A9 MPCore we can get this information from the Snoop Control Unit(SCU). On Cortex-A15 MPCore we have to read it from the system coprocessor(CP15), because the SCU on Cortex-A15 MPCore does not have software readable registers. This patch selects the correct method at runtime based on the CPU ID.

...

Quoted 23 lines

Quoted 22 lines

Quoted 16 lines

Quoted 13 lines

static void __init tegra_smp_init_cpus(void)
{

(quoted diff) +10 -1

-	unsigned int i, ncores = scu_get_core_count(scu_base);
+	unsigned int i, cpu_id, ncores;
+	u32 l2ctlr;
+	phys_addr_t pa;
+
+	cpu_id = read_cpuid(CPUID_ID) & CPU_MASK;
+	switch (cpu_id) {
+	case CPU_CORTEX_A15:
+		asm("mrc p15, 1, %0, c9, c0, 2\n" : "=r" (l2ctlr));
+		ncores = ((l2ctlr >> 24) & 3) + 1;
+		break;

[...]

Quoted 2 lines

As mentioned last time [1], you should get this information from the dt instead.

Most of platsmp.c:.smp_init_cpus() implementations seem just to overwrite # of cores by SCU/MRC detection. Is there any implementation to use the DT's # and skip SCU/MRC detection in .smp_init_cpus()?

Quoted 5 lines

Quoted 5 lines

As far as I can see, there's no other platform which just relies on arm_dt_init_cpu_maps. Until recently, it didn't exist, so that makes some sense. As far as I can see, for the Tegra 114 you only need your smp_init_cpus to call set_smp_cross_call(gic_raise_softirq). Everything else you do seems to be handled by arm_dt_init_cpus.

True.

Quoted 7 lines

Quoted 6 lines

I think the best option would be to have a separate smp_ops for your dt platforms where we know cpu nodes are populated (e.g. Tegra 114), where smp_init_cpus is different to that for non-dt platforms. That way non dt platforms can keep the SCU hack for now, and won't be broken, and the dt platforms are far removed from the SCU hack and just use common infrastructure.

Quoted 6 lines

Quoted 1 line

Tegra doesn't have any non-DT support now.

What about falling down to SCU/MRC hack only when DT detection fails or no /cpus entry in DT?

Can arm_dt_init_cpu_maps() return if it suceeds or not?

I think the best solution to this problem consists in /me adding a function, say, arm_dt_nb_cores(), that returns an error if DT probing failed and the number of cores if it succeeded.

If it fails, you can fall back to SCU detection of cores for legacy reasons (A9), but from A15 onwards I have quite a strong feeling against using L2 as number of cores detection mechanism since it does not work for multi-cluster systems. Fair enough, on Tegra this is not a problem (now) but I do not want this method to propagate to other platforms where multi-cluster number of cores detection will fail if we rely on the L2 mechanism.

This means that if the DT /cpu nodes are updated for all Tegra platforms, DT becomes the mechanism to detect the number of cores for all of them.

If you allow the A9 SCU mechanism to override the DT information (even if it is correct) we might end up in the rather unwieldy situation where there is a disconnect between the cpu_logical_map initialized entries and the HW probed number of cores (if the DT bindings are correct the cpu_logical_map entries are initialized by DT).

The gist is:

- if DT /cpu nodes are there and defined properly, we build the cpu_logical_map and count the number of cores from DT
- if DT /cpu nodes are missing or bogus we rely on the cpu_logical_map in smp_setup_processor_id() and fall back to legacy mechanism to detect the number of cores. This just for legacy boards/SoC.

Let me know if this is reasonable please.

Quoted 12 lines

Quoted 5 lines

If we're going to make Tegra114 read the CPU information from DT, then I'd rather just switch over all Tegra platforms to requiring CPU nodes in the DT. We have few enough SoC variants that it should be easy to switch everything to DT before adding Tegra114 support without causing any problems.

Possible.

Quoted 3 lines

But that all said, I'm not convinced it's a good idea to force the information to be present in DT when it's just duplicating stuff that can be runtime-probed from the HW...

Quoted 8 lines

That's also my concern somewhat.

The main purpose to DT CPU core detection is for multiple CPU clusters. SCU/MRC cannot know anything outside of their own clusters. In tegra, we have 4 Cortex-A9/A15 core plus 1 shadow core. Switching those clusters is done transparently. The last A9/A15 core is transparently switched back and forth to the shadow one. So in this case, SCU/MRC detection seems to work fine.

You can probe the number of cores through SCU but remember that MPIDRs of existing CPUs are generated, not probed if the DT is not present (smp_setup_processor_id()). cpu_logical_map content, before the DT bindings were introduced, or even now if DT /cpu nodes are bogus, is simply generated as a sequential number but is not *probeable* per-se.

Lorenzo Pieralisi <[email protected]> wrote @ Wed, 9 Jan 2013 12:34:32 +0100:

Quoted 13 lines

Quoted 12 lines

Quoted 3 lines

But that all said, I'm not convinced it's a good idea to force the information to be present in DT when it's just duplicating stuff that can be runtime-probed from the HW...

That's also my concern somewhat.

The main purpose to DT CPU core detection is for multiple CPU clusters. SCU/MRC cannot know anything outside of their own clusters. In tegra, we have 4 Cortex-A9/A15 core plus 1 shadow core. Switching those clusters is done transparently. The last A9/A15 core is transparently switched back and forth to the shadow one. So in this case, SCU/MRC detection seems to work fine.

Quoted 5 lines

You can probe the number of cores through SCU but remember that MPIDRs of existing CPUs are generated, not probed if the DT is not present (smp_setup_processor_id()). cpu_logical_map content, before the DT bindings were introduced, or even now if DT /cpu nodes are bogus, is simply generated as a sequential number but is not *probeable* per-se.

Honestly I don't understand the above so well. Could you please explain a little bit more?

Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
 drivers/clocksource/tegra20_timer.c                |   65 +++++++++++++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt new file +11 -0

new file mode 100644
index 0000000..9de936a
@@ -0,0 +1,11 @@
+NVIDIA Tegra Timer Stamp Counter(TSC)
+
+Required properties:
+- compatible : "nvidia,tegra114-tsc
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	tsc {
+		compatible = "nvidia,tegra114-tsc";
+		reg = <0x700f0000 0x20000>;
+	};

drivers/clocksource/tegra20_timer.c +5 -0

index 1d25de8..564266d 100644
@@ -30,6 +30,7 @@
 #include <asm/mach/time.h>
 #include <asm/smp_twd.h>
 #include <asm/sched_clock.h>
+#include <asm/arch_timer.h>
 
 #define RTC_SECONDS            0x08
 #define RTC_SHADOW_SECONDS     0x0c

@@ -271,10 +272,72 @@ static void __init tegra20_init_tmr(void)
 	clockevents_register_device(&tegra_clockevent);
 }
 
+#define TSC_CNTCR		0		/* TSC control registers */
+#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
+#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
+

drivers/clocksource/tegra20_timer.c +20 -0

+static int tegra_arch_timer_init(void)
+{
+	int err;
+	struct device_node *np;
+	struct clk *clk;
+	void __iomem *tsc_base;
+	u32 freq, val;
+
+	np = of_find_matching_node(NULL, tegra_tsc_match);
+	if (!np)
+		return -ENODEV;
+
+	tsc_base = of_iomap(np, 0);
+	if (!tsc_base)
+		return -ENODEV;
+
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 288

Quoted 8 lines · Hiroshi Doyu

Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

drivers/clocksource/tegra20_timer.c +1 -0

 
+/* FIXME: only secure mode is supported. */

And this is a bug, as far as I'm concerned.

Quoted 11 lines

drivers/clocksource/tegra20_timer.c +10 -0

+static int tegra_arch_timer_init(void)
 
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 318

Quoted 5 lines

drivers/clocksource/tegra20_timer.c +5 -0

+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);

No, not again! Like I said last year, this won't fly. So instead of trying to work around a broken firmware, let's do the right thing.

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 324

Quoted 6 lines

drivers/clocksource/tegra20_timer.c +6 -0

+
+	val = readl_relaxed(tsc_base + TSC_CNTCR);
+	val |= TSC_CNTCR_ENABLE | TSC_CNTCR_HDBG;
+	writel_relaxed(val, tsc_base + TSC_CNTCR);
+
+	err = arch_timer_of_register();

What about adding an optional property to the binding, pointing to the required clock? That would solve the above problem in a sensible way, and your kernel wouldn't go bust.

Quoted 23 lines · Hiroshi Doyu

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 288

Quoted 8 lines

Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

drivers/clocksource/tegra20_timer.c +1 -0

 
+/* FIXME: only secure mode is supported. */

And this is a bug, as far as I'm concerned.

Quoted 11 lines

drivers/clocksource/tegra20_timer.c +10 -0

+static int tegra_arch_timer_init(void)
 
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);

Quoted 8 lines

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 318

Quoted 5 lines

drivers/clocksource/tegra20_timer.c +5 -0

+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);

No, not again! Like I said last year, this won't fly. So instead of trying to work around a broken firmware, let's do the right thing.

OK, so I understand you want to firmware/bootloader/... to write the value into that register instead, so this all works in non-secure mode (which sounds like a fine objection), but ...

Stephen Warren <[email protected]> wrote @ Tue, 8 Jan 2013 23:41:42 +0100:

Quoted 33 lines · Marc Zyngier

Quoted 31 lines · Hiroshi Doyu

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 288

Quoted 8 lines

Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

drivers/clocksource/tegra20_timer.c +1 -0

 
+/* FIXME: only secure mode is supported. */

And this is a bug, as far as I'm concerned.

Quoted 6 lines

drivers/clocksource/tegra20_timer.c +5 -0

+static int tegra_arch_timer_init(void)
 
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 318

Quoted 10 lines

drivers/clocksource/tegra20_timer.c +10 -0

+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);

No, not again! Like I said last year, this won't fly. So instead of trying to work around a broken firmware, let's do the right thing.

Quoted 3 lines

OK, so I understand you want to firmware/bootloader/... to write the value into that register instead, so this all works in non-secure mode (which sounds like a fine objection), but ...

If possible, I want to run kernel without bootloader initializing TSC . IOW, I want to allow legacy bootloaders to boot kernel with ARM arch timer.

Quoted 38 lines

Stephen Warren <[email protected]> wrote @ Tue, 8 Jan 2013 23:41:42 +0100:

Quoted 36 lines · Marc Zyngier

Quoted 23 lines · Hiroshi Doyu

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 288

Quoted 8 lines

Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

drivers/clocksource/tegra20_timer.c +1 -0

 
+/* FIXME: only secure mode is supported. */

And this is a bug, as far as I'm concerned.

Quoted 11 lines

drivers/clocksource/tegra20_timer.c +10 -0

+static int tegra_arch_timer_init(void)
 
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");
+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);

Quoted 8 lines

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 318

Quoted 5 lines

drivers/clocksource/tegra20_timer.c +5 -0

+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);

No, not again! Like I said last year, this won't fly. So instead of trying to work around a broken firmware, let's do the right thing.

OK, so I understand you want to firmware/bootloader/... to write the value into that register instead, so this all works in non-secure mode (which sounds like a fine objection), but ...

Quoted 3 lines

If possible, I want to run kernel without bootloader initializing TSC . IOW, I want to allow legacy bootloaders to boot kernel with ARM arch timer.

Is it really hard to just fix the bootloader? Tegra114 is new enough, and upstream support new enough, it should be fine to require a fixed bootloader in order to run the upstream kernel even if we don't yet do this right in the downstream kernels.

Quoted 49 lines

Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

Signed-off-by Hiroshi Doyu <[email protected]>

---
 .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
 drivers/clocksource/tegra20_timer.c                |   65 +++++++++++++++++++-
 2 files changed, 75 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt new file +11 -0

new file mode 100644
index 0000000..9de936a
@@ -0,0 +1,11 @@
+NVIDIA Tegra Timer Stamp Counter(TSC)
+
+Required properties:
+- compatible : "nvidia,tegra114-tsc
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	tsc {
+		compatible = "nvidia,tegra114-tsc";
+		reg = <0x700f0000 0x20000>;
+	};

drivers/clocksource/tegra20_timer.c +5 -0

index 1d25de8..564266d 100644
@@ -30,6 +30,7 @@
 #include <asm/mach/time.h>
 #include <asm/smp_twd.h>
 #include <asm/sched_clock.h>
+#include <asm/arch_timer.h>
 
 #define RTC_SECONDS            0x08
 #define RTC_SHADOW_SECONDS     0x0c

@@ -271,10 +272,72 @@ static void __init tegra20_init_tmr(void)
 	clockevents_register_device(&tegra_clockevent);
 }
 
+#define TSC_CNTCR		0		/* TSC control registers */
+#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
+#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
+

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 288

Quoted 10 lines

drivers/clocksource/tegra20_timer.c +10 -0

+#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
+#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
+#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
+
+static const struct of_device_id tegra_tsc_match[] __initconst = {
+	{ .compatible = "nvidia,tegra114-tsc" },
+	{}
+};
+
+/* FIXME: only secure mode is supported. */

And this is a bug, as far as I'm concerned.

Quoted 20 lines

drivers/clocksource/tegra20_timer.c +20 -0

+static int tegra_arch_timer_init(void)
+{
+	int err;
+	struct device_node *np;
+	struct clk *clk;
+	void __iomem *tsc_base;
+	u32 freq, val;
+
+	np = of_find_matching_node(NULL, tegra_tsc_match);
+	if (!np)
+		return -ENODEV;
+
+	tsc_base = of_iomap(np, 0);
+	if (!tsc_base)
+		return -ENODEV;
+
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 318

Quoted 10 lines

drivers/clocksource/tegra20_timer.c +10 -0

+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);

No, not again! Like I said last year, this won't fly. So instead of trying to work around a broken firmware, let's do the right thing.

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 324

Quoted 6 lines

drivers/clocksource/tegra20_timer.c +6 -0

+
+	val = readl_relaxed(tsc_base + TSC_CNTCR);
+	val |= TSC_CNTCR_ENABLE | TSC_CNTCR_HDBG;
+	writel_relaxed(val, tsc_base + TSC_CNTCR);
+
+	err = arch_timer_of_register();

What about adding an optional property to the binding, pointing to the required clock? That would solve the above problem in a sensible way, and your kernel wouldn't go bust.

Hi Marc,

Marc Zyngier <[email protected]> wrote @ Tue, 8 Jan 2013 17:07:39 +0100:

Quoted 97 lines · Hiroshi Doyu

Quoted 49 lines

Add platform enabler for ARM arch_timer(TSC). TSC is more fine grained timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

Signed-off-by Hiroshi Doyu <[email protected]>

---
  .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
  drivers/clocksource/tegra20_timer.c                |   65 +++++++++++++++++++-
  2 files changed, 75 insertions(+), 1 deletion(-)

create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt new file +11 -0

new file mode 100644
index 0000000..9de936a
@@ -0,0 +1,11 @@
+NVIDIA Tegra Timer Stamp Counter(TSC)
+
+Required properties:
+- compatible : "nvidia,tegra114-tsc
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	tsc {
+		compatible = "nvidia,tegra114-tsc";
+		reg = <0x700f0000 0x20000>;
+	};

drivers/clocksource/tegra20_timer.c +5 -0

index 1d25de8..564266d 100644
@@ -30,6 +30,7 @@
  #include <asm/mach/time.h>
  #include <asm/smp_twd.h>
  #include <asm/sched_clock.h>
+#include <asm/arch_timer.h>
 
  #define RTC_SECONDS            0x08
  #define RTC_SHADOW_SECONDS     0x0c

@@ -271,10 +272,72 @@ static void __init tegra20_init_tmr(void)
  	clockevents_register_device(&tegra_clockevent);
  }
 
+#define TSC_CNTCR		0		/* TSC control registers */
+#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
+#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
+

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 288

Quoted 10 lines

drivers/clocksource/tegra20_timer.c +10 -0

+#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
+#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
+#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
+
+static const struct of_device_id tegra_tsc_match[] __initconst = {
+	{ .compatible = "nvidia,tegra114-tsc" },
+	{}
+};
+
+/* FIXME: only secure mode is supported. */

And this is a bug, as far as I'm concerned.

Quoted 20 lines

drivers/clocksource/tegra20_timer.c +20 -0

+static int tegra_arch_timer_init(void)
+{
+	int err;
+	struct device_node *np;
+	struct clk *clk;
+	void __iomem *tsc_base;
+	u32 freq, val;
+
+	np = of_find_matching_node(NULL, tegra_tsc_match);
+	if (!np)
+		return -ENODEV;
+
+	tsc_base = of_iomap(np, 0);
+	if (!tsc_base)
+		return -ENODEV;
+
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input clock.\n");

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 318

Quoted 10 lines

drivers/clocksource/tegra20_timer.c +10 -0

+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);

No, not again! Like I said last year, this won't fly. So instead of trying to work around a broken firmware, let's do the right thing.

Quoted 10 lines

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 324

Quoted 6 lines

drivers/clocksource/tegra20_timer.c +6 -0

+
+	val = readl_relaxed(tsc_base + TSC_CNTCR);
+	val |= TSC_CNTCR_ENABLE | TSC_CNTCR_HDBG;
+	writel_relaxed(val, tsc_base + TSC_CNTCR);
+
+	err = arch_timer_of_register();

What about adding an optional property to the binding, pointing to the required clock? That would solve the above problem in a sensible way, and your kernel wouldn't go bust.

Could you explain a bit more? We have the follwing DT entries related to this:

	tsc {
		compatible = "nvidia,tegra114-tsc";
		reg = <0x700f0000 0x20000>;
	};

	...

	timer {
		compatible = "arm,armv7-timer";
		interrupts = <1 13 0xf08>,
			     <1 14 0xf08>,
			     <1 11 0xf08>,
			     <1 10 0xf08>;
	};

Quoted 7 lines · Hiroshi Doyu

Hi Marc,

Marc Zyngier <[email protected]> wrote @ Tue, 8 Jan 2013 17:07:39 +0100:

Quoted 2 lines · Hiroshi Doyu

Quoted 1 line

Add platform enabler for ARM arch_timer(TSC). TSC is more fine

Quoted 79 lines

Quoted 79 lines

Quoted 54 lines

timer than TMR0. If it's available, it will be used for clock source and sched_clock. Otherwise, TMR0 is used. In any case TMR0 is necessary for clock event.

Signed-off-by Hiroshi Doyu <[email protected]>

---
  .../bindings/arm/tegra/nvidia,tegra114-tsc.txt     |   11 ++++
  drivers/clocksource/tegra20_timer.c                |   65
  +++++++++++++++++++-
  2 files changed, 75 insertions(+), 1 deletion(-)

create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt new file mode 100644
index 0000000..9de936a
--- /dev/null
+++
b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt

Documentation/devicetree/bindings/arm/tegra/nvidia,tegra114-tsc.txt new file +11 -0

@@ -0,0 +1,11 @@
+NVIDIA Tegra Timer Stamp Counter(TSC)
+
+Required properties:
+- compatible : "nvidia,tegra114-tsc
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	tsc {
+		compatible = "nvidia,tegra114-tsc";
+		reg = <0x700f0000 0x20000>;
+	};

drivers/clocksource/tegra20_timer.c +5 -0

index 1d25de8..564266d 100644
@@ -30,6 +30,7 @@
  #include <asm/mach/time.h>
  #include <asm/smp_twd.h>
  #include <asm/sched_clock.h>
+#include <asm/arch_timer.h>
 
  #define RTC_SECONDS            0x08
  #define RTC_SHADOW_SECONDS     0x0c

@@ -271,10 +272,72 @@ static void __init tegra20_init_tmr(void)
  	clockevents_register_device(&tegra_clockevent);
  }
 
+#define TSC_CNTCR		0		/* TSC control registers */
+#define TSC_CNTCR_ENABLE	(1 << 0)	/* Enable */
+#define TSC_CNTCR_HDBG		(1 << 1)	/* Halt on debug */
+

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 288

Quoted 10 lines

drivers/clocksource/tegra20_timer.c +10 -0

+#define TSC_CNTCV0		0x8		/* TSC counter (LSW) */
+#define TSC_CNTCV1		0xc		/* TSC counter (MSW) */
+#define TSC_CNTFID0		0x20		/* TSC freq id 0 */
+
+static const struct of_device_id tegra_tsc_match[] __initconst = {
+	{ .compatible = "nvidia,tegra114-tsc" },
+	{}
+};
+
+/* FIXME: only secure mode is supported. */

And this is a bug, as far as I'm concerned.

Quoted 12 lines

drivers/clocksource/tegra20_timer.c +12 -0

+static int tegra_arch_timer_init(void)
+{
+	int err;
+	struct device_node *np;
+	struct clk *clk;
+	void __iomem *tsc_base;
+	u32 freq, val;
+
+	np = of_find_matching_node(NULL, tegra_tsc_match);
+	if (!np)
+		return -ENODEV;
+

Quoted 8 lines

Quoted 8 lines

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void)

Quoted 8 lines

drivers/clocksource/tegra20_timer.c +8 -0

+	tsc_base = of_iomap(np, 0);
+	if (!tsc_base)
+		return -ENODEV;
+
+	clk = clk_get_sys("clk_m", NULL);
+	if (IS_ERR(clk)) {
+		freq = 12000000;
+		pr_warn("Unable to get timer clock. Assuming 12Mhz input

Quoted 25 lines

Quoted 24 lines

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 318

Quoted 10 lines

drivers/clocksource/tegra20_timer.c +10 -0

+	} else {
+		freq = clk_get_rate(clk);
+		clk_put(clk);
+	}
+	writel_relaxed(freq, tsc_base + TSC_CNTFID0);
+
+	/* CNTFRQ */
+	asm("mcr p15, 0, %0, c14, c0, 0\n" : : "r" (freq));
+	asm("mrc p15, 0, %0, c14, c0, 0\n" : "=r" (val));
+	BUG_ON(val != freq);

No, not again! Like I said last year, this won't fly. So instead of trying to work around a broken firmware, let's do the right thing.

drivers/clocksource/tegra20_timer.c static void __init tegra20_init_tmr(void) · line 324

Quoted 6 lines

drivers/clocksource/tegra20_timer.c +6 -0

+
+	val = readl_relaxed(tsc_base + TSC_CNTCR);
+	val |= TSC_CNTCR_ENABLE | TSC_CNTCR_HDBG;
+	writel_relaxed(val, tsc_base + TSC_CNTCR);
+
+	err = arch_timer_of_register();

What about adding an optional property to the binding, pointing to the required clock? That would solve the above problem in a sensible way, and your kernel wouldn't go bust.

Quoted 1 line

Could you explain a bit more? We have the follwing DT entries related

Quoted 17 lines

this:

	tsc {
		compatible = "nvidia,tegra114-tsc";
		reg = <0x700f0000 0x20000>;
	};

	...

	timer {
		compatible = "arm,armv7-timer";
		interrupts = <1 13 0xf08>,
			     <1 14 0xf08>,
			     <1 11 0xf08>,
			     <1 10 0xf08>;
	};

Arch timer frequency can be specified in DT since the code already has provision to extract it. Below is extract from OMAP5 DT code.

timer {
                                 compatible = "arm,armv7-timer";

                                 interrupts = <1 14 0x308>;
                                 clock-frequency = <6144000>;
                         };

I think thats what Marc is pointing out.

Almost. I already proposed this in the past, but because the source clock is variable in the Tegra case, this is not flexible enough.

What I was suggesting was to do the following:

timer {
        compatible = "arm,armv7-timer";
        [...]
        clocks = <&tsc>;
}

tsc: tsc {
        compatible = "nvidia,tegra114-tsc";
        reg = <0x700f0000 0x20000>;
        freq-range = <... ...>;
        clock-output-names = "tsc";
}

In the arch_timer code, start searching for the "clocks" property, and use that if there is one. Otherwise, fall back to "clock-frequency", and ultimately to reading CNTFRQ.

This requires some changes (converting the tsc code to be a clock), but this is at least a proper description of the hardware, and should give you the required flexibility.

Hi Mark,

Marc Zyngier <[email protected]> wrote @ Wed, 9 Jan 2013 10:01:05 +0100:

Quoted 22 lines

Almost. I already proposed this in the past, but because the source clock is variable in the Tegra case, this is not flexible enough.

What I was suggesting was to do the following:

timer {
        compatible = "arm,armv7-timer";
        [...]
        clocks = <&tsc>;
}

tsc: tsc {
        compatible = "nvidia,tegra114-tsc";
        reg = <0x700f0000 0x20000>;
        freq-range = <... ...>;
        clock-output-names = "tsc";
}

In the arch_timer code, start searching for the "clocks" property, and use that if there is one. Otherwise, fall back to "clock-frequency", and ultimately to reading CNTFRQ.

Quoted 3 lines

This requires some changes (converting the tsc code to be a clock), but this is at least a proper description of the hardware, and should give you the required flexibility.

The above seems ok to me. I'll drop this original patch from this series for now until T114 clock comes to implement correctly.

If this is indeed an A15 GIC, how about adding the GICH and GICV regions, as well as the VGIC maintenance interrupt?

Quoted 4 lines

Initial support for Tegra 114 SoC. This is expected to be included in the board DTS files, Tegra 114 SoC based evaluation board family.

arch/arm/boot/dts/tegra114.dtsi +0 -0

arch/arm/boot/dts/tegra114.dtsi line 13

Quoted 7 lines

arch/arm/boot/dts/tegra114.dtsi +7 -0

+	gic: interrupt-controller {
+		compatible = "arm,cortex-a15-gic", "arm,cortex-a9-gic";
+		reg = <0x50041000 0x1000
+		       0x50042000 0x1000>;
+		interrupt-controller;
+		#interrupt-cells = <3>;
+	};

Last time around, Marc Zyngier asked:

Quoted 2 lines

If this is indeed an A15 GIC, how about adding the GICH and GICV regions, as well as the VGIC maintenance interrupt?