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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 05/04/2024 16:53, 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>
> ---
> 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...

>  - add constraint for reg and reg-names

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

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.

> +%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:
> +    items:

Drop items.

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

Missing reg with constraints.

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



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

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

Best regards,
Krzysztof