| Message ID | 20241108143600.756224-6-herve.codina@bootlin.com |
|---|---|
| State | Needs Review / ACK |
| Headers | show |
| Series | Add support for the PCI host bridge device-tree node creation. | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
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
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é
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
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>; };
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(-)