Kim Phillips wrote:
On Sat, 26 May 2007 08:54:50 -0400 Jerry Van Baren gvb.uboot@gmail.com 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 vanbaren@cideas.com
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.*/#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