diff mbox series

[v5] dt-bindings: PCI: altera: Convert to YAML

Message ID 20240513205913.313592-1-matthew.gerlach@linux.intel.com
State New
Headers show
Series [v5] dt-bindings: PCI: altera: Convert to YAML | expand

Commit Message

matthew.gerlach@linux.intel.com May 13, 2024, 8:59 p.m. UTC
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>
---
v5:
 - add interrupt-conntroller #interrupt-cells to required field
 - don't touch original example dts

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     | 93 +++++++++++++++++++
 2 files changed, 93 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

Comments

Rob Herring May 13, 2024, 10:12 p.m. UTC | #1
On Mon, 13 May 2024 15:59:13 -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>
> ---
> v5:
>  - add interrupt-conntroller #interrupt-cells to required field
>  - don't touch original example dts
> 
> 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     | 93 +++++++++++++++++++
>  2 files changed, 93 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
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/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
	from schema $id: http://devicetree.org/schemas/altr,pcie-root-port.yaml#

doc reference errors (make refcheckdocs):
Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/pci/altera-pcie.txt
MAINTAINERS: Documentation/devicetree/bindings/pci/altera-pcie.txt

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240513205913.313592-1-matthew.gerlach@linux.intel.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
kernel test robot May 13, 2024, 11:40 p.m. UTC | #2
Hi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus linus/master v6.9 next-20240513]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/matthew-gerlach-linux-intel-com/dt-bindings-PCI-altera-Convert-to-YAML/20240514-050049
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20240513205913.313592-1-matthew.gerlach%40linux.intel.com
patch subject: [PATCH v5] dt-bindings: PCI: altera: Convert to YAML
reproduce: (https://download.01.org/0day-ci/archive/20240514/202405140709.5BmKZtT7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405140709.5BmKZtT7-lkp@intel.com/

All warnings (new ones prefixed by >>):

   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/devicetree/bindings/sound/fsl-asoc-card.txt references a file that doesn't exist: Documentation/devicetree/bindings/sound/fsl,asrc.txt
   Warning: Documentation/gpu/amdgpu/display/display-contributing.rst references a file that doesn't exist: Documentation/GPU/amdgpu/display/mpo-overview.rst
   Warning: Documentation/userspace-api/netlink/index.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
   Warning: Documentation/userspace-api/netlink/specs.rst references a file that doesn't exist: Documentation/networking/netlink_spec/index.rst
>> Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/pci/altera-pcie.txt
   Using alabaster theme
Rob Herring May 14, 2024, 1:17 p.m. UTC | #3
On Mon, May 13, 2024 at 05:12:42PM -0500, Rob Herring (Arm) wrote:
> 
> On Mon, 13 May 2024 15:59:13 -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>
> > ---
> > v5:
> >  - add interrupt-conntroller #interrupt-cells to required field
> >  - don't touch original example dts
> > 
> > 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     | 93 +++++++++++++++++++
> >  2 files changed, 93 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
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/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
> 	from schema $id: http://devicetree.org/schemas/altr,pcie-root-port.yaml#

You need 3 address cells after the phandles since the interrupt parent 
has 3 address cells. 

What does your actual DT contain and do interrupts work because 
interrupts never would have worked I think? Making the PCI host the 
interrupt parent didn't even work in the kernel until somewhat recently 
(maybe a few years now). That's why a bunch of PCI hosts have an 
interrupt-controller child node.

Rob
matthew.gerlach@linux.intel.com May 14, 2024, 6:30 p.m. UTC | #4
On Tue, 14 May 2024, Rob Herring wrote:

>>>
>>
>> My bot found errors running 'make dt_binding_check' on your patch:
>>
>> yamllint warnings/errors:
>>
>> dtschema/dtc warnings/errors:
>> /builds/robherring/dt-review-ci/linux/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
>> 	from schema $id: http://devicetree.org/schemas/altr,pcie-root-port.yaml#
>
> You need 3 address cells after the phandles since the interrupt parent
> has 3 address cells.

Thanks for the extra explanation. Adding 3 address cells of 0 made the 
warning go away.

>
> What does your actual DT contain and do interrupts work because
> interrupts never would have worked I think? Making the PCI host the
> interrupt parent didn't even work in the kernel until somewhat recently
> (maybe a few years now). That's why a bunch of PCI hosts have an
> interrupt-controller child node.

The following DT snippet comes from 
https://www.rocketboards.org/foswiki/Projects/Stratix10PCIeRootPortWithMSI

The Linux kernel version is 4.14.130-ltsi. Would the use of the msi-parent 
node make everything work?

pcie_1_pcie_a10_hip_avmm: pcie@0x010000000 {
 	compatible = "altr,pcie-root-port-1.0";
 	reg = <0xd0000000 0x10000000>,
 	      <0xff210000 0x00004000>;
 	reg-names = "Txs", "Cra";
 	interrupt-parent = <&arria10_hps_0_arm_gic_0>;
 	interrupts = <0 24 4>;
 	interrupt-controller;
 	#interrupt-cells = <1>;
 	device_type = "pci";
 	msi-parent = <&pcie_0_msi_to_gic_gen_0>;
 	bus-range = <0x00000000 0x000000ff>;
 	#address-cells = <3>;
 	#size-cells = <2>;
 	ranges = <0x82000000 0x00000000 0x00000000 0xd0000000 0x00000000 
0x10000000>;
 	interrupt-map-mask = <0 0 0 7>;
 	interrupt-map = <0 0 0 1 &pcie_0_pcie_a10_hip_avmm 1>,
 			<0 0 0 2 &pcie_0_pcie_a10_hip_avmm 2>,
 			<0 0 0 3 &pcie_0_pcie_a10_hip_avmm 3>,
 			<0 0 0 4 &pcie_0_pcie_a10_hip_avmm 4>;
};
pcie_0_msi_to_gic_gen_0: msi@0x100014080 {
 	compatible = "altr,msi-1.0", "altr,msi-1.0";
 	reg = <0x00000001 0x00014080 0x00000010>,
 	      <0x00000001 0x00014000 0x00000080>;
 	reg-names = "csr", "vector_slave";
 	interrupt-parent = <&arria10_hps_0_arm_gic_0>;
 	interrupts = <0 22 4>;
 	clocks = <&pcie_0_clk_100>;
 	msi-controller = <1>;
 	num-vectors = <32>;
};

I am doing something similar with newer HIP with the 6.1.68-lts, and it 
seems to work. I had experimented with an interrupt-controller child 
node, but I wanted to maintain the existing binding definition and 
compatibility with the accepted driver code.

Matthew Gerlach



>
> Rob
>
Rob Herring May 17, 2024, 7:56 p.m. UTC | #5
On Tue, May 14, 2024 at 11:30:05AM -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 14 May 2024, Rob Herring wrote:
> 
> > > > 
> > > 
> > > My bot found errors running 'make dt_binding_check' on your patch:
> > > 
> > > yamllint warnings/errors:
> > > 
> > > dtschema/dtc warnings/errors:
> > > /builds/robherring/dt-review-ci/linux/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
> > > 	from schema $id: http://devicetree.org/schemas/altr,pcie-root-port.yaml#
> > 
> > You need 3 address cells after the phandles since the interrupt parent
> > has 3 address cells.
> 
> Thanks for the extra explanation. Adding 3 address cells of 0 made the
> warning go away.
> 
> > 
> > What does your actual DT contain and do interrupts work because
> > interrupts never would have worked I think? Making the PCI host the
> > interrupt parent didn't even work in the kernel until somewhat recently
> > (maybe a few years now). That's why a bunch of PCI hosts have an
> > interrupt-controller child node.
> 
> The following DT snippet comes from
> https://www.rocketboards.org/foswiki/Projects/Stratix10PCIeRootPortWithMSI
> 
> The Linux kernel version is 4.14.130-ltsi. Would the use of the msi-parent
> node make everything work?

Possibly? I would think MSIs are preferred and almost anything should 
support MSIs now.

Rob
diff mbox series

Patch

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..7f02e7fb33e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
@@ -0,0 +1,93 @@ 
+# 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-controller: true
+
+  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-cells"
+  - interrupt-controller
+  - 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-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>;
+    };