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

[Manivannan Sadhasivam <mani@kernel.org>]

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
> 

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