On Wed, Apr 10, 2024 at 10:10:54AM +0530, Krishna Kurapati PSSNV wrote: > > > 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. I can't take it yet, based on the review of this patch, so I'll wait for a new version of the series. thanks, greg k-h
