| Message ID | 20190806214925.7534-1-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/7] xen/arm: extend device_tree_for_each_node | expand |
Hi Stefano, On 06/08/2019 22:49, Stefano Stabellini wrote: > Add new parameters to device_tree_for_each_node: node, depth, > address_cells, size_cells. address_cells (resp. size_cells) are named address_cells_p (resp. size_cells_p) in the code. But I am not convinced you need them. Per the DT spec (v0.2 section 2.3.5), the parent should either contain the #address-cells and #size-cells or default values (resp. 2 and 1) will be used. It is clearly stated that values are not inherited from the ancestors. So technically the implementation of device_tree_for_each_node() is incorrect. If you follow the spec here, then the address_cells_p and size_cells_p would become unnecessary. > Node is the parent node to start the search from; > depth is the min depth of the search (the depth of the parent node); > address_cells and size_cells are the respective parameters at the parent > node. Passing 0, 0, 0, 0 triggers the old behavior. > > We need this change because in follow-up patches we want to be able to > use reuse device_tree_for_each_node to call a function for each children > nodes of a provided node. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > --- > Changes in v4: > - add address_cells and size_cells parameters > > Changes in v3: > - improve commit message > - improve in-code comments > - improve code style > > Changes in v2: > - new > --- > xen/arch/arm/acpi/boot.c | 2 +- > xen/arch/arm/bootfdt.c | 21 +++++++++++++++------ > xen/include/xen/device_tree.h | 6 ++++-- > 3 files changed, 20 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > index 9b29769a10..d275f8c535 100644 > --- a/xen/arch/arm/acpi/boot.c > +++ b/xen/arch/arm/acpi/boot.c > @@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void) > */ > if ( param_acpi_off || ( !param_acpi_force > && device_tree_for_each_node(device_tree_flattened, > - dt_scan_depth1_nodes, NULL))) > + 0, 0, 0, 0, dt_scan_depth1_nodes, NULL))) NIT: Can we split the if? Cheers,
On 07/08/2019 17:08, Julien Grall wrote: > Hi Stefano, > > On 06/08/2019 22:49, Stefano Stabellini wrote: >> Add new parameters to device_tree_for_each_node: node, depth, >> address_cells, size_cells. > > address_cells (resp. size_cells) are named address_cells_p (resp. size_cells_p) > in the code. > > But I am not convinced you need them. Per the DT spec (v0.2 section 2.3.5), the > parent should either contain the #address-cells and #size-cells or default > values (resp. 2 and 1) will be used. It is clearly stated that values are not > inherited from the ancestors. > > So technically the implementation of device_tree_for_each_node() is incorrect. > If you follow the spec here, then the address_cells_p and size_cells_p would > become unnecessary. > >> Node is the parent node to start the search from; >> depth is the min depth of the search (the depth of the parent node); Actually, on the previous version [1] you agreed that this should be the children depth node and not the parent node. Why hasn't it been changed? >> address_cells and size_cells are the respective parameters at the parent >> node. Passing 0, 0, 0, 0 triggers the old behavior. >> We need this change because in follow-up patches we want to be able to >> use reuse device_tree_for_each_node to call a function for each children >> nodes of a provided node. >> >> Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> >> --- >> Changes in v4: >> - add address_cells and size_cells parameters >> >> Changes in v3: >> - improve commit message >> - improve in-code comments >> - improve code style >> >> Changes in v2: >> - new >> --- >> xen/arch/arm/acpi/boot.c | 2 +- >> xen/arch/arm/bootfdt.c | 21 +++++++++++++++------ >> xen/include/xen/device_tree.h | 6 ++++-- >> 3 files changed, 20 insertions(+), 9 deletions(-) >> >> diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c >> index 9b29769a10..d275f8c535 100644 >> --- a/xen/arch/arm/acpi/boot.c >> +++ b/xen/arch/arm/acpi/boot.c >> @@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void) >> */ >> if ( param_acpi_off || ( !param_acpi_force >> && >> device_tree_for_each_node(device_tree_flattened, >> - dt_scan_depth1_nodes, NULL))) >> + 0, 0, 0, 0, dt_scan_depth1_nodes, NULL))) > > NIT: Can we split the if? > > Cheers, > [1] <alpine.DEB.2.21.1908061206000.2451@sstabellini-ThinkPad-T480s>
On Wed, 7 Aug 2019, Julien Grall wrote: > Hi Stefano, > > On 06/08/2019 22:49, Stefano Stabellini wrote: > > Add new parameters to device_tree_for_each_node: node, depth, > > address_cells, size_cells. > > address_cells (resp. size_cells) are named address_cells_p (resp. > size_cells_p) in the code. > > But I am not convinced you need them. Per the DT spec (v0.2 section 2.3.5), > the parent should either contain the #address-cells and #size-cells or default > values (resp. 2 and 1) will be used. It is clearly stated that values are not > inherited from the ancestors. You are right, also on page 24 of the ePAPR. > So technically the implementation of device_tree_for_each_node() is incorrect. > If you follow the spec here, then the address_cells_p and size_cells_p would > become unnecessary. Indeed. > > Node is the parent node to start the search from; > > depth is the min depth of the search (the depth of the parent node); > > address_cells and size_cells are the respective parameters at the parent > > node. Passing 0, 0, 0, 0 triggers the old behavior. > > > We need this change because in follow-up patches we want to be able to > > use reuse device_tree_for_each_node to call a function for each children > > nodes of a provided node. > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > > --- > > Changes in v4: > > - add address_cells and size_cells parameters > > > > Changes in v3: > > - improve commit message > > - improve in-code comments > > - improve code style > > > > Changes in v2: > > - new > > --- > > xen/arch/arm/acpi/boot.c | 2 +- > > xen/arch/arm/bootfdt.c | 21 +++++++++++++++------ > > xen/include/xen/device_tree.h | 6 ++++-- > > 3 files changed, 20 insertions(+), 9 deletions(-) > > > > diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c > > index 9b29769a10..d275f8c535 100644 > > --- a/xen/arch/arm/acpi/boot.c > > +++ b/xen/arch/arm/acpi/boot.c > > @@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void) > > */ > > if ( param_acpi_off || ( !param_acpi_force > > && > > device_tree_for_each_node(device_tree_flattened, > > - dt_scan_depth1_nodes, > > NULL))) > > + 0, 0, 0, 0, dt_scan_depth1_nodes, NULL))) > > NIT: Can we split the if? Sure
diff --git a/xen/arch/arm/acpi/boot.c b/xen/arch/arm/acpi/boot.c index 9b29769a10..d275f8c535 100644 --- a/xen/arch/arm/acpi/boot.c +++ b/xen/arch/arm/acpi/boot.c @@ -248,7 +248,7 @@ int __init acpi_boot_table_init(void) */ if ( param_acpi_off || ( !param_acpi_force && device_tree_for_each_node(device_tree_flattened, - dt_scan_depth1_nodes, NULL))) + 0, 0, 0, 0, dt_scan_depth1_nodes, NULL))) goto disable; /* diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 891b4b66ff..dfce8c2bfe 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -77,6 +77,10 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, /** * device_tree_for_each_node - iterate over all device tree nodes * @fdt: flat device tree. + * @node: parent node to start the search from + * @depth: depth of the parent node + * @address_cells: address_cells at the parent node + * @size_cells: size_cells at the parent node * @func: function to call for each node. * @data: data to pass to @func. * @@ -86,17 +90,22 @@ static u32 __init device_tree_get_u32(const void *fdt, int node, * returns a value different from 0, that value is returned immediately. */ int __init device_tree_for_each_node(const void *fdt, + int node, int depth, + u32 address_cells_p, u32 size_cells_p, device_tree_node_func func, void *data) { - int node; - int depth; u32 address_cells[DEVICE_TREE_MAX_DEPTH]; u32 size_cells[DEVICE_TREE_MAX_DEPTH]; - int ret; + int ret, min_depth = depth; + + if ( depth > 0 ) + { + address_cells[depth - 1] = address_cells_p; + size_cells[depth - 1] = size_cells_p; + } - for ( node = 0, depth = 0; - node >=0 && depth >= 0; + for ( ; node >= 0 && depth >= min_depth; node = fdt_next_node(fdt, node, &depth) ) { const char *name = fdt_get_name(fdt, node, NULL); @@ -357,7 +366,7 @@ size_t __init boot_fdt_info(const void *fdt, paddr_t paddr) add_boot_module(BOOTMOD_FDT, paddr, fdt_totalsize(fdt), false); - device_tree_for_each_node((void *)fdt, early_scan_node, NULL); + device_tree_for_each_node((void *)fdt, 0, 0, 0, 0, early_scan_node, NULL); early_print_info(); return fdt_totalsize(fdt); diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 83156297e2..358c67c912 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -159,8 +159,10 @@ typedef int (*device_tree_node_func)(const void *fdt, extern const void *device_tree_flattened; int device_tree_for_each_node(const void *fdt, - device_tree_node_func func, - void *data); + int node, int depth, + u32 address_cells, u32 size_cells, + device_tree_node_func func, + void *data); /** * dt_unflatten_host_device_tree - Unflatten the host device tree
Add new parameters to device_tree_for_each_node: node, depth, address_cells, size_cells. Node is the parent node to start the search from; depth is the min depth of the search (the depth of the parent node); address_cells and size_cells are the respective parameters at the parent node. Passing 0, 0, 0, 0 triggers the old behavior. We need this change because in follow-up patches we want to be able to use reuse device_tree_for_each_node to call a function for each children nodes of a provided node. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> --- Changes in v4: - add address_cells and size_cells parameters Changes in v3: - improve commit message - improve in-code comments - improve code style Changes in v2: - new --- xen/arch/arm/acpi/boot.c | 2 +- xen/arch/arm/bootfdt.c | 21 +++++++++++++++------ xen/include/xen/device_tree.h | 6 ++++-- 3 files changed, 20 insertions(+), 9 deletions(-)