diff mbox series

[v2,1/3] dt-bindings: pci: Add devicetree binding for Qualcomm PCIe EP controller

Message ID 20210603103814.95177-2-manivannan.sadhasivam@linaro.org
State New
Headers show
Series Add Qualcomm PCIe Endpoint driver support | expand

Commit Message

Manivannan Sadhasivam June 3, 2021, 10:38 a.m. UTC
Add devicetree binding for Qualcomm PCIe EP controller used in platforms
like SDX55. The EP controller is based on the Designware core with
Qualcomm specific wrappers.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 144 ++++++++++++++++++
 1 file changed, 144 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml

Comments

Bjorn Andersson June 6, 2021, 3:13 a.m. UTC | #1
On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:

> Add devicetree binding for Qualcomm PCIe EP controller used in platforms

> like SDX55. The EP controller is based on the Designware core with

> Qualcomm specific wrappers.

> 

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

> ---

>  .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 144 ++++++++++++++++++

>  1 file changed, 144 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml

> 

> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml

> new file mode 100644

> index 000000000000..3e357cb03a5c

> --- /dev/null

> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml

> @@ -0,0 +1,144 @@

> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2

> +---

> +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#

> +$schema: http://devicetree.org/meta-schemas/core.yaml#

> +

> +title: Qualcomm PCIe Endpoint Controller binding

> +

> +maintainers:

> +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

> +

> +allOf:

> +  - $ref: "pci-ep.yaml#"

> +

> +properties:

> +  compatible:

> +    const: qcom,sdx55-pcie-ep


The binding looks good, but this is going to cause us an inevitable
warning as we'd have to describe the controller twice (rc + ep) in the
sdx55.dtsi.

@Rob, what do you suggest we do about this, because it's the same
problem currently responsible for hundreds of warnings in the case of
GENI (where each node is duplicated for different functions).

> +

> +  reg:

> +    items:

> +      - description: Qualcomm specific PARF configuration registers

> +      - description: Designware PCIe registers

> +      - description: External local bus interface registers

> +      - description: Address Translation Unit (ATU) registers

> +      - description: Memory region used to map remote RC address space

> +      - description: Qualcomm specific TCSR registers


tcsr is separate hardware block with "misc" registers, I think it's
better if we describe that as qcom,sdx55-tcsr, syscon and use the syscon
API to access those registers...

Regards,
Bjorn

> +

> +  reg-names:

> +    items:

> +      - const: parf

> +      - const: dbi

> +      - const: elbi

> +      - const: atu

> +      - const: addr_space

> +      - const: tcsr

> +

> +  clocks:

> +    items:

> +      - description: PCIe Auxiliary clock

> +      - description: PCIe CFG AHB clock

> +      - description: PCIe Master AXI clock

> +      - description: PCIe Slave AXI clock

> +      - description: PCIe Slave Q2A AXI clock

> +      - description: PCIe Sleep clock

> +      - description: PCIe Reference clock

> +

> +  clock-names:

> +    items:

> +      - const: aux

> +      - const: cfg

> +      - const: bus_master

> +      - const: bus_slave

> +      - const: slave_q2a

> +      - const: sleep

> +      - const: ref

> +

> +  interrupts:

> +    maxItems: 1

> +    description: PCIe Global interrupt

> +

> +  interrupt-names:

> +    const: global

> +

> +  reset-gpios:

> +    description: GPIO that is being used as PERST# input signal

> +    maxItems: 1

> +

> +  wake-gpios:

> +    description: GPIO that is being used as WAKE# output signal

> +    maxItems: 1

> +

> +  resets:

> +    maxItems: 1

> +

> +  reset-names:

> +    const: core

> +

> +  power-domains:

> +    maxItems: 1

> +

> +  phys:

> +    maxItems: 1

> +

> +  phy-names:

> +    const: pciephy

> +

> +  num-lanes:

> +    default: 2

> +

> +required:

> +  - compatible

> +  - reg

> +  - reg-names

> +  - clocks

> +  - clock-names

> +  - interrupts

> +  - interrupt-names

> +  - reset-gpios

> +  - resets

> +  - reset-names

> +  - power-domains

> +

> +unevaluatedProperties: false

> +

> +examples:

> +  - |

> +    #include <dt-bindings/clock/qcom,gcc-sdx55.h>

> +    #include <dt-bindings/gpio/gpio.h>

> +    #include <dt-bindings/interrupt-controller/arm-gic.h>

> +    pcie_ep: pcie-ep@40000000 {

> +        compatible = "qcom,sdx55-pcie-ep";

> +        reg = <0x01c00000 0x3000>,

> +              <0x40000000 0xf1d>,

> +              <0x40000f20 0xc8>,

> +              <0x40001000 0x1000>,

> +              <0x42000000 0x1000>,

> +              <0x01fcb000 0x1000>;

> +        reg-names = "parf", "dbi", "elbi", "atu", "addr_space",

> +                    "tcsr";

> +

> +        clocks = <&gcc GCC_PCIE_AUX_CLK>,

> +             <&gcc GCC_PCIE_CFG_AHB_CLK>,

> +             <&gcc GCC_PCIE_MSTR_AXI_CLK>,

> +             <&gcc GCC_PCIE_SLV_AXI_CLK>,

> +             <&gcc GCC_PCIE_SLV_Q2A_AXI_CLK>,

> +             <&gcc GCC_PCIE_SLEEP_CLK>,

> +             <&gcc GCC_PCIE_0_CLKREF_CLK>;

> +        clock-names = "aux", "cfg", "bus_master", "bus_slave",

> +                      "slave_q2a", "sleep", "ref";

> +

> +        interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;

> +        interrupt-names = "global";

> +        reset-gpios = <&tlmm 57 GPIO_ACTIVE_HIGH>;

> +        wake-gpios = <&tlmm 53 GPIO_ACTIVE_LOW>;

> +        resets = <&gcc GCC_PCIE_BCR>;

> +        reset-names = "core";

> +        power-domains = <&gcc PCIE_GDSC>;

> +        phys = <&pcie0_lane>;

> +        phy-names = "pciephy";

> +        max-link-speed = <3>;

> +        num-lanes = <2>;

> +

> +        status = "disabled";

> +    };

> -- 

> 2.25.1

>
Rob Herring June 10, 2021, 9:46 p.m. UTC | #2
On Sat, Jun 05, 2021 at 10:13:57PM -0500, Bjorn Andersson wrote:
> On Thu 03 Jun 05:38 CDT 2021, Manivannan Sadhasivam wrote:
> 
> > Add devicetree binding for Qualcomm PCIe EP controller used in platforms
> > like SDX55. The EP controller is based on the Designware core with
> > Qualcomm specific wrappers.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  .../devicetree/bindings/pci/qcom,pcie-ep.yaml | 144 ++++++++++++++++++
> >  1 file changed, 144 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > new file mode 100644
> > index 000000000000..3e357cb03a5c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
> > @@ -0,0 +1,144 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm PCIe Endpoint Controller binding
> > +
> > +maintainers:
> > +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > +
> > +allOf:
> > +  - $ref: "pci-ep.yaml#"
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,sdx55-pcie-ep
> 
> The binding looks good, but this is going to cause us an inevitable
> warning as we'd have to describe the controller twice (rc + ep) in the
> sdx55.dtsi.
> 
> @Rob, what do you suggest we do about this, because it's the same
> problem currently responsible for hundreds of warnings in the case of
> GENI (where each node is duplicated for different functions).

What determines the mode? Assuming it is fixed for a platform, can't you 
just have 2 .dtsi files and include the right one. The SoC file could 
have the common h/w specific parts (clks, resets, etc.) as those 
shouldn't really be different depending on the mode. IIRC, some PCI 
bindings do this by design (meaning there's only one definition).

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
new file mode 100644
index 000000000000..3e357cb03a5c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie-ep.yaml
@@ -0,0 +1,144 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/qcom,pcie-ep.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm PCIe Endpoint Controller binding
+
+maintainers:
+  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+allOf:
+  - $ref: "pci-ep.yaml#"
+
+properties:
+  compatible:
+    const: qcom,sdx55-pcie-ep
+
+  reg:
+    items:
+      - description: Qualcomm specific PARF configuration registers
+      - description: Designware PCIe registers
+      - description: External local bus interface registers
+      - description: Address Translation Unit (ATU) registers
+      - description: Memory region used to map remote RC address space
+      - description: Qualcomm specific TCSR registers
+
+  reg-names:
+    items:
+      - const: parf
+      - const: dbi
+      - const: elbi
+      - const: atu
+      - const: addr_space
+      - const: tcsr
+
+  clocks:
+    items:
+      - description: PCIe Auxiliary clock
+      - description: PCIe CFG AHB clock
+      - description: PCIe Master AXI clock
+      - description: PCIe Slave AXI clock
+      - description: PCIe Slave Q2A AXI clock
+      - description: PCIe Sleep clock
+      - description: PCIe Reference clock
+
+  clock-names:
+    items:
+      - const: aux
+      - const: cfg
+      - const: bus_master
+      - const: bus_slave
+      - const: slave_q2a
+      - const: sleep
+      - const: ref
+
+  interrupts:
+    maxItems: 1
+    description: PCIe Global interrupt
+
+  interrupt-names:
+    const: global
+
+  reset-gpios:
+    description: GPIO that is being used as PERST# input signal
+    maxItems: 1
+
+  wake-gpios:
+    description: GPIO that is being used as WAKE# output signal
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  reset-names:
+    const: core
+
+  power-domains:
+    maxItems: 1
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: pciephy
+
+  num-lanes:
+    default: 2
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - reset-gpios
+  - resets
+  - reset-names
+  - power-domains
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sdx55.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    pcie_ep: pcie-ep@40000000 {
+        compatible = "qcom,sdx55-pcie-ep";
+        reg = <0x01c00000 0x3000>,
+              <0x40000000 0xf1d>,
+              <0x40000f20 0xc8>,
+              <0x40001000 0x1000>,
+              <0x42000000 0x1000>,
+              <0x01fcb000 0x1000>;
+        reg-names = "parf", "dbi", "elbi", "atu", "addr_space",
+                    "tcsr";
+
+        clocks = <&gcc GCC_PCIE_AUX_CLK>,
+             <&gcc GCC_PCIE_CFG_AHB_CLK>,
+             <&gcc GCC_PCIE_MSTR_AXI_CLK>,
+             <&gcc GCC_PCIE_SLV_AXI_CLK>,
+             <&gcc GCC_PCIE_SLV_Q2A_AXI_CLK>,
+             <&gcc GCC_PCIE_SLEEP_CLK>,
+             <&gcc GCC_PCIE_0_CLKREF_CLK>;
+        clock-names = "aux", "cfg", "bus_master", "bus_slave",
+                      "slave_q2a", "sleep", "ref";
+
+        interrupts = <GIC_SPI 140 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "global";
+        reset-gpios = <&tlmm 57 GPIO_ACTIVE_HIGH>;
+        wake-gpios = <&tlmm 53 GPIO_ACTIVE_LOW>;
+        resets = <&gcc GCC_PCIE_BCR>;
+        reset-names = "core";
+        power-domains = <&gcc PCIE_GDSC>;
+        phys = <&pcie0_lane>;
+        phy-names = "pciephy";
+        max-link-speed = <3>;
+        num-lanes = <2>;
+
+        status = "disabled";
+    };