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

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

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

index 5845616..4b4b119 100644
@@ -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

Dear Wolfgang Denk,

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.

Clean up and document the code:

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

---
 post/lib_powerpc/multi.c |   27 ++++++++++++---------------
 1 files changed, 12 insertions(+), 15 deletions(-)

index 4b4b119..b8619de 100644
@@ -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

Dear Wolfgang Denk,

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.

Hello Wolfgang,

Looks good. A minor commend below.

Dear Anatolij Gustschin,

...

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

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.

---
 post/lib_powerpc/multi.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

index b8619de..6642ee3 100644
@@ -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

Hello Wolfgang,

Great! Thanks for fixing this bug!

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

Dear Anatolij Gustschin,

Thanks for testing and reporting it!

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

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

Dear Wolfgang Denk,

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