[U-Boot] [PATCH 02/12] SPL: NAND: add support for mxs nand

[Tim Harvey]

On Fri, May 2, 2014 at 1:56 PM, Scott Wood <scottwood at freescale.com> wrote:

Hi Scott,

> On Mon, 2014-04-28 at 13:17 -0700, Tim Harvey wrote:
>> +static void mxs_nand_command(struct mtd_info *mtd, unsigned int command,
>> +                          int column, int page_addr)
>> +{
>> +     register struct nand_chip *chip = mtd->priv;
>> +     int ctrl = NAND_NCE | NAND_CTRL_CLE | NAND_CTRL_CHANGE;
>> +     u32 timeo, time_start;
>> +
>> +     /* write out the command to the device */
>> +     chip->cmd_ctrl(mtd, command, ctrl);
>> +
>> +     /* Address cycle, when necessary */
>> +     ctrl = NAND_NCE | NAND_CTRL_ALE | NAND_CTRL_CHANGE;
>> +     /* Serially input address */
>> +     if (column != -1) {
>> +             ctrl = NAND_CTRL_ALE | NAND_CTRL_CHANGE;
>
> What happened to NAND_NCE?
>
> It looks like the cmd_ctrl in nand_mxs.c doesn't care about NAND_MCE at
> all, so there's no functional impact, but either use it consistently or
> don't use it at all.
>
>> +             chip->cmd_ctrl(mtd, column, ctrl);
>> +             ctrl &= ~NAND_CTRL_CHANGE;
>> +             chip->cmd_ctrl(mtd, column >> 8, ctrl);
>> +     }
>
> Why not pass the ctrl flags directly to cmd_ctrl as you do in most of
> the rest of the function?  nand_mxs.c doesn't even look at NAND_CTRL_CHANGE.

I'll simplify using only what nand_mxs.c pays attention to (NAND_ALE,
NAND_CLE), remove the ctrl var and repost

>
>> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>> +{
>> +     register struct nand_chip *chip;
>
> Why "register"?

cut-n-paste code from nand_base.c - will remove

>
>> +             if (!(page % nand_page_per_block)) {
>> +                     /*
>> +                      * Yes, new block. See if this block is good. If not,
>> +                      * loop until we find a good block.
>> +                      */
>> +                     while (is_badblock(&mtd, offs, 1)) {
>> +                             page = page + nand_page_per_block;
>> +                             /* Check i we've reached the end of flash. */
>> +                             if (page >= mtd.size >> chip->page_shift)
>> +                                     return -1;
>
> Why -1 rather than a real error value?

probably from the example I used from drivers/mtd/nand/mxc_nand_spl.c.
The calling functions don't appear to do any error checking, but I'll
replace it with -ENOMEM and -ENODEV if mxs_nand_init() fails.

Thanks for the review!

Tim

>
> -Scott
>
>