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

[Jerry Van Baren <gvb.uboot@gmail.com>]

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