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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Wed, Oct 10, 2012 at 07:35:47PM +0530, Vikas C Sajjan wrote:
> From: Vikas Sajjan <vikas.sajjan@xxxxxxxxxx>
> 
> Adding the suspend and resume funtionality to DWC3 core.
> 
> Signed-off-by: Abhilash Kesavan <a.kesavan@xxxxxxxxxxx>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan@xxxxxxxxxx>
> CC: Doug Anderson <dianders@xxxxxxxxxxxx>
> ---
>  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: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux