[PATCH v2 0/3] RK3588 PCIe2 support

[PATCH v2 0/3] RK3588 PCIe2 support

[Sebastian Reichel]

Hi,

This adds PCIe v2 support for RK3588. The series has been tested with
the onboard RTL8125 network card on Rockchip RK3588 EVB1 (&pcie2x1l1)
and Radxa Rock 5B (&pcie2x1l2).

Changes since v1:
 * https://lore.kernel.org/all/20230616170022.76107-1-sebastian.reichel@collabora.com/
 * Dropped patch adding 'RK3588' (queued by Rob)
 * Updated patch adding legacy-interrupt-controller according to comments
   from Rob and Serge
    - added missing additionalProperties: false
    - added all properties to new required property
    - removed useless quotes around interrupt-controller
    - added Rob's Reviewed-by
 * Updated patch adding the missing RK356x/RK3588 interrupt names, so that it
   provides more details about the interrupts
 * Updated the DT patch according to the comment from Jonas Karlman, so that
   the addresses are in 32 bit address space starting at 0x40000000

[0] https://lore.kernel.org/all/20230612171337.74576-1-sebastian.reichel@collabora.com/

Thanks,

-- Sebastian

Sebastian Reichel (3):
  dt-bindings: PCI: dwc: rockchip: Fix interrupt-names issue
  dt-bindings: PCI: dwc: rockchip: Add missing
    legacy-interrupt-controller
  arm64: dts: rockchip: rk3588: add PCIe2 support

 .../bindings/pci/rockchip-dw-pcie.yaml        |  48 ++++++++
 .../devicetree/bindings/pci/snps,dw-pcie.yaml |  76 +++++++++++-
 arch/arm64/boot/dts/rockchip/rk3588.dtsi      |  54 +++++++++
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 108 ++++++++++++++++++
 4 files changed, 285 insertions(+), 1 deletion(-)

-- 
2.40.1

[PATCH v2 1/3] dt-bindings: PCI: dwc: rockchip: Fix interrupt-names issue

[Sebastian Reichel]

The RK356x (and RK3588) have 5 ganged interrupts. For example the
"legacy" interrupt combines "inta/intb/intc/intd" with a register
providing the details.

Currently the binding is not specifying these interrupts resulting
in a bunch of errors for all rk356x boards using PCIe.

Fix this by specifying the interrupts and add them to the example
to prevent regressions.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../bindings/pci/rockchip-dw-pcie.yaml        | 18 +++++
 .../devicetree/bindings/pci/snps,dw-pcie.yaml | 76 ++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
index a4f61ced5e88..aad53c7d8485 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -60,6 +60,17 @@ properties:
       - const: aux
       - const: pipe
 
+  interrupts:
+    maxItems: 5
+
+  interrupt-names:
+    items:
+      - const: sys
+      - const: pmc
+      - const: msg
+      - const: legacy
+      - const: err
+
   msi-map: true
 
   num-lanes: true
@@ -108,6 +119,7 @@ unevaluatedProperties: false
 
 examples:
   - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
 
     bus {
         #address-cells = <2>;
@@ -127,6 +139,12 @@ examples:
                           "aclk_dbi", "pclk",
                           "aux";
             device_type = "pci";
+            interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "sys", "pmc", "msg", "legacy", "err";
             linux,pci-domain = <2>;
             max-link-speed = <2>;
             msi-map = <0x2000 &its 0x2000 0x1000>;
diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
index 1a83f0f65f19..973bf8f2730d 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
@@ -193,9 +193,83 @@ properties:
           oneOf:
             - description: See native "app" IRQ for details
               enum: [ intr ]
+        - description:
+            Combined legacy interrupt, which is used to signal the following
+            interrupts
+              * inta
+              * intb
+              * intc
+              * intd
+          const: legacy
+        - description:
+            Combined system interrupt, which is used to signal the following
+            interrupts
+              * phy_link_up
+              * dll_link_up
+              * link_req_rst_not
+              * hp_pme
+              * hp
+              * hp_msi
+              * link_auto_bw
+              * link_auto_bw_msi
+              * bw_mgt
+              * bw_mgt_msi
+              * edma_wr
+              * edma_rd
+              * dpa_sub_upd
+              * rbar_update
+              * link_eq_req
+              * ep_elbi_app
+          const: sys
+        - description:
+            Combined PM interrupt, which is used to signal the following
+            interrupts
+              * linkst_in_l1sub
+              * linkst_in_l1
+              * linkst_in_l2
+              * linkst_in_l0s
+              * linkst_out_l1sub
+              * linkst_out_l1
+              * linkst_out_l2
+              * linkst_out_l0s
+              * pm_dstate_update
+          const: pmc
+        - description:
+            Combined message interrupt, which is used to signal the following
+            interrupts
+              * ven_msg
+              * unlock_msg
+              * ltr_msg
+              * cfg_pme
+              * cfg_pme_msi
+              * pm_pme
+              * pm_to_ack
+              * pm_turnoff
+              * obff_idle
+              * obff_obff
+              * obff_cpu_active
+          const: msg
+        - description:
+            Combined error interrupt, which is used to signal the following
+            interrupts
+              * aer_rc_err
+              * aer_rc_err_msi
+              * rx_cpl_timeout
+              * tx_cpl_timeout
+              * cor_err_sent
+              * nf_err_sent
+              * f_err_sent
+              * cor_err_rx
+              * nf_err_rx
+              * f_err_rx
+              * radm_qoverflow
+          const: err
+
     allOf:
       - contains:
-          const: msi
+          enum:
+            - msi
+            - msg
 
 additionalProperties: true
 
-- 
2.40.1

[PATCH v2 2/3] dt-bindings: PCI: dwc: rockchip: Add missing legacy-interrupt-controller

[Sebastian Reichel]

Rockchip RK356x and RK3588 handle legacy interrupts via a ganged
interrupts. The RK356x DT implements this via a sub-node named
"legacy-interrupt-controller", just like a couple of other PCIe
implementations. This adds proper documentation for this and updates
the example to avoid regressions.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../bindings/pci/rockchip-dw-pcie.yaml        | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
index aad53c7d8485..8460b1a87248 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -71,6 +71,28 @@ properties:
       - const: legacy
       - const: err
 
+  legacy-interrupt-controller:
+    description: Interrupt controller node for handling legacy PCI interrupts.
+    type: object
+    additionalProperties: false
+    properties:
+      "#address-cells":
+        const: 0
+
+      "#interrupt-cells":
+        const: 1
+
+      interrupt-controller: true
+
+      interrupts:
+        items:
+          - description: combined legacy interrupt
+    required:
+      - "#address-cells"
+      - "#interrupt-cells"
+      - interrupt-controller
+      - interrupts
+
   msi-map: true
 
   num-lanes: true
@@ -158,6 +180,14 @@ examples:
             reset-names = "pipe";
             #address-cells = <3>;
             #size-cells = <2>;
+
+            legacy-interrupt-controller {
+                interrupt-controller;
+                #address-cells = <0>;
+                #interrupt-cells = <1>;
+                interrupt-parent = <&gic>;
+                interrupts = <GIC_SPI 72 IRQ_TYPE_EDGE_RISING>;
+            };
         };
     };
 ...
-- 
2.40.1

[PATCH v2 3/3] arm64: dts: rockchip: rk3588: add PCIe2 support

[Sebastian Reichel]

Add all three PCIe2 IP blocks to the RK3588 DT. Note, that RK3588
also has two PCIe3 IP blocks, that will be handled separately.

Co-developed-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588.dtsi  |  54 +++++++++++
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 108 ++++++++++++++++++++++
 2 files changed, 162 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
index 6be9bf81c09c..4d66ca6c2e4c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
@@ -80,6 +80,60 @@ i2s10_8ch: i2s@fde00000 {
 		status = "disabled";
 	};
 
+	pcie2x1l0: pcie@fe170000 {
+		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		bus-range = <0x20 0x2f>;
+		clocks = <&cru ACLK_PCIE_1L0_MSTR>, <&cru ACLK_PCIE_1L0_SLV>,
+			 <&cru ACLK_PCIE_1L0_DBI>, <&cru PCLK_PCIE_1L0>,
+			 <&cru CLK_PCIE_AUX2>, <&cru CLK_PCIE1L0_PIPE>;
+		clock-names = "aclk_mst", "aclk_slv",
+			      "aclk_dbi", "pclk",
+			      "aux", "pipe";
+		device_type = "pci";
+		interrupts = <GIC_SPI 243 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 242 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 241 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 239 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie2x1l0_intc 0>,
+				<0 0 0 2 &pcie2x1l0_intc 1>,
+				<0 0 0 3 &pcie2x1l0_intc 2>,
+				<0 0 0 4 &pcie2x1l0_intc 3>;
+		linux,pci-domain = <2>;
+		num-ib-windows = <8>;
+		num-ob-windows = <8>;
+		num-viewport = <4>;
+		max-link-speed = <2>;
+		msi-map = <0x2000 &its0 0x2000 0x1000>;
+		num-lanes = <1>;
+		phys = <&combphy1_ps PHY_TYPE_PCIE>;
+		phy-names = "pcie-phy";
+		power-domains = <&power RK3588_PD_PCIE>;
+		ranges = <0x01000000 0x0 0xf2100000 0x0 0xf2100000 0x0 0x00100000>,
+			 <0x02000000 0x0 0xf2200000 0x0 0xf2200000 0x0 0x00e00000>,
+			 <0x03000000 0x0 0x40000000 0x9 0x80000000 0x0 0x40000000>;
+		reg = <0xa 0x40800000 0x0 0x00400000>,
+		      <0x0 0xfe170000 0x0 0x00010000>,
+		      <0x0 0xf2000000 0x0 0x00100000>;
+		reg-names = "dbi", "apb", "config";
+		resets = <&cru SRST_PCIE2_POWER_UP>, <&cru SRST_P_PCIE2>;
+		reset-names = "pwr", "pipe";
+		status = "disabled";
+
+		pcie2x1l0_intc: legacy-interrupt-controller {
+			interrupt-controller;
+			#address-cells = <0>;
+			#interrupt-cells = <1>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SPI 240 IRQ_TYPE_EDGE_RISING 0>;
+		};
+	};
+
 	gmac0: ethernet@fe1b0000 {
 		compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
 		reg = <0x0 0xfe1b0000 0x0 0x10000>;
diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index c9f9dd2472f5..27d711d114d6 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -1227,6 +1227,114 @@ qos_vop_m1: qos@fdf82200 {
 		reg = <0x0 0xfdf82200 0x0 0x20>;
 	};
 
+	pcie2x1l1: pcie@fe180000 {
+		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		bus-range = <0x30 0x3f>;
+		clocks = <&cru ACLK_PCIE_1L1_MSTR>, <&cru ACLK_PCIE_1L1_SLV>,
+			 <&cru ACLK_PCIE_1L1_DBI>, <&cru PCLK_PCIE_1L1>,
+			 <&cru CLK_PCIE_AUX3>, <&cru CLK_PCIE1L1_PIPE>;
+		clock-names = "aclk_mst", "aclk_slv",
+			      "aclk_dbi", "pclk",
+			      "aux", "pipe";
+		device_type = "pci";
+		interrupts = <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 246 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie2x1l1_intc 0>,
+				<0 0 0 2 &pcie2x1l1_intc 1>,
+				<0 0 0 3 &pcie2x1l1_intc 2>,
+				<0 0 0 4 &pcie2x1l1_intc 3>;
+		linux,pci-domain = <3>;
+		num-ib-windows = <8>;
+		num-ob-windows = <8>;
+		num-viewport = <4>;
+		max-link-speed = <2>;
+		msi-map = <0x3000 &its0 0x3000 0x1000>;
+		num-lanes = <1>;
+		phys = <&combphy2_psu PHY_TYPE_PCIE>;
+		phy-names = "pcie-phy";
+		power-domains = <&power RK3588_PD_PCIE>;
+		ranges = <0x01000000 0x0 0xf3100000 0x0 0xf3100000 0x0 0x00100000>,
+			 <0x02000000 0x0 0xf3200000 0x0 0xf3200000 0x0 0x00e00000>,
+			 <0x03000000 0x0 0x40000000 0x9 0xc0000000 0x0 0x40000000>;
+		reg = <0xa 0x40c00000 0x0 0x00400000>,
+		      <0x0 0xfe180000 0x0 0x00010000>,
+		      <0x0 0xf3000000 0x0 0x00100000>;
+		reg-names = "dbi", "apb", "config";
+		resets = <&cru SRST_PCIE3_POWER_UP>, <&cru SRST_P_PCIE3>;
+		reset-names = "pwr", "pipe";
+		status = "disabled";
+
+		pcie2x1l1_intc: legacy-interrupt-controller {
+			interrupt-controller;
+			#address-cells = <0>;
+			#interrupt-cells = <1>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SPI 245 IRQ_TYPE_EDGE_RISING 0>;
+		};
+	};
+
+	pcie2x1l2: pcie@fe190000 {
+		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
+		#address-cells = <3>;
+		#size-cells = <2>;
+		bus-range = <0x40 0x4f>;
+		clocks = <&cru ACLK_PCIE_1L2_MSTR>, <&cru ACLK_PCIE_1L2_SLV>,
+			 <&cru ACLK_PCIE_1L2_DBI>, <&cru PCLK_PCIE_1L2>,
+			 <&cru CLK_PCIE_AUX4>, <&cru CLK_PCIE1L2_PIPE>;
+		clock-names = "aclk_mst", "aclk_slv",
+			      "aclk_dbi", "pclk",
+			      "aux", "pipe";
+		device_type = "pci";
+		interrupts = <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 250 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 249 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 7>;
+		interrupt-map = <0 0 0 1 &pcie2x1l2_intc 0>,
+				<0 0 0 2 &pcie2x1l2_intc 1>,
+				<0 0 0 3 &pcie2x1l2_intc 2>,
+				<0 0 0 4 &pcie2x1l2_intc 3>;
+		linux,pci-domain = <4>;
+		num-ib-windows = <8>;
+		num-ob-windows = <8>;
+		num-viewport = <4>;
+		max-link-speed = <2>;
+		msi-map = <0x4000 &its0 0x4000 0x1000>;
+		num-lanes = <1>;
+		phys = <&combphy0_ps PHY_TYPE_PCIE>;
+		phy-names = "pcie-phy";
+		power-domains = <&power RK3588_PD_PCIE>;
+		ranges = <0x01000000 0x0 0xf4100000 0x0 0xf4100000 0x0 0x00100000>,
+			 <0x02000000 0x0 0xf4200000 0x0 0xf4200000 0x0 0x00e00000>,
+			 <0x03000000 0x0 0x40000000 0xa 0x00000000 0x0 0x40000000>;
+		reg = <0xa 0x41000000 0x0 0x00400000>,
+		      <0x0 0xfe190000 0x0 0x00010000>,
+		      <0x0 0xf4000000 0x0 0x00100000>;
+		reg-names = "dbi", "apb", "config";
+		resets = <&cru SRST_PCIE4_POWER_UP>, <&cru SRST_P_PCIE4>;
+		reset-names = "pwr", "pipe";
+		status = "disabled";
+
+		pcie2x1l2_intc: legacy-interrupt-controller {
+			interrupt-controller;
+			#address-cells = <0>;
+			#interrupt-cells = <1>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SPI 250 IRQ_TYPE_EDGE_RISING 0>;
+		};
+	};
+
 	gmac1: ethernet@fe1c0000 {
 		compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
 		reg = <0x0 0xfe1c0000 0x0 0x10000>;
-- 
2.40.1

Re: [PATCH v2 0/3] RK3588 PCIe2 support

[Heiko Stuebner]

Hi Sebastian,

Am Donnerstag, 13. Juli 2023, 19:18:48 CEST schrieb Sebastian Reichel:
> Hi,
> 
> This adds PCIe v2 support for RK3588. The series has been tested with
> the onboard RTL8125 network card on Rockchip RK3588 EVB1 (&pcie2x1l1)
> and Radxa Rock 5B (&pcie2x1l2).
> 
> Changes since v1:

Didn't Rob write that he applied patches 1-3 of the v1 series? [0]
or did I miss further communication somehow?

Thanks
Heiko

[0] https://lore.kernel.org/r/20230626193238.GA3553158-robh@kernel.org

>  * https://lore.kernel.org/all/20230616170022.76107-1-sebastian.reichel@collabora.com/
>  * Dropped patch adding 'RK3588' (queued by Rob)
>  * Updated patch adding legacy-interrupt-controller according to comments
>    from Rob and Serge
>     - added missing additionalProperties: false
>     - added all properties to new required property
>     - removed useless quotes around interrupt-controller
>     - added Rob's Reviewed-by
>  * Updated patch adding the missing RK356x/RK3588 interrupt names, so that it
>    provides more details about the interrupts
>  * Updated the DT patch according to the comment from Jonas Karlman, so that
>    the addresses are in 32 bit address space starting at 0x40000000
> 
> [0] https://lore.kernel.org/all/20230612171337.74576-1-sebastian.reichel@collabora.com/
> 
> Thanks,
> 
> -- Sebastian
> 
> Sebastian Reichel (3):
>   dt-bindings: PCI: dwc: rockchip: Fix interrupt-names issue
>   dt-bindings: PCI: dwc: rockchip: Add missing
>     legacy-interrupt-controller
>   arm64: dts: rockchip: rk3588: add PCIe2 support
> 
>  .../bindings/pci/rockchip-dw-pcie.yaml        |  48 ++++++++
>  .../devicetree/bindings/pci/snps,dw-pcie.yaml |  76 +++++++++++-
>  arch/arm64/boot/dts/rockchip/rk3588.dtsi      |  54 +++++++++
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 108 ++++++++++++++++++
>  4 files changed, 285 insertions(+), 1 deletion(-)
> 
> 

Re: [PATCH v2 0/3] RK3588 PCIe2 support

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

Hi Heiko,

On Thu, Jul 13, 2023 at 10:09:29PM +0200, Heiko Stuebner wrote:
> Am Donnerstag, 13. Juli 2023, 19:18:48 CEST schrieb Sebastian Reichel:
> > This adds PCIe v2 support for RK3588. The series has been tested with
> > the onboard RTL8125 network card on Rockchip RK3588 EVB1 (&pcie2x1l1)
> > and Radxa Rock 5B (&pcie2x1l2).
> 
> Didn't Rob write that he applied patches 1-3 of the v1 series? [0]
> or did I miss further communication somehow?
> [0] https://lore.kernel.org/r/20230626193238.GA3553158-robh@kernel.org

Rob wrote, that he de-applied the first patch:

https://lore.kernel.org/all/CAL_Jsq+=kBrujhLW_KNRWpj+DQJbnXrA=RS3La5ekbJtji+xiQ@mail.gmail.com/

It seems the second one was also dropped, since I rebased on top of
6.5-rc1, which only had patch 3/4.

FWIW the remaining dt-bindings fix issues that also exist for
RK356x, so I guess there is no strict dependency. It might be
acceptable to merge the DTS patch already, so that we finally
get working network on Rock 5B. That would temporarily introduce
DT warnings though (RK3588 is currently warning-free).

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/3] RK3588 PCIe2 support

[Heiko Stuebner]

Hi Sebastian,

Am Donnerstag, 13. Juli 2023, 22:48:44 CEST schrieb Sebastian Reichel:
> On Thu, Jul 13, 2023 at 10:09:29PM +0200, Heiko Stuebner wrote:
> > Am Donnerstag, 13. Juli 2023, 19:18:48 CEST schrieb Sebastian Reichel:
> > > This adds PCIe v2 support for RK3588. The series has been tested with
> > > the onboard RTL8125 network card on Rockchip RK3588 EVB1 (&pcie2x1l1)
> > > and Radxa Rock 5B (&pcie2x1l2).
> > 
> > Didn't Rob write that he applied patches 1-3 of the v1 series? [0]
> > or did I miss further communication somehow?
> > [0] https://lore.kernel.org/r/20230626193238.GA3553158-robh@kernel.org
> 
> Rob wrote, that he de-applied the first patch:
> 
> https://lore.kernel.org/all/CAL_Jsq+=kBrujhLW_KNRWpj+DQJbnXrA=RS3La5ekbJtji+xiQ@mail.gmail.com/
> 
> It seems the second one was also dropped, since I rebased on top of
> 6.5-rc1, which only had patch 3/4.

thanks for the clarification :-) .

> FWIW the remaining dt-bindings fix issues that also exist for
> RK356x, so I guess there is no strict dependency. It might be
> acceptable to merge the DTS patch already, so that we finally
> get working network on Rock 5B. That would temporarily introduce
> DT warnings though (RK3588 is currently warning-free).

We're still early in the release cycle, so I guess we should
just give Rob(?) time to pick up the binding patches if they
look ok to him. And not introduce new warnings ;-)


Heiko

Re: [PATCH v2 3/3] arm64: dts: rockchip: rk3588: add PCIe2 support

[Jagan Teki]

On Thu, 13 Jul 2023 at 22:49, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Add all three PCIe2 IP blocks to the RK3588 DT. Note, that RK3588
> also has two PCIe3 IP blocks, that will be handled separately.
>
> Co-developed-by: Kever Yang <kever.yang@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---

Tested all pcie2x1l0 and pcie2x1l1 IP blocks in edgeble-neu6a, 6b.

Tested-by: Jagan Teki <jagan@edgeble.ai> # edgeble-neu6a, 6b
Reviewed-by: Jagan Teki <jagan@edgeble.ai>

Re: [PATCH v2 1/3] dt-bindings: PCI: dwc: rockchip: Fix interrupt-names issue

[Serge Semin]

Hi Sebastian

On Thu, Jul 13, 2023 at 07:18:49PM +0200, Sebastian Reichel wrote:
> The RK356x (and RK3588) have 5 ganged interrupts. For example the
> "legacy" interrupt combines "inta/intb/intc/intd" with a register
> providing the details.
> 
> Currently the binding is not specifying these interrupts resulting
> in a bunch of errors for all rk356x boards using PCIe.
> 
> Fix this by specifying the interrupts and add them to the example
> to prevent regressions.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  .../bindings/pci/rockchip-dw-pcie.yaml        | 18 +++++
>  .../devicetree/bindings/pci/snps,dw-pcie.yaml | 76 ++++++++++++++++++-
>  2 files changed, 93 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index a4f61ced5e88..aad53c7d8485 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -60,6 +60,17 @@ properties:
>        - const: aux
>        - const: pipe
>  
> +  interrupts:
> +    maxItems: 5
> +
> +  interrupt-names:
> +    items:
> +      - const: sys
> +      - const: pmc
> +      - const: msg
> +      - const: legacy
> +      - const: err
> +
>    msi-map: true
>  
>    num-lanes: true
> @@ -108,6 +119,7 @@ unevaluatedProperties: false
>  
>  examples:
>    - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>  
>      bus {
>          #address-cells = <2>;
> @@ -127,6 +139,12 @@ examples:
>                            "aclk_dbi", "pclk",
>                            "aux";
>              device_type = "pci";
> +            interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "sys", "pmc", "msg", "legacy", "err";
>              linux,pci-domain = <2>;
>              max-link-speed = <2>;
>              msi-map = <0x2000 &its 0x2000 0x1000>;
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> index 1a83f0f65f19..973bf8f2730d 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> @@ -193,9 +193,83 @@ properties:
>            oneOf:
>              - description: See native "app" IRQ for details
>                enum: [ intr ]
> +        - description:
> +            Combined legacy interrupt, which is used to signal the following
> +            interrupts
> +              * inta
> +              * intb
> +              * intc
> +              * intd
> +          const: legacy
> +        - description:
> +            Combined system interrupt, which is used to signal the following
> +            interrupts
> +              * phy_link_up
> +              * dll_link_up
> +              * link_req_rst_not
> +              * hp_pme
> +              * hp
> +              * hp_msi
> +              * link_auto_bw
> +              * link_auto_bw_msi
> +              * bw_mgt
> +              * bw_mgt_msi
> +              * edma_wr
> +              * edma_rd
> +              * dpa_sub_upd
> +              * rbar_update
> +              * link_eq_req
> +              * ep_elbi_app
> +          const: sys
> +        - description:
> +            Combined PM interrupt, which is used to signal the following
> +            interrupts
> +              * linkst_in_l1sub
> +              * linkst_in_l1
> +              * linkst_in_l2
> +              * linkst_in_l0s
> +              * linkst_out_l1sub
> +              * linkst_out_l1
> +              * linkst_out_l2
> +              * linkst_out_l0s
> +              * pm_dstate_update
> +          const: pmc
> +        - description:
> +            Combined message interrupt, which is used to signal the following
> +            interrupts
> +              * ven_msg
> +              * unlock_msg
> +              * ltr_msg
> +              * cfg_pme
> +              * cfg_pme_msi
> +              * pm_pme
> +              * pm_to_ack
> +              * pm_turnoff

> +              * obff_idle
> +              * obff_obff
> +              * obff_cpu_active

These are marked is "inputs" (from the DW PCIe controller point of
view) in the HW manual. Are you sure they are supposed to generate any
IRQ? Based on the DW PCIe HW-manual they are supposed to be set by the
_application_ (a.k.a your driver or vendor-specific RTL block) as a
request to the DW PCIe controller to emit an OBFF message. There is a
signal marked as "output" and named as "app_obff_msg_grant" which most
likely is relevant here.

> +          const: msg
> +        - description:
> +            Combined error interrupt, which is used to signal the following
> +            interrupts
> +              * aer_rc_err
> +              * aer_rc_err_msi
> +              * rx_cpl_timeout
> +              * tx_cpl_timeout
> +              * cor_err_sent
> +              * nf_err_sent
> +              * f_err_sent
> +              * cor_err_rx
> +              * nf_err_rx
> +              * f_err_rx
> +              * radm_qoverflow
> +          const: err

The most of the signals you cited in the description properties are a
part of the so called "System Information Interface" defined in the DW
PCIe databook. Here is what the doc says regarding these signals:

"The SII exchanges various system-related information between the
controller and your application. Most of the SII signals are provided
for flexibility. Your application is not required to use all of the
SII signals. Your application logic is expected to drive and monitor
the signals that it needs to function correctly. SII inputs that your
application does not require, must be driven to 0."

Amongst tons of various informational signals available in the
framework of SII, there is "SII: Interrupt Signals" which are normally
utilized by the vendor-specific controller implementations and which
are defined as generic in this DT-bindings. (MSI IRQ signal is defined
separately from SII as "MSI Interface Signals".)

What is normally expected is that all the generic SII IRQs are
supplied as the separate signals meanwhile the rest of the SII signals
are combined in an additional line named like "app".

In your case we find an intermix of the SII generic IRQs and some SII
signals (though some of the names listed in your descriptions don't
match to what is defined in the DW PCIe HW manual). So what you said
in v1:

On Thu, Jul 13, 2023 at 7:47PM +0200, Sebastian Reichel wrote:
> I suppose "sys", "pmc", "msg" and "err" all fit for "app", since
> they are vendor specific with the extra layer? But obviously I
> cannot specify "app" more than once."

is mainly correct. For instance, the most of the generic SII interrupt
signals are combined in your "sys" IRQ, like "hp", "bw_au", "bw_mg",
"dma", "l_eq"; your "pmc" and "msg" IRQs are a set of the SII signals
not listed in the "SII Interrupt Signals" list; the "err" IRQ has the
"aer" generic SII Interrupt, but the rest of the signals are common SII
signals.

I am not fully certain of what to do in this case. Some possible options:

1. Keep the names defined as is, add them to the list of generic IRQ
names, describe them as "Combined IRQ signals" but with no specific
signals listed and with some generic meaningful description.
Alternatively create a separate sub-schema in the generic
"interrupt-names" property constraints in the same way as it's done
for the "vendor-specific IRQ names" and do the same with the names
descriptions. In anyway move your detailed descriptions to the
Rockchip DW PCIe DT-schema. In this case we imply that your names
could be re-used for some other device bindings.

2. Keep the names defined as is, add them to the list of
"vendor-specific IRQ names" sub-schema in the "interrupt-names"
property, describe each of them as "Combined IRQ signals" but with no
specific signals listed and with some generic meaningful description.
Move your detailed descriptions to the Rockchip DW PCIe DT-schema.

3. Add "app_" prefix to all your IRQs (except "legacy") and convert
the generic "app" IRQ name constraint to accepting a pattern like
'^app(_.*)?$' or similar. Move your detailed descriptions to the
Rockchip DW PCIe DT-schema.

4. Add Rockchip-specific prefix to the names (except "legacy"), add
all of them (for instance as a pattern-like schema) to the
vendor-specific IRQ names part of the "interrupt-names" items list
with a description referring to the Rockchip DT-bindings. Move your
detailed descriptions to the Rockchip DW PCIe DT-schema.

Doubtfully the categorization chosen by the Rockchip HW designers is
fully universal so the names could be utilized for other devices. Thus
IMO the options 2-4 might be more preferable over 1.

In anyway the detailed descriptions with the listed lines should be
in the Rockchip DW PCIe DT-bindings since they are definitely
vendor-specific.

Regarding the "legacy" name used as a combined "int(a|b|c|d)" IRQ.
Alas we can't change it. So it's either option 1 or 2. 

What do you think? Rob, Krzysztof, any better idea?

> +

>      allOf:
>        - contains:
> -          const: msi
> +          enum:
> +            - msi
> +            - msg

Based on the above the "msg" interrupt doesn't get to be required.
Rob, is it possible to have a constraint which would require either
the "msi" IRQ name or the "msi-map" DT-property or both?

-Serge(y)

>  
>  additionalProperties: true
>  
> -- 
> 2.40.1
> 

Re: [PATCH v2 3/3] arm64: dts: rockchip: rk3588: add PCIe2 support

[Serge Semin]

On Thu, Jul 13, 2023 at 07:18:51PM +0200, Sebastian Reichel wrote:
> Add all three PCIe2 IP blocks to the RK3588 DT. Note, that RK3588
> also has two PCIe3 IP blocks, that will be handled separately.
> 
> Co-developed-by: Kever Yang <kever.yang@rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588.dtsi  |  54 +++++++++++
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 108 ++++++++++++++++++++++
>  2 files changed, 162 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
> index 6be9bf81c09c..4d66ca6c2e4c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
> @@ -80,6 +80,60 @@ i2s10_8ch: i2s@fde00000 {
>  		status = "disabled";
>  	};
>  
> +	pcie2x1l0: pcie@fe170000 {
> +		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		bus-range = <0x20 0x2f>;
> +		clocks = <&cru ACLK_PCIE_1L0_MSTR>, <&cru ACLK_PCIE_1L0_SLV>,
> +			 <&cru ACLK_PCIE_1L0_DBI>, <&cru PCLK_PCIE_1L0>,
> +			 <&cru CLK_PCIE_AUX2>, <&cru CLK_PCIE1L0_PIPE>;
> +		clock-names = "aclk_mst", "aclk_slv",
> +			      "aclk_dbi", "pclk",
> +			      "aux", "pipe";
> +		device_type = "pci";
> +		interrupts = <GIC_SPI 243 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 242 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 241 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 240 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 239 IRQ_TYPE_LEVEL_HIGH 0>;
> +		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 7>;
> +		interrupt-map = <0 0 0 1 &pcie2x1l0_intc 0>,
> +				<0 0 0 2 &pcie2x1l0_intc 1>,
> +				<0 0 0 3 &pcie2x1l0_intc 2>,
> +				<0 0 0 4 &pcie2x1l0_intc 3>;
> +		linux,pci-domain = <2>;

> +		num-ib-windows = <8>;
> +		num-ob-windows = <8>;
> +		num-viewport = <4>;

These properties are marked as deprecated and unused by the driver
anyway. You can freely drop them.

> +		max-link-speed = <2>;
> +		msi-map = <0x2000 &its0 0x2000 0x1000>;
> +		num-lanes = <1>;
> +		phys = <&combphy1_ps PHY_TYPE_PCIE>;
> +		phy-names = "pcie-phy";
> +		power-domains = <&power RK3588_PD_PCIE>;
> +		ranges = <0x01000000 0x0 0xf2100000 0x0 0xf2100000 0x0 0x00100000>,
> +			 <0x02000000 0x0 0xf2200000 0x0 0xf2200000 0x0 0x00e00000>,
> +			 <0x03000000 0x0 0x40000000 0x9 0x80000000 0x0 0x40000000>;
> +		reg = <0xa 0x40800000 0x0 0x00400000>,
> +		      <0x0 0xfe170000 0x0 0x00010000>,
> +		      <0x0 0xf2000000 0x0 0x00100000>;
> +		reg-names = "dbi", "apb", "config";
> +		resets = <&cru SRST_PCIE2_POWER_UP>, <&cru SRST_P_PCIE2>;
> +		reset-names = "pwr", "pipe";
> +		status = "disabled";
> +
> +		pcie2x1l0_intc: legacy-interrupt-controller {
> +			interrupt-controller;
> +			#address-cells = <0>;
> +			#interrupt-cells = <1>;
> +			interrupt-parent = <&gic>;
> +			interrupts = <GIC_SPI 240 IRQ_TYPE_EDGE_RISING 0>;
> +		};
> +	};
> +
>  	gmac0: ethernet@fe1b0000 {
>  		compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
>  		reg = <0x0 0xfe1b0000 0x0 0x10000>;
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index c9f9dd2472f5..27d711d114d6 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -1227,6 +1227,114 @@ qos_vop_m1: qos@fdf82200 {
>  		reg = <0x0 0xfdf82200 0x0 0x20>;
>  	};
>  
> +	pcie2x1l1: pcie@fe180000 {
> +		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		bus-range = <0x30 0x3f>;
> +		clocks = <&cru ACLK_PCIE_1L1_MSTR>, <&cru ACLK_PCIE_1L1_SLV>,
> +			 <&cru ACLK_PCIE_1L1_DBI>, <&cru PCLK_PCIE_1L1>,
> +			 <&cru CLK_PCIE_AUX3>, <&cru CLK_PCIE1L1_PIPE>;
> +		clock-names = "aclk_mst", "aclk_slv",
> +			      "aclk_dbi", "pclk",
> +			      "aux", "pipe";
> +		device_type = "pci";
> +		interrupts = <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 246 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH 0>;
> +		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 7>;
> +		interrupt-map = <0 0 0 1 &pcie2x1l1_intc 0>,
> +				<0 0 0 2 &pcie2x1l1_intc 1>,
> +				<0 0 0 3 &pcie2x1l1_intc 2>,
> +				<0 0 0 4 &pcie2x1l1_intc 3>;
> +		linux,pci-domain = <3>;

> +		num-ib-windows = <8>;
> +		num-ob-windows = <8>;
> +		num-viewport = <4>;

ditto

> +		max-link-speed = <2>;
> +		msi-map = <0x3000 &its0 0x3000 0x1000>;
> +		num-lanes = <1>;
> +		phys = <&combphy2_psu PHY_TYPE_PCIE>;
> +		phy-names = "pcie-phy";
> +		power-domains = <&power RK3588_PD_PCIE>;
> +		ranges = <0x01000000 0x0 0xf3100000 0x0 0xf3100000 0x0 0x00100000>,
> +			 <0x02000000 0x0 0xf3200000 0x0 0xf3200000 0x0 0x00e00000>,
> +			 <0x03000000 0x0 0x40000000 0x9 0xc0000000 0x0 0x40000000>;
> +		reg = <0xa 0x40c00000 0x0 0x00400000>,
> +		      <0x0 0xfe180000 0x0 0x00010000>,
> +		      <0x0 0xf3000000 0x0 0x00100000>;
> +		reg-names = "dbi", "apb", "config";
> +		resets = <&cru SRST_PCIE3_POWER_UP>, <&cru SRST_P_PCIE3>;
> +		reset-names = "pwr", "pipe";
> +		status = "disabled";
> +
> +		pcie2x1l1_intc: legacy-interrupt-controller {
> +			interrupt-controller;
> +			#address-cells = <0>;
> +			#interrupt-cells = <1>;
> +			interrupt-parent = <&gic>;
> +			interrupts = <GIC_SPI 245 IRQ_TYPE_EDGE_RISING 0>;
> +		};
> +	};
> +
> +	pcie2x1l2: pcie@fe190000 {
> +		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		bus-range = <0x40 0x4f>;
> +		clocks = <&cru ACLK_PCIE_1L2_MSTR>, <&cru ACLK_PCIE_1L2_SLV>,
> +			 <&cru ACLK_PCIE_1L2_DBI>, <&cru PCLK_PCIE_1L2>,
> +			 <&cru CLK_PCIE_AUX4>, <&cru CLK_PCIE1L2_PIPE>;
> +		clock-names = "aclk_mst", "aclk_slv",
> +			      "aclk_dbi", "pclk",
> +			      "aux", "pipe";
> +		device_type = "pci";
> +		interrupts = <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 250 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 249 IRQ_TYPE_LEVEL_HIGH 0>;
> +		interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 7>;
> +		interrupt-map = <0 0 0 1 &pcie2x1l2_intc 0>,
> +				<0 0 0 2 &pcie2x1l2_intc 1>,
> +				<0 0 0 3 &pcie2x1l2_intc 2>,
> +				<0 0 0 4 &pcie2x1l2_intc 3>;
> +		linux,pci-domain = <4>;

> +		num-ib-windows = <8>;
> +		num-ob-windows = <8>;
> +		num-viewport = <4>;

ditto

-Serge(y)

> +		max-link-speed = <2>;
> +		msi-map = <0x4000 &its0 0x4000 0x1000>;
> +		num-lanes = <1>;
> +		phys = <&combphy0_ps PHY_TYPE_PCIE>;
> +		phy-names = "pcie-phy";
> +		power-domains = <&power RK3588_PD_PCIE>;
> +		ranges = <0x01000000 0x0 0xf4100000 0x0 0xf4100000 0x0 0x00100000>,
> +			 <0x02000000 0x0 0xf4200000 0x0 0xf4200000 0x0 0x00e00000>,
> +			 <0x03000000 0x0 0x40000000 0xa 0x00000000 0x0 0x40000000>;
> +		reg = <0xa 0x41000000 0x0 0x00400000>,
> +		      <0x0 0xfe190000 0x0 0x00010000>,
> +		      <0x0 0xf4000000 0x0 0x00100000>;
> +		reg-names = "dbi", "apb", "config";
> +		resets = <&cru SRST_PCIE4_POWER_UP>, <&cru SRST_P_PCIE4>;
> +		reset-names = "pwr", "pipe";
> +		status = "disabled";
> +
> +		pcie2x1l2_intc: legacy-interrupt-controller {
> +			interrupt-controller;
> +			#address-cells = <0>;
> +			#interrupt-cells = <1>;
> +			interrupt-parent = <&gic>;
> +			interrupts = <GIC_SPI 250 IRQ_TYPE_EDGE_RISING 0>;
> +		};
> +	};
> +
>  	gmac1: ethernet@fe1c0000 {
>  		compatible = "rockchip,rk3588-gmac", "snps,dwmac-4.20a";
>  		reg = <0x0 0xfe1c0000 0x0 0x10000>;
> -- 
> 2.40.1
> 

Re: [PATCH v2 1/3] dt-bindings: PCI: dwc: rockchip: Fix interrupt-names issue

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 12143 bytes --]

Hi Serge,

On Fri, Jul 14, 2023 at 08:25:52PM +0300, Serge Semin wrote:
> On Thu, Jul 13, 2023 at 07:18:49PM +0200, Sebastian Reichel wrote:
> > The RK356x (and RK3588) have 5 ganged interrupts. For example the
> > "legacy" interrupt combines "inta/intb/intc/intd" with a register
> > providing the details.
> > 
> > Currently the binding is not specifying these interrupts resulting
> > in a bunch of errors for all rk356x boards using PCIe.
> > 
> > Fix this by specifying the interrupts and add them to the example
> > to prevent regressions.
> > 
> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > ---
> >  .../bindings/pci/rockchip-dw-pcie.yaml        | 18 +++++
> >  .../devicetree/bindings/pci/snps,dw-pcie.yaml | 76 ++++++++++++++++++-
> >  2 files changed, 93 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > index a4f61ced5e88..aad53c7d8485 100644
> > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > @@ -60,6 +60,17 @@ properties:
> >        - const: aux
> >        - const: pipe
> >  
> > +  interrupts:
> > +    maxItems: 5
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: sys
> > +      - const: pmc
> > +      - const: msg
> > +      - const: legacy
> > +      - const: err
> > +
> >    msi-map: true
> >  
> >    num-lanes: true
> > @@ -108,6 +119,7 @@ unevaluatedProperties: false
> >  
> >  examples:
> >    - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  
> >      bus {
> >          #address-cells = <2>;
> > @@ -127,6 +139,12 @@ examples:
> >                            "aclk_dbi", "pclk",
> >                            "aux";
> >              device_type = "pci";
> > +            interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
> > +                         <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
> > +            interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> >              linux,pci-domain = <2>;
> >              max-link-speed = <2>;
> >              msi-map = <0x2000 &its 0x2000 0x1000>;
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > index 1a83f0f65f19..973bf8f2730d 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > @@ -193,9 +193,83 @@ properties:
> >            oneOf:
> >              - description: See native "app" IRQ for details
> >                enum: [ intr ]
> > +        - description:
> > +            Combined legacy interrupt, which is used to signal the following
> > +            interrupts
> > +              * inta
> > +              * intb
> > +              * intc
> > +              * intd
> > +          const: legacy
> > +        - description:
> > +            Combined system interrupt, which is used to signal the following
> > +            interrupts
> > +              * phy_link_up
> > +              * dll_link_up
> > +              * link_req_rst_not
> > +              * hp_pme
> > +              * hp
> > +              * hp_msi
> > +              * link_auto_bw
> > +              * link_auto_bw_msi
> > +              * bw_mgt
> > +              * bw_mgt_msi
> > +              * edma_wr
> > +              * edma_rd
> > +              * dpa_sub_upd
> > +              * rbar_update
> > +              * link_eq_req
> > +              * ep_elbi_app
> > +          const: sys
> > +        - description:
> > +            Combined PM interrupt, which is used to signal the following
> > +            interrupts
> > +              * linkst_in_l1sub
> > +              * linkst_in_l1
> > +              * linkst_in_l2
> > +              * linkst_in_l0s
> > +              * linkst_out_l1sub
> > +              * linkst_out_l1
> > +              * linkst_out_l2
> > +              * linkst_out_l0s
> > +              * pm_dstate_update
> > +          const: pmc
> > +        - description:
> > +            Combined message interrupt, which is used to signal the following
> > +            interrupts
> > +              * ven_msg
> > +              * unlock_msg
> > +              * ltr_msg
> > +              * cfg_pme
> > +              * cfg_pme_msi
> > +              * pm_pme
> > +              * pm_to_ack
> > +              * pm_turnoff
> 
> > +              * obff_idle
> > +              * obff_obff
> > +              * obff_cpu_active
> 
> These are marked is "inputs" (from the DW PCIe controller point of
> view) in the HW manual. Are you sure they are supposed to generate any
> IRQ? Based on the DW PCIe HW-manual they are supposed to be set by the
> _application_ (a.k.a your driver or vendor-specific RTL block) as a
> request to the DW PCIe controller to emit an OBFF message. There is a
> signal marked as "output" and named as "app_obff_msg_grant" which most
> likely is relevant here.

I do not have the Synopsys HW manual, but RK3588 TRM has these:

obff_idle_int - controller received an 'IDLE' OBFF message.
obff_obff_int - controller received an 'OBFF' OBFF message.
obff_cpu_active_int - controller received an 'CPU Active' OBFF message.

> > +          const: msg
> > +        - description:
> > +            Combined error interrupt, which is used to signal the following
> > +            interrupts
> > +              * aer_rc_err
> > +              * aer_rc_err_msi
> > +              * rx_cpl_timeout
> > +              * tx_cpl_timeout
> > +              * cor_err_sent
> > +              * nf_err_sent
> > +              * f_err_sent
> > +              * cor_err_rx
> > +              * nf_err_rx
> > +              * f_err_rx
> > +              * radm_qoverflow
> > +          const: err
> 
> The most of the signals you cited in the description properties are a
> part of the so called "System Information Interface" defined in the DW
> PCIe databook. Here is what the doc says regarding these signals:
> 
> "The SII exchanges various system-related information between the
> controller and your application. Most of the SII signals are provided
> for flexibility. Your application is not required to use all of the
> SII signals. Your application logic is expected to drive and monitor
> the signals that it needs to function correctly. SII inputs that your
> application does not require, must be driven to 0."
> 
> Amongst tons of various informational signals available in the
> framework of SII, there is "SII: Interrupt Signals" which are normally
> utilized by the vendor-specific controller implementations and which
> are defined as generic in this DT-bindings. (MSI IRQ signal is defined
> separately from SII as "MSI Interface Signals".)
> 
> What is normally expected is that all the generic SII IRQs are
> supplied as the separate signals meanwhile the rest of the SII signals
> are combined in an additional line named like "app".
> 
> In your case we find an intermix of the SII generic IRQs and some SII
> signals (though some of the names listed in your descriptions don't
> match to what is defined in the DW PCIe HW manual). So what you said
> in v1:
> 
> On Thu, Jul 13, 2023 at 7:47PM +0200, Sebastian Reichel wrote:
> > I suppose "sys", "pmc", "msg" and "err" all fit for "app", since
> > they are vendor specific with the extra layer? But obviously I
> > cannot specify "app" more than once."
> 
> is mainly correct. For instance, the most of the generic SII interrupt
> signals are combined in your "sys" IRQ, like "hp", "bw_au", "bw_mg",
> "dma", "l_eq"; your "pmc" and "msg" IRQs are a set of the SII signals
> not listed in the "SII Interrupt Signals" list; the "err" IRQ has the
> "aer" generic SII Interrupt, but the rest of the signals are common SII
> signals.
> 
> I am not fully certain of what to do in this case. Some possible options:
> 
> 1. Keep the names defined as is, add them to the list of generic IRQ
> names, describe them as "Combined IRQ signals" but with no specific
> signals listed and with some generic meaningful description.
> Alternatively create a separate sub-schema in the generic
> "interrupt-names" property constraints in the same way as it's done
> for the "vendor-specific IRQ names" and do the same with the names
> descriptions. In anyway move your detailed descriptions to the
> Rockchip DW PCIe DT-schema. In this case we imply that your names
> could be re-used for some other device bindings.
> 
> 2. Keep the names defined as is, add them to the list of
> "vendor-specific IRQ names" sub-schema in the "interrupt-names"
> property, describe each of them as "Combined IRQ signals" but with no
> specific signals listed and with some generic meaningful description.
> Move your detailed descriptions to the Rockchip DW PCIe DT-schema.
> 
> 3. Add "app_" prefix to all your IRQs (except "legacy") and convert
> the generic "app" IRQ name constraint to accepting a pattern like
> '^app(_.*)?$' or similar. Move your detailed descriptions to the
> Rockchip DW PCIe DT-schema.
> 
> 4. Add Rockchip-specific prefix to the names (except "legacy"), add
> all of them (for instance as a pattern-like schema) to the
> vendor-specific IRQ names part of the "interrupt-names" items list
> with a description referring to the Rockchip DT-bindings. Move your
> detailed descriptions to the Rockchip DW PCIe DT-schema.
> 
> Doubtfully the categorization chosen by the Rockchip HW designers is
> fully universal so the names could be utilized for other devices. Thus
> IMO the options 2-4 might be more preferable over 1.
> 
> In anyway the detailed descriptions with the listed lines should be
> in the Rockchip DW PCIe DT-bindings since they are definitely
> vendor-specific.
> 
> Regarding the "legacy" name used as a combined "int(a|b|c|d)" IRQ.
> Alas we can't change it. So it's either option 1 or 2. 
> 
> What do you think? Rob, Krzysztof, any better idea?

Adding extra description to the interrupt-names list in the Rockchip
specific binding file does not work:

Additional properties are not allowed ('description' was unexpected)

But I suppose we are fine without them. People working on Rockchip
will easily find them in the TRM.

After looking at it again, my suggestion is to do the following
replacement in the Rockchip specific binding:

allOf:
  - $ref: /schemas/pci/snps,dw-pcie.yaml#

with

allOf:
  - $ref: /schemas/pci/pci-bus.yaml#
  - $ref: /schemas/pci/snps,dw-pcie-common.yaml#

Then the generic binding can stay as is. The Rockchip binding does
not use the "snps,dw-pcie" fallback compatible.

> >      allOf:
> >        - contains:
> > -          const: msi
> > +          enum:
> > +            - msi
> > +            - msg
> 
> Based on the above the "msg" interrupt doesn't get to be required.
> Rob, is it possible to have a constraint which would require either
> the "msi" IRQ name or the "msi-map" DT-property or both?

I played around a little bit and found this solution: Remove the
above allOf and instead add this to the allOf at the root level:

allOf:
  - if:
      not:
        required:
          - msi-map
    then:
      properties:
        interrupt-names:
         contains:
            const: msi

That might be sensible to do in the generic binding even when
Rockchip is no longer using it. The Rockchip binding has msi-map
in its required list, so it's fine without this.

Greetings,

-- Sebastian

> 
> -Serge(y)
> 
> >  
> >  additionalProperties: true
> >  
> > -- 
> > 2.40.1
> > 
> 
> -- 
> To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/3] dt-bindings: PCI: dwc: rockchip: Fix interrupt-names issue

[Serge Semin]

On Sat, Jul 15, 2023 at 12:44:44AM +0200, Sebastian Reichel wrote:
> Hi Serge,
> 
> On Fri, Jul 14, 2023 at 08:25:52PM +0300, Serge Semin wrote:
> > On Thu, Jul 13, 2023 at 07:18:49PM +0200, Sebastian Reichel wrote:
> > > The RK356x (and RK3588) have 5 ganged interrupts. For example the
> > > "legacy" interrupt combines "inta/intb/intc/intd" with a register
> > > providing the details.
> > > 
> > > Currently the binding is not specifying these interrupts resulting
> > > in a bunch of errors for all rk356x boards using PCIe.
> > > 
> > > Fix this by specifying the interrupts and add them to the example
> > > to prevent regressions.
> > > 
> > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> > > ---
> > >  .../bindings/pci/rockchip-dw-pcie.yaml        | 18 +++++
> > >  .../devicetree/bindings/pci/snps,dw-pcie.yaml | 76 ++++++++++++++++++-
> > >  2 files changed, 93 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > index a4f61ced5e88..aad53c7d8485 100644
> > > --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> > > @@ -60,6 +60,17 @@ properties:
> > >        - const: aux
> > >        - const: pipe
> > >  
> > > +  interrupts:
> > > +    maxItems: 5
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: sys
> > > +      - const: pmc
> > > +      - const: msg
> > > +      - const: legacy
> > > +      - const: err
> > > +
> > >    msi-map: true
> > >  
> > >    num-lanes: true
> > > @@ -108,6 +119,7 @@ unevaluatedProperties: false
> > >  
> > >  examples:
> > >    - |
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > >  
> > >      bus {
> > >          #address-cells = <2>;
> > > @@ -127,6 +139,12 @@ examples:
> > >                            "aclk_dbi", "pclk",
> > >                            "aux";
> > >              device_type = "pci";
> > > +            interrupts = <GIC_SPI 160 IRQ_TYPE_LEVEL_HIGH>,
> > > +                         <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>,
> > > +                         <GIC_SPI 158 IRQ_TYPE_LEVEL_HIGH>,
> > > +                         <GIC_SPI 157 IRQ_TYPE_LEVEL_HIGH>,
> > > +                         <GIC_SPI 156 IRQ_TYPE_LEVEL_HIGH>;
> > > +            interrupt-names = "sys", "pmc", "msg", "legacy", "err";
> > >              linux,pci-domain = <2>;
> > >              max-link-speed = <2>;
> > >              msi-map = <0x2000 &its 0x2000 0x1000>;
> > > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > index 1a83f0f65f19..973bf8f2730d 100644
> > > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie.yaml
> > > @@ -193,9 +193,83 @@ properties:
> > >            oneOf:
> > >              - description: See native "app" IRQ for details
> > >                enum: [ intr ]
> > > +        - description:
> > > +            Combined legacy interrupt, which is used to signal the following
> > > +            interrupts
> > > +              * inta
> > > +              * intb
> > > +              * intc
> > > +              * intd
> > > +          const: legacy
> > > +        - description:
> > > +            Combined system interrupt, which is used to signal the following
> > > +            interrupts
> > > +              * phy_link_up
> > > +              * dll_link_up
> > > +              * link_req_rst_not
> > > +              * hp_pme
> > > +              * hp
> > > +              * hp_msi
> > > +              * link_auto_bw
> > > +              * link_auto_bw_msi
> > > +              * bw_mgt
> > > +              * bw_mgt_msi
> > > +              * edma_wr
> > > +              * edma_rd
> > > +              * dpa_sub_upd
> > > +              * rbar_update
> > > +              * link_eq_req
> > > +              * ep_elbi_app
> > > +          const: sys
> > > +        - description:
> > > +            Combined PM interrupt, which is used to signal the following
> > > +            interrupts
> > > +              * linkst_in_l1sub
> > > +              * linkst_in_l1
> > > +              * linkst_in_l2
> > > +              * linkst_in_l0s
> > > +              * linkst_out_l1sub
> > > +              * linkst_out_l1
> > > +              * linkst_out_l2
> > > +              * linkst_out_l0s
> > > +              * pm_dstate_update
> > > +          const: pmc
> > > +        - description:
> > > +            Combined message interrupt, which is used to signal the following
> > > +            interrupts
> > > +              * ven_msg
> > > +              * unlock_msg
> > > +              * ltr_msg
> > > +              * cfg_pme
> > > +              * cfg_pme_msi
> > > +              * pm_pme
> > > +              * pm_to_ack
> > > +              * pm_turnoff
> > 
> > > +              * obff_idle
> > > +              * obff_obff
> > > +              * obff_cpu_active
> > 
> > These are marked is "inputs" (from the DW PCIe controller point of
> > view) in the HW manual. Are you sure they are supposed to generate any
> > IRQ? Based on the DW PCIe HW-manual they are supposed to be set by the
> > _application_ (a.k.a your driver or vendor-specific RTL block) as a
> > request to the DW PCIe controller to emit an OBFF message. There is a
> > signal marked as "output" and named as "app_obff_msg_grant" which most
> > likely is relevant here.
> 
> I do not have the Synopsys HW manual, but RK3588 TRM has these:
> 
> obff_idle_int - controller received an 'IDLE' OBFF message.
> obff_obff_int - controller received an 'OBFF' OBFF message.
> obff_cpu_active_int - controller received an 'CPU Active' OBFF message.

I failed to find any signal indicating the OBFF messages reception.
There is only the explicitly described SII OBFF Message Generation
Signals. Anyway it's not that important. It might be some
Rockchip-specific RTL block detecting incoming OBFF messages.

> 
> > > +          const: msg
> > > +        - description:
> > > +            Combined error interrupt, which is used to signal the following
> > > +            interrupts
> > > +              * aer_rc_err
> > > +              * aer_rc_err_msi
> > > +              * rx_cpl_timeout
> > > +              * tx_cpl_timeout
> > > +              * cor_err_sent
> > > +              * nf_err_sent
> > > +              * f_err_sent
> > > +              * cor_err_rx
> > > +              * nf_err_rx
> > > +              * f_err_rx
> > > +              * radm_qoverflow
> > > +          const: err
> > 
> > The most of the signals you cited in the description properties are a
> > part of the so called "System Information Interface" defined in the DW
> > PCIe databook. Here is what the doc says regarding these signals:
> > 
> > "The SII exchanges various system-related information between the
> > controller and your application. Most of the SII signals are provided
> > for flexibility. Your application is not required to use all of the
> > SII signals. Your application logic is expected to drive and monitor
> > the signals that it needs to function correctly. SII inputs that your
> > application does not require, must be driven to 0."
> > 
> > Amongst tons of various informational signals available in the
> > framework of SII, there is "SII: Interrupt Signals" which are normally
> > utilized by the vendor-specific controller implementations and which
> > are defined as generic in this DT-bindings. (MSI IRQ signal is defined
> > separately from SII as "MSI Interface Signals".)
> > 
> > What is normally expected is that all the generic SII IRQs are
> > supplied as the separate signals meanwhile the rest of the SII signals
> > are combined in an additional line named like "app".
> > 
> > In your case we find an intermix of the SII generic IRQs and some SII
> > signals (though some of the names listed in your descriptions don't
> > match to what is defined in the DW PCIe HW manual). So what you said
> > in v1:
> > 
> > On Thu, Jul 13, 2023 at 7:47PM +0200, Sebastian Reichel wrote:
> > > I suppose "sys", "pmc", "msg" and "err" all fit for "app", since
> > > they are vendor specific with the extra layer? But obviously I
> > > cannot specify "app" more than once."
> > 
> > is mainly correct. For instance, the most of the generic SII interrupt
> > signals are combined in your "sys" IRQ, like "hp", "bw_au", "bw_mg",
> > "dma", "l_eq"; your "pmc" and "msg" IRQs are a set of the SII signals
> > not listed in the "SII Interrupt Signals" list; the "err" IRQ has the
> > "aer" generic SII Interrupt, but the rest of the signals are common SII
> > signals.
> > 
> > I am not fully certain of what to do in this case. Some possible options:
> > 
> > 1. Keep the names defined as is, add them to the list of generic IRQ
> > names, describe them as "Combined IRQ signals" but with no specific
> > signals listed and with some generic meaningful description.
> > Alternatively create a separate sub-schema in the generic
> > "interrupt-names" property constraints in the same way as it's done
> > for the "vendor-specific IRQ names" and do the same with the names
> > descriptions. In anyway move your detailed descriptions to the
> > Rockchip DW PCIe DT-schema. In this case we imply that your names
> > could be re-used for some other device bindings.
> > 
> > 2. Keep the names defined as is, add them to the list of
> > "vendor-specific IRQ names" sub-schema in the "interrupt-names"
> > property, describe each of them as "Combined IRQ signals" but with no
> > specific signals listed and with some generic meaningful description.
> > Move your detailed descriptions to the Rockchip DW PCIe DT-schema.
> > 
> > 3. Add "app_" prefix to all your IRQs (except "legacy") and convert
> > the generic "app" IRQ name constraint to accepting a pattern like
> > '^app(_.*)?$' or similar. Move your detailed descriptions to the
> > Rockchip DW PCIe DT-schema.
> > 
> > 4. Add Rockchip-specific prefix to the names (except "legacy"), add
> > all of them (for instance as a pattern-like schema) to the
> > vendor-specific IRQ names part of the "interrupt-names" items list
> > with a description referring to the Rockchip DT-bindings. Move your
> > detailed descriptions to the Rockchip DW PCIe DT-schema.
> > 
> > Doubtfully the categorization chosen by the Rockchip HW designers is
> > fully universal so the names could be utilized for other devices. Thus
> > IMO the options 2-4 might be more preferable over 1.
> > 
> > In anyway the detailed descriptions with the listed lines should be
> > in the Rockchip DW PCIe DT-bindings since they are definitely
> > vendor-specific.
> > 
> > Regarding the "legacy" name used as a combined "int(a|b|c|d)" IRQ.
> > Alas we can't change it. So it's either option 1 or 2. 
> > 
> > What do you think? Rob, Krzysztof, any better idea?
> 

> Adding extra description to the interrupt-names list in the Rockchip
> specific binding file does not work:
> 
> Additional properties are not allowed ('description' was unexpected)
> 
> But I suppose we are fine without them. People working on Rockchip
> will easily find them in the TRM.

You can add them to the "interrupts" property describing each array
entry in accordance with the "interrupt-names" items.

> 
> After looking at it again, my suggestion is to do the following
> replacement in the Rockchip specific binding:
> 
> allOf:
>   - $ref: /schemas/pci/snps,dw-pcie.yaml#
> 
> with
> 
> allOf:
>   - $ref: /schemas/pci/pci-bus.yaml#
>   - $ref: /schemas/pci/snps,dw-pcie-common.yaml#

I was hoping to avoid that. The snps,dw-pcie-common.yaml schema was
intended to describe the common parts of the DW PCIe Root Port and
End-point DT-bindings and not being used directly. The original idea
behind all the created DW PCIe RP/EP bindings was to bring some order
to the already over-diverged resources naming hell. In case of the
Rockchip PCIe controller we've got only the peculiar interrupt
signals. The rest of the resources comply fine with the generic schema
and it's pretty much hard do decide what solution to select to keep
all the resources described and to use the generic DW PCIe Root Port
DT-bindings.  It would be pity to fallback to using the
snps,dw-pcie-common.yaml schema just because of the interrupt names.

amlogic,axg-pcie.yaml schema already slipped in behind my back (and
BTW contains a bug in the "select" property which makes it not being
selected for the Amlogic PCIe nodes evaluation at all). Since I am not
the driver maintainer that will likely happen again over and over. Sigh...

Anyways if no better solution comes up from Rob or Krzysztof and they
didn't approve any option suggested by me above I guess your
suggestion might be selected then.

> 
> Then the generic binding can stay as is. The Rockchip binding does
> not use the "snps,dw-pcie" fallback compatible.

And it's a correct way to do since the fallback compatible is not that
useful. In this particular case it's just useless seeing there is a
low-level platform driver for the Rockchip PCIe host controller.

> 
> > >      allOf:
> > >        - contains:
> > > -          const: msi
> > > +          enum:
> > > +            - msi
> > > +            - msg
> > 
> > Based on the above the "msg" interrupt doesn't get to be required.
> > Rob, is it possible to have a constraint which would require either
> > the "msi" IRQ name or the "msi-map" DT-property or both?
> 
> I played around a little bit and found this solution: Remove the
> above allOf and instead add this to the allOf at the root level:
> 
> allOf:
>   - if:
>       not:
>         required:
>           - msi-map
>     then:
>       properties:
>         interrupt-names:
>          contains:
>             const: msi
> 
> That might be sensible to do in the generic binding even when
> Rockchip is no longer using it. The Rockchip binding has msi-map
> in its required list, so it's fine without this.

Right. Don't know how come this solution didn't cross my mind that
evening. Anyway indeed it would be useful to have it in the generic DW
PCIe Root Port DT-schema irrespective from the Rockchip PCIe host
controller DT-schema implementation.

-Serge(y)

> 
> Greetings,
> 
> -- Sebastian
> 
> > 
> > -Serge(y)
> > 
> > >  
> > >  additionalProperties: true
> > >  
> > > -- 
> > > 2.40.1
> > > 
> > 
> > -- 
> > To unsubscribe, send mail to kernel-unsubscribe@lists.collabora.co.uk.