diff mbox series

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

Message ID 20241108143600.756224-6-herve.codina@bootlin.com
State Changes Requested
Headers show
Series Add support for the PCI host bridge device-tree node creation. | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied fail build log

Commit Message

Herve Codina Nov. 8, 2024, 2:35 p.m. UTC
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

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

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

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/of/empty_root.dts | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Rob Herring (Arm) Nov. 8, 2024, 4:03 p.m. UTC | #1
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?

> 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.

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

> 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?

>
> 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...

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

> +        */
> +       #address-cells = <0x02>;
> +       #size-cells = <0x01>;

I think we should just do 2 cells for size.

Rob
Herve Codina Nov. 8, 2024, 4:29 p.m. UTC | #2
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é
Rob Herring (Arm) Nov. 8, 2024, 5:24 p.m. UTC | #3
On Fri, Nov 8, 2024 at 10:29 AM Herve Codina <herve.codina@bootlin.com> wrote:
>
> 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

> > > +       /*
> > > +        * #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

My point was that "default" is meaningless because there are multiple
sources of what's default.

> >
> > 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?

Most systems are 64-bit today. And *all* ACPI based systems are. Not
that the DT has to match 32 vs 64 bit CPU, most of the time it does.

It also doesn't actually change anything for you because you're going
to have "pci" nodes and the "ranges" there takes #size-cells from the
pci node, not the parent.

Rob
Herve Codina Nov. 8, 2024, 5:39 p.m. UTC | #4
On Fri, 8 Nov 2024 11:24:36 -0600
Rob Herring <robh@kernel.org> wrote:

> On Fri, Nov 8, 2024 at 10:29 AM Herve Codina <herve.codina@bootlin.com> wrote:
> >
> > 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  
> 
> > > > +       /*
> > > > +        * #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  
> 
> My point was that "default" is meaningless because there are multiple
> sources of what's default.

I see thanks.
I will update the code comment.

> 
> > >
> > > 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?  
> 
> Most systems are 64-bit today. And *all* ACPI based systems are. Not
> that the DT has to match 32 vs 64 bit CPU, most of the time it does.
> 
> It also doesn't actually change anything for you because you're going
> to have "pci" nodes and the "ranges" there takes #size-cells from the
> pci node, not the parent.
> 

Right.
I will set:
  #address-cells = <0x02>;
  #size-cells = <0x02>;

Best regards,
Hervé
diff mbox series

Patch

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.
+	 */
+	#address-cells = <0x02>;
+	#size-cells = <0x01>;
 };