From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Schwarz <simonschwarzcor@googlemail.com>
Date: Fri, 29 Jul 2011 11:12:41 +0200
Subject: [U-Boot] [PATCH V6 3/5] nand spl: add NAND Library to new SPL
In-Reply-To: <20110728141612.1dade0ab@schlenkerla.am.freescale.net>
References: <1311771039-31691-1-git-send-email-simonschwarzcor@gmail.com>	<1311842291-24837-1-git-send-email-simonschwarzcor@gmail.com>	<1311842291-24837-4-git-send-email-simonschwarzcor@gmail.com>
	<20110728141612.1dade0ab@schlenkerla.am.freescale.net>
Message-ID: <4E327989.6070300@gmail.com>
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

On 07/28/2011 09:16 PM, Scott Wood wrote:
> On Thu, 28 Jul 2011 10:38:09 +0200
> Simon Schwarz<simonschwarzcor@googlemail.com>  wrote:
>
>> +CONFIG_SPL_POWER_SUPPORT (drivers/power/libpower.o)
>
> Not sure what this has to do with NAND.

right. This used by devkit8000 - will change the subject to "spl: add 
NAND and POWER library to new SPL"

>> +int nand_curr_device = -1;
>> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS;
>> +static nand_info_t info;
>> +nand_info_t nand_info[CONFIG_SYS_MAX_NAND_DEVICE];
>> +static struct nand_chip nand_chip;
>
> It doesn't look like nand_info or nand_curr_device are used.

Already deleted.

>
>> +/* nand_boot()-function from the old nand_spl ripped apart into
>> + * - nand_init()
>> + * - nand_spl_load_image()
>> + * - nand_deselect()
>> + */
>
> References to what the code used to look like should go in the commit
> message -- they're not relevant to someone reading this code years from
> now.
Changed.
>
> Splitting this up is going to add bytes -- is it really needed?
>
Yes it is. Since nand_boot did everything in the old spl. Now we have to 
use the functions from the outside.

The payload also can differ much more - u-boot image + environment. 
linux image with FDT image etc. It is also necessary for using 
parse_header by Aneesh.

>> +void nand_init(void)
>> +{
>> +	struct nand_chip nand_chip;
>
> This is shadowing the file-scope nand_chip.
Already deleted
>
>> +	/*
>> +	 * Init board specific nand support
>> +	 */
>> +	nand_chip.select_chip = NULL;
>> +	info.priv =&nand_chip;
>> +	nand_chip.IO_ADDR_R = nand_chip.IO_ADDR_W =
>> +		(void  __iomem *)CONFIG_SYS_NAND_BASE;
>> +	nand_chip.dev_ready = NULL;	/* preset to NULL */
>> +	nand_chip.options = 0;
>
> Once you switch to the BSS nand_cihp, you can get rid of the
> zero init.

do you mean when bss_init is done? If yes I will delete these.

>
>> +/* Copy image from NAND to RAM
>> + * offs: Offset in NAND flash
>> + * size: size of image
>> + * dst: destination pointer to RAM
>> + */
>> +void nand_spl_load_image(loff_t offs, unsigned int size, uchar *dst)
>> +{
>> +	nand_load(&info, offs, size, dst);
>> +}
>
> Just strip the mtd_info parameter from nand_load -- no need for a wrapper.

Since this was criticised by many -  I will drop the old interface and 
get rid of mtd_info everywhere in nand_spl_simple.c

>
> -Scott
>

Regards & thx for review!
Simon