[PATCH v2 0/6] Add support for the PCI host bridge device-tree node creation.

[PATCH v2 0/6] Add support for the PCI host bridge device-tree node creation.

[Herve Codina]

Hi,

This series adds support for creating a device-tree node for the PCI
host bridge on non device-tree based system.

Creating device-tree nodes for PCI devices and PCI-PCI bridges already
exists upstream. It was added in commit 407d1a51921e ("PCI: Create
device tree node for bridge"). Created device-tree nodes need a parent
node to be attached to. For the first level devices, on device-tree
based system, this parent node (i.e. the PCI host bridge) is described
in the base device-tree. The PCI bus related to this bridge (PCI root
bus) inherit of the PCI host bridge device-tree node.

The LAN966x PCI device driver was recently accepted [1] and relies on
this feature.

On system where the base hardware is not described by a device-tree, the
PCI host bridge to which first level created PCI devices need to be
attach to does not exist. This is the case for instance on ACPI
described systems such as x86.

This series goal is to handle this case.

In order to have the PCI host bridge device-tree node available even
on x86, this top level node is created (if not already present) based on
information computed by the PCI core. It follows the same mechanism as
the one used for PCI devices device-tree node creation.

As for device-tree based system, the PCI root bus handled by the PCI
host bridge inherit of this created node.

In order to have this feature available, a number of changes are needed:
  - Patch 1 and 2: Introduce and use device_{add,remove}_of_node().
    This function will also be used in the root PCI bus node creation.

  - Patch 3 and 4: Improve existing functions to reuse them in the root
    PCI bus node creation.

  - Patch 5: Set #address-cells and #size-cells in the empty device-tree
    root node.

  - Patch 6: The PCI host bridge device-tree node creation itself.

With those modifications, the LAN966x PCI device is working on x86 systems
and all device-tree kunit tests (including the of_unittest_pci_node test)
pass successfully with the following command:
  qemu-system-x86_64 -machine q35 -nographic \
    -kernel arch/x86_64/boot/bzImage --append console=ttyS0 \
    -device pcie-root-port,port=0x10,chassis=9,id=pci.9,bus=pcie.0,multifunction=on,addr=0x3 \
    -device pcie-root-port,port=0x11,chassis=10,id=pci.10,bus=pcie.0,addr=0x3.0x1 \
    -device x3130-upstream,id=pci.11,bus=pci.9,addr=0x0 \
    -device xio3130-downstream,port=0x0,chassis=11,id=pci.12,bus=pci.11,multifunction=on,addr=0x0 \
    -device i82801b11-bridge,id=pci.13,bus=pcie.0,addr=0x4 \
    -device pci-bridge,chassis_nr=14,id=pci.14,bus=pci.13,addr=0x0 \
    -device pci-testdev,bus=pci.12,addr=0x0

[1] https://lore.kernel.org/lkml/7512cbb7911b8395d926e9e9e390fbb55ce3aea9.camel@pengutronix.de/

Compare to previous iteration, this v2 series mainly:
  - Adds #address-cells/#size-cells in the empty root DT node.
  - Creates the device-tree node for the PCI root bridge and reuses it
    for the PCI root bus instead of just creating it for the PCI root
    bus.

Best regards,
Hervé Codina

Changes v1 -> v2
  v1: https://lore.kernel.org/lkml/20241104172001.165640-1-herve.codina@bootlin.com/

  - Patch 1
    Remove Cc: stable

  - Patch 2
    Remove Fixup tag and Cc: stable

  - Patches 3 and 4
    No changes

  - Patch 5
    Add #address-cells/#size-cells in the empty root DT node instead of
    updating default values for x86.
    Update commit log and commit title.

  - Patch 6
    Create device-tree node for the PCI host bridge and reuse it for
    the PCI root bus. Rename functions accordingly.
    Use "pci" instead of "pci-root" for the PCI host bridge node name.
    Use "res->start - windows->offset" for the PCI bus addresses.
    Update commit log and commit title.

Herve Codina (6):
  driver core: Introduce device_{add,remove}_of_node()
  PCI: of: Use device_{add,remove}_of_node() to attach of_node to
    existing device
  PCI: of_property: Add support for NULL pdev in of_pci_set_address()
  PCI: of_property: Constify parameter in of_pci_get_addr_flags()
  of: Add #address-cells/#size-cells in the device-tree root empty node
  PCI: of: Create device-tree PCI host bridge node

 drivers/base/core.c       |  52 +++++++++++++++++
 drivers/of/empty_root.dts |   8 ++-
 drivers/pci/of.c          |  98 +++++++++++++++++++++++++++++++-
 drivers/pci/of_property.c | 114 ++++++++++++++++++++++++++++++++++++--
 drivers/pci/pci.h         |   6 ++
 drivers/pci/probe.c       |   2 +
 drivers/pci/remove.c      |   2 +
 include/linux/device.h    |   2 +
 8 files changed, 276 insertions(+), 8 deletions(-)

-- 
2.46.2

[PATCH v2 1/6] driver core: Introduce device_{add,remove}_of_node()

[Herve Codina]

An of_node can be set to a device using device_set_node().
This function cannot prevent any of_node and/or fwnode overwrites.

When adding an of_node on an already present device, the following
operations need to be done:
- Attach the of_node if no of_node were already attached
- Attach the of_node as a fwnode if no fwnode were already attached

This is the purpose of device_add_of_node().
device_remove_of_node() reverts the operations done by
device_add_of_node().

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/base/core.c    | 52 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |  2 ++
 2 files changed, 54 insertions(+)

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 24c572031403..0aa63371f55d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -5118,6 +5118,58 @@ void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode)
 }
 EXPORT_SYMBOL_GPL(set_secondary_fwnode);
 
+/**
+ * device_remove_of_node - Remove an of_node from a device
+ * @dev: device whose device-tree node is being removed
+ */
+void device_remove_of_node(struct device *dev)
+{
+	dev = get_device(dev);
+	if (!dev)
+		return;
+
+	if (!dev->of_node)
+		goto end;
+
+	if (dev->fwnode == of_fwnode_handle(dev->of_node))
+		dev->fwnode = NULL;
+
+	of_node_put(dev->of_node);
+	dev->of_node = NULL;
+
+end:
+	put_device(dev);
+}
+EXPORT_SYMBOL_GPL(device_remove_of_node);
+
+/**
+ * device_add_of_node - Add an of_node to an existing device
+ * @dev: device whose device-tree node is being added
+ * @of_node: of_node to add
+ */
+void device_add_of_node(struct device *dev, struct device_node *of_node)
+{
+	if (!of_node)
+		return;
+
+	dev = get_device(dev);
+	if (!dev)
+		return;
+
+	if (WARN(dev->of_node, "%s: Cannot replace node %pOF with %pOF\n",
+		 dev_name(dev), dev->of_node, of_node))
+		goto end;
+
+	dev->of_node = of_node_get(of_node);
+
+	if (!dev->fwnode)
+		dev->fwnode = of_fwnode_handle(of_node);
+
+end:
+	put_device(dev);
+}
+EXPORT_SYMBOL_GPL(device_add_of_node);
+
 /**
  * device_set_of_node_from_dev - reuse device-tree node of another device
  * @dev: device whose device-tree node is being set
diff --git a/include/linux/device.h b/include/linux/device.h
index b4bde8d22697..e3aa25ce1f90 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1146,6 +1146,8 @@ int device_online(struct device *dev);
 void set_primary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
 void set_secondary_fwnode(struct device *dev, struct fwnode_handle *fwnode);
 void device_set_node(struct device *dev, struct fwnode_handle *fwnode);
+void device_add_of_node(struct device *dev, struct device_node *of_node);
+void device_remove_of_node(struct device *dev);
 void device_set_of_node_from_dev(struct device *dev, const struct device *dev2);
 
 static inline struct device_node *dev_of_node(struct device *dev)
-- 
2.46.2

[PATCH v2 2/6] PCI: of: Use device_{add,remove}_of_node() to attach of_node to existing device

[Herve Codina]

The commit 407d1a51921e ("PCI: Create device tree node for bridge")
creates of_node for PCI devices. The newly created of_node is attached
to an existing device. This is done setting directly pdev->dev.of_node
in the code.

Even if pdev->dev.of_node cannot be previously set, this doesn't handle
the fwnode field of the struct device. Indeed, this field needs to be
set if it hasn't already been set.

device_{add,remove}_of_node() have been introduced to handle this case.

Use them instead of the direct setting.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/pci/of.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index dacea3fc5128..141ffbb1b3e6 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -655,8 +655,8 @@ void of_pci_remove_node(struct pci_dev *pdev)
 	np = pci_device_to_OF_node(pdev);
 	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
 		return;
-	pdev->dev.of_node = NULL;
 
+	device_remove_of_node(&pdev->dev);
 	of_changeset_revert(np->data);
 	of_changeset_destroy(np->data);
 	of_node_put(np);
@@ -713,7 +713,7 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
 		goto out_free_node;
 
 	np->data = cset;
-	pdev->dev.of_node = np;
+	device_add_of_node(&pdev->dev, np);
 	kfree(name);
 
 	return;
-- 
2.46.2

[PATCH v2 3/6] PCI: of_property: Add support for NULL pdev in of_pci_set_address()

[Herve Codina]

The pdev (pointer to a struct pci_dev) parameter of of_pci_set_address()
cannot be NULL.

In order to reuse of_pci_set_address() when creating the PCI root bus
node, this function needs to support a NULL pdev parameter. Indeed, in
the case of the PCI root bus node creation, no pdev are available and
of_pci_set_address() will be used with the bridge windows.

Allow to call of_pci_set_address() with a NULL pdev.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/pci/of_property.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 5a0b98e69795..59cc5c851fc3 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -54,9 +54,13 @@ enum of_pci_prop_compatible {
 static void of_pci_set_address(struct pci_dev *pdev, u32 *prop, u64 addr,
 			       u32 reg_num, u32 flags, bool reloc)
 {
-	prop[0] = FIELD_PREP(OF_PCI_ADDR_FIELD_BUS, pdev->bus->number) |
-		FIELD_PREP(OF_PCI_ADDR_FIELD_DEV, PCI_SLOT(pdev->devfn)) |
-		FIELD_PREP(OF_PCI_ADDR_FIELD_FUNC, PCI_FUNC(pdev->devfn));
+	if (pdev)
+		prop[0] = FIELD_PREP(OF_PCI_ADDR_FIELD_BUS, pdev->bus->number) |
+			  FIELD_PREP(OF_PCI_ADDR_FIELD_DEV, PCI_SLOT(pdev->devfn)) |
+			  FIELD_PREP(OF_PCI_ADDR_FIELD_FUNC, PCI_FUNC(pdev->devfn));
+	else
+		prop[0] = 0;
+
 	prop[0] |= flags | reg_num;
 	if (!reloc) {
 		prop[0] |= OF_PCI_ADDR_FIELD_NONRELOC;
-- 
2.46.2

[PATCH v2 4/6] PCI: of_property: Constify parameter in of_pci_get_addr_flags()

[Herve Codina]

The res parameter has no reason to be a pointer to an un-const struct
resource. Indeed, struct resource is not supposed to be modified by the
function.

Constify the res parameter.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/pci/of_property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index 59cc5c851fc3..e56159cc48e8 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -69,7 +69,7 @@ static void of_pci_set_address(struct pci_dev *pdev, u32 *prop, u64 addr,
 	}
 }
 
-static int of_pci_get_addr_flags(struct resource *res, u32 *flags)
+static int of_pci_get_addr_flags(const struct resource *res, u32 *flags)
 {
 	u32 ss;
 
-- 
2.46.2

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

[Herve Codina]

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

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>;
 };
-- 
2.46.2

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

[Rob Herring]

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

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

[Herve Codina]

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é

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

[Rob Herring]

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

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

[Herve Codina]

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é

[PATCH v2 6/6] PCI: of: Create device-tree PCI host bridge node

[Herve Codina]

PCI devices device-tree nodes can be already created. This was
introduced by commit 407d1a51921e ("PCI: Create device tree node for
bridge").

In order to have device-tree nodes related to PCI devices attached on
their PCI root bus (the PCI bus handled by the PCI host bridge), a PCI
root bus device-tree node is needed. This root bus node will be used as
the parent node of the first level devices scanned on the bus. On
device-tree based systems, this PCI root bus device tree node is set to
the node of the related PCI host bridge. The PCI host bridge node is
available in the device-tree used to describe the hardware passed at
boot.

On non device-tree based system (such as ACPI), a device-tree node for
the PCI host bridge or for the root bus do not exist. Indeed, the PCI
host bridge is not described in a device-tree used at boot simply
because no device-tree are passed at boot.

The device-tree PCI host bridge node creation needs to be done at
runtime. This is done in the same way as for the creation of the PCI
device nodes. I.e. node and properties are created based on computed
information done by the PCI core. Also, as is done on device-tree based
systems, this PCI host bridge node is used for the PCI root bus.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/pci/of.c          |  94 ++++++++++++++++++++++++++++++++++-
 drivers/pci/of_property.c | 102 ++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h         |   6 +++
 drivers/pci/probe.c       |   2 +
 drivers/pci/remove.c      |   2 +
 5 files changed, 205 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/of.c b/drivers/pci/of.c
index 141ffbb1b3e6..316c31d13aa8 100644
--- a/drivers/pci/of.c
+++ b/drivers/pci/of.c
@@ -726,7 +726,99 @@ void of_pci_make_dev_node(struct pci_dev *pdev)
 out_free_name:
 	kfree(name);
 }
-#endif
+
+void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge)
+{
+	struct device_node *np;
+
+	np = pci_bus_to_OF_node(bridge->bus);
+	if (!np || !of_node_check_flag(np, OF_DYNAMIC))
+		return;
+
+	device_remove_of_node(&bridge->bus->dev);
+	device_remove_of_node(&bridge->dev);
+	of_changeset_revert(np->data);
+	of_changeset_destroy(np->data);
+	of_node_put(np);
+}
+
+void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge)
+{
+	struct device_node *np = NULL;
+	struct of_changeset *cset;
+	const char *name;
+	int ret;
+
+	/*
+	 * If there is already a device-tree node linked to the PCI bus handled
+	 * by this bridge (i.e. the PCI root bus), nothing to do.
+	 */
+	if (pci_bus_to_OF_node(bridge->bus))
+		return;
+
+	/* The root bus has no node. Check that the host bridge has no node too */
+	if (bridge->dev.of_node) {
+		pr_err("PCI host bridge of_node already set");
+		return;
+	}
+
+	/* Check if there is a DT root node to attach the created node */
+	if (!of_root) {
+		pr_err("of_root node is NULL, cannot create PCI host bridge node\n");
+		return;
+	}
+
+	name = kasprintf(GFP_KERNEL, "pci@%x,%x", pci_domain_nr(bridge->bus),
+			 bridge->bus->number);
+	if (!name)
+		return;
+
+	cset = kmalloc(sizeof(*cset), GFP_KERNEL);
+	if (!cset)
+		goto out_free_name;
+	of_changeset_init(cset);
+
+	np = of_changeset_create_node(cset, of_root, name);
+	if (!np)
+		goto out_destroy_cset;
+
+	ret = of_pci_add_host_bridge_properties(bridge, cset, np);
+	if (ret)
+		goto out_free_node;
+
+	/*
+	 * This of_node will be added to an existing device. The of_node parent
+	 * is the root OF node and so this node will be handled by the platform
+	 * bus. Avoid any new device creation.
+	 */
+	of_node_set_flag(np, OF_POPULATED);
+	np->fwnode.dev = &bridge->dev;
+	fwnode_dev_initialized(&np->fwnode, true);
+
+	ret = of_changeset_apply(cset);
+	if (ret)
+		goto out_free_node;
+
+	np->data = cset;
+
+	/* Add the of_node to host bridge and the root bus */
+	device_add_of_node(&bridge->dev, np);
+	device_add_of_node(&bridge->bus->dev, np);
+
+	kfree(name);
+
+	return;
+
+out_free_node:
+	of_node_put(np);
+out_destroy_cset:
+	of_changeset_destroy(cset);
+	kfree(cset);
+out_free_name:
+	kfree(name);
+}
+
+#endif /* CONFIG_PCI_DYNAMIC_OF_NODES */
 
 #endif /* CONFIG_PCI */
 
diff --git a/drivers/pci/of_property.c b/drivers/pci/of_property.c
index e56159cc48e8..e8e2c3ecc255 100644
--- a/drivers/pci/of_property.c
+++ b/drivers/pci/of_property.c
@@ -394,3 +394,105 @@ int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
 
 	return 0;
 }
+
+static bool of_pci_is_range_resource(const struct resource *res, u32 *flags)
+{
+	if (!(resource_type(res) & IORESOURCE_MEM) &&
+	    !(resource_type(res) & IORESOURCE_MEM_64))
+		return false;
+
+	if (of_pci_get_addr_flags(res, flags))
+		return false;
+
+	return true;
+}
+
+static int of_pci_host_bridge_prop_ranges(struct pci_host_bridge *bridge,
+					  struct of_changeset *ocs,
+					  struct device_node *np)
+{
+	struct resource_entry *window;
+	unsigned int ranges_sz = 0;
+	unsigned int n_range = 0;
+	struct resource *res;
+	int n_addr_cells;
+	u32 *ranges;
+	u64 val64;
+	u32 flags;
+	int ret;
+
+	n_addr_cells = of_n_addr_cells(np);
+	if (n_addr_cells <= 0 || n_addr_cells > 2)
+		return -EINVAL;
+
+	resource_list_for_each_entry(window, &bridge->windows) {
+		res = window->res;
+		if (!of_pci_is_range_resource(res, &flags))
+			continue;
+		n_range++;
+	}
+
+	if (!n_range)
+		return 0;
+
+	ranges = kcalloc(n_range,
+			 (OF_PCI_ADDRESS_CELLS + OF_PCI_SIZE_CELLS +
+			  n_addr_cells) * sizeof(*ranges),
+			 GFP_KERNEL);
+	if (!ranges)
+		return -ENOMEM;
+
+	resource_list_for_each_entry(window, &bridge->windows) {
+		res = window->res;
+		if (!of_pci_is_range_resource(res, &flags))
+			continue;
+
+		/* PCI bus address */
+		val64 = res->start;
+		of_pci_set_address(NULL, &ranges[ranges_sz], val64 - window->offset,
+				   0, flags, false);
+		ranges_sz += OF_PCI_ADDRESS_CELLS;
+
+		/* Host bus address */
+		if (n_addr_cells == 2)
+			ranges[ranges_sz++] = upper_32_bits(val64);
+		ranges[ranges_sz++] = lower_32_bits(val64);
+
+		/* Size */
+		val64 = resource_size(res);
+		ranges[ranges_sz] = upper_32_bits(val64);
+		ranges[ranges_sz + 1] = lower_32_bits(val64);
+		ranges_sz += OF_PCI_SIZE_CELLS;
+	}
+
+	ret = of_changeset_add_prop_u32_array(ocs, np, "ranges", ranges, ranges_sz);
+	kfree(ranges);
+	return ret;
+}
+
+int of_pci_add_host_bridge_properties(struct pci_host_bridge *bridge,
+				      struct of_changeset *ocs,
+				      struct device_node *np)
+{
+	int ret;
+
+	ret = of_changeset_add_prop_string(ocs, np, "device_type", "pci");
+	if (ret)
+		return ret;
+
+	ret = of_changeset_add_prop_u32(ocs, np, "#address-cells",
+					OF_PCI_ADDRESS_CELLS);
+	if (ret)
+		return ret;
+
+	ret = of_changeset_add_prop_u32(ocs, np, "#size-cells",
+					OF_PCI_SIZE_CELLS);
+	if (ret)
+		return ret;
+
+	ret = of_pci_host_bridge_prop_ranges(bridge, ocs, np);
+	if (ret)
+		return ret;
+
+	return 0;
+}
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 14d00ce45bfa..f2092a119f2c 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -802,9 +802,15 @@ void of_pci_make_dev_node(struct pci_dev *pdev);
 void of_pci_remove_node(struct pci_dev *pdev);
 int of_pci_add_properties(struct pci_dev *pdev, struct of_changeset *ocs,
 			  struct device_node *np);
+void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge);
+void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge);
+int of_pci_add_host_bridge_properties(struct pci_host_bridge *bridge, struct of_changeset *ocs,
+				      struct device_node *np);
 #else
 static inline void of_pci_make_dev_node(struct pci_dev *pdev) { }
 static inline void of_pci_remove_node(struct pci_dev *pdev) { }
+static inline void of_pci_make_host_bridge_node(struct pci_host_bridge *bridge) { }
+static inline void of_pci_remove_host_bridge_node(struct pci_host_bridge *bridge) { }
 #endif
 
 #ifdef CONFIG_PCIEAER
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 4f68414c3086..bfe89ce8d800 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1049,6 +1049,8 @@ static int pci_register_host_bridge(struct pci_host_bridge *bridge)
 		dev_info(&bus->dev, "root bus resource %pR%s\n", res, addr);
 	}
 
+	of_pci_make_host_bridge_node(bridge);
+
 	down_write(&pci_bus_sem);
 	list_add_tail(&bus->node, &pci_root_buses);
 	up_write(&pci_bus_sem);
diff --git a/drivers/pci/remove.c b/drivers/pci/remove.c
index e4ce1145aa3e..2c379c9ab5b4 100644
--- a/drivers/pci/remove.c
+++ b/drivers/pci/remove.c
@@ -160,6 +160,8 @@ void pci_stop_root_bus(struct pci_bus *bus)
 					 &bus->devices, bus_list)
 		pci_stop_bus_device(child);
 
+	of_pci_remove_host_bridge_node(host_bridge);
+
 	/* stop the host bridge */
 	device_release_driver(&host_bridge->dev);
 }
-- 
2.46.2