More clean up of cmd_bootm.c

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 <[email protected]>

---
 common/cmd_bootm.c |   98 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 65 insertions(+), 33 deletions(-)

common/cmd_bootm.c +2 -0

index 25b9d74..786ff6e 100644
@@ -514,6 +514,19 @@ fixup_silent_linux ()
 }
 #endif /* CONFIG_SILENT_CONSOLE */
 
+/*
+ * Some FDT-related defines to reduce clutter in the main code.

common/cmd_bootm.c +2 -5

+
 #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;

common/cmd_bootm.c +2 -1

 				do_reset (cmdtp, flag, argc, argv);
 			}
 			if (ntohl(hdr->ih_comp) != IH_COMP_NONE) {
-				puts ("ERROR: uImage is not uncompressed\nMust RESET board to recover\n");
+				puts ("ERROR: uImage is not uncompressed.\n"
+					"Must RESET the board to recover.\n");
 				do_reset (cmdtp, flag, argc, argv);
 			}

common/cmd_bootm.c +3 -2

 				do_reset (cmdtp, flag, argc, argv);
 			}
 

@@ -808,10 +824,11 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 				ntohl(hdr->ih_size));
 			of_flat_tree = (char *)ntohl(hdr->ih_load);
 		} else {
-			puts ("Did not find a flat flat device tree\nMust RESET board to recover\n");
+			puts ("Did not find a flat Flat Device Tree.\n"
+				"Must RESET the board to recover.\n");
 			do_reset (cmdtp, flag, argc, argv);
 		}
-		printf ("   Booting using flat device tree at 0x%x\n",
+		printf ("   Booting using the Flat Device Tree at 0x%x\n",
 				of_flat_tree);
 	} else if ((hdr->ih_type==IH_TYPE_MULTI) && (len_ptr[1]) && (len_ptr[2])) {
 		u_long tail    = ntohl(len_ptr[0]) % 4;

@@ -835,12 +852,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 			of_data += 4 - tail;
 		}

common/cmd_bootm.c +11 -4

 			do_reset (cmdtp, flag, argc, argv);
 		}
 	}

@@ -944,25 +960,30 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 			of_start, of_start + of_len - 1);
 		err = fdt_open_into((void *)of_data, (void *)of_start, of_len);
 		if (err != 0) {
-			printf ("libfdt: %s " __FILE__ " %d\n", fdt_strerror(err), __LINE__);
+			puts ("ERROR: Failed to move the Flat Device Tree.\n"
+				"Must RESET the board to recover.\n");
+			do_reset (cmdtp, flag, argc, argv);
 		}
 		/*
 		 * Add the chosen node if it doesn't exist, add the env and bd_t
 		 * if the user wants it (the logic is in the subroutines).
 		 */
 		if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
-			puts ("ERROR: Failed creating the /chosen node, aborting.\nMust RESET board to recover\n");
+			puts ("ERROR: Failed creating the /chosen node.\n"
+				"Must RESET the board to recover.\n");
 			do_reset (cmdtp, flag, argc, argv);
 		}
 #ifdef CONFIG_OF_HAS_UBOOT_ENV
 		if (fdt_env(of_flat_tree) < 0) {
-			puts ("ERROR: Failed creating the /u-boot-env node, aborting.\nMust RESET board to recover\n");
+			puts ("ERROR: Failed creating the /u-boot-env node.\n"
+				"Must RESET the board to recover.\n");
 			do_reset (cmdtp, flag, argc, argv);
 		}
 #endif
 #ifdef CONFIG_OF_HAS_BD_T
 		if (fdt_bd_t(of_flat_tree) < 0) {
-			puts ("ERROR: Failed creating the /bd_t node, aborting.\nMust RESET board to recover\n");
+			puts ("ERROR: Failed creating the /bd_t node.\n"
+				"Must RESET the board to recover.\n");
 			do_reset (cmdtp, flag, argc, argv);
 		}
 #endif

@@ -989,23 +1010,34 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 	}
 #endif
 #if defined(CONFIG_OF_FLAT_TREE)
+	/*
+	 * Create the /chosen node and modify the blob with board specific

common/cmd_bootm.c +6 -3

 	if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
-		puts ("ERROR: Failed to create the /chosen node, aborting.\nMust RESET board to recover\n");
+		puts ("ERROR: Failed to create the /chosen node.\n"
+			"Must RESET the board to recover.\n");
 		do_reset (cmdtp, flag, argc, argv);
 	}
 #ifdef CONFIG_OF_HAS_UBOOT_ENV
 	if (fdt_env(of_flat_tree) < 0) {
-		puts ("ERROR: Failed to create the /u-boot-env node, aborting.\nMust RESET board to recover\n");
+		puts ("ERROR: Failed to create the /u-boot-env node.\n"
+			"Must RESET the board to recover.\n");
 		do_reset (cmdtp, flag, argc, argv);
 	}
 #endif
 #ifdef CONFIG_OF_HAS_BD_T
 	if (fdt_bd_t(of_flat_tree) < 0) {
-		puts ("ERROR: Failed to create the /bd_t node, aborting.\nMust RESET board to recover\n");
+		puts ("ERROR: Failed to create the /bd_t node, aborting.\n"
+			"Must RESET the board to recover.\n");
 		do_reset (cmdtp, flag, argc, argv);
 	}
 #endif

Quoted 17 lines · On Sat, 26 May 2007 08:54:50 -0400 Jerry Van Baren <[email protected]> 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 <[email protected]>

---
 common/cmd_bootm.c |   98 ++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 65 insertions(+), 33 deletions(-)

common/cmd_bootm.c +0 -0

index 25b9d74..786ff6e 100644
@@ -514,6 +514,19 @@ fixup_silent_linux ()
 }
 #endif /* CONFIG_SILENT_CONSOLE */
 

common/cmd_bootm.c fixup_silent_linux () · line 528

Quoted 12 lines

common/cmd_bootm.c +12 -0

+/*
+ * 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.

Quoted 19 lines

common/cmd_bootm.c +2 -5

+
 #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,
 

common/cmd_bootm.c do_bootm_linux (cmd_tbl_t *cmdtp, int flag, · line 777

Quoted 6 lines

common/cmd_bootm.c +3 -1

 			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.

Quoted 7 lines

common/cmd_bootm.c +4 -0

 #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); */

common/cmd_bootm.c do_bootm_linux (cmd_tbl_t *cmdtp, int flag, · line 1016

Quoted 6 lines

#endif
#if defined(CONFIG_OF_LIBFDT)

common/cmd_bootm.c +4 -0

+	/*
+	 * Create the /chosen node and modify the blob with board specific
+	 * values as needed.
+	 */

duplicate comment; why not hoist it up?

Kim

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:

  370                         printf ("BUNZIP2 ERROR %d - must RESET board to recover\n", i);

Is printf() safe in this delicate condition? Should I strip all

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.

Quoted 3 lines · Kim Phillips

On Sat, 26 May 2007 08:54:50 -0400
Jerry Van Baren <[email protected]> wrote:

Quoted 8 lines

Quoted 8 lines

common/cmd_bootm.c +8 -0

+/*
+ * 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)

Quoted 8 lines

common/cmd_bootm.c fixup_silent_linux () · line 528

Quoted 4 lines

common/cmd_bootm.c +4 -0

+#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) {