| Message ID | 20240420145342.118643-1-matthew.gerlach@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] dt-bindings: PCI: altera: Convert to YAML | expand |
On Sat, Apr 20, 2024 at 09:53:42AM -0500, matthew.gerlach@linux.intel.com wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > Convert the device tree bindings for the Altera Root Port PCIe controller > from text to YAML. > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > --- > v4: > - reorder reg-names to match original binding > - move reg and reg-names to top level with limits. > > v3: > - Added years to copyright > - Correct order in file of allOf and unevaluatedProperties > - remove items: in compatible field > - fix reg and reg-names constraints > - replace deprecated pci-bus.yaml with pci-host-bridge.yaml > - fix entries in ranges property > - remove device_type from required > > v2: > - Move allOf: to bottom of file, just like example-schema is showing > - add constraint for reg and reg-names > - remove unneeded device_type > - drop #address-cells and #size-cells > - change minItems to maxItems for interrupts: > - change msi-parent to just "msi-parent: true" > - cleaned up required: > - make subject consistent with other commits coverting to YAML > - s/overt/onvert/g > --- > .../devicetree/bindings/pci/altera-pcie.txt | 50 ----------- > .../bindings/pci/altr,pcie-root-port.yaml | 88 +++++++++++++++++++ > 2 files changed, 88 insertions(+), 50 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/pci/altera-pcie.txt > create mode 100644 Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml > > diff --git a/Documentation/devicetree/bindings/pci/altera-pcie.txt b/Documentation/devicetree/bindings/pci/altera-pcie.txt > deleted file mode 100644 > index 816b244a221e..000000000000 > --- a/Documentation/devicetree/bindings/pci/altera-pcie.txt > +++ /dev/null > @@ -1,50 +0,0 @@ > -* Altera PCIe controller > - > -Required properties: > -- compatible : should contain "altr,pcie-root-port-1.0" or "altr,pcie-root-port-2.0" > -- reg: a list of physical base address and length for TXS and CRA. > - For "altr,pcie-root-port-2.0", additional HIP base address and length. > -- reg-names: must include the following entries: > - "Txs": TX slave port region > - "Cra": Control register access region > - "Hip": Hard IP region (if "altr,pcie-root-port-2.0") > -- interrupts: specifies the interrupt source of the parent interrupt > - controller. The format of the interrupt specifier depends > - on the parent interrupt controller. > -- device_type: must be "pci" > -- #address-cells: set to <3> > -- #size-cells: set to <2> > -- #interrupt-cells: set to <1> > -- ranges: describes the translation of addresses for root ports and > - standard PCI regions. > -- interrupt-map-mask and interrupt-map: standard PCI properties to define the > - mapping of the PCIe interface to interrupt numbers. > - > -Optional properties: > -- msi-parent: Link to the hardware entity that serves as the MSI controller > - for this PCIe controller. > -- bus-range: PCI bus numbers covered > - > -Example > - pcie_0: pcie@c00000000 { > - compatible = "altr,pcie-root-port-1.0"; > - reg = <0xc0000000 0x20000000>, > - <0xff220000 0x00004000>; > - reg-names = "Txs", "Cra"; > - interrupt-parent = <&hps_0_arm_gic_0>; > - interrupts = <0 40 4>; > - interrupt-controller; What happened to this? It is clearly needed since the interrupt-map below points back to this node. Note that that didn't work at one point in time, but I think we fixed it. It doesn't seem you are testing the binding against an actual DT. Please do that. Rob > - #interrupt-cells = <1>; > - bus-range = <0x0 0xFF>; > - device_type = "pci"; > - msi-parent = <&msi_to_gic_gen_0>; > - #address-cells = <3>; > - #size-cells = <2>; > - interrupt-map-mask = <0 0 0 7>; > - interrupt-map = <0 0 0 1 &pcie_0 1>, > - <0 0 0 2 &pcie_0 2>, > - <0 0 0 3 &pcie_0 3>, > - <0 0 0 4 &pcie_0 4>; > - ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000 > - 0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>; > - };
On Mon, 22 Apr 2024, Rob Herring wrote: > On Sat, Apr 20, 2024 at 09:53:42AM -0500, matthew.gerlach@linux.intel.com wrote: >> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> >> Convert the device tree bindings for the Altera Root Port PCIe controller >> from text to YAML. >> >> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >> --- >> v4: >> - reorder reg-names to match original binding >> - move reg and reg-names to top level with limits. >> >> v3: >> - Added years to copyright >> - Correct order in file of allOf and unevaluatedProperties >> - remove items: in compatible field >> - fix reg and reg-names constraints >> - replace deprecated pci-bus.yaml with pci-host-bridge.yaml >> - fix entries in ranges property >> - remove device_type from required >> >> v2: >> - Move allOf: to bottom of file, just like example-schema is showing >> - add constraint for reg and reg-names >> - remove unneeded device_type >> - drop #address-cells and #size-cells >> - change minItems to maxItems for interrupts: >> - change msi-parent to just "msi-parent: true" >> - cleaned up required: >> - make subject consistent with other commits coverting to YAML >> - s/overt/onvert/g >> --- >> .../devicetree/bindings/pci/altera-pcie.txt | 50 ----------- >> .../bindings/pci/altr,pcie-root-port.yaml | 88 +++++++++++++++++++ >> 2 files changed, 88 insertions(+), 50 deletions(-) >> delete mode 100644 Documentation/devicetree/bindings/pci/altera-pcie.txt >> create mode 100644 Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml >> >> diff --git a/Documentation/devicetree/bindings/pci/altera-pcie.txt b/Documentation/devicetree/bindings/pci/altera-pcie.txt >> deleted file mode 100644 >> index 816b244a221e..000000000000 >> --- a/Documentation/devicetree/bindings/pci/altera-pcie.txt >> +++ /dev/null >> @@ -1,50 +0,0 @@ >> -* Altera PCIe controller >> - >> -Required properties: >> -- compatible : should contain "altr,pcie-root-port-1.0" or "altr,pcie-root-port-2.0" >> -- reg: a list of physical base address and length for TXS and CRA. >> - For "altr,pcie-root-port-2.0", additional HIP base address and length. >> -- reg-names: must include the following entries: >> - "Txs": TX slave port region >> - "Cra": Control register access region >> - "Hip": Hard IP region (if "altr,pcie-root-port-2.0") >> -- interrupts: specifies the interrupt source of the parent interrupt >> - controller. The format of the interrupt specifier depends >> - on the parent interrupt controller. >> -- device_type: must be "pci" >> -- #address-cells: set to <3> >> -- #size-cells: set to <2> >> -- #interrupt-cells: set to <1> >> -- ranges: describes the translation of addresses for root ports and >> - standard PCI regions. >> -- interrupt-map-mask and interrupt-map: standard PCI properties to define the >> - mapping of the PCIe interface to interrupt numbers. >> - >> -Optional properties: >> -- msi-parent: Link to the hardware entity that serves as the MSI controller >> - for this PCIe controller. >> -- bus-range: PCI bus numbers covered >> - >> -Example >> - pcie_0: pcie@c00000000 { >> - compatible = "altr,pcie-root-port-1.0"; >> - reg = <0xc0000000 0x20000000>, >> - <0xff220000 0x00004000>; >> - reg-names = "Txs", "Cra"; >> - interrupt-parent = <&hps_0_arm_gic_0>; >> - interrupts = <0 40 4>; >> - interrupt-controller; > > What happened to this? It is clearly needed since the interrupt-map > below points back to this node. Note that that didn't work at one point > in time, but I think we fixed it. I think the DTs I was using test were created during the point in time when this did not work. The interrupt-controller boolean and #interrupt-cells property were in a sub node, and the interrupt-map pointed to the sub-node. Keeping everything in the base node maintains compatiblity. I will fix this for v5. > > It doesn't seem you are testing the binding against an actual DT. > Please do that. I need to fix the DTs I'm using for test :) Thanks for the feedback, Matthew Gerlach > > Rob > >> - #interrupt-cells = <1>; >> - bus-range = <0x0 0xFF>; >> - device_type = "pci"; >> - msi-parent = <&msi_to_gic_gen_0>; >> - #address-cells = <3>; >> - #size-cells = <2>; >> - interrupt-map-mask = <0 0 0 7>; >> - interrupt-map = <0 0 0 1 &pcie_0 1>, >> - <0 0 0 2 &pcie_0 2>, >> - <0 0 0 3 &pcie_0 3>, >> - <0 0 0 4 &pcie_0 4>; >> - ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000 >> - 0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>; >> - }; >
On Mon, 22 Apr 2024, matthew.gerlach@linux.intel.com wrote: > > > On Mon, 22 Apr 2024, Rob Herring wrote: > >> On Sat, Apr 20, 2024 at 09:53:42AM -0500, matthew.gerlach@linux.intel.com >> wrote: >>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> >>> Convert the device tree bindings for the Altera Root Port PCIe controller >>> from text to YAML. >>> >>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> >>> --- >>> v4: >>> - reorder reg-names to match original binding >>> - move reg and reg-names to top level with limits. >>> >>> v3: >>> - Added years to copyright >>> - Correct order in file of allOf and unevaluatedProperties >>> - remove items: in compatible field >>> - fix reg and reg-names constraints >>> - replace deprecated pci-bus.yaml with pci-host-bridge.yaml >>> - fix entries in ranges property >>> - remove device_type from required >>> >>> v2: >>> - Move allOf: to bottom of file, just like example-schema is showing >>> - add constraint for reg and reg-names >>> - remove unneeded device_type >>> - drop #address-cells and #size-cells >>> - change minItems to maxItems for interrupts: >>> - change msi-parent to just "msi-parent: true" >>> - cleaned up required: >>> - make subject consistent with other commits coverting to YAML >>> - s/overt/onvert/g >>> --- >>> .../devicetree/bindings/pci/altera-pcie.txt | 50 ----------- >>> .../bindings/pci/altr,pcie-root-port.yaml | 88 +++++++++++++++++++ >>> 2 files changed, 88 insertions(+), 50 deletions(-) >>> delete mode 100644 Documentation/devicetree/bindings/pci/altera-pcie.txt >>> create mode 100644 >>> Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/pci/altera-pcie.txt >>> b/Documentation/devicetree/bindings/pci/altera-pcie.txt >>> deleted file mode 100644 >>> index 816b244a221e..000000000000 >>> --- a/Documentation/devicetree/bindings/pci/altera-pcie.txt >>> +++ /dev/null >>> @@ -1,50 +0,0 @@ >>> -* Altera PCIe controller >>> - >>> -Required properties: >>> -- compatible : should contain "altr,pcie-root-port-1.0" or >>> "altr,pcie-root-port-2.0" >>> -- reg: a list of physical base address and length for TXS >>> and CRA. >>> - For "altr,pcie-root-port-2.0", additional HIP base address >>> and length. >>> -- reg-names: must include the following entries: >>> - "Txs": TX slave port region >>> - "Cra": Control register access region >>> - "Hip": Hard IP region (if "altr,pcie-root-port-2.0") >>> -- interrupts: specifies the interrupt source of the parent >>> interrupt >>> - controller. The format of the interrupt specifier depends >>> - on the parent interrupt controller. >>> -- device_type: must be "pci" >>> -- #address-cells: set to <3> >>> -- #size-cells: set to <2> >>> -- #interrupt-cells: set to <1> >>> -- ranges: describes the translation of addresses for root ports and >>> - standard PCI regions. >>> -- interrupt-map-mask and interrupt-map: standard PCI properties to >>> define the >>> - mapping of the PCIe interface to interrupt numbers. >>> - >>> -Optional properties: >>> -- msi-parent: Link to the hardware entity that serves as the MSI >>> controller >>> - for this PCIe controller. >>> -- bus-range: PCI bus numbers covered >>> - >>> -Example >>> - pcie_0: pcie@c00000000 { >>> - compatible = "altr,pcie-root-port-1.0"; >>> - reg = <0xc0000000 0x20000000>, >>> - <0xff220000 0x00004000>; >>> - reg-names = "Txs", "Cra"; >>> - interrupt-parent = <&hps_0_arm_gic_0>; >>> - interrupts = <0 40 4>; >>> - interrupt-controller; >> >> What happened to this? It is clearly needed since the interrupt-map >> below points back to this node. Note that that didn't work at one point >> in time, but I think we fixed it. > > I think the DTs I was using test were created during the point in time when > this did not work. The interrupt-controller boolean and #interrupt-cells > property were in a sub node, and the interrupt-map pointed to the sub-node. > Keeping everything in the base node maintains compatiblity. I will fix this > for v5. When I added the interrupt-controller boolean back to the base node, pcie_0, and fixed the interrupt-map in the yaml example to point to pcie_0, I get what appears to be the warning below from the dt_binding_check below. The exit code is still 0: make dt_binding_check DT_SCHEMA_FILES=altr,pcie-root-port.yaml SCHEMA Documentation/devicetree/bindings/processed-schema.json CHKDT Documentation/devicetree/bindings LINT Documentation/devicetree/bindings DTEX Documentation/devicetree/bindings/pci/altr,pcie-root-port.example.dts DTC_CHK Documentation/devicetree/bindings/pci/altr,pcie-root-port.example.dtb /home/mgerlach/git/linux-next/Documentation/devicetree/bindings/pci/altr,pcie-root-port.example.dtb: pcie@c00000000: interrupt-map: [[0, 0, 0, 1, 2, 1, 0, 0, 0], [2, 2, 2, 0, 0, 0, 3, 2, 3], [0, 0, 0, 4, 2, 4]] is too short The warning from checking a DTS with the binding provides a little more information, but the warning puzzles me because of the 9 entry lists: make CHECK_DTBS=y DT_SCHEMA_FILES=altr,pcie-root-port.yaml intel/socfpga_agilex7f_socdk_pcie_root_port.dtb DTC_CHK arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb arch/arm64/boot/dts/intel/socfpga_agilex_pcie_root_port.dtsi:36.4-39.46: Warning (interrupt_map): /soc@0/bridge@80000000/pcie@200000000:interrupt-map: Cell 13 is not a phandle(0) /home/mgerlach/git/linux-next/arch/arm64/boot/dts/intel/socfpga_agilex7f_socdk_pcie_root_port.dtb: pcie@200000000: interrupt-map: [[0, 0, 0, 1, 21, 1, 0, 0, 0], [2, 21, 2, 0, 0, 0, 3, 21, 3], [0, 0, 0, 4, 21, 4]] is too short The resulting device tree and driver code work as expected, but the warning is concerning. I can make the warning go away by putting interrupt-controller and #interrupt-cells in a subnode and make interrupt-map point to the subnode, but this requires changing the accepted driver code. Any suggestions on how to proceed? I think changing the existing driver seems wrong. Thanks, Matthew Gerlach > >> >> It doesn't seem you are testing the binding against an actual DT. >> Please do that. > > I need to fix the DTs I'm using for test :) > > Thanks for the feedback, > > Matthew Gerlach > >> >> Rob >> >>> - #interrupt-cells = <1>; >>> - bus-range = <0x0 0xFF>; >>> - device_type = "pci"; >>> - msi-parent = <&msi_to_gic_gen_0>; >>> - #address-cells = <3>; >>> - #size-cells = <2>; >>> - interrupt-map-mask = <0 0 0 7>; >>> - interrupt-map = <0 0 0 1 &pcie_0 1>, >>> - <0 0 0 2 &pcie_0 2>, >>> - <0 0 0 3 &pcie_0 3>, >>> - <0 0 0 4 &pcie_0 4>; >>> - ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 >>> 0x00000000 0x10000000 >>> - 0x82000000 0x00000000 0x10000000 0xd0000000 >>> 0x00000000 0x10000000>; >>> - }; >> >
diff --git a/Documentation/devicetree/bindings/pci/altera-pcie.txt b/Documentation/devicetree/bindings/pci/altera-pcie.txt deleted file mode 100644 index 816b244a221e..000000000000 --- a/Documentation/devicetree/bindings/pci/altera-pcie.txt +++ /dev/null @@ -1,50 +0,0 @@ -* Altera PCIe controller - -Required properties: -- compatible : should contain "altr,pcie-root-port-1.0" or "altr,pcie-root-port-2.0" -- reg: a list of physical base address and length for TXS and CRA. - For "altr,pcie-root-port-2.0", additional HIP base address and length. -- reg-names: must include the following entries: - "Txs": TX slave port region - "Cra": Control register access region - "Hip": Hard IP region (if "altr,pcie-root-port-2.0") -- interrupts: specifies the interrupt source of the parent interrupt - controller. The format of the interrupt specifier depends - on the parent interrupt controller. -- device_type: must be "pci" -- #address-cells: set to <3> -- #size-cells: set to <2> -- #interrupt-cells: set to <1> -- ranges: describes the translation of addresses for root ports and - standard PCI regions. -- interrupt-map-mask and interrupt-map: standard PCI properties to define the - mapping of the PCIe interface to interrupt numbers. - -Optional properties: -- msi-parent: Link to the hardware entity that serves as the MSI controller - for this PCIe controller. -- bus-range: PCI bus numbers covered - -Example - pcie_0: pcie@c00000000 { - compatible = "altr,pcie-root-port-1.0"; - reg = <0xc0000000 0x20000000>, - <0xff220000 0x00004000>; - reg-names = "Txs", "Cra"; - interrupt-parent = <&hps_0_arm_gic_0>; - interrupts = <0 40 4>; - interrupt-controller; - #interrupt-cells = <1>; - bus-range = <0x0 0xFF>; - device_type = "pci"; - msi-parent = <&msi_to_gic_gen_0>; - #address-cells = <3>; - #size-cells = <2>; - interrupt-map-mask = <0 0 0 7>; - interrupt-map = <0 0 0 1 &pcie_0 1>, - <0 0 0 2 &pcie_0 2>, - <0 0 0 3 &pcie_0 3>, - <0 0 0 4 &pcie_0 4>; - ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000 - 0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>; - }; diff --git a/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml new file mode 100644 index 000000000000..5794396f0986 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml @@ -0,0 +1,88 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +# Copyright (C) 2015, 2019, 2024, Intel Corporation +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/altr,pcie-root-port.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Altera PCIe Root Port + +maintainers: + - Matthew Gerlach <matthew.gerlach@linux.intel.com> + +properties: + compatible: + enum: + - altr,pcie-root-port-1.0 + - altr,pcie-root-port-2.0 + + reg: + items: + - description: TX slave port region + - description: Control register access region + - description: Hard IP region + minItems: 2 + + reg-names: + items: + - const: Txs + - const: Cra + - const: Hip + minItems: 2 + + interrupts: + maxItems: 1 + + interrupt-map-mask: + items: + - const: 0 + - const: 0 + - const: 0 + - const: 7 + + interrupt-map: + maxItems: 4 + + "#interrupt-cells": + const: 1 + + msi-parent: true + +required: + - compatible + - reg + - reg-names + - interrupts + - interrupt-map + - interrupt-map-mask + +allOf: + - $ref: /schemas/pci/pci-host-bridge.yaml# + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + pcie_0: pcie@c00000000 { + compatible = "altr,pcie-root-port-1.0"; + reg = <0xc0000000 0x20000000>, + <0xff220000 0x00004000>; + reg-names = "Txs", "Cra"; + interrupt-parent = <&hps_0_arm_gic_0>; + interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>; + #interrupt-cells = <1>; + bus-range = <0x0 0xff>; + device_type = "pci"; + msi-parent = <&msi_to_gic_gen_0>; + #address-cells = <3>; + #size-cells = <2>; + interrupt-map-mask = <0 0 0 7>; + interrupt-map = <0 0 0 1 &pcie_intc 1>, + <0 0 0 2 &pcie_intc 2>, + <0 0 0 3 &pcie_intc 3>, + <0 0 0 4 &pcie_intc 4>; + ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000>, + <0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>; + };