[U-Boot] [PATCH V3 1/8] arm: Add Prep subcommand support to bootm

[Andreas Bießmann]

Dear Simon,

Am 25.08.2011 10:33, schrieb Simon Schwarz:
> Adds prep subcommand to bootm implementation of ARM. When bootm is called with
> the subcommand prep the function stops right after ATAGS creation and before
> announce_and_cleanup.
> 
> This is used in savebp command
> 
> Signed-off-by: Simon Schwarz <simonschwarzcor at gmail.com>
> ----
> 
> V2 changes:
> nothing
> 
> V3 changes:
> nothing
> ---
>  arch/arm/lib/bootm.c |  116 +++++++++++++++++++++++++++----------------------
>  common/cmd_bootm.c   |    2 +-
>  2 files changed, 65 insertions(+), 53 deletions(-)
> 
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 802e833..d3152ae 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -1,4 +1,7 @@
> -/*
> +/* Copyright (C) 2011
> + * Corscience GmbH & Co. KG - Simon Schwarz <schwarz at corscience.de>
> + *  - Added prep subcommand support
> + *
>   * (C) Copyright 2002
>   * Sysgo Real-Time Solutions, GmbH <www.elinos.com>
>   * Marius Groeger <mgroeger at sysgo.de>
> @@ -55,7 +58,7 @@ static struct tag *params;
>  
>  static ulong get_sp(void);
>  #if defined(CONFIG_OF_LIBFDT)
> -static int bootm_linux_fdt(int machid, bootm_headers_t *images);
> +static int bootm_linux_fdt(int machid, bootm_headers_t *images, int flag);
>  #endif
>  
>  void arch_lmb_reserve(struct lmb *lmb)
> @@ -98,63 +101,67 @@ int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
>  	bd_t	*bd = gd->bd;
>  	char	*s;
>  	int	machid = bd->bi_arch_number;
> -	void	(*kernel_entry)(int zero, int arch, uint params);
> +	void	(*kernel_entry)(int zero, int arch, uint params) = NULL;

This should not be necessary, kernel_entry would be on bss which should
be initialized to zero by start.S.
>  
>  #ifdef CONFIG_CMDLINE_TAG
>  	char *commandline = getenv ("bootargs");
>  #endif
> -
> -	if ((flag != 0) && (flag != BOOTM_STATE_OS_GO))
> -		return 1;
> -
> -	s = getenv ("machid");
> -	if (s) {
> -		machid = simple_strtoul (s, NULL, 16);
> -		printf ("Using machid 0x%x from environment\n", machid);
> -	}
> -
> -	show_boot_progress (15);
> +	if ((flag != 0) && (!(flag & BOOTM_STATE_OS_GO ||
> +	 flag & BOOTM_STATE_OS_PREP)))

switch'n'case would be much cleaner here. And seperating the
functionality into functions would be nice too.

> +		return 1; /* subcommand not implemented */
> +	else if ((flag == 0) || flag & BOOTM_STATE_OS_PREP) {
> +		s = getenv("machid");
> +		if (s) {
> +			strict_strtoul(s, 16, (long unsigned int *) &machid);
> +			printf("Using machid 0x%x from environment\n", machid);
> +		}
> +
> +		show_boot_progress(15);
>  
>  #ifdef CONFIG_OF_LIBFDT
> -	if (images->ft_len)
> -		return bootm_linux_fdt(machid, images);
> +		if (images->ft_len)
> +			return bootm_linux_fdt(machid, images, flag);
>  #endif
>  
> -	kernel_entry = (void (*)(int, int, uint))images->ep;
> +		kernel_entry = (void (*)(int, int, uint))images->ep;
>  
> -	debug ("## Transferring control to Linux (at address %08lx) ...\n",
> -	       (ulong) kernel_entry);
> +		debug("## Transferring control to Linux (at address %08lx)" \
> +			"...\n", (ulong) kernel_entry);
>  
>  #if defined (CONFIG_SETUP_MEMORY_TAGS) || \
>      defined (CONFIG_CMDLINE_TAG) || \
>      defined (CONFIG_INITRD_TAG) || \
>      defined (CONFIG_SERIAL_TAG) || \
>      defined (CONFIG_REVISION_TAG)
> -	setup_start_tag (bd);
> +		setup_start_tag(bd);
>  #ifdef CONFIG_SERIAL_TAG
> -	setup_serial_tag (&params);
> +		setup_serial_tag(&params);
>  #endif
>  #ifdef CONFIG_REVISION_TAG
> -	setup_revision_tag (&params);
> +		setup_revision_tag(&params);
>  #endif
>  #ifdef CONFIG_SETUP_MEMORY_TAGS
> -	setup_memory_tags (bd);
> +		setup_memory_tags(bd);
>  #endif
>  #ifdef CONFIG_CMDLINE_TAG
> -	setup_commandline_tag (bd, commandline);
> +		setup_commandline_tag(bd, commandline);
>  #endif
>  #ifdef CONFIG_INITRD_TAG
> -	if (images->rd_start && images->rd_end)
> -		setup_initrd_tag (bd, images->rd_start, images->rd_end);
> +		if (images->rd_start && images->rd_end)
> +			setup_initrd_tag(bd, images->rd_start, images->rd_end);
>  #endif
> -	setup_end_tag(bd);
> +		setup_end_tag(bd);
>  #endif
> +		if (flag & BOOTM_STATE_OS_PREP)
> +			return 0;
> +	}
>  
> -	announce_and_cleanup();
> -
> -	kernel_entry(0, machid, bd->bi_boot_params);
> -	/* does not return */
> +	if (flag == 0 || flag & BOOTM_STATE_OS_GO) {

flag == 0? Shouldn't that be flag == BOOTM_STATE_OS_GO?

> +		announce_and_cleanup();
>  
> +		kernel_entry(0, machid, bd->bi_boot_params);
> +		/* does not return */
> +	}
>  	return 1;
>  }
>  
> @@ -174,10 +181,10 @@ static int fixup_memory_node(void *blob)
>  	return fdt_fixup_memory_banks(blob, start, size, CONFIG_NR_DRAM_BANKS);
>  }
>  
> -static int bootm_linux_fdt(int machid, bootm_headers_t *images)
> +static int bootm_linux_fdt(int machid, bootm_headers_t *images, int flag)
>  {
>  	ulong rd_len;
> -	void (*kernel_entry)(int zero, int dt_machid, void *dtblob);
> +	void (*kernel_entry)(int zero, int dt_machid, void *dtblob) = NULL;

same as above.

>  	ulong of_size = images->ft_len;
>  	char **of_flat_tree = &images->ft_addr;
>  	ulong *initrd_start = &images->initrd_start;
> @@ -185,34 +192,39 @@ static int bootm_linux_fdt(int machid, bootm_headers_t *images)
>  	struct lmb *lmb = &images->lmb;
>  	int ret;
>  
> -	kernel_entry = (void (*)(int, int, void *))images->ep;
> -
> -	boot_fdt_add_mem_rsv_regions(lmb, *of_flat_tree);
> +	if ((flag == 0) || flag & BOOTM_STATE_OS_PREP) {
> +		kernel_entry = (void (*)(int, int, void *))images->ep;
>  

same as above, a cleaner aproach is to gather information for fdt in a
dedicated function and in another do the hand-over to linux kernel.

> -	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;
> +		boot_fdt_add_mem_rsv_regions(lmb, *of_flat_tree);
>  
> -	ret = boot_relocate_fdt(lmb, of_flat_tree, &of_size);
> -	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;
>  
> -	debug("## Transferring control to Linux (at address %08lx) ...\n",
> -	       (ulong) kernel_entry);
> +		ret = boot_relocate_fdt(lmb, of_flat_tree, &of_size);
> +		if (ret)
> +			return ret;
>  
> -	fdt_chosen(*of_flat_tree, 1);
> +		debug("## Transferring control to Linux (at address %08lx)" \
> +			"...\n", (ulong) kernel_entry);
>  
> -	fixup_memory_node(*of_flat_tree);
> +		fdt_chosen(*of_flat_tree, 1);
>  
> -	fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
> +		fixup_memory_node(*of_flat_tree);
>  
> -	announce_and_cleanup();
> +		fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
>  
> -	kernel_entry(0, machid, *of_flat_tree);
> -	/* does not return */
> +		if (flag & BOOTM_STATE_OS_PREP)
> +			return 0;
> +	}
> +	if (flag == 0 || flag & BOOTM_STATE_OS_GO) {
> +		announce_and_cleanup();
>  
> +		kernel_entry(0, machid, *of_flat_tree);
> +		/* does not return */
> +	}
>  	return 1;
>  }
>  #endif
> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
> index 1966da4..c642299 100644
> --- a/common/cmd_bootm.c
> +++ b/common/cmd_bootm.c
> @@ -156,7 +156,7 @@ static boot_os_fn *boot_os[] = {
>  #endif
>  };
>  
> -static bootm_headers_t images;		/* pointers to os/initrd/fdt images */
> +bootm_headers_t images;		/* pointers to os/initrd/fdt images */

is this necessary?

>  
>  /* Allow for arch specific config before we boot */
>  void __arch_preboot_os(void)

regards

Andreas Bießmann