Re: [PATCH 1/4] usb: host: ohci-platform: Add basic runtime PM support
From: Alan Stern <hidden>
Date: 2017-05-18 15:25:07
Also in:
linux-omap
On Wed, 17 May 2017, Tony Lindgren wrote:
This is needed in preparation of adding support for omap3 and later OHCI. The runtime PM will only do something on platforms that implement it.
This patch isn't correct. See below.
quoted hunk
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: Hans de Goede <redacted> Cc: Rob Herring <redacted> Cc: Roger Quadros <redacted> Cc: Yoshihiro Shimoda <redacted> Signed-off-by: Tony Lindgren <redacted> --- drivers/usb/host/ohci-platform.c | 11 +++++++++++ 1 file changed, 11 insertions(+)diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c --- a/drivers/usb/host/ohci-platform.c +++ b/drivers/usb/host/ohci-platform.c@@ -24,6 +24,7 @@ #include <linux/err.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> +#include <linux/pm_runtime.h> #include <linux/reset.h> #include <linux/usb/ohci_pdriver.h> #include <linux/usb.h>@@ -51,6 +52,10 @@ static int ohci_platform_power_on(struct platform_device *dev) struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd); int clk, ret, phy_num; + ret = pm_runtime_get_sync(&dev->dev); + if (ret < 0) + return ret; + for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++) { ret = clk_prepare_enable(priv->clks[clk]); if (ret)@@ -96,6 +101,8 @@ static void ohci_platform_power_off(struct platform_device *dev) for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--) if (priv->clks[clk]) clk_disable_unprepare(priv->clks[clk]); + + pm_runtime_put_sync(&dev->dev); }
ohci_platform_power_on() is invoked (by default) by the runtime-resume callback routine ohci_platform_resume(), through the pdata->power_on method pointer. Likewise, ohci_platform_power_off() is invoked by the runtime-suspend callback routine. This means you shouldn't do pm_runtime_get/put calls from within these routines. Is there any particular reason you wanted to add these calls? In general, USB host controllers are expected to go into runtime suspend whenever there aren't any children keeping them awake. Hence they usually don't need to worry about initiating their own suspends and resumes.
quoted hunk
static struct hc_driver __read_mostly ohci_platform_hc_driver;@@ -242,6 +249,7 @@ static int ohci_platform_probe(struct platform_device *dev) } #endif + pm_runtime_enable(&dev->dev);
There should be a pm_runtime_set_active() just before this.
quoted hunk
if (pdata->power_on) { err = pdata->power_on(dev); if (err < 0)@@ -271,6 +279,7 @@ static int ohci_platform_probe(struct platform_device *dev) if (pdata->power_off) pdata->power_off(dev); err_reset: + pm_runtime_disable(&dev->dev); while (--rst >= 0) reset_control_assert(priv->resets[rst]); err_put_clks:@@ -305,6 +314,8 @@ static int ohci_platform_remove(struct platform_device *dev) usb_put_hcd(hcd); + pm_runtime_disable(&dev->dev); + if (pdata == &ohci_platform_defaults) dev->dev.platform_data = NULL;
These changes make sense. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html