On Wed, May 13, 2015 at 4:38 PM, Scott Wood scottwood@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