[PATCH 00/11] Add power management support for chipidea

[PATCH 00/11] Add power management support for chipidea

[Peter Chen]

Hi Greg,

This serial adds power management (system & runtime) for chipidea core.
With this, the chipidea controller can be at low power mode when it is not in
use, and the chipidea controller can be the system wakeup source. 
It needs to depend on my patch[1], since [1] adds some common PHY APIs, and
this serial uses it.

It has been verified at Freescale i.mx6Q SabreSD platform, I will verify
it at other FSL platforms during the patch review.

Hi Alan,

Due to chipidea core and imx concontroller needs some special operations
during the standard ehci routine, I override .hub_control, .bus_suspend,
and .bus_resume.

There is one special thing is I use flag ehci->bus_suspended to know it
was a global suspend before due to I need to notify PHY when the suspend
has finished (portsc.suspendM is set) and the resume signal has finished
(portsc.fpr is cleared) for high speed device, but there are two places
will send suspend/resume, and I don't want to patch ehci-hub.c (if you think
patch ehci-hub.c is a good way, I can do it).

The related host patches:

  usb: chipidea: host: add quirk for ehci operation
  usb: chipidea: host: add ehci quirk for imx controller

Hi Shawn,

The last two are devicetree related to enable runtime pm.

Peter Chen (11):
  usb: chipidea: Add power management support
  usb: chipidea: imx: add power management support
  usb: chipidea: usbmisc_imx: remove the controller's clock information
  usb: chipidea: add wakeup interrupt handler
  usb: chipidea: usbmisc_imx: add set_wakup API
  usb: chipidea: imx: call set_wakeup when necessary
  usb: chipidea: host: add quirk for ehci operation
  usb: chipidea: host: add ehci quirk for imx controller
  usb: chipidea: imx: Enable CI_HDRC_IMX_EHCI_QUIRK if the phy has
    notify APIs
  usb: chipidea: imx: add binding for supporting runtime pm
  ARM: dts: imx6qdl-sabresd: Enable runtime pm for usbotg and usb host
    1

 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 +
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi             |    2 +
 drivers/usb/chipidea/ci.h                          |    3 +
 drivers/usb/chipidea/ci_hdrc_imx.c                 |  140 +++++++++++++++-
 drivers/usb/chipidea/ci_hdrc_imx.h                 |    1 +
 drivers/usb/chipidea/core.c                        |  139 +++++++++++++++
 drivers/usb/chipidea/host.c                        |  180 ++++++++++++++++++++
 drivers/usb/chipidea/otg.c                         |    5 +
 drivers/usb/chipidea/usbmisc_imx.c                 |   59 +++++--
 include/linux/usb/chipidea.h                       |    2 +
 10 files changed, 512 insertions(+), 21 deletions(-)

[1] Add power management support for MXS PHY:
http://marc.info/?l=linux-usb&m=138156975205686&w=2

[PATCH 01/11] usb: chipidea: Add power management support

[Peter Chen]

This commit adds runtime and system power management support for
chipidea core. The runtime pm support is controlled by glue
layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci.h    |    2 +
 drivers/usb/chipidea/core.c  |  119 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/chipidea.h |    1 +
 3 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 1c94fc5..6c16d11 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -173,6 +173,8 @@ struct ci_hdrc {
 	struct dentry			*debugfs;
 	bool				id_event;
 	bool				b_sess_valid_event;
+	bool				supports_runtime_pm;
+	atomic_t			in_lpm;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 87546a0..7ffb8cb 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -578,6 +578,10 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	hw_phymode_configure(ci);
 
 	dr_mode = ci->platdata->dr_mode;
+
+	ci->supports_runtime_pm = !!(ci->platdata->flags &
+		CI_HDRC_SUPPORTS_RUNTIME_PM);
+
 	/* initialize role(s) before the interrupt is requested */
 	if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
 		ret = ci_hdrc_host_init(ci);
@@ -651,6 +655,13 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	if (ret)
 		goto stop;
 
+	device_set_wakeup_capable(&pdev->dev, true);
+
+	if (ci->supports_runtime_pm) {
+		pm_runtime_set_active(&pdev->dev);
+		pm_runtime_enable(&pdev->dev);
+	}
+
 	ret = dbg_create_files(ci);
 	if (!ret)
 		return 0;
@@ -668,6 +679,11 @@ static int ci_hdrc_remove(struct platform_device *pdev)
 {
 	struct ci_hdrc *ci = platform_get_drvdata(pdev);
 
+	if (ci->supports_runtime_pm) {
+		pm_runtime_get_sync(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
+		pm_runtime_put_noidle(&pdev->dev);
+	}
 	dbg_remove_files(ci);
 	free_irq(ci->irq, ci);
 	ci_role_destroy(ci);
@@ -678,11 +694,114 @@ static int ci_hdrc_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int ci_controller_suspend(struct device *dev)
+{
+	struct ci_hdrc *ci = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "at %s\n", __func__);
+
+	if (atomic_read(&ci->in_lpm))
+		return 0;
+
+	disable_irq(ci->irq);
+
+	if (ci->transceiver)
+		usb_phy_set_wakeup(ci->transceiver, true);
+
+	ci_hdrc_enter_lpm(ci, true);
+
+	if (ci->transceiver)
+		usb_phy_set_suspend(ci->transceiver, 1);
+
+	atomic_set(&ci->in_lpm, 1);
+
+	enable_irq(ci->irq);
+
+	return 0;
+}
+
+static int ci_controller_resume(struct device *dev)
+{
+	struct ci_hdrc *ci = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "at %s\n", __func__);
+
+	if (!atomic_read(&ci->in_lpm))
+		return 0;
+
+	ci_hdrc_enter_lpm(ci, false);
+
+	if (ci->transceiver) {
+		usb_phy_set_suspend(ci->transceiver, 0);
+		usb_phy_set_wakeup(ci->transceiver, false);
+	}
+
+	atomic_set(&ci->in_lpm, 0);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ci_suspend(struct device *dev)
+{
+	struct ci_hdrc *ci = dev_get_drvdata(dev);
+	int ret;
+
+	ret = ci_controller_suspend(dev);
+	if (ret)
+		return ret;
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(ci->irq);
+
+	return ret;
+}
+
+static int ci_resume(struct device *dev)
+{
+	struct ci_hdrc *ci = dev_get_drvdata(dev);
+	int ret;
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(ci->irq);
+
+	ret = ci_controller_resume(dev);
+	if (!ret && ci->supports_runtime_pm) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM_RUNTIME
+static int ci_runtime_suspend(struct device *dev)
+{
+	return ci_controller_suspend(dev);
+}
+
+static int ci_runtime_resume(struct device *dev)
+{
+	return ci_controller_resume(dev);
+}
+#endif /* CONFIG_PM_RUNTIME */
+
+#endif /* CONFIG_PM */
+static const struct dev_pm_ops ci_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ci_suspend, ci_resume)
+	SET_RUNTIME_PM_OPS(ci_runtime_suspend,
+			ci_runtime_resume, NULL)
+};
+
 static struct platform_driver ci_hdrc_driver = {
 	.probe	= ci_hdrc_probe,
 	.remove	= ci_hdrc_remove,
 	.driver	= {
 		.name	= "ci_hdrc",
+		.pm	= &ci_pm_ops,
 	},
 };
 
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 7d39967..667b46c 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -18,6 +18,7 @@ struct ci_hdrc_platform_data {
 	unsigned long	 flags;
 #define CI_HDRC_REGS_SHARED		BIT(0)
 #define CI_HDRC_REQUIRE_TRANSCEIVER	BIT(1)
+#define CI_HDRC_SUPPORTS_RUNTIME_PM	BIT(2)
 #define CI_HDRC_DISABLE_STREAMING	BIT(3)
 	/*
 	 * Only set it when DCCPARAMS.DC==1 and DCCPARAMS.HC==1,
-- 
1.7.1

[PATCH 02/11] usb: chipidea: imx: add power management support

[Peter Chen]

Add system and runtime power management support for imx gluy layer.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c |   99 ++++++++++++++++++++++++++++++++++-
 1 files changed, 96 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 023d3cb..9f66f93 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -28,6 +28,8 @@ struct ci_hdrc_imx_data {
 	struct platform_device *ci_pdev;
 	struct clk *clk;
 	struct imx_usbmisc_data *usbmisc_data;
+	atomic_t in_lpm;
+	bool supports_runtime_pm;
 };
 
 /* Common functions shared by usbmisc drivers */
@@ -82,6 +84,7 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 				  CI_HDRC_DISABLE_STREAMING,
 	};
 	int ret;
+	struct device_node *np = pdev->dev.of_node;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
@@ -115,6 +118,11 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 
 	pdata.phy = data->phy;
 
+	if (of_find_property(np, "supports_runtime_pm", NULL)) {
+		pdata.flags |= CI_HDRC_SUPPORTS_RUNTIME_PM;
+		data->supports_runtime_pm = true;
+	}
+
 	if (!pdev->dev.dma_mask)
 		pdev->dev.dma_mask = &pdev->dev.coherent_dma_mask;
 	if (!pdev->dev.coherent_dma_mask)
@@ -151,8 +159,12 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	pm_runtime_no_callbacks(&pdev->dev);
-	pm_runtime_enable(&pdev->dev);
+	device_set_wakeup_capable(&pdev->dev, true);
+
+	if (data->supports_runtime_pm) {
+		pm_runtime_set_active(&pdev->dev);
+		pm_runtime_enable(&pdev->dev);
+	}
 
 	return 0;
 
@@ -167,13 +179,93 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
 {
 	struct ci_hdrc_imx_data *data = platform_get_drvdata(pdev);
 
-	pm_runtime_disable(&pdev->dev);
 	ci_hdrc_remove_device(data->ci_pdev);
+	if (data->supports_runtime_pm) {
+		pm_runtime_get_sync(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
+		pm_runtime_put_noidle(&pdev->dev);
+	}
+	clk_disable_unprepare(data->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int imx_controller_suspend(struct device *dev)
+{
+	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "at %s\n", __func__);
+
+	if (atomic_read(&data->in_lpm))
+		return 0;
+
 	clk_disable_unprepare(data->clk);
 
+	atomic_set(&data->in_lpm, 1);
+
 	return 0;
 }
 
+static int imx_controller_resume(struct device *dev)
+{
+	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
+	int ret = 0;
+
+	dev_dbg(dev, "at %s\n", __func__);
+
+	if (!atomic_read(&data->in_lpm))
+		return 0;
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
+	atomic_set(&data->in_lpm, 0);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ci_hdrc_imx_suspend(struct device *dev)
+{
+	return imx_controller_suspend(dev);
+}
+
+static int ci_hdrc_imx_resume(struct device *dev)
+{
+	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
+	int ret;
+
+	ret = imx_controller_resume(dev);
+	if (!ret && data->supports_runtime_pm) {
+		pm_runtime_disable(dev);
+		pm_runtime_set_active(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return ret;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM_RUNTIME
+static int ci_hdrc_imx_runtime_suspend(struct device *dev)
+{
+	return imx_controller_suspend(dev);
+}
+
+static int ci_hdrc_imx_runtime_resume(struct device *dev)
+{
+	return imx_controller_resume(dev);
+}
+#endif /* CONFIG_PM_RUNTIME */
+
+#endif /* CONFIG_PM */
+static const struct dev_pm_ops ci_hdrc_imx_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ci_hdrc_imx_suspend, ci_hdrc_imx_resume)
+	SET_RUNTIME_PM_OPS(ci_hdrc_imx_runtime_suspend,
+			ci_hdrc_imx_runtime_resume, NULL)
+};
 static const struct of_device_id ci_hdrc_imx_dt_ids[] = {
 	{ .compatible = "fsl,imx27-usb", },
 	{ /* sentinel */ }
@@ -187,6 +279,7 @@ static struct platform_driver ci_hdrc_imx_driver = {
 		.name = "imx_usb",
 		.owner = THIS_MODULE,
 		.of_match_table = ci_hdrc_imx_dt_ids,
+		.pm = &ci_hdrc_imx_pm_ops,
 	 },
 };
 
-- 
1.7.1

[PATCH 03/11] usb: chipidea: usbmisc_imx: remove the controller's clock information

[Peter Chen]

Currently, the usbmisc is just an API supplier for controller
driver, the controller calls related APIs to handle different
things among the SoCs, before calling it, the clock must
be on. So the clock operation is useless for usbmisc, it also
increases the difficulties to manage the clock, especially at
runtime power management situation.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/usbmisc_imx.c |   18 ------------------
 1 files changed, 0 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
index 8a1094b..1fd9a12 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -11,7 +11,6 @@
 
 #include <linux/module.h>
 #include <linux/of_platform.h>
-#include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/delay.h>
@@ -40,7 +39,6 @@ struct usbmisc_ops {
 struct imx_usbmisc {
 	void __iomem *base;
 	spinlock_t lock;
-	struct clk *clk;
 	const struct usbmisc_ops *ops;
 };
 
@@ -177,7 +175,6 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
 {
 	struct resource	*res;
 	struct imx_usbmisc *data;
-	int ret;
 	struct of_device_id *tmp_dev;
 
 	if (usbmisc)
@@ -194,20 +191,6 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
 	if (IS_ERR(data->base))
 		return PTR_ERR(data->base);
 
-	data->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(data->clk)) {
-		dev_err(&pdev->dev,
-			"failed to get clock, err=%ld\n", PTR_ERR(data->clk));
-		return PTR_ERR(data->clk);
-	}
-
-	ret = clk_prepare_enable(data->clk);
-	if (ret) {
-		dev_err(&pdev->dev,
-			"clk_prepare_enable failed, err=%d\n", ret);
-		return ret;
-	}
-
 	tmp_dev = (struct of_device_id *)
 		of_match_device(usbmisc_imx_dt_ids, &pdev->dev);
 	data->ops = (const struct usbmisc_ops *)tmp_dev->data;
@@ -218,7 +201,6 @@ static int usbmisc_imx_probe(struct platform_device *pdev)
 
 static int usbmisc_imx_remove(struct platform_device *pdev)
 {
-	clk_disable_unprepare(usbmisc->clk);
 	usbmisc = NULL;
 	return 0;
 }
-- 
1.7.1

[PATCH 04/11] usb: chipidea: add wakeup interrupt handler

[Peter Chen]

When the controller is at suspend mode, it can be waken up by
external events (like vbus, dp/dm or id change). Once we receive
the wakeup interrupt, we need to resume the controller first, eg
open clocks, disable some wakeup settings, etc. After that, the
controller can receive the normal USB interrupts.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci.h   |    1 +
 drivers/usb/chipidea/core.c |   22 +++++++++++++++++++++-
 drivers/usb/chipidea/otg.c  |    5 +++++
 3 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 6c16d11..cc94d17 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -175,6 +175,7 @@ struct ci_hdrc {
 	bool				b_sess_valid_event;
 	bool				supports_runtime_pm;
 	atomic_t			in_lpm;
+	bool				wakeup_int;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci_hdrc *ci)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 7ffb8cb..d8c245b 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -190,6 +190,13 @@ static void ci_hdrc_enter_lpm(struct ci_hdrc *ci, bool enable)
 		 * than 1ms) to leave low power mode.
 		 */
 		usleep_range(1500, 2000);
+	} else if (!enable) {
+		/*
+		 * At wakeup interrupt, the phcd will be cleared by hardware
+		 * automatically, but the controller needs at least 1ms
+		 * to reflect PHY's status.
+		 */
+		usleep_range(1200, 1800);
 	}
 }
 
@@ -355,6 +362,13 @@ static irqreturn_t ci_irq(int irq, void *data)
 	irqreturn_t ret = IRQ_NONE;
 	u32 otgsc = 0;
 
+	if (atomic_read(&ci->in_lpm)) {
+		disable_irq_nosync(irq);
+		ci->wakeup_int = true;
+		pm_runtime_get(ci->dev);
+		return IRQ_HANDLED;
+	}
+
 	if (ci->is_otg)
 		otgsc = hw_read(ci, OP_OTGSC, ~0);
 
@@ -737,7 +751,13 @@ static int ci_controller_resume(struct device *dev)
 		usb_phy_set_wakeup(ci->transceiver, false);
 	}
 
-	atomic_set(&ci->in_lpm, 0);
+	if (ci->wakeup_int) {
+		ci->wakeup_int = false;
+		atomic_set(&ci->in_lpm, 0);
+		enable_irq(ci->irq);
+		pm_runtime_put(ci->dev);
+	} else
+		atomic_set(&ci->in_lpm, 0);
 
 	return 0;
 }
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 39bd7ec..54bc7c0 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -78,10 +78,15 @@ static void ci_otg_work(struct work_struct *work)
 
 	if (ci->id_event) {
 		ci->id_event = false;
+		/* Keep controller active during id switch */
+		pm_runtime_get_sync(ci->dev);
 		ci_handle_id_switch(ci);
+		pm_runtime_put_sync(ci->dev);
 	} else if (ci->b_sess_valid_event) {
 		ci->b_sess_valid_event = false;
+		pm_runtime_get_sync(ci->dev);
 		ci_handle_vbus_change(ci);
+		pm_runtime_put_sync(ci->dev);
 	} else
 		dev_err(ci->dev, "unexpected event occurs at %s\n", __func__);
 
-- 
1.7.1

[PATCH 05/11] usb: chipidea: usbmisc_imx: add set_wakup API

[Peter Chen]

It is used to enable USB wakeup

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.h |    1 +
 drivers/usb/chipidea/usbmisc_imx.c |   41 ++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h b/drivers/usb/chipidea/ci_hdrc_imx.h
index c727159..92f4c30 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -18,3 +18,4 @@ struct imx_usbmisc_data {
 
 int imx_usbmisc_init(struct imx_usbmisc_data *);
 int imx_usbmisc_init_post(struct imx_usbmisc_data *);
+int imx_usbmisc_set_wakeup(struct imx_usbmisc_data *, bool);
diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
index 1fd9a12..55b59e0 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -28,12 +28,18 @@
 #define MX53_BM_OVER_CUR_DIS_UHx	BIT(30)
 
 #define MX6_BM_OVER_CUR_DIS		BIT(7)
+#define MX6_BM_WAKEUP_ENABLE		BIT(10)
+#define MX6_BM_ID_WAKEUP		BIT(16)
+#define MX6_BM_VBUS_WAKEUP		BIT(17)
+#define MX6_BM_WAKEUP_INTR		BIT(31)
 
 struct usbmisc_ops {
 	/* It's called once when probe a usb device */
 	int (*init)(struct imx_usbmisc_data *data);
 	/* It's called once after adding a usb device */
 	int (*post)(struct imx_usbmisc_data *data);
+	/* It's called when we need to enable usb wakeup */
+	int (*set_wakeup)(struct imx_usbmisc_data *data, bool enabled);
 };
 
 struct imx_usbmisc {
@@ -122,6 +128,30 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data *data)
 	return 0;
 }
 
+static int usbmisc_imx6q_set_wakeup
+	(struct imx_usbmisc_data *data, bool enabled)
+{
+	unsigned long flags;
+	u32 reg, val = (MX6_BM_WAKEUP_ENABLE | MX6_BM_VBUS_WAKEUP
+		| MX6_BM_ID_WAKEUP);
+
+	if (data->index > 3)
+		return -EINVAL;
+
+	spin_lock_irqsave(&usbmisc->lock, flags);
+	reg = readl(usbmisc->base + data->index * 4);
+	if (enabled) {
+		writel(reg | val, usbmisc->base + data->index * 4);
+	} else {
+		if (reg & MX6_BM_WAKEUP_INTR)
+			pr_debug("wakeup int at ci_hdrc.%d\n", data->index);
+		writel(reg & ~val, usbmisc->base + data->index * 4);
+	}
+	spin_unlock_irqrestore(&usbmisc->lock, flags);
+
+	return 0;
+}
+
 static const struct usbmisc_ops imx25_usbmisc_ops = {
 	.post = usbmisc_imx25_post,
 };
@@ -132,6 +162,7 @@ static const struct usbmisc_ops imx53_usbmisc_ops = {
 
 static const struct usbmisc_ops imx6q_usbmisc_ops = {
 	.init = usbmisc_imx6q_init,
+	.set_wakeup = usbmisc_imx6q_set_wakeup,
 };
 
 int imx_usbmisc_init(struct imx_usbmisc_data *data)
@@ -154,6 +185,16 @@ int imx_usbmisc_init_post(struct imx_usbmisc_data *data)
 }
 EXPORT_SYMBOL_GPL(imx_usbmisc_init_post);
 
+int imx_usbmisc_set_wakeup(struct imx_usbmisc_data *data, bool enabled)
+{
+	if (!usbmisc)
+		return -ENODEV;
+	if (!usbmisc->ops->set_wakeup)
+		return 0;
+	return usbmisc->ops->set_wakeup(data, enabled);
+}
+EXPORT_SYMBOL_GPL(imx_usbmisc_set_wakeup);
+
 static const struct of_device_id usbmisc_imx_dt_ids[] = {
 	{
 		.compatible = "fsl,imx25-usbmisc",
-- 
1.7.1

[PATCH 06/11] usb: chipidea: imx: call set_wakeup when necessary

[Peter Chen]

- Disable wakeup after probe
- Enable wakeup during the suspend
- Disable wakeup after controller is active

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 9f66f93..0424cbf 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -157,6 +157,15 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (data->usbmisc_data) {
+		ret = imx_usbmisc_set_wakeup(data->usbmisc_data, false);
+		if (ret) {
+			dev_err(&pdev->dev, "usbmisc set_wakeup failed, ret=%d\n",
+					ret);
+			goto disable_device;
+		}
+	}
+
 	platform_set_drvdata(pdev, data);
 
 	device_set_wakeup_capable(&pdev->dev, true);
@@ -194,12 +203,23 @@ static int ci_hdrc_imx_remove(struct platform_device *pdev)
 static int imx_controller_suspend(struct device *dev)
 {
 	struct ci_hdrc_imx_data *data = dev_get_drvdata(dev);
+	int ret;
 
 	dev_dbg(dev, "at %s\n", __func__);
 
 	if (atomic_read(&data->in_lpm))
 		return 0;
 
+	if (data->usbmisc_data) {
+		ret = imx_usbmisc_set_wakeup(data->usbmisc_data, true);
+		if (ret) {
+			dev_err(dev,
+				"usbmisc set_wakeup failed, ret=%d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	clk_disable_unprepare(data->clk);
 
 	atomic_set(&data->in_lpm, 1);
@@ -221,8 +241,24 @@ static int imx_controller_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	if (data->usbmisc_data) {
+		ret = imx_usbmisc_set_wakeup(data->usbmisc_data, false);
+		if (ret) {
+			dev_err(dev,
+				"usbmisc set_wakeup failed, ret=%d\n",
+				ret);
+			ret = -EINVAL;
+			goto clk_disable;
+		}
+	}
+
 	atomic_set(&data->in_lpm, 0);
 
+	return 0;
+
+clk_disable:
+	clk_disable_unprepare(data->clk);
+
 	return ret;
 }
 
-- 
1.7.1

[PATCH 07/11] usb: chipidea: host: add quirk for ehci operation

[Peter Chen]

For chipidea controller, it does not follow ehci spec strictly.
Taking resume signal as an example, it will stop resume signal about
20-21ms later automatically, but standard ehci spec says, the resume
signal is controlled by software (clear portsc.PORT_RESUME).

This operation causes some remote wakeup problems for high speed
devices due to host controller does not send SOF in time since
software can't guarantee set run/stop bit in time (run/stop bit
was cleared at the ehci suspend routine).

When software sets run/stop bit, it needs 1 SoF time to make it effect.
If we close the PHY clock just after setting run/stop bit, it does
not be set in practice, so a software delay is needed.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/host.c |   51 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 6f96795..cbe95fc 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -33,6 +33,53 @@
 #include "host.h"
 
 static struct hc_driver __read_mostly ci_ehci_hc_driver;
+static int (*orig_bus_suspend)(struct usb_hcd *hcd);
+
+static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
+{
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	int port;
+	u32 tmp;
+
+	int ret = orig_bus_suspend(hcd);
+
+	if (ret)
+		return ret;
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem *reg = &ehci->regs->port_status[port];
+		u32 portsc = ehci_readl(ehci, reg);
+
+		if (portsc & PORT_CONNECT) {
+			/*
+			 * For chipidea, the resume signal will be ended
+			 * automatically, so for remote wakeup case, the
+			 * usbcmd.rs may not be set before the resume has
+			 * ended if other resume path consumes too much
+			 * time (~23ms-24ms), in that case, the SOF will not
+			 * send out within 3ms after resume ends, then the
+			 * device will enter suspend again.
+			 */
+			if (hcd->self.root_hub->do_remote_wakeup) {
+				ehci_dbg(ehci,
+					"Remote wakeup is enabled, "
+					"and device is on the port\n");
+
+				tmp = ehci_readl(ehci, &ehci->regs->command);
+				tmp |= CMD_RUN;
+				ehci_writel(ehci, tmp, &ehci->regs->command);
+				/*
+				 * It needs a short delay between set RUNSTOP
+				 * and set PHCD.
+				 */
+				udelay(125);
+			}
+		}
+	}
+
+	return 0;
+}
 
 static irqreturn_t host_irq(struct ci_hdrc *ci)
 {
@@ -132,5 +179,9 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
 
 	ehci_init_driver(&ci_ehci_hc_driver, NULL);
 
+	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
+
+	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
+
 	return 0;
 }
-- 
1.7.1

[PATCH 08/11] usb: chipidea: host: add ehci quirk for imx controller

[Peter Chen]

When the port goes to suspend or finishes resme, it needs to
notify PHY, it is not a standard EHCI operation, so we add a
quirk for it.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/host.c  |  129 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/chipidea.h |    1 +
 2 files changed, 130 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index cbe95fc..42a0bd4 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -34,6 +34,10 @@
 
 static struct hc_driver __read_mostly ci_ehci_hc_driver;
 static int (*orig_bus_suspend)(struct usb_hcd *hcd);
+static int (*orig_bus_resume)(struct usb_hcd *hcd);
+static int (*orig_hub_control)(struct usb_hcd *hcd,
+				u16 typeReq, u16 wValue, u16 wIndex,
+				char *buf, u16 wLength);
 
 static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
 {
@@ -75,12 +79,131 @@ static int ci_ehci_bus_suspend(struct usb_hcd *hcd)
 				 */
 				udelay(125);
 			}
+			if (hcd->phy && test_bit(port, &ehci->bus_suspended)
+				&& (ehci_port_speed(ehci, portsc) ==
+					USB_PORT_STAT_HIGH_SPEED))
+				/*
+				 * notify the USB PHY, it is for global
+				 * suspend case.
+				 */
+				usb_phy_notify_suspend(hcd->phy,
+					USB_SPEED_HIGH);
 		}
 	}
 
 	return 0;
 }
 
+static int ci_imx_ehci_bus_resume(struct usb_hcd *hcd)
+{
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	int port;
+
+	int ret = orig_bus_resume(hcd);
+
+	if (ret)
+		return ret;
+
+	port = HCS_N_PORTS(ehci->hcs_params);
+	while (port--) {
+		u32 __iomem *reg = &ehci->regs->port_status[port];
+		u32 portsc = ehci_readl(ehci, reg);
+		/*
+		 * Notify PHY after resume signal has finished, it is
+		 * for global suspend case.
+		 */
+		if (hcd->phy
+			&& test_bit(port, &ehci->bus_suspended)
+			&& (portsc & PORT_CONNECT)
+			&& (ehci_port_speed(ehci, portsc) ==
+				USB_PORT_STAT_HIGH_SPEED))
+			/* notify the USB PHY */
+			usb_phy_notify_resume(hcd->phy, USB_SPEED_HIGH);
+	}
+
+	return 0;
+}
+
+/* The below code is based on tegra ehci driver */
+static int ci_imx_ehci_hub_control(
+	struct usb_hcd	*hcd,
+	u16		typeReq,
+	u16		wValue,
+	u16		wIndex,
+	char		*buf,
+	u16		wLength
+)
+{
+	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
+	u32 __iomem	*status_reg;
+	u32		temp;
+	unsigned long	flags;
+	int		retval = 0;
+
+	status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1];
+
+	spin_lock_irqsave(&ehci->lock, flags);
+
+	if (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_SUSPEND) {
+		temp = ehci_readl(ehci, status_reg);
+		if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) != 0) {
+			retval = -EPIPE;
+			goto done;
+		}
+
+		temp &= ~(PORT_RWC_BITS | PORT_WKCONN_E);
+		temp |= PORT_WKDISC_E | PORT_WKOC_E;
+		ehci_writel(ehci, temp | PORT_SUSPEND, status_reg);
+
+		/*
+		 * If a transaction is in progress, there may be a delay in
+		 * suspending the port. Poll until the port is suspended.
+		 */
+		if (ehci_handshake(ehci, status_reg, PORT_SUSPEND,
+						PORT_SUSPEND, 5000))
+			ehci_err(ehci, "timeout waiting for SUSPEND\n");
+
+		spin_unlock_irqrestore(&ehci->lock, flags);
+		if (ehci_port_speed(ehci, temp) ==
+				USB_PORT_STAT_HIGH_SPEED && hcd->phy) {
+			/* notify the USB PHY */
+			usb_phy_notify_suspend(hcd->phy, USB_SPEED_HIGH);
+		}
+		spin_lock_irqsave(&ehci->lock, flags);
+
+		set_bit((wIndex & 0xff) - 1, &ehci->suspended_ports);
+		goto done;
+	}
+
+	/*
+	 * After resume has finished, it needs do some post resume
+	 * operation for some SoCs.
+	 */
+	else if (typeReq == ClearPortFeature &&
+					wValue == USB_PORT_FEAT_C_SUSPEND) {
+
+		/* Make sure the resume has finished, it should be finished */
+		if (ehci_handshake(ehci, status_reg, PORT_RESUME, 0, 25000))
+			ehci_err(ehci, "timeout waiting for resume\n");
+
+		temp = ehci_readl(ehci, status_reg);
+
+		if (ehci_port_speed(ehci, temp) ==
+				USB_PORT_STAT_HIGH_SPEED && hcd->phy) {
+			/* notify the USB PHY */
+			usb_phy_notify_resume(hcd->phy, USB_SPEED_HIGH);
+		}
+	}
+
+	spin_unlock_irqrestore(&ehci->lock, flags);
+
+	/* Handle the hub control events here */
+	return orig_hub_control(hcd, typeReq, wValue, wIndex, buf, wLength);
+done:
+	spin_unlock_irqrestore(&ehci->lock, flags);
+	return retval;
+}
+
 static irqreturn_t host_irq(struct ci_hdrc *ci)
 {
 	return usb_hcd_irq(ci->irq, ci->hcd);
@@ -180,8 +303,14 @@ int ci_hdrc_host_init(struct ci_hdrc *ci)
 	ehci_init_driver(&ci_ehci_hc_driver, NULL);
 
 	orig_bus_suspend = ci_ehci_hc_driver.bus_suspend;
+	orig_bus_resume = ci_ehci_hc_driver.bus_resume;
+	orig_hub_control = ci_ehci_hc_driver.hub_control;
 
 	ci_ehci_hc_driver.bus_suspend = ci_ehci_bus_suspend;
+	if (ci->platdata->flags & CI_HDRC_IMX_EHCI_QUIRK) {
+		ci_ehci_hc_driver.bus_resume = ci_imx_ehci_bus_resume;
+		ci_ehci_hc_driver.hub_control = ci_imx_ehci_hub_control;
+	}
 
 	return 0;
 }
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 667b46c..8ce3daf 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -25,6 +25,7 @@ struct ci_hdrc_platform_data {
 	 * but otg is not supported (no register otgsc).
 	 */
 #define CI_HDRC_DUAL_ROLE_NOT_OTG	BIT(4)
+#define CI_HDRC_IMX_EHCI_QUIRK		BIT(5)
 	enum usb_dr_mode	dr_mode;
 #define CI_HDRC_CONTROLLER_RESET_EVENT		0
 #define CI_HDRC_CONTROLLER_STOPPED_EVENT	1
-- 
1.7.1

[PATCH 09/11] usb: chipidea: imx: Enable CI_HDRC_IMX_EHCI_QUIRK if the phy has notify APIs

[Peter Chen]

If the PHY has .notify_suspend and .notify_resume, it means this
imx usb controller needs to add quirks for standard ehci routine.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci_hdrc_imx.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 0424cbf..c2e0d73 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -118,6 +118,11 @@ static int ci_hdrc_imx_probe(struct platform_device *pdev)
 
 	pdata.phy = data->phy;
 
+	/* notify APIs are used for adding quirks at standard EHCI routine */
+	if (pdata.phy && pdata.phy->notify_suspend
+			&& pdata.phy->notify_resume)
+		pdata.flags |= CI_HDRC_IMX_EHCI_QUIRK;
+
 	if (of_find_property(np, "supports_runtime_pm", NULL)) {
 		pdata.flags |= CI_HDRC_SUPPORTS_RUNTIME_PM;
 		data->supports_runtime_pm = true;
-- 
1.7.1

[PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm

[Peter Chen]

Add property for supporting runtime power management

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
index b4b5b79..f666598 100644
--- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
+++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
@@ -18,6 +18,7 @@ Optional properties:
 - vbus-supply: regulator for vbus
 - disable-over-current: disable over current detect
 - external-vbus-divider: enables off-chip resistor divider for Vbus
+- supports_runtime_pm: enable runtime pm support
 
 Examples:
 usb at 02184000 { /* USB OTG */
@@ -28,4 +29,5 @@ usb at 02184000 { /* USB OTG */
 	fsl,usbmisc = <&usbmisc 0>;
 	disable-over-current;
 	external-vbus-divider;
+	supports_runtime_pm;
 };
-- 
1.7.1

[PATCH 11/11] ARM: dts: imx6qdl-sabresd: Enable runtime pm for usbotg and usb host 1

[Peter Chen]

Enable runtime power management support for usbotg and usb host 1.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 arch/arm/boot/dts/imx6qdl-sabresd.dtsi |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index 64e454b..c8c57db 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -215,6 +215,7 @@
 
 &usbh1 {
 	vbus-supply = <&reg_usb_h1_vbus>;
+	supports_runtime_pm;
 	status = "okay";
 };
 
@@ -223,6 +224,7 @@
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_usbotg_2>;
 	disable-over-current;
+	supports_runtime_pm;
 	status = "okay";
 };
 
-- 
1.7.1

[PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm

[Alan Stern]

On Sat, 12 Oct 2013, Peter Chen wrote:

> Add property for supporting runtime power management
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index b4b5b79..f666598 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -18,6 +18,7 @@ Optional properties:
>  - vbus-supply: regulator for vbus
>  - disable-over-current: disable over current detect
>  - external-vbus-divider: enables off-chip resistor divider for Vbus
> +- supports_runtime_pm: enable runtime pm support
>  
>  Examples:
>  usb at 02184000 { /* USB OTG */
> @@ -28,4 +29,5 @@ usb at 02184000 { /* USB OTG */
>  	fsl,usbmisc = <&usbmisc 0>;
>  	disable-over-current;
>  	external-vbus-divider;
> +	supports_runtime_pm;
>  };

This does not sound like a property of the hardware.  What's the 
_hardware_ difference between parts that support runtime PM and parts 
that don't?

Alan Stern

[PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm

[Peter Chen]

On Sat, Oct 12, 2013 at 10:40:37AM -0400, Alan Stern wrote:
> On Sat, 12 Oct 2013, Peter Chen wrote:
> 
> > Add property for supporting runtime power management
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > index b4b5b79..f666598 100644
> > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > @@ -18,6 +18,7 @@ Optional properties:
> >  - vbus-supply: regulator for vbus
> >  - disable-over-current: disable over current detect
> >  - external-vbus-divider: enables off-chip resistor divider for Vbus
> > +- supports_runtime_pm: enable runtime pm support
> >  
> >  Examples:
> >  usb at 02184000 { /* USB OTG */
> > @@ -28,4 +29,5 @@ usb at 02184000 { /* USB OTG */
> >  	fsl,usbmisc = <&usbmisc 0>;
> >  	disable-over-current;
> >  	external-vbus-divider;
> > +	supports_runtime_pm;
> >  };
> 
> This does not sound like a property of the hardware.  What's the 
> _hardware_ difference between parts that support runtime PM and parts 
> that don't?

Thanks.

>From my point, all hardware using chipidea core should support runtime pm.
But some of platforms need special glue layer operations to support
it, it will break other platforms if enable chipidea core runtime pm.
Since device tree describes hardware property, maybe I should move
it to glue layer, or do you have any suggestions?

-- 

Best Regards,
Peter Chen

[PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm

[Peter Chen]

On Mon, Oct 14, 2013 at 03:39:36AM +0200, Marek Vasut wrote:
> Dear Peter Chen,
> 
> > On Sat, Oct 12, 2013 at 10:40:37AM -0400, Alan Stern wrote:
> > > On Sat, 12 Oct 2013, Peter Chen wrote:
> > > > Add property for supporting runtime power management
> > > > 
> > > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > > ---
> > > > 
> > > >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 ++
> > > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt index
> > > > b4b5b79..f666598 100644
> > > > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > > 
> > > > @@ -18,6 +18,7 @@ Optional properties:
> > > >  - vbus-supply: regulator for vbus
> > > >  - disable-over-current: disable over current detect
> > > >  - external-vbus-divider: enables off-chip resistor divider for Vbus
> > > > 
> > > > +- supports_runtime_pm: enable runtime pm support
> > > > 
> > > >  Examples:
> > > >  usb at 02184000 { /* USB OTG */
> > > > 
> > > > @@ -28,4 +29,5 @@ usb at 02184000 { /* USB OTG */
> > > > 
> > > >  	fsl,usbmisc = <&usbmisc 0>;
> > > >  	disable-over-current;
> > > >  	external-vbus-divider;
> > > > 
> > > > +	supports_runtime_pm;
> > > > 
> > > >  };
> > > 
> > > This does not sound like a property of the hardware.  What's the
> > > _hardware_ difference between parts that support runtime PM and parts
> > > that don't?
> > 
> > Thanks.
> > 
> > From my point, all hardware using chipidea core should support runtime pm.
> > But some of platforms need special glue layer operations to support
> > it, it will break other platforms if enable chipidea core runtime pm.
> > Since device tree describes hardware property, maybe I should move
> > it to glue layer, or do you have any suggestions?
> 
> You should certainly move it out of DT. This is linux-specific property, it has 
> nothing to do with HW. The best course of action would be to fix those platforms 
> that are broken by runtime PM.
> 

Thanks, I move it out of DT. But I can only verify it at i.mx platform,
I will add a flag at glue layer.

-- 

Best Regards,
Peter Chen

[PATCH 10/11] usb: chipidea: imx: add binding for supporting runtime pm

[Marek Vasut]

Dear Peter Chen,

> On Sat, Oct 12, 2013 at 10:40:37AM -0400, Alan Stern wrote:
> > On Sat, 12 Oct 2013, Peter Chen wrote:
> > > Add property for supporting runtime power management
> > > 
> > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > ---
> > > 
> > >  .../devicetree/bindings/usb/ci13xxx-imx.txt        |    2 ++
> > >  1 files changed, 2 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt index
> > > b4b5b79..f666598 100644
> > > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> > > 
> > > @@ -18,6 +18,7 @@ Optional properties:
> > >  - vbus-supply: regulator for vbus
> > >  - disable-over-current: disable over current detect
> > >  - external-vbus-divider: enables off-chip resistor divider for Vbus
> > > 
> > > +- supports_runtime_pm: enable runtime pm support
> > > 
> > >  Examples:
> > >  usb at 02184000 { /* USB OTG */
> > > 
> > > @@ -28,4 +29,5 @@ usb at 02184000 { /* USB OTG */
> > > 
> > >  	fsl,usbmisc = <&usbmisc 0>;
> > >  	disable-over-current;
> > >  	external-vbus-divider;
> > > 
> > > +	supports_runtime_pm;
> > > 
> > >  };
> > 
> > This does not sound like a property of the hardware.  What's the
> > _hardware_ difference between parts that support runtime PM and parts
> > that don't?
> 
> Thanks.
> 
> From my point, all hardware using chipidea core should support runtime pm.
> But some of platforms need special glue layer operations to support
> it, it will break other platforms if enable chipidea core runtime pm.
> Since device tree describes hardware property, maybe I should move
> it to glue layer, or do you have any suggestions?

You should certainly move it out of DT. This is linux-specific property, it has 
nothing to do with HW. The best course of action would be to fix those platforms 
that are broken by runtime PM.

Best regards,
Marek Vasut

[PATCH 01/11] usb: chipidea: Add power management support

[Peter Chen]

On Mon, Oct 14, 2013 at 10:04:58AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> Peter Chen wrote:
> > This commit adds runtime and system power management support for
> > chipidea core. The runtime pm support is controlled by glue
> > layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
> > 
> [...]
> > +#ifdef CONFIG_PM
> > +static int ci_controller_suspend(struct device *dev)
> > +{
> > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > +
> > +	dev_dbg(dev, "at %s\n", __func__);
> > +
> > +	if (atomic_read(&ci->in_lpm))
> > +		return 0;
> > +
> What does this 'atomic_read()' buy you over just testing/assinging a
> simple integer. Note that just because the function has 'atomic' in
> its name the sequence:
> 	atomic_read();
> ...
> 	atomic_set();
> does not magically become an atomic operation.

I just want the read and set are atomic, not the operations
between atomic_read and atomic_set.

-- 

Best Regards,
Peter Chen

[PATCH 01/11] usb: chipidea: Add power management support

[Lothar Waßmann]

Hi,

Peter Chen wrote:
> This commit adds runtime and system power management support for
> chipidea core. The runtime pm support is controlled by glue
> layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
> 
[...]
> +#ifdef CONFIG_PM
> +static int ci_controller_suspend(struct device *dev)
> +{
> +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> +
> +	dev_dbg(dev, "at %s\n", __func__);
> +
> +	if (atomic_read(&ci->in_lpm))
> +		return 0;
> +
What does this 'atomic_read()' buy you over just testing/assinging a
simple integer. Note that just because the function has 'atomic' in
its name the sequence:
	atomic_read();
...
	atomic_set();
does not magically become an atomic operation.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 01/11] usb: chipidea: Add power management support

[Sascha Hauer]

On Mon, Oct 14, 2013 at 03:55:48PM +0800, Peter Chen wrote:
> On Mon, Oct 14, 2013 at 10:04:58AM +0200, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Peter Chen wrote:
> > > This commit adds runtime and system power management support for
> > > chipidea core. The runtime pm support is controlled by glue
> > > layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
> > > 
> > [...]
> > > +#ifdef CONFIG_PM
> > > +static int ci_controller_suspend(struct device *dev)
> > > +{
> > > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > +
> > > +	dev_dbg(dev, "at %s\n", __func__);
> > > +
> > > +	if (atomic_read(&ci->in_lpm))
> > > +		return 0;
> > > +
> > What does this 'atomic_read()' buy you over just testing/assinging a
> > simple integer. Note that just because the function has 'atomic' in
> > its name the sequence:
> > 	atomic_read();
> > ...
> > 	atomic_set();
> > does not magically become an atomic operation.
> 
> I just want the read and set are atomic, not the operations
> between atomic_read and atomic_set.

It makes no sense to use atomic operations here. atomic types are to
atomically read/modify/write variables. If all you ever do is
atomic_read and atomic_set then you can use a simple bool variable.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 01/11] usb: chipidea: Add power management support

[Peter Chen]

On Mon, Oct 14, 2013 at 10:42:16AM +0200, Sascha Hauer wrote:
> On Mon, Oct 14, 2013 at 03:55:48PM +0800, Peter Chen wrote:
> > On Mon, Oct 14, 2013 at 10:04:58AM +0200, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > Peter Chen wrote:
> > > > This commit adds runtime and system power management support for
> > > > chipidea core. The runtime pm support is controlled by glue
> > > > layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
> > > > 
> > > [...]
> > > > +#ifdef CONFIG_PM
> > > > +static int ci_controller_suspend(struct device *dev)
> > > > +{
> > > > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > > +
> > > > +	dev_dbg(dev, "at %s\n", __func__);
> > > > +
> > > > +	if (atomic_read(&ci->in_lpm))
> > > > +		return 0;
> > > > +
> > > What does this 'atomic_read()' buy you over just testing/assinging a
> > > simple integer. Note that just because the function has 'atomic' in
> > > its name the sequence:
> > > 	atomic_read();
> > > ...
> > > 	atomic_set();
> > > does not magically become an atomic operation.
> > 
> > I just want the read and set are atomic, not the operations
> > between atomic_read and atomic_set.
> 
> It makes no sense to use atomic operations here. atomic types are to
> atomically read/modify/write variables. If all you ever do is
> atomic_read and atomic_set then you can use a simple bool variable.
> 

It is for ARM, but for other platforms, it may not.

-- 

Best Regards,
Peter Chen

[PATCH 01/11] usb: chipidea: Add power management support

[Sascha Hauer]

On Mon, Oct 14, 2013 at 05:04:21PM +0800, Peter Chen wrote:
> On Mon, Oct 14, 2013 at 10:42:16AM +0200, Sascha Hauer wrote:
> > On Mon, Oct 14, 2013 at 03:55:48PM +0800, Peter Chen wrote:
> > > On Mon, Oct 14, 2013 at 10:04:58AM +0200, Lothar Wa?mann wrote:
> > > > Hi,
> > > > 
> > > > Peter Chen wrote:
> > > > > This commit adds runtime and system power management support for
> > > > > chipidea core. The runtime pm support is controlled by glue
> > > > > layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
> > > > > 
> > > > [...]
> > > > > +#ifdef CONFIG_PM
> > > > > +static int ci_controller_suspend(struct device *dev)
> > > > > +{
> > > > > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > > > +
> > > > > +	dev_dbg(dev, "at %s\n", __func__);
> > > > > +
> > > > > +	if (atomic_read(&ci->in_lpm))
> > > > > +		return 0;
> > > > > +
> > > > What does this 'atomic_read()' buy you over just testing/assinging a
> > > > simple integer. Note that just because the function has 'atomic' in
> > > > its name the sequence:
> > > > 	atomic_read();
> > > > ...
> > > > 	atomic_set();
> > > > does not magically become an atomic operation.
> > > 
> > > I just want the read and set are atomic, not the operations
> > > between atomic_read and atomic_set.
> > 
> > It makes no sense to use atomic operations here. atomic types are to
> > atomically read/modify/write variables. If all you ever do is
> > atomic_read and atomic_set then you can use a simple bool variable.
> > 
> 
> It is for ARM, but for other platforms, it may not.

It is for other platforms aswell. Otherwise the kernel would break into
pieces.

You only need atomic ops when you want to atomically read/modify/write a
variable.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Mon, Oct 14, 2013 at 03:55:48PM +0800, Peter Chen wrote:
> On Mon, Oct 14, 2013 at 10:04:58AM +0200, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Peter Chen wrote:
> > > This commit adds runtime and system power management support for
> > > chipidea core. The runtime pm support is controlled by glue
> > > layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
> > > 
> > [...]
> > > +#ifdef CONFIG_PM
> > > +static int ci_controller_suspend(struct device *dev)
> > > +{
> > > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > +
> > > +	dev_dbg(dev, "at %s\n", __func__);
> > > +
> > > +	if (atomic_read(&ci->in_lpm))
> > > +		return 0;
> > > +
> > What does this 'atomic_read()' buy you over just testing/assinging a
> > simple integer. Note that just because the function has 'atomic' in
> > its name the sequence:
> > 	atomic_read();
> > ...
> > 	atomic_set();
> > does not magically become an atomic operation.
> 
> I just want the read and set are atomic, not the operations
> between atomic_read and atomic_set.

There is nothing magical about atomic_read() or atomic_set():

#define atomic_read(v)  (*(volatile int *)&(v)->counter)
#define atomic_set(v,i) (((v)->counter) = (i))

You might as well just read and write the variable directly if you're
going to do this.  The atomicity of atomic_t comes from the *other*
operations which can be done on an atomic_t, not from some magical
macro which starts with the name "atomic_".

Lesson 1: whenever you see atomic_read() and atomic_set() in a patch
be very very very suspicious; it's 99% of times plainly wrong and a
sign of something being totally broken.

[PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Mon, Oct 14, 2013 at 05:04:21PM +0800, Peter Chen wrote:
> It is for ARM, but for other platforms, it may not.

Wrong.  atomic_read() and atomic_set() are defined the same way and
have the same lack of atomicity across all architectures.  There is
nothing special about these over a standard load/store.  Your code
using this is quite simply broken and founded on false assumptions
about these macros.

[PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Sat, Oct 12, 2013 at 05:35:03PM +0800, Peter Chen wrote:
> This commit adds runtime and system power management support for
> chipidea core. The runtime pm support is controlled by glue
> layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.

Let's look at the locking.

1. Runtime PM.  These callbacks are locked with a spinlock, which holds
   dev->power.lock.  This lock is taken either with or without disabling
   IRQs depending on whether runtime PM is IRQ safe or not.

2. Normal PM.  These callbacks are locked by holding dev->mutex.

Now, there's a little bit of protection between these two operations -
when normal PM places a device into a low power state, it 'gets' a
reference on the runtime PM to ensure no runtime PM transitions occur
while normal PM is active.  (See pm_runtime_get_noresume() in
device_prepare().)  This is only dropped when the normal PM resumes the
device.

Moreover, all runtime PM events are flushed before the suspend callback
occurs (see the pm_runtime_barrier() in __device_suspend()).

What that means is that you can't receive any runtime PM events while
you are in your suspend/resume callbacks.  So, each call is mutually
exclusive.

So, runtime PM callbacks vs normal PM callbacks for any single device
are all called with mutual exclusion - you won't have two running at
any time.

Hence, for the reasons stated previously about the non-atomic nature of
atomic_read()/atomic_set(), there's even more reasons that their use
here is just mere obfuscation: the accesses to this state tracking
variable is already guaranteed to be single-threaded by core code, so
the use of atomic_read()/atomic_set() just adds additional needless
confusion to this code.

[PATCH 01/11] usb: chipidea: Add power management support

[Peter Chen]

On Mon, Oct 14, 2013 at 12:01:08PM +0100, Russell King - ARM Linux wrote:
> On Sat, Oct 12, 2013 at 05:35:03PM +0800, Peter Chen wrote:
> > This commit adds runtime and system power management support for
> > chipidea core. The runtime pm support is controlled by glue
> > layer, it can be enabled by flag CI_HDRC_SUPPORTS_RUNTIME_PM.
> 
> Let's look at the locking.
> 
> 1. Runtime PM.  These callbacks are locked with a spinlock, which holds
>    dev->power.lock.  This lock is taken either with or without disabling
>    IRQs depending on whether runtime PM is IRQ safe or not.
> 
> 2. Normal PM.  These callbacks are locked by holding dev->mutex.
> 
> Now, there's a little bit of protection between these two operations -
> when normal PM places a device into a low power state, it 'gets' a
> reference on the runtime PM to ensure no runtime PM transitions occur
> while normal PM is active.  (See pm_runtime_get_noresume() in
> device_prepare().)  This is only dropped when the normal PM resumes the
> device.
> 
> Moreover, all runtime PM events are flushed before the suspend callback
> occurs (see the pm_runtime_barrier() in __device_suspend()).
> 
> What that means is that you can't receive any runtime PM events while
> you are in your suspend/resume callbacks.  So, each call is mutually
> exclusive.
> 
> So, runtime PM callbacks vs normal PM callbacks for any single device
> are all called with mutual exclusion - you won't have two running at
> any time.
> 
> Hence, for the reasons stated previously about the non-atomic nature of
> atomic_read()/atomic_set(), there's even more reasons that their use
> here is just mere obfuscation: the accesses to this state tracking
> variable is already guaranteed to be single-threaded by core code, so
> the use of atomic_read()/atomic_set() just adds additional needless
> confusion to this code.
> 

Many Thanks, Russell.

So, the lessons for this topic are:

- If one atomic variable's operation only includes one instruction like
atomic_read and atomic_set, it is not meaningful for using atomic
operation, we can just use bool instead of it.

- The runtime pm itself, normal pm itself, runtime pm and normal pm
are all already exclusion, we don't need protection for variables
who are only used at pm situation.

-- 

Best Regards,
Peter Chen

[PATCH 01/11] usb: chipidea: Add power management support

[Russell King - ARM Linux]

On Tue, Oct 15, 2013 at 10:18:15AM +0800, Peter Chen wrote:
> So, the lessons for this topic are:
> 
> - If one atomic variable's operation only includes one instruction like
> atomic_read and atomic_set, it is not meaningful for using atomic
> operation, we can just use bool instead of it.

The lesson here is that these are 100% equivalent as far as safety from
races is concerned:

	a = atomic_read(&v);		a = v->counter;

	atomic_set(&v, b);		v->counter = b;

and in general, whenever atomic_read() gets used it's almost certainly
a sign of a bug.

Consider this (similar has been submitted):

	a = atomic_read(&v);
	if (a != 0)
		a += 1;

	atomic_set(&v, a);

and people have thought that somehow this is magically safe from races
because they're using atomic_t, and somehow that saves the universe.
The above is in fact no safer than:

	a = *v;
	if (a != 0)
		a += 1;
	*v = a;

The only thing that using atomic_* does is add a false sense of security
and a level of obfuscation to catch the unwary reviewer.

The reason is quite simple: a single access read in itself is atomic.
Either it has read the value, or it hasn't.  A single access store is
itself atomic.  Either the data has been written, or it hasn't.  The
issue is _always_ what you do around it.