diff mbox series

[v9,01/10] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport

Message ID 20230621043628.21485-2-quic_kriskura@quicinc.com
State New
Headers show
Series Add multiport support for DWC3 controllers | expand

Commit Message

Krishna Kurapati June 21, 2023, 4:36 a.m. UTC
Add the compatible string for SC8280 Multiport USB controller from
Qualcomm.

There are 4 power event irq interrupts supported by this controller
(one for each port of multiport). Added all the 4 as non-optional
interrupts for SC8280XP-MP

Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM
IRQ's related to 4 ports of SC8280XP Teritiary controller.

Also added ss phy irq for both SS Ports.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 .../devicetree/bindings/usb/qcom,dwc3.yaml    | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Rob Herring (Arm) June 23, 2023, 8:41 p.m. UTC | #1
On Wed, 21 Jun 2023 10:06:19 +0530, Krishna Kurapati wrote:
> Add the compatible string for SC8280 Multiport USB controller from
> Qualcomm.
> 
> There are 4 power event irq interrupts supported by this controller
> (one for each port of multiport). Added all the 4 as non-optional
> interrupts for SC8280XP-MP
> 
> Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM
> IRQ's related to 4 ports of SC8280XP Teritiary controller.
> 
> Also added ss phy irq for both SS Ports.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  .../devicetree/bindings/usb/qcom,dwc3.yaml    | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>
Johan Hovold June 27, 2023, 11:20 a.m. UTC | #2
On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote:
> Add the compatible string for SC8280 Multiport USB controller from
> Qualcomm.
> 
> There are 4 power event irq interrupts supported by this controller
> (one for each port of multiport). Added all the 4 as non-optional
> interrupts for SC8280XP-MP
> 
> Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM
> IRQ's related to 4 ports of SC8280XP Teritiary controller.
> 
> Also added ss phy irq for both SS Ports.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
 
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sc8280xp-dwc3-mp
> +    then:
> +      properties:
> +        interrupts:

You also need minItems 14 here.

> +          maxItems: 14
> +        interrupt-names:

And here, I think.

> +          items:
> +            - const: dp1_hs_phy_irq
> +            - const: dm1_hs_phy_irq
> +            - const: dp2_hs_phy_irq
> +            - const: dm2_hs_phy_irq
> +            - const: dp3_hs_phy_irq
> +            - const: dm4_hs_phy_irq
> +            - const: dp4_hs_phy_irq
> +            - const: dm4_hs_phy_irq
> +            - const: ss1_phy_irq
> +            - const: ss2_phy_irq
> +            - const: pwr_event_1
> +            - const: pwr_event_2
> +            - const: pwr_event_3
> +            - const: pwr_event_4

The naming here is inconsistent and interrupts should not have "_irq"
suffixes (even if some of the current ones do for historical reasons).

I believe these should be named

	pwr_event_1
	dp_hs_phy_1
	dm_hs_phy_1
	ss_phy_1

	pwr_event_2
	dp_hs_phy_2
	dm_hs_phy_2
	ss_phy_2

	pwr_event_3
	dp_hs_phy_3
	dm_hs_phy_3

	pwr_event_4
	dp_hs_phy_4
	dm_hs_phy_4

or similar and be grouped by port while using the the
qcom,sc8280xp-dwc ordering for the individual lines.

Side note: Please note how the above interrupt properties can also be
used to infer the number of HS and SS ports.

> +
>  additionalProperties: false
>  
>  examples:

Johan
Johan Hovold June 27, 2023, 3:38 p.m. UTC | #3
On Tue, Jun 27, 2023 at 01:20:59PM +0200, Johan Hovold wrote:
> On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote:

> > +          items:
> > +            - const: dp1_hs_phy_irq
> > +            - const: dm1_hs_phy_irq
> > +            - const: dp2_hs_phy_irq
> > +            - const: dm2_hs_phy_irq
> > +            - const: dp3_hs_phy_irq
> > +            - const: dm4_hs_phy_irq
> > +            - const: dp4_hs_phy_irq
> > +            - const: dm4_hs_phy_irq
> > +            - const: ss1_phy_irq
> > +            - const: ss2_phy_irq
> > +            - const: pwr_event_1
> > +            - const: pwr_event_2
> > +            - const: pwr_event_3
> > +            - const: pwr_event_4
> 
> The naming here is inconsistent and interrupts should not have "_irq"
> suffixes (even if some of the current ones do for historical reasons).
> 
> I believe these should be named
> 
> 	pwr_event_1
> 	dp_hs_phy_1
> 	dm_hs_phy_1
> 	ss_phy_1
> 
> 	pwr_event_2
> 	dp_hs_phy_2
> 	dm_hs_phy_2
> 	ss_phy_2
> 
> 	pwr_event_3
> 	dp_hs_phy_3
> 	dm_hs_phy_3
> 
> 	pwr_event_4
> 	dp_hs_phy_4
> 	dm_hs_phy_4
> 
> or similar and be grouped by port while using the the
> qcom,sc8280xp-dwc ordering for the individual lines.

Perhaps the ordering you suggested is fine too, but I'd probably move
the pwr_event ones first to match qcom,sc8280xp-dwc then, that is:

 	pwr_event_1
 	pwr_event_2
 	pwr_event_3
 	pwr_event_4
 	dp_hs_phy_1
 	dm_hs_phy_1
 	dp_hs_phy_2
 	dm_hs_phy_2
 	dp_hs_phy_3
 	dm_hs_phy_3
 	dp_hs_phy_4
 	dm_hs_phy_4
 	ss_phy_1
 	ss_phy_2

so we have them grouped as pwr_event followed by HS and with SS last.

> Side note: Please note how the above interrupt properties can also be
> used to infer the number of HS and SS ports.

Johan
Krishna Kurapati July 2, 2023, 7:11 p.m. UTC | #4
On 6/27/2023 9:08 PM, Johan Hovold wrote:
> On Tue, Jun 27, 2023 at 01:20:59PM +0200, Johan Hovold wrote:
>> On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote:
> 
>>> +          items:
>>> +            - const: dp1_hs_phy_irq
>>> +            - const: dm1_hs_phy_irq
>>> +            - const: dp2_hs_phy_irq
>>> +            - const: dm2_hs_phy_irq
>>> +            - const: dp3_hs_phy_irq
>>> +            - const: dm4_hs_phy_irq
>>> +            - const: dp4_hs_phy_irq
>>> +            - const: dm4_hs_phy_irq
>>> +            - const: ss1_phy_irq
>>> +            - const: ss2_phy_irq
>>> +            - const: pwr_event_1
>>> +            - const: pwr_event_2
>>> +            - const: pwr_event_3
>>> +            - const: pwr_event_4
>>
>> The naming here is inconsistent and interrupts should not have "_irq"
>> suffixes (even if some of the current ones do for historical reasons).
>>
>> I believe these should be named
>>
>> 	pwr_event_1
>> 	dp_hs_phy_1
>> 	dm_hs_phy_1
>> 	ss_phy_1
>>
>> 	pwr_event_2
>> 	dp_hs_phy_2
>> 	dm_hs_phy_2
>> 	ss_phy_2
>>
>> 	pwr_event_3
>> 	dp_hs_phy_3
>> 	dm_hs_phy_3
>>
>> 	pwr_event_4
>> 	dp_hs_phy_4
>> 	dm_hs_phy_4
>>
>> or similar and be grouped by port while using the the
>> qcom,sc8280xp-dwc ordering for the individual lines.
> 
> Perhaps the ordering you suggested is fine too, but I'd probably move
> the pwr_event ones first to match qcom,sc8280xp-dwc then, that is:
> 
>   	pwr_event_1
>   	pwr_event_2
>   	pwr_event_3
>   	pwr_event_4
>   	dp_hs_phy_1
>   	dm_hs_phy_1
>   	dp_hs_phy_2
>   	dm_hs_phy_2
>   	dp_hs_phy_3
>   	dm_hs_phy_3
>   	dp_hs_phy_4
>   	dm_hs_phy_4
>   	ss_phy_1
>   	ss_phy_2
> 
> so we have them grouped as pwr_event followed by HS and with SS last.
> 
>> Side note: Please note how the above interrupt properties can also be
>> used to infer the number of HS and SS ports.
> 
> Johan

Can't we just cleanup all at once later ? Might not be a good idea for 
some properties in the file to have _irq and for some to not have it. I 
will modify the order though.

Regards,
Krishna,
Johan Hovold July 21, 2023, 8:10 a.m. UTC | #5
On Mon, Jul 03, 2023 at 12:41:59AM +0530, Krishna Kurapati PSSNV wrote:
> On 6/27/2023 9:08 PM, Johan Hovold wrote:
> > On Tue, Jun 27, 2023 at 01:20:59PM +0200, Johan Hovold wrote:
> >> On Wed, Jun 21, 2023 at 10:06:19AM +0530, Krishna Kurapati wrote:
> > 
> >>> +          items:
> >>> +            - const: dp1_hs_phy_irq
> >>> +            - const: dm1_hs_phy_irq
> >>> +            - const: dp2_hs_phy_irq
> >>> +            - const: dm2_hs_phy_irq
> >>> +            - const: dp3_hs_phy_irq
> >>> +            - const: dm4_hs_phy_irq
> >>> +            - const: dp4_hs_phy_irq
> >>> +            - const: dm4_hs_phy_irq
> >>> +            - const: ss1_phy_irq
> >>> +            - const: ss2_phy_irq
> >>> +            - const: pwr_event_1
> >>> +            - const: pwr_event_2
> >>> +            - const: pwr_event_3
> >>> +            - const: pwr_event_4
> >>
> >> The naming here is inconsistent and interrupts should not have "_irq"
> >> suffixes (even if some of the current ones do for historical reasons).
> >>
> >> I believe these should be named
> >>
> >> 	pwr_event_1
> >> 	dp_hs_phy_1
> >> 	dm_hs_phy_1
> >> 	ss_phy_1
> >>
> >> 	pwr_event_2
> >> 	dp_hs_phy_2
> >> 	dm_hs_phy_2
> >> 	ss_phy_2
> >>
> >> 	pwr_event_3
> >> 	dp_hs_phy_3
> >> 	dm_hs_phy_3
> >>
> >> 	pwr_event_4
> >> 	dp_hs_phy_4
> >> 	dm_hs_phy_4
> >>
> >> or similar and be grouped by port while using the the
> >> qcom,sc8280xp-dwc ordering for the individual lines.
> > 
> > Perhaps the ordering you suggested is fine too, but I'd probably move
> > the pwr_event ones first to match qcom,sc8280xp-dwc then, that is:
> > 
> >   	pwr_event_1
> >   	pwr_event_2
> >   	pwr_event_3
> >   	pwr_event_4
> >   	dp_hs_phy_1
> >   	dm_hs_phy_1
> >   	dp_hs_phy_2
> >   	dm_hs_phy_2
> >   	dp_hs_phy_3
> >   	dm_hs_phy_3
> >   	dp_hs_phy_4
> >   	dm_hs_phy_4
> >   	ss_phy_1
> >   	ss_phy_2
> > 
> > so we have them grouped as pwr_event followed by HS and with SS last.
> > 
> >> Side note: Please note how the above interrupt properties can also be
> >> used to infer the number of HS and SS ports.

> Can't we just cleanup all at once later ? Might not be a good idea for 
> some properties in the file to have _irq and for some to not have it. I 
> will modify the order though.

No, DT bindings generally need to be as correct as possible from the
start as they form an ABI. So please drop the _irq suffix from all of
the new indexed names.

Johan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index d84281926f10..fb3988208b27 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -26,6 +26,7 @@  properties:
           - qcom,sc7180-dwc3
           - qcom,sc7280-dwc3
           - qcom,sc8280xp-dwc3
+          - qcom,sc8280xp-dwc3-mp
           - qcom,sdm660-dwc3
           - qcom,sdm670-dwc3
           - qcom,sdm845-dwc3
@@ -262,6 +263,7 @@  allOf:
           contains:
             enum:
               - qcom,sc8280xp-dwc3
+              - qcom,sc8280xp-dwc3-mp
     then:
       properties:
         clocks:
@@ -455,6 +457,33 @@  allOf:
             - const: dm_hs_phy_irq
             - const: ss_phy_irq
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc8280xp-dwc3-mp
+    then:
+      properties:
+        interrupts:
+          maxItems: 14
+        interrupt-names:
+          items:
+            - const: dp1_hs_phy_irq
+            - const: dm1_hs_phy_irq
+            - const: dp2_hs_phy_irq
+            - const: dm2_hs_phy_irq
+            - const: dp3_hs_phy_irq
+            - const: dm4_hs_phy_irq
+            - const: dp4_hs_phy_irq
+            - const: dm4_hs_phy_irq
+            - const: ss1_phy_irq
+            - const: ss2_phy_irq
+            - const: pwr_event_1
+            - const: pwr_event_2
+            - const: pwr_event_3
+            - const: pwr_event_4
+
 additionalProperties: false
 
 examples: