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

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

Hello Thinh,

I hope I didn't cut out too much of the discussion:

> > --- a/drivers/usb/dwc3/ulpi.c
> > +++ b/drivers/usb/dwc3/ulpi.c
> >
> > +static void dwc3_ulpi_detect_quirks(struct dwc3 *dwc) 
> > +... 
> > +	switch (vendor_id) {
> > +	case USB_VENDOR_MICROCHIP:
> > +		switch (product_id) {
> > +		case 0x0009:
> > +			/* Microchip USB3340 ULPI PHY */
> > +			dwc->enable_usb2_transceiver_delay = true; 
> > ...
>
> 
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> >
> > +static void dwc3_hs_apply_ulpi_quirks(struct dwc3 *dwc)
> > +{
> > +   if (dwc->enable_usb2_transceiver_delay) {
> > +	    int index;
> > +       u32 reg;
> > +
> > +	   for (index = 0; index < dwc->num_usb2_ports; index++) {
> > +          reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));
> > +          reg |= DWC3_GUSB2PHYCFG_XCVRDLY;
> > +          dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
> > +      }
> > +  }  
> > ...
>
> This should be placed in dwc3_ulpi_init() or dwc3_ulpi_detect_quirks().
>
> ...
>
> Do we really need this field enable_usb2_transceiver_delay anymore now
> that we have this check base on the ProductID?

I have thought about this, but I think I have to do it like above.

I think modifying the GUSB2PHYCFG in drivers/usb/dwc3/ulpi.c is the wrong
place.
"ulpi.c" currently doesn't touch the DWC3 controller itself at all.

It seems all of the config for GUSB2PHYCFG is done via `dwc3_core_init()`.
This function is also called for resuming operation after a suspend:

dwc3_plat_runtime_resume
  dwc3_runtime_resume
    dwc3_resume_common
      dwc3_core_init_for_resume
        dwc3_core_init

So I think the right place to modify GUSB2PHYCFG according to the detected
UPLI PHY is in dwc3_core_init().

Note: dwc3_core_init() detects the ULPI PHY only once:

   if (!dwc->ulpi_ready) {
      ...
      dwc->ulpi_ready = true;
   }

My previous v3 patch has the bug to call `dwc3_hs_apply_ulpi_quirks`
(to be renamed in v4) ONLY ONCE. I think that's wrong:

The strategy is: When the ULPI PHY is detected, set
   dwc->enable_usb2_transceiver_delay
if necessary.
Then use this bit to modify GUSB2PHYCFG if necessary:
* Once after the ULPI PHY is detected initially,
  via dwc3_core_probe().
* Each time operation is resumed after a suspend.
  That's why `dwc3_hs_apply_ulpi_quirks` has to be called
  each time dwc3_core_init() is called.


**ALTERNATIVE**
Store the found ULPI Vendor/Product ID in `struct dwc3` instead of the
`enable_usb2_transceiver_delay` boolean and then directly use the
ULPI Vendor/Product ID to apply the necessary config of GUSB2PHYCFG in
`dwc3_hs_apply_ulpi_quirks`.

What do you think?



I will try to incorporate all other feedback you gave me.

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

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