On 09/06/16 12:27, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rogerq@xxxxxx> writes:
>> [ Unknown signature status ]
>> 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.
>
> okay, cool.
>
>>>>> 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.
>
> You already have a full reenumeration anyway. But to really answer your
> question, it depends on how xHCI was integrated in TI's SoC. Can you
> process remote wakeup? Does that work today without my PM rework?
We've never tested remote wakeup but I have an action item to see if
we can make it work.
>
>>> 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.
>
> oh, hibernation is also used by XHCI. It's just that XHCI has its own
> registers to mess with that.
oh ok.
>
>> 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.
>
> I remember devices being reenumerated in resume with TI's tree. Can you
> confirm if that has changed since then?
It depends on how deep into power down the SoC goes into. (standby vs deep-sleep)
See. 6.4.3.6 Supported Low Power USB Wakeup Scenarios in [1]
[1] - AM437x TRM http://www.ti.com/lit/pdf/spruhl7
As per that remote-wakeup and connect/disconnect detection (as host) is possible
when in standby mode but not when in deep sleep.
This is the kernel log on TI kernel on AM437x
root@rockdesk:~# lsusb -t
/: Bus 02.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 5000M
/: Bus 01.Port 1: Dev 1, Class=root_hub, Driver=xhci-hcd/1p, 480M
|__ Port 1: Dev 5, If 0, Class=HID, Driver=usbhid, 12M
|__ Port 1: Dev 5, If 1, Class=HID, Driver=usbhid, 12M
|__ Port 1: Dev 5, If 2, Class=HID, Driver=usbhid, 12M
root@rockdesk:~# echo standby > /sys/power/state
[ 471.406585] PM: Syncing filesystems ... done.
[ 471.420959] Freezing user space processes ... (elapsed 0.004 seconds) done.
[ 471.434387] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
[ 471.444061] Suspending console(s) (use no_console_suspend to debug)
[ 471.524291] PM: suspend of devices complete after 72.204 msecs
[ 471.528045] PM: late suspend of devices complete after 3.723 msecs
[ 471.532470] PM: noirq suspend of devices complete after 4.394 msecs
[ 471.532470] Disabling non-boot CPUs ...
[ 471.532470] PM: Could not transition all powerdomains to target state
[ 471.551849] PM: noirq resume of devices complete after 19.134 msecs
[ 471.555419] PM: early resume of devices complete after 2.899 msecs
[ 471.556762] net eth0: initializing cpsw version 1.15 (0)
[ 471.556793] net eth0: initialized cpsw ale version 1.4
[ 471.634033] net eth0: phy found : id is : 0x221622
[ 471.977966] usb 1-1: reset full-speed USB device number 5 using xhci-hcd
[ 472.128082] PM: resume of devices complete after 572.662 msecs
[ 472.201232] Restarting tasks ... done.
-bash: echo: write error: Operation not permitted
root@rockdesk:~# [ 474.657379] cpsw 4a100000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
root@rockdesk:~#
root@rockdesk:~# echo mem > /sys/power/state
[ 490.833740] PM: Syncing filesystems ... done.
[ 490.847167] Freezing user space processes ... (elapsed 0.004 seconds) done.
[ 490.859100] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
[ 490.868774] Suspending console(s) (use no_console_suspend to debug)
[ 490.931030] PM: suspend of devices complete after 54.077 msecs
[ 490.935028] PM: late suspend of devices complete after 3.936 msecs
[ 490.939453] PM: noirq suspend of devices complete after 4.394 msecs
[ 490.939483] Disabling non-boot CPUs ...
[ 490.939483] PM: Successfully put all powerdomains to target state
[ 490.974365] PM: noirq resume of devices complete after 34.729 msecs
[ 490.977874] PM: early resume of devices complete after 2.899 msecs
[ 490.979278] net eth0: initializing cpsw version 1.15 (0)
[ 490.979309] net eth0: initialized cpsw ale version 1.4
[ 491.053985] net eth0: phy found : id is : 0x221622
[ 491.182189] usb usb1: root hub lost power or was reset
[ 491.182220] usb usb2: root hub lost power or was reset
[ 491.523132] usb 1-1: reset full-speed USB device number 5 using xhci-hcd
[ 491.678070] PM: resume of devices complete after 700.164 msecs
[ 491.760070] Restarting tasks ... done.
root@rockdesk:~#
root@rockdesk:~# [ 494.056823] cpsw 4a100000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
How to be sure if device was re-enumerated or not. At least I don't see a disconnect.
>
>>>>> 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.
>
> But DWC3 is not controlling VBUS, so that shouldn't matter in any
> case. IIRC, your VBUS was controlled by a GPIO, right?
Nope. VBUS is controlled internally, most likely via some XHCI integration
logic.
>
>>>> 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.
>
> oh, I see. Then that's simple enough to fix :-) Here you go:
>
> @@ -1094,6 +1094,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc)
> spin_lock_irqsave(&dwc->lock, flags);
> dwc3_gadget_suspend(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
> + dwc3_core_exit(dwc);
> break;
> case USB_DR_MODE_HOST:
> default:
core_exit twice for PERIPHERAL/OTG case?
> @@ -1111,13 +1112,13 @@ static int dwc3_resume_common(struct dwc3 *dwc)
> unsigned long flags;
> int ret;
>
> - ret = dwc3_core_init(dwc);
> - if (ret)
> - return ret;
> -
> switch (dwc->dr_mode) {
> case USB_DR_MODE_PERIPHERAL:
> case USB_DR_MODE_OTG:
> + ret = dwc3_core_init(dwc);
> + if (ret)
> + return ret;
> +
> spin_lock_irqsave(&dwc->lock, flags);
> dwc3_gadget_resume(dwc);
> spin_unlock_irqrestore(&dwc->lock, flags);
>
> Can you test that?
Can you send a diff that applies on patch 18 please?
And what happens in case where XHCI context is really lost?
And again if in OTG mode and role was host we do a core_init?
>
>>>> 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.
>
> we don't have means to know that context has been lost, that discussion
> will get us nowhere ;-)
>
oh ok. That's bad then.
cheers,
-roger
Attachment:
signature.asc
Description: OpenPGP digital signature
