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

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

[Kumar Gala]

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

---
 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 */
+cmd_tbl_t cmd_bootm_sub[] = {
+	U_BOOT_CMD_MKENT(start, 0, 1, (void *)BOOTM_STATE_START, "", ""),
+	U_BOOT_CMD_MKENT(loados, 0, 1, (void *)BOOTM_STATE_LOADOS, "", ""),
+#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SPARC)
+	U_BOOT_CMD_MKENT(ramdisk, 0, 1, (void *)BOOTM_STATE_RAMDISK, "", ""),
+#endif
+#ifdef CONFIG_OF_LIBFDT
+	U_BOOT_CMD_MKENT(fdt, 0, 1, (void *)BOOTM_STATE_FDT, "", ""),
+#endif
+	U_BOOT_CMD_MKENT(bdt, 0, 1, (void *)BOOTM_STATE_OS_BD_T, "", ""),
+	U_BOOT_CMD_MKENT(cmdline, 0, 1, (void *)BOOTM_STATE_OS_CMDLINE, "", ""),
+	U_BOOT_CMD_MKENT(prep, 0, 1, (void *)BOOTM_STATE_OS_PREP, "", ""),
+	U_BOOT_CMD_MKENT(go, 0, 1, (void *)BOOTM_STATE_OS_GO, "", ""),
+};
+
+int do_bootm_subcommand (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+	int ret = 0;
+	int state;
+	cmd_tbl_t *c;
+
+	c = find_cmd_tbl(argv[1], &cmd_bootm_sub[0], ARRAY_SIZE(cmd_bootm_sub));
+
+	if (c) {
+		state = (int)c->cmd;
+
+		/* treat start special since it resets the state machine */
+		if (state == BOOTM_STATE_START) {
+			argc--;
+			argv++;
+			return bootm_start(cmdtp, flag, argc, argv);
+		}
+	}
+	/* Unrecognized command */
+	else {
+		printf ("Usage:\n%s\n", cmdtp->usage);
+		return 1;
+	}
+
+	if (images.state >= state) {
+		printf ("Trying to execute a command out of order\n");
+		printf ("Usage:\n%s\n", cmdtp->usage);
+		return 1;
+	}
+
+	images.state |= state;
+
+	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];
+
+			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
+#ifdef CONFIG_OF_LIBFDT
+		case BOOTM_STATE_FDT:
+		{
+			ulong bootmap_base = getenv_bootm_low();
+			ret = boot_relocate_fdt(&images.lmb, bootmap_base,
+				&images.ft_addr, &images.ft_len);
+			break;
+		}
+#endif
+		case BOOTM_STATE_OS_CMDLINE:
+			do_bootm_linux(BOOTM_STATE_OS_CMDLINE, argc, argv, &images);
+			break;
+		case BOOTM_STATE_OS_BD_T:
+			do_bootm_linux(BOOTM_STATE_OS_BD_T, argc, argv, &images);
+			break;
+		case BOOTM_STATE_OS_PREP:
+			do_bootm_linux(BOOTM_STATE_OS_PREP, argc, argv, &images);
+			break;
+		case BOOTM_STATE_OS_GO:
+			disable_interrupts();
+			do_bootm_linux(BOOTM_STATE_OS_GO, argc, argv, &images);
+			break;
+	}
+
+	return ret;
+}
+
 /*******************************************************************/
 /* bootm - boot application image from image in memory */
 /*******************************************************************/
@@ -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"
+	"\tstart [addr [arg ...]]\n"
+	"\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"
 );
 
 /*******************************************************************/
diff --git a/include/image.h b/include/image.h
index 82e6345..557e72a 100644
--- a/include/image.h
+++ b/include/image.h
@@ -228,6 +228,7 @@ typedef struct bootm_headers {
 #endif
 #endif
 
+#ifndef USE_HOSTCC
 	image_info_t	os;		/* os image info */
 	ulong		ep;		/* entry point of OS */
 
@@ -238,8 +239,25 @@ typedef struct bootm_headers {
 #endif
 	ulong		ft_len;		/* length of flat device tree */
 
+	ulong		initrd_start;
+	ulong		initrd_end;
+	ulong		cmdline_start;
+	ulong		cmdline_end;
+	bd_t		*kbd;
+#endif
+
 	int		verify;		/* getenv("verify")[0] != 'n' */
-	int		valid;		/* set to 1 if we've set values in the header */
+
+#define	BOOTM_STATE_START	(0x00000001)
+#define	BOOTM_STATE_LOADOS	(0x00000002)
+#define	BOOTM_STATE_RAMDISK	(0x00000004)
+#define	BOOTM_STATE_FDT		(0x00000008)
+#define	BOOTM_STATE_OS_CMDLINE	(0x00000010)
+#define	BOOTM_STATE_OS_BD_T	(0x00000020)
+#define	BOOTM_STATE_OS_PREP	(0x00000040)
+#define	BOOTM_STATE_OS_GO	(0x00000080)
+	int		state;
+
 #ifndef USE_HOSTCC
 	struct lmb	lmb;		/* for memory mgmt */
 #endif
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c
index 38266e1..8ac0de0 100644
--- a/lib_ppc/bootm.c
+++ b/lib_ppc/bootm.c
@@ -47,6 +47,7 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+extern int do_reset (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
 extern ulong get_effective_memsize(void);
 static ulong get_sp (void);
 static void set_clocks_in_mhz (bd_t *kbd);
@@ -55,30 +56,78 @@ static void set_clocks_in_mhz (bd_t *kbd);
 #define CFG_LINUX_LOWMEM_MAX_SIZE	(768*1024*1024)
 #endif
 
-__attribute__((noinline))
-int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
+static void boot_jump_linux(bootm_headers_t *images)
 {
-	ulong	sp;
-
-	ulong	initrd_start, initrd_end;
-	ulong	rd_len;
-	ulong	size;
-	phys_size_t bootm_size;
-
-	ulong	cmd_start, cmd_end, bootmap_base;
-	bd_t	*kbd;
 	void	(*kernel)(bd_t *, ulong r4, ulong r5, ulong r6,
 			  ulong r7, ulong r8, ulong r9);
-	int	ret;
-	ulong	of_size = images->ft_len;
-	struct lmb *lmb = &images->lmb;
-
-#if defined(CONFIG_OF_LIBFDT)
-	char	*of_flat_tree = images->ft_addr;
+#ifdef CONFIG_OF_LIBFDT
+	char *of_flat_tree = images->ft_addr;
 #endif
 
 	kernel = (void (*)(bd_t *, ulong, ulong, ulong,
 			   ulong, ulong, ulong))images->ep;
+	debug ("## Transferring control to Linux (at address %08lx) ...\n",
+		(ulong)kernel);
+
+	show_boot_progress (15);
+
+#if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500)
+	unlock_ram_in_cache();
+#endif
+
+#if defined(CONFIG_OF_LIBFDT)
+	if (of_flat_tree) {	/* device tree; boot new style */
+		/*
+		 * Linux Kernel Parameters (passing device tree):
+		 *   r3: pointer to the fdt
+		 *   r4: 0
+		 *   r5: 0
+		 *   r6: epapr magic
+		 *   r7: size of IMA in bytes
+		 *   r8: 0
+		 *   r9: 0
+		 */
+#if defined(CONFIG_85xx) || defined(CONFIG_440)
+ #define EPAPR_MAGIC	(0x45504150)
+#else
+ #define EPAPR_MAGIC	(0x65504150)
+#endif
+
+		debug ("   Booting using OF flat tree...\n");
+		(*kernel) ((bd_t *)of_flat_tree, 0, 0, EPAPR_MAGIC,
+			   CFG_BOOTMAPSZ, 0, 0);
+		/* does not return */
+	} else
+#endif
+	{
+		/*
+		 * Linux Kernel Parameters (passing board info data):
+		 *   r3: ptr to board info data
+		 *   r4: initrd_start or 0 if no initrd
+		 *   r5: initrd_end - unused if r4 is 0
+		 *   r6: Start of command line string
+		 *   r7: End   of command line string
+		 *   r8: 0
+		 *   r9: 0
+		 */
+		ulong cmd_start = images->cmdline_start;
+		ulong cmd_end = images->cmdline_end;
+		ulong initrd_start = images->initrd_start;
+		ulong initrd_end = images->initrd_end;
+		bd_t *kbd = images->kbd;
+
+		debug ("   Booting using board info...\n");
+		(*kernel) (kbd, initrd_start, initrd_end,
+			   cmd_start, cmd_end, 0, 0);
+		/* does not return */
+	}
+	return ;
+}
+
+static void boot_prep_linux(struct lmb *lmb)
+{
+	phys_size_t bootm_size;
+	ulong size, sp, bootmap_base;
 
 	bootmap_base = getenv_bootm_low();
 	bootm_size = getenv_bootm_size();
@@ -116,127 +165,156 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 	sp -= 1024;
 	lmb_reserve(lmb, sp, (CFG_SDRAM_BASE + get_effective_memsize() - sp));
 
+	return ;
+}
+
+static int boot_cmdline_linux(bootm_headers_t *images)
+{
+	ulong bootmap_base = getenv_bootm_low();
+	ulong of_size = images->ft_len;
+	struct lmb *lmb = &images->lmb;
+	ulong *cmd_start = &images->cmdline_start;
+	ulong *cmd_end = &images->cmdline_end;
+
+	int ret = 0;
+
 	if (!of_size) {
 		/* allocate space and init command line */
-		ret = boot_get_cmdline (lmb, &cmd_start, &cmd_end, bootmap_base);
+		ret = boot_get_cmdline (lmb, cmd_start, cmd_end, bootmap_base);
 		if (ret) {
 			puts("ERROR with allocation of cmdline\n");
-			goto error;
+			return ret;
 		}
+	}
+
+	return ret;
+}
+
+static int boot_bd_t_linux(bootm_headers_t *images)
+{
+	ulong bootmap_base = getenv_bootm_low();
+	ulong of_size = images->ft_len;
+	struct lmb *lmb = &images->lmb;
+	bd_t **kbd = &images->kbd;
 
+	int ret = 0;
+
+	if (!of_size) {
 		/* allocate space for kernel copy of board info */
-		ret = boot_get_kbd (lmb, &kbd, bootmap_base);
+		ret = boot_get_kbd (lmb, kbd, bootmap_base);
 		if (ret) {
 			puts("ERROR with allocation of kernel bd\n");
-			goto error;
+			return ret;
 		}
-		set_clocks_in_mhz(kbd);
+		set_clocks_in_mhz(*kbd);
 	}
 
+	return ret;
+}
+
+static int boot_body_linux(bootm_headers_t *images)
+{
+	ulong rd_len, bootmap_base = getenv_bootm_low();
+	ulong of_size = images->ft_len;
+	struct lmb *lmb = &images->lmb;
+	ulong *initrd_start = &images->initrd_start;
+	ulong *initrd_end = &images->initrd_end;
+#if defined(CONFIG_OF_LIBFDT)
+	char **of_flat_tree = &images->ft_addr;
+#endif
+
+	int ret;
+
+	/* allocate space and init command line */
+	ret = boot_cmdline_linux(images);
+	if (ret)
+		return ret;
+
+	/* allocate space for kernel copy of board info */
+	ret = boot_bd_t_linux(images);
+	if (ret)
+		return ret;
+
 	rd_len = images->rd_end - images->rd_start;
+	ret = boot_ramdisk_high (lmb, images->rd_start, rd_len, initrd_start, initrd_end);
+	if (ret)
+		return ret;
 
 #if defined(CONFIG_OF_LIBFDT)
-	ret = boot_relocate_fdt(lmb, bootmap_base, &of_flat_tree, &of_size);
+	ret = boot_relocate_fdt(lmb, bootmap_base, of_flat_tree, &of_size);
 	if (ret)
-		goto error;
+		return ret;
 
 	/*
 	 * Add the chosen node if it doesn't exist, add the env and bd_t
 	 * if the user wants it (the logic is in the subroutines).
 	 */
 	if (of_size) {
-		if (fdt_chosen(of_flat_tree, 0) < 0) {
+		if (fdt_chosen(*of_flat_tree, 0) < 0) {
 			puts ("ERROR: ");
 			puts ("/chosen node create failed");
 			puts (" - must RESET the board to recover.\n");
-			goto error;
+			return -1;
 		}
 #ifdef CONFIG_OF_BOARD_SETUP
 		/* Call the board-specific fixup routine */
-		ft_board_setup(of_flat_tree, gd->bd);
+		ft_board_setup(*of_flat_tree, gd->bd);
 #endif
-	}
 
-	/* Fixup the fdt memreserve now that we know how big it is */
-	if (of_flat_tree) {
 		/* Delete the old LMB reservation */
-		lmb_free(lmb, (phys_addr_t)(u32)of_flat_tree,
-				(phys_size_t)fdt_totalsize(of_flat_tree));
+		lmb_free(lmb, (phys_addr_t)(u32)*of_flat_tree,
+				(phys_size_t)fdt_totalsize(*of_flat_tree));
 
-		ret = fdt_resize(of_flat_tree);
+		ret = fdt_resize(*of_flat_tree);
 		if (ret < 0)
-			goto error;
+			return ret;
 		of_size = ret;
 
-		if ((of_flat_tree) && (initrd_start && initrd_end))
+		if (*initrd_start && *initrd_end)
 			of_size += FDT_RAMDISK_OVERHEAD;
 		/* Create a new LMB reservation */
-		lmb_reserve(lmb, (ulong)of_flat_tree, of_size);
+		lmb_reserve(lmb, (ulong)*of_flat_tree, of_size);
+
+		/* fixup the initrd now that we know where it should be */
+		if (*initrd_start && *initrd_end)
+			fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
 	}
 #endif	/* CONFIG_OF_LIBFDT */
+	return 0;
+}
 
-	ret = boot_ramdisk_high (lmb, images->rd_start, rd_len, &initrd_start, &initrd_end);
-	if (ret)
-		goto error;
-
-#if defined(CONFIG_OF_LIBFDT)
-	/* fixup the initrd now that we know where it should be */
-	if ((of_flat_tree) && (initrd_start && initrd_end))
-		fdt_initrd(of_flat_tree, initrd_start, initrd_end, 1);
-#endif
-	debug ("## Transferring control to Linux (at address %08lx) ...\n",
-		(ulong)kernel);
+__attribute__((noinline))
+int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
+{
+	int	ret;
 
-	show_boot_progress (15);
+	if (flag & BOOTM_STATE_OS_CMDLINE) {
+		boot_cmdline_linux(images);
+		return 0;
+	}
 
-#if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500)
-	unlock_ram_in_cache();
-#endif
+	if (flag & BOOTM_STATE_OS_BD_T) {
+		boot_bd_t_linux(images);
+		return 0;
+	}
 
-#if defined(CONFIG_OF_LIBFDT)
-	if (of_flat_tree) {	/* device tree; boot new style */
-		/*
-		 * Linux Kernel Parameters (passing device tree):
-		 *   r3: pointer to the fdt
-		 *   r4: 0
-		 *   r5: 0
-		 *   r6: epapr magic
-		 *   r7: size of IMA in bytes
-		 *   r8: 0
-		 *   r9: 0
-		 */
-#if defined(CONFIG_85xx) || defined(CONFIG_440)
- #define EPAPR_MAGIC	(0x45504150)
-#else
- #define EPAPR_MAGIC	(0x65504150)
-#endif
+	if (flag & BOOTM_STATE_OS_PREP) {
+		boot_prep_linux(&images->lmb);
+		return 0;
+	}
 
-		debug ("   Booting using OF flat tree...\n");
-		(*kernel) ((bd_t *)of_flat_tree, 0, 0, EPAPR_MAGIC,
-			   CFG_BOOTMAPSZ, 0, 0);
-		/* does not return */
-	} else
-#endif
-	{
-		/*
-		 * Linux Kernel Parameters (passing board info data):
-		 *   r3: ptr to board info data
-		 *   r4: initrd_start or 0 if no initrd
-		 *   r5: initrd_end - unused if r4 is 0
-		 *   r6: Start of command line string
-		 *   r7: End   of command line string
-		 *   r8: 0
-		 *   r9: 0
-		 */
-		debug ("   Booting using board info...\n");
-		(*kernel) (kbd, initrd_start, initrd_end,
-			   cmd_start, cmd_end, 0, 0);
-		/* does not return */
+	if (flag & BOOTM_STATE_OS_GO) {
+		boot_jump_linux(images);
+		return 0;
 	}
-	return 1;
 
-error:
-	return 1;
+	boot_prep_linux(&images->lmb);
+	ret = boot_body_linux(images);
+	if (ret)
+		return ret;
+	boot_jump_linux(images);
+
+	return 0;
 }
 
 static ulong get_sp (void)
-- 
1.5.5.1

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

[Kumar Gala]

On Sep 23, 2008, at 10:07 AM, 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.
>
> ---
> common/cmd_bootm.c |  142 ++++++++++++++++++++++++++++-
> include/image.h    |   20 ++++-
> lib_ppc/bootm.c    |  262 +++++++++++++++++++++++++++++++++ 
> +------------------
> 3 files changed, 330 insertions(+), 94 deletions(-)

Guys,

Any comments on this?

- k

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

[Jerry Van Baren]

Kumar Gala wrote:
> On Sep 23, 2008, at 10:07 AM, 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.
>>
>> ---
>> common/cmd_bootm.c |  142 ++++++++++++++++++++++++++++-
>> include/image.h    |   20 ++++-
>> lib_ppc/bootm.c    |  262 +++++++++++++++++++++++++++++++++ 
>> +------------------
>> 3 files changed, 330 insertions(+), 94 deletions(-)
> 
> Guys,
> 
> Any comments on this?
> 
> - k

I didn't get to it over the weekend.   Here in the North summer is a 
fleeting thing to be savored while it lasts.  :-/

Sorry,
gvb

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

[Kumar Gala]

On Sep 29, 2008, at 8:49 AM, Jerry Van Baren wrote:

> Kumar Gala wrote:
>> On Sep 23, 2008, at 10:07 AM, 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.
>>>
>>> ---
>>> common/cmd_bootm.c |  142 ++++++++++++++++++++++++++++-
>>> include/image.h    |   20 ++++-
>>> lib_ppc/bootm.c    |  262 +++++++++++++++++++++++++++++++++  
>>> +------------------
>>> 3 files changed, 330 insertions(+), 94 deletions(-)
>> Guys,
>> Any comments on this?
>> - k
>
> I didn't get to it over the weekend.   Here in the North summer is a  
> fleeting thing to be savored while it lasts.  :-/

Any more feedback on this towards getting something into -next?

- k

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

[Jerry Van Baren]

Kumar Gala wrote:
> 
> On Sep 29, 2008, at 8:49 AM, Jerry Van Baren wrote:
> 
>> Kumar Gala wrote:
>>> On Sep 23, 2008, at 10:07 AM, 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.
>>>>
>>>> ---
>>>> common/cmd_bootm.c |  142 ++++++++++++++++++++++++++++-
>>>> include/image.h    |   20 ++++-
>>>> lib_ppc/bootm.c    |  262 +++++++++++++++++++++++++++++++++ 
>>>> +------------------
>>>> 3 files changed, 330 insertions(+), 94 deletions(-)
>>> Guys,
>>> Any comments on this?
>>> - k
>>
>> I didn't get to it over the weekend.   Here in the North summer is a 
>> fleeting thing to be savored while it lasts.  :-/
> 
> Any more feedback on this towards getting something into -next?
> 
> - k

Hi Kumar,

I've been working on the micro side (the bootm [a-z]+ commands) and the 
macro side (understanding cmd_bootm.c and image.c).

I've been keeping notes on the denx.de wiki (inappropriately) on the FDT 
page.  They aren't entirely coherent either.  :-/

I applied your changeset and verified that the traditional bootm syntax 
worked.  I then tried to figure out a working sequence of bootm [a-z]+ 
commands and failed.
   <http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Sequence>
* I didn't see any command for disabling interrupts
* When I do the "bootm loados" command, my target (MPC8360EMDS) reboots
   * Could be because interrupts are not disabled but we overwrite the 
exception vectors.
   * Could be I'm doing something wrong.
Do you have a boot script "use case" to illustrate a sequence that works 
for you?

I have also been looking at the functions defined in cmd_bootm.c and 
image.c.  Image.c is HUGE.  Cmd_bootm.c is pretty complex and part of 
the complexity is that it is closely coupled with image.c functions.
* I would like to separate and understand the control flow separate from 
the image handling, fdt handling, bd_t handling, etc.
* Image.c is WAY too large: 2x what it probably should be.  Looking at 
it, it seems like we should pull all the FIT stuff out of image.c and 
make a new fit_image.c.  (I don't know what sort of coupling there would 
be between a legacy image.c and a new fit_image.c, I suspect not much.)

Enumerating the functions:
<http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Refactoring_cmd_bootm_c>
<http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Refactoring_image_c>

Best regards,
gvb

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

[Kumar Gala]

On Oct 8, 2008, at 8:17 AM, Jerry Van Baren wrote:

> Kumar Gala wrote:
>> On Sep 29, 2008, at 8:49 AM, Jerry Van Baren wrote:
>>> Kumar Gala wrote:
>>>> On Sep 23, 2008, at 10:07 AM, 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.
>>>>>
>>>>> ---
>>>>> common/cmd_bootm.c |  142 ++++++++++++++++++++++++++++-
>>>>> include/image.h    |   20 ++++-
>>>>> lib_ppc/bootm.c    |  262 +++++++++++++++++++++++++++++++++  
>>>>> +------------------
>>>>> 3 files changed, 330 insertions(+), 94 deletions(-)
>>>> Guys,
>>>> Any comments on this?
>>>> - k
>>>
>>> I didn't get to it over the weekend.   Here in the North summer is  
>>> a fleeting thing to be savored while it lasts.  :-/
>> Any more feedback on this towards getting something into -next?
>> - k
>
> Hi Kumar,
>
> I've been working on the micro side (the bootm [a-z]+ commands) and  
> the macro side (understanding cmd_bootm.c and image.c).
>
> I've been keeping notes on the denx.de wiki (inappropriately) on the  
> FDT page.  They aren't entirely coherent either.  :-/
>
> I applied your changeset and verified that the traditional bootm  
> syntax worked.  I then tried to figure out a working sequence of  
> bootm [a-z]+ commands and failed.
>  <http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Sequence>
> * I didn't see any command for disabling interrupts

You have to have the interrupt command built in.  than you can do  
'interrupts off'

>
> * When I do the "bootm loados" command, my target (MPC8360EMDS)  
> reboots
>  * Could be because interrupts are not disabled but we overwrite the  
> exception vectors.
>  * Could be I'm doing something wrong.

this is most likely the case.

> Do you have a boot script "use case" to illustrate a sequence that  
> works for you?

yes, will post in a follow up.

> I have also been looking at the functions defined in cmd_bootm.c and  
> image.c.  Image.c is HUGE.  Cmd_bootm.c is pretty complex and part  
> of the complexity is that it is closely coupled with image.c  
> functions.
> * I would like to separate and understand the control flow separate  
> from the image handling, fdt handling, bd_t handling, etc.
> * Image.c is WAY too large: 2x what it probably should be.  Looking  
> at it, it seems like we should pull all the FIT stuff out of image.c  
> and make a new fit_image.c.  (I don't know what sort of coupling  
> there would be between a legacy image.c and a new fit_image.c, I  
> suspect not much.)
>
> Enumerating the functions:
> <http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Refactoring_cmd_bootm_c 
> >
> <http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Refactoring_image_c>

refactoring image.c is orthogonal to the functionality I'm trying to  
enable.

- k

[U-Boot] [RFC][PATCH v4] bootm: Add sub commands (sequence of commands to replicate bootm)

[Kumar Gala]

> I applied your changeset and verified that the traditional bootm  
> syntax worked.  I then tried to figure out a working sequence of  
> bootm [a-z]+ commands and failed.
>  <http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Sequence>
> * I didn't see any command for disabling interrupts
> * When I do the "bootm loados" command, my target (MPC8360EMDS)  
> reboots
>  * Could be because interrupts are not disabled but we overwrite the  
> exception vectors.
>  * Could be I'm doing something wrong.
> Do you have a boot script "use case" to illustrate a sequence that  
> works for you?

setenv bootm_low  00000000
setenv bootm_size 10000000
bootm start 0x1000000 0x1300000 0xf00000
interrupts off
bootm loados
bootm ramdisk
bootm fdt
fdt boardsetup
fdt chosen $initrd_start $initrd_end
bootm prep
bootm go

- k

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

[Jerry Van Baren]

Kumar Gala wrote:
> 
> On Oct 8, 2008, at 8:17 AM, Jerry Van Baren wrote:
> 
>> Kumar Gala wrote:
>>> On Sep 29, 2008, at 8:49 AM, Jerry Van Baren wrote:
>>>> Kumar Gala wrote:
>>>>> On Sep 23, 2008, at 10:07 AM, 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.
>>>>>>
>>>>>> ---
>>>>>> common/cmd_bootm.c |  142 ++++++++++++++++++++++++++++-
>>>>>> include/image.h    |   20 ++++-
>>>>>> lib_ppc/bootm.c    |  262 +++++++++++++++++++++++++++++++++ 
>>>>>> +------------------
>>>>>> 3 files changed, 330 insertions(+), 94 deletions(-)
>>>>> Guys,
>>>>> Any comments on this?
>>>>> - k
>>>>
>>>> I didn't get to it over the weekend.   Here in the North summer is a 
>>>> fleeting thing to be savored while it lasts.  :-/
>>> Any more feedback on this towards getting something into -next?
>>> - k
>>
>> Hi Kumar,
>>
>> I've been working on the micro side (the bootm [a-z]+ commands) and 
>> the macro side (understanding cmd_bootm.c and image.c).
>>
>> I've been keeping notes on the denx.de wiki (inappropriately) on the 
>> FDT page.  They aren't entirely coherent either.  :-/
>>
>> I applied your changeset and verified that the traditional bootm 
>> syntax worked.  I then tried to figure out a working sequence of bootm 
>> [a-z]+ commands and failed.
>>  <http://www.denx.de/wiki/view/U-Boot/UBootFdtInfo#Sequence>
>> * I didn't see any command for disabling interrupts
> 
> You have to have the interrupt command built in.  than you can do 
> 'interrupts off'

Yup, I'll have to fix that.

>> * When I do the "bootm loados" command, my target (MPC8360EMDS) reboots
>>  * Could be because interrupts are not disabled but we overwrite the 
>> exception vectors.
>>  * Could be I'm doing something wrong.
> 
> this is most likely the case.

Kumar being kind.  :-D

>> Do you have a boot script "use case" to illustrate a sequence that 
>> works for you?
> 
> yes, will post in a follow up.

Thanks, I'll run that tonight.

>> I have also been looking at the functions defined in cmd_bootm.c and 
>> image.c.  Image.c is HUGE.  Cmd_bootm.c is pretty complex and part of 

[snip]

> refactoring image.c is orthogonal to the functionality I'm trying to 
> enable.

Yup.  I took my eye off the ball.

Best regards,
gvb

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

[Jerry Van Baren]

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

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

[Kumar Gala]

On Oct 8, 2008, at 8:48 PM, Jerry Van Baren wrote:

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

ack.

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

>> @@ -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")

ack

>
>
>> +	"\tstart [addr [arg ...]]\n"
>
> The following is more descriptive?
> 	start <os_addr> [(-|<ramdisk_addr>) [<fdt_addr>]]

nack.  I left this as is to match the bootm help

>> +	"\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"

- k

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

[Kumar Gala]

On Oct 8, 2008, at 8:48 PM, Jerry Van Baren wrote:

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

I think my solution is a bit cleaner for us (adding  
arch_lmb_reserve).  Take a look and see how that works for you.

- k

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

[Jerry Van Baren]

Kumar Gala wrote:
> 
> On Oct 8, 2008, at 8:48 PM, Jerry Van Baren wrote:
> 
>> 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.
> 
> I think my solution is a bit cleaner for us (adding arch_lmb_reserve).  
> Take a look and see how that works for you.
> 
> - k

Oh yes, my hack was just to verify that reserving the stack space solved 
my problem.  It was not a solution.

Thanks,
gvb