From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Schwarz Date: Mon, 05 Sep 2011 16:06:37 +0200 Subject: [U-Boot] [PATCH] arm: Add Prep subcommand support to bootm In-Reply-To: <4E64AB6D.9040501@gmail.com> References: <1314634093-8494-1-git-send-email-simonschwarzcor@gmail.com> <4E64AB6D.9040501@gmail.com> Message-ID: <4E64D76D.6000705@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >> ---- > > 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 >> + * - Added prep subcommand support >> + * - Reorganized source - modeled after powerpc version >> + * >> * (C) Copyright 2002 >> * Sysgo Real-Time Solutions, GmbH >> * Marius Groeger >> @@ -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