Re: [PATCH v2 5/6] of: Add #address-cells/#size-cells in the device-tree root empty node

[Herve Codina <herve.codina@bootlin.com>]

Hi Rob,

On Fri, 8 Nov 2024 10:03:31 -0600
Rob Herring <robh@kernel.org> wrote:

> On Fri, Nov 8, 2024 at 8:36 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > On systems where ACPI is enabled or when a device-tree is not passed to
> > the kernel by the bootloader, a device-tree root empty node is created.
> > This device-tree root empty node doesn't have the #address-cells and the  
> 
> and the?

#size-cells properties.

Will be updated.

> 
> > This leads to the use of the default address cells and size cells values
> > which are defined in the code to 1 for address cells and 1 for size cells  
> 
> Missing period.

Will be updated.

> 
> >
> > According to the devicetree specification and the OpenFirmware standard
> > (IEEE 1275-1994) the default value for #address-cells should be 2.
> >
> > Also, according to the devicetree specification, the #address-cells and
> > the #size-cells are required properties in the root node.
> >
> > Modern implementation should have the #address-cells and the #size-cells
> > properties set and should not rely on default values.
> >
> > On x86, this root empty node is used and the code default values are
> > used.
> >
> > In preparation of the support for device-tree overlay on PCI devices
> > feature on x86 (i.e. the creation of the PCI root bus device-tree node),
> > the default value for #address-cells needs to be updated. Indeed, on
> > x86_64, addresses are on 64bits and the upper part of an address is
> > needed for correct address translations. On x86_32 having the default
> > value updated does not lead to issues while the uppert part of a 64bits  
> 
> upper

Will be updated.

> 
> > address is zero.
> >
> > Changing the default value for all architectures may break device-tree
> > compatibility. Indeed, existing dts file without the #address-cells
> > property set in the root node will not be compatible with this
> > modification.
> >
> > Instead of updating default values, add required #address-cells and  
> 
> and?

#size-cells properties in the device-tree empty root node.

Will be updated.

> 
> >
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  drivers/of/empty_root.dts | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
> > index cf9e97a60f48..5017579f34dc 100644
> > --- a/drivers/of/empty_root.dts
> > +++ b/drivers/of/empty_root.dts
> > @@ -2,5 +2,11 @@
> >  /dts-v1/;
> >
> >  / {
> > -
> > +       /*
> > +        * #address-cells/#size-cells are required properties at root node
> > +        * according to the devicetree specification. Use same values as default
> > +        * values mentioned for #address-cells/#size-cells properties.  
> 
> Which default? We have multiple...

I will reword:
  Use values mentioned in the devicetree specification as default values
  for #address-cells and #size-cells properties


> 
> There's also dtc's idea of default which IIRC is 2 and 1 like OpenFirmware.

I can re-add this part in the commit log:
  The device tree compiler already uses 2 as default value for address cells
  and 1 for size cells. The powerpc PROM code also use 2 as default value
  for #address-cells and 1 for #size-cells. Modern implementation should
  have the #address-cells and the #size-cells properties set and should
  not rely on default values.

In your opinion, does it make sense?

> 
> > +        */
> > +       #address-cells = <0x02>;
> > +       #size-cells = <0x01>;  
> 
> I think we should just do 2 cells for size.

Why using 2 for #size-cells?

I understand that allows to have size defined on 64bits but is that needed?
How to justify this value here?

Best regards,
Hervé