Skip to contentSkip to conversation

ARM: CPU hotplug: fix hard-coded control register constants

DISCUSSION viresh kumar 4 participants 5 messages Jan 14, 2011 – Jan 15, 2011 1 message not in the archive

Subsystems (4)
Arm (32-bit) Arm sub-architectures Arm Samsung Exynos Tegra architecture support
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

viresh kumar

author
#1 lore raw

Russell,

I was just tracking latest changes in realview, for preparing SPEAr support V4. I found below patch and didn't understood it very well.

Quoted 18 lines

Subject: [PATCH] ARM: CPU hotplug: fix hard-coded control register constants

Use the definition we've provided in asm/system.h rather than numeric constants.

Signed-off-by Russell King <[email protected]>
---
 arch/arm/mach-realview/hotplug.c |    8 ++++----
 arch/arm/mach-s5pv310/hotplug.c  |    8 ++++----
 arch/arm/mach-tegra/hotplug.c    |    8 ++++----
 3 files changed, 12 insertions(+), 12 deletions(-)
arch/arm/mach-realview/hotplug.c +0 -0
index b6387cf..a87523d 100644
@@ -31,10 +31,10 @@ static inline void cpu_enter_lowpower(void)
 	"	bic	%0, %0, #0x20\n"

arch/arm/mach-realview/hotplug.c static inline void cpu_enter_lowpower(void) · line 39

Quoted 10 lines
arch/arm/mach-realview/hotplug.c +2 -2
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	"	mrc	p15, 0, %0, c1, c0, 0\n"
-	"	bic	%0, %0, #0x04\n"
+	"	bic	%0, %0, %2\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	  : "=&r" (v)
-	  : "r" (0)
+	  : "r" (0), "Ir" (CR_C)
 	  : "cc");
 }

This looks fine as you have replaced 0x04 with CR_C, which itself is (1 << 2).

Quoted 6 lines
arch/arm/mach-realview/hotplug.c +0 -1
@@ -43,13 +43,13 @@ static inline void cpu_leave_lowpower(void)
 	unsigned int v;
 
 	asm volatile(	"mrc	p15, 0, %0, c1, c0, 0\n"
-	"	orr	%0, %0, #0x04\n"

arch/arm/mach-realview/hotplug.c static inline void cpu_leave_lowpower(void) · line 54

Quoted 10 lines
arch/arm/mach-realview/hotplug.c +2 -1
+	"	orr	%0, %0, %1\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
 	"	orr	%0, %0, #0x20\n"
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	  : "=&r" (v)
-	  :
+	  : "Ir" (CR_C)
 	  : "cc");
 }

fine here also.

arch/arm/mach-s5pv310/hotplug.c static inline void cpu_enter_lowpower(void) · line 33

Quoted 11 lines
arch/arm/mach-s5pv310/hotplug.c +1 -1
index 951ba6d..afa5392 100644
@@ -30,13 +30,13 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, #0x20\n"
+	"	bic	%0, %0, %2\n"

but why replace 0x20 with CR_C instead of CR_D

Quoted 31 lines
arch/arm/mach-s5pv310/hotplug.c +3 -3
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	"	mrc	p15, 0, %0, c1, c0, 0\n"
 	"	bic	%0, %0, #0x04\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	  : "=&r" (v)
-	  : "r" (0)
+	  : "r" (0), "Ir" (CR_C)
 	  : "cc");
 }
 

@@ -46,13 +46,13 @@ static inline void cpu_leave_lowpower(void)
 
 	asm volatile(
 	"mrc	p15, 0, %0, c1, c0, 0\n"
-	"	orr	%0, %0, #0x04\n"
+	"	orr	%0, %0, %1\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
 	"	orr	%0, %0, #0x20\n"
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	  : "=&r" (v)
-	  :
+	  : "Ir" (CR_C)
 	  : "cc");
 }
arch/arm/mach-tegra/hotplug.c +0 -0
index 17faf77..a5cb1ce 100644

arch/arm/mach-tegra/hotplug.c static inline void cpu_enter_lowpower(void) · line 29

Quoted 5 lines
arch/arm/mach-tegra/hotplug.c +1 -1
@@ -26,13 +26,13 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, #0x20\n"
+	"	bic	%0, %0, %2\n"

here also.

Was this code wrong earlier??

Signature · 1 line
viresh
#2 lore raw
Quoted 4 lines · viresh kumar
Quoted 4 lines
arch/arm/mach-s5pv310/hotplug.c +0 -0
index 951ba6d..afa5392 100644
Quoted 8 lines

arch/arm/mach-s5pv310/hotplug.c static inline void cpu_enter_lowpower(void) · line 33

Quoted 6 lines
arch/arm/mach-s5pv310/hotplug.c +1 -1
@@ -30,13 +30,13 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, #0x20\n"
+	"	bic	%0, %0, %2\n"

but why replace 0x20 with CR_C instead of CR_D

Looks like a mistake - it should've been the one below.

Quoted 8 lines

arch/arm/mach-s5pv310/hotplug.c static inline void cpu_enter_lowpower(void) · line 40

Quoted 8 lines
arch/arm/mach-s5pv310/hotplug.c +1 -1
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	"	mrc	p15, 0, %0, c1, c0, 0\n"
 	"	bic	%0, %0, #0x04\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	  : "=&r" (v)
-	  : "r" (0)
+	  : "r" (0), "Ir" (CR_C)
 	  : "cc");

In any case, this code is probably doing the wrong thing - if s5pv310 is not an ARMv6 MPCore the its fiddling with the auxillary control register is already broken.

Quoted 4 lines
Quoted 4 lines
arch/arm/mach-tegra/hotplug.c +0 -0
index 17faf77..a5cb1ce 100644
Quoted 8 lines

arch/arm/mach-tegra/hotplug.c static inline void cpu_enter_lowpower(void) · line 29

Quoted 6 lines
arch/arm/mach-tegra/hotplug.c +1 -1
@@ -26,13 +26,13 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, #0x20\n"
+	"	bic	%0, %0, %2\n"

here also.

Same comment.

#3 lore raw

Here's the fix which restores the old (but probably still broken) behaviour:

Subject: [PATCH] ARM: fix wrongly patched constants

e3d9c625 (ARM: CPU hotplug: fix hard-coded control register constants) changed the wrong constants in the hotplug assembly code. Fix this.

Reported-by viresh kumar <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 arch/arm/mach-s5pv310/hotplug.c |    4 ++--
 arch/arm/mach-tegra/hotplug.c   |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm/mach-s5pv310/hotplug.c +2 -2
index afa5392..c24235c 100644
@@ -30,10 +30,10 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, %2\n"
+	"	bic	%0, %0, #0x20\n"
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	"	mrc	p15, 0, %0, c1, c0, 0\n"
-	"	bic	%0, %0, #0x04\n"
+	"	bic	%0, %0, %2\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	  : "=&r" (v)
 	  : "r" (0), "Ir" (CR_C)
arch/arm/mach-tegra/hotplug.c +2 -2
index a5cb1ce..f329404 100644
@@ -26,10 +26,10 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, %2\n"
+	"	bic	%0, %0, #0x20\n"
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	"	mrc	p15, 0, %0, c1, c0, 0\n"
-	"	bic	%0, %0, #0x04\n"
+	"	bic	%0, %0, %2\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	  : "=&r" (v)
 	  : "r" (0), "Ir" (CR_C)
Signature · 1 line
1.6.2.5
Acked-by
#4 lore raw
Quoted 37 lines · Russell King - ARM Linux

Subject: [PATCH] ARM: fix wrongly patched constants

e3d9c625 (ARM: CPU hotplug: fix hard-coded control register constants) changed the wrong constants in the hotplug assembly code. Fix this.

Reported-by viresh kumar <[email protected]>
Signed-off-by Russell King <[email protected]>
---
 arch/arm/mach-s5pv310/hotplug.c |    4 ++--
 arch/arm/mach-tegra/hotplug.c   |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)
arch/arm/mach-s5pv310/hotplug.c +2 -2
index afa5392..c24235c 100644
@@ -30,10 +30,10 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, %2\n"
+	"	bic	%0, %0, #0x20\n"
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	"	mrc	p15, 0, %0, c1, c0, 0\n"
-	"	bic	%0, %0, #0x04\n"
+	"	bic	%0, %0, %2\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	  : "=&r" (v)
 	  : "r" (0), "Ir" (CR_C)
arch/arm/mach-tegra/hotplug.c +0 -0
index a5cb1ce..f329404 100644
@@ -26,10 +26,10 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */

arch/arm/mach-tegra/hotplug.c static inline void cpu_enter_lowpower(void) · line 35

Quoted 10 lines
arch/arm/mach-tegra/hotplug.c +2 -2
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, %2\n"
+	"	bic	%0, %0, #0x20\n"
 	"	mcr	p15, 0, %0, c1, c0, 1\n"
 	"	mrc	p15, 0, %0, c1, c0, 0\n"
-	"	bic	%0, %0, #0x04\n"
+	"	bic	%0, %0, %2\n"
 	"	mcr	p15, 0, %0, c1, c0, 0\n"
 	  : "=&r" (v)
 	  : "r" (0), "Ir" (CR_C)
Acked-by Viresh Kumar <[email protected]>
#5 lore raw
In reply to Russell King - ARM Linux
Quoted 21 lines · Russell King

Sent: Friday, January 14, 2011 5:05 AM

On Fri, Jan 14, 2011 at 05:20:03PM +0530, viresh kumar wrote:
...
In any case, this code is probably doing the wrong thing - if s5pv310 is not an ARMv6 MPCore the its fiddling with the auxillary control register is already broken.

Quoted 4 lines
Quoted 4 lines
arch/arm/mach-tegra/hotplug.c +0 -0
index 17faf77..a5cb1ce 100644
Quoted 8 lines

arch/arm/mach-tegra/hotplug.c static inline void cpu_enter_lowpower(void) · line 29

Quoted 6 lines
arch/arm/mach-tegra/hotplug.c +1 -1
@@ -26,13 +26,13 @@ static inline void cpu_enter_lowpower(void)
 	 * Turn off coherency
 	 */
 	"	mrc	p15, 0, %0, c1, c0, 1\n"
-	"	bic	%0, %0, #0x20\n"
+	"	bic	%0, %0, %2\n"

here also.

Quoted 1 line

Same comment.

Just as an FYI, in various downstream trees, that file (for mach-tegra) has been removed and replaced with completely different code. However, the replacement isn't in the process of being upstreamed right yet. Specifically:

git://android.git.kernel.org/kernel/tegra.git

linux-tegra-2.6.37

cb8e70bd4fa41ea7ac66f01f7425943db99ac7d3
ARM: tegra: Add suspend and hotplug support

Hope this helps.

Signature · 1 line
nvpublic