On 09/06/16 13:58, Roger Quadros wrote: > 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. So it seems if I disable USB_DEFAULT_PERSIST then I can see devices being re-enumerated on the ti-4.4 kernel on both am437x and dra7x. Let me also cross check on ti-4.1 and v4.7-rc1 and report how it is there. -- cheers, -roger
Attachment:
signature.asc
Description: OpenPGP digital signature