[U-Boot] [PATCH V6 3/5] nand spl: add NAND Library to new SPL

[Simon Schwarz <simonschwarzcor@googlemail.com>]

Hi Aneesh,

On 07/28/2011 01:54 PM, Aneesh V wrote:
> On Thursday 28 July 2011 02:08 PM, Simon Schwarz wrote:
>> Insert some NAND driver sources into NAND SPL library.
>>
>> Signed-off-by: Simon Schwarz<simonschwarzcor@gmail.com>
>
> [snip ..]
>
>> +
>> +int nand_curr_device = -1;
>
> Is nand_curr_device used anywhere?

Was used in nand.c - this isn't included anymore -> deleted

>
>> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS;
>> +static nand_info_t info;
>> +nand_info_t nand_info[CONFIG_SYS_MAX_NAND_DEVICE];
>
> Is nand_info used anywhere?

Same as above -> deleted.

>
>> +static struct nand_chip nand_chip;
>
> Is nand_chip used anywhere? I see that this definition is shadowed in
> function nand_init().

Deleted the double definition.

nand_chip is used in:
- nand_command
- nand_is_bad_block
- nand_read_page
- nand_init
- nand_deselect

>
>> +
>> +#if (CONFIG_SYS_NAND_PAGE_SIZE<= 512)
>
> [snip ..]
>
>> +/*
>> + * omap_spl_read_buf16 - [DEFAULT] read chip data into buffer
>> + * @mtd: MTD device structure
>> + * @buf: buffer to store date
>
> typo: date instead of data.
>

see below for solution (btw. this typo comes from nand_base.c)

>> + * @len: number of bytes to read
>> + *
>> + * Default read function for 16bit buswith
>> + *
>> + * This function is based on nand_read_buf16 from nand_base.c. This
>> version
>> + * reads 32bit not 16bit although the bus only has 16bit.
>> + */
>> +static void omap_spl_read_buf16(struct mtd_info *mtd, uint8_t *buf,
>> int len)
>> +{
>> + int i;
>> + struct nand_chip *chip = mtd->priv;
>> + u32 *p = (u32 *) buf;
>
> Why this variable p?
It is used to cast the 8-bit buffer variable into a 32bit one. Actually 
the same is done for the 16bit implementation. (There it is the adaption 
to the bus width - why 32bit here see below)
>
>> + len>>= 2;
>> +
>> + for (i = 0; i< len; i++)
>> + p[i] = readl(chip->IO_ADDR_R);
>> +}
>
> Should this function be called omap_spl_read_buf32() ?
> Or still better, should this be added as nand_read_buf32() in
> nand_base.c itself?

Oh. There I played around with the Access Size Adaptation of the GPMC - 
It is still a x16 interface - this is what the 16 refers to IMHO. But 
for sake of simplicity I will change this back to 16bit access - I don't 
think that there is a big performance impact although I didn't measure it.

I cloned them because the functions in nand_base.c are static.

My solution: deleted the cloned functions - use these from nand_base by 
removing the static modifier and add them to nand.h

This leaves nand_base.c inconsistent - some functions are static, some 
not - maybe we should un-static all read/write functions as they are 
normally used in SPL?

>> +
>> +/*
>> + * omap_spl_read_buf - [DEFAULT] read chip data into buffer
>> + * @mtd: MTD device structure
>> + * @buf: buffer to store date
>> + * @len: number of bytes to read
>> + *
>> + * Default read function for 8bit buswith
>> + *
>> + * This is the same function as this from nand_base.c nand_read_buf
>> + */
>> +static void omap_spl_read_buf(struct mtd_info *mtd, uint8_t *buf, int
>> len)
>> +{
>> + int i;
>> + struct nand_chip *chip = mtd->priv;
>> +
>> + for (i = 0; i< len; i++)
>> + buf[i] = readb(chip->IO_ADDR_R);
>> +}
>> +#endif
>
> What is the difference between this function and nand_read_buf() in
> nand_base.c ?
none - static is the problem. Did the same as with the x16 version above.

>
> best regards,
> Aneesh

Regards & thx for the review!
Simon