[U-Boot] [PATCH] powerpc: fix register usage in some inline assembly code

[U-Boot] [PATCH] powerpc: fix register usage in some inline assembly code

[Timur Tabi]

In some usages of inline assembly, hard-coded registers were specified when a
scratch register should have been used instead.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

I can't test the kgdb changes, but the rest appears to be okay.

 arch/powerpc/lib/kgdb.c |   25 +++++++++++++++----------
 arch/powerpc/lib/time.c |    5 ++++-
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/arch/powerpc/lib/kgdb.c b/arch/powerpc/lib/kgdb.c
index 1ec6818..19a56db 100644
--- a/arch/powerpc/lib/kgdb.c
+++ b/arch/powerpc/lib/kgdb.c
@@ -12,11 +12,13 @@ void breakinst(void);
 int
 kgdb_setjmp(long *buf)
 {
-	asm ("mflr 0; stw 0,0(%0);"
-	     "stw 1,4(%0); stw 2,8(%0);"
-	     "mfcr 0; stw 0,12(%0);"
-	     "stmw 13,16(%0)"
-	     : : "r" (buf));
+	unsigned long temp;
+
+	asm volatile("mflr %0; stw %0,0(%1);"
+	     "stw %%r1,4(%1); stw %%r2,8(%1);"
+	     "mfcr %0; stw %0,12(%1);"
+	     "stmw %%r13,16(%1)"
+	     : "=&r"(temp) : "r" (buf));
 	/* XXX should save fp regs as well */
 	return 0;
 }
@@ -24,13 +26,16 @@ kgdb_setjmp(long *buf)
 void
 kgdb_longjmp(long *buf, int val)
 {
+	unsigned long temp;
+
 	if (val == 0)
 		val = 1;
-	asm ("lmw 13,16(%0);"
-	     "lwz 0,12(%0); mtcrf 0x38,0;"
-	     "lwz 0,0(%0); lwz 1,4(%0); lwz 2,8(%0);"
-	     "mtlr 0; mr 3,%1"
-	     : : "r" (buf), "r" (val));
+
+	asm volatile("lmw %%r13,16(%1);"
+	     "lwz %0,12(%1); mtcrf 0x38,%0;"
+	     "lwz %0,0(%1); lwz %%r1,4(%1); lwz %%r2,8(%1);"
+	     "mtlr %0; mr %%r3,%2"
+	     : "=&r"(temp) : "r" (buf), "r" (val));
 }
 
 static inline unsigned long
diff --git a/arch/powerpc/lib/time.c b/arch/powerpc/lib/time.c
index 2909961..34633c3 100644
--- a/arch/powerpc/lib/time.c
+++ b/arch/powerpc/lib/time.c
@@ -78,6 +78,8 @@ unsigned long ticks2usec(unsigned long ticks)
 
 int init_timebase (void)
 {
+	unsigned long temp;
+
 #if defined(CONFIG_5xx) || defined(CONFIG_8xx)
 	volatile immap_t *immap = (immap_t *) CONFIG_SYS_IMMR;
 
@@ -86,7 +88,8 @@ int init_timebase (void)
 #endif
 
 	/* reset */
-	asm ("li 3,0 ; mttbu 3 ; mttbl 3 ;");
+	asm volatile("li %0,0 ; mttbu %0 ; mttbl %0;"
+	     : "=&r"(temp) );
 
 #if defined(CONFIG_5xx) || defined(CONFIG_8xx)
 	/* enable */
-- 
1.7.2.3

[U-Boot] [PATCH] powerpc: fix register usage in some inline assembly code

[Wolfgang Denk]

Dear Timur Tabi,

In message <1292368731-3819-1-git-send-email-timur@freescale.com> you wrote:
> In some usages of inline assembly, hard-coded registers were specified when a
> scratch register should have been used instead.

Is this just a cosmetic improvement, or are you fixing any real bug?
If so, what exactly is the problem?


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Conceptual integrity in turn dictates that the  design  must  proceed
from  one  mind,  or  from  a  very small number of agreeing resonant
minds.               - Frederick Brooks Jr., "The Mythical Man Month"

[U-Boot] [PATCH] powerpc: fix register usage in some inline assembly code

[Tabi Timur-B04825]

Wolfgang Denk wrote:
> In message<1292368731-3819-1-git-send-email-timur@freescale.com>  you wrote:
>> >  In some usages of inline assembly, hard-coded registers were specified when a
>> >  scratch register should have been used instead.

> Is this just a cosmetic improvement, or are you fixing any real bug?
> If so, what exactly is the problem?

Well, I'm not a expert on inline assembly, so I could be missing something, but IMHO it is fixing a real bug, although the current code does technically work.

The problem is that the code was modifying registers and gcc was unaware of that.  So there is a possibility that gcc could have put something important in those registers.  It appears that the authors of that code knew which registers were unused, and just made sure to use those.

-- 
Timur Tabi
Linux kernel developer

[U-Boot] [PATCH] powerpc: fix register usage in some inline assembly code

[Kumar Gala]

On Dec 15, 2010, at 6:32 AM, Tabi Timur-B04825 wrote:

> Wolfgang Denk wrote:
>> In message<1292368731-3819-1-git-send-email-timur@freescale.com>  you wrote:
>>>> In some usages of inline assembly, hard-coded registers were specified when a
>>>> scratch register should have been used instead.
> 
>> Is this just a cosmetic improvement, or are you fixing any real bug?
>> If so, what exactly is the problem?
> 
> Well, I'm not a expert on inline assembly, so I could be missing something, but IMHO it is fixing a real bug, although the current code does technically work.
> 
> The problem is that the code was modifying registers and gcc was unaware of that.  So there is a possibility that gcc could have put something important in those registers.  It appears that the authors of that code knew which registers were unused, and just made sure to use those.

I doubt we disagree that the change is a good one, just trying to understand if it was because of an actual issue you where seeing or just because of some code review that you made it.

ie, is this needed in v2010.12 or goes into 'next'

- k

[U-Boot] [PATCH] powerpc: fix register usage in some inline assembly code

[Timur Tabi]

Kumar Gala wrote:
> I doubt we disagree that the change is a good one, just trying to understand
> if it was because of an actual issue you where seeing or just because of some
> code review that you made it.

Just a code review.

> 
> ie, is this needed in v2010.12 or goes into 'next'

Definitely 'next'.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH] powerpc: fix register usage in some inline assembly code

[Wolfgang Denk]

Dear Timur Tabi,

In message <1292368731-3819-1-git-send-email-timur@freescale.com> you wrote:
> In some usages of inline assembly, hard-coded registers were specified when a
> scratch register should have been used instead.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> 
> I can't test the kgdb changes, but the rest appears to be okay.
> 
>  arch/powerpc/lib/kgdb.c |   25 +++++++++++++++----------
>  arch/powerpc/lib/time.c |    5 ++++-
>  2 files changed, 19 insertions(+), 11 deletions(-)

Applied to "next", thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Every program has at least one bug and can be shortened by  at  least
one  instruction  --  from  which,  by induction, one can deduce that
every program can be reduced to one instruction which doesn't work.