Re: [PATCH v20 4/9] usb: dwc3: core: Refactor PHY logic to support Multiport Controller
From: Krishna Kurapati PSSNV
Date: Wed Apr 10 2024 - 00:41:39 EST
On 4/9/2024 11:43 PM, Thinh Nguyen wrote:
On Tue, Apr 09, 2024, Krishna Kurapati PSSNV wrote:
On 4/9/2024 6:41 AM, Thinh Nguyen wrote:
On Mon, Apr 08, 2024, Krishna Kurapati wrote:
Currently the DWC3 driver supports only single port controller
which requires at least one HS PHY and at most 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 PHYs supported
by a multiport controller. Limit support to multiport controllers
with up to four ports for now (e.g. as needed for SC8280XP).
Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Reviewed-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
---
drivers/usb/dwc3/core.c | 251 ++++++++++++++++++++++++++++------------
drivers/usb/dwc3/core.h | 14 ++-
drivers/usb/dwc3/drd.c | 15 ++-
3 files changed, 193 insertions(+), 87 deletions(-)
<snip>
@@ -1937,6 +2020,10 @@ static int dwc3_get_num_ports(struct dwc3 *dwc)
iounmap(base);
+ if (dwc->num_usb2_ports > DWC3_MAX_PORTS ||
+ dwc->num_usb3_ports > DWC3_MAX_PORTS)
+ return -ENOMEM;
This should be -EINVAL.
+
return 0;
}
<snip>
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 341e4c73cb2e..df2e111aa848 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -33,6 +33,12 @@
#include <linux/power_supply.h>
+/*
+ * Maximum number of ports currently supported for multiport
+ * controllers.
This macro here is being used per USB2 vs USB3 ports rather than USB2 +
USB3, unlike the xHCI MAXPORTS. You can clarify in the comment and
rename the macro to avoid any confusion. You can also create 2 separate
macros for number of USB2 and USB3 ports even if they share the same
value.
As noted[*], we support have different max number of usb2 ports vs usb3
ports. I would suggest splitting the macros.
Hi Thinh,
This macro was intended only to identify how many USB2 (or USB3) Phy's were
serviced/operated by this driver, not how many logical ports present (like
That's not what you described in the comment right above the macro...
in xHCI). I don't think it would be confusing currently given that it is
only used to identify number of generic phy instances to allocate and not
used for any other purpose. Once the num_usb2_ports and num_usb3_ports are
read by get_num_ports(...) call, they directly indicate how many ports are
Those fields are clear. But for DWC3_MAX_PORTS, based on the name and
comment of the macro, it's not clear.
HS and SS respectively. Keeping the same in mind, I returned ENOMEM above
(as you mentioned) because we don't allocate more than DWC3_MAX_PORTS and if
the number of hs or ss ports is more than that, we simply return ENOMEM
saying the driver doesn't support operating those many phy's.
The error code -ENOMEM indicates out of memory failure. The check
condition dwc->num_usb2_ports > DWC3_MAX_PORTS indicates invalid config.
There's no allocation in that check.
[*] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230801013031.ft3zpoatiyfegmh6@xxxxxxxxxxxx/__;!!A4F2R9G_pg!azHqgm92ENkFQrpv6Fhs6PCe210VGOAIrsuGFhrgmfaor8N_kWLu6rxkPpbeCBTLL4NbUpOWlQ0ufmP9DFwO9iFc0XdSEg$
+ */
+#define DWC3_MAX_PORTS 4
+
But it's not a big issue whether you decided to push a new version or a
create a separate patch for the comments above. Here's my Ack:
Since this is not a bug, I would prefer to make a separate patch to rename
the macros. (If that is fine).
That is fine with me. Thanks for your effort pursuing and continue
working on this series.
Thanks Thinh. If there are no other issues, I will wait till Greg picks
the series up. Thanks for the reviews throughout the series.
Regards,
Krishna,