Re: [PATCH v2] dt-bindings: PCI: altera: Convert to YAML

From: matthew . gerlach
Date: Mon Apr 08 2024 - 16:34:46 EST



On Sun, 7 Apr 2024, Krzysztof Kozlowski wrote:

On 05/04/2024 16:53, matthew.gerlach@xxxxxxxxxxxxxxx wrote:
From: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>

Convert the device tree bindings for the Altera Root Port PCIe controller
from text to YAML.

Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
---
v2:
- Move allOf: to bottom of file, just like example-schema is showing

No, just open it and you will see it is placed differently...

I see what you mean. I will match the ordering of example-schema.


- add constraint for reg and reg-names

Not complete...

I will complete.


- 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 | 106 ++++++++++++++++++
2 files changed, 106 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;
- #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..999dcda05f55
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/altr,pcie-root-port.yaml
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright (C) 2024, Intel Corporation

This is derivative of previous work, which is easily visible by doing
the same mistakes in DTS as they were before.

This is definitely derivative of previous work, and I want to fix the DTS mistakes too.


You now added fresh copyrights ignoring all previous work, even though
you copied it. I don't agree.

If you want to ignore previous copyrights, then at least don't copy
existing code... although even that would not be sufficient.

Ignoring previous copyrights was not my intent. There is no copyright statement in the original text version of the device tree bindings. Should that lack of copyright statement carry forward?


+%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@xxxxxxxxxxxxxxx>
+
+properties:
+ compatible:
+ items:

Drop items.

I will drop the items.


+ - enum:
+ - altr,pcie-root-port-1.0
+ - altr,pcie-root-port-2.0
+

Missing reg with constraints.

I will add the following here:

reg:
minItems: 2
maxItems: 3

reg-names:
minItems: 2
maxItems: 3


+ 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
+ - device_type
+ - interrupts
+ - interrupt-map
+ - interrupt-map-mask
+
+unevaluatedProperties: false
+
+allOf:
+ - $ref: /schemas/pci/pci-bus.yaml#

That's deprecated, as explained in its description. You should use
pci-host-bridge.yaml.

I will switch to pci-host-bridge.yaml.




+ - if:
+ properties:
+ compatible:
+ enum:
+ - altr,pcie-root-port-1.0
+ then:
+ properties:
+ reg:
+ items:
+ - description: TX slave port region
+ - description: Control register access region
+
+ reg-names:
+ items:
+ - const: Txs
+ - const: Cra
+
+ else:
+ properties:
+ reg:
+ items:
+ - description: Hard IP region
+ - description: TX slave port region
+ - description: Control register access region
+
+ reg-names:
+ items:
+ - const: Hip
+ - const: Txs
+ - const: Cra
+

unevaluated goes here, just like example-schema.

Yes, just like the example-schema


+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>;

That's two entries.

I will fix the broken DTS as follows:

ranges = <0x82000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x10000000>,
<0x82000000 0x00000000 0x10000000 0xd0000000 0x00000000 0x10000000>;

Best regards,
Krzysztof



Thank you for the review,
Matthew Gerlach