* [PACTH v6 0/2] usb: xhci: plat: Enable runtime PM @ 2016-08-10 20:32 robert.foss 2016-08-10 20:32 ` [PACTH v6 1/2] " robert.foss 2016-08-10 20:32 ` [PACTH v6 2/2] usb: xhci: plat: Enable async suspend/resume robert.foss 0 siblings, 2 replies; 7+ messages in thread From: robert.foss @ 2016-08-10 20:32 UTC (permalink / raw) To: mathias.nyman, gregkh, linux-usb, linux-kernel, Julius Werner, Andrew Bresticker, Felipe Balbi Cc: Robert Foss From: Robert Foss <robert.foss@collabora.com> This series enables runtime PM and asynchronous resume/suspend support for xhci-plat devices. Changes since v1: - Added Signed-off-by: Robert Foss <robert.foss@collabora.com> - Added proper metadata tags to series Changes since v2: - Added missing changelog to cover-letter - Added error checking to pm_runtime_get_sync() calls Changes since v3: - Decrement usage_counter for failed pm_runtime_get*() calls Changes since v4: - Added missing brackets Changes since v5: - Switched out atomic_dec() calls with pm_runtime_put() calls Thanks Filipe for having a look at all of these revisions, I appreciate it! Andrew Bresticker (1): usb: xhci: plat: Enable async suspend/resume Robert Foss (1): usb: xhci: plat: Enable runtime PM drivers/usb/host/xhci-plat.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PACTH v6 1/2] usb: xhci: plat: Enable runtime PM 2016-08-10 20:32 [PACTH v6 0/2] usb: xhci: plat: Enable runtime PM robert.foss @ 2016-08-10 20:32 ` robert.foss 2016-08-23 3:23 ` [PACTH,v6,1/2] " Brian Norris 2016-08-10 20:32 ` [PACTH v6 2/2] usb: xhci: plat: Enable async suspend/resume robert.foss 1 sibling, 1 reply; 7+ messages in thread From: robert.foss @ 2016-08-10 20:32 UTC (permalink / raw) To: mathias.nyman, gregkh, linux-usb, linux-kernel, Julius Werner, Andrew Bresticker, Felipe Balbi Cc: Robert Foss From: Robert Foss <robert.foss@collabora.com> Enable runtime PM for the xhci-plat device so that the parent device may implement runtime PM. Signed-off-by: Robert Foss <robert.foss@collabora.com> Tested-by: Robert Foss <robert.foss@collabora.com> --- drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index ed56bf9..ba4efe7 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev) if (ret) goto dealloc_usb2_hcd; + pm_runtime_set_active(&pdev->dev); + pm_runtime_enable(&pdev->dev); + return 0; @@ -274,6 +277,8 @@ static int xhci_plat_remove(struct platform_device *dev) struct xhci_hcd *xhci = hcd_to_xhci(hcd); struct clk *clk = xhci->clk; + pm_runtime_disable(&dev->dev); + usb_remove_hcd(xhci->shared_hcd); usb_phy_shutdown(hcd->usb_phy); @@ -292,6 +297,13 @@ static int xhci_plat_suspend(struct device *dev) { struct usb_hcd *hcd = dev_get_drvdata(dev); struct xhci_hcd *xhci = hcd_to_xhci(hcd); + int ret; + + ret = pm_runtime_get_sync(dev); + if (ret < 0) { + pm_runtime_put(dev); + return ret; + } /* * xhci_suspend() needs `do_wakeup` to know whether host is allowed @@ -301,15 +313,28 @@ static int xhci_plat_suspend(struct device *dev) * reconsider this when xhci_plat_suspend enlarges its scope, e.g., * also applies to runtime suspend. */ - return xhci_suspend(xhci, device_may_wakeup(dev)); + ret = xhci_suspend(xhci, device_may_wakeup(dev)); + pm_runtime_put(dev); + + return ret; } static int xhci_plat_resume(struct device *dev) { struct usb_hcd *hcd = dev_get_drvdata(dev); struct xhci_hcd *xhci = hcd_to_xhci(hcd); + int ret; - return xhci_resume(xhci, 0); + ret = pm_runtime_get_sync(dev); + if (ret < 0) { + pm_runtime_put(dev); + return ret; + } + + ret = xhci_resume(xhci, 0); + pm_runtime_put(dev); + + return ret; } static const struct dev_pm_ops xhci_plat_pm_ops = { -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM 2016-08-10 20:32 ` [PACTH v6 1/2] " robert.foss @ 2016-08-23 3:23 ` Brian Norris 2016-08-24 20:48 ` Robert Foss 0 siblings, 1 reply; 7+ messages in thread From: Brian Norris @ 2016-08-23 3:23 UTC (permalink / raw) To: robert.foss Cc: mathias.nyman, gregkh, linux-usb, linux-kernel, Julius Werner, Andrew Bresticker, Felipe Balbi, Baolin Wang, zyx, wulf + others Hi Robert and Felipe, I have a few questions for one or both of you. I'm not really an expert on runtime PM, so please take my questions with a grain of salt. On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote: > From: Robert Foss <robert.foss@collabora.com> > > Enable runtime PM for the xhci-plat device so that the parent device > may implement runtime PM. > > Signed-off-by: Robert Foss <robert.foss@collabora.com> > > Tested-by: Robert Foss <robert.foss@collabora.com> > --- > drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > index ed56bf9..ba4efe7 100644 > --- a/drivers/usb/host/xhci-plat.c > +++ b/drivers/usb/host/xhci-plat.c > @@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev) > if (ret) > goto dealloc_usb2_hcd; > > + pm_runtime_set_active(&pdev->dev); > + pm_runtime_enable(&pdev->dev); > + How does it help to enable PM runtime like this, if you don't have any kind of runtime_{suspend,resume}() callbacks? I suspect that this patch set was derived from the Chromium OS kernel tree, where we were supporting a Tegra XHCI chipset: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.10/drivers/usb/host/xhci-tegra.c#1920 It looks like the driver was refactored to not use xhci-plat.c before it was upstreamed (and runtime PM support was dropped along the way). So, I'm wondering how I might actually use this? Particularly, I'm looking at trying out runtime suspend for a DWC3 controller in host mode, and it looks like I'd have to do some layer-violating calls to xhci_suspend()/xhci_resume() from the parent dwc3 device, or else rewrite drivers/usb/dwc3/host.c to avoid using xhci-plat.c. (I also see that Baolin, CC'd here, was interested in dwc3 [1].) Or possibly an enlightening question for me: if you don't mind, how are you utilizing runtime PM in conjunction with xhci-plat.c, Robert? Presumably some other parent device/driver is doing some additional management of the XHCI core? Regards, Brian [1] [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume https://lkml.org/lkml/2016/7/15/181 https://patchwork.kernel.org/patch/9231417/ > return 0; > > > @@ -274,6 +277,8 @@ static int xhci_plat_remove(struct platform_device *dev) > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > struct clk *clk = xhci->clk; > > + pm_runtime_disable(&dev->dev); > + > usb_remove_hcd(xhci->shared_hcd); > usb_phy_shutdown(hcd->usb_phy); > > @@ -292,6 +297,13 @@ static int xhci_plat_suspend(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + int ret; > + > + ret = pm_runtime_get_sync(dev); > + if (ret < 0) { > + pm_runtime_put(dev); > + return ret; > + } > > /* > * xhci_suspend() needs `do_wakeup` to know whether host is allowed > @@ -301,15 +313,28 @@ static int xhci_plat_suspend(struct device *dev) > * reconsider this when xhci_plat_suspend enlarges its scope, e.g., > * also applies to runtime suspend. > */ > - return xhci_suspend(xhci, device_may_wakeup(dev)); > + ret = xhci_suspend(xhci, device_may_wakeup(dev)); > + pm_runtime_put(dev); > + > + return ret; > } > > static int xhci_plat_resume(struct device *dev) > { > struct usb_hcd *hcd = dev_get_drvdata(dev); > struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + int ret; > > - return xhci_resume(xhci, 0); > + ret = pm_runtime_get_sync(dev); > + if (ret < 0) { > + pm_runtime_put(dev); > + return ret; > + } > + > + ret = xhci_resume(xhci, 0); > + pm_runtime_put(dev); > + > + return ret; > } > > static const struct dev_pm_ops xhci_plat_pm_ops = { ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM 2016-08-23 3:23 ` [PACTH,v6,1/2] " Brian Norris @ 2016-08-24 20:48 ` Robert Foss 2016-08-26 22:11 ` Brian Norris 0 siblings, 1 reply; 7+ messages in thread From: Robert Foss @ 2016-08-24 20:48 UTC (permalink / raw) To: Brian Norris Cc: mathias.nyman, gregkh, linux-usb, linux-kernel, Julius Werner, Andrew Bresticker, Felipe Balbi, Baolin Wang, zyx, wulf On 2016-08-22 11:23 PM, Brian Norris wrote: > + others > > Hi Robert and Felipe, > > I have a few questions for one or both of you. I'm not really an expert > on runtime PM, so please take my questions with a grain of salt. > > On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote: >> From: Robert Foss <robert.foss@collabora.com> >> >> Enable runtime PM for the xhci-plat device so that the parent device >> may implement runtime PM. >> >> Signed-off-by: Robert Foss <robert.foss@collabora.com> >> >> Tested-by: Robert Foss <robert.foss@collabora.com> >> --- >> drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- >> 1 file changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c >> index ed56bf9..ba4efe7 100644 >> --- a/drivers/usb/host/xhci-plat.c >> +++ b/drivers/usb/host/xhci-plat.c >> @@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev) >> if (ret) >> goto dealloc_usb2_hcd; >> >> + pm_runtime_set_active(&pdev->dev); >> + pm_runtime_enable(&pdev->dev); >> + > > How does it help to enable PM runtime like this, if you don't have any > kind of runtime_{suspend,resume}() callbacks? Andrew, I think you understand the inner workings of this code better than me, maybe you could give a short summary? > > I suspect that this patch set was derived from the Chromium OS kernel > tree, where we were supporting a Tegra XHCI chipset: > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.10/drivers/usb/host/xhci-tegra.c#1920 > > It looks like the driver was refactored to not use xhci-plat.c before it > was upstreamed (and runtime PM support was dropped along the way). > > So, I'm wondering how I might actually use this? Particularly, I'm > looking at trying out runtime suspend for a DWC3 controller in host > mode, and it looks like I'd have to do some layer-violating calls to > xhci_suspend()/xhci_resume() from the parent dwc3 device, or else > rewrite drivers/usb/dwc3/host.c to avoid using xhci-plat.c. > > (I also see that Baolin, CC'd here, was interested in dwc3 [1].) > > Or possibly an enlightening question for me: if you don't mind, how are > you utilizing runtime PM in conjunction with xhci-plat.c, Robert? > Presumably some other parent device/driver is doing some additional > management of the XHCI core? > > Regards, > Brian > > [1] [PATCH 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume > https://lkml.org/lkml/2016/7/15/181 > https://patchwork.kernel.org/patch/9231417/ > >> return 0; >> >> >> @@ -274,6 +277,8 @@ static int xhci_plat_remove(struct platform_device *dev) >> struct xhci_hcd *xhci = hcd_to_xhci(hcd); >> struct clk *clk = xhci->clk; >> >> + pm_runtime_disable(&dev->dev); >> + >> usb_remove_hcd(xhci->shared_hcd); >> usb_phy_shutdown(hcd->usb_phy); >> >> @@ -292,6 +297,13 @@ static int xhci_plat_suspend(struct device *dev) >> { >> struct usb_hcd *hcd = dev_get_drvdata(dev); >> struct xhci_hcd *xhci = hcd_to_xhci(hcd); >> + int ret; >> + >> + ret = pm_runtime_get_sync(dev); >> + if (ret < 0) { >> + pm_runtime_put(dev); >> + return ret; >> + } >> >> /* >> * xhci_suspend() needs `do_wakeup` to know whether host is allowed >> @@ -301,15 +313,28 @@ static int xhci_plat_suspend(struct device *dev) >> * reconsider this when xhci_plat_suspend enlarges its scope, e.g., >> * also applies to runtime suspend. >> */ >> - return xhci_suspend(xhci, device_may_wakeup(dev)); >> + ret = xhci_suspend(xhci, device_may_wakeup(dev)); >> + pm_runtime_put(dev); >> + >> + return ret; >> } >> >> static int xhci_plat_resume(struct device *dev) >> { >> struct usb_hcd *hcd = dev_get_drvdata(dev); >> struct xhci_hcd *xhci = hcd_to_xhci(hcd); >> + int ret; >> >> - return xhci_resume(xhci, 0); >> + ret = pm_runtime_get_sync(dev); >> + if (ret < 0) { >> + pm_runtime_put(dev); >> + return ret; >> + } >> + >> + ret = xhci_resume(xhci, 0); >> + pm_runtime_put(dev); >> + >> + return ret; >> } >> >> static const struct dev_pm_ops xhci_plat_pm_ops = { ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM 2016-08-24 20:48 ` Robert Foss @ 2016-08-26 22:11 ` Brian Norris 2016-08-26 22:13 ` Brian Norris 0 siblings, 1 reply; 7+ messages in thread From: Brian Norris @ 2016-08-26 22:11 UTC (permalink / raw) To: Robert Foss Cc: mathias.nyman, gregkh, linux-usb, linux-kernel, Julius Werner, Andrew Bresticker, Felipe Balbi, Baolin Wang, zyx, wulf Hi, On Wed, Aug 24, 2016 at 04:48:01PM -0400, Robert Foss wrote: > On 2016-08-22 11:23 PM, Brian Norris wrote: > >+ others > > > >Hi Robert and Felipe, > > > >I have a few questions for one or both of you. I'm not really an expert > >on runtime PM, so please take my questions with a grain of salt. > > > >On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote: > >>From: Robert Foss <robert.foss@collabora.com> > >> > >>Enable runtime PM for the xhci-plat device so that the parent device > >>may implement runtime PM. > >> > >>Signed-off-by: Robert Foss <robert.foss@collabora.com> > >> > >>Tested-by: Robert Foss <robert.foss@collabora.com> > >>--- > >> drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- > >> 1 file changed, 27 insertions(+), 2 deletions(-) > >> > >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > >>index ed56bf9..ba4efe7 100644 > >>--- a/drivers/usb/host/xhci-plat.c > >>+++ b/drivers/usb/host/xhci-plat.c > >>@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev) > >> if (ret) > >> goto dealloc_usb2_hcd; > >> > >>+ pm_runtime_set_active(&pdev->dev); > >>+ pm_runtime_enable(&pdev->dev); > >>+ > > > >How does it help to enable PM runtime like this, if you don't have any > >kind of runtime_{suspend,resume}() callbacks? > > Andrew, I think you understand the inner workings of this code > better than me, maybe you could give a short summary? I believe Andrew is fairly busy, and I already talked with him a bit about this. This is needed (as per your (or his?) commit message) only if you have a parent device that wants to implement runtime PM. Now depending on what you want to do with "runtime PM", this might mean the parent device has to suspend xhci_{suspend,resume}() on behalf of xhci-plat.c. Not very nice layering IMO, but it has been done before... So I guess this comes down to "what does a parent device/driver want to do"? If that's (e.g.) just putting some PHY into a slightly lower power mode, then maybe it's fine for xhci-plat not to do anything else. But if you actually want to completely power down the parent bus, reset an accompanying dual-role/OTG controller, etc., then this really isn't sufficient, AFAICT. But maybe that's just an indictment of the poor structure of dwc3's host-mode support, more than it is an indictment of this patch. Brian ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PACTH,v6,1/2] usb: xhci: plat: Enable runtime PM 2016-08-26 22:11 ` Brian Norris @ 2016-08-26 22:13 ` Brian Norris 0 siblings, 0 replies; 7+ messages in thread From: Brian Norris @ 2016-08-26 22:13 UTC (permalink / raw) To: Robert Foss Cc: mathias.nyman, gregkh, linux-usb, linux-kernel, Julius Werner, Andrew Bresticker, Felipe Balbi, Baolin Wang, zyw, wulf Corrected a bouncing email address (sorry for the noise) On Fri, Aug 26, 2016 at 03:11:36PM -0700, Brian Norris wrote: > Hi, > > On Wed, Aug 24, 2016 at 04:48:01PM -0400, Robert Foss wrote: > > On 2016-08-22 11:23 PM, Brian Norris wrote: > > >+ others > > > > > >Hi Robert and Felipe, > > > > > >I have a few questions for one or both of you. I'm not really an expert > > >on runtime PM, so please take my questions with a grain of salt. > > > > > >On Wed, Aug 10, 2016 at 04:32:15PM -0400, robert.foss@collabora.com wrote: > > >>From: Robert Foss <robert.foss@collabora.com> > > >> > > >>Enable runtime PM for the xhci-plat device so that the parent device > > >>may implement runtime PM. > > >> > > >>Signed-off-by: Robert Foss <robert.foss@collabora.com> > > >> > > >>Tested-by: Robert Foss <robert.foss@collabora.com> > > >>--- > > >> drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- > > >> 1 file changed, 27 insertions(+), 2 deletions(-) > > >> > > >>diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c > > >>index ed56bf9..ba4efe7 100644 > > >>--- a/drivers/usb/host/xhci-plat.c > > >>+++ b/drivers/usb/host/xhci-plat.c > > >>@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev) > > >> if (ret) > > >> goto dealloc_usb2_hcd; > > >> > > >>+ pm_runtime_set_active(&pdev->dev); > > >>+ pm_runtime_enable(&pdev->dev); > > >>+ > > > > > >How does it help to enable PM runtime like this, if you don't have any > > >kind of runtime_{suspend,resume}() callbacks? > > > > Andrew, I think you understand the inner workings of this code > > better than me, maybe you could give a short summary? > > I believe Andrew is fairly busy, and I already talked with him a bit > about this. This is needed (as per your (or his?) commit message) only > if you have a parent device that wants to implement runtime PM. Now > depending on what you want to do with "runtime PM", this might mean > the parent device has to suspend xhci_{suspend,resume}() on behalf of > xhci-plat.c. Not very nice layering IMO, but it has been done before... > > So I guess this comes down to "what does a parent device/driver want to > do"? If that's (e.g.) just putting some PHY into a slightly lower power > mode, then maybe it's fine for xhci-plat not to do anything else. But if > you actually want to completely power down the parent bus, reset an > accompanying dual-role/OTG controller, etc., then this really isn't > sufficient, AFAICT. But maybe that's just an indictment of the poor > structure of dwc3's host-mode support, more than it is an indictment of > this patch. > > Brian ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PACTH v6 2/2] usb: xhci: plat: Enable async suspend/resume 2016-08-10 20:32 [PACTH v6 0/2] usb: xhci: plat: Enable runtime PM robert.foss 2016-08-10 20:32 ` [PACTH v6 1/2] " robert.foss @ 2016-08-10 20:32 ` robert.foss 1 sibling, 0 replies; 7+ messages in thread From: robert.foss @ 2016-08-10 20:32 UTC (permalink / raw) To: mathias.nyman, gregkh, linux-usb, linux-kernel, Julius Werner, Andrew Bresticker, Felipe Balbi Cc: Robert Foss From: Andrew Bresticker <abrestic@chromium.org> USB host controllers can take a significant amount of time to suspend and resume, adding several hundred miliseconds to the kernel resume time. Since the XHCI controller has no outside dependencies (other than clocks, which are suspended late/resumed early), allow it to suspend and resume asynchronously. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> Tested-by: Andrew Bresticker <abrestic@chromium.org> Tested-by: Robert Foss <robert.foss@collabora.com> Signed-off-by: Robert Foss <robert.foss@collabora.com> --- drivers/usb/host/xhci-plat.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index ba4efe7..c35b7fe 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -248,6 +248,7 @@ static int xhci_plat_probe(struct platform_device *pdev) pm_runtime_set_active(&pdev->dev); pm_runtime_enable(&pdev->dev); + device_enable_async_suspend(&pdev->dev); return 0; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-08-26 22:14 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-10 20:32 [PACTH v6 0/2] usb: xhci: plat: Enable runtime PM robert.foss 2016-08-10 20:32 ` [PACTH v6 1/2] " robert.foss 2016-08-23 3:23 ` [PACTH,v6,1/2] " Brian Norris 2016-08-24 20:48 ` Robert Foss 2016-08-26 22:11 ` Brian Norris 2016-08-26 22:13 ` Brian Norris 2016-08-10 20:32 ` [PACTH v6 2/2] usb: xhci: plat: Enable async suspend/resume robert.foss
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).