LinuxLists.cc - [PATCH v2 00/10] usb: dwc3/xhci/phy: Enable runtime power management

[permalink] [raw]

Subject: [PATCH v2 03/10] usb: dwc3: Enable runtime pm only after PHYs are initialized

Allow dwc3 to enable auto power management only after its PHYs
are initialized so that any further PHY handling by dwc3's
runtime power management callbacks is fine.

Signed-off-by: Vivek Gautam <[email protected]>
---
drivers/usb/dwc3/core.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 85914e0..2a77327 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -453,10 +453,6 @@ static int dwc3_probe(struct platform_device *pdev)

dwc->needs_fifo_resize = of_property_read_bool(node, "tx-fifo-resize");

- pm_runtime_enable(dev);
- pm_runtime_get_sync(dev);
- pm_runtime_forbid(dev);
-
dwc3_cache_hwparams(dwc);

ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
@@ -478,6 +474,10 @@ static int dwc3_probe(struct platform_device *pdev)
goto err1;
}

+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+ pm_runtime_forbid(dev);
+
if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
mode = DWC3_MODE_HOST;
else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
--
1.7.6.5

2013-03-02 13:25:31

[permalink] [raw]

Subject: [PATCH v2 05/10] usb: dwc3: exynos: Enable runtime power management

Enabling runtime power management on dwc3-exynos
letting dwc3 controller to be autosuspended on exynos
platform when not in use.

Signed-off-by: Vivek Gautam <[email protected]>
---
drivers/usb/dwc3/dwc3-exynos.c | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index e6771d9..28b5f8a 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -19,6 +19,7 @@
#include <linux/platform_data/dwc3-exynos.h>
#include <linux/dma-mapping.h>
#include <linux/clk.h>
+#include <linux/pm_runtime.h>
#include <linux/usb/otg.h>
#include <linux/usb/nop-usb-xceiv.h>
#include <linux/of.h>
@@ -143,6 +144,10 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
exynos->dev = dev;
exynos->clk = clk;

+ pm_runtime_enable(dev);
+ pm_runtime_get_sync(dev);
+ pm_runtime_forbid(dev);
+
clk_enable(exynos->clk);

ret = platform_device_add_resources(dwc3, pdev->resource,
@@ -158,10 +163,14 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
goto err2;
}

+ pm_runtime_put_sync(dev);
+ pm_runtime_allow(dev);
+
return 0;

err2:
clk_disable(clk);
+ pm_runtime_disable(dev);
err1:
platform_device_put(dwc3);

@@ -172,6 +181,10 @@ static int dwc3_exynos_remove(struct platform_device *pdev)
{
struct dwc3_exynos *exynos = platform_get_drvdata(pdev);

+ if (!pm_runtime_suspended(&pdev->dev))
+ pm_runtime_put(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
platform_device_unregister(exynos->dwc3);
platform_device_unregister(exynos->usb2_phy);
platform_device_unregister(exynos->usb3_phy);
--
1.7.6.5

2013-03-02 13:25:39

[permalink] [raw]

Subject: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

By enabling runtime pm in this driver allows users of
xhci-plat to enter into runtime pm. This is not full
runtime pm support (AKA xhci-plat doesn't actually power
anything off when in runtime suspend mode) but,
just basic enablement.

Signed-off-by: Vivek Gautam <[email protected]>
CC: Doug Anderson <[email protected]>
---
drivers/usb/host/xhci-plat.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c9c7e13..595cb52 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -12,6 +12,7 @@
*/

#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
#include <linux/module.h>
#include <linux/slab.h>

@@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
if (ret)
goto put_usb3_hcd;

+ pm_runtime_enable(&pdev->dev);
+
return 0;

put_usb3_hcd:
@@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);

+ if (!pm_runtime_suspended(&dev->dev))
+ pm_runtime_put(&dev->dev);
+ pm_runtime_disable(&dev->dev);
+
usb_remove_hcd(xhci->shared_hcd);
usb_put_hcd(xhci->shared_hcd);

--
1.7.6.5

2013-03-02 13:25:47

[permalink] [raw]

Subject: [PATCH v2 07/10] usb: phy: samsung: Enable runtime power management on usb3phy

Enable autosuspending of Samsung usb3.0 PHY

Signed-off-by: Vivek Gautam <[email protected]>
---
drivers/usb/phy/samsung-usb3phy.c | 7 +++++++
1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/phy/samsung-usb3phy.c b/drivers/usb/phy/samsung-usb3phy.c
index 70e2c7b..7594cc7 100644
--- a/drivers/usb/phy/samsung-usb3phy.c
+++ b/drivers/usb/phy/samsung-usb3phy.c
@@ -24,6 +24,7 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/of.h>
+#include <linux/pm_runtime.h>
#include <linux/usb/samsung_usb_phy.h>
#include <linux/platform_data/samsung-usbphy.h>

@@ -287,6 +288,8 @@ static int samsung_usb3phy_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, sphy);

+ pm_runtime_enable(&pdev->dev);
+
return usb_add_phy(&sphy->phy, USB_PHY_TYPE_USB3);
}

@@ -296,6 +299,10 @@ static int samsung_usb3phy_remove(struct platform_device *pdev)

usb_remove_phy(&sphy->phy);

+ if (!pm_runtime_suspended(&pdev->dev))
+ pm_runtime_put(&pdev->dev);
+ pm_runtime_disable(&pdev->dev);
+
if (sphy->pmuregs)
iounmap(sphy->pmuregs);
if (sphy->sysreg)
--
1.7.6.5

2013-03-02 13:25:53

[permalink] [raw]

Subject: [PATCH v2 09/10] usb: phy: samsung: Add support for PHY ref_clk gpio

Exynos5250 has external PLL (XusbXTI) for USB 3.0 PHY's
ref_pad_clk. So use this clock based on availability of
gpio to power control this PLL, otherwise use internal
clock only from XXTI.

Signed-off-by: Vivek Gautam <[email protected]>
---
drivers/usb/phy/samsung-usb3phy.c | 14 ++++++++++----
drivers/usb/phy/samsung-usbphy.c | 26 ++++++++++++++++++++++++++
drivers/usb/phy/samsung-usbphy.h | 1 +
3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/phy/samsung-usb3phy.c b/drivers/usb/phy/samsung-usb3phy.c
index 46dd97c..16b024e 100644
--- a/drivers/usb/phy/samsung-usb3phy.c
+++ b/drivers/usb/phy/samsung-usb3phy.c
@@ -24,6 +24,7 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/of.h>
+#include <linux/gpio.h>
#include <linux/pm_runtime.h>
#include <linux/usb/samsung_usb_phy.h>
#include <linux/platform_data/samsung-usbphy.h>
@@ -224,12 +225,17 @@ static int samsung_exynos5_usb3phy_init(struct usb_phy *phy, bool use_ext_clk)
*/
static int samsung_usb3phy_init(struct usb_phy *phy)
{
+ struct samsung_usbphy *sphy = phy_to_sphy(phy);
+
/*
- * We start with using PHY refclk from external PLL,
- * once runtime suspend for the device is called this
- * will change to internal core clock
+ * We check if we have a PHY ref_clk gpio available, then only
+ * use XusbXTI (external PLL); otherwise use internal core clock
+ * from XXTI.
*/
- return samsung_exynos5_usb3phy_init(phy, true);
+ if (gpio_is_valid(sphy->phyclk_gpio))
+ return samsung_exynos5_usb3phy_init(phy, true);
+ else
+ return samsung_exynos5_usb3phy_init(phy, false);
}

/*
diff --git a/drivers/usb/phy/samsung-usbphy.c b/drivers/usb/phy/samsung-usbphy.c
index ab4fa11..6968d12 100644
--- a/drivers/usb/phy/samsung-usbphy.c
+++ b/drivers/usb/phy/samsung-usbphy.c
@@ -27,6 +27,7 @@
#include <linux/io.h>
#include <linux/of.h>
#include <linux/of_address.h>
+#include <linux/of_gpio.h>
#include <linux/usb/samsung_usb_phy.h>

#include "samsung-usbphy.h"
@@ -34,6 +35,7 @@
int samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
{
struct device_node *usbphy_sys;
+ int ret;

/* Getting node for system controller interface for usb-phy */
usbphy_sys = of_get_child_by_name(sphy->dev->of_node, "usbphy-sys");
@@ -58,6 +60,30 @@ int samsung_usbphy_parse_dt(struct samsung_usbphy *sphy)
if (sphy->sysreg == NULL)
dev_warn(sphy->dev, "Can't get usb-phy sysreg cfg register\n");

+ /* Getting PHY clk gpio here to enable/disable PHY clock PLL, if any */
+ sphy->phyclk_gpio = of_get_named_gpio(sphy->dev->of_node,
+ "samsung,phyclk-gpio", 0);
+ /*
+ * We don't want to return error code here in case we don't get the
+ * PHY clock gpio, some PHYs may not have it.
+ */
+ if (gpio_is_valid(sphy->phyclk_gpio)) {
+ ret = gpio_request_one(sphy->phyclk_gpio, GPIOF_INIT_HIGH,
+ "samsung_usb_phy_clock_en");
+ if (ret) {
+ /*
+ * We don't want to return error code here,
+ * sometimes either of usb2 phy or usb3 phy may not
+ * have the PHY clock gpio.
+ */
+ dev_err(sphy->dev, "can't request phyclk gpio %d\n",
+ sphy->phyclk_gpio);
+ sphy->phyclk_gpio = -EINVAL;
+ }
+ } else {
+ dev_warn(sphy->dev, "Can't get usb-phy clock gpio\n");
+ }
+
of_node_put(usbphy_sys);

return 0;
diff --git a/drivers/usb/phy/samsung-usbphy.h b/drivers/usb/phy/samsung-usbphy.h
index f7e657d..1921ab0 100644
--- a/drivers/usb/phy/samsung-usbphy.h
+++ b/drivers/usb/phy/samsung-usbphy.h
@@ -300,6 +300,7 @@ struct samsung_usbphy {
enum samsung_usb_phy_type phy_type;
atomic_t phy_usage;
spinlock_t lock;
+ int phyclk_gpio;
};

#define phy_to_sphy(x) container_of((x), struct samsung_usbphy, phy)
--
1.7.6.5

2013-03-02 13:26:17

[permalink] [raw]

Subject: [PATCH v2 08/10] usb: phy: samsung: Add support for external reference clock

The PHY controller can choose between ref_pad_clk (XusbXTI-external PLL),
or EXTREFCLK (XXTI-internal clock crystal) to generate the required clock.
Adding the provision for ref_pad_clk here.

Signed-off-by: Vivek Gautam <[email protected]>
---
drivers/usb/phy/samsung-usb3phy.c | 46 ++++++++++++++++++++++++++++++++-----
1 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/phy/samsung-usb3phy.c b/drivers/usb/phy/samsung-usb3phy.c
index 7594cc7..46dd97c 100644
--- a/drivers/usb/phy/samsung-usb3phy.c
+++ b/drivers/usb/phy/samsung-usb3phy.c
@@ -33,7 +33,7 @@
/*
* Sets the phy clk as EXTREFCLK (XXTI) which is internal clock from clock core.
*/
-static u32 samsung_usb3phy_set_refclk(struct samsung_usbphy *sphy)
+static u32 samsung_usb3phy_set_refclk_int(struct samsung_usbphy *sphy)
{
u32 reg;
u32 refclk;
@@ -66,7 +66,22 @@ static u32 samsung_usb3phy_set_refclk(struct samsung_usbphy *sphy)
return reg;
}

-static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy)
+/*
+ * Sets the phy clk as ref_pad_clk (XusbXTI) which is clock from external PLL.
+ */
+static u32 samsung_usb3phy_set_refclk_ext(void)
+{
+ u32 reg;
+
+ reg = PHYCLKRST_REFCLKSEL_PAD_REFCLK |
+ PHYCLKRST_FSEL_PAD_100MHZ |
+ PHYCLKRST_MPLL_MULTIPLIER_100MHZ_REF;
+
+ return reg;
+}
+
+static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy,
+ bool use_ext_clk)
{
void __iomem *regs = sphy->regs;
u32 phyparam0;
@@ -81,7 +96,10 @@ static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy)

phyparam0 = readl(regs + EXYNOS5_DRD_PHYPARAM0);
/* Select PHY CLK source */
- phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
+ if (use_ext_clk)
+ phyparam0 |= PHYPARAM0_REF_USE_PAD;
+ else
+ phyparam0 &= ~PHYPARAM0_REF_USE_PAD;
/* Set Loss-of-Signal Detector sensitivity */
phyparam0 &= ~PHYPARAM0_REF_LOSLEVEL_MASK;
phyparam0 |= PHYPARAM0_REF_LOSLEVEL;
@@ -116,7 +134,10 @@ static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy)
/* UTMI Power Control */
writel(PHYUTMI_OTGDISABLE, regs + EXYNOS5_DRD_PHYUTMI);

- phyclkrst = samsung_usb3phy_set_refclk(sphy);
+ if (use_ext_clk)
+ phyclkrst = samsung_usb3phy_set_refclk_ext();
+ else
+ phyclkrst = samsung_usb3phy_set_refclk_int(sphy);

phyclkrst |= PHYCLKRST_PORTRESET |
/* Digital power supply in normal operating mode */
@@ -164,7 +185,7 @@ static void samsung_exynos5_usb3phy_disable(struct samsung_usbphy *sphy)
writel(phytest, regs + EXYNOS5_DRD_PHYTEST);
}

-static int samsung_usb3phy_init(struct usb_phy *phy)
+static int samsung_exynos5_usb3phy_init(struct usb_phy *phy, bool use_ext_clk)
{
struct samsung_usbphy *sphy;
unsigned long flags;
@@ -188,7 +209,7 @@ static int samsung_usb3phy_init(struct usb_phy *phy)
samsung_usbphy_set_isolation(sphy, false);

/* Initialize usb phy registers */
- samsung_exynos5_usb3phy_enable(sphy);
+ samsung_exynos5_usb3phy_enable(sphy, use_ext_clk);

spin_unlock_irqrestore(&sphy->lock, flags);

@@ -199,6 +220,19 @@ static int samsung_usb3phy_init(struct usb_phy *phy)
}

/*
+ * The function passed to the usb driver for phy initialization
+ */
+static int samsung_usb3phy_init(struct usb_phy *phy)
+{
+ /*
+ * We start with using PHY refclk from external PLL,
+ * once runtime suspend for the device is called this
+ * will change to internal core clock
+ */
+ return samsung_exynos5_usb3phy_init(phy, true);
+}
+
+/*
* The function passed to the usb driver for phy shutdown
*/
static void samsung_usb3phy_shutdown(struct usb_phy *phy)
--
1.7.6.5

2013-03-02 13:25:23

[permalink] [raw]

Subject: [PATCH v2 01/10] usb: phy: Add APIs for runtime power management

Adding APIs to handle runtime power management on PHY
devices. PHY consumers may need to wake-up/suspend PHYs
when they work across autosuspend.

Signed-off-by: Vivek Gautam <[email protected]>
---
include/linux/usb/phy.h | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
index 15847cb..0fe7cac 100644
--- a/include/linux/usb/phy.h
+++ b/include/linux/usb/phy.h
@@ -276,4 +276,30 @@ static inline const char *usb_phy_type_string(enum usb_phy_type type)
return "UNKNOWN PHY TYPE";
}
}
+
+#define USB_PHY_AUTOPM(function) \
+static inline int usb_phy_autopm_##function(struct usb_phy *x) \
+{ \
+ if (!x || !x->dev) { \
+ dev_err(x->dev, "no PHY or attached device available\n"); \
+ return -ENODEV; \
+ } \
+ \
+ pm_runtime_##function(x->dev); \
+ \
+ return 0; \
+}
+USB_PHY_AUTOPM(enable)
+USB_PHY_AUTOPM(disable)
+USB_PHY_AUTOPM(get)
+USB_PHY_AUTOPM(get_sync)
+USB_PHY_AUTOPM(put)
+USB_PHY_AUTOPM(put_sync)
+USB_PHY_AUTOPM(allow)
+USB_PHY_AUTOPM(forbid)
+USB_PHY_AUTOPM(suspend)
+USB_PHY_AUTOPM(autosuspend)
+USB_PHY_AUTOPM(resume)
+USB_PHY_AUTOPM(set_active)
+
#endif /* __LINUX_USB_PHY_H */
--
1.7.6.5

2013-03-02 13:27:15

[permalink] [raw]

Subject: [PATCH v2 04/10] usb: dwc3: Add runtime power management callbacks

Right now it doesn't handle full runtime suspend/resume
functionality. However it allows to handle PHYs' sleep
and wakeup across runtime suspend/resume.

Signed-off-by: Vivek Gautam <[email protected]>
---
drivers/usb/dwc3/core.c | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 2a77327..45e1aae 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -704,11 +704,38 @@ static int dwc3_resume(struct device *dev)
return 0;
}

+#ifdef CONFIG_PM_RUNTIME
+static int dwc3_runtime_suspend(struct device *dev)
+{
+ struct dwc3 *dwc = dev_get_drvdata(dev);
+
+ usb_phy_autopm_put_sync(dwc->usb2_phy);
+ usb_phy_autopm_put_sync(dwc->usb3_phy);
+
+ return 0;
+}
+
+static int dwc3_runtime_resume(struct device *dev)
+{
+ struct dwc3 *dwc = dev_get_drvdata(dev);
+
+ usb_phy_autopm_get_sync(dwc->usb2_phy);
+ usb_phy_autopm_get_sync(dwc->usb3_phy);
+
+ return 0;
+}
+#else
+#define dwc3_runtime_suspend NULL
+#define dwc3_runtime_resume NULL
+#endif
+
static const struct dev_pm_ops dwc3_dev_pm_ops = {
.prepare = dwc3_prepare,
.complete = dwc3_complete,

SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
+ SET_RUNTIME_PM_OPS(dwc3_runtime_suspend,
+ dwc3_runtime_resume, NULL)
};

#define DWC3_PM_OPS &(dwc3_dev_pm_ops)
--
1.7.6.5

2013-03-02 13:28:17

[permalink] [raw]

Subject: [PATCH v2 02/10] USB: dwc3: Adjust runtime pm to allow autosuspend

The current code in the dwc3 probe effectively disables runtime pm
from ever working because it calls a get() that was never put() until
device removal. Change the runtime pm code to match the standard
formula and allow runtime pm to function.

Signed-off-by: Vivek Gautam <[email protected]>
CC: Doug Anderson <[email protected]>
---
drivers/usb/dwc3/core.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index be0672f..85914e0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -528,6 +528,7 @@ static int dwc3_probe(struct platform_device *pdev)
goto err3;
}

+ pm_runtime_put_sync(dev);
pm_runtime_allow(dev);

return 0;
@@ -557,6 +558,7 @@ err1:

err0:
dwc3_free_event_buffers(dwc);
+ pm_runtime_disable(&pdev->dev);

return ret;
}
@@ -568,7 +570,8 @@ static int dwc3_remove(struct platform_device *pdev)
usb_phy_set_suspend(dwc->usb2_phy, 1);
usb_phy_set_suspend(dwc->usb3_phy, 1);

- pm_runtime_put(&pdev->dev);
+ if (!pm_runtime_suspended(&pdev->dev))
+ pm_runtime_put(&pdev->dev);
pm_runtime_disable(&pdev->dev);

dwc3_debugfs_exit(dwc);
--
1.7.6.5

2013-03-02 15:53:19

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

On Sat, 2 Mar 2013, Vivek Gautam wrote:

> By enabling runtime pm in this driver allows users of
> xhci-plat to enter into runtime pm. This is not full
> runtime pm support (AKA xhci-plat doesn't actually power
> anything off when in runtime suspend mode) but,
> just basic enablement.
>
> Signed-off-by: Vivek Gautam <[email protected]>
> CC: Doug Anderson <[email protected]>
> ---
> drivers/usb/host/xhci-plat.c | 7 +++++++
> 1 files changed, 7 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index c9c7e13..595cb52 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -12,6 +12,7 @@
> */
>
> #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> #include <linux/module.h>
> #include <linux/slab.h>
>
> @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (ret)
> goto put_usb3_hcd;
>
> + pm_runtime_enable(&pdev->dev);

This is generally not a good idea. You shouldn't enable a device for
runtime PM without first telling the PM core what state it is in.

> @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> struct usb_hcd *hcd = platform_get_drvdata(dev);
> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>
> + if (!pm_runtime_suspended(&dev->dev))
> + pm_runtime_put(&dev->dev);
> + pm_runtime_disable(&dev->dev);
> +
> usb_remove_hcd(xhci->shared_hcd);
> usb_put_hcd(xhci->shared_hcd);

This is very strange. Why have a pm_runtime_put with no balancing
pm_runtime_get?

And why use an async routine when you're about to unbind the driver?
Don't you want the callback to occur before the unbinding?

Alan Stern

2013-03-02 20:39:28

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

Hi,

On Sat, Mar 02, 2013 at 10:53:16AM -0500, Alan Stern wrote:
> On Sat, 2 Mar 2013, Vivek Gautam wrote:
>
> > By enabling runtime pm in this driver allows users of
> > xhci-plat to enter into runtime pm. This is not full
> > runtime pm support (AKA xhci-plat doesn't actually power
> > anything off when in runtime suspend mode) but,
> > just basic enablement.
> >
> > Signed-off-by: Vivek Gautam <[email protected]>
> > CC: Doug Anderson <[email protected]>
> > ---
> > drivers/usb/host/xhci-plat.c | 7 +++++++
> > 1 files changed, 7 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index c9c7e13..595cb52 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -12,6 +12,7 @@
> > */
> >
> > #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/module.h>
> > #include <linux/slab.h>
> >
> > @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > if (ret)
> > goto put_usb3_hcd;
> >
> > + pm_runtime_enable(&pdev->dev);
>
> This is generally not a good idea. You shouldn't enable a device for
> runtime PM without first telling the PM core what state it is in.
>
> > @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> > struct usb_hcd *hcd = platform_get_drvdata(dev);
> > struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> >
> > + if (!pm_runtime_suspended(&dev->dev))
> > + pm_runtime_put(&dev->dev);
> > + pm_runtime_disable(&dev->dev);
> > +
> > usb_remove_hcd(xhci->shared_hcd);
> > usb_put_hcd(xhci->shared_hcd);
>
> This is very strange. Why have a pm_runtime_put with no balancing
> pm_runtime_get?

this is good point and, in fact, a doubt I have myself. How are we
supposed to check if device is suspended ? In case it _is_ suspended we
might not be able to read device's registers due to clocks possibly
being gated.

Also, considering that some drivers are used in multiple platforms and
those might behave differently when it comes to clock handling, how do
we do that ? Should we require drivers to explicitly clk_get();
clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ?

While that's doable, I don't see how that'd be doable for OMAP since
they want to hide clock handling from drivers.

Any tips ?

--
balbi

Attachments:

(No filename) (2.33 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-03-02 22:02:16

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

On Sat, 2 Mar 2013, Felipe Balbi wrote:

> > > @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> > > struct usb_hcd *hcd = platform_get_drvdata(dev);
> > > struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > >
> > > + if (!pm_runtime_suspended(&dev->dev))
> > > + pm_runtime_put(&dev->dev);
> > > + pm_runtime_disable(&dev->dev);
> > > +
> > > usb_remove_hcd(xhci->shared_hcd);
> > > usb_put_hcd(xhci->shared_hcd);
> >
> > This is very strange. Why have a pm_runtime_put with no balancing
> > pm_runtime_get?
>
> this is good point and, in fact, a doubt I have myself. How are we
> supposed to check if device is suspended ? In case it _is_ suspended we
> might not be able to read device's registers due to clocks possibly
> being gated.

That's really a separate question. It has a simple answer, though: If
you want to access a device's registers, call pm_runtime_get_sync()
beforehand and pm_runtime_put() (or _put_sync()) afterward. Then it
won't matter if the device was suspended originally.

If you actually do want to tell whether or not a device is suspended
and nothing more, call pm_runtime_status_suspended(). Of course, this
is racy -- the power state might change right after you make the call.

> Also, considering that some drivers are used in multiple platforms and
> those might behave differently when it comes to clock handling, how do
> we do that ? Should we require drivers to explicitly clk_get();
> clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ?

I don't know much about clock handling. In general, the
pm_runtime_set_active() and pm_runtime_enable() parts should be done by
the subsystem, not the driver, whenever possible.

> While that's doable, I don't see how that'd be doable for OMAP since
> they want to hide clock handling from drivers.
>
> Any tips ?

Whichever piece of code is responsible for associating a clock with a
device should also be responsible for making sure that neither is
suspended when the driver's probe routine runs. I'm not sure how
different platforms do this; using a PM domain could be one answer.

All this is somewhat off the point of my original comment, however.
Drivers must be sure to balance their pm_runtime_get() and _put()
calls. Having an unbalanced _put() in the remove routine is almost
certainly a mistake -- especially if it is conditional on the device's
power state, because a device can remain unsuspended even after the
driver does a pm_runtime_put(). For example, this will happen if the
user wrote "on" to /sys/.../power/control.

Alan Stern

2013-03-02 23:21:52

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

Hi,

On Sat, Mar 02, 2013 at 05:02:13PM -0500, Alan Stern wrote:
> On Sat, 2 Mar 2013, Felipe Balbi wrote:
>
> > > > @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
> > > > struct usb_hcd *hcd = platform_get_drvdata(dev);
> > > > struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > > >
> > > > + if (!pm_runtime_suspended(&dev->dev))
> > > > + pm_runtime_put(&dev->dev);
> > > > + pm_runtime_disable(&dev->dev);
> > > > +
> > > > usb_remove_hcd(xhci->shared_hcd);
> > > > usb_put_hcd(xhci->shared_hcd);
> > >
> > > This is very strange. Why have a pm_runtime_put with no balancing
> > > pm_runtime_get?
> >
> > this is good point and, in fact, a doubt I have myself. How are we
> > supposed to check if device is suspended ? In case it _is_ suspended we
> > might not be able to read device's registers due to clocks possibly
> > being gated.
>
> That's really a separate question. It has a simple answer, though: If
> you want to access a device's registers, call pm_runtime_get_sync()
> beforehand and pm_runtime_put() (or _put_sync()) afterward. Then it
> won't matter if the device was suspended originally.

that's alright, but how do you want me to check if my device is enabled
or not before pm_runtime_enable() ?

> If you actually do want to tell whether or not a device is suspended
> and nothing more, call pm_runtime_status_suspended(). Of course, this
> is racy -- the power state might change right after you make the call.

right.

> > Also, considering that some drivers are used in multiple platforms and
> > those might behave differently when it comes to clock handling, how do
> > we do that ? Should we require drivers to explicitly clk_get();
> > clk_prepare_enable(); pm_runtime_set_active(); pm_runtime_enable() ?
>
> I don't know much about clock handling. In general, the
> pm_runtime_set_active() and pm_runtime_enable() parts should be done by
> the subsystem, not the driver, whenever possible.

good to know :-) Though I disagree with calling pm_runtime_enable() at
the subsystem level.

This means we can add pm_runtime_set_active()

> > While that's doable, I don't see how that'd be doable for OMAP since
> > they want to hide clock handling from drivers.
> >
> > Any tips ?
>
> Whichever piece of code is responsible for associating a clock with a
> device should also be responsible for making sure that neither is
> suspended when the driver's probe routine runs. I'm not sure how
> different platforms do this; using a PM domain could be one answer.

that's alright, but it generates a different set of problems. That same
piece of code which associates a device with its clock, doesn't really
know if the driver is even available which means we could be enabling
clocks for no reason and just wasting precious battery juice ;-)

> All this is somewhat off the point of my original comment, however.
> Drivers must be sure to balance their pm_runtime_get() and _put()
> calls. Having an unbalanced _put() in the remove routine is almost
> certainly a mistake -- especially if it is conditional on the device's
> power state, because a device can remain unsuspended even after the
> driver does a pm_runtime_put(). For example, this will happen if the
> user wrote "on" to /sys/.../power/control.

indeed... Makes sense. I'll consider mailing linux-pm for the rest of
the discussion, cheers.

--
balbi

Attachments:

(No filename) (3.32 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-03-02 23:24:40

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

Hi,

On Sun, Mar 03, 2013 at 01:21:32AM +0200, Felipe Balbi wrote:
> > I don't know much about clock handling. In general, the
> > pm_runtime_set_active() and pm_runtime_enable() parts should be done by
> > the subsystem, not the driver, whenever possible.
>
> good to know :-) Though I disagree with calling pm_runtime_enable() at
> the subsystem level.
>
> This means we can add pm_runtime_set_active()

ignore this last line, forgot to delete it.

--
balbi

Attachments:

(No filename) (465.00 B)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-03-04 08:08:49

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

Hi,

On Sat, Mar 2, 2013 at 9:23 PM, Alan Stern <[email protected]> wrote:
> On Sat, 2 Mar 2013, Vivek Gautam wrote:
>
>> By enabling runtime pm in this driver allows users of
>> xhci-plat to enter into runtime pm. This is not full
>> runtime pm support (AKA xhci-plat doesn't actually power
>> anything off when in runtime suspend mode) but,
>> just basic enablement.
>>
>> Signed-off-by: Vivek Gautam <[email protected]>
>> CC: Doug Anderson <[email protected]>
>> ---
>> drivers/usb/host/xhci-plat.c | 7 +++++++
>> 1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index c9c7e13..595cb52 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -12,6 +12,7 @@
>> */
>>
>> #include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> #include <linux/module.h>
>> #include <linux/slab.h>
>>
>> @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
>> if (ret)
>> goto put_usb3_hcd;
>>
>> + pm_runtime_enable(&pdev->dev);
>
> This is generally not a good idea. You shouldn't enable a device for
> runtime PM without first telling the PM core what state it is in.
>
Right, but i am not completely sure on any fixed path to follow for
enabling runtime
power management on a device. :-(
Does it become necessary to "pm_runtime_set_active" or
"pm_runtime_set_suspended" a device
before "pm_runtime_enable" ? pm_runtime_enable would just try to
decrement the disable_depth
of the device.

>> @@ -174,6 +177,10 @@ static int xhci_plat_remove(struct platform_device *dev)
>> struct usb_hcd *hcd = platform_get_drvdata(dev);
>> struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>
>> + if (!pm_runtime_suspended(&dev->dev))
>> + pm_runtime_put(&dev->dev);
>> + pm_runtime_disable(&dev->dev);
>> +
>> usb_remove_hcd(xhci->shared_hcd);
>> usb_put_hcd(xhci->shared_hcd);
>
> This is very strange. Why have a pm_runtime_put with no balancing
> pm_runtime_get?
>
> And why use an async routine when you're about to unbind the driver?
> Don't you want the callback to occur before the unbinding?
>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Thanks & Regards
Vivek

2013-03-04 15:29:36

[permalink] [raw]

Subject: Re: [PATCH v2 06/10] usb: xhci: Enable runtime pm in xhci-plat

On Mon, 4 Mar 2013, Vivek Gautam wrote:

> >> @@ -149,6 +150,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >> if (ret)
> >> goto put_usb3_hcd;
> >>
> >> + pm_runtime_enable(&pdev->dev);
> >
> > This is generally not a good idea. You shouldn't enable a device for
> > runtime PM without first telling the PM core what state it is in.
> >
> Right, but i am not completely sure on any fixed path to follow for
> enabling runtime
> power management on a device. :-(
> Does it become necessary to "pm_runtime_set_active" or
> "pm_runtime_set_suspended" a device
> before "pm_runtime_enable" ?

Yes, it usually is necesary. And especially here, because the runtime
PM core sets the initial status of every device to RPM_SUSPENDED.

> pm_runtime_enable would just try to
> decrement the disable_depth
> of the device.

That's right. And once that happens, the PM core would think the
device was suspended even though it was really at full power.

Alan Stern

2013-03-04 15:29:41

[permalink] [raw]

Subject: Re: [PATCH v2 01/10] usb: phy: Add APIs for runtime power management

Hi,

On Sat, Mar 02, 2013 at 06:53:02PM +0530, Vivek Gautam wrote:
> Adding APIs to handle runtime power management on PHY
> devices. PHY consumers may need to wake-up/suspend PHYs
> when they work across autosuspend.
>
> Signed-off-by: Vivek Gautam <[email protected]>
> ---
> include/linux/usb/phy.h | 26 ++++++++++++++++++++++++++
> 1 files changed, 26 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h
> index 15847cb..0fe7cac 100644
> --- a/include/linux/usb/phy.h
> +++ b/include/linux/usb/phy.h
> @@ -276,4 +276,30 @@ static inline const char *usb_phy_type_string(enum usb_phy_type type)
> return "UNKNOWN PHY TYPE";
> }
> }
> +
> +#define USB_PHY_AUTOPM(function) \
> +static inline int usb_phy_autopm_##function(struct usb_phy *x) \
> +{ \
> + if (!x || !x->dev) { \
> + dev_err(x->dev, "no PHY or attached device available\n"); \
> + return -ENODEV; \
> + } \
> + \
> + pm_runtime_##function(x->dev); \

please make the definitions explicit (not using a macro) and use:

return pm_runtime_foo();

where applicable. We don't want to return 0 if pm_runtime_get_sync()
fails.

--
balbi

Attachments:

(No filename) (1.21 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-03-04 15:57:16