diff mbox series

[v11,02/13] dt-bindings: usb: Add bindings for multiport properties on DWC3 controller

Message ID 20230828133033.11988-3-quic_kriskura@quicinc.com
State Accepted
Commit eb3f1d9e42b1499152442e97b51bc1bcfee29d71
Headers show
Series Add multiport support for DWC3 controllers | expand

Commit Message

Krishna Kurapati Aug. 28, 2023, 1:30 p.m. UTC
Add bindings to indicate properties required to support multiport
on Synopsys DWC3 controller.

Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/usb/snps,dwc3.yaml          | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Johan Hovold Nov. 10, 2023, 1:28 p.m. UTC | #1
On Mon, Aug 28, 2023 at 07:00:22PM +0530, Krishna Kurapati wrote:
> Add bindings to indicate properties required to support multiport
> on Synopsys DWC3 controller.
> 
> Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml          | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index a696f23730d3..5bc941355b43 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -85,15 +85,16 @@ properties:
>  
>    phys:
>      minItems: 1
> -    maxItems: 2
> +    maxItems: 8
>  
>    phy-names:
>      minItems: 1
> -    maxItems: 2
> -    items:
> -      enum:
> -        - usb2-phy
> -        - usb3-phy
> +    maxItems: 8
> +    oneOf:
> +      - items:
> +          enum: [ usb2-phy, usb3-phy ]
> +      - items:
> +          pattern: "^usb[23]-port[0-3]$"

Shouldn't this just be

	pattern: "^usb[23]-[0-3]$"

so that it matches the names that are used by the nvidia bindings?

We already have some inconsistency in that Amlogic uses a variant based
on the legacy names that needlessly includes "phy" in the names:

	const: usb2-phy0
	const: usb2-phy1
	const: usb3-phy0
	...

I don't think we should be introducing a third naming scheme here so I
suggest just following the nvidia bindings.

Johan
Krishna Kurapati Nov. 11, 2023, 8:30 a.m. UTC | #2
On 11/10/2023 6:58 PM, Johan Hovold wrote:
> On Mon, Aug 28, 2023 at 07:00:22PM +0530, Krishna Kurapati wrote:
>> Add bindings to indicate properties required to support multiport
>> on Synopsys DWC3 controller.
>>
>> Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>   .../devicetree/bindings/usb/snps,dwc3.yaml          | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index a696f23730d3..5bc941355b43 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -85,15 +85,16 @@ properties:
>>   
>>     phys:
>>       minItems: 1
>> -    maxItems: 2
>> +    maxItems: 8
>>   
>>     phy-names:
>>       minItems: 1
>> -    maxItems: 2
>> -    items:
>> -      enum:
>> -        - usb2-phy
>> -        - usb3-phy
>> +    maxItems: 8
>> +    oneOf:
>> +      - items:
>> +          enum: [ usb2-phy, usb3-phy ]
>> +      - items:
>> +          pattern: "^usb[23]-port[0-3]$"
> 
> Shouldn't this just be
> 
> 	pattern: "^usb[23]-[0-3]$"
> 
> so that it matches the names that are used by the nvidia bindings?
> 
> We already have some inconsistency in that Amlogic uses a variant based
> on the legacy names that needlessly includes "phy" in the names:
> 
> 	const: usb2-phy0
> 	const: usb2-phy1
> 	const: usb3-phy0
> 	...
> 
> I don't think we should be introducing a third naming scheme here so I
> suggest just following the nvidia bindings.
> 
In that case, why don't we use  "^usb[23]-phy[0-3]$". I think its close 
to what we have on dwc3 core already today (usb2-phy/usb3-phy).

Regards,
Krishna,
Krishna Kurapati Nov. 11, 2023, 9:47 a.m. UTC | #3
On 11/11/2023 2:00 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 11/10/2023 6:58 PM, Johan Hovold wrote:
>> On Mon, Aug 28, 2023 at 07:00:22PM +0530, Krishna Kurapati wrote:
>>> Add bindings to indicate properties required to support multiport
>>> on Synopsys DWC3 controller.
>>>
>>> Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>> ---
>>>   .../devicetree/bindings/usb/snps,dwc3.yaml          | 13 +++++++------
>>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml 
>>> b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> index a696f23730d3..5bc941355b43 100644
>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> @@ -85,15 +85,16 @@ properties:
>>>     phys:
>>>       minItems: 1
>>> -    maxItems: 2
>>> +    maxItems: 8
>>>     phy-names:
>>>       minItems: 1
>>> -    maxItems: 2
>>> -    items:
>>> -      enum:
>>> -        - usb2-phy
>>> -        - usb3-phy
>>> +    maxItems: 8
>>> +    oneOf:
>>> +      - items:
>>> +          enum: [ usb2-phy, usb3-phy ]
>>> +      - items:
>>> +          pattern: "^usb[23]-port[0-3]$"
>>
>> Shouldn't this just be
>>
>>     pattern: "^usb[23]-[0-3]$"
>>
>> so that it matches the names that are used by the nvidia bindings?
>>
>> We already have some inconsistency in that Amlogic uses a variant based
>> on the legacy names that needlessly includes "phy" in the names:
>>
>>     const: usb2-phy0
>>     const: usb2-phy1
>>     const: usb3-phy0
>>     ...
>>
>> I don't think we should be introducing a third naming scheme here so I
>> suggest just following the nvidia bindings.
>> > In that case, why don't we use  "^usb[23]-phy[0-3]$". I think its close
> to what we have on dwc3 core already today (usb2-phy/usb3-phy).
> 
I mean, it isn't needless. It is a phy and shouldn't the binding suggest 
that and include "-phy" in the name ?

Regards,
Krishna,
Johan Hovold Nov. 11, 2023, 10:55 a.m. UTC | #4
On Sat, Nov 11, 2023 at 03:17:40PM +0530, Krishna Kurapati PSSNV wrote:
> On 11/11/2023 2:00 PM, Krishna Kurapati PSSNV wrote:
> > On 11/10/2023 6:58 PM, Johan Hovold wrote:

> >>>     phy-names:
> >>>       minItems: 1
> >>> -    maxItems: 2
> >>> -    items:
> >>> -      enum:
> >>> -        - usb2-phy
> >>> -        - usb3-phy
> >>> +    maxItems: 8
> >>> +    oneOf:
> >>> +      - items:
> >>> +          enum: [ usb2-phy, usb3-phy ]
> >>> +      - items:
> >>> +          pattern: "^usb[23]-port[0-3]$"
> >>
> >> Shouldn't this just be
> >>
> >>     pattern: "^usb[23]-[0-3]$"
> >>
> >> so that it matches the names that are used by the nvidia bindings?
> >>
> >> We already have some inconsistency in that Amlogic uses a variant based
> >> on the legacy names that needlessly includes "phy" in the names:
> >>
> >>     const: usb2-phy0
> >>     const: usb2-phy1
> >>     const: usb3-phy0
> >>     ...
> >>
> >> I don't think we should be introducing a third naming scheme here so I
> >> suggest just following the nvidia bindings.

> >> > In that case, why don't we use  "^usb[23]-phy[0-3]$". I think its close
> > to what we have on dwc3 core already today (usb2-phy/usb3-phy).
>
> I mean, it isn't needless. It is a phy and shouldn't the binding suggest 
> that and include "-phy" in the name ?

No, adding a '-phy' suffix to each name is unnecessary since the
property is called 'phy-names'.

This is also documented:

	For names used in {clock,dma,interrupt,reset}-names, do not add
	any suffix, e.g.: "tx" instead of "txirq" (for interrupt).

	https://docs.kernel.org/devicetree/bindings/writing-bindings.html

and we've already discussed this when I asked you to drop the likewise
redundant '_irq' suffix from the interrupt names.

Johan
Krishna Kurapati Nov. 11, 2023, 1:04 p.m. UTC | #5
On 11/11/2023 4:25 PM, Johan Hovold wrote:
> On Sat, Nov 11, 2023 at 03:17:40PM +0530, Krishna Kurapati PSSNV wrote:
>> On 11/11/2023 2:00 PM, Krishna Kurapati PSSNV wrote:
>>> On 11/10/2023 6:58 PM, Johan Hovold wrote:
> 
>>>>>      phy-names:
>>>>>        minItems: 1
>>>>> -    maxItems: 2
>>>>> -    items:
>>>>> -      enum:
>>>>> -        - usb2-phy
>>>>> -        - usb3-phy
>>>>> +    maxItems: 8
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          enum: [ usb2-phy, usb3-phy ]
>>>>> +      - items:
>>>>> +          pattern: "^usb[23]-port[0-3]$"
>>>>
>>>> Shouldn't this just be
>>>>
>>>>      pattern: "^usb[23]-[0-3]$"
>>>>
>>>> so that it matches the names that are used by the nvidia bindings?
>>>>
>>>> We already have some inconsistency in that Amlogic uses a variant based
>>>> on the legacy names that needlessly includes "phy" in the names:
>>>>
>>>>      const: usb2-phy0
>>>>      const: usb2-phy1
>>>>      const: usb3-phy0
>>>>      ...
>>>>
>>>> I don't think we should be introducing a third naming scheme here so I
>>>> suggest just following the nvidia bindings.
> 
>>>>> In that case, why don't we use  "^usb[23]-phy[0-3]$". I think its close
>>> to what we have on dwc3 core already today (usb2-phy/usb3-phy).
>>
>> I mean, it isn't needless. It is a phy and shouldn't the binding suggest
>> that and include "-phy" in the name ?
> 
> No, adding a '-phy' suffix to each name is unnecessary since the
> property is called 'phy-names'.
> 
> This is also documented:
> 
> 	For names used in {clock,dma,interrupt,reset}-names, do not add
> 	any suffix, e.g.: "tx" instead of "txirq" (for interrupt).
> 
> 	https://docs.kernel.org/devicetree/bindings/writing-bindings.html
> 

Thanks for the explanation.

> and we've already discussed this when I asked you to drop the likewise
> redundant '_irq' suffix from the interrupt names.

Yes, we did discuss this in irq context. But in this case I thought it 
was fine because we already have usb(2/3)-"phy" already present.

When pushing v14, will make this change to usb(2/3)-(0-3) and skip port/phy.

Regards,
Krishna,
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index a696f23730d3..5bc941355b43 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -85,15 +85,16 @@  properties:
 
   phys:
     minItems: 1
-    maxItems: 2
+    maxItems: 8
 
   phy-names:
     minItems: 1
-    maxItems: 2
-    items:
-      enum:
-        - usb2-phy
-        - usb3-phy
+    maxItems: 8
+    oneOf:
+      - items:
+          enum: [ usb2-phy, usb3-phy ]
+      - items:
+          pattern: "^usb[23]-port[0-3]$"
 
   power-domains:
     description: