Change in ...coreboot[master]: device_tree: Match debug output format to dtc -O dts output
Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32867
to review the following change.
Change subject: device_tree: Match debug output format to dtc -O dts output ......................................................................
device_tree: Match debug output format to dtc -O dts output
This patch updates the device tree dumping functions (not compiled by default but available for debugging) to output properties and nodes in a format similar to .dts files that is very close to what dtc outputs when you decompile a .dtb with it. This makes it easier to match device tree dumps from coreboot with device tree dumps generated by other device tree tooling.
This patch was adapted from depthcharge's http://crosreview.com/1536386
Change-Id: Ib40e50d906aff05473a70c4fc9b124d63232558c Signed-off-by: Julius Werner jwerner@chromium.org --- M src/lib/device_tree.c 1 file changed, 38 insertions(+), 11 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/32867/1
diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index bbcd7c0..3c4bd24 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -80,21 +80,36 @@
static void print_indent(int depth) { - while (depth--) - printk(BIOS_DEBUG, " "); + printk(BIOS_DEBUG, "%*s", depth * 8, ""); }
static void print_property(const struct fdt_property *prop, int depth) { + int is_string = prop->size > 0 && + ((char *)prop->data)[prop->size - 1] == '\0'; + + if (is_string) + for (const char *c = prop->data; *c != '\0'; c++) + if (!isprint(*c)) + is_string = 0; + print_indent(depth); - printk(BIOS_DEBUG, "prop "%s" (%d bytes).\n", prop->name, prop->size); - print_indent(depth + 1); - for (int i = 0; i < MIN(25, prop->size); i++) { - printk(BIOS_DEBUG, "%02x ", ((uint8_t *)prop->data)[i]); + if (is_string) { + printk(BIOS_DEBUG, "%s = "%s";\n", + prop->name, (const char *)prop->data); + } else { + printk(BIOS_DEBUG, "%s = < ", prop->name); + for (int i = 0; i < MIN(128, prop->size); i += 4) { + uint32_t val = 0; + for (int j = 0; j < MIN(4, prop->size - i); j++) + val |= ((uint8_t *)prop->data)[i + j] << + (24 - j * 8); + printk(BIOS_DEBUG, "%#.2x ", val); + } + if (prop->size > 128) + printk(BIOS_DEBUG, "..."); + printk(BIOS_DEBUG, ">;\n"); } - if (prop->size > 25) - printk(BIOS_DEBUG, "..."); - printk(BIOS_DEBUG, "\n"); }
static int print_flat_node(const void *blob, uint32_t start_offset, int depth) @@ -109,7 +124,7 @@ offset += size;
print_indent(depth); - printk(BIOS_DEBUG, "name = %s\n", name); + printk(BIOS_DEBUG, "%s {\n", name);
struct fdt_property prop; while ((size = fdt_next_property(blob, offset, &prop))) { @@ -118,9 +133,14 @@ offset += size; }
+ printk(BIOS_DEBUG, "\n"); // empty line between props and nodes + while ((size = print_flat_node(blob, offset, depth + 1))) offset += size;
+ print_indent(depth); + printk(BIOS_DEBUG, "}\n"); + return offset - start_offset + sizeof(uint32_t); }
@@ -458,15 +478,22 @@ static void print_node(const struct device_tree_node *node, int depth) { print_indent(depth); - printk(BIOS_DEBUG, "name = %s\n", node->name); + if (depth == 0) // root node has no name, print a starting slash + printk(BIOS_DEBUG, "/"); + printk(BIOS_DEBUG, "%s {\n", node->name);
struct device_tree_property *prop; list_for_each(prop, node->properties, list_node) print_property(&prop->prop, depth + 1);
+ printk(BIOS_DEBUG, "\n"); // empty line between props and nodes + struct device_tree_node *child; list_for_each(child, node->children, list_node) print_node(child, depth + 1); + + print_indent(depth); + printk(BIOS_DEBUG, "};\n"); }
void dt_print_node(const struct device_tree_node *node)
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32867 )
Change subject: device_tree: Match debug output format to dtc -O dts output ......................................................................
Patch Set 5: Code-Review+2
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32867 )
Change subject: device_tree: Match debug output format to dtc -O dts output ......................................................................
Patch Set 5:
(3 comments)
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c@136 PS5, Line 136: should we use space to replace tab here?
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c@481 PS5, Line 481: should we use space to replace tab here?
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c@489 PS5, Line 489: should we use space to replace tab here?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32867 )
Change subject: device_tree: Match debug output format to dtc -O dts output ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c@481 PS5, Line 481:
should we use space to replace tab here?
I mean, I don't know... do you think I should? I think tabs can help make comments stand out more and be more readable when there's space for it. There's no strict rule one way or the other about it in the style guide as far as I'm aware. (And all these formatting details will be lost like tears in the rain when the big clang-format bomb drops anyway...)
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32867 )
Change subject: device_tree: Match debug output format to dtc -O dts output ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32867/5/src/lib/device_tree.c@481 PS5, Line 481:
I mean, I don't know... […]
I think Google coding style suggests always doing two space for inline comment, while in this file you can find one space, tab, multi-tabs; but it's true that is not clearly defined in Coreboot/Linux coding style, so yes let's forget about this especially clang-format is coming.
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32867 )
Change subject: device_tree: Match debug output format to dtc -O dts output ......................................................................
device_tree: Match debug output format to dtc -O dts output
This patch updates the device tree dumping functions (not compiled by default but available for debugging) to output properties and nodes in a format similar to .dts files that is very close to what dtc outputs when you decompile a .dtb with it. This makes it easier to match device tree dumps from coreboot with device tree dumps generated by other device tree tooling.
This patch was adapted from depthcharge's http://crosreview.com/1536386
Change-Id: Ib40e50d906aff05473a70c4fc9b124d63232558c Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32867 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/device_tree.c 1 file changed, 38 insertions(+), 11 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index bbcd7c0..3c4bd24 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -80,21 +80,36 @@
static void print_indent(int depth) { - while (depth--) - printk(BIOS_DEBUG, " "); + printk(BIOS_DEBUG, "%*s", depth * 8, ""); }
static void print_property(const struct fdt_property *prop, int depth) { + int is_string = prop->size > 0 && + ((char *)prop->data)[prop->size - 1] == '\0'; + + if (is_string) + for (const char *c = prop->data; *c != '\0'; c++) + if (!isprint(*c)) + is_string = 0; + print_indent(depth); - printk(BIOS_DEBUG, "prop "%s" (%d bytes).\n", prop->name, prop->size); - print_indent(depth + 1); - for (int i = 0; i < MIN(25, prop->size); i++) { - printk(BIOS_DEBUG, "%02x ", ((uint8_t *)prop->data)[i]); + if (is_string) { + printk(BIOS_DEBUG, "%s = "%s";\n", + prop->name, (const char *)prop->data); + } else { + printk(BIOS_DEBUG, "%s = < ", prop->name); + for (int i = 0; i < MIN(128, prop->size); i += 4) { + uint32_t val = 0; + for (int j = 0; j < MIN(4, prop->size - i); j++) + val |= ((uint8_t *)prop->data)[i + j] << + (24 - j * 8); + printk(BIOS_DEBUG, "%#.2x ", val); + } + if (prop->size > 128) + printk(BIOS_DEBUG, "..."); + printk(BIOS_DEBUG, ">;\n"); } - if (prop->size > 25) - printk(BIOS_DEBUG, "..."); - printk(BIOS_DEBUG, "\n"); }
static int print_flat_node(const void *blob, uint32_t start_offset, int depth) @@ -109,7 +124,7 @@ offset += size;
print_indent(depth); - printk(BIOS_DEBUG, "name = %s\n", name); + printk(BIOS_DEBUG, "%s {\n", name);
struct fdt_property prop; while ((size = fdt_next_property(blob, offset, &prop))) { @@ -118,9 +133,14 @@ offset += size; }
+ printk(BIOS_DEBUG, "\n"); // empty line between props and nodes + while ((size = print_flat_node(blob, offset, depth + 1))) offset += size;
+ print_indent(depth); + printk(BIOS_DEBUG, "}\n"); + return offset - start_offset + sizeof(uint32_t); }
@@ -458,15 +478,22 @@ static void print_node(const struct device_tree_node *node, int depth) { print_indent(depth); - printk(BIOS_DEBUG, "name = %s\n", node->name); + if (depth == 0) // root node has no name, print a starting slash + printk(BIOS_DEBUG, "/"); + printk(BIOS_DEBUG, "%s {\n", node->name);
struct device_tree_property *prop; list_for_each(prop, node->properties, list_node) print_property(&prop->prop, depth + 1);
+ printk(BIOS_DEBUG, "\n"); // empty line between props and nodes + struct device_tree_node *child; list_for_each(child, node->children, list_node) print_node(child, depth + 1); + + print_indent(depth); + printk(BIOS_DEBUG, "};\n"); }
void dt_print_node(const struct device_tree_node *node)
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)