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

[Kim Phillips <kim.phillips@freescale.com>]

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