[U-Boot] [PATCH] nand: mxs_nand_spl: support use of env in SPL

[Tim Harvey]

On Wed, May 13, 2015 at 4:38 PM, Scott Wood <scottwood at freescale.com> wrote:
> On Fri, 2015-05-08 at 14:39 -0700, Tim Harvey wrote:
>> -int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>> +int mtd_read(struct mtd_info *mtd, loff_t offs, size_t size, size_t *retlen,
>> +          uchar *buf)
>>  {
>>       struct nand_chip *chip;
>>       unsigned int page;
>>       unsigned int nand_page_per_block;
>>       unsigned int sz = 0;
>> +     nand_info_t *info = &nand_info[0];
>
> Any reason not to use the passed-in mtd pointer (and fix the
> nand_spl_load_image wrapper to pass in &nand_info[0])?

Hi Scott,

No reason - I think I just got confused because I didn't realize
nand_info_t was a typedef for struct mtd_info.

>
>> +/* setup mtd and nand structs and init mxs_nand driver */
>> +static int mxs_nand_init(void)
>> +{
>> +     nand_info_t *info = &nand_info[0];
>> +
>> +     /* return if already initalized */
>> +     if (nand_chip.numchips)
>> +             return 0;
>> +
>> +     /* init mxs nand driver */
>> +     board_nand_init(&nand_chip);
>> +     info->priv = &nand_chip;
>> +     /* set mtd functions */
>> +     nand_chip.cmdfunc = mxs_nand_command;
>> +     nand_chip.numchips = 1;
>> +
>> +     /* identify flash device */
>> +     puts("NAND : ");
>> +     if (mxs_flash_ident(info)) {
>> +             printf("Failed to identify\n");
>> +             return -1;
>> +     }
>> +
>> +     /* allocate and initialize buffers */
>> +     nand_chip.buffers = memalign(ARCH_DMA_MINALIGN,
>> +                                  sizeof(*nand_chip.buffers));
>> +     nand_chip.oob_poi = nand_chip.buffers->databuf + info->writesize;
>> +     /* setup flash layout (does not scan as we override that) */
>> +     info->size = nand_chip.chipsize;
>> +     nand_chip.scan_bbt(info);
>> +
>> +     printf("%llu MiB\n", (info->size / (1024 * 1024)));
>> +     return 0;
>> +}
>
> Why did this function need to be moved?
>
>> +int nand_spl_load_image(uint32_t offs, unsigned int size, void *buf)
>> +{
>> +     return mtd_read(NULL, offs, size, NULL, buf);
>> +}
>
> It'd be nice to keep the wrapper near the function it wraps.

It doesn't need to be moved and moving the location of the wrapper as
well makes the patch much easier to read/review.

>
> I don't see any other such wrappers; is there no other driver that
> currently works with SPL env?

I don't know - this got me digging into why I needed mtd_read()
implemented all of the sudden once I enabled CONFIG_SPL_ENV_SUPPORT.

It turns out that this is needed because env_nand.c:readenv() calls
nand_read_skip_bad, which uses the mtd layer. For SPL the
functionality of readenv() is pretty much already in
nand_spl_load_image() so I find if do the following (instead of making
any changes to mxs_nand_spl.c) which allows the mtd layers to not be
used I save 4KB in the SPL:

diff --git a/common/env_nand.c b/common/env_nand.c
index cc7e979..97d10a2 100644
--- a/common/env_nand.c
+++ b/common/env_nand.c
@@ -260,6 +260,9 @@ int saveenv(void)
 }
 #endif /* CMD_SAVEENV */

+#if defined(CONFIG_SPL_BUILD)
+#define readenv(offset, buf) nand_spl_load_image(offset, CONFIG_ENV_SIZE, buf)
+#else
 static int readenv(size_t offset, u_char *buf)
 {
        size_t end = offset + CONFIG_ENV_RANGE;
@@ -295,6 +298,7 @@ static int readenv(size_t offset, u_char *buf)

        return 0;
 }
+#endif /* #if defined(CONFIG_SPL_BUILD) */

 #ifdef CONFIG_ENV_OFFSET_OOB
 int get_nand_env_oob(nand_info_t *nand, unsigned long *result)

I don't think I'm loosing any necessary functionality in the SPL by
doing the above and it saves me 4KB in the SPL which is precious
space.

>
>>  int nand_default_bbt(struct mtd_info *mtd)
>>  {
>>       return 0;
>> @@ -223,6 +232,7 @@ int nand_default_bbt(struct mtd_info *mtd)
>>
>>  void nand_init(void)
>>  {
>> +     mxs_nand_init();
>>  }
>
> Do you still need the "return if already initialized" check with this
> change?  How is this change related to the rest?

The check can now be eliminated as I'm moving the init to where it
likely should have been all along instead of in nand_spl_load_image().
In fact, what should be done is to simply rename the static int
mxs_nand_init() to nand_init() instead of calling the static function.

So while I could revise the above patch based on some of the things
you pointed out here it wouldn't be necessary (although I could do it
as a cleanup) with the above patch instead of env_nand.c.

What do you think?

Tim