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

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