Add device tree binding documentation for PCIe in MT7621 SoCs.
Signed-off-by: Sergio Paracuellos <[email protected]>
---
Hi Rob,
Some concerns here. I was not be able to found any case similar to
this binding where sub-nodes describing each pcie port interface
are needed. I added them to the 'examples' directly without saying
anything about properties in any other place since its properties
seems to be covered in 'pci-bus.yaml' schema definition. I don't
know if this is the way, I have checked against schema and I noticed
I am forced to add 'device_type' property in each subnode because
schema checker complains that this is mandatory. So I have added
it and schema is properly being validated:
Before add the 'device_type' in each subnode:
/home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@0,0: 'device_type' is a required property
From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
/home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@1,0: 'device_type' is a required property
From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
/home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@2,0: 'device_type' is a required property
From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
After adding it:
CHKDT Documentation/devicetree/bindings/processed-schema-examples.json
SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json
DTEX Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dts
DTC Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
CHECK Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
Looks a bit redundant and maybe I am doing something wrong...
Thanks in advance for clarification.
Best regards,
Sergio Paracuellos
.../bindings/pci/mediatek,mt7621-pci.yaml | 144 ++++++++++++++++++
.../mt7621-pci/mediatek,mt7621-pci.txt | 104 -------------
2 files changed, 144 insertions(+), 104 deletions(-)
create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
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..9c1d05d929a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.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/mediatek,mt7621-pci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek MT7621 PCIe controller
+
+maintainers:
+ - Sergio Paracuellos <[email protected]>
+
+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
+
+ interrupts:
+ maxItems: 3
+
+ 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
+
+required:
+ - compatible
+ - reg
+ - ranges
+ - interrupts
+ - 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";
+ bus-range = <0 255>;
+ ranges = <0x02000000 0 0x00000000 0x60000000 0 0x10000000>, /* pci memory */
+ <0x01000000 0 0x00000000 0x1e160000 0 0x00010000>; /* io space */
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>,
+ <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";
+ ranges;
+ bus-range = <0x00 0xff>;
+ };
+
+ pcie@1,0 {
+ reg = <0x0800 0 0 0 0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ device_type = "pci";
+ ranges;
+ bus-range = <0x00 0xff>;
+ };
+
+ pcie@2,0 {
+ reg = <0x1000 0 0 0 0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ device_type = "pci";
+ ranges;
+ bus-range = <0x00 0xff>;
+ };
+ };
+...
diff --git a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt b/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
deleted file mode 100644
index 327a68267309..000000000000
--- a/drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
+++ /dev/null
@@ -1,104 +0,0 @@
-MediaTek MT7621 PCIe controller
-
-Required properties:
-- compatible: "mediatek,mt7621-pci"
-- device_type: Must be "pci"
-- reg: Base addresses and lengths of the PCIe subsys and root ports.
-- bus-range: Range of bus numbers associated with this controller.
-- #address-cells: Address representation for root ports (must be 3)
-- pinctrl-names : The pin control state names.
-- pinctrl-0: The "default" pinctrl state.
-- #size-cells: Size representation for root ports (must be 2)
-- ranges: Ranges for the PCI memory and I/O regions.
-- #interrupt-cells: Must be 1
-- interrupt-map-mask and interrupt-map: Standard PCI IRQ mapping properties.
- Please refer to the standard PCI bus binding document for a more detailed
- explanation.
-- status: either "disabled" or "okay".
-- resets: Must contain an entry for each entry in reset-names.
- See ../reset/reset.txt for details.
-- reset-names: Must be "pcie0", "pcie1", "pcieN"... based on the number of
- root ports.
-- clocks: Must contain an entry for each entry in clock-names.
- See ../clocks/clock-bindings.txt for details.
-- clock-names: Must be "pcie0", "pcie1", "pcieN"... based on the number of
- root ports.
-- reset-gpios: GPIO specs for the reset pins.
-
-In addition, the device tree node must have sub-nodes describing each PCIe port
-interface, having the following mandatory properties:
-
-Required properties:
-- reg: Only the first four bytes are used to refer to the correct bus number
- and device number.
-- #address-cells: Must be 3
-- #size-cells: Must be 2
-- ranges: Sub-ranges distributed from the PCIe controller node. An empty
- property is sufficient.
-- bus-range: Range of bus numbers associated with this port.
-
-Example for MT7621:
-
- pcie: pcie@1e140000 {
- compatible = "mediatek,mt7621-pci";
- reg = <0x1e140000 0x100 /* host-pci bridge registers */
- 0x1e142000 0x100 /* pcie port 0 RC control registers */
- 0x1e143000 0x100 /* pcie port 1 RC control registers */
- 0x1e144000 0x100>; /* pcie port 2 RC control registers */
-
- #address-cells = <3>;
- #size-cells = <2>;
-
- pinctrl-names = "default";
- pinctrl-0 = <&pcie_pins>;
-
- device_type = "pci";
-
- bus-range = <0 255>;
- ranges = <
- 0x02000000 0 0x00000000 0x60000000 0 0x10000000 /* pci memory */
- 0x01000000 0 0x00000000 0x1e160000 0 0x00010000 /* io space */
- >;
-
- #interrupt-cells = <1>;
- interrupt-map-mask = <0xF0000 0 0 1>;
- interrupt-map = <0x10000 0 0 1 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>,
- <0x20000 0 0 1 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>,
- <0x30000 0 0 1 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
-
- status = "disabled";
-
- resets = <&rstctrl 24 &rstctrl 25 &rstctrl 26>;
- reset-names = "pcie0", "pcie1", "pcie2";
- clocks = <&clkctrl 24 &clkctrl 25 &clkctrl 26>;
- clock-names = "pcie0", "pcie1", "pcie2";
-
- reset-gpios = <&gpio 19 GPIO_ACTIVE_LOW>,
- <&gpio 8 GPIO_ACTIVE_LOW>,
- <&gpio 7 GPIO_ACTIVE_LOW>;
-
- pcie@0,0 {
- reg = <0x0000 0 0 0 0>;
- #address-cells = <3>;
- #size-cells = <2>;
- ranges;
- bus-range = <0x00 0xff>;
- };
-
- pcie@1,0 {
- reg = <0x0800 0 0 0 0>;
- #address-cells = <3>;
- #size-cells = <2>;
- ranges;
- bus-range = <0x00 0xff>;
- };
-
- pcie@2,0 {
- reg = <0x1000 0 0 0 0>;
- #address-cells = <3>;
- #size-cells = <2>;
- ranges;
- bus-range = <0x00 0xff>;
- };
- };
-
--
2.25.1
On Sat, May 01, 2021 at 03:36:46PM +0200, Sergio Paracuellos wrote:
> Add device tree binding documentation for PCIe in MT7621 SoCs.
>
> Signed-off-by: Sergio Paracuellos <[email protected]>
> ---
>
> Hi Rob,
>
> Some concerns here. I was not be able to found any case similar to
> this binding where sub-nodes describing each pcie port interface
> are needed. I added them to the 'examples' directly without saying
> anything about properties in any other place since its properties
> seems to be covered in 'pci-bus.yaml' schema definition. I don't
> know if this is the way, I have checked against schema and I noticed
> I am forced to add 'device_type' property in each subnode because
> schema checker complains that this is mandatory. So I have added
> it and schema is properly being validated:
>
> Before add the 'device_type' in each subnode:
> /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@0,0: 'device_type' is a required property
> >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@1,0: 'device_type' is a required property
> >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@2,0: 'device_type' is a required property
> >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
Each port is a PCI bridge, right? If so, then 'pcie' for the node name
and 'device_type = "pci";' are correct.
>
> After adding it:
> CHKDT Documentation/devicetree/bindings/processed-schema-examples.json
Validates all the schema
> SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json
Preprocesses all the schema
> DTEX Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dts
Extracts the example to dts file
> DTC Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
Converts the example to yaml
> CHECK Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
Runs the checks.
>
> Looks a bit redundant and maybe I am doing something wrong...
>
> Thanks in advance for clarification.
>
> Best regards,
> Sergio Paracuellos
>
>
> .../bindings/pci/mediatek,mt7621-pci.yaml | 144 ++++++++++++++++++
> .../mt7621-pci/mediatek,mt7621-pci.txt | 104 -------------
> 2 files changed, 144 insertions(+), 104 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
> delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
>
> 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..9c1d05d929a2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.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/mediatek,mt7621-pci.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek MT7621 PCIe controller
> +
> +maintainers:
> + - Sergio Paracuellos <[email protected]>
> +
> +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
Are these config space registers or MT7621 specific?
> +
> + ranges:
> + maxItems: 2
> +
> + interrupts:
> + maxItems: 3
What are the 3 interrupts?
> +
> + resets:
> + items:
> + - description: pcie port 0 reset.
> + - description: pcie port 1 reset.
> + - description: pcie port 2 reset.
This and clocks should perhaps be in each child node.
> +
> + 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
> +
> +required:
> + - compatible
> + - reg
> + - ranges
> + - interrupts
> + - 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";
> + bus-range = <0 255>;
That's the default, so not needed.
> + ranges = <0x02000000 0 0x00000000 0x60000000 0 0x10000000>, /* pci memory */
> + <0x01000000 0 0x00000000 0x1e160000 0 0x00010000>; /* io space */
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>,
> + <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";
> + ranges;
> + bus-range = <0x00 0xff>;
Besides being the default, I don't think this makes sense here as it
belongs in the parent unless you had some subset of bus numbers allowed
by the parent.
> + };
> +
> + pcie@1,0 {
> + reg = <0x0800 0 0 0 0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + ranges;
> + bus-range = <0x00 0xff>;
> + };
> +
> + pcie@2,0 {
> + reg = <0x1000 0 0 0 0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + device_type = "pci";
> + ranges;
> + bus-range = <0x00 0xff>;
> + };
> + };
> +...
Hi Rob,
Thanks for the review.
On Thu, May 6, 2021 at 5:18 PM Rob Herring <[email protected]> wrote:
>
> On Sat, May 01, 2021 at 03:36:46PM +0200, Sergio Paracuellos wrote:
> > Add device tree binding documentation for PCIe in MT7621 SoCs.
> >
> > Signed-off-by: Sergio Paracuellos <[email protected]>
> > ---
> >
> > Hi Rob,
> >
> > Some concerns here. I was not be able to found any case similar to
> > this binding where sub-nodes describing each pcie port interface
> > are needed. I added them to the 'examples' directly without saying
> > anything about properties in any other place since its properties
> > seems to be covered in 'pci-bus.yaml' schema definition. I don't
> > know if this is the way, I have checked against schema and I noticed
> > I am forced to add 'device_type' property in each subnode because
> > schema checker complains that this is mandatory. So I have added
> > it and schema is properly being validated:
> >
> > Before add the 'device_type' in each subnode:
> > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@0,0: 'device_type' is a required property
> > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@1,0: 'device_type' is a required property
> > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@2,0: 'device_type' is a required property
> > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
>
> Each port is a PCI bridge, right? If so, then 'pcie' for the node name
> and 'device_type = "pci";' are correct.
Yes it is, thanks for clarification.
>
> >
> > After adding it:
> > CHKDT Documentation/devicetree/bindings/processed-schema-examples.json
>
> Validates all the schema
>
> > SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json
>
> Preprocesses all the schema
>
> > DTEX Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dts
>
> Extracts the example to dts file
>
> > DTC Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
>
> Converts the example to yaml
>
> > CHECK Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
>
> Runs the checks.
>
> >
> > Looks a bit redundant and maybe I am doing something wrong...
I meant redundant the 'device_type=pci' in all of the child nodes, not
the messages I got when check against the schema but thanks also for
explanation :).
> >
> > Thanks in advance for clarification.
> >
> > Best regards,
> > Sergio Paracuellos
> >
> >
> > .../bindings/pci/mediatek,mt7621-pci.yaml | 144 ++++++++++++++++++
> > .../mt7621-pci/mediatek,mt7621-pci.txt | 104 -------------
> > 2 files changed, 144 insertions(+), 104 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
> > delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
> >
> > 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..9c1d05d929a2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.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/mediatek,mt7621-pci.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek MT7621 PCIe controller
> > +
> > +maintainers:
> > + - Sergio Paracuellos <[email protected]>
> > +
> > +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
>
> Are these config space registers or MT7621 specific?
All of them are MT7621 specific.
>
> > +
> > + ranges:
> > + maxItems: 2
> > +
> > + interrupts:
> > + maxItems: 3
>
> What are the 3 interrupts?
These are one interrupt per root port. In next version this will
change in favour of using interrupt-map and interrupt-map-mask instead
of use interrupts and a custom 'map_irq' callback in driver code.
Please see:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=aed0b711cc791d075e716c397ff6b26bf50345a6
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=3e278e3064511b1606d406db0e26b2fee593fb55
This is the way used in mt7623 already mainlined binding.
> > +
> > + resets:
> > + items:
> > + - description: pcie port 0 reset.
> > + - description: pcie port 1 reset.
> > + - description: pcie port 2 reset.
>
> This and clocks should perhaps be in each child node.
I followed here style in mt7623 already mainlined bindings which are
in the main node. Is there a strong reason to be changed into child
nodes or can I maintain this as it is?
>
> > +
> > + 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
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - ranges
> > + - interrupts
> > + - 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";
> > + bus-range = <0 255>;
>
> That's the default, so not needed.
Ok, will drop this.
>
> > + ranges = <0x02000000 0 0x00000000 0x60000000 0 0x10000000>, /* pci memory */
> > + <0x01000000 0 0x00000000 0x1e160000 0 0x00010000>; /* io space */
> > + interrupt-parent = <&gic>;
> > + interrupts = <GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>,
> > + <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>,
> > + <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";
> > + ranges;
> > + bus-range = <0x00 0xff>;
>
> Besides being the default, I don't think this makes sense here as it
> belongs in the parent unless you had some subset of bus numbers allowed
> by the parent.
That's not the case so I will drop bus-ranges property from all the
child nodes also.
>
> > + };
> > +
> > + pcie@1,0 {
> > + reg = <0x0800 0 0 0 0>;
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> > + device_type = "pci";
> > + ranges;
> > + bus-range = <0x00 0xff>;
> > + };
> > +
> > + pcie@2,0 {
> > + reg = <0x1000 0 0 0 0>;
> > + #address-cells = <3>;
> > + #size-cells = <2>;
> > + device_type = "pci";
> > + ranges;
> > + bus-range = <0x00 0xff>;
> > + };
> > + };
> > +...
Best regards,
Sergio Paracuellos
On Thu, May 6, 2021 at 11:41 AM Sergio Paracuellos
<[email protected]> wrote:
>
> Hi Rob,
>
> Thanks for the review.
>
> On Thu, May 6, 2021 at 5:18 PM Rob Herring <[email protected]> wrote:
> >
> > On Sat, May 01, 2021 at 03:36:46PM +0200, Sergio Paracuellos wrote:
> > > Add device tree binding documentation for PCIe in MT7621 SoCs.
> > >
> > > Signed-off-by: Sergio Paracuellos <[email protected]>
> > > ---
> > >
> > > Hi Rob,
> > >
> > > Some concerns here. I was not be able to found any case similar to
> > > this binding where sub-nodes describing each pcie port interface
> > > are needed. I added them to the 'examples' directly without saying
> > > anything about properties in any other place since its properties
> > > seems to be covered in 'pci-bus.yaml' schema definition. I don't
> > > know if this is the way, I have checked against schema and I noticed
> > > I am forced to add 'device_type' property in each subnode because
> > > schema checker complains that this is mandatory. So I have added
> > > it and schema is properly being validated:
> > >
> > > Before add the 'device_type' in each subnode:
> > > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@0,0: 'device_type' is a required property
> > > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> > > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@1,0: 'device_type' is a required property
> > > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> > > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@2,0: 'device_type' is a required property
> > > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> >
> > Each port is a PCI bridge, right? If so, then 'pcie' for the node name
> > and 'device_type = "pci";' are correct.
>
> Yes it is, thanks for clarification.
>
> >
> > >
> > > After adding it:
> > > CHKDT Documentation/devicetree/bindings/processed-schema-examples.json
> >
> > Validates all the schema
> >
> > > SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json
> >
> > Preprocesses all the schema
> >
> > > DTEX Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dts
> >
> > Extracts the example to dts file
> >
> > > DTC Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
> >
> > Converts the example to yaml
> >
> > > CHECK Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
> >
> > Runs the checks.
> >
> > >
> > > Looks a bit redundant and maybe I am doing something wrong...
>
> I meant redundant the 'device_type=pci' in all of the child nodes, not
> the messages I got when check against the schema but thanks also for
> explanation :).
>
> > >
> > > Thanks in advance for clarification.
> > >
> > > Best regards,
> > > Sergio Paracuellos
> > >
> > >
> > > .../bindings/pci/mediatek,mt7621-pci.yaml | 144 ++++++++++++++++++
> > > .../mt7621-pci/mediatek,mt7621-pci.txt | 104 -------------
> > > 2 files changed, 144 insertions(+), 104 deletions(-)
> > > create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
> > > delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
> > >
> > > 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..9c1d05d929a2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.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/mediatek,mt7621-pci.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: MediaTek MT7621 PCIe controller
> > > +
> > > +maintainers:
> > > + - Sergio Paracuellos <[email protected]>
> > > +
> > > +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
> >
> > Are these config space registers or MT7621 specific?
>
> All of them are MT7621 specific.
>
> >
> > > +
> > > + ranges:
> > > + maxItems: 2
> > > +
> > > + interrupts:
> > > + maxItems: 3
> >
> > What are the 3 interrupts?
>
> These are one interrupt per root port. In next version this will
> change in favour of using interrupt-map and interrupt-map-mask instead
> of use interrupts and a custom 'map_irq' callback in driver code.
> Please see:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=aed0b711cc791d075e716c397ff6b26bf50345a6
> https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=3e278e3064511b1606d406db0e26b2fee593fb55
>
> This is the way used in mt7623 already mainlined binding.
>
> > > +
> > > + resets:
> > > + items:
> > > + - description: pcie port 0 reset.
> > > + - description: pcie port 1 reset.
> > > + - description: pcie port 2 reset.
> >
> > This and clocks should perhaps be in each child node.
>
> I followed here style in mt7623 already mainlined bindings which are
> in the main node. Is there a strong reason to be changed into child
> nodes or can I maintain this as it is?
Okay, I had no idea because you didn't mention it. Why are you
creating a new binding then? Looks like they are pretty similar. At
least don't invent new *-names.
However, you should be aware of this pending change:
https://lore.kernel.org/linux-pci/[email protected]/
So perhaps mt7621 should follow that?
Rob
Hi Rob,
On Fri, May 7, 2021 at 10:38 PM Rob Herring <[email protected]> wrote:
>
> On Thu, May 6, 2021 at 11:41 AM Sergio Paracuellos
> <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > Thanks for the review.
> >
> > On Thu, May 6, 2021 at 5:18 PM Rob Herring <[email protected]> wrote:
> > >
> > > On Sat, May 01, 2021 at 03:36:46PM +0200, Sergio Paracuellos wrote:
> > > > Add device tree binding documentation for PCIe in MT7621 SoCs.
> > > >
> > > > Signed-off-by: Sergio Paracuellos <[email protected]>
> > > > ---
> > > >
> > > > Hi Rob,
> > > >
> > > > Some concerns here. I was not be able to found any case similar to
> > > > this binding where sub-nodes describing each pcie port interface
> > > > are needed. I added them to the 'examples' directly without saying
> > > > anything about properties in any other place since its properties
> > > > seems to be covered in 'pci-bus.yaml' schema definition. I don't
> > > > know if this is the way, I have checked against schema and I noticed
> > > > I am forced to add 'device_type' property in each subnode because
> > > > schema checker complains that this is mandatory. So I have added
> > > > it and schema is properly being validated:
> > > >
> > > > Before add the 'device_type' in each subnode:
> > > > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@0,0: 'device_type' is a required property
> > > > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> > > > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@1,0: 'device_type' is a required property
> > > > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> > > > /home/sergio/staging/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml: pcie@2,0: 'device_type' is a required property
> > > > >From schema: /home/sergio/.local/lib/python3.9/site-packages/dtschema/schemas/pci/pci-bus.yaml
> > >
> > > Each port is a PCI bridge, right? If so, then 'pcie' for the node name
> > > and 'device_type = "pci";' are correct.
> >
> > Yes it is, thanks for clarification.
> >
> > >
> > > >
> > > > After adding it:
> > > > CHKDT Documentation/devicetree/bindings/processed-schema-examples.json
> > >
> > > Validates all the schema
> > >
> > > > SCHEMA Documentation/devicetree/bindings/processed-schema-examples.json
> > >
> > > Preprocesses all the schema
> > >
> > > > DTEX Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dts
> > >
> > > Extracts the example to dts file
> > >
> > > > DTC Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
> > >
> > > Converts the example to yaml
> > >
> > > > CHECK Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.example.dt.yaml
> > >
> > > Runs the checks.
> > >
> > > >
> > > > Looks a bit redundant and maybe I am doing something wrong...
> >
> > I meant redundant the 'device_type=pci' in all of the child nodes, not
> > the messages I got when check against the schema but thanks also for
> > explanation :).
> >
> > > >
> > > > Thanks in advance for clarification.
> > > >
> > > > Best regards,
> > > > Sergio Paracuellos
> > > >
> > > >
> > > > .../bindings/pci/mediatek,mt7621-pci.yaml | 144 ++++++++++++++++++
> > > > .../mt7621-pci/mediatek,mt7621-pci.txt | 104 -------------
> > > > 2 files changed, 144 insertions(+), 104 deletions(-)
> > > > create mode 100644 Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.yaml
> > > > delete mode 100644 drivers/staging/mt7621-pci/mediatek,mt7621-pci.txt
> > > >
> > > > 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..9c1d05d929a2
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/pci/mediatek,mt7621-pci.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/mediatek,mt7621-pci.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: MediaTek MT7621 PCIe controller
> > > > +
> > > > +maintainers:
> > > > + - Sergio Paracuellos <[email protected]>
> > > > +
> > > > +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
> > >
> > > Are these config space registers or MT7621 specific?
> >
> > All of them are MT7621 specific.
> >
> > >
> > > > +
> > > > + ranges:
> > > > + maxItems: 2
> > > > +
> > > > + interrupts:
> > > > + maxItems: 3
> > >
> > > What are the 3 interrupts?
> >
> > These are one interrupt per root port. In next version this will
> > change in favour of using interrupt-map and interrupt-map-mask instead
> > of use interrupts and a custom 'map_irq' callback in driver code.
> > Please see:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=aed0b711cc791d075e716c397ff6b26bf50345a6
> > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h=staging-testing&id=3e278e3064511b1606d406db0e26b2fee593fb55
> >
> > This is the way used in mt7623 already mainlined binding.
> >
> > > > +
> > > > + resets:
> > > > + items:
> > > > + - description: pcie port 0 reset.
> > > > + - description: pcie port 1 reset.
> > > > + - description: pcie port 2 reset.
> > >
> > > This and clocks should perhaps be in each child node.
> >
> > I followed here style in mt7623 already mainlined bindings which are
> > in the main node. Is there a strong reason to be changed into child
> > nodes or can I maintain this as it is?
>
> Okay, I had no idea because you didn't mention it. Why are you
> creating a new binding then? Looks like they are pretty similar. At
> least don't invent new *-names.
Old ramips based SoCs like mt7621 are not really maintained by
mediatek. They are focused in arm-based stuff (like mentioned mt7623
SoC). Ramips drivers and so on normally comes from openwrt community
based on SDK driver code. Until now (near to three years?) no comments
from mediatek have come from any of my changes and new drivers for
this SoC (like the clock one I recently be able to mainline) I am
trying to maintain. That is why if there is not a real problem I do
think is better this to have its own binding.
What do you mean with "don't invent new *-names" part?
>
> However, you should be aware of this pending change:
>
> https://lore.kernel.org/linux-pci/[email protected]/
>
> So perhaps mt7621 should follow that?
Thanks for the link. Because there is no documentation at all for the
Pcie in MT7621 SoCs, and old drivers I have been cleaning and
maintaining from a while now had nothing MSI related I think I cannot
suppose anything and don't really know if mt7621 should follow that.
Both Mt7622 and Mt7612 SocS seem to be arm64 based architectures. All
the ramips stuff and this kind of SoCs are not really maintained,
AFAICT, but are supported because of openwrt people use them a lot.
>
> Rob
Thanks for the review and comments.
Best regards,
Sergio Paracuellos