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

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

[York Sun]

Add weak functions to enable architecture depended preparation, address
advancing, cleaning up and error handling.

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

diff --git a/post/drivers/memory.c b/post/drivers/memory.c
index 0062360..203bc36 100644
--- a/post/drivers/memory.c
+++ b/post/drivers/memory.c
@@ -452,31 +452,60 @@ static int memory_post_tests (unsigned long start, unsigned long size)
 	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;
+}
 
-	if (flags & POST_SLOWTEST) {
-		ret = memory_post_tests (CONFIG_SYS_SDRAM_BASE, memsize);
-	} else {			/* POST_NORMAL */
+__attribute__((weak))
+int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
+{
+	return 1;
+}
 
-		unsigned long i;
+__attribute__((weak))
+int arch_memory_test_cleanup(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
+{
+	return 0;
+}
 
-		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);
-		}
-	}
+__attribute__((weak))
+void arch_memory_failure_handle(void)
+{
+	return;
+}
 
+int memory_post_test(int flags)
+{
+	int ret = 0;
+	phys_addr_t phys_offset = 0;
+	u32 memsize, vstart;
+
+	arch_memory_test_prepare(&vstart, &memsize, &phys_offset);
+	do {
+		if (flags & POST_SLOWTEST) {
+			ret = memory_post_tests(vstart, memsize);
+		} 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);
+				if (ret == 0)
+					ret = memory_post_tests((i << 20) + 0xff800, 0x800);
+			}
+		}
+	} 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

[U-Boot] [PATCH V2 2/7] Adding more control to physical address mapping

[York Sun]

A worker function __setup_ddr_tlbs() is introduced to implement more
control on physical address mapping.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/tlb.c |   16 ++++++++++------
 1 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/tlb.c b/arch/powerpc/cpu/mpc85xx/tlb.c
index f2833a5..019fab7 100644
--- a/arch/powerpc/cpu/mpc85xx/tlb.c
+++ b/arch/powerpc/cpu/mpc85xx/tlb.c
@@ -245,7 +245,8 @@ void init_addr_map(void)
 }
 #endif
 
-unsigned int setup_ddr_tlbs(unsigned int memsize_in_meg)
+unsigned int
+__setup_ddr_tlbs(phys_addr_t p_addr, unsigned int memsize_in_meg)
 {
 	int i;
 	unsigned int tlb_size;
@@ -275,21 +276,24 @@ unsigned int setup_ddr_tlbs(unsigned int memsize_in_meg)
 
 		tlb_size = (camsize - 10) / 2;
 
-		set_tlb(1, ram_tlb_address, ram_tlb_address,
+		set_tlb(1, ram_tlb_address, p_addr,
 			MAS3_SX|MAS3_SW|MAS3_SR, 0,
 			0, ram_tlb_index, tlb_size, 1);
 
 		size -= 1ULL << camsize;
 		memsize -= 1ULL << camsize;
 		ram_tlb_address += 1UL << camsize;
+		p_addr += 1UL << camsize;
 	}
 
 	if (memsize)
 		print_size(memsize, " left unmapped\n");
-
-	/*
-	 * Confirm that the requested amount of memory was mapped.
-	 */
 	return memsize_in_meg;
 }
+
+unsigned int setup_ddr_tlbs(unsigned int memsize_in_meg)
+{
+	return
+		__setup_ddr_tlbs(CONFIG_SYS_DDR_SDRAM_BASE, memsize_in_meg);
+}
 #endif /* !CONFIG_NAND_SPL */
-- 
1.7.0.4

[U-Boot] [PATCH V2 2/7] Adding more control to physical address mapping

[Wolfgang Denk]

Dear York Sun,

In message <1285691891-32700-2-git-send-email-yorksun@freescale.com> you wrote:
> A worker function __setup_ddr_tlbs() is introduced to implement more
> control on physical address mapping.

The __ name prefix has a special, reserved meaning. What is your
rationale for using it here?

> +unsigned int setup_ddr_tlbs(unsigned int memsize_in_meg)
> +{
> +	return
> +		__setup_ddr_tlbs(CONFIG_SYS_DDR_SDRAM_BASE, memsize_in_meg);

Remove line break.

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 first thing we do is kill all the lawyers.
(Shakespeare. II Henry VI, Act IV, scene ii)

[U-Boot] [PATCH V2 3/7] Add memory test feature for mpc85xx POST.

[York Sun]

The memory test is performed after DDR initialization when U-boot stills runs
in flash and cache. Whole memory can be tested. It is mapped 2GB at a time
using a sliding TLB window. After the testing, DDR is remapped with up to 2GB
memory from the lowest address as normal.

If memory test fails, DDR DIMM SPD and DDR controller registers are dumped.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/cpu.c |  197 ++++++++++++++++++++++++++++++++++++++++
 doc/README.fsl-ddr             |   13 +++
 2 files changed, 210 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
index 3f80700..f60fa47 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu.c
@@ -34,6 +34,9 @@
 #include <asm/io.h>
 #include <asm/mmu.h>
 #include <asm/fsl_law.h>
+#include <post.h>
+#include <asm/processor.h>
+#include <asm/fsl_ddr_sdram.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -282,3 +285,197 @@ void mpc85xx_reginfo(void)
 	print_laws();
 	print_lbc_regs();
 }
+
+#if CONFIG_POST & CONFIG_SYS_POST_MEMORY
+
+/* Board-specific functions defined in each board's ddr.c */
+void fsl_ddr_get_spd(generic_spd_eeprom_t *ctrl_dimms_spd,
+	unsigned int ctrl_num);
+void read_tlbcam_entry(int idx, u32 *valid, u32 *tsize, unsigned long *epn,
+		       phys_addr_t *rpn);
+unsigned int __setup_ddr_tlbs(phys_addr_t p_addr, unsigned int memsize_in_meg);
+
+static void dump_spd_ddr_reg(void)
+{
+	int i, j, k, m;
+	u8 *p_8;
+	u32 *p_32;
+	ccsr_ddr_t *ddr[CONFIG_NUM_DDR_CONTROLLERS];
+	generic_spd_eeprom_t
+		spd[CONFIG_NUM_DDR_CONTROLLERS][CONFIG_DIMM_SLOTS_PER_CTLR];
+
+	for (i = 0; i < CONFIG_NUM_DDR_CONTROLLERS; i++)
+		fsl_ddr_get_spd(spd[i], i);
+
+	puts("SPD data of all dimms (zero vaule is omitted)...\n");
+	puts("Byte (hex)  ");
+	k = 1;
+	for (i = 0; i < CONFIG_NUM_DDR_CONTROLLERS; i++) {
+		for (j = 0; j < CONFIG_DIMM_SLOTS_PER_CTLR; j++)
+			printf("Dimm%d ", k++);
+	}
+	puts("\n");
+	for (k = 0; k < sizeof(generic_spd_eeprom_t); k++) {
+		m = 0;
+		printf("%3d (0x%02x)  ", k, k);
+		for (i = 0; i < CONFIG_NUM_DDR_CONTROLLERS; i++) {
+			for (j = 0; j < CONFIG_DIMM_SLOTS_PER_CTLR; j++) {
+				p_8 = (u8 *) &spd[i][j];
+				if (p_8[k]) {
+					printf("0x%02x  ", p_8[k]);
+					m++;
+				} else
+					puts("      ");
+			}
+		}
+		if (m)
+			puts("\n");
+		else
+			puts("\r");
+	}
+
+	for (i = 0; i < CONFIG_NUM_DDR_CONTROLLERS; i++) {
+		switch (i) {
+		case 0:
+			ddr[i] = (void *)CONFIG_SYS_MPC85xx_DDR_ADDR;
+			break;
+#ifdef CONFIG_SYS_MPC85xx_DDR2_ADDR
+		case 1:
+			ddr[i] = (void *)CONFIG_SYS_MPC85xx_DDR2_ADDR;
+			break;
+#endif
+		default:
+			printf("%s unexpected controller number = %u\n",
+				__func__, i);
+			return;
+		}
+	}
+	printf("DDR registers dump for all controllers "
+		"(zero vaule is omitted)...\n");
+	puts("Offset (hex)   ");
+	for (i = 0; i < CONFIG_NUM_DDR_CONTROLLERS; i++)
+		printf("     Base + 0x%04x", (u32)ddr[i] & 0xFFFF);
+	puts("\n");
+	for (k = 0; k < sizeof(ccsr_ddr_t)/4; k++) {
+		m = 0;
+		printf("%6d (0x%04x)", k * 4, k * 4);
+		for (i = 0; i < CONFIG_NUM_DDR_CONTROLLERS; i++) {
+			p_32 = (u32 *) ddr[i];
+			if (p_32[k]) {
+				printf("        0x%08x", p_32[k]);
+				m++;
+			} else
+				puts("                  ");
+		}
+		if (m)
+			puts("\n");
+		else
+			puts("\r");
+	}
+	puts("\n");
+}
+
+static int reset_tlb(phys_addr_t p_addr, u32 size, phys_addr_t *phys_offset)
+{
+	u32 vstart = CONFIG_SYS_DDR_SDRAM_BASE;
+	unsigned long epn;
+	u32 tsize, valid, ptr;
+	phys_addr_t rpn = 0;
+	int ddr_esel;
+
+	ptr = vstart;
+	while (ptr < (vstart + size)) {
+		ddr_esel = find_tlb_idx((void *)ptr, 1);
+		if (ddr_esel != -1) {
+			read_tlbcam_entry(ddr_esel, &valid, &tsize, &epn, &rpn);
+			disable_tlb(ddr_esel);
+		}
+		ptr += TSIZE_TO_BYTES(tsize);
+	}
+	/* Setup new tlb to cover the physical address */
+	__setup_ddr_tlbs(p_addr, size>>20);
+
+	ptr = vstart;
+	ddr_esel = find_tlb_idx((void *)ptr, 1);
+	if (ddr_esel != -1) {
+		read_tlbcam_entry(ddr_esel, &valid, &tsize, &epn, phys_offset);
+	} else {
+		printf("TLB error in function %s\n", __func__);
+		return -1;
+	}
+	return 0;
+}
+
+int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
+{
+	phys_addr_t test_cap, p_addr;
+	phys_size_t p_size = min(gd->ram_size, CONFIG_MAX_MEM_MAPPED);
+#if !defined(CONFIG_PHYS_64BIT) || \
+    !defined(CONFIG_SYS_INIT_RAM_ADDR_PHYS) || \
+	(CONFIG_SYS_INIT_RAM_ADDR_PHYS < 0x100000000ull)
+		test_cap = p_size;
+#else
+		test_cap = gd->ram_size;
+#endif
+	p_addr = (*vstart) + (*size) + (*phys_offset);
+	if (p_addr < test_cap - 1) {
+		p_size = min(test_cap - p_addr, CONFIG_MAX_MEM_MAPPED);
+		if (reset_tlb(p_addr, p_size, phys_offset) == -1)
+			return -1;
+		*vstart = CONFIG_SYS_DDR_SDRAM_BASE;
+		*size = (u32) p_size;
+		printf("Testing 0x%08llx - 0x%08llx\n",
+			(u64)(*vstart) + (*phys_offset),
+			(u64)(*vstart) + (*phys_offset) + (*size) - 1);
+	} else
+		return 1;
+	return 0;
+}
+
+int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
+{
+	phys_size_t p_size = min(gd->ram_size, CONFIG_MAX_MEM_MAPPED);
+	*vstart = CONFIG_SYS_DDR_SDRAM_BASE;
+	*size = (u32) p_size;	/* CONFIG_MAX_MEM_MAPPED < 4G */
+	*phys_offset = 0;
+#if !defined(CONFIG_PHYS_64BIT) || \
+    !defined(CONFIG_SYS_INIT_RAM_ADDR_PHYS) || \
+	(CONFIG_SYS_INIT_RAM_ADDR_PHYS < 0x100000000ull)
+		if (gd->ram_size > CONFIG_MAX_MEM_MAPPED) {
+			puts("Cannot test more than ");
+			print_size(CONFIG_MAX_MEM_MAPPED,
+				" without proper 36BIT support.\n");
+		}
+#endif
+	printf("Testing 0x%08llx - 0x%08llx\n",
+		(u64)(*vstart) + (*phys_offset),
+		(u64)(*vstart) + (*phys_offset) + (*size) - 1);
+	return 0;
+}
+int arch_memory_test_cleanup(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
+{
+	unsigned long epn;
+	u32 tsize, valid, ptr;
+	phys_addr_t rpn = 0;
+	int ddr_esel;
+	/* disable the TLBs for this testing */
+	ptr = *vstart;
+	while (ptr < (*vstart) + (*size)) {
+		ddr_esel = find_tlb_idx((void *)ptr, 1);
+		if (ddr_esel != -1) {
+			read_tlbcam_entry(ddr_esel, &valid, &tsize, &epn, &rpn);
+			disable_tlb(ddr_esel);
+		}
+		ptr += TSIZE_TO_BYTES(tsize);
+	}
+	puts("Remap DDR ");
+	setup_ddr_tlbs(gd->ram_size>>20);
+	puts("\n");
+	return 0;
+}
+
+void arch_memory_failure_handle(void)
+{
+	dump_spd_ddr_reg();
+}
+#endif
diff --git a/doc/README.fsl-ddr b/doc/README.fsl-ddr
index e108a0d..bc67349 100644
--- a/doc/README.fsl-ddr
+++ b/doc/README.fsl-ddr
@@ -78,6 +78,19 @@ If the DDR controller supports address hashing, it can be enabled by hwconfig.
 Syntax is:
 hwconfig=fsl_ddr:addr_hash=true
 
+
+Memory testing options for mpc85xx
+==================================
+1. Memory test can be done one U-boot prompt comes up using mtest, or
+2. Memory test can be done with Power-On-Self-Test function, activated at compile time.
+
+   In order to enable the POST memory test, CONFIG_POST needs to be
+   defined in board configuraiton header file. By default, POST memory test performs
+   a fast test. A slow test can be enabled by changing the flag at compiling time.
+   To test memory bigger than 2GB, 36BIT support is needed. Memory is tested within
+   a 2GB window. TLBs are used to map the virtual 2GB window to physical address
+   so that all physical memory can be tested.
+
 Combination of hwconfig
 =======================
 Hwconfig can be combined with multiple parameters, for example, on a supported
-- 
1.7.0.4

[U-Boot] [PATCH V2 3/7] Add memory test feature for mpc85xx POST.

[Wolfgang Denk]

Dear York Sun,

In message <1285691891-32700-3-git-send-email-yorksun@freescale.com> you wrote:
> The memory test is performed after DDR initialization when U-boot stills runs
> in flash and cache. Whole memory can be tested. It is mapped 2GB at a time
> using a sliding TLB window. After the testing, DDR is remapped with up to 2GB
> memory from the lowest address as normal.
> 
> If memory test fails, DDR DIMM SPD and DDR controller registers are dumped.
> 
> Signed-off-by: York Sun <yorksun@freescale.com>

Please see previous comments about empty lines etc. to make to code
more readable.

Also, please add some comments to explain what you are actually doing.

> +Memory testing options for mpc85xx
> +==================================
> +1. Memory test can be done one U-boot prompt comes up using mtest, or

s/one/once/ ?

> +2. Memory test can be done with Power-On-Self-Test function, activated at compile time.
> +
> +   In order to enable the POST memory test, CONFIG_POST needs to be
> +   defined in board configuraiton header file. By default, POST memory test performs
> +   a fast test. A slow test can be enabled by changing the flag at compiling time.
> +   To test memory bigger than 2GB, 36BIT support is needed. Memory is tested within
> +   a 2GB window. TLBs are used to map the virtual 2GB window to physical address
> +   so that all physical memory can be tested.

Lines too long.


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
A supercomputer is a machine that runs an endless loop in 2 seconds.

[U-Boot] [PATCH V2 4/7] Fix address for POST for 85xx with CPM

[York Sun]

The address used for post_word_load and post_word_store is in the dual port
RAM for processors with CPM.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/commproc.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/commproc.c b/arch/powerpc/cpu/mpc85xx/commproc.c
index f0fd1cb..1671b5e 100644
--- a/arch/powerpc/cpu/mpc85xx/commproc.c
+++ b/arch/powerpc/cpu/mpc85xx/commproc.c
@@ -189,7 +189,7 @@ m8560_cpm_extcbrg(uint brg, uint rate, uint extclk, int pinsel)
 void post_word_store (ulong a)
 {
 	volatile ulong *save_addr =
-		(volatile ulong *)(CONFIG_SYS_IMMR + CPM_POST_WORD_ADDR);
+		(volatile ulong *)(CONFIG_SYS_MPC85xx_CPM_ADDR + CPM_POST_WORD_ADDR);
 
 	*save_addr = a;
 }
@@ -197,7 +197,7 @@ void post_word_store (ulong a)
 ulong post_word_load (void)
 {
 	volatile ulong *save_addr =
-		(volatile ulong *)(CONFIG_SYS_IMMR + CPM_POST_WORD_ADDR);
+		(volatile ulong *)(CONFIG_SYS_MPC85xx_CPM_ADDR + CPM_POST_WORD_ADDR);
 
 	return *save_addr;
 }
-- 
1.7.0.4

[U-Boot] [PATCH V2 5/7] Setup POST word for generic mpc85xx

[York Sun]

Using PIC TFRR register for post word load/store for generic MPC85xx.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 arch/powerpc/cpu/mpc85xx/cpu.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/cpu/mpc85xx/cpu.c b/arch/powerpc/cpu/mpc85xx/cpu.c
index f60fa47..7662cb8 100644
--- a/arch/powerpc/cpu/mpc85xx/cpu.c
+++ b/arch/powerpc/cpu/mpc85xx/cpu.c
@@ -286,6 +286,23 @@ void mpc85xx_reginfo(void)
 	print_lbc_regs();
 }
 
+#ifdef CONFIG_POST
+
+__attribute__((weak))
+void post_word_store(ulong a)
+{
+	void *save_addr = (void *)(CONFIG_SYS_MPC8xxx_PIC_ADDR + offsetof(ccsr_pic_t, tfrr));
+	out_be32(save_addr, a);
+}
+__attribute__((weak))
+ulong post_word_load(void)
+{
+	void *save_addr = (void *)(CONFIG_SYS_MPC8xxx_PIC_ADDR + offsetof(ccsr_pic_t, tfrr));
+	return in_be32(save_addr);
+}
+
+#endif /* CONFIG_POST */
+
 #if CONFIG_POST & CONFIG_SYS_POST_MEMORY
 
 /* Board-specific functions defined in each board's ddr.c */
-- 
1.7.0.4

[U-Boot] [PATCH V2 6/7] Enable POST memory test for corenet_ds

[York Sun]

Signed-off-by: York Sun <yorksun@freescale.com>
---
 include/configs/corenet_ds.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/configs/corenet_ds.h b/include/configs/corenet_ds.h
index cbe5024..5ce0efd 100644
--- a/include/configs/corenet_ds.h
+++ b/include/configs/corenet_ds.h
@@ -86,6 +86,7 @@
 #define CONFIG_SYS_NUM_ADDR_MAP		64	/* number of TLB1 entries */
 #endif
 
+#define CONFIG_POST CONFIG_SYS_POST_MEMORY	/* test POST memory test */
 #define CONFIG_SYS_MEMTEST_START	0x00200000	/* memtest works on */
 #define CONFIG_SYS_MEMTEST_END		0x00400000
 #define CONFIG_SYS_ALT_MEMTEST
-- 
1.7.0.4

[U-Boot] [PATCH V2 7/7] Enable POST memory test for P2020DS

[York Sun]

Signed-off-by: York Sun <yorksun@freescale.com>
---
 include/configs/P2020DS.h |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/configs/P2020DS.h b/include/configs/P2020DS.h
index 79ce2c0..923072a 100644
--- a/include/configs/P2020DS.h
+++ b/include/configs/P2020DS.h
@@ -73,8 +73,9 @@
 #define CONFIG_SYS_NUM_ADDR_MAP		16	/* number of TLB1 entries */
 #endif
 
-#define CONFIG_SYS_MEMTEST_START	0x00000000	/* memtest works on */
-#define CONFIG_SYS_MEMTEST_END		0x7fffffff
+#define CONFIG_POST CONFIG_SYS_POST_MEMORY	/* test POST memory test */
+#define CONFIG_SYS_MEMTEST_START	0x00200000	/* memtest works on */
+#define CONFIG_SYS_MEMTEST_END		0x00400000
 #define CONFIG_PANIC_HANG	/* do not reset board on panic */
 
 /*
-- 
1.7.0.4

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

[Wolfgang Denk]

Dear York Sun,

In message <1285691891-32700-1-git-send-email-yorksun@freescale.com> you wrote:
> Add weak functions to enable architecture depended preparation, address
> advancing, cleaning up and error handling.
> 
> Signed-off-by: York Sun <yorksun@freescale.com>

This needs some comments to explain the interfaces you are providing.

> -int memory_post_test (int flags)
> +__attribute__((weak))
> +int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)

phys_offset is unused here. Drop it?

> -	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;

Blank line after declarations, please.

> +	*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;

Should this eventually be

		*size = (ulong)bd - *vstart;
??

> +__attribute__((weak))
> +int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
> +{
> +	return 1;
> +}

Unused arguments?

Don't you get compiler warnings for these?

> +__attribute__((weak))
> +int arch_memory_test_cleanup(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
> +{
> +	return 0;
> +}

Ditto ?

> +int memory_post_test(int flags)
> +{
> +	int ret = 0;
> +	phys_addr_t phys_offset = 0;
> +	u32 memsize, vstart;
> +
> +	arch_memory_test_prepare(&vstart, &memsize, &phys_offset);
> +	do {

Empty line before the do, please.

> +		if (flags & POST_SLOWTEST) {
> +			ret = memory_post_tests(vstart, memsize);
> +		} 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);
> +				if (ret == 0)
> +					ret = memory_post_tests((i << 20) + 0xff800, 0x800);

Line too long.

> +			}
> +		}
> +	} while (!ret && !arch_memory_test_advance(&vstart, &memsize, &phys_offset));
> +	arch_memory_test_cleanup(&vstart, &memsize, &phys_offset);

Empty line before and after, please.

> +	if (ret)
> +		arch_memory_failure_handle();
>  	return ret;

Empty line before.

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
It is much easier to suggest solutions when you know nothing

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

[York Sun]

On Tue, 2010-09-28 at 19:31 +0200, Wolfgang Denk wrote:

> > -int memory_post_test (int flags)
> > +__attribute__((weak))
> > +int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
> 
> phys_offset is unused here. Drop it?
> 

The phys_offset is not used by _this_ weak function but it is used by
another function implemented in the third patch. Is it OK to leave the
unused phys_offset here?

Regards,

York

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

[Wolfgang Denk]

Dear York Sun,

In message <1285696512.30239.14.camel@oslab-l1> you wrote:
> On Tue, 2010-09-28 at 19:31 +0200, Wolfgang Denk wrote:
> 
> > > -int memory_post_test (int flags)
> > > +__attribute__((weak))
> > > +int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
> > 
> > phys_offset is unused here. Drop it?
> > 
> 
> The phys_offset is not used by _this_ weak function but it is used by
> another function implemented in the third patch. Is it OK to leave the
> unused phys_offset here?

What does the compiler say?

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
He'd heard her use that sweet, innocent  tone  of  voice  before.  It
meant that, pretty soon, there was going to be trouble.
                                        - Terry Pratchett, _Truckers_

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

[York Sun]

On Tue, 2010-09-28 at 20:50 +0200, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1285696512.30239.14.camel@oslab-l1> you wrote:
> > On Tue, 2010-09-28 at 19:31 +0200, Wolfgang Denk wrote:
> > 
> > > > -int memory_post_test (int flags)
> > > > +__attribute__((weak))
> > > > +int arch_memory_test_prepare(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
> > > 
> > > phys_offset is unused here. Drop it?
> > > 
> > 
> > The phys_offset is not used by _this_ weak function but it is used by
> > another function implemented in the third patch. Is it OK to leave the
> > unused phys_offset here?
> 
> What does the compiler say?

I don't have any warning or error.

York

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

[Wolfgang Denk]

Dear York Sun,

In message <1285701869.30239.16.camel@oslab-l1> you wrote:
>
> > > The phys_offset is not used by _this_ weak function but it is used by
> > > another function implemented in the third patch. Is it OK to leave the
> > > unused phys_offset here?
> > 
> > What does the compiler say?
> 
> I don't have any warning or error.

Ok, fine. 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
I am not now, nor have I ever been, a member of the demigodic party.
                                                   -- Dennis Ritchie

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

[Scott Wood]

On Tue, 28 Sep 2010 19:31:30 +0200
Wolfgang Denk <wd@denx.de> wrote:

> > +__attribute__((weak))
> > +int arch_memory_test_advance(u32 *vstart, u32 *size, phys_addr_t *phys_offset)
> > +{
> > +	return 1;
> > +}
> 
> Unused arguments?
> 
> Don't you get compiler warnings for these?

Such warnings are not part of -Wall, which is a good thing, as
the arguments may be there to satisfy an interface (esp. with function
pointers or conditional compilation) and not because this instance of
the function actually needs them.  This seems to be the case here.

-Scott