[PATCH v4 00/13] PCI: dw-rockchip: Add endpoint mode support

[PATCH v4 00/13] PCI: dw-rockchip: Add endpoint mode support

[Niklas Cassel]

Hello all,

This series adds PCIe endpoint mode support for the rockchip rk3588 and
rk3568 SoCs.

This series is based on: pci/next
(git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git)

This series can also be found in git:
https://github.com/floatious/linux/commits/rockchip-pcie-ep-v4

Testing done:
This series has been tested with two rock5b:s, one running in RC mode and
one running in EP mode. This series has also been tested with an Intel x86
host and rock5b running in EP mode.

BAR4 exposes the ATU Port Logic Structure and the DMA Port Logic Structure
to the host. The EPC controller driver thus disables this BAR as init time,
because if it doesn't, when the host writes the test pattern to this BAR,
all the iATU settings will get wiped, resulting in all further BAR accesses
being non-functional.

Running pcitest.sh (modified to also perform the READ and WRITE tests with
the -d option, i.e. with DMA enabled) results in the following:

$ /usr/bin/pcitest.sh
BAR tests

BAR0:           OKAY
BAR1:           OKAY
BAR2:           OKAY
BAR3:           OKAY
BAR4:           NOT OKAY
BAR5:           OKAY

Interrupt tests

SET IRQ TYPE TO LEGACY:         OKAY
LEGACY IRQ:     NOT OKAY
SET IRQ TYPE TO MSI:            OKAY
MSI1:           OKAY
MSI2:           OKAY
MSI3:           OKAY
MSI4:           OKAY
MSI5:           OKAY
MSI6:           OKAY
MSI7:           OKAY
MSI8:           OKAY
MSI9:           OKAY
MSI10:          OKAY
MSI11:          OKAY
MSI12:          OKAY
MSI13:          OKAY
MSI14:          OKAY
MSI15:          OKAY
MSI16:          OKAY
MSI17:          OKAY
MSI18:          OKAY
MSI19:          OKAY
MSI20:          OKAY
MSI21:          OKAY
MSI22:          OKAY
MSI23:          OKAY
MSI24:          OKAY
MSI25:          OKAY
MSI26:          OKAY
MSI27:          OKAY
MSI28:          OKAY
MSI29:          OKAY
MSI30:          OKAY
MSI31:          OKAY
MSI32:          OKAY

SET IRQ TYPE TO MSI-X:          OKAY
MSI-X1:         OKAY
MSI-X2:         OKAY
MSI-X3:         OKAY
MSI-X4:         OKAY
MSI-X5:         OKAY
MSI-X6:         OKAY
MSI-X7:         OKAY
MSI-X8:         OKAY
MSI-X9:         OKAY
MSI-X10:                OKAY
MSI-X11:                OKAY
MSI-X12:                OKAY
MSI-X13:                OKAY
MSI-X14:                OKAY
MSI-X15:                OKAY
MSI-X16:                OKAY
MSI-X17:                OKAY
MSI-X18:                OKAY
MSI-X19:                OKAY
MSI-X20:                OKAY
MSI-X21:                OKAY
MSI-X22:                OKAY
MSI-X23:                OKAY
MSI-X24:                OKAY
MSI-X25:                OKAY
MSI-X26:                OKAY
MSI-X27:                OKAY
MSI-X28:                OKAY
MSI-X29:                OKAY
MSI-X30:                OKAY
MSI-X31:                OKAY
MSI-X32:                OKAY

Read Tests

SET IRQ TYPE TO MSI:            OKAY
READ (      1 bytes):           OKAY
READ (   1024 bytes):           OKAY
READ (   1025 bytes):           OKAY
READ (1024000 bytes):           OKAY
READ (1024001 bytes):           OKAY

Write Tests

WRITE (      1 bytes):          OKAY
WRITE (   1024 bytes):          OKAY
WRITE (   1025 bytes):          OKAY
WRITE (1024000 bytes):          OKAY
WRITE (1024001 bytes):          OKAY

Copy Tests

COPY (      1 bytes):           OKAY
COPY (   1024 bytes):           OKAY
COPY (   1025 bytes):           OKAY
COPY (1024000 bytes):           OKAY
COPY (1024001 bytes):           OKAY

Read Tests DMA

READ (      1 bytes):           OKAY
READ (   1024 bytes):           OKAY
READ (   1025 bytes):           OKAY
READ (1024000 bytes):           OKAY
READ (1024001 bytes):           OKAY

Write Tests DMA

WRITE (      1 bytes):          OKAY
WRITE (   1024 bytes):          OKAY
WRITE (   1025 bytes):          OKAY
WRITE (1024000 bytes):          OKAY
WRITE (1024001 bytes):          OKAY

Corresponding output on the EP side:
rockchip-dw-pcie a40000000.pcie-ep: EP cannot raise INTX IRQs
pci_epf_test pci_epf_test.0: WRITE => Size: 1 B, DMA: NO, Time: 0.000000292 s, Rate: 3424 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1024 B, DMA: NO, Time: 0.000007583 s, Rate: 135038 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1025 B, DMA: NO, Time: 0.000007584 s, Rate: 135152 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1024000 B, DMA: NO, Time: 0.009164167 s, Rate: 111739 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1024001 B, DMA: NO, Time: 0.009164458 s, Rate: 111736 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1 B, DMA: NO, Time: 0.000001750 s, Rate: 571 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1024 B, DMA: NO, Time: 0.000147875 s, Rate: 6924 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1025 B, DMA: NO, Time: 0.000149041 s, Rate: 6877 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1024000 B, DMA: NO, Time: 0.147537833 s, Rate: 6940 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: NO, Time: 0.147533750 s, Rate: 6940 KB/s
pci_epf_test pci_epf_test.0: COPY => Size: 1 B, DMA: NO, Time: 0.000003208 s, Rate: 311 KB/s
pci_epf_test pci_epf_test.0: COPY => Size: 1024 B, DMA: NO, Time: 0.000156625 s, Rate: 6537 KB/s
pci_epf_test pci_epf_test.0: COPY => Size: 1025 B, DMA: NO, Time: 0.000158375 s, Rate: 6471 KB/s
pci_epf_test pci_epf_test.0: COPY => Size: 1024000 B, DMA: NO, Time: 0.156902666 s, Rate: 6526 KB/s
pci_epf_test pci_epf_test.0: COPY => Size: 1024001 B, DMA: NO, Time: 0.156847833 s, Rate: 6528 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1 B, DMA: YES, Time: 0.000185500 s, Rate: 5 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1024 B, DMA: YES, Time: 0.000177334 s, Rate: 5774 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1025 B, DMA: YES, Time: 0.000178792 s, Rate: 5732 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1024000 B, DMA: YES, Time: 0.000486209 s, Rate: 2106090 KB/s
pci_epf_test pci_epf_test.0: WRITE => Size: 1024001 B, DMA: YES, Time: 0.000486791 s, Rate: 2103574 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1 B, DMA: YES, Time: 0.000177333 s, Rate: 5 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1024 B, DMA: YES, Time: 0.000177625 s, Rate: 5764 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1025 B, DMA: YES, Time: 0.000171208 s, Rate: 5986 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1024000 B, DMA: YES, Time: 0.000701167 s, Rate: 1460422 KB/s
pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: YES, Time: 0.000702625 s, Rate: 1457393 KB/s

Kind regards,
Niklas

---
Changes in v4:
- Rebased on pci/next
- Link to v3: https://lore.kernel.org/r/20240508-rockchip-pcie-ep-v1-v3-0-1748e202b084@kernel.org

Changes in v3:
- Renamed rockchip_pcie_ltssm() to rockchip_pcie_get_ltssm()
- Reworded some commit messages to avoid the term "patches".
- Dropped patch that added explicit rockchip,rk3588-pcie compatible
- Moved !IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_HOST) check to proper patch.
- Added comment in front of rk3588 epc_features describing why BAR4 is
  reserved.
- Added another struct epc_features for rk3568 as it does not have the
  ATU regs mapped to BAR4 (like rk3588 does).
- Picked up tags from Rob and Mani. Thank you!
- Link to v2: https://lore.kernel.org/r/20240430-rockchip-pcie-ep-v1-v2-0-a0f5ee2a77b6@kernel.org

Changes in v2:
- Rebased on v4 of the pci-epf-rework series that we depend on.
- Picked up tags from Rob.
- Split dw-rockchip DT binding in to common, RC and EP parts.
- Added support for rk3568 in DT binding and driver.
- Added a new patch that fixed "combined legacy IRQ description".
- Link to v1: https://lore.kernel.org/r/20240424-rockchip-pcie-ep-v1-v1-0-b1a02ddad650@kernel.org

---
Niklas Cassel (13):
      dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific reg-name
      dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific interrupt-names
      dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs
      dt-bindings: PCI: rockchip-dw-pcie: Prepare for Endpoint mode support
      dt-bindings: PCI: rockchip-dw-pcie: Fix description of legacy irq
      dt-bindings: rockchip: Add DesignWare based PCIe Endpoint controller
      PCI: dw-rockchip: Fix weird indentation
      PCI: dw-rockchip: Add rockchip_pcie_get_ltssm() helper
      PCI: dw-rockchip: Refactor the driver to prepare for EP mode
      PCI: dw-rockchip: Add endpoint mode support
      misc: pci_endpoint_test: Add support for rockchip rk3588
      arm64: dts: rockchip: Add PCIe endpoint mode support
      arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode

 .../bindings/pci/rockchip-dw-pcie-common.yaml      | 126 +++++++++
 .../bindings/pci/rockchip-dw-pcie-ep.yaml          |  95 +++++++
 .../devicetree/bindings/pci/rockchip-dw-pcie.yaml  |  93 +------
 .../devicetree/bindings/pci/snps,dw-pcie-ep.yaml   |  13 +-
 arch/arm64/boot/dts/rockchip/Makefile              |   5 +
 .../boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso  |  25 ++
 .../dts/rockchip/rk3588-rock-5b-pcie-srns.dtso     |  16 ++
 arch/arm64/boot/dts/rockchip/rk3588.dtsi           |  35 +++
 drivers/misc/pci_endpoint_test.c                   |  11 +
 drivers/pci/controller/dwc/Kconfig                 |  17 +-
 drivers/pci/controller/dwc/pcie-dw-rockchip.c      | 308 +++++++++++++++++++--
 11 files changed, 620 insertions(+), 124 deletions(-)
---
base-commit: e3fca37312892122d73f8c5293c0d1cc8c34500b
change-id: 20240424-rockchip-pcie-ep-v1-87c78b16d53c

Best regards,
-- 
Niklas Cassel <cassel@kernel.org>

[PATCH v4 01/13] dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific reg-name

[Niklas Cassel]

Considering that some drivers (e.g. pcie-dw-rockchip.c) already use the
reg-name "apb" for the device tree binding in Root Complex mode
(snps,dw-pcie.yaml), it doesn't make sense that those drivers should use a
different reg-name when running in Endpoint mode (snps,dw-pcie-ep.yaml).

Therefore, since "apb" is already defined in snps,dw-pcie.yaml, add it
also for snps,dw-pcie-ep.yaml.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
index bbdb01d22848..00dec01f1f73 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
@@ -100,7 +100,7 @@ properties:
             for new bindings.
           oneOf:
             - description: See native 'elbi/app' CSR region for details.
-              enum: [ link, appl ]
+              enum: [ apb, link, appl ]
             - description: See native 'atu' CSR region for details.
               enum: [ atu_dma ]
     allOf:

-- 
2.45.1

[PATCH v4 02/13] dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific interrupt-names

[Niklas Cassel]

Considering that some drivers (e.g. pcie-dw-rockchip.c) already use the
interrupt-names "sys", "pmc", "msg", "err" for the device tree binding in
Root Complex mode (snps,dw-pcie.yaml), it doesn't make sense that those
drivers should use different interrupt-names when running in Endpoint mode
(snps,dw-pcie-ep.yaml).

Therefore, since "sys", "pmc", "msg", "err" are already defined in
snps,dw-pcie.yaml, add them also for snps,dw-pcie-ep.yaml.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
index 00dec01f1f73..f5f12cbc2cb3 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
@@ -156,7 +156,7 @@ properties:
             for new bindings.
           oneOf:
             - description: See native "app" IRQ for details
-              enum: [ intr ]
+              enum: [ intr, sys, pmc, msg, err ]
 
   max-functions:
     maximum: 32

-- 
2.45.1

[PATCH v4 03/13] dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs

[Niklas Cassel]

The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
that are triggered when the PCIe controller (when running in Endpoint mode)
has sent an Assert_INTA Message to the upstream device.

Some DWC controllers have these interrupt in a combined interrupt signal.

Add the description of these interrupts to the device tree binding.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
index f5f12cbc2cb3..f474b9e3fc7e 100644
--- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
+++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
@@ -151,6 +151,15 @@ properties:
             Application-specific IRQ raised depending on the vendor-specific
             events basis.
           const: app
+        - description:
+            Interrupts triggered when the controller itself (in Endpoint mode)
+            has sent an Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message to
+            the upstream device.
+          pattern: "^tx_int(a|b|c|d)$"
+        - description:
+            Combined interrupt signal raised when the controller has sent an
+            Assert_INT{A,B,C,D} message. See "^tx_int(a|b|c|d)$" for details.
+          const: legacy
         - description:
             Vendor-specific IRQ names. Consider using the generic names above
             for new bindings.

-- 
2.45.1

[PATCH v4 04/13] dt-bindings: PCI: rockchip-dw-pcie: Prepare for Endpoint mode support

[Niklas Cassel]

Refactor the rockchip-dw-pcie binding to move generic properties to a new
rockchip-dw-pcie-common binding that can be shared by both RC and EP mode.

No functional change intended.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../bindings/pci/rockchip-dw-pcie-common.yaml      | 111 +++++++++++++++++++++
 .../devicetree/bindings/pci/rockchip-dw-pcie.yaml  |  93 +----------------
 2 files changed, 114 insertions(+), 90 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
new file mode 100644
index 000000000000..60d190a77580
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
@@ -0,0 +1,111 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie-common.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DesignWare based PCIe RC/EP controller on Rockchip SoCs
+
+maintainers:
+  - Shawn Lin <shawn.lin@rock-chips.com>
+  - Simon Xue <xxm@rock-chips.com>
+  - Heiko Stuebner <heiko@sntech.de>
+
+description: |+
+  Generic properties for the DesignWare based PCIe RC/EP controller on Rockchip
+  SoCs.
+
+properties:
+  clocks:
+    minItems: 5
+    items:
+      - description: AHB clock for PCIe master
+      - description: AHB clock for PCIe slave
+      - description: AHB clock for PCIe dbi
+      - description: APB clock for PCIe
+      - description: Auxiliary clock for PCIe
+      - description: PIPE clock
+      - description: Reference clock for PCIe
+
+  clock-names:
+    minItems: 5
+    items:
+      - const: aclk_mst
+      - const: aclk_slv
+      - const: aclk_dbi
+      - const: pclk
+      - const: aux
+      - const: pipe
+      - const: ref
+
+  interrupts:
+    items:
+      - 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
+      - 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
+      - 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
+      - description:
+          Combined legacy interrupt, which is used to signal the following
+          interrupts - inta, intb, intc, intd
+      - 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
+
+  interrupt-names:
+    items:
+      - const: sys
+      - const: pmc
+      - const: msg
+      - const: legacy
+      - const: err
+
+  num-lanes: true
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pcie-phy
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    minItems: 1
+    maxItems: 2
+
+  reset-names:
+    oneOf:
+      - const: pipe
+      - items:
+          - const: pwr
+          - const: pipe
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - num-lanes
+  - phys
+  - phy-names
+  - power-domains
+  - resets
+  - reset-names
+
+additionalProperties: true
+
+...
diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
index 5f719218c472..550d8a684af3 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: DesignWare based PCIe controller on Rockchip SoCs
+title: DesignWare based PCIe Root Complex controller on Rockchip SoCs
 
 maintainers:
   - Shawn Lin <shawn.lin@rock-chips.com>
@@ -12,12 +12,13 @@ maintainers:
   - Heiko Stuebner <heiko@sntech.de>
 
 description: |+
-  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
+  RK3568 SoC PCIe Root Complex controller is based on the Synopsys DesignWare
   PCIe IP and thus inherits all the common properties defined in
   snps,dw-pcie.yaml.
 
 allOf:
   - $ref: /schemas/pci/snps,dw-pcie.yaml#
+  - $ref: /schemas/pci/rockchip-dw-pcie-common.yaml#
 
 properties:
   compatible:
@@ -40,61 +41,6 @@ properties:
       - const: apb
       - const: config
 
-  clocks:
-    minItems: 5
-    items:
-      - description: AHB clock for PCIe master
-      - description: AHB clock for PCIe slave
-      - description: AHB clock for PCIe dbi
-      - description: APB clock for PCIe
-      - description: Auxiliary clock for PCIe
-      - description: PIPE clock
-      - description: Reference clock for PCIe
-
-  clock-names:
-    minItems: 5
-    items:
-      - const: aclk_mst
-      - const: aclk_slv
-      - const: aclk_dbi
-      - const: pclk
-      - const: aux
-      - const: pipe
-      - const: ref
-
-  interrupts:
-    items:
-      - 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
-      - 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
-      - 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
-      - description:
-          Combined legacy interrupt, which is used to signal the following
-          interrupts - inta, intb, intc, intd
-      - 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
-
-  interrupt-names:
-    items:
-      - const: sys
-      - const: pmc
-      - const: msg
-      - const: legacy
-      - const: err
-
   legacy-interrupt-controller:
     description: Interrupt controller node for handling legacy PCI interrupts.
     type: object
@@ -119,47 +65,14 @@ properties:
 
   msi-map: true
 
-  num-lanes: true
-
-  phys:
-    maxItems: 1
-
-  phy-names:
-    const: pcie-phy
-
-  power-domains:
-    maxItems: 1
-
   ranges:
     minItems: 2
     maxItems: 3
 
-  resets:
-    minItems: 1
-    maxItems: 2
-
-  reset-names:
-    oneOf:
-      - const: pipe
-      - items:
-          - const: pwr
-          - const: pipe
-
   vpcie3v3-supply: true
 
 required:
-  - compatible
-  - reg
-  - reg-names
-  - clocks
-  - clock-names
   - msi-map
-  - num-lanes
-  - phys
-  - phy-names
-  - power-domains
-  - resets
-  - reset-names
 
 unevaluatedProperties: false
 

-- 
2.45.1

[PATCH v4 05/13] dt-bindings: PCI: rockchip-dw-pcie: Fix description of legacy irq

[Niklas Cassel]

The descriptions of the combined interrupt signals (level1) mention
all the lower interrupt signals (level2) for each combined interrupt,
regardless if the lower (level2) signal is RC or EP specific.

E.g. the description of "Combined system interrupt" includes rbar_update,
which is EP specific, and the description of "Combined message interrupt"
includes obff_idle, obff_obff, obff_cpu_active, which are all EP specific.

The only exception is the "Combined legacy interrupt", which for some
reason does not provide an exhaustive list of the lower (level2) signals.

Add the missing lower interrupt signals: tx_inta, tx_intb, tx_intc, and
tx_intd for the "Combined legacy interrupt", as per the rk3568 and rk3588
Technical Reference Manuals, such that the descriptions of the combined
interrupt signals are consistent.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
index 60d190a77580..ec5e6a3d048e 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
@@ -56,7 +56,8 @@ properties:
           pm_pme, pm_to_ack, pm_turnoff, obff_idle, obff_obff, obff_cpu_active
       - description:
           Combined legacy interrupt, which is used to signal the following
-          interrupts - inta, intb, intc, intd
+          interrupts - inta, intb, intc, intd, tx_inta, tx_intb, tx_intc,
+          tx_intd
       - description:
           Combined error interrupt, which is used to signal the following
           interrupts - aer_rc_err, aer_rc_err_msi, rx_cpl_timeout,

-- 
2.45.1

[PATCH v4 06/13] dt-bindings: rockchip: Add DesignWare based PCIe Endpoint controller

[Niklas Cassel]

Document DT bindings for PCIe Endpoint controller found in Rockchip SoCs.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
 .../bindings/pci/rockchip-dw-pcie-common.yaml      | 14 ++++
 .../bindings/pci/rockchip-dw-pcie-ep.yaml          | 95 ++++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
index ec5e6a3d048e..cc9adfc7611c 100644
--- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
@@ -39,6 +39,7 @@ properties:
       - const: ref
 
   interrupts:
+    minItems: 5
     items:
       - description:
           Combined system interrupt, which is used to signal the following
@@ -63,14 +64,27 @@ properties:
           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
+      - description:
+          eDMA write channel 0 interrupt
+      - description:
+          eDMA write channel 1 interrupt
+      - description:
+          eDMA read channel 0 interrupt
+      - description:
+          eDMA read channel 1 interrupt
 
   interrupt-names:
+    minItems: 5
     items:
       - const: sys
       - const: pmc
       - const: msg
       - const: legacy
       - const: err
+      - const: dma0
+      - const: dma1
+      - const: dma2
+      - const: dma3
 
   num-lanes: true
 
diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
new file mode 100644
index 000000000000..e0c8668afc01
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie-ep.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: DesignWare based PCIe Endpoint controller on Rockchip SoCs
+
+maintainers:
+  - Niklas Cassel <cassel@kernel.org>
+
+description: |+
+  RK3588 SoC PCIe Endpoint controller is based on the Synopsys DesignWare
+  PCIe IP and thus inherits all the common properties defined in
+  snps,dw-pcie-ep.yaml.
+
+allOf:
+  - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
+  - $ref: /schemas/pci/rockchip-dw-pcie-common.yaml#
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3568-pcie-ep
+      - rockchip,rk3588-pcie-ep
+
+  reg:
+    items:
+      - description: Data Bus Interface (DBI) registers
+      - description: Data Bus Interface (DBI) shadow registers
+      - description: Rockchip designed configuration registers
+      - description: Memory region used to map remote RC address space
+      - description: Address Translation Unit (ATU) registers
+
+  reg-names:
+    items:
+      - const: dbi
+      - const: dbi2
+      - const: apb
+      - const: addr_space
+      - const: atu
+
+required:
+  - interrupts
+  - interrupt-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/power/rk3588-power.h>
+    #include <dt-bindings/reset/rockchip,rk3588-cru.h>
+
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pcie3x4_ep: pcie-ep@fe150000 {
+            compatible = "rockchip,rk3588-pcie-ep";
+            clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
+                     <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
+                     <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>;
+            clock-names = "aclk_mst", "aclk_slv",
+                          "aclk_dbi", "pclk",
+                          "aux", "pipe";
+            interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH 0>,
+                         <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH 0>;
+            interrupt-names = "sys", "pmc", "msg", "legacy", "err",
+                              "dma0", "dma1", "dma2", "dma3";
+            max-link-speed = <3>;
+            num-lanes = <4>;
+            phys = <&pcie30phy>;
+            phy-names = "pcie-phy";
+            power-domains = <&power RK3588_PD_PCIE>;
+            reg = <0xa 0x40000000 0x0 0x00100000>,
+                  <0xa 0x40100000 0x0 0x00100000>,
+                  <0x0 0xfe150000 0x0 0x00010000>,
+                  <0x9 0x00000000 0x0 0x40000000>,
+                  <0xa 0x40300000 0x0 0x00100000>;
+            reg-names = "dbi", "dbi2", "apb", "addr_space", "atu";
+            resets = <&cru SRST_PCIE0_POWER_UP>, <&cru SRST_P_PCIE0>;
+            reset-names = "pwr", "pipe";
+        };
+    };
+...

-- 
2.45.1

[PATCH v4 07/13] PCI: dw-rockchip: Fix weird indentation

[Niklas Cassel]

Fix the indentation of rockchip_pcie_{readl,writel}_apb() parameters to
match the opening parenthesis.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 61b1acba7182..3dfed08ef456 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -60,14 +60,13 @@ struct rockchip_pcie {
 	struct irq_domain		*irq_domain;
 };
 
-static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip,
-					     u32 reg)
+static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg)
 {
 	return readl_relaxed(rockchip->apb_base + reg);
 }
 
-static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip,
-						u32 val, u32 reg)
+static void rockchip_pcie_writel_apb(struct rockchip_pcie *rockchip, u32 val,
+				     u32 reg)
 {
 	writel_relaxed(val, rockchip->apb_base + reg);
 }

-- 
2.45.1

[PATCH v4 08/13] PCI: dw-rockchip: Add rockchip_pcie_get_ltssm() helper

[Niklas Cassel]

Add a rockchip_pcie_ltssm() helper function that reads the LTSSM status.
This helper will be used in additional places in follow-up commits.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 3dfed08ef456..1380e3a5284b 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -143,6 +143,11 @@ static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
 	return 0;
 }
 
+static u32 rockchip_pcie_get_ltssm(struct rockchip_pcie *rockchip)
+{
+	return rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS);
+}
+
 static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
 {
 	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
@@ -152,7 +157,7 @@ static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
 static int rockchip_pcie_link_up(struct dw_pcie *pci)
 {
 	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
-	u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS);
+	u32 val = rockchip_pcie_get_ltssm(rockchip);
 
 	if ((val & PCIE_LINKUP) == PCIE_LINKUP &&
 	    (val & PCIE_LTSSM_STATUS_MASK) == PCIE_L0S_ENTRY)

-- 
2.45.1

[PATCH v4 09/13] PCI: dw-rockchip: Refactor the driver to prepare for EP mode

[Niklas Cassel]

This refactors the driver to prepare for EP mode.
Add of-match data to the existing compatible, and explicitly define it as
DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up
commit in a much less intrusive way, which makes the follup-up commit much
easier to review.

No functional change intended.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 84 +++++++++++++++++++--------
 1 file changed, 60 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index 1380e3a5284b..e133511692af 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -49,15 +49,20 @@
 #define PCIE_LTSSM_STATUS_MASK		GENMASK(5, 0)
 
 struct rockchip_pcie {
-	struct dw_pcie			pci;
-	void __iomem			*apb_base;
-	struct phy			*phy;
-	struct clk_bulk_data		*clks;
-	unsigned int			clk_cnt;
-	struct reset_control		*rst;
-	struct gpio_desc		*rst_gpio;
-	struct regulator                *vpcie3v3;
-	struct irq_domain		*irq_domain;
+	struct dw_pcie				pci;
+	void __iomem				*apb_base;
+	struct phy				*phy;
+	struct clk_bulk_data			*clks;
+	unsigned int				clk_cnt;
+	struct reset_control			*rst;
+	struct gpio_desc			*rst_gpio;
+	struct regulator			*vpcie3v3;
+	struct irq_domain			*irq_domain;
+	const struct rockchip_pcie_of_data	*data;
+};
+
+struct rockchip_pcie_of_data {
+	enum dw_pcie_device_mode mode;
 };
 
 static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg)
@@ -195,7 +200,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
 	struct device *dev = rockchip->pci.dev;
-	u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
 	int irq, ret;
 
 	irq = of_irq_get_byname(dev->of_node, "legacy");
@@ -209,12 +213,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
 					 rockchip);
 
-	/* LTSSM enable control mode */
-	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
-
-	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
-				 PCIE_CLIENT_GENERAL_CONTROL);
-
 	return 0;
 }
 
@@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = {
 	.start_link = rockchip_pcie_start_link,
 };
 
+static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip)
+{
+	struct dw_pcie_rp *pp;
+	u32 val;
+
+	/* LTSSM enable control mode */
+	val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
+	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
+
+	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
+				 PCIE_CLIENT_GENERAL_CONTROL);
+
+	pp = &rockchip->pci.pp;
+	pp->ops = &rockchip_pcie_host_ops;
+
+	return dw_pcie_host_init(pp);
+}
+
 static int rockchip_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct rockchip_pcie *rockchip;
-	struct dw_pcie_rp *pp;
+	const struct rockchip_pcie_of_data *data;
 	int ret;
 
+	data = of_device_get_match_data(dev);
+	if (!data)
+		return -EINVAL;
+
 	rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL);
 	if (!rockchip)
 		return -ENOMEM;
@@ -309,9 +329,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 
 	rockchip->pci.dev = dev;
 	rockchip->pci.ops = &dw_pcie_ops;
-
-	pp = &rockchip->pci.pp;
-	pp->ops = &rockchip_pcie_host_ops;
+	rockchip->data = data;
 
 	ret = rockchip_pcie_resource_get(pdev, rockchip);
 	if (ret)
@@ -347,10 +365,21 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		goto deinit_phy;
 
-	ret = dw_pcie_host_init(pp);
-	if (!ret)
-		return 0;
+	switch (data->mode) {
+	case DW_PCIE_RC_TYPE:
+		ret = rockchip_pcie_configure_rc(rockchip);
+		if (ret)
+			goto deinit_clk;
+		break;
+	default:
+		dev_err(dev, "INVALID device type %d\n", data->mode);
+		ret = -EINVAL;
+		goto deinit_clk;
+	}
+
+	return 0;
 
+deinit_clk:
 	clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks);
 deinit_phy:
 	rockchip_pcie_phy_deinit(rockchip);
@@ -361,8 +390,15 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
+	.mode = DW_PCIE_RC_TYPE,
+};
+
 static const struct of_device_id rockchip_pcie_of_match[] = {
-	{ .compatible = "rockchip,rk3568-pcie", },
+	{
+		.compatible = "rockchip,rk3568-pcie",
+		.data = &rockchip_pcie_rc_of_data_rk3568,
+	},
 	{},
 };
 

-- 
2.45.1

[PATCH v4 10/13] PCI: dw-rockchip: Add endpoint mode support

[Niklas Cassel]

The PCIe controller in rk3568 and rk3588 can operate in endpoint mode.
This endpoint mode support heavily leverages the existing code in
pcie-designware-ep.c.

Add support for endpoint mode to the existing pcie-dw-rockchip glue
driver.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/controller/dwc/Kconfig            |  17 ++-
 drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++
 2 files changed, 224 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 8afacc90c63b..9fae0d977271 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP
 	  SoCs. To compile this driver as a module, choose M here: the module
 	  will be called pcie-rcar-gen4.ko. This uses the DesignWare core.
 
+config PCIE_ROCKCHIP_DW
+	bool
+
 config PCIE_ROCKCHIP_DW_HOST
-	bool "Rockchip DesignWare PCIe controller"
-	select PCIE_DW
+	bool "Rockchip DesignWare PCIe controller (host mode)"
 	select PCIE_DW_HOST
 	depends on PCI_MSI
 	depends on ARCH_ROCKCHIP || COMPILE_TEST
 	depends on OF
 	help
 	  Enables support for the DesignWare PCIe controller in the
-	  Rockchip SoC except RK3399.
+	  Rockchip SoC (except RK3399) to work in host mode.
+
+config PCIE_ROCKCHIP_DW_EP
+	bool "Rockchip DesignWare PCIe controller (endpoint mode)"
+	select PCIE_DW_EP
+	depends on ARCH_ROCKCHIP || COMPILE_TEST
+	depends on OF
+	help
+	  Enables support for the DesignWare PCIe controller in the
+	  Rockchip SoC (except RK3399) to work in endpoint mode.
 
 config PCI_EXYNOS
 	tristate "Samsung Exynos PCIe controller"
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
index e133511692af..347721207161 100644
--- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
+++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
@@ -34,10 +34,16 @@
 #define to_rockchip_pcie(x) dev_get_drvdata((x)->dev)
 
 #define PCIE_CLIENT_RC_MODE		HIWORD_UPDATE_BIT(0x40)
+#define PCIE_CLIENT_EP_MODE		HIWORD_UPDATE(0xf0, 0x0)
 #define PCIE_CLIENT_ENABLE_LTSSM	HIWORD_UPDATE_BIT(0xc)
+#define PCIE_CLIENT_DISABLE_LTSSM	HIWORD_UPDATE(0x0c, 0x8)
+#define PCIE_CLIENT_INTR_STATUS_MISC	0x10
+#define PCIE_CLIENT_INTR_MASK_MISC	0x24
 #define PCIE_SMLH_LINKUP		BIT(16)
 #define PCIE_RDLH_LINKUP		BIT(17)
 #define PCIE_LINKUP			(PCIE_SMLH_LINKUP | PCIE_RDLH_LINKUP)
+#define PCIE_RDLH_LINK_UP_CHGED		BIT(1)
+#define PCIE_LINK_REQ_RST_NOT_INT	BIT(2)
 #define PCIE_L0S_ENTRY			0x11
 #define PCIE_CLIENT_GENERAL_CONTROL	0x0
 #define PCIE_CLIENT_INTR_STATUS_LEGACY	0x8
@@ -63,6 +69,7 @@ struct rockchip_pcie {
 
 struct rockchip_pcie_of_data {
 	enum dw_pcie_device_mode mode;
+	const struct pci_epc_features *epc_features;
 };
 
 static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg)
@@ -159,6 +166,12 @@ static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
 				 PCIE_CLIENT_GENERAL_CONTROL);
 }
 
+static void rockchip_pcie_disable_ltssm(struct rockchip_pcie *rockchip)
+{
+	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_DISABLE_LTSSM,
+				 PCIE_CLIENT_GENERAL_CONTROL);
+}
+
 static int rockchip_pcie_link_up(struct dw_pcie *pci)
 {
 	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
@@ -195,6 +208,13 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci)
 	return 0;
 }
 
+static void rockchip_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
+
+	rockchip_pcie_disable_ltssm(rockchip);
+}
+
 static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -220,6 +240,82 @@ static const struct dw_pcie_host_ops rockchip_pcie_host_ops = {
 	.init = rockchip_pcie_host_init,
 };
 
+static void rockchip_pcie_ep_init(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	enum pci_barno bar;
+
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
+		dw_pcie_ep_reset_bar(pci, bar);
+};
+
+static int rockchip_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
+				   unsigned int type, u16 interrupt_num)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+
+	switch (type) {
+	case PCI_IRQ_INTX:
+		return dw_pcie_ep_raise_intx_irq(ep, func_no);
+	case PCI_IRQ_MSI:
+		return dw_pcie_ep_raise_msi_irq(ep, func_no, interrupt_num);
+	case PCI_IRQ_MSIX:
+		return dw_pcie_ep_raise_msix_irq(ep, func_no, interrupt_num);
+	default:
+		dev_err(pci->dev, "UNKNOWN IRQ type\n");
+	}
+
+	return 0;
+}
+
+static const struct pci_epc_features rockchip_pcie_epc_features_rk3568 = {
+	.linkup_notifier = true,
+	.msi_capable = true,
+	.msix_capable = true,
+	.align = SZ_64K,
+	.bar[BAR_0] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_1] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_2] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_3] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_4] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_5] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+};
+
+/*
+ * BAR4 on rk3588 exposes the ATU Port Logic Structure to the host regardless of
+ * iATU settings for BAR4. This means that BAR4 cannot be used by an EPF driver,
+ * so mark it as RESERVED. (rockchip_pcie_ep_init() will disable all BARs by
+ * default.) If the host could write to BAR4, the iATU settings (for all other
+ * BARs) would be overwritten, resulting in (all other BARs) no longer working.
+ */
+static const struct pci_epc_features rockchip_pcie_epc_features_rk3588 = {
+	.linkup_notifier = true,
+	.msi_capable = true,
+	.msix_capable = true,
+	.align = SZ_64K,
+	.bar[BAR_0] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_1] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_2] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_3] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+	.bar[BAR_4] = { .type = BAR_RESERVED, },
+	.bar[BAR_5] = { .type = BAR_FIXED, .fixed_size = SZ_1M, },
+};
+
+static const struct pci_epc_features *
+rockchip_pcie_get_features(struct dw_pcie_ep *ep)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
+
+	return rockchip->data->epc_features;
+}
+
+static const struct dw_pcie_ep_ops rockchip_pcie_ep_ops = {
+	.init = rockchip_pcie_ep_init,
+	.raise_irq = rockchip_pcie_raise_irq,
+	.get_features = rockchip_pcie_get_features,
+};
+
 static int rockchip_pcie_clk_init(struct rockchip_pcie *rockchip)
 {
 	struct device *dev = rockchip->pci.dev;
@@ -290,13 +386,47 @@ static void rockchip_pcie_phy_deinit(struct rockchip_pcie *rockchip)
 static const struct dw_pcie_ops dw_pcie_ops = {
 	.link_up = rockchip_pcie_link_up,
 	.start_link = rockchip_pcie_start_link,
+	.stop_link = rockchip_pcie_stop_link,
 };
 
+static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
+{
+	struct rockchip_pcie *rockchip = arg;
+	struct dw_pcie *pci = &rockchip->pci;
+	struct device *dev = pci->dev;
+	u32 reg, val;
+
+	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
+
+	dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg);
+	dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
+
+	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
+		dev_dbg(dev, "hot reset or link-down reset\n");
+		dw_pcie_ep_linkdown(&pci->ep);
+	}
+
+	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
+		val = rockchip_pcie_get_ltssm(rockchip);
+		if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
+			dev_dbg(dev, "link up\n");
+			dw_pcie_ep_linkup(&pci->ep);
+		}
+	}
+
+	rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
+
+	return IRQ_HANDLED;
+}
+
 static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip)
 {
 	struct dw_pcie_rp *pp;
 	u32 val;
 
+	if (!IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_HOST))
+		return -ENODEV;
+
 	/* LTSSM enable control mode */
 	val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
 	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
@@ -310,6 +440,63 @@ static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip)
 	return dw_pcie_host_init(pp);
 }
 
+static int rockchip_pcie_configure_ep(struct platform_device *pdev,
+				      struct rockchip_pcie *rockchip)
+{
+	struct device *dev = &pdev->dev;
+	int irq, ret;
+	u32 val;
+
+	if (!IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_EP))
+		return -ENODEV;
+
+	irq = platform_get_irq_byname(pdev, "sys");
+	if (irq < 0) {
+		dev_err(dev, "missing sys IRQ resource\n");
+		return irq;
+	}
+
+	ret = devm_request_threaded_irq(dev, irq, NULL,
+					rockchip_pcie_ep_sys_irq_thread,
+					IRQF_ONESHOT, "pcie-sys", rockchip);
+	if (ret) {
+		dev_err(dev, "failed to request PCIe sys IRQ\n");
+		return ret;
+	}
+
+	/* LTSSM enable control mode */
+	val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
+	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
+
+	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_EP_MODE,
+				 PCIE_CLIENT_GENERAL_CONTROL);
+
+	rockchip->pci.ep.ops = &rockchip_pcie_ep_ops;
+	rockchip->pci.ep.page_size = SZ_64K;
+
+	dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
+
+	ret = dw_pcie_ep_init(&rockchip->pci.ep);
+	if (ret) {
+		dev_err(dev, "failed to initialize endpoint\n");
+		return ret;
+	}
+
+	ret = dw_pcie_ep_init_registers(&rockchip->pci.ep);
+	if (ret) {
+		dev_err(dev, "failed to initialize DWC endpoint registers\n");
+		dw_pcie_ep_deinit(&rockchip->pci.ep);
+		return ret;
+	}
+
+	dw_pcie_ep_init_notify(&rockchip->pci.ep);
+
+	/* unmask DLL up/down indicator and hot reset/link-down reset */
+	rockchip_pcie_writel_apb(rockchip, 0x60000, PCIE_CLIENT_INTR_MASK_MISC);
+
+	return ret;
+}
+
 static int rockchip_pcie_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -371,6 +558,11 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 		if (ret)
 			goto deinit_clk;
 		break;
+	case DW_PCIE_EP_TYPE:
+		ret = rockchip_pcie_configure_ep(pdev, rockchip);
+		if (ret)
+			goto deinit_clk;
+		break;
 	default:
 		dev_err(dev, "INVALID device type %d\n", data->mode);
 		ret = -EINVAL;
@@ -394,11 +586,29 @@ static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = {
 	.mode = DW_PCIE_RC_TYPE,
 };
 
+static const struct rockchip_pcie_of_data rockchip_pcie_ep_of_data_rk3568 = {
+	.mode = DW_PCIE_EP_TYPE,
+	.epc_features = &rockchip_pcie_epc_features_rk3568,
+};
+
+static const struct rockchip_pcie_of_data rockchip_pcie_ep_of_data_rk3588 = {
+	.mode = DW_PCIE_EP_TYPE,
+	.epc_features = &rockchip_pcie_epc_features_rk3588,
+};
+
 static const struct of_device_id rockchip_pcie_of_match[] = {
 	{
 		.compatible = "rockchip,rk3568-pcie",
 		.data = &rockchip_pcie_rc_of_data_rk3568,
 	},
+	{
+		.compatible = "rockchip,rk3568-pcie-ep",
+		.data = &rockchip_pcie_ep_of_data_rk3568,
+	},
+	{
+		.compatible = "rockchip,rk3588-pcie-ep",
+		.data = &rockchip_pcie_ep_of_data_rk3588,
+	},
 	{},
 };
 

-- 
2.45.1

[PATCH v4 11/13] misc: pci_endpoint_test: Add support for rockchip rk3588

[Niklas Cassel]

Rockchip rk3588 requires 64k alignment.
While there is an existing device_id:vendor_id in the driver with 64k
alignment, that device_id:vendor_id is am654, which uses BAR2 instead of
BAR0 as the test_reg_bar, and also has special is_am654_pci_dev() checks
in the driver to disallow BAR0. In order to allow testing all BARs, add a
new rk3588 entry in the driver.

We intentionally do not add the vendor id to pci_ids.h, since the policy
for that file is that the vendor id has to be used by multiple drivers.

Hopefully, this new entry will be short-lived, as there is a series on the
mailing list which intends to move the address alignment restrictions from
this driver to the endpoint side.

Add a new entry for rk3588 in order to allow us to test all BARs.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/misc/pci_endpoint_test.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 4f3ec1f2ba9f..0ffc8e02b863 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -85,6 +85,9 @@
 #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
 #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
 
+#define PCI_VENDOR_ID_ROCKCHIP			0x1d87
+#define PCI_DEVICE_ID_ROCKCHIP_RK3588		0x3588
+
 static DEFINE_IDA(pci_endpoint_test_ida);
 
 #define to_endpoint_test(priv) container_of((priv), struct pci_endpoint_test, \
@@ -1006,6 +1009,11 @@ static const struct pci_endpoint_test_data j721e_data = {
 	.irq_type = IRQ_TYPE_MSI,
 };
 
+static const struct pci_endpoint_test_data rk3588_data = {
+	.alignment = SZ_64K,
+	.irq_type = IRQ_TYPE_MSI,
+};
+
 static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_DRA74x),
 	  .driver_data = (kernel_ulong_t)&default_data,
@@ -1043,6 +1051,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721S2),
 	  .driver_data = (kernel_ulong_t)&j721e_data,
 	},
+	{ PCI_DEVICE(PCI_VENDOR_ID_ROCKCHIP, PCI_DEVICE_ID_ROCKCHIP_RK3588),
+	  .driver_data = (kernel_ulong_t)&rk3588_data,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(pci, pci_endpoint_test_tbl);

-- 
2.45.1

[PATCH v4 12/13] arm64: dts: rockchip: Add PCIe endpoint mode support

[Niklas Cassel]

Add a device tree node representing PCIe endpoint mode.

The controller can either be configured to run in Root Complex or Endpoint
node.

If a user wants to run the controller in endpoint mode, the user has to
disable the pcie3x4 node and enable the pcie3x4_ep node.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 arch/arm64/boot/dts/rockchip/rk3588.dtsi | 35 ++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
index 5984016b5f96..6b5bf1055143 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
@@ -186,6 +186,41 @@ pcie3x4_intc: legacy-interrupt-controller {
 		};
 	};
 
+	pcie3x4_ep: pcie-ep@fe150000 {
+		compatible = "rockchip,rk3588-pcie-ep";
+		clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
+			 <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
+			 <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>;
+		clock-names = "aclk_mst", "aclk_slv",
+			      "aclk_dbi", "pclk",
+			      "aux", "pipe";
+		interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH 0>,
+			     <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH 0>;
+		interrupt-names = "sys", "pmc", "msg", "legacy", "err",
+				  "dma0", "dma1", "dma2", "dma3";
+		max-link-speed = <3>;
+		num-lanes = <4>;
+		phys = <&pcie30phy>;
+		phy-names = "pcie-phy";
+		power-domains = <&power RK3588_PD_PCIE>;
+		reg = <0xa 0x40000000 0x0 0x00100000>,
+		      <0xa 0x40100000 0x0 0x00100000>,
+		      <0x0 0xfe150000 0x0 0x00010000>,
+		      <0x9 0x00000000 0x0 0x40000000>,
+		      <0xa 0x40300000 0x0 0x00100000>;
+		reg-names = "dbi", "dbi2", "apb", "addr_space", "atu";
+		resets = <&cru SRST_PCIE0_POWER_UP>, <&cru SRST_P_PCIE0>;
+		reset-names = "pwr", "pipe";
+		status = "disabled";
+	};
+
 	pcie3x2: pcie@fe160000 {
 		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
 		#address-cells = <3>;

-- 
2.45.1

[PATCH v4 13/13] arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode

[Niklas Cassel]

Add rock5b overlays for PCIe endpoint mode support.

If using the rock5b as an endpoint against a normal PC, only the
rk3588-rock-5b-pcie-ep.dtbo needs to be applied.

If using two rock5b:s, with one board as EP and the other board as RC,
rk3588-rock-5b-pcie-ep.dtbo and rk3588-rock-5b-pcie-srns.dtbo has to
be applied to the respective boards.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 arch/arm64/boot/dts/rockchip/Makefile              |  5 +++++
 .../boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso  | 25 ++++++++++++++++++++++
 .../dts/rockchip/rk3588-rock-5b-pcie-srns.dtso     | 16 ++++++++++++++
 3 files changed, 46 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index f42fa62b4064..df7f5103b018 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -124,6 +124,8 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-ok3588-c.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-orangepi-5-plus.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb
@@ -134,3 +136,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6s.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-nanopi-r6c.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-rock-5a.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588s-orangepi-5.dtb
+
+# Enable support for device-tree overlays
+DTC_FLAGS_rk3588-rock-5b += -@
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso
new file mode 100644
index 000000000000..672d748fcc67
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * DT-overlay to run the PCIe3_4L Dual Mode controller in Endpoint mode
+ * in the SRNS (Separate Reference Clock No Spread) configuration.
+ *
+ * NOTE: If using a setup with two ROCK 5B:s, with one board running in
+ * RC mode and the other board running in EP mode, see also the device
+ * tree overlay: rk3588-rock-5b-pcie-srns.dtso.
+ */
+
+/dts-v1/;
+/plugin/;
+
+&pcie30phy {
+	rockchip,rx-common-refclk-mode = <0 0 0 0>;
+};
+
+&pcie3x4 {
+	status = "disabled";
+};
+
+&pcie3x4_ep {
+	vpcie3v3-supply = <&vcc3v3_pcie30>;
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso
new file mode 100644
index 000000000000..1a0f1af65c43
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtso
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * DT-overlay to run the PCIe3_4L Dual Mode controller in Root Complex
+ * mode in the SRNS (Separate Reference Clock No Spread) configuration.
+ *
+ * This device tree overlay is only needed (on the RC side) when running
+ * a setup with two ROCK 5B:s, with one board running in RC mode and the
+ * other board running in EP mode.
+ */
+
+/dts-v1/;
+/plugin/;
+
+&pcie30phy {
+	rockchip,rx-common-refclk-mode = <0 0 0 0>;
+};

-- 
2.45.1

Re: [PATCH v4 00/13] PCI: dw-rockchip: Add endpoint mode support

[Kever Yang]

Hi Niklas,

On 2024/5/29 16:28, Niklas Cassel wrote:
> Hello all,
>
> This series adds PCIe endpoint mode support for the rockchip rk3588 and
> rk3568 SoCs.
>
> This series is based on: pci/next
> (git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git)
>
> This series can also be found in git:
> https://github.com/floatious/linux/commits/rockchip-pcie-ep-v4
>
> Testing done:
> This series has been tested with two rock5b:s, one running in RC mode and
> one running in EP mode. This series has also been tested with an Intel x86
> host and rock5b running in EP mode.

I'm interesting how you test in Intel x86 host, PC scan the PCIe device 
in BIOS, do you

power on the EP before PC power on?


Thanks,
- Kever
>
> BAR4 exposes the ATU Port Logic Structure and the DMA Port Logic Structure
> to the host. The EPC controller driver thus disables this BAR as init time,
> because if it doesn't, when the host writes the test pattern to this BAR,
> all the iATU settings will get wiped, resulting in all further BAR accesses
> being non-functional.
>
> Running pcitest.sh (modified to also perform the READ and WRITE tests with
> the -d option, i.e. with DMA enabled) results in the following:
>
> $ /usr/bin/pcitest.sh
> BAR tests
>
> BAR0:           OKAY
> BAR1:           OKAY
> BAR2:           OKAY
> BAR3:           OKAY
> BAR4:           NOT OKAY
> BAR5:           OKAY
>
> Interrupt tests
>
> SET IRQ TYPE TO LEGACY:         OKAY
> LEGACY IRQ:     NOT OKAY
> SET IRQ TYPE TO MSI:            OKAY
> MSI1:           OKAY
> MSI2:           OKAY
> MSI3:           OKAY
> MSI4:           OKAY
> MSI5:           OKAY
> MSI6:           OKAY
> MSI7:           OKAY
> MSI8:           OKAY
> MSI9:           OKAY
> MSI10:          OKAY
> MSI11:          OKAY
> MSI12:          OKAY
> MSI13:          OKAY
> MSI14:          OKAY
> MSI15:          OKAY
> MSI16:          OKAY
> MSI17:          OKAY
> MSI18:          OKAY
> MSI19:          OKAY
> MSI20:          OKAY
> MSI21:          OKAY
> MSI22:          OKAY
> MSI23:          OKAY
> MSI24:          OKAY
> MSI25:          OKAY
> MSI26:          OKAY
> MSI27:          OKAY
> MSI28:          OKAY
> MSI29:          OKAY
> MSI30:          OKAY
> MSI31:          OKAY
> MSI32:          OKAY
>
> SET IRQ TYPE TO MSI-X:          OKAY
> MSI-X1:         OKAY
> MSI-X2:         OKAY
> MSI-X3:         OKAY
> MSI-X4:         OKAY
> MSI-X5:         OKAY
> MSI-X6:         OKAY
> MSI-X7:         OKAY
> MSI-X8:         OKAY
> MSI-X9:         OKAY
> MSI-X10:                OKAY
> MSI-X11:                OKAY
> MSI-X12:                OKAY
> MSI-X13:                OKAY
> MSI-X14:                OKAY
> MSI-X15:                OKAY
> MSI-X16:                OKAY
> MSI-X17:                OKAY
> MSI-X18:                OKAY
> MSI-X19:                OKAY
> MSI-X20:                OKAY
> MSI-X21:                OKAY
> MSI-X22:                OKAY
> MSI-X23:                OKAY
> MSI-X24:                OKAY
> MSI-X25:                OKAY
> MSI-X26:                OKAY
> MSI-X27:                OKAY
> MSI-X28:                OKAY
> MSI-X29:                OKAY
> MSI-X30:                OKAY
> MSI-X31:                OKAY
> MSI-X32:                OKAY
>
> Read Tests
>
> SET IRQ TYPE TO MSI:            OKAY
> READ (      1 bytes):           OKAY
> READ (   1024 bytes):           OKAY
> READ (   1025 bytes):           OKAY
> READ (1024000 bytes):           OKAY
> READ (1024001 bytes):           OKAY
>
> Write Tests
>
> WRITE (      1 bytes):          OKAY
> WRITE (   1024 bytes):          OKAY
> WRITE (   1025 bytes):          OKAY
> WRITE (1024000 bytes):          OKAY
> WRITE (1024001 bytes):          OKAY
>
> Copy Tests
>
> COPY (      1 bytes):           OKAY
> COPY (   1024 bytes):           OKAY
> COPY (   1025 bytes):           OKAY
> COPY (1024000 bytes):           OKAY
> COPY (1024001 bytes):           OKAY
>
> Read Tests DMA
>
> READ (      1 bytes):           OKAY
> READ (   1024 bytes):           OKAY
> READ (   1025 bytes):           OKAY
> READ (1024000 bytes):           OKAY
> READ (1024001 bytes):           OKAY
>
> Write Tests DMA
>
> WRITE (      1 bytes):          OKAY
> WRITE (   1024 bytes):          OKAY
> WRITE (   1025 bytes):          OKAY
> WRITE (1024000 bytes):          OKAY
> WRITE (1024001 bytes):          OKAY
>
> Corresponding output on the EP side:
> rockchip-dw-pcie a40000000.pcie-ep: EP cannot raise INTX IRQs
> pci_epf_test pci_epf_test.0: WRITE => Size: 1 B, DMA: NO, Time: 0.000000292 s, Rate: 3424 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1024 B, DMA: NO, Time: 0.000007583 s, Rate: 135038 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1025 B, DMA: NO, Time: 0.000007584 s, Rate: 135152 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1024000 B, DMA: NO, Time: 0.009164167 s, Rate: 111739 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1024001 B, DMA: NO, Time: 0.009164458 s, Rate: 111736 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1 B, DMA: NO, Time: 0.000001750 s, Rate: 571 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1024 B, DMA: NO, Time: 0.000147875 s, Rate: 6924 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1025 B, DMA: NO, Time: 0.000149041 s, Rate: 6877 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1024000 B, DMA: NO, Time: 0.147537833 s, Rate: 6940 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: NO, Time: 0.147533750 s, Rate: 6940 KB/s
> pci_epf_test pci_epf_test.0: COPY => Size: 1 B, DMA: NO, Time: 0.000003208 s, Rate: 311 KB/s
> pci_epf_test pci_epf_test.0: COPY => Size: 1024 B, DMA: NO, Time: 0.000156625 s, Rate: 6537 KB/s
> pci_epf_test pci_epf_test.0: COPY => Size: 1025 B, DMA: NO, Time: 0.000158375 s, Rate: 6471 KB/s
> pci_epf_test pci_epf_test.0: COPY => Size: 1024000 B, DMA: NO, Time: 0.156902666 s, Rate: 6526 KB/s
> pci_epf_test pci_epf_test.0: COPY => Size: 1024001 B, DMA: NO, Time: 0.156847833 s, Rate: 6528 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1 B, DMA: YES, Time: 0.000185500 s, Rate: 5 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1024 B, DMA: YES, Time: 0.000177334 s, Rate: 5774 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1025 B, DMA: YES, Time: 0.000178792 s, Rate: 5732 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1024000 B, DMA: YES, Time: 0.000486209 s, Rate: 2106090 KB/s
> pci_epf_test pci_epf_test.0: WRITE => Size: 1024001 B, DMA: YES, Time: 0.000486791 s, Rate: 2103574 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1 B, DMA: YES, Time: 0.000177333 s, Rate: 5 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1024 B, DMA: YES, Time: 0.000177625 s, Rate: 5764 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1025 B, DMA: YES, Time: 0.000171208 s, Rate: 5986 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1024000 B, DMA: YES, Time: 0.000701167 s, Rate: 1460422 KB/s
> pci_epf_test pci_epf_test.0: READ => Size: 1024001 B, DMA: YES, Time: 0.000702625 s, Rate: 1457393 KB/s
>
> Kind regards,
> Niklas
>
> ---
> Changes in v4:
> - Rebased on pci/next
> - Link to v3: https://lore.kernel.org/r/20240508-rockchip-pcie-ep-v1-v3-0-1748e202b084@kernel.org
>
> Changes in v3:
> - Renamed rockchip_pcie_ltssm() to rockchip_pcie_get_ltssm()
> - Reworded some commit messages to avoid the term "patches".
> - Dropped patch that added explicit rockchip,rk3588-pcie compatible
> - Moved !IS_ENABLED(CONFIG_PCIE_ROCKCHIP_DW_HOST) check to proper patch.
> - Added comment in front of rk3588 epc_features describing why BAR4 is
>    reserved.
> - Added another struct epc_features for rk3568 as it does not have the
>    ATU regs mapped to BAR4 (like rk3588 does).
> - Picked up tags from Rob and Mani. Thank you!
> - Link to v2: https://lore.kernel.org/r/20240430-rockchip-pcie-ep-v1-v2-0-a0f5ee2a77b6@kernel.org
>
> Changes in v2:
> - Rebased on v4 of the pci-epf-rework series that we depend on.
> - Picked up tags from Rob.
> - Split dw-rockchip DT binding in to common, RC and EP parts.
> - Added support for rk3568 in DT binding and driver.
> - Added a new patch that fixed "combined legacy IRQ description".
> - Link to v1: https://lore.kernel.org/r/20240424-rockchip-pcie-ep-v1-v1-0-b1a02ddad650@kernel.org
>
> ---
> Niklas Cassel (13):
>        dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific reg-name
>        dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific interrupt-names
>        dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs
>        dt-bindings: PCI: rockchip-dw-pcie: Prepare for Endpoint mode support
>        dt-bindings: PCI: rockchip-dw-pcie: Fix description of legacy irq
>        dt-bindings: rockchip: Add DesignWare based PCIe Endpoint controller
>        PCI: dw-rockchip: Fix weird indentation
>        PCI: dw-rockchip: Add rockchip_pcie_get_ltssm() helper
>        PCI: dw-rockchip: Refactor the driver to prepare for EP mode
>        PCI: dw-rockchip: Add endpoint mode support
>        misc: pci_endpoint_test: Add support for rockchip rk3588
>        arm64: dts: rockchip: Add PCIe endpoint mode support
>        arm64: dts: rockchip: Add rock5b overlays for PCIe endpoint mode
>
>   .../bindings/pci/rockchip-dw-pcie-common.yaml      | 126 +++++++++
>   .../bindings/pci/rockchip-dw-pcie-ep.yaml          |  95 +++++++
>   .../devicetree/bindings/pci/rockchip-dw-pcie.yaml  |  93 +------
>   .../devicetree/bindings/pci/snps,dw-pcie-ep.yaml   |  13 +-
>   arch/arm64/boot/dts/rockchip/Makefile              |   5 +
>   .../boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtso  |  25 ++
>   .../dts/rockchip/rk3588-rock-5b-pcie-srns.dtso     |  16 ++
>   arch/arm64/boot/dts/rockchip/rk3588.dtsi           |  35 +++
>   drivers/misc/pci_endpoint_test.c                   |  11 +
>   drivers/pci/controller/dwc/Kconfig                 |  17 +-
>   drivers/pci/controller/dwc/pcie-dw-rockchip.c      | 308 +++++++++++++++++++--
>   11 files changed, 620 insertions(+), 124 deletions(-)
> ---
> base-commit: e3fca37312892122d73f8c5293c0d1cc8c34500b
> change-id: 20240424-rockchip-pcie-ep-v1-87c78b16d53c
>
> Best regards,

Re: [PATCH v4 00/13] PCI: dw-rockchip: Add endpoint mode support

[Damien Le Moal]

On 6/4/24 10:45, Kever Yang wrote:
> Hi Niklas,
> 
> On 2024/5/29 16:28, Niklas Cassel wrote:
>> Hello all,
>>
>> This series adds PCIe endpoint mode support for the rockchip rk3588 and
>> rk3568 SoCs.
>>
>> This series is based on: pci/next
>> (git://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git)
>>
>> This series can also be found in git:
>> https://github.com/floatious/linux/commits/rockchip-pcie-ep-v4
>>
>> Testing done:
>> This series has been tested with two rock5b:s, one running in RC mode and
>> one running in EP mode. This series has also been tested with an Intel x86
>> host and rock5b running in EP mode.
> 
> I'm interesting how you test in Intel x86 host, PC scan the PCIe device 
> in BIOS, do you
> 
> power on the EP before PC power on?

Yes. Same with any host, not just x86.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 01/13] dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific reg-name

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:28:55AM +0200, Niklas Cassel wrote:
> Considering that some drivers (e.g. pcie-dw-rockchip.c) already use the
> reg-name "apb" for the device tree binding in Root Complex mode
> (snps,dw-pcie.yaml), it doesn't make sense that those drivers should use a
> different reg-name when running in Endpoint mode (snps,dw-pcie-ep.yaml).
> 
> Therefore, since "apb" is already defined in snps,dw-pcie.yaml, add it
> also for snps,dw-pcie-ep.yaml.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> index bbdb01d22848..00dec01f1f73 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> @@ -100,7 +100,7 @@ properties:
>              for new bindings.
>            oneOf:
>              - description: See native 'elbi/app' CSR region for details.
> -              enum: [ link, appl ]
> +              enum: [ apb, link, appl ]
>              - description: See native 'atu' CSR region for details.
>                enum: [ atu_dma ]
>      allOf:
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 02/13] dt-bindings: PCI: snps,dw-pcie-ep: Add vendor specific interrupt-names

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:28:56AM +0200, Niklas Cassel wrote:
> Considering that some drivers (e.g. pcie-dw-rockchip.c) already use the
> interrupt-names "sys", "pmc", "msg", "err" for the device tree binding in
> Root Complex mode (snps,dw-pcie.yaml), it doesn't make sense that those
> drivers should use different interrupt-names when running in Endpoint mode
> (snps,dw-pcie-ep.yaml).
> 
> Therefore, since "sys", "pmc", "msg", "err" are already defined in
> snps,dw-pcie.yaml, add them also for snps,dw-pcie-ep.yaml.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> index 00dec01f1f73..f5f12cbc2cb3 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> @@ -156,7 +156,7 @@ properties:
>              for new bindings.
>            oneOf:
>              - description: See native "app" IRQ for details
> -              enum: [ intr ]
> +              enum: [ intr, sys, pmc, msg, err ]
>  
>    max-functions:
>      maximum: 32
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 03/13] dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:28:57AM +0200, Niklas Cassel wrote:
> The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
> that are triggered when the PCIe controller (when running in Endpoint mode)
> has sent an Assert_INTA Message to the upstream device.
> 
> Some DWC controllers have these interrupt in a combined interrupt signal.
> 
> Add the description of these interrupts to the device tree binding.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Nit: We recently changed the driver instances of 'LEGACY' to 'INTX'. But the
binding it still using 'legacy'. Considering that the 'legacy' IRQ added to the
RC binding recently (ebce9f6623a7), should we rename it?

This will force the driver to support both 'legacy' and 'intx' for backwards
compatibility.

But irrespective of that,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> index f5f12cbc2cb3..f474b9e3fc7e 100644
> --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> @@ -151,6 +151,15 @@ properties:
>              Application-specific IRQ raised depending on the vendor-specific
>              events basis.
>            const: app
> +        - description:
> +            Interrupts triggered when the controller itself (in Endpoint mode)
> +            has sent an Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message to
> +            the upstream device.
> +          pattern: "^tx_int(a|b|c|d)$"
> +        - description:
> +            Combined interrupt signal raised when the controller has sent an
> +            Assert_INT{A,B,C,D} message. See "^tx_int(a|b|c|d)$" for details.
> +          const: legacy
>          - description:
>              Vendor-specific IRQ names. Consider using the generic names above
>              for new bindings.
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 04/13] dt-bindings: PCI: rockchip-dw-pcie: Prepare for Endpoint mode support

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:28:58AM +0200, Niklas Cassel wrote:
> Refactor the rockchip-dw-pcie binding to move generic properties to a new
> rockchip-dw-pcie-common binding that can be shared by both RC and EP mode.
> 
> No functional change intended.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  .../bindings/pci/rockchip-dw-pcie-common.yaml      | 111 +++++++++++++++++++++
>  .../devicetree/bindings/pci/rockchip-dw-pcie.yaml  |  93 +----------------
>  2 files changed, 114 insertions(+), 90 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> new file mode 100644
> index 000000000000..60d190a77580
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> @@ -0,0 +1,111 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DesignWare based PCIe RC/EP controller on Rockchip SoCs
> +
> +maintainers:
> +  - Shawn Lin <shawn.lin@rock-chips.com>
> +  - Simon Xue <xxm@rock-chips.com>
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +description: |+
> +  Generic properties for the DesignWare based PCIe RC/EP controller on Rockchip
> +  SoCs.
> +
> +properties:
> +  clocks:
> +    minItems: 5
> +    items:
> +      - description: AHB clock for PCIe master
> +      - description: AHB clock for PCIe slave
> +      - description: AHB clock for PCIe dbi
> +      - description: APB clock for PCIe
> +      - description: Auxiliary clock for PCIe
> +      - description: PIPE clock
> +      - description: Reference clock for PCIe
> +
> +  clock-names:
> +    minItems: 5
> +    items:
> +      - const: aclk_mst
> +      - const: aclk_slv
> +      - const: aclk_dbi
> +      - const: pclk
> +      - const: aux
> +      - const: pipe
> +      - const: ref
> +
> +  interrupts:
> +    items:
> +      - 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
> +      - 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
> +      - 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
> +      - description:
> +          Combined legacy interrupt, which is used to signal the following
> +          interrupts - inta, intb, intc, intd
> +      - 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
> +
> +  interrupt-names:
> +    items:
> +      - const: sys
> +      - const: pmc
> +      - const: msg
> +      - const: legacy
> +      - const: err
> +
> +  num-lanes: true
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: pcie-phy
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    minItems: 1
> +    maxItems: 2
> +
> +  reset-names:
> +    oneOf:
> +      - const: pipe
> +      - items:
> +          - const: pwr
> +          - const: pipe
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - num-lanes
> +  - phys
> +  - phy-names
> +  - power-domains
> +  - resets
> +  - reset-names
> +
> +additionalProperties: true
> +
> +...
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> index 5f719218c472..550d8a684af3 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/pci/rockchip-dw-pcie.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: DesignWare based PCIe controller on Rockchip SoCs
> +title: DesignWare based PCIe Root Complex controller on Rockchip SoCs
>  
>  maintainers:
>    - Shawn Lin <shawn.lin@rock-chips.com>
> @@ -12,12 +12,13 @@ maintainers:
>    - Heiko Stuebner <heiko@sntech.de>
>  
>  description: |+
> -  RK3568 SoC PCIe host controller is based on the Synopsys DesignWare
> +  RK3568 SoC PCIe Root Complex controller is based on the Synopsys DesignWare
>    PCIe IP and thus inherits all the common properties defined in
>    snps,dw-pcie.yaml.
>  
>  allOf:
>    - $ref: /schemas/pci/snps,dw-pcie.yaml#
> +  - $ref: /schemas/pci/rockchip-dw-pcie-common.yaml#
>  
>  properties:
>    compatible:
> @@ -40,61 +41,6 @@ properties:
>        - const: apb
>        - const: config
>  
> -  clocks:
> -    minItems: 5
> -    items:
> -      - description: AHB clock for PCIe master
> -      - description: AHB clock for PCIe slave
> -      - description: AHB clock for PCIe dbi
> -      - description: APB clock for PCIe
> -      - description: Auxiliary clock for PCIe
> -      - description: PIPE clock
> -      - description: Reference clock for PCIe
> -
> -  clock-names:
> -    minItems: 5
> -    items:
> -      - const: aclk_mst
> -      - const: aclk_slv
> -      - const: aclk_dbi
> -      - const: pclk
> -      - const: aux
> -      - const: pipe
> -      - const: ref
> -
> -  interrupts:
> -    items:
> -      - 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
> -      - 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
> -      - 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
> -      - description:
> -          Combined legacy interrupt, which is used to signal the following
> -          interrupts - inta, intb, intc, intd
> -      - 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
> -
> -  interrupt-names:
> -    items:
> -      - const: sys
> -      - const: pmc
> -      - const: msg
> -      - const: legacy
> -      - const: err
> -
>    legacy-interrupt-controller:
>      description: Interrupt controller node for handling legacy PCI interrupts.
>      type: object
> @@ -119,47 +65,14 @@ properties:
>  
>    msi-map: true
>  
> -  num-lanes: true
> -
> -  phys:
> -    maxItems: 1
> -
> -  phy-names:
> -    const: pcie-phy
> -
> -  power-domains:
> -    maxItems: 1
> -
>    ranges:
>      minItems: 2
>      maxItems: 3
>  
> -  resets:
> -    minItems: 1
> -    maxItems: 2
> -
> -  reset-names:
> -    oneOf:
> -      - const: pipe
> -      - items:
> -          - const: pwr
> -          - const: pipe
> -
>    vpcie3v3-supply: true
>  
>  required:
> -  - compatible
> -  - reg
> -  - reg-names
> -  - clocks
> -  - clock-names
>    - msi-map
> -  - num-lanes
> -  - phys
> -  - phy-names
> -  - power-domains
> -  - resets
> -  - reset-names
>  
>  unevaluatedProperties: false
>  
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 05/13] dt-bindings: PCI: rockchip-dw-pcie: Fix description of legacy irq

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:28:59AM +0200, Niklas Cassel wrote:
> The descriptions of the combined interrupt signals (level1) mention
> all the lower interrupt signals (level2) for each combined interrupt,
> regardless if the lower (level2) signal is RC or EP specific.
> 
> E.g. the description of "Combined system interrupt" includes rbar_update,
> which is EP specific, and the description of "Combined message interrupt"
> includes obff_idle, obff_obff, obff_cpu_active, which are all EP specific.
> 
> The only exception is the "Combined legacy interrupt", which for some
> reason does not provide an exhaustive list of the lower (level2) signals.
> 
> Add the missing lower interrupt signals: tx_inta, tx_intb, tx_intc, and
> tx_intd for the "Combined legacy interrupt", as per the rk3568 and rk3588
> Technical Reference Manuals, such that the descriptions of the combined
> interrupt signals are consistent.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> index 60d190a77580..ec5e6a3d048e 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> @@ -56,7 +56,8 @@ properties:
>            pm_pme, pm_to_ack, pm_turnoff, obff_idle, obff_obff, obff_cpu_active
>        - description:
>            Combined legacy interrupt, which is used to signal the following
> -          interrupts - inta, intb, intc, intd
> +          interrupts - inta, intb, intc, intd, tx_inta, tx_intb, tx_intc,
> +          tx_intd
>        - description:
>            Combined error interrupt, which is used to signal the following
>            interrupts - aer_rc_err, aer_rc_err_msi, rx_cpl_timeout,
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 06/13] dt-bindings: rockchip: Add DesignWare based PCIe Endpoint controller

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:29:00AM +0200, Niklas Cassel wrote:
> Document DT bindings for PCIe Endpoint controller found in Rockchip SoCs.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Couple of nitpicks below. With those fixed,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> ---
>  .../bindings/pci/rockchip-dw-pcie-common.yaml      | 14 ++++
>  .../bindings/pci/rockchip-dw-pcie-ep.yaml          | 95 ++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> index ec5e6a3d048e..cc9adfc7611c 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-common.yaml
> @@ -39,6 +39,7 @@ properties:
>        - const: ref
>  
>    interrupts:
> +    minItems: 5
>      items:
>        - description:
>            Combined system interrupt, which is used to signal the following
> @@ -63,14 +64,27 @@ properties:
>            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
> +      - description:
> +          eDMA write channel 0 interrupt
> +      - description:
> +          eDMA write channel 1 interrupt
> +      - description:
> +          eDMA read channel 0 interrupt
> +      - description:
> +          eDMA read channel 1 interrupt
>  
>    interrupt-names:
> +    minItems: 5
>      items:
>        - const: sys
>        - const: pmc
>        - const: msg
>        - const: legacy
>        - const: err
> +      - const: dma0
> +      - const: dma1
> +      - const: dma2
> +      - const: dma3
>  
>    num-lanes: true
>  
> diff --git a/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
> new file mode 100644
> index 000000000000..e0c8668afc01
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/rockchip-dw-pcie-ep.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: DesignWare based PCIe Endpoint controller on Rockchip SoCs
> +
> +maintainers:
> +  - Niklas Cassel <cassel@kernel.org>
> +
> +description: |+
> +  RK3588 SoC PCIe Endpoint controller is based on the Synopsys DesignWare
> +  PCIe IP and thus inherits all the common properties defined in
> +  snps,dw-pcie-ep.yaml.
> +
> +allOf:
> +  - $ref: /schemas/pci/snps,dw-pcie-ep.yaml#
> +  - $ref: /schemas/pci/rockchip-dw-pcie-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3568-pcie-ep
> +      - rockchip,rk3588-pcie-ep
> +
> +  reg:
> +    items:
> +      - description: Data Bus Interface (DBI) registers
> +      - description: Data Bus Interface (DBI) shadow registers
> +      - description: Rockchip designed configuration registers
> +      - description: Memory region used to map remote RC address space
> +      - description: Address Translation Unit (ATU) registers

Nit: Internal Address Translation Unit (iATU)

> +
> +  reg-names:
> +    items:
> +      - const: dbi
> +      - const: dbi2
> +      - const: apb
> +      - const: addr_space
> +      - const: atu
> +
> +required:
> +  - interrupts
> +  - interrupt-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/power/rk3588-power.h>
> +    #include <dt-bindings/reset/rockchip,rk3588-cru.h>
> +
> +    bus {

Even though 'bus' node is also used in many bindings, I prefer 'soc' since it
fits better IMO.

> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pcie3x4_ep: pcie-ep@fe150000 {
> +            compatible = "rockchip,rk3588-pcie-ep";
> +            clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
> +                     <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
> +                     <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>;
> +            clock-names = "aclk_mst", "aclk_slv",
> +                          "aclk_dbi", "pclk",
> +                          "aux", "pipe";
> +            interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH 0>,
> +                         <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH 0>;
> +            interrupt-names = "sys", "pmc", "msg", "legacy", "err",
> +                              "dma0", "dma1", "dma2", "dma3";
> +            max-link-speed = <3>;
> +            num-lanes = <4>;
> +            phys = <&pcie30phy>;
> +            phy-names = "pcie-phy";
> +            power-domains = <&power RK3588_PD_PCIE>;
> +            reg = <0xa 0x40000000 0x0 0x00100000>,
> +                  <0xa 0x40100000 0x0 0x00100000>,
> +                  <0x0 0xfe150000 0x0 0x00010000>,
> +                  <0x9 0x00000000 0x0 0x40000000>,
> +                  <0xa 0x40300000 0x0 0x00100000>;
> +            reg-names = "dbi", "dbi2", "apb", "addr_space", "atu";

Can you move 'reg' and 'reg-names' below 'compatible' to align with common
convention?

- Mani

> +            resets = <&cru SRST_PCIE0_POWER_UP>, <&cru SRST_P_PCIE0>;
> +            reset-names = "pwr", "pipe";
> +        };
> +    };
> +...
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 08/13] PCI: dw-rockchip: Add rockchip_pcie_get_ltssm() helper

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:29:02AM +0200, Niklas Cassel wrote:
> Add a rockchip_pcie_ltssm() helper function that reads the LTSSM status.
> This helper will be used in additional places in follow-up commits.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 3dfed08ef456..1380e3a5284b 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -143,6 +143,11 @@ static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
>  	return 0;
>  }
>  
> +static u32 rockchip_pcie_get_ltssm(struct rockchip_pcie *rockchip)
> +{
> +	return rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS);
> +}
> +
>  static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
>  {
>  	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_ENABLE_LTSSM,
> @@ -152,7 +157,7 @@ static void rockchip_pcie_enable_ltssm(struct rockchip_pcie *rockchip)
>  static int rockchip_pcie_link_up(struct dw_pcie *pci)
>  {
>  	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
> -	u32 val = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_LTSSM_STATUS);
> +	u32 val = rockchip_pcie_get_ltssm(rockchip);
>  
>  	if ((val & PCIE_LINKUP) == PCIE_LINKUP &&
>  	    (val & PCIE_LTSSM_STATUS_MASK) == PCIE_L0S_ENTRY)
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 09/13] PCI: dw-rockchip: Refactor the driver to prepare for EP mode

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:29:03AM +0200, Niklas Cassel wrote:
> This refactors the driver to prepare for EP mode.
> Add of-match data to the existing compatible, and explicitly define it as
> DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up
> commit in a much less intrusive way, which makes the follup-up commit much
> easier to review.
> 
> No functional change intended.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Few nitpicks below. With those addressed,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 84 +++++++++++++++++++--------
>  1 file changed, 60 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 1380e3a5284b..e133511692af 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -49,15 +49,20 @@
>  #define PCIE_LTSSM_STATUS_MASK		GENMASK(5, 0)
>  
>  struct rockchip_pcie {
> -	struct dw_pcie			pci;
> -	void __iomem			*apb_base;
> -	struct phy			*phy;
> -	struct clk_bulk_data		*clks;
> -	unsigned int			clk_cnt;
> -	struct reset_control		*rst;
> -	struct gpio_desc		*rst_gpio;
> -	struct regulator                *vpcie3v3;
> -	struct irq_domain		*irq_domain;
> +	struct dw_pcie				pci;
> +	void __iomem				*apb_base;
> +	struct phy				*phy;
> +	struct clk_bulk_data			*clks;
> +	unsigned int				clk_cnt;
> +	struct reset_control			*rst;
> +	struct gpio_desc			*rst_gpio;
> +	struct regulator			*vpcie3v3;
> +	struct irq_domain			*irq_domain;
> +	const struct rockchip_pcie_of_data	*data;

I prefer to avoid aligning the struct members just for this reason. Once you add
a member with a bigger name, then you need to align others also. Please just use
space.

> +};
> +
> +struct rockchip_pcie_of_data {
> +	enum dw_pcie_device_mode mode;
>  };
>  
>  static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg)
> @@ -195,7 +200,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct rockchip_pcie *rockchip = to_rockchip_pcie(pci);
>  	struct device *dev = rockchip->pci.dev;
> -	u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
>  	int irq, ret;
>  
>  	irq = of_irq_get_byname(dev->of_node, "legacy");
> @@ -209,12 +213,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp)
>  	irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler,
>  					 rockchip);
>  
> -	/* LTSSM enable control mode */
> -	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> -
> -	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
> -				 PCIE_CLIENT_GENERAL_CONTROL);
> -
>  	return 0;
>  }
>  
> @@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = {
>  	.start_link = rockchip_pcie_start_link,
>  };
>  
> +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip)
> +{
> +	struct dw_pcie_rp *pp;
> +	u32 val;
> +
> +	/* LTSSM enable control mode */
> +	val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> +	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> +
> +	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
> +				 PCIE_CLIENT_GENERAL_CONTROL);
> +
> +	pp = &rockchip->pci.pp;
> +	pp->ops = &rockchip_pcie_host_ops;
> +
> +	return dw_pcie_host_init(pp);
> +}
> +
>  static int rockchip_pcie_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct rockchip_pcie *rockchip;
> -	struct dw_pcie_rp *pp;
> +	const struct rockchip_pcie_of_data *data;
>  	int ret;
>  
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return -EINVAL;

-ENODATA?

> +
>  	rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL);
>  	if (!rockchip)
>  		return -ENOMEM;
> @@ -309,9 +329,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  
>  	rockchip->pci.dev = dev;
>  	rockchip->pci.ops = &dw_pcie_ops;
> -
> -	pp = &rockchip->pci.pp;
> -	pp->ops = &rockchip_pcie_host_ops;
> +	rockchip->data = data;
>  
>  	ret = rockchip_pcie_resource_get(pdev, rockchip);
>  	if (ret)
> @@ -347,10 +365,21 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto deinit_phy;
>  
> -	ret = dw_pcie_host_init(pp);
> -	if (!ret)
> -		return 0;

Thanks a lot for getting rid of this ugly piece of code!

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 10/13] PCI: dw-rockchip: Add endpoint mode support

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:29:04AM +0200, Niklas Cassel wrote:
> The PCIe controller in rk3568 and rk3588 can operate in endpoint mode.
> This endpoint mode support heavily leverages the existing code in
> pcie-designware-ep.c.
> 
> Add support for endpoint mode to the existing pcie-dw-rockchip glue
> driver.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Couple of comments below. With those addressed,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> ---
>  drivers/pci/controller/dwc/Kconfig            |  17 ++-
>  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++
>  2 files changed, 224 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index 8afacc90c63b..9fae0d977271 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP
>  	  SoCs. To compile this driver as a module, choose M here: the module
>  	  will be called pcie-rcar-gen4.ko. This uses the DesignWare core.
>  
> +config PCIE_ROCKCHIP_DW
> +	bool

Where is this symbol used?

> +
>  config PCIE_ROCKCHIP_DW_HOST
> -	bool "Rockchip DesignWare PCIe controller"
> -	select PCIE_DW
> +	bool "Rockchip DesignWare PCIe controller (host mode)"
>  	select PCIE_DW_HOST
>  	depends on PCI_MSI
>  	depends on ARCH_ROCKCHIP || COMPILE_TEST
>  	depends on OF
>  	help
>  	  Enables support for the DesignWare PCIe controller in the
> -	  Rockchip SoC except RK3399.
> +	  Rockchip SoC (except RK3399) to work in host mode.
> +
> +config PCIE_ROCKCHIP_DW_EP
> +	bool "Rockchip DesignWare PCIe controller (endpoint mode)"
> +	select PCIE_DW_EP
> +	depends on ARCH_ROCKCHIP || COMPILE_TEST
> +	depends on OF
> +	help
> +	  Enables support for the DesignWare PCIe controller in the
> +	  Rockchip SoC (except RK3399) to work in endpoint mode.
>  
>  config PCI_EXYNOS
>  	tristate "Samsung Exynos PCIe controller"
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index e133511692af..347721207161 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c

[...]

> +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> +{
> +	struct rockchip_pcie *rockchip = arg;
> +	struct dw_pcie *pci = &rockchip->pci;
> +	struct device *dev = pci->dev;
> +	u32 reg, val;
> +
> +	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
> +
> +	dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg);
> +	dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
> +
> +	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
> +		dev_dbg(dev, "hot reset or link-down reset\n");
> +		dw_pcie_ep_linkdown(&pci->ep);
> +	}
> +
> +	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> +		val = rockchip_pcie_get_ltssm(rockchip);
> +		if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> +			dev_dbg(dev, "link up\n");
> +			dw_pcie_ep_linkup(&pci->ep);
> +		}
> +	}
> +
> +	rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);

It is recommended to clear the IRQs at the start of the handler (after status
read).

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 12/13] arm64: dts: rockchip: Add PCIe endpoint mode support

[Manivannan Sadhasivam]

On Wed, May 29, 2024 at 10:29:06AM +0200, Niklas Cassel wrote:
> Add a device tree node representing PCIe endpoint mode.
> 
> The controller can either be configured to run in Root Complex or Endpoint
> node.
> 
> If a user wants to run the controller in endpoint mode, the user has to
> disable the pcie3x4 node and enable the pcie3x4_ep node.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

One nitpick below.

> ---
>  arch/arm64/boot/dts/rockchip/rk3588.dtsi | 35 ++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588.dtsi b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
> index 5984016b5f96..6b5bf1055143 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588.dtsi
> @@ -186,6 +186,41 @@ pcie3x4_intc: legacy-interrupt-controller {
>  		};
>  	};
>  
> +	pcie3x4_ep: pcie-ep@fe150000 {
> +		compatible = "rockchip,rk3588-pcie-ep";
> +		clocks = <&cru ACLK_PCIE_4L_MSTR>, <&cru ACLK_PCIE_4L_SLV>,
> +			 <&cru ACLK_PCIE_4L_DBI>, <&cru PCLK_PCIE_4L>,
> +			 <&cru CLK_PCIE_AUX0>, <&cru CLK_PCIE4L_PIPE>;
> +		clock-names = "aclk_mst", "aclk_slv",
> +			      "aclk_dbi", "pclk",
> +			      "aux", "pipe";
> +		interrupts = <GIC_SPI 263 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 262 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 261 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 260 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 259 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 269 IRQ_TYPE_LEVEL_HIGH 0>,
> +			     <GIC_SPI 270 IRQ_TYPE_LEVEL_HIGH 0>;
> +		interrupt-names = "sys", "pmc", "msg", "legacy", "err",
> +				  "dma0", "dma1", "dma2", "dma3";
> +		max-link-speed = <3>;
> +		num-lanes = <4>;
> +		phys = <&pcie30phy>;
> +		phy-names = "pcie-phy";
> +		power-domains = <&power RK3588_PD_PCIE>;
> +		reg = <0xa 0x40000000 0x0 0x00100000>,
> +		      <0xa 0x40100000 0x0 0x00100000>,
> +		      <0x0 0xfe150000 0x0 0x00010000>,
> +		      <0x9 0x00000000 0x0 0x40000000>,
> +		      <0xa 0x40300000 0x0 0x00100000>;
> +		reg-names = "dbi", "dbi2", "apb", "addr_space", "atu";

As suggested in the bindings patch, please move these reg properties below
compatible.

- Mani

> +		resets = <&cru SRST_PCIE0_POWER_UP>, <&cru SRST_P_PCIE0>;
> +		reset-names = "pwr", "pipe";
> +		status = "disabled";
> +	};
> +
>  	pcie3x2: pcie@fe160000 {
>  		compatible = "rockchip,rk3588-pcie", "rockchip,rk3568-pcie";
>  		#address-cells = <3>;
> 
> -- 
> 2.45.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 03/13] dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs

[Niklas Cassel]

On Wed, Jun 05, 2024 at 01:04:02PM +0530, Manivannan Sadhasivam wrote:
> On Wed, May 29, 2024 at 10:28:57AM +0200, Niklas Cassel wrote:
> > The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
> > that are triggered when the PCIe controller (when running in Endpoint mode)
> > has sent an Assert_INTA Message to the upstream device.
> >
> > Some DWC controllers have these interrupt in a combined interrupt signal.
> >
> > Add the description of these interrupts to the device tree binding.
> >
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
>
> Nit: We recently changed the driver instances of 'LEGACY' to 'INTX'. But the
> binding it still using 'legacy'. Considering that the 'legacy' IRQ added to the
> RC binding recently (ebce9f6623a7), should we rename it?
>
> This will force the driver to support both 'legacy' and 'intx' for backwards
> compatibility.

I don't think this is true.


Look at snps,dw-pcie.yaml in 6.10-rc2:

The individual interrupts are called:
            Legacy A/B/C/D interrupt signal. Basically it's triggered by
            receiving a Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message
            from the downstream device.
          pattern: "^int(a|b|c|d)$"

The combined interrupt is called:
            Combined Legacy A/B/C/D interrupt signal. See "^int(a|b|c|d)$" for
            details.
          const: legacy

So you use 'inta', 'intb', 'intc', 'intd' if your SoC has a dedicated
interrupt line for each of these irqs.

If the SoC simply has a single combined interrupt line for these irqs,
then you use 'legacy'


This patch simply adds:
'tx_inta', 'tx_intb', 'tx_intc', 'tx_intd' as individual interrupts
and the combined interrupt 'legacy' to snps,dw-pcie-ep.yaml.


Patch ebce9f6623a7 simply allowed the combined interrupt line 'legacy'
to be used by the rockchip-dw-pcie.yaml binding.
This is because the way that device tree is designed. You need to specify
something both in the generic binding (which specifies everything),
and in the glue driver binding, to specify the subset that is allowed by
the glue driver.


Since a controller cannot run in both EP and RC mode at the same time,
I think that it is fine that this patch reuses the name 'legacy' for the
combined interrupt.

And as you can see in patch 5 in this series, rk3588 actually uses a single
combined IRQ (called legacy) for 'inta', 'intb', 'intc', 'intd', 'tx_inta',
'tx_intb', 'tx_intc', 'tx_intd'.


Kind regards,
Niklas


>
> But irrespective of that,
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>
> - Mani
>
> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > index f5f12cbc2cb3..f474b9e3fc7e 100644
> > --- a/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > +++ b/Documentation/devicetree/bindings/pci/snps,dw-pcie-ep.yaml
> > @@ -151,6 +151,15 @@ properties:
> >              Application-specific IRQ raised depending on the vendor-specific
> >              events basis.
> >            const: app
> > +        - description:
> > +            Interrupts triggered when the controller itself (in Endpoint mode)
> > +            has sent an Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message to
> > +            the upstream device.
> > +          pattern: "^tx_int(a|b|c|d)$"
> > +        - description:
> > +            Combined interrupt signal raised when the controller has sent an
> > +            Assert_INT{A,B,C,D} message. See "^tx_int(a|b|c|d)$" for details.
> > +          const: legacy
> >          - description:
> >              Vendor-specific IRQ names. Consider using the generic names above
> >              for new bindings.
> >
> > --
> > 2.45.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 09/13] PCI: dw-rockchip: Refactor the driver to prepare for EP mode

[Niklas Cassel]

On Wed, Jun 05, 2024 at 01:36:40PM +0530, Manivannan Sadhasivam wrote:
> On Wed, May 29, 2024 at 10:29:03AM +0200, Niklas Cassel wrote:
> > This refactors the driver to prepare for EP mode.
> > Add of-match data to the existing compatible, and explicitly define it as
> > DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up
> > commit in a much less intrusive way, which makes the follup-up commit much
> > easier to review.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Few nitpicks below. With those addressed,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> > ---

(snip)

> > @@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> >  	.start_link = rockchip_pcie_start_link,
> >  };
> >  
> > +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip)
> > +{
> > +	struct dw_pcie_rp *pp;
> > +	u32 val;
> > +
> > +	/* LTSSM enable control mode */
> > +	val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> > +	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> > +
> > +	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
> > +				 PCIE_CLIENT_GENERAL_CONTROL);
> > +
> > +	pp = &rockchip->pci.pp;
> > +	pp->ops = &rockchip_pcie_host_ops;
> > +
> > +	return dw_pcie_host_init(pp);
> > +}
> > +
> >  static int rockchip_pcie_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct rockchip_pcie *rockchip;
> > -	struct dw_pcie_rp *pp;
> > +	const struct rockchip_pcie_of_data *data;
> >  	int ret;
> >  
> > +	data = of_device_get_match_data(dev);
> > +	if (!data)
> > +		return -EINVAL;
> 
> -ENODATA?

-EINVAL seems to be most common:
$ git grep -A 5 of_device_get_match_data drivers/pci/



Kind regards,
Niklas

Re: [PATCH v4 10/13] PCI: dw-rockchip: Add endpoint mode support

[Niklas Cassel]

On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote:
> On Wed, May 29, 2024 at 10:29:04AM +0200, Niklas Cassel wrote:
> > The PCIe controller in rk3568 and rk3588 can operate in endpoint mode.
> > This endpoint mode support heavily leverages the existing code in
> > pcie-designware-ep.c.
> > 
> > Add support for endpoint mode to the existing pcie-dw-rockchip glue
> > driver.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Couple of comments below. With those addressed,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> > ---
> >  drivers/pci/controller/dwc/Kconfig            |  17 ++-
> >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++
> >  2 files changed, 224 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index 8afacc90c63b..9fae0d977271 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP
> >  	  SoCs. To compile this driver as a module, choose M here: the module
> >  	  will be called pcie-rcar-gen4.ko. This uses the DesignWare core.
> >  
> > +config PCIE_ROCKCHIP_DW
> > +	bool
> 
> Where is this symbol used?

It is supposed to be used by
drivers/pci/controller/dwc/Makefile

such that the driver is compiled if either _EP or _HOST is selected, just
like how it is done for other drivers that support both in the same driver.
Looks like I missed to update Makefile...
Good catch, thank you!


> > +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> > +{
> > +	struct rockchip_pcie *rockchip = arg;
> > +	struct dw_pcie *pci = &rockchip->pci;
> > +	struct device *dev = pci->dev;
> > +	u32 reg, val;
> > +
> > +	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
> > +
> > +	dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg);
> > +	dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
> > +
> > +	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
> > +		dev_dbg(dev, "hot reset or link-down reset\n");
> > +		dw_pcie_ep_linkdown(&pci->ep);
> > +	}
> > +
> > +	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> > +		val = rockchip_pcie_get_ltssm(rockchip);
> > +		if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> > +			dev_dbg(dev, "link up\n");
> > +			dw_pcie_ep_linkup(&pci->ep);
> > +		}
> > +	}
> > +
> > +	rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> 
> It is recommended to clear the IRQs at the start of the handler (after status
> read).

Can you quote a reference in the databook to back this recommendation?

Otherwise I would lean towards keeping it like it is, since this is how
it looks in the downstream driver (that *should* be well proven), and it
also matches how it's done in dra7xx.

(And since you ack only the events you read, you can not accidentally
clear another type of event.)


Kind regards,
Niklas

Re: [PATCH v4 03/13] dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs

[Manivannan Sadhasivam]

On Wed, Jun 05, 2024 at 06:20:58PM +0200, Niklas Cassel wrote:
> On Wed, Jun 05, 2024 at 01:04:02PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, May 29, 2024 at 10:28:57AM +0200, Niklas Cassel wrote:
> > > The DWC core has four interrupt signals: tx_inta, tx_intb, tx_intc, tx_intd
> > > that are triggered when the PCIe controller (when running in Endpoint mode)
> > > has sent an Assert_INTA Message to the upstream device.
> > >
> > > Some DWC controllers have these interrupt in a combined interrupt signal.
> > >
> > > Add the description of these interrupts to the device tree binding.
> > >
> > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> >
> > Nit: We recently changed the driver instances of 'LEGACY' to 'INTX'. But the
> > binding it still using 'legacy'. Considering that the 'legacy' IRQ added to the
> > RC binding recently (ebce9f6623a7), should we rename it?
> >
> > This will force the driver to support both 'legacy' and 'intx' for backwards
> > compatibility.
> 
> I don't think this is true.
> 
> 
> Look at snps,dw-pcie.yaml in 6.10-rc2:
> 
> The individual interrupts are called:
>             Legacy A/B/C/D interrupt signal. Basically it's triggered by
>             receiving a Assert_INT{A,B,C,D}/Desassert_INT{A,B,C,D} message
>             from the downstream device.
>           pattern: "^int(a|b|c|d)$"
> 
> The combined interrupt is called:
>             Combined Legacy A/B/C/D interrupt signal. See "^int(a|b|c|d)$" for
>             details.
>           const: legacy
> 
> So you use 'inta', 'intb', 'intc', 'intd' if your SoC has a dedicated
> interrupt line for each of these irqs.
> 
> If the SoC simply has a single combined interrupt line for these irqs,
> then you use 'legacy'
> 
> 
> This patch simply adds:
> 'tx_inta', 'tx_intb', 'tx_intc', 'tx_intd' as individual interrupts
> and the combined interrupt 'legacy' to snps,dw-pcie-ep.yaml.
> 
> 
> Patch ebce9f6623a7 simply allowed the combined interrupt line 'legacy'
> to be used by the rockchip-dw-pcie.yaml binding.
> This is because the way that device tree is designed. You need to specify
> something both in the generic binding (which specifies everything),
> and in the glue driver binding, to specify the subset that is allowed by
> the glue driver.
> 
> 
> Since a controller cannot run in both EP and RC mode at the same time,
> I think that it is fine that this patch reuses the name 'legacy' for the
> combined interrupt.
> 
> And as you can see in patch 5 in this series, rk3588 actually uses a single
> combined IRQ (called legacy) for 'inta', 'intb', 'intc', 'intd', 'tx_inta',
> 'tx_intb', 'tx_intc', 'tx_intd'.
> 

I think you misunderstood what I was asking. I was just asking if we still want
to keep the term 'legacy' for INTx IRQs in DT binding or not, since we recently
got rid of that terminology in PCI drivers.

But if the rockchip TRM defines it as 'legacy' then it should be called as is in
the rockchip binding. But I don't think DWC Spec also defines it that way (I
haven't checked).

It is a question for Rob and Bjorn.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 09/13] PCI: dw-rockchip: Refactor the driver to prepare for EP mode

[Manivannan Sadhasivam]

On Wed, Jun 05, 2024 at 07:57:12PM +0200, Niklas Cassel wrote:
> On Wed, Jun 05, 2024 at 01:36:40PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, May 29, 2024 at 10:29:03AM +0200, Niklas Cassel wrote:
> > > This refactors the driver to prepare for EP mode.
> > > Add of-match data to the existing compatible, and explicitly define it as
> > > DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up
> > > commit in a much less intrusive way, which makes the follup-up commit much
> > > easier to review.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > 
> > Few nitpicks below. With those addressed,
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > > ---
> 
> (snip)
> 
> > > @@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> > >  	.start_link = rockchip_pcie_start_link,
> > >  };
> > >  
> > > +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip)
> > > +{
> > > +	struct dw_pcie_rp *pp;
> > > +	u32 val;
> > > +
> > > +	/* LTSSM enable control mode */
> > > +	val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE);
> > > +	rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL);
> > > +
> > > +	rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE,
> > > +				 PCIE_CLIENT_GENERAL_CONTROL);
> > > +
> > > +	pp = &rockchip->pci.pp;
> > > +	pp->ops = &rockchip_pcie_host_ops;
> > > +
> > > +	return dw_pcie_host_init(pp);
> > > +}
> > > +
> > >  static int rockchip_pcie_probe(struct platform_device *pdev)
> > >  {
> > >  	struct device *dev = &pdev->dev;
> > >  	struct rockchip_pcie *rockchip;
> > > -	struct dw_pcie_rp *pp;
> > > +	const struct rockchip_pcie_of_data *data;
> > >  	int ret;
> > >  
> > > +	data = of_device_get_match_data(dev);
> > > +	if (!data)
> > > +		return -EINVAL;
> > 
> > -ENODATA?
> 
> -EINVAL seems to be most common:
> $ git grep -A 5 of_device_get_match_data drivers/pci/
> 

Yeah, but we abused -EINVAL a lot ;) Nowadays, I prefer to use more apt error
codes.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 10/13] PCI: dw-rockchip: Add endpoint mode support

[Manivannan Sadhasivam]

On Wed, Jun 05, 2024 at 08:58:06PM +0200, Niklas Cassel wrote:
> On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, May 29, 2024 at 10:29:04AM +0200, Niklas Cassel wrote:
> > > The PCIe controller in rk3568 and rk3588 can operate in endpoint mode.
> > > This endpoint mode support heavily leverages the existing code in
> > > pcie-designware-ep.c.
> > > 
> > > Add support for endpoint mode to the existing pcie-dw-rockchip glue
> > > driver.
> > > 
> > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > 
> > Couple of comments below. With those addressed,
> > 
> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > 
> > > ---
> > >  drivers/pci/controller/dwc/Kconfig            |  17 ++-
> > >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++
> > >  2 files changed, 224 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > index 8afacc90c63b..9fae0d977271 100644
> > > --- a/drivers/pci/controller/dwc/Kconfig
> > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP
> > >  	  SoCs. To compile this driver as a module, choose M here: the module
> > >  	  will be called pcie-rcar-gen4.ko. This uses the DesignWare core.
> > >  
> > > +config PCIE_ROCKCHIP_DW
> > > +	bool
> > 
> > Where is this symbol used?
> 
> It is supposed to be used by
> drivers/pci/controller/dwc/Makefile
> 
> such that the driver is compiled if either _EP or _HOST is selected, just
> like how it is done for other drivers that support both in the same driver.
> Looks like I missed to update Makefile...
> Good catch, thank you!
> 
> 
> > > +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> > > +{
> > > +	struct rockchip_pcie *rockchip = arg;
> > > +	struct dw_pcie *pci = &rockchip->pci;
> > > +	struct device *dev = pci->dev;
> > > +	u32 reg, val;
> > > +
> > > +	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
> > > +
> > > +	dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg);
> > > +	dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
> > > +
> > > +	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
> > > +		dev_dbg(dev, "hot reset or link-down reset\n");
> > > +		dw_pcie_ep_linkdown(&pci->ep);
> > > +	}
> > > +
> > > +	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> > > +		val = rockchip_pcie_get_ltssm(rockchip);
> > > +		if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> > > +			dev_dbg(dev, "link up\n");
> > > +			dw_pcie_ep_linkup(&pci->ep);
> > > +		}
> > > +	}
> > > +
> > > +	rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> > 
> > It is recommended to clear the IRQs at the start of the handler (after status
> > read).
> 
> Can you quote a reference in the databook to back this recommendation?
> 

It is just a general recommendation.

> Otherwise I would lean towards keeping it like it is, since this is how
> it looks in the downstream driver (that *should* be well proven), and it
> also matches how it's done in dra7xx.
> 
> (And since you ack only the events you read, you can not accidentally
> clear another type of event.)
> 

I haven't read the TRM, but if the IRQ line is level triggered, then if you do
not clear the IRQs immediately, you will miss some events. So I always suggest
to clear the IRQs at the start of the handler for all the platforms.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 03/13] dt-bindings: PCI: snps,dw-pcie-ep: Add tx_int{a,b,c,d} legacy irqs

[Niklas Cassel]

On Thu, Jun 06, 2024 at 11:55:38AM +0530, Manivannan Sadhasivam wrote:
> 
> I think you misunderstood what I was asking. I was just asking if we still want
> to keep the term 'legacy' for INTx IRQs in DT binding or not, since we recently
> got rid of that terminology in PCI drivers.

I still don't think that I understand :)

In snps,dw-pcie.yaml we currently (6.10-rc2) have:

const: legacy
(for the combined IRQ)

pattern: "^int(a|b|c|d)$"
(for the individual IRQs)

So we will need to support these indefinitely.

What is it that you would want to rename?

the combined irq?

Doesn't sound like a good idea to me, as we would need to support two
(perhaps that is what you meant).

But even if you wanted to rename it, it would be hard to come up with
a name. Perhaps intx, but that would be super confusing since we already
have inta, intb, intc, intd.

I think it is best just to not touch the binding.

In kernel macros could (that have already been renamed from legacy to intx)
doesn't really have anything to do with the DT binding IMO.


Kind regards,
Niklas

Re: [PATCH v4 10/13] PCI: dw-rockchip: Add endpoint mode support

[Niklas Cassel]

On Thu, Jun 06, 2024 at 12:01:28PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jun 05, 2024 at 08:58:06PM +0200, Niklas Cassel wrote:
> > On Wed, Jun 05, 2024 at 01:47:53PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, May 29, 2024 at 10:29:04AM +0200, Niklas Cassel wrote:
> > > > The PCIe controller in rk3568 and rk3588 can operate in endpoint mode.
> > > > This endpoint mode support heavily leverages the existing code in
> > > > pcie-designware-ep.c.
> > > > 
> > > > Add support for endpoint mode to the existing pcie-dw-rockchip glue
> > > > driver.
> > > > 
> > > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > 
> > > Couple of comments below. With those addressed,
> > > 
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > 
> > > > ---
> > > >  drivers/pci/controller/dwc/Kconfig            |  17 ++-
> > > >  drivers/pci/controller/dwc/pcie-dw-rockchip.c | 210 ++++++++++++++++++++++++++
> > > >  2 files changed, 224 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > > > index 8afacc90c63b..9fae0d977271 100644
> > > > --- a/drivers/pci/controller/dwc/Kconfig
> > > > +++ b/drivers/pci/controller/dwc/Kconfig
> > > > @@ -311,16 +311,27 @@ config PCIE_RCAR_GEN4_EP
> > > >  	  SoCs. To compile this driver as a module, choose M here: the module
> > > >  	  will be called pcie-rcar-gen4.ko. This uses the DesignWare core.
> > > >  
> > > > +config PCIE_ROCKCHIP_DW
> > > > +	bool
> > > 
> > > Where is this symbol used?
> > 
> > It is supposed to be used by
> > drivers/pci/controller/dwc/Makefile
> > 
> > such that the driver is compiled if either _EP or _HOST is selected, just
> > like how it is done for other drivers that support both in the same driver.
> > Looks like I missed to update Makefile...
> > Good catch, thank you!
> > 
> > 
> > > > +static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
> > > > +{
> > > > +	struct rockchip_pcie *rockchip = arg;
> > > > +	struct dw_pcie *pci = &rockchip->pci;
> > > > +	struct device *dev = pci->dev;
> > > > +	u32 reg, val;
> > > > +
> > > > +	reg = rockchip_pcie_readl_apb(rockchip, PCIE_CLIENT_INTR_STATUS_MISC);
> > > > +
> > > > +	dev_dbg(dev, "PCIE_CLIENT_INTR_STATUS_MISC: %#x\n", reg);
> > > > +	dev_dbg(dev, "LTSSM_STATUS: %#x\n", rockchip_pcie_get_ltssm(rockchip));
> > > > +
> > > > +	if (reg & PCIE_LINK_REQ_RST_NOT_INT) {
> > > > +		dev_dbg(dev, "hot reset or link-down reset\n");
> > > > +		dw_pcie_ep_linkdown(&pci->ep);
> > > > +	}
> > > > +
> > > > +	if (reg & PCIE_RDLH_LINK_UP_CHGED) {
> > > > +		val = rockchip_pcie_get_ltssm(rockchip);
> > > > +		if ((val & PCIE_LINKUP) == PCIE_LINKUP) {
> > > > +			dev_dbg(dev, "link up\n");
> > > > +			dw_pcie_ep_linkup(&pci->ep);
> > > > +		}
> > > > +	}
> > > > +
> > > > +	rockchip_pcie_writel_apb(rockchip, reg, PCIE_CLIENT_INTR_STATUS_MISC);
> > > 
> > > It is recommended to clear the IRQs at the start of the handler (after status
> > > read).
> > 
> > Can you quote a reference in the databook to back this recommendation?
> > 
> 
> It is just a general recommendation.
> 
> > Otherwise I would lean towards keeping it like it is, since this is how
> > it looks in the downstream driver (that *should* be well proven), and it
> > also matches how it's done in dra7xx.
> > 
> > (And since you ack only the events you read, you can not accidentally
> > clear another type of event.)
> > 
> 
> I haven't read the TRM, but if the IRQ line is level triggered, then if you do
> not clear the IRQs immediately, you will miss some events. So I always suggest
> to clear the IRQs at the start of the handler for all the platforms.

They are level triggered.
In this specific case, what could happen is that we fail to trigger an IRQ
if we get two succeeding hot/link-down resets, or two succeding link ups.

In neither case is this a serious case (compared to e.g. a host driver
missing a MSI irq), but I have incorporated your suggestion for v5,
thank you!


Kind regards,
Niklas