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

Simon Schwarz simonschwarzcor at googlemail.com
Mon Sep 5 16:06:37 CEST 2011


Dear Andreas,

On 09/05/2011 12:58 PM, Andreas Bießmann wrote:
> 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.
fixed.
>
>>
>> Signed-off-by: Simon Schwarz<simonschwarzcor at gmail.com>
>> ----
>
> this four times '-' will not be recognized as comment section by git am
>
fixed
>>
>> 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)
done.
>
>>   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()?

Nope. I added something before and there were major changes in the file 
(moved large codejunks around)- not the best environment for a pretty 
diff i guess...
>
>> -
>> -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() \
>
Hmmm. This warning is not emitted with my checkpatch.pl (V 0.31)

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

I readded the #ifdef CONFIG_SERIAL_TAG. This avoids the above
problems.
>> +
>>   void setup_serial_tag (struct tag **tmp)
>
> ------------------------^
> Remove whitespace between function name and parameter list (fix globally
> when you edit that file).
done.
>
>>   {
>>   	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 ^
>
done.
>> +	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?
>

common/bootm.c calls subcommands like this:
case BOOTM_STATE_OS_BD_T:
   ret = boot_fn(BOOTM_STATE_OS_BD_T, argc, argv, &images);
     if (ret)
            printf ("bdt subcommand not supported\n");
     break;

So I assume a value different from zero is interpreted as not 
implemented. But as I just saw these returns were never returned to 
common/bootm.c ...

Anyway, removed theses functions.

>> +}
>> +
>> +/* 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().

My reasons to do it that way:
- It is similar to create_fdt - duno if it is just me but I see this as 
more consistent
- The requirements list will be there anyway setup_start_tag and 
setup_end_tag have to be protected. The only way to make it smaller
would be a macro doing the same.

I have no strong feeling about how to do that but IMHO it is better 
readable now - if there are more votes for moving this to 
boot_prep_linux i will do it.

>
>> +#else
>> +	printf("FDT and ATAGS failed - hanging\n");
>
> Wrong text here, only FDT did fail, ATAGS where not defined at build time.
>
Actually it is that both aren't defined because the FDT has it's own 
hang in case of failure. So I will change to "FDT and ATAGS support not 
compiled in - hanging".

>> +	hang();
>> +#endif
>> +	}
>> +
>> +	kernel_entry = (void (*)(int, int, uint))images->ep;
>
> NAK, setting (and using) kernel_entry is part of GO subcommand.
>

changed.

>> +}
>> +
>> +/* Subcommand: GO */
>> +static void boot_jump_linux(bootm_headers_t *images)
>> +{
>> +	int	machid = gd->bd->bi_arch_number;
>> +	char	*s;
>
> No tab here ^
changed.
>
> Declare kernel_entry function pointer here.
done.
>
>> +	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.
Hm. I considered it too noisy since the the machid is normally not 
displayed on boot. Will change anyway...

>
>> +	}
>> +
>
> 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); ?
>
Ha, this was a leftover from debugging - I deleted it.

>> +	debug("## Transferring control to Linux (at address %08lx)" \
>> +		"...\n", (ulong) kernel_entry);
>
> Use kernel_entry function pointer here ...
>
>> +	show_boot_progress(15);1
>> +	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/
done
>
>> + */
>> +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.
>

changed.

But they are shown in bootm help message regardles of the architecture. 
Shouldn't we add #ifdefs in the help message then? (or, and I really 
hate to bring this up, change the way the helpmessage is created if the
function is arch dependent)

>> +
>> +	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.
>
I already changed this. Just waited for more comments to send in a new 
version.
> best regards
>
> Andreas Bießmann

Regards & thx
Simon


More information about the U-Boot mailing list