[U-Boot-Users] [PATCH libfdt 3/3] 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 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
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);@@ -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);*/ if (fdt_chosen(of_flat_tree, initrd_start, initrd_end, 0) < 0) {
- 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).
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.*/#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");#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");#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");#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
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.*/#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
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
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?
- 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.
- 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
participants (3)
-
Jerry Van Baren -
Kim Phillips -
Wolfgang Denk