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

[Thinh Nguyen]

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.

> I believe this covers all cases and I can read this in get_properties
> function. Let me know your opinion if this design is good to proceed
> further.
> 

Thanks,
Thinh