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 <simonschwarz...@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 <schw...@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 <mgroe...@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 (¶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() \ > + 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(¶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? > +} > + > +/* 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
