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

[Wolfgang Denk]

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 ?

> CONFIG_CMD_SPL_NAND_OFS
> 	Offset in NAND to use

What about other storage devices?

...
> +/* 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?

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

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

> +/*
> + * 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.

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

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
That said, there may be good reasons for what you did beyond obsequi-
ous sycophantic parody. Perhaps you might be so kind as to elucidate.
         -- Tom Christiansen in <5ldjbm$jtk$1 at csnews.cs.colorado.edu>