Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531060 > unrolled thread

[PATCH v4 1/2] usb: host: plat: Enable xhci plat runtime PM

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 1/2] usb: host: plat: Enable xhci plat runtime PM Baolin Wang <baolin.wang@linaro.org> - 2016-11-28 07:50 +0100
    [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Baolin Wang <baolin.wang@linaro.org> - 2016-11-28 07:50 +0100
      Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Baolin Wang <baolin.wang@linaro.org> - 2016-12-08 08:20 +0100
        Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Felipe Balbi <balbi@kernel.org> - 2016-12-08 10:50 +0100
          Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Baolin Wang <baolin.wang@linaro.org> - 2016-12-08 11:30 +0100
            Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Felipe Balbi <balbi@kernel.org> - 2016-12-08 12:10 +0100
              Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Baolin Wang <baolin.wang@linaro.org> - 2016-12-08 12:30 +0100
                Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Felipe Balbi <balbi@kernel.org> - 2016-12-08 19:00 +0100
                  Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume Baolin Wang <baolin.wang@linaro.org> - 2016-12-09 04:30 +0100
    Re: [PATCH v4 1/2] usb: host: plat: Enable xhci plat runtime PM Baolin Wang <baolin.wang@linaro.org> - 2016-12-08 08:10 +0100

#1531060 — [PATCH v4 1/2] usb: host: plat: Enable xhci plat runtime PM

Enable the xhci plat runtime PM for parent device to suspend/resume xhci.
Also call pm_runtime_get_noresume() in probe() function in case the parent
device doesn't call suspend/resume callback by runtime PM now.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v3:
 - Fix kbuild error.

Changes since v2:
 - Add pm_runtime_get_noresume() in probe() function.
 - Add pm_runtime_set_suspended()/pm_runtime_put_noidle() in remove() function.

Changes since v1:
 - No updates.
---
 drivers/usb/host/xhci-plat.c |   41 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index ed56bf9..5805c6a 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -246,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto dealloc_usb2_hcd;
 
+	pm_runtime_get_noresume(&pdev->dev);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	return 0;
 
 
@@ -274,6 +278,10 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	struct clk *clk = xhci->clk;
 
+	pm_runtime_set_suspended(&dev->dev);
+	pm_runtime_put_noidle(&dev->dev);
+	pm_runtime_disable(&dev->dev);
+
 	usb_remove_hcd(xhci->shared_hcd);
 	usb_phy_shutdown(hcd->usb_phy);
 
@@ -311,14 +319,37 @@ static int xhci_plat_resume(struct device *dev)
 
 	return xhci_resume(xhci, 0);
 }
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM
+static int xhci_plat_runtime_suspend(struct device *dev)
+{
+	struct usb_hcd  *hcd = dev_get_drvdata(dev);
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+
+	return xhci_suspend(xhci, device_may_wakeup(dev));
+}
+
+static int xhci_plat_runtime_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);
+}
+
+static int xhci_plat_runtime_idle(struct device *dev)
+{
+	return 0;
+}
+#endif /* CONFIG_PM */
 
 static const struct dev_pm_ops xhci_plat_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
+
+	SET_RUNTIME_PM_OPS(xhci_plat_runtime_suspend, xhci_plat_runtime_resume,
+			   xhci_plat_runtime_idle)
 };
-#define DEV_PM_OPS	(&xhci_plat_pm_ops)
-#else
-#define DEV_PM_OPS	NULL
-#endif /* CONFIG_PM */
 
 static const struct acpi_device_id usb_xhci_acpi_match[] = {
 	/* XHCI-compliant USB Controller */
@@ -332,7 +363,7 @@ static int xhci_plat_resume(struct device *dev)
 	.remove	= xhci_plat_remove,
 	.driver	= {
 		.name = "xhci-hcd",
-		.pm = DEV_PM_OPS,
+		.pm = &xhci_plat_pm_ops,
 		.of_match_table = of_match_ptr(usb_xhci_of_match),
 		.acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
 	},
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1531061 — [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

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.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes since v3:
 - No updates.

Changes since v2:
 - Remove pm_children_suspended() and other unused macros.

 Changes since v1:
   - Add pm_runtime.h head file to avoid kbuild error.
---
 drivers/usb/dwc3/Kconfig |    7 +++++++
 drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h  |   15 +++++++++++++++
 drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index a45b4f1..47bb2f3 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
 
 endchoice
 
+config USB_DWC3_HOST_SUSPEND
+	bool "Choose if the DWC3 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.
+
 comment "Platform Glue Driver Support"
 
 config USB_DWC3_OMAP
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9a4a5e4..7ad4bc3 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
 	pm_runtime_enable(dev);
+	pm_suspend_ignore_children(dev, true);
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		goto err1;
@@ -1215,15 +1216,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;
@@ -1245,12 +1258,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 b585a30..db41908 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1226,4 +1226,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
 { }
 #endif
 
+#if IS_ENABLED(CONFIG_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 ed82464..8e5309d6 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 
 #include "core.h"
 
@@ -130,3 +131,39 @@ void dwc3_host_exit(struct dwc3 *dwc)
 			  dev_name(&dwc->xhci->dev));
 	platform_device_unregister(dwc->xhci);
 }
+
+#ifdef CONFIG_USB_DWC3_HOST_SUSPEND
+int dwc3_host_suspend(struct dwc3 *dwc)
+{
+	struct device *xhci = &dwc->xhci->dev;
+	int ret;
+
+	/*
+	 * Note: if we get the -EBUSY, which means the xHCI children devices are
+	 * not in suspend state yet, the glue layer need to wait for a while and
+	 * try to suspend xHCI device again.
+	 */
+	ret = pm_runtime_put_sync(xhci);
+	if (ret) {
+		dev_err(xhci, "failed to suspend xHCI device\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+int dwc3_host_resume(struct dwc3 *dwc)
+{
+	struct device *xhci = &dwc->xhci->dev;
+	int ret;
+
+	/* Resume the xHCI device synchronously. */
+	ret = pm_runtime_get_sync(xhci);
+	if (ret) {
+		dev_err(xhci, "failed to resume xHCI device\n");
+		return ret;
+	}
+
+	return 0;
+}
+#endif
-- 
1.7.9.5

[toc] | [prev] | [next] | [standalone]

#1538345 — Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

Hi Felipe,

On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
> 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.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes since v3:
>  - No updates.
>
> Changes since v2:
>  - Remove pm_children_suspended() and other unused macros.
>
>  Changes since v1:
>    - Add pm_runtime.h head file to avoid kbuild error.
> ---
>  drivers/usb/dwc3/Kconfig |    7 +++++++
>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index a45b4f1..47bb2f3 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>
>  endchoice
>
> +config USB_DWC3_HOST_SUSPEND
> +       bool "Choose if the DWC3 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.
> +
>  comment "Platform Glue Driver Support"
>
>  config USB_DWC3_OMAP
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9a4a5e4..7ad4bc3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>         pm_runtime_use_autosuspend(dev);
>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>         pm_runtime_enable(dev);
> +       pm_suspend_ignore_children(dev, true);
>         ret = pm_runtime_get_sync(dev);
>         if (ret < 0)
>                 goto err1;
> @@ -1215,15 +1216,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;
> @@ -1245,12 +1258,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 b585a30..db41908 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1226,4 +1226,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>  { }
>  #endif
>
> +#if IS_ENABLED(CONFIG_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 ed82464..8e5309d6 100644
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -16,6 +16,7 @@
>   */
>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>
>  #include "core.h"
>
> @@ -130,3 +131,39 @@ void dwc3_host_exit(struct dwc3 *dwc)
>                           dev_name(&dwc->xhci->dev));
>         platform_device_unregister(dwc->xhci);
>  }
> +
> +#ifdef CONFIG_USB_DWC3_HOST_SUSPEND
> +int dwc3_host_suspend(struct dwc3 *dwc)
> +{
> +       struct device *xhci = &dwc->xhci->dev;
> +       int ret;
> +
> +       /*
> +        * Note: if we get the -EBUSY, which means the xHCI children devices are
> +        * not in suspend state yet, the glue layer need to wait for a while and
> +        * try to suspend xHCI device again.
> +        */
> +       ret = pm_runtime_put_sync(xhci);
> +       if (ret) {
> +               dev_err(xhci, "failed to suspend xHCI device\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +int dwc3_host_resume(struct dwc3 *dwc)
> +{
> +       struct device *xhci = &dwc->xhci->dev;
> +       int ret;
> +
> +       /* Resume the xHCI device synchronously. */
> +       ret = pm_runtime_get_sync(xhci);
> +       if (ret) {
> +               dev_err(xhci, "failed to resume xHCI device\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +#endif
> --
> 1.7.9.5
>

How do you think this patch and do you have any suggestion? Thanks.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1538404 — Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
> Hi Felipe,
>
> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>> 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.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes since v3:
>>  - No updates.
>>
>> Changes since v2:
>>  - Remove pm_children_suspended() and other unused macros.
>>
>>  Changes since v1:
>>    - Add pm_runtime.h head file to avoid kbuild error.
>> ---
>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>> index a45b4f1..47bb2f3 100644
>> --- a/drivers/usb/dwc3/Kconfig
>> +++ b/drivers/usb/dwc3/Kconfig
>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>
>>  endchoice
>>
>> +config USB_DWC3_HOST_SUSPEND
>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y

why do you need another Kconfig for this? Just enable it already :-p

>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 9a4a5e4..7ad4bc3 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>         pm_runtime_use_autosuspend(dev);
>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>         pm_runtime_enable(dev);
>> +       pm_suspend_ignore_children(dev, true);

why do you need this?

>> @@ -1215,15 +1216,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;
>> @@ -1245,12 +1258,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 b585a30..db41908 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -1226,4 +1226,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>  { }
>>  #endif
>>
>> +#if IS_ENABLED(CONFIG_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 ed82464..8e5309d6 100644
>> --- a/drivers/usb/dwc3/host.c
>> +++ b/drivers/usb/dwc3/host.c
>> @@ -16,6 +16,7 @@
>>   */
>>
>>  #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #include "core.h"
>>
>> @@ -130,3 +131,39 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>                           dev_name(&dwc->xhci->dev));
>>         platform_device_unregister(dwc->xhci);
>>  }
>> +
>> +#ifdef CONFIG_USB_DWC3_HOST_SUSPEND
>> +int dwc3_host_suspend(struct dwc3 *dwc)
>> +{
>> +       struct device *xhci = &dwc->xhci->dev;
>> +       int ret;
>> +
>> +       /*
>> +        * Note: if we get the -EBUSY, which means the xHCI children devices are
>> +        * not in suspend state yet, the glue layer need to wait for a while and
>> +        * try to suspend xHCI device again.
>> +        */
>> +       ret = pm_runtime_put_sync(xhci);
>> +       if (ret) {
>> +               dev_err(xhci, "failed to suspend xHCI device\n");
>> +               return ret;
>> +       }

return pm_runtime_put_sync() is probably enough here.

>> +
>> +       return 0;
>> +}
>> +
>> +int dwc3_host_resume(struct dwc3 *dwc)
>> +{
>> +       struct device *xhci = &dwc->xhci->dev;
>> +       int ret;
>> +
>> +       /* Resume the xHCI device synchronously. */
>> +       ret = pm_runtime_get_sync(xhci);

likewise.

>> +       if (ret) {
>> +               dev_err(xhci, "failed to resume xHCI device\n");
>> +               return ret;
>> +       }
>> +
>> +       return 0;
>> +}
>> +#endif
>> --
>> 1.7.9.5
>>
>
> How do you think this patch and do you have any suggestion? Thanks.
>
> -- 
> Baolin.wang
> Best Regards
> --
> 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

[toc] | [prev] | [next] | [standalone]

#1538417 — Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

Hi,

On 8 December 2016 at 17:40, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi Felipe,
>>
>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>>> 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.
>>>
>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>> ---
>>> Changes since v3:
>>>  - No updates.
>>>
>>> Changes since v2:
>>>  - Remove pm_children_suspended() and other unused macros.
>>>
>>>  Changes since v1:
>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>> ---
>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>> index a45b4f1..47bb2f3 100644
>>> --- a/drivers/usb/dwc3/Kconfig
>>> +++ b/drivers/usb/dwc3/Kconfig
>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>
>>>  endchoice
>>>
>>> +config USB_DWC3_HOST_SUSPEND
>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>
> why do you need another Kconfig for this? Just enable it already :-p

I assume some platforms may do not need this feature. But okay, I can
remove this kconfig and enable it.

>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 9a4a5e4..7ad4bc3 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>         pm_runtime_use_autosuspend(dev);
>>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>>         pm_runtime_enable(dev);
>>> +       pm_suspend_ignore_children(dev, true);
>
> why do you need this?

Since the dwc3 device is the parent deive of xhci device, if we want
to suspend dwc3 device, we need to suspend child device (xhci device)
manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
pm_runtime_put_sync(), it will check if the children (xhci device) of
dwc3 device have been in suspend state, if not we will suspend dwc3
device failed.

We get/put the child device manually in parent device's runtime
callback, we need to ignore the child device's runtime state, or it
will failed due to the dependency.

>
>>> @@ -1215,15 +1216,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;
>>> @@ -1245,12 +1258,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 b585a30..db41908 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -1226,4 +1226,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
>>>  { }
>>>  #endif
>>>
>>> +#if IS_ENABLED(CONFIG_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 ed82464..8e5309d6 100644
>>> --- a/drivers/usb/dwc3/host.c
>>> +++ b/drivers/usb/dwc3/host.c
>>> @@ -16,6 +16,7 @@
>>>   */
>>>
>>>  #include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>>
>>>  #include "core.h"
>>>
>>> @@ -130,3 +131,39 @@ void dwc3_host_exit(struct dwc3 *dwc)
>>>                           dev_name(&dwc->xhci->dev));
>>>         platform_device_unregister(dwc->xhci);
>>>  }
>>> +
>>> +#ifdef CONFIG_USB_DWC3_HOST_SUSPEND
>>> +int dwc3_host_suspend(struct dwc3 *dwc)
>>> +{
>>> +       struct device *xhci = &dwc->xhci->dev;
>>> +       int ret;
>>> +
>>> +       /*
>>> +        * Note: if we get the -EBUSY, which means the xHCI children devices are
>>> +        * not in suspend state yet, the glue layer need to wait for a while and
>>> +        * try to suspend xHCI device again.
>>> +        */
>>> +       ret = pm_runtime_put_sync(xhci);
>>> +       if (ret) {
>>> +               dev_err(xhci, "failed to suspend xHCI device\n");
>>> +               return ret;
>>> +       }
>
> return pm_runtime_put_sync() is probably enough here.

OK.

>
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +int dwc3_host_resume(struct dwc3 *dwc)
>>> +{
>>> +       struct device *xhci = &dwc->xhci->dev;
>>> +       int ret;
>>> +
>>> +       /* Resume the xHCI device synchronously. */
>>> +       ret = pm_runtime_get_sync(xhci);
>
> likewise.

OK. I will fix these in next version. Thanks.

>
>>> +       if (ret) {
>>> +               dev_err(xhci, "failed to resume xHCI device\n");
>>> +               return ret;
>>> +       }
>>> +
>>> +       return 0;
>>> +}
>>> +#endif
>>> --
>>> 1.7.9.5
>>>
>>
>> How do you think this patch and do you have any suggestion? Thanks.
>>
>> --
>> Baolin.wang
>> Best Regards
>> --
>> 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



-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1538432 — Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

[Multipart message — attachments visible in raw view] — view raw

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>> ---
>>>> Changes since v3:
>>>>  - No updates.
>>>>
>>>> Changes since v2:
>>>>  - Remove pm_children_suspended() and other unused macros.
>>>>
>>>>  Changes since v1:
>>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>>> ---
>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>> index a45b4f1..47bb2f3 100644
>>>> --- a/drivers/usb/dwc3/Kconfig
>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>>
>>>>  endchoice
>>>>
>>>> +config USB_DWC3_HOST_SUSPEND
>>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>
>> why do you need another Kconfig for this? Just enable it already :-p
>
> I assume some platforms may do not need this feature. But okay, I can
> remove this kconfig and enable it.

thanks :-)

>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 9a4a5e4..7ad4bc3 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>         pm_runtime_use_autosuspend(dev);
>>>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>>>         pm_runtime_enable(dev);
>>>> +       pm_suspend_ignore_children(dev, true);
>>
>> why do you need this?
>
> Since the dwc3 device is the parent deive of xhci device, if we want
> to suspend dwc3 device, we need to suspend child device (xhci device)
> manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
> pm_runtime_put_sync(), it will check if the children (xhci device) of
> dwc3 device have been in suspend state, if not we will suspend dwc3
> device failed.
>
> We get/put the child device manually in parent device's runtime
> callback, we need to ignore the child device's runtime state, or it
> will failed due to the dependency.

I see. Good explanation :-)

There's one thing though, if you want to runtime suspend the gadget and
dwc3 is working on peripheral mode, host side (XHCI) should already be
runtime suspended because there's nothing attached to it. Why isn't it
runtime suspended?

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1538447 — Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

Hi,

On 8 December 2016 at 19:02, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>> ---
>>>>> Changes since v3:
>>>>>  - No updates.
>>>>>
>>>>> Changes since v2:
>>>>>  - Remove pm_children_suspended() and other unused macros.
>>>>>
>>>>>  Changes since v1:
>>>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>>>> ---
>>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>>> index a45b4f1..47bb2f3 100644
>>>>> --- a/drivers/usb/dwc3/Kconfig
>>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>>>
>>>>>  endchoice
>>>>>
>>>>> +config USB_DWC3_HOST_SUSPEND
>>>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>>
>>> why do you need another Kconfig for this? Just enable it already :-p
>>
>> I assume some platforms may do not need this feature. But okay, I can
>> remove this kconfig and enable it.
>
> thanks :-)
>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 9a4a5e4..7ad4bc3 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>         pm_runtime_use_autosuspend(dev);
>>>>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>>>>         pm_runtime_enable(dev);
>>>>> +       pm_suspend_ignore_children(dev, true);
>>>
>>> why do you need this?
>>
>> Since the dwc3 device is the parent deive of xhci device, if we want
>> to suspend dwc3 device, we need to suspend child device (xhci device)
>> manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
>> pm_runtime_put_sync(), it will check if the children (xhci device) of
>> dwc3 device have been in suspend state, if not we will suspend dwc3
>> device failed.
>>
>> We get/put the child device manually in parent device's runtime
>> callback, we need to ignore the child device's runtime state, or it
>> will failed due to the dependency.
>
> I see. Good explanation :-)
>
> There's one thing though, if you want to runtime suspend the gadget and
> dwc3 is working on peripheral mode, host side (XHCI) should already be
> runtime suspended because there's nothing attached to it. Why isn't it
> runtime suspended?

Since we have get the runtime count for xHCI device in
xhci_plat_probe(), in case it will suspend automatically if some
controllers do not want xHCI enters runtime suspend automatically. So
the parent device (dwc3 device) need to get/put the xHCI device's
runtime count  to resume/suspend xHCI.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1538764 — Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

Hi,

Baolin Wang <baolin.wang@linaro.org> writes:
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>>> 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.
>>>>>>
>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>> ---
>>>>>> Changes since v3:
>>>>>>  - No updates.
>>>>>>
>>>>>> Changes since v2:
>>>>>>  - Remove pm_children_suspended() and other unused macros.
>>>>>>
>>>>>>  Changes since v1:
>>>>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>>>>> ---
>>>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>>>> index a45b4f1..47bb2f3 100644
>>>>>> --- a/drivers/usb/dwc3/Kconfig
>>>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>>>>
>>>>>>  endchoice
>>>>>>
>>>>>> +config USB_DWC3_HOST_SUSPEND
>>>>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>>>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>>>
>>>> why do you need another Kconfig for this? Just enable it already :-p
>>>
>>> I assume some platforms may do not need this feature. But okay, I can
>>> remove this kconfig and enable it.
>>
>> thanks :-)
>>
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 9a4a5e4..7ad4bc3 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>>         pm_runtime_use_autosuspend(dev);
>>>>>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>>>>>         pm_runtime_enable(dev);
>>>>>> +       pm_suspend_ignore_children(dev, true);
>>>>
>>>> why do you need this?
>>>
>>> Since the dwc3 device is the parent deive of xhci device, if we want
>>> to suspend dwc3 device, we need to suspend child device (xhci device)
>>> manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
>>> pm_runtime_put_sync(), it will check if the children (xhci device) of
>>> dwc3 device have been in suspend state, if not we will suspend dwc3
>>> device failed.
>>>
>>> We get/put the child device manually in parent device's runtime
>>> callback, we need to ignore the child device's runtime state, or it
>>> will failed due to the dependency.
>>
>> I see. Good explanation :-)
>>
>> There's one thing though, if you want to runtime suspend the gadget and
>> dwc3 is working on peripheral mode, host side (XHCI) should already be
>> runtime suspended because there's nothing attached to it. Why isn't it
>> runtime suspended?
>
> Since we have get the runtime count for xHCI device in
> xhci_plat_probe(), in case it will suspend automatically if some
> controllers do not want xHCI enters runtime suspend automatically. So
> the parent device (dwc3 device) need to get/put the xHCI device's
> runtime count  to resume/suspend xHCI.

IMHO, that's a bug in xhci-plat, not something dwc3 should work around.

-- 
balbi

[toc] | [prev] | [next] | [standalone]

#1539049 — Re: [PATCH v4 2/2] usb: dwc3: core: Support the dwc3 host suspend/resume

Hi,

On 9 December 2016 at 01:52, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>>>> On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>>>>>>> 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.
>>>>>>>
>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>>>>>>> ---
>>>>>>> Changes since v3:
>>>>>>>  - No updates.
>>>>>>>
>>>>>>> Changes since v2:
>>>>>>>  - Remove pm_children_suspended() and other unused macros.
>>>>>>>
>>>>>>>  Changes since v1:
>>>>>>>    - Add pm_runtime.h head file to avoid kbuild error.
>>>>>>> ---
>>>>>>>  drivers/usb/dwc3/Kconfig |    7 +++++++
>>>>>>>  drivers/usb/dwc3/core.c  |   26 +++++++++++++++++++++++++-
>>>>>>>  drivers/usb/dwc3/core.h  |   15 +++++++++++++++
>>>>>>>  drivers/usb/dwc3/host.c  |   37 +++++++++++++++++++++++++++++++++++++
>>>>>>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
>>>>>>> index a45b4f1..47bb2f3 100644
>>>>>>> --- a/drivers/usb/dwc3/Kconfig
>>>>>>> +++ b/drivers/usb/dwc3/Kconfig
>>>>>>> @@ -47,6 +47,13 @@ config USB_DWC3_DUAL_ROLE
>>>>>>>
>>>>>>>  endchoice
>>>>>>>
>>>>>>> +config USB_DWC3_HOST_SUSPEND
>>>>>>> +       bool "Choose if the DWC3 host (xhci) can be suspend/resume"
>>>>>>> +       depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
>>>>>
>>>>> why do you need another Kconfig for this? Just enable it already :-p
>>>>
>>>> I assume some platforms may do not need this feature. But okay, I can
>>>> remove this kconfig and enable it.
>>>
>>> thanks :-)
>>>
>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>>> index 9a4a5e4..7ad4bc3 100644
>>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>>> @@ -1091,6 +1091,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>>>         pm_runtime_use_autosuspend(dev);
>>>>>>>         pm_runtime_set_autosuspend_delay(dev, DWC3_DEFAULT_AUTOSUSPEND_DELAY);
>>>>>>>         pm_runtime_enable(dev);
>>>>>>> +       pm_suspend_ignore_children(dev, true);
>>>>>
>>>>> why do you need this?
>>>>
>>>> Since the dwc3 device is the parent deive of xhci device, if we want
>>>> to suspend dwc3 device, we need to suspend child device (xhci device)
>>>> manually by issuing pm_runtime_put_sync() in dwc3_host_suspend(). In
>>>> pm_runtime_put_sync(), it will check if the children (xhci device) of
>>>> dwc3 device have been in suspend state, if not we will suspend dwc3
>>>> device failed.
>>>>
>>>> We get/put the child device manually in parent device's runtime
>>>> callback, we need to ignore the child device's runtime state, or it
>>>> will failed due to the dependency.
>>>
>>> I see. Good explanation :-)
>>>
>>> There's one thing though, if you want to runtime suspend the gadget and
>>> dwc3 is working on peripheral mode, host side (XHCI) should already be
>>> runtime suspended because there's nothing attached to it. Why isn't it
>>> runtime suspended?
>>
>> Since we have get the runtime count for xHCI device in
>> xhci_plat_probe(), in case it will suspend automatically if some
>> controllers do not want xHCI enters runtime suspend automatically. So
>> the parent device (dwc3 device) need to get/put the xHCI device's
>> runtime count  to resume/suspend xHCI.
>
> IMHO, that's a bug in xhci-plat, not something dwc3 should work around.

Maybe you misunderstood my meaning and I should make it clear.

If dwc3 is just working on peripheral mode, then no need to initialize
the host (xhci) things, which means it is invalid to runtime
suspend/resume xHCI device and we can just runtime suspend/resume the
gadget.

If dwc3 is working on OTG mode, which will create and add the USB hcd
for host side. Then if we want to suspend dwc3, we need to suspend
xHCI device manually though now dwc3 acts as peripheral. The USB
device (bus) of host side will runtime suspend automatically if there
are no slave attached (by
usb_runtime_suspend()--->usb_suspend_both()--->usb_suspend_interface()--->usb_suspend_device()--->generic_suspend()--->hcd_bus_suspend()--->xhci_bus_suspend()),
but we should not suspend xHCI device (issuing xhci_suspend()) now,
since some controllers did not implement the runtime PM to control
xHCI device's runtime state, which will cause controllers can not
resume xHCI device (issuing xhci_resume()) if the xHCI device suspend
automatically by its child device. Thus we should get the runtime
count for xHCI device in xhci_plat_probe() in case xHCI device will
also suspend automatically by its child device. According to that, for
xHCI device's parent: dwc3 device, we should put the xHCI device's
runtime count to suspend xHCI device manually. Hope I make it clear.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [next] | [standalone]

#1538344

Hi Mathias and Felipe,

On 28 November 2016 at 14:43, Baolin Wang <baolin.wang@linaro.org> wrote:
> Enable the xhci plat runtime PM for parent device to suspend/resume xhci.
> Also call pm_runtime_get_noresume() in probe() function in case the parent
> device doesn't call suspend/resume callback by runtime PM now.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes since v3:
>  - Fix kbuild error.
>
> Changes since v2:
>  - Add pm_runtime_get_noresume() in probe() function.
>  - Add pm_runtime_set_suspended()/pm_runtime_put_noidle() in remove() function.
>
> Changes since v1:
>  - No updates.
> ---
>  drivers/usb/host/xhci-plat.c |   41 ++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index ed56bf9..5805c6a 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -246,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
>         if (ret)
>                 goto dealloc_usb2_hcd;
>
> +       pm_runtime_get_noresume(&pdev->dev);
> +       pm_runtime_set_active(&pdev->dev);
> +       pm_runtime_enable(&pdev->dev);
> +
>         return 0;
>
>
> @@ -274,6 +278,10 @@ static int xhci_plat_remove(struct platform_device *dev)
>         struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>         struct clk *clk = xhci->clk;
>
> +       pm_runtime_set_suspended(&dev->dev);
> +       pm_runtime_put_noidle(&dev->dev);
> +       pm_runtime_disable(&dev->dev);
> +
>         usb_remove_hcd(xhci->shared_hcd);
>         usb_phy_shutdown(hcd->usb_phy);
>
> @@ -311,14 +319,37 @@ static int xhci_plat_resume(struct device *dev)
>
>         return xhci_resume(xhci, 0);
>  }
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM
> +static int xhci_plat_runtime_suspend(struct device *dev)
> +{
> +       struct usb_hcd  *hcd = dev_get_drvdata(dev);
> +       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> +       return xhci_suspend(xhci, device_may_wakeup(dev));
> +}
> +
> +static int xhci_plat_runtime_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);
> +}
> +
> +static int xhci_plat_runtime_idle(struct device *dev)
> +{
> +       return 0;
> +}
> +#endif /* CONFIG_PM */
>
>  static const struct dev_pm_ops xhci_plat_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(xhci_plat_suspend, xhci_plat_resume)
> +
> +       SET_RUNTIME_PM_OPS(xhci_plat_runtime_suspend, xhci_plat_runtime_resume,
> +                          xhci_plat_runtime_idle)
>  };
> -#define DEV_PM_OPS     (&xhci_plat_pm_ops)
> -#else
> -#define DEV_PM_OPS     NULL
> -#endif /* CONFIG_PM */
>
>  static const struct acpi_device_id usb_xhci_acpi_match[] = {
>         /* XHCI-compliant USB Controller */
> @@ -332,7 +363,7 @@ static int xhci_plat_resume(struct device *dev)
>         .remove = xhci_plat_remove,
>         .driver = {
>                 .name = "xhci-hcd",
> -               .pm = DEV_PM_OPS,
> +               .pm = &xhci_plat_pm_ops,
>                 .of_match_table = of_match_ptr(usb_xhci_of_match),
>                 .acpi_match_table = ACPI_PTR(usb_xhci_acpi_match),
>         },
> --
> 1.7.9.5
>

Do you have any comments about this patch? Thanks.

-- 
Baolin.wang
Best Regards

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web