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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
>> ---
>> .../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
On 09/04/2024 22:19, [email protected] 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
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 <[email protected]>
> ---
> .../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/[email protected]
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.
On 09/04/2024 22:19, [email protected] 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
On 10/04/2024 08:59, Krzysztof Kozlowski wrote:
> On 09/04/2024 22:19, [email protected] 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
On 4/10/24 02:02, Krzysztof Kozlowski wrote:
> On 10/04/2024 08:59, Krzysztof Kozlowski wrote:
>> On 09/04/2024 22:19, [email protected] 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
On 10/04/2024 18:29, [email protected] 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, [email protected] 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
On 4/10/24 14:36, Krzysztof Kozlowski wrote:
> On 10/04/2024 18:29, [email protected] 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, [email protected] 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
On 11/04/2024 19:24, [email protected] 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