Hi, Roger Quadros <rogerq@xxxxxx> writes: >>>> 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. alright, let me know. >>> 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. okay, so we actually want to power the hole thing down when in deep sleep. What does deep sleep map to in linux parlance? suspend-to-ram? suspend-to-disk? seems like we need better granularity for our suspend/resume calls. That is, however, an optimization which can be done for v4.9 as long as my previous diff works. > 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 you didn't actually reach standby at all. > 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. you wouldn't see a disconnect if that happens with IRQs off. The best way is to see if VBUS dropped at all. Also, I guess USB Persist plays a role here. Do you have USB_DEFAULT_PERSIST enabled? Can you try disabling that and checking if it changes anything? >>>>>> @@ -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. on the DRD port? Which Soc are you talking about? Well, AM437x has the DRVVBUS signal which, for the most part at least, works fine in HW mode. There are some concerns about VBUS contention (ping Dave K about that, btw. You wanna know about that). AM57x, however, has ID and VBUS tied to GPIOs (or was it only VBUS?) >>>>> 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? diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 80e9affd3d77..9da0b4dfb0ac 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1118,6 +1118,7 @@ static int dwc3_suspend(struct device *dev) case USB_DR_MODE_PERIPHERAL: case USB_DR_MODE_OTG: dwc3_gadget_suspend(dwc); + dwc3_core_exit(dwc); break; case USB_DR_MODE_HOST: default: @@ -1125,8 +1126,6 @@ static int dwc3_suspend(struct device *dev) break; } - dwc3_core_exit(dwc); - pinctrl_pm_select_sleep_state(dev); return 0; @@ -1139,15 +1138,15 @@ static int dwc3_resume(struct device *dev) pinctrl_pm_select_default_state(dev); - 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; + dwc3_gadget_resume(dwc); - /* FALLTHROUGH */ + break; case USB_DR_MODE_HOST: default: /* do nothing */ > And what happens in case where XHCI context is really lost? there's really know way of knowing about that today. Moreover, that's no different then what we have today. > And again if in OTG mode and role was host we do a core_init? well, then you should patch that switch() statement, right? We don't have OTG in mainline yet, so I can't cover that case, can I? >>>>> 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. not really bad, actually. It's just that certain PM details shouldn't be dealt with by drivers anyway. PM is very arch-specific and that's why it's hidden away in PCI/ACPI or genpd ;-) Drivers should save context when possible and when it makes sense. For DWC3 it doesn't make sense to even try to save context unless you have Hibernation enabled. The short explanation for that is that DWC3's context isn't exactly visible to the driver; it's easy to conclude that when you realize that endpoints are configured via *commands*. If you save and just rewrite the contents of the EP-related registers, that'll actually break the core. DWC3 endpoints have a special state machine to get started and that's why we _must_ walk through each command or, in case hibernation is enabled, we issue a command to save endpoint state. cheers -- balbi
Attachment:
signature.asc
Description: PGP signature
