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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 22 Apr 2022 at 16:10, Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> 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?

Ack, I'll squash the snps,dw-pcie patch into this one.

>
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxx>
> > +  - Stanimir Varbanov <svarbanov@xxxxxxxxxx>
> > +
> > +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.

Ack, dropping.

>
> > +
> > +  perst-gpios:
> > +    description: GPIO controlled connection to PERST# signal
> > +    maxItems: 1
> > +
>
> You miss here power-domains, resets and reset-names with min/maxItems.

power-domains are described later in the non-8064 cases.

Will add everything here.

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

Ack dropping.

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

They are used on newer platforms. Will mention it.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-names
>
> What about interrupt-cells, clocks, clock-names, resets, reset-names?

Ugh. Missed them, adding back.

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

Ok, I'll merge all apq8064 and ipq8064 entries into a single if/then condition.

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

I wonder why the dt_bindings check didn't warn me about it.

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

Ack

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

Ack


--
With best wishes
Dmitry



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux