Re: [PATCH v20 4/9] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Thinh Nguyen]

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.

BR,
Thinh