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?
>> 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.
> 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?
>>>> 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?
>>> 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:
@@ -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?
>>> 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 ;-)
--
balbi
Attachment:
signature.asc
Description: PGP signature
