[PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport

Krishna Kurapati posted 9 patches 2 years, 9 months ago
There is a newer version of this series
[PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
Posted by Krishna Kurapati 2 years, 9 months ago
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

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

diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index d84281926f10..35a895e90001 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,26 @@ allOf:
             - const: dm_hs_phy_irq
             - const: ss_phy_irq
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sc8280xp-dwc3-mp
+    then:
+      properties:
+        interrupts:
+          maxItems: 7
+        interrupt-names:
+          items:
+            - const: dp_hs_phy_irq
+            - const: dm_hs_phy_irq
+            - const: ss_phy_irq
+            - const: pwr_event_1
+            - const: pwr_event_2
+            - const: pwr_event_3
+            - const: pwr_event_4
+
 additionalProperties: false
 
 examples:
-- 
2.40.0
Re: [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
Posted by Johan Hovold 2 years, 8 months ago
On Sun, May 14, 2023 at 11:19:09AM +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
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  .../devicetree/bindings/usb/qcom,dwc3.yaml    | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
 
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sc8280xp-dwc3-mp
> +    then:
> +      properties:
> +        interrupts:
> +          maxItems: 7
> +        interrupt-names:
> +          items:
> +            - const: dp_hs_phy_irq
> +            - const: dm_hs_phy_irq
> +            - const: ss_phy_irq

I assume that these are only for the first port, and that you need to
define these interrupts also for ports 2-4.

> +            - const: pwr_event_1
> +            - const: pwr_event_2
> +            - const: pwr_event_3
> +            - const: pwr_event_4
> +
>  additionalProperties: false
>  
>  examples:

Johan
Re: [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
Posted by Krishna Kurapati PSSNV 2 years, 8 months ago

On 5/16/2023 4:29 PM, Johan Hovold wrote:
> On Sun, May 14, 2023 at 11:19:09AM +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
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   .../devicetree/bindings/usb/qcom,dwc3.yaml    | 22 +++++++++++++++++++
>>   1 file changed, 22 insertions(+)
>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,sc8280xp-dwc3-mp
>> +    then:
>> +      properties:
>> +        interrupts:
>> +          maxItems: 7
>> +        interrupt-names:
>> +          items:
>> +            - const: dp_hs_phy_irq
>> +            - const: dm_hs_phy_irq
>> +            - const: ss_phy_irq
> 
> I assume that these are only for the first port, and that you need to
> define these interrupts also for ports 2-4.
> 

Hi Johan,

  I wanted to add them when wakeup-source is enabled but since you 
mentioned that these must be added now and driver support can be added 
later, I will make a patch separately for this in v9.

Hi Krzysztof,

  Can I use the following notation for the new interrupts ?

dp_hs_port2_irq
dm_hs_port2_irq
dp_hs_port3_irq
dm_hs_port3_irq
dp_hs_port4_irq
dm_hs_port4_irq


That way the interrupt names for first port will be same as ones for 
single port.

Wanted to clarify this before I make a formal patch.

Regards,
Krishna,
Re: [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
Posted by Johan Hovold 2 years, 8 months ago
On Wed, May 17, 2023 at 04:40:11PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/16/2023 4:29 PM, Johan Hovold wrote:
> > On Sun, May 14, 2023 at 11:19:09AM +0530, Krishna Kurapati wrote:

> >> +        interrupts:
> >> +          maxItems: 7
> >> +        interrupt-names:
> >> +          items:
> >> +            - const: dp_hs_phy_irq
> >> +            - const: dm_hs_phy_irq
> >> +            - const: ss_phy_irq
> > 
> > I assume that these are only for the first port, and that you need to
> > define these interrupts also for ports 2-4.

>   I wanted to add them when wakeup-source is enabled but since you 
> mentioned that these must be added now and driver support can be added 
> later, I will make a patch separately for this in v9.

>   Can I use the following notation for the new interrupts ?
> 
> dp_hs_port2_irq
> dm_hs_port2_irq
> dp_hs_port3_irq
> dm_hs_port3_irq
> dp_hs_port4_irq
> dm_hs_port4_irq
> 
> 
> That way the interrupt names for first port will be same as ones for 
> single port.

For consistency, I'd say: use the same scheme also for port1. Perhaps
"port" is unnecessary too.

And since these are getting new names, you can drop the redundant "_irq"
suffix as you did for the power-event lines.

For example:

	pwr_event_1
	dp_hs_phy_1
	dm_hs_phy_1
	ss_phy_1
	...

> Wanted to clarify this before I make a formal patch.

Note that I have some more comments on the remaining patches in the
series that you may want to wait for before posting v9.

Johan
Re: [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
Posted by Krishna Kurapati PSSNV 2 years, 8 months ago

On 5/17/2023 5:14 PM, Johan Hovold wrote:
> On Wed, May 17, 2023 at 04:40:11PM +0530, Krishna Kurapati PSSNV wrote:
>> On 5/16/2023 4:29 PM, Johan Hovold wrote:
>>> On Sun, May 14, 2023 at 11:19:09AM +0530, Krishna Kurapati wrote:
> 
>>>> +        interrupts:
>>>> +          maxItems: 7
>>>> +        interrupt-names:
>>>> +          items:
>>>> +            - const: dp_hs_phy_irq
>>>> +            - const: dm_hs_phy_irq
>>>> +            - const: ss_phy_irq
>>>
>>> I assume that these are only for the first port, and that you need to
>>> define these interrupts also for ports 2-4.
> 
>>    I wanted to add them when wakeup-source is enabled but since you
>> mentioned that these must be added now and driver support can be added
>> later, I will make a patch separately for this in v9.
> 
>>    Can I use the following notation for the new interrupts ?
>>
>> dp_hs_port2_irq
>> dm_hs_port2_irq
>> dp_hs_port3_irq
>> dm_hs_port3_irq
>> dp_hs_port4_irq
>> dm_hs_port4_irq
>>
>>
>> That way the interrupt names for first port will be same as ones for
>> single port.
> 
> For consistency, I'd say: use the same scheme also for port1. Perhaps
> "port" is unnecessary too.
> 
> And since these are getting new names, you can drop the redundant "_irq"
> suffix as you did for the power-event lines.
> 
Hi Johan,

   The reason I wanted to mark it as dp_hs_portX_irq is to keep code 
changes to driver simple. The existing code to read current IRQ's can 
stay as it. Only need to add changes for reading IRQ's of new ports.

> For example:
> 
> 	pwr_event_1
> 	dp_hs_phy_1
> 	dm_hs_phy_1
> 	ss_phy_1
> 	...
> 
>> Wanted to clarify this before I make a formal patch.
> 
> Note that I have some more comments on the remaining patches in the
> series that you may want to wait for before posting v9.
> 
> Johan

Sure, Will wait till end of week for all comments and push v9 next week.

Regards,
Krishna,
Re: [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
Posted by Johan Hovold 2 years, 8 months ago
On Wed, May 17, 2023 at 05:49:13PM +0530, Krishna Kurapati PSSNV wrote:
> On 5/17/2023 5:14 PM, Johan Hovold wrote:
> > On Wed, May 17, 2023 at 04:40:11PM +0530, Krishna Kurapati PSSNV wrote:
> >> On 5/16/2023 4:29 PM, Johan Hovold wrote:
> >>> On Sun, May 14, 2023 at 11:19:09AM +0530, Krishna Kurapati wrote:
> > 
> >>>> +        interrupts:
> >>>> +          maxItems: 7
> >>>> +        interrupt-names:
> >>>> +          items:
> >>>> +            - const: dp_hs_phy_irq
> >>>> +            - const: dm_hs_phy_irq
> >>>> +            - const: ss_phy_irq
> >>>
> >>> I assume that these are only for the first port, and that you need to
> >>> define these interrupts also for ports 2-4.
> > 
> >>    I wanted to add them when wakeup-source is enabled but since you
> >> mentioned that these must be added now and driver support can be added
> >> later, I will make a patch separately for this in v9.
> > 
> >>    Can I use the following notation for the new interrupts ?
> >>
> >> dp_hs_port2_irq
> >> dm_hs_port2_irq
> >> dp_hs_port3_irq
> >> dm_hs_port3_irq
> >> dp_hs_port4_irq
> >> dm_hs_port4_irq
> >>
> >>
> >> That way the interrupt names for first port will be same as ones for
> >> single port.
> > 
> > For consistency, I'd say: use the same scheme also for port1. Perhaps
> > "port" is unnecessary too.
> > 
> > And since these are getting new names, you can drop the redundant "_irq"
> > suffix as you did for the power-event lines.

>    The reason I wanted to mark it as dp_hs_portX_irq is to keep code 
> changes to driver simple. The existing code to read current IRQ's can 
> stay as it. Only need to add changes for reading IRQ's of new ports.

I understand why you want to do it this way, but again, the devicetree
binding is supposed to be hardware description that is independent from
any particular implementation.

This is also why I said that it may be preferable/easier to just
implement wakeup for MP from the start.
 
> > For example:
> > 
> > 	pwr_event_1
> > 	dp_hs_phy_1
> > 	dm_hs_phy_1
> > 	ss_phy_1
> > 	...

Johan
Re: [PATCH v8 1/9] dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
Posted by Krzysztof Kozlowski 2 years, 9 months ago
On 14/05/2023 07:49, 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
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof