[U-Boot] [PATCH v4 19/20] SPL: NAND: Enhance drivers/mtd/nand/nand_spl_simple.c

[Tom Rini]

On Mon, Aug 27, 2012 at 11:16:45AM -0500, Scott Wood wrote:
> On 08/27/2012 09:37 AM, Tom Rini wrote:
> > On 08/24/2012 05:09 PM, Scott Wood wrote:
> >> On 08/24/2012 06:58 PM, Tom Rini wrote:
> >>> Takes the load function from arch/arm/cpu/armv7/omap-common/spl_nand.c
> >>> instead.  This will allow for easier integration of SPL-boots-Linux code on
> >>> other arches.
> >>>
> >>> Signed-off-by: Tom Rini <trini at ti.com>
> >>> ---
> >>> Changes in v4:
> >>> - Leave nand_spl_load.c alone, move the new load into nand_spl_simple.c
> >> [snip]
> >>> +void spl_nand_load_image(void)
> >>> +{
> >>> +	struct image_header *header;
> >>> +	int *src __attribute__((unused));
> >>> +	int *dst __attribute__((unused));
> >>> +
> >>> +	nand_init();
> >>> +
> >>> +	/* use CONFIG_SYS_TEXT_BASE as temporary storage area */
> >>> +	header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
> >>> +#ifdef CONFIG_SPL_OS_BOOT
> >>> +	if (!spl_start_uboot()) {
> >>> +		/*
> >>> +		 * load parameter image
> >>> +		 * load to temp position since nand_spl_load_image reads
> >>> +		 * a whole block which is typically larger than
> >>> +		 * CONFIG_CMD_SPL_WRITE_SIZE therefore may overwrite
> >>> +		 * following sections like BSS
> >>> +		 */
> >>> +		nand_spl_load_image(CONFIG_CMD_SPL_NAND_OFS,
> >>> +			CONFIG_CMD_SPL_WRITE_SIZE,
> >>> +			(void *)CONFIG_SYS_TEXT_BASE);
> >>> +		/* copy to destintion */
> >>> +		for (dst = (int *)CONFIG_SYS_SPL_ARGS_ADDR,
> >>> +				src = (int *)CONFIG_SYS_TEXT_BASE;
> >>> +				src < (int *)(CONFIG_SYS_TEXT_BASE +
> >>> +				CONFIG_CMD_SPL_WRITE_SIZE);
> >>> +				src++, dst++) {
> >>> +			writel(readl(src), dst);
> >>> +		}
> >>> +
> >>> +		/* load linux */
> >>> +		nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
> >>> +			CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> +		spl_parse_image_header(header);
> >>> +		if (header->ih_os == IH_OS_LINUX) {
> >>> +			/* happy - was a linux */
> >>> +			nand_spl_load_image(CONFIG_SYS_NAND_SPL_KERNEL_OFFS,
> >>> +				spl_image.size, (void *)spl_image.load_addr);
> >>> +			nand_deselect();
> >>> +			return;
> >>> +		} else {
> >>> +			puts("The Expected Linux image was not "
> >>> +				"found. Please check your NAND "
> >>> +				"configuration.\n");
> >>> +			puts("Trying to start u-boot now...\n");
> >>> +		}
> >>> +	}
> >>> +#endif
> >>> +#ifdef CONFIG_NAND_ENV_DST
> >>> +	nand_spl_load_image(CONFIG_ENV_OFFSET,
> >>> +		CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> +	spl_parse_image_header(header);
> >>> +	nand_spl_load_image(CONFIG_ENV_OFFSET, spl_image.size,
> >>> +		(void *)spl_image.load_addr);
> >>> +#ifdef CONFIG_ENV_OFFSET_REDUND
> >>> +	nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND,
> >>> +		CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> +	spl_parse_image_header(header);
> >>> +	nand_spl_load_image(CONFIG_ENV_OFFSET_REDUND, spl_image.size,
> >>> +		(void *)spl_image.load_addr);
> >>> +#endif
> >>> +#endif
> >>> +	/* Load u-boot */
> >>> +	nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
> >>> +		CONFIG_SYS_NAND_PAGE_SIZE, (void *)header);
> >>> +	spl_parse_image_header(header);
> >>> +	nand_spl_load_image(CONFIG_SYS_NAND_U_BOOT_OFFS,
> >>> +		spl_image.size, (void *)spl_image.load_addr);
> >>> +	nand_deselect();
> >>> +}
> >>
> >> Will this refuse to link if spl_parse_image_header is not present, or
> >> will gc-sections remove it before the error is given?  Does this
> >> function leave any anonymous data that isn't cleaned up by gc-sections?
> >> Again, this file must not grow for users that don't need the new features.
> > 
> > Yes, spl_nand_load_image will be garbage collected and not link-error if
> > not called.  But note that all users of this file have been converted to
> > CONFIG_SPL_FRAMEWORK and would be using this function.
> 
> There are still a lot of nand_spl targets that have not yet been
> converted, some of which will be future users of this file (such as ppc
> 4xx).  This file is a replacement for nand_spl/nand_boot.c and will be
> used by the same SPLs.
> 
> >> What is the benefit of putting this in nand_spl_simple.c versus another
> >> file?  What if someone wants to use this with a different NAND boot
> >> implementation?
> > 
> > I would start by questioning the need of a 3rd SPL framework.
> 
> The "simple" driver does not work for all hardware.  This is why we have
> nand_spl/nand_boot_fsl_elbc.c and others in addition to
> nand_spl/nand_boot.c.  It's not a "3rd SPL framework", just a different
> NAND implementation.

The question boils down to, what are your size constraints?  I guess
what I'm saying is, if it's <4kb, it's not using this file nor the
framework.  If we've got more than 4kb to work with, it's using the
framework (with changes if needed, of course) and I guess we could move
the function to common/spl/spl_nand.c and add
drivers/mtd/nand/nand_spl_fsl_elbc.c and so on.  Now that I've had more
coffee, do I follow your suggestion right?

-- 
Tom