[U-Boot] [PATCH V9 2/4] omap-common: Add NAND SPL linux booting

[Simon Schwarz]

On 12/07/2011 04:39 PM, Stefano Babic wrote:
> On 06/12/2011 19:34, Simon Schwarz wrote:
>> From: Simon Schwarz<simonschwarzcor at googlemail.com>
>>
>> This implements booting of Linux from NAND in SPL
>>
>> Related config parameters:
>> CONFIG_SYS_NAND_SPL_KERNEL_OFFS
>> 	Offset in NAND of direct boot kernel image to use in SPL
>> CONFIG_SYS_SPL_ARGS_ADDR
>> 	Address where the kernel boot arguments are expected - this is
>> 	normally RAM-start + 0x100 (on ARM)
>>
>> Signed-off-by: Simon Schwarz<simonschwarzcor at gmail.com>
>>
>> ---
>
> Hi Simon,
>
>
>>
>> diff --git a/arch/arm/cpu/armv7/omap-common/spl_nand.c b/arch/arm/cpu/armv7/omap-common/spl_nand.c
>> index 38d06b1..db52879 100644
>> --- a/arch/arm/cpu/armv7/omap-common/spl_nand.c
>> +++ b/arch/arm/cpu/armv7/omap-common/spl_nand.c
>> @@ -24,6 +24,7 @@
>>   #include<asm/u-boot.h>
>>   #include<asm/utils.h>
>>   #include<asm/arch/sys_proto.h>
>> +#include<asm/io.h>
>>   #include<nand.h>
>>   #include<version.h>
>>   #include<asm/omap_common.h>
>> @@ -32,6 +33,7 @@
>>   void spl_nand_load_image(void)
>>   {
>>   	struct image_header *header;
>> +	int *src, *dst;
>>   	switch (omap_boot_mode()) {
>>   	case NAND_MODE_HW_ECC:
>>   		debug("spl: nand - using hw ecc\n");
>> @@ -45,26 +47,56 @@ void spl_nand_load_image(void)
>>
>>   	/*use CONFIG_SYS_TEXT_BASE as temporary storage area */
>>   	header = (struct image_header *)(CONFIG_SYS_TEXT_BASE);
>> +#ifdef CONFIG_SPL_OS_BOOT
>> +	if (!spl_uboot_key()) {
>
> Using a gpio or whatever mechanis is baord specific. What about to set a
> weak function and let that the board decides itself what to do ?
Sounds reasonable to me. So I'am ok with this.

>
> Not always a GPIO can be used for this purpose.
>
> I have noted another issue. If the kernel image is not found, the
> fallback is to load u-boot. However, u-boot is searched at the address
> in NAND of the kernel, not at CONFIG_SYS_NAND_U_BOOT_OFFS.
>
> This means that if the kernel is not loaded at all, the SPL starts to
> start a u-boot.bin instead of u-boot.img loaded at
> CONFIG_CMD_SPL_NAND_OFS, and this fails.
>
>> +		/*
>> +		 * load parameter image
>> +		 * load to temp position since nand_spl_load_image reads
>> +		 * a whole block which is typically larger than
>> +		 * CONFIG_CMD_SAVEBP_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);
>
> So if the parse function fails, we should have the fallback to u-boot,
> exactly as we have now the fallback from u-boot.img to u-boot.bin.
>
Hm, I don't think that we want any fallback here. I would prefere an 
error message and hang. The direct boot is designed to be used in the 
field - so IMHO we don't want to start the bootloader automatically if a 
normal startup fails.

>
> The function you write are in omap-common/spl_nand.c, but they are not
> specific for OMAP, and any architecture can profit from your work. What
> about to move common functions away from TI related directory ? I do not
> know which is the best place, maybe a new directory under the root as
> spllib ?
Hm thats generally right. I would prefer lib_spl. Somehow it feels like 
going back to nand_spl times. Duno if there are better places?

The first two points I will fix/rework hopfully on monday (I'am not in 
the next two days)

Regards
Simon