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

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

[Vikas C Sajjan]

From: Vikas Sajjan <vikas.sajjan@linaro.org>

This patchset adds suspend/resume functionality to dwc3-core layer
and xhci-platform driver. It also adds S2R support to dwc3-exynos
glue layer.

Based on 'usb-next' branch.

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

 drivers/usb/dwc3/core.c        |  268 +++++++++++++++++++++++++---------------
 drivers/usb/dwc3/dwc3-exynos.c |   60 +++++++++
 drivers/usb/host/xhci-plat.c   |   44 +++++++
 3 files changed, 273 insertions(+), 99 deletions(-)

-- 
1.7.6.5

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

[Vikas C Sajjan]

From: Vikas Sajjan <vikas.sajjan@linaro.org>

Adding the suspend and resume funtionality to DWC3 core.

Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
CC: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/dwc3/core.c |  268 +++++++++++++++++++++++++++++-----------------
 1 files changed, 169 insertions(+), 99 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b415c0c..58b51e1 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -70,51 +70,20 @@ MODULE_PARM_DESC(maximum_speed, "Maximum supported speed.");
 
 static DECLARE_BITMAP(dwc3_devs, DWC3_DEVS_POSSIBLE);
 
-int dwc3_get_device_id(void)
-{
-	int		id;
-
-again:
-	id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
-	if (id < DWC3_DEVS_POSSIBLE) {
-		int old;
-
-		old = test_and_set_bit(id, dwc3_devs);
-		if (old)
-			goto again;
-	} else {
-		pr_err("dwc3: no space for new device\n");
-		id = -ENOMEM;
-	}
-
-	return id;
-}
-EXPORT_SYMBOL_GPL(dwc3_get_device_id);
-
-void dwc3_put_device_id(int id)
-{
-	int			ret;
-
-	if (id < 0)
-		return;
-
-	ret = test_bit(id, dwc3_devs);
-	WARN(!ret, "dwc3: ID %d not in use\n", id);
-	smp_mb__before_clear_bit();
-	clear_bit(id, dwc3_devs);
-}
-EXPORT_SYMBOL_GPL(dwc3_put_device_id);
-
-void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
+static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
 {
-	u32 reg;
+	struct dwc3_hwparams	*parms = &dwc->hwparams;
 
-	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
-	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
-	reg |= DWC3_GCTL_PRTCAPDIR(mode);
-	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+	parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
+	parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
+	parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
+	parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
+	parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
+	parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
+	parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
+	parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
+	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
 }
-
 /**
  * dwc3_core_soft_reset - Issues core soft reset and PHY reset
  * @dwc: pointer to our context structure
@@ -160,6 +129,102 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
 }
 
+static int dwc3_core_reset(struct dwc3 *dwc)
+{
+	unsigned long	timeout;
+	u32	reg;
+
+	/* issue device SoftReset too */
+	timeout = jiffies + msecs_to_jiffies(500);
+	dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
+	do {
+		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
+		if (!(reg & DWC3_DCTL_CSFTRST))
+			break;
+
+		if (time_after(jiffies, timeout)) {
+			dev_err(dwc->dev, "Reset Timed Out\n");
+			return -ETIMEDOUT;
+		}
+
+		cpu_relax();
+	} while (true);
+
+	dwc3_core_soft_reset(dwc);
+
+	dwc3_cache_hwparams(dwc);
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
+	reg &= ~DWC3_GCTL_DISSCRAMBLE;
+
+	switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
+	case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
+		reg &= ~DWC3_GCTL_DSBLCLKGTNG;
+		break;
+	default:
+		dev_dbg(dwc->dev, "No power optimization available\n");
+	}
+
+	/*
+	 * WORKAROUND: DWC3 revisions <1.90a have a bug
+	 * where the device can fail to connect at SuperSpeed
+	 * and falls back to high-speed mode which causes
+	 * the device to enter a Connect/Disconnect loop
+	 */
+	if (dwc->revision < DWC3_REVISION_190A)
+		reg |= DWC3_GCTL_U2RSTECN;
+
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	return 0;
+}
+
+int dwc3_get_device_id(void)
+{
+	int		id;
+
+again:
+	id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
+	if (id < DWC3_DEVS_POSSIBLE) {
+		int old;
+
+		old = test_and_set_bit(id, dwc3_devs);
+		if (old)
+			goto again;
+	} else {
+		pr_err("dwc3: no space for new device\n");
+		id = -ENOMEM;
+	}
+
+	return id;
+}
+EXPORT_SYMBOL_GPL(dwc3_get_device_id);
+
+void dwc3_put_device_id(int id)
+{
+	int			ret;
+
+	if (id < 0)
+		return;
+
+	ret = test_bit(id, dwc3_devs);
+	WARN(!ret, "dwc3: ID %d not in use\n", id);
+	smp_mb__before_clear_bit();
+	clear_bit(id, dwc3_devs);
+}
+EXPORT_SYMBOL_GPL(dwc3_put_device_id);
+
+void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
+{
+	u32 reg;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
+	reg |= DWC3_GCTL_PRTCAPDIR(mode);
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+}
+
 /**
  * dwc3_free_one_event_buffer - Frees one event buffer
  * @dwc: Pointer to our controller context structure
@@ -303,20 +368,7 @@ static void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
 	}
 }
 
-static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
-{
-	struct dwc3_hwparams	*parms = &dwc->hwparams;
 
-	parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
-	parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
-	parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
-	parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
-	parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
-	parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
-	parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
-	parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
-	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
-}
 
 /**
  * dwc3_core_init - Low-level initialization of DWC3 Core
@@ -326,7 +378,6 @@ static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
  */
 static int __devinit dwc3_core_init(struct dwc3 *dwc)
 {
-	unsigned long		timeout;
 	u32			reg;
 	int			ret;
 
@@ -339,49 +390,9 @@ static int __devinit dwc3_core_init(struct dwc3 *dwc)
 	}
 	dwc->revision = reg;
 
-	/* issue device SoftReset too */
-	timeout = jiffies + msecs_to_jiffies(500);
-	dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
-	do {
-		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
-		if (!(reg & DWC3_DCTL_CSFTRST))
-			break;
-
-		if (time_after(jiffies, timeout)) {
-			dev_err(dwc->dev, "Reset Timed Out\n");
-			ret = -ETIMEDOUT;
-			goto err0;
-		}
-
-		cpu_relax();
-	} while (true);
-
-	dwc3_core_soft_reset(dwc);
-
-	dwc3_cache_hwparams(dwc);
-
-	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
-	reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
-	reg &= ~DWC3_GCTL_DISSCRAMBLE;
-
-	switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
-	case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
-		reg &= ~DWC3_GCTL_DSBLCLKGTNG;
-		break;
-	default:
-		dev_dbg(dwc->dev, "No power optimization available\n");
-	}
-
-	/*
-	 * WORKAROUND: DWC3 revisions <1.90a have a bug
-	 * where the device can fail to connect at SuperSpeed
-	 * and falls back to high-speed mode which causes
-	 * the device to enter a Connect/Disconnect loop
-	 */
-	if (dwc->revision < DWC3_REVISION_190A)
-		reg |= DWC3_GCTL_U2RSTECN;
-
-	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+	ret = dwc3_core_reset(dwc);
+	if (ret < 0)
+		goto err0;
 
 	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
 	if (ret) {
@@ -622,11 +633,70 @@ static int __devexit dwc3_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int dwc3_core_resume(struct device *dev)
+{
+	struct dwc3	*dwc;
+	int	ret;
+
+	dwc = dev_get_drvdata(dev);
+	if (!dwc)
+		return -EINVAL;
+
+	ret = dwc3_core_reset(dwc);
+	if (ret < 0)
+		return ret;
+
+	ret = dwc3_event_buffers_setup(dwc);
+	if (ret < 0)
+		return ret;
+
+	switch (dwc->mode) {
+	case DWC3_MODE_DEVICE:
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+		break;
+	case DWC3_MODE_HOST:
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+		break;
+	case DWC3_MODE_DRD:
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
+	}
+
+	/* runtime set active to reflect active state. */
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static int dwc3_core_suspend(struct device *dev)
+{
+	struct dwc3	*dwc;
+
+	dwc = dev_get_drvdata(dev);
+	if (!dwc)
+		return -EINVAL;
+
+	dwc3_event_buffers_cleanup(dwc);
+
+	return 0;
+}
+
+static const struct dev_pm_ops dwc3_core_pm_ops = {
+	.suspend		= dwc3_core_suspend,
+	.resume			= dwc3_core_resume,
+};
+#endif /* CONFIG_PM */
+
 static struct platform_driver dwc3_driver = {
 	.probe		= dwc3_probe,
 	.remove		= __devexit_p(dwc3_remove),
 	.driver		= {
 		.name	= "dwc3",
+#ifdef CONFIG_PM
+		.pm = &dwc3_core_pm_ops,
+#endif
 	},
 };
 
-- 
1.7.6.5

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

[Felipe Balbi]

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

Hi,

On Wed, Oct 10, 2012 at 07:35:47PM +0530, Vikas C Sajjan wrote:
> From: Vikas Sajjan <vikas.sajjan@linaro.org>
> 
> Adding the suspend and resume funtionality to DWC3 core.
> 
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/usb/dwc3/core.c |  268 +++++++++++++++++++++++++++++-----------------
>  1 files changed, 169 insertions(+), 99 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index b415c0c..58b51e1 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -70,51 +70,20 @@ MODULE_PARM_DESC(maximum_speed, "Maximum supported speed.");
>  
>  static DECLARE_BITMAP(dwc3_devs, DWC3_DEVS_POSSIBLE);
>  
> -int dwc3_get_device_id(void)
> -{
> -	int		id;
> -
> -again:
> -	id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> -	if (id < DWC3_DEVS_POSSIBLE) {
> -		int old;
> -
> -		old = test_and_set_bit(id, dwc3_devs);
> -		if (old)
> -			goto again;
> -	} else {
> -		pr_err("dwc3: no space for new device\n");
> -		id = -ENOMEM;
> -	}
> -
> -	return id;
> -}
> -EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> -
> -void dwc3_put_device_id(int id)
> -{
> -	int			ret;
> -
> -	if (id < 0)
> -		return;
> -
> -	ret = test_bit(id, dwc3_devs);
> -	WARN(!ret, "dwc3: ID %d not in use\n", id);
> -	smp_mb__before_clear_bit();
> -	clear_bit(id, dwc3_devs);
> -}
> -EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> -
> -void dwc3_set_mode(struct dwc3 *dwc, u32 mode)

why are you even moving all this code around ? Doesn't look necessary.

> +static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
>  {
> -	u32 reg;
> +	struct dwc3_hwparams	*parms = &dwc->hwparams;
>  
> -	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> -	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> -	reg |= DWC3_GCTL_PRTCAPDIR(mode);
> -	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +	parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> +	parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> +	parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> +	parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> +	parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> +	parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> +	parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> +	parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> +	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
>  }
> -
>  /**
>   * dwc3_core_soft_reset - Issues core soft reset and PHY reset
>   * @dwc: pointer to our context structure
> @@ -160,6 +129,102 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>  }
>  
> +static int dwc3_core_reset(struct dwc3 *dwc)

this looks unnecessary.

> +{
> +	unsigned long	timeout;
> +	u32	reg;
> +
> +	/* issue device SoftReset too */
> +	timeout = jiffies + msecs_to_jiffies(500);
> +	dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> +	do {
> +		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> +		if (!(reg & DWC3_DCTL_CSFTRST))
> +			break;
> +
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(dwc->dev, "Reset Timed Out\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		cpu_relax();
> +	} while (true);
> +
> +	dwc3_core_soft_reset(dwc);
> +
> +	dwc3_cache_hwparams(dwc);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> +	reg &= ~DWC3_GCTL_DISSCRAMBLE;
> +
> +	switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> +	case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> +		reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> +		break;
> +	default:
> +		dev_dbg(dwc->dev, "No power optimization available\n");
> +	}
> +
> +	/*
> +	 * WORKAROUND: DWC3 revisions <1.90a have a bug
> +	 * where the device can fail to connect at SuperSpeed
> +	 * and falls back to high-speed mode which causes
> +	 * the device to enter a Connect/Disconnect loop
> +	 */
> +	if (dwc->revision < DWC3_REVISION_190A)
> +		reg |= DWC3_GCTL_U2RSTECN;
> +
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> +	return 0;
> +}
> +
> +int dwc3_get_device_id(void)
> +{
> +	int		id;
> +
> +again:
> +	id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> +	if (id < DWC3_DEVS_POSSIBLE) {
> +		int old;
> +
> +		old = test_and_set_bit(id, dwc3_devs);
> +		if (old)
> +			goto again;
> +	} else {
> +		pr_err("dwc3: no space for new device\n");
> +		id = -ENOMEM;
> +	}
> +
> +	return id;
> +}
> +EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> +
> +void dwc3_put_device_id(int id)
> +{
> +	int			ret;
> +
> +	if (id < 0)
> +		return;
> +
> +	ret = test_bit(id, dwc3_devs);
> +	WARN(!ret, "dwc3: ID %d not in use\n", id);
> +	smp_mb__before_clear_bit();
> +	clear_bit(id, dwc3_devs);
> +}
> +EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> +
> +void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> +{
> +	u32 reg;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> +	reg |= DWC3_GCTL_PRTCAPDIR(mode);
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +}
> +
>  /**
>   * dwc3_free_one_event_buffer - Frees one event buffer
>   * @dwc: Pointer to our controller context structure
> @@ -303,20 +368,7 @@ static void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
>  	}
>  }
>  
> -static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
> -{
> -	struct dwc3_hwparams	*parms = &dwc->hwparams;
>  
> -	parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> -	parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> -	parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> -	parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> -	parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> -	parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> -	parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> -	parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> -	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
> -}
>  
>  /**
>   * dwc3_core_init - Low-level initialization of DWC3 Core
> @@ -326,7 +378,6 @@ static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
>   */
>  static int __devinit dwc3_core_init(struct dwc3 *dwc)
>  {
> -	unsigned long		timeout;
>  	u32			reg;
>  	int			ret;
>  
> @@ -339,49 +390,9 @@ static int __devinit dwc3_core_init(struct dwc3 *dwc)
>  	}
>  	dwc->revision = reg;
>  
> -	/* issue device SoftReset too */
> -	timeout = jiffies + msecs_to_jiffies(500);
> -	dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> -	do {
> -		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> -		if (!(reg & DWC3_DCTL_CSFTRST))
> -			break;
> -
> -		if (time_after(jiffies, timeout)) {
> -			dev_err(dwc->dev, "Reset Timed Out\n");
> -			ret = -ETIMEDOUT;
> -			goto err0;
> -		}
> -
> -		cpu_relax();
> -	} while (true);
> -
> -	dwc3_core_soft_reset(dwc);
> -
> -	dwc3_cache_hwparams(dwc);
> -
> -	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> -	reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> -	reg &= ~DWC3_GCTL_DISSCRAMBLE;
> -
> -	switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> -	case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> -		reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> -		break;
> -	default:
> -		dev_dbg(dwc->dev, "No power optimization available\n");
> -	}
> -
> -	/*
> -	 * WORKAROUND: DWC3 revisions <1.90a have a bug
> -	 * where the device can fail to connect at SuperSpeed
> -	 * and falls back to high-speed mode which causes
> -	 * the device to enter a Connect/Disconnect loop
> -	 */
> -	if (dwc->revision < DWC3_REVISION_190A)
> -		reg |= DWC3_GCTL_U2RSTECN;
> -
> -	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +	ret = dwc3_core_reset(dwc);
> +	if (ret < 0)
> +		goto err0;
>  
>  	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);

I would rather move dwc3_alloc_event_buffers() outside of
dwc3_core_init() and use that directly on ->resume(). dwc3_core_init()
shouldn't be doing any memory allocations, so we can re-use it, and it
doesn't matter when we allocate the event buffers anyway.

> @@ -622,11 +633,70 @@ static int __devexit dwc3_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int dwc3_core_resume(struct device *dev)
> +{
> +	struct dwc3	*dwc;
> +	int	ret;
> +
> +	dwc = dev_get_drvdata(dev);

can be done together with declaration.

> +	if (!dwc)
> +		return -EINVAL;

unnecessary.

> +	ret = dwc3_core_reset(dwc);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = dwc3_event_buffers_setup(dwc);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (dwc->mode) {
> +	case DWC3_MODE_DEVICE:
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> +		break;
> +	case DWC3_MODE_HOST:
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> +		break;
> +	case DWC3_MODE_DRD:
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> +	}
> +
> +	/* runtime set active to reflect active state. */
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static int dwc3_core_suspend(struct device *dev)
> +{
> +	struct dwc3	*dwc;
> +
> +	dwc = dev_get_drvdata(dev);
> +	if (!dwc)
> +		return -EINVAL;
> +
> +	dwc3_event_buffers_cleanup(dwc);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops dwc3_core_pm_ops = {

A little nit-picky, but I would rather you remove the _core part of the
prefix :-p likewise for suspend and resume callbacks.

> +	.suspend		= dwc3_core_suspend,
> +	.resume			= dwc3_core_resume,
> +};

#define DWC3_PM_OPS	&(dwc3_pm_ops)
#else
#define DWC3_PM_OPS	NULL

> +#endif /* CONFIG_PM */
> +
>  static struct platform_driver dwc3_driver = {
>  	.probe		= dwc3_probe,
>  	.remove		= __devexit_p(dwc3_remove),
>  	.driver		= {
>  		.name	= "dwc3",
> +#ifdef CONFIG_PM
> +		.pm = &dwc3_core_pm_ops,
> +#endif

remove ifdef:

		.pm = DWC3_PM_OPS,

-- 
balbi

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

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

[Vikas Sajjan]

Hi Felipe,

On 11 October 2012 13:17, Felipe Balbi <balbi@ti.com> wrote:
>
> Hi,
>
> On Wed, Oct 10, 2012 at 07:35:47PM +0530, Vikas C Sajjan wrote:
> > From: Vikas Sajjan <vikas.sajjan@linaro.org>
> >
> > Adding the suspend and resume funtionality to DWC3 core.
> >
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
> > CC: Doug Anderson <dianders@chromium.org>
> > ---
> >  drivers/usb/dwc3/core.c |  268
> > +++++++++++++++++++++++++++++-----------------
> >  1 files changed, 169 insertions(+), 99 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index b415c0c..58b51e1 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -70,51 +70,20 @@ MODULE_PARM_DESC(maximum_speed, "Maximum supported
> > speed.");
> >
> >  static DECLARE_BITMAP(dwc3_devs, DWC3_DEVS_POSSIBLE);
> >
> > -int dwc3_get_device_id(void)
> > -{
> > -     int             id;
> > -
> > -again:
> > -     id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> > -     if (id < DWC3_DEVS_POSSIBLE) {
> > -             int old;
> > -
> > -             old = test_and_set_bit(id, dwc3_devs);
> > -             if (old)
> > -                     goto again;
> > -     } else {
> > -             pr_err("dwc3: no space for new device\n");
> > -             id = -ENOMEM;
> > -     }
> > -
> > -     return id;
> > -}
> > -EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> > -
> > -void dwc3_put_device_id(int id)
> > -{
> > -     int                     ret;
> > -
> > -     if (id < 0)
> > -             return;
> > -
> > -     ret = test_bit(id, dwc3_devs);
> > -     WARN(!ret, "dwc3: ID %d not in use\n", id);
> > -     smp_mb__before_clear_bit();
> > -     clear_bit(id, dwc3_devs);
> > -}
> > -EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> > -
> > -void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>
> why are you even moving all this code around ? Doesn't look necessary.
>
> > +static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
> >  {
> > -     u32 reg;
> > +     struct dwc3_hwparams    *parms = &dwc->hwparams;
> >
> > -     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > -     reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> > -     reg |= DWC3_GCTL_PRTCAPDIR(mode);
> > -     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +     parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> > +     parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> > +     parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> > +     parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> > +     parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> > +     parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> > +     parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> > +     parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> > +     parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
> >  }
> > -
> >  /**
> >   * dwc3_core_soft_reset - Issues core soft reset and PHY reset
> >   * @dwc: pointer to our context structure
> > @@ -160,6 +129,102 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
> >       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> >  }
> >
> > +static int dwc3_core_reset(struct dwc3 *dwc)
>
> this looks unnecessary.
>
dwc3_core_reset() function is added to avoid the code duplication, and
can be reused both in probe and resume.
> > +{
> > +     unsigned long   timeout;
> > +     u32     reg;
> > +
> > +     /* issue device SoftReset too */
> > +     timeout = jiffies + msecs_to_jiffies(500);
> > +     dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> > +     do {
> > +             reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> > +             if (!(reg & DWC3_DCTL_CSFTRST))
> > +                     break;
> > +
> > +             if (time_after(jiffies, timeout)) {
> > +                     dev_err(dwc->dev, "Reset Timed Out\n");
> > +                     return -ETIMEDOUT;
> > +             }
> > +
> > +             cpu_relax();
> > +     } while (true);
> > +
> > +     dwc3_core_soft_reset(dwc);
> > +
> > +     dwc3_cache_hwparams(dwc);
> > +
> > +     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > +     reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> > +     reg &= ~DWC3_GCTL_DISSCRAMBLE;
> > +
> > +     switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> > +     case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> > +             reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> > +             break;
> > +     default:
> > +             dev_dbg(dwc->dev, "No power optimization available\n");
> > +     }
> > +
> > +     /*
> > +      * WORKAROUND: DWC3 revisions <1.90a have a bug
> > +      * where the device can fail to connect at SuperSpeed
> > +      * and falls back to high-speed mode which causes
> > +      * the device to enter a Connect/Disconnect loop
> > +      */
> > +     if (dwc->revision < DWC3_REVISION_190A)
> > +             reg |= DWC3_GCTL_U2RSTECN;
> > +
> > +     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +
> > +     return 0;
> > +}
> > +
> > +int dwc3_get_device_id(void)
> > +{
> > +     int             id;
> > +
> > +again:
> > +     id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> > +     if (id < DWC3_DEVS_POSSIBLE) {
> > +             int old;
> > +
> > +             old = test_and_set_bit(id, dwc3_devs);
> > +             if (old)
> > +                     goto again;
> > +     } else {
> > +             pr_err("dwc3: no space for new device\n");
> > +             id = -ENOMEM;
> > +     }
> > +
> > +     return id;
> > +}
> > +EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> > +
> > +void dwc3_put_device_id(int id)
> > +{
> > +     int                     ret;
> > +
> > +     if (id < 0)
> > +             return;
> > +
> > +     ret = test_bit(id, dwc3_devs);
> > +     WARN(!ret, "dwc3: ID %d not in use\n", id);
> > +     smp_mb__before_clear_bit();
> > +     clear_bit(id, dwc3_devs);
> > +}
> > +EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> > +
> > +void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> > +{
> > +     u32 reg;
> > +
> > +     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > +     reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> > +     reg |= DWC3_GCTL_PRTCAPDIR(mode);
> > +     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +}
> > +
> >  /**
> >   * dwc3_free_one_event_buffer - Frees one event buffer
> >   * @dwc: Pointer to our controller context structure
> > @@ -303,20 +368,7 @@ static void dwc3_event_buffers_cleanup(struct dwc3
> > *dwc)
> >       }
> >  }
> >
> > -static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
> > -{
> > -     struct dwc3_hwparams    *parms = &dwc->hwparams;
> >
> > -     parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> > -     parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> > -     parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> > -     parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> > -     parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> > -     parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> > -     parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> > -     parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> > -     parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
> > -}
> >
> >  /**
> >   * dwc3_core_init - Low-level initialization of DWC3 Core
> > @@ -326,7 +378,6 @@ static void __devinit dwc3_cache_hwparams(struct
> > dwc3 *dwc)
> >   */
> >  static int __devinit dwc3_core_init(struct dwc3 *dwc)
> >  {
> > -     unsigned long           timeout;
> >       u32                     reg;
> >       int                     ret;
> >
> > @@ -339,49 +390,9 @@ static int __devinit dwc3_core_init(struct dwc3
> > *dwc)
> >       }
> >       dwc->revision = reg;
> >
> > -     /* issue device SoftReset too */
> > -     timeout = jiffies + msecs_to_jiffies(500);
> > -     dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> > -     do {
> > -             reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> > -             if (!(reg & DWC3_DCTL_CSFTRST))
> > -                     break;
> > -
> > -             if (time_after(jiffies, timeout)) {
> > -                     dev_err(dwc->dev, "Reset Timed Out\n");
> > -                     ret = -ETIMEDOUT;
> > -                     goto err0;
> > -             }
> > -
> > -             cpu_relax();
> > -     } while (true);
> > -
> > -     dwc3_core_soft_reset(dwc);
> > -
> > -     dwc3_cache_hwparams(dwc);
> > -
> > -     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > -     reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> > -     reg &= ~DWC3_GCTL_DISSCRAMBLE;
> > -
> > -     switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> > -     case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> > -             reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> > -             break;
> > -     default:
> > -             dev_dbg(dwc->dev, "No power optimization available\n");
> > -     }
> > -
> > -     /*
> > -      * WORKAROUND: DWC3 revisions <1.90a have a bug
> > -      * where the device can fail to connect at SuperSpeed
> > -      * and falls back to high-speed mode which causes
> > -      * the device to enter a Connect/Disconnect loop
> > -      */
> > -     if (dwc->revision < DWC3_REVISION_190A)
> > -             reg |= DWC3_GCTL_U2RSTECN;
> > -
> > -     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +     ret = dwc3_core_reset(dwc);
> > +     if (ret < 0)
> > +             goto err0;
> >
> >       ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
>
> I would rather move dwc3_alloc_event_buffers() outside of
> dwc3_core_init() and use that directly on ->resume(). dwc3_core_init()
> shouldn't be doing any memory allocations, so we can re-use it, and it
> doesn't matter when we allocate the event buffers anyway.
>
dwc3_alloc_event_buffers() was already inside the dwc3_core_init() in
the existing code, the only change I made is, a common function called
 dwc3_core_reset() is added which will be used both in probe and
resume.
so basically resume only does dwc3_core_reset() and
dwc3_event_buffers_setup() as dwc3_alloc_event_buffers() is done in
probe.

> > @@ -622,11 +633,70 @@ static int __devexit dwc3_remove(struct
> > platform_device *pdev)
> >       return 0;
> >  }
> >
> > +#ifdef CONFIG_PM
> > +static int dwc3_core_resume(struct device *dev)
> > +{
> > +     struct dwc3     *dwc;
> > +     int     ret;
> > +
> > +     dwc = dev_get_drvdata(dev);
>
> can be done together with declaration.
>
> > +     if (!dwc)
> > +             return -EINVAL;
>
> unnecessary.
>
> > +     ret = dwc3_core_reset(dwc);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = dwc3_event_buffers_setup(dwc);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     switch (dwc->mode) {
> > +     case DWC3_MODE_DEVICE:
> > +             dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> > +             break;
> > +     case DWC3_MODE_HOST:
> > +             dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> > +             break;
> > +     case DWC3_MODE_DRD:
> > +             dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> > +     }
> > +
> > +     /* runtime set active to reflect active state. */
> > +     pm_runtime_disable(dev);
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_enable(dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int dwc3_core_suspend(struct device *dev)
> > +{
> > +     struct dwc3     *dwc;
> > +
> > +     dwc = dev_get_drvdata(dev);
> > +     if (!dwc)
> > +             return -EINVAL;
> > +
> > +     dwc3_event_buffers_cleanup(dwc);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dev_pm_ops dwc3_core_pm_ops = {
>
> A little nit-picky, but I would rather you remove the _core part of the
> prefix :-p likewise for suspend and resume callbacks.
>
> > +     .suspend                = dwc3_core_suspend,
> > +     .resume                 = dwc3_core_resume,
> > +};
>
> #define DWC3_PM_OPS     &(dwc3_pm_ops)
> #else
> #define DWC3_PM_OPS     NULL
>
> > +#endif /* CONFIG_PM */
> > +
> >  static struct platform_driver dwc3_driver = {
> >       .probe          = dwc3_probe,
> >       .remove         = __devexit_p(dwc3_remove),
> >       .driver         = {
> >               .name   = "dwc3",
> > +#ifdef CONFIG_PM
> > +             .pm = &dwc3_core_pm_ops,
> > +#endif
>
> remove ifdef:
>
>                 .pm = DWC3_PM_OPS,
>
> --
> balbi




--
Thanks and Regards
 Vikas Sajjan

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

[Felipe Balbi]

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

Hi,

On Fri, Oct 12, 2012 at 02:29:39PM +0530, Vikas Sajjan wrote:
> > > @@ -339,49 +390,9 @@ static int __devinit dwc3_core_init(struct dwc3
> > > *dwc)
> > >       }
> > >       dwc->revision = reg;
> > >
> > > -     /* issue device SoftReset too */
> > > -     timeout = jiffies + msecs_to_jiffies(500);
> > > -     dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> > > -     do {
> > > -             reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> > > -             if (!(reg & DWC3_DCTL_CSFTRST))
> > > -                     break;
> > > -
> > > -             if (time_after(jiffies, timeout)) {
> > > -                     dev_err(dwc->dev, "Reset Timed Out\n");
> > > -                     ret = -ETIMEDOUT;
> > > -                     goto err0;
> > > -             }
> > > -
> > > -             cpu_relax();
> > > -     } while (true);
> > > -
> > > -     dwc3_core_soft_reset(dwc);
> > > -
> > > -     dwc3_cache_hwparams(dwc);
> > > -
> > > -     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > > -     reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> > > -     reg &= ~DWC3_GCTL_DISSCRAMBLE;
> > > -
> > > -     switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> > > -     case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> > > -             reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> > > -             break;
> > > -     default:
> > > -             dev_dbg(dwc->dev, "No power optimization available\n");
> > > -     }
> > > -
> > > -     /*
> > > -      * WORKAROUND: DWC3 revisions <1.90a have a bug
> > > -      * where the device can fail to connect at SuperSpeed
> > > -      * and falls back to high-speed mode which causes
> > > -      * the device to enter a Connect/Disconnect loop
> > > -      */
> > > -     if (dwc->revision < DWC3_REVISION_190A)
> > > -             reg |= DWC3_GCTL_U2RSTECN;
> > > -
> > > -     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > > +     ret = dwc3_core_reset(dwc);
> > > +     if (ret < 0)
> > > +             goto err0;
> > >
> > >       ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
> >
> > I would rather move dwc3_alloc_event_buffers() outside of
> > dwc3_core_init() and use that directly on ->resume(). dwc3_core_init()
> > shouldn't be doing any memory allocations, so we can re-use it, and it
> > doesn't matter when we allocate the event buffers anyway.
> >
> dwc3_alloc_event_buffers() was already inside the dwc3_core_init() in
> the existing code, the only change I made is, a common function called
>  dwc3_core_reset() is added which will be used both in probe and
> resume.
> so basically resume only does dwc3_core_reset() and
> dwc3_event_buffers_setup() as dwc3_alloc_event_buffers() is done in
> probe.

we already have too many functions call *reset*, adding another one will
just make code more difficult to follow and it will be error-prone.

This is what I suggested:

commit 9aba985696690c1e8c2cf4f34e35b3f5b296175d
Author: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Date:   Thu Oct 11 13:54:36 2012 +0300

    usb: dwc3: core: move event buffer allocation out of dwc3_core_init()
    
    This patch is in preparation for adding PM support
    dwc3 driver. We want to re-use dwc3_core_init and
    dwc3_core_exit() functions on resume() and suspend()
    callbacks respectively.
    
    Moving even buffer allocation away from dwc3_core_init()
    will allow us to reuse the event buffer which was allocated
    long ago on our probe() routine.
    
    Signed-off-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 8d543ea..b923183 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -381,24 +381,14 @@ static int __devinit dwc3_core_init(struct dwc3 *dwc)
 
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
 
-	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
-	if (ret) {
-		dev_err(dwc->dev, "failed to allocate event buffers\n");
-		ret = -ENOMEM;
-		goto err1;
-	}
-
 	ret = dwc3_event_buffers_setup(dwc);
 	if (ret) {
 		dev_err(dwc->dev, "failed to setup event buffers\n");
-		goto err1;
+		goto err0;
 	}
 
 	return 0;
 
-err1:
-	dwc3_free_event_buffers(dwc);
-
 err0:
 	return ret;
 }
@@ -406,7 +396,6 @@ err0:
 static void dwc3_core_exit(struct dwc3 *dwc)
 {
 	dwc3_event_buffers_cleanup(dwc);
-	dwc3_free_event_buffers(dwc);
 }
 
 #define DWC3_ALIGN_MASK		(16 - 1)
@@ -509,10 +498,17 @@ static int __devinit dwc3_probe(struct platform_device *pdev)
 	pm_runtime_get_sync(dev);
 	pm_runtime_forbid(dev);
 
+	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
+	if (ret) {
+		dev_err(dwc->dev, "failed to allocate event buffers\n");
+		ret = -ENOMEM;
+		goto err0;
+	}
+
 	ret = dwc3_core_init(dwc);
 	if (ret) {
 		dev_err(dev, "failed to initialize core\n");
-		return ret;
+		goto err0;
 	}
 
 	mode = DWC3_MODE(dwc->hwparams.hwparams0);
@@ -584,6 +580,9 @@ err2:
 err1:
 	dwc3_core_exit(dwc);
 
+err0:
+	dwc3_free_event_buffers(dwc);
+
 	return ret;
 }
 

please write your patches on top of this change. I will push it to my
kernel.org tree, branch dwc3. Note that it will still rebase that branch
on top of v3.7-rc1 once it's out, I'm pushing that patch now so you can
use it on your PM changes. Note that it will still rebase that branch on
top of v3.7-rc1 once it's out, I'm pushing that patch now so you can use
it on your PM changes. Note that it will still rebase that branch on top
of v3.7-rc1 once it's out, I'm pushing that patch now so you can use it
on your PM changes.

-- 
balbi

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

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

[kishon]

Hi,

On Wednesday 10 October 2012 07:35 PM, Vikas C Sajjan wrote:
> From: Vikas Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>
> Adding the suspend and resume funtionality to DWC3 core.
>
> Signed-off-by: Abhilash Kesavan <a.kesavan-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> CC: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>   drivers/usb/dwc3/core.c |  268 +++++++++++++++++++++++++++++-----------------
>   1 files changed, 169 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index b415c0c..58b51e1 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -70,51 +70,20 @@ MODULE_PARM_DESC(maximum_speed, "Maximum supported speed.");
>
>   static DECLARE_BITMAP(dwc3_devs, DWC3_DEVS_POSSIBLE);
>
> -int dwc3_get_device_id(void)
> -{
> -	int		id;
> -
> -again:
> -	id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> -	if (id < DWC3_DEVS_POSSIBLE) {
> -		int old;
> -
> -		old = test_and_set_bit(id, dwc3_devs);
> -		if (old)
> -			goto again;
> -	} else {
> -		pr_err("dwc3: no space for new device\n");
> -		id = -ENOMEM;
> -	}
> -
> -	return id;
> -}
> -EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> -
> -void dwc3_put_device_id(int id)
> -{
> -	int			ret;
> -
> -	if (id < 0)
> -		return;
> -
> -	ret = test_bit(id, dwc3_devs);
> -	WARN(!ret, "dwc3: ID %d not in use\n", id);
> -	smp_mb__before_clear_bit();
> -	clear_bit(id, dwc3_devs);
> -}
> -EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> -
> -void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> +static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
>   {
> -	u32 reg;
> +	struct dwc3_hwparams	*parms = &dwc->hwparams;
>
> -	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> -	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> -	reg |= DWC3_GCTL_PRTCAPDIR(mode);
> -	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +	parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> +	parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> +	parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> +	parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> +	parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> +	parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> +	parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> +	parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> +	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
>   }
> -
>   /**
>    * dwc3_core_soft_reset - Issues core soft reset and PHY reset
>    * @dwc: pointer to our context structure
> @@ -160,6 +129,102 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>   	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>   }
>
> +static int dwc3_core_reset(struct dwc3 *dwc)
> +{
> +	unsigned long	timeout;
> +	u32	reg;
> +
> +	/* issue device SoftReset too */
> +	timeout = jiffies + msecs_to_jiffies(500);
> +	dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> +	do {
> +		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> +		if (!(reg & DWC3_DCTL_CSFTRST))
> +			break;
> +
> +		if (time_after(jiffies, timeout)) {
> +			dev_err(dwc->dev, "Reset Timed Out\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		cpu_relax();
> +	} while (true);
> +
> +	dwc3_core_soft_reset(dwc);
> +
> +	dwc3_cache_hwparams(dwc);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> +	reg &= ~DWC3_GCTL_DISSCRAMBLE;
> +
> +	switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> +	case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> +		reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> +		break;
> +	default:
> +		dev_dbg(dwc->dev, "No power optimization available\n");
> +	}
> +
> +	/*
> +	 * WORKAROUND: DWC3 revisions <1.90a have a bug
> +	 * where the device can fail to connect at SuperSpeed
> +	 * and falls back to high-speed mode which causes
> +	 * the device to enter a Connect/Disconnect loop
> +	 */
> +	if (dwc->revision < DWC3_REVISION_190A)
> +		reg |= DWC3_GCTL_U2RSTECN;
> +
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> +	return 0;
> +}
> +
> +int dwc3_get_device_id(void)
> +{
> +	int		id;
> +
> +again:
> +	id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> +	if (id < DWC3_DEVS_POSSIBLE) {
> +		int old;
> +
> +		old = test_and_set_bit(id, dwc3_devs);
> +		if (old)
> +			goto again;
> +	} else {
> +		pr_err("dwc3: no space for new device\n");
> +		id = -ENOMEM;
> +	}
> +
> +	return id;
> +}
> +EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> +
> +void dwc3_put_device_id(int id)
> +{
> +	int			ret;
> +
> +	if (id < 0)
> +		return;
> +
> +	ret = test_bit(id, dwc3_devs);
> +	WARN(!ret, "dwc3: ID %d not in use\n", id);
> +	smp_mb__before_clear_bit();
> +	clear_bit(id, dwc3_devs);
> +}
> +EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> +
> +void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> +{
> +	u32 reg;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> +	reg |= DWC3_GCTL_PRTCAPDIR(mode);
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +}
> +
>   /**
>    * dwc3_free_one_event_buffer - Frees one event buffer
>    * @dwc: Pointer to our controller context structure
> @@ -303,20 +368,7 @@ static void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
>   	}
>   }
>
> -static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
> -{
> -	struct dwc3_hwparams	*parms = &dwc->hwparams;
>
> -	parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> -	parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> -	parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> -	parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> -	parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> -	parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> -	parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> -	parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> -	parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
> -}
>
>   /**
>    * dwc3_core_init - Low-level initialization of DWC3 Core
> @@ -326,7 +378,6 @@ static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
>    */
>   static int __devinit dwc3_core_init(struct dwc3 *dwc)
>   {
> -	unsigned long		timeout;
>   	u32			reg;
>   	int			ret;
>
> @@ -339,49 +390,9 @@ static int __devinit dwc3_core_init(struct dwc3 *dwc)
>   	}
>   	dwc->revision = reg;
>
> -	/* issue device SoftReset too */
> -	timeout = jiffies + msecs_to_jiffies(500);
> -	dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> -	do {
> -		reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> -		if (!(reg & DWC3_DCTL_CSFTRST))
> -			break;
> -
> -		if (time_after(jiffies, timeout)) {
> -			dev_err(dwc->dev, "Reset Timed Out\n");
> -			ret = -ETIMEDOUT;
> -			goto err0;
> -		}
> -
> -		cpu_relax();
> -	} while (true);
> -
> -	dwc3_core_soft_reset(dwc);
> -
> -	dwc3_cache_hwparams(dwc);
> -
> -	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> -	reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> -	reg &= ~DWC3_GCTL_DISSCRAMBLE;
> -
> -	switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> -	case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> -		reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> -		break;
> -	default:
> -		dev_dbg(dwc->dev, "No power optimization available\n");
> -	}
> -
> -	/*
> -	 * WORKAROUND: DWC3 revisions <1.90a have a bug
> -	 * where the device can fail to connect at SuperSpeed
> -	 * and falls back to high-speed mode which causes
> -	 * the device to enter a Connect/Disconnect loop
> -	 */
> -	if (dwc->revision < DWC3_REVISION_190A)
> -		reg |= DWC3_GCTL_U2RSTECN;
> -
> -	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +	ret = dwc3_core_reset(dwc);
> +	if (ret < 0)
> +		goto err0;
>
>   	ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
>   	if (ret) {
> @@ -622,11 +633,70 @@ static int __devexit dwc3_remove(struct platform_device *pdev)
>   	return 0;
>   }
>
> +#ifdef CONFIG_PM
> +static int dwc3_core_resume(struct device *dev)
> +{
> +	struct dwc3	*dwc;
> +	int	ret;
> +
> +	dwc = dev_get_drvdata(dev);
> +	if (!dwc)
> +		return -EINVAL;
> +
> +	ret = dwc3_core_reset(dwc);

Do you have to add core_reset because of any issues you observed? In the 
ideal working case, you shouldn't have to do reset every-time on resume.

Maybe I have missed your mail so I'm asking Felipe's question again. how 
has this patch series been tested?

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

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

[Vikas Sajjan]

Hi kishon,

On 12 October 2012 16:27, kishon <kishon@ti.com> wrote:
> Hi,
>
>
> On Wednesday 10 October 2012 07:35 PM, Vikas C Sajjan wrote:
>>
>> From: Vikas Sajjan <vikas.sajjan@linaro.org>
>>
>> Adding the suspend and resume funtionality to DWC3 core.
>>
>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>> Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
>> CC: Doug Anderson <dianders@chromium.org>
>> ---
>>   drivers/usb/dwc3/core.c |  268
>> +++++++++++++++++++++++++++++-----------------
>>   1 files changed, 169 insertions(+), 99 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index b415c0c..58b51e1 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>>
>> @@ -70,51 +70,20 @@ MODULE_PARM_DESC(maximum_speed, "Maximum supported
>> speed.");
>>
>>   static DECLARE_BITMAP(dwc3_devs, DWC3_DEVS_POSSIBLE);
>>
>> -int dwc3_get_device_id(void)
>> -{
>> -       int             id;
>> -
>> -again:
>> -       id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
>> -       if (id < DWC3_DEVS_POSSIBLE) {
>> -               int old;
>> -
>> -               old = test_and_set_bit(id, dwc3_devs);
>> -               if (old)
>> -                       goto again;
>> -       } else {
>> -               pr_err("dwc3: no space for new device\n");
>> -               id = -ENOMEM;
>> -       }
>> -
>> -       return id;
>> -}
>> -EXPORT_SYMBOL_GPL(dwc3_get_device_id);
>> -
>> -void dwc3_put_device_id(int id)
>> -{
>> -       int                     ret;
>> -
>> -       if (id < 0)
>> -               return;
>> -
>> -       ret = test_bit(id, dwc3_devs);
>> -       WARN(!ret, "dwc3: ID %d not in use\n", id);
>> -       smp_mb__before_clear_bit();
>> -       clear_bit(id, dwc3_devs);
>> -}
>> -EXPORT_SYMBOL_GPL(dwc3_put_device_id);
>> -
>> -void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>> +static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
>>   {
>> -       u32 reg;
>> +       struct dwc3_hwparams    *parms = &dwc->hwparams;
>>
>> -       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> -       reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>> -       reg |= DWC3_GCTL_PRTCAPDIR(mode);
>> -       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +       parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
>> +       parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
>> +       parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
>> +       parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
>> +       parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
>> +       parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
>> +       parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
>> +       parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
>> +       parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
>>   }
>> -
>>   /**
>>    * dwc3_core_soft_reset - Issues core soft reset and PHY reset
>>    * @dwc: pointer to our context structure
>> @@ -160,6 +129,102 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
>>         dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>>   }
>>
>> +static int dwc3_core_reset(struct dwc3 *dwc)
>> +{
>> +       unsigned long   timeout;
>> +       u32     reg;
>> +
>> +       /* issue device SoftReset too */
>> +       timeout = jiffies + msecs_to_jiffies(500);
>> +       dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
>> +       do {
>> +               reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>> +               if (!(reg & DWC3_DCTL_CSFTRST))
>> +                       break;
>> +
>> +               if (time_after(jiffies, timeout)) {
>> +                       dev_err(dwc->dev, "Reset Timed Out\n");
>> +                       return -ETIMEDOUT;
>> +               }
>> +
>> +               cpu_relax();
>> +       } while (true);
>> +
>> +       dwc3_core_soft_reset(dwc);
>> +
>> +       dwc3_cache_hwparams(dwc);
>> +
>> +       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> +       reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
>> +       reg &= ~DWC3_GCTL_DISSCRAMBLE;
>> +
>> +       switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
>> +       case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
>> +               reg &= ~DWC3_GCTL_DSBLCLKGTNG;
>> +               break;
>> +       default:
>> +               dev_dbg(dwc->dev, "No power optimization available\n");
>> +       }
>> +
>> +       /*
>>
>> +        * WORKAROUND: DWC3 revisions <1.90a have a bug
>> +        * where the device can fail to connect at SuperSpeed
>> +        * and falls back to high-speed mode which causes
>> +        * the device to enter a Connect/Disconnect loop
>> +        */
>> +       if (dwc->revision < DWC3_REVISION_190A)
>> +               reg |= DWC3_GCTL_U2RSTECN;
>> +
>> +       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +
>> +       return 0;
>> +}
>> +
>> +int dwc3_get_device_id(void)
>> +{
>> +       int             id;
>> +
>> +again:
>> +       id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
>> +       if (id < DWC3_DEVS_POSSIBLE) {
>> +               int old;
>> +
>> +               old = test_and_set_bit(id, dwc3_devs);
>> +               if (old)
>> +                       goto again;
>> +       } else {
>> +               pr_err("dwc3: no space for new device\n");
>> +               id = -ENOMEM;
>> +       }
>> +
>> +       return id;
>> +}
>> +EXPORT_SYMBOL_GPL(dwc3_get_device_id);
>> +
>> +void dwc3_put_device_id(int id)
>> +{
>> +       int                     ret;
>> +
>> +       if (id < 0)
>> +               return;
>> +
>> +       ret = test_bit(id, dwc3_devs);
>> +       WARN(!ret, "dwc3: ID %d not in use\n", id);
>> +       smp_mb__before_clear_bit();
>> +       clear_bit(id, dwc3_devs);
>> +}
>> +EXPORT_SYMBOL_GPL(dwc3_put_device_id);
>> +
>> +void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>> +{
>> +       u32 reg;
>> +
>> +       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> +       reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>> +       reg |= DWC3_GCTL_PRTCAPDIR(mode);
>> +       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +}
>> +
>>   /**
>>    * dwc3_free_one_event_buffer - Frees one event buffer
>>    * @dwc: Pointer to our controller context structure
>> @@ -303,20 +368,7 @@ static void dwc3_event_buffers_cleanup(struct dwc3
>> *dwc)
>>         }
>>   }
>>
>> -static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
>> -{
>> -       struct dwc3_hwparams    *parms = &dwc->hwparams;
>>
>> -       parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
>> -       parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
>> -       parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
>> -       parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
>> -       parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
>> -       parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
>> -       parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
>> -       parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
>> -       parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
>> -}
>>
>>   /**
>>    * dwc3_core_init - Low-level initialization of DWC3 Core
>> @@ -326,7 +378,6 @@ static void __devinit dwc3_cache_hwparams(struct dwc3
>> *dwc)
>>    */
>>   static int __devinit dwc3_core_init(struct dwc3 *dwc)
>>   {
>> -       unsigned long           timeout;
>>         u32                     reg;
>>         int                     ret;
>>
>> @@ -339,49 +390,9 @@ static int __devinit dwc3_core_init(struct dwc3 *dwc)
>>         }
>>         dwc->revision = reg;
>>
>> -       /* issue device SoftReset too */
>> -       timeout = jiffies + msecs_to_jiffies(500);
>> -       dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
>> -       do {
>> -               reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>> -               if (!(reg & DWC3_DCTL_CSFTRST))
>> -                       break;
>> -
>> -               if (time_after(jiffies, timeout)) {
>> -                       dev_err(dwc->dev, "Reset Timed Out\n");
>> -                       ret = -ETIMEDOUT;
>> -                       goto err0;
>> -               }
>> -
>> -               cpu_relax();
>> -       } while (true);
>> -
>> -       dwc3_core_soft_reset(dwc);
>> -
>> -       dwc3_cache_hwparams(dwc);
>> -
>> -       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>> -       reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
>> -       reg &= ~DWC3_GCTL_DISSCRAMBLE;
>> -
>> -       switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
>> -       case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
>> -               reg &= ~DWC3_GCTL_DSBLCLKGTNG;
>> -               break;
>> -       default:
>> -               dev_dbg(dwc->dev, "No power optimization available\n");
>> -       }
>> -
>> -       /*
>> -        * WORKAROUND: DWC3 revisions <1.90a have a bug
>> -        * where the device can fail to connect at SuperSpeed
>> -        * and falls back to high-speed mode which causes
>> -        * the device to enter a Connect/Disconnect loop
>> -        */
>> -       if (dwc->revision < DWC3_REVISION_190A)
>> -               reg |= DWC3_GCTL_U2RSTECN;
>> -
>> -       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +       ret = dwc3_core_reset(dwc);
>> +       if (ret < 0)
>> +               goto err0;
>>
>>         ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
>>         if (ret) {
>>
>> @@ -622,11 +633,70 @@ static int __devexit dwc3_remove(struct
>> platform_device *pdev)
>>         return 0;
>>   }
>>
>> +#ifdef CONFIG_PM
>> +static int dwc3_core_resume(struct device *dev)
>> +{
>> +       struct dwc3     *dwc;
>> +       int     ret;
>> +
>> +       dwc = dev_get_drvdata(dev);
>> +       if (!dwc)
>> +               return -EINVAL;
>> +
>> +       ret = dwc3_core_reset(dwc);
>
>
> Do you have to add core_reset because of any issues you observed? In the
> ideal working case, you shouldn't have to do reset every-time on resume.
>
Initially we tested on kernel 3.4 with which we had some issue in detecting
USB devices on SS hub. But now as you pointed out, we tested after removing
core_reset  during resume, and it seems to be working fine.
Thanks for pointing out, we shall re-confirm and update the new patch-set soon.

> Maybe I have missed your mail so I'm asking Felipe's question again. how has
> this patch series been tested?
>
We tested the patch series on 'usb-next' on greg's tree after applying
our local patches
for exynos5 USB3.0 PHY support and also reverting the patch for dwc3-core
"usb: dwc3: add basic PHY support" since our USB3.0 PHY driver is underway.

> Thanks
> Kishon



-- 
Thanks and Regards
 Vikas Sajjan

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

[Felipe Balbi]

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

On Fri, Oct 12, 2012 at 07:24:33PM +0530, Vikas Sajjan wrote:
> Hi kishon,
> 
> On 12 October 2012 16:27, kishon <kishon-l0cyMroinI0@public.gmane.org> wrote:
> > Hi,
> >
> >
> > On Wednesday 10 October 2012 07:35 PM, Vikas C Sajjan wrote:
> >>
> >> From: Vikas Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>
> >> Adding the suspend and resume funtionality to DWC3 core.
> >>
> >> Signed-off-by: Abhilash Kesavan <a.kesavan-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >> Signed-off-by: Vikas C Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >> CC: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >> ---
> >>   drivers/usb/dwc3/core.c |  268
> >> +++++++++++++++++++++++++++++-----------------
> >>   1 files changed, 169 insertions(+), 99 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >> index b415c0c..58b51e1 100644
> >> --- a/drivers/usb/dwc3/core.c
> >> +++ b/drivers/usb/dwc3/core.c
> >>
> >> @@ -70,51 +70,20 @@ MODULE_PARM_DESC(maximum_speed, "Maximum supported
> >> speed.");
> >>
> >>   static DECLARE_BITMAP(dwc3_devs, DWC3_DEVS_POSSIBLE);
> >>
> >> -int dwc3_get_device_id(void)
> >> -{
> >> -       int             id;
> >> -
> >> -again:
> >> -       id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> >> -       if (id < DWC3_DEVS_POSSIBLE) {
> >> -               int old;
> >> -
> >> -               old = test_and_set_bit(id, dwc3_devs);
> >> -               if (old)
> >> -                       goto again;
> >> -       } else {
> >> -               pr_err("dwc3: no space for new device\n");
> >> -               id = -ENOMEM;
> >> -       }
> >> -
> >> -       return id;
> >> -}
> >> -EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> >> -
> >> -void dwc3_put_device_id(int id)
> >> -{
> >> -       int                     ret;
> >> -
> >> -       if (id < 0)
> >> -               return;
> >> -
> >> -       ret = test_bit(id, dwc3_devs);
> >> -       WARN(!ret, "dwc3: ID %d not in use\n", id);
> >> -       smp_mb__before_clear_bit();
> >> -       clear_bit(id, dwc3_devs);
> >> -}
> >> -EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> >> -
> >> -void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> >> +static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
> >>   {
> >> -       u32 reg;
> >> +       struct dwc3_hwparams    *parms = &dwc->hwparams;
> >>
> >> -       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> >> -       reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> >> -       reg |= DWC3_GCTL_PRTCAPDIR(mode);
> >> -       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> >> +       parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> >> +       parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> >> +       parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> >> +       parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> >> +       parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> >> +       parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> >> +       parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> >> +       parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> >> +       parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
> >>   }
> >> -
> >>   /**
> >>    * dwc3_core_soft_reset - Issues core soft reset and PHY reset
> >>    * @dwc: pointer to our context structure
> >> @@ -160,6 +129,102 @@ static void dwc3_core_soft_reset(struct dwc3 *dwc)
> >>         dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> >>   }
> >>
> >> +static int dwc3_core_reset(struct dwc3 *dwc)
> >> +{
> >> +       unsigned long   timeout;
> >> +       u32     reg;
> >> +
> >> +       /* issue device SoftReset too */
> >> +       timeout = jiffies + msecs_to_jiffies(500);
> >> +       dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> >> +       do {
> >> +               reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> >> +               if (!(reg & DWC3_DCTL_CSFTRST))
> >> +                       break;
> >> +
> >> +               if (time_after(jiffies, timeout)) {
> >> +                       dev_err(dwc->dev, "Reset Timed Out\n");
> >> +                       return -ETIMEDOUT;
> >> +               }
> >> +
> >> +               cpu_relax();
> >> +       } while (true);
> >> +
> >> +       dwc3_core_soft_reset(dwc);
> >> +
> >> +       dwc3_cache_hwparams(dwc);
> >> +
> >> +       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> >> +       reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> >> +       reg &= ~DWC3_GCTL_DISSCRAMBLE;
> >> +
> >> +       switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> >> +       case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> >> +               reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> >> +               break;
> >> +       default:
> >> +               dev_dbg(dwc->dev, "No power optimization available\n");
> >> +       }
> >> +
> >> +       /*
> >>
> >> +        * WORKAROUND: DWC3 revisions <1.90a have a bug
> >> +        * where the device can fail to connect at SuperSpeed
> >> +        * and falls back to high-speed mode which causes
> >> +        * the device to enter a Connect/Disconnect loop
> >> +        */
> >> +       if (dwc->revision < DWC3_REVISION_190A)
> >> +               reg |= DWC3_GCTL_U2RSTECN;
> >> +
> >> +       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +int dwc3_get_device_id(void)
> >> +{
> >> +       int             id;
> >> +
> >> +again:
> >> +       id = find_first_zero_bit(dwc3_devs, DWC3_DEVS_POSSIBLE);
> >> +       if (id < DWC3_DEVS_POSSIBLE) {
> >> +               int old;
> >> +
> >> +               old = test_and_set_bit(id, dwc3_devs);
> >> +               if (old)
> >> +                       goto again;
> >> +       } else {
> >> +               pr_err("dwc3: no space for new device\n");
> >> +               id = -ENOMEM;
> >> +       }
> >> +
> >> +       return id;
> >> +}
> >> +EXPORT_SYMBOL_GPL(dwc3_get_device_id);
> >> +
> >> +void dwc3_put_device_id(int id)
> >> +{
> >> +       int                     ret;
> >> +
> >> +       if (id < 0)
> >> +               return;
> >> +
> >> +       ret = test_bit(id, dwc3_devs);
> >> +       WARN(!ret, "dwc3: ID %d not in use\n", id);
> >> +       smp_mb__before_clear_bit();
> >> +       clear_bit(id, dwc3_devs);
> >> +}
> >> +EXPORT_SYMBOL_GPL(dwc3_put_device_id);
> >> +
> >> +void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> >> +{
> >> +       u32 reg;
> >> +
> >> +       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> >> +       reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
> >> +       reg |= DWC3_GCTL_PRTCAPDIR(mode);
> >> +       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> >> +}
> >> +
> >>   /**
> >>    * dwc3_free_one_event_buffer - Frees one event buffer
> >>    * @dwc: Pointer to our controller context structure
> >> @@ -303,20 +368,7 @@ static void dwc3_event_buffers_cleanup(struct dwc3
> >> *dwc)
> >>         }
> >>   }
> >>
> >> -static void __devinit dwc3_cache_hwparams(struct dwc3 *dwc)
> >> -{
> >> -       struct dwc3_hwparams    *parms = &dwc->hwparams;
> >>
> >> -       parms->hwparams0 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS0);
> >> -       parms->hwparams1 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS1);
> >> -       parms->hwparams2 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS2);
> >> -       parms->hwparams3 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS3);
> >> -       parms->hwparams4 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS4);
> >> -       parms->hwparams5 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS5);
> >> -       parms->hwparams6 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS6);
> >> -       parms->hwparams7 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS7);
> >> -       parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
> >> -}
> >>
> >>   /**
> >>    * dwc3_core_init - Low-level initialization of DWC3 Core
> >> @@ -326,7 +378,6 @@ static void __devinit dwc3_cache_hwparams(struct dwc3
> >> *dwc)
> >>    */
> >>   static int __devinit dwc3_core_init(struct dwc3 *dwc)
> >>   {
> >> -       unsigned long           timeout;
> >>         u32                     reg;
> >>         int                     ret;
> >>
> >> @@ -339,49 +390,9 @@ static int __devinit dwc3_core_init(struct dwc3 *dwc)
> >>         }
> >>         dwc->revision = reg;
> >>
> >> -       /* issue device SoftReset too */
> >> -       timeout = jiffies + msecs_to_jiffies(500);
> >> -       dwc3_writel(dwc->regs, DWC3_DCTL, DWC3_DCTL_CSFTRST);
> >> -       do {
> >> -               reg = dwc3_readl(dwc->regs, DWC3_DCTL);
> >> -               if (!(reg & DWC3_DCTL_CSFTRST))
> >> -                       break;
> >> -
> >> -               if (time_after(jiffies, timeout)) {
> >> -                       dev_err(dwc->dev, "Reset Timed Out\n");
> >> -                       ret = -ETIMEDOUT;
> >> -                       goto err0;
> >> -               }
> >> -
> >> -               cpu_relax();
> >> -       } while (true);
> >> -
> >> -       dwc3_core_soft_reset(dwc);
> >> -
> >> -       dwc3_cache_hwparams(dwc);
> >> -
> >> -       reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> >> -       reg &= ~DWC3_GCTL_SCALEDOWN_MASK;
> >> -       reg &= ~DWC3_GCTL_DISSCRAMBLE;
> >> -
> >> -       switch (DWC3_GHWPARAMS1_EN_PWROPT(dwc->hwparams.hwparams1)) {
> >> -       case DWC3_GHWPARAMS1_EN_PWROPT_CLK:
> >> -               reg &= ~DWC3_GCTL_DSBLCLKGTNG;
> >> -               break;
> >> -       default:
> >> -               dev_dbg(dwc->dev, "No power optimization available\n");
> >> -       }
> >> -
> >> -       /*
> >> -        * WORKAROUND: DWC3 revisions <1.90a have a bug
> >> -        * where the device can fail to connect at SuperSpeed
> >> -        * and falls back to high-speed mode which causes
> >> -        * the device to enter a Connect/Disconnect loop
> >> -        */
> >> -       if (dwc->revision < DWC3_REVISION_190A)
> >> -               reg |= DWC3_GCTL_U2RSTECN;
> >> -
> >> -       dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> >> +       ret = dwc3_core_reset(dwc);
> >> +       if (ret < 0)
> >> +               goto err0;
> >>
> >>         ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
> >>         if (ret) {
> >>
> >> @@ -622,11 +633,70 @@ static int __devexit dwc3_remove(struct
> >> platform_device *pdev)
> >>         return 0;
> >>   }
> >>
> >> +#ifdef CONFIG_PM
> >> +static int dwc3_core_resume(struct device *dev)
> >> +{
> >> +       struct dwc3     *dwc;
> >> +       int     ret;
> >> +
> >> +       dwc = dev_get_drvdata(dev);
> >> +       if (!dwc)
> >> +               return -EINVAL;
> >> +
> >> +       ret = dwc3_core_reset(dwc);
> >
> >
> > Do you have to add core_reset because of any issues you observed? In the
> > ideal working case, you shouldn't have to do reset every-time on resume.
> >
> Initially we tested on kernel 3.4 with which we had some issue in detecting
> USB devices on SS hub. But now as you pointed out, we tested after removing
> core_reset  during resume, and it seems to be working fine.
> Thanks for pointing out, we shall re-confirm and update the new patch-set soon.
> 
> > Maybe I have missed your mail so I'm asking Felipe's question again. how has
> > this patch series been tested?
> >
> We tested the patch series on 'usb-next' on greg's tree after applying
> our local patches
> for exynos5 USB3.0 PHY support and also reverting the patch for dwc3-core
> "usb: dwc3: add basic PHY support" since our USB3.0 PHY driver is underway.

Yes, the idea of that patch was to simply add basic PHY support to dwc3,
once PHY drivers where actually available, those should be removed and
moved over to use the actual PHY driver.

-- 
balbi

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

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

[Vikas C Sajjan]

From: Vikas Sajjan <vikas.sajjan@linaro.org>

Adding the suspend and resume functionality for the XHCI driver

Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
CC: Doug Anderson <dianders@chromium.org>
---
 drivers/usb/host/xhci-plat.c |   44 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

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

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

[Sarah Sharp]

On Wed, Oct 10, 2012 at 07:35:48PM +0530, Vikas C Sajjan wrote:
> From: Vikas Sajjan <vikas.sajjan@linaro.org>
> 
> Adding the suspend and resume functionality for the XHCI driver
> 
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
> CC: Doug Anderson <dianders@chromium.org>

Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>

Felipe, you want to take this?

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

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

[Felipe Balbi]

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

On Wed, Oct 10, 2012 at 10:58:16AM -0700, Sarah Sharp wrote:
> On Wed, Oct 10, 2012 at 07:35:48PM +0530, Vikas C Sajjan wrote:
> > From: Vikas Sajjan <vikas.sajjan@linaro.org>
> > 
> > Adding the suspend and resume functionality for the XHCI driver
> > 
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
> > CC: Doug Anderson <dianders@chromium.org>
> 
> Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
> 
> Felipe, you want to take this?

I can, will get to these patches in a while.

-- 
balbi

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

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

[Felipe Balbi]

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

On Wed, Oct 10, 2012 at 07:35:48PM +0530, Vikas C Sajjan wrote:
> From: Vikas Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> 
> Adding the suspend and resume functionality for the XHCI driver
> 
> Signed-off-by: Abhilash Kesavan <a.kesavan-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> CC: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  drivers/usb/host/xhci-plat.c |   44 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index df90fe5..384cf4d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -185,11 +185,55 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int xhci_plat_suspend(struct device *dev)
> +{
> +	struct usb_hcd		*hcd;
> +	struct xhci_hcd		*xhci;
> +
> +	hcd = dev_get_drvdata(dev);

make it a single line, together with declaration.

> +	if (!hcd)
> +		return -EINVAL;

remove this.

> +	xhci = hcd_to_xhci(hcd);

make it a single line, together with declaration.

> +	/* Make sure that the HCD Core as set state HC_STATE_SUSPENDED */

HCD Core *HAS* set ??? I think this also deserves a "to" after "state":

	/* Make sure HCD Core has set state to HC_STATE_SUSPENDED */

> +	if (hcd->state != HC_STATE_SUSPENDED ||
> +		xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> +		return -EINVAL;
> +
> +	return xhci_suspend(xhci);
> +}
> +
> +static int xhci_plat_resume(struct device *dev)
> +{
> +	struct usb_hcd		*hcd;
> +	struct xhci_hcd		*xhci;
> +
> +	hcd = dev_get_drvdata(dev);

make it a single line, together with declaration.


> +	if (!hcd)
> +		return -EINVAL;

remove this.

> +
> +	xhci = hcd_to_xhci(hcd);

make it a single line, together with declaration.

> +	return xhci_resume(xhci, 0);
> +}

this should look like:

static int xhci_plat_resume(struct device *dev)
{
	struct usb_hcd		*hcd = dev_get_drvdata(dev);
	struct xhci_hcd		*xhci = hcd_to_xhci(hcd);

	return xhci_resume(xhci, 0);
}

> +static const struct dev_pm_ops xhci_plat_pm_ops = {
> +	.suspend		= xhci_plat_suspend,
> +	.resume			= xhci_plat_resume,
> +};

#define XHCI_PLAT_PM_OPS	(&xhci_plat_pm_ops)
#else
#define XHCI_PLAT_PM_OPS	NULL
#endif

> +#endif
> +
>  static struct platform_driver usb_xhci_driver = {
>  	.probe	= xhci_plat_probe,
>  	.remove	= xhci_plat_remove,
>  	.driver	= {
>  		.name = "xhci-hcd",
> +#ifdef CONFIG_PM
> +		.pm = &xhci_plat_pm_ops,
> +#endif

		.pm = XHCI_PLAT_PM_OPS,

-- 
balbi

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

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

[Felipe Balbi]

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

Hi,

On Thu, Oct 11, 2012 at 02:22:42PM +0530, Vikas Sajjan wrote:
> Hi Felipe,
> 
> Thanks for the comments, will get back with modifications.

cool, but please avoid top posting ;-)

-- 
balbi

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

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

[Vikas C Sajjan]

From: Vikas Sajjan <vikas.sajjan-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Adding the suspend and resume functionality to exynos dwc3 driver

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

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index ca65978..1154cee 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
+#include <linux/pm_runtime.h>
 #include <linux/platform_device.h>
 #include <linux/platform_data/dwc3-exynos.h>
 #include <linux/dma-mapping.h>
@@ -200,11 +201,70 @@ static int __devexit dwc3_exynos_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int dwc3_exynos_suspend(struct device *dev)
+{
+	struct dwc3_exynos_data	*pdata = dev->platform_data;
+	struct dwc3_exynos	*exynos;
+	struct platform_device *pdev = to_platform_device(dev);
+
+	exynos = dev_get_drvdata(dev);
+
+	if (!exynos)
+		return -EINVAL;
+
+	if (pdata && pdata->phy_exit)
+		pdata->phy_exit(pdev, pdata->phy_type);
+
+	clk_disable(exynos->clk);
+
+	return 0;
+}
+
+static int dwc3_exynos_resume(struct device *dev)
+{
+	struct dwc3_exynos_data	*pdata = dev->platform_data;
+	struct dwc3_exynos	*exynos;
+	struct platform_device *pdev = to_platform_device(dev);
+
+	exynos = dev_get_drvdata(dev);
+
+	if (!exynos)
+		return -EINVAL;
+
+	clk_enable(exynos->clk);
+
+	/* PHY initialization */
+	if (!pdata) {
+		dev_dbg(&pdev->dev, "missing platform data\n");
+	} else {
+		if (pdata->phy_init)
+			pdata->phy_init(pdev, pdata->phy_type);
+	}
+
+	/* runtime set active to reflect active state. */
+	pm_runtime_disable(dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
+	return 0;
+}
+
+static const struct dev_pm_ops dwc3_exynos_pm_ops = {
+	.suspend		= dwc3_exynos_suspend,
+	.resume			= dwc3_exynos_resume,
+};
+#endif /* CONFIG_PM */
+
 static struct platform_driver dwc3_exynos_driver = {
 	.probe		= dwc3_exynos_probe,
 	.remove		= __devexit_p(dwc3_exynos_remove),
 	.driver		= {
 		.name	= "exynos-dwc3",
+#ifdef CONFIG_PM
+		.pm = &dwc3_exynos_pm_ops,
+#endif
+
 	},
 };
 
-- 
1.7.6.5

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

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

[Felipe Balbi]

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

Hi,

On Wed, Oct 10, 2012 at 07:35:49PM +0530, Vikas C Sajjan wrote:
> From: Vikas Sajjan <vikas.sajjan@linaro.org>
> 
> Adding the suspend and resume functionality to exynos dwc3 driver
> 
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/usb/dwc3/dwc3-exynos.c |   60 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 60 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
> index ca65978..1154cee 100644
> --- a/drivers/usb/dwc3/dwc3-exynos.c
> +++ b/drivers/usb/dwc3/dwc3-exynos.c
> @@ -15,6 +15,7 @@
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/dwc3-exynos.h>
>  #include <linux/dma-mapping.h>
> @@ -200,11 +201,70 @@ static int __devexit dwc3_exynos_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int dwc3_exynos_suspend(struct device *dev)
> +{
> +	struct dwc3_exynos_data	*pdata = dev->platform_data;
> +	struct dwc3_exynos	*exynos;
> +	struct platform_device *pdev = to_platform_device(dev);

you shouldn't have to access platform device here.

> +
> +	exynos = dev_get_drvdata(dev);
> +
> +	if (!exynos)
> +		return -EINVAL;

this shouldn't ever be the case and if it is, it deserves the Oops.

> +
> +	if (pdata && pdata->phy_exit)
> +		pdata->phy_exit(pdev, pdata->phy_type);

NAK, you should introduce a proper PHY driver and handle PHY suspend
generically in dwc3/core.c

> +	clk_disable(exynos->clk);
> +
> +	return 0;
> +}
> +
> +static int dwc3_exynos_resume(struct device *dev)
> +{
> +	struct dwc3_exynos_data	*pdata = dev->platform_data;
> +	struct dwc3_exynos	*exynos;
> +	struct platform_device *pdev = to_platform_device(dev);

ditto

> +	exynos = dev_get_drvdata(dev);
> +
> +	if (!exynos)
> +		return -EINVAL;

ditto

> +	clk_enable(exynos->clk);
> +
> +	/* PHY initialization */
> +	if (!pdata) {
> +		dev_dbg(&pdev->dev, "missing platform data\n");
> +	} else {
> +		if (pdata->phy_init)
> +			pdata->phy_init(pdev, pdata->phy_type);
> +	}

missing PHY driver

> +	/* runtime set active to reflect active state. */
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops dwc3_exynos_pm_ops = {
> +	.suspend		= dwc3_exynos_suspend,
> +	.resume			= dwc3_exynos_resume,
> +};

right here you should:

#define DWC3_EXYNOS_DEV_PM_OPS	&(dwc3_exynos_pm_ops)
#else
#define DWC3_EXYNOS_DEV_PM_OPS	NULL

> +#endif /* CONFIG_PM */
> +
>  static struct platform_driver dwc3_exynos_driver = {
>  	.probe		= dwc3_exynos_probe,
>  	.remove		= __devexit_p(dwc3_exynos_remove),
>  	.driver		= {
>  		.name	= "exynos-dwc3",
> +#ifdef CONFIG_PM
> +		.pm = &dwc3_exynos_pm_ops,

		.pm = DWC3_EXYNOS_DEV_PM_OPS,

> +#endif

avoid the ifdeferry all over, see above.

-- 
balbi

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

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

[Felipe Balbi]

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

On Wed, Oct 10, 2012 at 07:35:46PM +0530, Vikas C Sajjan wrote:
> From: Vikas Sajjan <vikas.sajjan@linaro.org>
> 
> This patchset adds suspend/resume functionality to dwc3-core layer
> and xhci-platform driver. It also adds S2R support to dwc3-exynos
> glue layer.

how has this been tested ? For how long ?

> Based on 'usb-next' branch.
> 
> Vikas Sajjan (3):
>   usb: dwc3: Add the suspend/resume functionality
>   usb: xhci: Add the suspend/resume functionality
>   exynos5: usb: dwc3: Add the suspend/resume functionality
> 
>  drivers/usb/dwc3/core.c        |  268 +++++++++++++++++++++++++---------------
>  drivers/usb/dwc3/dwc3-exynos.c |   60 +++++++++
>  drivers/usb/host/xhci-plat.c   |   44 +++++++
>  3 files changed, 273 insertions(+), 99 deletions(-)
> 
> -- 
> 1.7.6.5
> 

-- 
balbi

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