+ 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@xxxxxxxxxxxxx wrote:
> From: Robert Foss <robert.foss@xxxxxxxxxxxxx>
>
> Enable runtime PM for the xhci-plat device so that the parent device
> may implement runtime PM.
>
> Signed-off-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
>
> Tested-by: Robert Foss <robert.foss@xxxxxxxxxxxxx>
> ---
> 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 = {
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
