[PATCH v3 0/3] USB: dwc3: Add suspend/resume support

[PATCH v3 0/3] USB: dwc3: Add suspend/resume support

[Vikas Sajjan]

Changes from v2:
 - Changed CONFIG_PM to CONFIG_PM_SLEEP
 - Used SET_SYSTEM_SLEEP_PM_OPS in dev_pm_ops
 - Modified the commit log for each of the patch 
 - Added dwc3_core_init() during resume and dwc3_core_exit() during suspend in core.c 

Based on 'usb-next' of greg's tree.
Tested USB detection and enumeration across multiple cycles of
suspend/resume using 2.0 and 3.0 devices on D-link SS hub.


Vikas Sajjan (3):
  usb: dwc3: Add the suspend/resume functionality
  usb: xhci: Add the suspend/resume functionality
  exynos5: usb: dwc3: Add suspend/resume functionality

 drivers/usb/dwc3/core.c        |   44 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc3/dwc3-exynos.c |   31 ++++++++++++++++++++++++++++
 drivers/usb/host/xhci-plat.c   |   28 +++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 0 deletions(-)

-- 
1.7.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Vikas Sajjan]

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);
+
+	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,
 	},
 };
 
-- 
1.7.6.5

[PATCH v3 2/3] usb: xhci: Add the suspend/resume functionality

[Vikas Sajjan]

Adds power management support to XHCI platform driver.
This patch facilitates the transition of xHCI host controller
between S0 and S3/S4 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/host/xhci-plat.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index df90fe5..eaf3a07 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -185,11 +185,39 @@ static int xhci_plat_remove(struct platform_device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int xhci_plat_suspend(struct device *dev)
+{
+	struct usb_hcd	*hcd	= dev_get_drvdata(dev);
+	struct xhci_hcd	*xhci	= hcd_to_xhci(hcd);
+
+	/* Make sure that the HCD Core has set state to HC_STATE_SUSPENDED */
+	if (hcd->state != HC_STATE_SUSPENDED ||
+		xhci->shared_hcd->state != HC_STATE_SUSPENDED)
+		return -EINVAL;
+
+	return xhci_suspend(xhci);
+}
+
+static int xhci_plat_resume(struct device *dev)
+{
+	struct usb_hcd	*hcd	= dev_get_drvdata(dev);
+	struct xhci_hcd	*xhci	= hcd_to_xhci(hcd);
+
+	return xhci_resume(xhci, 0);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops xhci_plat_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
+};
+
 static struct platform_driver usb_xhci_driver = {
 	.probe	= xhci_plat_probe,
 	.remove	= xhci_plat_remove,
 	.driver	= {
 		.name = "xhci-hcd",
+		.pm = &xhci_plat_pm_ops,
 	},
 };
 MODULE_ALIAS("platform:xhci-hcd");
-- 
1.7.6.5

[PATCH v3 3/3] exynos5: usb: dwc3: Add suspend/resume functionality

[Vikas Sajjan]

Adds suspend and resume callbacks to exynos dwc3 driver as part of
power management support.
This change does gating of dwc3 clock during suspend/resume cycles.

Signed-off-by: Abhilash Kesavan <a.kesavan-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Vikas C Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
CC: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 drivers/usb/dwc3/dwc3-exynos.c |   31 +++++++++++++++++++++++++++++++
 1 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index ca65978..8623b70 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/dwc3-exynos.h>
 #include <linux/dma-mapping.h>
@@ -200,11 +201,41 @@ static int __devexit dwc3_exynos_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int dwc3_exynos_suspend(struct device *dev)
+{
+	struct dwc3_exynos *exynos = dev_get_drvdata(dev);
+
+	clk_disable(exynos->clk);
+
+	return 0;
+}
+
+static int dwc3_exynos_resume(struct device *dev)
+{
+	struct dwc3_exynos *exynos = dev_get_drvdata(dev);
+
+	clk_enable(exynos->clk);
+
+	/* runtime set active to reflect active state. */
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static const struct dev_pm_ops dwc3_exynos_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwc3_exynos_suspend, dwc3_exynos_resume)
+};
+
 static struct platform_driver dwc3_exynos_driver = {
 	.probe		= dwc3_exynos_probe,
 	.remove		= __devexit_p(dwc3_exynos_remove),
 	.driver		= {
 		.name	= "exynos-dwc3",
+		.pm	= &dwc3_exynos_pm_ops,
 	},
 };
 
-- 
1.7.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/3] usb: xhci: Add the suspend/resume functionality

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2876 bytes --]

Hi,

On Tue, Oct 16, 2012 at 03:15:37PM +0530, Vikas Sajjan wrote:
> Adds power management support to XHCI platform driver.
> This patch facilitates the transition of xHCI host controller
> between S0 and S3/S4 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/host/xhci-plat.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index df90fe5..eaf3a07 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -185,11 +185,39 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int xhci_plat_suspend(struct device *dev)
> +{
> +	struct usb_hcd	*hcd	= dev_get_drvdata(dev);
> +	struct xhci_hcd	*xhci	= hcd_to_xhci(hcd);
> +
> +	/* Make sure that the HCD Core has set state to HC_STATE_SUSPENDED */
> +	if (hcd->state != HC_STATE_SUSPENDED ||
> +		xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> +		return -EINVAL;
> +
> +	return xhci_suspend(xhci);

this is pretty much what xhci_pci_suspend() is doing. Sarah, would you
be ok with a patch such as:

usb: host: xhci: move HC_STATE_SUSPENDED check to xhci_suspend()

[ STILL NEED TO WRITE A PROPER COMMIT LOG ]

NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
---

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 8345d7c..aeb3973 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -218,15 +218,8 @@ static void xhci_pci_remove(struct pci_dev *dev)
 static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 {
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
-	int	retval = 0;
 
-	if (hcd->state != HC_STATE_SUSPENDED ||
-			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
-		return -EINVAL;
-
-	retval = xhci_suspend(xhci);
-
-	return retval;
+	return xhci_suspend(xhci);
 }
 
 static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 8d7fcbb..b85029e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -879,6 +879,10 @@ int xhci_suspend(struct xhci_hcd *xhci)
 	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
 	u32			command;
 
+	if (hcd->state != HC_STATE_SUSPENDED ||
+			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
+		return -EINVAL;
+
 	spin_lock_irq(&xhci->lock);
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);

do you think there is any reason to keep replicating the
HC_STATE_SUSPENDED test all over the place ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2412 bytes --]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> CC: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Vikas Sajjan]

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



-- 
Thanks and Regards
 Vikas Sajjan

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4311 bytes --]

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-l0cyMroinI0@public.gmane.org> 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> CC: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/3] usb: xhci: Add the suspend/resume functionality

[Sarah Sharp]

On Tue, Oct 16, 2012 at 12:59:33PM +0300, Felipe Balbi wrote:
> Hi,
> 
> > +#ifdef CONFIG_PM_SLEEP
> > +static int xhci_plat_suspend(struct device *dev)
> > +{
> > +	struct usb_hcd	*hcd	= dev_get_drvdata(dev);
> > +	struct xhci_hcd	*xhci	= hcd_to_xhci(hcd);
> > +
> > +	/* Make sure that the HCD Core has set state to HC_STATE_SUSPENDED */
> > +	if (hcd->state != HC_STATE_SUSPENDED ||
> > +		xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> > +		return -EINVAL;
> > +
> > +	return xhci_suspend(xhci);
> 
> this is pretty much what xhci_pci_suspend() is doing. Sarah, would you
> be ok with a patch such as:

Yes, that patch looks fine.

> usb: host: xhci: move HC_STATE_SUSPENDED check to xhci_suspend()
> 
> [ STILL NEED TO WRITE A PROPER COMMIT LOG ]
> 
> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
> 
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 8345d7c..aeb3973 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -218,15 +218,8 @@ static void xhci_pci_remove(struct pci_dev *dev)
>  static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>  {
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> -	int	retval = 0;
>  
> -	if (hcd->state != HC_STATE_SUSPENDED ||
> -			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> -		return -EINVAL;
> -
> -	retval = xhci_suspend(xhci);
> -
> -	return retval;
> +	return xhci_suspend(xhci);
>  }
>  
>  static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 8d7fcbb..b85029e 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -879,6 +879,10 @@ int xhci_suspend(struct xhci_hcd *xhci)
>  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
>  	u32			command;
>  
> +	if (hcd->state != HC_STATE_SUSPENDED ||
> +			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> +		return -EINVAL;
> +
>  	spin_lock_irq(&xhci->lock);
>  	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
>  	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);
> 
> do you think there is any reason to keep replicating the
> HC_STATE_SUSPENDED test all over the place ?

Nope, no reason I can see.  I'm pretty sure I only check the suspended
state in case the code in the USB core to handle the host controller
suspend ever breaks for the corner case of the host hardware needing two
roothubs (USB 2.0 and USB 3.0 in this case).  It doesn't matter if we
check that in the PCI/platform suspend or in xhci_suspend, as long as it
gets checked.

Sarah Sharp

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Sarah Sharp]

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-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Alan Stern]

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-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/3] usb: xhci: Add the suspend/resume functionality

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2900 bytes --]

Hi,

On Thu, Oct 18, 2012 at 07:59:56AM -0700, Sarah Sharp wrote:
> On Tue, Oct 16, 2012 at 12:59:33PM +0300, Felipe Balbi wrote:
> > Hi,
> > 
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int xhci_plat_suspend(struct device *dev)
> > > +{
> > > +	struct usb_hcd	*hcd	= dev_get_drvdata(dev);
> > > +	struct xhci_hcd	*xhci	= hcd_to_xhci(hcd);
> > > +
> > > +	/* Make sure that the HCD Core has set state to HC_STATE_SUSPENDED */
> > > +	if (hcd->state != HC_STATE_SUSPENDED ||
> > > +		xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> > > +		return -EINVAL;
> > > +
> > > +	return xhci_suspend(xhci);
> > 
> > this is pretty much what xhci_pci_suspend() is doing. Sarah, would you
> > be ok with a patch such as:
> 
> Yes, that patch looks fine.
> 
> > usb: host: xhci: move HC_STATE_SUSPENDED check to xhci_suspend()
> > 
> > [ STILL NEED TO WRITE A PROPER COMMIT LOG ]
> > 
> > NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> > ---
> > 
> > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> > index 8345d7c..aeb3973 100644
> > --- a/drivers/usb/host/xhci-pci.c
> > +++ b/drivers/usb/host/xhci-pci.c
> > @@ -218,15 +218,8 @@ static void xhci_pci_remove(struct pci_dev *dev)
> >  static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> >  {
> >  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> > -	int	retval = 0;
> >  
> > -	if (hcd->state != HC_STATE_SUSPENDED ||
> > -			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> > -		return -EINVAL;
> > -
> > -	retval = xhci_suspend(xhci);
> > -
> > -	return retval;
> > +	return xhci_suspend(xhci);
> >  }
> >  
> >  static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
> > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> > index 8d7fcbb..b85029e 100644
> > --- a/drivers/usb/host/xhci.c
> > +++ b/drivers/usb/host/xhci.c
> > @@ -879,6 +879,10 @@ int xhci_suspend(struct xhci_hcd *xhci)
> >  	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
> >  	u32			command;
> >  
> > +	if (hcd->state != HC_STATE_SUSPENDED ||
> > +			xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> > +		return -EINVAL;
> > +
> >  	spin_lock_irq(&xhci->lock);
> >  	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> >  	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &xhci->shared_hcd->flags);
> > 
> > do you think there is any reason to keep replicating the
> > HC_STATE_SUSPENDED test all over the place ?
> 
> Nope, no reason I can see.  I'm pretty sure I only check the suspended
> state in case the code in the USB core to handle the host controller
> suspend ever breaks for the corner case of the host hardware needing two
> roothubs (USB 2.0 and USB 3.0 in this case).  It doesn't matter if we
> check that in the PCI/platform suspend or in xhci_suspend, as long as it
> gets checked.

cool, I'll send a proper patch tomorrow ;-)

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Vikas Sajjan]

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.

-- 
Thanks and Regards
 Vikas Sajjan

Re: [PATCH v3 1/3] usb: dwc3: Add the suspend/resume functionality

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1085 bytes --]

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.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]