Hi,
On 22/04/2022 19:52, Paran Lee wrote:
It doesn't seem necessary to do duplicate ternary operation and calculation
of order shift using fdt32_to_cpu macro.
Signed-off-by: Paran Lee <[email protected]>
---
xen/arch/arm/bootfdt.c | 12 ++++++++++--
xen/common/libfdt/fdt.c | 10 +++++-----
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e318ef9603..e5b885a7f2 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -159,8 +159,16 @@ int __init device_tree_for_each_node(const void *fdt, int
node,
continue;
}
- as = depth > 0 ? address_cells[depth-1] : DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
- ss = depth > 0 ? size_cells[depth-1] : DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
+ if ( depth > 0 )
+ {
+ as = address_cells[depth-1];
+ ss = size_cells[depth-1];
+ }
+ else
+ {
+ as = DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
+ ss = DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
+ }
IHMO the original code is easier to read. That said, in the two cases, I
think this is a bit pointless to check if the depth is > 0 at every
iteration because it will mostly be only always true but for one node.
So I would go with the following code (not tested):
diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e318ef960386..a382e10065f9 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -144,10 +144,13 @@ int __init device_tree_for_each_node(const void
*fdt, int node,
*/
int depth = 0;
const int first_node = node;
- u32 address_cells[DEVICE_TREE_MAX_DEPTH];
- u32 size_cells[DEVICE_TREE_MAX_DEPTH];
+ u32 address_cells[DEVICE_TREE_MAX_DEPTH + 1];
+ u32 size_cells[DEVICE_TREE_MAX_DEPTH + 1];
int ret;
+ address_cells[0] = DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
+ size_cells[0] = DT_ROOT_NOT_SIZE_CELLS_DEFAULT;
+
do {
const char *name = fdt_get_name(fdt, node, NULL);
u32 as, ss;
@@ -159,13 +162,13 @@ int __init device_tree_for_each_node(const void
*fdt, int node,
continue;
}
- as = depth > 0 ? address_cells[depth-1] :
DT_ROOT_NODE_ADDR_CELLS_DEFAULT;
- ss = depth > 0 ? size_cells[depth-1] :
DT_ROOT_NODE_SIZE_CELLS_DEFAULT;
+ as = address_cells[depth];
+ ss = size_cells[depth];
- address_cells[depth] = device_tree_get_u32(fdt, node,
+ address_cells[depth + 1] = device_tree_get_u32(fdt, node,
"#address-cells", as);
- size_cells[depth] = device_tree_get_u32(fdt, node,
- "#size-cells", ss);
+ size_cells[depth + 1] = device_tree_get_u32(fdt, node,
+ "#size-cells", ss);
/* skip the first node */
if ( node != first_node )
Any thoughts?
address_cells[depth] = device_tree_get_u32(fdt, node,
"#address-cells", as);
diff --git a/xen/common/libfdt/fdt.c b/xen/common/libfdt/fdt.c
index 9fe7cf4b74..a507169d29 100644
--- a/xen/common/libfdt/fdt.c
+++ b/xen/common/libfdt/fdt.c
fdt.c is an (old) verbatim copy from DTC (see [1]). I would rather
prefer to keep this directory as a verbatim copy because it makes easier
to sync with the upstream version.
In fact, there are a patch on xen-devel ([1]) to re-sync. So any of your
changes would be lost. Therefore, please send this change to the DTC
mailing list. We will pick it up on the next re-sync.
Cheers,
[1] https://github.com/dgibson/dtc
[2]
https://lore.kernel.org/xen-devel/[email protected]/
--
Julien Grall
