Fix RP1 DeviceTree hierarchy and drop overlay support

The current RP1 implementation is plagued by several issues, as follows:

- the node name for RP1 is too specific and should be generic instead (see [1]).

- the fully defined DTS has its PCI hierarchy wrongly described. There should be a PCI root port between the root complex and the endpoint (see [1]).

- since CONFIG_PCI_DYNAMIC_OF_NODES can be dropped in the future becoming an automatically enabled feature, it would be wise to not depend on it (see [2]).

- overlay support has led to a lot of confusion. It's not really usable right now and users are not even used to it (see [3]).

This patch aims at solving the aforementioned problems by amending the PCI topology as follows:

  ...
  pcie@1000120000 {
    ...

    pci@0,0 {
      device_type = "pci";
      reg = <0x00 0x00 0x00 0x00 0x00>;
      ...

      dev@0,0 {
        compatible = "pci1de4,1";
        reg = <0x10000 0x00 0x00 0x00 0x00>;
        ...

        pci-ep-bus@1 {
          compatible = "simple-bus";
          ...

          /* peripherals child nodes */
        }; 
      }; 
    }; 

};

The reg property is important since it permits the binding the OF device_node structure to the pci_dev, encoding the BDF in the upper portion of the address.

This patch also drops the overlay support in favor of the fully described DT while streamlining it as a result.

Links:
[1] - https://lore.kernel.org/all/aTvz_OeVnciiqATz@apocalypse/
[2] - https://lore.kernel.org/all/CAL_JsqJUzB71QdMcxJtNZ7raoPcK+SfTh7EVzGmk=syo8xLKQw@mail.gmail.com/
[3] - https://lore.kernel.org/all/CAL_JsqJUzB71QdMcxJtNZ7raoPcK+SfTh7EVzGmk=syo8xLKQw@mail.gmail.com/

Andrea della Porta (4):
dt-bindings: misc: pci1de4,1: add required reg property for endpoint
misc: rp1: drop overlay support
arm64: dts: broadcom: bcm2712: fix RP1 endpoint PCI topology
arm64: dts: broadcom: rp1: drop RP1 overlay

 .../devicetree/bindings/misc/pci1de4,1.yaml   |  8 +++-
 arch/arm64/boot/dts/broadcom/Makefile         |  4 +-
 ...-ovl-rp1.dts => bcm2712-rpi-5-b-base.dtsi} |  0
 .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     | 39 ++++++++++++-------
 arch/arm64/boot/dts/broadcom/rp1-nexus.dtsi   | 14 -------
 arch/arm64/boot/dts/broadcom/rp1.dtso         | 11 ------
 drivers/misc/rp1/Kconfig                      |  6 +--
 drivers/misc/rp1/Makefile                     |  3 +-
 drivers/misc/rp1/rp1-pci.dtso                 | 25 ------------
 drivers/misc/rp1/rp1_pci.c                    | 37 ++----------------
 drivers/pci/quirks.c                          |  1 -
 11 files changed, 40 insertions(+), 108 deletions(-)
 rename arch/arm64/boot/dts/broadcom/{bcm2712-rpi-5-b-ovl-rp1.dts => bcm2712-rpi-5-b-base.dtsi} (100%)
 delete mode 100644 arch/arm64/boot/dts/broadcom/rp1-nexus.dtsi
 delete mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso
 delete mode 100644 drivers/misc/rp1/rp1-pci.dtso

2.35.3

The PCI subsystem links an endpoint Device Tree node to its corresponding pci_dev structure only if the Bus/Device/Function (BDF) encoded in the 'reg' property matches the actual hardware topology.

Add the 'reg' property and mark it as required to ensure proper binding between the device_node and the pci_dev.

Update the example to reflect this requirement.

---
 Documentation/devicetree/bindings/misc/pci1de4,1.yaml | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

index 2f9a7a554ed8a..17a8c19af8cc2 100644
@@ -25,6 +25,10 @@ properties:
     items:
       - const: pci1de4,1
 
+  reg:
+    maxItems: 1
+    description: The PCI Bus-Device-Function address.
+
   '#interrupt-cells':
     const: 2
     description: |

@@ -101,6 +105,7 @@ unevaluatedProperties: false
 
 required:
   - compatible
+  - reg
   - '#interrupt-cells'
   - interrupt-controller
   - pci-ep-bus@1

@@ -111,8 +116,9 @@ examples:
         #address-cells = <3>;
         #size-cells = <2>;
 
-        rp1@0,0 {
+        dev@0,0 {
             compatible = "pci1de4,1";
+            reg = <0x10000 0x0 0x0 0x0 0x0>;
             ranges = <0x01 0x00 0x00000000  0x82010000 0x00 0x00  0x00 0x400000>;
             #address-cells = <3>;
             #size-cells = <2>;

2.35.3

From: Florian Fainelli <[email protected]>

Applied to https://github.com/Broadcom/stblinux/commits/devicetree-arm64/fixes, thanks! --
Florian

The RP1 driver can load an overlay at runtime to describe the inner peripherals. This has led to a lot of confusion regarding the naming of nodes, their topology and the reclaiming of related node resources.

Since the overlay is currently not fully functional, drop its support in the driver in favor of the fully described static DT.

This also means that this driver does not depend on CONFIG_PCI_DYNAMIC_OF_NODES and no longer requires PCI quirks to dynamically create the intermediate PCI nodes.

---
 drivers/misc/rp1/Kconfig      |  6 +-----
 drivers/misc/rp1/Makefile     |  3 +--
 drivers/misc/rp1/rp1-pci.dtso | 25 -----------------------
 drivers/misc/rp1/rp1_pci.c    | 37 ++++-------------------------------
 drivers/pci/quirks.c          |  1 -
 5 files changed, 6 insertions(+), 66 deletions(-)
 delete mode 100644 drivers/misc/rp1/rp1-pci.dtso

index 5232e70d3079b..2c13b3968b011 100644
@@ -5,8 +5,7 @@
 
 config MISC_RP1
 	tristate "RaspberryPi RP1 misc device"
-	depends on OF_IRQ && OF_OVERLAY && PCI_MSI && PCI_QUIRKS
-	select PCI_DYNAMIC_OF_NODES
+	depends on OF_IRQ && PCI_MSI
 	help
 	  Support the RP1 peripheral chip found on Raspberry Pi 5 board.
 

@@ -15,6 +14,3 @@ config MISC_RP1
 
 	  The driver is responsible for enabling the DT node once the PCIe
 	  endpoint has been configured, and handling interrupts.
-
-	  This driver uses an overlay to load other drivers to support for
-	  RP1 internal sub-devices.

index 508b4cb056277..ab32b433d7ede 100644
@@ -1,3 +1,2 @@
 # SPDX-License-Identifier: GPL-2.0-only
-obj-$(CONFIG_MISC_RP1)		+= rp1-pci.o
-rp1-pci-objs			:= rp1_pci.o rp1-pci.dtbo.o
+obj-$(CONFIG_MISC_RP1)		+= rp1_pci.o

deleted file mode 100644
index eea826b36e029..0000000000000
@@ -1,25 +0,0 @@
-// SPDX-License-Identifier: (GPL-2.0 OR MIT)
-
-/*
- * The dts overlay is included from the dts directory so
- * it can be possible to check it with CHECK_DTBS while
- * also compile it from the driver source directory.
- */
-
-/dts-v1/;
-/plugin/;
-
-/ {
-	fragment@0 {
-		target-path="";
-		__overlay__ {
-			compatible = "pci1de4,1";
-			#address-cells = <3>;
-			#size-cells = <2>;
-			interrupt-controller;
-			#interrupt-cells = <2>;
-
-			#include "arm64/broadcom/rp1-common.dtsi"
-		};
-	};
-};

index a342bcc6164bb..d210da84c30a2 100644
@@ -34,16 +34,11 @@
 /* Interrupts */
 #define RP1_INT_END		61
 
-/* Embedded dtbo symbols created by cmd_wrap_S_dtb in scripts/Makefile.lib */
-extern char __dtbo_rp1_pci_begin[];
-extern char __dtbo_rp1_pci_end[];
-
 struct rp1_dev {
 	struct pci_dev *pdev;
 	struct irq_domain *domain;
 	struct irq_data *pcie_irqds[64];
 	void __iomem *bar1;
-	int ovcs_id;	/* overlay changeset id */
 	bool level_triggered_irq[RP1_INT_END];
 };
 

@@ -184,24 +179,13 @@ static void rp1_unregister_interrupts(struct pci_dev *pdev)
 
 static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
-	u32 dtbo_size = __dtbo_rp1_pci_end - __dtbo_rp1_pci_begin;
-	void *dtbo_start = __dtbo_rp1_pci_begin;
 	struct device *dev = &pdev->dev;
 	struct device_node *rp1_node;
-	bool skip_ovl = true;
 	struct rp1_dev *rp1;
 	int err = 0;
 	int i;
 
-	/*
-	 * Either use rp1_nexus node if already present in DT, or
-	 * set a flag to load it from overlay at runtime
-	 */
-	rp1_node = of_find_node_by_name(NULL, "rp1_nexus");
-	if (!rp1_node) {
-		rp1_node = dev_of_node(dev);
-		skip_ovl = false;
-	}
+	rp1_node = dev_of_node(dev);
 
 	if (!rp1_node) {
 		dev_err(dev, "Missing of_node for device\n");

@@ -276,42 +260,29 @@ static int rp1_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 						 rp1_chained_handle_irq, rp1);
 	}
 
-	if (!skip_ovl) {
-		err = of_overlay_fdt_apply(dtbo_start, dtbo_size, &rp1->ovcs_id,
-					   rp1_node);
-		if (err)
-			goto err_unregister_interrupts;
-	}
-
 	err = of_platform_default_populate(rp1_node, NULL, dev);
 	if (err) {
 		dev_err_probe(&pdev->dev, err, "Error populating devicetree\n");
-		goto err_unload_overlay;
+		goto err_unregister_interrupts;
 	}
 
-	if (skip_ovl)
-		of_node_put(rp1_node);
+	of_node_put(rp1_node);
 
 	return 0;
 
-err_unload_overlay:
-	of_overlay_remove(&rp1->ovcs_id);
 err_unregister_interrupts:
 	rp1_unregister_interrupts(pdev);
 err_put_node:
-	if (skip_ovl)
-		of_node_put(rp1_node);
+	of_node_put(rp1_node);
 
 	return err;
 }
 
 static void rp1_remove(struct pci_dev *pdev)
 {
-	struct rp1_dev *rp1 = pci_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
 
 	of_platform_depopulate(dev);
-	of_overlay_remove(&rp1->ovcs_id);
 	rp1_unregister_interrupts(pdev);
 }
 

index b9c252aa6fe08..280cd50d693bd 100644
@@ -6308,7 +6308,6 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5020, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_XILINX, 0x5021, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_REDHAT, 0x0005, of_pci_make_dev_node);
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_EFAR, 0x9660, of_pci_make_dev_node);
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_RPI, PCI_DEVICE_ID_RPI_RP1_C0, of_pci_make_dev_node);
 
 /*
  * Devices known to require a longer delay before first config space access

2.35.3

From: Florian Fainelli <[email protected]>

Applied to https://github.com/Broadcom/stblinux/commits/devicetree-arm64/fixes, thanks! --
Florian

The node describing the RP1 endpoint currently uses a specific name ('rp1_nexus') that does not correctly reflect the PCI topology.

Update the DT with the correct topology and use generic node names.

Additionally, since the driver dropped overlay support in favor of a fully described DT, rename '...-ovl-rp1.dts' to '...-base.dtsi' for inclusion in the board DTB, as it is no longer compiled as a standalone DTB.

---
The DTC compiler produces the following warning:

WARNING: DT compatible string vendor "pci1de4" appears un-documented -- check ./Documentation/devicetree/bindings/vendor-prefixes.yaml #91: FILE: arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b.dts:31:

                        compatible = "pci1de4,1";

I'm not sure wheter I should add that compatible to vendor-prefixes.yaml or change the compatible to something already recognized as a vendor, such as 'raspberrypi,pci1de4,1'.
I'd prefer the former to be consistent to what would be filled by enabling CONFIG_PCI_DYNAMIC_OF_NODES. Any hint will be really appreciated.

---
 arch/arm64/boot/dts/broadcom/Makefile         |  1 -
 ...-ovl-rp1.dts => bcm2712-rpi-5-b-base.dtsi} |  0
 .../boot/dts/broadcom/bcm2712-rpi-5-b.dts     | 39 ++++++++++++-------
 3 files changed, 26 insertions(+), 14 deletions(-)
 rename arch/arm64/boot/dts/broadcom/{bcm2712-rpi-5-b-ovl-rp1.dts => bcm2712-rpi-5-b-base.dtsi} (100%)

index 83d45afc6588e..d43901404c955 100644
@@ -7,7 +7,6 @@ dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-400.dtb \
 			      bcm2711-rpi-4-b.dtb \
 			      bcm2711-rpi-cm4-io.dtb \
 			      bcm2712-rpi-5-b.dtb \
-			      bcm2712-rpi-5-b-ovl-rp1.dtb \
 			      bcm2712-d-rpi-5-b.dtb \
 			      bcm2837-rpi-2-b.dtb \
 			      bcm2837-rpi-3-a-plus.dtb \

similarity index 100%
rename from arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b-ovl-rp1.dts
rename to arch/arm64/boot/dts/broadcom/bcm2712-rpi-5-b-base.dtsi

index 3e0319fdb93f7..2856082814462 100644
@@ -1,22 +1,16 @@
 // SPDX-License-Identifier: (GPL-2.0 OR MIT)
 /*
- * bcm2712-rpi-5-b-ovl-rp1.dts is the overlay-ready DT which will make
- * the RP1 driver to load the RP1 dtb overlay at runtime, while
- * bcm2712-rpi-5-b.dts (this file) is the fully defined one (i.e. it
- * already contains RP1 node, so no overlay is loaded nor needed).
- * This file is intended to host the override nodes for the RP1 peripherals,
- * e.g. to declare the phy of the ethernet interface or the custom pin setup
- * for several RP1 peripherals.
- * This in turn is due to the fact that there's no current generic
- * infrastructure to reference nodes (i.e. the nodes in rp1-common.dtsi) that
- * are not yet defined in the DT since they are loaded at runtime via overlay.
+ * As a loose attempt to separate RP1 customizations from SoC peripherals
+ * definitioni, this file is intended to host the override nodes for the RP1
+ * peripherals, e.g. to declare the phy of the ethernet interface or custom
+ * pin setup.
  * All other nodes that do not have anything to do with RP1 should be added
- * to the included bcm2712-rpi-5-b-ovl-rp1.dts instead.
+ * to the included bcm2712-rpi-5-b-base.dtsi instead.
  */
 
 /dts-v1/;
 
-#include "bcm2712-rpi-5-b-ovl-rp1.dts"
+#include "bcm2712-rpi-5-b-base.dtsi"
 
 / {
 	aliases {

@@ -25,7 +19,26 @@ aliases {
 };
 
 &pcie2 {
-	#include "rp1-nexus.dtsi"
+	pci@0,0 {
+		reg = <0x0 0x0 0x0 0x0 0x0>;
+		ranges;
+		bus-range = <0 1>;
+		device_type = "pci";
+		#address-cells = <3>;
+		#size-cells = <2>;
+
+		dev@0,0 {
+			compatible = "pci1de4,1";
+			reg = <0x10000 0x0 0x0 0x0 0x0>;
+			ranges = <0x1 0x0 0x0 0x82010000 0x0 0x0 0x0 0x400000>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			#include "rp1-common.dtsi"
+		};
+	};
 };
 
 &rp1_eth {

2.35.3

From: Florian Fainelli <[email protected]>

Applied to https://github.com/Broadcom/stblinux/commits/devicetree-arm64/fixes, thanks! --
Florian

RP1 support loaded from overlay has been dropped from the driver and the DTB intended to be loaded with the overlay no longer exists.

Drop unused include file and overlay.

---
Just a heads up: This patch removes rp1.dtbo from the Makefile, just like this [1] proposed patch also does, but the merge is trivial.

[1] - https://lore.kernel.org/all/[email protected]/

---
 arch/arm64/boot/dts/broadcom/Makefile       |  3 +--
 arch/arm64/boot/dts/broadcom/rp1-nexus.dtsi | 14 --------------
 arch/arm64/boot/dts/broadcom/rp1.dtso       | 11 -----------
 3 files changed, 1 insertion(+), 27 deletions(-)
 delete mode 100644 arch/arm64/boot/dts/broadcom/rp1-nexus.dtsi
 delete mode 100644 arch/arm64/boot/dts/broadcom/rp1.dtso

index d43901404c955..01ecfa3041845 100644
@@ -13,8 +13,7 @@ dtb-$(CONFIG_ARCH_BCM2835) += bcm2711-rpi-400.dtb \
 			      bcm2837-rpi-3-b.dtb \
 			      bcm2837-rpi-3-b-plus.dtb \
 			      bcm2837-rpi-cm3-io3.dtb \
-			      bcm2837-rpi-zero-2-w.dtb \
-			      rp1.dtbo
+			      bcm2837-rpi-zero-2-w.dtb
 
 subdir-y	+= bcmbca
 subdir-y	+= northstar2

deleted file mode 100644
index 0ef30d7f1c352..0000000000000
@@ -1,14 +0,0 @@
-// SPDX-License-Identifier: (GPL-2.0 OR MIT)
-
-rp1_nexus {
-	compatible = "pci1de4,1";
-	#address-cells = <3>;
-	#size-cells = <2>;
-	ranges = <0x01 0x00 0x00000000
-		  0x02000000 0x00 0x00000000
-		  0x0 0x400000>;
-	interrupt-controller;
-	#interrupt-cells = <2>;
-
-	#include "rp1-common.dtsi"
-};

deleted file mode 100644
index ab4f146d22c06..0000000000000
@@ -1,11 +0,0 @@
-// SPDX-License-Identifier: (GPL-2.0 OR MIT)
-
-/dts-v1/;
-/plugin/;
-
-&pcie2 {
-	#address-cells = <3>;
-	#size-cells = <2>;
-
-	#include "rp1-nexus.dtsi"
-};

2.35.3

From: Florian Fainelli <[email protected]>

Applied to https://github.com/Broadcom/stblinux/commits/devicetree-arm64/fixes, thanks! --
Florian

Timing could not have been better as I was just going to apply Rob's patch:

https://lore.kernel.org/all/[email protected]

Yours is definitively more complete. Rob, Krzysztof, can you review this series and I will fast track it today.

Thanks!

Florian

Thanks for doing this.