[PATCH v6 1/2] dt-bindings: PCI: altera: Convert to YAML

[PATCH v6 1/2] dt-bindings: PCI: altera: Convert to YAML

[matthew.gerlach]

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>
---
v6:
 - Fix dt_binding_check warnings by creating interrupt-controller subnode
   and fixing interrupt-map.
 - Updated filename in MAINTAINERS.

v5:
 - add interrupt-controller #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     | 114 ++++++++++++++++++
 MAINTAINERS                                   |   2 +-
 3 files changed, 115 insertions(+), 51 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;
-		#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..08ee04f6e004
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
@@ -0,0 +1,114 @@
+# 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
+
+  interrupt-controller:
+    description: identifies the node as an interrupt controller
+    type: object
+    properties:
+      interrupt-controller: true
+
+      "#address-cells":
+        const: 0
+
+      "#interrupt-cells":
+        const: 1
+
+    required:
+      - interrupt-controller
+      - "#address-cells"
+      - "#interrupt-cells"
+
+    additionalProperties: false
+
+  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-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>;
+        pcie_intc: interrupt-controller {
+            interrupt-controller;
+            #address-cells = <0>;
+            #interrupt-cells = <1>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 670b8201973b..c8120cb9d340 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17142,7 +17142,7 @@ PCI DRIVER FOR ALTERA PCIE IP
 M:	Joyce Ooi <joyce.ooi@intel.com>
 L:	linux-pci@vger.kernel.org
 S:	Supported
-F:	Documentation/devicetree/bindings/pci/altera-pcie.txt
+F:	Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
 F:	drivers/pci/controller/pcie-altera.c
 
 PCI DRIVER FOR APPLIEDMICRO XGENE
-- 
2.34.1

[PATCH v6 2/2] PCI: altera: support dt binding update

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

Add support for the device tree binding update. As part of
converting the binding document from text to yaml, with schema
validation, a device tree subnode was added to properly map
legacy interrupts. Maintain backward compatibility with previous binding.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
 drivers/pci/controller/pcie-altera.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index a9536dc4bf96..88511fa2f078 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -667,11 +667,20 @@ static void altera_pcie_isr(struct irq_desc *desc)
 static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
-	struct device_node *node = dev->of_node;
+	struct device_node *node, *child;
 
 	/* Setup INTx */
+	child = of_get_next_child(dev->of_node, NULL);
+	if (child)
+		node = child;
+	else
+		node = dev->of_node;
+
 	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
-					&intx_domain_ops, pcie);
+						 &intx_domain_ops, pcie);
+	if (child)
+		of_node_put(child);
+
 	if (!pcie->irq_domain) {
 		dev_err(dev, "Failed to get a INTx IRQ domain\n");
 		return -ENOMEM;
-- 
2.34.1

Re: [PATCH v6 2/2] PCI: altera: support dt binding update

[Bjorn Helgaas]

On Tue, Jun 11, 2024 at 11:35:25AM -0500, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add support for the device tree binding update. As part of
> converting the binding document from text to yaml, with schema
> validation, a device tree subnode was added to properly map
> legacy interrupts. Maintain backward compatibility with previous binding.

If something was *added* to the binding, I think it would be helpful
to split that into two patches: (1) convert to YAML with zero
functional changes, (2) add the new stuff.  Adding something at the
same time as changing the format makes it hard to review.

Then we could have a more specific subject and commit log for *this*
patch.

> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> ---
>  drivers/pci/controller/pcie-altera.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
> index a9536dc4bf96..88511fa2f078 100644
> --- a/drivers/pci/controller/pcie-altera.c
> +++ b/drivers/pci/controller/pcie-altera.c
> @@ -667,11 +667,20 @@ static void altera_pcie_isr(struct irq_desc *desc)
>  static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
>  {
>  	struct device *dev = &pcie->pdev->dev;
> -	struct device_node *node = dev->of_node;
> +	struct device_node *node, *child;
>  
>  	/* Setup INTx */
> +	child = of_get_next_child(dev->of_node, NULL);
> +	if (child)
> +		node = child;
> +	else
> +		node = dev->of_node;
> +
>  	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
> -					&intx_domain_ops, pcie);
> +						 &intx_domain_ops, pcie);
> +	if (child)
> +		of_node_put(child);
> +
>  	if (!pcie->irq_domain) {
>  		dev_err(dev, "Failed to get a INTx IRQ domain\n");
>  		return -ENOMEM;
> -- 
> 2.34.1
> 

Re: [PATCH v6 2/2] PCI: altera: support dt binding update

[matthew.gerlach]

On Tue, 11 Jun 2024, Bjorn Helgaas wrote:

> On Tue, Jun 11, 2024 at 11:35:25AM -0500, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add support for the device tree binding update. As part of
>> converting the binding document from text to yaml, with schema
>> validation, a device tree subnode was added to properly map
>> legacy interrupts. Maintain backward compatibility with previous binding.
>
> If something was *added* to the binding, I think it would be helpful
> to split that into two patches: (1) convert to YAML with zero
> functional changes, (2) add the new stuff.  Adding something at the
> same time as changing the format makes it hard to review.

Thanks for feedback. It was during the conversion to YAML that a problem 
with the original binding was discovered. As Rob Herring pointed out in
https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240513205913.313592-1-matthew.gerlach@linux.intel.com/

"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."

This was an attempt to fix the problem. I can resubmit a conversion to YAML 
with zero functional changes.

Matthew Gerlach


>
> Then we could have a more specific subject and commit log for *this*
> patch.
>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>>  drivers/pci/controller/pcie-altera.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
>> index a9536dc4bf96..88511fa2f078 100644
>> --- a/drivers/pci/controller/pcie-altera.c
>> +++ b/drivers/pci/controller/pcie-altera.c
>> @@ -667,11 +667,20 @@ static void altera_pcie_isr(struct irq_desc *desc)
>>  static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
>>  {
>>  	struct device *dev = &pcie->pdev->dev;
>> -	struct device_node *node = dev->of_node;
>> +	struct device_node *node, *child;
>>
>>  	/* Setup INTx */
>> +	child = of_get_next_child(dev->of_node, NULL);
>> +	if (child)
>> +		node = child;
>> +	else
>> +		node = dev->of_node;
>> +
>>  	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
>> -					&intx_domain_ops, pcie);
>> +						 &intx_domain_ops, pcie);
>> +	if (child)
>> +		of_node_put(child);
>> +
>>  	if (!pcie->irq_domain) {
>>  		dev_err(dev, "Failed to get a INTx IRQ domain\n");
>>  		return -ENOMEM;
>> --
>> 2.34.1
>>
>

Re: [PATCH v6 2/2] PCI: altera: support dt binding update

[Rob Herring]

On Wed, Jun 12, 2024 at 08:12:05AM -0700, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Tue, 11 Jun 2024, Bjorn Helgaas wrote:
> 
> > On Tue, Jun 11, 2024 at 11:35:25AM -0500, matthew.gerlach@linux.intel.com wrote:
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > Add support for the device tree binding update. As part of
> > > converting the binding document from text to yaml, with schema
> > > validation, a device tree subnode was added to properly map
> > > legacy interrupts. Maintain backward compatibility with previous binding.
> > 
> > If something was *added* to the binding, I think it would be helpful
> > to split that into two patches: (1) convert to YAML with zero
> > functional changes, (2) add the new stuff.  Adding something at the
> > same time as changing the format makes it hard to review.

The policy for conversions is changes to match reality are fine, just 
need to be noted in the commit message. That generally implies no driver 
or dts changes which is not the case here. 

> Thanks for feedback. It was during the conversion to YAML that a problem
> with the original binding was discovered. As Rob Herring pointed out in
> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240513205913.313592-1-matthew.gerlach@linux.intel.com/
> 
> "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."
> 
> This was an attempt to fix the problem. I can resubmit a conversion to YAML
> with zero functional changes.

I wasn't suggesting you fix it. Just something I noticed looking at 
the other issue. If no one noticed or cared, why bother? It should work 
fine for recent kernels.

Rob

Re: [PATCH v6 2/2] PCI: altera: support dt binding update

[matthew.gerlach]

On Thu, 13 Jun 2024, Rob Herring wrote:

> On Wed, Jun 12, 2024 at 08:12:05AM -0700, matthew.gerlach@linux.intel.com wrote:
>>
>>
>> On Tue, 11 Jun 2024, Bjorn Helgaas wrote:
>>
>>> On Tue, Jun 11, 2024 at 11:35:25AM -0500, matthew.gerlach@linux.intel.com wrote:
>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>
>>>> Add support for the device tree binding update. As part of
>>>> converting the binding document from text to yaml, with schema
>>>> validation, a device tree subnode was added to properly map
>>>> legacy interrupts. Maintain backward compatibility with previous binding.
>>>
>>> If something was *added* to the binding, I think it would be helpful
>>> to split that into two patches: (1) convert to YAML with zero
>>> functional changes, (2) add the new stuff.  Adding something at the
>>> same time as changing the format makes it hard to review.
>
> The policy for conversions is changes to match reality are fine, just
> need to be noted in the commit message. That generally implies no driver
> or dts changes which is not the case here.
>
>> Thanks for feedback. It was during the conversion to YAML that a problem
>> with the original binding was discovered. As Rob Herring pointed out in
>> https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240513205913.313592-1-matthew.gerlach@linux.intel.com/
>>
>> "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."
>>
>> This was an attempt to fix the problem. I can resubmit a conversion to YAML
>> with zero functional changes.
>
> I wasn't suggesting you fix it. Just something I noticed looking at
> the other issue. If no one noticed or cared, why bother? It should work
> fine for recent kernels.

Thanks for the feedback. I can resubmit a single conversion commit that 
passes the schema check by adding 3 address fields as you suggested. I 
will also mention this slight modification in the commit message.

Matthew

>
> Rob
>