On 09/06/16 11:00, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@xxxxxx> writes:
>> Felipe,
>>
>> On 30/05/16 14:34, Felipe Balbi wrote:
>>> now that we have re-factored dwc3_core_init() and
>>> dwc3_core_exit() we can use them for suspend/resume
>>> operations.
>>>
>>> This will help us avoid some common mistakes when
>>> patching code when we have duplicated pieces of code
>>> doing the same thing.
>>>
>>> Signed-off-by: Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx>
>>> ---
>>> drivers/usb/dwc3/core.c | 59 +++++--------------------------------------------
>>> drivers/usb/dwc3/core.h | 3 ---
>>> 2 files changed, 5 insertions(+), 57 deletions(-)
>>
>> This is the bad patch that breaks dwc3 on system suspend/resume.
>> Please see my comments below.
>
> Okay, so I take it you have suspend/resume working in mainline for
> AM437x and DRA7x?
Works for DRA7x but couldn't get it working on AM437x.
>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index cbdefbb3d302..80e9affd3d77 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1113,33 +1113,19 @@ static int dwc3_remove(struct platform_device *pdev)
>>> static int dwc3_suspend(struct device *dev)
>>> {
>>> struct dwc3 *dwc = dev_get_drvdata(dev);
>>> - unsigned long flags;
>>> -
>>> - spin_lock_irqsave(&dwc->lock, flags);
>>
>> Is it safe to call dwc3_gadget_suspend() or dwc3_core_exit without
>> holding dwc->lock?
>
> I'll review this part.
>
>>> switch (dwc->dr_mode) {
>>> case USB_DR_MODE_PERIPHERAL:
>>> case USB_DR_MODE_OTG:
>>> dwc3_gadget_suspend(dwc);
>>> - /* FALLTHROUGH */
>>> + break;
>>> case USB_DR_MODE_HOST:
>>> default:
>>> - dwc3_event_buffers_cleanup(dwc);
>>> + /* do nothing */
>>> break;
>>> }
>>>
>>> - dwc->gctl = dwc3_readl(dwc->regs, DWC3_GCTL);
>>> - spin_unlock_irqrestore(&dwc->lock, flags);
>>> -
>>> - usb_phy_shutdown(dwc->usb3_phy);
>>> - usb_phy_shutdown(dwc->usb2_phy);
>>> - phy_exit(dwc->usb2_generic_phy);
>>> - phy_exit(dwc->usb3_generic_phy);
>>> -
>>> - usb_phy_set_suspend(dwc->usb2_phy, 1);
>>> - usb_phy_set_suspend(dwc->usb3_phy, 1);
>>> - WARN_ON(phy_power_off(dwc->usb2_generic_phy) < 0);
>>> - WARN_ON(phy_power_off(dwc->usb3_generic_phy) < 0);
>>> + dwc3_core_exit(dwc);
>>
>> dwc3_core_exit() does a phy_power_off() as well which changes the
>> behaviour.
>
> so TI's dwc3 wakes up only via PHY? Had forgotten about that :-s
>
>> How can we power off the PHY if we want remote wakeup or
>> connect/disconnect to be detected? Also, this would break the USB link
>> to a suspended device.
>
> we're only suspending with cable detached, if you need to maintain
That's true only for device mode. What about host mode? Devices are still
connected to the host port and are most probably just suspended, not
disconnected.
> connection to a host, then you need to configure your core with SNPS
> Hibernation feature. This has nothing to do with that.
Let's keep device and host part separate.
For device mode we are ok with loosing connection to the host as
we don't support the hibernation feature.
My concern is more about the host mode.
>
>>> pinctrl_pm_select_sleep_state(dev);
>>>
>>> @@ -1149,36 +1135,14 @@ static int dwc3_suspend(struct device *dev)
>>> static int dwc3_resume(struct device *dev)
>>> {
>>> struct dwc3 *dwc = dev_get_drvdata(dev);
>>> - unsigned long flags;
>>> int ret;
>>>
>>> pinctrl_pm_select_default_state(dev);
>>>
>>> - usb_phy_set_suspend(dwc->usb2_phy, 0);
>>> - usb_phy_set_suspend(dwc->usb3_phy, 0);
>>> - ret = phy_power_on(dwc->usb2_generic_phy);
>>> - if (ret < 0)
>>> + ret = dwc3_core_init(dwc);
>>> + if (ret)
>>> return ret;
>>
>> You've replaced just restoring gctl with a full core_init().
>
> yes, for a reason
>
>> We will loose XHCI host controller state and the devices connected to
>> it will have to be re-enumerated.
>
> if you're going to suspend and you drop all your power rails, you will,
> anyway, see full reenumeration. In fact, that's what I remember from
> TI's tree as well. When resuming from suspend, there was a full
> reenumeration of all devices when DWC3 was acting as host. So what are
> you really pointing at here?
At least on dra7-evm, I don't see VBUS dropping during system suspend.
And without this patch the devices connected in host mode resumed correctly.
>
>> So this cant be done IMO.
>
> why not?
If host controller state is maintained during a suspend/resume then
you are unnecessary doing a full re-init and loosing context.
And on dra7-evm that seems to be the case.
>
>> Why not just restore GCTL instead of doing a full core re-init?
>
> restoring GCTL is not enough. Not even close ;-) In fact, unless we have
> hibernation, there's no way to save all necessary state. Also, if we're
> going to system suspend we're gonna loose all context anyway (this is
> true in TI's and Intel's platforms at least, with the slight difference
> that Intel can support hibernation feature) and, if we're talking about
> runtime PM, we're only doing that when cable is disconnected which means
> that saving any state is pointless anyway.
>
Agreed if the context is lost. But it might not be so in all cases.
So at least we need to check if controller state was lost or not before
doing a reset and full re-init.
cheers,
-roger
Attachment:
signature.asc
Description: OpenPGP digital signature
