[RFC v4 0/5] Add multiport support for DWC3 controllers

[RFC v4 0/5] Add multiport support for DWC3 controllers

[Krishna Kurapati]

Currently the DWC3 driver supports only single port controller which
requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
DWC3 controller with multiple ports that can operate in host mode.
Some of the port supports both SS+HS and other port supports only HS
mode.

This change primarily refactors the Phy logic in core driver to allow
multiport support with Generic Phy's.

Chananges have been tested on  QCOM SoC SA8295P which has 4 ports (2
are HS+SS capable and 2 are HS only capable).

Changes in v4:
Added DT support for SA8295p.

Changes in v3:
Incase any PHY init fails, then clear/exit the PHYs that
are already initialized.

Changes in v2:
Changed dwc3_count_phys to return the number of PHY Phandles in the node.
This will be used now in dwc3_extract_num_phys to increment num_usb2_phy 
and num_usb3_phy.

Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its
structure such that the first half is for HS-PHY and second half is for
SS-PHY.

In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is
present, pass proper SS_IDX else pass -1.

Link to v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r
Link to v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r

Krishna Kurapati (5):
  dt-bindings: usb: Add bindings to support multiport properties
  usb: dwc3: core: Refactor PHY logic to support Multiport Controller
  usb: dwc3: core: Do not setup event buffers for host only controllers
  usb: dwc3: qcom: Add multiport controller support for qcom wrapper
  arm: dts: msm: Add multiport controller node for usb

 .../devicetree/bindings/usb/snps,dwc3.yaml    |  42 ++-
 arch/arm64/boot/dts/qcom/sa8295p-adp.dts      |  49 +++
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi        |  60 ++++
 drivers/usb/dwc3/core.c                       | 325 +++++++++++++-----
 drivers/usb/dwc3/core.h                       |  15 +-
 drivers/usb/dwc3/drd.c                        |  14 +-
 drivers/usb/dwc3/dwc3-qcom.c                  |  28 +-
 7 files changed, 429 insertions(+), 104 deletions(-)

-- 
2.39.0

[RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties

[Krishna Kurapati]

Add bindings to indicate properties required to support multiport
on Snps Dwc3 controller.

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

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 6d78048c4613..3ea051beb2f8 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -81,15 +81,26 @@ 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:
+        enum:
+          - usb2-phy_port0
+          - usb2-phy_port1
+          - usb2-phy_port2
+          - usb2-phy_port3
+          - usb3-phy_port0
+          - usb3-phy_port1
+          - usb3-phy_port2
+          - usb3-phy_port3
 
   resets:
     minItems: 1
@@ -360,6 +371,22 @@ properties:
     description:
       Enable USB remote wakeup.
 
+  num-ports:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      This property indicates the number of ports present on the target that
+      are to be serviced by the DWC3 controller.
+    minimum: 1
+    maximum: 4
+
+  num-ss-ports:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      This property indicates the number of SS capable ports present on the
+      target that are to be serviced by the DWC3 controller.
+    minimum: 1
+    maximum: 4
+
 unevaluatedProperties: false
 
 required:
@@ -388,4 +415,18 @@ examples:
       snps,dis_u2_susphy_quirk;
       snps,dis_enblslpm_quirk;
     };
+  - |
+    usb@4a000000 {
+      compatible = "snps,dwc3";
+      reg = <0x4a000000 0xcfff>;
+      interrupts = <0 92 4>;
+      clocks = <&clk 1>, <&clk 2>, <&clk 3>;
+      clock-names = "bus_early", "ref", "suspend";
+      num-ports = <2>;
+      num-ss-ports = <1>;
+      phys = <&usb2_phy0>, <&usb3_phy0>, <&usb2_phy1>;
+      phy-names = "usb2-phy_port0", "usb3-phy_port0", "usb2-phy_port1";
+      snps,dis_u2_susphy_quirk;
+      snps,dis_enblslpm_quirk;
+    };
 ...
-- 
2.39.0

Re: [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties

[Rob Herring]

On Sun, 15 Jan 2023 17:11:42 +0530, Krishna Kurapati wrote:
> Add bindings to indicate properties required to support multiport
> on Snps Dwc3 controller.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml    | 53 ++++++++++++++++---
>  1 file changed, 47 insertions(+), 6 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/usb/snps,dwc3.yaml:90:5: [warning] wrong indentation: expected 6 but found 4 (indentation)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230115114146.12628-2-quic_kriskura@quicinc.com

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.

Re: [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties

[Rob Herring]

On Sun, Jan 15, 2023 at 05:11:42PM +0530, Krishna Kurapati wrote:
> Add bindings to indicate properties required to support multiport
> on Snps Dwc3 controller.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  .../devicetree/bindings/usb/snps,dwc3.yaml    | 53 ++++++++++++++++---
>  1 file changed, 47 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> index 6d78048c4613..3ea051beb2f8 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
> @@ -81,15 +81,26 @@ 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:
> +        enum:
> +          - usb2-phy_port0
> +          - usb2-phy_port1
> +          - usb2-phy_port2
> +          - usb2-phy_port3
> +          - usb3-phy_port0
> +          - usb3-phy_port1
> +          - usb3-phy_port2
> +          - usb3-phy_port3

usbN-portM

>  
>    resets:
>      minItems: 1
> @@ -360,6 +371,22 @@ properties:
>      description:
>        Enable USB remote wakeup.
>  
> +  num-ports:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      This property indicates the number of ports present on the target that
> +      are to be serviced by the DWC3 controller.
> +    minimum: 1
> +    maximum: 4
> +
> +  num-ss-ports:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      This property indicates the number of SS capable ports present on the
> +      target that are to be serviced by the DWC3 controller.
> +    minimum: 1
> +    maximum: 4

This information is redundant. 'phy-names' tells you how many ports of 
each.

> +
>  unevaluatedProperties: false
>  
>  required:
> @@ -388,4 +415,18 @@ examples:
>        snps,dis_u2_susphy_quirk;
>        snps,dis_enblslpm_quirk;
>      };
> +  - |
> +    usb@4a000000 {
> +      compatible = "snps,dwc3";
> +      reg = <0x4a000000 0xcfff>;
> +      interrupts = <0 92 4>;
> +      clocks = <&clk 1>, <&clk 2>, <&clk 3>;
> +      clock-names = "bus_early", "ref", "suspend";
> +      num-ports = <2>;
> +      num-ss-ports = <1>;
> +      phys = <&usb2_phy0>, <&usb3_phy0>, <&usb2_phy1>;
> +      phy-names = "usb2-phy_port0", "usb3-phy_port0", "usb2-phy_port1";
> +      snps,dis_u2_susphy_quirk;
> +      snps,dis_enblslpm_quirk;
> +    };

Does a different number of phys really need its own example?

Rob

Re: [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties

[Krishna Kurapati PSSNV]

On 1/16/2023 10:04 PM, Rob Herring wrote:
> On Sun, Jan 15, 2023 at 05:11:42PM +0530, Krishna Kurapati wrote:
>> Add bindings to indicate properties required to support multiport
>> on Snps Dwc3 controller.
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   .../devicetree/bindings/usb/snps,dwc3.yaml    | 53 ++++++++++++++++---
>>   1 file changed, 47 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> index 6d78048c4613..3ea051beb2f8 100644
>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>> @@ -81,15 +81,26 @@ 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:
>> +        enum:
>> +          - usb2-phy_port0
>> +          - usb2-phy_port1
>> +          - usb2-phy_port2
>> +          - usb2-phy_port3
>> +          - usb3-phy_port0
>> +          - usb3-phy_port1
>> +          - usb3-phy_port2
>> +          - usb3-phy_port3
> 
> usbN-portM
> 
>>   
>>     resets:
>>       minItems: 1
>> @@ -360,6 +371,22 @@ properties:
>>       description:
>>         Enable USB remote wakeup.
>>   
>> +  num-ports:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      This property indicates the number of ports present on the target that
>> +      are to be serviced by the DWC3 controller.
>> +    minimum: 1
>> +    maximum: 4
>> +
>> +  num-ss-ports:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      This property indicates the number of SS capable ports present on the
>> +      target that are to be serviced by the DWC3 controller.
>> +    minimum: 1
>> +    maximum: 4
> 
> This information is redundant. 'phy-names' tells you how many ports of
> each.
> 
Hi Rob,

  Thanks for the review. The reason I wanted to introduce two more 
variables is to get info on number of ports  and ss-capable ports 
present on hardware whether or not the user provides them in DTSI file.

In the code there are two types of per port / per phy operations:
a) Modifying GUSB2PFYCFG and GUSB3PIPECTL registers per phy.
b) Generic Phy operations - per phy.

In today's code, if someone doesn't mention the SSPHY in DTSI, 
dwc->usb3_generic_phy will be NULL and any call to phy operations will 
just bail out. And irrespective of whether we provide SS Phy in DTSI or 
not, we still configure GUSB3PIPECTL register.

Consider the following cases:

1. There are 3 ports and 2 of them are SS capable and all phy's are 
mentioned in DTSI.

phy-names= "usb2-port0", "usb3-port0", "usb2-port1", "usb3-port1", 
"usb2-port2"

When we count them in the driver, we get num ports as 3 (presuming 
num-ports = num of hs ports) and num-ss-ports = 2.

Since there is no ambiguity in which all ports to configure, we can 
modify GUSB2PHYCFG registers for all 3 HS Phy's and GUSB3PIPECTL for 
both SS Phy's.
This is a proper scenario.

2. If the user skips providing SS Phy on Port-0, then:

phy-names= "usb2-port0", "usb2-port1", "usb3-port1", "usb2-port2"

If we count the phys, we end up getting num-ports=3 and num-ss-ports=1.

Since in the driver code, we are not keeping track of which ports are SS 
capable and which ones are not, we end up configuring
GUSB2PIPECTL(port-0) instead of port-1  as the num-ss-ports is "1" which 
is incorrect.

3. If the user skips providing one complete port, in this case port-1 is 
skipped, then:

phy-names= "usb2-port0", "usb3-port0", "usb2-port2"

If we count the phys, we end up getting num-ports=2 and num-ss-ports=1.

Since in the driver code, we are not keeping track of which ports are SS 
capable and which ones are not, we end up configuring 
GUSB2PHYCFG(port-0) and GUSB2PHYCFG(port-1) instead of port-2 which is 
incorrect.

To avoid these scenarios, if we can get the exact number of SS Ports and 
Ports in total present on the HW, we can configure all the registers 
whether the phy's are provided in DTSI or not. (This is of no harm I 
believe as it still works in today's code)

Incase the 2nd and 3rd scenarios are not allowed and user *MUST* declare 
all the phy's in the DTSI, then I can go ahead and remove these 
properties and count them in the driver code.

Thanks,
Krishna,

>> +
>>   unevaluatedProperties: false
>>   
>>   required:
>> @@ -388,4 +415,18 @@ examples:
>>         snps,dis_u2_susphy_quirk;
>>         snps,dis_enblslpm_quirk;
>>       };
>> +  - |
>> +    usb@4a000000 {
>> +      compatible = "snps,dwc3";
>> +      reg = <0x4a000000 0xcfff>;
>> +      interrupts = <0 92 4>;
>> +      clocks = <&clk 1>, <&clk 2>, <&clk 3>;
>> +      clock-names = "bus_early", "ref", "suspend";
>> +      num-ports = <2>;
>> +      num-ss-ports = <1>;
>> +      phys = <&usb2_phy0>, <&usb3_phy0>, <&usb2_phy1>;
>> +      phy-names = "usb2-phy_port0", "usb3-phy_port0", "usb2-phy_port1";
>> +      snps,dis_u2_susphy_quirk;
>> +      snps,dis_enblslpm_quirk;
>> +    };
> 
> Does a different number of phys really need its own example?
> 
> Rob

Re: [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties

[Krzysztof Kozlowski]

On 17/01/2023 10:01, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/16/2023 10:04 PM, Rob Herring wrote:
>> On Sun, Jan 15, 2023 at 05:11:42PM +0530, Krishna Kurapati wrote:
>>> Add bindings to indicate properties required to support multiport
>>> on Snps Dwc3 controller.
>>>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>>>   .../devicetree/bindings/usb/snps,dwc3.yaml    | 53 ++++++++++++++++---
>>>   1 file changed, 47 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> index 6d78048c4613..3ea051beb2f8 100644
>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>> @@ -81,15 +81,26 @@ 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:
>>> +        enum:
>>> +          - usb2-phy_port0
>>> +          - usb2-phy_port1
>>> +          - usb2-phy_port2
>>> +          - usb2-phy_port3
>>> +          - usb3-phy_port0
>>> +          - usb3-phy_port1
>>> +          - usb3-phy_port2
>>> +          - usb3-phy_port3
>>
>> usbN-portM
>>
>>>   
>>>     resets:
>>>       minItems: 1
>>> @@ -360,6 +371,22 @@ properties:
>>>       description:
>>>         Enable USB remote wakeup.
>>>   
>>> +  num-ports:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      This property indicates the number of ports present on the target that
>>> +      are to be serviced by the DWC3 controller.
>>> +    minimum: 1
>>> +    maximum: 4
>>> +
>>> +  num-ss-ports:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      This property indicates the number of SS capable ports present on the
>>> +      target that are to be serviced by the DWC3 controller.
>>> +    minimum: 1
>>> +    maximum: 4
>>
>> This information is redundant. 'phy-names' tells you how many ports of
>> each.
>>
> Hi Rob,
> 
>   Thanks for the review. The reason I wanted to introduce two more 
> variables is to get info on number of ports  and ss-capable ports 
> present on hardware whether or not the user provides them in DTSI file.
> 
> In the code there are two types of per port / per phy operations:
> a) Modifying GUSB2PFYCFG and GUSB3PIPECTL registers per phy.
> b) Generic Phy operations - per phy.
> 
> In today's code, if someone doesn't mention the SSPHY in DTSI, 
> dwc->usb3_generic_phy will be NULL and any call to phy operations will 
> just bail out. And irrespective of whether we provide SS Phy in DTSI or 
> not, we still configure GUSB3PIPECTL register.
> 
> Consider the following cases:
> 
> 1. There are 3 ports and 2 of them are SS capable and all phy's are 
> mentioned in DTSI.
> 
> phy-names= "usb2-port0", "usb3-port0", "usb2-port1", "usb3-port1", 
> "usb2-port2"
> 
> When we count them in the driver, we get num ports as 3 (presuming 
> num-ports = num of hs ports) and num-ss-ports = 2.
> 
> Since there is no ambiguity in which all ports to configure, we can 
> modify GUSB2PHYCFG registers for all 3 HS Phy's and GUSB3PIPECTL for 
> both SS Phy's.
> This is a proper scenario.
> 
> 2. If the user skips providing SS Phy on Port-0, then:
> 
> phy-names= "usb2-port0", "usb2-port1", "usb3-port1", "usb2-port2"
> 
> If we count the phys, we end up getting num-ports=3 and num-ss-ports=1.
> 
> Since in the driver code, we are not keeping track of which ports are SS 
> capable and which ones are not, we end up configuring
> GUSB2PIPECTL(port-0) instead of port-1  as the num-ss-ports is "1" which 
> is incorrect.
> 
> 3. If the user skips providing one complete port, in this case port-1 is 
> skipped, then:
> 
> phy-names= "usb2-port0", "usb3-port0", "usb2-port2"
> 
> If we count the phys, we end up getting num-ports=2 and num-ss-ports=1.
> 
> Since in the driver code, we are not keeping track of which ports are SS 
> capable and which ones are not, we end up configuring 
> GUSB2PHYCFG(port-0) and GUSB2PHYCFG(port-1) instead of port-2 which is 
> incorrect.

Why? You know you have port-2 from the phy name, so why would you ignore
this information?

> 
> To avoid these scenarios, if we can get the exact number of SS Ports and 
> Ports in total present on the HW, we can configure all the registers 
> whether the phy's are provided in DTSI or not. (This is of no harm I 
> believe as it still works in today's code)

Doesn't the driver know how many phys it has in such case through
respective compatible?

> 
> Incase the 2nd and 3rd scenarios are not allowed and user *MUST* declare 
> all the phy's in the DTSI, then I can go ahead and remove these 
> properties and count them in the driver code.


Why you cannot then configure all phys in the driver all ports as some
safe default and then customize it depending on the actual port used?


Best regards,
Krzysztof

Re: [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties

[Krishna Kurapati PSSNV]

On 1/17/2023 4:32 PM, Krzysztof Kozlowski wrote:
> On 17/01/2023 10:01, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/16/2023 10:04 PM, Rob Herring wrote:
>>> On Sun, Jan 15, 2023 at 05:11:42PM +0530, Krishna Kurapati wrote:
>>>> Add bindings to indicate properties required to support multiport
>>>> on Snps Dwc3 controller.
>>>>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>> ---
>>>>    .../devicetree/bindings/usb/snps,dwc3.yaml    | 53 ++++++++++++++++---
>>>>    1 file changed, 47 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> index 6d78048c4613..3ea051beb2f8 100644
>>>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
>>>> @@ -81,15 +81,26 @@ 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:
>>>> +        enum:
>>>> +          - usb2-phy_port0
>>>> +          - usb2-phy_port1
>>>> +          - usb2-phy_port2
>>>> +          - usb2-phy_port3
>>>> +          - usb3-phy_port0
>>>> +          - usb3-phy_port1
>>>> +          - usb3-phy_port2
>>>> +          - usb3-phy_port3
>>>
>>> usbN-portM
>>>
>>>>    
>>>>      resets:
>>>>        minItems: 1
>>>> @@ -360,6 +371,22 @@ properties:
>>>>        description:
>>>>          Enable USB remote wakeup.
>>>>    
>>>> +  num-ports:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      This property indicates the number of ports present on the target that
>>>> +      are to be serviced by the DWC3 controller.
>>>> +    minimum: 1
>>>> +    maximum: 4
>>>> +
>>>> +  num-ss-ports:
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>> +    description:
>>>> +      This property indicates the number of SS capable ports present on the
>>>> +      target that are to be serviced by the DWC3 controller.
>>>> +    minimum: 1
>>>> +    maximum: 4
>>>
>>> This information is redundant. 'phy-names' tells you how many ports of
>>> each.
>>>
>> Hi Rob,
>>
>>    Thanks for the review. The reason I wanted to introduce two more
>> variables is to get info on number of ports  and ss-capable ports
>> present on hardware whether or not the user provides them in DTSI file.
>>
>> In the code there are two types of per port / per phy operations:
>> a) Modifying GUSB2PFYCFG and GUSB3PIPECTL registers per phy.
>> b) Generic Phy operations - per phy.
>>
>> In today's code, if someone doesn't mention the SSPHY in DTSI,
>> dwc->usb3_generic_phy will be NULL and any call to phy operations will
>> just bail out. And irrespective of whether we provide SS Phy in DTSI or
>> not, we still configure GUSB3PIPECTL register.
>>
>> Consider the following cases:
>>
>> 1. There are 3 ports and 2 of them are SS capable and all phy's are
>> mentioned in DTSI.
>>
>> phy-names= "usb2-port0", "usb3-port0", "usb2-port1", "usb3-port1",
>> "usb2-port2"
>>
>> When we count them in the driver, we get num ports as 3 (presuming
>> num-ports = num of hs ports) and num-ss-ports = 2.
>>
>> Since there is no ambiguity in which all ports to configure, we can
>> modify GUSB2PHYCFG registers for all 3 HS Phy's and GUSB3PIPECTL for
>> both SS Phy's.
>> This is a proper scenario.
>>
>> 2. If the user skips providing SS Phy on Port-0, then:
>>
>> phy-names= "usb2-port0", "usb2-port1", "usb3-port1", "usb2-port2"
>>
>> If we count the phys, we end up getting num-ports=3 and num-ss-ports=1.
>>
>> Since in the driver code, we are not keeping track of which ports are SS
>> capable and which ones are not, we end up configuring
>> GUSB2PIPECTL(port-0) instead of port-1  as the num-ss-ports is "1" which
>> is incorrect.
>>
>> 3. If the user skips providing one complete port, in this case port-1 is
>> skipped, then:
>>
>> phy-names= "usb2-port0", "usb3-port0", "usb2-port2"
>>
>> If we count the phys, we end up getting num-ports=2 and num-ss-ports=1.
>>
>> Since in the driver code, we are not keeping track of which ports are SS
>> capable and which ones are not, we end up configuring
>> GUSB2PHYCFG(port-0) and GUSB2PHYCFG(port-1) instead of port-2 which is
>> incorrect.
> 
> Why? You know you have port-2 from the phy name, so why would you ignore
> this information?
> 
Hi Krzysztof,

Thanks for the review,

   The concern I had with that approach is that, we need to keep track 
of per port supported speeds in some array /flags and check them 
whenever we are modifying the dwc3-phy registers. This makes the code a 
little unreadable.
>>
>> To avoid these scenarios, if we can get the exact number of SS Ports and
>> Ports in total present on the HW, we can configure all the registers
>> whether the phy's are provided in DTSI or not. (This is of no harm I
>> believe as it still works in today's code)
> 
> Doesn't the driver know how many phys it has in such case through
> respective compatible?
> 
The core driver has only one compatible currently "snps,dwc3".

Are you suggesting to add new compatible to driver core in case any 
multiport device is being used and get this info from there ?

>>
>> Incase the 2nd and 3rd scenarios are not allowed and user *MUST* declare
>> all the phy's in the DTSI, then I can go ahead and remove these
>> properties and count them in the driver code.
> 
> 
> Why you cannot then configure all phys in the driver all ports as some
> safe default and then customize it depending on the actual port used?
> 
To do this, we still need to get info on number of hs/ss phy's present 
on hardware and currently there is no register I believe in DWC3 core 
global address space that can give this info. I see that HCSPARAMS1 Reg 
gives some info but that is not accessible from driver core.

According to databook:

"Number of Ports (MaxPorts)
-> Number of ports implemented is defined by the parameter
(`DWC_USB3_HOST_NUM_U2_ROOT_PORTS +
`DWC_USB3_HOST_NUM_U3_ROOT_PORTS)
-> Number of ports enabled is controlled by the core input signals
host_num_u2_port[3:0]+host_num_u3_port[3:0]"
Regards,
Krishna,

Re: [RFC v4 1/5] dt-bindings: usb: Add bindings to support multiport properties

[Bjorn Andersson]

On Sun, Jan 15, 2023 at 05:11:42PM +0530, Krishna Kurapati wrote:
> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
[..]
>    phy-names:
>      minItems: 1
> -    maxItems: 2
> -    items:
> -      enum:
> -        - usb2-phy
> -        - usb3-phy
> +    maxItems: 8
> +    oneOf:
> +    - items:
> +        enum:
> +          - usb2-phy
> +          - usb3-phy
> +    - items:
> +        enum:
> +          - usb2-phy_port0
> +          - usb2-phy_port1
> +          - usb2-phy_port2
> +          - usb2-phy_port3
> +          - usb3-phy_port0
> +          - usb3-phy_port1
> +          - usb3-phy_port2
> +          - usb3-phy_port3

How about expressing this as:

    oneOf:
      - items:
          enum: [ usb2-phy, usb3-phy ]
      - items:
          pattern: "^usb[23]-phy_port[0-3]$"

Regards,
Bjorn

[RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati]

Currently the DWC3 driver supports only single port controller
which requires at most one HS and one SS PHY.

But the DWC3 USB controller can be connected to multiple ports and
each port can have their own PHYs. Each port of the multiport
controller can either be HS+SS capable or HS only capable
Proper quantification of them is required to modify GUSB2PHYCFG
and GUSB3PIPECTL registers appropriately.

Add support for detecting, obtaining and configuring phy's supported
by a multiport controller and limit the max number of ports
supported to 4.

Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
 drivers/usb/dwc3/core.h |  15 +-
 drivers/usb/dwc3/drd.c  |  14 +-
 3 files changed, 244 insertions(+), 89 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 476b63618511..7e0a9a598dfd 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
 {
 	struct dwc3 *dwc = work_to_dwc(work);
 	unsigned long flags;
-	int ret;
+	int ret, i;
 	u32 reg;
 	u32 desired_dr_role;
 
@@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
 		} else {
 			if (dwc->usb2_phy)
 				otg_set_vbus(dwc->usb2_phy->otg, true);
-			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
-			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
+			for (i = 0; i < dwc->num_ports; i++) {
+				phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
+				phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
+			}
 			if (dwc->dis_split_quirk) {
 				reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
 				reg |= DWC3_GUCTL3_SPLITDISABLE;
@@ -216,8 +218,8 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 		if (dwc->usb2_phy)
 			otg_set_vbus(dwc->usb2_phy->otg, false);
-		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
-		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
 
 		ret = dwc3_gadget_init(dwc);
 		if (ret)
@@ -659,22 +661,14 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
 	return ret;
 }
 
-/**
- * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
- * @dwc: Pointer to our controller context structure
- *
- * Returns 0 on success. The USB PHY interfaces are configured but not
- * initialized. The PHY interfaces and the PHYs get initialized together with
- * the core in dwc3_core_init.
- */
-static int dwc3_phy_setup(struct dwc3 *dwc)
+static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
 {
 	unsigned int hw_mode;
 	u32 reg;
 
 	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
 
-	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(index));
 
 	/*
 	 * Make sure UX_EXIT_PX is cleared as that causes issues with some
@@ -729,9 +723,19 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->dis_del_phy_power_chg_quirk)
 		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
 
-	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);
 
-	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+	return 0;
+}
+
+static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index)
+{
+	unsigned int hw_mode;
+	u32 reg;
+
+	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
 
 	/* Select the HS PHY interface */
 	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
@@ -743,7 +747,7 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 		} else if (dwc->hsphy_interface &&
 				!strncmp(dwc->hsphy_interface, "ulpi", 4)) {
 			reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
 		} else {
 			/* Relying on default value. */
 			if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI))
@@ -800,7 +804,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
 	if (dwc->dis_u2_freeclk_exists_quirk || dwc->gfladj_refclk_lpm_sel)
 		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
 
-	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
+
+	return 0;
+}
+
+/**
+ * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
+ * @dwc: Pointer to our controller context structure
+ *
+ * Returns 0 on success. The USB PHY interfaces are configured but not
+ * initialized. The PHY interfaces and the PHYs get initialized together with
+ * the core in dwc3_core_init.
+ */
+static int dwc3_phy_setup(struct dwc3 *dwc)
+{
+	int i;
+	int ret;
+
+	for (i = 0; i < dwc->num_ss_ports; i++) {
+		ret = dwc3_ss_phy_setup(dwc, i);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		ret = dwc3_hs_phy_setup(dwc, i);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }
@@ -839,17 +871,25 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
 
 static void dwc3_core_exit(struct dwc3 *dwc)
 {
+	int i;
+
 	dwc3_event_buffers_cleanup(dwc);
 
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
-	phy_power_off(dwc->usb2_generic_phy);
-	phy_power_off(dwc->usb3_generic_phy);
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		phy_power_off(dwc->usb2_generic_phy[i]);
+		phy_power_off(dwc->usb3_generic_phy[i]);
+	}
 
 	usb_phy_shutdown(dwc->usb2_phy);
 	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		phy_exit(dwc->usb2_generic_phy[i]);
+		phy_exit(dwc->usb3_generic_phy[i]);
+	}
 
 	dwc3_clk_disable(dwc);
 	reset_control_assert(dwc->reset);
@@ -1085,6 +1125,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	unsigned int		hw_mode;
 	u32			reg;
 	int			ret;
+	int			i, j;
 
 	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
 
@@ -1119,14 +1160,27 @@ static int dwc3_core_init(struct dwc3 *dwc)
 
 	usb_phy_init(dwc->usb2_phy);
 	usb_phy_init(dwc->usb3_phy);
-	ret = phy_init(dwc->usb2_generic_phy);
-	if (ret < 0)
-		goto err0a;
 
-	ret = phy_init(dwc->usb3_generic_phy);
-	if (ret < 0) {
-		phy_exit(dwc->usb2_generic_phy);
-		goto err0a;
+	for (i = 0; i < dwc->num_ports; i++) {
+		ret = phy_init(dwc->usb2_generic_phy[i]);
+		if (ret < 0) {
+			/* clean up prior initialized HS PHYs */
+			for (j = 0; j < i; j++)
+				phy_exit(dwc->usb2_generic_phy[j]);
+			goto err0a;
+		}
+	}
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		ret = phy_init(dwc->usb3_generic_phy[i]);
+		if (ret < 0) {
+			/* clean up prior initialized SS PHYs */
+			for (j = 0; j < i; j++)
+				phy_exit(dwc->usb3_generic_phy[j]);
+			for (j = 0; j < dwc->num_ports; j++)
+				phy_exit(dwc->usb2_generic_phy[j]);
+			goto err0a;
+		}
 	}
 
 	ret = dwc3_core_soft_reset(dwc);
@@ -1136,15 +1190,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
 	    !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
 		if (!dwc->dis_u3_susphy_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
-			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+			for (i = 0; i < dwc->num_ss_ports; i++) {
+				reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
+				reg |= DWC3_GUSB3PIPECTL_SUSPHY;
+				dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg);
+			}
 		}
 
 		if (!dwc->dis_u2_susphy_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+			for (i = 0; i < dwc->num_ports; i++) {
+				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+				reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+			}
 		}
 	}
 
@@ -1168,13 +1226,25 @@ static int dwc3_core_init(struct dwc3 *dwc)
 
 	usb_phy_set_suspend(dwc->usb2_phy, 0);
 	usb_phy_set_suspend(dwc->usb3_phy, 0);
-	ret = phy_power_on(dwc->usb2_generic_phy);
-	if (ret < 0)
-		goto err2;
 
-	ret = phy_power_on(dwc->usb3_generic_phy);
-	if (ret < 0)
-		goto err3;
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		ret = phy_power_on(dwc->usb2_generic_phy[i]);
+		if (ret < 0) {
+			for (j = 0; j < i; j++)
+				phy_power_off(dwc->usb2_generic_phy[j]);
+			goto err2;
+		}
+	}
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		ret = phy_power_on(dwc->usb3_generic_phy[i]);
+		if (ret < 0) {
+			for (j = 0; j < i; j++)
+				phy_power_off(dwc->usb3_generic_phy[j]);
+			goto err3;
+		}
+	}
 
 	ret = dwc3_event_buffers_setup(dwc);
 	if (ret) {
@@ -1297,10 +1367,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	return 0;
 
 err4:
-	phy_power_off(dwc->usb3_generic_phy);
+	for (i = 0; i < dwc->num_ports; i++)
+		phy_power_off(dwc->usb3_generic_phy[i]);
 
 err3:
-	phy_power_off(dwc->usb2_generic_phy);
+	for (i = 0; i < dwc->num_ports; i++)
+		phy_power_off(dwc->usb2_generic_phy[i]);
 
 err2:
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
@@ -1309,8 +1381,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
 err1:
 	usb_phy_shutdown(dwc->usb2_phy);
 	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		phy_exit(dwc->usb2_generic_phy[i]);
+		phy_exit(dwc->usb3_generic_phy[i]);
+	}
 
 err0a:
 	dwc3_ulpi_exit(dwc);
@@ -1319,6 +1394,38 @@ static int dwc3_core_init(struct dwc3 *dwc)
 	return ret;
 }
 
+static int dwc3_get_multiport_phys(struct dwc3 *dwc)
+{
+	int ret;
+	struct device *dev = dwc->dev;
+	int i;
+	char phy_name[15];
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		sprintf(phy_name, "usb2-phy_port%d", i);
+		dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name);
+		if (IS_ERR(dwc->usb2_generic_phy[i])) {
+			ret = PTR_ERR(dwc->usb2_generic_phy[i]);
+			if (ret == -ENOSYS || ret == -ENODEV)
+				dwc->usb2_generic_phy[i] = NULL;
+			else
+				return dev_err_probe(dev, ret, "usb2 phy: %s not configured\n", phy_name);
+		}
+
+		sprintf(phy_name, "usb3-phy_port%d", i);
+		dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
+		if (IS_ERR(dwc->usb3_generic_phy[i])) {
+			ret = PTR_ERR(dwc->usb3_generic_phy[i]);
+			if (ret == -ENOSYS || ret == -ENODEV)
+				dwc->usb3_generic_phy[i] = NULL;
+			else
+				return dev_err_probe(dev, ret, "usb3 phy: %s not configured\n", phy_name);
+		}
+	}
+
+	return 0;
+}
+
 static int dwc3_core_get_phy(struct dwc3 *dwc)
 {
 	struct device		*dev = dwc->dev;
@@ -1349,31 +1456,37 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
 			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
 	}
 
-	dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
-	if (IS_ERR(dwc->usb2_generic_phy)) {
-		ret = PTR_ERR(dwc->usb2_generic_phy);
+	if (dwc->num_ports > 1)
+		goto get_multiport_phys;
+
+	dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy");
+	if (IS_ERR(dwc->usb2_generic_phy[0])) {
+		ret = PTR_ERR(dwc->usb2_generic_phy[0]);
 		if (ret == -ENOSYS || ret == -ENODEV)
-			dwc->usb2_generic_phy = NULL;
+			dwc->usb2_generic_phy[0] = NULL;
 		else
 			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
 	}
 
-	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
-	if (IS_ERR(dwc->usb3_generic_phy)) {
-		ret = PTR_ERR(dwc->usb3_generic_phy);
+	dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy");
+	if (IS_ERR(dwc->usb3_generic_phy[0])) {
+		ret = PTR_ERR(dwc->usb3_generic_phy[0]);
 		if (ret == -ENOSYS || ret == -ENODEV)
-			dwc->usb3_generic_phy = NULL;
+			dwc->usb3_generic_phy[0] = NULL;
 		else
 			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
 	}
 
 	return 0;
+
+get_multiport_phys:
+	return dwc3_get_multiport_phys(dwc);
 }
 
 static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
-	int ret;
+	int ret, i;
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
@@ -1381,8 +1494,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 
 		if (dwc->usb2_phy)
 			otg_set_vbus(dwc->usb2_phy->otg, false);
-		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
-		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
+		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
 
 		ret = dwc3_gadget_init(dwc);
 		if (ret)
@@ -1393,8 +1506,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 
 		if (dwc->usb2_phy)
 			otg_set_vbus(dwc->usb2_phy->otg, true);
-		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
-		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
+		for (i = 0; i < dwc->num_ports; i++) {
+			phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
+			phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
+		}
 
 		ret = dwc3_host_init(dwc);
 		if (ret)
@@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	dwc->dis_split_quirk = device_property_read_bool(dev,
 				"snps,dis-split-quirk");
 
+
+	/*
+	 * If no mulitport properties are defined, default
+	 * the port count to '1'.
+	 */
+	ret = device_property_read_u32(dev, "num-ports",
+				&dwc->num_ports);
+	if (ret)
+		dwc->num_ports = 1;
+
+	ret = device_property_read_u32(dev, "num-ss-ports",
+				&dwc->num_ss_ports);
+	if (ret)
+		dwc->num_ss_ports = 1;
+
 	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
 	dwc->tx_de_emphasis = tx_de_emphasis;
 
@@ -1755,8 +1885,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	struct device		*dev = &pdev->dev;
 	struct resource		*res, dwc_res;
 	struct dwc3		*dwc;
-
-	int			ret;
+	int			ret, i;
 
 	void __iomem		*regs;
 
@@ -1933,17 +2062,24 @@ static int dwc3_probe(struct platform_device *pdev)
 
 err5:
 	dwc3_debugfs_exit(dwc);
+
 	dwc3_event_buffers_cleanup(dwc);
 
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
-	phy_power_off(dwc->usb2_generic_phy);
-	phy_power_off(dwc->usb3_generic_phy);
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		phy_power_off(dwc->usb2_generic_phy[i]);
+		phy_power_off(dwc->usb3_generic_phy[i]);
+	}
 
 	usb_phy_shutdown(dwc->usb2_phy);
 	usb_phy_shutdown(dwc->usb3_phy);
-	phy_exit(dwc->usb2_generic_phy);
-	phy_exit(dwc->usb3_generic_phy);
+
+	for (i = 0; i < dwc->num_ports; i++) {
+		phy_exit(dwc->usb2_generic_phy[i]);
+		phy_exit(dwc->usb3_generic_phy[i]);
+	}
 
 	dwc3_ulpi_exit(dwc);
 
@@ -2025,6 +2161,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
 
 static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
+	int i;
 	unsigned long	flags;
 	u32 reg;
 
@@ -2045,17 +2182,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 		/* Let controller to suspend HSPHY before PHY driver suspends */
 		if (dwc->dis_u2_susphy_quirk ||
 		    dwc->dis_enblslpm_quirk) {
-			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-			reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
-				DWC3_GUSB2PHYCFG_SUSPHY;
-			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+			for (i = 0; i < dwc->num_ports; i++) {
+				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+				reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
+					DWC3_GUSB2PHYCFG_SUSPHY;
+				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+			}
 
 			/* Give some time for USB2 PHY to suspend */
 			usleep_range(5000, 6000);
 		}
 
-		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
-		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
+		for (i = 0; i < dwc->num_ports; i++) {
+			phy_pm_runtime_put_sync(dwc->usb2_generic_phy[i]);
+			phy_pm_runtime_put_sync(dwc->usb3_generic_phy[i]);
+		}
 		break;
 	case DWC3_GCTL_PRTCAP_OTG:
 		/* do nothing during runtime_suspend */
@@ -2084,6 +2225,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
 	int		ret;
+	int		i;
 	u32		reg;
 
 	switch (dwc->current_dr_role) {
@@ -2104,17 +2246,21 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 			break;
 		}
 		/* Restore GUSB2PHYCFG bits that were modified in suspend */
-		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-		if (dwc->dis_u2_susphy_quirk)
-			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+		for (i = 0; i < dwc->num_ports; i++) {
+			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+			if (dwc->dis_u2_susphy_quirk)
+				reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
 
-		if (dwc->dis_enblslpm_quirk)
-			reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
+			if (dwc->dis_enblslpm_quirk)
+				reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
 
-		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+		}
 
-		phy_pm_runtime_get_sync(dwc->usb2_generic_phy);
-		phy_pm_runtime_get_sync(dwc->usb3_generic_phy);
+		for (i = 0; i < dwc->num_ports; i++) {
+			phy_pm_runtime_get_sync(dwc->usb2_generic_phy[i]);
+			phy_pm_runtime_get_sync(dwc->usb3_generic_phy[i]);
+		}
 		break;
 	case DWC3_GCTL_PRTCAP_OTG:
 		/* nothing to do on runtime_resume */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 8f9959ba9fd4..2f82eda9d44f 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -35,6 +35,9 @@
 
 #define DWC3_MSG_MAX	500
 
+/* Number of ports supported by a multiport controller */
+#define MAX_PORTS_SUPPORTED	4
+
 /* Global constants */
 #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
 #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
@@ -1023,8 +1026,10 @@ struct dwc3_scratchpad_array {
  * @usb_psy: pointer to power supply interface.
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
- * @usb2_generic_phy: pointer to USB2 PHY
- * @usb3_generic_phy: pointer to USB3 PHY
+ * @num_ports: Indicates number of usb ports supported by the controller.
+ * @num_ss_ports: Indicates number of ss capable ports supported by controller
+ * @usb2_generic_phy: pointer to array of USB2 PHY's
+ * @usb3_generic_phy: pointer to array of USB3 PHY's
  * @phys_ready: flag to indicate that PHYs are ready
  * @ulpi: pointer to ulpi interface
  * @ulpi_ready: flag to indicate that ULPI is initialized
@@ -1157,8 +1162,10 @@ struct dwc3 {
 	struct usb_phy		*usb2_phy;
 	struct usb_phy		*usb3_phy;
 
-	struct phy		*usb2_generic_phy;
-	struct phy		*usb3_generic_phy;
+	u32			num_ports;
+	u32			num_ss_ports;
+	struct phy		*usb2_generic_phy[MAX_PORTS_SUPPORTED];
+	struct phy		*usb3_generic_phy[MAX_PORTS_SUPPORTED];
 
 	bool			phys_ready;
 
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 039bf241769a..ea86ff01433b 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -327,7 +327,7 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc)
 
 void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
 {
-	int ret;
+	int ret, i;
 	u32 reg;
 	int id;
 	unsigned long flags;
@@ -386,9 +386,11 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
 		} else {
 			if (dwc->usb2_phy)
 				otg_set_vbus(dwc->usb2_phy->otg, true);
-			if (dwc->usb2_generic_phy)
-				phy_set_mode(dwc->usb2_generic_phy,
-					     PHY_MODE_USB_HOST);
+			for (i = 0; i < dwc->num_ports; i++) {
+				if (dwc->usb2_generic_phy[i])
+					phy_set_mode(dwc->usb2_generic_phy[i],
+						     PHY_MODE_USB_HOST);
+			}
 		}
 		break;
 	case DWC3_OTG_ROLE_DEVICE:
@@ -400,8 +402,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
 
 		if (dwc->usb2_phy)
 			otg_set_vbus(dwc->usb2_phy->otg, false);
-		if (dwc->usb2_generic_phy)
-			phy_set_mode(dwc->usb2_generic_phy,
+		if (dwc->usb2_generic_phy[0])
+			phy_set_mode(dwc->usb2_generic_phy[0],
 				     PHY_MODE_USB_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret)
-- 
2.39.0

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

Hi,

On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller
> which requires at most one HS and one SS PHY.

Add note here that multi-port is for host mode for clarity.

> 
> But the DWC3 USB controller can be connected to multiple ports and
> each port can have their own PHYs. Each port of the multiport
> controller can either be HS+SS capable or HS only capable
> Proper quantification of them is required to modify GUSB2PHYCFG
> and GUSB3PIPECTL registers appropriately.
> 
> Add support for detecting, obtaining and configuring phy's supported
> by a multiport controller and limit the max number of ports
> supported to 4.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>  drivers/usb/dwc3/core.h |  15 +-
>  drivers/usb/dwc3/drd.c  |  14 +-
>  3 files changed, 244 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..7e0a9a598dfd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>  {
>  	struct dwc3 *dwc = work_to_dwc(work);
>  	unsigned long flags;
> -	int ret;
> +	int ret, i;

Can we declare variables in separate lines here and other places.

>  	u32 reg;
>  	u32 desired_dr_role;
>  
> @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		} else {
>  			if (dwc->usb2_phy)
>  				otg_set_vbus(dwc->usb2_phy->otg, true);
> -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> +			for (i = 0; i < dwc->num_ports; i++) {
> +				phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
> +				phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
> +			}
>  			if (dwc->dis_split_quirk) {
>  				reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
>  				reg |= DWC3_GUCTL3_SPLITDISABLE;
> @@ -216,8 +218,8 @@ static void __dwc3_set_mode(struct work_struct *work)
>  
>  		if (dwc->usb2_phy)
>  			otg_set_vbus(dwc->usb2_phy->otg, false);
> -		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
> -		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
> +		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
> +		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
>  
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret)
> @@ -659,22 +661,14 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> -/**
> - * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
> - * @dwc: Pointer to our controller context structure
> - *
> - * Returns 0 on success. The USB PHY interfaces are configured but not
> - * initialized. The PHY interfaces and the PHYs get initialized together with
> - * the core in dwc3_core_init.
> - */
> -static int dwc3_phy_setup(struct dwc3 *dwc)
> +static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
>  {
>  	unsigned int hw_mode;
>  	u32 reg;
>  
>  	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>  
> -	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(index));
>  
>  	/*
>  	 * Make sure UX_EXIT_PX is cleared as that causes issues with some
> @@ -729,9 +723,19 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	if (dwc->dis_del_phy_power_chg_quirk)
>  		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
>  
> -	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> +	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);
>  
> -	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +	return 0;
> +}
> +
> +static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index)
> +{
> +	unsigned int hw_mode;
> +	u32 reg;
> +
> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
>  
>  	/* Select the HS PHY interface */
>  	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
> @@ -743,7 +747,7 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  		} else if (dwc->hsphy_interface &&
>  				!strncmp(dwc->hsphy_interface, "ulpi", 4)) {
>  			reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
>  		} else {
>  			/* Relying on default value. */
>  			if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI))
> @@ -800,7 +804,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	if (dwc->dis_u2_freeclk_exists_quirk || dwc->gfladj_refclk_lpm_sel)
>  		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
>  
> -	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
> +
> +	return 0;
> +}
> +
> +/**
> + * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
> + * @dwc: Pointer to our controller context structure
> + *
> + * Returns 0 on success. The USB PHY interfaces are configured but not
> + * initialized. The PHY interfaces and the PHYs get initialized together with
> + * the core in dwc3_core_init.
> + */
> +static int dwc3_phy_setup(struct dwc3 *dwc)
> +{
> +	int i;
> +	int ret;
> +
> +	for (i = 0; i < dwc->num_ss_ports; i++) {
> +		ret = dwc3_ss_phy_setup(dwc, i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = dwc3_hs_phy_setup(dwc, i);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -839,17 +871,25 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
>  
>  static void dwc3_core_exit(struct dwc3 *dwc)
>  {
> +	int i;
> +
>  	dwc3_event_buffers_cleanup(dwc);
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> -	phy_power_off(dwc->usb2_generic_phy);
> -	phy_power_off(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_power_off(dwc->usb2_generic_phy[i]);
> +		phy_power_off(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
> -	phy_exit(dwc->usb2_generic_phy);
> -	phy_exit(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_exit(dwc->usb2_generic_phy[i]);
> +		phy_exit(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	dwc3_clk_disable(dwc);
>  	reset_control_assert(dwc->reset);
> @@ -1085,6 +1125,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	unsigned int		hw_mode;
>  	u32			reg;
>  	int			ret;
> +	int			i, j;
>  
>  	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>  
> @@ -1119,14 +1160,27 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  
>  	usb_phy_init(dwc->usb2_phy);
>  	usb_phy_init(dwc->usb3_phy);
> -	ret = phy_init(dwc->usb2_generic_phy);
> -	if (ret < 0)
> -		goto err0a;
>  
> -	ret = phy_init(dwc->usb3_generic_phy);
> -	if (ret < 0) {
> -		phy_exit(dwc->usb2_generic_phy);
> -		goto err0a;
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_init(dwc->usb2_generic_phy[i]);
> +		if (ret < 0) {
> +			/* clean up prior initialized HS PHYs */
> +			for (j = 0; j < i; j++)
> +				phy_exit(dwc->usb2_generic_phy[j]);
> +			goto err0a;
> +		}
> +	}
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_init(dwc->usb3_generic_phy[i]);
> +		if (ret < 0) {
> +			/* clean up prior initialized SS PHYs */
> +			for (j = 0; j < i; j++)
> +				phy_exit(dwc->usb3_generic_phy[j]);
> +			for (j = 0; j < dwc->num_ports; j++)
> +				phy_exit(dwc->usb2_generic_phy[j]);
> +			goto err0a;
> +		}
>  	}
>  
>  	ret = dwc3_core_soft_reset(dwc);
> @@ -1136,15 +1190,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
>  	    !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
>  		if (!dwc->dis_u3_susphy_quirk) {
> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> -			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> -			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> +			for (i = 0; i < dwc->num_ss_ports; i++) {
> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
> +				reg |= DWC3_GUSB3PIPECTL_SUSPHY;
> +				dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg);
> +			}
>  		}
>  
>  		if (!dwc->dis_u2_susphy_quirk) {
> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +			for (i = 0; i < dwc->num_ports; i++) {
> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
> +				reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
> +			}
>  		}
>  	}
>  
> @@ -1168,13 +1226,25 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 0);
>  	usb_phy_set_suspend(dwc->usb3_phy, 0);
> -	ret = phy_power_on(dwc->usb2_generic_phy);
> -	if (ret < 0)
> -		goto err2;
>  
> -	ret = phy_power_on(dwc->usb3_generic_phy);
> -	if (ret < 0)
> -		goto err3;
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_power_on(dwc->usb2_generic_phy[i]);
> +		if (ret < 0) {
> +			for (j = 0; j < i; j++)
> +				phy_power_off(dwc->usb2_generic_phy[j]);
> +			goto err2;
> +		}
> +	}
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_power_on(dwc->usb3_generic_phy[i]);
> +		if (ret < 0) {
> +			for (j = 0; j < i; j++)
> +				phy_power_off(dwc->usb3_generic_phy[j]);
> +			goto err3;
> +		}
> +	}
>  
>  	ret = dwc3_event_buffers_setup(dwc);
>  	if (ret) {
> @@ -1297,10 +1367,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	return 0;
>  
>  err4:
> -	phy_power_off(dwc->usb3_generic_phy);
> +	for (i = 0; i < dwc->num_ports; i++)
> +		phy_power_off(dwc->usb3_generic_phy[i]);
>  
>  err3:
> -	phy_power_off(dwc->usb2_generic_phy);
> +	for (i = 0; i < dwc->num_ports; i++)
> +		phy_power_off(dwc->usb2_generic_phy[i]);
>  
>  err2:
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
> @@ -1309,8 +1381,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  err1:
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
> -	phy_exit(dwc->usb2_generic_phy);
> -	phy_exit(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_exit(dwc->usb2_generic_phy[i]);
> +		phy_exit(dwc->usb3_generic_phy[i]);
> +	}
>  
>  err0a:
>  	dwc3_ulpi_exit(dwc);
> @@ -1319,6 +1394,38 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +static int dwc3_get_multiport_phys(struct dwc3 *dwc)
> +{
> +	int ret;
> +	struct device *dev = dwc->dev;
> +	int i;
> +	char phy_name[15];
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		sprintf(phy_name, "usb2-phy_port%d", i);
> +		dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name);
> +		if (IS_ERR(dwc->usb2_generic_phy[i])) {
> +			ret = PTR_ERR(dwc->usb2_generic_phy[i]);
> +			if (ret == -ENOSYS || ret == -ENODEV)
> +				dwc->usb2_generic_phy[i] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "usb2 phy: %s not configured\n", phy_name);
> +		}
> +
> +		sprintf(phy_name, "usb3-phy_port%d", i);
> +		dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
> +		if (IS_ERR(dwc->usb3_generic_phy[i])) {
> +			ret = PTR_ERR(dwc->usb3_generic_phy[i]);
> +			if (ret == -ENOSYS || ret == -ENODEV)
> +				dwc->usb3_generic_phy[i] = NULL;
> +			else
> +				return dev_err_probe(dev, ret, "usb3 phy: %s not configured\n", phy_name);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int dwc3_core_get_phy(struct dwc3 *dwc)
>  {
>  	struct device		*dev = dwc->dev;
> @@ -1349,31 +1456,37 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>  			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>  	}
>  
> -	dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> -	if (IS_ERR(dwc->usb2_generic_phy)) {
> -		ret = PTR_ERR(dwc->usb2_generic_phy);
> +	if (dwc->num_ports > 1)
> +		goto get_multiport_phys;

Can we avoid goto and just return dwc3_get_multiport_phys(dwc) here?
It's easier to read IMO.

> +
> +	dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy");
> +	if (IS_ERR(dwc->usb2_generic_phy[0])) {
> +		ret = PTR_ERR(dwc->usb2_generic_phy[0]);
>  		if (ret == -ENOSYS || ret == -ENODEV)
> -			dwc->usb2_generic_phy = NULL;
> +			dwc->usb2_generic_phy[0] = NULL;
>  		else
>  			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>  	}
>  
> -	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> -	if (IS_ERR(dwc->usb3_generic_phy)) {
> -		ret = PTR_ERR(dwc->usb3_generic_phy);
> +	dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy");
> +	if (IS_ERR(dwc->usb3_generic_phy[0])) {
> +		ret = PTR_ERR(dwc->usb3_generic_phy[0]);
>  		if (ret == -ENOSYS || ret == -ENODEV)
> -			dwc->usb3_generic_phy = NULL;
> +			dwc->usb3_generic_phy[0] = NULL;
>  		else
>  			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>  	}
>  
>  	return 0;
> +
> +get_multiport_phys:
> +	return dwc3_get_multiport_phys(dwc);
>  }
>  
>  static int dwc3_core_init_mode(struct dwc3 *dwc)
>  {
>  	struct device *dev = dwc->dev;
> -	int ret;
> +	int ret, i;
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> @@ -1381,8 +1494,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  
>  		if (dwc->usb2_phy)
>  			otg_set_vbus(dwc->usb2_phy->otg, false);
> -		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
> -		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
> +		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
> +		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
>  
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret)
> @@ -1393,8 +1506,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  
>  		if (dwc->usb2_phy)
>  			otg_set_vbus(dwc->usb2_phy->otg, true);
> -		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> -		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> +		for (i = 0; i < dwc->num_ports; i++) {
> +			phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
> +			phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
> +		}
>  
>  		ret = dwc3_host_init(dwc);
>  		if (ret)
> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +
> +	/*
> +	 * If no mulitport properties are defined, default

multi*

> +	 * the port count to '1'.
> +	 */

Can we initialize num_ports and num_ss_ports to 1 instead of setting it
on error (similar to how we handle other properties).

> +	ret = device_property_read_u32(dev, "num-ports",
> +				&dwc->num_ports);
> +	if (ret)
> +		dwc->num_ports = 1;
> +
> +	ret = device_property_read_u32(dev, "num-ss-ports",
> +				&dwc->num_ss_ports);
> +	if (ret)
> +		dwc->num_ss_ports = 1;
> +
>  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>  	dwc->tx_de_emphasis = tx_de_emphasis;
>  
> @@ -1755,8 +1885,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	struct device		*dev = &pdev->dev;
>  	struct resource		*res, dwc_res;
>  	struct dwc3		*dwc;
> -
> -	int			ret;
> +	int			ret, i;
>  
>  	void __iomem		*regs;
>  
> @@ -1933,17 +2062,24 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  err5:
>  	dwc3_debugfs_exit(dwc);
> +
>  	dwc3_event_buffers_cleanup(dwc);
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> -	phy_power_off(dwc->usb2_generic_phy);
> -	phy_power_off(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_power_off(dwc->usb2_generic_phy[i]);
> +		phy_power_off(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
> -	phy_exit(dwc->usb2_generic_phy);
> -	phy_exit(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_exit(dwc->usb2_generic_phy[i]);
> +		phy_exit(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	dwc3_ulpi_exit(dwc);
>  
> @@ -2025,6 +2161,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>  
>  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
> +	int i;
>  	unsigned long	flags;
>  	u32 reg;
>  
> @@ -2045,17 +2182,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  		/* Let controller to suspend HSPHY before PHY driver suspends */
>  		if (dwc->dis_u2_susphy_quirk ||
>  		    dwc->dis_enblslpm_quirk) {
> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -			reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
> -				DWC3_GUSB2PHYCFG_SUSPHY;
> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +			for (i = 0; i < dwc->num_ports; i++) {
> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
> +				reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
> +					DWC3_GUSB2PHYCFG_SUSPHY;
> +				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
> +			}
>  
>  			/* Give some time for USB2 PHY to suspend */
>  			usleep_range(5000, 6000);
>  		}
>  
> -		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
> -		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
> +		for (i = 0; i < dwc->num_ports; i++) {
> +			phy_pm_runtime_put_sync(dwc->usb2_generic_phy[i]);
> +			phy_pm_runtime_put_sync(dwc->usb3_generic_phy[i]);
> +		}
>  		break;
>  	case DWC3_GCTL_PRTCAP_OTG:
>  		/* do nothing during runtime_suspend */
> @@ -2084,6 +2225,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	unsigned long	flags;
>  	int		ret;
> +	int		i;
>  	u32		reg;
>  
>  	switch (dwc->current_dr_role) {
> @@ -2104,17 +2246,21 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  			break;
>  		}
>  		/* Restore GUSB2PHYCFG bits that were modified in suspend */
> -		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -		if (dwc->dis_u2_susphy_quirk)
> -			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> +		for (i = 0; i < dwc->num_ports; i++) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
> +			if (dwc->dis_u2_susphy_quirk)
> +				reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>  
> -		if (dwc->dis_enblslpm_quirk)
> -			reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
> +			if (dwc->dis_enblslpm_quirk)
> +				reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
>  
> -		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
> +		}
>  
> -		phy_pm_runtime_get_sync(dwc->usb2_generic_phy);
> -		phy_pm_runtime_get_sync(dwc->usb3_generic_phy);
> +		for (i = 0; i < dwc->num_ports; i++) {
> +			phy_pm_runtime_get_sync(dwc->usb2_generic_phy[i]);
> +			phy_pm_runtime_get_sync(dwc->usb3_generic_phy[i]);
> +		}
>  		break;
>  	case DWC3_GCTL_PRTCAP_OTG:
>  		/* nothing to do on runtime_resume */
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 8f9959ba9fd4..2f82eda9d44f 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -35,6 +35,9 @@
>  
>  #define DWC3_MSG_MAX	500
>  
> +/* Number of ports supported by a multiport controller */
> +#define MAX_PORTS_SUPPORTED	4
> +
>  /* Global constants */
>  #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
>  #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
> @@ -1023,8 +1026,10 @@ struct dwc3_scratchpad_array {
>   * @usb_psy: pointer to power supply interface.
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
> - * @usb2_generic_phy: pointer to USB2 PHY
> - * @usb3_generic_phy: pointer to USB3 PHY
> + * @num_ports: Indicates number of usb ports supported by the controller.

Note that this is the total number of usb ports including the SS capable
ones.

> + * @num_ss_ports: Indicates number of ss capable ports supported by controller
> + * @usb2_generic_phy: pointer to array of USB2 PHY's
> + * @usb3_generic_phy: pointer to array of USB3 PHY's
>   * @phys_ready: flag to indicate that PHYs are ready
>   * @ulpi: pointer to ulpi interface
>   * @ulpi_ready: flag to indicate that ULPI is initialized
> @@ -1157,8 +1162,10 @@ struct dwc3 {
>  	struct usb_phy		*usb2_phy;
>  	struct usb_phy		*usb3_phy;
>  
> -	struct phy		*usb2_generic_phy;
> -	struct phy		*usb3_generic_phy;
> +	u32			num_ports;
> +	u32			num_ss_ports;
> +	struct phy		*usb2_generic_phy[MAX_PORTS_SUPPORTED];
> +	struct phy		*usb3_generic_phy[MAX_PORTS_SUPPORTED];
>  
>  	bool			phys_ready;
>  
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 039bf241769a..ea86ff01433b 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -327,7 +327,7 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc)
>  
>  void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>  {
> -	int ret;
> +	int ret, i;
>  	u32 reg;
>  	int id;
>  	unsigned long flags;
> @@ -386,9 +386,11 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>  		} else {
>  			if (dwc->usb2_phy)
>  				otg_set_vbus(dwc->usb2_phy->otg, true);
> -			if (dwc->usb2_generic_phy)
> -				phy_set_mode(dwc->usb2_generic_phy,
> -					     PHY_MODE_USB_HOST);
> +			for (i = 0; i < dwc->num_ports; i++) {
> +				if (dwc->usb2_generic_phy[i])
> +					phy_set_mode(dwc->usb2_generic_phy[i],
> +						     PHY_MODE_USB_HOST);
> +			}
>  		}
>  		break;
>  	case DWC3_OTG_ROLE_DEVICE:
> @@ -400,8 +402,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>  
>  		if (dwc->usb2_phy)
>  			otg_set_vbus(dwc->usb2_phy->otg, false);
> -		if (dwc->usb2_generic_phy)
> -			phy_set_mode(dwc->usb2_generic_phy,
> +		if (dwc->usb2_generic_phy[0])
> +			phy_set_mode(dwc->usb2_generic_phy[0],
>  				     PHY_MODE_USB_DEVICE);
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret)
> -- 
> 2.39.0
> 

No major issue I see here. Just some minor nits. Once you feel it's
ready, please drop the RFC tags on resubmission.

Thanks,
Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
> Hi,
> 
> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>> Currently the DWC3 driver supports only single port controller
>> which requires at most one HS and one SS PHY.
> 
> Add note here that multi-port is for host mode for clarity.
> 
>>
>> But the DWC3 USB controller can be connected to multiple ports and
>> each port can have their own PHYs. Each port of the multiport
>> controller can either be HS+SS capable or HS only capable
>> Proper quantification of them is required to modify GUSB2PHYCFG
>> and GUSB3PIPECTL registers appropriately.
>>
>> Add support for detecting, obtaining and configuring phy's supported
>> by a multiport controller and limit the max number of ports
>> supported to 4.
>>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>   drivers/usb/dwc3/core.h |  15 +-
>>   drivers/usb/dwc3/drd.c  |  14 +-
>>   3 files changed, 244 insertions(+), 89 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 476b63618511..7e0a9a598dfd 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>   {
>>   	struct dwc3 *dwc = work_to_dwc(work);
>>   	unsigned long flags;
>> -	int ret;
>> +	int ret, i;
> 
> Can we declare variables in separate lines here and other places.
> 
>>   	u32 reg;
>>   	u32 desired_dr_role;
>>   
>> @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
>>   		} else {
>>   			if (dwc->usb2_phy)
>>   				otg_set_vbus(dwc->usb2_phy->otg, true);
>> -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>> -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>> +			for (i = 0; i < dwc->num_ports; i++) {
>> +				phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
>> +				phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
>> +			}
>>   			if (dwc->dis_split_quirk) {
>>   				reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
>>   				reg |= DWC3_GUCTL3_SPLITDISABLE;
>> @@ -216,8 +218,8 @@ static void __dwc3_set_mode(struct work_struct *work)
>>   
>>   		if (dwc->usb2_phy)
>>   			otg_set_vbus(dwc->usb2_phy->otg, false);
>> -		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>> -		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>> +		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
>> +		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
>>   
>>   		ret = dwc3_gadget_init(dwc);
>>   		if (ret)
>> @@ -659,22 +661,14 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
>>   	return ret;
>>   }
>>   
>> -/**
>> - * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
>> - * @dwc: Pointer to our controller context structure
>> - *
>> - * Returns 0 on success. The USB PHY interfaces are configured but not
>> - * initialized. The PHY interfaces and the PHYs get initialized together with
>> - * the core in dwc3_core_init.
>> - */
>> -static int dwc3_phy_setup(struct dwc3 *dwc)
>> +static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
>>   {
>>   	unsigned int hw_mode;
>>   	u32 reg;
>>   
>>   	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>   
>> -	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(index));
>>   
>>   	/*
>>   	 * Make sure UX_EXIT_PX is cleared as that causes issues with some
>> @@ -729,9 +723,19 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>   	if (dwc->dis_del_phy_power_chg_quirk)
>>   		reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;
>>   
>> -	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>> +	dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);
>>   
>> -	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> +	return 0;
>> +}
>> +
>> +static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index)
>> +{
>> +	unsigned int hw_mode;
>> +	u32 reg;
>> +
>> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> +
>> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
>>   
>>   	/* Select the HS PHY interface */
>>   	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
>> @@ -743,7 +747,7 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>   		} else if (dwc->hsphy_interface &&
>>   				!strncmp(dwc->hsphy_interface, "ulpi", 4)) {
>>   			reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
>> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
>>   		} else {
>>   			/* Relying on default value. */
>>   			if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI))
>> @@ -800,7 +804,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>   	if (dwc->dis_u2_freeclk_exists_quirk || dwc->gfladj_refclk_lpm_sel)
>>   		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
>>   
>> -	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> +	dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
>> + * @dwc: Pointer to our controller context structure
>> + *
>> + * Returns 0 on success. The USB PHY interfaces are configured but not
>> + * initialized. The PHY interfaces and the PHYs get initialized together with
>> + * the core in dwc3_core_init.
>> + */
>> +static int dwc3_phy_setup(struct dwc3 *dwc)
>> +{
>> +	int i;
>> +	int ret;
>> +
>> +	for (i = 0; i < dwc->num_ss_ports; i++) {
>> +		ret = dwc3_ss_phy_setup(dwc, i);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = dwc3_hs_phy_setup(dwc, i);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -839,17 +871,25 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
>>   
>>   static void dwc3_core_exit(struct dwc3 *dwc)
>>   {
>> +	int i;
>> +
>>   	dwc3_event_buffers_cleanup(dwc);
>>   
>>   	usb_phy_set_suspend(dwc->usb2_phy, 1);
>>   	usb_phy_set_suspend(dwc->usb3_phy, 1);
>> -	phy_power_off(dwc->usb2_generic_phy);
>> -	phy_power_off(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_power_off(dwc->usb2_generic_phy[i]);
>> +		phy_power_off(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	usb_phy_shutdown(dwc->usb2_phy);
>>   	usb_phy_shutdown(dwc->usb3_phy);
>> -	phy_exit(dwc->usb2_generic_phy);
>> -	phy_exit(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_exit(dwc->usb2_generic_phy[i]);
>> +		phy_exit(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	dwc3_clk_disable(dwc);
>>   	reset_control_assert(dwc->reset);
>> @@ -1085,6 +1125,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	unsigned int		hw_mode;
>>   	u32			reg;
>>   	int			ret;
>> +	int			i, j;
>>   
>>   	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>   
>> @@ -1119,14 +1160,27 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   
>>   	usb_phy_init(dwc->usb2_phy);
>>   	usb_phy_init(dwc->usb3_phy);
>> -	ret = phy_init(dwc->usb2_generic_phy);
>> -	if (ret < 0)
>> -		goto err0a;
>>   
>> -	ret = phy_init(dwc->usb3_generic_phy);
>> -	if (ret < 0) {
>> -		phy_exit(dwc->usb2_generic_phy);
>> -		goto err0a;
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_init(dwc->usb2_generic_phy[i]);
>> +		if (ret < 0) {
>> +			/* clean up prior initialized HS PHYs */
>> +			for (j = 0; j < i; j++)
>> +				phy_exit(dwc->usb2_generic_phy[j]);
>> +			goto err0a;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_init(dwc->usb3_generic_phy[i]);
>> +		if (ret < 0) {
>> +			/* clean up prior initialized SS PHYs */
>> +			for (j = 0; j < i; j++)
>> +				phy_exit(dwc->usb3_generic_phy[j]);
>> +			for (j = 0; j < dwc->num_ports; j++)
>> +				phy_exit(dwc->usb2_generic_phy[j]);
>> +			goto err0a;
>> +		}
>>   	}
>>   
>>   	ret = dwc3_core_soft_reset(dwc);
>> @@ -1136,15 +1190,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
>>   	    !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
>>   		if (!dwc->dis_u3_susphy_quirk) {
>> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>> -			reg |= DWC3_GUSB3PIPECTL_SUSPHY;
>> -			dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>> +			for (i = 0; i < dwc->num_ss_ports; i++) {
>> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
>> +				reg |= DWC3_GUSB3PIPECTL_SUSPHY;
>> +				dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg);
>> +			}
>>   		}
>>   
>>   		if (!dwc->dis_u2_susphy_quirk) {
>> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> -			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
>> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> +			for (i = 0; i < dwc->num_ports; i++) {
>> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
>> +				reg |= DWC3_GUSB2PHYCFG_SUSPHY;
>> +				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
>> +			}
>>   		}
>>   	}
>>   
>> @@ -1168,13 +1226,25 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   
>>   	usb_phy_set_suspend(dwc->usb2_phy, 0);
>>   	usb_phy_set_suspend(dwc->usb3_phy, 0);
>> -	ret = phy_power_on(dwc->usb2_generic_phy);
>> -	if (ret < 0)
>> -		goto err2;
>>   
>> -	ret = phy_power_on(dwc->usb3_generic_phy);
>> -	if (ret < 0)
>> -		goto err3;
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_power_on(dwc->usb2_generic_phy[i]);
>> +		if (ret < 0) {
>> +			for (j = 0; j < i; j++)
>> +				phy_power_off(dwc->usb2_generic_phy[j]);
>> +			goto err2;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_power_on(dwc->usb3_generic_phy[i]);
>> +		if (ret < 0) {
>> +			for (j = 0; j < i; j++)
>> +				phy_power_off(dwc->usb3_generic_phy[j]);
>> +			goto err3;
>> +		}
>> +	}
>>   
>>   	ret = dwc3_event_buffers_setup(dwc);
>>   	if (ret) {
>> @@ -1297,10 +1367,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	return 0;
>>   
>>   err4:
>> -	phy_power_off(dwc->usb3_generic_phy);
>> +	for (i = 0; i < dwc->num_ports; i++)
>> +		phy_power_off(dwc->usb3_generic_phy[i]);
>>   
>>   err3:
>> -	phy_power_off(dwc->usb2_generic_phy);
>> +	for (i = 0; i < dwc->num_ports; i++)
>> +		phy_power_off(dwc->usb2_generic_phy[i]);
>>   
>>   err2:
>>   	usb_phy_set_suspend(dwc->usb2_phy, 1);
>> @@ -1309,8 +1381,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   err1:
>>   	usb_phy_shutdown(dwc->usb2_phy);
>>   	usb_phy_shutdown(dwc->usb3_phy);
>> -	phy_exit(dwc->usb2_generic_phy);
>> -	phy_exit(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_exit(dwc->usb2_generic_phy[i]);
>> +		phy_exit(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   err0a:
>>   	dwc3_ulpi_exit(dwc);
>> @@ -1319,6 +1394,38 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	return ret;
>>   }
>>   
>> +static int dwc3_get_multiport_phys(struct dwc3 *dwc)
>> +{
>> +	int ret;
>> +	struct device *dev = dwc->dev;
>> +	int i;
>> +	char phy_name[15];
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		sprintf(phy_name, "usb2-phy_port%d", i);
>> +		dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name);
>> +		if (IS_ERR(dwc->usb2_generic_phy[i])) {
>> +			ret = PTR_ERR(dwc->usb2_generic_phy[i]);
>> +			if (ret == -ENOSYS || ret == -ENODEV)
>> +				dwc->usb2_generic_phy[i] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret, "usb2 phy: %s not configured\n", phy_name);
>> +		}
>> +
>> +		sprintf(phy_name, "usb3-phy_port%d", i);
>> +		dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
>> +		if (IS_ERR(dwc->usb3_generic_phy[i])) {
>> +			ret = PTR_ERR(dwc->usb3_generic_phy[i]);
>> +			if (ret == -ENOSYS || ret == -ENODEV)
>> +				dwc->usb3_generic_phy[i] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret, "usb3 phy: %s not configured\n", phy_name);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int dwc3_core_get_phy(struct dwc3 *dwc)
>>   {
>>   	struct device		*dev = dwc->dev;
>> @@ -1349,31 +1456,37 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>   			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>>   	}
>>   
>> -	dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> -	if (IS_ERR(dwc->usb2_generic_phy)) {
>> -		ret = PTR_ERR(dwc->usb2_generic_phy);
>> +	if (dwc->num_ports > 1)
>> +		goto get_multiport_phys;
> 
> Can we avoid goto and just return dwc3_get_multiport_phys(dwc) here?
> It's easier to read IMO.
> 
>> +
>> +	dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy");
>> +	if (IS_ERR(dwc->usb2_generic_phy[0])) {
>> +		ret = PTR_ERR(dwc->usb2_generic_phy[0]);
>>   		if (ret == -ENOSYS || ret == -ENODEV)
>> -			dwc->usb2_generic_phy = NULL;
>> +			dwc->usb2_generic_phy[0] = NULL;
>>   		else
>>   			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>>   	}
>>   
>> -	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> -	if (IS_ERR(dwc->usb3_generic_phy)) {
>> -		ret = PTR_ERR(dwc->usb3_generic_phy);
>> +	dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy");
>> +	if (IS_ERR(dwc->usb3_generic_phy[0])) {
>> +		ret = PTR_ERR(dwc->usb3_generic_phy[0]);
>>   		if (ret == -ENOSYS || ret == -ENODEV)
>> -			dwc->usb3_generic_phy = NULL;
>> +			dwc->usb3_generic_phy[0] = NULL;
>>   		else
>>   			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>>   	}
>>   
>>   	return 0;
>> +
>> +get_multiport_phys:
>> +	return dwc3_get_multiport_phys(dwc);
>>   }
>>   
>>   static int dwc3_core_init_mode(struct dwc3 *dwc)
>>   {
>>   	struct device *dev = dwc->dev;
>> -	int ret;
>> +	int ret, i;
>>   
>>   	switch (dwc->dr_mode) {
>>   	case USB_DR_MODE_PERIPHERAL:
>> @@ -1381,8 +1494,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>   
>>   		if (dwc->usb2_phy)
>>   			otg_set_vbus(dwc->usb2_phy->otg, false);
>> -		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
>> -		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
>> +		phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
>> +		phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);
>>   
>>   		ret = dwc3_gadget_init(dwc);
>>   		if (ret)
>> @@ -1393,8 +1506,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>   
>>   		if (dwc->usb2_phy)
>>   			otg_set_vbus(dwc->usb2_phy->otg, true);
>> -		phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>> -		phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>> +		for (i = 0; i < dwc->num_ports; i++) {
>> +			phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
>> +			phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
>> +		}
>>   
>>   		ret = dwc3_host_init(dwc);
>>   		if (ret)
>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>   	dwc->dis_split_quirk = device_property_read_bool(dev,
>>   				"snps,dis-split-quirk");
>>   
>> +
>> +	/*
>> +	 * If no mulitport properties are defined, default
> 
> multi*
> 
>> +	 * the port count to '1'.
>> +	 */
> 
> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
> on error (similar to how we handle other properties).
> 
Hi Thinh,

   Thanks for the review. On the bindings, Rob and Krzysztof have 
suggested to get the num-ports and num-ss-ports by counting the 
Phy-names in DT.

Since there may be many cases where the user might skip giving any Phy's 
or even skip different ports in the DT if he doesn't want to use them, 
can we design/refactor the below logic as follows while mandating the 
fact that user must give the SS Phy's if any starting from Port-0.:

num-ss-ports = max_port_index (usb3-portX) + 1
num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1

Ex: If there are 3 ports and only 1 is SS capable and user decides to 
skip port-2 HS Phy.

case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"

In both cases, only one SS is present, just the order is changed. (Not 
sure if last few ports can be made SS Capable instead of the first ports 
on any HW) ?

But according to the above formula:

In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong

I believe this covers all cases and I can read this in get_properties 
function. Let me know your opinion if this design is good to proceed 
further.

Regards,
Krishna,

>> +	ret = device_property_read_u32(dev, "num-ports",
>> +				&dwc->num_ports);
>> +	if (ret)
>> +		dwc->num_ports = 1;
>> +
>> +	ret = device_property_read_u32(dev, "num-ss-ports",
>> +				&dwc->num_ss_ports);
>> +	if (ret)
>> +		dwc->num_ss_ports = 1;
>> +
>>   	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>   	dwc->tx_de_emphasis = tx_de_emphasis;
>>   
>> @@ -1755,8 +1885,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>   	struct device		*dev = &pdev->dev;
>>   	struct resource		*res, dwc_res;
>>   	struct dwc3		*dwc;
>> -
>> -	int			ret;
>> +	int			ret, i;
>>   
>>   	void __iomem		*regs;
>>   
>> @@ -1933,17 +2062,24 @@ static int dwc3_probe(struct platform_device *pdev)
>>   
>>   err5:
>>   	dwc3_debugfs_exit(dwc);
>> +
>>   	dwc3_event_buffers_cleanup(dwc);
>>   
>>   	usb_phy_set_suspend(dwc->usb2_phy, 1);
>>   	usb_phy_set_suspend(dwc->usb3_phy, 1);
>> -	phy_power_off(dwc->usb2_generic_phy);
>> -	phy_power_off(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_power_off(dwc->usb2_generic_phy[i]);
>> +		phy_power_off(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	usb_phy_shutdown(dwc->usb2_phy);
>>   	usb_phy_shutdown(dwc->usb3_phy);
>> -	phy_exit(dwc->usb2_generic_phy);
>> -	phy_exit(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_exit(dwc->usb2_generic_phy[i]);
>> +		phy_exit(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	dwc3_ulpi_exit(dwc);
>>   
>> @@ -2025,6 +2161,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>>   
>>   static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>   {
>> +	int i;
>>   	unsigned long	flags;
>>   	u32 reg;
>>   
>> @@ -2045,17 +2182,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>   		/* Let controller to suspend HSPHY before PHY driver suspends */
>>   		if (dwc->dis_u2_susphy_quirk ||
>>   		    dwc->dis_enblslpm_quirk) {
>> -			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> -			reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
>> -				DWC3_GUSB2PHYCFG_SUSPHY;
>> -			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> +			for (i = 0; i < dwc->num_ports; i++) {
>> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
>> +				reg |=  DWC3_GUSB2PHYCFG_ENBLSLPM |
>> +					DWC3_GUSB2PHYCFG_SUSPHY;
>> +				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
>> +			}
>>   
>>   			/* Give some time for USB2 PHY to suspend */
>>   			usleep_range(5000, 6000);
>>   		}
>>   
>> -		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
>> -		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
>> +		for (i = 0; i < dwc->num_ports; i++) {
>> +			phy_pm_runtime_put_sync(dwc->usb2_generic_phy[i]);
>> +			phy_pm_runtime_put_sync(dwc->usb3_generic_phy[i]);
>> +		}
>>   		break;
>>   	case DWC3_GCTL_PRTCAP_OTG:
>>   		/* do nothing during runtime_suspend */
>> @@ -2084,6 +2225,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>   {
>>   	unsigned long	flags;
>>   	int		ret;
>> +	int		i;
>>   	u32		reg;
>>   
>>   	switch (dwc->current_dr_role) {
>> @@ -2104,17 +2246,21 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>   			break;
>>   		}
>>   		/* Restore GUSB2PHYCFG bits that were modified in suspend */
>> -		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> -		if (dwc->dis_u2_susphy_quirk)
>> -			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>> +		for (i = 0; i < dwc->num_ports; i++) {
>> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
>> +			if (dwc->dis_u2_susphy_quirk)
>> +				reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>>   
>> -		if (dwc->dis_enblslpm_quirk)
>> -			reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
>> +			if (dwc->dis_enblslpm_quirk)
>> +				reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
>>   
>> -		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
>> +		}
>>   
>> -		phy_pm_runtime_get_sync(dwc->usb2_generic_phy);
>> -		phy_pm_runtime_get_sync(dwc->usb3_generic_phy);
>> +		for (i = 0; i < dwc->num_ports; i++) {
>> +			phy_pm_runtime_get_sync(dwc->usb2_generic_phy[i]);
>> +			phy_pm_runtime_get_sync(dwc->usb3_generic_phy[i]);
>> +		}
>>   		break;
>>   	case DWC3_GCTL_PRTCAP_OTG:
>>   		/* nothing to do on runtime_resume */
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 8f9959ba9fd4..2f82eda9d44f 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -35,6 +35,9 @@
>>   
>>   #define DWC3_MSG_MAX	500
>>   
>> +/* Number of ports supported by a multiport controller */
>> +#define MAX_PORTS_SUPPORTED	4
>> +
>>   /* Global constants */
>>   #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
>>   #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
>> @@ -1023,8 +1026,10 @@ struct dwc3_scratchpad_array {
>>    * @usb_psy: pointer to power supply interface.
>>    * @usb2_phy: pointer to USB2 PHY
>>    * @usb3_phy: pointer to USB3 PHY
>> - * @usb2_generic_phy: pointer to USB2 PHY
>> - * @usb3_generic_phy: pointer to USB3 PHY
>> + * @num_ports: Indicates number of usb ports supported by the controller.
> 
> Note that this is the total number of usb ports including the SS capable
> ones.
> 
>> + * @num_ss_ports: Indicates number of ss capable ports supported by controller
>> + * @usb2_generic_phy: pointer to array of USB2 PHY's
>> + * @usb3_generic_phy: pointer to array of USB3 PHY's
>>    * @phys_ready: flag to indicate that PHYs are ready
>>    * @ulpi: pointer to ulpi interface
>>    * @ulpi_ready: flag to indicate that ULPI is initialized
>> @@ -1157,8 +1162,10 @@ struct dwc3 {
>>   	struct usb_phy		*usb2_phy;
>>   	struct usb_phy		*usb3_phy;
>>   
>> -	struct phy		*usb2_generic_phy;
>> -	struct phy		*usb3_generic_phy;
>> +	u32			num_ports;
>> +	u32			num_ss_ports;
>> +	struct phy		*usb2_generic_phy[MAX_PORTS_SUPPORTED];
>> +	struct phy		*usb3_generic_phy[MAX_PORTS_SUPPORTED];
>>   
>>   	bool			phys_ready;
>>   
>> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
>> index 039bf241769a..ea86ff01433b 100644
>> --- a/drivers/usb/dwc3/drd.c
>> +++ b/drivers/usb/dwc3/drd.c
>> @@ -327,7 +327,7 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc)
>>   
>>   void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>>   {
>> -	int ret;
>> +	int ret, i;
>>   	u32 reg;
>>   	int id;
>>   	unsigned long flags;
>> @@ -386,9 +386,11 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>>   		} else {
>>   			if (dwc->usb2_phy)
>>   				otg_set_vbus(dwc->usb2_phy->otg, true);
>> -			if (dwc->usb2_generic_phy)
>> -				phy_set_mode(dwc->usb2_generic_phy,
>> -					     PHY_MODE_USB_HOST);
>> +			for (i = 0; i < dwc->num_ports; i++) {
>> +				if (dwc->usb2_generic_phy[i])
>> +					phy_set_mode(dwc->usb2_generic_phy[i],
>> +						     PHY_MODE_USB_HOST);
>> +			}
>>   		}
>>   		break;
>>   	case DWC3_OTG_ROLE_DEVICE:
>> @@ -400,8 +402,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>>   
>>   		if (dwc->usb2_phy)
>>   			otg_set_vbus(dwc->usb2_phy->otg, false);
>> -		if (dwc->usb2_generic_phy)
>> -			phy_set_mode(dwc->usb2_generic_phy,
>> +		if (dwc->usb2_generic_phy[0])
>> +			phy_set_mode(dwc->usb2_generic_phy[0],
>>   				     PHY_MODE_USB_DEVICE);
>>   		ret = dwc3_gadget_init(dwc);
>>   		if (ret)
>> -- 
>> 2.39.0
>>
> 
> No major issue I see here. Just some minor nits. Once you feel it's
> ready, please drop the RFC tags on resubmission.

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

Hi,

On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > Currently the DWC3 driver supports only single port controller
> > > which requires at most one HS and one SS PHY.
> > 
> > Add note here that multi-port is for host mode for clarity.
> > 
> > > 
> > > But the DWC3 USB controller can be connected to multiple ports and
> > > each port can have their own PHYs. Each port of the multiport
> > > controller can either be HS+SS capable or HS only capable
> > > Proper quantification of them is required to modify GUSB2PHYCFG
> > > and GUSB3PIPECTL registers appropriately.
> > > 
> > > Add support for detecting, obtaining and configuring phy's supported
> > > by a multiport controller and limit the max number of ports
> > > supported to 4.
> > > 
> > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > >   drivers/usb/dwc3/core.h |  15 +-
> > >   drivers/usb/dwc3/drd.c  |  14 +-
> > >   3 files changed, 244 insertions(+), 89 deletions(-)
> > > 

<snip>

> > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> > >   	dwc->dis_split_quirk = device_property_read_bool(dev,
> > >   				"snps,dis-split-quirk");
> > > +
> > > +	/*
> > > +	 * If no mulitport properties are defined, default
> > 
> > multi*
> > 
> > > +	 * the port count to '1'.
> > > +	 */
> > 
> > Can we initialize num_ports and num_ss_ports to 1 instead of setting it
> > on error (similar to how we handle other properties).
> > 
> Hi Thinh,
> 
>   Thanks for the review. On the bindings, Rob and Krzysztof have suggested
> to get the num-ports and num-ss-ports by counting the Phy-names in DT.

This may be a bit problematic for non-DT device. Currently pci devices
pass fake DT properties to send these kinds of info. But that's fine,
we can enhance dwc3 for non-DT devices later.

> 
> Since there may be many cases where the user might skip giving any Phy's or
> even skip different ports in the DT if he doesn't want to use them, can we
> design/refactor the below logic as follows while mandating the fact that
> user must give the SS Phy's if any starting from Port-0.:
> 
> num-ss-ports = max_port_index (usb3-portX) + 1
> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
> 
> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
> port-2 HS Phy.
> 
> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
> 
> In both cases, only one SS is present, just the order is changed. (Not sure
> if last few ports can be made SS Capable instead of the first ports on any
> HW) ?
> 
> But according to the above formula:
> 
> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
> 

Can't we just walk through all the phy names to figure that out? Let's
not require the user to specify Port-0 is SS capable if they can skip
it.

> I believe this covers all cases and I can read this in get_properties
> function. Let me know your opinion if this design is good to proceed
> further.
> 

Thanks,
Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
> Hi,
> 
> On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>> Currently the DWC3 driver supports only single port controller
>>>> which requires at most one HS and one SS PHY.
>>>
>>> Add note here that multi-port is for host mode for clarity.
>>>
>>>>
>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>> each port can have their own PHYs. Each port of the multiport
>>>> controller can either be HS+SS capable or HS only capable
>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>> and GUSB3PIPECTL registers appropriately.
>>>>
>>>> Add support for detecting, obtaining and configuring phy's supported
>>>> by a multiport controller and limit the max number of ports
>>>> supported to 4.
>>>>
>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>
> 
> <snip>
> 
>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>    	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>    				"snps,dis-split-quirk");
>>>> +
>>>> +	/*
>>>> +	 * If no mulitport properties are defined, default
>>>
>>> multi*
>>>
>>>> +	 * the port count to '1'.
>>>> +	 */
>>>
>>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
>>> on error (similar to how we handle other properties).
>>>
>> Hi Thinh,
>>
>>    Thanks for the review. On the bindings, Rob and Krzysztof have suggested
>> to get the num-ports and num-ss-ports by counting the Phy-names in DT.
> 
> This may be a bit problematic for non-DT device. Currently pci devices
> pass fake DT properties to send these kinds of info. But that's fine,
> we can enhance dwc3 for non-DT devices later.
> 
>>
>> Since there may be many cases where the user might skip giving any Phy's or
>> even skip different ports in the DT if he doesn't want to use them, can we
>> design/refactor the below logic as follows while mandating the fact that
>> user must give the SS Phy's if any starting from Port-0.:
>>
>> num-ss-ports = max_port_index (usb3-portX) + 1
>> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
>>
>> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
>> port-2 HS Phy.
>>
>> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
>>
>> In both cases, only one SS is present, just the order is changed. (Not sure
>> if last few ports can be made SS Capable instead of the first ports on any
>> HW) ?
>>
>> But according to the above formula:
>>
>> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
>> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
>>
> 
> Can't we just walk through all the phy names to figure that out? Let's
> not require the user to specify Port-0 is SS capable if they can skip
> it.
> 
Hi Thinh,

Thanks for the review.

   May be I wasn't able to convey my intention properly in my previous 
thread. The above suggested method doesn't tell that user must not skip 
any phy.

Let us take the below case for 2 Port (HS+SS) capable controller.
If the user skips ss-phy 2, then:

phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"

We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If 
we parse phy-names and find it out, we see there are 2 hs-phy's and 
1-ssphy and num-ports = 2 and num-ss-ports = 1.

If the user skips ss-phy-1, then phy-names would be something like the 
below:

phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";

We need to handle two types of interpretations here while parsing the 
phy-names:

a) There are 2 SS Phy's and we just skipped the first one. In this 
scenario, if we consider (num-ss-ports = 2) and (num-ports = 2) by using 
the above formula as reference, we configure both GUSB3PIPECTL registers 
present in the address map although we never use SS Phy-1 but nothing 
must break. All ports would still work as the user intends with the 
exception of GUSB3PIPECTL1 (for-port1) still being modified.

b) The second interpretation is something like, port-1 is only HS 
capable and only Port-2 is SS Capable, and so in the phy-names only 
port-2 has been provided a SS Phy. Just by parsing through the 
phy-names, it would not be possible to get that info. So if we consider 
num-ss-ports as 2 in this scenario, we end up meddling with wrong 
registers (as there is only 1 GUSB3PIPECTL reg and we are assuing there 
are 2). I wanted to make sure that this scenario was not possible.

num-ss-ports = max_port_index (usb3-portX) + 1
num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1

Taking case of a quad port controller with all ports SS Capable, if the 
user wants to completely skip port-4. Then with above formula, we get 
(num-ports = 3) and (num-ss-ports = 3) by parsing the phy-names and we 
will configure exactly those dwc3-phy registers and not touch the port-4 
registers which is still fine.

Please let me know if the above idea helps us in this scenario.

Regards,
Krishna,

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > > > Currently the DWC3 driver supports only single port controller
> > > > > which requires at most one HS and one SS PHY.
> > > > 
> > > > Add note here that multi-port is for host mode for clarity.
> > > > 
> > > > > 
> > > > > But the DWC3 USB controller can be connected to multiple ports and
> > > > > each port can have their own PHYs. Each port of the multiport
> > > > > controller can either be HS+SS capable or HS only capable
> > > > > Proper quantification of them is required to modify GUSB2PHYCFG
> > > > > and GUSB3PIPECTL registers appropriately.
> > > > > 
> > > > > Add support for detecting, obtaining and configuring phy's supported
> > > > > by a multiport controller and limit the max number of ports
> > > > > supported to 4.
> > > > > 
> > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > ---
> > > > >    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > > > >    drivers/usb/dwc3/core.h |  15 +-
> > > > >    drivers/usb/dwc3/drd.c  |  14 +-
> > > > >    3 files changed, 244 insertions(+), 89 deletions(-)
> > > > > 
> > 
> > <snip>
> > 
> > > > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> > > > >    	dwc->dis_split_quirk = device_property_read_bool(dev,
> > > > >    				"snps,dis-split-quirk");
> > > > > +
> > > > > +	/*
> > > > > +	 * If no mulitport properties are defined, default
> > > > 
> > > > multi*
> > > > 
> > > > > +	 * the port count to '1'.
> > > > > +	 */
> > > > 
> > > > Can we initialize num_ports and num_ss_ports to 1 instead of setting it
> > > > on error (similar to how we handle other properties).
> > > > 
> > > Hi Thinh,
> > > 
> > >    Thanks for the review. On the bindings, Rob and Krzysztof have suggested
> > > to get the num-ports and num-ss-ports by counting the Phy-names in DT.
> > 
> > This may be a bit problematic for non-DT device. Currently pci devices
> > pass fake DT properties to send these kinds of info. But that's fine,
> > we can enhance dwc3 for non-DT devices later.
> > 
> > > 
> > > Since there may be many cases where the user might skip giving any Phy's or
> > > even skip different ports in the DT if he doesn't want to use them, can we
> > > design/refactor the below logic as follows while mandating the fact that
> > > user must give the SS Phy's if any starting from Port-0.:
> > > 
> > > num-ss-ports = max_port_index (usb3-portX) + 1
> > > num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
> > > 
> > > Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
> > > port-2 HS Phy.
> > > 
> > > case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
> > > case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
> > > 
> > > In both cases, only one SS is present, just the order is changed. (Not sure
> > > if last few ports can be made SS Capable instead of the first ports on any
> > > HW) ?
> > > 
> > > But according to the above formula:
> > > 
> > > In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
> > > In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
> > > 
> > 
> > Can't we just walk through all the phy names to figure that out? Let's
> > not require the user to specify Port-0 is SS capable if they can skip
> > it.
> > 
> Hi Thinh,
> 
> Thanks for the review.
> 
>   May be I wasn't able to convey my intention properly in my previous
> thread. The above suggested method doesn't tell that user must not skip any
> phy.
> 
> Let us take the below case for 2 Port (HS+SS) capable controller.
> If the user skips ss-phy 2, then:
> 
> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
> 
> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we
> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and
> num-ports = 2 and num-ss-ports = 1.
> 
> If the user skips ss-phy-1, then phy-names would be something like the
> below:
> 
> phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";
> 
> We need to handle two types of interpretations here while parsing the
> phy-names:
> 
> a) There are 2 SS Phy's and we just skipped the first one. In this scenario,
> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above
> formula as reference, we configure both GUSB3PIPECTL registers present in
> the address map although we never use SS Phy-1 but nothing must break. All
> ports would still work as the user intends with the exception of
> GUSB3PIPECTL1 (for-port1) still being modified.
> 
> b) The second interpretation is something like, port-1 is only HS capable
> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been
> provided a SS Phy. Just by parsing through the phy-names, it would not be
> possible to get that info. So if we consider num-ss-ports as 2 in this
> scenario, we end up meddling with wrong registers (as there is only 1
> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that
> this scenario was not possible.
> 
> num-ss-ports = max_port_index (usb3-portX) + 1
> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1
> 
> Taking case of a quad port controller with all ports SS Capable, if the user
> wants to completely skip port-4. Then with above formula, we get (num-ports
> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure
> exactly those dwc3-phy registers and not touch the port-4 registers which is
> still fine.
> 
> Please let me know if the above idea helps us in this scenario.
> 

This becomes rather more complicated because the user can skip certain
port in the DT. We have access to the host registers. Can we just
temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
capability before handing control over to the xHCI driver. We would be
able to get the num_ports and num_ss_ports then.

Similarly, the xhci driver doesn't care whether the user skips certain
port in the DT, it only checks and operates based on the capability
registers.

If we have the exact num_ports and num_ss_ports, we can be sure the
setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.

Thanks,
Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/21/2023 4:14 AM, Thinh Nguyen wrote:
> On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
>>>>> Hi,
>>>>>
>>>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>>>> Currently the DWC3 driver supports only single port controller
>>>>>> which requires at most one HS and one SS PHY.
>>>>>
>>>>> Add note here that multi-port is for host mode for clarity.
>>>>>
>>>>>>
>>>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>>>> each port can have their own PHYs. Each port of the multiport
>>>>>> controller can either be HS+SS capable or HS only capable
>>>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>>>> and GUSB3PIPECTL registers appropriately.
>>>>>>
>>>>>> Add support for detecting, obtaining and configuring phy's supported
>>>>>> by a multiport controller and limit the max number of ports
>>>>>> supported to 4.
>>>>>>
>>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>> ---
>>>>>>     drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>>>     drivers/usb/dwc3/core.h |  15 +-
>>>>>>     drivers/usb/dwc3/drd.c  |  14 +-
>>>>>>     3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>>
>>>
>>> <snip>
>>>
>>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>>>     	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>>     				"snps,dis-split-quirk");
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If no mulitport properties are defined, default
>>>>>
>>>>> multi*
>>>>>
>>>>>> +	 * the port count to '1'.
>>>>>> +	 */
>>>>>
>>>>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
>>>>> on error (similar to how we handle other properties).
>>>>>
>>>> Hi Thinh,
>>>>
>>>>     Thanks for the review. On the bindings, Rob and Krzysztof have suggested
>>>> to get the num-ports and num-ss-ports by counting the Phy-names in DT.
>>>
>>> This may be a bit problematic for non-DT device. Currently pci devices
>>> pass fake DT properties to send these kinds of info. But that's fine,
>>> we can enhance dwc3 for non-DT devices later.
>>>
>>>>
>>>> Since there may be many cases where the user might skip giving any Phy's or
>>>> even skip different ports in the DT if he doesn't want to use them, can we
>>>> design/refactor the below logic as follows while mandating the fact that
>>>> user must give the SS Phy's if any starting from Port-0.:
>>>>
>>>> num-ss-ports = max_port_index (usb3-portX) + 1
>>>> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
>>>>
>>>> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
>>>> port-2 HS Phy.
>>>>
>>>> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>>> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
>>>>
>>>> In both cases, only one SS is present, just the order is changed. (Not sure
>>>> if last few ports can be made SS Capable instead of the first ports on any
>>>> HW) ?
>>>>
>>>> But according to the above formula:
>>>>
>>>> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
>>>> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
>>>>
>>>
>>> Can't we just walk through all the phy names to figure that out? Let's
>>> not require the user to specify Port-0 is SS capable if they can skip
>>> it.
>>>
>> Hi Thinh,
>>
>> Thanks for the review.
>>
>>    May be I wasn't able to convey my intention properly in my previous
>> thread. The above suggested method doesn't tell that user must not skip any
>> phy.
>>
>> Let us take the below case for 2 Port (HS+SS) capable controller.
>> If the user skips ss-phy 2, then:
>>
>> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>
>> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we
>> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and
>> num-ports = 2 and num-ss-ports = 1.
>>
>> If the user skips ss-phy-1, then phy-names would be something like the
>> below:
>>
>> phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";
>>
>> We need to handle two types of interpretations here while parsing the
>> phy-names:
>>
>> a) There are 2 SS Phy's and we just skipped the first one. In this scenario,
>> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above
>> formula as reference, we configure both GUSB3PIPECTL registers present in
>> the address map although we never use SS Phy-1 but nothing must break. All
>> ports would still work as the user intends with the exception of
>> GUSB3PIPECTL1 (for-port1) still being modified.
>>
>> b) The second interpretation is something like, port-1 is only HS capable
>> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been
>> provided a SS Phy. Just by parsing through the phy-names, it would not be
>> possible to get that info. So if we consider num-ss-ports as 2 in this
>> scenario, we end up meddling with wrong registers (as there is only 1
>> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that
>> this scenario was not possible.
>>
>> num-ss-ports = max_port_index (usb3-portX) + 1
>> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1
>>
>> Taking case of a quad port controller with all ports SS Capable, if the user
>> wants to completely skip port-4. Then with above formula, we get (num-ports
>> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure
>> exactly those dwc3-phy registers and not touch the port-4 registers which is
>> still fine.
>>
>> Please let me know if the above idea helps us in this scenario.
>>
> 
> This becomes rather more complicated because the user can skip certain
> port in the DT. We have access to the host registers. Can we just
> temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
> capability before handing control over to the xHCI driver. We would be
> able to get the num_ports and num_ss_ports then.
> 
> Similarly, the xhci driver doesn't care whether the user skips certain
> port in the DT, it only checks and operates based on the capability
> registers.
> 
> If we have the exact num_ports and num_ss_ports, we can be sure the
> setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.
> 
Hi Thinh,

Thanks for the review.
Sure, I can explore this option and get the port info. This must make 
things easier.

Regards,
Krishna,

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/21/2023 4:14 AM, Thinh Nguyen wrote:
> On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
>>>>> Hi,
>>>>>
>>>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>>>> Currently the DWC3 driver supports only single port controller
>>>>>> which requires at most one HS and one SS PHY.
>>>>>
>>>>> Add note here that multi-port is for host mode for clarity.
>>>>>
>>>>>>
>>>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>>>> each port can have their own PHYs. Each port of the multiport
>>>>>> controller can either be HS+SS capable or HS only capable
>>>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>>>> and GUSB3PIPECTL registers appropriately.
>>>>>>
>>>>>> Add support for detecting, obtaining and configuring phy's supported
>>>>>> by a multiport controller and limit the max number of ports
>>>>>> supported to 4.
>>>>>>
>>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>> ---
>>>>>>     drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>>>     drivers/usb/dwc3/core.h |  15 +-
>>>>>>     drivers/usb/dwc3/drd.c  |  14 +-
>>>>>>     3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>>
>>>
>>> <snip>
>>>
>>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>>>     	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>>     				"snps,dis-split-quirk");
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If no mulitport properties are defined, default
>>>>>
>>>>> multi*
>>>>>
>>>>>> +	 * the port count to '1'.
>>>>>> +	 */
>>>>>
>>>>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
>>>>> on error (similar to how we handle other properties).
>>>>>
>>>> Hi Thinh,
>>>>
>>>>     Thanks for the review. On the bindings, Rob and Krzysztof have suggested
>>>> to get the num-ports and num-ss-ports by counting the Phy-names in DT.
>>>
>>> This may be a bit problematic for non-DT device. Currently pci devices
>>> pass fake DT properties to send these kinds of info. But that's fine,
>>> we can enhance dwc3 for non-DT devices later.
>>>
>>>>
>>>> Since there may be many cases where the user might skip giving any Phy's or
>>>> even skip different ports in the DT if he doesn't want to use them, can we
>>>> design/refactor the below logic as follows while mandating the fact that
>>>> user must give the SS Phy's if any starting from Port-0.:
>>>>
>>>> num-ss-ports = max_port_index (usb3-portX) + 1
>>>> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
>>>>
>>>> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
>>>> port-2 HS Phy.
>>>>
>>>> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>>> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
>>>>
>>>> In both cases, only one SS is present, just the order is changed. (Not sure
>>>> if last few ports can be made SS Capable instead of the first ports on any
>>>> HW) ?
>>>>
>>>> But according to the above formula:
>>>>
>>>> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
>>>> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
>>>>
>>>
>>> Can't we just walk through all the phy names to figure that out? Let's
>>> not require the user to specify Port-0 is SS capable if they can skip
>>> it.
>>>
>> Hi Thinh,
>>
>> Thanks for the review.
>>
>>    May be I wasn't able to convey my intention properly in my previous
>> thread. The above suggested method doesn't tell that user must not skip any
>> phy.
>>
>> Let us take the below case for 2 Port (HS+SS) capable controller.
>> If the user skips ss-phy 2, then:
>>
>> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>
>> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we
>> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and
>> num-ports = 2 and num-ss-ports = 1.
>>
>> If the user skips ss-phy-1, then phy-names would be something like the
>> below:
>>
>> phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";
>>
>> We need to handle two types of interpretations here while parsing the
>> phy-names:
>>
>> a) There are 2 SS Phy's and we just skipped the first one. In this scenario,
>> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above
>> formula as reference, we configure both GUSB3PIPECTL registers present in
>> the address map although we never use SS Phy-1 but nothing must break. All
>> ports would still work as the user intends with the exception of
>> GUSB3PIPECTL1 (for-port1) still being modified.
>>
>> b) The second interpretation is something like, port-1 is only HS capable
>> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been
>> provided a SS Phy. Just by parsing through the phy-names, it would not be
>> possible to get that info. So if we consider num-ss-ports as 2 in this
>> scenario, we end up meddling with wrong registers (as there is only 1
>> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that
>> this scenario was not possible.
>>
>> num-ss-ports = max_port_index (usb3-portX) + 1
>> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1
>>
>> Taking case of a quad port controller with all ports SS Capable, if the user
>> wants to completely skip port-4. Then with above formula, we get (num-ports
>> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure
>> exactly those dwc3-phy registers and not touch the port-4 registers which is
>> still fine.
>>
>> Please let me know if the above idea helps us in this scenario.
>>
> 
> This becomes rather more complicated because the user can skip certain
> port in the DT. We have access to the host registers. Can we just
> temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
> capability before handing control over to the xHCI driver. We would be
> able to get the num_ports and num_ss_ports then.
> 
> Similarly, the xhci driver doesn't care whether the user skips certain
> port in the DT, it only checks and operates based on the capability
> registers.
> 
> If we have the exact num_ports and num_ss_ports, we can be sure the
> setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.
> 

Hi Thinh,

   Thanks for the suggestion. Is the following diff / implementation 
good enough ? I Wanted to get it clarified from upstream as I am using 
*ioremap/iounmap* directly instead of *devm_* API's

I tested it and it works fine on SA8295P. Will do some further testing 
on other devices as well.


+static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
+{
+       void __iomem            *regs;
+       struct resource         dwc_res;
+       unsigned int            hw_mode;
+       u32                     offset;
+       u32                     temp;
+       u8                      major_revision;
+       u8                      minor_revision;
+
+       /*
+        * Request memory region including xHCI regs,
+        * since it is needed to get port info
+        */
+       dwc_res = *res;
+       dwc_res.start += 0;
+
+       regs = ioremap(dwc_res.start, resource_size(&dwc_res));
+       if (IS_ERR(regs)) {
+               return PTR_ERR(regs);
+       }
+
+       /*
+        * If the controller is not host-only, then it must be a
+        * single port controller.
+        */
+       temp = readl(regs + DWC3_GHWPARAMS0);
+       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
+       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
+               dwc->num_ports = 1;
+               dwc->num_ss_ports = 1;
+               return 0;
+       }
+
+       offset = xhci_find_next_ext_cap(regs, 0,
+                                       XHCI_EXT_CAPS_PROTOCOL);
+       while (offset) {
+               temp = readl(regs + offset);
+               major_revision = XHCI_EXT_PORT_MAJOR(temp);;
+               minor_revision = XHCI_EXT_PORT_MINOR(temp);
+
+               temp = readl(regs + offset + 0x08);
+               if (major_revision == 0x03) {
+                       dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
+               } else if (major_revision <= 0x02) {
+                       dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
+               } else {
+                       dev_err(dwc->dev, "revision gone wrong\n");
+                       return -EINVAL;
+               }
+
+               offset = xhci_find_next_ext_cap(regs, offset,
+                                               XHCI_EXT_CAPS_PROTOCOL);
+       }
+
+       temp = readl(regs + DWC3_XHCI_HCSPARAMS1_OFFSET);
+       if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
+               dev_err(dwc->dev, "inconsistency in port info\n");
+               return -EINVAL;
+       }
+
+       dev_info(dwc->dev, "num_ports: %d, num_ss_ports: %d\n", 
dwc->num_ports, dwc->num_ss_ports);
+       iounmap(regs);
+       return 0;
+}
+
  static int dwc3_probe(struct platform_device *pdev)
  {
         struct device           *dev = &pdev->dev;
@@ -1912,6 +1964,10 @@ static int dwc3_probe(struct platform_device *pdev)
         dwc->xhci_resources[0].flags = res->flags;
         dwc->xhci_resources[0].name = res->name;

+       ret = dwc3_read_port_info(dwc, res);
+       if (ret)
+               return ret;
+
         /*
          * Request memory region but exclude xHCI regs,
          * since it will be requested by the xhci-plat driver.
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2f82eda9d44f..8535425b81d4 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -38,6 +38,9 @@
  /* Numer of ports supported by a multiport controller */
  #define MAX_PORTS_SUPPORTED    4

+/* XHCI Reg constants */
+#define DWC3_XHCI_HCSPARAMS1_OFFSET    0x04
+
  /* Global constants */
  #define DWC3_PULL_UP_TIMEOUT   500     /* ms */
  #define DWC3_BOUNCE_SIZE       1024    /* size of a superspeed bulk */



Please let me know if this would be acceptable.

BR,
Krishna,

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

On Wed, Jan 25, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/21/2023 4:14 AM, Thinh Nguyen wrote:
> > 
> > This becomes rather more complicated because the user can skip certain
> > port in the DT. We have access to the host registers. Can we just
> > temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
> > capability before handing control over to the xHCI driver. We would be
> > able to get the num_ports and num_ss_ports then.
> > 
> > Similarly, the xhci driver doesn't care whether the user skips certain
> > port in the DT, it only checks and operates based on the capability
> > registers.
> > 
> > If we have the exact num_ports and num_ss_ports, we can be sure the
> > setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.
> > 
> 
> Hi Thinh,
> 
>   Thanks for the suggestion. Is the following diff / implementation good
> enough ? I Wanted to get it clarified from upstream as I am using
> *ioremap/iounmap* directly instead of *devm_* API's
> 
> I tested it and it works fine on SA8295P. Will do some further testing on
> other devices as well.
> 
> 
> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
> +{
> +       void __iomem            *regs;
> +       struct resource         dwc_res;
> +       unsigned int            hw_mode;
> +       u32                     offset;
> +       u32                     temp;
> +       u8                      major_revision;
> +       u8                      minor_revision;
> +
> +       /*
> +        * Request memory region including xHCI regs,
> +        * since it is needed to get port info
> +        */
> +       dwc_res = *res;
> +       dwc_res.start += 0;
> +
> +       regs = ioremap(dwc_res.start, resource_size(&dwc_res));
> +       if (IS_ERR(regs)) {
> +               return PTR_ERR(regs);
> +       }

We don't need to ioremap the whole region. Just do it for
the xhci_resources[0]

> +
> +       /*
> +        * If the controller is not host-only, then it must be a
> +        * single port controller.
> +        */
> +       temp = readl(regs + DWC3_GHWPARAMS0);
> +       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
> +       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> +               dwc->num_ports = 1;
> +               dwc->num_ss_ports = 1;
> +               return 0;
> +       }

This check should be done before we get into this function.

> +
> +       offset = xhci_find_next_ext_cap(regs, 0,
> +                                       XHCI_EXT_CAPS_PROTOCOL);
> +       while (offset) {
> +               temp = readl(regs + offset);
> +               major_revision = XHCI_EXT_PORT_MAJOR(temp);;
> +               minor_revision = XHCI_EXT_PORT_MINOR(temp);

We probably don't need minor revision.

> +
> +               temp = readl(regs + offset + 0x08);
> +               if (major_revision == 0x03) {
> +                       dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
> +               } else if (major_revision <= 0x02) {
> +                       dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
> +               } else {
> +                       dev_err(dwc->dev, "revision gone wrong\n");
> +                       return -EINVAL;
> +               }
> +
> +               offset = xhci_find_next_ext_cap(regs, offset,
> +                                               XHCI_EXT_CAPS_PROTOCOL);
> +       }
> +
> +       temp = readl(regs + DWC3_XHCI_HCSPARAMS1_OFFSET);
> +       if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
> +               dev_err(dwc->dev, "inconsistency in port info\n");
> +               return -EINVAL;
> +       }
> +
> +       dev_info(dwc->dev, "num_ports: %d, num_ss_ports: %d\n",
> dwc->num_ports, dwc->num_ss_ports);
> +       iounmap(regs);

Make sure to iounmap on all the early return/error cases.

> +       return 0;
> +}
> +
>  static int dwc3_probe(struct platform_device *pdev)
>  {
>         struct device           *dev = &pdev->dev;
> @@ -1912,6 +1964,10 @@ static int dwc3_probe(struct platform_device *pdev)
>         dwc->xhci_resources[0].flags = res->flags;
>         dwc->xhci_resources[0].name = res->name;
> 
> +       ret = dwc3_read_port_info(dwc, res);

This should be called after some initializations to make sure some
clocks are enabled. Otherwise some devices may not able to access the
registers. Preferably after dwc3_cache_hwparams() but before
dwc3_core_init().

> +       if (ret)
> +               return ret;
> +
>         /*
>          * Request memory region but exclude xHCI regs,
>          * since it will be requested by the xhci-plat driver.
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f82eda9d44f..8535425b81d4 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -38,6 +38,9 @@
>  /* Numer of ports supported by a multiport controller */
>  #define MAX_PORTS_SUPPORTED    4
> 
> +/* XHCI Reg constants */
> +#define DWC3_XHCI_HCSPARAMS1_OFFSET    0x04

Change to DWC3_XHCI_HCSPARAMS1

> +
>  /* Global constants */
>  #define DWC3_PULL_UP_TIMEOUT   500     /* ms */
>  #define DWC3_BOUNCE_SIZE       1024    /* size of a superspeed bulk */
> 
> 
> 
> Please let me know if this would be acceptable.
> 

It looks fine to me. Please review the comments and remmove debug
prints and any other cleanup for a proper patch.

Thanks,
Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Jack Pham]

On Wed, Jan 25, 2023 at 07:08:10PM +0000, Thinh Nguyen wrote:

<snip>

> > +       /*
> > +        * If the controller is not host-only, then it must be a
> > +        * single port controller.
> > +        */

Thinh, is this a correct assumption?  Is it possible for the IP to be
synthesized to support both dual-role and multiple ports?  We know that
when operating in device mode only the first port can be used but the
additional ports would be usable when in host.

Thanks,
Jack

> > +       temp = readl(regs + DWC3_GHWPARAMS0);
> > +       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
> > +       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> > +               dwc->num_ports = 1;
> > +               dwc->num_ss_ports = 1;
> > +               return 0;
> > +       }
> 
> This check should be done before we get into this function.

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

On Wed, Jan 25, 2023, Jack Pham wrote:
> On Wed, Jan 25, 2023 at 07:08:10PM +0000, Thinh Nguyen wrote:
> 
> <snip>
> 
> > > +       /*
> > > +        * If the controller is not host-only, then it must be a
> > > +        * single port controller.
> > > +        */
> 
> Thinh, is this a correct assumption?  Is it possible for the IP to be
> synthesized to support both dual-role and multiple ports?  We know that
> when operating in device mode only the first port can be used but the
> additional ports would be usable when in host.
> 

Yes. Maybe we should rephrase it a bit. Perhaps something like this:
"Currently, only host mode supports multi-port"

In case this may change in the future.

BR,
Thinh


> 
> > > +       temp = readl(regs + DWC3_GHWPARAMS0);
> > > +       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
> > > +       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> > > +               dwc->num_ports = 1;
> > > +               dwc->num_ss_ports = 1;
> > > +               return 0;
> > > +       }
> > 
> > This check should be done before we get into this function.

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

On Thu, Jan 19, 2023, Thinh Nguyen wrote:
> Hi,
> 
> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > Currently the DWC3 driver supports only single port controller
> > which requires at most one HS and one SS PHY.
> 
> Add note here that multi-port is for host mode for clarity.
> 
> > 
> > But the DWC3 USB controller can be connected to multiple ports and
> > each port can have their own PHYs. Each port of the multiport
> > controller can either be HS+SS capable or HS only capable
> > Proper quantification of them is required to modify GUSB2PHYCFG
> > and GUSB3PIPECTL registers appropriately.
> > 
> > Add support for detecting, obtaining and configuring phy's supported
> > by a multiport controller and limit the max number of ports
> > supported to 4.
> > 
> > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >  drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> >  drivers/usb/dwc3/core.h |  15 +-
> >  drivers/usb/dwc3/drd.c  |  14 +-
> >  3 files changed, 244 insertions(+), 89 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 476b63618511..7e0a9a598dfd 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
> >  {
> >  	struct dwc3 *dwc = work_to_dwc(work);
> >  	unsigned long flags;
> > -	int ret;
> > +	int ret, i;
> 
> Can we declare variables in separate lines here and other places.
> 
> >  	u32 reg;
> >  	u32 desired_dr_role;
> >  
> > @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
> >  		} else {
> >  			if (dwc->usb2_phy)
> >  				otg_set_vbus(dwc->usb2_phy->otg, true);
> > -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> > -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> > +			for (i = 0; i < dwc->num_ports; i++) {

BTW, is num_ports the total of usb2 + usb3 ports?

> > +				phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
> > +				phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
> > +			}
> >  			if (dwc->dis_split_quirk) {

Thanks,
Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
> On Thu, Jan 19, 2023, Thinh Nguyen wrote:
>> Hi,
>>
>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>> Currently the DWC3 driver supports only single port controller
>>> which requires at most one HS and one SS PHY.
>>
>> Add note here that multi-port is for host mode for clarity.
>>
>>>
>>> But the DWC3 USB controller can be connected to multiple ports and
>>> each port can have their own PHYs. Each port of the multiport
>>> controller can either be HS+SS capable or HS only capable
>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>> and GUSB3PIPECTL registers appropriately.
>>>
>>> Add support for detecting, obtaining and configuring phy's supported
>>> by a multiport controller and limit the max number of ports
>>> supported to 4.
>>>
>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>>>   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>   drivers/usb/dwc3/core.h |  15 +-
>>>   drivers/usb/dwc3/drd.c  |  14 +-
>>>   3 files changed, 244 insertions(+), 89 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 476b63618511..7e0a9a598dfd 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>   {
>>>   	struct dwc3 *dwc = work_to_dwc(work);
>>>   	unsigned long flags;
>>> -	int ret;
>>> +	int ret, i;
>>
>> Can we declare variables in separate lines here and other places.
>>
>>>   	u32 reg;
>>>   	u32 desired_dr_role;
>>>   
>>> @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>   		} else {
>>>   			if (dwc->usb2_phy)
>>>   				otg_set_vbus(dwc->usb2_phy->otg, true);
>>> -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>> -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>> +			for (i = 0; i < dwc->num_ports; i++) {
> 
> BTW, is num_ports the total of usb2 + usb3 ports?
Hi Thinh,

   No, num_ports is just the total number of hw usb ports present 
(presuming each port is hs capable, this is just the number of HS Phy's 
available).

Regards,
KRishna,
> 
>>> +				phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
>>> +				phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
>>> +			}
>>>   			if (dwc->dis_split_quirk) {
> 
> Thanks,
> Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
> > On Thu, Jan 19, 2023, Thinh Nguyen wrote:
> > > Hi,
> > > 
> > > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > > Currently the DWC3 driver supports only single port controller
> > > > which requires at most one HS and one SS PHY.
> > > 
> > > Add note here that multi-port is for host mode for clarity.
> > > 
> > > > 
> > > > But the DWC3 USB controller can be connected to multiple ports and
> > > > each port can have their own PHYs. Each port of the multiport
> > > > controller can either be HS+SS capable or HS only capable
> > > > Proper quantification of them is required to modify GUSB2PHYCFG
> > > > and GUSB3PIPECTL registers appropriately.
> > > > 
> > > > Add support for detecting, obtaining and configuring phy's supported
> > > > by a multiport controller and limit the max number of ports
> > > > supported to 4.
> > > > 
> > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > ---
> > > >   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > > >   drivers/usb/dwc3/core.h |  15 +-
> > > >   drivers/usb/dwc3/drd.c  |  14 +-
> > > >   3 files changed, 244 insertions(+), 89 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 476b63618511..7e0a9a598dfd 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > >   {
> > > >   	struct dwc3 *dwc = work_to_dwc(work);
> > > >   	unsigned long flags;
> > > > -	int ret;
> > > > +	int ret, i;
> > > 
> > > Can we declare variables in separate lines here and other places.
> > > 
> > > >   	u32 reg;
> > > >   	u32 desired_dr_role;
> > > > @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > >   		} else {
> > > >   			if (dwc->usb2_phy)
> > > >   				otg_set_vbus(dwc->usb2_phy->otg, true);
> > > > -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> > > > -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> > > > +			for (i = 0; i < dwc->num_ports; i++) {
> > 
> > BTW, is num_ports the total of usb2 + usb3 ports?
> Hi Thinh,
> 
>   No, num_ports is just the total number of hw usb ports present (presuming
> each port is hs capable, this is just the number of HS Phy's available).
> 

I see, thanks. Can you also make this clear in its description. I got
mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS.

Thanks,
Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/21/2023 7:49 AM, Thinh Nguyen wrote:
> On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
>>> On Thu, Jan 19, 2023, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>>> Currently the DWC3 driver supports only single port controller
>>>>> which requires at most one HS and one SS PHY.
>>>>
>>>> Add note here that multi-port is for host mode for clarity.
>>>>
>>>>>
>>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>>> each port can have their own PHYs. Each port of the multiport
>>>>> controller can either be HS+SS capable or HS only capable
>>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>>> and GUSB3PIPECTL registers appropriately.
>>>>>
>>>>> Add support for detecting, obtaining and configuring phy's supported
>>>>> by a multiport controller and limit the max number of ports
>>>>> supported to 4.
>>>>>
>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>> ---
>>>>>    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 476b63618511..7e0a9a598dfd 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>>    {
>>>>>    	struct dwc3 *dwc = work_to_dwc(work);
>>>>>    	unsigned long flags;
>>>>> -	int ret;
>>>>> +	int ret, i;
>>>>
>>>> Can we declare variables in separate lines here and other places.
>>>>
>>>>>    	u32 reg;
>>>>>    	u32 desired_dr_role;
>>>>> @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>>    		} else {
>>>>>    			if (dwc->usb2_phy)
>>>>>    				otg_set_vbus(dwc->usb2_phy->otg, true);
>>>>> -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
>>>>> -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
>>>>> +			for (i = 0; i < dwc->num_ports; i++) {
>>>
>>> BTW, is num_ports the total of usb2 + usb3 ports?
>> Hi Thinh,
>>
>>    No, num_ports is just the total number of hw usb ports present (presuming
>> each port is hs capable, this is just the number of HS Phy's available).
>>
> 
> I see, thanks. Can you also make this clear in its description. I got
> mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS.
> 
> Thanks,
> Thinh

Sure, Will add this to commit text.
But as you rightly mentioned, HCSPARAMS1 gives the total number of HS+SS 
Phy's available (HCSPARAMS1.MAXPORTS). Is there a way to segregate this 
value (to just number of hs and ss).

Regards,
Krishna,

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/21/2023 7:49 AM, Thinh Nguyen wrote:
> > On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
> > > > On Thu, Jan 19, 2023, Thinh Nguyen wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > > > > Currently the DWC3 driver supports only single port controller
> > > > > > which requires at most one HS and one SS PHY.
> > > > > 
> > > > > Add note here that multi-port is for host mode for clarity.
> > > > > 
> > > > > > 
> > > > > > But the DWC3 USB controller can be connected to multiple ports and
> > > > > > each port can have their own PHYs. Each port of the multiport
> > > > > > controller can either be HS+SS capable or HS only capable
> > > > > > Proper quantification of them is required to modify GUSB2PHYCFG
> > > > > > and GUSB3PIPECTL registers appropriately.
> > > > > > 
> > > > > > Add support for detecting, obtaining and configuring phy's supported
> > > > > > by a multiport controller and limit the max number of ports
> > > > > > supported to 4.
> > > > > > 
> > > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > > ---
> > > > > >    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > > > > >    drivers/usb/dwc3/core.h |  15 +-
> > > > > >    drivers/usb/dwc3/drd.c  |  14 +-
> > > > > >    3 files changed, 244 insertions(+), 89 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > index 476b63618511..7e0a9a598dfd 100644
> > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > > > >    {
> > > > > >    	struct dwc3 *dwc = work_to_dwc(work);
> > > > > >    	unsigned long flags;
> > > > > > -	int ret;
> > > > > > +	int ret, i;
> > > > > 
> > > > > Can we declare variables in separate lines here and other places.
> > > > > 
> > > > > >    	u32 reg;
> > > > > >    	u32 desired_dr_role;
> > > > > > @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > > > >    		} else {
> > > > > >    			if (dwc->usb2_phy)
> > > > > >    				otg_set_vbus(dwc->usb2_phy->otg, true);
> > > > > > -			phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
> > > > > > -			phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
> > > > > > +			for (i = 0; i < dwc->num_ports; i++) {
> > > > 
> > > > BTW, is num_ports the total of usb2 + usb3 ports?
> > > Hi Thinh,
> > > 
> > >    No, num_ports is just the total number of hw usb ports present (presuming
> > > each port is hs capable, this is just the number of HS Phy's available).
> > > 
> > 
> > I see, thanks. Can you also make this clear in its description. I got
> > mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS.
> > 
> > Thanks,
> > Thinh
> 
> Sure, Will add this to commit text.
> But as you rightly mentioned, HCSPARAMS1 gives the total number of HS+SS
> Phy's available (HCSPARAMS1.MAXPORTS). Is there a way to segregate this
> value (to just number of hs and ss).
> 

We need to walk through each port and check its capability, and we can
check the port's major/minor revision to determine whether it's SS
capable. See xhci driver's logic and how it calls xhci_add_in_port().

Thanks,
Thinh

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Andrew Halaney]

On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller
> which requires at most one HS and one SS PHY.
> 
> But the DWC3 USB controller can be connected to multiple ports and
> each port can have their own PHYs. Each port of the multiport
> controller can either be HS+SS capable or HS only capable
> Proper quantification of them is required to modify GUSB2PHYCFG
> and GUSB3PIPECTL registers appropriately.
> 
> Add support for detecting, obtaining and configuring phy's supported
> by a multiport controller and limit the max number of ports
> supported to 4.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>  drivers/usb/dwc3/core.h |  15 +-
>  drivers/usb/dwc3/drd.c  |  14 +-
>  3 files changed, 244 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..7e0a9a598dfd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c

<snip>

> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +
> +	/*
> +	 * If no mulitport properties are defined, default
> +	 * the port count to '1'.
> +	 */
> +	ret = device_property_read_u32(dev, "num-ports",
> +				&dwc->num_ports);
> +	if (ret)
> +		dwc->num_ports = 1;
> +
> +	ret = device_property_read_u32(dev, "num-ss-ports",
> +				&dwc->num_ss_ports);
> +	if (ret)
> +		dwc->num_ss_ports = 1;

By using this DT property instead of using the number of each phy type you
find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
when there's no phy to go along with it.

I ran into this when testing on sa8540p-ride, which only uses one of the
ports on the multiport controller. I didn't enable the other phys (not
sure if that was smart or not) and overrode phy-names/phys, but did not
override num-ports/num-ss-ports, which resulted in that. Nothing bad
happened on a quick test.. but I thought I'd highlight that as another
downside of decoupling this value from the number of phys you grab.

Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
series (probably needs clean up after review, and will definitely need
alteration after you update the dt-binding again). If not I'll continue
to test/review so please CC me!:


From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
From: Andrew Halaney <ahalaney@redhat.com>
Date: Thu, 19 Jan 2023 14:53:38 -0600
Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
Content-type: text/plain

There is now support for the multiport USB controller this uses
so enable it.

The board only has a single port hooked up (despite it being wired up to
the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
which by default on boot is selected to mux properly. Grab the gpio
controlling that and ensure it stays in the right position so USB 2.0
continues to be routed from the external port to the SoC.

Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
---
 arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
index 97957f3baa64..56d4f43faa1e 100644
--- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
@@ -246,6 +246,21 @@ &usb_0_qmpphy {
 	status = "okay";
 };
 
+&usb_2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb2_en_state>;
+
+	status = "okay";
+};
+
+&usb_2_dwc3 {
+	dr_mode = "host";
+	num-ports = <1>;
+	num-ss-ports = <1>;
+	phy-names = "usb2-phy", "usb3-phy";
+	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
+};
+
 &usb_2_hsphy0 {
 	vdda-pll-supply = <&vreg_l5a>;
 	vdda18-supply = <&vreg_l7g>;
@@ -313,4 +328,13 @@ wake-pins {
 			bias-pull-up;
 		};
 	};
+
+	usb2_en_state: usb2-en-state {
+		/* TS3USB221A USB2.0 mux select */
+		pins = "gpio24";
+		function = "gpio";
+		drive-strength = <2>;
+		bias-disable;
+		output-low;
+	};
 };
-- 
2.39.0

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/20/2023 3:39 AM, Andrew Halaney wrote:
> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>> Currently the DWC3 driver supports only single port controller
>> which requires at most one HS and one SS PHY.
>>
>> But the DWC3 USB controller can be connected to multiple ports and
>> each port can have their own PHYs. Each port of the multiport
>> controller can either be HS+SS capable or HS only capable
>> Proper quantification of them is required to modify GUSB2PHYCFG
>> and GUSB3PIPECTL registers appropriately.
>>
>> Add support for detecting, obtaining and configuring phy's supported
>> by a multiport controller and limit the max number of ports
>> supported to 4.
>>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>   drivers/usb/dwc3/core.h |  15 +-
>>   drivers/usb/dwc3/drd.c  |  14 +-
>>   3 files changed, 244 insertions(+), 89 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 476b63618511..7e0a9a598dfd 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
> 
> <snip>
> 
>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>   	dwc->dis_split_quirk = device_property_read_bool(dev,
>>   				"snps,dis-split-quirk");
>>   
>> +
>> +	/*
>> +	 * If no mulitport properties are defined, default
>> +	 * the port count to '1'.
>> +	 */
>> +	ret = device_property_read_u32(dev, "num-ports",
>> +				&dwc->num_ports);
>> +	if (ret)
>> +		dwc->num_ports = 1;
>> +
>> +	ret = device_property_read_u32(dev, "num-ss-ports",
>> +				&dwc->num_ss_ports);
>> +	if (ret)
>> +		dwc->num_ss_ports = 1;
> 
> By using this DT property instead of using the number of each phy type you
> find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
> when there's no phy to go along with it.
> 
Hi Andrew,

  Thanks for the review. Yes, this decoupling is still there and its 
fine I believe.

> I ran into this when testing on sa8540p-ride, which only uses one of the
> ports on the multiport controller. I didn't enable the other phys (not
> sure if that was smart or not) and overrode phy-names/phys, but did not
> override num-ports/num-ss-ports, which resulted in that. Nothing bad
> happened on a quick test.. but I thought I'd highlight that as another
> downside of decoupling this value from the number of phys you grab.
> 
If we do not override phy-names or num-ports/num-ss-ports info in DT, 
they are just defaulted to '1' and as per the current logic only port-1 
registers must be configured. Isn't that the case happening ?

> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
> series (probably needs clean up after review, and will definitely need
> alteration after you update the dt-binding again). If not I'll continue
> to test/review so please CC me!:
> 
> 
Sure, I can add this patch (probably will add the other phy's too) 
during the final submission.

>  From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
> From: Andrew Halaney <ahalaney@redhat.com>
> Date: Thu, 19 Jan 2023 14:53:38 -0600
> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
> Content-type: text/plain
> 
> There is now support for the multiport USB controller this uses
> so enable it.
> 
> The board only has a single port hooked up (despite it being wired up to
> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> which by default on boot is selected to mux properly. Grab the gpio
> controlling that and ensure it stays in the right position so USB 2.0
> continues to be routed from the external port to the SoC.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index 97957f3baa64..56d4f43faa1e 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>   	status = "okay";
>   };
>   
> +&usb_2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&usb2_en_state>;
> +
> +	status = "okay";
> +};
> +
> +&usb_2_dwc3 {
> +	dr_mode = "host";
> +	num-ports = <1>;
> +	num-ss-ports = <1>;

More over, if this is a multiport controller and you are using only 
port-1, it is as good as a single port controller I believe and the 
normal DT convention must work. Adding these properties as "1" is not 
required as the driver logic defaults them to "1" if they are not found.

Just to add a point here (as I was not clear in DT Binding description, 
My bad), the num-ports and num-ss-ports must indicate the HS/SS Phys 
present on HW whether they are used in DT or not. Just to cover all 
cases which user can use [1].

[]1: 
https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/

Regards,
Krishna,

> +	phy-names = "usb2-phy", "usb3-phy";
> +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> +};
> +
>   &usb_2_hsphy0 {
>   	vdda-pll-supply = <&vreg_l5a>;
>   	vdda18-supply = <&vreg_l7g>;
> @@ -313,4 +328,13 @@ wake-pins {
>   			bias-pull-up;
>   		};
>   	};
> +
> +	usb2_en_state: usb2-en-state {
> +		/* TS3USB221A USB2.0 mux select */
> +		pins = "gpio24";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;
> +	};
>   };

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Andrew Halaney]

On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
> > On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
> > > Currently the DWC3 driver supports only single port controller
> > > which requires at most one HS and one SS PHY.
> > > 
> > > But the DWC3 USB controller can be connected to multiple ports and
> > > each port can have their own PHYs. Each port of the multiport
> > > controller can either be HS+SS capable or HS only capable
> > > Proper quantification of them is required to modify GUSB2PHYCFG
> > > and GUSB3PIPECTL registers appropriately.
> > > 
> > > Add support for detecting, obtaining and configuring phy's supported
> > > by a multiport controller and limit the max number of ports
> > > supported to 4.
> > > 
> > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > >   drivers/usb/dwc3/core.h |  15 +-
> > >   drivers/usb/dwc3/drd.c  |  14 +-
> > >   3 files changed, 244 insertions(+), 89 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 476b63618511..7e0a9a598dfd 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > 
> > <snip>
> > 
> > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> > >   	dwc->dis_split_quirk = device_property_read_bool(dev,
> > >   				"snps,dis-split-quirk");
> > > +
> > > +	/*
> > > +	 * If no mulitport properties are defined, default
> > > +	 * the port count to '1'.
> > > +	 */
> > > +	ret = device_property_read_u32(dev, "num-ports",
> > > +				&dwc->num_ports);
> > > +	if (ret)
> > > +		dwc->num_ports = 1;
> > > +
> > > +	ret = device_property_read_u32(dev, "num-ss-ports",
> > > +				&dwc->num_ss_ports);
> > > +	if (ret)
> > > +		dwc->num_ss_ports = 1;
> > 
> > By using this DT property instead of using the number of each phy type you
> > find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
> > when there's no phy to go along with it.
> > 
> Hi Andrew,
> 
>  Thanks for the review. Yes, this decoupling is still there and its fine I
> believe.
> 
> > I ran into this when testing on sa8540p-ride, which only uses one of the
> > ports on the multiport controller. I didn't enable the other phys (not
> > sure if that was smart or not) and overrode phy-names/phys, but did not
> > override num-ports/num-ss-ports, which resulted in that. Nothing bad
> > happened on a quick test.. but I thought I'd highlight that as another
> > downside of decoupling this value from the number of phys you grab.
> > 
> If we do not override phy-names or num-ports/num-ss-ports info in DT, they
> are just defaulted to '1' and as per the current logic only port-1 registers
> must be configured. Isn't that the case happening ?
> 

In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created!
So unless I override them I get this from your sc8280xp.dtsi:

+                       usb_2_dwc3: usb@a400000 {
+                               compatible = "snps,dwc3";
+                               reg = <0 0x0a400000 0 0xcd00>;
+                               interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
+                               iommus = <&apps_smmu 0x800 0x0>;
+                               num-ports = <4>;
+                               num-ss-ports = <2>;
+                               phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
+                                       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
+                                       <&usb_2_hsphy2>,
+                                       <&usb_2_hsphy3>;
+                               phy-names = "usb2-phy_port0", "usb3-phy_port0",
+                                               "usb2-phy_port1", "usb3-phy_port1",
+                                               "usb2-phy_port2",
+                                               "usb2-phy_port3";
+                       };

Since this board only uses one port of the multiport controller, I
redefined phys/phy-names to indicate that. I figured that was more
desireable than enabling unnecessary phys. Without overriding
num-ports/num-ss-ports all the for loops in this patch would act like
the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
multiple times etc as well as look for the multiport phy-names and fail
to actually get any phys. Hope that makes sense!

> > Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
> > series (probably needs clean up after review, and will definitely need
> > alteration after you update the dt-binding again). If not I'll continue
> > to test/review so please CC me!:
> > 
> > 
> Sure, I can add this patch (probably will add the other phy's too) during
> the final submission.

I don't have a great understanding of the mapping of the phys to
physical connections (as well as what registers like DWC3_GUSB2PHYCFG do),
so if it makes more sense to enable all the relevant SoC phys, write
those registers in the DWC3 IP, etc, and only use one of the actual
board outputs then feel free. I think this is a good example of "what if
a board designer only uses a single port of the multiport IP" imo.

> 
> >  From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
> > From: Andrew Halaney <ahalaney@redhat.com>
> > Date: Thu, 19 Jan 2023 14:53:38 -0600
> > Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
> > Content-type: text/plain
> > 
> > There is now support for the multiport USB controller this uses
> > so enable it.
> > 
> > The board only has a single port hooked up (despite it being wired up to
> > the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> > which by default on boot is selected to mux properly. Grab the gpio
> > controlling that and ensure it stays in the right position so USB 2.0
> > continues to be routed from the external port to the SoC.
> > 
> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > ---
> >   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
> >   1 file changed, 24 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > index 97957f3baa64..56d4f43faa1e 100644
> > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > @@ -246,6 +246,21 @@ &usb_0_qmpphy {
> >   	status = "okay";
> >   };
> > +&usb_2 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&usb2_en_state>;
> > +
> > +	status = "okay";
> > +};
> > +
> > +&usb_2_dwc3 {
> > +	dr_mode = "host";
> > +	num-ports = <1>;
> > +	num-ss-ports = <1>;
> 
> More over, if this is a multiport controller and you are using only port-1,
> it is as good as a single port controller I believe and the normal DT
> convention must work. Adding these properties as "1" is not required as the
> driver logic defaults them to "1" if they are not found.

See above comment about inheriting from sc8280xp.dtsi and needing to
override their values.

> 
> Just to add a point here (as I was not clear in DT Binding description, My
> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on
> HW whether they are used in DT or not. Just to cover all cases which user
> can use [1].
> 
> []1:
> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/

Ok, if you're going with that approach of "must indicate the HS/SS Phys
present on HW whether they are used in the DT or not" (/me assumes DT
here means on the board and not an incorrect coding of the DT) then I
suppose I should not have overridden anything but phys/phy-names to
indicate that I'm only using the first port (and used the multiport
phy-names convention). It looks like in that link you also mention that
it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
defined, which was my concern and reasoning above for overriding
num-ports/num-ss-ports.

Thanks,
Andrew

> 
> Regards,
> Krishna,
> 
> > +	phy-names = "usb2-phy", "usb3-phy";
> > +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> > +};
> > +
> >   &usb_2_hsphy0 {
> >   	vdda-pll-supply = <&vreg_l5a>;
> >   	vdda18-supply = <&vreg_l7g>;
> > @@ -313,4 +328,13 @@ wake-pins {
> >   			bias-pull-up;
> >   		};
> >   	};
> > +
> > +	usb2_en_state: usb2-en-state {
> > +		/* TS3USB221A USB2.0 mux select */
> > +		pins = "gpio24";
> > +		function = "gpio";
> > +		drive-strength = <2>;
> > +		bias-disable;
> > +		output-low;
> > +	};
> >   };
> 

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/20/2023 8:07 PM, Andrew Halaney wrote:
> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>> Currently the DWC3 driver supports only single port controller
>>>> which requires at most one HS and one SS PHY.
>>>>
>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>> each port can have their own PHYs. Each port of the multiport
>>>> controller can either be HS+SS capable or HS only capable
>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>> and GUSB3PIPECTL registers appropriately.
>>>>
>>>> Add support for detecting, obtaining and configuring phy's supported
>>>> by a multiport controller and limit the max number of ports
>>>> supported to 4.
>>>>
>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 476b63618511..7e0a9a598dfd 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>
>>> <snip>
>>>
>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>    	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>    				"snps,dis-split-quirk");
>>>> +
>>>> +	/*
>>>> +	 * If no mulitport properties are defined, default
>>>> +	 * the port count to '1'.
>>>> +	 */
>>>> +	ret = device_property_read_u32(dev, "num-ports",
>>>> +				&dwc->num_ports);
>>>> +	if (ret)
>>>> +		dwc->num_ports = 1;
>>>> +
>>>> +	ret = device_property_read_u32(dev, "num-ss-ports",
>>>> +				&dwc->num_ss_ports);
>>>> +	if (ret)
>>>> +		dwc->num_ss_ports = 1;
>>>
>>> By using this DT property instead of using the number of each phy type you
>>> find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
>>> when there's no phy to go along with it.
>>>
>> Hi Andrew,
>>
>>   Thanks for the review. Yes, this decoupling is still there and its fine I
>> believe.
>>
>>> I ran into this when testing on sa8540p-ride, which only uses one of the
>>> ports on the multiport controller. I didn't enable the other phys (not
>>> sure if that was smart or not) and overrode phy-names/phys, but did not
>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>> happened on a quick test.. but I thought I'd highlight that as another
>>> downside of decoupling this value from the number of phys you grab.
>>>
>> If we do not override phy-names or num-ports/num-ss-ports info in DT, they
>> are just defaulted to '1' and as per the current logic only port-1 registers
>> must be configured. Isn't that the case happening ?
>>
> 
> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created!
> So unless I override them I get this from your sc8280xp.dtsi:
> 
> +                       usb_2_dwc3: usb@a400000 {
> +                               compatible = "snps,dwc3";
> +                               reg = <0 0x0a400000 0 0xcd00>;
> +                               interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> +                               iommus = <&apps_smmu 0x800 0x0>;
> +                               num-ports = <4>;
> +                               num-ss-ports = <2>;
> +                               phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
> +                                       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
> +                                       <&usb_2_hsphy2>,
> +                                       <&usb_2_hsphy3>;
> +                               phy-names = "usb2-phy_port0", "usb3-phy_port0",
> +                                               "usb2-phy_port1", "usb3-phy_port1",
> +                                               "usb2-phy_port2",
> +                                               "usb2-phy_port3";
> +                       };
> 
> Since this board only uses one port of the multiport controller, I
> redefined phys/phy-names to indicate that. I figured that was more
> desireable than enabling unnecessary phys. Without overriding
> num-ports/num-ss-ports all the for loops in this patch would act like
> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
> multiple times etc as well as look for the multiport phy-names and fail
> to actually get any phys. Hope that makes sense!
> 
Hi Andrew,

  My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that 
case it makes complete sense to override the num-ports and num-ss-ports 
to "1" and the usb phy-names.
>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>> series (probably needs clean up after review, and will definitely need
>>> alteration after you update the dt-binding again). If not I'll continue
>>> to test/review so please CC me!:
>>>
>>>
>> Sure, I can add this patch (probably will add the other phy's too) during
>> the final submission.
> 
> I don't have a great understanding of the mapping of the phys to
> physical connections (as well as what registers like DWC3_GUSB2PHYCFG do),
> so if it makes more sense to enable all the relevant SoC phys, write
> those registers in the DWC3 IP, etc, and only use one of the actual
> board outputs then feel free. I think this is a good example of "what if
> a board designer only uses a single port of the multiport IP" imo.
> Agreed. This could be a good example of multi port with only single port 
working.

>>
>>>   From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
>>> From: Andrew Halaney <ahalaney@redhat.com>
>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>> Content-type: text/plain
>>>
>>> There is now support for the multiport USB controller this uses
>>> so enable it.
>>>
>>> The board only has a single port hooked up (despite it being wired up to
>>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
>>> which by default on boot is selected to mux properly. Grab the gpio
>>> controlling that and ensure it stays in the right position so USB 2.0
>>> continues to be routed from the external port to the SoC.
>>>
>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>> ---
>>>    arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
>>>    1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> index 97957f3baa64..56d4f43faa1e 100644
>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>>    	status = "okay";
>>>    };
>>> +&usb_2 {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&usb2_en_state>;
>>> +
>>> +	status = "okay";
>>> +};
>>> +
>>> +&usb_2_dwc3 {
>>> +	dr_mode = "host";
>>> +	num-ports = <1>;
>>> +	num-ss-ports = <1>;
>>
>> More over, if this is a multiport controller and you are using only port-1,
>> it is as good as a single port controller I believe and the normal DT
>> convention must work. Adding these properties as "1" is not required as the
>> driver logic defaults them to "1" if they are not found.
> 
> See above comment about inheriting from sc8280xp.dtsi and needing to
> override their values.
> 
>>
>> Just to add a point here (as I was not clear in DT Binding description, My
>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on
>> HW whether they are used in DT or not. Just to cover all cases which user
>> can use [1].
>>
>> []1:
>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
> 
> Ok, if you're going with that approach of "must indicate the HS/SS Phys
> present on HW whether they are used in the DT or not" (/me assumes DT
> here means on the board and not an incorrect coding of the DT) then I
> suppose I should not have overridden anything but phys/phy-names to
> indicate that I'm only using the first port (and used the multiport
> phy-names convention). It looks like in that link you also mention that
> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
> defined, which was my concern and reasoning above for overriding
> num-ports/num-ss-ports.
> 
> Thanks,
> Andrew
> 
Actually, I was trying to mandate that rule to take care of cases where 
the phy's for say port2 or port3 are missing for a quad port controller 
in dtsi and we don't want to end up configuring wrong dwc3-phy regs.

For just the first port, the changes you have mentioned must be 
sufficient. (Furthermore, thanks for the review and testing it on 
sa8295-ride and confirming nothing breaks while the first port is enabled)

Regards,
Krishna,
>>
>> Regards,
>> Krishna,
>>
>>> +	phy-names = "usb2-phy", "usb3-phy";
>>> +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>> +};
>>> +
>>>    &usb_2_hsphy0 {
>>>    	vdda-pll-supply = <&vreg_l5a>;
>>>    	vdda18-supply = <&vreg_l7g>;
>>> @@ -313,4 +328,13 @@ wake-pins {
>>>    			bias-pull-up;
>>>    		};
>>>    	};
>>> +
>>> +	usb2_en_state: usb2-en-state {
>>> +		/* TS3USB221A USB2.0 mux select */
>>> +		pins = "gpio24";
>>> +		function = "gpio";
>>> +		drive-strength = <2>;
>>> +		bias-disable;
>>> +		output-low;
>>> +	};
>>>    };
>>
> 

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV]

On 1/20/2023 8:43 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 8:07 PM, Andrew Halaney wrote:
>> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>>
>>>
>>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>>> Currently the DWC3 driver supports only single port controller
>>>>> which requires at most one HS and one SS PHY.
>>>>>
>>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>>> each port can have their own PHYs. Each port of the multiport
>>>>> controller can either be HS+SS capable or HS only capable
>>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>>> and GUSB3PIPECTL registers appropriately.
>>>>>
>>>>> Add support for detecting, obtaining and configuring phy's supported
>>>>> by a multiport controller and limit the max number of ports
>>>>> supported to 4.
>>>>>
>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>> ---
>>>>>    drivers/usb/dwc3/core.c | 304 
>>>>> +++++++++++++++++++++++++++++-----------
>>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 476b63618511..7e0a9a598dfd 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>
>>>> <snip>
>>>>
>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 
>>>>> *dwc)
>>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>                    "snps,dis-split-quirk");
>>>>> +
>>>>> +    /*
>>>>> +     * If no mulitport properties are defined, default
>>>>> +     * the port count to '1'.
>>>>> +     */
>>>>> +    ret = device_property_read_u32(dev, "num-ports",
>>>>> +                &dwc->num_ports);
>>>>> +    if (ret)
>>>>> +        dwc->num_ports = 1;
>>>>> +
>>>>> +    ret = device_property_read_u32(dev, "num-ss-ports",
>>>>> +                &dwc->num_ss_ports);
>>>>> +    if (ret)
>>>>> +        dwc->num_ss_ports = 1;
>>>>
>>>> By using this DT property instead of using the number of each phy 
>>>> type you
>>>> find you can get into situations where you're writing 
>>>> DWC3_GUSB2PHYCFG, etc,
>>>> when there's no phy to go along with it.
>>>>
>>> Hi Andrew,
>>>
>>>   Thanks for the review. Yes, this decoupling is still there and its 
>>> fine I
>>> believe.
>>>
>>>> I ran into this when testing on sa8540p-ride, which only uses one of 
>>>> the
>>>> ports on the multiport controller. I didn't enable the other phys (not
>>>> sure if that was smart or not) and overrode phy-names/phys, but did not
>>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>>> happened on a quick test.. but I thought I'd highlight that as another
>>>> downside of decoupling this value from the number of phys you grab.
>>>>
>>> If we do not override phy-names or num-ports/num-ss-ports info in DT, 
>>> they
>>> are just defaulted to '1' and as per the current logic only port-1 
>>> registers
>>> must be configured. Isn't that the case happening ?
>>>
>>
>> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've 
>> created!
>> So unless I override them I get this from your sc8280xp.dtsi:
>>
>> +                       usb_2_dwc3: usb@a400000 {
>> +                               compatible = "snps,dwc3";
>> +                               reg = <0 0x0a400000 0 0xcd00>;
>> +                               interrupts = <GIC_SPI 133 
>> IRQ_TYPE_LEVEL_HIGH>;
>> +                               iommus = <&apps_smmu 0x800 0x0>;
>> +                               num-ports = <4>;
>> +                               num-ss-ports = <2>;
>> +                               phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
>> +                                       <&usb_2_hsphy1>, 
>> <&usb_2_qmpphy1>,
>> +                                       <&usb_2_hsphy2>,
>> +                                       <&usb_2_hsphy3>;
>> +                               phy-names = "usb2-phy_port0", 
>> "usb3-phy_port0",
>> +                                               "usb2-phy_port1", 
>> "usb3-phy_port1",
>> +                                               "usb2-phy_port2",
>> +                                               "usb2-phy_port3";
>> +                       };
>>
>> Since this board only uses one port of the multiport controller, I
>> redefined phys/phy-names to indicate that. I figured that was more
>> desireable than enabling unnecessary phys. Without overriding
>> num-ports/num-ss-ports all the for loops in this patch would act like
>> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
>> multiple times etc as well as look for the multiport phy-names and fail
>> to actually get any phys. Hope that makes sense!
>>
> Hi Andrew,
> 
>   My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that 
> case it makes complete sense to override the num-ports and num-ss-ports 
> to "1" and the usb phy-names.
>>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>>> series (probably needs clean up after review, and will definitely need
>>>> alteration after you update the dt-binding again). If not I'll continue
>>>> to test/review so please CC me!:
>>>>
>>>>
>>> Sure, I can add this patch (probably will add the other phy's too) 
>>> during
>>> the final submission.
>>
>> I don't have a great understanding of the mapping of the phys to
>> physical connections (as well as what registers like DWC3_GUSB2PHYCFG 
>> do),
>> so if it makes more sense to enable all the relevant SoC phys, write
>> those registers in the DWC3 IP, etc, and only use one of the actual
>> board outputs then feel free. I think this is a good example of "what if
>> a board designer only uses a single port of the multiport IP" imo.
>> Agreed. This could be a good example of multi port with only single port 

Typo in the previous mail. Correcting it here.

> working.
Agreed, The dt-patch you provided will be a good working example of 
getting just a single port working for a multiport controller.

Regards,
Krishna,

>>>
>>>>   From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 
>>>> 2001
>>>> From: Andrew Halaney <ahalaney@redhat.com>
>>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>>> Content-type: text/plain
>>>>
>>>> There is now support for the multiport USB controller this uses
>>>> so enable it.
>>>>
>>>> The board only has a single port hooked up (despite it being wired 
>>>> up to
>>>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
>>>> which by default on boot is selected to mux properly. Grab the gpio
>>>> controlling that and ensure it stays in the right position so USB 2.0
>>>> continues to be routed from the external port to the SoC.
>>>>
>>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 
>>>> +++++++++++++++++++++++
>>>>    1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts 
>>>> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>> index 97957f3baa64..56d4f43faa1e 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>>>        status = "okay";
>>>>    };
>>>> +&usb_2 {
>>>> +    pinctrl-names = "default";
>>>> +    pinctrl-0 = <&usb2_en_state>;
>>>> +
>>>> +    status = "okay";
>>>> +};
>>>> +
>>>> +&usb_2_dwc3 {
>>>> +    dr_mode = "host";
>>>> +    num-ports = <1>;
>>>> +    num-ss-ports = <1>;
>>>
>>> More over, if this is a multiport controller and you are using only 
>>> port-1,
>>> it is as good as a single port controller I believe and the normal DT
>>> convention must work. Adding these properties as "1" is not required 
>>> as the
>>> driver logic defaults them to "1" if they are not found.
>>
>> See above comment about inheriting from sc8280xp.dtsi and needing to
>> override their values.
>>
>>>
>>> Just to add a point here (as I was not clear in DT Binding 
>>> description, My
>>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys 
>>> present on
>>> HW whether they are used in DT or not. Just to cover all cases which 
>>> user
>>> can use [1].
>>>
>>> []1:
>>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ 
>>>
>>
>> Ok, if you're going with that approach of "must indicate the HS/SS Phys
>> present on HW whether they are used in the DT or not" (/me assumes DT
>> here means on the board and not an incorrect coding of the DT) then I
>> suppose I should not have overridden anything but phys/phy-names to
>> indicate that I'm only using the first port (and used the multiport
>> phy-names convention). It looks like in that link you also mention that
>> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
>> defined, which was my concern and reasoning above for overriding
>> num-ports/num-ss-ports.
>>
>> Thanks,
>> Andrew
>>
> Actually, I was trying to mandate that rule to take care of cases where 
> the phy's for say port2 or port3 are missing for a quad port controller 
> in dtsi and we don't want to end up configuring wrong dwc3-phy regs.
> 
> For just the first port, the changes you have mentioned must be 
> sufficient. (Furthermore, thanks for the review and testing it on 
> sa8295-ride and confirming nothing breaks while the first port is enabled)
> 
> Regards,
> Krishna,
>>>
>>> Regards,
>>> Krishna,
>>>
>>>> +    phy-names = "usb2-phy", "usb3-phy";
>>>> +    phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>>> +};
>>>> +
>>>>    &usb_2_hsphy0 {
>>>>        vdda-pll-supply = <&vreg_l5a>;
>>>>        vdda18-supply = <&vreg_l7g>;
>>>> @@ -313,4 +328,13 @@ wake-pins {
>>>>                bias-pull-up;
>>>>            };
>>>>        };
>>>> +
>>>> +    usb2_en_state: usb2-en-state {
>>>> +        /* TS3USB221A USB2.0 mux select */
>>>> +        pins = "gpio24";
>>>> +        function = "gpio";
>>>> +        drive-strength = <2>;
>>>> +        bias-disable;
>>>> +        output-low;
>>>> +    };
>>>>    };
>>>
>>

Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Shazad Hussain]

On 1/20/2023 8:48 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 8:43 PM, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 8:07 PM, Andrew Halaney wrote:
>>> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>>>> Currently the DWC3 driver supports only single port controller
>>>>>> which requires at most one HS and one SS PHY.
>>>>>>
>>>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>>>> each port can have their own PHYs. Each port of the multiport
>>>>>> controller can either be HS+SS capable or HS only capable
>>>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>>>> and GUSB3PIPECTL registers appropriately.
>>>>>>
>>>>>> Add support for detecting, obtaining and configuring phy's supported
>>>>>> by a multiport controller and limit the max number of ports
>>>>>> supported to 4.
>>>>>>
>>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>> ---
>>>>>>    drivers/usb/dwc3/core.c | 304 
>>>>>> +++++++++++++++++++++++++++++-----------
>>>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 476b63618511..7e0a9a598dfd 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>
>>>>> <snip>
>>>>>
>>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 
>>>>>> *dwc)
>>>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>>                    "snps,dis-split-quirk");
>>>>>> +
>>>>>> +    /*
>>>>>> +     * If no mulitport properties are defined, default
>>>>>> +     * the port count to '1'.
>>>>>> +     */
>>>>>> +    ret = device_property_read_u32(dev, "num-ports",
>>>>>> +                &dwc->num_ports);
>>>>>> +    if (ret)
>>>>>> +        dwc->num_ports = 1;
>>>>>> +
>>>>>> +    ret = device_property_read_u32(dev, "num-ss-ports",
>>>>>> +                &dwc->num_ss_ports);
>>>>>> +    if (ret)
>>>>>> +        dwc->num_ss_ports = 1;
>>>>>
>>>>> By using this DT property instead of using the number of each phy 
>>>>> type you
>>>>> find you can get into situations where you're writing 
>>>>> DWC3_GUSB2PHYCFG, etc,
>>>>> when there's no phy to go along with it.
>>>>>
>>>> Hi Andrew,
>>>>
>>>>   Thanks for the review. Yes, this decoupling is still there and its 
>>>> fine I
>>>> believe.
>>>>
>>>>> I ran into this when testing on sa8540p-ride, which only uses one 
>>>>> of the
>>>>> ports on the multiport controller. I didn't enable the other phys (not
>>>>> sure if that was smart or not) and overrode phy-names/phys, but did 
>>>>> not
>>>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>>>> happened on a quick test.. but I thought I'd highlight that as another
>>>>> downside of decoupling this value from the number of phys you grab.
>>>>>
>>>> If we do not override phy-names or num-ports/num-ss-ports info in 
>>>> DT, they
>>>> are just defaulted to '1' and as per the current logic only port-1 
>>>> registers
>>>> must be configured. Isn't that the case happening ?
>>>>
>>>
>>> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've 
>>> created!
>>> So unless I override them I get this from your sc8280xp.dtsi:
>>>
>>> +                       usb_2_dwc3: usb@a400000 {
>>> +                               compatible = "snps,dwc3";
>>> +                               reg = <0 0x0a400000 0 0xcd00>;
>>> +                               interrupts = <GIC_SPI 133 
>>> IRQ_TYPE_LEVEL_HIGH>;
>>> +                               iommus = <&apps_smmu 0x800 0x0>;
>>> +                               num-ports = <4>;
>>> +                               num-ss-ports = <2>;
>>> +                               phys = <&usb_2_hsphy0>, 
>>> <&usb_2_qmpphy0>,
>>> +                                       <&usb_2_hsphy1>, 
>>> <&usb_2_qmpphy1>,
>>> +                                       <&usb_2_hsphy2>,
>>> +                                       <&usb_2_hsphy3>;
>>> +                               phy-names = "usb2-phy_port0", 
>>> "usb3-phy_port0",
>>> +                                               "usb2-phy_port1", 
>>> "usb3-phy_port1",
>>> +                                               "usb2-phy_port2",
>>> +                                               "usb2-phy_port3";
>>> +                       };
>>>
>>> Since this board only uses one port of the multiport controller, I
>>> redefined phys/phy-names to indicate that. I figured that was more
>>> desireable than enabling unnecessary phys. Without overriding
>>> num-ports/num-ss-ports all the for loops in this patch would act like
>>> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
>>> multiple times etc as well as look for the multiport phy-names and fail
>>> to actually get any phys. Hope that makes sense!
>>>
>> Hi Andrew,
>>
>>   My Bad. I missed the fact that it was based on sc8280xp.dtsi. In 
>> that case it makes complete sense to override the num-ports and 
>> num-ss-ports to "1" and the usb phy-names.
>>>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>>>> series (probably needs clean up after review, and will definitely need
>>>>> alteration after you update the dt-binding again). If not I'll 
>>>>> continue
>>>>> to test/review so please CC me!:
>>>>>
>>>>>
>>>> Sure, I can add this patch (probably will add the other phy's too) 
>>>> during
>>>> the final submission.
>>>
>>> I don't have a great understanding of the mapping of the phys to
>>> physical connections (as well as what registers like DWC3_GUSB2PHYCFG 
>>> do),
>>> so if it makes more sense to enable all the relevant SoC phys, write
>>> those registers in the DWC3 IP, etc, and only use one of the actual
>>> board outputs then feel free. I think this is a good example of "what if
>>> a board designer only uses a single port of the multiport IP" imo.
>>> Agreed. This could be a good example of multi port with only single port 
> 
> Typo in the previous mail. Correcting it here.
> 
>> working.
> Agreed, The dt-patch you provided will be a good working example of 
> getting just a single port working for a multiport controller.
> 
> Regards,
> Krishna,
> 
>>>>
>>>>>   From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 
>>>>> 2001
>>>>> From: Andrew Halaney <ahalaney@redhat.com>
>>>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>>>> Content-type: text/plain
>>>>>
>>>>> There is now support for the multiport USB controller this uses
>>>>> so enable it.
>>>>>
>>>>> The board only has a single port hooked up (despite it being wired 
>>>>> up to
>>>>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
>>>>> which by default on boot is selected to mux properly. Grab the gpio
>>>>> controlling that and ensure it stays in the right position so USB 2.0
>>>>> continues to be routed from the external port to the SoC.
>>>>>
>>>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 
>>>>> +++++++++++++++++++++++
>>>>>    1 file changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts 
>>>>> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>>> index 97957f3baa64..56d4f43faa1e 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>>>>        status = "okay";
>>>>>    };
>>>>> +&usb_2 {
>>>>> +    pinctrl-names = "default";
>>>>> +    pinctrl-0 = <&usb2_en_state>;
>>>>> +
>>>>> +    status = "okay";
>>>>> +};
>>>>> +
>>>>> +&usb_2_dwc3 {
>>>>> +    dr_mode = "host";
>>>>> +    num-ports = <1>;
>>>>> +    num-ss-ports = <1>;
>>>>
>>>> More over, if this is a multiport controller and you are using only 
>>>> port-1,
>>>> it is as good as a single port controller I believe and the normal DT
>>>> convention must work. Adding these properties as "1" is not required 
>>>> as the
>>>> driver logic defaults them to "1" if they are not found.
>>>
>>> See above comment about inheriting from sc8280xp.dtsi and needing to
>>> override their values.
>>>
>>>>
>>>> Just to add a point here (as I was not clear in DT Binding 
>>>> description, My
>>>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys 
>>>> present on
>>>> HW whether they are used in DT or not. Just to cover all cases which 
>>>> user
>>>> can use [1].
>>>>
>>>> []1:
>>>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
>>>
>>> Ok, if you're going with that approach of "must indicate the HS/SS Phys
>>> present on HW whether they are used in the DT or not" (/me assumes DT
>>> here means on the board and not an incorrect coding of the DT) then I
>>> suppose I should not have overridden anything but phys/phy-names to
>>> indicate that I'm only using the first port (and used the multiport
>>> phy-names convention). It looks like in that link you also mention that
>>> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
>>> defined, which was my concern and reasoning above for overriding
>>> num-ports/num-ss-ports.
>>>
>>> Thanks,
>>> Andrew
>>>
>> Actually, I was trying to mandate that rule to take care of cases 
>> where the phy's for say port2 or port3 are missing for a quad port 
>> controller in dtsi and we don't want to end up configuring wrong 
>> dwc3-phy regs.
>>
>> For just the first port, the changes you have mentioned must be 
>> sufficient. (Furthermore, thanks for the review and testing it on 
>> sa8295-ride and confirming nothing breaks while the first port is 
>> enabled)

I agree that for ride platform the changes provided with overridden
phys in DT for the sa8540-ride.dts makes more sense with only having
port0 of multi port on this platform.
Looking forward to validate the next version on ride platform.

Krishna, please keep me in cc in the next version :).

-Shazad

>>
>> Regards,
>> Krishna,
>>>>
>>>> Regards,
>>>> Krishna,
>>>>
>>>>> +    phy-names = "usb2-phy", "usb3-phy";
>>>>> +    phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>>>> +};
>>>>> +
>>>>>    &usb_2_hsphy0 {
>>>>>        vdda-pll-supply = <&vreg_l5a>;
>>>>>        vdda18-supply = <&vreg_l7g>;
>>>>> @@ -313,4 +328,13 @@ wake-pins {
>>>>>                bias-pull-up;
>>>>>            };
>>>>>        };
>>>>> +
>>>>> +    usb2_en_state: usb2-en-state {
>>>>> +        /* TS3USB221A USB2.0 mux select */
>>>>> +        pins = "gpio24";
>>>>> +        function = "gpio";
>>>>> +        drive-strength = <2>;
>>>>> +        bias-disable;
>>>>> +        output-low;
>>>>> +    };
>>>>>    };
>>>>
>>>

[RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for host only controllers

[Krishna Kurapati]

Multiport controllers being host-only capable do not have GEVNTADDR
HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event
buffers during core_init can cause an SMMU Fault. Avoid event buffers
setup if the GHWPARAMS0 tells that the controller is host-only.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/core.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 7e0a9a598dfd..f61ebddaecc0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
 
 static void dwc3_core_exit(struct dwc3 *dwc)
 {
-	int i;
+	int		i;
+	unsigned int	hw_mode;
 
-	dwc3_event_buffers_cleanup(dwc);
+	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
+		dwc3_event_buffers_cleanup(dwc);
 
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
@@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		}
 	}
 
-	ret = dwc3_event_buffers_setup(dwc);
-	if (ret) {
-		dev_err(dwc->dev, "failed to setup event buffers\n");
-		goto err4;
+	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
+		ret = dwc3_event_buffers_setup(dwc);
+		if (ret) {
+			dev_err(dwc->dev, "failed to setup event buffers\n");
+			goto err4;
+		}
 	}
 
 	/*
@@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	struct resource		*res, dwc_res;
 	struct dwc3		*dwc;
 	int			ret, i;
-
+	unsigned int		hw_mode;
 	void __iomem		*regs;
 
 	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
@@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev)
 err5:
 	dwc3_debugfs_exit(dwc);
 
-	dwc3_event_buffers_cleanup(dwc);
+	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
+		dwc3_event_buffers_cleanup(dwc);
 
 	usb_phy_set_suspend(dwc->usb2_phy, 1);
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
-- 
2.39.0

Re: [RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for host only controllers

[Thinh Nguyen]

On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> Multiport controllers being host-only capable do not have GEVNTADDR
> HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event

I think you should reword "present" to something else. They're still
present, but those registers are to be set while operating in device
mode. The rest looks fine.

Thanks,
Thinh

> buffers during core_init can cause an SMMU Fault. Avoid event buffers
> setup if the GHWPARAMS0 tells that the controller is host-only.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 7e0a9a598dfd..f61ebddaecc0 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
>  
>  static void dwc3_core_exit(struct dwc3 *dwc)
>  {
> -	int i;
> +	int		i;
> +	unsigned int	hw_mode;
>  
> -	dwc3_event_buffers_cleanup(dwc);
> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
> +		dwc3_event_buffers_cleanup(dwc);
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> @@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  		}
>  	}
>  
> -	ret = dwc3_event_buffers_setup(dwc);
> -	if (ret) {
> -		dev_err(dwc->dev, "failed to setup event buffers\n");
> -		goto err4;
> +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> +		ret = dwc3_event_buffers_setup(dwc);
> +		if (ret) {
> +			dev_err(dwc->dev, "failed to setup event buffers\n");
> +			goto err4;
> +		}
>  	}
>  
>  	/*
> @@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	struct resource		*res, dwc_res;
>  	struct dwc3		*dwc;
>  	int			ret, i;
> -
> +	unsigned int		hw_mode;
>  	void __iomem		*regs;
>  
>  	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> @@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev)
>  err5:
>  	dwc3_debugfs_exit(dwc);
>  
> -	dwc3_event_buffers_cleanup(dwc);
> +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
> +		dwc3_event_buffers_cleanup(dwc);
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> -- 
> 2.39.0
> 

Re: [RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for host only controllers

[Jack Pham]

Hi Thinh,

On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote:
> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > Multiport controllers being host-only capable do not have GEVNTADDR

Multiport may not be relevant here.  Host-only is though.

> > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event
> 
> I think you should reword "present" to something else. They're still
> present

In our case we have an instance where the IP is statically configured
via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe
that none of the registers pertaining to device mode (including GEVNT*
and of course all the D* ones) are even *present* in the register map.
If we try to access them we encounter some kind of access error or stall
(or translation fault as described).  So the approach here is to first
verify by checking the HWPARAMS0 register if the HW is even capable of
device mode in the first place.

> but those registers are to be set while operating in device
> mode. The rest looks fine.

Are you suggesting only touching the GEVNT* registers when *operating*
in device mode, even in the case of a dual-role capable controller?  In
that case would it make more sense to additionally move the calls to
dwc3_event_buffers_{setup,cleanup} out of core.c and into
dwc3_gadget_{init,exit} perhaps?  That way we avoid them completely
unless and until we switch into peripheral mode (assuming controller
supports that, which we should already have checks for).  Moreover, if
the devicetree dr_mode property is set to host-only we'd also avoid
calling these.

> > buffers during core_init can cause an SMMU Fault. Avoid event buffers
> > setup if the GHWPARAMS0 tells that the controller is host-only.
> > 
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >  drivers/usb/dwc3/core.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 7e0a9a598dfd..f61ebddaecc0 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
> >  
> >  static void dwc3_core_exit(struct dwc3 *dwc)
> >  {
> > -	int i;
> > +	int		i;
> > +	unsigned int	hw_mode;
> >  
> > -	dwc3_event_buffers_cleanup(dwc);
> > +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)

If we stick with this approach, we probably could just check
dwc->dr_mode instead as probe should have already set that to be an
intersection between the values given in devicetree "dr_mode" and the
HWPARAMS0 capability.

Thanks,
Jack

> > +		dwc3_event_buffers_cleanup(dwc);
> >  
> >  	usb_phy_set_suspend(dwc->usb2_phy, 1);
> >  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> > @@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >  		}
> >  	}
> >  
> > -	ret = dwc3_event_buffers_setup(dwc);
> > -	if (ret) {
> > -		dev_err(dwc->dev, "failed to setup event buffers\n");
> > -		goto err4;
> > +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> > +		ret = dwc3_event_buffers_setup(dwc);
> > +		if (ret) {
> > +			dev_err(dwc->dev, "failed to setup event buffers\n");
> > +			goto err4;
> > +		}
> >  	}
> >  
> >  	/*
> > @@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >  	struct resource		*res, dwc_res;
> >  	struct dwc3		*dwc;
> >  	int			ret, i;
> > -
> > +	unsigned int		hw_mode;
> >  	void __iomem		*regs;
> >  
> >  	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> > @@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev)
> >  err5:
> >  	dwc3_debugfs_exit(dwc);
> >  
> > -	dwc3_event_buffers_cleanup(dwc);
> > +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
> > +		dwc3_event_buffers_cleanup(dwc);
> >  
> >  	usb_phy_set_suspend(dwc->usb2_phy, 1);
> >  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> > -- 
> > 2.39.0
> > 

Re: [RFC v4 3/5] usb: dwc3: core: Do not setup event buffers for host only controllers

[Thinh Nguyen]

On Wed, Jan 18, 2023, Jack Pham wrote:
> Hi Thinh,
> 
> On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote:
> > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > Multiport controllers being host-only capable do not have GEVNTADDR
> 
> Multiport may not be relevant here.  Host-only is though.
> 
> > > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event
> > 
> > I think you should reword "present" to something else. They're still
> > present
> 
> In our case we have an instance where the IP is statically configured
> via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe
> that none of the registers pertaining to device mode (including GEVNT*
> and of course all the D* ones) are even *present* in the register map.
> If we try to access them we encounter some kind of access error or stall
> (or translation fault as described).  So the approach here is to first
> verify by checking the HWPARAMS0 register if the HW is even capable of
> device mode in the first place.

I see.

> 
> > but those registers are to be set while operating in device
> > mode. The rest looks fine.
> 
> Are you suggesting only touching the GEVNT* registers when *operating*
> in device mode, even in the case of a dual-role capable controller?  In
> that case would it make more sense to additionally move the calls to
> dwc3_event_buffers_{setup,cleanup} out of core.c and into
> dwc3_gadget_{init,exit} perhaps?  That way we avoid them completely

While it shouldn't be a problem for DRD, it may be cleaner to do that.

> unless and until we switch into peripheral mode (assuming controller
> supports that, which we should already have checks for).  Moreover, if
> the devicetree dr_mode property is set to host-only we'd also avoid
> calling these.
> 
> > > buffers during core_init can cause an SMMU Fault. Avoid event buffers
> > > setup if the GHWPARAMS0 tells that the controller is host-only.
> > > 
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >  drivers/usb/dwc3/core.c | 23 +++++++++++++++--------
> > >  1 file changed, 15 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 7e0a9a598dfd..f61ebddaecc0 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
> > >  
> > >  static void dwc3_core_exit(struct dwc3 *dwc)
> > >  {
> > > -	int i;
> > > +	int		i;
> > > +	unsigned int	hw_mode;
> > >  
> > > -	dwc3_event_buffers_cleanup(dwc);
> > > +	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> > > +	if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST)
> 
> If we stick with this approach, we probably could just check
> dwc->dr_mode instead as probe should have already set that to be an
> intersection between the values given in devicetree "dr_mode" and the
> HWPARAMS0 capability.
> 

What we have here should not break DRD, so it's fine either way.

Thanks,
Thinh

[RFC v4 4/5] usb: dwc3: qcom: Add multiport controller support for qcom wrapper

[Krishna Kurapati]

QCOM SoC SA8295P supports 2 HS+SS ports and 2 HS only ports. Add
support for configuring PWR_EVENT_IRQ's for all the ports during
suspend/resume.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index b0a0351d2d8b..feef0e737bc9 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -37,7 +37,10 @@
 #define PIPE3_PHYSTATUS_SW			BIT(3)
 #define PIPE_UTMI_CLK_DIS			BIT(8)
 
-#define PWR_EVNT_IRQ_STAT_REG			0x58
+#define PWR_EVNT_IRQ1_STAT_REG			0x58
+#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
+#define PWR_EVNT_IRQ3_STAT_REG			0x228
+#define PWR_EVNT_IRQ4_STAT_REG			0x238
 #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
 #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
 
@@ -93,6 +96,13 @@ struct dwc3_qcom {
 	struct icc_path		*icc_path_apps;
 };
 
+static u32 pwr_evnt_irq_stat_reg_offset[4] = {
+			PWR_EVNT_IRQ1_STAT_REG,
+			PWR_EVNT_IRQ2_STAT_REG,
+			PWR_EVNT_IRQ3_STAT_REG,
+			PWR_EVNT_IRQ4_STAT_REG,
+		};
+
 static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
 {
 	u32 reg;
@@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 {
 	u32 val;
 	int i, ret;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 
 	if (qcom->is_suspended)
 		return 0;
 
-	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
-	if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
-		dev_err(qcom->dev, "HS-PHY not in L2\n");
+	for (i = 0; i < dwc->num_ports; i++) {
+		val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
+		if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
+			dev_err(qcom->dev, "HS-PHY%d not in L2\n", i);
+	}
 
 	for (i = qcom->num_clocks - 1; i >= 0; i--)
 		clk_disable_unprepare(qcom->clks[i]);
@@ -446,6 +459,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 {
 	int ret;
 	int i;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 
 	if (!qcom->is_suspended)
 		return 0;
@@ -467,8 +481,10 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 		dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
 
 	/* Clear existing events from PHY related to L2 in/out */
-	dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
-			  PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
+	for (i = 0; i < dwc->num_ports; i++)
+		dwc3_qcom_setbits(qcom->qscratch_base,
+			pwr_evnt_irq_stat_reg_offset[i],
+			PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
 
 	qcom->is_suspended = false;
 
-- 
2.39.0

[RFC v4 5/5] arm: dts: msm: Add multiport controller node for usb

[Krishna Kurapati]

Add USB and DWC3 node for teritiary port of SC8280 along
with multiport IRQ's and phy's.

Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi   | 60 ++++++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
index 84cb6f3eeb56..f9eb854c3444 100644
--- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
@@ -422,6 +422,20 @@ &usb_1_qmpphy {
 	status = "okay";
 };
 
+&usb_2 {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&usb2_en_state>,
+			<&usb3_en_state>,
+			<&usb4_en_state>,
+			<&usb5_en_state>;
+};
+
+&usb_2_dwc3 {
+	dr_mode = "host";
+};
+
 &usb_2_hsphy0 {
 	vdda-pll-supply = <&vreg_l5a>;
 	vdda18-supply = <&vreg_l7g>;
@@ -472,6 +486,41 @@ &xo_board_clk {
 	clock-frequency = <38400000>;
 };
 
+/* PINCTRL */
+&pm8450c_gpios {
+	usb2_en_state: usb2-en-state {
+		pins = "gpio9";
+		function = "normal";
+		output-high;
+		power-source = <0>;
+	};
+};
+
+&pm8450e_gpios {
+	usb3_en_state: usb3-en-state {
+		pins = "gpio5";
+		function = "normal";
+		output-high;
+		power-source = <0>;
+	};
+};
+
+&pm8450g_gpios {
+	usb4_en_state: usb4-en-state {
+		pins = "gpio5";
+		function = "normal";
+		output-high;
+		power-source = <0>;
+	};
+
+	usb5_en_state: usb5-en-state {
+		pins = "gpio9";
+		function = "normal";
+		output-high;
+		power-source = <0>;
+	};
+};
+
 /* PINCTRL */
 
 &tlmm {
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 109c9d2b684d..e9866ab5c6e2 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -1969,6 +1969,66 @@ usb_1_dwc3: usb@a800000 {
 			};
 		};
 
+		usb_2: usb@a4f8800 {
+			compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
+			reg = <0 0x0a4f8800 0 0x400>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+
+			clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>,
+				 <&gcc GCC_USB30_MP_MASTER_CLK>,
+				 <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>,
+				 <&gcc GCC_USB30_MP_SLEEP_CLK>,
+				 <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>,
+				 <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>,
+				 <&gcc GCC_SYS_NOC_USB_AXI_CLK>;
+			clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi",
+				      "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys";
+
+			assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+					  <&gcc GCC_USB30_MP_MASTER_CLK>;
+			assigned-clock-rates = <19200000>, <200000000>;
+
+			interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
+						<&pdc 126 IRQ_TYPE_EDGE_RISING>,
+						<&pdc 16 IRQ_TYPE_LEVEL_HIGH>;
+
+			interrupt-names = "dp_hs_phy_irq", "dm_hs_phy_irq",
+						"ss_phy_irq";
+
+			power-domains = <&gcc USB30_MP_GDSC>;
+
+			resets = <&gcc GCC_USB30_MP_BCR>;
+
+			interconnects = <&aggre1_noc MASTER_USB3_1 0 &mc_virt SLAVE_EBI1 0>,
+					<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_1 0>;
+			interconnect-names = "usb-ddr", "apps-usb";
+
+			required-opps = <&rpmhpd_opp_nom>;
+
+			status = "disabled";
+
+			usb_2_dwc3: usb@a400000 {
+				compatible = "snps,dwc3";
+				reg = <0 0x0a400000 0 0xcd00>;
+				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
+				iommus = <&apps_smmu 0x800 0x0>;
+				num-ports = <4>;
+				num-ss-ports = <2>;
+				phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
+					<&usb_2_hsphy1>, <&usb_2_qmpphy1>,
+					<&usb_2_hsphy2>,
+					<&usb_2_hsphy3>;
+				phy-names = "usb2-phy_port0", "usb3-phy_port0",
+						"usb2-phy_port1", "usb3-phy_port1",
+						"usb2-phy_port2",
+						"usb2-phy_port3";
+			};
+		};
+
 		pdc: interrupt-controller@b220000 {
 			compatible = "qcom,sc8280xp-pdc", "qcom,pdc";
 			reg = <0 0x0b220000 0 0x30000>, <0 0x17c000f0 0 0x60>;
-- 
2.39.0

Re: [RFC v4 5/5] arm: dts: msm: Add multiport controller node for usb

[Bjorn Andersson]

On Sun, Jan 15, 2023 at 05:11:46PM +0530, Krishna Kurapati wrote:
> Add USB and DWC3 node for teritiary port of SC8280 along
> with multiport IRQ's and phy's.
> 

Very nice.

Please make the subject prefix "arm64: dts: qcom: sc8280xp:", to match
other changes in the sc8280xp.dtsi.

> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 +++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi   | 60 ++++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> index 84cb6f3eeb56..f9eb854c3444 100644
> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> @@ -422,6 +422,20 @@ &usb_1_qmpphy {
>  	status = "okay";
>  };
>  
> +&usb_2 {
> +	status = "okay";

Please the status property last in the node.

> +
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&usb2_en_state>,
> +			<&usb3_en_state>,
> +			<&usb4_en_state>,
> +			<&usb5_en_state>;
> +};
> +
> +&usb_2_dwc3 {
> +	dr_mode = "host";
> +};
> +
>  &usb_2_hsphy0 {
>  	vdda-pll-supply = <&vreg_l5a>;
>  	vdda18-supply = <&vreg_l7g>;
> @@ -472,6 +486,41 @@ &xo_board_clk {
>  	clock-frequency = <38400000>;
>  };
>  
> +/* PINCTRL */

No need to repeat this comment, its purpose is to indicate that nodes
above are sorted alphabetically and pinctrl-related nodes are kept here
at the end of the file. Please place your nodes below the existing /*
PINCTRL */ comment below.

Thanks,
Bjorn

> +&pm8450c_gpios {
> +	usb2_en_state: usb2-en-state {
> +		pins = "gpio9";
> +		function = "normal";
> +		output-high;
> +		power-source = <0>;
> +	};
> +};
> +
> +&pm8450e_gpios {
> +	usb3_en_state: usb3-en-state {
> +		pins = "gpio5";
> +		function = "normal";
> +		output-high;
> +		power-source = <0>;
> +	};
> +};
> +
> +&pm8450g_gpios {
> +	usb4_en_state: usb4-en-state {
> +		pins = "gpio5";
> +		function = "normal";
> +		output-high;
> +		power-source = <0>;
> +	};
> +
> +	usb5_en_state: usb5-en-state {
> +		pins = "gpio9";
> +		function = "normal";
> +		output-high;
> +		power-source = <0>;
> +	};
> +};
> +
>  /* PINCTRL */
>  
>  &tlmm {
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 109c9d2b684d..e9866ab5c6e2 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -1969,6 +1969,66 @@ usb_1_dwc3: usb@a800000 {
>  			};
>  		};
>  
> +		usb_2: usb@a4f8800 {
> +			compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
> +			reg = <0 0x0a4f8800 0 0x400>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +
> +			clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>,
> +				 <&gcc GCC_USB30_MP_MASTER_CLK>,
> +				 <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>,
> +				 <&gcc GCC_USB30_MP_SLEEP_CLK>,
> +				 <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
> +				 <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>,
> +				 <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>,
> +				 <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>,
> +				 <&gcc GCC_SYS_NOC_USB_AXI_CLK>;
> +			clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi",
> +				      "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys";
> +
> +			assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
> +					  <&gcc GCC_USB30_MP_MASTER_CLK>;
> +			assigned-clock-rates = <19200000>, <200000000>;
> +
> +			interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
> +						<&pdc 126 IRQ_TYPE_EDGE_RISING>,
> +						<&pdc 16 IRQ_TYPE_LEVEL_HIGH>;
> +
> +			interrupt-names = "dp_hs_phy_irq", "dm_hs_phy_irq",
> +						"ss_phy_irq";
> +
> +			power-domains = <&gcc USB30_MP_GDSC>;
> +
> +			resets = <&gcc GCC_USB30_MP_BCR>;
> +
> +			interconnects = <&aggre1_noc MASTER_USB3_1 0 &mc_virt SLAVE_EBI1 0>,
> +					<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_1 0>;
> +			interconnect-names = "usb-ddr", "apps-usb";
> +
> +			required-opps = <&rpmhpd_opp_nom>;
> +
> +			status = "disabled";
> +
> +			usb_2_dwc3: usb@a400000 {
> +				compatible = "snps,dwc3";
> +				reg = <0 0x0a400000 0 0xcd00>;
> +				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> +				iommus = <&apps_smmu 0x800 0x0>;
> +				num-ports = <4>;
> +				num-ss-ports = <2>;
> +				phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
> +					<&usb_2_hsphy1>, <&usb_2_qmpphy1>,
> +					<&usb_2_hsphy2>,
> +					<&usb_2_hsphy3>;
> +				phy-names = "usb2-phy_port0", "usb3-phy_port0",
> +						"usb2-phy_port1", "usb3-phy_port1",
> +						"usb2-phy_port2",
> +						"usb2-phy_port3";
> +			};
> +		};
> +
>  		pdc: interrupt-controller@b220000 {
>  			compatible = "qcom,sc8280xp-pdc", "qcom,pdc";
>  			reg = <0 0x0b220000 0 0x30000>, <0 0x17c000f0 0 0x60>;
> -- 
> 2.39.0
> 

Re: [RFC v4 5/5] arm: dts: msm: Add multiport controller node for usb

[Krishna Kurapati PSSNV]

On 1/18/2023 11:58 PM, Bjorn Andersson wrote:
> On Sun, Jan 15, 2023 at 05:11:46PM +0530, Krishna Kurapati wrote:
>> Add USB and DWC3 node for teritiary port of SC8280 along
>> with multiport IRQ's and phy's.
>>
> 
> Very nice.
> 
> Please make the subject prefix "arm64: dts: qcom: sc8280xp:", to match
> other changes in the sc8280xp.dtsi.
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 +++++++++++++++++++
>>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi   | 60 ++++++++++++++++++++++++
>>   2 files changed, 109 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> index 84cb6f3eeb56..f9eb854c3444 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> @@ -422,6 +422,20 @@ &usb_1_qmpphy {
>>   	status = "okay";
>>   };
>>   
>> +&usb_2 {
>> +	status = "okay";
> 
> Please the status property last in the node.
> 
>> +
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&usb2_en_state>,
>> +			<&usb3_en_state>,
>> +			<&usb4_en_state>,
>> +			<&usb5_en_state>;
>> +};
>> +
>> +&usb_2_dwc3 {
>> +	dr_mode = "host";
>> +};
>> +
>>   &usb_2_hsphy0 {
>>   	vdda-pll-supply = <&vreg_l5a>;
>>   	vdda18-supply = <&vreg_l7g>;
>> @@ -472,6 +486,41 @@ &xo_board_clk {
>>   	clock-frequency = <38400000>;
>>   };
>>   
>> +/* PINCTRL */
> 
> No need to repeat this comment, its purpose is to indicate that nodes
> above are sorted alphabetically and pinctrl-related nodes are kept here
> at the end of the file. Please place your nodes below the existing /*
> PINCTRL */ comment below.
> 
> Thanks,
> Bjorn
> 
>> +&pm8450c_gpios {
>> +	usb2_en_state: usb2-en-state {
>> +		pins = "gpio9";
>> +		function = "normal";
>> +		output-high;
>> +		power-source = <0>;
>> +	};
>> +};
>> +
>> +&pm8450e_gpios {
>> +	usb3_en_state: usb3-en-state {
>> +		pins = "gpio5";
>> +		function = "normal";
>> +		output-high;
>> +		power-source = <0>;
>> +	};
>> +};
>> +
>> +&pm8450g_gpios {
>> +	usb4_en_state: usb4-en-state {
>> +		pins = "gpio5";
>> +		function = "normal";
>> +		output-high;
>> +		power-source = <0>;
>> +	};
>> +
>> +	usb5_en_state: usb5-en-state {
>> +		pins = "gpio9";
>> +		function = "normal";
>> +		output-high;
>> +		power-source = <0>;
>> +	};
>> +};
>> +
>>   /* PINCTRL */
>>   
>>   &tlmm {
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index 109c9d2b684d..e9866ab5c6e2 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -1969,6 +1969,66 @@ usb_1_dwc3: usb@a800000 {
>>   			};
>>   		};
>>   
>> +		usb_2: usb@a4f8800 {
>> +			compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
>> +			reg = <0 0x0a4f8800 0 0x400>;
>> +			#address-cells = <2>;
>> +			#size-cells = <2>;
>> +			ranges;
>> +
>> +			clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>,
>> +				 <&gcc GCC_USB30_MP_MASTER_CLK>,
>> +				 <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>,
>> +				 <&gcc GCC_USB30_MP_SLEEP_CLK>,
>> +				 <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
>> +				 <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>,
>> +				 <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>,
>> +				 <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>,
>> +				 <&gcc GCC_SYS_NOC_USB_AXI_CLK>;
>> +			clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi",
>> +				      "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys";
>> +
>> +			assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
>> +					  <&gcc GCC_USB30_MP_MASTER_CLK>;
>> +			assigned-clock-rates = <19200000>, <200000000>;
>> +
>> +			interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
>> +						<&pdc 126 IRQ_TYPE_EDGE_RISING>,
>> +						<&pdc 16 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> +			interrupt-names = "dp_hs_phy_irq", "dm_hs_phy_irq",
>> +						"ss_phy_irq";
>> +
>> +			power-domains = <&gcc USB30_MP_GDSC>;
>> +
>> +			resets = <&gcc GCC_USB30_MP_BCR>;
>> +
>> +			interconnects = <&aggre1_noc MASTER_USB3_1 0 &mc_virt SLAVE_EBI1 0>,
>> +					<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_1 0>;
>> +			interconnect-names = "usb-ddr", "apps-usb";
>> +
>> +			required-opps = <&rpmhpd_opp_nom>;
>> +
>> +			status = "disabled";
>> +
>> +			usb_2_dwc3: usb@a400000 {
>> +				compatible = "snps,dwc3";
>> +				reg = <0 0x0a400000 0 0xcd00>;
>> +				interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
>> +				iommus = <&apps_smmu 0x800 0x0>;
>> +				num-ports = <4>;
>> +				num-ss-ports = <2>;
>> +				phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
>> +					<&usb_2_hsphy1>, <&usb_2_qmpphy1>,
>> +					<&usb_2_hsphy2>,
>> +					<&usb_2_hsphy3>;
>> +				phy-names = "usb2-phy_port0", "usb3-phy_port0",
>> +						"usb2-phy_port1", "usb3-phy_port1",
>> +						"usb2-phy_port2",
>> +						"usb2-phy_port3";
>> +			};
>> +		};
>> +
>>   		pdc: interrupt-controller@b220000 {
>>   			compatible = "qcom,sc8280xp-pdc", "qcom,pdc";
>>   			reg = <0 0x0b220000 0 0x30000>, <0 0x17c000f0 0 0x60>;
>> -- 
>> 2.39.0
>>

Hi Bjorn,

  Thanks for the review. Will make sure to incorporate these changes in 
the next version.

Regards,
Krishna,

Re: [RFC v4 0/5] Add multiport support for DWC3 controllers

[Bjorn Andersson]

On Sun, Jan 15, 2023 at 05:11:41PM +0530, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller which
> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
> DWC3 controller with multiple ports that can operate in host mode.
> Some of the port supports both SS+HS and other port supports only HS
> mode.
> 
> This change primarily refactors the Phy logic in core driver to allow
> multiport support with Generic Phy's.
> 
> Chananges have been tested on  QCOM SoC SA8295P which has 4 ports (2
> are HS+SS capable and 2 are HS only capable).
> 

I can confirm that applying this series allow me to use all 6 USB ports
on the ADP. Looking forward to v5.

Thanks,
Bjorn

> Changes in v4:
> Added DT support for SA8295p.
> 
> Changes in v3:
> Incase any PHY init fails, then clear/exit the PHYs that
> are already initialized.
> 
> Changes in v2:
> Changed dwc3_count_phys to return the number of PHY Phandles in the node.
> This will be used now in dwc3_extract_num_phys to increment num_usb2_phy 
> and num_usb3_phy.
> 
> Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its
> structure such that the first half is for HS-PHY and second half is for
> SS-PHY.
> 
> In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is
> present, pass proper SS_IDX else pass -1.
> 
> Link to v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r
> Link to v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r
> 
> Krishna Kurapati (5):
>   dt-bindings: usb: Add bindings to support multiport properties
>   usb: dwc3: core: Refactor PHY logic to support Multiport Controller
>   usb: dwc3: core: Do not setup event buffers for host only controllers
>   usb: dwc3: qcom: Add multiport controller support for qcom wrapper
>   arm: dts: msm: Add multiport controller node for usb
> 
>  .../devicetree/bindings/usb/snps,dwc3.yaml    |  42 ++-
>  arch/arm64/boot/dts/qcom/sa8295p-adp.dts      |  49 +++
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi        |  60 ++++
>  drivers/usb/dwc3/core.c                       | 325 +++++++++++++-----
>  drivers/usb/dwc3/core.h                       |  15 +-
>  drivers/usb/dwc3/drd.c                        |  14 +-
>  drivers/usb/dwc3/dwc3-qcom.c                  |  28 +-
>  7 files changed, 429 insertions(+), 104 deletions(-)
> 
> -- 
> 2.39.0
> 

Re: [RFC v4 0/5] Add multiport support for DWC3 controllers

[Krishna Kurapati PSSNV]

On 1/19/2023 9:13 AM, Bjorn Andersson wrote:
> On Sun, Jan 15, 2023 at 05:11:41PM +0530, Krishna Kurapati wrote:
>> Currently the DWC3 driver supports only single port controller which
>> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
>> DWC3 controller with multiple ports that can operate in host mode.
>> Some of the port supports both SS+HS and other port supports only HS
>> mode.
>>
>> This change primarily refactors the Phy logic in core driver to allow
>> multiport support with Generic Phy's.
>>
>> Chananges have been tested on  QCOM SoC SA8295P which has 4 ports (2
>> are HS+SS capable and 2 are HS only capable).
>>
> 
> I can confirm that applying this series allow me to use all 6 USB ports
> on the ADP. Looking forward to v5.
> 
> Thanks,
> Bjorn
> 
Thanks Bjorn for testing it out.

Regards,
Krishna,
>> Changes in v4:
>> Added DT support for SA8295p.
>>
>> Changes in v3:
>> Incase any PHY init fails, then clear/exit the PHYs that
>> are already initialized.
>>
>> Changes in v2:
>> Changed dwc3_count_phys to return the number of PHY Phandles in the node.
>> This will be used now in dwc3_extract_num_phys to increment num_usb2_phy
>> and num_usb3_phy.
>>
>> Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its
>> structure such that the first half is for HS-PHY and second half is for
>> SS-PHY.
>>
>> In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is
>> present, pass proper SS_IDX else pass -1.
>>
>> Link to v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r
>> Link to v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r
>>
>> Krishna Kurapati (5):
>>    dt-bindings: usb: Add bindings to support multiport properties
>>    usb: dwc3: core: Refactor PHY logic to support Multiport Controller
>>    usb: dwc3: core: Do not setup event buffers for host only controllers
>>    usb: dwc3: qcom: Add multiport controller support for qcom wrapper
>>    arm: dts: msm: Add multiport controller node for usb
>>
>>   .../devicetree/bindings/usb/snps,dwc3.yaml    |  42 ++-
>>   arch/arm64/boot/dts/qcom/sa8295p-adp.dts      |  49 +++
>>   arch/arm64/boot/dts/qcom/sc8280xp.dtsi        |  60 ++++
>>   drivers/usb/dwc3/core.c                       | 325 +++++++++++++-----
>>   drivers/usb/dwc3/core.h                       |  15 +-
>>   drivers/usb/dwc3/drd.c                        |  14 +-
>>   drivers/usb/dwc3/dwc3-qcom.c                  |  28 +-
>>   7 files changed, 429 insertions(+), 104 deletions(-)
>>
>> -- 
>> 2.39.0
>>