[U-Boot] CPU POST on MPC5200 fails with U-Boot built using GCC 4.6.1

[U-Boot] CPU POST on MPC5200 fails with U-Boot built using GCC 4.6.1

[Anatolij Gustschin]

Hi,

I noticed a problem while testing Wolfgang's patch

PPC: fix "Warning: FOO uses hard float, BAR uses soft float".

U-Boot built for TQM5200 using ELDK-5.1 powerpc toolchain
seems to have issues in CPU POST test. I see similar problem
both with and also without Wolfgang's patch applied:

U-Boot 2011.12-rc3-00002-gaf44f4b (Dec 23 2011 - 00:08:10)

CPU:   MPC5200 v1.2, Core v1.1 at 396 MHz
       Bus 132 MHz, IPB 132 MHz, PCI 66 MHz
Board: TQM5200 (TQ-Components GmbH)
       on a STK52xx carrier board
I2C:   85 kHz, ready
DRAM:  64 MiB
Flash: 32 MiB
In:    serial
Out:   serial
Err:   serial
Net:   FEC
POST i2c PASSED
POST cpu Error at multi test !
FAILED
^^^^^^

IDE:   Bus 0: OK 
  Device 0: Model: 32MB CHH Firm: Rev 1.01 Ser#: 403303C48A30730031
            Type: Removable Hard Disk
            Capacity: 30.5 MB = 0.0 GB (62592 x 512)
  Device 1: not available
SRAM:  512 kB
VGA:   SMI501 (Voyager) with 8 MB
PS/2:  No device found
Kbd:   reset failed, no ACK

Type run flash_nfs to mount root filesystem over NFS

Anatolij

[U-Boot] [PATCH 1/3] post/lib_powerpc/multi.c: make checkpatch clean

[Wolfgang Denk]

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Kim Phillips <kim.phillips@freescale.com>
Cc: Andy Fleming <afleming@gmail.com>

---
 post/lib_powerpc/multi.c |   54 +++++++++++++++++++++------------------------
 1 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/post/lib_powerpc/multi.c b/post/lib_powerpc/multi.c
index 5845616..4b4b119 100644
--- a/post/lib_powerpc/multi.c
+++ b/post/lib_powerpc/multi.c
@@ -38,45 +38,41 @@
 
 #if CONFIG_POST & CONFIG_SYS_POST_CPU
 
-extern void cpu_post_exec_02 (ulong *code, ulong op1, ulong op2);
+extern void cpu_post_exec_02(ulong * code, ulong op1, ulong op2);
 
-int cpu_post_test_multi (void)
+int cpu_post_test_multi(void)
 {
-    int ret = 0;
-    unsigned int i;
-    int flag = disable_interrupts();
+	int ret = 0;
+	unsigned int i;
+	int flag = disable_interrupts();
 
-    if (ret == 0)
-    {
-	ulong src [26], dst [26];
+	if (ret == 0) {
+		ulong src[26], dst[26];
 
-	ulong code[] =
-	{
-	    ASM_LMW(5, 3, 0),
-	    ASM_STMW(5, 4, 0),
-	    ASM_BLR,
-	};
+		ulong code[] = {
+			ASM_LMW(5, 3, 0),
+			ASM_STMW(5, 4, 0),
+			ASM_BLR,
+		};
 
-	for (i = 0; i < ARRAY_SIZE(src); ++i)
-	{
-	    src[i] = i;
-	    dst[i] = 0;
-	}
+		for (i = 0; i < ARRAY_SIZE(src); ++i) {
+			src[i] = i;
+			dst[i] = 0;
+		}
 
-	cpu_post_exec_02(code, (ulong)src, (ulong)dst);
+		cpu_post_exec_02(code, (ulong) src, (ulong) dst);
 
-	ret = memcmp(src, dst, sizeof(dst)) == 0 ? 0 : -1;
-    }
+		ret = memcmp(src, dst, sizeof(dst)) == 0 ? 0 : -1;
+	}
 
-    if (ret != 0)
-    {
-	post_log ("Error at multi test !\n");
-    }
+	if (ret != 0) {
+		post_log("Error@multi test !\n");
+	}
 
-    if (flag)
-	enable_interrupts();
+	if (flag)
+		enable_interrupts();
 
-    return ret;
+	return ret;
 }
 
 #endif
-- 
1.7.6.4

[U-Boot] [PATCH 2/3] post/lib_powerpc/multi.c: code cleanup

[Wolfgang Denk]

Clean up and document the code:

- get rid of unneeded code block
- add comment which code is generated

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Kim Phillips <kim.phillips@freescale.com>
Cc: Andy Fleming <afleming@gmail.com>
---
 post/lib_powerpc/multi.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/post/lib_powerpc/multi.c b/post/lib_powerpc/multi.c
index 4b4b119..b8619de 100644
--- a/post/lib_powerpc/multi.c
+++ b/post/lib_powerpc/multi.c
@@ -44,26 +44,23 @@ int cpu_post_test_multi(void)
 {
 	int ret = 0;
 	unsigned int i;
+	ulong src[26], dst[26];
 	int flag = disable_interrupts();
 
-	if (ret == 0) {
-		ulong src[26], dst[26];
+	ulong code[] = {
+		ASM_LMW(5, 3, 0),	/* lmw	r5, 0(r3)	*/
+		ASM_STMW(5, 4, 0),	/* stmr	r5, 0(r4)	*/
+		ASM_BLR,		/* blr			*/
+	};
 
-		ulong code[] = {
-			ASM_LMW(5, 3, 0),
-			ASM_STMW(5, 4, 0),
-			ASM_BLR,
-		};
-
-		for (i = 0; i < ARRAY_SIZE(src); ++i) {
-			src[i] = i;
-			dst[i] = 0;
-		}
+	for (i = 0; i < ARRAY_SIZE(src); ++i) {
+		src[i] = i;
+		dst[i] = 0;
+	}
 
-		cpu_post_exec_02(code, (ulong) src, (ulong) dst);
+	cpu_post_exec_02(code, (ulong) src, (ulong) dst);
 
-		ret = memcmp(src, dst, sizeof(dst)) == 0 ? 0 : -1;
-	}
+	ret = memcmp(src, dst, sizeof(dst)) == 0 ? 0 : -1;
 
 	if (ret != 0) {
 		post_log("Error@multi test !\n");
-- 
1.7.6.4

[U-Boot] [PATCH 3/3] post/lib_powerpc/multi.c: fix stack overflow error

[Wolfgang Denk]

The code and comment disagreed: the comment claimed that r6...r31
were copied, and consequently the arrays for "src" and "dst" were
declared with 26 entries, but the actual code ("lmw r5,0(r3)" and
"stmw r5,0(r4)") copied _27_ words (r5 through r31), which resulted
in false "POST cpu Error at multi test" messages.

Fix the comment and the array sizes.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Kim Phillips <kim.phillips@freescale.com>
Cc: Andy Fleming <afleming@gmail.com>
---
 post/lib_powerpc/multi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/post/lib_powerpc/multi.c b/post/lib_powerpc/multi.c
index b8619de..6642ee3 100644
--- a/post/lib_powerpc/multi.c
+++ b/post/lib_powerpc/multi.c
@@ -27,9 +27,9 @@
  * CPU test
  * Load/store multiple word instructions:	lmw, stmw
  *
- * 26 consecutive words are loaded from a source memory buffer
- * into GPRs r6 through r31. After that, 26 consecutive words are stored
- * from the GPRs r6 through r31 into a target memory buffer. The contents
+ * 27 consecutive words are loaded from a source memory buffer
+ * into GPRs r5 through r31. After that, 27 consecutive words are stored
+ * from the GPRs r5 through r31 into a target memory buffer. The contents
  * of the source and target buffers are then compared.
  */
 
@@ -44,7 +44,7 @@ int cpu_post_test_multi(void)
 {
 	int ret = 0;
 	unsigned int i;
-	ulong src[26], dst[26];
+	ulong src[27], dst[27];
 	int flag = disable_interrupts();
 
 	ulong code[] = {
-- 
1.7.6.4

[U-Boot] [PATCH 1/3] post/lib_powerpc/multi.c: make checkpatch clean

[Anatolij Gustschin]

Hello Wolfgang,

Looks good. A minor commend below.

On Fri, 23 Dec 2011 12:29:10 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Kim Phillips <kim.phillips@freescale.com>
> Cc: Andy Fleming <afleming@gmail.com>
> ---
>  post/lib_powerpc/multi.c |   54 +++++++++++++++++++++------------------------
>  1 files changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/post/lib_powerpc/multi.c b/post/lib_powerpc/multi.c
> index 5845616..4b4b119 100644
> --- a/post/lib_powerpc/multi.c
> +++ b/post/lib_powerpc/multi.c
> @@ -38,45 +38,41 @@
>  
>  #if CONFIG_POST & CONFIG_SYS_POST_CPU
>  
> -extern void cpu_post_exec_02 (ulong *code, ulong op1, ulong op2);
> +extern void cpu_post_exec_02(ulong * code, ulong op1, ulong op2);

IIRC checkpatch complains like "foo * bar" should be "foo *bar",
There is surely no need to resubmit the patch, it could be changed
when applying. Otherwise

Acked-by: Anatolij Gustschin <agust@denx.de>

Thanks,
Anatolij

[U-Boot] [PATCH 2/3] post/lib_powerpc/multi.c: code cleanup

[Anatolij Gustschin]

On Fri, 23 Dec 2011 12:29:11 +0100
Wolfgang Denk <wd@denx.de> wrote:

> Clean up and document the code:
> 
> - get rid of unneeded code block
> - add comment which code is generated
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Kim Phillips <kim.phillips@freescale.com>
> Cc: Andy Fleming <afleming@gmail.com>

Acked-by: Anatolij Gustschin <agust@denx.de>

[U-Boot] [PATCH 3/3] post/lib_powerpc/multi.c: fix stack overflow error

[Anatolij Gustschin]

Hello Wolfgang,

On Fri, 23 Dec 2011 12:29:12 +0100
Wolfgang Denk <wd@denx.de> wrote:

> The code and comment disagreed: the comment claimed that r6...r31
> were copied, and consequently the arrays for "src" and "dst" were
> declared with 26 entries, but the actual code ("lmw r5,0(r3)" and
> "stmw r5,0(r4)") copied _27_ words (r5 through r31), which resulted
> in false "POST cpu Error at multi test" messages.

Great! Thanks for fixing this bug!

Acked-by: Anatolij Gustschin <agust@denx.de>
Tested-by: Anatolij Gustschin <agust@denx.de>

But I wonder why didn't we see it with U-Boot built using older
GCC versions.

Since only 26 words will be compared after the test, the issue
only shows up if the destination buffer is placed at lower
addresses on the stack than the source buffer. In this case the
first word in the source buffer is overwritten. GCC 4.6.1 generated
code which changed the order of src[] and dst[] on the stack and
the hidden bug showed up.

Here is a partial dump of each buffer and additionally a
dump of the following word. The buffer address is in
parenthesis:

with GCC 4.2.2:

00: src(03e51c74) 0x00000000, dst(03e51cdc) 0x00000000
01: src(03e51c78) 0x00000001, dst(03e51ce0) 0x00000000
...
25: src(03e51cd8) 0x00000019, dst(03e51d40) 0x00000000
26: src(03e51cdc) 0x00000000, dst(03e51d44) 0x00000000

Test result:

00: src(03e51c74) 0x00000000, dst(03e51cdc) 0x00000000
01: src(03e51c78) 0x00000001, dst(03e51ce0) 0x00000001
...
25: src(03e51cd8) 0x00000019, dst(03e51d40) 0x00000019
26: src(03e51cdc) 0x00000000, dst(03e51d44) 0x00000000


with GCC 4.6.1:

00: src(03e57cf4) 0x00000000, dst(03e57c8c) 0x00000000
01: src(03e57cf8) 0x00000001, dst(03e57c90) 0x00000000
...
25: src(03e57d58) 0x00000019, dst(03e57cf0) 0x00000000
26: src(03e57d5c) 0x03f9c3c0, dst(03e57cf4) 0x00000000

Test result:
Error at multi test !
00: src(03e57cf4) 0x03f9c3c0, dst(03e57c8c) 0x00000000
01: src(03e57cf8) 0x00000001, dst(03e57c90) 0x00000001
...
25: src(03e57d58) 0x00000019, dst(03e57cf0) 0x00000019
26: src(03e57d5c) 0x03f9c3c0, dst(03e57cf4) 0x03f9c3c0

Thanks,
Anatolij

[U-Boot] [PATCH 1/3] post/lib_powerpc/multi.c: make checkpatch clean

[Wolfgang Denk]

Dear Wolfgang Denk,

In message <1324639752-26856-1-git-send-email-wd@denx.de> you wrote:
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Kim Phillips <kim.phillips@freescale.com>
> Cc: Andy Fleming <afleming@gmail.com>
> 
> ---
>  post/lib_powerpc/multi.c |   54 +++++++++++++++++++++------------------------
>  1 files changed, 25 insertions(+), 29 deletions(-)

Applied, 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
The typical page layout program is nothing more  than  an  electronic
light table for cutting and pasting documents.

[U-Boot] [PATCH 1/3] post/lib_powerpc/multi.c: make checkpatch clean

[Wolfgang Denk]

Dear Anatolij Gustschin,

In message <20111223133952.5e09a8b4@wker> you wrote:
> 
> Looks good. A minor commend below.
...
> > -extern void cpu_post_exec_02 (ulong *code, ulong op1, ulong op2);
> > +extern void cpu_post_exec_02(ulong * code, ulong op1, ulong op2);
> 
> IIRC checkpatch complains like "foo * bar" should be "foo *bar",
> There is surely no need to resubmit the patch, it could be changed
> when applying. Otherwise

You are right.  Fixed this.  Thanks for the review!

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
"Science makes godlike -- it is all over with priests and  gods  when
man becomes scientific. Moral: science is the forbidden as such -- it
alone  is  forbidden. Science is the *first* sin, the *original* sin.
*This alone is  morality.*  ``Thou  shalt  not  know''  --  the  rest
follows."                                       - Friedrich Nietzsche

[U-Boot] [PATCH 2/3] post/lib_powerpc/multi.c: code cleanup

[Wolfgang Denk]

Dear Wolfgang Denk,

In message <1324639752-26856-2-git-send-email-wd@denx.de> you wrote:
> Clean up and document the code:
> 
> - get rid of unneeded code block
> - add comment which code is generated
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Kim Phillips <kim.phillips@freescale.com>
> Cc: Andy Fleming <afleming@gmail.com>
> ---
>  post/lib_powerpc/multi.c |   27 ++++++++++++---------------
>  1 files changed, 12 insertions(+), 15 deletions(-)

Applied, 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
Q: What do you get when you cross an ethernet with an income statement?
A: A local area networth.

[U-Boot] [PATCH 3/3] post/lib_powerpc/multi.c: fix stack overflow error

[Wolfgang Denk]

Dear Wolfgang Denk,

In message <1324639752-26856-3-git-send-email-wd@denx.de> you wrote:
> The code and comment disagreed: the comment claimed that r6...r31
> were copied, and consequently the arrays for "src" and "dst" were
> declared with 26 entries, but the actual code ("lmw r5,0(r3)" and
> "stmw r5,0(r4)") copied _27_ words (r5 through r31), which resulted
> in false "POST cpu Error at multi test" messages.
> 
> Fix the comment and the array sizes.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: Kim Phillips <kim.phillips@freescale.com>
> Cc: Andy Fleming <afleming@gmail.com>
> ---
>  post/lib_powerpc/multi.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)

Applied, 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
"Love is an ideal thing, marriage a real thing; a  confusion  of  the
real with the ideal never goes unpunished."                  - Goethe

[U-Boot] [PATCH 3/3] post/lib_powerpc/multi.c: fix stack overflow error

[Wolfgang Denk]

Dear Anatolij Gustschin,

In message <20111223170640.18cfe56f@wker> you wrote:
> 
> > The code and comment disagreed: the comment claimed that r6...r31
> > were copied, and consequently the arrays for "src" and "dst" were
> > declared with 26 entries, but the actual code ("lmw r5,0(r3)" and
> > "stmw r5,0(r4)") copied _27_ words (r5 through r31), which resulted
> > in false "POST cpu Error at multi test" messages.
> 
> Great! Thanks for fixing this bug!

Thanks for testing and reporting it!

> But I wonder why didn't we see it with U-Boot built using older
> GCC versions.

Yes, I was surprised,too, and suspected a compiler problem instead...

> Since only 26 words will be compared after the test, the issue
> only shows up if the destination buffer is placed at lower
> addresses on the stack than the source buffer. In this case the
> first word in the source buffer is overwritten. GCC 4.6.1 generated
> code which changed the order of src[] and dst[] on the stack and
> the hidden bug showed up.

This matches my own analysis.  Actually the code generated by gcc
4.5.1 and 4.6.1 looks _really_ different in a lot of places; it seems
a lot has been changed in GCC again.

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
Few people do business well who do nothing else.
                                       -- Philip Earl of Chesterfield