[PATCH v2 1/2] dt-bindings: PCI: qcom: adjust iommu-map for different SoC

[PATCH v2 1/2] dt-bindings: PCI: qcom: adjust iommu-map for different SoC

[Krzysztof Kozlowski]

The PCIe controller on SDX55 has five entries in its iommu-map, MSM8998
has one and SDM845 has sixteen, so allow wider number of items to fix
dtbs_check warnings like:

  qcom-sdx55-mtp.dtb: pcie@1c00000: iommu-map: [[0, 21, 512, 1], [256, 21, 513, 1],
    [512, 21, 514, 1], [768, 21, 515, 1], [1024, 21, 516, 1]] is too long

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes in v2:
1. Add Acs/Rb.
---
 Documentation/devicetree/bindings/pci/qcom,pcie.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index 8bfae8eb79a3..14d25e8a18e4 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -62,7 +62,8 @@ properties:
     maxItems: 8
 
   iommu-map:
-    maxItems: 2
+    minItems: 1
+    maxItems: 16
 
   # Common definitions for clocks, clock-names and reset.
   # Platform constraints are described later.
-- 
2.34.1

[PATCH v2 2/2] dt-bindings: PCI: qcom: correct clocks for SC8180x and SM8150

[Krzysztof Kozlowski]

PCI node in Qualcomm SC8180x DTS has 8 clocks, while one on SM8150 has 7
clocks:

  sc8180x-primus.dtb: pci@1c00000: 'oneOf' conditional failed, one must be fixed:
    ['pipe', 'aux', 'cfg', 'bus_master', 'bus_slave', 'slave_q2a', 'ref', 'tbu'] is too short

  sm8150-hdk.dtb: pci@1c00000: 'oneOf' conditional failed, one must be fixed:
    ['pipe', 'aux', 'cfg', 'bus_master', 'bus_slave', 'slave_q2a', 'tbu'] is too short

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Changes in v2:
1. Add Acs/Rb.
2. Correct error message for sm8150.
---
 .../devicetree/bindings/pci/qcom,pcie.yaml    | 58 ++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index 14d25e8a18e4..4c993ea97d7c 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -479,6 +479,35 @@ allOf:
           items:
             - const: pci # PCIe core reset
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pcie-sc8180x
+    then:
+      oneOf:
+        - properties:
+            clocks:
+              minItems: 8
+              maxItems: 8
+            clock-names:
+              items:
+                - const: pipe # PIPE clock
+                - const: aux # Auxiliary clock
+                - const: cfg # Configuration clock
+                - const: bus_master # Master AXI clock
+                - const: bus_slave # Slave AXI clock
+                - const: slave_q2a # Slave Q2A clock
+                - const: ref # REFERENCE clock
+                - const: tbu # PCIe TBU clock
+      properties:
+        resets:
+          maxItems: 1
+        reset-names:
+          items:
+            - const: pci # PCIe core reset
+
   - if:
       properties:
         compatible:
@@ -527,8 +556,35 @@ allOf:
         compatible:
           contains:
             enum:
-              - qcom,pcie-sc8180x
               - qcom,pcie-sm8150
+    then:
+      oneOf:
+        - properties:
+            clocks:
+              minItems: 7
+              maxItems: 7
+            clock-names:
+              items:
+                - const: pipe # PIPE clock
+                - const: aux # Auxiliary clock
+                - const: cfg # Configuration clock
+                - const: bus_master # Master AXI clock
+                - const: bus_slave # Slave AXI clock
+                - const: slave_q2a # Slave Q2A clock
+                - const: tbu # PCIe TBU clock
+      properties:
+        resets:
+          maxItems: 1
+        reset-names:
+          items:
+            - const: pci # PCIe core reset
+
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,pcie-sm8250
     then:
       oneOf:
-- 
2.34.1

Re: [PATCH v2 2/2] dt-bindings: PCI: qcom: correct clocks for SC8180x and SM8150

[Dmitry Baryshkov]

On Mon, 20 Nov 2023 at 09:09, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> PCI node in Qualcomm SC8180x DTS has 8 clocks, while one on SM8150 has 7
> clocks:
>
>   sc8180x-primus.dtb: pci@1c00000: 'oneOf' conditional failed, one must be fixed:
>     ['pipe', 'aux', 'cfg', 'bus_master', 'bus_slave', 'slave_q2a', 'ref', 'tbu'] is too short
>
>   sm8150-hdk.dtb: pci@1c00000: 'oneOf' conditional failed, one must be fixed:
>     ['pipe', 'aux', 'cfg', 'bus_master', 'bus_slave', 'slave_q2a', 'tbu'] is too short
>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Changes in v2:
> 1. Add Acs/Rb.
> 2. Correct error message for sm8150.
> ---
>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 58 ++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index 14d25e8a18e4..4c993ea97d7c 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -479,6 +479,35 @@ allOf:
>            items:
>              - const: pci # PCIe core reset
>
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,pcie-sc8180x
> +    then:
> +      oneOf:
> +        - properties:
> +            clocks:
> +              minItems: 8
> +              maxItems: 8
> +            clock-names:
> +              items:
> +                - const: pipe # PIPE clock
> +                - const: aux # Auxiliary clock
> +                - const: cfg # Configuration clock
> +                - const: bus_master # Master AXI clock
> +                - const: bus_slave # Slave AXI clock
> +                - const: slave_q2a # Slave Q2A clock
> +                - const: ref # REFERENCE clock
> +                - const: tbu # PCIe TBU clock
> +      properties:
> +        resets:
> +          maxItems: 1
> +        reset-names:
> +          items:
> +            - const: pci # PCIe core reset
> +
>    - if:
>        properties:
>          compatible:
> @@ -527,8 +556,35 @@ allOf:
>          compatible:
>            contains:
>              enum:
> -              - qcom,pcie-sc8180x
>                - qcom,pcie-sm8150
> +    then:
> +      oneOf:
> +        - properties:
> +            clocks:
> +              minItems: 7
> +              maxItems: 7
> +            clock-names:
> +              items:
> +                - const: pipe # PIPE clock
> +                - const: aux # Auxiliary clock
> +                - const: cfg # Configuration clock
> +                - const: bus_master # Master AXI clock
> +                - const: bus_slave # Slave AXI clock
> +                - const: slave_q2a # Slave Q2A clock

Mani promised to check whether we should use the 'ref' clock for the
PCIe hosts or not.
I'd ask to delay this patch until we finish that investigation.

> +                - const: tbu # PCIe TBU clock
> +      properties:
> +        resets:
> +          maxItems: 1
> +        reset-names:
> +          items:
> +            - const: pci # PCIe core reset
> +
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
>                - qcom,pcie-sm8250
>      then:
>        oneOf:
> --
> 2.34.1
>
>


-- 
With best wishes
Dmitry

Re: [PATCH v2 2/2] dt-bindings: PCI: qcom: correct clocks for SC8180x and SM8150

[Krzysztof Kozlowski]

On 20/11/2023 11:11, Dmitry Baryshkov wrote:
>> +    then:
>> +      oneOf:
>> +        - properties:
>> +            clocks:
>> +              minItems: 7
>> +              maxItems: 7
>> +            clock-names:
>> +              items:
>> +                - const: pipe # PIPE clock
>> +                - const: aux # Auxiliary clock
>> +                - const: cfg # Configuration clock
>> +                - const: bus_master # Master AXI clock
>> +                - const: bus_slave # Slave AXI clock
>> +                - const: slave_q2a # Slave Q2A clock
> 
> Mani promised to check whether we should use the 'ref' clock for the
> PCIe hosts or not.
> I'd ask to delay this patch until we finish that investigation.

Right. I thought that his Rb-tag solves it, but if not - let's wait.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] dt-bindings: PCI: qcom: correct clocks for SC8180x and SM8150

[Manivannan Sadhasivam]

On Mon, Nov 20, 2023 at 11:48:25AM +0100, Krzysztof Kozlowski wrote:
> On 20/11/2023 11:11, Dmitry Baryshkov wrote:
> >> +    then:
> >> +      oneOf:
> >> +        - properties:
> >> +            clocks:
> >> +              minItems: 7
> >> +              maxItems: 7
> >> +            clock-names:
> >> +              items:
> >> +                - const: pipe # PIPE clock
> >> +                - const: aux # Auxiliary clock
> >> +                - const: cfg # Configuration clock
> >> +                - const: bus_master # Master AXI clock
> >> +                - const: bus_slave # Slave AXI clock
> >> +                - const: slave_q2a # Slave Q2A clock
> > 
> > Mani promised to check whether we should use the 'ref' clock for the
> > PCIe hosts or not.
> > I'd ask to delay this patch until we finish that investigation.
> 
> Right. I thought that his Rb-tag solves it, but if not - let's wait.
> 

We discussed mostly offline, after I gave my R-b tag.

I checked with Qcom on the use of "ref" clock in both PCIe and PHY nodes.
It turned out that both nodes indeed need the "ref" clock, but not the
GCC.*CLKREF that comes out of GCC.

GCC.*CLKREF is only needed by the PHY node since PHY uses it for it's internal
logic. For PCIe node, RPMH_CXO_CLK should be used as "ref" clock since it is
used by the PCIe IP internally. This behavior applies to other peripherals like
UFS, USB as well with same inconsistency in DT.

So we need to fix this for those peripherals also. I can take up PCIe and UFS,
and someone needs to fix USB.

And for this patch, "ref" clock needs to be added to SM8150.

Thanks Dmitry for pointing this out mess!

- Mani

> Best regards,
> Krzysztof
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: adjust iommu-map for different SoC

[David]

> +    minItems: 1
Hello Krzysztof,

the driver will accept 0 just fine, so I think this definition may be wrong.

I sent just generic "dt-bindings: PCI: qcom: delimit number of iommu-map entries" which doesn't care about the numbers (in similar fashion as other bindings having iommu-map).

Tell me what you think.

David

Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: adjust iommu-map for different SoC

[Manivannan Sadhasivam]

On Fri, Dec 29, 2023 at 04:36:31PM +0100, David wrote:
> > +    minItems: 1
> Hello Krzysztof,
> 
> the driver will accept 0 just fine, so I think this definition may be wrong.
> 

It's not entirely wrong but the actual SID mapping differs between SoCs.

> I sent just generic "dt-bindings: PCI: qcom: delimit number of iommu-map entries" which doesn't care about the numbers (in similar fashion as other bindings having iommu-map).
> 

No, we should not just ignore the MAX limit. If you add <N> number of entries
exceeding the max SID assigned to PCIe bus, it will fail.

- Mani

> Tell me what you think.
> 
> David
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v2 1/2] dt-bindings: PCI: qcom: adjust iommu-map for different SoC

[David Heidelberg]

On 29/12/2023 18:17, Manivannan Sadhasivam wrote:
> On Fri, Dec 29, 2023 at 04:36:31PM +0100, David wrote:
>>> +    minItems: 1
>> Hello Krzysztof,
>>
>> the driver will accept 0 just fine, so I think this definition may be wrong.
>>
> It's not entirely wrong but the actual SID mapping differs between SoCs.
Sure, I think I can live with this.
>
>> I sent just generic "dt-bindings: PCI: qcom: delimit number of iommu-map entries" which doesn't care about the numbers (in similar fashion as other bindings having iommu-map).
>>
> No, we should not just ignore the MAX limit. If you add <N> number of entries
> exceeding the max SID assigned to PCIe bus, it will fail.
>
> - Mani

Make sense, thanks for explanation.

Reviewed-by: David Heidelberg <david@ixit.cz>

>> Tell me what you think.
>>
>> David
>>
-- 
David Heidelberg