On 10/01/18 15:33, Felipe Balbi wrote:

Hi,

Roger Quadros rogerq@ti.com writes:

...

Felipe,

On 10/01/18 15:11, Roger Quadros wrote:

...

The USB PHYs should be requested only once during the life cycle of this driver.

As dwc3_core_init() is called during system suspend/resume it will result in multiple calls to dwc3_core_get_phy() which is wrong.

To prevent that let's move dwc3_core_get_phy() call outside dwc3_core_init().

Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys") Cc: linux-stable stable@vger.kernel.org # >= v4.13 Signed-off-by: Roger Quadros rogerq@ti.com

FYI. this patch brings the code back to revert 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before initializing phys") revert f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get the PHY")

So looks like this will break ULPI PHY case?

Where do we initialize ULPI PHY, in dwc3_phy_setup()?

if so then 541768b08a40 breaks the ULPI PHY case as well, right?

indeed, that commit regressed ULPI PHYs :-(

Seems like it should be more like below:

@@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc) dwc->maximum_speed = USB_SPEED_HIGH; }

• ret = dwc3_core_get_phy(dwc);

• ret = dwc3_phy_setup(dwc);

But can we do a dwc3_phy_setup() without doing the soft reset of the controller first?

if (ret) goto err0;

• ret = dwc3_core_soft_reset(dwc);

• ret = dwc3_core_get_phy(dwc);

we can get_phy in dwc3_core_init() as it will get called on resume(). This was the $subject of this patch.

if (ret) goto err0;

• ret = dwc3_phy_setup(dwc);

• ret = dwc3_core_soft_reset(dwc); if (ret) goto err0;

And maybe we rename dwc3_phy_setup() to dwc3_phy_intf_config() just to make the name match what the function actually does. Can you check that it won't regress the case reported by Carlos? If that works, then we would have to move BOTH dwc3_phy_setup() (dwc3_phy_intf_config()) and dwc3_core_get_phy() outside of dwc3_core_init(), which would mean duplicated code in suspend/resume handlers.

I'm sure we can sort that out in another way; but the proper order is:

-> initialize ULPI (if necessary) -> get phy -> soft reset