diff mbox series

[v2,1/2] dt-bindings: dwc2: Add bindings for new Ingenic SoCs.

Message ID 1649788201-87620-2-git-send-email-zhouyanjie@wanyeetech.com
State Superseded
Headers show
Series Add OTG support for Ingenic SoCs. | expand

Commit Message

Zhou Yanjie April 12, 2022, 6:30 p.m. UTC
Add the dwc2 bindings for the JZ4775 SoC, the JZ4780 SoC, the X1000
SoC, the X1600 SoC, the X1700 SoC, the X1830 SoC, and the X2000 SoC
from Ingenic.

Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
Acked-by: Rob Herring <robh@kernel.org>
---

Notes:
    v1->v2:
    Add Rob Herring's Acked-by.

 Documentation/devicetree/bindings/usb/dwc2.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

H. Nikolaus Schaller April 13, 2022, 7:22 a.m. UTC | #1
Hi,

> Am 12.04.2022 um 20:30 schrieb 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>:
> 
> Add the dwc2 bindings for the JZ4775 SoC, the JZ4780 SoC, the X1000
> SoC, the X1600 SoC, the X1700 SoC, the X1830 SoC, and the X2000 SoC
> from Ingenic.
> 
> Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> 
> Notes:
>    v1->v2:
>    Add Rob Herring's Acked-by.
> 
> Documentation/devicetree/bindings/usb/dwc2.yaml | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml b/Documentation/devicetree/bindings/usb/dwc2.yaml
> index 4cebce6..c6e8c0b 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.yaml
> +++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
> @@ -17,6 +17,13 @@ properties:
>     oneOf:
>       - const: brcm,bcm2835-usb
>       - const: hisilicon,hi6220-usb
> +      - const: ingenic,jz4775-otg
> +      - const: ingenic,jz4780-otg
> +      - const: ingenic,x1000-otg
> +      - const: ingenic,x1600-otg
> +      - const: ingenic,x1700-otg
> +      - const: ingenic,x1830-otg
> +      - const: ingenic,x2000-otg

I have merged it with my recently proposed removal of
ingenic,jz4780-otg in jz4780.dtsi but there was no dtbscheck
complaint about missing snps,dwc2.

So I think should it be:

      - items:
          - enum:
              - const: ingenic,jz4775-otg
              - const: ingenic,jz4780-otg
              - const: ingenic,x1000-otg
              - const: ingenic,x1600-otg
              - const: ingenic,x1700-otg
              - const: ingenic,x1830-otg
              - const: ingenic,x2000-otg
          - const: snps,dwc2

similar to the entry for amlogic?

>       - items:
>           - const: rockchip,rk3066-usb
>           - const: snps,dwc2
> -- 
> 2.7.4
> 

BR and thanks,
Nikolaus
H. Nikolaus Schaller April 13, 2022, 7:30 p.m. UTC | #2
Hi,


> Am 13.04.2022 um 20:55 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
> 
> Hi Nikolaus,
> 
> On 2022/4/13 下午3:22, H. Nikolaus Schaller wrote:
>> Hi,
>> 
>> 
>>> Am 12.04.2022 um 20:30 schrieb 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>> :
>>> 
>>> Add the dwc2 bindings for the JZ4775 SoC, the JZ4780 SoC, the X1000
>>> SoC, the X1600 SoC, the X1700 SoC, the X1830 SoC, and the X2000 SoC
>>> from Ingenic.
>>> 
>>> Signed-off-by: 周琰杰 (Zhou Yanjie) 
>>> <zhouyanjie@wanyeetech.com>
>>> 
>>> Acked-by: Rob Herring 
>>> <robh@kernel.org>
>>> 
>>> ---
>>> 
>>> Notes:
>>>    v1->v2:
>>>    Add Rob Herring's Acked-by.
>>> 
>>> Documentation/devicetree/bindings/usb/dwc2.yaml | 7 +++++++
>>> 1 file changed, 7 insertions(+)
>>> 
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml b/Documentation/devicetree/bindings/usb/dwc2.yaml
>>> index 4cebce6..c6e8c0b 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc2.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
>>> @@ -17,6 +17,13 @@ properties:
>>>     oneOf:
>>>       - const: brcm,bcm2835-usb
>>>       - const: hisilicon,hi6220-usb
>>> +      - const: ingenic,jz4775-otg
>>> +      - const: ingenic,jz4780-otg
>>> +      - const: ingenic,x1000-otg
>>> +      - const: ingenic,x1600-otg
>>> +      - const: ingenic,x1700-otg
>>> +      - const: ingenic,x1830-otg
>>> +      - const: ingenic,x2000-otg
>>> 
>> I have merged it with my recently proposed removal of
>> ingenic,jz4780-otg in jz4780.dtsi but there was no dtbscheck
>> complaint about missing snps,dwc2.
>> 
>> So I think should it be:
>> 
>>       - items:
>>           - enum:
>>               - const: ingenic,jz4775-otg
>>               - const: ingenic,jz4780-otg
>>               - const: ingenic,x1000-otg
>>               - const: ingenic,x1600-otg
>>               - const: ingenic,x1700-otg
>>               - const: ingenic,x1830-otg
>>               - const: ingenic,x2000-otg
>> 

PS: the const: above should be removed (I hadn't run it through the compiler).

>>           - const: snps,dwc2

here it is needed.

>> 
>> similar to the entry for amlogic?
>> 
> 
> 
> Or we can just remove the "snps,dwc2" from jz4780.dtsi?

Well, my recent proposal to fix dtbscheck was the other way round:
remove "ingenic,jz4780-otg" from jz4780.dtsi and leave it out here.

> I'm not too sure, but since we already have a dedicated "ingenic, jz4780-otg", it seems "snps,dwc2" is redundant.

As far as I see there is no driver specialization compatible to
"ingenic,jz4780-otg". `grep ingenic,jz4780-otg *` only shows the .dtsi (and the new .yaml).

So we need "snps,dwc2" to get any driver match and I thought the "ingenic,jz4780-otg" is redundant.

But maintainers convinced me to keep it as a dummy compatible in the .dtsi for potential future
specialization (which does not exist and seems not to be necessary). Unless I can convince them 
that this is never ever needed. Which is beyond my knowledge and almost everyone.

So we can't remove the "snps,dwc2" here.

Well, we can with more work elsewhere.
You have to extend the dwc2_of_match_table to include all ingenic devices.

Therefore we now know 3 potential solutions:
a) remove "ingenic,jz4780-otg" from jz4780.dtsi (my proposal)
b) add "ingenic,jz4780-otg" to dwc2.yaml together with "snps,dwc2" (your proposal + my suggestion here)
c) add only "ingenic,jz4780-otg" to dwc2.yaml and extend the match table in drivers//usb/dwc2/params.c (new proposals)

From consistency point of view I think variant b) is the right one. a) was rejected and c) only adds redundant code.

I am open to anything as long as the dtbscheck doesn't complain any more.

BR an thanks,
Nikolaus
Zhou Yanjie April 13, 2022, 7:42 p.m. UTC | #3
Hi Nikolaus,

On 2022/4/14 上午3:30, H. Nikolaus Schaller wrote:
> Hi,
>
>
>> Am 13.04.2022 um 20:55 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
>>
>> Hi Nikolaus,
>>
>> On 2022/4/13 下午3:22, H. Nikolaus Schaller wrote:
>>> Hi,
>>>
>>>
>>>> Am 12.04.2022 um 20:30 schrieb 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com>
>>>> :
>>>>
>>>> Add the dwc2 bindings for the JZ4775 SoC, the JZ4780 SoC, the X1000
>>>> SoC, the X1600 SoC, the X1700 SoC, the X1830 SoC, and the X2000 SoC
>>>> from Ingenic.
>>>>
>>>> Signed-off-by: 周琰杰 (Zhou Yanjie)
>>>> <zhouyanjie@wanyeetech.com>
>>>>
>>>> Acked-by: Rob Herring
>>>> <robh@kernel.org>
>>>>
>>>> ---
>>>>
>>>> Notes:
>>>>     v1->v2:
>>>>     Add Rob Herring's Acked-by.
>>>>
>>>> Documentation/devicetree/bindings/usb/dwc2.yaml | 7 +++++++
>>>> 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml b/Documentation/devicetree/bindings/usb/dwc2.yaml
>>>> index 4cebce6..c6e8c0b 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc2.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
>>>> @@ -17,6 +17,13 @@ properties:
>>>>      oneOf:
>>>>        - const: brcm,bcm2835-usb
>>>>        - const: hisilicon,hi6220-usb
>>>> +      - const: ingenic,jz4775-otg
>>>> +      - const: ingenic,jz4780-otg
>>>> +      - const: ingenic,x1000-otg
>>>> +      - const: ingenic,x1600-otg
>>>> +      - const: ingenic,x1700-otg
>>>> +      - const: ingenic,x1830-otg
>>>> +      - const: ingenic,x2000-otg
>>>>
>>> I have merged it with my recently proposed removal of
>>> ingenic,jz4780-otg in jz4780.dtsi but there was no dtbscheck
>>> complaint about missing snps,dwc2.
>>>
>>> So I think should it be:
>>>
>>>        - items:
>>>            - enum:
>>>                - const: ingenic,jz4775-otg
>>>                - const: ingenic,jz4780-otg
>>>                - const: ingenic,x1000-otg
>>>                - const: ingenic,x1600-otg
>>>                - const: ingenic,x1700-otg
>>>                - const: ingenic,x1830-otg
>>>                - const: ingenic,x2000-otg
>>>
> PS: the const: above should be removed (I hadn't run it through the compiler).
>
>>>            - const: snps,dwc2
> here it is needed.
>
>>> similar to the entry for amlogic?
>>>
>>
>> Or we can just remove the "snps,dwc2" from jz4780.dtsi?
> Well, my recent proposal to fix dtbscheck was the other way round:
> remove "ingenic,jz4780-otg" from jz4780.dtsi and leave it out here.
>
>> I'm not too sure, but since we already have a dedicated "ingenic, jz4780-otg", it seems "snps,dwc2" is redundant.
> As far as I see there is no driver specialization compatible to
> "ingenic,jz4780-otg". `grep ingenic,jz4780-otg *` only shows the .dtsi (and the new .yaml).
>
> So we need "snps,dwc2" to get any driver match and I thought the "ingenic,jz4780-otg" is redundant.
>
> But maintainers convinced me to keep it as a dummy compatible in the .dtsi for potential future
> specialization (which does not exist and seems not to be necessary). Unless I can convince them
> that this is never ever needed. Which is beyond my knowledge and almost everyone.
>
> So we can't remove the "snps,dwc2" here.
>
> Well, we can with more work elsewhere.
> You have to extend the dwc2_of_match_table to include all ingenic devices.
>
> Therefore we now know 3 potential solutions:
> a) remove "ingenic,jz4780-otg" from jz4780.dtsi (my proposal)
> b) add "ingenic,jz4780-otg" to dwc2.yaml together with "snps,dwc2" (your proposal + my suggestion here)
> c) add only "ingenic,jz4780-otg" to dwc2.yaml and extend the match table in drivers//usb/dwc2/params.c (new proposals)
>
>  From consistency point of view I think variant b) is the right one. a) was rejected and c) only adds redundant code.


You are probably right, I forgot that the OTG driver only exists in my local
repository and is not merged into the mainline. If we remove "snps,dwc2",
the OTG of the JZ4780 in the current version will not work properly.

I will send v3 tomorrow.


Thanks and best regards!


> I am open to anything as long as the dtbscheck doesn't complain any more.
>
> BR an thanks,
> Nikolaus
Krzysztof Kozlowski April 14, 2022, 7:32 a.m. UTC | #4
On 13/04/2022 21:30, H. Nikolaus Schaller wrote:
> So we need "snps,dwc2" to get any driver match and I thought the "ingenic,jz4780-otg" is redundant.
> 
> But maintainers convinced me to keep it as a dummy compatible in the .dtsi for potential future
> specialization (which does not exist and seems not to be necessary). 

Isn't exactly the next patch 2/2 using such specialization?

> Unless I can convince them 
> that this is never ever needed. Which is beyond my knowledge and almost everyone.
> 
> So we can't remove the "snps,dwc2" here.
> 
> Well, we can with more work elsewhere.
> You have to extend the dwc2_of_match_table to include all ingenic devices.
> 
> Therefore we now know 3 potential solutions:
> a) remove "ingenic,jz4780-otg" from jz4780.dtsi (my proposal)
> b) add "ingenic,jz4780-otg" to dwc2.yaml together with "snps,dwc2" (your proposal + my suggestion here)
> c) add only "ingenic,jz4780-otg" to dwc2.yaml and extend the match table in drivers//usb/dwc2/params.c (new proposals)
> 
> From consistency point of view I think variant b) is the right one. a) was rejected and c) only adds redundant code.

c) was already proposed by Zhou, so if you think the code is not correct
(the params for jz4780) maybe nack it there, so we will know that driver
needs fixes.

Best regards,
Krzysztof
H. Nikolaus Schaller April 14, 2022, 10 a.m. UTC | #5
> Am 14.04.2022 um 09:32 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
> 
> On 13/04/2022 21:30, H. Nikolaus Schaller wrote:
>> So we need "snps,dwc2" to get any driver match and I thought the "ingenic,jz4780-otg" is redundant.
>> 
>> But maintainers convinced me to keep it as a dummy compatible in the .dtsi for potential future
>> specialization (which does not exist and seems not to be necessary). 
> 
> Isn't exactly the next patch 2/2 using such specialization?
> 
>> Unless I can convince them 
>> that this is never ever needed. Which is beyond my knowledge and almost everyone.
>> 
>> So we can't remove the "snps,dwc2" here.
>> 
>> Well, we can with more work elsewhere.
>> You have to extend the dwc2_of_match_table to include all ingenic devices.
>> 
>> Therefore we now know 3 potential solutions:
>> a) remove "ingenic,jz4780-otg" from jz4780.dtsi (my proposal)
>> b) add "ingenic,jz4780-otg" to dwc2.yaml together with "snps,dwc2" (your proposal + my suggestion here)
>> c) add only "ingenic,jz4780-otg" to dwc2.yaml and extend the match table in drivers//usb/dwc2/params.c (new proposals)
>> 
>> From consistency point of view I think variant b) is the right one. a) was rejected and c) only adds redundant code.
> 
> c) was already proposed by Zhou, so if you think the code is not correct
> (the params for jz4780) maybe nack it there, so we will know that driver
> needs fixes.

Ah, ok. Now I see. I was just focussed on this patch and related dtbscheck
messages and did not read patch 2/2.

Yes, looking at both, they are variant c). Sorry that I didn't see it earlier.

As said: I am open to anything as long as the dtbscheck doesn't complain any more.

BR and sorry for the confusion,
Nikolaus
Zhou Yanjie April 14, 2022, 5:11 p.m. UTC | #6
Hi,

On 2022/4/14 下午6:00, H. Nikolaus Schaller wrote:
>
>> Am 14.04.2022 um 09:32 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>>
>> On 13/04/2022 21:30, H. Nikolaus Schaller wrote:
>>> So we need "snps,dwc2" to get any driver match and I thought the "ingenic,jz4780-otg" is redundant.
>>>
>>> But maintainers convinced me to keep it as a dummy compatible in the .dtsi for potential future
>>> specialization (which does not exist and seems not to be necessary).
>> Isn't exactly the next patch 2/2 using such specialization?
>>
>>> Unless I can convince them
>>> that this is never ever needed. Which is beyond my knowledge and almost everyone.
>>>
>>> So we can't remove the "snps,dwc2" here.
>>>
>>> Well, we can with more work elsewhere.
>>> You have to extend the dwc2_of_match_table to include all ingenic devices.
>>>
>>> Therefore we now know 3 potential solutions:
>>> a) remove "ingenic,jz4780-otg" from jz4780.dtsi (my proposal)
>>> b) add "ingenic,jz4780-otg" to dwc2.yaml together with "snps,dwc2" (your proposal + my suggestion here)
>>> c) add only "ingenic,jz4780-otg" to dwc2.yaml and extend the match table in drivers//usb/dwc2/params.c (new proposals)
>>>
>>>  From consistency point of view I think variant b) is the right one. a) was rejected and c) only adds redundant code.
>> c) was already proposed by Zhou, so if you think the code is not correct
>> (the params for jz4780) maybe nack it there, so we will know that driver
>> needs fixes.
> Ah, ok. Now I see. I was just focussed on this patch and related dtbscheck
> messages and did not read patch 2/2.
>
> Yes, looking at both, they are variant c). Sorry that I didn't see it earlier.

It looks like we need a [3/3] to remove "snps,dwc2", which not only solves
the dtbscheck complaining problem, but also doesn't affect normal use after
removing "snps,dwc2".


>
> As said: I am open to anything as long as the dtbscheck doesn't complain any more.
>
> BR and sorry for the confusion,
> Nikolaus
H. Nikolaus Schaller April 14, 2022, 5:18 p.m. UTC | #7
Hi,

> Am 14.04.2022 um 19:11 schrieb Zhou Yanjie <zhouyanjie@wanyeetech.com>:
> 
> Hi,
> 
> On 2022/4/14 下午6:00, H. Nikolaus Schaller wrote:
>> 
>>> Am 14.04.2022 um 09:32 schrieb Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>:
>>> 
>>> On 13/04/2022 21:30, H. Nikolaus Schaller wrote:
>>>> So we need "snps,dwc2" to get any driver match and I thought the "ingenic,jz4780-otg" is redundant.
>>>> 
>>>> But maintainers convinced me to keep it as a dummy compatible in the .dtsi for potential future
>>>> specialization (which does not exist and seems not to be necessary).
>>> Isn't exactly the next patch 2/2 using such specialization?
>>> 
>>>> Unless I can convince them
>>>> that this is never ever needed. Which is beyond my knowledge and almost everyone.
>>>> 
>>>> So we can't remove the "snps,dwc2" here.
>>>> 
>>>> Well, we can with more work elsewhere.
>>>> You have to extend the dwc2_of_match_table to include all ingenic devices.
>>>> 
>>>> Therefore we now know 3 potential solutions:
>>>> a) remove "ingenic,jz4780-otg" from jz4780.dtsi (my proposal)
>>>> b) add "ingenic,jz4780-otg" to dwc2.yaml together with "snps,dwc2" (your proposal + my suggestion here)
>>>> c) add only "ingenic,jz4780-otg" to dwc2.yaml and extend the match table in drivers//usb/dwc2/params.c (new proposals)
>>>> 
>>>> From consistency point of view I think variant b) is the right one. a) was rejected and c) only adds redundant code.
>>> c) was already proposed by Zhou, so if you think the code is not correct
>>> (the params for jz4780) maybe nack it there, so we will know that driver
>>> needs fixes.
>> Ah, ok. Now I see. I was just focussed on this patch and related dtbscheck
>> messages and did not read patch 2/2.
>> 
>> Yes, looking at both, they are variant c). Sorry that I didn't see it earlier.
> 
> It looks like we need a [3/3] to remove "snps,dwc2", which not only solves
> the dtbscheck complaining problem, but also doesn't affect normal use after
> removing "snps,dwc2".

Yes, that seems to be the right thing to do.

BR and thanks,
Nikolaus
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/dwc2.yaml b/Documentation/devicetree/bindings/usb/dwc2.yaml
index 4cebce6..c6e8c0b 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.yaml
+++ b/Documentation/devicetree/bindings/usb/dwc2.yaml
@@ -17,6 +17,13 @@  properties:
     oneOf:
       - const: brcm,bcm2835-usb
       - const: hisilicon,hi6220-usb
+      - const: ingenic,jz4775-otg
+      - const: ingenic,jz4780-otg
+      - const: ingenic,x1000-otg
+      - const: ingenic,x1600-otg
+      - const: ingenic,x1700-otg
+      - const: ingenic,x1830-otg
+      - const: ingenic,x2000-otg
       - items:
           - const: rockchip,rk3066-usb
           - const: snps,dwc2