[PATCH 0/4] Support dwc3 host suspend/resume

[PATCH 0/4] Support dwc3 host suspend/resume

[Baolin Wang]

For mobile devices, they usually require very strict power management.
Such as dwc3 controller, we should enter suspend mode when no cable plug in,
then we can power off the dwc3 controller for saving power.

Now dwc3 gadget can support suspend/resume well, but we also want to suspend/
resume the host when slave device is detached or attached.

Baolin Wang (4):
  usb: host: xhci: Move the xhci quirks checking to the right place
  usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for
    xhci private data
  usb: dwc3: core: Move the mode setting to the right place
  usb: dwc3: core: Support the dwc3 host suspend/resume

 drivers/usb/dwc3/Kconfig         |    7 +++++++
 drivers/usb/dwc3/core.c          |   43 ++++++++++++++++++++++++++++++++++----
 drivers/usb/dwc3/core.h          |   15 +++++++++++++
 drivers/usb/dwc3/host.c          |   32 ++++++++++++++++++++++++++++
 drivers/usb/host/xhci-plat.c     |   11 ++++++----
 include/linux/usb/xhci_pdriver.h |    3 +++
 6 files changed, 103 insertions(+), 8 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] usb: host: xhci: Move the xhci quirks checking to the right place

[Baolin Wang]

It will reset the xhci quirks in xhci_gen_setup() function when xhci try to
add one hcd, thus we need to move the XHCI_LPM_SUPPORT quirk checking after
adding hcd.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/host/xhci-plat.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 1f3f981..e2e2487 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -223,10 +223,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		goto disable_clk;
 	}
 
-	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
-			(pdata && pdata->usb3_lpm_capable))
-		xhci->quirks |= XHCI_LPM_SUPPORT;
-
 	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
 		xhci->shared_hcd->can_do_streams = 1;
 
@@ -250,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto dealloc_usb2_hcd;
 
+	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
+			(pdata && pdata->usb3_lpm_capable))
+		xhci->quirks |= XHCI_LPM_SUPPORT;
+
 	return 0;
 
 
-- 
1.7.9.5

[PATCH 2/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data

[Baolin Wang]

Now some usb controllers (such as dwc3 controller) need 'XHCI_SLOW_SUSPEND'
quirk when suspending the xhci, thus we need to add 'usb3_slow_suspend' member
in xhci platform data to support this.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/host/xhci-plat.c     |    3 +++
 include/linux/usb/xhci_pdriver.h |    3 +++
 2 files changed, 6 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index e2e2487..162f17c 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			(pdata && pdata->usb3_lpm_capable))
 		xhci->quirks |= XHCI_LPM_SUPPORT;
 
+	if (pdata && pdata->usb3_slow_suspend)
+		xhci->quirks |= XHCI_SLOW_SUSPEND;
+
 	return 0;
 
 
diff --git a/include/linux/usb/xhci_pdriver.h b/include/linux/usb/xhci_pdriver.h
index 376654b..1276de1 100644
--- a/include/linux/usb/xhci_pdriver.h
+++ b/include/linux/usb/xhci_pdriver.h
@@ -18,10 +18,13 @@
  *
  * @usb3_lpm_capable:	determines if this xhci platform supports USB3
  *			LPM capability
+ * @usb3_slow_suspend:	determines if it need an extraordinary delay when
+ *			suspending xhci.
  *
  */
 struct usb_xhci_pdata {
 	unsigned	usb3_lpm_capable:1;
+	unsigned	usb3_slow_suspend:1;
 };
 
 #endif /* __USB_CORE_XHCI_PDRIVER_H */
-- 
1.7.9.5

[PATCH 3/4] usb: dwc3: core: Move the mode setting to the right place

[Baolin Wang]

When dwc3 core enters into suspend mode, the system (especially for mobile
device) may power off the dwc3 controller for power saving, that will cause
dwc3 controller lost the mode operation when resuming dwc3 core.

Thus we can move the mode setting into dwc3_core_init() function to avoid this
issue.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/dwc3/core.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9466431..1485480 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -666,6 +666,21 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		goto err4;
 	}
 
+	switch (dwc->dr_mode) {
+	case USB_DR_MODE_PERIPHERAL:
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+		break;
+	case USB_DR_MODE_HOST:
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+		break;
+	case USB_DR_MODE_OTG:
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
+		break;
+	default:
+		dev_warn(dwc->dev, "Unsupported mode %d\n", dwc->dr_mode);
+		break;
+	}
+
 	return 0;
 
 err4:
@@ -763,7 +778,6 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
@@ -772,7 +786,6 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_HOST:
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
 		ret = dwc3_host_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
@@ -781,7 +794,6 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
 		ret = dwc3_host_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
-- 
1.7.9.5

[PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[Baolin Wang]

For some mobile devices with strict power management, we also want to suspend
the host when the slave is detached for power saving.

Thus we add the host suspend/resume functions to support this requirement, and
we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
suspending the xhci.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 drivers/usb/dwc3/Kconfig |    7 +++++++
 drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h  |   15 +++++++++++++++
 drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index a64ce1c..725d2bd 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
 	  This is the default mode of working of DWC3 controller where
 	  both host and gadget features are enabled.
 
+config USB_DWC3_HOST_SUSPEND
+	bool "Choose if the host (xhci) can be suspend/resume"
+	depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
+	help
+	  We can suspend the host when the slave is detached for power saving,
+	  and resume the host when one slave is attached.
+
 endchoice
 
 comment "Platform Glue Driver Support"
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1485480..5140b4d 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
 static int dwc3_suspend_common(struct dwc3 *dwc)
 {
 	unsigned long	flags;
+	int		ret;
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
+		spin_lock_irqsave(&dwc->lock, flags);
+		dwc3_gadget_suspend(dwc);
+		spin_unlock_irqrestore(&dwc->lock, flags);
+		break;
 	case USB_DR_MODE_OTG:
+		ret = dwc3_host_suspend(dwc);
+		if (ret)
+			return ret;
+
 		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_suspend(dwc);
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
 	case USB_DR_MODE_HOST:
+		ret = dwc3_host_suspend(dwc);
+		if (ret)
+			return ret;
 	default:
 		/* do nothing */
 		break;
@@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
+		spin_lock_irqsave(&dwc->lock, flags);
+		dwc3_gadget_resume(dwc);
+		spin_unlock_irqrestore(&dwc->lock, flags);
+		break;
 	case USB_DR_MODE_OTG:
+		ret = dwc3_host_resume(dwc);
+		if (ret)
+			return ret;
+
 		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_resume(dwc);
 		spin_unlock_irqrestore(&dwc->lock, flags);
-		/* FALLTHROUGH */
+		break;
 	case USB_DR_MODE_HOST:
+		ret = dwc3_host_resume(dwc);
+		if (ret)
+			return ret;
 	default:
 		/* do nothing */
 		break;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 45d6de5..0ba203e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
 { }
 #endif
 
+#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
+int dwc3_host_suspend(struct dwc3 *dwc);
+int dwc3_host_resume(struct dwc3 *dwc);
+#else
+static inline int dwc3_host_suspend(struct dwc3 *dwc)
+{
+	return 0;
+}
+
+static inline int dwc3_host_resume(struct dwc3 *dwc)
+{
+	return 0;
+}
+#endif
+
 #endif /* __DRIVERS_USB_DWC3_CORE_H */
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 2e960ed..2ec3eff 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -17,8 +17,11 @@
 
 #include <linux/platform_device.h>
 #include <linux/usb/xhci_pdriver.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
 
 #include "core.h"
+#include "../host/xhci.h"
 
 int dwc3_host_init(struct dwc3 *dwc)
 {
@@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
 	memset(&pdata, 0, sizeof(pdata));
 
 	pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
+	/* dwc3 controller need an extraordinary delay when suspending xhci. */
+	pdata.usb3_slow_suspend = 1;
 
 	ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
 	if (ret) {
@@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
 			  dev_name(&dwc->xhci->dev));
 	platform_device_unregister(dwc->xhci);
 }
+
+int dwc3_host_suspend(struct dwc3 *dwc)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	int ret, cnt = 20;
+
+try_again:
+	 /* We should wait for xhci bus has been into suspend mode firstly. */
+	ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
+	if (ret && --cnt > 0) {
+		dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
+			 ret);
+		msleep(200);
+		goto try_again;
+	}
+
+	return ret;
+}
+
+int dwc3_host_resume(struct dwc3 *dwc)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+	return xhci_resume(xhci, 0);
+}
-- 
1.7.9.5

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[kbuild test robot]

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

Hi,

[auto build test ERROR on balbi-usb/next]
[cannot apply to v4.7-rc7 next-20160715]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Support-dwc3-host-suspend-resume/20160715-193735
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: x86_64-randconfig-s1-07152158 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `dwc3_runtime_suspend':
>> core.c:(.text+0x221c35): undefined reference to `dwc3_gadget_suspend'
   drivers/built-in.o: In function `dwc3_runtime_resume':
>> core.c:(.text+0x223178): undefined reference to `dwc3_gadget_resume'
>> core.c:(.text+0x22319a): undefined reference to `dwc3_gadget_process_pending_events'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 23497 bytes --]

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[kbuild test robot]

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

Hi,

[auto build test ERROR on balbi-usb/next]
[cannot apply to v4.7-rc7 next-20160715]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Baolin-Wang/Support-dwc3-host-suspend-resume/20160715-193735
base:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next
config: x86_64-randconfig-s2-07152253 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "dwc3_gadget_resume" [drivers/usb/dwc3/dwc3.ko] undefined!
>> ERROR: "dwc3_gadget_process_pending_events" [drivers/usb/dwc3/dwc3.ko] undefined!
>> ERROR: "dwc3_gadget_suspend" [drivers/usb/dwc3/dwc3.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22223 bytes --]

Re: [PATCH 1/4] usb: host: xhci: Move the xhci quirks checking to the right place

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> It will reset the xhci quirks in xhci_gen_setup() function when xhci try to
> add one hcd, thus we need to move the XHCI_LPM_SUPPORT quirk checking after
> adding hcd.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/host/xhci-plat.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 1f3f981..e2e2487 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -223,10 +223,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  		goto disable_clk;
>  	}
>  
> -	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
> -			(pdata && pdata->usb3_lpm_capable))
> -		xhci->quirks |= XHCI_LPM_SUPPORT;
> -
>  	if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>  		xhci->shared_hcd->can_do_streams = 1;
>  
> @@ -250,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto dealloc_usb2_hcd;
>  
> +	if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
> +			(pdata && pdata->usb3_lpm_capable))
> +		xhci->quirks |= XHCI_LPM_SUPPORT;
> +

Mathias, any comments here?

>  	return 0;
>  
>  
> -- 
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 2/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Now some usb controllers (such as dwc3 controller) need 'XHCI_SLOW_SUSPEND'
> quirk when suspending the xhci, thus we need to add 'usb3_slow_suspend' member
> in xhci platform data to support this.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/host/xhci-plat.c     |    3 +++
>  include/linux/usb/xhci_pdriver.h |    3 +++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index e2e2487..162f17c 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  			(pdata && pdata->usb3_lpm_capable))
>  		xhci->quirks |= XHCI_LPM_SUPPORT;
>  
> +	if (pdata && pdata->usb3_slow_suspend)
> +		xhci->quirks |= XHCI_SLOW_SUSPEND;

I remember having a discussion about this with Paul Z and it turned out
that we really didn't need SLOW_SUSPEND. Can you describe further in
what situation you need this quirk?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> For some mobile devices with strict power management, we also want to suspend
> the host when the slave is detached for power saving.
>
> Thus we add the host suspend/resume functions to support this requirement, and
> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
> suspending the xhci.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
>  drivers/usb/dwc3/Kconfig |    7 +++++++
>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>  4 files changed, 78 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index a64ce1c..725d2bd 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>  	  This is the default mode of working of DWC3 controller where
>  	  both host and gadget features are enabled.
>  
> +config USB_DWC3_HOST_SUSPEND
> +	bool "Choose if the host (xhci) can be suspend/resume"
> +	depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
> +	help
> +	  We can suspend the host when the slave is detached for power saving,
> +	  and resume the host when one slave is attached.
> +
>  endchoice
>  
>  comment "Platform Glue Driver Support"
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 1485480..5140b4d 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>  static int dwc3_suspend_common(struct dwc3 *dwc)
>  {
>  	unsigned long	flags;
> +	int		ret;
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		dwc3_gadget_suspend(dwc);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		break;
>  	case USB_DR_MODE_OTG:
> +		ret = dwc3_host_suspend(dwc);
> +		if (ret)
> +			return ret;
> +
>  		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_suspend(dwc);
>  		spin_unlock_irqrestore(&dwc->lock, flags);
>  		break;
>  	case USB_DR_MODE_HOST:
> +		ret = dwc3_host_suspend(dwc);
> +		if (ret)
> +			return ret;
>  	default:
>  		/* do nothing */
>  		break;
> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		dwc3_gadget_resume(dwc);
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		break;
>  	case USB_DR_MODE_OTG:
> +		ret = dwc3_host_resume(dwc);
> +		if (ret)
> +			return ret;
> +
>  		spin_lock_irqsave(&dwc->lock, flags);
>  		dwc3_gadget_resume(dwc);
>  		spin_unlock_irqrestore(&dwc->lock, flags);
> -		/* FALLTHROUGH */
> +		break;
>  	case USB_DR_MODE_HOST:
> +		ret = dwc3_host_resume(dwc);
> +		if (ret)
> +			return ret;
>  	default:
>  		/* do nothing */
>  		break;
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 45d6de5..0ba203e 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>  { }
>  #endif
>  
> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
> +int dwc3_host_suspend(struct dwc3 *dwc);
> +int dwc3_host_resume(struct dwc3 *dwc);
> +#else
> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
> +{
> +	return 0;
> +}
> +
> +static inline int dwc3_host_resume(struct dwc3 *dwc)
> +{
> +	return 0;
> +}
> +#endif
> +
>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
> index 2e960ed..2ec3eff 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -17,8 +17,11 @@
>  
>  #include <linux/platform_device.h>
>  #include <linux/usb/xhci_pdriver.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
>  
>  #include "core.h"
> +#include "../host/xhci.h"
>  
>  int dwc3_host_init(struct dwc3 *dwc)
>  {
> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>  	memset(&pdata, 0, sizeof(pdata));
>  
>  	pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
> +	/* dwc3 controller need an extraordinary delay when suspending xhci. */
> +	pdata.usb3_slow_suspend = 1;
>  
>  	ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>  	if (ret) {
> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>  			  dev_name(&dwc->xhci->dev));
>  	platform_device_unregister(dwc->xhci);
>  }
> +
> +int dwc3_host_suspend(struct dwc3 *dwc)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	int ret, cnt = 20;
> +
> +try_again:
> +	 /* We should wait for xhci bus has been into suspend mode firstly. */
> +	ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
> +	if (ret && --cnt > 0) {
> +		dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
> +			 ret);
> +		msleep(200);
> +		goto try_again;
> +	}
> +
> +	return ret;
> +}
> +
> +int dwc3_host_resume(struct dwc3 *dwc)
> +{
> +	struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> +	return xhci_resume(xhci, 0);
> +}

This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
xhci_supend() has already been called. Why isn't it called for you?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 2/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data

[Baolin Wang]

Hi Felipe,

On 18 August 2016 at 15:18, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Now some usb controllers (such as dwc3 controller) need 'XHCI_SLOW_SUSPEND'
>> quirk when suspending the xhci, thus we need to add 'usb3_slow_suspend' member
>> in xhci platform data to support this.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/host/xhci-plat.c     |    3 +++
>>  include/linux/usb/xhci_pdriver.h |    3 +++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index e2e2487..162f17c 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>                       (pdata && pdata->usb3_lpm_capable))
>>               xhci->quirks |= XHCI_LPM_SUPPORT;
>>
>> +     if (pdata && pdata->usb3_slow_suspend)
>> +             xhci->quirks |= XHCI_SLOW_SUSPEND;
>
> I remember having a discussion about this with Paul Z and it turned out
> that we really didn't need SLOW_SUSPEND. Can you describe further in
> what situation you need this quirk?

On my dwc3 platform, xhci suspend will be failed if we have not
enabled XHCI_SLOW_SUSPEND quirk.


-- 
Baolin.wang
Best Regards

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[Baolin Wang]

Hi Felipe,

On 18 August 2016 at 15:33, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> For some mobile devices with strict power management, we also want to suspend
>> the host when the slave is detached for power saving.
>>
>> Thus we add the host suspend/resume functions to support this requirement, and
>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>> suspending the xhci.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>>  4 files changed, 78 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index a64ce1c..725d2bd 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>         This is the default mode of working of DWC3 controller where
>>         both host and gadget features are enabled.
>>
>> +config USB_DWC3_HOST_SUSPEND
>> +     bool "Choose if the host (xhci) can be suspend/resume"
>> +     depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>> +     help
>> +       We can suspend the host when the slave is detached for power saving,
>> +       and resume the host when one slave is attached.
>> +
>>  endchoice
>>
>>  comment "Platform Glue Driver Support"
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 1485480..5140b4d 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>  {
>>       unsigned long   flags;
>> +     int             ret;
>>
>>       switch (dwc->dr_mode) {
>>       case USB_DR_MODE_PERIPHERAL:
>> +             spin_lock_irqsave(&dwc->lock, flags);
>> +             dwc3_gadget_suspend(dwc);
>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>> +             break;
>>       case USB_DR_MODE_OTG:
>> +             ret = dwc3_host_suspend(dwc);
>> +             if (ret)
>> +                     return ret;
>> +
>>               spin_lock_irqsave(&dwc->lock, flags);
>>               dwc3_gadget_suspend(dwc);
>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>               break;
>>       case USB_DR_MODE_HOST:
>> +             ret = dwc3_host_suspend(dwc);
>> +             if (ret)
>> +                     return ret;
>>       default:
>>               /* do nothing */
>>               break;
>> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>
>>       switch (dwc->dr_mode) {
>>       case USB_DR_MODE_PERIPHERAL:
>> +             spin_lock_irqsave(&dwc->lock, flags);
>> +             dwc3_gadget_resume(dwc);
>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>> +             break;
>>       case USB_DR_MODE_OTG:
>> +             ret = dwc3_host_resume(dwc);
>> +             if (ret)
>> +                     return ret;
>> +
>>               spin_lock_irqsave(&dwc->lock, flags);
>>               dwc3_gadget_resume(dwc);
>>               spin_unlock_irqrestore(&dwc->lock, flags);
>> -             /* FALLTHROUGH */
>> +             break;
>>       case USB_DR_MODE_HOST:
>> +             ret = dwc3_host_resume(dwc);
>> +             if (ret)
>> +                     return ret;
>>       default:
>>               /* do nothing */
>>               break;
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 45d6de5..0ba203e 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>  { }
>>  #endif
>>
>> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
>> +int dwc3_host_suspend(struct dwc3 *dwc);
>> +int dwc3_host_resume(struct dwc3 *dwc);
>> +#else
>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>> +{
>> +     return 0;
>> +}
>> +
>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>> index 2e960ed..2ec3eff 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -17,8 +17,11 @@
>>
>>  #include <linux/platform_device.h>
>>  #include <linux/usb/xhci_pdriver.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/hcd.h>
>>
>>  #include "core.h"
>> +#include "../host/xhci.h"
>>
>>  int dwc3_host_init(struct dwc3 *dwc)
>>  {
>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>       memset(&pdata, 0, sizeof(pdata));
>>
>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>> +     pdata.usb3_slow_suspend = 1;
>>
>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>       if (ret) {
>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>                         dev_name(&dwc->xhci->dev));
>>       platform_device_unregister(dwc->xhci);
>>  }
>> +
>> +int dwc3_host_suspend(struct dwc3 *dwc)
>> +{
>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +     int ret, cnt = 20;
>> +
>> +try_again:
>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>> +     if (ret && --cnt > 0) {
>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>> +                      ret);
>> +             msleep(200);
>> +             goto try_again;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +int dwc3_host_resume(struct dwc3 *dwc)
>> +{
>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +
>> +     return xhci_resume(xhci, 0);
>> +}
>
> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
> xhci_supend() has already been called. Why isn't it called for you?

xhci-plat.c did not set the runtime PM callbacks, how can we issued
xhci_supend() by runtime PM from dwc3?

-- 
Baolin.wang
Best Regards

Re: [PATCH 2/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Hi Felipe,
>
> On 18 August 2016 at 15:18, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Now some usb controllers (such as dwc3 controller) need 'XHCI_SLOW_SUSPEND'
>>> quirk when suspending the xhci, thus we need to add 'usb3_slow_suspend' member
>>> in xhci platform data to support this.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>>  drivers/usb/host/xhci-plat.c     |    3 +++
>>>  include/linux/usb/xhci_pdriver.h |    3 +++
>>>  2 files changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>> index e2e2487..162f17c 100644
>>> --- a/drivers/usb/host/xhci-plat.c
>>> +++ b/drivers/usb/host/xhci-plat.c
>>> @@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>>                       (pdata && pdata->usb3_lpm_capable))
>>>               xhci->quirks |= XHCI_LPM_SUPPORT;
>>>
>>> +     if (pdata && pdata->usb3_slow_suspend)
>>> +             xhci->quirks |= XHCI_SLOW_SUSPEND;
>>
>> I remember having a discussion about this with Paul Z and it turned out
>> that we really didn't need SLOW_SUSPEND. Can you describe further in
>> what situation you need this quirk?
>
> On my dwc3 platform, xhci suspend will be failed if we have not
> enabled XHCI_SLOW_SUSPEND quirk.

fail how? What error do you see? Do you have some traces of what's
happening? Did you try figuring out if this is, perhaps, caused by some
call ordering which is wrong? Perhaps disabling PHYs too early or
something like that?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Hi Felipe,
>
> On 18 August 2016 at 15:33, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> For some mobile devices with strict power management, we also want to suspend
>>> the host when the slave is detached for power saving.
>>>
>>> Thus we add the host suspend/resume functions to support this requirement, and
>>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>>> suspending the xhci.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>>>  4 files changed, 78 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>> index a64ce1c..725d2bd 100644
>>> --- a/drivers/usb/dwc3/Kconfig
>>> +++ b/drivers/usb/dwc3/Kconfig
>>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>>         This is the default mode of working of DWC3 controller where
>>>         both host and gadget features are enabled.
>>>
>>> +config USB_DWC3_HOST_SUSPEND
>>> +     bool "Choose if the host (xhci) can be suspend/resume"
>>> +     depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>> +     help
>>> +       We can suspend the host when the slave is detached for power saving,
>>> +       and resume the host when one slave is attached.
>>> +
>>>  endchoice
>>>
>>>  comment "Platform Glue Driver Support"
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 1485480..5140b4d 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>>  {
>>>       unsigned long   flags;
>>> +     int             ret;
>>>
>>>       switch (dwc->dr_mode) {
>>>       case USB_DR_MODE_PERIPHERAL:
>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>> +             dwc3_gadget_suspend(dwc);
>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>> +             break;
>>>       case USB_DR_MODE_OTG:
>>> +             ret = dwc3_host_suspend(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>               dwc3_gadget_suspend(dwc);
>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>>               break;
>>>       case USB_DR_MODE_HOST:
>>> +             ret = dwc3_host_suspend(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>>       default:
>>>               /* do nothing */
>>>               break;
>>> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>>
>>>       switch (dwc->dr_mode) {
>>>       case USB_DR_MODE_PERIPHERAL:
>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>> +             dwc3_gadget_resume(dwc);
>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>> +             break;
>>>       case USB_DR_MODE_OTG:
>>> +             ret = dwc3_host_resume(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>> +
>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>               dwc3_gadget_resume(dwc);
>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>> -             /* FALLTHROUGH */
>>> +             break;
>>>       case USB_DR_MODE_HOST:
>>> +             ret = dwc3_host_resume(dwc);
>>> +             if (ret)
>>> +                     return ret;
>>>       default:
>>>               /* do nothing */
>>>               break;
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 45d6de5..0ba203e 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>  { }
>>>  #endif
>>>
>>> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
>>> +int dwc3_host_suspend(struct dwc3 *dwc);
>>> +int dwc3_host_resume(struct dwc3 *dwc);
>>> +#else
>>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>>> +{
>>> +     return 0;
>>> +}
>>> +
>>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>>> +{
>>> +     return 0;
>>> +}
>>> +#endif
>>> +
>>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>> index 2e960ed..2ec3eff 100644
>>> --- a/drivers/usb/dwc3/host.c
>>> +++ b/drivers/usb/dwc3/host.c
>>> @@ -17,8 +17,11 @@
>>>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/usb/xhci_pdriver.h>
>>> +#include <linux/usb.h>
>>> +#include <linux/usb/hcd.h>
>>>
>>>  #include "core.h"
>>> +#include "../host/xhci.h"
>>>
>>>  int dwc3_host_init(struct dwc3 *dwc)
>>>  {
>>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>       memset(&pdata, 0, sizeof(pdata));
>>>
>>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>>> +     pdata.usb3_slow_suspend = 1;
>>>
>>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>>       if (ret) {
>>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>                         dev_name(&dwc->xhci->dev));
>>>       platform_device_unregister(dwc->xhci);
>>>  }
>>> +
>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>> +{
>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>> +     int ret, cnt = 20;
>>> +
>>> +try_again:
>>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>>> +     if (ret && --cnt > 0) {
>>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>>> +                      ret);
>>> +             msleep(200);
>>> +             goto try_again;
>>> +     }
>>> +
>>> +     return ret;
>>> +}
>>> +
>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>> +{
>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>> +
>>> +     return xhci_resume(xhci, 0);
>>> +}
>>
>> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
>> xhci_supend() has already been called. Why isn't it called for you?
>
> xhci-plat.c did not set the runtime PM callbacks, how can we issued
> xhci_supend() by runtime PM from dwc3?

wouldn't it be nicer to just initialize PM runtime callbacks from
xhci-plat? It just needs to be added and verified, right?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 2/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data

[Baolin Wang]

Hi Felipe,

On 18 August 2016 at 20:25, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi Felipe,
>>
>> On 18 August 2016 at 15:18, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> Now some usb controllers (such as dwc3 controller) need 'XHCI_SLOW_SUSPEND'
>>>> quirk when suspending the xhci, thus we need to add 'usb3_slow_suspend' member
>>>> in xhci platform data to support this.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>>  drivers/usb/host/xhci-plat.c     |    3 +++
>>>>  include/linux/usb/xhci_pdriver.h |    3 +++
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>>> index e2e2487..162f17c 100644
>>>> --- a/drivers/usb/host/xhci-plat.c
>>>> +++ b/drivers/usb/host/xhci-plat.c
>>>> @@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>>>                       (pdata && pdata->usb3_lpm_capable))
>>>>               xhci->quirks |= XHCI_LPM_SUPPORT;
>>>>
>>>> +     if (pdata && pdata->usb3_slow_suspend)
>>>> +             xhci->quirks |= XHCI_SLOW_SUSPEND;
>>>
>>> I remember having a discussion about this with Paul Z and it turned out
>>> that we really didn't need SLOW_SUSPEND. Can you describe further in
>>> what situation you need this quirk?
>>
>> On my dwc3 platform, xhci suspend will be failed if we have not
>> enabled XHCI_SLOW_SUSPEND quirk.
>
> fail how? What error do you see? Do you have some traces of what's
> happening? Did you try figuring out if this is, perhaps, caused by some
> call ordering which is wrong? Perhaps disabling PHYs too early or
> something like that?

It shows the warning "WARN: xHC CMD_RUN timeout" when running
xhci_suspend(). If I enbale XHCI_SLOW_SUSPEND quirk, then it can work
well. I did not try to figure out other things, due to I think the
dwc3 need XHCI_SLOW_SUSPEND quirk. But I can re-try to figure out if
there are other issues if you still believe that dwc3 does not need
XHCI_SLOW_SUSPEND quirk. Thanks.

-- 
Baolin.wang
Best Regards

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[Baolin Wang]

Hi Felipe,

On 18 August 2016 at 20:25, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi Felipe,
>>
>> On 18 August 2016 at 15:33, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> For some mobile devices with strict power management, we also want to suspend
>>>> the host when the slave is detached for power saving.
>>>>
>>>> Thus we add the host suspend/resume functions to support this requirement, and
>>>> we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
>>>> suspending the xhci.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>  drivers/usb/dwc3/core.c  |   25 ++++++++++++++++++++++++-
>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>  drivers/usb/dwc3/host.c  |   32 ++++++++++++++++++++++++++++++++
>>>>  4 files changed, 78 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>> index a64ce1c..725d2bd 100644
>>>> --- a/drivers/usb/dwc3/Kconfig
>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>> @@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
>>>>         This is the default mode of working of DWC3 controller where
>>>>         both host and gadget features are enabled.
>>>>
>>>> +config USB_DWC3_HOST_SUSPEND
>>>> +     bool "Choose if the host (xhci) can be suspend/resume"
>>>> +     depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>>> +     help
>>>> +       We can suspend the host when the slave is detached for power saving,
>>>> +       and resume the host when one slave is attached.
>>>> +
>>>>  endchoice
>>>>
>>>>  comment "Platform Glue Driver Support"
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 1485480..5140b4d 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
>>>>  static int dwc3_suspend_common(struct dwc3 *dwc)
>>>>  {
>>>>       unsigned long   flags;
>>>> +     int             ret;
>>>>
>>>>       switch (dwc->dr_mode) {
>>>>       case USB_DR_MODE_PERIPHERAL:
>>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>>> +             dwc3_gadget_suspend(dwc);
>>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +             break;
>>>>       case USB_DR_MODE_OTG:
>>>> +             ret = dwc3_host_suspend(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +
>>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>>               dwc3_gadget_suspend(dwc);
>>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>>>               break;
>>>>       case USB_DR_MODE_HOST:
>>>> +             ret = dwc3_host_suspend(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>>       default:
>>>>               /* do nothing */
>>>>               break;
>>>> @@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
>>>>
>>>>       switch (dwc->dr_mode) {
>>>>       case USB_DR_MODE_PERIPHERAL:
>>>> +             spin_lock_irqsave(&dwc->lock, flags);
>>>> +             dwc3_gadget_resume(dwc);
>>>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>>>> +             break;
>>>>       case USB_DR_MODE_OTG:
>>>> +             ret = dwc3_host_resume(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>> +
>>>>               spin_lock_irqsave(&dwc->lock, flags);
>>>>               dwc3_gadget_resume(dwc);
>>>>               spin_unlock_irqrestore(&dwc->lock, flags);
>>>> -             /* FALLTHROUGH */
>>>> +             break;
>>>>       case USB_DR_MODE_HOST:
>>>> +             ret = dwc3_host_resume(dwc);
>>>> +             if (ret)
>>>> +                     return ret;
>>>>       default:
>>>>               /* do nothing */
>>>>               break;
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>>> index 45d6de5..0ba203e 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>>  { }
>>>>  #endif
>>>>
>>>> +#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
>>>> +int dwc3_host_suspend(struct dwc3 *dwc);
>>>> +int dwc3_host_resume(struct dwc3 *dwc);
>>>> +#else
>>>> +static inline int dwc3_host_suspend(struct dwc3 *dwc)
>>>> +{
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static inline int dwc3_host_resume(struct dwc3 *dwc)
>>>> +{
>>>> +     return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>  #endif /* __DRIVERS_USB_DWC3_CORE_H */
>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>> index 2e960ed..2ec3eff 100644
>>>> --- a/drivers/usb/dwc3/host.c
>>>> +++ b/drivers/usb/dwc3/host.c
>>>> @@ -17,8 +17,11 @@
>>>>
>>>>  #include <linux/platform_device.h>
>>>>  #include <linux/usb/xhci_pdriver.h>
>>>> +#include <linux/usb.h>
>>>> +#include <linux/usb/hcd.h>
>>>>
>>>>  #include "core.h"
>>>> +#include "../host/xhci.h"
>>>>
>>>>  int dwc3_host_init(struct dwc3 *dwc)
>>>>  {
>>>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>>       memset(&pdata, 0, sizeof(pdata));
>>>>
>>>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>>>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>>>> +     pdata.usb3_slow_suspend = 1;
>>>>
>>>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>>>       if (ret) {
>>>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>>                         dev_name(&dwc->xhci->dev));
>>>>       platform_device_unregister(dwc->xhci);
>>>>  }
>>>> +
>>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>>> +{
>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> +     int ret, cnt = 20;
>>>> +
>>>> +try_again:
>>>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>>>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>>>> +     if (ret && --cnt > 0) {
>>>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>>>> +                      ret);
>>>> +             msleep(200);
>>>> +             goto try_again;
>>>> +     }
>>>> +
>>>> +     return ret;
>>>> +}
>>>> +
>>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>>> +{
>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> +
>>>> +     return xhci_resume(xhci, 0);
>>>> +}
>>>
>>> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
>>> xhci_supend() has already been called. Why isn't it called for you?
>>
>> xhci-plat.c did not set the runtime PM callbacks, how can we issued
>> xhci_supend() by runtime PM from dwc3?
>
> wouldn't it be nicer to just initialize PM runtime callbacks from
> xhci-plat? It just needs to be added and verified, right?

Yes, you are right. Alan and Peter also suggested me to implement the
runtime PM callbacks for xhci-plat. I am also testing this and I will
send out the patch if it works well. So please ignore this patch.
Thanks for your comments.

-- 
Baolin.wang
Best Regards

Re: [PATCH 2/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data

[Felipe Balbi]

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


Hi Baolin,

Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>>>> index e2e2487..162f17c 100644
>>>>> --- a/drivers/usb/host/xhci-plat.c
>>>>> +++ b/drivers/usb/host/xhci-plat.c
>>>>> @@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>>>>                       (pdata && pdata->usb3_lpm_capable))
>>>>>               xhci->quirks |= XHCI_LPM_SUPPORT;
>>>>>
>>>>> +     if (pdata && pdata->usb3_slow_suspend)
>>>>> +             xhci->quirks |= XHCI_SLOW_SUSPEND;
>>>>
>>>> I remember having a discussion about this with Paul Z and it turned out
>>>> that we really didn't need SLOW_SUSPEND. Can you describe further in
>>>> what situation you need this quirk?
>>>
>>> On my dwc3 platform, xhci suspend will be failed if we have not
>>> enabled XHCI_SLOW_SUSPEND quirk.
>>
>> fail how? What error do you see? Do you have some traces of what's
>> happening? Did you try figuring out if this is, perhaps, caused by some
>> call ordering which is wrong? Perhaps disabling PHYs too early or
>> something like that?
>
> It shows the warning "WARN: xHC CMD_RUN timeout" when running
> xhci_suspend(). If I enbale XHCI_SLOW_SUSPEND quirk, then it can work
> well. I did not try to figure out other things, due to I think the
> dwc3 need XHCI_SLOW_SUSPEND quirk. But I can re-try to figure out if
> there are other issues if you still believe that dwc3 does not need
> XHCI_SLOW_SUSPEND quirk. Thanks.

When I discussed this with Paul Z, he told me there was no known
DWC3 SoC that really needed SLOW_SUSPEND, so it's likely to be a bug in
either xhci or dwc3.

Please try to track this down. I would start looking at ordering with
PHY poweroff or something like that.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume

[Felipe Balbi]

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


Hi,

Baolin Wang <baolin.wang@linaro.org> writes:

[...]

>>>>> diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
>>>>> index 2e960ed..2ec3eff 100644
>>>>> --- a/drivers/usb/dwc3/host.c
>>>>> +++ b/drivers/usb/dwc3/host.c
>>>>> @@ -17,8 +17,11 @@
>>>>>
>>>>>  #include <linux/platform_device.h>
>>>>>  #include <linux/usb/xhci_pdriver.h>
>>>>> +#include <linux/usb.h>
>>>>> +#include <linux/usb/hcd.h>
>>>>>
>>>>>  #include "core.h"
>>>>> +#include "../host/xhci.h"
>>>>>
>>>>>  int dwc3_host_init(struct dwc3 *dwc)
>>>>>  {
>>>>> @@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
>>>>>       memset(&pdata, 0, sizeof(pdata));
>>>>>
>>>>>       pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
>>>>> +     /* dwc3 controller need an extraordinary delay when suspending xhci. */
>>>>> +     pdata.usb3_slow_suspend = 1;
>>>>>
>>>>>       ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>>>>>       if (ret) {
>>>>> @@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>>>                         dev_name(&dwc->xhci->dev));
>>>>>       platform_device_unregister(dwc->xhci);
>>>>>  }
>>>>> +
>>>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>>>> +{
>>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>>> +     int ret, cnt = 20;
>>>>> +
>>>>> +try_again:
>>>>> +      /* We should wait for xhci bus has been into suspend mode firstly. */
>>>>> +     ret = xhci_suspend(xhci, device_may_wakeup(&dwc->xhci->dev));
>>>>> +     if (ret && --cnt > 0) {
>>>>> +             dev_warn(dwc->dev, "xhci suspend failed %d, try again...\n",
>>>>> +                      ret);
>>>>> +             msleep(200);
>>>>> +             goto try_again;
>>>>> +     }
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>>>> +{
>>>>> +     struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
>>>>> +     struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>>> +
>>>>> +     return xhci_resume(xhci, 0);
>>>>> +}
>>>>
>>>> This is wrong. XHCI is a child of dwc3, when dwc3 suspends,
>>>> xhci_supend() has already been called. Why isn't it called for you?
>>>
>>> xhci-plat.c did not set the runtime PM callbacks, how can we issued
>>> xhci_supend() by runtime PM from dwc3?
>>
>> wouldn't it be nicer to just initialize PM runtime callbacks from
>> xhci-plat? It just needs to be added and verified, right?
>
> Yes, you are right. Alan and Peter also suggested me to implement the
> runtime PM callbacks for xhci-plat. I am also testing this and I will
> send out the patch if it works well. So please ignore this patch.
> Thanks for your comments.

cool, thanks a lot :-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 2/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data

[Baolin Wang]

Hi Felipe,

On 18 August 2016 at 21:42, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi Baolin,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>>>>> index e2e2487..162f17c 100644
>>>>>> --- a/drivers/usb/host/xhci-plat.c
>>>>>> +++ b/drivers/usb/host/xhci-plat.c
>>>>>> @@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>>>>>                       (pdata && pdata->usb3_lpm_capable))
>>>>>>               xhci->quirks |= XHCI_LPM_SUPPORT;
>>>>>>
>>>>>> +     if (pdata && pdata->usb3_slow_suspend)
>>>>>> +             xhci->quirks |= XHCI_SLOW_SUSPEND;
>>>>>
>>>>> I remember having a discussion about this with Paul Z and it turned out
>>>>> that we really didn't need SLOW_SUSPEND. Can you describe further in
>>>>> what situation you need this quirk?
>>>>
>>>> On my dwc3 platform, xhci suspend will be failed if we have not
>>>> enabled XHCI_SLOW_SUSPEND quirk.
>>>
>>> fail how? What error do you see? Do you have some traces of what's
>>> happening? Did you try figuring out if this is, perhaps, caused by some
>>> call ordering which is wrong? Perhaps disabling PHYs too early or
>>> something like that?
>>
>> It shows the warning "WARN: xHC CMD_RUN timeout" when running
>> xhci_suspend(). If I enbale XHCI_SLOW_SUSPEND quirk, then it can work
>> well. I did not try to figure out other things, due to I think the
>> dwc3 need XHCI_SLOW_SUSPEND quirk. But I can re-try to figure out if
>> there are other issues if you still believe that dwc3 does not need
>> XHCI_SLOW_SUSPEND quirk. Thanks.
>
> When I discussed this with Paul Z, he told me there was no known
> DWC3 SoC that really needed SLOW_SUSPEND, so it's likely to be a bug in
> either xhci or dwc3.
>
> Please try to track this down. I would start looking at ordering with
> PHY poweroff or something like that.

OK. Thanks.

-- 
Baolin.wang
Best Regards

Re: [PATCH 1/4] usb: host: xhci: Move the xhci quirks checking to the right place

[Baolin Wang]

Hi Mathias,

On 18 August 2016 at 15:17, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> It will reset the xhci quirks in xhci_gen_setup() function when xhci try to
>> add one hcd, thus we need to move the XHCI_LPM_SUPPORT quirk checking after
>> adding hcd.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>>  drivers/usb/host/xhci-plat.c |    8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 1f3f981..e2e2487 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -223,10 +223,6 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>               goto disable_clk;
>>       }
>>
>> -     if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
>> -                     (pdata && pdata->usb3_lpm_capable))
>> -             xhci->quirks |= XHCI_LPM_SUPPORT;
>> -
>>       if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>>               xhci->shared_hcd->can_do_streams = 1;
>>
>> @@ -250,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>       if (ret)
>>               goto dealloc_usb2_hcd;
>>
>> +     if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
>> +                     (pdata && pdata->usb3_lpm_capable))
>> +             xhci->quirks |= XHCI_LPM_SUPPORT;
>> +
>
> Mathias, any comments here?

Do you have any comments about this? Thanks.

-- 
Baolin.wang
Best Regards