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

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

[Kumar Gala]

Posting this again for discussion.  The two features I'm interested in
enabling are:

* Having the ability to modify the device tree before its passed to
  the kernel but after 'fdt boardsetup'

* Ability to do all setup but not actually jumping to the kernel.
  (This is useful as a way to setup the memory image [kernel, ramdisk,
   fdt, etc] for a different cpu than the boot one)

Having bootm sub-commands allows both of these as we can break up
the sequeunce of steps that are part of the bootm process.

- k

---
 common/cmd_bootm.c |  112 +++++++++++++++++++++++++++
 include/image.h    |   11 +++
 lib_ppc/bootm.c    |  212 ++++++++++++++++++++++++++++++----------------------
 3 files changed, 244 insertions(+), 91 deletions(-)

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index 19257bb..0f6bd06 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)
@@ -376,6 +377,89 @@ static int bootm_load_os(image_info_t os, ulong *load_end, int boot_progress)
 	return 0;
 }
 
+int do_bootm_subcommand (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
+{
+	int ret = 0;
+
+	/* start */
+	if (argv[1][0] == 's') {
+		argc--;
+		argv++;
+		return bootm_start(cmdtp, flag, argc, argv);
+	}
+
+	if (!images.valid) {
+		printf("Need to call %s start first\n", argv[0]);
+		return 1;
+	}
+
+	/* load os */
+	if (argv[1][0] == 'l') {
+		ulong load_end;
+		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));
+
+		return ret;
+	}
+	/* initrd relocate */
+#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SPARC)
+	else if (argv[1][0] == 'i') {
+		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);
+
+		return ret;
+	}
+#endif
+#ifdef CONFIG_OF_LIBFDT
+	/* fdt relocate */
+	else if (argv[1][0] == 'f') {
+		ulong bootmap_base = getenv_bootm_low();
+		ret = boot_relocate_fdt(&images.lmb, bootmap_base,
+			&images.ft_addr, &images.ft_len);
+	}
+#endif
+#if 0
+are these really common ??? or is there any harm??
+	/* bd_t setup */
+	else if (argv[1][0] == 'p') {
+	}
+	/* cmd setup */
+	else if (argv[1][0] == 'c') {
+	}
+#endif
+	/* prep os */
+	else if (argv[1][0] == 'p') {
+		return do_bootm_linux(BOOT_OS_PREP, argc, argv, &images);
+	}
+	/* go */
+	else if (argv[1][0] == 'g') {
+		disable_interrupts();
+		do_bootm_linux(BOOT_OS_GO, argc, argv, &images);
+		return 0;
+	}
+	else {
+		/* Unrecognized command */
+		printf ("Usage:\n%s\n", cmdtp->usage);
+		return 1;
+	}
+
+	return ret;
+}
+
 /*******************************************************************/
 /* bootm - boot application image from image in memory */
 /*******************************************************************/
@@ -386,6 +470,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 +883,17 @@ U_BOOT_CMD(
 	"\tUse iminfo command to get the list of existing component\n"
 	"\timages and configurations.\n"
 #endif
+	"\t\nSub-commands to do part of the bootm sequence:\n"
+	"\tstart [addr [arg ...]]\n"
+	"\tloados - load OS image\n"
+	"\tprepos - OS specific prep before relocation or go\n"
+#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SPARC)
+	"\tinitrd - relocate initrd, set env initrd_start/initrd_end\n"
+#endif
+#if defined(CONFIG_OF_LIBFDT)
+	"\tfdt - relocate initrd\n"
+#endif
+	"\tgo - start os\n"
 );
 
 /*******************************************************************/
diff --git a/include/image.h b/include/image.h
index 82e6345..2c323ab 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,6 +239,13 @@ 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 */
 #ifndef USE_HOSTCC
@@ -640,4 +648,7 @@ static inline int fit_image_check_target_arch (const void *fdt, int node)
 #endif /* CONFIG_FIT_VERBOSE */
 #endif /* CONFIG_FIT */
 
+#define BOOT_OS_PREP	0x01
+#define BOOT_OS_GO	0x02
+
 #endif	/* __IMAGE_H__ */
diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c
index 38266e1..208ed3b 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;
+
+	kernel = (void (*)(bd_t *, ulong, ulong, ulong,
+			   ulong, ulong, ulong))images->ep;
+#ifdef CONFIG_OF_LIBFDT
+	char *of_flat_tree = images->ft_addr;
+#endif
+	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)
-	char	*of_flat_tree = images->ft_addr;
+	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
 
-	kernel = (void (*)(bd_t *, ulong, ulong, ulong,
-			   ulong, ulong, ulong))images->ep;
+		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,29 +165,51 @@ 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_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;
+	bd_t **kbd = &images->kbd;
+	ulong *cmd_start = &images->cmdline_start;
+	ulong *cmd_end = &images->cmdline_end;
+	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;
+
 	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;
 		}
 
 		/* 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);
 	}
 
 	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
@@ -149,94 +220,53 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
 			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);
 #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))
 			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);
 	}
 #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);
-
-	show_boot_progress (15);
-
-#if defined(CFG_INIT_RAM_LOCK) && !defined(CONFIG_E500)
-	unlock_ram_in_cache();
-#endif
+__attribute__((noinline))
+int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
+{
+	int	ret;
 
-#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 & BOOT_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 & BOOT_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] bootm: Add sub commands

[Jerry Van Baren]

Kumar Gala wrote:
> Posting this again for discussion.  The two features I'm interested in
> enabling are:
> 
> * Having the ability to modify the device tree before its passed to
>   the kernel but after 'fdt boardsetup'
> 
> * Ability to do all setup but not actually jumping to the kernel.
>   (This is useful as a way to setup the memory image [kernel, ramdisk,
>    fdt, etc] for a different cpu than the boot one)
> 
> Having bootm sub-commands allows both of these as we can break up
> the sequeunce of steps that are part of the bootm process.
> 
> - k

Hi Kumar,

Looks like a good proposal.  I've been rather distracted the last couple 
of weeks, but I'll put some eyeball time and runtime on your changes.

[snip]

> +#if 0
> +are these really common ??? or is there any harm??
> +	/* bd_t setup */
> +	else if (argv[1][0] == 'p') {
> +	}

If we are using a FDT, there is no reason for a bd_t as part of the boot 
process.  I vote for removal.
   (typo s/p/b/?)

> +	/* cmd setup */
> +	else if (argv[1][0] == 'c') {
> +	}

I don't know what "cmd setup" is/was off-hand, have to look into that. 
Probably also a removal.

[snip]

> @@ -782,6 +883,17 @@ U_BOOT_CMD(
>  	"\tUse iminfo command to get the list of existing component\n"
>  	"\timages and configurations.\n"
>  #endif
> +	"\t\nSub-commands to do part of the bootm sequence:\n"
> +	"\tstart [addr [arg ...]]\n"
> +	"\tloados - load OS image\n"
> +	"\tprepos - OS specific prep before relocation or go\n"
> +#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SPARC)
> +	"\tinitrd - relocate initrd, set env initrd_start/initrd_end\n"
> +#endif
> +#if defined(CONFIG_OF_LIBFDT)
> +	"\tfdt - relocate initrd\n"

Cut'n'paste? s/initrd/the flattened device tree/

[snip]

> diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c
> index 38266e1..208ed3b 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;
> +
> +	kernel = (void (*)(bd_t *, ulong, ulong, ulong,
> +			   ulong, ulong, ulong))images->ep;
> +#ifdef CONFIG_OF_LIBFDT
> +	char *of_flat_tree = images->ft_addr;
> +#endif

This should be moved above the "kernel = " line to keep it with the rest 
of the declarations, yes?

[big snip]

Thanks!
gvb

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

[Wolfgang Denk]

Dear Kumar Gala,

In message <1221688841-3197-1-git-send-email-galak@kernel.crashing.org> you wrote:
> 
> Having bootm sub-commands allows both of these as we can break up
> the sequeunce of steps that are part of the bootm process.

OK.

> +int do_bootm_subcommand (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	int ret = 0;
> +
> +	/* start */
> +	if (argv[1][0] == 's') {

I think just matching on the first letter is to restrictive; we'll end
up with artificical command "names" that nobody can remember.

> +	if (!images.valid) {
> +		printf("Need to call %s start first\n", argv[0]);
> +		return 1;
> +	}

We should probably set up a small state machine.

> +	else if (argv[1][0] == 'i') {

And we need comments what all this means. WTF is 'i' ?

> +#if 0
> +are these really common ??? or is there any harm??
> +	/* bd_t setup */
> +	else if (argv[1][0] == 'p') {
> +	}

Yes, we still have plenty of systems running in the field with
arch/ppc and even with 2.4 kernels.

> +	/* prep os */
> +	else if (argv[1][0] == 'p') {
> +		return do_bootm_linux(BOOT_OS_PREP, argc, argv, &images);
> +	}

We already had 'p' above.


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
"There was no difference between  the  behavior  of  a  god  and  the
operations of pure chance..."   - Thomas Pynchon, _Gravity's Rainbow_

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

[Jerry Van Baren]

Wolfgang Denk wrote:
> Dear Kumar Gala,
> 
> In message <1221688841-3197-1-git-send-email-galak@kernel.crashing.org> you wrote:
>> Having bootm sub-commands allows both of these as we can break up
>> the sequeunce of steps that are part of the bootm process.
> 
> OK.
> 
>> +int do_bootm_subcommand (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>> +{
>> +	int ret = 0;
>> +
>> +	/* start */
>> +	if (argv[1][0] == 's') {
> 
> I think just matching on the first letter is to restrictive; we'll end
> up with artificical command "names" that nobody can remember.

That is what the all the other command subcommands do: match on the 
minimum unique sequence, preferably one letter.

>> +	else if (argv[1][0] == 'i') {
> 
> And we need comments what all this means. WTF is 'i' ?

Ich in German :-D  (That is a bad cross-lingual pun: "ick" is what you 
say in English when you step in dog poo.)

You snipped the preceding comment line.  Granted, it was pretty terse:
> +	/* initrd relocate */

The "help" string helps:

> @@ -782,6 +883,17 @@ U_BOOT_CMD(
>  	"\tUse iminfo command to get the list of existing component\n"
>  	"\timages and configurations.\n"
>  #endif
> +	"\t\nSub-commands to do part of the bootm sequence:\n"
> +	"\tstart [addr [arg ...]]\n"
> +	"\tloados - load OS image\n"
> +	"\tprepos - OS specific prep before relocation or go\n"
> +#if defined(CONFIG_PPC) || defined(CONFIG_M68K) || defined(CONFIG_SPARC)
> +	"\tinitrd - relocate initrd, set env initrd_start/initrd_end\n"
> +#endif
> +#if defined(CONFIG_OF_LIBFDT)
> +	"\tfdt - relocate initrd\n"
> +#endif
> +	"\tgo - start os\n"
>  );

[snip]

> Best regards,
> 
> Wolfgang Denk

Ditto,
gvb

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

[Kumar Gala]

On Sep 17, 2008, at 6:57 PM, Jerry Van Baren wrote:

> Kumar Gala wrote:
>> Posting this again for discussion.  The two features I'm interested  
>> in
>> enabling are:
>> * Having the ability to modify the device tree before its passed to
>>  the kernel but after 'fdt boardsetup'
>> * Ability to do all setup but not actually jumping to the kernel.
>>  (This is useful as a way to setup the memory image [kernel, ramdisk,
>>   fdt, etc] for a different cpu than the boot one)
>> Having bootm sub-commands allows both of these as we can break up
>> the sequeunce of steps that are part of the bootm process.
>> - k
>
> Hi Kumar,
>
> Looks like a good proposal.  I've been rather distracted the last  
> couple of weeks, but I'll put some eyeball time and runtime on your  
> changes.

thanks

>> +#if 0
>> +are these really common ??? or is there any harm??
>> +	/* bd_t setup */
>> +	else if (argv[1][0] == 'p') {
>> +	}
>
> If we are using a FDT, there is no reason for a bd_t as part of the  
> boot process.  I vote for removal.
>  (typo s/p/b/?)

should be 'b'
>
>
>> +	/* cmd setup */
>> +	else if (argv[1][0] == 'c') {
>> +	}
>
> I don't know what "cmd setup" is/was off-hand, have to look into  
> that. Probably also a removal.

cmdline

> [snip]

Sounds like WD thinks we need this for supporting 2.4 kernels.

>> @@ -782,6 +883,17 @@ U_BOOT_CMD(
>> 	"\tUse iminfo command to get the list of existing component\n"
>> 	"\timages and configurations.\n"
>> #endif
>> +	"\t\nSub-commands to do part of the bootm sequence:\n"
>> +	"\tstart [addr [arg ...]]\n"
>> +	"\tloados - load OS image\n"
>> +	"\tprepos - OS specific prep before relocation or go\n"
>> +#if defined(CONFIG_PPC) || defined(CONFIG_M68K) ||  
>> defined(CONFIG_SPARC)
>> +	"\tinitrd - relocate initrd, set env initrd_start/initrd_end\n"
>> +#endif
>> +#if defined(CONFIG_OF_LIBFDT)
>> +	"\tfdt - relocate initrd\n"
>
> Cut'n'paste? s/initrd/the flattened device tree/
>
> [snip]

yep, fixed.

>> diff --git a/lib_ppc/bootm.c b/lib_ppc/bootm.c
>> index 38266e1..208ed3b 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;
>> +
>> +	kernel = (void (*)(bd_t *, ulong, ulong, ulong,
>> +			   ulong, ulong, ulong))images->ep;
>> +#ifdef CONFIG_OF_LIBFDT
>> +	char *of_flat_tree = images->ft_addr;
>> +#endif
>
> This should be moved above the "kernel = " line to keep it with the  
> rest of the declarations, yes?
>
> [big snip]

yep, fix.

- k

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

[Kumar Gala]

On Sep 18, 2008, at 1:54 AM, Wolfgang Denk wrote:

>> +int do_bootm_subcommand (cmd_tbl_t *cmdtp, int flag, int argc,  
>> char *argv[])
>> +{
>> +	int ret = 0;
>> +
>> +	/* start */
>> +	if (argv[1][0] == 's') {
>
> I think just matching on the first letter is to restrictive; we'll end
> up with artificical command "names" that nobody can remember.

I'm happy to move to using strncmp, but I agree with Jerry that other  
subcommand do the shortest match possible.

- k

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

[Kumar Gala]

>> +int do_bootm_subcommand (cmd_tbl_t *cmdtp, int flag, int argc,  
>> char *argv[])
>> +{
>> +	int ret = 0;
>> +
>> +	/* start */
>> +	if (argv[1][0] == 's') {
>
> I think just matching on the first letter is to restrictive; we'll end
> up with artificical command "names" that nobody can remember.
>
>> +	if (!images.valid) {
>> +		printf("Need to call %s start first\n", argv[0]);
>> +		return 1;
>> +	}
>
> We should probably set up a small state machine.

A small state machine for what?

>> +	else if (argv[1][0] == 'i') {
>
> And we need comments what all this means. WTF is 'i' ?

initrd, I can change it to 'r' for ramdisk if that is better.

>> +#if 0
>> +are these really common ??? or is there any harm??
>> +	/* bd_t setup */
>> +	else if (argv[1][0] == 'p') {
>> +	}
>
> Yes, we still have plenty of systems running in the field with
> arch/ppc and even with 2.4 kernels.
>
>> +	/* prep os */
>> +	else if (argv[1][0] == 'p') {
>> +		return do_bootm_linux(BOOT_OS_PREP, argc, argv, &images);
>> +	}
>
> We already had 'p' above.

- k

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

[Wolfgang Denk]

Dear Kumar Gala,

In message <6ADB3DCC-693A-41EB-BDCD-0B20865F677E@kernel.crashing.org> you wrote:
> 
> >> +	/* start */
> >> +	if (argv[1][0] == 's') {
> >
> > I think just matching on the first letter is to restrictive; we'll end
> > up with artificical command "names" that nobody can remember.
> 
> I'm happy to move to using strncmp, but I agree with Jerry that other  
> subcommand do the shortest match possible.

Agreed. The shortest unambiguous match shall be  sufficient.  On  the
other  hand  we  should  allow for longer subcopmmand names, or we'll
very soon see artifical command identifiers that nobody can  remember
well  because  they  were  artificially  chosen to use a still unused
letter.

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
I have made mistakes, but have never made the mistake of  claiming  I
never made one.                                     - James G. Bennet

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

[Wolfgang Denk]

Dear Kumar Gala,

In message <CF82B92D-9821-495A-AA5A-192B83C8619E@kernel.crashing.org> you wrote:
>
> >> +	if (!images.valid) {
> >> +		printf("Need to call %s start first\n", argv[0]);
> >> +		return 1;
> >> +	}
> >
> > We should probably set up a small state machine.
> 
> A small state machine for what?

Well, we have states - here you code that a certain operation is  not
allowed   unless   something  else  has  been  done  before.  SImilar
dependencies may follow - after uncompressing and loading the  kernel
image  it  might  for  example  be  not  allowed  any more to turn on
interrupts, etc. Of course we can  handle  all  this  in  a  maze  of
conditional statements. Or we could use a small state machine.

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
Conscious is when you are aware of something, and conscience is  when
you wish you weren't.

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

[Kumar Gala]

On Sep 22, 2008, at 4:09 PM, Wolfgang Denk wrote:

> Dear Kumar Gala,
>
> In message <CF82B92D-9821-495A- 
> AA5A-192B83C8619E at kernel.crashing.org> you wrote:
>>
>>>> +	if (!images.valid) {
>>>> +		printf("Need to call %s start first\n", argv[0]);
>>>> +		return 1;
>>>> +	}
>>>
>>> We should probably set up a small state machine.
>>
>> A small state machine for what?
>
> Well, we have states - here you code that a certain operation is  not
> allowed   unless   something  else  has  been  done  before.  SImilar
> dependencies may follow - after uncompressing and loading the  kernel
> image  it  might  for  example  be  not  allowed  any more to turn on
> interrupts, etc. Of course we can  handle  all  this  in  a  maze  of
> conditional statements. Or we could use a small state machine.

So I'm not sure I completely understand what this state machine would  
look like that you are thinking about.

I get the issue, just not seeing the implementation you are thinking  
about.

- k

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

[Wolfgang Denk]

Dear Kumar,

in message <460D947F-9265-409E-A2E7-F079C52542CF@kernel.crashing.org> you wrote:
> 
> > Well, we have states - here you code that a certain operation is  not
> > allowed   unless   something  else  has  been  done  before.  SImilar
> > dependencies may follow - after uncompressing and loading the  kernel
> > image  it  might  for  example  be  not  allowed  any more to turn on
> > interrupts, etc. Of course we can  handle  all  this  in  a  maze  of
> > conditional statements. Or we could use a small state machine.
> 
> So I'm not sure I completely understand what this state machine would  
> look like that you are thinking about.

It would describe legal state changes - each sub-command would only be
allowed in a certain set of states, and if run cause a transition into
another state.

> I get the issue, just not seeing the implementation you are thinking  
> about.

He, implementation. The details are left to the reader :-)

Sorry, I'm not even thinking about any actual code yet.

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
"We don't care.  We don't have to.  We're the Phone Company."