diff mbox series

[04/12] dt-bindings: rockchip: Add DesignWare based PCIe Endpoint controller

Message ID 20240424-rockchip-pcie-ep-v1-v1-4-b1a02ddad650@kernel.org (mailing list archive)
State Superseded
Headers show
Series PCI: dw-rockchip: Add endpoint mode support | expand

Commit Message

Niklas Cassel April 24, 2024, 3:16 p.m. UTC
Document DT bindings for PCIe Endpoint controller found in Rockchip SoCs.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 .../bindings/pci/rockchip-dw-pcie-ep.yaml          | 192 +++++++++++++++++++++
 1 file changed, 192 insertions(+)

Comments

Rob Herring (Arm) April 25, 2024, 4:08 p.m. UTC | #1
On Wed, Apr 24, 2024 at 05:16:22PM +0200, Niklas Cassel wrote:
> Document DT bindings for PCIe Endpoint controller found in Rockchip SoCs.
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  .../bindings/pci/rockchip-dw-pcie-ep.yaml          | 192 +++++++++++++++++++++
>  1 file changed, 192 insertions(+)
> 
> 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..57a6c542058f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
> @@ -0,0 +1,192 @@
> +# 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#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: rockchip,rk3588-pcie-ep

3568 doesn't support endpoint mode? It would be good to keep the 
bindings aligned.

> +
> +  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
> +
> +  clocks:
> +    minItems: 6
> +    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: 6
> +    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 - 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,
> +          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:
> +    items:
> +      - const: sys
> +      - const: pmc
> +      - const: msg
> +      - const: legacy
> +      - const: err
> +      - const: dma0
> +      - const: dma1
> +      - const: dma2
> +      - const: dma3
> +
> +  num-lanes: true
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    const: pcie-phy
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  resets:
> +    maxItems: 2
> +
> +  reset-names:
> +    items:
> +      - const: pwr
> +      - const: pipe

Most of this is all duplicated from rockchip-dw-pcie.yaml. Pull out the 
common bits to a separate schema file and reference it from the RC and 
endpoint schemas.

You'll need to add to interrupts/interrupt-names in the common schema 
and then restrict the number of items here and in the RC schema.

> +
> +  vpcie3v3-supply: true

This doesn't make sense for endpoint mode. At least in the sense this 
is supposed to be a standard slot voltage driven from the host side.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - num-lanes
> +  - phys
> +  - phy-names
> +  - power-domains
> +  - resets
> +  - reset-names

A bunch or all? of these can be in the common schema too.

> +
> +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.44.0
>
Niklas Cassel April 30, 2024, 11:34 a.m. UTC | #2
On Thu, Apr 25, 2024 at 11:08:09AM -0500, Rob Herring wrote:
> On Wed, Apr 24, 2024 at 05:16:22PM +0200, Niklas Cassel wrote:
> > Document DT bindings for PCIe Endpoint controller found in Rockchip SoCs.
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > ---
> >  .../bindings/pci/rockchip-dw-pcie-ep.yaml          | 192 +++++++++++++++++++++
> >  1 file changed, 192 insertions(+)
> > 
> > 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..57a6c542058f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
> > @@ -0,0 +1,192 @@
> > +# 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#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: rockchip,rk3588-pcie-ep
> 
> 3568 doesn't support endpoint mode? It would be good to keep the 
> bindings aligned.

It does.
However, it does not have the dedicated IRQ lines for the eDMA interrupts.
I will add rk3568 to the DT binding and to the driver.
If someone wants eDMA functional for rk3568, there is further code needed,
but EP mode without eDMA should work on rk3568 as is.


> > +  phys:
> > +    maxItems: 1
> > +
> > +  phy-names:
> > +    const: pcie-phy
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  resets:
> > +    maxItems: 2
> > +
> > +  reset-names:
> > +    items:
> > +      - const: pwr
> > +      - const: pipe
> 
> Most of this is all duplicated from rockchip-dw-pcie.yaml. Pull out the 
> common bits to a separate schema file and reference it from the RC and 
> endpoint schemas.

Ok, will fix in V2.


> You'll need to add to interrupts/interrupt-names in the common schema 
> and then restrict the number of items here and in the RC schema.

Remember that eDMA can be used also in RC mode.
Even if the RC binding doesn't allow it right now, these interrupts could
be optional also for RC mode, in case someone actually wants to use them
in the future.


> > +
> > +  vpcie3v3-supply: true
> 
> This doesn't make sense for endpoint mode. At least in the sense this 
> is supposed to be a standard slot voltage driven from the host side.

I tried not supplying the regulator for the EP side on my rock5b
(rk3588 based) platform.
The driver (in EP mode) probes correctly, but does not work without this,
regardless of how I try. Boot EP first, boot RC first.

Looking at the rock5b schematic:
https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf
Page 7, specifically VCC3V3_PCIE30.
It does seem to only support sourcing VIN from a regulator on the local
board (VCC5V0_SYS).

(Looking at a vendor using this SoC in a board that supports EP mode
(Mixtile Blade 3), they do supply the regulator also for the EP-mode
DT node.)

I will drop the "vpcie3v3-supply" from the EP binding and keep it
only in the RC binding. (As perhaps some other rk3588 based board can
actually source the 3.3v from the PCIe slot in EP mode.)

I will keep it in the rock5b (a rk3588 based board) DT overlay,
as it is obviously needed for rock5b.


> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - num-lanes
> > +  - phys
> > +  - phy-names
> > +  - power-domains
> > +  - resets
> > +  - reset-names
> 
> A bunch or all? of these can be in the common schema too.

Ok, will fix in V2.


Kind regards,
Niklas
diff mbox series

Patch

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..57a6c542058f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rockchip-dw-pcie-ep.yaml
@@ -0,0 +1,192 @@ 
+# 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#
+
+properties:
+  compatible:
+    items:
+      - const: 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
+
+  clocks:
+    minItems: 6
+    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: 6
+    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 - 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,
+          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:
+    items:
+      - const: sys
+      - const: pmc
+      - const: msg
+      - const: legacy
+      - const: err
+      - const: dma0
+      - const: dma1
+      - const: dma2
+      - const: dma3
+
+  num-lanes: true
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pcie-phy
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 2
+
+  reset-names:
+    items:
+      - const: pwr
+      - const: pipe
+
+  vpcie3v3-supply: true
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - num-lanes
+  - phys
+  - phy-names
+  - power-domains
+  - resets
+  - reset-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";
+        };
+    };
+...