| Message ID | 1350380738-32473-2-git-send-email-vikas.sajjan@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Oct 16, 2012 at 03:15:36PM +0530, Vikas Sajjan wrote: > Adds suspend and resume callbacks as part of the power management > support to DWC3 controller Driver. > This patch facilitates transition of DWC3 controller between D0 and D3 > power states during suspend/resume cycles. > > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org> > CC: Doug Anderson <dianders@chromium.org> > --- > drivers/usb/dwc3/core.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 44 insertions(+), 0 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 5db4c76..9f35cf8 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -621,11 +621,55 @@ static int __devexit dwc3_remove(struct platform_device *pdev) > return 0; > } > > +#ifdef CONFIG_PM_SLEEP > +static int dwc3_resume(struct device *dev) > +{ > + struct dwc3 *dwc = dev_get_drvdata(dev); > + int ret; > + > + ret = dwc3_core_init(dwc); > + if (ret < 0) > + return ret; > + > + switch (dwc->mode) { > + case DWC3_MODE_DEVICE: > + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); > + break; > + case DWC3_MODE_HOST: > + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST); > + break; > + case DWC3_MODE_DRD: > + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG); > + } > + > + /* runtime set active to reflect active state. */ > + pm_runtime_disable(dev); > + pm_runtime_set_active(dev); > + pm_runtime_enable(dev); > + > + return 0; > +} > + > +static int dwc3_suspend(struct device *dev) > +{ > + struct dwc3 *dwc = dev_get_drvdata(dev); > + there is one check you need to do here. If you are playing the peripheral role, you can't allow suspend unless link is in U3 or you're not enumerated. Have you tested running 'echo mem > /sys/power/state' on your device while you're transferring data in a USB session with the Host ? I'll ask again, how was this tested ? What did you actually run (which commands, which use cases have you validated) ? I'm not asking only which platform you used, I need to know how you exercised this feature, how did you trigger suspend/resume, in which conditions (enumerated ? bus suspended ? no cable attached ?), etc. cheers
Hi Felipe, On 16 October 2012 15:36, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Tue, Oct 16, 2012 at 03:15:36PM +0530, Vikas Sajjan wrote: >> Adds suspend and resume callbacks as part of the power management >> support to DWC3 controller Driver. >> This patch facilitates transition of DWC3 controller between D0 and D3 >> power states during suspend/resume cycles. >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org> >> CC: Doug Anderson <dianders@chromium.org> >> --- >> drivers/usb/dwc3/core.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 44 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 5db4c76..9f35cf8 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -621,11 +621,55 @@ static int __devexit dwc3_remove(struct platform_device *pdev) >> return 0; >> } >> >> +#ifdef CONFIG_PM_SLEEP >> +static int dwc3_resume(struct device *dev) >> +{ >> + struct dwc3 *dwc = dev_get_drvdata(dev); >> + int ret; >> + >> + ret = dwc3_core_init(dwc); >> + if (ret < 0) >> + return ret; >> + >> + switch (dwc->mode) { >> + case DWC3_MODE_DEVICE: >> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); >> + break; >> + case DWC3_MODE_HOST: >> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST); >> + break; >> + case DWC3_MODE_DRD: >> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG); >> + } >> + >> + /* runtime set active to reflect active state. */ >> + pm_runtime_disable(dev); >> + pm_runtime_set_active(dev); >> + pm_runtime_enable(dev); >> + >> + return 0; >> +} >> + >> +static int dwc3_suspend(struct device *dev) >> +{ >> + struct dwc3 *dwc = dev_get_drvdata(dev); >> + > > there is one check you need to do here. If you are playing the > peripheral role, you can't allow suspend unless link is in U3 or you're > not enumerated. > > Have you tested running 'echo mem > /sys/power/state' on your device > while you're transferring data in a USB session with the Host ? > we have tested only when DWC3 is in HOST mode. > I'll ask again, how was this tested ? What did you actually run (which > commands, which use cases have you validated) ? I'm not asking only > which platform you used, I need to know how you exercised this feature, > how did you trigger suspend/resume, in which conditions (enumerated ? > bus suspended ? no cable attached ?), etc. > 1) We have tested by connecting a USB Mouse, USB 2.0 Mass Storage Device and USB 3.0 Mass Storage Device on a SS hub connected on USB 3.0 port to Exynos5250 board, in which DWC3 is configured for HOST mode. 2) We tested by running command echo +20 > /sys/class/rtc/rtc0/wakealarm && echo mem > /sys/power/state > cheers > > -- > balbi
Hi, On Tue, Oct 16, 2012 at 05:10:39PM +0530, Vikas Sajjan wrote: > Hi Felipe, > > On 16 October 2012 15:36, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Tue, Oct 16, 2012 at 03:15:36PM +0530, Vikas Sajjan wrote: > >> Adds suspend and resume callbacks as part of the power management > >> support to DWC3 controller Driver. > >> This patch facilitates transition of DWC3 controller between D0 and D3 > >> power states during suspend/resume cycles. > >> > >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > >> Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org> > >> CC: Doug Anderson <dianders@chromium.org> > >> --- > >> drivers/usb/dwc3/core.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > >> 1 files changed, 44 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> index 5db4c76..9f35cf8 100644 > >> --- a/drivers/usb/dwc3/core.c > >> +++ b/drivers/usb/dwc3/core.c > >> @@ -621,11 +621,55 @@ static int __devexit dwc3_remove(struct platform_device *pdev) > >> return 0; > >> } > >> > >> +#ifdef CONFIG_PM_SLEEP > >> +static int dwc3_resume(struct device *dev) > >> +{ > >> + struct dwc3 *dwc = dev_get_drvdata(dev); > >> + int ret; > >> + > >> + ret = dwc3_core_init(dwc); > >> + if (ret < 0) > >> + return ret; > >> + > >> + switch (dwc->mode) { > >> + case DWC3_MODE_DEVICE: > >> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >> + break; > >> + case DWC3_MODE_HOST: > >> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST); > >> + break; > >> + case DWC3_MODE_DRD: > >> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG); > >> + } > >> + > >> + /* runtime set active to reflect active state. */ > >> + pm_runtime_disable(dev); > >> + pm_runtime_set_active(dev); > >> + pm_runtime_enable(dev); > >> + > >> + return 0; > >> +} > >> + > >> +static int dwc3_suspend(struct device *dev) > >> +{ > >> + struct dwc3 *dwc = dev_get_drvdata(dev); > >> + > > > > there is one check you need to do here. If you are playing the > > peripheral role, you can't allow suspend unless link is in U3 or you're > > not enumerated. > > > > Have you tested running 'echo mem > /sys/power/state' on your device > > while you're transferring data in a USB session with the Host ? > > > we have tested only when DWC3 is in HOST mode. I'm very sorry but this isn't enough :-( We need to make sure there are no regressions on peripheral role. > > I'll ask again, how was this tested ? What did you actually run (which > > commands, which use cases have you validated) ? I'm not asking only > > which platform you used, I need to know how you exercised this feature, > > how did you trigger suspend/resume, in which conditions (enumerated ? > > bus suspended ? no cable attached ?), etc. > > > 1) We have tested by connecting a USB Mouse, USB 2.0 Mass Storage Device and > USB 3.0 Mass Storage Device on a SS hub connected on USB 3.0 port to > Exynos5250 board, in which DWC3 is configured for HOST mode. did you have a transfer going through when you suspended ? If you didn't, then you haven't stressed well enough. > 2) We tested by running command > echo +20 > /sys/class/rtc/rtc0/wakealarm && echo mem > /sys/power/state that's ok, but try something like: # dd if=/dev/urandom of=/dev/sdXX bs=1M count=99999999 & # echo +20 > /sys/class/rtc/rtc0/wakealarm && echo mem > /sys/power/state Better yet, generate a big (and I really mean big :-) file with a known pattern (make a sequence from 0x00 to 0xff and back to zero, over and over again) then write that file to the mass storage device, while transfer is on the fly, suspend, then resume and see if transfer continues, then later verify the bytes by reading back the data and comparing with original source file. Do this for both Host and Peripheral roles. I have a rather big suspicion that it won't work, in which case we need to make sure to prevent suspend while transfers are on the fly or do something else.
On Tue, Oct 16, 2012 at 04:53:28PM +0300, Felipe Balbi wrote: > Hi, > > On Tue, Oct 16, 2012 at 05:10:39PM +0530, Vikas Sajjan wrote: > > Hi Felipe, ... > did you have a transfer going through when you suspended ? If you > didn't, then you haven't stressed well enough. > > > 2) We tested by running command > > echo +20 > /sys/class/rtc/rtc0/wakealarm && echo mem > /sys/power/state > > that's ok, but try something like: > > # dd if=/dev/urandom of=/dev/sdXX bs=1M count=99999999 & > # echo +20 > /sys/class/rtc/rtc0/wakealarm && echo mem > /sys/power/state > > Better yet, generate a big (and I really mean big :-) file with a known > pattern (make a sequence from 0x00 to 0xff and back to zero, over and > over again) then write that file to the mass storage device, while > transfer is on the fly, suspend, then resume and see if transfer > continues, then later verify the bytes by reading back the data and > comparing with original source file. > > Do this for both Host and Peripheral roles. I have a rather big > suspicion that it won't work, in which case we need to make sure to > prevent suspend while transfers are on the fly or do something else. For system suspend while the DW3 hardware is in host mode, doesn't the USB core prevent drivers from submitting URBs just before the PCI/platform suspend is called? Alan? If the USB core doesn't quiesce URBs before system suspend, then we probably have a larger issue that the generic xHCI driver should take care of. Sarah Sharp -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 18 Oct 2012, Sarah Sharp wrote: > For system suspend while the DW3 hardware is in host mode, doesn't the > USB core prevent drivers from submitting URBs just before the > PCI/platform suspend is called? Alan? Sure it does. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Oct 18, 2012 at 12:44:50PM -0400, Alan Stern wrote: > On Thu, 18 Oct 2012, Sarah Sharp wrote: > > > For system suspend while the DW3 hardware is in host mode, doesn't the > > USB core prevent drivers from submitting URBs just before the > > PCI/platform suspend is called? Alan? > > Sure it does. ok great, so my concerns is limited to the gadget side ;-) I still want to see both sides tested, though. (sorry guys, but with DWC3 now passing full USB3 certification, I want to be very careful with patches I accept, specially related to PM ;-)
Hi Felipe, On 18 October 2012 22:59, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Thu, Oct 18, 2012 at 12:44:50PM -0400, Alan Stern wrote: >> On Thu, 18 Oct 2012, Sarah Sharp wrote: >> >> > For system suspend while the DW3 hardware is in host mode, doesn't the >> > USB core prevent drivers from submitting URBs just before the >> > PCI/platform suspend is called? Alan? >> >> Sure it does. > > ok great, so my concerns is limited to the gadget side ;-) I still want > to see both sides tested, though. > > (sorry guys, but with DWC3 now passing full USB3 certification, I want > to be very careful with patches I accept, specially related to PM ;-) > > -- > balbi Will test both HOST and GADGET mode as per your suggestion and update you.
Hi, On Fri, Oct 26, 2012 at 10:34:22AM +0530, Vikas Sajjan wrote: > Hi Felipe, > > On 18 October 2012 22:59, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > > > On Thu, Oct 18, 2012 at 12:44:50PM -0400, Alan Stern wrote: > >> On Thu, 18 Oct 2012, Sarah Sharp wrote: > >> > >> > For system suspend while the DW3 hardware is in host mode, doesn't the > >> > USB core prevent drivers from submitting URBs just before the > >> > PCI/platform suspend is called? Alan? > >> > >> Sure it does. > > > > ok great, so my concerns is limited to the gadget side ;-) I still want > > to see both sides tested, though. > > > > (sorry guys, but with DWC3 now passing full USB3 certification, I want > > to be very careful with patches I accept, specially related to PM ;-) > > > > -- > > balbi > > Will test both HOST and GADGET mode as per your suggestion and update you. cool, thanks. Hope we can still make it to v3.8. I guess we have about 2 weeks (maybe 3) to get code to Greg so it soaks in linux-next long enough before the merge window opens.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 5db4c76..9f35cf8 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -621,11 +621,55 @@ static int __devexit dwc3_remove(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM_SLEEP +static int dwc3_resume(struct device *dev) +{ + struct dwc3 *dwc = dev_get_drvdata(dev); + int ret; + + ret = dwc3_core_init(dwc); + if (ret < 0) + return ret; + + switch (dwc->mode) { + case DWC3_MODE_DEVICE: + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE); + break; + case DWC3_MODE_HOST: + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST); + break; + case DWC3_MODE_DRD: + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG); + } + + /* runtime set active to reflect active state. */ + pm_runtime_disable(dev); + pm_runtime_set_active(dev); + pm_runtime_enable(dev); + + return 0; +} + +static int dwc3_suspend(struct device *dev) +{ + struct dwc3 *dwc = dev_get_drvdata(dev); + + dwc3_core_exit(dwc); + + return 0; +} +#endif /* CONFIG_PM_SLEEP */ + +static const struct dev_pm_ops dwc3_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume) +}; + static struct platform_driver dwc3_driver = { .probe = dwc3_probe, .remove = __devexit_p(dwc3_remove), .driver = { .name = "dwc3", + .pm = &dwc3_pm_ops, }, };