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

From: Krishna Kurapati PSSNV
Date: Fri Jan 20 2023 - 21:11:15 EST



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@xxxxxxxxxxx>
Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
---
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,