usb: xhci: plat: Enable runtime PM
Subsystems (2)
v6 cover usb: xhci: plat: Enable runtime PM
From: Robert Foss <[email protected]>
This series enables runtime PM and asynchronous resume/suspend support for xhci-plat devices.
Changes since v1:
- Added Signed-off-by: Robert Foss <[email protected]>
- 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(-)
Signature · 1 line
2.7.4
PATCH v6 1/2 usb: xhci: plat: Enable runtime PM
From: Robert Foss <[email protected]>
Enable runtime PM for the xhci-plat device so that the parent device may implement runtime PM.
--- drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)
drivers/usb/host/xhci-plat.c +27 -2
@@ -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 = {
Signature · 1 line
2.7.4
+ 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.
Quoted 17 lines · On Wed, Aug 10, 2016 at 04:32:15PM -0400, [email protected] wrote:
From: Robert Foss <[email protected]>
Enable runtime PM for the xhci-plat device so that the parent device may implement runtime PM.
Signed-off-by Robert Foss <[email protected]>Tested-by Robert Foss <[email protected]>--- drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)drivers/usb/host/xhci-plat.c +0
-0
drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 251
Quoted 6 lines
drivers/usb/host/xhci-plat.c +3
-0@@ -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:
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/
Quoted 57 lines
drivers/usb/host/xhci-plat.c +24
-2return 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 = {
Quoted 28 lines · Brian Norris
+ 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.
Quoted 20 lines · On Wed, Aug 10, 2016 at 04:32:15PM -0400, [email protected] wrote:
From: Robert Foss <[email protected]>
Enable runtime PM for the xhci-plat device so that the parent device may implement runtime PM.
Signed-off-by Robert Foss <[email protected]>Tested-by Robert Foss <[email protected]>--- drivers/usb/host/xhci-plat.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)drivers/usb/host/xhci-plat.c +0
-0@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)if (ret) goto dealloc_usb2_hcd;
Quoted 6 lines
drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 251
Quoted 3 lines
drivers/usb/host/xhci-plat.c +3
-0+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?
Quoted 86 lines
I suspect that this patch set was derived from the Chromium OS kernel tree, where we were supporting a Tegra XHCI chipset:
It looks like the driver was refactored to not use
xhci-plat.cbefore 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.cto avoid usingxhci-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/Quoted 57 lines
drivers/usb/host/xhci-plat.c +24
-2return 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 = {
Hi,
Quoted 36 lines · Robert Foss
Quoted 28 lines · Brian Norris
+ 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.
Quoted 20 lines · On Wed, Aug 10, 2016 at 04:32:15PM -0400, [email protected] wrote:
From: Robert Foss <[email protected]>
Enable runtime PM for the xhci-plat device so that the parent device may implement runtime PM.
Signed-off-by Robert Foss <[email protected]>Tested-by Robert Foss <[email protected]>---
drivers/usb/host/xhci-plat.c| 29 +++++++++++++++++++++++++++--1 file changed, 27 insertions(+), 2 deletions(-)drivers/usb/host/xhci-plat.c +0
-0@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)if (ret) goto dealloc_usb2_hcd;Quoted 6 lines
drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 251
Quoted 3 lines
drivers/usb/host/xhci-plat.c +3
-0+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?
Quoted 2 lines
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
Corrected a bouncing email address (sorry for the noise)
Quoted 59 lines · Brian Norris
Hi,
Quoted 36 lines · Robert Foss
Quoted 28 lines · Brian Norris
+ 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.
Quoted 20 lines · On Wed, Aug 10, 2016 at 04:32:15PM -0400, [email protected] wrote:
From: Robert Foss <[email protected]>
Enable runtime PM for the xhci-plat device so that the parent device may implement runtime PM.
Signed-off-by Robert Foss <[email protected]>Tested-by Robert Foss <[email protected]>---
drivers/usb/host/xhci-plat.c| 29 +++++++++++++++++++++++++++--1 file changed, 27 insertions(+), 2 deletions(-)drivers/usb/host/xhci-plat.c +0
-0@@ -246,6 +246,9 @@ static int xhci_plat_probe(struct platform_device *pdev)if (ret) goto dealloc_usb2_hcd;Quoted 6 lines
drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 251
Quoted 3 lines
drivers/usb/host/xhci-plat.c +3
-0+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?
Quoted 2 lines
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
PATCH v6 2/2 usb: xhci: plat: Enable async suspend/resume
In reply to [email protected]From: Andrew Bresticker <[email protected]>
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.
--- drivers/usb/host/xhci-plat.c | 1 + 1 file changed, 1 insertion(+)
drivers/usb/host/xhci-plat.c +1 -0
@@ -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 ;
Signature · 1 line
2.7.4