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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
> > 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@quicinc.com>
> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > ---
> > > > >    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.
> > 
> Hi Thinh,
> 
> Thanks for the review.
> 
>   May be I wasn't able to convey my intention properly in my previous
> thread. The above suggested method doesn't tell that user must not skip any
> phy.
> 
> Let us take the below case for 2 Port (HS+SS) capable controller.
> If the user skips ss-phy 2, then:
> 
> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
> 
> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we
> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and
> num-ports = 2 and num-ss-ports = 1.
> 
> If the user skips ss-phy-1, then phy-names would be something like the
> below:
> 
> phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";
> 
> We need to handle two types of interpretations here while parsing the
> phy-names:
> 
> a) There are 2 SS Phy's and we just skipped the first one. In this scenario,
> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above
> formula as reference, we configure both GUSB3PIPECTL registers present in
> the address map although we never use SS Phy-1 but nothing must break. All
> ports would still work as the user intends with the exception of
> GUSB3PIPECTL1 (for-port1) still being modified.
> 
> b) The second interpretation is something like, port-1 is only HS capable
> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been
> provided a SS Phy. Just by parsing through the phy-names, it would not be
> possible to get that info. So if we consider num-ss-ports as 2 in this
> scenario, we end up meddling with wrong registers (as there is only 1
> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that
> this scenario was not possible.
> 
> num-ss-ports = max_port_index (usb3-portX) + 1
> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1
> 
> Taking case of a quad port controller with all ports SS Capable, if the user
> wants to completely skip port-4. Then with above formula, we get (num-ports
> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure
> exactly those dwc3-phy registers and not touch the port-4 registers which is
> still fine.
> 
> Please let me know if the above idea helps us in this scenario.
> 

This becomes rather more complicated because the user can skip certain
port in the DT. We have access to the host registers. Can we just
temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
capability before handing control over to the xHCI driver. We would be
able to get the num_ports and num_ss_ports then.

Similarly, the xhci driver doesn't care whether the user skips certain
port in the DT, it only checks and operates based on the capability
registers.

If we have the exact num_ports and num_ss_ports, we can be sure the
setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.

Thanks,
Thinh