[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c

[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c

[Jerry Van Baren]

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
+
+
 #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");
 				do_reset (cmdtp, flag, argc, argv);
 			}
 
@@ -773,7 +784,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 			header.ih_hcrc = 0;
 
 			if(checksum != crc32(0, (uchar *)&header, sizeof(image_header_t))) {
-				puts ("ERROR: Flat Device Tree header checksum is invalid\nMust RESET board to recover\n");
+				puts ("ERROR: The Flat Device Tree header "
+					"checksum is invalid.\n"
+					"Must RESET the board to recover.\n");
 				do_reset (cmdtp, flag, argc, argv);
 			}
 
@@ -781,25 +794,28 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 			addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
 
 			if(checksum != crc32(0, (uchar *)addr, ntohl(hdr->ih_size))) {
-				puts ("ERROR: Flat Device Tree checksum is invalid\nMust RESET board to recover\n");
+				puts ("ERROR: The Flat Device Tree checksum "
+					"is invalid.\n"
+					"Must RESET the board to recover.\n");
 				do_reset (cmdtp, flag, argc, argv);
 			}
 			printf("OK\n");
 
 			if (ntohl(hdr->ih_type) != IH_TYPE_FLATDT) {
-				puts ("ERROR: uImage not Flat Device Tree type\nMust RESET board to recover\n");
+				puts ("ERROR: uImage is not a "
+					"Flat Device Tree type.\n"
+					"Must RESET the board to recover.\n");
 				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);
 			}
-#if defined(CONFIG_OF_LIBFDT)
-			if (fdt_check_header(of_flat_tree + sizeof(image_header_t)) != 0) {
-#else
-			if (*((ulong *)(of_flat_tree + sizeof(image_header_t))) != OF_DT_HEADER) {
-#endif
-				puts ("ERROR: uImage data is not a flat device tree\nMust RESET board to recover\n");
+			if (FDT_VALIDATE) {
+				puts ("ERROR: uImage data is not "
+					"a Flat Device Tree.\n"
+					"Must RESET the board to recover.\n");
 				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;
 		}
 
-#if defined(CONFIG_OF_LIBFDT)
-		if (fdt_check_header((void *)of_data) != 0) {
-#else
-		if (((struct boot_param_header *)of_data)->magic != OF_DT_HEADER) {
-#endif
-			puts ("ERROR: image is not a flat device tree\nMust RESET board to recover\n");
+		if (FDT_VALIDATE) {
+			puts ("ERROR: image is not a Flat Device Tree.\n"
+				"Must RESET the board to recover.\n");
 			do_reset (cmdtp, flag, argc, argv);
 		}
 
@@ -849,7 +863,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 #else
 		if (((struct boot_param_header *)of_data)->totalsize != ntohl(len_ptr[2])) {
 #endif
-			puts ("ERROR: flat device tree size does not agree with image\nMust RESET board to recover\n");
+			puts ("ERROR: Flat Device Tree size does not "
+				"agree with the image size.\n"
+				"Must RESET the board to recover.\n");
 			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
+	 * 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.
+	 */
 	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
@@ -1024,7 +1056,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
 	if (of_flat_tree) {	/* device tree; boot new style */
 		/*
 		 * Linux Kernel Parameters (passing device tree):
-		 *   r3: ptr to flattened device tree, followed by the board info data
+		 *   r3: pointer to the fdt, followed by the board info data
 		 *   r4: physical pointer to the kernel itself
 		 *   r5: NULL
 		 *   r6: NULL
-- 
1.4.4.4

[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c

[Kim Phillips]

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.

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

>  				do_reset (cmdtp, flag, argc, argv);
>  			}
>  
> @@ -773,7 +784,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
>  			header.ih_hcrc = 0;
>  
>  			if(checksum != crc32(0, (uchar *)&header, sizeof(image_header_t))) {
> -				puts ("ERROR: Flat Device Tree header checksum is invalid\nMust RESET board to recover\n");
> +				puts ("ERROR: The Flat Device Tree header "
> +					"checksum is invalid.\n"
> +					"Must RESET the board to recover.\n");

ditto.

>  				do_reset (cmdtp, flag, argc, argv);
>  			}
>  
> @@ -781,25 +794,28 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
>  			addr = (ulong)((uchar *)(hdr) + sizeof(image_header_t));
>  
>  			if(checksum != crc32(0, (uchar *)addr, ntohl(hdr->ih_size))) {
> -				puts ("ERROR: Flat Device Tree checksum is invalid\nMust RESET board to recover\n");
> +				puts ("ERROR: The Flat Device Tree checksum "
> +					"is invalid.\n"
> +					"Must RESET the board to recover.\n");

"error: invalid FDT checksum. Reset the board" ?

>  				do_reset (cmdtp, flag, argc, argv);
>  			}
>  			printf("OK\n");
>  
>  			if (ntohl(hdr->ih_type) != IH_TYPE_FLATDT) {
> -				puts ("ERROR: uImage not Flat Device Tree type\nMust RESET board to recover\n");
> +				puts ("ERROR: uImage is not a "
> +					"Flat Device Tree type.\n"
> +					"Must RESET the board to recover.\n");

ditto.

>  				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);
>  			}
> -#if defined(CONFIG_OF_LIBFDT)
> -			if (fdt_check_header(of_flat_tree + sizeof(image_header_t)) != 0) {
> -#else
> -			if (*((ulong *)(of_flat_tree + sizeof(image_header_t))) != OF_DT_HEADER) {
> -#endif
> -				puts ("ERROR: uImage data is not a flat device tree\nMust RESET board to recover\n");
> +			if (FDT_VALIDATE) {
> +				puts ("ERROR: uImage data is not "
> +					"a Flat Device Tree.\n"
> +					"Must RESET the board to recover.\n");

ditto.

>  				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;
>  		}
>  
> -#if defined(CONFIG_OF_LIBFDT)
> -		if (fdt_check_header((void *)of_data) != 0) {
> -#else
> -		if (((struct boot_param_header *)of_data)->magic != OF_DT_HEADER) {
> -#endif
> -			puts ("ERROR: image is not a flat device tree\nMust RESET board to recover\n");
> +		if (FDT_VALIDATE) {
> +			puts ("ERROR: image is not a Flat Device Tree.\n"
> +				"Must RESET the board to recover.\n");

ditto.

>  			do_reset (cmdtp, flag, argc, argv);
>  		}
>  
> @@ -849,7 +863,9 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
>  #else
>  		if (((struct boot_param_header *)of_data)->totalsize != ntohl(len_ptr[2])) {
>  #endif
> -			puts ("ERROR: flat device tree size does not agree with image\nMust RESET board to recover\n");
> +			puts ("ERROR: Flat Device Tree size does not "
> +				"agree with the image size.\n"
> +				"Must RESET the board to recover.\n");

ditto.

>  			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
> +	 * 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?

>  	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
> @@ -1024,7 +1056,7 @@ do_bootm_linux (cmd_tbl_t *cmdtp, int flag,
>  	if (of_flat_tree) {	/* device tree; boot new style */
>  		/*
>  		 * Linux Kernel Parameters (passing device tree):
> -		 *   r3: ptr to flattened device tree, followed by the board info data
> +		 *   r3: pointer to the fdt, followed by the board info data

Kim

[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c

[Jerry Van Baren]

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

[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c

[Wolfgang Denk]

In message <465CD759.9090303@gmail.com> you wrote:
>
> 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:

The general rule is to use puts() for printing constant strings without
formatting.

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

Here we have the "%d", thus we need printf().

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

It's not safe in the sense that I would rely my life on it.  It's  an
attempt  to get the error message out and may or may not work. puts()
requires a little less code so it is a little more  likely  to  work,
but that doesn't make much of a difference.

> 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.

Yes, please do - especially as I see no reason what the udelay() could
be useful for.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Man is the best computer we can put aboard a spacecraft ...  and  the
only one that can be mass produced with unskilled labor.
                                                  - Wernher von Braun

[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c

[Kim Phillips]

On Tue, 29 May 2007 21:46:01 -0400
Jerry Van Baren <gvb.uboot@gmail.com> wrote:

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

I disagree; I can't immediately see what the latter is doing.  I can't
see what variables it accesses, and the name doesn't match either
operation.  FDT_CHECK_HEADER(of_flat_tree) would be a much better
implementation IMO.  This is in direct violation of CodingStyle ch. 12,
item 2 - did you read it?

> 
> 1) The horrible #if is repeated in 3 places, so it is 9x ugly

that's valid, but I don't mind it too much, esp. given the conversion
stage we're in.

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

retiring OF_FLAT_TREE does not automatically remove the #define.  LIBFDT
can happily continue to use it if the person removing OF_FLAT_TREE
forgets to remove it.  If you're telling me it'll be you, and you won't
forget, that's fine; I'm just interested in protecting and maintaining
readability of the code at this point.

Kim