Hi,
Roger Quadros <rogerq@xxxxxx> 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@xxxxxxxxxxxxxxx> # >= v4.13
>> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
>
> 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);
if (ret)
goto err0;
- ret = dwc3_core_soft_reset(dwc);
+ ret = dwc3_core_get_phy(dwc);
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
--
balbi
Attachment:
signature.asc
Description: PGP signature
