Received-SPF: pass (google.com: domain of linux-kernel+bounces-135908-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 8 Apr 2024 13:34:24 -0700 (PDT)
From: matthew.gerlach@linux.intel.com
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
cc: bhelgaas@google.com, lpieralisi@kernel.org, kw@linux.com, robh@kernel.org, 
    krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
    linux-pci@vger.kernel.org, devicetree@vger.kernel.org, 
    linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] dt-bindings: PCI: altera: Convert to YAML
In-Reply-To: <2ece9ac2-899c-4185-b0f3-8ab939afc1e5@linaro.org>
Message-ID: <alpine.DEB.2.22.394.2404081309050.381257@sj-4150-psse-sw-opae-dev2>
References: <20240405145322.3805828-1-matthew.gerlach@linux.intel.com> <2ece9ac2-899c-4185-b0f3-8ab939afc1e5@linaro.org>
User-Agent: Alpine 2.22 (DEB 394 2020-01-19)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed


On Sun, 7 Apr 2024, Krzysztof Kozlowski wrote:

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

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@linux.intel.com>
>> +
>> +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