Hi Rob,

On Fri, Jun 4, 2021 at 11:32 PM Sergio Paracuellos
[off-list ref] wrote:

Hi Rob,

Thanks for the review.

On Fri, Jun 4, 2021 at 9:35 PM Rob Herring [off-list ref] wrote:

quoted

On Sat, May 15, 2021 at 02:40:52PM +0200, Sergio Paracuellos wrote:

quoted

Add device tree binding documentation for PCIe in MT7621 SoCs.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../bindings/pci/mediatek,mt7621-pci.yaml     | 149 ++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml

diff --git a/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml b/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
new file mode 100644
index 000000000000..7f5f9d583032
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml

@@ -0,0 +1,149 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/mediatek,mt7621-pci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT7621 PCIe controller
+
+maintainers:
+  - Sergio Paracuellos <sergio.paracuellos@gmail.com>
+
+description: |+
+  MediaTek MT7621 PCIe subsys supports single Root complex (RC)
+  with 3 Root Ports. Each Root Ports supports a Gen1 1-lane Link
+
+allOf:
+  - $ref: /schemas/pci/pci-bus.yaml#
+
+properties:
+  compatible:
+    const: mediatek,mt7621-pci
+
+  reg:
+    items:
+      - description: host-pci bridge registers
+      - description: pcie port 0 RC control registers
+      - description: pcie port 1 RC control registers
+      - description: pcie port 2 RC control registers
+
+  ranges:
+    maxItems: 2
+
+  resets:
+    items:
+      - description: pcie port 0 reset.
+      - description: pcie port 1 reset.
+      - description: pcie port 2 reset.
+
+  reset-names:
+    items:
+      - const: pcie0
+      - const: pcie1
+      - const: pcie2
+
+  clocks:
+    items:
+      - description: pcie port 0 clock.
+      - description: pcie port 1 clock.
+      - description: pcie port 2 clock.
+
+  clock-names:
+    items:
+      - const: pcie0
+      - const: pcie1
+      - const: pcie2
+
+  phys:
+    items:
+      - description: Dual-ported phy for pcie port 0 and 1.
+      - description: Phy for pcie port 2.
+
+  phy-names:
+    items:
+      - const: pcie-phy0
+      - const: pcie-phy2

If you're going to keep the ports (and I think that's right because
there's only a single PCI address space AFAICT), then I think you should
move resets, clocks, and phys into each port node.

So you'll need to define 'pcie@[0-2],0' node with those properties under
it.

Ok I will move these stuff to each port node. So each port node will
be similar to:

pcie@0,0 {
    reg = <0x0000 0 0 0 0>;
    #address-cells = <3>;
    #size-cells = <2>;
    device_type = "pci";
    #interrupt-cells = <1>;
    clocks = <&clkctrl 24>;
    resets = <&rstctrl 24>;
    phys = <&pcie0_phy 1>;
    interrupt-map-mask = <0 0 0 0>;
    interrupt-map = <0 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
    ranges;
};

How can I be sure by schema that the clocks, reset and phy properties
are in each port node if I move them from the parent? By now each port
node is just validating because of ' $ref:
/schemas/pci/pci-bus.yaml#'.

Thanks in advance for your time.

So I think, this should be enough:

%YAML 1.2
---
$id: http://devicetree.org/schemas/pci/mediatek,mt7621-pci.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#

title: MediaTek MT7621 PCIe controller

maintainers:
  - Sergio Paracuellos [off-list ref]

description: |+
  MediaTek MT7621 PCIe subsys supports single Root complex (RC)
  with 3 Root Ports. Each Root Ports supports a Gen1 1-lane Link

allOf:
  - $ref: /schemas/pci/pci-bus.yaml#

properties:
  compatible:
    const: mediatek,mt7621-pci

  reg:
    items:
      - description: host-pci bridge registers
      - description: pcie port 0 RC control registers
      - description: pcie port 1 RC control registers
      - description: pcie port 2 RC control registers

  ranges:
    maxItems: 2

patternProperties:
  'pcie@[0-2],0':
    type: object
    $ref: /schemas/pci/pci-bus.yaml#

    properties:
      resets:
        maxItems: 1

      clocks:
        maxItems: 1

      phys:
        maxItems: 1

    required:
      - "#interrupt-cells"
      - interrupt-map-mask
      - interrupt-map
      - resets
      - clocks
      - phys
      - phy-names
      - ranges

    unevaluatedProperties: false

required:
  - compatible
  - reg
  - ranges
  - "#interrupt-cells"
  - interrupt-map-mask
  - interrupt-map
  - reset-gpios

unevaluatedProperties: false

examples:
  - |
    #include <dt-bindings/gpio/gpio.h>
    #include <dt-bindings/interrupt-controller/mips-gic.h>

    pcie: pcie@1e140000 {
        compatible = "mediatek,mt7621-pci";
        reg = <0x1e140000 0x100>,
              <0x1e142000 0x100>,
              <0x1e143000 0x100>,
              <0x1e144000 0x100>;

        #address-cells = <3>;
        #size-cells = <2>;
        pinctrl-names = "default";
        pinctrl-0 = <&pcie_pins>;
        device_type = "pci";
        ranges = <0x02000000 0 0x00000000 0x60000000 0 0x10000000>,
/* pci memory */
                 <0x01000000 0 0x00000000 0x1e160000 0 0x00010000>;
/* io space */
        #interrupt-cells = <1>;
        interrupt-map-mask = <0xF800 0 0 0>;
        interrupt-map = <0x0000 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>,
                        <0x0800 0 0 0 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>,
                        <0x1000 0 0 0 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
        reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;

        pcie@0,0 {
            reg = <0x0000 0 0 0 0>;
            #address-cells = <3>;
            #size-cells = <2>;
            device_type = "pci";
            #interrupt-cells = <1>;
            interrupt-map-mask = <0 0 0 0>;
            interrupt-map = <0 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
            resets = <&rstctrl 24>;
            clocks = <&clkctrl 24>;
            phys = <&pcie0_phy 1>;
            phy-names = "pcie-phy0";
            ranges;
        };

        pcie@1,0 {
            reg = <0x0800 0 0 0 0>;
            #address-cells = <3>;
            #size-cells = <2>;
            device_type = "pci";
            #interrupt-cells = <1>;
            interrupt-map-mask = <0 0 0 0>;
            interrupt-map = <0 0 0 0 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>;
            resets = <&rstctrl 25>;
            clocks = <&clkctrl 25>;
            phys = <&pcie0_phy 1>;
            phy-names = "pcie-phy1";
            ranges;
        };

        pcie@2,0 {
            reg = <0x1000 0 0 0 0>;
            #address-cells = <3>;
            #size-cells = <2>;
            device_type = "pci";
            #interrupt-cells = <1>;
            interrupt-map-mask = <0 0 0 0>;
            interrupt-map = <0 0 0 0 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
            resets = <&rstctrl 26>;
            clocks = <&clkctrl 26>;
            phys = <&pcie2_phy 0>;
            phy-names = "pcie-phy2";
            ranges;
        };
    };
...

I will send these bindings for v2 if you are ok with them.

Thanks,
     Sergio Paracuellos

Best regards,
    Sergio Paracuellos

quoted

quoted

+
+required:
+  - compatible
+  - reg
+  - ranges
+  - "#interrupt-cells"
+  - interrupt-map-mask
+  - interrupt-map
+  - resets
+  - reset-names
+  - clocks
+  - clock-names
+  - phys
+  - phy-names
+  - reset-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/mips-gic.h>
+
+    pcie: pcie@1e140000 {
+        compatible = "mediatek,mt7621-pci";
+        reg = <0x1e140000 0x100>,
+              <0x1e142000 0x100>,
+              <0x1e143000 0x100>,
+              <0x1e144000 0x100>;
+
+        #address-cells = <3>;
+        #size-cells = <2>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&pcie_pins>;
+        device_type = "pci";
+        ranges = <0x02000000 0 0x00000000 0x60000000 0 0x10000000>,  /* pci memory */
+                 <0x01000000 0 0x00000000 0x1e160000 0 0x00010000>;  /* io space */
+        #interrupt-cells = <1>;
+        interrupt-map-mask = <0xF800 0 0 0>;
+        interrupt-map = <0x0000 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x0800 0 0 0 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>,
+                        <0x1000 0 0 0 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
+        resets = <&rstctrl 24>, <&rstctrl 25>, <&rstctrl 26>;
+        reset-names = "pcie0", "pcie1", "pcie2";
+        clocks = <&clkctrl 24>, <&clkctrl 25>, <&clkctrl 26>;
+        clock-names = "pcie0", "pcie1", "pcie2";
+        phys = <&pcie0_phy 1>, <&pcie2_phy 0>;
+        phy-names = "pcie-phy0", "pcie-phy2";
+        reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>;
+
+        pcie@0,0 {
+            reg = <0x0000 0 0 0 0>;
+            #address-cells = <3>;
+            #size-cells = <2>;
+            device_type = "pci";
+            #interrupt-cells = <1>;
+            interrupt-map-mask = <0 0 0 0>;
+            interrupt-map = <0 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
+            ranges;
+        };
+
+        pcie@1,0 {
+            reg = <0x0800 0 0 0 0>;
+            #address-cells = <3>;
+            #size-cells = <2>;
+            device_type = "pci";
+            #interrupt-cells = <1>;
+            interrupt-map-mask = <0 0 0 0>;
+            interrupt-map = <0 0 0 0 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>;
+            ranges;
+        };
+
+        pcie@2,0 {
+            reg = <0x1000 0 0 0 0>;
+            #address-cells = <3>;
+            #size-cells = <2>;
+            device_type = "pci";
+            #interrupt-cells = <1>;
+            interrupt-map-mask = <0 0 0 0>;
+            interrupt-map = <0 0 0 0 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
+            ranges;
+        };
+    };
+...
--
2.25.1