Re: [PATCH v4 1/1] usb: dwc3: Support USB3340x ULPI PHY high-speed negotiation.

[Ingo Rohloff <ingo.rohloff@lauterbach.com>]

Hello Thinh,

> > +static void dwc3_ulpi_detect_config(struct dwc3 *dwc)  
> 
> We can rename this to dwc3_ulpi_get_properties() and declare it in
> core.h. Then we can have the dwc3_get_properties() call this function.

I tested this and this doesn't work:
At the time dwc3_get_properties() runs, the DWC3 controller hasn't been
initialized at all yet.

Any access to a DWC3 hardware register fails (I think the clocks to the
DWC3 IP are not even enabled at that point of time) and the kernel crashes.

To read out the ULPI PHY Vendor/Product ID at least the ULPI
interface of the DWC3 controller must be up and running.

> This will ensure that the order of the setting of the GUSB2PHYCFG is
> done in dwc3_hs_phy_setup(), which is prior to registering the ulpi phy.

I originally had the same idea: Make sure GUSB2PHYCFG settings are
correct before I do any access to the ULPI PHY; that's why I originally
used a device tree property.

But because I now use the Vendor/Product ID, this doesn't work;
instead what I implemented right now does this:

- First setup the ULPI interface to be able to access the ULPI PHY
- Check what ULPI PHY model is connected (via Vendor/Product ID)
- Apply further workarounds depending on the detected ULPI PHY model.

The setting of the XCVRDLY in GUSB2PHYCFG only has an effect once you
enable the USB connection.


> > +static void dwc3_ulpi_apply_config(struct dwc3 *dwc)
> > +{
> >    ...
>
> This should be done in dwc3_hs_phy_setup(). See more comments about this
> below.

The problem is: As far as I can tell, dwc3_hs_phy_setup() has to run first,
to setup the DWC3 controller to access the ULPI PHY at all.

Right at the beginning the code in dwc3_hs_phy_setup() reads:

	/* Select the HS PHY interface */
	switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
	case DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI:

I think that's also the reason why dwc3_core_init() uses this order

	ret = dwc3_phy_setup(dwc);
	...
	if (!dwc->ulpi_ready) {
		ret = dwc3_core_ulpi_init(dwc);

You first have to make sure that dwc3_phy_setup() has setup the interfaces
to the PHYs (both High-Speed and Super-Speed) before trying to
access the HS PHY.


> > +static void dwc3_ulpi_detect_config(struct dwc3 *dwc) 
> > +{
> > +	u16 product_id;
> > +	u16 vendor_id;
> > +	int ret;
> > +
> > +	/* Test the interface */
> > ...
> 
> Do we need to check for data integrity here? That check will be done
> during ulpi init, where it has proper checks and can fail properly
> there.

Thanks!
OMG I am stupid :-):
I misunderstood the code of ulpi_register_interface():
I thought ulpi_register_interface() would only do this check and read out
the Vendor/Product ID, if there is a matching device tree node, but it
turns out it will always do the check and read out the
Vendor/Product ID, even if there is no matching device tree node.

This in turn means: I don't even have to replicate any code, 
because ulpi_register_interface() will already read out the
ULPI PHYs Vendor/Product ID.

If drivers/usb/dwc3/core.c includes "linux/ulpi/driver.h" the code in
core.c already has access to the ULPI PHY Vendor/Product ID via
   dwc->ulpi->id.vendor
   dwc->ulpi->id.product

Which means: I directly can use this to enable the XCVRDLY in GUSB2PHYCFG
if the specific Vendor/Product ID is found.

What do you think?


with best regards
  Ingo


-- 


-------------------------------------------------------------------------
Dipl.-Inform.
Ingo ROHLOFF
Senior Staff Embedded Systems Engineering
phone +49 8102 9876-142 - ingo.rohloff@lauterbach.com

Lauterbach Engineering GmbH & Co. KG
Altlaufstrasse 40, 85635 Hoehenkirchen-Siegertsbrunn, GERMANY
www.lauterbach.com

Registered Office: Hoehenkirchen-Siegertsbrunn, Germany,
Local Court: Munich, HRA 87406, VAT ID: DE246770537,
Managing Directors: Lothar Lauterbach, Stephan Lauterbach, Dr. Thomas
Ullmann

-------------------------------------------------------------------------