[U-Boot] [RFC][PATCH v4] bootm: Add sub commands

[Jerry Van Baren <gvb.uboot@gmail.com>]

Kumar Gala wrote:
> * Use new find_cmd_tbl() to process sub-commands
> 
> If this looks good I'll go ahead and clean it up for the other arches and OSes.

Hi Kumar,

Thanks to your sequence hint, interrupt command hint, and one additional 
piece, I have this working now.

The missing piece is reserving memory for the stack before copying the 
ramdisk to high mem.  It looks like this was lost when you consolidated 
the machine-specific lib_*/bootm.c do_bootm_linux() functions into one. 
  Without the reservation code, the copy overwrites the stack. 
Seriously bad karma.

See below for a proof-of-concept hack.

> ---
>  common/cmd_bootm.c |  142 ++++++++++++++++++++++++++++-
>  include/image.h    |   20 ++++-
>  lib_ppc/bootm.c    |  262 ++++++++++++++++++++++++++++++++++------------------
>  3 files changed, 330 insertions(+), 94 deletions(-)
> 
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 19257bb..e6dcb7a 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -34,6 +34,7 @@
>  #include <bzlib.h>
>  #include <environment.h>
>  #include <lmb.h>
> +#include <linux/ctype.h>
>  #include <asm/byteorder.h>
>  
>  #if defined(CONFIG_CMD_USB)
> @@ -273,7 +274,7 @@ static int bootm_start(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  	}
>  
>  	images.os.start = (ulong)os_hdr;
> -	images.valid = 1;
> +	images.state = BOOTM_STATE_START;
>  
>  	return 0;
>  }
> @@ -376,6 +377,113 @@ static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)
>  	return 0;
>  }
>  
> +/* we over load the cmd field with our state machine info instead of a
> + * function pointer */

Nitpick: comment format and s/over load/overload/
/*
  * We overload the cmd field with our state machine info instead of a
  * function pointer
  */

[snip]

> +int do_bootm_subcommand (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{

[snip]

> +	switch (state) {
> +		ulong load_end;
> +		case BOOTM_STATE_START:
> +			/* should never occur */
> +			break;
> +		case BOOTM_STATE_LOADOS:
> +			ret = bootm_load_os(images.os, &load_end, 0);
> +			if (ret)
> +				return ret;
> +
> +			lmb_reserve(&images.lmb, images.os.load,
> +					(load_end - images.os.load));
> +			break;
> +#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SPARC)
> +		case BOOTM_STATE_RAMDISK:
> +		{
> +			ulong rd_len = images.rd_end - images.rd_start;
> +			char str[17];

We need to reserve the memory used by the stack here or earlier. 
Unfortunately, all three target CPU architectures do this differently. 
Below is my proof-of-concept hack.  I did not attempt to generalize it 
(yet).

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index e6dcb7a..067fbcd 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -60,6 +60,10 @@

  DECLARE_GLOBAL_DATA_PTR;

+/**** START HACK ****/
+extern ulong get_effective_memsize(void);
+/**** END HACK ****/
+
  extern int gunzip (void *dst, int dstlen, unsigned char *src, unsigned 
long *lenp);
  #ifndef CFG_BOOTM_LEN
  #define CFG_BOOTM_LEN	0x800000	/* use 8MByte as default max gunzip size */
@@ -443,8 +447,18 @@ int do_bootm_subcommand (cmd_tbl_t *cmdtp, int 
flag, int argc, char *argv[])
  		case BOOTM_STATE_RAMDISK:
  		{
  			ulong rd_len = images.rd_end - images.rd_start;
+			ulong sp;
  			char str[17];

+/**** START HACK ****/
+			asm( "mr %0,1": "=r"(sp) : )
+			debug ("## Current stack ends at 0x%08lx\n", sp);
+
+			/* adjust sp by 1K to be safe */
+			sp -= 1024;
+			lmb_reserve(&images.lmb, sp, (CFG_SDRAM_BASE + 
get_effective_memsize() - sp));
+/**** END HACK ****/
+
  			ret = boot_ramdisk_high(&images.lmb, images.rd_start,
  				rd_len, &images.initrd_start, &images.initrd_end);
  			if (ret)

> +			ret = boot_ramdisk_high(&images.lmb, images.rd_start,
> +				rd_len, &images.initrd_start, &images.initrd_end);
> +			if (ret)
> +				return ret;
> +
> +			sprintf(str, "%lx", images.initrd_start);
> +			setenv("initrd_start", str);
> +			sprintf(str, "%lx", images.initrd_end);
> +			setenv("initrd_end", str);
> +		}
> +			break;
> +#endif

[snip]

> @@ -386,6 +494,23 @@ int do_bootm (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  	ulong		load_end = 0;
>  	int		ret;
>  
> +	/* determine if we have a sub command */
> +	if (argc > 1) {
> +		char *endp;
> +
> +		simple_strtoul(argv[1], &endp, 16);
> +		/* endp pointing to NULL means that argv[1] was just a
> +		 * valid number, pass it along to the normal bootm processing
> +		 *
> +		 * If endp is ':' or '#' assume a FIT identifier so pass
> +		 * along for normal processing.
> +		 *
> +		 * Right now we assume the first arg should never be '-'
> +		 */
> +		if ((*endp != 0) && (*endp != ':') && (*endp != '#'))
> +			return do_bootm_subcommand(cmdtp, flag, argc, argv);
> +	}
> +
>  	if (bootm_start(cmdtp, flag, argc, argv))
>  		return 1;
>  
> @@ -782,6 +907,21 @@ U_BOOT_CMD(
>  	"\tUse iminfo command to get the list of existing component\n"
>  	"\timages and configurations.\n"
>  #endif
> +	"\nSub-commands to do part of the bootm sequence.  The sub-commands "
> +	"must be\n"
> +	"issued in the order below (its ok to not issue all sub-commands):\n"

s/its/it's/  (contraction "it is", not possessive like "his")

> +	"\tstart [addr [arg ...]]\n"

The following is more descriptive?
	start <os_addr> [(-|<ramdisk_addr>) [<fdt_addr>]]

> +	"\tloados  - load OS image\n"
> +#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SPARC)
> +	"\tramdisk - relocate initrd, set env initrd_start/initrd_end\n"
> +#endif
> +#if defined(CONFIG_OF_LIBFDT)
> +	"\tfdt     - relocate flat device tree\n"
> +#endif
> +	"\tbdt     - OS specific bd_t processing\n"
> +	"\tcmdline - OS specific command line processing/setup\n"
> +	"\tprepos  - OS specific prep before relocation or go\n"
> +	"\tgo      - start OS\n"

[snip]

Thanks,
gvb