[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 (¶ms);
>> -#endif
>> -#ifdef CONFIG_REVISION_TAG
>> - setup_revision_tag (¶ms);
>> -#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(¶ms);
>> +#endif
>> +#ifdef CONFIG_CMDLINE_TAG
>> + setup_commandline_tag(gd->bd, commandline);
>> +#endif
>> +#ifdef CONFIG_REVISION_TAG
>> + setup_revision_tag(¶ms);
>> +#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