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

Message ID	20121016095932.GA17416@arwen.pp.htv.fi (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-omap-owner@vger.kernel.org> Date: Tue, 16 Oct 2012 12:59:33 +0300 From: Felipe Balbi <balbi@ti.com> To: Vikas Sajjan <vikas.sajjan@linaro.org> CC: <linux-usb@vger.kernel.org>, <linux-omap@vger.kernel.org>, <gregkh@linuxfoundation.org>, <sarah.a.sharp@linux.intel.com>, <balbi@ti.com>, <joshi@samsung.com>, <kishon@ti.com>, Abhilash Kesavan <a.kesavan@samsung.com>, Doug Anderson <dianders@chromium.org> Subject: Re: [PATCH v3 2/3] usb: xhci: Add the suspend/resume functionality Message-ID: <20121016095932.GA17416@arwen.pp.htv.fi> Reply-To: <balbi@ti.com> References: <1350380738-32473-1-git-send-email-vikas.sajjan@linaro.org> <1350380738-32473-3-git-send-email-vikas.sajjan@linaro.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="sdtB3X0nJg68CQEu" Content-Disposition: inline In-Reply-To: <1350380738-32473-3-git-send-email-vikas.sajjan@linaro.org> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-omap-owner@vger.kernel.org Precedence: bulk

Message ID

20121016095932.GA17416@arwen.pp.htv.fi (mailing list archive)

State

New, archived

Headers

Date: Tue, 16 Oct 2012 12:59:33 +0300
From: Felipe Balbi <balbi@ti.com>
To: Vikas Sajjan <vikas.sajjan@linaro.org>
CC: <linux-usb@vger.kernel.org>, <linux-omap@vger.kernel.org>,
	<gregkh@linuxfoundation.org>, <sarah.a.sharp@linux.intel.com>,
	<balbi@ti.com>, <joshi@samsung.com>, <kishon@ti.com>,
	Abhilash Kesavan <a.kesavan@samsung.com>,
	Doug Anderson <dianders@chromium.org>
Subject: Re: [PATCH v3 2/3] usb: xhci: Add the suspend/resume functionality
Message-ID: <20121016095932.GA17416@arwen.pp.htv.fi>
Reply-To: <balbi@ti.com>
References: <1350380738-32473-1-git-send-email-vikas.sajjan@linaro.org>
	<1350380738-32473-3-git-send-email-vikas.sajjan@linaro.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
	protocol="application/pgp-signature"; boundary="sdtB3X0nJg68CQEu"
Content-Disposition: inline
In-Reply-To: <1350380738-32473-3-git-send-email-vikas.sajjan@linaro.org>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-omap-owner@vger.kernel.org
Precedence: bulk

Commit Message

Felipe Balbi Oct. 16, 2012, 9:59 a.m. UTC

Hi,

On Tue, Oct 16, 2012 at 03:15:37PM +0530, Vikas Sajjan wrote:
> Adds power management support to XHCI platform driver.
> This patch facilitates the transition of xHCI host controller
> between S0 and S3/S4 power states, during suspend/resume cycles.
> 
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Vikas C Sajjan <vikas.sajjan@linaro.org>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/usb/host/xhci-plat.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index df90fe5..eaf3a07 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -185,11 +185,39 @@ static int xhci_plat_remove(struct platform_device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int xhci_plat_suspend(struct device *dev)
> +{
> +	struct usb_hcd	*hcd	= dev_get_drvdata(dev);
> +	struct xhci_hcd	*xhci	= hcd_to_xhci(hcd);
> +
> +	/* Make sure that the HCD Core has set state to HC_STATE_SUSPENDED */
> +	if (hcd->state != HC_STATE_SUSPENDED ||
> +		xhci->shared_hcd->state != HC_STATE_SUSPENDED)
> +		return -EINVAL;
> +
> +	return xhci_suspend(xhci);

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

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

[ STILL NEED TO WRITE A PROPER COMMIT LOG ]

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


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

Comments

Sarah Sharp Oct. 18, 2012, 2:59 p.m. UTC | #1

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

Yes, that patch looks fine.

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

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

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

Felipe Balbi Oct. 18, 2012, 5:30 p.m. UTC | #2

Hi,

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

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

cheers

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

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

Commit Message

Comments

Patch