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
