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?
>> 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
connection to a host, then you need to configure your core with SNPS
Hibernation feature. This has nothing to do with that.
>> 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?
> So this cant be done IMO.
why not?
> 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
