From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wolfgang Denk <wd@denx.de>
Date: Tue, 28 Sep 2010 19:31:30 +0200
Subject: [U-Boot] [PATCH V2 1/7] Expand POST memory test to support
	arch-depended implementation.
In-Reply-To: <1285691891-32700-1-git-send-email-yorksun@freescale.com>
References: <1285691891-32700-1-git-send-email-yorksun@freescale.com>
Message-ID: <20100928173130.CDDF9D52190@gemini.denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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