Re: [PATCH v4 3/6] dt-bindings: PCI: qcom: Add IPQ5018 SoC

[George Moussalem]

On 3/14/25 12:20, Krzysztof Kozlowski wrote:
> On Fri, Mar 14, 2025 at 09:56:41AM +0400, George Moussalem wrote:
> > From: Nitheesh Sekar <quic_nsekar@xxxxxxxxxxx>
> >
> > Add support for the PCIe controller on the Qualcomm
> > IPQ5108 SoC to the bindings.
> >
> > Signed-off-by: Nitheesh Sekar <quic_nsekar@xxxxxxxxxxx>
> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@xxxxxxxxxxx>
> > Signed-off-by: George Moussalem <george.moussalem@xxxxxxxxxxx>
> > ---
> >   .../devicetree/bindings/pci/qcom,pcie.yaml    | 59 +++++++++++++++++++
> >   1 file changed, 59 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > index 8f628939209e..d8befaa558e2 100644
> > --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> > @@ -21,6 +21,7 @@ properties:
> >             - qcom,pcie-apq8064
> >             - qcom,pcie-apq8084
> >             - qcom,pcie-ipq4019
> > +          - qcom,pcie-ipq5018
> >             - qcom,pcie-ipq6018
> >             - qcom,pcie-ipq8064
> >             - qcom,pcie-ipq8064-v2
> > @@ -322,6 +323,63 @@ allOf:
> >               - const: ahb # AHB reset
> >               - const: phy_ahb # PHY AHB reset
> >   
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,pcie-ipq5018
> > +    then:
> > +      properties:
> > +        reg:
> > +          minItems: 5
> > +          maxItems: 5
> > +        reg-names:
> > +          items:
> > +            - const: parf # Qualcomm specific registers
> > +            - const: dbi # DesignWare PCIe registers
> > +            - const: elbi # External local bus interface registers
> > +            - const: atu # ATU address space
> > +            - const: config # PCIe configuration space
>
> Keep the same order as other IPQ, so dbi+elbi+atu+parf+config. Same for
> everything else, so standard rule applies: devices are supposed to use
> ordering from existing variants.
>
> There is some huge mess with IPQ PCI bindings, including things on the
> list. Apparently it became my job to oversee Qualcomm PCI work... well,
> I do not have time for that, so rather I expect contributors to
> cooperate in this matter.
>
> Don't throw your patches over the wall.
>
> If you need to rework the patch, take the ownership and rework it.

Thanks Krzysztof. I did reorder them deliberately based on unit 
addresses as discussed also in other threads about IPQ9574 and IPQ5332 
as I thought it would be neater that way. I'll change it back, reuse 
other sections in the dt as much as possible, and follow your guidance 
instead.

> > +        clocks:
> > +          minItems: 6
> > +          maxItems: 6
> > +        clock-names:
> > +          items:
> > +            - const: iface # PCIe to SysNOC BIU clock
> > +            - const: axi_m # AXI Master clock
> > +            - const: axi_s # AXI Slave clock
> > +            - const: ahb # AHB clock
> > +            - const: aux # Auxiliary clock
> > +            - const: axi_bridge # AXI bridge clock
> > +        resets:
> > +          minItems: 8
> > +          maxItems: 8
> > +        reset-names:
> > +          items:
> > +            - const: pipe # PIPE reset
> > +            - const: sleep # Sleep reset
> > +            - const: sticky # Core sticky reset
> > +            - const: axi_m # AXI master reset
> > +            - const: axi_s # AXI slave reset
> > +            - const: ahb # AHB reset
> > +            - const: axi_m_sticky # AXI master sticky reset
> > +            - const: axi_s_sticky # AXI slave sticky reset
> > +        interrupts:
> > +          minItems: 8
> > +          maxItems: 8
>
> 8 items...
>
> > +        interrupt-names:
> > +          items:
> > +            - const: msi0
> > +            - const: msi1
> > +            - const: msi2
> > +            - const: msi3
> > +            - const: msi4
> > +            - const: msi5
> > +            - const: msi6
> > +            - const: msi7
> > +            - const: global
>
> And here 9 items. You got comment on this. What's more, I doubt that DTS
> was tsted.

Will fix, thanks.

> Best regards,
> Krzysztof

Best regards,
George