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

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 Schwarzsimonschwarzcor@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 Schwarzschwarz@corscience.de
• • • Added prep subcommand support
• • • Reorganized source - modeled after powerpc version
• • (C) Copyright 2002
  • Sysgo Real-Time Solutions, GmbH<www.elinos.com>
  • Marius Groegermgroeger@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