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

[Simon Schwarz <simonschwarzcor@googlemail.com>]

Dear Andreas,

On 08/25/2011 11:40 AM, Andreas Bie?mann wrote:
> 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@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@corscience.de>
>> + *  - Added prep subcommand support
>> + *
>>    * (C) Copyright 2002
>>    * Sysgo Real-Time Solutions, GmbH<www.elinos.com>
>>    * Marius Groeger<mgroeger@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.

I added this because of compiler warnings that it could be used 
uninitialized.

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

Hehe. Somehow I did know that this topic will come up. I intended to 
change the code as little as possible.

So essentially this would mean to rewrite this to reflect the structure 
of the ppc version. Will do if there are no objections.

If there are no objections I also would like to separate this patch from 
this series. This has some advantages:
- Support for the prep subcommand is essential for saving the boot 
parameters. (if prep is in saving can also be done manually)
- I think that there won't be much discussion about the usefulness of 
implementing this - just some about the how.

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

flag = 0 means that no subcommand was issued -> execute everything.

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

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

If no objections - will be done in the rewrite.

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

This is done to be able to get the fdt address in arch-dependent savebp 
cmd. Suggestions for a cleaner solution are welcome ;)

>
>>
>>   /* Allow for arch specific config before we boot */
>>   void __arch_preboot_os(void)
>
> regards
>
> Andreas Bie?mann

Regards, thx for reviewing
Simon