| Message ID | DS7PR19MB88834CAC414A0C2B4D71D57C9DD22@DS7PR19MB8883.namprd19.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | Enable IPQ5018 PCI support | expand |
On Fri, Mar 14, 2025 at 09:56:41AM +0400, George Moussalem wrote: > From: Nitheesh Sekar <quic_nsekar@quicinc.com> > > Add support for the PCIe controller on the Qualcomm > IPQ5108 SoC to the bindings. > > Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: George Moussalem <george.moussalem@outlook.com> > --- > .../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. > + 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. Best regards, Krzysztof
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@quicinc.com> >> >> Add support for the PCIe controller on the Qualcomm >> IPQ5108 SoC to the bindings. >> >> Signed-off-by: Nitheesh Sekar <quic_nsekar@quicinc.com> >> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Signed-off-by: George Moussalem <george.moussalem@outlook.com> >> --- >> .../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
On 14/03/2025 09:42, George Moussalem wrote: >>> + 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 Which discusses were that? What were the reasons to start with parf? > other sections in the dt as much as possible, and follow your guidance > instead. Best regards, Krzysztof
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 + 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 + interrupt-names: + items: + - const: msi0 + - const: msi1 + - const: msi2 + - const: msi3 + - const: msi4 + - const: msi5 + - const: msi6 + - const: msi7 + - const: global + - if: properties: compatible: @@ -562,6 +620,7 @@ allOf: enum: - qcom,pcie-apq8064 - qcom,pcie-ipq4019 + - qcom,pcie-ipq5018 - qcom,pcie-ipq8064 - qcom,pcie-ipq8064v2 - qcom,pcie-ipq8074