Hello Patrick Rudolph,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/32865
to review the following change.
Change subject: device_tree: Have absolute paths start with '/' ......................................................................
device_tree: Have absolute paths start with '/'
Currently DT paths are *not* expected to start with '/'. This is not what the spec says (see Devicetree Specification v0.2, 2.2.3 Path Names) and also not what is done by Linux.
Change dt_find_node_by_path() to expect paths to start with '/' and add a leading '/' to all DT path strings. Besides the compatibility with the spec this change is also needed to support aliases in the future.
This patch was adapted from depthcharge's http://crosreview.com/1252770
Change-Id: Ibdf59ccbb4ead38c6193b630642fd1f1e847dd89 Signed-off-by: Julius Werner jwerner@chromium.org --- M src/lib/device_tree.c M src/soc/cavium/cn81xx/soc.c 2 files changed, 9 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/32865/1
diff --git a/src/lib/device_tree.c b/src/lib/device_tree.c index 3e1dd35..e26021e 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -556,7 +556,7 @@ * * @param tree The device tree to search. * @param path A string representing a path in the device tree, with - * nodes separated by '/'. Example: "soc/firmware/coreboot" + * nodes separated by '/'. Example: "/firmware/coreboot" * @param addrcp Pointer that will be updated with any #address-cells * value found in the path. May be NULL to ignore. * @param sizecp Pointer that will be updated with any #size-cells @@ -565,13 +565,13 @@ * @return The found/created node, or NULL. * * It is the caller responsibility to provide the correct path string, namely - * not starting or ending with a '/', and not having "//" anywhere in it. + * starting with a '/', not ending in a '/' and not having "//" anywhere in it. */ struct device_tree_node *dt_find_node_by_path(struct device_tree *tree, const char *path, u32 *addrcp, u32 *sizecp, int create) { - char *dup_path = strdup(path); + char *dup_path = strdup(&path[1]); /* remove leading '/' */ /* Hopefully enough depth for any node. */ const char *path_array[15]; int i; @@ -995,7 +995,7 @@ struct device_tree_node *reserved; u32 addr = 0, size = 0;
- reserved = dt_find_node_by_path(tree, "reserved-memory", &addr, + reserved = dt_find_node_by_path(tree, "/reserved-memory", &addr, &size, 1); if (!reserved) return NULL; diff --git a/src/soc/cavium/cn81xx/soc.c b/src/soc/cavium/cn81xx/soc.c index 20542e5..efe7d6d 100644 --- a/src/soc/cavium/cn81xx/soc.c +++ b/src/soc/cavium/cn81xx/soc.c @@ -185,7 +185,7 @@ size_t i;
/* Set the sclk clock rate. */ - dt_node = dt_find_node_by_path(tree, "soc@0/sclk", NULL, NULL, 0); + dt_node = dt_find_node_by_path(tree, "/soc@0/sclk", NULL, NULL, 0); if (dt_node) { const u32 freq = thunderx_get_io_clock(); printk(BIOS_INFO, "%s: Set SCLK to %u Hz\n", __func__, freq); @@ -195,7 +195,7 @@ __func__);
/* Set refclkuaa clock rate. */ - dt_node = dt_find_node_by_path(tree, "soc@0/refclkuaa", NULL, + dt_node = dt_find_node_by_path(tree, "/soc@0/refclkuaa", NULL, NULL, 0); if (dt_node) { const u32 freq = uart_platform_refclk(); @@ -211,7 +211,7 @@ char path[32]; const uint64_t addr = UAAx_PF_BAR0(i); /* Remove the node */ - snprintf(path, sizeof(path), "soc@0/serial@%llx", addr); + snprintf(path, sizeof(path), "/soc@0/serial@%llx", addr); dt_node = dt_find_node_by_path(tree, path, NULL, NULL, 0); if (!dt_node || uart_is_enabled(i)) { printk(BIOS_INFO, "%s: ignoring %s\n", __func__, path); @@ -227,7 +227,7 @@ u32 phandle = 0; const uint64_t addr = PEM_PEMX_PF_BAR0(i); /* Remove the node */ - snprintf(path, sizeof(path), "soc@0/pci@%llx", addr); + snprintf(path, sizeof(path), "/soc@0/pci@%llx", addr); dt_node = dt_find_node_by_path(tree, path, NULL, NULL, 0); if (!dt_node || bdk_pcie_is_running(0, i)) { printk(BIOS_INFO, "%s: ignoring %s\n", __func__, path); @@ -239,7 +239,7 @@ list_remove(&dt_node->list_node);
/* Remove phandle to non existing nodes */ - snprintf(path, sizeof(path), "soc@0/smmu0@%llx", SMMU_PF_BAR0); + snprintf(path, sizeof(path), "/soc@0/smmu0@%llx", SMMU_PF_BAR0); dt_node = dt_find_node_by_path(tree, path, NULL, NULL, 0); if (!dt_node) { printk(BIOS_ERR, "%s: SMMU entry not found\n",
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32865 )
Change subject: device_tree: Have absolute paths start with '/' ......................................................................
Patch Set 5: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/32865/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32865/5/src/lib/device_tree.c@574 PS5, Line 574: dup_path should we add some assert to alert if path[0] is not / ?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32865 )
Change subject: device_tree: Have absolute paths start with '/' ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/32865/5/src/lib/device_tree.c File src/lib/device_tree.c:
https://review.coreboot.org/#/c/32865/5/src/lib/device_tree.c@574 PS5, Line 574: dup_path
should we add some assert to alert if path[0] is not / ?
This all gets changed again with the very next patch so there's no point of adding stuff here. I just wanted to keep the patches in the same increments as they were added in depthcharge (to make it easier to compare). This code is not part of the final version of this patch train.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32865 )
Change subject: device_tree: Have absolute paths start with '/' ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32865 )
Change subject: device_tree: Have absolute paths start with '/' ......................................................................
device_tree: Have absolute paths start with '/'
Currently DT paths are *not* expected to start with '/'. This is not what the spec says (see Devicetree Specification v0.2, 2.2.3 Path Names) and also not what is done by Linux.
Change dt_find_node_by_path() to expect paths to start with '/' and add a leading '/' to all DT path strings. Besides the compatibility with the spec this change is also needed to support aliases in the future.
This patch was adapted from depthcharge's http://crosreview.com/1252770
Change-Id: Ibdf59ccbb4ead38c6193b630642fd1f1e847dd89 Signed-off-by: Julius Werner jwerner@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/32865 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/lib/device_tree.c M src/soc/cavium/cn81xx/soc.c 2 files changed, 9 insertions(+), 9 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 3e1dd35..e26021e 100644 --- a/src/lib/device_tree.c +++ b/src/lib/device_tree.c @@ -556,7 +556,7 @@ * * @param tree The device tree to search. * @param path A string representing a path in the device tree, with - * nodes separated by '/'. Example: "soc/firmware/coreboot" + * nodes separated by '/'. Example: "/firmware/coreboot" * @param addrcp Pointer that will be updated with any #address-cells * value found in the path. May be NULL to ignore. * @param sizecp Pointer that will be updated with any #size-cells @@ -565,13 +565,13 @@ * @return The found/created node, or NULL. * * It is the caller responsibility to provide the correct path string, namely - * not starting or ending with a '/', and not having "//" anywhere in it. + * starting with a '/', not ending in a '/' and not having "//" anywhere in it. */ struct device_tree_node *dt_find_node_by_path(struct device_tree *tree, const char *path, u32 *addrcp, u32 *sizecp, int create) { - char *dup_path = strdup(path); + char *dup_path = strdup(&path[1]); /* remove leading '/' */ /* Hopefully enough depth for any node. */ const char *path_array[15]; int i; @@ -995,7 +995,7 @@ struct device_tree_node *reserved; u32 addr = 0, size = 0;
- reserved = dt_find_node_by_path(tree, "reserved-memory", &addr, + reserved = dt_find_node_by_path(tree, "/reserved-memory", &addr, &size, 1); if (!reserved) return NULL; diff --git a/src/soc/cavium/cn81xx/soc.c b/src/soc/cavium/cn81xx/soc.c index 20542e5..efe7d6d 100644 --- a/src/soc/cavium/cn81xx/soc.c +++ b/src/soc/cavium/cn81xx/soc.c @@ -185,7 +185,7 @@ size_t i;
/* Set the sclk clock rate. */ - dt_node = dt_find_node_by_path(tree, "soc@0/sclk", NULL, NULL, 0); + dt_node = dt_find_node_by_path(tree, "/soc@0/sclk", NULL, NULL, 0); if (dt_node) { const u32 freq = thunderx_get_io_clock(); printk(BIOS_INFO, "%s: Set SCLK to %u Hz\n", __func__, freq); @@ -195,7 +195,7 @@ __func__);
/* Set refclkuaa clock rate. */ - dt_node = dt_find_node_by_path(tree, "soc@0/refclkuaa", NULL, + dt_node = dt_find_node_by_path(tree, "/soc@0/refclkuaa", NULL, NULL, 0); if (dt_node) { const u32 freq = uart_platform_refclk(); @@ -211,7 +211,7 @@ char path[32]; const uint64_t addr = UAAx_PF_BAR0(i); /* Remove the node */ - snprintf(path, sizeof(path), "soc@0/serial@%llx", addr); + snprintf(path, sizeof(path), "/soc@0/serial@%llx", addr); dt_node = dt_find_node_by_path(tree, path, NULL, NULL, 0); if (!dt_node || uart_is_enabled(i)) { printk(BIOS_INFO, "%s: ignoring %s\n", __func__, path); @@ -227,7 +227,7 @@ u32 phandle = 0; const uint64_t addr = PEM_PEMX_PF_BAR0(i); /* Remove the node */ - snprintf(path, sizeof(path), "soc@0/pci@%llx", addr); + snprintf(path, sizeof(path), "/soc@0/pci@%llx", addr); dt_node = dt_find_node_by_path(tree, path, NULL, NULL, 0); if (!dt_node || bdk_pcie_is_running(0, i)) { printk(BIOS_INFO, "%s: ignoring %s\n", __func__, path); @@ -239,7 +239,7 @@ list_remove(&dt_node->list_node);
/* Remove phandle to non existing nodes */ - snprintf(path, sizeof(path), "soc@0/smmu0@%llx", SMMU_PF_BAR0); + snprintf(path, sizeof(path), "/soc@0/smmu0@%llx", SMMU_PF_BAR0); dt_node = dt_find_node_by_path(tree, path, NULL, NULL, 0); if (!dt_node) { printk(BIOS_ERR, "%s: SMMU entry not found\n",
-
Hung-Te Lin (Code Review) -
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)