Re: [U-Boot] [PATCH V4 2/5] omap-common: add nand spl support
Dear Scott Wood,
On 07/27/2011 11:38 PM, Scott Wood wrote:
On Wed, 27 Jul 2011 10:42:22 +0200 Simon Schwarzsimonschwarzcor@googlemail.com wrote:
Dear Scott Wood,
On 07/26/2011 08:06 PM, Scott Wood wrote:
On Tue, 26 Jul 2011 14:09:15 +0200 Simon Schwarzsimonschwarzcor@googlemail.com wrote:
+#ifdef CONFIG_SPL_NAND_SUPPORT +static void nand_load_image(void) +{
- gpmc_init();
- nand_init();
- nand_copy_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
CONFIG_SYS_NAND_U_BOOT_SIZE,(uchar *)CONFIG_SYS_NAND_U_BOOT_DST);+#ifdef CONFIG_NAND_ENV_DST
- nand_copy_image(CONFIG_ENV_OFFSET, CONFIG_ENV_SIZE,
(uchar *)CONFIG_NAND_ENV_DST);+#ifdef CONFIG_ENV_OFFSET_REDUND
- nand_copy_image(CONFIG_ENV_OFFSET_REDUND, CONFIG_ENV_SIZE,
(uchar *)CONFIG_NAND_ENV_DST + CONFIG_ENV_SIZE);+#endif +#endif
- parse_image_header((struct image_header *)CONFIG_SYS_NAND_U_BOOT_DST);
+} +#endif /* CONFIG_SPL_NAND_SUPPORT */
I'm not sure that "load" versus "copy" conveys the difference between this function and the low-level nand_copy_image.
The actual difference is that nand_load has an mtd_info struct as additional paramter.
Hmm? nand_load_image() takes no arguments. I don't see a nand_load().
The actual difference is that one is a low-level "move this from here to there" function, and the other is driving hardware init and then performing a series of calls to the low-level function, supplying the information about what is to be loaded where.
We have different code - sorry for the confusion. see below.
The device to use is selected in nand_init and I don't see a reason why this should be passed around in the interface - in the spl all data is typically loaded from one chip - this also was the implementation before.
Sure.
Where is nand_copy_image() defined?
It's in drivers/mtd/nand/nand_spl.c
Where is drivers/mtd/nand/nand_spl.c? It's not in Wolfgang's current tree, nor in u-boot-ti, and I didn't see it in these patches. Did you forget to "git add"?
arrghh. You are right. I forgot a git add. V6 will change this...
Note that there will not be one implementation of nand_copy_image suitable for all hardware, just as currently nand_spl/nand_boot.c is not used for all NAND SPL targets.
Hm. I know that. I just adapated the old nand_boot.c.
AFAIK the other implementations use prefixes for the function names - therefore we can just add them to the nand-spl-library and gcc will do the rest.
Regards & thx for the review! Simon
On Thu, 28 Jul 2011 09:51:01 +0200 Simon Schwarz simonschwarzcor@googlemail.com wrote:
On 07/27/2011 11:38 PM, Scott Wood wrote:
Note that there will not be one implementation of nand_copy_image suitable for all hardware, just as currently nand_spl/nand_boot.c is not used for all NAND SPL targets.
Hm. I know that. I just adapated the old nand_boot.c.
While we're moving things around, could we call it something like "nand_spl_simple.c"?
AFAIK the other implementations use prefixes for the function names - therefore we can just add them to the nand-spl-library and gcc will do the rest.
The other implementations do not have prefixes -- they all are entered via nand_boot(). More importantly, not all implementations are buildable for all targets. They depend on certain #defines that may not be there. This includes the "simple" implementation.
-Scott
Hi Scott,
On 07/28/2011 08:56 PM, Scott Wood wrote:
On Thu, 28 Jul 2011 09:51:01 +0200 Simon Schwarzsimonschwarzcor@googlemail.com wrote:
On 07/27/2011 11:38 PM, Scott Wood wrote:
Note that there will not be one implementation of nand_copy_image suitable for all hardware, just as currently nand_spl/nand_boot.c is not used for all NAND SPL targets.
Hm. I know that. I just adapated the old nand_boot.c.
While we're moving things around, could we call it something like "nand_spl_simple.c"?
Sure, if there are no arguments against -> will do.
AFAIK the other implementations use prefixes for the function names - therefore we can just add them to the nand-spl-library and gcc will do the rest.
The other implementations do not have prefixes -- they all are entered via nand_boot(). More importantly, not all implementations are buildable for all targets. They depend on certain #defines that may not be there. This includes the "simple" implementation.
Hm - so adding #ifdefs is inevitable then? Will do if there are no objections.
-Scott
Regards & thx for review! Simon
participants (2)
-
Scott Wood -
Simon Schwarz