[U-Boot-Users] [PATCH libfdt 3/3] More clean up of cmd_bootm.c
Jerry Van Baren
gvb.uboot at gmail.com
Wed May 30 03:46:01 CEST 2007
Kim Phillips wrote:
> On Sat, 26 May 2007 08:54:50 -0400
> Jerry Van Baren <gvb.uboot at 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 at 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
More information about the U-Boot
mailing list