[U-Boot] [PATCH] arm: Add Prep subcommand support to bootm

[Andreas Bießmann]

Dear Simon,

Am Mo 29 Aug 2011 18:08:13 CEST, Simon Schwarz schrieb:
> 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

savebp? I thought this command is now 'spl' with some subcommands.

>
> Signed-off-by: Simon Schwarz <simonschwarzcor at gmail.com>
> ----

this four times '-' will not be recognized as comment section by git am

>
> V2 changes:
> nothing
>
> V3 changes:
> nothing
>
> changes after slicing this from patch
> http://article.gmane.org/gmane.comp.boot-loaders.u-boot/106422:
> DEL Prototype declaration - not necessary if reordered
> DEL Most #ifdefs - relying on -ffunction-sections and --gc-sections to handle unused
> CHG reorganized bootm. powerpc implementation was the model
> ADD get_board_serial fake implementation - this is needed if setup_serial_tag
> 	is compiled but the board doesn't support it - tradeoff for removing
> 	#ifdefs
> ---
>  arch/arm/lib/bootm.c |  330 +++++++++++++++++++++++++------------------------
>  1 files changed, 168 insertions(+), 162 deletions(-)
>
> diff --git a/arch/arm/lib/bootm.c b/arch/arm/lib/bootm.c
> index 802e833..5f112e2 100644
> --- a/arch/arm/lib/bootm.c
> +++ b/arch/arm/lib/bootm.c
> @@ -1,4 +1,8 @@
> -/*
> +/* Copyright (C) 2011
> + * Corscience GmbH & Co. KG - Simon Schwarz <schwarz at corscience.de>
> + *  - Added prep subcommand support
> + *  - Reorganized source - modeled after powerpc version
> + *
>   * (C) Copyright 2002
>   * Sysgo Real-Time Solutions, GmbH <www.elinos.com>
>   * Marius Groeger <mgroeger at sysgo.de>
> @@ -32,31 +36,16 @@
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> -#if defined (CONFIG_SETUP_MEMORY_TAGS) || \
> -    defined (CONFIG_CMDLINE_TAG) || \
> -    defined (CONFIG_INITRD_TAG) || \
> -    defined (CONFIG_SERIAL_TAG) || \
> -    defined (CONFIG_REVISION_TAG)
> -static void setup_start_tag (bd_t *bd);
> -
> -# ifdef CONFIG_SETUP_MEMORY_TAGS
> -static void setup_memory_tags (bd_t *bd);
> -# endif
> -static void setup_commandline_tag (bd_t *bd, char *commandline);
> -
> -# ifdef CONFIG_INITRD_TAG
> -static void setup_initrd_tag (bd_t *bd, ulong initrd_start,
> -			      ulong initrd_end);
> -# endif
> -static void setup_end_tag (bd_t *bd);
> -
> +static void (*kernel_entry)(int zero, int arch, uint params);

NAK (see later on)

>  static struct tag *params;
> -#endif /* CONFIG_SETUP_MEMORY_TAGS || CONFIG_CMDLINE_TAG || CONFIG_INITRD_TAG */
>  
> -static ulong get_sp(void);
> -#if defined(CONFIG_OF_LIBFDT)
> -static int bootm_linux_fdt(int machid, bootm_headers_t *images);
> -#endif
> +static ulong get_sp(void)
> +{
> +	ulong ret;
> +
> +	asm("mov %0, sp" : "=r"(ret) : );
> +	return ret;
> +}
>  
>  void arch_lmb_reserve(struct lmb *lmb)
>  {
> @@ -80,85 +69,6 @@ void arch_lmb_reserve(struct lmb *lmb)
>  		    gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size - sp);
>  }
>  
> -static void announce_and_cleanup(void)
> -{
> -	printf("\nStarting kernel ...\n\n");
> -
> -#ifdef CONFIG_USB_DEVICE
> -	{
> -		extern void udc_disconnect(void);
> -		udc_disconnect();
> -	}
> -#endif
> -	cleanup_before_linux();
> -}

I can not see why git decided to remove that here and add it later on ..
did you change anything in announce_and_cleanup()?

> -
> -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);
> -
> -#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);
> -
> -#ifdef CONFIG_OF_LIBFDT
> -	if (images->ft_len)
> -		return bootm_linux_fdt(machid, images);
> -#endif
> -
> -	kernel_entry = (void (*)(int, int, uint))images->ep;
> -
> -	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);
> -#ifdef CONFIG_SERIAL_TAG
> -	setup_serial_tag (&params);
> -#endif
> -#ifdef CONFIG_REVISION_TAG
> -	setup_revision_tag (&params);
> -#endif
> -#ifdef CONFIG_SETUP_MEMORY_TAGS
> -	setup_memory_tags (bd);
> -#endif
> -#ifdef CONFIG_CMDLINE_TAG
> -	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);
> -#endif
> -	setup_end_tag(bd);
> -#endif
> -
> -	announce_and_cleanup();
> -
> -	kernel_entry(0, machid, bd->bi_boot_params);
> -	/* does not return */
> -
> -	return 1;
> -}
> -
> -#if defined(CONFIG_OF_LIBFDT)
>  static int fixup_memory_node(void *blob)
>  {
>  	bd_t	*bd = gd->bd;
> @@ -174,54 +84,19 @@ 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 void announce_and_cleanup(void)
>  {
> -	ulong rd_len;
> -	void (*kernel_entry)(int zero, int dt_machid, void *dtblob);
> -	ulong of_size = images->ft_len;
> -	char **of_flat_tree = &images->ft_addr;
> -	ulong *initrd_start = &images->initrd_start;
> -	ulong *initrd_end = &images->initrd_end;
> -	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);
> -
> -	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;
> -
> -	ret = boot_relocate_fdt(lmb, of_flat_tree, &of_size);
> -	if (ret)
> -		return ret;
> -
> -	debug("## Transferring control to Linux (at address %08lx) ...\n",
> -	       (ulong) kernel_entry);
> -
> -	fdt_chosen(*of_flat_tree, 1);
> -
> -	fixup_memory_node(*of_flat_tree);
> -
> -	fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
> -
> -	announce_and_cleanup();
> -
> -	kernel_entry(0, machid, *of_flat_tree);
> -	/* does not return */
> +	printf("\nStarting kernel ...\n\n");
>  
> -	return 1;
> -}
> +#ifdef CONFIG_USB_DEVICE
> +	{
> +		extern void udc_disconnect(void);
> +		udc_disconnect();
> +	}
>  #endif
> +	cleanup_before_linux();
> +}
>  
> -#if defined (CONFIG_SETUP_MEMORY_TAGS) || \
> -    defined (CONFIG_CMDLINE_TAG) || \
> -    defined (CONFIG_INITRD_TAG) || \
> -    defined (CONFIG_SERIAL_TAG) || \
> -    defined (CONFIG_REVISION_TAG)
>  static void setup_start_tag (bd_t *bd)
>  {
>  	params = (struct tag *) bd->bi_boot_params;
> @@ -237,7 +112,6 @@ static void setup_start_tag (bd_t *bd)
>  }
>  
>  
> -#ifdef CONFIG_SETUP_MEMORY_TAGS
>  static void setup_memory_tags (bd_t *bd)
>  {
>  	int i;
> @@ -252,8 +126,6 @@ static void setup_memory_tags (bd_t *bd)
>  		params = tag_next (params);
>  	}
>  }
> -#endif /* CONFIG_SETUP_MEMORY_TAGS */
> -
>  
>  static void setup_commandline_tag (bd_t *bd, char *commandline)
>  {
> @@ -280,8 +152,6 @@ static void setup_commandline_tag (bd_t *bd, char *commandline)
>  	params = tag_next (params);
>  }
>  
> -
> -#ifdef CONFIG_INITRD_TAG
>  static void setup_initrd_tag (bd_t *bd, ulong initrd_start, ulong initrd_end)
>  {
>  	/* an ATAG_INITRD node tells the kernel where the compressed
> @@ -295,9 +165,18 @@ static void setup_initrd_tag (bd_t *bd, ulong initrd_start, ulong initrd_end)
>  
>  	params = tag_next (params);
>  }
> -#endif /* CONFIG_INITRD_TAG */
>  
> -#ifdef CONFIG_SERIAL_TAG
> +/* This is a workaround - if boards don't implement
> + * get_board_serial */
> +__attribute__((weak))
> +void get_board_serial(struct tag_serialnr *serialnr)
> +{
> +	printf("This board does not implement get_board_serial() \
> +		but calls it serialnr is filled with junk!\n");

WARNING: Avoid line continuations in quoted strings
#282: FILE: arch/arm/lib/bootm.c:174:
+	printf("This board does not implement get_board_serial() \

> +	serialnr->high = 0xABCDF;
> +	serialnr->low = 0xABCDF;
> +}

This was not mentioned in commit message, please split this into another
patch (if really required). I vote for 'remove that snipped completely'
cause we should see at compiletime that this funtion is mising.

BTW: you could remove the forward declaration for 'void
get_board_serial(struct tag_serialnr *serialnr)' from setup_serial_tag()
if you implement it some lines above.

> +
>  void setup_serial_tag (struct tag **tmp)

------------------------^
Remove whitespace between function name and parameter list (fix globally
when you edit that file).

>  {
>  	struct tag *params = *tmp;
> @@ -312,9 +191,7 @@ void setup_serial_tag (struct tag **tmp)
>  	params = tag_next (params);
>  	*tmp = params;
>  }
> -#endif
>  
> -#ifdef CONFIG_REVISION_TAG
>  void setup_revision_tag(struct tag **in_params)
>  {
>  	u32 rev = 0;
> @@ -326,19 +203,148 @@ void setup_revision_tag(struct tag **in_params)
>  	params->u.revision.rev = rev;
>  	params = tag_next (params);
>  }
> -#endif  /* CONFIG_REVISION_TAG */
>  
>  static void setup_end_tag (bd_t *bd)
>  {
>  	params->hdr.tag = ATAG_NONE;
>  	params->hdr.size = 0;
>  }
> -#endif /* CONFIG_SETUP_MEMORY_TAGS || CONFIG_CMDLINE_TAG || CONFIG_INITRD_TAG */
>  
> -static ulong get_sp(void)
> +static void create_atags(bootm_headers_t *images)
>  {
> -	ulong ret;
> +	bd_t	*bd = gd->bd;

no tab here ^

> +	char *commandline = getenv("bootargs");
>  
> -	asm("mov %0, sp" : "=r"(ret) : );
> -	return ret;
> +	setup_start_tag(bd);
> +#ifdef CONFIG_SERIAL_TAG
> +	setup_serial_tag(&params);
> +#endif
> +#ifdef CONFIG_CMDLINE_TAG
> +	setup_commandline_tag(gd->bd, commandline);
> +#endif
> +#ifdef CONFIG_REVISION_TAG
> +	setup_revision_tag(&params);
> +#endif
> +#ifdef CONFIG_SETUP_MEMORY_TAGS
> +	setup_memory_tags(bd);
> +#endif
> +#ifdef CONFIG_INITRD_TAG
> +	if (images->rd_start && images->rd_end)
> +		setup_initrd_tag(bd, images->rd_start, images->rd_end);
> +#endif
> +	setup_end_tag(bd);
> +}
> +
> +static int create_fdt(bootm_headers_t *images)
> +{
> +	ulong of_size = images->ft_len;
> +	char **of_flat_tree = &images->ft_addr;
> +	ulong *initrd_start = &images->initrd_start;
> +	ulong *initrd_end = &images->initrd_end;
> +	struct lmb *lmb = &images->lmb;
> +	ulong rd_len;
> +	int ret;
> +
> +	debug("using: FDT\n");
> +
> +	boot_fdt_add_mem_rsv_regions(lmb, *of_flat_tree);
> +
> +	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;
> +
> +	ret = boot_relocate_fdt(lmb, of_flat_tree, &of_size);
> +	if (ret)
> +		return ret;
> +
> +	fdt_chosen(*of_flat_tree, 1);
> +	fixup_memory_node(*of_flat_tree);
> +	fdt_initrd(*of_flat_tree, *initrd_start, *initrd_end, 1);
> +
> +	return 0;
> +}
> +
> +/* Subcommand: CMDLINE */
> +static int boot_cmdline_linux(bootm_headers_t *images)
> +{
> +	return -1; /* not implemented */
> +}
> +
> +/* Subcommand: BDT */
> +static int boot_bd_t_linux(bootm_headers_t *images)
> +{
> +	return -1; /* not implemented */

Shouldn't that return 0 for 'no error' even if not implemented?

> +}
> +
> +/* Subcommand: PREP */
> +static void boot_prep_linux(bootm_headers_t *images)
> +{
> +#ifdef CONFIG_OF_LIBFDT
> +	if (images->ft_len) {
> +		debug("using: FDT\n");
> +		if (create_fdt(images)) {
> +			printf("FDT creation failed! hanging...");
> +			hang();
> +		}
> +	} else
> +#endif
> +	{
> +#if defined(CONFIG_SETUP_MEMORY_TAGS) || \
> +	defined(CONFIG_CMDLINE_TAG) || \
> +	defined(CONFIG_INITRD_TAG) || \
> +	defined(CONFIG_SERIAL_TAG) || \
> +	defined(CONFIG_REVISION_TAG)
> +		debug("using: ATAGS\n");
> +		create_atags(images);

I can not see any reason why we should keep create_atags() as another
function here, I think it is cleaner to move the content of
create_atags() to boot_prep_linux() and remove this large list of
requirements for create_atags().

> +#else
> +	printf("FDT and ATAGS failed - hanging\n");

Wrong text here, only FDT did fail, ATAGS where not defined at build time.

> +	hang();
> +#endif
> +	}
> +
> +	kernel_entry = (void (*)(int, int, uint))images->ep;

NAK, setting (and using) kernel_entry is part of GO subcommand.

> +}
> +
> +/* Subcommand: GO */
> +static void boot_jump_linux(bootm_headers_t *images)
> +{
> +	int	machid = gd->bd->bi_arch_number;
> +	char	*s;

No tab here ^

Declare kernel_entry function pointer here.

> +	s = getenv("machid");
> +	if (s) {
> +		strict_strtoul(s, 16, (long unsigned int *) &machid);

How about strict_strtoul() returning something wrong?

> +		debug("Using machid 0x%x from environment\n", machid);

Yoiu should use printf() here as it was bfore so one could see that fact
when booting.

> +	}
> +

Set kernel_entry function pointer here.

> +	debug("Using machid 0x%x from bd\n", machid);

This statement is wrong ... you need to set this in an else statement.
Or reword the content. How about:

debug("Using machid 0x%x\n", machid); ?

> +	debug("## Transferring control to Linux (at address %08lx)" \
> +		"...\n", (ulong) kernel_entry);

Use kernel_entry function pointer here ...

> +	show_boot_progress(15);
> +	announce_and_cleanup();
> +	kernel_entry(0, machid, gd->bd->bi_boot_params);

and here.

> +}
> +
> +/* Main Entry point for arm bootm implementation
> + *
> + * Modeled after the powerpc implementation
> + * DIFFERENCE: Instead of calling prep and go at the end
> + * they are called if subommand is equal 0.

s/subommand/subcommand/

> + */
> +int do_bootm_linux(int flag, int argc, char *argv[], bootm_headers_t *images)
> +{
> +	if (flag & BOOTM_STATE_OS_CMDLINE)
> +		boot_cmdline_linux(images);
> +
> +	if (flag & BOOTM_STATE_OS_BD_T)
> +		boot_bd_t_linux(images);

NAK, remove these two functions. Since the ARM linux boot requirements
are different to powerpc we do not need these two states of bootm at all.

The powerpc entry_32.S (in linux) show they need commandline pointer
apart from 'residual board info' pointer. The arm implementation in
head.S (also linux source) says:

---8<---
 * This is normally called from the decompressor code.  The requirements
 * are: MMU = off, D-cache = off, I-cache = dont care, r0 = 0,
 * r1 = machine nr, r2 = atags or dtb pointer.
--->8---

For arm we do not need to prepare the cmdline apart from 'bd_t', we just
need to setup the ATAGS (ord FDT) which contains all information we
need. This could all be done in the prep state.

> +
> +	if (flag & BOOTM_STATE_OS_PREP || flag == 0)
> +		boot_prep_linux(images);
> +
> +	if (flag & BOOTM_STATE_OS_GO || flag == 0)
> +		boot_jump_linux(images);
> +
> +	return 0;
>  }

NAK, the ppc implementation does here

if (flag & FLAG) {
  do_flag_specific()
  return
}
...
do whatever to do without any flag set

which seems much cleaner to me.

I personally dislike the 'if specific flag set or no flag set then do
...' logic here.

best regards

Andreas Bießmann