On 09/06/16 12:27, Felipe Balbi wrote:
>
> Hi,
>
> Roger Quadros <rog...@ti.com> writes:
>> [ Unknown signature status ]
>> On 09/06/16 11:00, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Roger Quadros <rog...@ti.com> 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.ba...@linux.intel.com>
>>>>> ---
>>>>> 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
signature.asc
Description: OpenPGP digital signature
