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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@quicinc.com>
> > > > > Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
> > > > > ---
> > > > >    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@synopsys.com/__;!!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