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

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

[Vivek Gautam]

This patch-series enables runtime power management on xhci-plat,
dwc3-core, dwc3-exynos as well as on samsung-usb3 type PHY.
This allows usb 3.0 host ports to be power managed at runtime.
We also turn off the PHY ref_clk PLL, which supplies reference clock
to USB3 type phy, when ports are not in use.

Based on 'usb-next' alongwith following patch series:
  [PATCH v4 00/11] usb: dwc3: PM support patchset
  http://www.mail-archive.com/linux-usb@vger.kernel.org/msg14676.html

  [PATCH v2] usb: xhci: add the suspend/resume functionality
  http://www.mail-archive.com/linux-usb@vger.kernel.org/msg14614.html

  [PATCH v6 0/2] Adding USB 3.0 DRD-phy support for exynos5250
  http://www.mail-archive.com/linux-usb@vger.kernel.org/msg15813.html

Changes from v1:
 - Adding required PHY APIs to handle runtime power management
   instead of directly twiddling with phy->dev.
 - Handling runtime power management of usb PHYs in dwc3 core
   driver instead of in any glue layer.
 - Splitting the patch "[PATCH 4/4] usb: phy: samsung: Enable runtime power management on samsung-usb"
   to required number to bifurcate functionality.

Vivek Gautam (10):
  usb: phy: Add APIs for runtime power management
  USB: dwc3: Adjust runtime pm to allow autosuspend
  usb: dwc3: Enable runtime pm only after PHYs are initialized
  usb: dwc3: Add runtime power management callbacks
  usb: dwc3: exynos: Enable runtime power management
  usb: xhci: Enable runtime pm in xhci-plat
  usb: phy: samsung: Enable runtime power management on usb3phy
  usb: phy: samsung: Add support for external reference clock
  usb: phy: samsung: Add support for PHY ref_clk gpio
  usb: phy: samsung: Add support for PHY refclk switching

 drivers/usb/dwc3/core.c           |   40 +++++++++++--
 drivers/usb/dwc3/dwc3-exynos.c    |   13 ++++
 drivers/usb/host/xhci-plat.c      |    7 ++
 drivers/usb/phy/samsung-usb3phy.c |  122 +++++++++++++++++++++++++++++++++++--
 drivers/usb/phy/samsung-usbphy.c  |   26 ++++++++
 drivers/usb/phy/samsung-usbphy.h  |    1 +
 include/linux/usb/phy.h           |   26 ++++++++
 7 files changed, 224 insertions(+), 11 deletions(-)

-- 
1.7.6.5

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 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

--
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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
CC: Doug Anderson <dianders@chromium.org>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 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

--
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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
CC: Doug Anderson <dianders@chromium.org>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
CC: Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
 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

--
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

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

[Vivek Gautam]

Enable autosuspending of Samsung usb3.0 PHY

Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 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

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

[Vivek Gautam]

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 <gautam.vivek@samsung.com>
---
 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

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

[Alan Stern]

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 <gautam.vivek@samsung.com>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  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

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

[Alan Stern]

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 <gautam.vivek@samsung.com>
> CC: Doug Anderson <dianders@chromium.org>
> ---
>  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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

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 <gautam.vivek@samsung.com>
> > CC: Doug Anderson <dianders@chromium.org>
> > ---
> >  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2456 bytes --]

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 <gautam.vivek@samsung.com>
> > CC: Doug Anderson <dianders@chromium.org>
> > ---
> >  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Alan Stern]

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

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

[Alan Stern]

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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3480 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3480 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Vivek Gautam]

Hi,


On Sat, Mar 2, 2013 at 9:23 PM, Alan Stern <stern@rowland.harvard.edu> 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 <gautam.vivek@samsung.com>
>> CC: Doug Anderson <dianders@chromium.org>
>> ---
>>  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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Thanks & Regards
Vivek

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

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 <gautam.vivek@samsung.com>
> ---
>  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

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 <gautam.vivek@samsung.com>
> ---
>  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Alan Stern]

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

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

[Alan Stern]

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

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

[Alan Stern]

On Sun, 3 Mar 2013, Felipe Balbi wrote:

> > > 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() ?

You're not supposed to check.  Just make sure your own 
pm_runtime_enable() and _disable() calls are done correctly, and let 
the PM core worry about the rest.  :-)

More to the point, the question of what code enables a device for
runtime PM is decided by the subsystem.  Many subsystems will do it
automatically so that their drivers don't have to worry about it.  
Other subsystems will leave it entirely up to the drivers.  You have to
know what the subsystem wants.

In this case, it's appropriate to do the enable here in the probe
routine because the platform core doesn't do it for you.  This also
means the driver should disable the device in the remove routine.

> > 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.

It depends on the subsystem.  For some (like the USB host subsystem),
it is appropriate.

> > 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 ;-)

Better than wasting our precious bodily fluids...  :-)

I guess the best answer is to set up the association but then leave the
device and clocks in a runtime-suspended status.  Then do a
pm_runtime_get_sync() just before calling the driver's probe routine
and a pm_runtime_put_sync() just after calling the remove routine.  
That should leave the device and the clocks in the state the driver
expects.  (But be careful that these two calls don't invoke the
driver's runtime-PM callbacks!)

Probably nobody has thought these problems through very carefully for 
the platform subsystem.  Nevertheless, that's where the decisions need 
to be made.

Alan Stern

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

[Alan Stern]

On Sun, 3 Mar 2013, Felipe Balbi wrote:

> > > 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() ?

You're not supposed to check.  Just make sure your own 
pm_runtime_enable() and _disable() calls are done correctly, and let 
the PM core worry about the rest.  :-)

More to the point, the question of what code enables a device for
runtime PM is decided by the subsystem.  Many subsystems will do it
automatically so that their drivers don't have to worry about it.  
Other subsystems will leave it entirely up to the drivers.  You have to
know what the subsystem wants.

In this case, it's appropriate to do the enable here in the probe
routine because the platform core doesn't do it for you.  This also
means the driver should disable the device in the remove routine.

> > 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.

It depends on the subsystem.  For some (like the USB host subsystem),
it is appropriate.

> > 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 ;-)

Better than wasting our precious bodily fluids...  :-)

I guess the best answer is to set up the association but then leave the
device and clocks in a runtime-suspended status.  Then do a
pm_runtime_get_sync() just before calling the driver's probe routine
and a pm_runtime_put_sync() just after calling the remove routine.  
That should leave the device and the clocks in the state the driver
expects.  (But be careful that these two calls don't invoke the
driver's runtime-PM callbacks!)

Probably nobody has thought these problems through very carefully for 
the platform subsystem.  Nevertheless, that's where the decisions need 
to be made.

Alan Stern