+Kishon, On 09/06/16 14:50, Felipe Balbi wrote: > > 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? SoC standby == echo standby > /sys/power/state SoC deepsleep == echo mem > /sys/power/state > > 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. Yeah ti-4.4 is still not there yet. I tried it on ti-4.1 with USB_DEFAULT_PERSIST disabled and I could see the devices disconnect and re-enumerate on am437x. I do see VBUS being turned off and need to figure out how to prevent that if we want to support the remote wakeup case. On dra7-evm I do see VBUS being left ON, but I do see the devices disconnect and re-enumerate on system resume. However, if I comment out the phy_exit()/init(), phy_power_off/on() part then I do see the devices resume correctly. So looks like we need better granularity for PHY power management as well? > >> 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? dra7xx SoC has a dedicated usb_drvvbus pin that controls the VBUS regulator. The ID and VBUS event input does come over a GPIO though. > > 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?) ID is over GPIO, VBUS input comes via PMIC. It does have usb_drvvbus as well that controls the VBUS regulator. > >>>>>> 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. > OK. So then even without the diff, host is working fine as re-enumeration is expected. However device is still broken with the below backtrace on dra7-evm. I can't test on am437x-evm as suspend/resume doesn't work there. root@rockdesk:~# ./suspend.sh [ 51.596774] PM: Syncing filesystems ... done. [ 51.609571] Freezing user space processes ... (elapsed 0.003 seconds) done. [ 51.620806] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done. [ 51.652549] PM: suspend of devices complete after 17.709 msecs [ 51.664166] PM: late suspend of devices complete after 5.395 msecs [ 51.682110] omap_hwmod: gpio7: _wait_target_disable failed [ 51.694788] omap_hwmod: gpio6: _wait_target_disable failed [ 51.701182] PM: noirq suspend of devices complete after 30.503 msecs [ 51.707869] Disabling non-boot CPUs ... [ 51.713454] CPU1: shutdown [ 51.740773] Powerdomain (l4per_pwrdm) didn't enter target state 1 [ 51.740773] Powerdomain (l3init_pwrdm) didn't enter target state 1 [ 51.740773] Could not enter target state in pm_suspend [ 51.740773] A possible cause could be an old bootloader - try u-boot >= v2012.07 [ 51.740826] Enabling non-boot CPUs ... [ 51.790136] CPU1: smp_ops.cpu_die() returned, trying to resuscitate [ 51.790666] CPU1 is up [ 51.803574] PM: noirq resume of devices complete after 3.813 msecs [ 51.814593] PM: early resume of devices complete after 3.766 msecs [ 51.823500] net eth0: initializing cpsw version 1.15 (0) [ 51.910502] Generic PHY 48485000.mdio:02: attached PHY driver [Generic PHY] (mii_bus:phy_addr=48485000.mdio:02, irq=-1) [ 51.930423] usb usb1: root hub lost power or was reset [ 51.935812] usb usb2: root hub lost power or was reset [ 52.148494] ata1: SATA link down (SStatus 0 SControl 300) [ 52.166756] PM: resume of devices complete after 345.657 msecs [ 52.177507] Restarting tasks ... [ 52.181794] usb 1-1: USB disconnect, device number 3 [ 52.192765] done. root@rockdesk:~# root@rockdesk:~# [ 52.293578] ------------[ cut here ]------------ [ 52.298452] WARNING: CPU: 1 PID: 2120 at drivers/usb/dwc3/gadget.c:2253 dwc3_stop_active_transfer.constprop.1+0x94/0xb0 [dwc3] [ 52.310400] Modules linked in: usb_f_ss_lb g_zero libcomposite usb_storage xhci_plat_hcd xhci_hcd usbcore evdev m25p80 spi_nor dwc3 udc_core usb_common snd_soc_davinci_mcasp snd_soc_edma snd_soc_omap snd_soce [ 52.354125] CPU: 1 PID: 2120 Comm: irq/451-dwc3 Tainted: G W 4.7.0-rc1-00018-gd3a19ee #873 [ 52.363977] Hardware name: Generic DRA74X (Flattened Device Tree) [ 52.370384] [<c011010c>] (unwind_backtrace) from [<c010c224>] (show_stack+0x10/0x14) [ 52.378513] [<c010c224>] (show_stack) from [<c0485e48>] (dump_stack+0xac/0xe0) [ 52.386093] [<c0485e48>] (dump_stack) from [<c0137e14>] (__warn+0xd8/0x104) [ 52.393408] [<c0137e14>] (__warn) from [<c0137eec>] (warn_slowpath_null+0x20/0x28) [ 52.401368] [<c0137eec>] (warn_slowpath_null) from [<bf18c894>] (dwc3_stop_active_transfer.constprop.1+0x94/0xb0 [dwc3]) [ 52.412839] [<bf18c894>] (dwc3_stop_active_transfer.constprop.1 [dwc3]) from [<bf18d434>] (dwc3_remove_requests+0x20/0xac [dwc3]) [ 52.425091] [<bf18d434>] (dwc3_remove_requests [dwc3]) from [<bf18e77c>] (__dwc3_gadget_ep_disable+0x30/0x118 [dwc3]) [ 52.436254] [<bf18e77c>] (__dwc3_gadget_ep_disable [dwc3]) from [<bf18ec90>] (dwc3_gadget_ep_disable+0x3c/0xc4 [dwc3]) [ 52.447510] [<bf18ec90>] (dwc3_gadget_ep_disable [dwc3]) from [<bf27390c>] (disable_ep+0x2c/0x68 [usb_f_ss_lb]) [ 52.458100] [<bf27390c>] (disable_ep [usb_f_ss_lb]) from [<bf274a8c>] (disable_endpoints+0x18/0x50 [usb_f_ss_lb]) [ 52.468879] [<bf274a8c>] (disable_endpoints [usb_f_ss_lb]) from [<bf274af0>] (disable_source_sink+0x2c/0x34 [usb_f_ss_lb]) [ 52.480487] [<bf274af0>] (disable_source_sink [usb_f_ss_lb]) from [<bf259cc0>] (reset_config+0x48/0x7c [libcomposite]) [ 52.491746] [<bf259cc0>] (reset_config [libcomposite]) from [<bf259d20>] (composite_disconnect+0x2c/0x54 [libcomposite]) [ 52.503170] [<bf259d20>] (composite_disconnect [libcomposite]) from [<bf17e354>] (usb_gadget_udc_reset+0x10/0x34 [udc_core]) [ 52.514972] [<bf17e354>] (usb_gadget_udc_reset [udc_core]) from [<bf18d518>] (dwc3_gadget_reset_interrupt+0x58/0x1ec [dwc3]) [ 52.526775] [<bf18d518>] (dwc3_gadget_reset_interrupt [dwc3]) from [<bf18de70>] (dwc3_thread_interrupt+0x32c/0xaac [dwc3]) [ 52.538383] [<bf18de70>] (dwc3_thread_interrupt [dwc3]) from [<c01a18b0>] (irq_thread_fn+0x1c/0x54) [ 52.547876] [<c01a18b0>] (irq_thread_fn) from [<c01a1b84>] (irq_thread+0x120/0x1f0) [ 52.555916] [<c01a1b84>] (irq_thread) from [<c015b440>] (kthread+0xd0/0xf0) [ 52.563218] [<c015b440>] (kthread) from [<c01078f0>] (ret_from_fork+0x14/0x24) [ 52.570787] ---[ end trace ba99cf1a8e21418c ]--- [ 52.576648] usb 1-1: new high-speed USB device number 4 using xhci-hcd [ 52.717105] usb 1-1: New USB device found, idVendor=0781, idProduct=5539 [ 52.724137] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=3 [ 52.731651] usb 1-1: Product: Cruzer Micro [ 52.735946] usb 1-1: Manufacturer: SanDisk [ 52.740259] usb 1-1: SerialNumber: 173733160C524A8E [ 52.747831] usb-storage 1-1:1.0: USB Mass Storage device detected [ 52.755166] scsi host1: usb-storage 1-1:1.0 root@rockdesk:~# [ 53.757532] scsi 1:0:0:0: Direct-Access SanDisk Cruzer Micro 8.02 PQ: 0 ANSI: 0 CCS [ 53.769662] sd 1:0:0:0: [sda] 15695871 512-byte logical blocks: (8.04 GB/7.48 GiB) [ 53.778285] sd 1:0:0:0: [sda] Write Protect is off [ 53.783634] sd 1:0:0:0: [sda] No Caching mode page found [ 53.789240] sd 1:0:0:0: [sda] Assuming drive cache: write through [ 53.803439] sda: [ 53.807870] sd 1:0:0:0: [sda] Attached SCSI removable disk [ 53.913550] cpsw 48484000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx -- cheers, -roger
Attachment:
signature.asc
Description: OpenPGP digital signature
