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

[Scott Wood]

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.

-Scott