Re: [PATCH 1/6] dt-bindings: pci/qcom,pcie: convert to YAML

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

On 22/04/2022 13:48, Dmitry Baryshkov wrote:
> Changes to examples:
>  - Inline clock and reset numbers rather than including dt-bindings
>    files because of conflicts between the headers
>  - Split ranges properties to follow current practice
> 
> Changes to the schema:
>  - Fixed the ordering of clock-names/reset-names according to
>    the dtsi files.
>  - Mark vdda-supply as required only for apq/ipq8064 (as it was marked
>    as generally required in the txt file).
> 
> Note: while it was not clearly described in text schema, the majority of
> Qualcomm platforms follow the snps,dw-pcie schema and use two
> compatibility strings in the DT files: platform-specific one and a
> fallback to the generic snps,dw-pcie one. This will be sorted out in the
> next patches.

I don't get why you add snps,dw-pcie fallback here, even though original
bindings (except examples, which are not bindings) were not mentioning
it. Maybe just skip it?

> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt     | 397 ------------
>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 607 ++++++++++++++++++
>  2 files changed, 607 insertions(+), 397 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.txt
>  create mode 100644 Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> 

(...)

> -	};
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> new file mode 100644
> index 000000000000..89a1021df9bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -0,0 +1,607 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm PCI express root complex
> +
> +maintainers:
> +  - Bjorn Andersson <bjorn.andersson@linaro.org>
> +  - Stanimir Varbanov <svarbanov@mm-sol.com>
> +
> +description: |
> +  Qualcomm PCIe root complex controller is bansed on the Synopsys DesignWare
> +  PCIe IP.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,pcie-ipq8064
> +          - qcom,pcie-ipq8064-v2
> +          - qcom,pcie-apq8064
> +          - qcom,pcie-apq8084
> +          - qcom,pcie-msm8996
> +          - qcom,pcie-ipq4019
> +          - qcom,pcie-ipq8074
> +          - qcom,pcie-qcs404
> +          - qcom,pcie-sc8180x
> +          - qcom,pcie-sdm845
> +          - qcom,pcie-sm8250
> +          - qcom,pcie-sm8450-pcie0
> +          - qcom,pcie-sm8450-pcie1
> +          - qcom,pcie-ipq6018
> +      - const: snps,dw-pcie
> +
> +  reg:
> +    minItems: 4
> +    maxItems: 5
> +
> +  reg-names:
> +    minItems: 4
> +    maxItems: 5
> +    items:
> +      enum:
> +        - parf # Qualcomm specific registers
> +        - dbi # DesignWare PCIe registers
> +        - elbi # External local bus interface registers
> +        - config # PCIe configuration space
> +        - atu # ATU address space (optional)> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    items:
> +      - const: "msi"
> +
> +  clocks: true

min/maxItems

same for clock-names

> +
> +  vdda-supply:
> +    description: A phandle to the core analog power supply
> +
> +  vdda_phy-supply:
> +    description: A phandle to the core analog power supply for PHY
> +
> +  vdda_refclk-supply:
> +    description: A phandle to the core analog power supply for IC which generates reference clock
> +
> +  vddpe-3v3-supply:
> +    description: A phandle to the PCIe endpoint power supply
> +
> +  phys:
> +    maxItems: 1
> +
> +  phy-names:
> +    items:
> +      - const: "pciephy"
> +
> +  perst-gpio:
> +    description: GPIO pin number of PERST# signal
> +    maxItems: 1
> +    deprecated: true

Old binding did not have it.

> +
> +  perst-gpios:
> +    description: GPIO controlled connection to PERST# signal
> +    maxItems: 1
> +

You miss here power-domains, resets and reset-names with min/maxItems.

> +  wake-gpio:
> +    description: GPIO pin number of WAKE# signal
> +    maxItems: 1
> +    deprecated: true

Old binding did not have it.

> +
> +  wake-gpios:
> +    description: GPIO controlled connection to WAKE# signal
> +    maxItems: 1
> +
> +  iommu-map: true
> +  iommu-map-mask: true

Not present in old binding. If this is trully needed, mention it in
commit msg.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names

What about interrupt-cells, clocks, clock-names, resets, reset-names?

> +
> +allOf:
> +  - $ref: /schemas/pci/pci-bus.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-apq8064
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 3
> +          maxItems: 3
> +        clock-names:
> +          items:
> +            - const: core # Clocks the pcie hw block
> +            - const: iface # Configuration AHB clock
> +            - const: phy # Clocks the pcie PHY block
> +        resets:
> +          minItems: 5
> +          maxItems: 5
> +        reset-names:
> +          items:
> +            - const: axi # AXI reset
> +            - const: ahb # AHB reset
> +            - const: por # POR reset
> +            - const: pci # PCI reset
> +            - const: phy # PHY reset

Missing required properties e.g. some supplies.
Plus one blank line.

The same applies below to other ifs.

> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-ipq8064
> +              - qcom,pcie-ipq8064v2

(...)

> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    pcie@1b500000 {
> +      compatible = "qcom,pcie-ipq8064", "snps,dw-pcie";
> +      reg = <0x1b500000 0x1000
> +             0x1b502000 0x80
> +             0x1b600000 0x100
> +             0x0ff00000 0x100000>;

Convert the example to match current bindings, so reg should be split.

> +      reg-names = "dbi", "elbi", "parf", "config";
> +      device_type = "pci";
> +      linux,pci-domain = <0>;
> +      bus-range = <0x00 0xff>;
> +      num-lanes = <1>;
> +      #address-cells = <3>;
> +      #size-cells = <2>;
> +      ranges = <0x81000000 0 0 0x0fe00000 0 0x00100000>, 
> +               <0x82000000 0 0 0x08000000 0 0x07e00000>; 
> +      interrupts = <GIC_SPI 238 IRQ_TYPE_NONE>;

Looks like wrong IRQ flag.

> +      interrupt-names = "msi";
> +      #interrupt-cells = <1>;
> +      interrupt-map-mask = <0 0 0 0x7>;
> +      interrupt-map = <0 0 0 1 &intc 0 36 IRQ_TYPE_LEVEL_HIGH>, 
> +                      <0 0 0 2 &intc 0 37 IRQ_TYPE_LEVEL_HIGH>, 
> +                      <0 0 0 3 &intc 0 38 IRQ_TYPE_LEVEL_HIGH>, 
> +                      <0 0 0 4 &intc 0 39 IRQ_TYPE_LEVEL_HIGH>; 
> +      clocks = <&gcc 41>,
> +               <&gcc 43>,
> +               <&gcc 44>,
> +               <&gcc 42>,
> +               <&gcc 248>;
> +      clock-names = "core", "iface", "phy", "aux", "ref";
> +      resets = <&gcc 27>,
> +               <&gcc 26>,
> +               <&gcc 25>,
> +               <&gcc 24>,
> +               <&gcc 23>,
> +               <&gcc 22>;
> +      reset-names = "axi", "ahb", "por", "pci", "phy", "ext";
> +      pinctrl-0 = <&pcie_pins_default>;
> +      pinctrl-names = "default";
> +      vdda-supply = <&pm8921_s3>;
> +      vdda_phy-supply = <&pm8921_lvs6>;
> +      vdda_refclk-supply = <&ext_3p3v>;
> +    };
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    pcie@fc520000 {
> +      compatible = "qcom,pcie-apq8084", "snps,dw-pcie";
> +      reg = <0xfc520000 0x2000>,
> +            <0xff000000 0x1000>,
> +            <0xff001000 0x1000>,
> +            <0xff002000 0x2000>;
> +      reg-names = "parf", "dbi", "elbi", "config";
> +      device_type = "pci";
> +      linux,pci-domain = <0>;
> +      bus-range = <0x00 0xff>;
> +      num-lanes = <1>;
> +      #address-cells = <3>;
> +      #size-cells = <2>;
> +      ranges = <0x81000000 0 0          0xff200000 0 0x00100000>, 
> +               <0x82000000 0 0x00300000 0xff300000 0 0x00d00000>; 
> +      interrupts = <GIC_SPI 243 IRQ_TYPE_NONE>;

Ditto.


Best regards,
Krzysztof