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

[Aneesh V]

On Thursday 28 July 2011 07:34 PM, Simon Schwarz wrote:
> 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 at 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

Ok. I have to admit that I am not a NAND expert and I do not understand
this code well.

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

No. If it's an OMAP specific optimization, I don't see a reason to
remove it. Looks like that may actually improve performance. However,
you may have to take into account of the alignment of buffer, the size
requested etc. Please have a look at the implementation in drivers/mtd
/nand/davinci_nand.c(although the implementation here seems to be for
8-bit devices, something similar may be possible for 16-bit)

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

I hope there won't be any name-space conflict due to this.

best regards,
Aneesh