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

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

[viresh kumar]

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.

> 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 <rmk+kernel@arm.linux.org.uk>
> ---
>  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(-)
> 
> diff --git a/arch/arm/mach-realview/hotplug.c b/arch/arm/mach-realview/hotplug.c
> index b6387cf..a87523d 100644
> --- a/arch/arm/mach-realview/hotplug.c
> +++ b/arch/arm/mach-realview/hotplug.c
> @@ -31,10 +31,10 @@ static inline void cpu_enter_lowpower(void)
>  	"	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)
> +	  : "r" (0), "Ir" (CR_C)
>  	  : "cc");
>  }

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

>  
> @@ -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"
> +	"	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.

>  
> diff --git a/arch/arm/mach-s5pv310/hotplug.c b/arch/arm/mach-s5pv310/hotplug.c
> index 951ba6d..afa5392 100644
> --- a/arch/arm/mach-s5pv310/hotplug.c
> +++ b/arch/arm/mach-s5pv310/hotplug.c
> @@ -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

>  	"	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");
>  }
>  
> diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c
> index 17faf77..a5cb1ce 100644
> --- a/arch/arm/mach-tegra/hotplug.c
> +++ b/arch/arm/mach-tegra/hotplug.c
> @@ -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??

-- 
viresh

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

[Russell King - ARM Linux]

On Fri, Jan 14, 2011 at 05:20:03PM +0530, viresh kumar wrote:
> > diff --git a/arch/arm/mach-s5pv310/hotplug.c b/arch/arm/mach-s5pv310/hotplug.c
> > index 951ba6d..afa5392 100644
> > --- a/arch/arm/mach-s5pv310/hotplug.c
> > +++ b/arch/arm/mach-s5pv310/hotplug.c
> > @@ -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.

> >  	"	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.

> > diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c
> > index 17faf77..a5cb1ce 100644
> > --- a/arch/arm/mach-tegra/hotplug.c
> > +++ b/arch/arm/mach-tegra/hotplug.c
> > @@ -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.

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

[Russell King - ARM Linux]

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 <viresh.kumar@st.com>
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/mach-s5pv310/hotplug.c |    4 ++--
 arch/arm/mach-tegra/hotplug.c   |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-s5pv310/hotplug.c b/arch/arm/mach-s5pv310/hotplug.c
index afa5392..c24235c 100644
--- a/arch/arm/mach-s5pv310/hotplug.c
+++ b/arch/arm/mach-s5pv310/hotplug.c
@@ -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)
diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c
index a5cb1ce..f329404 100644
--- a/arch/arm/mach-tegra/hotplug.c
+++ b/arch/arm/mach-tegra/hotplug.c
@@ -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)
-- 
1.6.2.5

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

[Stephen Warren]

Russell King wrote:
> 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.
> 
> > > diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c
> > > index 17faf77..a5cb1ce 100644
> > > --- a/arch/arm/mach-tegra/hotplug.c
> > > +++ b/arch/arm/mach-tegra/hotplug.c
> > > @@ -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.

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.

-- 
nvpublic

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

[viresh kumar]

On 1/14/11, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 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 <viresh.kumar@st.com>
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  arch/arm/mach-s5pv310/hotplug.c |    4 ++--
>  arch/arm/mach-tegra/hotplug.c   |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm/mach-s5pv310/hotplug.c
> b/arch/arm/mach-s5pv310/hotplug.c
> index afa5392..c24235c 100644
> --- a/arch/arm/mach-s5pv310/hotplug.c
> +++ b/arch/arm/mach-s5pv310/hotplug.c
> @@ -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)
> diff --git a/arch/arm/mach-tegra/hotplug.c b/arch/arm/mach-tegra/hotplug.c
> index a5cb1ce..f329404 100644
> --- a/arch/arm/mach-tegra/hotplug.c
> +++ b/arch/arm/mach-tegra/hotplug.c
> @@ -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)

Acked-by: Viresh Kumar <viresh.kumar@st.com>