[Patch v2 00/10] Add power management support for chipidea

[Patch v2 00/10] 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]-"Add power management support for MXS PHY"
adds some common PHY APIs, and this serial uses it.

It has been verified at Freescale i.mx6Q/DL 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

Changes for v2:
- Do not use atomic_read and atomic_set for ci->in_lpm
- Do not use "supports_runtime_pm" as DT property due to it is
not a hardware feature, instead of it, we use compatible string
to enable it.

Peter Chen (10):
  usb: chipidea: Add power management support
  usb: chipidea: imx: add power management support
  usb: chipidea: add wakeup interrupt handler
  usb: chipidea: usbmisc_imx: remove the controller's clock information
  usb: chipidea: usbmisc_imx: add set_wakup API
  usb: chipidea: imx: call set_wakeup when necessary
  usb: chipidea: imx: Enable runtime pm support for imx6 SoC serial
  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

 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 +
 8 files changed, 508 insertions(+), 21 deletions(-)

[1]:http://marc.info/?l=linux-usb&m=138242248913823&w=2

[Patch v2 01/10] 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..6ea1892 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;
+	bool				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..a5b8f43 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 (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);
+
+	ci->in_lpm = true;
+
+	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 (!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);
+	}
+
+	ci->in_lpm = false;
+
+	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 v2 02/10] 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 |   94 ++++++++++++++++++++++++++++++++++-
 1 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
index 023d3cb..a1e5634 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;
+	bool supports_runtime_pm;
+	bool in_lpm;
 };
 
 /* 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) {
@@ -151,8 +154,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 +174,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 (data->in_lpm)
+		return 0;
+
+	clk_disable_unprepare(data->clk);
+
+	data->in_lpm = true;
+
+	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 (!data->in_lpm)
+		return 0;
+
+	ret = clk_prepare_enable(data->clk);
+	if (ret)
+		return ret;
+
+	data->in_lpm = false;
+
+	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 +274,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 v2 03/10] 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 |   20 ++++++++++++++++++++
 drivers/usb/chipidea/otg.c  |    5 +++++
 3 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 6ea1892..38598b3 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;
 	bool				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 a5b8f43..a9497eb 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 (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);
 
@@ -739,6 +753,12 @@ static int ci_controller_resume(struct device *dev)
 
 	ci->in_lpm = false;
 
+	if (ci->wakeup_int) {
+		ci->wakeup_int = false;
+		enable_irq(ci->irq);
+		pm_runtime_put(ci->dev);
+	}
+
 	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 v2 04/10] 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 v2 05/10] usb: chipidea: usbmisc_imx: add set_wakup API

[Peter Chen]

It is used to enable USB wakeup, currently only imx6 SoC serial
usb's wakeup is enabled.

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 v2 06/10] 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 a1e5634..2a14152 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -152,6 +152,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);
@@ -189,12 +198,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 (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);
 
 	data->in_lpm = true;
@@ -218,6 +238,22 @@ static int imx_controller_resume(struct device *dev)
 
 	data->in_lpm = false;
 
+	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;
+		}
+	}
+
+	return 0;
+
+clk_disable:
+	clk_disable_unprepare(data->clk);
+
 	return ret;
 }
 
-- 
1.7.1

[Patch v2 07/10] usb: chipidea: imx: Enable runtime pm support for imx6 SoC serial

[Peter Chen]

Currently, only imx6 SoC serial adds wakeup logic, so only
enable runtime pm for imx6.

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 2a14152..d4775b3 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;
 
+	if (of_device_is_compatible(np, "fsl,imx6q-usb")) {
+		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)
-- 
1.7.1

[Patch v2 08/10] 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 59e6020..283b385 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)
 {
@@ -134,5 +181,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 v2 09/10] 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 283b385..c1d05c4 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);
@@ -182,8 +305,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 v2 10/10] 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 d4775b3..4045c76 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_device_is_compatible(np, "fsl,imx6q-usb")) {
 		pdata.flags |= CI_HDRC_SUPPORTS_RUNTIME_PM;
 		data->supports_runtime_pm = true;
-- 
1.7.1

[Patch v2 08/10] usb: chipidea: host: add quirk for ehci operation

[Alan Stern]

On Tue, 22 Oct 2013, Peter Chen wrote:

> 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 59e6020..283b385 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);
> +			}
> +		}
> +	}

This code is a little strange.  If there is only one port then you 
don't need the loop.  But if there is more than one port then you don't 
want to put the udelay inside the loop.  (If two devices are attached, 
you don't want to delay the suspend by 250 us.)

Perhaps you should add a "break;" statement following the udelay.

Alan Stern

[Patch v2 09/10] usb: chipidea: host: add ehci quirk for imx controller

[Alan Stern]

On Tue, 22 Oct 2013, Peter Chen wrote:

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

Actually, this _should_ be a standard EHCI operation.  But we have to
figure out a way to do it that will work on all platforms.

Felipe, any ideas?

Alan Stern

> 
> 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 283b385..c1d05c4 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;
> +}

[Patch v2 09/10] usb: chipidea: host: add ehci quirk for imx controller

[Felipe Balbi]

Hi,

On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote:
> On Tue, 22 Oct 2013, Peter Chen wrote:
> 
> > 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.
> 
> Actually, this _should_ be a standard EHCI operation.  But we have to
> figure out a way to do it that will work on all platforms.
> 
> Felipe, any ideas?

isn't it enough to call usb_phy_set_suspend() at apropriate places ?

This should work on all platforms if the PHY driver is implemented
correctly.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131023/0d6262c5/attachment.sig>

[Patch v2 09/10] usb: chipidea: host: add ehci quirk for imx controller

[Alan Stern]

On Wed, 23 Oct 2013, Felipe Balbi wrote:

> Hi,
> 
> On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote:
> > On Tue, 22 Oct 2013, Peter Chen wrote:
> > 
> > > 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.
> > 
> > Actually, this _should_ be a standard EHCI operation.  But we have to
> > figure out a way to do it that will work on all platforms.
> > 
> > Felipe, any ideas?
> 
> isn't it enough to call usb_phy_set_suspend() at apropriate places ?
> 
> This should work on all platforms if the PHY driver is implemented
> correctly.

On Tue, 22 Oct 2013, Peter Chen wrote:

> +				/*
> +				 * notify the USB PHY, it is for global
> +				 * suspend case.
> +				 */
> +				usb_phy_notify_suspend(hcd->phy,
> +					USB_SPEED_HIGH);

Hmmm.  This calls usb_phy_notify_suspend(), and later on, 
usb_phy_notify_resume().  AFAICT, those routines don't exist.

Instead we have usb_phy_set_suspend(), as Felipe mentioned.  It has a
different interface, because the second argument specifies whether we
are entering or leaving suspend, not the connection speed.

Peter, you need to straighten this out.

Also, there's the question of where are the appropriate places to make
the calls?  After the root hub goes into suspend, I suppose.  But when
is the right time during resume?

And what if there is more than one port on the root hub?

Alan Stern

[Patch v2 08/10] usb: chipidea: host: add quirk for ehci operation

[Peter Chen]

On Wed, Oct 23, 2013 at 10:42:58AM -0400, Alan Stern wrote:
> On Tue, 22 Oct 2013, Peter Chen wrote:
> 
> > 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 59e6020..283b385 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);
> > +			}
> > +		}
> > +	}
> 
> This code is a little strange.  If there is only one port then you 
> don't need the loop.  But if there is more than one port then you don't 
> want to put the udelay inside the loop.  (If two devices are attached, 
> you don't want to delay the suspend by 250 us.)
> 

Yes, you are right. I will add break, that is once there is port connected,
delay 125us, then quit while.

-- 

Best Regards,
Peter Chen

[Patch v2 09/10] usb: chipidea: host: add ehci quirk for imx controller

[Peter Chen]

On Wed, Oct 23, 2013 at 02:45:39PM -0400, Alan Stern wrote:
> On Wed, 23 Oct 2013, Felipe Balbi wrote:
> 
> > Hi,
> > 
> > On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote:
> > > On Tue, 22 Oct 2013, Peter Chen wrote:
> > > 
> > > > 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.
> > > 
> > > Actually, this _should_ be a standard EHCI operation.  But we have to
> > > figure out a way to do it that will work on all platforms.
> > > 
> > > Felipe, any ideas?
> > 
> > isn't it enough to call usb_phy_set_suspend() at apropriate places ?
> > 
> > This should work on all platforms if the PHY driver is implemented
> > correctly.
> 
> On Tue, 22 Oct 2013, Peter Chen wrote:
> 
> > +				/*
> > +				 * notify the USB PHY, it is for global
> > +				 * suspend case.
> > +				 */
> > +				usb_phy_notify_suspend(hcd->phy,
> > +					USB_SPEED_HIGH);
> 
> Hmmm.  This calls usb_phy_notify_suspend(), and later on, 
> usb_phy_notify_resume().  AFAICT, those routines don't exist.
> 

Hi Alan and Felipe,

It is at my another patchset: "Add power management support for MXS PHY"
http://marc.info/?l=linux-usb&m=138242248913823&w=2
Since they are two things, one is from PHY, the other is for controller.
I split them to two patchset.

For these two notifications, please see:
[Patch v2 07/14] usb: phy: add notify suspend and resume callback
http://marc.info/?l=linux-usb&m=138242252713848&w=2

[Patch v2 08/14] usb: phy-mxs: Add implementation of
nofity_suspend and notify_resume
http://marc.info/?l=linux-usb&m=138242253313856&w=2

> Instead we have usb_phy_set_suspend(), as Felipe mentioned.  It has a
> different interface, because the second argument specifies whether we
> are entering or leaving suspend, not the connection speed.
> 
> Peter, you need to straighten this out.

.set_suspend is different with .notify_suspend.

.set_suspend is called when the PHY enters suspend.
.notify_suspend is called when port enters suspends (setting portsc.suspendM,
and stop sending SOF).

After calling .notify_suspend, the PHY may still need to access,
eg, set PHY wakeup setting, etc.

The .notify_suspend is in ehci operation, .set_suspend can be at
controller driver, since when to put the PHY enters suspend may
platform specific.

> 
> Also, there's the question of where are the appropriate places to make
> the calls?  After the root hub goes into suspend, I suppose.  But when
> is the right time during resume?
> 

If we can take it as standard EHCI operation, we need to put it at below
places:
	
For .notify_suspend, we need to put it after set portsc.suspendM.
- For selective suspend, at ehci_hub_control
- For global suspend, at ehci_bus_suspend

For .notify_resume, we need to put it after clear portsc.fpr,
it is similar for .notify_suspend
- For selective suspend, at ehci_hub_control
- For global suspend, at ehci_bus_resume

-- 

Best Regards,
Peter Chen

[Patch v2 08/10] usb: chipidea: host: add quirk for ehci operation

[Peter Chen]

On Thu, Oct 24, 2013 at 07:51:03AM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> Peter Chen wrote:
> > 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 59e6020..283b385 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");
> >
> Please don't line wrap message texts, since it makes it harder to grep
> the kernel source for messages found in a logfile.
> 
> 

Thanks, will change.

-- 

Best Regards,
Peter Chen

[Patch v2 08/10] usb: chipidea: host: add quirk for ehci operation

[Lothar Waßmann]

Hi,

Peter Chen wrote:
> 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 59e6020..283b385 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");
>
Please don't line wrap message texts, since it makes it harder to grep
the kernel source for messages found in a logfile.


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 v2 09/10] usb: chipidea: host: add ehci quirk for imx controller

[Felipe Balbi]

Hi,

On Thu, Oct 24, 2013 at 09:33:26AM +0800, Peter Chen wrote:
> On Wed, Oct 23, 2013 at 02:45:39PM -0400, Alan Stern wrote:
> > On Wed, 23 Oct 2013, Felipe Balbi wrote:
> > 
> > > Hi,
> > > 
> > > On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote:
> > > > On Tue, 22 Oct 2013, Peter Chen wrote:
> > > > 
> > > > > 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.
> > > > 
> > > > Actually, this _should_ be a standard EHCI operation.  But we have to
> > > > figure out a way to do it that will work on all platforms.
> > > > 
> > > > Felipe, any ideas?
> > > 
> > > isn't it enough to call usb_phy_set_suspend() at apropriate places ?
> > > 
> > > This should work on all platforms if the PHY driver is implemented
> > > correctly.
> > 
> > On Tue, 22 Oct 2013, Peter Chen wrote:
> > 
> > > +				/*
> > > +				 * notify the USB PHY, it is for global
> > > +				 * suspend case.
> > > +				 */
> > > +				usb_phy_notify_suspend(hcd->phy,
> > > +					USB_SPEED_HIGH);
> > 
> > Hmmm.  This calls usb_phy_notify_suspend(), and later on, 
> > usb_phy_notify_resume().  AFAICT, those routines don't exist.
> > 
> 
> Hi Alan and Felipe,
> 
> It is at my another patchset: "Add power management support for MXS PHY"
> http://marc.info/?l=linux-usb&m=138242248913823&w=2
> Since they are two things, one is from PHY, the other is for controller.
> I split them to two patchset.
> 
> For these two notifications, please see:
> [Patch v2 07/14] usb: phy: add notify suspend and resume callback
> http://marc.info/?l=linux-usb&m=138242252713848&w=2
> 
> [Patch v2 08/14] usb: phy-mxs: Add implementation of
> nofity_suspend and notify_resume
> http://marc.info/?l=linux-usb&m=138242253313856&w=2
> 
> > Instead we have usb_phy_set_suspend(), as Felipe mentioned.  It has a
> > different interface, because the second argument specifies whether we
> > are entering or leaving suspend, not the connection speed.
> > 
> > Peter, you need to straighten this out.
> 
> .set_suspend is different with .notify_suspend.
> 
> .set_suspend is called when the PHY enters suspend.
> .notify_suspend is called when port enters suspends (setting portsc.suspendM,
> and stop sending SOF).
> 
> After calling .notify_suspend, the PHY may still need to access,
> eg, set PHY wakeup setting, etc.

right, this just means we need usage counters for the PHY layer. So that
usb_phy_set_suspend() only decreaments a counter and will only suspend
the PHY when said counter reaches zero.

Also, why can't you *always* setup wakeup events for the PHY ? Everytime
you're about to suspend the PHY, enable its wakeup events.

> The .notify_suspend is in ehci operation, .set_suspend can be at
> controller driver, since when to put the PHY enters suspend may
> platform specific.

not really, what's platform specific is *HOW* to suspend the PHY.
*WHERE* to tell it to suspend is really more of a policy decision.

> > Also, there's the question of where are the appropriate places to make
> > the calls?  After the root hub goes into suspend, I suppose.  But when
> > is the right time during resume?
> > 
> 
> If we can take it as standard EHCI operation, we need to put it at below
> places:
> 	
> For .notify_suspend, we need to put it after set portsc.suspendM.

why do you need this notification for suspendM ? PHY should
automatically enter a lower power state automatically when suspendM
signal is asserted.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131024/182e5a77/attachment-0001.sig>

[Patch v2 09/10] usb: chipidea: host: add ehci quirk for imx controller

[Peter Chen]

On Thu, Oct 24, 2013 at 07:04:11AM -0500, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Oct 24, 2013 at 09:33:26AM +0800, Peter Chen wrote:
> > On Wed, Oct 23, 2013 at 02:45:39PM -0400, Alan Stern wrote:
> > > On Wed, 23 Oct 2013, Felipe Balbi wrote:
> > > 
> > > > Hi,
> > > > 
> > > > On Wed, Oct 23, 2013 at 10:46:09AM -0400, Alan Stern wrote:
> > > > > On Tue, 22 Oct 2013, Peter Chen wrote:
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > Actually, this _should_ be a standard EHCI operation.  But we have to
> > > > > figure out a way to do it that will work on all platforms.
> > > > > 
> > > > > Felipe, any ideas?
> > > > 
> > > > isn't it enough to call usb_phy_set_suspend() at apropriate places ?
> > > > 
> > > > This should work on all platforms if the PHY driver is implemented
> > > > correctly.
> > > 
> > > On Tue, 22 Oct 2013, Peter Chen wrote:
> > > 
> > > > +				/*
> > > > +				 * notify the USB PHY, it is for global
> > > > +				 * suspend case.
> > > > +				 */
> > > > +				usb_phy_notify_suspend(hcd->phy,
> > > > +					USB_SPEED_HIGH);
> > > 
> > > Hmmm.  This calls usb_phy_notify_suspend(), and later on, 
> > > usb_phy_notify_resume().  AFAICT, those routines don't exist.
> > > 
> > 
> > Hi Alan and Felipe,
> > 
> > It is at my another patchset: "Add power management support for MXS PHY"
> > http://marc.info/?l=linux-usb&m=138242248913823&w=2
> > Since they are two things, one is from PHY, the other is for controller.
> > I split them to two patchset.
> > 
> > For these two notifications, please see:
> > [Patch v2 07/14] usb: phy: add notify suspend and resume callback
> > http://marc.info/?l=linux-usb&m=138242252713848&w=2
> > 
> > [Patch v2 08/14] usb: phy-mxs: Add implementation of
> > nofity_suspend and notify_resume
> > http://marc.info/?l=linux-usb&m=138242253313856&w=2
> > 
> > > Instead we have usb_phy_set_suspend(), as Felipe mentioned.  It has a
> > > different interface, because the second argument specifies whether we
> > > are entering or leaving suspend, not the connection speed.
> > > 
> > > Peter, you need to straighten this out.
> > 
> > .set_suspend is different with .notify_suspend.
> > 
> > .set_suspend is called when the PHY enters suspend.
> > .notify_suspend is called when port enters suspends (setting portsc.suspendM,
> > and stop sending SOF).
> > 
> > After calling .notify_suspend, the PHY may still need to access,
> > eg, set PHY wakeup setting, etc.
> 
> right, this just means we need usage counters for the PHY layer. So that
> usb_phy_set_suspend() only decreaments a counter and will only suspend
> the PHY when said counter reaches zero.
> 
> Also, why can't you *always* setup wakeup events for the PHY ? Everytime
> you're about to suspend the PHY, enable its wakeup events.

Most of cases, the user doesn't want USB wakeup event. At these cases,
if the PHY's wakeup is set, when external signal changes (id/vbus/dpdm),
the PHY will be woken up and more power consumption will be.

> 
> > The .notify_suspend is in ehci operation, .set_suspend can be at
> > controller driver, since when to put the PHY enters suspend may
> > platform specific.
> 
> not really, what's platform specific is *HOW* to suspend the PHY.
> *WHERE* to tell it to suspend is really more of a policy decision.
> 

How to suspend the PHY is indeed platform specific, but sometimes,
the whole suspend PHY operation may not only include PHY register, but also
needs controller register, Eg, for chipidea controller, if we have not used
chipidea PHY, we need to control both controller register (portsc.phcd)
and PHY's register (Different register mapping), so it is hard
to include two operations at usb_phy_set_suspend, we have to put them
at controller's suspend.

> > > Also, there's the question of where are the appropriate places to make
> > > the calls?  After the root hub goes into suspend, I suppose.  But when
> > > is the right time during resume?
> > > 
> > 
> > If we can take it as standard EHCI operation, we need to put it at below
> > places:
> > 	
> > For .notify_suspend, we need to put it after set portsc.suspendM.
> 
> why do you need this notification for suspendM ? PHY should
> automatically enter a lower power state automatically when suspendM
> signal is asserted.
> 

For some PHYs, it may correct. For chipidea controller, it needs to set
portsc.phcd to let the PHY enter suspend. The software needs to control it.

-- 

Best Regards,
Peter Chen