[PATCH] arm: exynos: generalize power register address calculation

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
 arch/arm/mach-exynos/hotplug.c  |   10 +++++++---
 arch/arm/mach-exynos/platsmp.c  |   13 ++++++++-----
 arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 5eead53..460aec0 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -92,11 +92,15 @@ static inline void cpu_leave_lowpower(void)
 
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
+	unsigned long phys_cpu = cpu_logical_map(cpu);
+	unsigned int cpunr;
+
+	cpunr = phys_cpu & 0xF00 ? (4 + phys_cpu & 0xFF) : phys_cpu & 0xFF;
 	for (;;) {
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+		/* make cpu to be turned off at next WFI command */
+		if (cpu)
+			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		/*
 		 * here's the WFI
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 8ea02f6..e694bdf 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -92,6 +92,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
 	unsigned long phys_cpu = cpu_logical_map(cpu);
+	unsigned int cpunr;
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -109,14 +110,16 @@ 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)) {
+	cpunr = phys_cpu & 0xF00 ? (4 + phys_cpu & 0xFF) : phys_cpu & 0xFF;
+	if (!(__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
+		& S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		timeout = 10;
 
-		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		/* wait max 10 ms until secondary cpu is on */
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -125,7 +128,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		}
 
 		if (timeout == 0) {
-			printk(KERN_ERR "cpu1 power enable failed");
+			pr_err("cpu%x power enable failed", cpu);
 			spin_unlock(&boot_lock);
 			return -ETIMEDOUT;
 		}
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 7c029ce..16e17e4 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -104,8 +104,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
+#define S5P_ARM_CORE_STATUS(_cpunr)		\
+		(S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
-- 
1.7.9.5

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
 arch/arm/mach-exynos/hotplug.c  |   10 +++++++---
 arch/arm/mach-exynos/platsmp.c  |   13 ++++++++-----
 arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 5eead53..460aec0 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -92,11 +92,15 @@ static inline void cpu_leave_lowpower(void)
 
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
+	unsigned long phys_cpu = cpu_logical_map(cpu);
+	unsigned int cpunr;
+
+	cpunr = phys_cpu & 0xF00 ? (4 + phys_cpu & 0xFF) : phys_cpu & 0xFF;
 	for (;;) {
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+		/* make cpu to be turned off at next WFI command */
+		if (cpu)
+			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		/*
 		 * here's the WFI
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 8ea02f6..e694bdf 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -92,6 +92,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
 	unsigned long phys_cpu = cpu_logical_map(cpu);
+	unsigned int cpunr;
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -109,14 +110,16 @@ 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)) {
+	cpunr = phys_cpu & 0xF00 ? (4 + phys_cpu & 0xFF) : phys_cpu & 0xFF;
+	if (!(__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
+		& S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		timeout = 10;
 
-		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		/* wait max 10 ms until secondary cpu is on */
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -125,7 +128,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		}
 
 		if (timeout == 0) {
-			printk(KERN_ERR "cpu1 power enable failed");
+			pr_err("cpu%x power enable failed", cpu);
 			spin_unlock(&boot_lock);
 			return -ETIMEDOUT;
 		}
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 7c029ce..16e17e4 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -104,8 +104,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
+#define S5P_ARM_CORE_STATUS(_cpunr)		\
+		(S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
-- 
1.7.9.5

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
changes in v2 : Used existing macros for clusterid and cpuid calculation

 arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
 arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
 arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 5eead53..eab6121 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -17,6 +17,7 @@
 
 #include <asm/cacheflush.h>
 #include <asm/cp15.h>
+#include <asm/cputype.h>
 #include <asm/smp_plat.h>
 
 #include <plat/cpu.h>
@@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
 
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
+	unsigned int mpidr, cpunr, cluster;
+
+	mpidr = cpu_logical_map(cpu);
+	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	/* Maximum possible cpus in a cluster can be 4 */
+	cpunr += cluster * 4;
 	for (;;) {
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+		/* make cpu to be turned off at next WFI command */
+		if (cpu)
+			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		/*
 		 * here's the WFI
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 8ea02f6..8d06b2c 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -22,6 +22,7 @@
 #include <linux/io.h>
 
 #include <asm/cacheflush.h>
+#include <asm/cputype.h>
 #include <asm/smp_plat.h>
 #include <asm/smp_scu.h>
 #include <asm/firmware.h>
@@ -92,6 +93,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
 	unsigned long phys_cpu = cpu_logical_map(cpu);
+	unsigned int mpidr, cpunr, cluster;
+
+	mpidr = cpu_logical_map(cpu);
+	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	/* Maximum possible cpus in a cluster can be 4 */
+	cpunr += cluster * 4;
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -109,14 +118,15 @@ 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(cpunr))
+		& S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		timeout = 10;
 
-		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		/* wait max 10 ms until secondary cpu is on */
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -125,7 +135,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		}
 
 		if (timeout == 0) {
-			printk(KERN_ERR "cpu1 power enable failed");
+			pr_err("cpu%x power enable failed", cpu);
 			spin_unlock(&boot_lock);
 			return -ETIMEDOUT;
 		}
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 7c029ce..16e17e4 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -104,8 +104,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
+#define S5P_ARM_CORE_STATUS(_cpunr)		\
+		(S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
-- 
1.7.9.5

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
changes in v2 : Used existing macros for clusterid and cpuid calculation

 arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
 arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
 arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
 3 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 5eead53..eab6121 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -17,6 +17,7 @@
 
 #include <asm/cacheflush.h>
 #include <asm/cp15.h>
+#include <asm/cputype.h>
 #include <asm/smp_plat.h>
 
 #include <plat/cpu.h>
@@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
 
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
+	unsigned int mpidr, cpunr, cluster;
+
+	mpidr = cpu_logical_map(cpu);
+	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	/* Maximum possible cpus in a cluster can be 4 */
+	cpunr += cluster * 4;
 	for (;;) {
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+		/* make cpu to be turned off at next WFI command */
+		if (cpu)
+			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		/*
 		 * here's the WFI
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 8ea02f6..8d06b2c 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -22,6 +22,7 @@
 #include <linux/io.h>
 
 #include <asm/cacheflush.h>
+#include <asm/cputype.h>
 #include <asm/smp_plat.h>
 #include <asm/smp_scu.h>
 #include <asm/firmware.h>
@@ -92,6 +93,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
 	unsigned long phys_cpu = cpu_logical_map(cpu);
+	unsigned int mpidr, cpunr, cluster;
+
+	mpidr = cpu_logical_map(cpu);
+	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
+	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+
+	/* Maximum possible cpus in a cluster can be 4 */
+	cpunr += cluster * 4;
 
 	/*
 	 * Set synchronisation state between this boot processor
@@ -109,14 +118,15 @@ 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(cpunr))
+		& S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		timeout = 10;
 
-		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		/* wait max 10 ms until secondary cpu is on */
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -125,7 +135,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		}
 
 		if (timeout == 0) {
-			printk(KERN_ERR "cpu1 power enable failed");
+			pr_err("cpu%x power enable failed", cpu);
 			spin_unlock(&boot_lock);
 			return -ETIMEDOUT;
 		}
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 7c029ce..16e17e4 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -104,8 +104,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
+#define S5P_ARM_CORE_STATUS(_cpunr)		\
+		(S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
-- 
1.7.9.5

Re: [PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

Hi Chander,

On 09.04.2014 13:09, Chander Kashyap wrote:
> Currently status/configuration power register values are hard-coded for cpu1.
>
> Make it generic so that it is useful for SoC's with more than two cpus.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
> changes in v2 : Used existing macros for clusterid and cpuid calculation
>
>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>   3 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 5eead53..eab6121 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -17,6 +17,7 @@
>
>   #include <asm/cacheflush.h>
>   #include <asm/cp15.h>
> +#include <asm/cputype.h>
>   #include <asm/smp_plat.h>
>
>   #include <plat/cpu.h>
> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>
>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   {
> +	unsigned int mpidr, cpunr, cluster;
> +
> +	mpidr = cpu_logical_map(cpu);
> +	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	/* Maximum possible cpus in a cluster can be 4 */
> +	cpunr += cluster * 4;

I believe this is rather a weak assumption. First of all, the limit 
seems to be hardcoded only for the few existing SoCs. In addition, the 
value is not used as a maximum, but rather it is assumed that each 
cluster has always four cores.

Moreover, it is assumed here that the mapping between core ID 
(calculated by the equation below) and PMU core numbers is 1:1, which is 
not true. On Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 
it is 0x0a, which will lead to completely wrong register offsets.

I believe the proper way to deal with this is to provide per-CPU 
property in DT called "samsung,pmu-offset" that could be used be code 
like this to calculate register addresses properly.

For now, I would recommend doing the above ignoring cluster ID 
completely to not break (and actually fix) single cluster systems and 
existing multi cluster ones on which only the first cluster is supported 
now.

After that, per-CPU PMU offset should be implemented to support 
multi-cluster SoCs with proper support of multiple clusters.

Best regards,
Tomasz

[PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

Hi Chander,

On 09.04.2014 13:09, Chander Kashyap wrote:
> Currently status/configuration power register values are hard-coded for cpu1.
>
> Make it generic so that it is useful for SoC's with more than two cpus.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
> changes in v2 : Used existing macros for clusterid and cpuid calculation
>
>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>   3 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 5eead53..eab6121 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -17,6 +17,7 @@
>
>   #include <asm/cacheflush.h>
>   #include <asm/cp15.h>
> +#include <asm/cputype.h>
>   #include <asm/smp_plat.h>
>
>   #include <plat/cpu.h>
> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>
>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   {
> +	unsigned int mpidr, cpunr, cluster;
> +
> +	mpidr = cpu_logical_map(cpu);
> +	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	/* Maximum possible cpus in a cluster can be 4 */
> +	cpunr += cluster * 4;

I believe this is rather a weak assumption. First of all, the limit 
seems to be hardcoded only for the few existing SoCs. In addition, the 
value is not used as a maximum, but rather it is assumed that each 
cluster has always four cores.

Moreover, it is assumed here that the mapping between core ID 
(calculated by the equation below) and PMU core numbers is 1:1, which is 
not true. On Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 
it is 0x0a, which will lead to completely wrong register offsets.

I believe the proper way to deal with this is to provide per-CPU 
property in DT called "samsung,pmu-offset" that could be used be code 
like this to calculate register addresses properly.

For now, I would recommend doing the above ignoring cluster ID 
completely to not break (and actually fix) single cluster systems and 
existing multi cluster ones on which only the first cluster is supported 
now.

After that, per-CPU PMU offset should be implemented to support 
multi-cluster SoCs with proper support of multiple clusters.

Best regards,
Tomasz

Re: [PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,

On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Chander,
>
>
> On 09.04.2014 13:09, Chander Kashyap wrote:
>>
>> Currently status/configuration power register values are hard-coded for
>> cpu1.
>>
>> Make it generic so that it is useful for SoC's with more than two cpus.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>
>>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>   3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> b/arch/arm/mach-exynos/hotplug.c
>> index 5eead53..eab6121 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -17,6 +17,7 @@
>>
>>   #include <asm/cacheflush.h>
>>   #include <asm/cp15.h>
>> +#include <asm/cputype.h>
>>   #include <asm/smp_plat.h>
>>
>>   #include <plat/cpu.h>
>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>
>>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   {
>> +       unsigned int mpidr, cpunr, cluster;
>> +
>> +       mpidr = cpu_logical_map(cpu);
>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +
>> +       /* Maximum possible cpus in a cluster can be 4 */
>> +       cpunr += cluster * 4;
>
>
> I believe this is rather a weak assumption. First of all, the limit seems to
> be hardcoded only for the few existing SoCs. In addition, the value is not
> used as a maximum, but rather it is assumed that each cluster has always
> four cores.

The MPIDR register contains 2 bits for cpu id. Hence maximum number of
cpus can be 4 only (A15/A9/A7).

>
> Moreover, it is assumed here that the mapping between core ID (calculated by
> the equation below) and PMU core numbers is 1:1, which is not true. On
> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
> which will lead to completely wrong register offsets.

Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
will be 0,1 and Exynos4x12 will be 0,1,2,3.

So it will not break.


>
> I believe the proper way to deal with this is to provide per-CPU property in
> DT called "samsung,pmu-offset" that could be used be code like this to
> calculate register addresses properly.
>
> For now, I would recommend doing the above ignoring cluster ID completely to
> not break (and actually fix) single cluster systems and existing multi
> cluster ones on which only the first cluster is supported now.
>
> After that, per-CPU PMU offset should be implemented to support
> multi-cluster SoCs with proper support of multiple clusters.

As of now the smp-boot (cores > 2) is broken. This is required to fix it.

>
> Best regards,
> Tomasz



-- 
with warm regards,
Chander Kashyap

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,

On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Chander,
>
>
> On 09.04.2014 13:09, Chander Kashyap wrote:
>>
>> Currently status/configuration power register values are hard-coded for
>> cpu1.
>>
>> Make it generic so that it is useful for SoC's with more than two cpus.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>
>>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>   3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> b/arch/arm/mach-exynos/hotplug.c
>> index 5eead53..eab6121 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -17,6 +17,7 @@
>>
>>   #include <asm/cacheflush.h>
>>   #include <asm/cp15.h>
>> +#include <asm/cputype.h>
>>   #include <asm/smp_plat.h>
>>
>>   #include <plat/cpu.h>
>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>
>>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   {
>> +       unsigned int mpidr, cpunr, cluster;
>> +
>> +       mpidr = cpu_logical_map(cpu);
>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +
>> +       /* Maximum possible cpus in a cluster can be 4 */
>> +       cpunr += cluster * 4;
>
>
> I believe this is rather a weak assumption. First of all, the limit seems to
> be hardcoded only for the few existing SoCs. In addition, the value is not
> used as a maximum, but rather it is assumed that each cluster has always
> four cores.

The MPIDR register contains 2 bits for cpu id. Hence maximum number of
cpus can be 4 only (A15/A9/A7).

>
> Moreover, it is assumed here that the mapping between core ID (calculated by
> the equation below) and PMU core numbers is 1:1, which is not true. On
> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
> which will lead to completely wrong register offsets.

Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
will be 0,1 and Exynos4x12 will be 0,1,2,3.

So it will not break.


>
> I believe the proper way to deal with this is to provide per-CPU property in
> DT called "samsung,pmu-offset" that could be used be code like this to
> calculate register addresses properly.
>
> For now, I would recommend doing the above ignoring cluster ID completely to
> not break (and actually fix) single cluster systems and existing multi
> cluster ones on which only the first cluster is supported now.
>
> After that, per-CPU PMU offset should be implemented to support
> multi-cluster SoCs with proper support of multiple clusters.

As of now the smp-boot (cores > 2) is broken. This is required to fix it.

>
> Best regards,
> Tomasz



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

On 09.04.2014 15:49, Chander Kashyap wrote:
> Hi Tomasz,
>
> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Chander,
>>
>>
>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>
>>> Currently status/configuration power register values are hard-coded for
>>> cpu1.
>>>
>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>
>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>> ---
>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>
>>>    arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>    arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>    arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>    3 files changed, 34 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>> b/arch/arm/mach-exynos/hotplug.c
>>> index 5eead53..eab6121 100644
>>> --- a/arch/arm/mach-exynos/hotplug.c
>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>> @@ -17,6 +17,7 @@
>>>
>>>    #include <asm/cacheflush.h>
>>>    #include <asm/cp15.h>
>>> +#include <asm/cputype.h>
>>>    #include <asm/smp_plat.h>
>>>
>>>    #include <plat/cpu.h>
>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>
>>>    static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>>    {
>>> +       unsigned int mpidr, cpunr, cluster;
>>> +
>>> +       mpidr = cpu_logical_map(cpu);
>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>> +
>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>> +       cpunr += cluster * 4;
>>
>>
>> I believe this is rather a weak assumption. First of all, the limit seems to
>> be hardcoded only for the few existing SoCs. In addition, the value is not
>> used as a maximum, but rather it is assumed that each cluster has always
>> four cores.
>
> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
> cpus can be 4 only (A15/A9/A7).
>

This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4 
little cores. Are you sure that PMU register layout on Exynos5260 
matches your equation?

>>
>> Moreover, it is assumed here that the mapping between core ID (calculated by
>> the equation below) and PMU core numbers is 1:1, which is not true. On
>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>> which will lead to completely wrong register offsets.
>
> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>
> So it will not break.

I already have patches ready fixing GIC driver, just waiting for 
3.15-rc1 to be released. Anyway, CPU topology in DT is mandatory and 
Exynos4 device tree files need to be fixed to contain them. This needs 
to be accounted for in any changes touching CPU topology related code.

>
>
>>
>> I believe the proper way to deal with this is to provide per-CPU property in
>> DT called "samsung,pmu-offset" that could be used be code like this to
>> calculate register addresses properly.
>>
>> For now, I would recommend doing the above ignoring cluster ID completely to
>> not break (and actually fix) single cluster systems and existing multi
>> cluster ones on which only the first cluster is supported now.
>>
>> After that, per-CPU PMU offset should be implemented to support
>> multi-cluster SoCs with proper support of multiple clusters.
>
> As of now the smp-boot (cores > 2) is broken. This is required to fix it.

SMP boot works fine on all four cores of Exynos 4412. Obiously 
hot-(un)plug doesn't, but this is another issue.

Best regards,
Tomasz

[PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

On 09.04.2014 15:49, Chander Kashyap wrote:
> Hi Tomasz,
>
> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>> Hi Chander,
>>
>>
>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>
>>> Currently status/configuration power register values are hard-coded for
>>> cpu1.
>>>
>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>
>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>> ---
>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>
>>>    arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>    arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>    arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>    3 files changed, 34 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>> b/arch/arm/mach-exynos/hotplug.c
>>> index 5eead53..eab6121 100644
>>> --- a/arch/arm/mach-exynos/hotplug.c
>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>> @@ -17,6 +17,7 @@
>>>
>>>    #include <asm/cacheflush.h>
>>>    #include <asm/cp15.h>
>>> +#include <asm/cputype.h>
>>>    #include <asm/smp_plat.h>
>>>
>>>    #include <plat/cpu.h>
>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>
>>>    static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>>    {
>>> +       unsigned int mpidr, cpunr, cluster;
>>> +
>>> +       mpidr = cpu_logical_map(cpu);
>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>> +
>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>> +       cpunr += cluster * 4;
>>
>>
>> I believe this is rather a weak assumption. First of all, the limit seems to
>> be hardcoded only for the few existing SoCs. In addition, the value is not
>> used as a maximum, but rather it is assumed that each cluster has always
>> four cores.
>
> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
> cpus can be 4 only (A15/A9/A7).
>

This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4 
little cores. Are you sure that PMU register layout on Exynos5260 
matches your equation?

>>
>> Moreover, it is assumed here that the mapping between core ID (calculated by
>> the equation below) and PMU core numbers is 1:1, which is not true. On
>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>> which will lead to completely wrong register offsets.
>
> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>
> So it will not break.

I already have patches ready fixing GIC driver, just waiting for 
3.15-rc1 to be released. Anyway, CPU topology in DT is mandatory and 
Exynos4 device tree files need to be fixed to contain them. This needs 
to be accounted for in any changes touching CPU topology related code.

>
>
>>
>> I believe the proper way to deal with this is to provide per-CPU property in
>> DT called "samsung,pmu-offset" that could be used be code like this to
>> calculate register addresses properly.
>>
>> For now, I would recommend doing the above ignoring cluster ID completely to
>> not break (and actually fix) single cluster systems and existing multi
>> cluster ones on which only the first cluster is supported now.
>>
>> After that, per-CPU PMU offset should be implemented to support
>> multi-cluster SoCs with proper support of multiple clusters.
>
> As of now the smp-boot (cores > 2) is broken. This is required to fix it.

SMP boot works fine on all four cores of Exynos 4412. Obiously 
hot-(un)plug doesn't, but this is another issue.

Best regards,
Tomasz

Re: [PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,

On 9 April 2014 20:15, Tomasz Figa <t.figa@samsung.com> wrote:
> On 09.04.2014 15:49, Chander Kashyap wrote:
>>
>> Hi Tomasz,
>>
>> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>>>
>>> Hi Chander,
>>>
>>>
>>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>>
>>>>
>>>> Currently status/configuration power register values are hard-coded for
>>>> cpu1.
>>>>
>>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>>
>>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>>> ---
>>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>>
>>>>    arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>>    arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>>    arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>>    3 files changed, 34 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>>> b/arch/arm/mach-exynos/hotplug.c
>>>> index 5eead53..eab6121 100644
>>>> --- a/arch/arm/mach-exynos/hotplug.c
>>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>>> @@ -17,6 +17,7 @@
>>>>
>>>>    #include <asm/cacheflush.h>
>>>>    #include <asm/cp15.h>
>>>> +#include <asm/cputype.h>
>>>>    #include <asm/smp_plat.h>
>>>>
>>>>    #include <plat/cpu.h>
>>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>>
>>>>    static inline void platform_do_lowpower(unsigned int cpu, int
>>>> *spurious)
>>>>    {
>>>> +       unsigned int mpidr, cpunr, cluster;
>>>> +
>>>> +       mpidr = cpu_logical_map(cpu);
>>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>>> +
>>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>>> +       cpunr += cluster * 4;
>>>
>>>
>>>
>>> I believe this is rather a weak assumption. First of all, the limit seems
>>> to
>>> be hardcoded only for the few existing SoCs. In addition, the value is
>>> not
>>> used as a maximum, but rather it is assumed that each cluster has always
>>> four cores.
>>
>>
>> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
>> cpus can be 4 only (A15/A9/A7).
>>
>
> This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4
> little cores. Are you sure that PMU register layout on Exynos5260 matches
> your equation?
>

Yes the equation covers that as the PMU register layout takes care for that:
Address offset are as follows:
2 Big Cores:
cpu0 : 2000
cpu1: 2080

4 Little cores:

cpu0: 2200
cpu1: 2280
cpu2: 2300
cpu3: 2380

>
>>>
>>> Moreover, it is assumed here that the mapping between core ID (calculated
>>> by
>>> the equation below) and PMU core numbers is 1:1, which is not true. On
>>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>>> which will lead to completely wrong register offsets.
>>
>>
>> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
>> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
>> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>>
>> So it will not break.
>
>
> I already have patches ready fixing GIC driver, just waiting for 3.15-rc1 to
> be released. Anyway, CPU topology in DT is mandatory and Exynos4 device tree
> files need to be fixed to contain them. This needs to be accounted for in
> any changes touching CPU topology related code.
>

That's great.

>
>>
>>
>>>
>>> I believe the proper way to deal with this is to provide per-CPU property
>>> in
>>> DT called "samsung,pmu-offset" that could be used be code like this to
>>> calculate register addresses properly.
>>>
>>> For now, I would recommend doing the above ignoring cluster ID completely
>>> to
>>> not break (and actually fix) single cluster systems and existing multi
>>> cluster ones on which only the first cluster is supported now.
>>>
>>> After that, per-CPU PMU offset should be implemented to support
>>> multi-cluster SoCs with proper support of multiple clusters.
>>
>>
>> As of now the smp-boot (cores > 2) is broken. This is required to fix it.
>
>
> SMP boot works fine on all four cores of Exynos 4412. Obiously hot-(un)plug
> doesn't, but this is another issue.
>

It works as of now as at power on all the cores powered on. Hence the
powerOn in platsmp.c doent make any difference,  It breaks in hotplug
as we always poweron cpu1, not the correct cpu.

> Best regards,
> Tomasz



-- 
with warm regards,
Chander Kashyap

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,

On 9 April 2014 20:15, Tomasz Figa <t.figa@samsung.com> wrote:
> On 09.04.2014 15:49, Chander Kashyap wrote:
>>
>> Hi Tomasz,
>>
>> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>>>
>>> Hi Chander,
>>>
>>>
>>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>>
>>>>
>>>> Currently status/configuration power register values are hard-coded for
>>>> cpu1.
>>>>
>>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>>
>>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>>> ---
>>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>>
>>>>    arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>>    arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>>    arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>>    3 files changed, 34 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>>> b/arch/arm/mach-exynos/hotplug.c
>>>> index 5eead53..eab6121 100644
>>>> --- a/arch/arm/mach-exynos/hotplug.c
>>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>>> @@ -17,6 +17,7 @@
>>>>
>>>>    #include <asm/cacheflush.h>
>>>>    #include <asm/cp15.h>
>>>> +#include <asm/cputype.h>
>>>>    #include <asm/smp_plat.h>
>>>>
>>>>    #include <plat/cpu.h>
>>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>>
>>>>    static inline void platform_do_lowpower(unsigned int cpu, int
>>>> *spurious)
>>>>    {
>>>> +       unsigned int mpidr, cpunr, cluster;
>>>> +
>>>> +       mpidr = cpu_logical_map(cpu);
>>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>>> +
>>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>>> +       cpunr += cluster * 4;
>>>
>>>
>>>
>>> I believe this is rather a weak assumption. First of all, the limit seems
>>> to
>>> be hardcoded only for the few existing SoCs. In addition, the value is
>>> not
>>> used as a maximum, but rather it is assumed that each cluster has always
>>> four cores.
>>
>>
>> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
>> cpus can be 4 only (A15/A9/A7).
>>
>
> This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4
> little cores. Are you sure that PMU register layout on Exynos5260 matches
> your equation?
>

Yes the equation covers that as the PMU register layout takes care for that:
Address offset are as follows:
2 Big Cores:
cpu0 : 2000
cpu1: 2080

4 Little cores:

cpu0: 2200
cpu1: 2280
cpu2: 2300
cpu3: 2380

>
>>>
>>> Moreover, it is assumed here that the mapping between core ID (calculated
>>> by
>>> the equation below) and PMU core numbers is 1:1, which is not true. On
>>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>>> which will lead to completely wrong register offsets.
>>
>>
>> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
>> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
>> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>>
>> So it will not break.
>
>
> I already have patches ready fixing GIC driver, just waiting for 3.15-rc1 to
> be released. Anyway, CPU topology in DT is mandatory and Exynos4 device tree
> files need to be fixed to contain them. This needs to be accounted for in
> any changes touching CPU topology related code.
>

That's great.

>
>>
>>
>>>
>>> I believe the proper way to deal with this is to provide per-CPU property
>>> in
>>> DT called "samsung,pmu-offset" that could be used be code like this to
>>> calculate register addresses properly.
>>>
>>> For now, I would recommend doing the above ignoring cluster ID completely
>>> to
>>> not break (and actually fix) single cluster systems and existing multi
>>> cluster ones on which only the first cluster is supported now.
>>>
>>> After that, per-CPU PMU offset should be implemented to support
>>> multi-cluster SoCs with proper support of multiple clusters.
>>
>>
>> As of now the smp-boot (cores > 2) is broken. This is required to fix it.
>
>
> SMP boot works fine on all four cores of Exynos 4412. Obiously hot-(un)plug
> doesn't, but this is another issue.
>

It works as of now as at power on all the cores powered on. Hence the
powerOn in platsmp.c doent make any difference,  It breaks in hotplug
as we always poweron cpu1, not the correct cpu.

> Best regards,
> Tomasz



-- 
with warm regards,
Chander Kashyap

Re: [PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi,

On 10 April 2014 11:18, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> Hi Tomasz,
>
> On 9 April 2014 20:15, Tomasz Figa <t.figa@samsung.com> wrote:
>> On 09.04.2014 15:49, Chander Kashyap wrote:
>>>
>>> Hi Tomasz,
>>>
>>> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>>>>
>>>> Hi Chander,
>>>>
>>>>
>>>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>>>
>>>>>
>>>>> Currently status/configuration power register values are hard-coded for
>>>>> cpu1.
>>>>>
>>>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>>>
>>>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>>>> ---
>>>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>>>
>>>>>    arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>>>    arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>>>    arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>>>    3 files changed, 34 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>>>> b/arch/arm/mach-exynos/hotplug.c
>>>>> index 5eead53..eab6121 100644
>>>>> --- a/arch/arm/mach-exynos/hotplug.c
>>>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>>>> @@ -17,6 +17,7 @@
>>>>>
>>>>>    #include <asm/cacheflush.h>
>>>>>    #include <asm/cp15.h>
>>>>> +#include <asm/cputype.h>
>>>>>    #include <asm/smp_plat.h>
>>>>>
>>>>>    #include <plat/cpu.h>
>>>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>>>
>>>>>    static inline void platform_do_lowpower(unsigned int cpu, int
>>>>> *spurious)
>>>>>    {
>>>>> +       unsigned int mpidr, cpunr, cluster;
>>>>> +
>>>>> +       mpidr = cpu_logical_map(cpu);
>>>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>>>> +
>>>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>>>> +       cpunr += cluster * 4;
>>>>
>>>>
>>>>
>>>> I believe this is rather a weak assumption. First of all, the limit seems
>>>> to
>>>> be hardcoded only for the few existing SoCs. In addition, the value is
>>>> not
>>>> used as a maximum, but rather it is assumed that each cluster has always
>>>> four cores.
>>>
>>>
>>> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
>>> cpus can be 4 only (A15/A9/A7).
>>>
>>
>> This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4
>> little cores. Are you sure that PMU register layout on Exynos5260 matches
>> your equation?
>>
>
> Yes the equation covers that as the PMU register layout takes care for that:
> Address offset are as follows:
> 2 Big Cores:
> cpu0 : 2000
> cpu1: 2080
>
> 4 Little cores:
>
> cpu0: 2200
> cpu1: 2280
> cpu2: 2300
> cpu3: 2380
>
>>
>>>>
>>>> Moreover, it is assumed here that the mapping between core ID (calculated
>>>> by
>>>> the equation below) and PMU core numbers is 1:1, which is not true. On
>>>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>>>> which will lead to completely wrong register offsets.
>>>
>>>
>>> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
>>> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
>>> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>>>
>>> So it will not break.
>>
>>
>> I already have patches ready fixing GIC driver, just waiting for 3.15-rc1 to
>> be released. Anyway, CPU topology in DT is mandatory and Exynos4 device tree
>> files need to be fixed to contain them. This needs to be accounted for in
>> any changes touching CPU topology related code.
>>
>
> That's great.
>
>>
>>>
>>>
>>>>
>>>> I believe the proper way to deal with this is to provide per-CPU property
>>>> in
>>>> DT called "samsung,pmu-offset" that could be used be code like this to
>>>> calculate register addresses properly.
>>>>
>>>> For now, I would recommend doing the above ignoring cluster ID completely
>>>> to
>>>> not break (and actually fix) single cluster systems and existing multi
>>>> cluster ones on which only the first cluster is supported now.
>>>>
>>>> After that, per-CPU PMU offset should be implemented to support
>>>> multi-cluster SoCs with proper support of multiple clusters.
>>>
>>>
>>> As of now the smp-boot (cores > 2) is broken. This is required to fix it.
>>
>>
>> SMP boot works fine on all four cores of Exynos 4412. Obiously hot-(un)plug
>> doesn't, but this is another issue.
>>
>
> It works as of now as at power on all the cores powered on. Hence the
> powerOn in platsmp.c doent make any difference,  It breaks in hotplug
> as we always poweron cpu1, not the correct cpu.
>
>> Best regards,
>> Tomasz
>
>
>
> --
> with warm regards,
> Chander Kashyap

Any other comments on this patch. If not then can it be merged?

-- 
with warm regards,
Chander Kashyap

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi,

On 10 April 2014 11:18, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> Hi Tomasz,
>
> On 9 April 2014 20:15, Tomasz Figa <t.figa@samsung.com> wrote:
>> On 09.04.2014 15:49, Chander Kashyap wrote:
>>>
>>> Hi Tomasz,
>>>
>>> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>>>>
>>>> Hi Chander,
>>>>
>>>>
>>>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>>>
>>>>>
>>>>> Currently status/configuration power register values are hard-coded for
>>>>> cpu1.
>>>>>
>>>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>>>
>>>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>>>> ---
>>>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>>>
>>>>>    arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>>>    arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>>>    arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>>>    3 files changed, 34 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>>>> b/arch/arm/mach-exynos/hotplug.c
>>>>> index 5eead53..eab6121 100644
>>>>> --- a/arch/arm/mach-exynos/hotplug.c
>>>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>>>> @@ -17,6 +17,7 @@
>>>>>
>>>>>    #include <asm/cacheflush.h>
>>>>>    #include <asm/cp15.h>
>>>>> +#include <asm/cputype.h>
>>>>>    #include <asm/smp_plat.h>
>>>>>
>>>>>    #include <plat/cpu.h>
>>>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>>>
>>>>>    static inline void platform_do_lowpower(unsigned int cpu, int
>>>>> *spurious)
>>>>>    {
>>>>> +       unsigned int mpidr, cpunr, cluster;
>>>>> +
>>>>> +       mpidr = cpu_logical_map(cpu);
>>>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>>>> +
>>>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>>>> +       cpunr += cluster * 4;
>>>>
>>>>
>>>>
>>>> I believe this is rather a weak assumption. First of all, the limit seems
>>>> to
>>>> be hardcoded only for the few existing SoCs. In addition, the value is
>>>> not
>>>> used as a maximum, but rather it is assumed that each cluster has always
>>>> four cores.
>>>
>>>
>>> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
>>> cpus can be 4 only (A15/A9/A7).
>>>
>>
>> This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4
>> little cores. Are you sure that PMU register layout on Exynos5260 matches
>> your equation?
>>
>
> Yes the equation covers that as the PMU register layout takes care for that:
> Address offset are as follows:
> 2 Big Cores:
> cpu0 : 2000
> cpu1: 2080
>
> 4 Little cores:
>
> cpu0: 2200
> cpu1: 2280
> cpu2: 2300
> cpu3: 2380
>
>>
>>>>
>>>> Moreover, it is assumed here that the mapping between core ID (calculated
>>>> by
>>>> the equation below) and PMU core numbers is 1:1, which is not true. On
>>>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>>>> which will lead to completely wrong register offsets.
>>>
>>>
>>> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
>>> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
>>> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>>>
>>> So it will not break.
>>
>>
>> I already have patches ready fixing GIC driver, just waiting for 3.15-rc1 to
>> be released. Anyway, CPU topology in DT is mandatory and Exynos4 device tree
>> files need to be fixed to contain them. This needs to be accounted for in
>> any changes touching CPU topology related code.
>>
>
> That's great.
>
>>
>>>
>>>
>>>>
>>>> I believe the proper way to deal with this is to provide per-CPU property
>>>> in
>>>> DT called "samsung,pmu-offset" that could be used be code like this to
>>>> calculate register addresses properly.
>>>>
>>>> For now, I would recommend doing the above ignoring cluster ID completely
>>>> to
>>>> not break (and actually fix) single cluster systems and existing multi
>>>> cluster ones on which only the first cluster is supported now.
>>>>
>>>> After that, per-CPU PMU offset should be implemented to support
>>>> multi-cluster SoCs with proper support of multiple clusters.
>>>
>>>
>>> As of now the smp-boot (cores > 2) is broken. This is required to fix it.
>>
>>
>> SMP boot works fine on all four cores of Exynos 4412. Obiously hot-(un)plug
>> doesn't, but this is another issue.
>>
>
> It works as of now as at power on all the cores powered on. Hence the
> powerOn in platsmp.c doent make any difference,  It breaks in hotplug
> as we always poweron cpu1, not the correct cpu.
>
>> Best regards,
>> Tomasz
>
>
>
> --
> with warm regards,
> Chander Kashyap

Any other comments on this patch. If not then can it be merged?

-- 
with warm regards,
Chander Kashyap

Re: [PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

On 14.04.2014 06:27, Chander Kashyap wrote:
> Hi,
>
> On 10 April 2014 11:18, Chander Kashyap <chander.kashyap@linaro.org> wrote:
>> Hi Tomasz,
>>
>> On 9 April 2014 20:15, Tomasz Figa <t.figa@samsung.com> wrote:
>>> On 09.04.2014 15:49, Chander Kashyap wrote:
>>>>
>>>> Hi Tomasz,
>>>>
>>>> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>>>>>
>>>>> Hi Chander,
>>>>>
>>>>>
>>>>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>>>>
>>>>>>
>>>>>> Currently status/configuration power register values are hard-coded for
>>>>>> cpu1.
>>>>>>
>>>>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>>>>
>>>>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>>>>> ---
>>>>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>>>>
>>>>>>     arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>>>>     arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>>>>     arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>>>>     3 files changed, 34 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>>>>> b/arch/arm/mach-exynos/hotplug.c
>>>>>> index 5eead53..eab6121 100644
>>>>>> --- a/arch/arm/mach-exynos/hotplug.c
>>>>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>>
>>>>>>     #include <asm/cacheflush.h>
>>>>>>     #include <asm/cp15.h>
>>>>>> +#include <asm/cputype.h>
>>>>>>     #include <asm/smp_plat.h>
>>>>>>
>>>>>>     #include <plat/cpu.h>
>>>>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>>>>
>>>>>>     static inline void platform_do_lowpower(unsigned int cpu, int
>>>>>> *spurious)
>>>>>>     {
>>>>>> +       unsigned int mpidr, cpunr, cluster;
>>>>>> +
>>>>>> +       mpidr = cpu_logical_map(cpu);
>>>>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>>>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>>>>> +
>>>>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>>>>> +       cpunr += cluster * 4;
>>>>>
>>>>>
>>>>>
>>>>> I believe this is rather a weak assumption. First of all, the limit seems
>>>>> to
>>>>> be hardcoded only for the few existing SoCs. In addition, the value is
>>>>> not
>>>>> used as a maximum, but rather it is assumed that each cluster has always
>>>>> four cores.
>>>>
>>>>
>>>> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
>>>> cpus can be 4 only (A15/A9/A7).
>>>>
>>>
>>> This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4
>>> little cores. Are you sure that PMU register layout on Exynos5260 matches
>>> your equation?
>>>
>>
>> Yes the equation covers that as the PMU register layout takes care for that:
>> Address offset are as follows:
>> 2 Big Cores:
>> cpu0 : 2000
>> cpu1: 2080
>>
>> 4 Little cores:
>>
>> cpu0: 2200
>> cpu1: 2280
>> cpu2: 2300
>> cpu3: 2380
>>
>>>
>>>>>
>>>>> Moreover, it is assumed here that the mapping between core ID (calculated
>>>>> by
>>>>> the equation below) and PMU core numbers is 1:1, which is not true. On
>>>>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>>>>> which will lead to completely wrong register offsets.
>>>>
>>>>
>>>> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
>>>> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
>>>> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>>>>
>>>> So it will not break.
>>>
>>>
>>> I already have patches ready fixing GIC driver, just waiting for 3.15-rc1 to
>>> be released. Anyway, CPU topology in DT is mandatory and Exynos4 device tree
>>> files need to be fixed to contain them. This needs to be accounted for in
>>> any changes touching CPU topology related code.
>>>
>>
>> That's great.
>>
>>>
>>>>
>>>>
>>>>>
>>>>> I believe the proper way to deal with this is to provide per-CPU property
>>>>> in
>>>>> DT called "samsung,pmu-offset" that could be used be code like this to
>>>>> calculate register addresses properly.
>>>>>
>>>>> For now, I would recommend doing the above ignoring cluster ID completely
>>>>> to
>>>>> not break (and actually fix) single cluster systems and existing multi
>>>>> cluster ones on which only the first cluster is supported now.
>>>>>
>>>>> After that, per-CPU PMU offset should be implemented to support
>>>>> multi-cluster SoCs with proper support of multiple clusters.
>>>>
>>>>
>>>> As of now the smp-boot (cores > 2) is broken. This is required to fix it.
>>>
>>>
>>> SMP boot works fine on all four cores of Exynos 4412. Obiously hot-(un)plug
>>> doesn't, but this is another issue.
>>>
>>
>> It works as of now as at power on all the cores powered on. Hence the
>> powerOn in platsmp.c doent make any difference,  It breaks in hotplug
>> as we always poweron cpu1, not the correct cpu.
>>
>>> Best regards,
>>> Tomasz
>>
>>
>>
>> --
>> with warm regards,
>> Chander Kashyap
>
> Any other comments on this patch. If not then can it be merged?
>

Please make the patch account for supported Exynos 4 SoCs, with topology 
data specified. The fact that GIC driver is buggy right now doesn't mean 
that newly added code should assume that topology is not specified.

Best regards,
Tomasz

[PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

On 14.04.2014 06:27, Chander Kashyap wrote:
> Hi,
>
> On 10 April 2014 11:18, Chander Kashyap <chander.kashyap@linaro.org> wrote:
>> Hi Tomasz,
>>
>> On 9 April 2014 20:15, Tomasz Figa <t.figa@samsung.com> wrote:
>>> On 09.04.2014 15:49, Chander Kashyap wrote:
>>>>
>>>> Hi Tomasz,
>>>>
>>>> On 9 April 2014 17:19, Tomasz Figa <t.figa@samsung.com> wrote:
>>>>>
>>>>> Hi Chander,
>>>>>
>>>>>
>>>>> On 09.04.2014 13:09, Chander Kashyap wrote:
>>>>>>
>>>>>>
>>>>>> Currently status/configuration power register values are hard-coded for
>>>>>> cpu1.
>>>>>>
>>>>>> Make it generic so that it is useful for SoC's with more than two cpus.
>>>>>>
>>>>>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>>>>>> ---
>>>>>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>>>>>
>>>>>>     arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>>>>>     arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>>>>>     arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>>>>>     3 files changed, 34 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/arm/mach-exynos/hotplug.c
>>>>>> b/arch/arm/mach-exynos/hotplug.c
>>>>>> index 5eead53..eab6121 100644
>>>>>> --- a/arch/arm/mach-exynos/hotplug.c
>>>>>> +++ b/arch/arm/mach-exynos/hotplug.c
>>>>>> @@ -17,6 +17,7 @@
>>>>>>
>>>>>>     #include <asm/cacheflush.h>
>>>>>>     #include <asm/cp15.h>
>>>>>> +#include <asm/cputype.h>
>>>>>>     #include <asm/smp_plat.h>
>>>>>>
>>>>>>     #include <plat/cpu.h>
>>>>>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>>>>>
>>>>>>     static inline void platform_do_lowpower(unsigned int cpu, int
>>>>>> *spurious)
>>>>>>     {
>>>>>> +       unsigned int mpidr, cpunr, cluster;
>>>>>> +
>>>>>> +       mpidr = cpu_logical_map(cpu);
>>>>>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>>>>>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>>>>>> +
>>>>>> +       /* Maximum possible cpus in a cluster can be 4 */
>>>>>> +       cpunr += cluster * 4;
>>>>>
>>>>>
>>>>>
>>>>> I believe this is rather a weak assumption. First of all, the limit seems
>>>>> to
>>>>> be hardcoded only for the few existing SoCs. In addition, the value is
>>>>> not
>>>>> used as a maximum, but rather it is assumed that each cluster has always
>>>>> four cores.
>>>>
>>>>
>>>> The MPIDR register contains 2 bits for cpu id. Hence maximum number of
>>>> cpus can be 4 only (A15/A9/A7).
>>>>
>>>
>>> This is not what I meant. Exynos5260 contains 2 big cores (not 4) and 4
>>> little cores. Are you sure that PMU register layout on Exynos5260 matches
>>> your equation?
>>>
>>
>> Yes the equation covers that as the PMU register layout takes care for that:
>> Address offset are as follows:
>> 2 Big Cores:
>> cpu0 : 2000
>> cpu1: 2080
>>
>> 4 Little cores:
>>
>> cpu0: 2200
>> cpu1: 2280
>> cpu2: 2300
>> cpu3: 2380
>>
>>>
>>>>>
>>>>> Moreover, it is assumed here that the mapping between core ID (calculated
>>>>> by
>>>>> the equation below) and PMU core numbers is 1:1, which is not true. On
>>>>> Exynos4210, the cluster ID is always 0x09 and on Exynos4x12 it is 0x0a,
>>>>> which will lead to completely wrong register offsets.
>>>>
>>>>
>>>> Exynos4210 and Exynos4x12, cluster ids are not passed from DT as it
>>>> breaks the gic_init_bases. Hence the Physical CpuID for Exynos4210
>>>> will be 0,1 and Exynos4x12 will be 0,1,2,3.
>>>>
>>>> So it will not break.
>>>
>>>
>>> I already have patches ready fixing GIC driver, just waiting for 3.15-rc1 to
>>> be released. Anyway, CPU topology in DT is mandatory and Exynos4 device tree
>>> files need to be fixed to contain them. This needs to be accounted for in
>>> any changes touching CPU topology related code.
>>>
>>
>> That's great.
>>
>>>
>>>>
>>>>
>>>>>
>>>>> I believe the proper way to deal with this is to provide per-CPU property
>>>>> in
>>>>> DT called "samsung,pmu-offset" that could be used be code like this to
>>>>> calculate register addresses properly.
>>>>>
>>>>> For now, I would recommend doing the above ignoring cluster ID completely
>>>>> to
>>>>> not break (and actually fix) single cluster systems and existing multi
>>>>> cluster ones on which only the first cluster is supported now.
>>>>>
>>>>> After that, per-CPU PMU offset should be implemented to support
>>>>> multi-cluster SoCs with proper support of multiple clusters.
>>>>
>>>>
>>>> As of now the smp-boot (cores > 2) is broken. This is required to fix it.
>>>
>>>
>>> SMP boot works fine on all four cores of Exynos 4412. Obiously hot-(un)plug
>>> doesn't, but this is another issue.
>>>
>>
>> It works as of now as at power on all the cores powered on. Hence the
>> powerOn in platsmp.c doent make any difference,  It breaks in hotplug
>> as we always poweron cpu1, not the correct cpu.
>>
>>> Best regards,
>>> Tomasz
>>
>>
>>
>> --
>> with warm regards,
>> Chander Kashyap
>
> Any other comments on this patch. If not then can it be merged?
>

Please make the patch account for supported Exynos 4 SoCs, with topology 
data specified. The fact that GIC driver is buggy right now doesn't mean 
that newly added code should assume that topology is not specified.

Best regards,
Tomasz

Re: [PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

Hi Chander,

Few more comments inline.

On 09.04.2014 13:09, Chander Kashyap wrote:
> Currently status/configuration power register values are hard-coded for cpu1.
>
> Make it generic so that it is useful for SoC's with more than two cpus.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
> changes in v2 : Used existing macros for clusterid and cpuid calculation
>
>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>   3 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 5eead53..eab6121 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -17,6 +17,7 @@
>
>   #include <asm/cacheflush.h>
>   #include <asm/cp15.h>
> +#include <asm/cputype.h>
>   #include <asm/smp_plat.h>
>
>   #include <plat/cpu.h>
> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>
>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   {
> +	unsigned int mpidr, cpunr, cluster;
> +
> +	mpidr = cpu_logical_map(cpu);
> +	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	/* Maximum possible cpus in a cluster can be 4 */
> +	cpunr += cluster * 4;

This could be put in a helper, as it seems to be used both by hotplug 
and platsmp code. Also 4 could be defined as a preprocessor macro.

>   	for (;;) {
>
> -		/* make cpu1 to be turned off at next WFI command */
> -		if (cpu == 1)
> -			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> +		/* make cpu to be turned off at next WFI command */
> +		if (cpu)

Do you need this if here at all? I don't think there would be any 
problem with this function called for CPU 0 (which shouldn't happen 
anyway, without CPU 0 hotplug support enabled).

> +			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		/*
>   		 * here's the WFI
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index 8ea02f6..8d06b2c 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -22,6 +22,7 @@
>   #include <linux/io.h>
>
>   #include <asm/cacheflush.h>
> +#include <asm/cputype.h>
>   #include <asm/smp_plat.h>
>   #include <asm/smp_scu.h>
>   #include <asm/firmware.h>
> @@ -92,6 +93,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   {
>   	unsigned long timeout;
>   	unsigned long phys_cpu = cpu_logical_map(cpu);
> +	unsigned int mpidr, cpunr, cluster;
> +
> +	mpidr = cpu_logical_map(cpu);
> +	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	/* Maximum possible cpus in a cluster can be 4 */
> +	cpunr += cluster * 4;

Here basically the same code is used as in hotplug.c, so a helper would 
be nice. (e.g. cpunr = exynos_pmu_cpunr(mpidr)).

>
>   	/*
>   	 * Set synchronisation state between this boot processor
> @@ -109,14 +118,15 @@ 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(cpunr))
> +		& S5P_CORE_LOCAL_PWR_EN)) {
>   		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
> -			     S5P_ARM_CORE1_CONFIGURATION);
> +			     S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		timeout = 10;
>
> -		/* wait max 10 ms until cpu1 is on */
> -		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> +		/* wait max 10 ms until secondary cpu is on */
> +		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>   			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
>   			if (timeout-- == 0)
>   				break;
> @@ -125,7 +135,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   		}
>
>   		if (timeout == 0) {
> -			printk(KERN_ERR "cpu1 power enable failed");
> +			pr_err("cpu%x power enable failed", cpu);

Shouldn't %d be used instead? "cpu a" on a 10-core machine would sound 
weird.

>   			spin_unlock(&boot_lock);
>   			return -ETIMEDOUT;
>   		}
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index 7c029ce..16e17e4 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -104,8 +104,13 @@
>   #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
> +#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
> +
> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
> +		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
> +#define S5P_ARM_CORE_STATUS(_cpunr)		\
> +		(S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)

Please wrap _cpunr in parentheses to make sure that passed value doesn't 
affect the order of computations.

Best regards,
Tomasz

[PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

Hi Chander,

Few more comments inline.

On 09.04.2014 13:09, Chander Kashyap wrote:
> Currently status/configuration power register values are hard-coded for cpu1.
>
> Make it generic so that it is useful for SoC's with more than two cpus.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
> changes in v2 : Used existing macros for clusterid and cpuid calculation
>
>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>   3 files changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 5eead53..eab6121 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -17,6 +17,7 @@
>
>   #include <asm/cacheflush.h>
>   #include <asm/cp15.h>
> +#include <asm/cputype.h>
>   #include <asm/smp_plat.h>
>
>   #include <plat/cpu.h>
> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>
>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   {
> +	unsigned int mpidr, cpunr, cluster;
> +
> +	mpidr = cpu_logical_map(cpu);
> +	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	/* Maximum possible cpus in a cluster can be 4 */
> +	cpunr += cluster * 4;

This could be put in a helper, as it seems to be used both by hotplug 
and platsmp code. Also 4 could be defined as a preprocessor macro.

>   	for (;;) {
>
> -		/* make cpu1 to be turned off at next WFI command */
> -		if (cpu == 1)
> -			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> +		/* make cpu to be turned off at next WFI command */
> +		if (cpu)

Do you need this if here at all? I don't think there would be any 
problem with this function called for CPU 0 (which shouldn't happen 
anyway, without CPU 0 hotplug support enabled).

> +			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		/*
>   		 * here's the WFI
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index 8ea02f6..8d06b2c 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -22,6 +22,7 @@
>   #include <linux/io.h>
>
>   #include <asm/cacheflush.h>
> +#include <asm/cputype.h>
>   #include <asm/smp_plat.h>
>   #include <asm/smp_scu.h>
>   #include <asm/firmware.h>
> @@ -92,6 +93,14 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   {
>   	unsigned long timeout;
>   	unsigned long phys_cpu = cpu_logical_map(cpu);
> +	unsigned int mpidr, cpunr, cluster;
> +
> +	mpidr = cpu_logical_map(cpu);
> +	cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
> +	cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +
> +	/* Maximum possible cpus in a cluster can be 4 */
> +	cpunr += cluster * 4;

Here basically the same code is used as in hotplug.c, so a helper would 
be nice. (e.g. cpunr = exynos_pmu_cpunr(mpidr)).

>
>   	/*
>   	 * Set synchronisation state between this boot processor
> @@ -109,14 +118,15 @@ 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(cpunr))
> +		& S5P_CORE_LOCAL_PWR_EN)) {
>   		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
> -			     S5P_ARM_CORE1_CONFIGURATION);
> +			     S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		timeout = 10;
>
> -		/* wait max 10 ms until cpu1 is on */
> -		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> +		/* wait max 10 ms until secondary cpu is on */
> +		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>   			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
>   			if (timeout-- == 0)
>   				break;
> @@ -125,7 +135,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   		}
>
>   		if (timeout == 0) {
> -			printk(KERN_ERR "cpu1 power enable failed");
> +			pr_err("cpu%x power enable failed", cpu);

Shouldn't %d be used instead? "cpu a" on a 10-core machine would sound 
weird.

>   			spin_unlock(&boot_lock);
>   			return -ETIMEDOUT;
>   		}
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index 7c029ce..16e17e4 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -104,8 +104,13 @@
>   #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
> +#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
> +
> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
> +		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
> +#define S5P_ARM_CORE_STATUS(_cpunr)		\
> +		(S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)

Please wrap _cpunr in parentheses to make sure that passed value doesn't 
affect the order of computations.

Best regards,
Tomasz

Re: [PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,
Thanks for the review comments,

On 14 April 2014 22:58, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Chander,
>
> Few more comments inline.
>
>
> On 09.04.2014 13:09, Chander Kashyap wrote:
>>
>> Currently status/configuration power register values are hard-coded for
>> cpu1.
>>
>> Make it generic so that it is useful for SoC's with more than two cpus.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>
>>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>   3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> b/arch/arm/mach-exynos/hotplug.c
>> index 5eead53..eab6121 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -17,6 +17,7 @@
>>
>>   #include <asm/cacheflush.h>
>>   #include <asm/cp15.h>
>> +#include <asm/cputype.h>
>>   #include <asm/smp_plat.h>
>>
>>   #include <plat/cpu.h>
>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>
>>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   {
>> +       unsigned int mpidr, cpunr, cluster;
>> +
>> +       mpidr = cpu_logical_map(cpu);
>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +
>> +       /* Maximum possible cpus in a cluster can be 4 */
>> +       cpunr += cluster * 4;
>
>
> This could be put in a helper, as it seems to be used both by hotplug and
> platsmp code. Also 4 could be defined as a preprocessor macro.
>

Sounds great. I will modify this.


>
>>         for (;;) {
>>
>> -               /* make cpu1 to be turned off at next WFI command */
>> -               if (cpu == 1)
>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> +               /* make cpu to be turned off at next WFI command */
>> +               if (cpu)
>
>
> Do you need this if here at all? I don't think there would be any problem
> with this function called for CPU 0 (which shouldn't happen anyway, without
> CPU 0 hotplug support enabled).
>

Well this is to avoid corner case where someone does cpu-hotplug-out
on cpu0 manually.

>
>> +                       __raw_writel(0,
>> S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 /*
>>                  * here's the WFI
>> diff --git a/arch/arm/mach-exynos/platsmp.c
>> b/arch/arm/mach-exynos/platsmp.c
>> index 8ea02f6..8d06b2c 100644
>> --- a/arch/arm/mach-exynos/platsmp.c
>> +++ b/arch/arm/mach-exynos/platsmp.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/io.h>
>>
>>   #include <asm/cacheflush.h>
>> +#include <asm/cputype.h>
>>   #include <asm/smp_plat.h>
>>   #include <asm/smp_scu.h>
>>   #include <asm/firmware.h>
>> @@ -92,6 +93,14 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>   {
>>         unsigned long timeout;
>>         unsigned long phys_cpu = cpu_logical_map(cpu);
>> +       unsigned int mpidr, cpunr, cluster;
>> +
>> +       mpidr = cpu_logical_map(cpu);
>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +
>> +       /* Maximum possible cpus in a cluster can be 4 */
>> +       cpunr += cluster * 4;
>
>
> Here basically the same code is used as in hotplug.c, so a helper would be
> nice. (e.g. cpunr = exynos_pmu_cpunr(mpidr)).

Yes,

>
>
>>
>>         /*
>>          * Set synchronisation state between this boot processor
>> @@ -109,14 +118,15 @@ 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(cpunr))
>> +               & S5P_CORE_LOCAL_PWR_EN)) {
>>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>> -                            S5P_ARM_CORE1_CONFIGURATION);
>> +                            S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 timeout = 10;
>>
>> -               /* wait max 10 ms until cpu1 is on */
>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>> +               /* wait max 10 ms until secondary cpu is on */
>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>>                         & S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
>> {
>>                         if (timeout-- == 0)
>>                                 break;
>> @@ -125,7 +135,7 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>                 }
>>
>>                 if (timeout == 0) {
>> -                       printk(KERN_ERR "cpu1 power enable failed");
>> +                       pr_err("cpu%x power enable failed", cpu);
>
>
> Shouldn't %d be used instead? "cpu a" on a 10-core machine would sound
> weird.

Ok.

>
>
>>                         spin_unlock(&boot_lock);
>>                         return -ETIMEDOUT;
>>                 }
>> diff --git a/arch/arm/mach-exynos/regs-pmu.h
>> b/arch/arm/mach-exynos/regs-pmu.h
>> index 7c029ce..16e17e4 100644
>> --- a/arch/arm/mach-exynos/regs-pmu.h
>> +++ b/arch/arm/mach-exynos/regs-pmu.h
>> @@ -104,8 +104,13 @@
>>   #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_CORE0_CONFIGURATION            S5P_PMUREG(0x2000)
>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
>> +
>> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)     \
>> +               (S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
>> +#define S5P_ARM_CORE_STATUS(_cpunr)            \
>> +               (S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)
>
>
> Please wrap _cpunr in parentheses to make sure that passed value doesn't
> affect the order of computations.

ok.

>
> Best regards,
> Tomasz

Thanks again,

-- 
with warm regards,
Chander Kashyap

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,
Thanks for the review comments,

On 14 April 2014 22:58, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Chander,
>
> Few more comments inline.
>
>
> On 09.04.2014 13:09, Chander Kashyap wrote:
>>
>> Currently status/configuration power register values are hard-coded for
>> cpu1.
>>
>> Make it generic so that it is useful for SoC's with more than two cpus.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>> changes in v2 : Used existing macros for clusterid and cpuid calculation
>>
>>   arch/arm/mach-exynos/hotplug.c  |   15 ++++++++++++---
>>   arch/arm/mach-exynos/platsmp.c  |   20 +++++++++++++++-----
>>   arch/arm/mach-exynos/regs-pmu.h |    9 +++++++--
>>   3 files changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> b/arch/arm/mach-exynos/hotplug.c
>> index 5eead53..eab6121 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -17,6 +17,7 @@
>>
>>   #include <asm/cacheflush.h>
>>   #include <asm/cp15.h>
>> +#include <asm/cputype.h>
>>   #include <asm/smp_plat.h>
>>
>>   #include <plat/cpu.h>
>> @@ -92,11 +93,19 @@ static inline void cpu_leave_lowpower(void)
>>
>>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   {
>> +       unsigned int mpidr, cpunr, cluster;
>> +
>> +       mpidr = cpu_logical_map(cpu);
>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +
>> +       /* Maximum possible cpus in a cluster can be 4 */
>> +       cpunr += cluster * 4;
>
>
> This could be put in a helper, as it seems to be used both by hotplug and
> platsmp code. Also 4 could be defined as a preprocessor macro.
>

Sounds great. I will modify this.


>
>>         for (;;) {
>>
>> -               /* make cpu1 to be turned off at next WFI command */
>> -               if (cpu == 1)
>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> +               /* make cpu to be turned off at next WFI command */
>> +               if (cpu)
>
>
> Do you need this if here at all? I don't think there would be any problem
> with this function called for CPU 0 (which shouldn't happen anyway, without
> CPU 0 hotplug support enabled).
>

Well this is to avoid corner case where someone does cpu-hotplug-out
on cpu0 manually.

>
>> +                       __raw_writel(0,
>> S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 /*
>>                  * here's the WFI
>> diff --git a/arch/arm/mach-exynos/platsmp.c
>> b/arch/arm/mach-exynos/platsmp.c
>> index 8ea02f6..8d06b2c 100644
>> --- a/arch/arm/mach-exynos/platsmp.c
>> +++ b/arch/arm/mach-exynos/platsmp.c
>> @@ -22,6 +22,7 @@
>>   #include <linux/io.h>
>>
>>   #include <asm/cacheflush.h>
>> +#include <asm/cputype.h>
>>   #include <asm/smp_plat.h>
>>   #include <asm/smp_scu.h>
>>   #include <asm/firmware.h>
>> @@ -92,6 +93,14 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>   {
>>         unsigned long timeout;
>>         unsigned long phys_cpu = cpu_logical_map(cpu);
>> +       unsigned int mpidr, cpunr, cluster;
>> +
>> +       mpidr = cpu_logical_map(cpu);
>> +       cpunr = MPIDR_AFFINITY_LEVEL(mpidr, 0);
>> +       cluster = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +
>> +       /* Maximum possible cpus in a cluster can be 4 */
>> +       cpunr += cluster * 4;
>
>
> Here basically the same code is used as in hotplug.c, so a helper would be
> nice. (e.g. cpunr = exynos_pmu_cpunr(mpidr)).

Yes,

>
>
>>
>>         /*
>>          * Set synchronisation state between this boot processor
>> @@ -109,14 +118,15 @@ 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(cpunr))
>> +               & S5P_CORE_LOCAL_PWR_EN)) {
>>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>> -                            S5P_ARM_CORE1_CONFIGURATION);
>> +                            S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 timeout = 10;
>>
>> -               /* wait max 10 ms until cpu1 is on */
>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>> +               /* wait max 10 ms until secondary cpu is on */
>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>>                         & S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
>> {
>>                         if (timeout-- == 0)
>>                                 break;
>> @@ -125,7 +135,7 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>                 }
>>
>>                 if (timeout == 0) {
>> -                       printk(KERN_ERR "cpu1 power enable failed");
>> +                       pr_err("cpu%x power enable failed", cpu);
>
>
> Shouldn't %d be used instead? "cpu a" on a 10-core machine would sound
> weird.

Ok.

>
>
>>                         spin_unlock(&boot_lock);
>>                         return -ETIMEDOUT;
>>                 }
>> diff --git a/arch/arm/mach-exynos/regs-pmu.h
>> b/arch/arm/mach-exynos/regs-pmu.h
>> index 7c029ce..16e17e4 100644
>> --- a/arch/arm/mach-exynos/regs-pmu.h
>> +++ b/arch/arm/mach-exynos/regs-pmu.h
>> @@ -104,8 +104,13 @@
>>   #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_CORE0_CONFIGURATION            S5P_PMUREG(0x2000)
>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
>> +
>> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)     \
>> +               (S5P_ARM_CORE0_CONFIGURATION + 0x80 * _cpunr)
>> +#define S5P_ARM_CORE_STATUS(_cpunr)            \
>> +               (S5P_ARM_CORE0_STATUS + 0x80 * _cpunr)
>
>
> Please wrap _cpunr in parentheses to make sure that passed value doesn't
> affect the order of computations.

ok.

>
> Best regards,
> Tomasz

Thanks again,

-- 
with warm regards,
Chander Kashyap

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
changes in v3:
	1. Move cpunr calculation to a macro
	2. Changed printk format specifier from unsigned hex to unsigned decimal
Changes in v2:
	1. Used existing macros for clusterid and cpuid calculation

 arch/arm/mach-exynos/hotplug.c  |    7 ++++---
 arch/arm/mach-exynos/platsmp.c  |   13 +++++++------
 arch/arm/mach-exynos/regs-pmu.h |   15 +++++++++++++--
 3 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 5eead53..9f74be2 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -92,11 +92,12 @@ static inline void cpu_leave_lowpower(void)
 
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
+	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
 	for (;;) {
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+		/* make cpu to be turned off at next WFI command */
+		if (cpu)
+			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		/*
 		 * here's the WFI
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 03e5e9f..d9c182f 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -90,7 +90,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
 	unsigned long phys_cpu = cpu_logical_map(cpu);
-
+	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
 	/*
 	 * Set synchronisation state between this boot processor
 	 * and the secondary one
@@ -107,14 +107,15 @@ 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(cpunr))
+		& S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		timeout = 10;
 
-		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		/* wait max 10 ms until secondary cpu is on */
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -123,7 +124,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		}
 
 		if (timeout == 0) {
-			printk(KERN_ERR "cpu1 power enable failed");
+			pr_err("cpu%u power enable failed", cpu);
 			spin_unlock(&boot_lock);
 			return -ETIMEDOUT;
 		}
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 4f6a256..0de6df4 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -105,8 +105,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
+#define S5P_ARM_CORE_STATUS(_cpunr)		\
+		(S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
@@ -313,4 +318,10 @@
 
 #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
 
+#include <asm/cputype.h>
+#define MAX_CPUS_IN_CLUSTER	4
+#define ENYNOS_PMU_CPUNR(mpidr) \
+		((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER) \
+		  + MPIDR_AFFINITY_LEVEL(mpidr, 0));
+
 #endif /* __ASM_ARCH_REGS_PMU_H */
-- 
1.7.9.5

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
---
changes in v3:
	1. Move cpunr calculation to a macro
	2. Changed printk format specifier from unsigned hex to unsigned decimal
Changes in v2:
	1. Used existing macros for clusterid and cpuid calculation

 arch/arm/mach-exynos/hotplug.c  |    7 ++++---
 arch/arm/mach-exynos/platsmp.c  |   13 +++++++------
 arch/arm/mach-exynos/regs-pmu.h |   15 +++++++++++++--
 3 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
index 5eead53..9f74be2 100644
--- a/arch/arm/mach-exynos/hotplug.c
+++ b/arch/arm/mach-exynos/hotplug.c
@@ -92,11 +92,12 @@ static inline void cpu_leave_lowpower(void)
 
 static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
 {
+	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
 	for (;;) {
 
-		/* make cpu1 to be turned off at next WFI command */
-		if (cpu == 1)
-			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
+		/* make cpu to be turned off at next WFI command */
+		if (cpu)
+			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		/*
 		 * here's the WFI
diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
index 03e5e9f..d9c182f 100644
--- a/arch/arm/mach-exynos/platsmp.c
+++ b/arch/arm/mach-exynos/platsmp.c
@@ -90,7 +90,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 {
 	unsigned long timeout;
 	unsigned long phys_cpu = cpu_logical_map(cpu);
-
+	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
 	/*
 	 * Set synchronisation state between this boot processor
 	 * and the secondary one
@@ -107,14 +107,15 @@ 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(cpunr))
+		& S5P_CORE_LOCAL_PWR_EN)) {
 		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
-			     S5P_ARM_CORE1_CONFIGURATION);
+			     S5P_ARM_CORE_CONFIGURATION(cpunr));
 
 		timeout = 10;
 
-		/* wait max 10 ms until cpu1 is on */
-		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
+		/* wait max 10 ms until secondary cpu is on */
+		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
 			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
 			if (timeout-- == 0)
 				break;
@@ -123,7 +124,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
 		}
 
 		if (timeout == 0) {
-			printk(KERN_ERR "cpu1 power enable failed");
+			pr_err("cpu%u power enable failed", cpu);
 			spin_unlock(&boot_lock);
 			return -ETIMEDOUT;
 		}
diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 4f6a256..0de6df4 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -105,8 +105,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
+#define S5P_ARM_CORE_STATUS(_cpunr)		\
+		(S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
@@ -313,4 +318,10 @@
 
 #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
 
+#include <asm/cputype.h>
+#define MAX_CPUS_IN_CLUSTER	4
+#define ENYNOS_PMU_CPUNR(mpidr) \
+		((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER) \
+		  + MPIDR_AFFINITY_LEVEL(mpidr, 0));
+
 #endif /* __ASM_ARCH_REGS_PMU_H */
-- 
1.7.9.5

Re: [PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

Hi Chander,

On 15.04.2014 09:38, Chander Kashyap wrote:
> Currently status/configuration power register values are hard-coded for cpu1.
>
> Make it generic so that it is useful for SoC's with more than two cpus.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
> changes in v3:
> 	1. Move cpunr calculation to a macro
> 	2. Changed printk format specifier from unsigned hex to unsigned decimal
> Changes in v2:
> 	1. Used existing macros for clusterid and cpuid calculation
>
>   arch/arm/mach-exynos/hotplug.c  |    7 ++++---
>   arch/arm/mach-exynos/platsmp.c  |   13 +++++++------
>   arch/arm/mach-exynos/regs-pmu.h |   15 +++++++++++++--
>   3 files changed, 24 insertions(+), 11 deletions(-)
>

Now as I think of it, the code that is touched by this patch is not 
supposed to be used on multi-cluster systems. Instead a separate MCPM 
driver should. As far as I know, somebody is said to be already working 
on this.

This means that we don't need to consider multi-cluster support in this 
patch and simplify any calculations to just account for core ID. This 
would also eliminate any need to handle non-zero cluster ID on 
single-cluster SoCs.

Please correct me if I'm wrong.

> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 5eead53..9f74be2 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -92,11 +92,12 @@ static inline void cpu_leave_lowpower(void)
>
>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   {
> +	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>   	for (;;) {
>
> -		/* make cpu1 to be turned off at next WFI command */
> -		if (cpu == 1)
> -			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> +		/* make cpu to be turned off at next WFI command */
> +		if (cpu)

As I mentioned in my previous reply, I don't see what could go wrong if 
this check is omitted. What happens if CPU0 is being hot-unplugged?

If hardware doesn't support this (but I don't see any mention about this 
in the documentation), such hotplug attempt should either simply fail on 
.cpu_kill() operation for CPU0 or even have CPU0 marked as non-hotpluggable.

> +			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		/*
>   		 * here's the WFI
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index 03e5e9f..d9c182f 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -90,7 +90,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   {
>   	unsigned long timeout;
>   	unsigned long phys_cpu = cpu_logical_map(cpu);
> -
> +	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>   	/*
>   	 * Set synchronisation state between this boot processor
>   	 * and the secondary one
> @@ -107,14 +107,15 @@ 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(cpunr))
> +		& S5P_CORE_LOCAL_PWR_EN)) {
>   		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
> -			     S5P_ARM_CORE1_CONFIGURATION);
> +			     S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		timeout = 10;
>
> -		/* wait max 10 ms until cpu1 is on */
> -		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> +		/* wait max 10 ms until secondary cpu is on */
> +		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>   			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
>   			if (timeout-- == 0)
>   				break;
> @@ -123,7 +124,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   		}
>
>   		if (timeout == 0) {
> -			printk(KERN_ERR "cpu1 power enable failed");
> +			pr_err("cpu%u power enable failed", cpu);
>   			spin_unlock(&boot_lock);
>   			return -ETIMEDOUT;
>   		}
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index 4f6a256..0de6df4 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -105,8 +105,13 @@
>   #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
> +#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
> +
> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
> +		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
> +#define S5P_ARM_CORE_STATUS(_cpunr)		\
> +		(S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
>
>   #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
>   #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
> @@ -313,4 +318,10 @@
>
>   #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
>
> +#include <asm/cputype.h>
> +#define MAX_CPUS_IN_CLUSTER	4
> +#define ENYNOS_PMU_CPUNR(mpidr) \
> +		((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER) \
> +		  + MPIDR_AFFINITY_LEVEL(mpidr, 0));

Static inline would be preferred and then name in lowercase.

Best regards,
Tomasz

[PATCH] arm: exynos: generalize power register address calculation

[Tomasz Figa]

Hi Chander,

On 15.04.2014 09:38, Chander Kashyap wrote:
> Currently status/configuration power register values are hard-coded for cpu1.
>
> Make it generic so that it is useful for SoC's with more than two cpus.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> ---
> changes in v3:
> 	1. Move cpunr calculation to a macro
> 	2. Changed printk format specifier from unsigned hex to unsigned decimal
> Changes in v2:
> 	1. Used existing macros for clusterid and cpuid calculation
>
>   arch/arm/mach-exynos/hotplug.c  |    7 ++++---
>   arch/arm/mach-exynos/platsmp.c  |   13 +++++++------
>   arch/arm/mach-exynos/regs-pmu.h |   15 +++++++++++++--
>   3 files changed, 24 insertions(+), 11 deletions(-)
>

Now as I think of it, the code that is touched by this patch is not 
supposed to be used on multi-cluster systems. Instead a separate MCPM 
driver should. As far as I know, somebody is said to be already working 
on this.

This means that we don't need to consider multi-cluster support in this 
patch and simplify any calculations to just account for core ID. This 
would also eliminate any need to handle non-zero cluster ID on 
single-cluster SoCs.

Please correct me if I'm wrong.

> diff --git a/arch/arm/mach-exynos/hotplug.c b/arch/arm/mach-exynos/hotplug.c
> index 5eead53..9f74be2 100644
> --- a/arch/arm/mach-exynos/hotplug.c
> +++ b/arch/arm/mach-exynos/hotplug.c
> @@ -92,11 +92,12 @@ static inline void cpu_leave_lowpower(void)
>
>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>   {
> +	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>   	for (;;) {
>
> -		/* make cpu1 to be turned off at next WFI command */
> -		if (cpu == 1)
> -			__raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
> +		/* make cpu to be turned off at next WFI command */
> +		if (cpu)

As I mentioned in my previous reply, I don't see what could go wrong if 
this check is omitted. What happens if CPU0 is being hot-unplugged?

If hardware doesn't support this (but I don't see any mention about this 
in the documentation), such hotplug attempt should either simply fail on 
.cpu_kill() operation for CPU0 or even have CPU0 marked as non-hotpluggable.

> +			__raw_writel(0, S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		/*
>   		 * here's the WFI
> diff --git a/arch/arm/mach-exynos/platsmp.c b/arch/arm/mach-exynos/platsmp.c
> index 03e5e9f..d9c182f 100644
> --- a/arch/arm/mach-exynos/platsmp.c
> +++ b/arch/arm/mach-exynos/platsmp.c
> @@ -90,7 +90,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   {
>   	unsigned long timeout;
>   	unsigned long phys_cpu = cpu_logical_map(cpu);
> -
> +	unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>   	/*
>   	 * Set synchronisation state between this boot processor
>   	 * and the secondary one
> @@ -107,14 +107,15 @@ 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(cpunr))
> +		& S5P_CORE_LOCAL_PWR_EN)) {
>   		__raw_writel(S5P_CORE_LOCAL_PWR_EN,
> -			     S5P_ARM_CORE1_CONFIGURATION);
> +			     S5P_ARM_CORE_CONFIGURATION(cpunr));
>
>   		timeout = 10;
>
> -		/* wait max 10 ms until cpu1 is on */
> -		while ((__raw_readl(S5P_ARM_CORE1_STATUS)
> +		/* wait max 10 ms until secondary cpu is on */
> +		while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>   			& S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN) {
>   			if (timeout-- == 0)
>   				break;
> @@ -123,7 +124,7 @@ static int exynos_boot_secondary(unsigned int cpu, struct task_struct *idle)
>   		}
>
>   		if (timeout == 0) {
> -			printk(KERN_ERR "cpu1 power enable failed");
> +			pr_err("cpu%u power enable failed", cpu);
>   			spin_unlock(&boot_lock);
>   			return -ETIMEDOUT;
>   		}
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index 4f6a256..0de6df4 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -105,8 +105,13 @@
>   #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
> +#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
> +
> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)	\
> +		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
> +#define S5P_ARM_CORE_STATUS(_cpunr)		\
> +		(S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
>
>   #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
>   #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
> @@ -313,4 +318,10 @@
>
>   #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
>
> +#include <asm/cputype.h>
> +#define MAX_CPUS_IN_CLUSTER	4
> +#define ENYNOS_PMU_CPUNR(mpidr) \
> +		((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER) \
> +		  + MPIDR_AFFINITY_LEVEL(mpidr, 0));

Static inline would be preferred and then name in lowercase.

Best regards,
Tomasz

Re: [PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,

On 18 April 2014 19:42, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Chander,
>
>
> On 15.04.2014 09:38, Chander Kashyap wrote:
>>
>> Currently status/configuration power register values are hard-coded for
>> cpu1.
>>
>> Make it generic so that it is useful for SoC's with more than two cpus.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>> changes in v3:
>>         1. Move cpunr calculation to a macro
>>         2. Changed printk format specifier from unsigned hex to unsigned
>> decimal
>> Changes in v2:
>>         1. Used existing macros for clusterid and cpuid calculation
>>
>>   arch/arm/mach-exynos/hotplug.c  |    7 ++++---
>>   arch/arm/mach-exynos/platsmp.c  |   13 +++++++------
>>   arch/arm/mach-exynos/regs-pmu.h |   15 +++++++++++++--
>>   3 files changed, 24 insertions(+), 11 deletions(-)
>>
>
> Now as I think of it, the code that is touched by this patch is not supposed
> to be used on multi-cluster systems. Instead a separate MCPM driver should.
> As far as I know, somebody is said to be already working on this.
>
> This means that we don't need to consider multi-cluster support in this
> patch and simplify any calculations to just account for core ID. This would
> also eliminate any need to handle non-zero cluster ID on single-cluster
> SoCs.
>
> Please correct me if I'm wrong.

Yes thats true.
As mcpm code is in review process, so i will remove platsmp.c and
hotplug.c changes and respin the patch.
The  patch will add support for cpunr calculation and offset calculation.

>
>
>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> b/arch/arm/mach-exynos/hotplug.c
>> index 5eead53..9f74be2 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -92,11 +92,12 @@ static inline void cpu_leave_lowpower(void)
>>
>>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   {
>> +       unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>>         for (;;) {
>>
>> -               /* make cpu1 to be turned off at next WFI command */
>> -               if (cpu == 1)
>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> +               /* make cpu to be turned off at next WFI command */
>> +               if (cpu)
>
>
> As I mentioned in my previous reply, I don't see what could go wrong if this
> check is omitted. What happens if CPU0 is being hot-unplugged?
>
> If hardware doesn't support this (but I don't see any mention about this in
> the documentation), such hotplug attempt should either simply fail on
> .cpu_kill() operation for CPU0 or even have CPU0 marked as non-hotpluggable.

That makes more sense, but i need to cross verify.

>
>
>> +                       __raw_writel(0,
>> S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 /*
>>                  * here's the WFI
>> diff --git a/arch/arm/mach-exynos/platsmp.c
>> b/arch/arm/mach-exynos/platsmp.c
>> index 03e5e9f..d9c182f 100644
>> --- a/arch/arm/mach-exynos/platsmp.c
>> +++ b/arch/arm/mach-exynos/platsmp.c
>> @@ -90,7 +90,7 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>   {
>>         unsigned long timeout;
>>         unsigned long phys_cpu = cpu_logical_map(cpu);
>> -
>> +       unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>>         /*
>>          * Set synchronisation state between this boot processor
>>          * and the secondary one
>> @@ -107,14 +107,15 @@ 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(cpunr))
>> +               & S5P_CORE_LOCAL_PWR_EN)) {
>>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>> -                            S5P_ARM_CORE1_CONFIGURATION);
>> +                            S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 timeout = 10;
>>
>> -               /* wait max 10 ms until cpu1 is on */
>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>> +               /* wait max 10 ms until secondary cpu is on */
>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>>                         & S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
>> {
>>                         if (timeout-- == 0)
>>                                 break;
>> @@ -123,7 +124,7 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>                 }
>>
>>                 if (timeout == 0) {
>> -                       printk(KERN_ERR "cpu1 power enable failed");
>> +                       pr_err("cpu%u power enable failed", cpu);
>>                         spin_unlock(&boot_lock);
>>                         return -ETIMEDOUT;
>>                 }
>> diff --git a/arch/arm/mach-exynos/regs-pmu.h
>> b/arch/arm/mach-exynos/regs-pmu.h
>> index 4f6a256..0de6df4 100644
>> --- a/arch/arm/mach-exynos/regs-pmu.h
>> +++ b/arch/arm/mach-exynos/regs-pmu.h
>> @@ -105,8 +105,13 @@
>>   #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_CORE0_CONFIGURATION            S5P_PMUREG(0x2000)
>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
>> +
>> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)     \
>> +               (S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
>> +#define S5P_ARM_CORE_STATUS(_cpunr)            \
>> +               (S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
>>
>>   #define S5P_PAD_RET_MAUDIO_OPTION             S5P_PMUREG(0x3028)
>>   #define S5P_PAD_RET_GPIO_OPTION                       S5P_PMUREG(0x3108)
>> @@ -313,4 +318,10 @@
>>
>>   #define EXYNOS5_OPTION_USE_RETENTION                          (1 << 4)
>>
>> +#include <asm/cputype.h>
>> +#define MAX_CPUS_IN_CLUSTER    4
>> +#define ENYNOS_PMU_CPUNR(mpidr) \
>> +               ((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER) \
>> +                 + MPIDR_AFFINITY_LEVEL(mpidr, 0));
>
>
> Static inline would be preferred and then name in lowercase.

ok

>
> Best regards,
> Tomasz

 Thanks again

-- 
with warm regards,
Chander Kashyap

[PATCH] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Hi Tomasz,

On 18 April 2014 19:42, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Chander,
>
>
> On 15.04.2014 09:38, Chander Kashyap wrote:
>>
>> Currently status/configuration power register values are hard-coded for
>> cpu1.
>>
>> Make it generic so that it is useful for SoC's with more than two cpus.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> ---
>> changes in v3:
>>         1. Move cpunr calculation to a macro
>>         2. Changed printk format specifier from unsigned hex to unsigned
>> decimal
>> Changes in v2:
>>         1. Used existing macros for clusterid and cpuid calculation
>>
>>   arch/arm/mach-exynos/hotplug.c  |    7 ++++---
>>   arch/arm/mach-exynos/platsmp.c  |   13 +++++++------
>>   arch/arm/mach-exynos/regs-pmu.h |   15 +++++++++++++--
>>   3 files changed, 24 insertions(+), 11 deletions(-)
>>
>
> Now as I think of it, the code that is touched by this patch is not supposed
> to be used on multi-cluster systems. Instead a separate MCPM driver should.
> As far as I know, somebody is said to be already working on this.
>
> This means that we don't need to consider multi-cluster support in this
> patch and simplify any calculations to just account for core ID. This would
> also eliminate any need to handle non-zero cluster ID on single-cluster
> SoCs.
>
> Please correct me if I'm wrong.

Yes thats true.
As mcpm code is in review process, so i will remove platsmp.c and
hotplug.c changes and respin the patch.
The  patch will add support for cpunr calculation and offset calculation.

>
>
>> diff --git a/arch/arm/mach-exynos/hotplug.c
>> b/arch/arm/mach-exynos/hotplug.c
>> index 5eead53..9f74be2 100644
>> --- a/arch/arm/mach-exynos/hotplug.c
>> +++ b/arch/arm/mach-exynos/hotplug.c
>> @@ -92,11 +92,12 @@ static inline void cpu_leave_lowpower(void)
>>
>>   static inline void platform_do_lowpower(unsigned int cpu, int *spurious)
>>   {
>> +       unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>>         for (;;) {
>>
>> -               /* make cpu1 to be turned off at next WFI command */
>> -               if (cpu == 1)
>> -                       __raw_writel(0, S5P_ARM_CORE1_CONFIGURATION);
>> +               /* make cpu to be turned off at next WFI command */
>> +               if (cpu)
>
>
> As I mentioned in my previous reply, I don't see what could go wrong if this
> check is omitted. What happens if CPU0 is being hot-unplugged?
>
> If hardware doesn't support this (but I don't see any mention about this in
> the documentation), such hotplug attempt should either simply fail on
> .cpu_kill() operation for CPU0 or even have CPU0 marked as non-hotpluggable.

That makes more sense, but i need to cross verify.

>
>
>> +                       __raw_writel(0,
>> S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 /*
>>                  * here's the WFI
>> diff --git a/arch/arm/mach-exynos/platsmp.c
>> b/arch/arm/mach-exynos/platsmp.c
>> index 03e5e9f..d9c182f 100644
>> --- a/arch/arm/mach-exynos/platsmp.c
>> +++ b/arch/arm/mach-exynos/platsmp.c
>> @@ -90,7 +90,7 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>   {
>>         unsigned long timeout;
>>         unsigned long phys_cpu = cpu_logical_map(cpu);
>> -
>> +       unsigned int cpunr = ENYNOS_PMU_CPUNR(cpu_logical_map(cpu));
>>         /*
>>          * Set synchronisation state between this boot processor
>>          * and the secondary one
>> @@ -107,14 +107,15 @@ 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(cpunr))
>> +               & S5P_CORE_LOCAL_PWR_EN)) {
>>                 __raw_writel(S5P_CORE_LOCAL_PWR_EN,
>> -                            S5P_ARM_CORE1_CONFIGURATION);
>> +                            S5P_ARM_CORE_CONFIGURATION(cpunr));
>>
>>                 timeout = 10;
>>
>> -               /* wait max 10 ms until cpu1 is on */
>> -               while ((__raw_readl(S5P_ARM_CORE1_STATUS)
>> +               /* wait max 10 ms until secondary cpu is on */
>> +               while ((__raw_readl(S5P_ARM_CORE_STATUS(cpunr))
>>                         & S5P_CORE_LOCAL_PWR_EN) != S5P_CORE_LOCAL_PWR_EN)
>> {
>>                         if (timeout-- == 0)
>>                                 break;
>> @@ -123,7 +124,7 @@ static int exynos_boot_secondary(unsigned int cpu,
>> struct task_struct *idle)
>>                 }
>>
>>                 if (timeout == 0) {
>> -                       printk(KERN_ERR "cpu1 power enable failed");
>> +                       pr_err("cpu%u power enable failed", cpu);
>>                         spin_unlock(&boot_lock);
>>                         return -ETIMEDOUT;
>>                 }
>> diff --git a/arch/arm/mach-exynos/regs-pmu.h
>> b/arch/arm/mach-exynos/regs-pmu.h
>> index 4f6a256..0de6df4 100644
>> --- a/arch/arm/mach-exynos/regs-pmu.h
>> +++ b/arch/arm/mach-exynos/regs-pmu.h
>> @@ -105,8 +105,13 @@
>>   #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_CORE0_CONFIGURATION            S5P_PMUREG(0x2000)
>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
>> +
>> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr)     \
>> +               (S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
>> +#define S5P_ARM_CORE_STATUS(_cpunr)            \
>> +               (S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
>>
>>   #define S5P_PAD_RET_MAUDIO_OPTION             S5P_PMUREG(0x3028)
>>   #define S5P_PAD_RET_GPIO_OPTION                       S5P_PMUREG(0x3108)
>> @@ -313,4 +318,10 @@
>>
>>   #define EXYNOS5_OPTION_USE_RETENTION                          (1 << 4)
>>
>> +#include <asm/cputype.h>
>> +#define MAX_CPUS_IN_CLUSTER    4
>> +#define ENYNOS_PMU_CPUNR(mpidr) \
>> +               ((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER) \
>> +                 + MPIDR_AFFINITY_LEVEL(mpidr, 0));
>
>
> Static inline would be preferred and then name in lowercase.

ok

>
> Best regards,
> Tomasz

 Thanks again

-- 
with warm regards,
Chander Kashyap

[PATCH v4] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
Signed-off-by: Chander Kashyap <k.chander@samsung.org>
---
changes in v4:
	1: Dropped changes in platsmp.c and hotplug.c as those are taken care by
	   Tomasz Patches.
	2. Converted ENYNOS_PMU_CPUNR macro to static inline function
changes in v3:
	1. Move cpunr calculation to a macro
	2. Changed printk format specifier from unsigned hex to unsigned decimal
Changes in v2:
	1. Used existing macros for clusterid and cpuid calculation

 arch/arm/mach-exynos/regs-pmu.h |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 4f6a256..f39e78c 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -105,8 +105,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr) \
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
+#define S5P_ARM_CORE_STATUS(_cpunr) \
+		(S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
@@ -313,4 +318,13 @@
 
 #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
 
+#include <asm/cputype.h>
+#define MAX_CPUS_IN_CLUSTER	4
+
+static inline unsigned int enynos_pmu_cpunr(unsigned int mpidr)
+{
+	return ((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER)
+		 + MPIDR_AFFINITY_LEVEL(mpidr, 0));
+}
+
 #endif /* __ASM_ARCH_REGS_PMU_H */
-- 
1.7.9.5

[PATCH v4] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
Signed-off-by: Chander Kashyap <k.chander@samsung.org>
---
changes in v4:
	1: Dropped changes in platsmp.c and hotplug.c as those are taken care by
	   Tomasz Patches.
	2. Converted ENYNOS_PMU_CPUNR macro to static inline function
changes in v3:
	1. Move cpunr calculation to a macro
	2. Changed printk format specifier from unsigned hex to unsigned decimal
Changes in v2:
	1. Used existing macros for clusterid and cpuid calculation

 arch/arm/mach-exynos/regs-pmu.h |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 4f6a256..f39e78c 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -105,8 +105,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr) \
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
+#define S5P_ARM_CORE_STATUS(_cpunr) \
+		(S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
@@ -313,4 +318,13 @@
 
 #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
 
+#include <asm/cputype.h>
+#define MAX_CPUS_IN_CLUSTER	4
+
+static inline unsigned int enynos_pmu_cpunr(unsigned int mpidr)
+{
+	return ((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER)
+		 + MPIDR_AFFINITY_LEVEL(mpidr, 0));
+}
+
 #endif /* __ASM_ARCH_REGS_PMU_H */
-- 
1.7.9.5

[PATCH v5] arm: exynos: generalize power register address calculation

[Chander Kashyap]

Currently status/configuration power register values are hard-coded for cpu1.

Make it generic so that it is useful for SoC's with more than two cpus.

Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
Signed-off-by: Chander Kashyap <k.chander@samsung.com>
---
changes in v5:
	1. Fix typo: enynos_pmu_cpunr -> exynos_pmu_cpunr
changes in v4:
	1: Dropped changes in platsmp.c and hotplug.c as those are taken care by
	   Tomasz Patches.
	2. Converted ENYNOS_PMU_CPUNR macro to static inline function
changes in v3:
	1. Move cpunr calculation to a macro
	2. Changed printk format specifier from unsigned hex to unsigned decimal
Changes in v2:
	1. Used existing macros for clusterid and cpuid calculation

 arch/arm/mach-exynos/regs-pmu.h |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
index 4f6a256..f39e78c 100644
--- a/arch/arm/mach-exynos/regs-pmu.h
+++ b/arch/arm/mach-exynos/regs-pmu.h
@@ -105,8 +105,13 @@
 #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_CORE0_CONFIGURATION		S5P_PMUREG(0x2000)
+#define S5P_ARM_CORE0_STATUS			S5P_PMUREG(0x2004)
+
+#define S5P_ARM_CORE_CONFIGURATION(_cpunr) \
+		(S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
+#define S5P_ARM_CORE_STATUS(_cpunr) \
+		(S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
 
 #define S5P_PAD_RET_MAUDIO_OPTION		S5P_PMUREG(0x3028)
 #define S5P_PAD_RET_GPIO_OPTION			S5P_PMUREG(0x3108)
@@ -313,4 +318,13 @@
 
 #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
 
+#include <asm/cputype.h>
+#define MAX_CPUS_IN_CLUSTER	4
+
+static inline unsigned int exynos_pmu_cpunr(unsigned int mpidr)
+{
+	return ((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER)
+		 + MPIDR_AFFINITY_LEVEL(mpidr, 0));
+}
+
 #endif /* __ASM_ARCH_REGS_PMU_H */
-- 
1.7.9.5

Re: [PATCH v5] arm: exynos: generalize power register address calculation

[Chander Kashyap]

On 22 April 2014 17:55, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> Currently status/configuration power register values are hard-coded for cpu1.
>
> Make it generic so that it is useful for SoC's with more than two cpus.
>
> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
> ---
> changes in v5:
>         1. Fix typo: enynos_pmu_cpunr -> exynos_pmu_cpunr
> changes in v4:
>         1: Dropped changes in platsmp.c and hotplug.c as those are taken care by
>            Tomasz Patches.
>         2. Converted ENYNOS_PMU_CPUNR macro to static inline function
> changes in v3:
>         1. Move cpunr calculation to a macro
>         2. Changed printk format specifier from unsigned hex to unsigned decimal
> Changes in v2:
>         1. Used existing macros for clusterid and cpuid calculation
>
>  arch/arm/mach-exynos/regs-pmu.h |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
> index 4f6a256..f39e78c 100644
> --- a/arch/arm/mach-exynos/regs-pmu.h
> +++ b/arch/arm/mach-exynos/regs-pmu.h
> @@ -105,8 +105,13 @@
>  #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_CORE0_CONFIGURATION            S5P_PMUREG(0x2000)
> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
> +
> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr) \
> +               (S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
> +#define S5P_ARM_CORE_STATUS(_cpunr) \
> +               (S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
>
>  #define S5P_PAD_RET_MAUDIO_OPTION              S5P_PMUREG(0x3028)
>  #define S5P_PAD_RET_GPIO_OPTION                        S5P_PMUREG(0x3108)
> @@ -313,4 +318,13 @@
>
>  #define EXYNOS5_OPTION_USE_RETENTION                           (1 << 4)
>
> +#include <asm/cputype.h>
> +#define MAX_CPUS_IN_CLUSTER    4
> +
> +static inline unsigned int exynos_pmu_cpunr(unsigned int mpidr)
> +{
> +       return ((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER)
> +                + MPIDR_AFFINITY_LEVEL(mpidr, 0));
> +}
> +
>  #endif /* __ASM_ARCH_REGS_PMU_H */
> --
> 1.7.9.5
>

Any other comment on this. If not can this be merged?


-- 
with warm regards,
Chander Kashyap

Re: [PATCH v5] arm: exynos: generalize power register address calculation

[Chander Kashyap]

On 24 April 2014 13:18, Chander Kashyap <chander.kashyap@linaro.org> wrote:
> On 22 April 2014 17:55, Chander Kashyap <chander.kashyap@linaro.org> wrote:
>> Currently status/configuration power register values are hard-coded for cpu1.
>>
>> Make it generic so that it is useful for SoC's with more than two cpus.
>>
>> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org>
>> Signed-off-by: Chander Kashyap <k.chander@samsung.com>
>> ---
>> changes in v5:
>>         1. Fix typo: enynos_pmu_cpunr -> exynos_pmu_cpunr
>> changes in v4:
>>         1: Dropped changes in platsmp.c and hotplug.c as those are taken care by
>>            Tomasz Patches.
>>         2. Converted ENYNOS_PMU_CPUNR macro to static inline function
>> changes in v3:
>>         1. Move cpunr calculation to a macro
>>         2. Changed printk format specifier from unsigned hex to unsigned decimal
>> Changes in v2:
>>         1. Used existing macros for clusterid and cpuid calculation
>>
>>  arch/arm/mach-exynos/regs-pmu.h |   18 ++++++++++++++++--
>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/mach-exynos/regs-pmu.h b/arch/arm/mach-exynos/regs-pmu.h
>> index 4f6a256..f39e78c 100644
>> --- a/arch/arm/mach-exynos/regs-pmu.h
>> +++ b/arch/arm/mach-exynos/regs-pmu.h
>> @@ -105,8 +105,13 @@
>>  #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_CORE0_CONFIGURATION            S5P_PMUREG(0x2000)
>> +#define S5P_ARM_CORE0_STATUS                   S5P_PMUREG(0x2004)
>> +
>> +#define S5P_ARM_CORE_CONFIGURATION(_cpunr) \
>> +               (S5P_ARM_CORE0_CONFIGURATION + 0x80 * (_cpunr))
>> +#define S5P_ARM_CORE_STATUS(_cpunr) \
>> +               (S5P_ARM_CORE0_STATUS + 0x80 * (_cpunr))
>>
>>  #define S5P_PAD_RET_MAUDIO_OPTION              S5P_PMUREG(0x3028)
>>  #define S5P_PAD_RET_GPIO_OPTION                        S5P_PMUREG(0x3108)
>> @@ -313,4 +318,13 @@
>>
>>  #define EXYNOS5_OPTION_USE_RETENTION                           (1 << 4)
>>
>> +#include <asm/cputype.h>
>> +#define MAX_CPUS_IN_CLUSTER    4
>> +
>> +static inline unsigned int exynos_pmu_cpunr(unsigned int mpidr)
>> +{
>> +       return ((MPIDR_AFFINITY_LEVEL(mpidr, 1) * MAX_CPUS_IN_CLUSTER)
>> +                + MPIDR_AFFINITY_LEVEL(mpidr, 0));
>> +}
>> +
>>  #endif /* __ASM_ARCH_REGS_PMU_H */
>> --
>> 1.7.9.5
>>
>
> Any other comment on this. If not can this be merged?

Please reject this patch as some of changes also done by Tomasz in his patches.
>
>
> --
> with warm regards,
> Chander Kashyap



-- 
with warm regards,
Chander Kashyap