[U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.

[U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.

[sun york-R58495]

Wolfgang,

Without progress indicator, slowe test on memory takes minutes and it looks like hanging. You probably don't want to run it every time the board boots up.

York Sun
----- Original Message -----
From:"Wolfgang Denk" <wd@denx.de>
To:"York Sun" <yorksun@freescale.com>
Cc:"u-boot at lists.denx.de" <u-boot@lists.denx.de>
Sent:8/29/2010 3:56 AM
Subject:Re: [U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.


Dear York Sun,

In message <1282944356-4020-2-git-send-email-yorksun@freescale.com> you wrote:
> Add progress indicator for slow test. It is useful when the testing
> takes too longer to finish. The indicator is reused from flash
> programming.
> 
> Hwconfig is used to turn on slow test when not enabled by flag.

NAK.

POST is supposed to be an automatic, unmonitored functionality.
Results are suposed to be reported through a mechanism compatible to
Linux' syslog system.

There is no place for progress indicators here.


Please note that your subject "support arch-depended implementation"
and the commit message "Add progress indicator" are seriously out of
sync as well.  Another reason for a NAK.

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
Voodoo Programming: Things programmers do that  they  know  shouldn't
work  but they try anyway, and which sometimes actually work, such as
recompiling everything.                             - Karl Lehenbauer

[U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.

[Bas Mevissen]

On Thu, 02 Sep 2010 09:48:06 +0200, Wolfgang Denk <wd@denx.de> wrote:

> And if we add such progress indicators, then we have
> to make sure it integrates well with existing use.
> 

One could define a global environment variable or command to enable or
disable progress indicators. Just like "hash on" and "hash off" in the
FTP control protocol.

-- 
Bas

[U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.

[Wolfgang Denk]

Dear Scott Wood,

In message <20100830144648.10446b6b@schlenkerla.am.freescale.net> you wrote:
>
> > POST is supposed to be an automatic, unmonitored functionality.
> > Results are suposed to be reported through a mechanism compatible to
> > Linux' syslog system.
> > 
> > There is no place for progress indicators here.
> 
> Why did you insist that he use the POST subsystem if you don't consider
> it appropriate for what he's doing?

But I do consider it appropriate for the purpose; I just don't like
what the patch added.

> Or is it your opinion that it's absolutely useless to have a progress
> indicator on a memory test[1], or that such things have no place
> anywhere in U-Boot?

I do not think that progress indicators are useless. But I consider
this to be a separate set oif changes, that should be split into a
separate commit. And if we add such progress indicators, then we have
to make sure it integrates well with existing use.

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
I have yet to add the ESP-driver to the kernel to read  the  mind  of
the user...                                       - Linus Torvalds in
      <Pine.LNX.3.91.960426110644.24860I-100000@linux.cs.Helsinki.FI>

[U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.

[Scott Wood]

On Sun, 29 Aug 2010 10:56:47 +0200
Wolfgang Denk <wd@denx.de> wrote:

> Dear York Sun,
> 
> In message <1282944356-4020-2-git-send-email-yorksun@freescale.com> you wrote:
> > Add progress indicator for slow test. It is useful when the testing
> > takes too longer to finish. The indicator is reused from flash
> > programming.
> > 
> > Hwconfig is used to turn on slow test when not enabled by flag.
> 
> NAK.
> 
> POST is supposed to be an automatic, unmonitored functionality.
> Results are suposed to be reported through a mechanism compatible to
> Linux' syslog system.
> 
> There is no place for progress indicators here.

Why did you insist that he use the POST subsystem if you don't consider
it appropriate for what he's doing?

Or is it your opinion that it's absolutely useless to have a progress
indicator on a memory test[1], or that such things have no place
anywhere in U-Boot?

-Scott

[1] I don't care how automatic it is, if something is taking more than a few
seconds during boot, I'm going to be "monitoring" it as I sit there
twiddling my thumbs -- and if it takes that long, I'd rather it not be
automatic at all.

[U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.

[Wolfgang Denk]

Dear York Sun,

In message <1282944356-4020-2-git-send-email-yorksun@freescale.com> you wrote:
> Add progress indicator for slow test. It is useful when the testing
> takes too longer to finish. The indicator is reused from flash
> programming.
> 
> Hwconfig is used to turn on slow test when not enabled by flag.

NAK.

POST is supposed to be an automatic, unmonitored functionality.
Results are suposed to be reported through a mechanism compatible to
Linux' syslog system.

There is no place for progress indicators here.


Please note that your subject "support arch-depended implementation"
and the commit message "Add progress indicator" are seriously out of
sync as well.  Another reason for a NAK.

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
Voodoo Programming: Things programmers do that  they  know  shouldn't
work  but they try anyway, and which sometimes actually work, such as
recompiling everything.                             - Karl Lehenbauer

[U-Boot] [PATCH 2/7] Expand POST memory test to support arch-depended implementation.

[York Sun]

Add progress indicator for slow test. It is useful when the testing
takes too longer to finish. The indicator is reused from flash
programming.

Hwconfig is used to turn on slow test when not enabled by flag.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 post/drivers/memory.c |  211 +++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 171 insertions(+), 40 deletions(-)

diff --git a/post/drivers/memory.c b/post/drivers/memory.c
index 0062360..4020fc2 100644
--- a/post/drivers/memory.c
+++ b/post/drivers/memory.c
@@ -22,7 +22,7 @@
  */
 
 #include <common.h>
-
+#include <hwconfig.h>
 /* Memory test
  *
  * General observations:
@@ -172,6 +172,21 @@ DECLARE_GLOBAL_DATA_PTR;
 #warning "Injecting address line errors for testing purposes"
 #endif
 
+#define TOTAL_PROGRESS_DOTS 45
+#define TOTAL_PROGRESS_NUMBERS 9
+#define PROGRESS_DOTS_PER_NUMBER (TOTAL_PROGRESS_DOTS/TOTAL_PROGRESS_NUMBERS)
+#define TEST_SHOW_PROGRESS(scale, dots, digit, dots_sub) \
+{ \
+	dots -= (dots_sub); \
+	if ((scale > 0) && (dots <= 0)) { \
+		if ((digit % PROGRESS_DOTS_PER_NUMBER) == 0) \
+			printf("%d", digit / PROGRESS_DOTS_PER_NUMBER); \
+		else \
+			putc('.'); \
+		digit--; \
+		dots += (scale); \
+	} \
+}
 
 /*
  * This function performs a double word move from the data at
@@ -291,21 +306,34 @@ static int memory_post_addrline(ulong *testaddr, ulong *base, ulong size)
 	return ret;
 }
 
-static int memory_post_test1 (unsigned long start,
+static int memory_post_test1(unsigned long start,
 			      unsigned long size,
-			      unsigned long val)
+			      unsigned long val,
+			      int fast)
 {
 	unsigned long i;
 	ulong *mem = (ulong *) start;
 	ulong readback;
 	int ret = 0;
+	int digit, dots;
+	int scale;
+
+	scale = (int)(((size >> 22) + TOTAL_PROGRESS_DOTS - 1) /
+		TOTAL_PROGRESS_DOTS);
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
 
 	for (i = 0; i < size / sizeof (ulong); i++) {
 		mem[i] = val;
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
 	}
-
+	if (!fast)
+		printf("Filled with 0x%lx\n", val);
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
 	for (i = 0; i < size / sizeof (ulong) && ret == 0; i++) {
 		readback = mem[i];
 		if (readback != val) {
@@ -318,24 +346,45 @@ static int memory_post_test1 (unsigned long start,
 		}
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
+	}
+	if (!fast) {
+		if (ret == 0)
+			puts("Verified OK.\n\n");
+		else
+			puts("\nFailed!\n\n");
 	}
-
 	return ret;
 }
 
-static int memory_post_test2 (unsigned long start, unsigned long size)
+static int memory_post_test2(unsigned long start, unsigned long size, int fast)
 {
 	unsigned long i;
 	ulong *mem = (ulong *) start;
 	ulong readback;
 	int ret = 0;
+	int digit, dots;
+	int scale;
+
+	scale = (int)(((size >> 22) + TOTAL_PROGRESS_DOTS - 1) /
+		TOTAL_PROGRESS_DOTS);
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
 
 	for (i = 0; i < size / sizeof (ulong); i++) {
 		mem[i] = 1 << (i % 32);
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
 	}
 
+	if (!fast)
+		printf("Filled with bit-flip pattern\n");
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
+
 	for (i = 0; i < size / sizeof (ulong) && ret == 0; i++) {
 		readback = mem[i];
 		if (readback != (1 << (i % 32))) {
@@ -348,24 +397,45 @@ static int memory_post_test2 (unsigned long start, unsigned long size)
 		}
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
+	}
+	if (!fast) {
+		if (ret == 0)
+			puts("Verified OK.\n\n");
+		else
+			puts("\nFailed!\n\n");
 	}
-
 	return ret;
 }
 
-static int memory_post_test3 (unsigned long start, unsigned long size)
+static int memory_post_test3(unsigned long start, unsigned long size, int fast)
 {
 	unsigned long i;
 	ulong *mem = (ulong *) start;
 	ulong readback;
 	int ret = 0;
+	int digit, dots;
+	int scale;
+
+	scale = (int)(((size >> 22) + TOTAL_PROGRESS_DOTS - 1) /
+		TOTAL_PROGRESS_DOTS);
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
 
 	for (i = 0; i < size / sizeof (ulong); i++) {
 		mem[i] = i;
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
 	}
 
+	if (!fast)
+		printf("Filled with address pattern\n");
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
+
 	for (i = 0; i < size / sizeof (ulong) && ret == 0; i++) {
 		readback = mem[i];
 		if (readback != i) {
@@ -378,24 +448,45 @@ static int memory_post_test3 (unsigned long start, unsigned long size)
 		}
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
+	}
+	if (!fast) {
+		if (ret == 0)
+			puts("Verified OK.\n\n");
+		else
+			puts("\nFailed!\n\n");
 	}
-
 	return ret;
 }
 
-static int memory_post_test4 (unsigned long start, unsigned long size)
+static int memory_post_test4(unsigned long start, unsigned long size, int fast)
 {
 	unsigned long i;
 	ulong *mem = (ulong *) start;
 	ulong readback;
 	int ret = 0;
+	int digit, dots;
+	int scale;
+
+	scale = (int)(((size >> 22) + TOTAL_PROGRESS_DOTS - 1) /
+		TOTAL_PROGRESS_DOTS);
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
 
 	for (i = 0; i < size / sizeof (ulong); i++) {
 		mem[i] = ~i;
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
 	}
 
+	if (!fast)
+		printf("Filled with address complement pattern\n");
+	digit = TOTAL_PROGRESS_DOTS;
+	dots  = 0;
+
 	for (i = 0; i < size / sizeof (ulong) && ret == 0; i++) {
 		readback = mem[i];
 		if (readback != ~i) {
@@ -408,75 +499,115 @@ static int memory_post_test4 (unsigned long start, unsigned long size)
 		}
 		if (i % 1024 == 0)
 			WATCHDOG_RESET ();
+		if (!fast && (i & 0xFFFFF) == 0)
+			TEST_SHOW_PROGRESS(scale, dots, digit, 1);
+	}
+	if (!fast) {
+		if (ret == 0)
+			puts("Verified OK.\n\n");
+		else
+			puts("\nFailed!\n\n");
 	}
-
 	return ret;
 }
 
-static int memory_post_tests (unsigned long start, unsigned long size)
+static int memory_post_tests(unsigned long start, unsigned long size, int fast)
 {
 	int ret = 0;
 
 	if (ret == 0)
-		ret = memory_post_dataline ((unsigned long long *)start);
+		ret = memory_post_dataline((unsigned long long *)start);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_addrline ((ulong *)start, (ulong *)start, size);
+		ret = memory_post_addrline((ulong *)start, (ulong *)start, size);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_addrline ((ulong *)(start + size - 8),
+		ret = memory_post_addrline((ulong *)(start + size - 8),
 					    (ulong *)start, size);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_test1 (start, size, 0x00000000);
+		ret = memory_post_test1(start, size, 0x00000000, fast);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_test1 (start, size, 0xffffffff);
+		ret = memory_post_test1(start, size, 0xffffffff, fast);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_test1 (start, size, 0x55555555);
+		ret = memory_post_test1(start, size, 0x55555555, fast);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_test1 (start, size, 0xaaaaaaaa);
+		ret = memory_post_test1(start, size, 0xaaaaaaaa, fast);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_test2 (start, size);
+		ret = memory_post_test2(start, size, fast);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_test3 (start, size);
+		ret = memory_post_test3(start, size, fast);
 	WATCHDOG_RESET ();
 	if (ret == 0)
-		ret = memory_post_test4 (start, size);
+		ret = memory_post_test4(start, size, fast);
 	WATCHDOG_RESET ();
 
 	return ret;
 }
 
-int memory_post_test (int flags)
+__attribute__((weak))
+int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
 {
-	int ret = 0;
 	bd_t *bd = gd->bd;
-	unsigned long memsize = (bd->bi_memsize >= 256 << 20 ?
-				 256 << 20 : bd->bi_memsize) - (1 << 20);
-
+	*vstart = CONFIG_SYS_SDRAM_BASE;
+	*size = (bd->bi_memsize >= 256 << 20 ?
+			256 << 20 : bd->bi_memsize) - (1 << 20);
 	/* Limit area to be tested with the board info struct */
-	if (CONFIG_SYS_SDRAM_BASE + memsize > (ulong)bd)
-		memsize = (ulong)bd - CONFIG_SYS_SDRAM_BASE;
+	if ((*vstart) + (*size) > (ulong)bd)
+		*size = (ulong)bd - CONFIG_SYS_SDRAM_BASE;
+	return 0;
+}
+
+__attribute__((weak))
+int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
+{
+	return 1;
+}
 
-	if (flags & POST_SLOWTEST) {
-		ret = memory_post_tests (CONFIG_SYS_SDRAM_BASE, memsize);
-	} else {			/* POST_NORMAL */
+__attribute__((weak))
+int arch_memory_test_cleanup(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
+{
+	return 0;
+}
 
-		unsigned long i;
+__attribute__((weak))
+void arch_memory_failure_handle(void)
+{
+	return;
+}
 
-		for (i = 0; i < (memsize >> 20) && ret == 0; i++) {
-			if (ret == 0)
-				ret = memory_post_tests (i << 20, 0x800);
-			if (ret == 0)
-				ret = memory_post_tests ((i << 20) + 0xff800, 0x800);
+int memory_post_test(int flags)
+{
+	int fast, ret = 0;
+	phys_addr_t phys_offset = 0;
+	u32 memsize, vstart;
+	if (hwconfig("memtest"))
+		fast = !hwconfig_arg_cmp("memtest", "slow");
+	else
+		fast = 1;
+
+	arch_memory_test_prepare(&vstart, &memsize, &phys_offset);
+	do {
+		if ((!fast) || (flags & POST_SLOWTEST)) {
+			ret = memory_post_tests(vstart, memsize, fast);
+		} else {			/* POST_NORMAL */
+			unsigned long i;
+			for (i = 0; i < (memsize >> 20) && ret == 0; i++) {
+				if (ret == 0)
+					ret = memory_post_tests(i << 20, 0x800, fast);
+				if (ret == 0)
+					ret = memory_post_tests((i << 20) + 0xff800, 0x800, fast);
+			}
 		}
-	}
-
+	} while (!ret && !arch_memory_test_advance(&vstart, &memsize, &phys_offset));
+	arch_memory_test_cleanup(&vstart, &memsize, &phys_offset);
+	if (ret)
+		arch_memory_failure_handle();
 	return ret;
 }
 
-- 
1.7.0.4