[U-Boot] [PATCH V13 01/12] Add cmd_spl command

[Stefano Babic]

On 13/02/2012 08:54, Wolfgang Denk wrote:
> Dear Stefano Babic,
> 
> In message <1328350963-30989-2-git-send-email-sbabic at denx.de> you wrote:
>> From: Simon Schwarz <simonschwarzcor at googlemail.com>
>>
>> This adds a spl command to the u-boot.
>>
>> Related config:
>> CONFIG_CMD_CPL
>> 	activate/deactivate the command
> 
> CONFIG_CMD_CPL??? Is this a typo, and you mean CONFIG_CMD_SPL ?

Yes, this a type - must be fixed.

> 
>> CONFIG_CMD_SPL_NAND_OFS
>> 	Offset in NAND to use
> 
> What about other storage devices?

Booting from other storage devices is not yet implemented, but it is
surely desired. I can imagine that after this patchset will flow into
mainline, there will be more use cases, and further storages will be added.

At the moment, only NAND is implemented and for this reason
only CONFIG_CMD_SPL_NAND_OFS is defined. This leaves the possibility to
have different offsets from different boot devices for the kernel
parameter area.

> 
> ...
>> +/* Calls bootm with the parameters given */
>> +static int call_bootm(int argc, char * const argv[], char *subcommand[])
>> +{
>> +	char *bootm_argv[5];
>> +	char command[] = "do_bootm";
> ...
>> +}
> 
> Why is this needed?  Why don't you run do_bootm() directly?

Mmmhh... At least using find_cmd("do_bootm") in do_bootm should be not
required,
if this is what you mind.

>> +static int spl_export_fdt(int argc, char * const argv[])
>> +{
> ...
>> +	/* inspect paramters and execute bootm */
>> +	argc--;
>> +	argv++;
>> +	if (call_bootm(argc, argv, subcommand))
>> +		return -1;
>> +
>> +	printf("Argument image is now in RAM: 0x%p\n",
>> +		(void *)images.ft_addr);
>> +	return 0;
>> +#else
>> +	printf("Das U-Boot was build without fdt support - aborting\n");
>> +	return -1;
>> +#endif
>> +}
>> +
>> +/* assemble the bootm patameters for atags creation */
>> +static int spl_export_atags(int argc, char * const argv[])
>> +{
> ...
>> +	/* inspect parameters and execute bootm */
>> +	argc--;
>> +	argv++;
>> +	if (call_bootm(argc, argv, subcommand))
>> +		return -1;
>> +
>> +	printf("Argument image is now in RAM at: 0x%p\n",
>> +		(void *)gd->bd->bi_boot_params);
>> +	return 0;
>> +#endif
>> +	printf("Das U-Boot was build without ATAGS support - aborting\n");
>> +	return -1;
>> +}
> 
> This is basicly identical code, only the data structures differ.
> 
> Please use a common function, and pass (a pointer to) the data as
> argument.

Clear, thanks, I'll fix it.

> 
>> +	if (c) {
>> +		cmd = (int)c->cmd;
>> +		switch (cmd) {
>> +		case SPL_EXPORT_FDT:
>> +			argc--;
>> +			argv++;
>> +			return spl_export_fdt(argc, argv);
>> +			break;
>> +		case SPL_EXPORT_ATAGS:
>> +			argc--;
>> +			argv++;
>> +			return spl_export_atags(argc, argv);
>> +			break;
>> +		default:
> 
> This will also eliminate this switch() - you just have to set the
> pointer to the respective data to pass.

You're right !

> 
>> +/*
>> + * Arguments:
>> + * 1: subcommand
>> + * 2: image_type
>> + * 3: nand_offset
>> + * 4: kernel_addr
>> + * 5: initrd_addr
>> + * 6: fdt_adr
>> + */
> 
> I think it is broken to restrict this by design to NAND booting only.

It is not. The comment is wrong and does not correspond to the
implemented command. The result of spl export is not written
automatically to storage, and this let the possibility to write it into
the correct storage device.

"spl export <img=atags|fdt> [kernel_addr] [initrd_addr] "

nand_offset is something obsolete - and the whole comment does not add
more info as we can read after the U_BOOT_CMD. I will drop it.

> 
> This is a command that attemtps to deal with SPL booting in general,
> so it should support all possible kinds of boot media - NOR, NAND,
> MMC, SPI flash, USB, ...

Right. However, this restriction is not in this patch (apart the wrong
comment). I think the command is already general and not limited to a
storage device. But there is this limitation in another patch.

The restriction about storage media is in board_init_r(), in
[PATCH V13 03/12] omap-common: Add NAND SPL linux booting. Maybe you can
take a look.

Here the selection if booting the kernel or u-boot is taken inside
spl_nand_load_image(). So at this point we have already decided which is
the storage media, and this function starts the kernel. So maybe we
should also rename it to a general name, such as spl_load_image().

Really this function has not a lot to do to do with NAND, except the
fact it calls nand_spl_load_image() to copy from NAND to RAM.
Your comment applies very well to this function. It should be made
more general, maybe adding function pointers that must be called after
selecting the right device, something like:

	device_media->load_image(addr, size);

where device_media->load_image points to the specific devic media
function to copy an image, for NAND it is nand_spl_load_image.

What do you think about ?

We have already discussed how we can proceed with this feature and how
to merge it into mainline. Maybe it is worth to spend again a couple of
words. The other main restriction is the fact that this feature is
OMAP-only, and not so general as it should be. Not only, also the SPL
framework runs mainly on TI SOCs, and it is really a pity. Most of part
that are now hidden in omap-common/ can be generalized for the other
SOCs, and duplication can be avoided. IMHO there is already some code
that can be factorized between OMAp and Davinci.

Because there is a high probability to break other boards (I have
already posted patches in this direction, but I broke davinci), we
decided to make several steps. Firstly pushing this feature, then making
SPL common - I will add now also making booting from SPL device-independent.

My question is if we proceed with several steps, or we want to push only
a complete version. Any thought about this ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================