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

[Tom Rini <trini@ti.com>]

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@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.

> 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.  We need
to be able to support the 4KB case, and we do (by not touching
anything).  We need to be able to support the 16KB case (cam_4xx_enc,
others) and we do in CONFIG_SPL_FRAMEWORK.  We also fit in the 8KB case
(hawkboard) with CONFIG_SPL_FRAMEWORK.  And of course, we fit in the
"lots" of space and big feature case.

-- 
Tom