From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerry Van Baren Date: Tue, 29 May 2007 21:46:01 -0400 Subject: [U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c In-Reply-To: <20070529160507.7ea8af20.kim.phillips@freescale.com> References: <20070526125450.GD626@cideas.com> <20070529160507.7ea8af20.kim.phillips@freescale.com> Message-ID: <465CD759.9090303@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 Kim Phillips wrote: > On Sat, 26 May 2007 08:54:50 -0400 > Jerry Van Baren wrote: > >> Removed redundant calls to fdt_chosen(), fdt_env(), and fdt_bd_t() >> Fixed most overlength lines. Some lines remain longer than 80 characters, >> but it isn't obvious to the author how to wrap them in a readable way. >> >> Signed-off-by: Gerald Van Baren >> --- >> common/cmd_bootm.c | 98 ++++++++++++++++++++++++++++++++++----------------- >> 1 files changed, 65 insertions(+), 33 deletions(-) >> >> diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c >> index 25b9d74..786ff6e 100644 >> --- a/common/cmd_bootm.c >> +++ b/common/cmd_bootm.c >> @@ -514,6 +514,19 @@ fixup_silent_linux () >> } >> #endif /* CONFIG_SILENT_CONSOLE */ >> >> +/* >> + * Some FDT-related defines to reduce clutter in the main code. >> + */ >> +#if defined(CONFIG_OF_FLAT_TREE) >> +#define FDT_VALIDATE \ >> + (*((ulong *)(of_flat_tree + sizeof(image_header_t))) != OF_DT_HEADER) >> +#endif >> +#if defined(CONFIG_OF_LIBFDT) >> +#define FDT_VALIDATE \ >> + (fdt_check_header(of_flat_tree + sizeof(image_header_t)) != 0) >> +#endif >> + > > I'd actually prefer to see what the code is doing, where it's doing it. > Please read linux-2.6/Documentation/CodingStyle, chapter 12, while > you're at it. My intention is to removed CONFIG_OF_FLAT_TREE once CONFIG_OF_LIBFDT is stable and all the boards have been converted over. CONFIG_OF_LIBFDT makes CONFIG_OF_FLAT_TREE obsolete, but we have are straddling the fence at the moment (ouch). The above #define makes a rather horrible #if: #if defined(CONFIG_OF_LIBFDT) if (fdt_check_header(of_flat_tree) == 0) { #else if (*(ulong *)of_flat_tree == OF_DT_HEADER) { #endif into a readable one: if (FDT_VALIDATE) { 1) The horrible #if is repeated in 3 places, so it is 9x ugly 2) When CONFIG_OF_FLAT_TREE is retired, the #define will be removed and since it will then be a simple expression: if (fdt_check_header(of_flat_tree) == 0) { >> + >> #ifdef CONFIG_PPC >> static void __attribute__((noinline)) >> do_bootm_linux (cmd_tbl_t *cmdtp, int flag, >> @@ -748,11 +761,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag, >> if(argc > 3) { >> of_flat_tree = (char *) simple_strtoul(argv[3], NULL, 16); >> hdr = (image_header_t *)of_flat_tree; >> -#if defined(CONFIG_OF_LIBFDT) >> - if (fdt_check_header(of_flat_tree) == 0) { >> -#else >> - if (*(ulong *)of_flat_tree == OF_DT_HEADER) { >> -#endif >> + if (FDT_VALIDATE) { >> #ifndef CFG_NO_FLASH >> if (addr2info((ulong)of_flat_tree) != NULL) >> of_data = (ulong)of_flat_tree; >> @@ -763,7 +772,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag, >> >> if ((ntohl(hdr->ih_load) < ((unsigned long)hdr + ntohl(hdr->ih_size) + sizeof(hdr))) && >> ((ntohl(hdr->ih_load) + ntohl(hdr->ih_size)) > (unsigned long)hdr)) { >> - puts ("ERROR: Load address overwrites Flat Device Tree uImage\nMust RESET board to recover\n"); >> + puts ("ERROR: Load address overwrites the " >> + "Flat Device Tree uImage.\n" >> + "Must RESET the board to recover.\n"); > > you have overly verbose error messages. How's something simple like > "dt image overwritten - reset the board." ? Gets the point across, saves > some readability in the code. Yeah, the messages got longer as I edited and debugged. ;-) An example of an original error message format: puts ("GUNZIP ERROR - must RESET board to recover\n"); (lots of shouting because it is a Very Bad Thing[tm) so I would propose messages on the order of puts ("FDT overwritten - must RESET board to recover\n"); Question for Wolfgang D: It looks like the error messages originally only used puts() and, I would speculate, printf()s were added later. I'm deducing this from the original, the CONFIG_BZIP2 addition does a printf() on the error: 369 if (i != BZ_OK) { 370 printf ("BUNZIP2 ERROR %d - must RESET board to recover\n", i); 371 SHOW_BOOT_PROGRESS (-6); 372 udelay(100000); 373 do_reset (cmdtp, flag, argc, argv); 374 } Is printf() safe in this delicate condition? Should I strip all printf()s _that are used in the "delicate" section_ from the file (losing some diagnostic information)? Should I strip out the udelay()s too? As you pointed out previously, udelay() is not safe on some boards that use interrupts to measure the delay. I feel another, separate, cleanup patch coming on. :-/ [snip the dittos] >> #if defined(CONFIG_OF_FLAT_TREE) >> + /* >> + * Create the /chosen node and modify the blob with board specific >> + * values as needed. >> + */ >> ft_setup(of_flat_tree, kbd, initrd_start, initrd_end); >> /* ft_dump_blob(of_flat_tree); */ >> #endif >> #if defined(CONFIG_OF_LIBFDT) >> + /* >> + * Create the /chosen node and modify the blob with board specific >> + * values as needed. >> + */ > > duplicate comment; why not hoist it up? See intro remarks about the plan to remove CONFIG_OF_FLAT_TREE, however in this case I could hoist it up as you point out with a net gain. Sometimes one is blinded by his focus on the goal. ;-) [snip] > > Kim Thanks, gvb