[PATCH v2 1/7] dt-bindings: clock: Add PCIe pipe related clocks for IPQ9574

[PATCH v2 1/7] dt-bindings: clock: Add PCIe pipe related clocks for IPQ9574

[Alexandru Gagniuc]

Add defines for the missing PCIe PIPE clocks.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 include/dt-bindings/clock/qcom,ipq9574-gcc.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/dt-bindings/clock/qcom,ipq9574-gcc.h b/include/dt-bindings/clock/qcom,ipq9574-gcc.h
index 08fd3a37acaa..52123c5a09fa 100644
--- a/include/dt-bindings/clock/qcom,ipq9574-gcc.h
+++ b/include/dt-bindings/clock/qcom,ipq9574-gcc.h
@@ -216,4 +216,8 @@
 #define GCC_CRYPTO_AHB_CLK				207
 #define GCC_USB0_PIPE_CLK				208
 #define GCC_USB0_SLEEP_CLK				209
+#define GCC_PCIE0_PIPE_CLK				210
+#define GCC_PCIE1_PIPE_CLK				211
+#define GCC_PCIE2_PIPE_CLK				212
+#define GCC_PCIE3_PIPE_CLK				213
 #endif
-- 
2.40.1

[PATCH v2 3/7] dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller

[Alexandru Gagniuc]

IPQ9574 has PCIe controllers which are almost identical to IPQ6018.
The only difference is that the "iface" clock is not required.
Document this difference along with the compatible string.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 .../devicetree/bindings/pci/qcom,pcie.yaml    | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
index cf9a6910b542..1915bea580d3 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
@@ -26,6 +26,7 @@ properties:
           - qcom,pcie-ipq8064-v2
           - qcom,pcie-ipq8074
           - qcom,pcie-ipq8074-gen3
+          - qcom,pcie-ipq9574
           - qcom,pcie-msm8996
           - qcom,pcie-qcs404
           - qcom,pcie-sdm845
@@ -397,6 +398,37 @@ allOf:
             - const: axi_m_sticky # AXI Master Sticky reset
             - const: axi_s_sticky # AXI Slave Sticky reset
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,pcie-ipq9574
+    then:
+      properties:
+        clocks:
+          minItems: 4
+          maxItems: 4
+        clock-names:
+          items:
+            - const: axi_m # AXI Master clock
+            - const: axi_s # AXI Slave clock
+            - const: axi_bridge # AXI bridge clock
+            - const: rchng
+        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
+
   - if:
       properties:
         compatible:
@@ -507,6 +539,7 @@ allOf:
                 - qcom,pcie-ipq8064v2
                 - qcom,pcie-ipq8074
                 - qcom,pcie-ipq8074-gen3
+                - qcom,pcie-ipq9574
                 - qcom,pcie-qcs404
     then:
       required:
@@ -566,6 +599,7 @@ allOf:
               - qcom,pcie-ipq8064-v2
               - qcom,pcie-ipq8074
               - qcom,pcie-ipq8074-gen3
+              - qcom,pcie-ipq9574
               - qcom,pcie-qcs404
     then:
       properties:
-- 
2.40.1

Re: [PATCH v2 3/7] dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller

[Krzysztof Kozlowski]

On 09/04/2024 21:08, Alexandru Gagniuc wrote:
> IPQ9574 has PCIe controllers which are almost identical to IPQ6018.
> The only difference is that the "iface" clock is not required.
> Document this difference along with the compatible string.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.yaml    | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> index cf9a6910b542..1915bea580d3 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
> @@ -26,6 +26,7 @@ properties:
>            - qcom,pcie-ipq8064-v2
>            - qcom,pcie-ipq8074
>            - qcom,pcie-ipq8074-gen3
> +          - qcom,pcie-ipq9574
>            - qcom,pcie-msm8996
>            - qcom,pcie-qcs404
>            - qcom,pcie-sdm845
> @@ -397,6 +398,37 @@ allOf:
>              - const: axi_m_sticky # AXI Master Sticky reset
>              - const: axi_s_sticky # AXI Slave Sticky reset
>  

Where do you constrain the reg?

Best regards,
Krzysztof

Re: [PATCH v2 3/7] dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller

[mr.nuke.me]

On 4/9/24 15:08, Krzysztof Kozlowski wrote:
> On 09/04/2024 21:08, Alexandru Gagniuc wrote:
>> IPQ9574 has PCIe controllers which are almost identical to IPQ6018.
>> The only difference is that the "iface" clock is not required.
>> Document this difference along with the compatible string.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   .../devicetree/bindings/pci/qcom,pcie.yaml    | 34 +++++++++++++++++++
>>   1 file changed, 34 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> index cf9a6910b542..1915bea580d3 100644
>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>> @@ -26,6 +26,7 @@ properties:
>>             - qcom,pcie-ipq8064-v2
>>             - qcom,pcie-ipq8074
>>             - qcom,pcie-ipq8074-gen3
>> +          - qcom,pcie-ipq9574
>>             - qcom,pcie-msm8996
>>             - qcom,pcie-qcs404
>>             - qcom,pcie-sdm845
>> @@ -397,6 +398,37 @@ allOf:
>>               - const: axi_m_sticky # AXI Master Sticky reset
>>               - const: axi_s_sticky # AXI Slave Sticky reset
>>   
> 
> Where do you constrain the reg?

I didn't realize that was also required -- the make checks should have 
picked this up too? I might be invoking the tests incorrectly.

I should add the ipq9574 in the same list as ipq8074-gen3 and ipq6018, 
correct?

Alex

Re: [PATCH v2 3/7] dt-bindings: PCI: qcom: Add IPQ9574 PCIe controller

[Krzysztof Kozlowski]

On 11/04/2024 20:00, mr.nuke.me@gmail.com wrote:
> 
> 
> On 4/9/24 15:08, Krzysztof Kozlowski wrote:
>> On 09/04/2024 21:08, Alexandru Gagniuc wrote:
>>> IPQ9574 has PCIe controllers which are almost identical to IPQ6018.
>>> The only difference is that the "iface" clock is not required.
>>> Document this difference along with the compatible string.
>>>
>>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>>> ---
>>>   .../devicetree/bindings/pci/qcom,pcie.yaml    | 34 +++++++++++++++++++
>>>   1 file changed, 34 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> index cf9a6910b542..1915bea580d3 100644
>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>> @@ -26,6 +26,7 @@ properties:
>>>             - qcom,pcie-ipq8064-v2
>>>             - qcom,pcie-ipq8074
>>>             - qcom,pcie-ipq8074-gen3
>>> +          - qcom,pcie-ipq9574
>>>             - qcom,pcie-msm8996
>>>             - qcom,pcie-qcs404
>>>             - qcom,pcie-sdm845
>>> @@ -397,6 +398,37 @@ allOf:
>>>               - const: axi_m_sticky # AXI Master Sticky reset
>>>               - const: axi_s_sticky # AXI Slave Sticky reset
>>>   
>>
>> Where do you constrain the reg?
> 
> I didn't realize that was also required -- the make checks should have 
> picked this up too? I might be invoking the tests incorrectly.
> 
> I should add the ipq9574 in the same list as ipq8074-gen3 and ipq6018, 
> correct?

If you add new variant, look at existing compatibles where they appear.
If there is a if: constraining compatibles, then it's a hint you should
do the same for your device. So yes, you must constrain all properties
which are made flexible in top-level properties.

Best regards,
Krzysztof

[PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Alexandru Gagniuc]

The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two
extra clocks named "anoc" and "snoc". Document this, and add a
new compatible string for this PHY.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 .../phy/qcom,ipq8074-qmp-pcie-phy.yaml        | 31 ++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
index 634cec5d57ea..017ad65a9a3c 100644
--- a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
@@ -19,19 +19,22 @@ properties:
       - qcom,ipq6018-qmp-pcie-phy
       - qcom,ipq8074-qmp-gen3-pcie-phy
       - qcom,ipq8074-qmp-pcie-phy
+      - qcom,ipq9574-qmp-gen3x2-pcie-phy
 
   reg:
     items:
       - description: serdes
 
   clocks:
-    maxItems: 3
+    minItems: 3
 
   clock-names:
     items:
       - const: aux
       - const: cfg_ahb
       - const: pipe
+      - const: anoc
+      - const: snoc
 
   resets:
     maxItems: 2
@@ -61,6 +64,32 @@ required:
   - clock-output-names
   - "#phy-cells"
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq6018-qmp-pcie-phy
+              - qcom,ipq8074-qmp-gen3-pcie-phy
+              - qcom,ipq8074-qmp-pcie-phy
+    then:
+      properties:
+        clocks:
+          maxItems: 3
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,ipq9574-qmp-gen3x2-pcie-phy
+    then:
+      properties:
+        clocks:
+          minItems: 5
+          maxItems: 5
+
 additionalProperties: false
 
 examples:
-- 
2.40.1

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Krzysztof Kozlowski]

On 09/04/2024 21:08, Alexandru Gagniuc wrote:
> The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two
> extra clocks named "anoc" and "snoc". Document this, and add a
> new compatible string for this PHY.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  .../phy/qcom,ipq8074-qmp-pcie-phy.yaml        | 31 ++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
> index 634cec5d57ea..017ad65a9a3c 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
> @@ -19,19 +19,22 @@ properties:
>        - qcom,ipq6018-qmp-pcie-phy
>        - qcom,ipq8074-qmp-gen3-pcie-phy
>        - qcom,ipq8074-qmp-pcie-phy
> +      - qcom,ipq9574-qmp-gen3x2-pcie-phy
>  
>    reg:
>      items:
>        - description: serdes
>  
>    clocks:
> -    maxItems: 3
> +    minItems: 3

Which binding inspired you to such change? No, you need maxItems. See
your previous patches here how it is done.


>  
>    clock-names:
>      items:
>        - const: aux
>        - const: cfg_ahb
>        - const: pipe
> +      - const: anoc
> +      - const: snoc

OK, you did not test it. Neither this, nor DTS. I stop review, please
test first.

Best regards,
Krzysztof

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[mr.nuke.me]

On 4/9/24 15:09, Krzysztof Kozlowski wrote:
> On 09/04/2024 21:08, Alexandru Gagniuc wrote:
>> The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two
>> extra clocks named "anoc" and "snoc". Document this, and add a
>> new compatible string for this PHY.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>   .../phy/qcom,ipq8074-qmp-pcie-phy.yaml        | 31 ++++++++++++++++++-
>>   1 file changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
>> index 634cec5d57ea..017ad65a9a3c 100644
>> --- a/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
>> +++ b/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.yaml
>> @@ -19,19 +19,22 @@ properties:
>>         - qcom,ipq6018-qmp-pcie-phy
>>         - qcom,ipq8074-qmp-gen3-pcie-phy
>>         - qcom,ipq8074-qmp-pcie-phy
>> +      - qcom,ipq9574-qmp-gen3x2-pcie-phy
>>   
>>     reg:
>>       items:
>>         - description: serdes
>>   
>>     clocks:
>> -    maxItems: 3
>> +    minItems: 3
> 
> Which binding inspired you to such change? No, you need maxItems. See
> your previous patches here how it is done.
> 
> 
>>   
>>     clock-names:
>>       items:
>>         - const: aux
>>         - const: cfg_ahb
>>         - const: pipe
>> +      - const: anoc
>> +      - const: snoc
> 
> OK, you did not test it. Neither this, nor DTS. I stop review, please
> test first.

I ran both `checkpatch.pl` and `make dt_binding_check`. What in this 
patch makes you say I "did not test it", and what test or tests did I miss?

Alex

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Krzysztof Kozlowski]

On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote:

>> Which binding inspired you to such change? No, you need maxItems. See
>> your previous patches here how it is done.
>>
>>
>>>   
>>>     clock-names:
>>>       items:
>>>         - const: aux
>>>         - const: cfg_ahb
>>>         - const: pipe
>>> +      - const: anoc
>>> +      - const: snoc
>>
>> OK, you did not test it. Neither this, nor DTS. I stop review, please
>> test first.
> 
> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this 
> patch makes you say I "did not test it", and what test or tests did I miss?

You affect existing bindings, so you must test your and entire existing
DTS. You affect, by introducing new errors, in existing DTS.

Best regards,
Krzysztof

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Krzysztof Kozlowski]

On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote:
>>
>>
>>>   
>>>     clock-names:
>>>       items:
>>>         - const: aux
>>>         - const: cfg_ahb
>>>         - const: pipe
>>> +      - const: anoc
>>> +      - const: snoc
>>
>> OK, you did not test it. Neither this, nor DTS. I stop review, please
>> test first.
> 
> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this 
> patch makes you say I "did not test it", and what test or tests did I miss?
> 

... and no, you did not. If you tested, you would easily see error:
	clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short

When you receive comment from reviewer, please investigate thoroughly
what could get wrong. Don't answer just to get rid of reviewer. It's
fine to make mistakes, but if reviewer points to issue and you
immediately respond "no issue", that's waste of my time.

Look at entire code of qcom,pcie how it is organized. Or:
https://elixir.bootlin.com/linux/v6.8/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L132


Best regards,
Krzysztof

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Krzysztof Kozlowski]

On 10/04/2024 08:59, Krzysztof Kozlowski wrote:
> On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote:
>>>
>>>
>>>>   
>>>>     clock-names:
>>>>       items:
>>>>         - const: aux
>>>>         - const: cfg_ahb
>>>>         - const: pipe
>>>> +      - const: anoc
>>>> +      - const: snoc
>>>
>>> OK, you did not test it. Neither this, nor DTS. I stop review, please
>>> test first.
>>
>> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this 
>> patch makes you say I "did not test it", and what test or tests did I miss?
>>
> 
> ... and no, you did not. If you tested, you would easily see error:
> 	clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short
> 
> When you receive comment from reviewer, please investigate thoroughly
> what could get wrong. Don't answer just to get rid of reviewer. It's
> fine to make mistakes, but if reviewer points to issue and you
> immediately respond "no issue", that's waste of my time.

To clarify: "no issue" response is waste of my time. If you responded
"oh, I see the error, but I don't know how to fix it", it would be ok, I
can clarify and help in this.

Best regards,
Krzysztof

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[mr.nuke.me]

On 4/10/24 02:02, Krzysztof Kozlowski wrote:
> On 10/04/2024 08:59, Krzysztof Kozlowski wrote:
>> On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote:
>>>>
>>>>
>>>>>    
>>>>>      clock-names:
>>>>>        items:
>>>>>          - const: aux
>>>>>          - const: cfg_ahb
>>>>>          - const: pipe
>>>>> +      - const: anoc
>>>>> +      - const: snoc
>>>>
>>>> OK, you did not test it. Neither this, nor DTS. I stop review, please
>>>> test first.
>>>
>>> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this
>>> patch makes you say I "did not test it", and what test or tests did I miss?
>>>
>>
>> ... and no, you did not. If you tested, you would easily see error:
>> 	clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short
>>
>> When you receive comment from reviewer, please investigate thoroughly
>> what could get wrong. Don't answer just to get rid of reviewer. It's
>> fine to make mistakes, but if reviewer points to issue and you
>> immediately respond "no issue", that's waste of my time.
> 
> To clarify: "no issue" response is waste of my time. If you responded
> "oh, I see the error, but I don't know how to fix it", it would be ok, I
> can clarify and help in this.

I apologize if I gave you this impression. I tried to follow the testing 
process, it did not turn out as expected. Obviously, I missed something. 
I tried to ask what I missed, and in order for that question to make 
sense, I need to describe what I tried.

It turns out what I missed was "make check_dtbs". I only found that out 
after an automated email from Rob describing some troubleshooting steps.

If I may have a few sentences to rant, I see the dt-schema as a hurdle 
to making an otherwise useful change. I am told I can ask for help when 
I get stuck, yet I manage to insult the maintainer by aking for help. I 
find this very intimidating.

Alex

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Krzysztof Kozlowski]

On 10/04/2024 18:29, mr.nuke.me@gmail.com wrote:
> 
> 
> On 4/10/24 02:02, Krzysztof Kozlowski wrote:
>> On 10/04/2024 08:59, Krzysztof Kozlowski wrote:
>>> On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote:
>>>>>
>>>>>
>>>>>>    
>>>>>>      clock-names:
>>>>>>        items:
>>>>>>          - const: aux
>>>>>>          - const: cfg_ahb
>>>>>>          - const: pipe
>>>>>> +      - const: anoc
>>>>>> +      - const: snoc
>>>>>
>>>>> OK, you did not test it. Neither this, nor DTS. I stop review, please
>>>>> test first.
>>>>
>>>> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this
>>>> patch makes you say I "did not test it", and what test or tests did I miss?
>>>>
>>>
>>> ... and no, you did not. If you tested, you would easily see error:
>>> 	clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short
>>>
>>> When you receive comment from reviewer, please investigate thoroughly
>>> what could get wrong. Don't answer just to get rid of reviewer. It's
>>> fine to make mistakes, but if reviewer points to issue and you
>>> immediately respond "no issue", that's waste of my time.
>>
>> To clarify: "no issue" response is waste of my time. If you responded
>> "oh, I see the error, but I don't know how to fix it", it would be ok, I
>> can clarify and help in this.
> 
> I apologize if I gave you this impression. I tried to follow the testing 
> process, it did not turn out as expected. Obviously, I missed something. 
> I tried to ask what I missed, and in order for that question to make 
> sense, I need to describe what I tried.
> 
> It turns out what I missed was "make check_dtbs". I only found that out 
> after an automated email from Rob describing some troubleshooting steps.

No, the dt_binding_check fails. You don't need to go to dtbs_check even,
because the binding already has a failure.

> 
> If I may have a few sentences to rant, I see the dt-schema as a hurdle 
> to making an otherwise useful change. I am told I can ask for help when 
> I get stuck, yet I manage to insult the maintainer by aking for help. I 
> find this very intimidating.

I don't feel insulted but I feel my time is wasted if I tell you to test
your binding and you immediately within few minutes respond "I tested",
but then:
1. Bot confirms it was not tested,
2. I apply your patch and test it and see the failure.

Best regards,
Krzysztof

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[mr.nuke.me]

On 4/10/24 14:36, Krzysztof Kozlowski wrote:
> On 10/04/2024 18:29, mr.nuke.me@gmail.com wrote:
>>
>>
>> On 4/10/24 02:02, Krzysztof Kozlowski wrote:
>>> On 10/04/2024 08:59, Krzysztof Kozlowski wrote:
>>>> On 09/04/2024 22:19, mr.nuke.me@gmail.com wrote:
>>>>>>
>>>>>>
>>>>>>>     
>>>>>>>       clock-names:
>>>>>>>         items:
>>>>>>>           - const: aux
>>>>>>>           - const: cfg_ahb
>>>>>>>           - const: pipe
>>>>>>> +      - const: anoc
>>>>>>> +      - const: snoc
>>>>>>
>>>>>> OK, you did not test it. Neither this, nor DTS. I stop review, please
>>>>>> test first.
>>>>>
>>>>> I ran both `checkpatch.pl` and `make dt_binding_check`. What in this
>>>>> patch makes you say I "did not test it", and what test or tests did I miss?
>>>>>
>>>>
>>>> ... and no, you did not. If you tested, you would easily see error:
>>>> 	clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short
>>>>
>>>> When you receive comment from reviewer, please investigate thoroughly
>>>> what could get wrong. Don't answer just to get rid of reviewer. It's
>>>> fine to make mistakes, but if reviewer points to issue and you
>>>> immediately respond "no issue", that's waste of my time.
>>>
>>> To clarify: "no issue" response is waste of my time. If you responded
>>> "oh, I see the error, but I don't know how to fix it", it would be ok, I
>>> can clarify and help in this.
>>
>> I apologize if I gave you this impression. I tried to follow the testing
>> process, it did not turn out as expected. Obviously, I missed something.
>> I tried to ask what I missed, and in order for that question to make
>> sense, I need to describe what I tried.
>>
>> It turns out what I missed was "make check_dtbs". I only found that out
>> after an automated email from Rob describing some troubleshooting steps.
> 
> No, the dt_binding_check fails. You don't need to go to dtbs_check even,
> because the binding already has a failure.
> 
>>
>> If I may have a few sentences to rant, I see the dt-schema as a hurdle
>> to making an otherwise useful change. I am told I can ask for help when
>> I get stuck, yet I manage to insult the maintainer by aking for help. I
>> find this very intimidating.
> 
> I don't feel insulted but I feel my time is wasted if I tell you to test
> your binding and you immediately within few minutes respond "I tested",
> but then:
> 1. Bot confirms it was not tested,
> 2. I apply your patch and test it and see the failure.

Thank you for double checking, and I am sorry this escalated in this 
manner. I believed you the first time that something is wrong, and I had 
a hard time figuring out what.

I am now able to repro the errors, and the below changes appear to work. 
Is that sufficient?

    clocks:
      minItems: 3
      maxItems: 5

    clock-names:
      minItems: 3
      items:
        - ... (5 clock names here)

For ipq6018/ipq8074:

       properties:
         clocks:
           maxItems: 3
         clock-names:
           maxItems: 3

For ipq9574:

       properties:
         clocks:
           minItems: 5
         clock-names:
           minItems: 5

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Krzysztof Kozlowski]

On 11/04/2024 19:24, mr.nuke.me@gmail.com wrote:
> 
> I am now able to repro the errors, and the below changes appear to work. 
> Is that sufficient?
> 
>     clocks:
>       minItems: 3
>       maxItems: 5
> 
>     clock-names:
>       minItems: 3
>       items:
>         - ... (5 clock names here)
> 
> For ipq6018/ipq8074:
> 
>        properties:
>          clocks:
>            maxItems: 3
>          clock-names:
>            maxItems: 3
> 
> For ipq9574:
> 
>        properties:
>          clocks:
>            minItems: 5
>          clock-names:
>            minItems: 5


Yes, looks good.

Best regards,
Krzysztof

Re: [PATCH v2 5/7] dt-bindings: phy: qcom,ipq8074-qmp-pcie: add ipq9574 gen3x2 PHY

[Rob Herring]

On Tue, 09 Apr 2024 14:08:31 -0500, Alexandru Gagniuc wrote:
> The IPQ9574 gen3x2 PHY is very similar to IPQ6018. It requires two
> extra clocks named "anoc" and "snoc". Document this, and add a
> new compatible string for this PHY.
> 
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  .../phy/qcom,ipq8074-qmp-pcie-phy.yaml        | 31 ++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/phy/qcom,ipq8074-qmp-pcie-phy.example.dtb: phy@84000: clock-names: ['aux', 'cfg_ahb', 'pipe'] is too short
	from schema $id: http://devicetree.org/schemas/phy/qcom,ipq8074-qmp-pcie-phy.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240409190833.3485824-6-mr.nuke.me@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

[PATCH v2 7/7] arm64: dts: qcom: ipq9574: add PCIe2 nodes

[Alexandru Gagniuc]

On ipq9574, there are 4 PCIe controllers. Describe the pcie2 node, and
its PHY in devicetree.

Only pcie2 is described, because only hardware using that controller
was available for testing.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 94 ++++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 7f2e5cbf3bbb..61c518f2a05a 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -300,7 +300,7 @@ gcc: clock-controller@1800000 {
 				 <0>,
 				 <0>,
 				 <0>,
-				 <0>,
+				 <&pcie2_phy>,
 				 <0>,
 				 <0>;
 			#clock-cells = <1>;
@@ -745,6 +745,98 @@ frame@b128000 {
 				status = "disabled";
 			};
 		};
+
+		pcie2_phy: phy@8c000 {
+			compatible = "qcom,ipq9574-qmp-gen3x2-pcie-phy";
+			reg = <0x0008c000 0x14f4>;
+
+			clocks = <&gcc GCC_PCIE2_AUX_CLK>,
+				 <&gcc GCC_PCIE2_AHB_CLK>,
+				 <&gcc GCC_PCIE2_PIPE_CLK>,
+				 <&gcc GCC_ANOC_PCIE2_2LANE_M_CLK>,
+				 <&gcc GCC_SNOC_PCIE2_2LANE_S_CLK>;
+			clock-names = "aux",
+				      "cfg_ahb",
+				      "pipe",
+				      "anoc",
+				      "snoc";
+
+			clock-output-names = "pcie_phy2_pipe_clk";
+			#clock-cells = <0>;
+			#phy-cells = <0>;
+
+			resets = <&gcc GCC_PCIE2_PHY_BCR>,
+				 <&gcc GCC_PCIE2PHY_PHY_BCR>;
+			reset-names = "phy",
+				      "common";
+			status = "disabled";
+		};
+
+		pcie2: pcie@20000000 {
+			compatible = "qcom,pcie-ipq9574";
+			reg = <0x20000000 0xf1d>,
+			      <0x20000f20 0xa8>,
+			      <0x20001000 0x1000>,
+			      <0x00088000 0x4000>,
+			      <0x20100000 0x1000>;
+			reg-names = "dbi", "elbi", "atu", "parf", "config";
+
+			ranges = <0x81000000 0x0 0x20200000 0x20200000 0x0 0x00100000>,	/* I/O */
+				 <0x82000000 0x0 0x20300000 0x20300000 0x0 0x07d00000>;	/* MEM */
+
+			device_type = "pci";
+			linux,pci-domain = <3>;
+			bus-range = <0x00 0xff>;
+			num-lanes = <2>;
+			max-link-speed = <3>;
+			#address-cells = <3>;
+			#size-cells = <2>;
+
+			phys = <&pcie2_phy>;
+			phy-names = "pciephy";
+
+			interrupts = <GIC_SPI 125 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "msi";
+			msi-parent = <&v2m0>;
+
+			#interrupt-cells = <1>;
+			interrupt-map-mask = <0 0 0 0x7>;
+			interrupt-map = <0 0 0 1 &intc 0 0 164
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_a */
+					<0 0 0 2 &intc 0 0 165
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_b */
+					<0 0 0 3 &intc 0 0 186
+					 IRQ_TYPE_LEVEL_HIGH>, /* int_c */
+					<0 0 0 4 &intc 0 0 187
+					 IRQ_TYPE_LEVEL_HIGH>; /* int_d */
+
+			clocks = <&gcc GCC_PCIE2_AXI_M_CLK>,
+				 <&gcc GCC_PCIE2_AXI_S_CLK>,
+				 <&gcc GCC_PCIE2_AXI_S_BRIDGE_CLK>,
+				 <&gcc GCC_PCIE2_RCHNG_CLK>;
+			clock-names = "axi_m",
+				      "axi_s",
+				      "axi_bridge",
+				      "rchng";
+
+			resets = <&gcc GCC_PCIE2_PIPE_ARES>,
+				 <&gcc GCC_PCIE2_CORE_STICKY_ARES>,
+				 <&gcc GCC_PCIE2_AXI_S_STICKY_ARES>,
+				 <&gcc GCC_PCIE2_AXI_S_ARES>,
+				 <&gcc GCC_PCIE2_AXI_M_STICKY_ARES>,
+				 <&gcc GCC_PCIE2_AXI_M_ARES>,
+				 <&gcc GCC_PCIE2_AUX_ARES>,
+				 <&gcc GCC_PCIE2_AHB_ARES>;
+			reset-names = "pipe",
+				      "sticky",
+				      "axi_s_sticky",
+				      "axi_s",
+				      "axi_m_sticky",
+				      "axi_m",
+				      "aux",
+				      "ahb";
+			status = "disabled";
+		};
 	};
 
 	thermal-zones {
-- 
2.40.1