Re: [Xen-devel] [PATCH v4 1/7] xen/arm: extend device_tree_for_each_node

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> > 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel