[PATCH v7 0/3] usb: cdns3: add runtime pm support

[PATCH v7 0/3] usb: cdns3: add runtime pm support

[Peter Chen]

Hi Felipe,

In this series, it adds cdns3 runtime PM support, and verified at
NXP i.MX8QM and i.MX8QXP platforms.

Changes for v7:
- Fix one coding style issue [Patch 2/3]
- Fix one sparese isse Reported-by: kernel test robot <lkp@intel.com> [Patch 3/3]

Changes for v6:
- Add Pawel's reviewed-by
- Remove xhci-plat patches, which was sent by xhci patch series [1]
- Rebased on the newest usb/next

Changes for v5:
- Address Greg's comments for more obvious PHY power controller APIs [Patch 1/2]
- One build warning from kernel test robot

Changes for v4:
- Address Jun Li's comments for cdns3 core changes [Patch 2]
- Some small fixes for cdns3-imx for CLK_125_REQ bit
- Rebase the latest usb-next

Changes for v3:
Add Jun Li’s reviewed-by [Patch 1 and Patch 6]
Add Mathias’s acked-by [Patch 4-6]
Some wakeup logic improvement [Patch 2]
Add dedicated wakeup interrupt for core, and improve the commit log [Patch]
Fix build error found by kbuild test robot [Patch 3]
Using xhci_plat_priv quirk for skip PHY initialization [patch 7, patch 9]
Some other typo and tiny improvements

Changes for v2:
- Add the 1st patch. Without it, the build on the usb-next will fail.
- Change the subject for cover letter a little to reflect all contents.

[1] https://www.spinics.net/lists/linux-usb/msg199399.html

Peter Chen (3):
  usb: cdns3: introduce set_phy_power_on{off} APIs
  usb: cdns3: add runtime PM support
  usb: cdns3: imx: add glue layer runtime pm implementation

 drivers/usb/cdns3/cdns3-imx.c | 203 ++++++++++++++++++++++++++++++++--
 drivers/usb/cdns3/core.c      | 195 ++++++++++++++++++++++++++------
 drivers/usb/cdns3/core.h      |  16 +++
 drivers/usb/cdns3/drd.c       |   3 +
 drivers/usb/cdns3/gadget.c    |   4 +
 drivers/usb/cdns3/host.c      |   7 ++
 6 files changed, 385 insertions(+), 43 deletions(-)

-- 
2.17.1

[PATCH v7 1/3] usb: cdns3: introduce set_phy_power_on{off} APIs

[Peter Chen]

Since we have both USB2 and USB3 PHYs for cdns3 controller, it is
better we have unity APIs to handle both USB2 and USB3's power, it
could simplify code for error handling and further power management
implementation.

Reviewed-by: Pawel Laszczak <pawell@cadence.com>
Reviewed-by: Jun Li <jun.li@nxp.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/core.c | 43 ++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 5c1586ec7824..e56dbb6a898c 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -371,6 +371,27 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
 	return ret;
 }
 
+static int set_phy_power_on(struct cdns3 *cdns)
+{
+	int ret;
+
+	ret = phy_power_on(cdns->usb2_phy);
+	if (ret)
+		return ret;
+
+	ret = phy_power_on(cdns->usb3_phy);
+	if (ret)
+		phy_power_off(cdns->usb2_phy);
+
+	return ret;
+}
+
+static void set_phy_power_off(struct cdns3 *cdns)
+{
+	phy_power_off(cdns->usb3_phy);
+	phy_power_off(cdns->usb2_phy);
+}
+
 /**
  * cdns3_probe - probe for cdns3 core device
  * @pdev: Pointer to cdns3 core platform device
@@ -463,14 +484,10 @@ static int cdns3_probe(struct platform_device *pdev)
 	if (ret)
 		goto err1;
 
-	ret = phy_power_on(cdns->usb2_phy);
+	ret = set_phy_power_on(cdns);
 	if (ret)
 		goto err2;
 
-	ret = phy_power_on(cdns->usb3_phy);
-	if (ret)
-		goto err3;
-
 	sw_desc.set = cdns3_role_set;
 	sw_desc.get = cdns3_role_get;
 	sw_desc.allow_userspace_control = true;
@@ -482,16 +499,16 @@ static int cdns3_probe(struct platform_device *pdev)
 	if (IS_ERR(cdns->role_sw)) {
 		ret = PTR_ERR(cdns->role_sw);
 		dev_warn(dev, "Unable to register Role Switch\n");
-		goto err4;
+		goto err3;
 	}
 
 	ret = cdns3_drd_init(cdns);
 	if (ret)
-		goto err5;
+		goto err4;
 
 	ret = cdns3_core_init_role(cdns);
 	if (ret)
-		goto err5;
+		goto err4;
 
 	device_set_wakeup_capable(dev, true);
 	pm_runtime_set_active(dev);
@@ -508,14 +525,11 @@ static int cdns3_probe(struct platform_device *pdev)
 	dev_dbg(dev, "Cadence USB3 core: probe succeed\n");
 
 	return 0;
-err5:
+err4:
 	cdns3_drd_exit(cdns);
 	usb_role_switch_unregister(cdns->role_sw);
-err4:
-	phy_power_off(cdns->usb3_phy);
-
 err3:
-	phy_power_off(cdns->usb2_phy);
+	set_phy_power_off(cdns);
 err2:
 	phy_exit(cdns->usb3_phy);
 err1:
@@ -539,8 +553,7 @@ static int cdns3_remove(struct platform_device *pdev)
 	pm_runtime_put_noidle(&pdev->dev);
 	cdns3_exit_roles(cdns);
 	usb_role_switch_unregister(cdns->role_sw);
-	phy_power_off(cdns->usb2_phy);
-	phy_power_off(cdns->usb3_phy);
+	set_phy_power_off(cdns);
 	phy_exit(cdns->usb2_phy);
 	phy_exit(cdns->usb3_phy);
 	return 0;
-- 
2.17.1

[PATCH v7 2/3] usb: cdns3: add runtime PM support

[Peter Chen]

Introduce runtime PM and wakeup interrupt handler for cdns3,
the runtime PM is default off since other cdns3 may not
implement glue layer support for runtime PM.

One typical wakeup event use case is xHCI runtime suspend will clear
USBCMD.RS bit, after that the xHCI will not trigger any interrupts,
so its parent (cdns core device) needs to resume xHCI device when
any (wakeup) events occurs at host port.

When the controller is in low power mode, the lpm flag will be set.
The interrupt triggered later than lpm flag is set considers as
wakeup interrupt and handled at cdns_wakeup_irq. Once the wakeup
occurs, it first disables interrupt to avoid later interrupt
occurrence since the controller is in low power mode at that
time, and access registers may be invalid at that time. At wakeup
handler, it will call pm_request_resume to wakeup xHCI device, and
at runtime resume handler, it will enable interrupt again.

The API platform_suspend is introduced for glue layer to implement
platform specific PM sequence.

Reviewed-by: Pawel Laszczak <pawell@cadence.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/core.c   | 152 ++++++++++++++++++++++++++++++++-----
 drivers/usb/cdns3/core.h   |  16 ++++
 drivers/usb/cdns3/drd.c    |   3 +
 drivers/usb/cdns3/gadget.c |   4 +
 drivers/usb/cdns3/host.c   |   7 ++
 5 files changed, 165 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index e56dbb6a898c..faee5ec5fc20 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -392,6 +392,29 @@ static void set_phy_power_off(struct cdns3 *cdns)
 	phy_power_off(cdns->usb2_phy);
 }
 
+/**
+ * cdns3_wakeup_irq - interrupt handler for wakeup events
+ * @irq: irq number for cdns3 core device
+ * @data: structure of cdns3
+ *
+ * Returns IRQ_HANDLED or IRQ_NONE
+ */
+static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
+{
+	struct cdns3 *cdns = data;
+
+	if (cdns->in_lpm) {
+		disable_irq_nosync(irq);
+		cdns->wakeup_pending = true;
+		if ((cdns->role == USB_ROLE_HOST) && cdns->host_dev)
+			pm_request_resume(&cdns->host_dev->dev);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
 /**
  * cdns3_probe - probe for cdns3 core device
  * @pdev: Pointer to cdns3 core platform device
@@ -418,6 +441,7 @@ static int cdns3_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	cdns->dev = dev;
+	cdns->pdata = dev_get_platdata(dev);
 
 	platform_set_drvdata(pdev, cdns);
 
@@ -466,6 +490,15 @@ static int cdns3_probe(struct platform_device *pdev)
 
 	cdns->otg_res = *res;
 
+	cdns->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
+	if (cdns->wakeup_irq == -EPROBE_DEFER)
+		return cdns->wakeup_irq;
+
+	if (cdns->wakeup_irq < 0) {
+		dev_dbg(dev, "couldn't get wakeup irq\n");
+		cdns->wakeup_irq = 0x0;
+	}
+
 	mutex_init(&cdns->mutex);
 
 	cdns->usb2_phy = devm_phy_optional_get(dev, "cdns3,usb2-phy");
@@ -502,6 +535,19 @@ static int cdns3_probe(struct platform_device *pdev)
 		goto err3;
 	}
 
+	if (cdns->wakeup_irq) {
+		ret = devm_request_threaded_irq(cdns->dev, cdns->wakeup_irq,
+						cdns3_wakeup_irq,
+						NULL,
+						IRQF_SHARED,
+						dev_name(cdns->dev), cdns);
+
+		if (ret) {
+			dev_err(cdns->dev, "couldn't register wakeup irq handler\n");
+			goto err3;
+		}
+	}
+
 	ret = cdns3_drd_init(cdns);
 	if (ret)
 		goto err4;
@@ -510,9 +556,11 @@ static int cdns3_probe(struct platform_device *pdev)
 	if (ret)
 		goto err4;
 
+	spin_lock_init(&cdns->lock);
 	device_set_wakeup_capable(dev, true);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
+	pm_runtime_forbid(dev);
 
 	/*
 	 * The controller needs less time between bus and controller suspend,
@@ -559,52 +607,122 @@ static int cdns3_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 
-static int cdns3_suspend(struct device *dev)
+static int cdns3_set_platform_suspend(struct device *dev,
+		bool suspend, bool wakeup)
+{
+	struct cdns3 *cdns = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (cdns->pdata && cdns->pdata->platform_suspend)
+		ret = cdns->pdata->platform_suspend(dev, suspend, wakeup);
+
+	return ret;
+}
+
+static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
 {
 	struct cdns3 *cdns = dev_get_drvdata(dev);
+	bool wakeup;
 	unsigned long flags;
 
-	if (cdns->role == USB_ROLE_HOST)
+	if (cdns->in_lpm)
 		return 0;
 
-	if (pm_runtime_status_suspended(dev))
-		pm_runtime_resume(dev);
+	if (PMSG_IS_AUTO(msg))
+		wakeup = true;
+	else
+		wakeup = device_may_wakeup(dev);
 
-	if (cdns->roles[cdns->role]->suspend) {
-		spin_lock_irqsave(&cdns->gadget_dev->lock, flags);
-		cdns->roles[cdns->role]->suspend(cdns, false);
-		spin_unlock_irqrestore(&cdns->gadget_dev->lock, flags);
-	}
+	cdns3_set_platform_suspend(cdns->dev, true, wakeup);
+	set_phy_power_off(cdns);
+	spin_lock_irqsave(&cdns->lock, flags);
+	cdns->in_lpm = true;
+	spin_unlock_irqrestore(&cdns->lock, flags);
+	dev_dbg(cdns->dev, "%s ends\n", __func__);
 
 	return 0;
 }
 
-static int cdns3_resume(struct device *dev)
+static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
 {
 	struct cdns3 *cdns = dev_get_drvdata(dev);
+	int ret;
 	unsigned long flags;
 
-	if (cdns->role == USB_ROLE_HOST)
+	if (!cdns->in_lpm)
 		return 0;
 
-	if (cdns->roles[cdns->role]->resume) {
-		spin_lock_irqsave(&cdns->gadget_dev->lock, flags);
+	ret = set_phy_power_on(cdns);
+	if (ret)
+		return ret;
+
+	cdns3_set_platform_suspend(cdns->dev, false, false);
+
+	spin_lock_irqsave(&cdns->lock, flags);
+	if (cdns->roles[cdns->role]->resume && !PMSG_IS_AUTO(msg))
 		cdns->roles[cdns->role]->resume(cdns, false);
-		spin_unlock_irqrestore(&cdns->gadget_dev->lock, flags);
+
+	cdns->in_lpm = false;
+	spin_unlock_irqrestore(&cdns->lock, flags);
+	if (cdns->wakeup_pending) {
+		cdns->wakeup_pending = false;
+		enable_irq(cdns->wakeup_irq);
+	}
+	dev_dbg(cdns->dev, "%s ends\n", __func__);
+
+	return ret;
+}
+
+static int cdns3_runtime_suspend(struct device *dev)
+{
+	return cdns3_controller_suspend(dev, PMSG_AUTO_SUSPEND);
+}
+
+static int cdns3_runtime_resume(struct device *dev)
+{
+	return cdns3_controller_resume(dev, PMSG_AUTO_RESUME);
+}
+#ifdef CONFIG_PM_SLEEP
+
+static int cdns3_suspend(struct device *dev)
+{
+	struct cdns3 *cdns = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	if (pm_runtime_status_suspended(dev))
+		pm_runtime_resume(dev);
+
+	if (cdns->roles[cdns->role]->suspend) {
+		spin_lock_irqsave(&cdns->lock, flags);
+		cdns->roles[cdns->role]->suspend(cdns, false);
+		spin_unlock_irqrestore(&cdns->lock, flags);
 	}
 
+	return cdns3_controller_suspend(dev, PMSG_SUSPEND);
+}
+
+static int cdns3_resume(struct device *dev)
+{
+	int ret;
+
+	ret = cdns3_controller_resume(dev, PMSG_RESUME);
+	if (ret)
+		return ret;
+
 	pm_runtime_disable(dev);
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 
-	return 0;
+	return ret;
 }
-#endif
+#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
 
 static const struct dev_pm_ops cdns3_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(cdns3_suspend, cdns3_resume)
+	SET_RUNTIME_PM_OPS(cdns3_runtime_suspend, cdns3_runtime_resume, NULL)
 };
 
 #ifdef CONFIG_OF
diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 1ad1f1fe61e9..1b1707796db2 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -38,6 +38,12 @@ struct cdns3_role_driver {
 };
 
 #define CDNS3_XHCI_RESOURCES_NUM	2
+
+struct cdns3_platform_data {
+	int (*platform_suspend)(struct device *dev,
+			bool suspend, bool wakeup);
+};
+
 /**
  * struct cdns3 - Representation of Cadence USB3 DRD controller.
  * @dev: pointer to Cadence device struct
@@ -50,6 +56,7 @@ struct cdns3_role_driver {
  * @otg_regs: pointer to base of otg registers
  * @otg_irq: irq number for otg controller
  * @dev_irq: irq number for device controller
+ * @wakeup_irq: irq number for wakeup event, it is optional
  * @roles: array of supported roles for this controller
  * @role: current role
  * @host_dev: the child host device pointer for cdns3 core
@@ -62,6 +69,10 @@ struct cdns3_role_driver {
  *           This field based on firmware setting, kernel configuration
  *           and hardware configuration.
  * @role_sw: pointer to role switch object.
+ * @in_lpm: indicate the controller is in low power mode
+ * @wakeup_pending: wakeup interrupt pending
+ * @pdata: platform data from glue layer
+ * @lock: spinlock structure
  */
 struct cdns3 {
 	struct device			*dev;
@@ -79,6 +90,7 @@ struct cdns3 {
 
 	int				otg_irq;
 	int				dev_irq;
+	int				wakeup_irq;
 	struct cdns3_role_driver	*roles[USB_ROLE_DEVICE + 1];
 	enum usb_role			role;
 	struct platform_device		*host_dev;
@@ -89,6 +101,10 @@ struct cdns3 {
 	struct mutex			mutex;
 	enum usb_dr_mode		dr_mode;
 	struct usb_role_switch		*role_sw;
+	bool				in_lpm;
+	bool				wakeup_pending;
+	struct cdns3_platform_data	*pdata;
+	spinlock_t			lock;
 };
 
 int cdns3_hw_role_switch(struct cdns3 *cdns);
diff --git a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c
index 6234bcd6158a..5f2685cadf5e 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -293,6 +293,9 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
 	if (cdns->dr_mode != USB_DR_MODE_OTG)
 		return IRQ_NONE;
 
+	if (cdns->in_lpm)
+		return ret;
+
 	reg = readl(&cdns->otg_regs->ivect);
 
 	if (!reg)
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index dea649ee173b..8bbb38cd560b 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1769,9 +1769,13 @@ static void cdns3_check_usb_interrupt_proceed(struct cdns3_device *priv_dev,
 static irqreturn_t cdns3_device_irq_handler(int irq, void *data)
 {
 	struct cdns3_device *priv_dev = data;
+	struct cdns3 *cdns = dev_get_drvdata(priv_dev->dev);
 	irqreturn_t ret = IRQ_NONE;
 	u32 reg;
 
+	if (cdns->in_lpm)
+		return ret;
+
 	/* check USB device interrupt */
 	reg = readl(&priv_dev->regs->usb_ists);
 	if (reg) {
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 36c63d9ecd37..b3e2cb69762c 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -13,11 +13,13 @@
 #include "core.h"
 #include "drd.h"
 #include "host-export.h"
+#include <linux/usb/hcd.h>
 
 static int __cdns3_host_init(struct cdns3 *cdns)
 {
 	struct platform_device *xhci;
 	int ret;
+	struct usb_hcd *hcd;
 
 	cdns3_drd_host_on(cdns);
 
@@ -43,6 +45,11 @@ static int __cdns3_host_init(struct cdns3 *cdns)
 		goto err1;
 	}
 
+	/* Glue needs to access xHCI region register for Power management */
+	hcd = platform_get_drvdata(xhci);
+	if (hcd)
+		cdns->xhci_regs = hcd->regs;
+
 	return 0;
 err1:
 	platform_device_put(xhci);
-- 
2.17.1

[PATCH v7 3/3] usb: cdns3: imx: add glue layer runtime pm implementation

[Peter Chen]

Add imx glue layer runtime pm implementation, and the runtime
pm is default off.

Reviewed-by: Pawel Laszczak <pawell@cadence.com>
Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/cdns3-imx.c | 203 ++++++++++++++++++++++++++++++++--
 1 file changed, 192 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-imx.c b/drivers/usb/cdns3/cdns3-imx.c
index aba988e71958..a413df26e948 100644
--- a/drivers/usb/cdns3/cdns3-imx.c
+++ b/drivers/usb/cdns3/cdns3-imx.c
@@ -15,6 +15,8 @@
 #include <linux/io.h>
 #include <linux/of_platform.h>
 #include <linux/iopoll.h>
+#include <linux/pm_runtime.h>
+#include "core.h"
 
 #define USB3_CORE_CTRL1    0x00
 #define USB3_CORE_CTRL2    0x04
@@ -32,7 +34,7 @@
 /* Register bits definition */
 
 /* USB3_CORE_CTRL1 */
-#define SW_RESET_MASK	(0x3f << 26)
+#define SW_RESET_MASK	GENMASK(31, 26)
 #define PWR_SW_RESET	BIT(31)
 #define APB_SW_RESET	BIT(30)
 #define AXI_SW_RESET	BIT(29)
@@ -44,17 +46,17 @@
 #define OC_DISABLE	BIT(9)
 #define MDCTRL_CLK_SEL	BIT(7)
 #define MODE_STRAP_MASK	(0x7)
-#define DEV_MODE	(1 << 2)
-#define HOST_MODE	(1 << 1)
-#define OTG_MODE	(1 << 0)
+#define DEV_MODE	BIT(2)
+#define HOST_MODE	BIT(1)
+#define OTG_MODE	BIT(0)
 
 /* USB3_INT_REG */
 #define CLK_125_REQ	BIT(29)
 #define LPM_CLK_REQ	BIT(28)
 #define DEVU3_WAEKUP_EN	BIT(14)
 #define OTG_WAKEUP_EN	BIT(12)
-#define DEV_INT_EN (3 << 8) /* DEV INT b9:8 */
-#define HOST_INT1_EN (1 << 0) /* HOST INT b7:0 */
+#define DEV_INT_EN	GENMASK(9, 8) /* DEV INT b9:8 */
+#define HOST_INT1_EN	BIT(0) /* HOST INT b7:0 */
 
 /* USB3_CORE_STATUS */
 #define MDCTRL_CLK_STATUS	BIT(15)
@@ -62,15 +64,34 @@
 #define HOST_POWER_ON_READY	BIT(12)
 
 /* USB3_SSPHY_STATUS */
-#define CLK_VALID_MASK		(0x3f << 26)
-#define CLK_VALID_COMPARE_BITS	(0xf << 28)
-#define PHY_REFCLK_REQ		(1 << 0)
+#define CLK_VALID_MASK		GENMASK(31, 26)
+#define CLK_VALID_COMPARE_BITS	GENMASK(31, 28)
+#define PHY_REFCLK_REQ		BIT(0)
+
+/* OTG registers definition */
+#define OTGSTS		0x4
+/* OTGSTS */
+#define OTG_NRDY	BIT(11)
+
+/* xHCI registers definition  */
+#define XECP_PM_PMCSR		0x8018
+#define XECP_AUX_CTRL_REG1	0x8120
+
+/* Register bits definition */
+/* XECP_AUX_CTRL_REG1 */
+#define CFG_RXDET_P3_EN		BIT(15)
+
+/* XECP_PM_PMCSR */
+#define PS_MASK			GENMASK(1, 0)
+#define PS_D0			0
+#define PS_D1			1
 
 struct cdns_imx {
 	struct device *dev;
 	void __iomem *noncore;
 	struct clk_bulk_data *clks;
 	int num_clks;
+	struct platform_device *cdns3_pdev;
 };
 
 static inline u32 cdns_imx_readl(struct cdns_imx *data, u32 offset)
@@ -126,6 +147,20 @@ static int cdns_imx_noncore_init(struct cdns_imx *data)
 	return ret;
 }
 
+static int cdns_imx_platform_suspend(struct device *dev,
+	bool suspend, bool wakeup);
+static struct cdns3_platform_data cdns_imx_pdata = {
+	.platform_suspend = cdns_imx_platform_suspend,
+};
+
+static struct of_dev_auxdata cdns_imx_auxdata[] = {
+	{
+	.compatible = "cdns,usb3",
+	.platform_data = &cdns_imx_pdata,
+	},
+	{},
+};
+
 static int cdns_imx_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -162,14 +197,18 @@ static int cdns_imx_probe(struct platform_device *pdev)
 	if (ret)
 		goto err;
 
-	ret = of_platform_populate(node, NULL, NULL, dev);
+	ret = of_platform_populate(node, NULL, cdns_imx_auxdata, dev);
 	if (ret) {
 		dev_err(dev, "failed to create children: %d\n", ret);
 		goto err;
 	}
 
-	return ret;
+	device_set_wakeup_capable(dev, true);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	pm_runtime_forbid(dev);
 
+	return ret;
 err:
 	clk_bulk_disable_unprepare(data->num_clks, data->clks);
 	return ret;
@@ -194,6 +233,147 @@ static int cdns_imx_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static void cdns3_set_wakeup(struct cdns_imx *data, bool enable)
+{
+	u32 value;
+
+	value = cdns_imx_readl(data, USB3_INT_REG);
+	if (enable)
+		value |= OTG_WAKEUP_EN | DEVU3_WAEKUP_EN;
+	else
+		value &= ~(OTG_WAKEUP_EN | DEVU3_WAEKUP_EN);
+
+	cdns_imx_writel(data, USB3_INT_REG, value);
+}
+
+static int cdns_imx_platform_suspend(struct device *dev,
+		bool suspend, bool wakeup)
+{
+	struct cdns3 *cdns = dev_get_drvdata(dev);
+	struct device *parent = dev->parent;
+	struct cdns_imx *data = dev_get_drvdata(parent);
+	void __iomem *otg_regs = (void __iomem *)(cdns->otg_regs);
+	void __iomem *xhci_regs = cdns->xhci_regs;
+	u32 value;
+	int ret = 0;
+
+	if (cdns->role != USB_ROLE_HOST)
+		return 0;
+
+	if (suspend) {
+		/* SW request low power when all usb ports allow to it ??? */
+		value = readl(xhci_regs + XECP_PM_PMCSR);
+		value &= ~PS_MASK;
+		value |= PS_D1;
+		writel(value, xhci_regs + XECP_PM_PMCSR);
+
+		/* mdctrl_clk_sel */
+		value = cdns_imx_readl(data, USB3_CORE_CTRL1);
+		value |= MDCTRL_CLK_SEL;
+		cdns_imx_writel(data, USB3_CORE_CTRL1, value);
+
+		/* wait for mdctrl_clk_status */
+		value = cdns_imx_readl(data, USB3_CORE_STATUS);
+		ret = readl_poll_timeout(data->noncore + USB3_CORE_STATUS, value,
+			(value & MDCTRL_CLK_STATUS) == MDCTRL_CLK_STATUS,
+			10, 100000);
+		if (ret)
+			dev_warn(parent, "wait mdctrl_clk_status timeout\n");
+
+		/* wait lpm_clk_req to be 0 */
+		value = cdns_imx_readl(data, USB3_INT_REG);
+		ret = readl_poll_timeout(data->noncore + USB3_INT_REG, value,
+			(value & LPM_CLK_REQ) != LPM_CLK_REQ,
+			10, 100000);
+		if (ret)
+			dev_warn(parent, "wait lpm_clk_req timeout\n");
+
+		/* wait phy_refclk_req to be 0 */
+		value = cdns_imx_readl(data, USB3_SSPHY_STATUS);
+		ret = readl_poll_timeout(data->noncore + USB3_SSPHY_STATUS, value,
+			(value & PHY_REFCLK_REQ) != PHY_REFCLK_REQ,
+			10, 100000);
+		if (ret)
+			dev_warn(parent, "wait phy_refclk_req timeout\n");
+
+		cdns3_set_wakeup(data, wakeup);
+	} else {
+		cdns3_set_wakeup(data, false);
+
+		/* SW request D0 */
+		value = readl(xhci_regs + XECP_PM_PMCSR);
+		value &= ~PS_MASK;
+		value |= PS_D0;
+		writel(value, xhci_regs + XECP_PM_PMCSR);
+
+		/* clr CFG_RXDET_P3_EN */
+		value = readl(xhci_regs + XECP_AUX_CTRL_REG1);
+		value &= ~CFG_RXDET_P3_EN;
+		writel(value, xhci_regs + XECP_AUX_CTRL_REG1);
+
+		/* clear mdctrl_clk_sel */
+		value = cdns_imx_readl(data, USB3_CORE_CTRL1);
+		value &= ~MDCTRL_CLK_SEL;
+		cdns_imx_writel(data, USB3_CORE_CTRL1, value);
+
+		/* wait CLK_125_REQ to be 1 */
+		value = cdns_imx_readl(data, USB3_INT_REG);
+		ret = readl_poll_timeout(data->noncore + USB3_INT_REG, value,
+			(value & CLK_125_REQ) == CLK_125_REQ,
+			10, 100000);
+		if (ret)
+			dev_warn(parent, "wait CLK_125_REQ timeout\n");
+
+		/* wait for mdctrl_clk_status is cleared */
+		value = cdns_imx_readl(data, USB3_CORE_STATUS);
+		ret = readl_poll_timeout(data->noncore + USB3_CORE_STATUS, value,
+			(value & MDCTRL_CLK_STATUS) != MDCTRL_CLK_STATUS,
+			10, 100000);
+		if (ret)
+			dev_warn(parent, "wait mdctrl_clk_status cleared timeout\n");
+
+		/* Wait until OTG_NRDY is 0 */
+		value = readl(otg_regs + OTGSTS);
+		ret = readl_poll_timeout(otg_regs + OTGSTS, value,
+			(value & OTG_NRDY) != OTG_NRDY,
+			10, 100000);
+		if (ret)
+			dev_warn(parent, "wait OTG ready timeout\n");
+	}
+
+	return ret;
+
+}
+
+static int cdns_imx_resume(struct device *dev)
+{
+	struct cdns_imx *data = dev_get_drvdata(dev);
+
+	return clk_bulk_prepare_enable(data->num_clks, data->clks);
+}
+
+static int cdns_imx_suspend(struct device *dev)
+{
+	struct cdns_imx *data = dev_get_drvdata(dev);
+
+	clk_bulk_disable_unprepare(data->num_clks, data->clks);
+
+	return 0;
+}
+#else
+static int cdns_imx_platform_suspend(struct device *dev,
+	bool suspend, bool wakeup)
+{
+	return 0;
+}
+
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops cdns_imx_pm_ops = {
+	SET_RUNTIME_PM_OPS(cdns_imx_suspend, cdns_imx_resume, NULL)
+};
+
 static const struct of_device_id cdns_imx_of_match[] = {
 	{ .compatible = "fsl,imx8qm-usb3", },
 	{},
@@ -206,6 +386,7 @@ static struct platform_driver cdns_imx_driver = {
 	.driver		= {
 		.name	= "cdns3-imx",
 		.of_match_table	= cdns_imx_of_match,
+		.pm	= &cdns_imx_pm_ops,
 	},
 };
 module_platform_driver(cdns_imx_driver);
-- 
2.17.1

Re: [PATCH v7 1/3] usb: cdns3: introduce set_phy_power_on{off} APIs

[Felipe Balbi]

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

Peter Chen <peter.chen@nxp.com> writes:

> Since we have both USB2 and USB3 PHYs for cdns3 controller, it is
> better we have unity APIs to handle both USB2 and USB3's power, it
> could simplify code for error handling and further power management
> implementation.
>
> Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> Reviewed-by: Jun Li <jun.li@nxp.com>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/core.c | 43 ++++++++++++++++++++++++++--------------
>  1 file changed, 28 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index 5c1586ec7824..e56dbb6a898c 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -371,6 +371,27 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
>  	return ret;
>  }
>  
> +static int set_phy_power_on(struct cdns3 *cdns)

care to keep the cdns3_ prefix? It's useful when you want to use
e.g. ftrace and function_graph trace

> +{
> +	int ret;
> +
> +	ret = phy_power_on(cdns->usb2_phy);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_power_on(cdns->usb3_phy);

let's say we fail here

> +	if (ret)
> +		phy_power_off(cdns->usb2_phy);

usb2_phy will be powered off..

> +	return ret;
> +}
> +
> +static void set_phy_power_off(struct cdns3 *cdns)
> +{
> +	phy_power_off(cdns->usb3_phy);
> +	phy_power_off(cdns->usb2_phy);

Won't this cause a problem?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v7 2/3] usb: cdns3: add runtime PM support

[Felipe Balbi]

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


Hi,

Peter Chen <peter.chen@nxp.com> writes:
> Introduce runtime PM and wakeup interrupt handler for cdns3,
> the runtime PM is default off since other cdns3 may not
> implement glue layer support for runtime PM.
>
> One typical wakeup event use case is xHCI runtime suspend will clear
> USBCMD.RS bit, after that the xHCI will not trigger any interrupts,
> so its parent (cdns core device) needs to resume xHCI device when
> any (wakeup) events occurs at host port.
>
> When the controller is in low power mode, the lpm flag will be set.
> The interrupt triggered later than lpm flag is set considers as
> wakeup interrupt and handled at cdns_wakeup_irq. Once the wakeup
> occurs, it first disables interrupt to avoid later interrupt
> occurrence since the controller is in low power mode at that
> time, and access registers may be invalid at that time. At wakeup
> handler, it will call pm_request_resume to wakeup xHCI device, and
> at runtime resume handler, it will enable interrupt again.
>
> The API platform_suspend is introduced for glue layer to implement
> platform specific PM sequence.

can't you rely on parent->child relationship here?

> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> index e56dbb6a898c..faee5ec5fc20 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -392,6 +392,29 @@ static void set_phy_power_off(struct cdns3 *cdns)
>  	phy_power_off(cdns->usb2_phy);
>  }
>  
> +/**
> + * cdns3_wakeup_irq - interrupt handler for wakeup events
> + * @irq: irq number for cdns3 core device
> + * @data: structure of cdns3
> + *
> + * Returns IRQ_HANDLED or IRQ_NONE
> + */
> +static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
> +{
> +	struct cdns3 *cdns = data;
> +
> +	if (cdns->in_lpm) {
> +		disable_irq_nosync(irq);

why do you need to call disable_irq_nosync()? interrupts are already
disabled.

> +		cdns->wakeup_pending = true;
> +		if ((cdns->role == USB_ROLE_HOST) && cdns->host_dev)
> +			pm_request_resume(&cdns->host_dev->dev);

nothing for peripheral mode?

> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
>  /**
>   * cdns3_probe - probe for cdns3 core device
>   * @pdev: Pointer to cdns3 core platform device
> @@ -418,6 +441,7 @@ static int cdns3_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	cdns->dev = dev;
> +	cdns->pdata = dev_get_platdata(dev);
>  
>  	platform_set_drvdata(pdev, cdns);
>  
> @@ -466,6 +490,15 @@ static int cdns3_probe(struct platform_device *pdev)
>  
>  	cdns->otg_res = *res;
>  
> +	cdns->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> +	if (cdns->wakeup_irq == -EPROBE_DEFER)
> +		return cdns->wakeup_irq;
> +
> +	if (cdns->wakeup_irq < 0) {

should be <= 0, no?

> @@ -502,6 +535,19 @@ static int cdns3_probe(struct platform_device *pdev)
>  		goto err3;
>  	}
>  
> +	if (cdns->wakeup_irq) {
> +		ret = devm_request_threaded_irq(cdns->dev, cdns->wakeup_irq,
> +						cdns3_wakeup_irq,
> +						NULL,

if the thread handler is NULL, why don't you use devm_request_irq()?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v7 3/3] usb: cdns3: imx: add glue layer runtime pm implementation

[Felipe Balbi]

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

Peter Chen <peter.chen@nxp.com> writes:

> Add imx glue layer runtime pm implementation, and the runtime
> pm is default off.
>
> Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/cdns3-imx.c | 203 ++++++++++++++++++++++++++++++++--
>  1 file changed, 192 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-imx.c b/drivers/usb/cdns3/cdns3-imx.c
> index aba988e71958..a413df26e948 100644
> --- a/drivers/usb/cdns3/cdns3-imx.c
> +++ b/drivers/usb/cdns3/cdns3-imx.c
> @@ -15,6 +15,8 @@
>  #include <linux/io.h>
>  #include <linux/of_platform.h>
>  #include <linux/iopoll.h>
> +#include <linux/pm_runtime.h>
> +#include "core.h"
>  
>  #define USB3_CORE_CTRL1    0x00
>  #define USB3_CORE_CTRL2    0x04
> @@ -32,7 +34,7 @@
>  /* Register bits definition */
>  
>  /* USB3_CORE_CTRL1 */
> -#define SW_RESET_MASK	(0x3f << 26)
> +#define SW_RESET_MASK	GENMASK(31, 26)

why is this part of adding imx runtime pm?

> @@ -44,17 +46,17 @@
>  #define OC_DISABLE	BIT(9)
>  #define MDCTRL_CLK_SEL	BIT(7)
>  #define MODE_STRAP_MASK	(0x7)
> -#define DEV_MODE	(1 << 2)
> -#define HOST_MODE	(1 << 1)
> -#define OTG_MODE	(1 << 0)
> +#define DEV_MODE	BIT(2)
> +#define HOST_MODE	BIT(1)
> +#define OTG_MODE	BIT(0)

and these?

>  
>  /* USB3_INT_REG */
>  #define CLK_125_REQ	BIT(29)
>  #define LPM_CLK_REQ	BIT(28)
>  #define DEVU3_WAEKUP_EN	BIT(14)
>  #define OTG_WAKEUP_EN	BIT(12)
> -#define DEV_INT_EN (3 << 8) /* DEV INT b9:8 */
> -#define HOST_INT1_EN (1 << 0) /* HOST INT b7:0 */
> +#define DEV_INT_EN	GENMASK(9, 8) /* DEV INT b9:8 */
> +#define HOST_INT1_EN	BIT(0) /* HOST INT b7:0 */

what about these?

> @@ -62,15 +64,34 @@
>  #define HOST_POWER_ON_READY	BIT(12)
>  
>  /* USB3_SSPHY_STATUS */
> -#define CLK_VALID_MASK		(0x3f << 26)
> -#define CLK_VALID_COMPARE_BITS	(0xf << 28)
> -#define PHY_REFCLK_REQ		(1 << 0)
> +#define CLK_VALID_MASK		GENMASK(31, 26)
> +#define CLK_VALID_COMPARE_BITS	GENMASK(31, 28)
> +#define PHY_REFCLK_REQ		BIT(0)

these?

> +/* OTG registers definition */
> +#define OTGSTS		0x4
> +/* OTGSTS */
> +#define OTG_NRDY	BIT(11)
> +
> +/* xHCI registers definition  */
> +#define XECP_PM_PMCSR		0x8018
> +#define XECP_AUX_CTRL_REG1	0x8120
> +
> +/* Register bits definition */
> +/* XECP_AUX_CTRL_REG1 */
> +#define CFG_RXDET_P3_EN		BIT(15)
> +
> +/* XECP_PM_PMCSR */
> +#define PS_MASK			GENMASK(1, 0)
> +#define PS_D0			0
> +#define PS_D1			1

I think only these are part of $subject

>  struct cdns_imx {
>  	struct device *dev;
>  	void __iomem *noncore;
>  	struct clk_bulk_data *clks;
>  	int num_clks;
> +	struct platform_device *cdns3_pdev;
>  };
>  
>  static inline u32 cdns_imx_readl(struct cdns_imx *data, u32 offset)
> @@ -126,6 +147,20 @@ static int cdns_imx_noncore_init(struct cdns_imx *data)
>  	return ret;
>  }
>  
> +static int cdns_imx_platform_suspend(struct device *dev,
> +	bool suspend, bool wakeup);
> +static struct cdns3_platform_data cdns_imx_pdata = {

make it const?

> +	.platform_suspend = cdns_imx_platform_suspend,
> +};
> +
> +static struct of_dev_auxdata cdns_imx_auxdata[] = {

const?

> +	{
> +	.compatible = "cdns,usb3",
> +	.platform_data = &cdns_imx_pdata,
> +	},

bad indentation

> @@ -194,6 +233,147 @@ static int cdns_imx_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static void cdns3_set_wakeup(struct cdns_imx *data, bool enable)
> +{
> +	u32 value;
> +
> +	value = cdns_imx_readl(data, USB3_INT_REG);
> +	if (enable)
> +		value |= OTG_WAKEUP_EN | DEVU3_WAEKUP_EN;
> +	else
> +		value &= ~(OTG_WAKEUP_EN | DEVU3_WAEKUP_EN);
> +
> +	cdns_imx_writel(data, USB3_INT_REG, value);
> +}
> +
> +static int cdns_imx_platform_suspend(struct device *dev,
> +		bool suspend, bool wakeup)
> +{
> +	struct cdns3 *cdns = dev_get_drvdata(dev);
> +	struct device *parent = dev->parent;
> +	struct cdns_imx *data = dev_get_drvdata(parent);
> +	void __iomem *otg_regs = (void __iomem *)(cdns->otg_regs);

why the cast?

> +	void __iomem *xhci_regs = cdns->xhci_regs;
> +	u32 value;
> +	int ret = 0;
> +
> +	if (cdns->role != USB_ROLE_HOST)
> +		return 0;
> +
> +	if (suspend) {
> +		/* SW request low power when all usb ports allow to it ??? */
> +		value = readl(xhci_regs + XECP_PM_PMCSR);
> +		value &= ~PS_MASK;
> +		value |= PS_D1;
> +		writel(value, xhci_regs + XECP_PM_PMCSR);
> +
> +		/* mdctrl_clk_sel */
> +		value = cdns_imx_readl(data, USB3_CORE_CTRL1);
> +		value |= MDCTRL_CLK_SEL;
> +		cdns_imx_writel(data, USB3_CORE_CTRL1, value);
> +
> +		/* wait for mdctrl_clk_status */
> +		value = cdns_imx_readl(data, USB3_CORE_STATUS);
> +		ret = readl_poll_timeout(data->noncore + USB3_CORE_STATUS, value,
> +			(value & MDCTRL_CLK_STATUS) == MDCTRL_CLK_STATUS,
> +			10, 100000);
> +		if (ret)
> +			dev_warn(parent, "wait mdctrl_clk_status timeout\n");
> +
> +		/* wait lpm_clk_req to be 0 */
> +		value = cdns_imx_readl(data, USB3_INT_REG);
> +		ret = readl_poll_timeout(data->noncore + USB3_INT_REG, value,
> +			(value & LPM_CLK_REQ) != LPM_CLK_REQ,
> +			10, 100000);
> +		if (ret)
> +			dev_warn(parent, "wait lpm_clk_req timeout\n");
> +
> +		/* wait phy_refclk_req to be 0 */
> +		value = cdns_imx_readl(data, USB3_SSPHY_STATUS);
> +		ret = readl_poll_timeout(data->noncore + USB3_SSPHY_STATUS, value,
> +			(value & PHY_REFCLK_REQ) != PHY_REFCLK_REQ,
> +			10, 100000);
> +		if (ret)
> +			dev_warn(parent, "wait phy_refclk_req timeout\n");
> +
> +		cdns3_set_wakeup(data, wakeup);
> +	} else {
> +		cdns3_set_wakeup(data, false);
> +
> +		/* SW request D0 */
> +		value = readl(xhci_regs + XECP_PM_PMCSR);
> +		value &= ~PS_MASK;
> +		value |= PS_D0;
> +		writel(value, xhci_regs + XECP_PM_PMCSR);
> +
> +		/* clr CFG_RXDET_P3_EN */
> +		value = readl(xhci_regs + XECP_AUX_CTRL_REG1);
> +		value &= ~CFG_RXDET_P3_EN;
> +		writel(value, xhci_regs + XECP_AUX_CTRL_REG1);
> +
> +		/* clear mdctrl_clk_sel */
> +		value = cdns_imx_readl(data, USB3_CORE_CTRL1);
> +		value &= ~MDCTRL_CLK_SEL;
> +		cdns_imx_writel(data, USB3_CORE_CTRL1, value);
> +
> +		/* wait CLK_125_REQ to be 1 */
> +		value = cdns_imx_readl(data, USB3_INT_REG);
> +		ret = readl_poll_timeout(data->noncore + USB3_INT_REG, value,
> +			(value & CLK_125_REQ) == CLK_125_REQ,
> +			10, 100000);
> +		if (ret)
> +			dev_warn(parent, "wait CLK_125_REQ timeout\n");
> +
> +		/* wait for mdctrl_clk_status is cleared */
> +		value = cdns_imx_readl(data, USB3_CORE_STATUS);
> +		ret = readl_poll_timeout(data->noncore + USB3_CORE_STATUS, value,
> +			(value & MDCTRL_CLK_STATUS) != MDCTRL_CLK_STATUS,
> +			10, 100000);
> +		if (ret)
> +			dev_warn(parent, "wait mdctrl_clk_status cleared timeout\n");
> +
> +		/* Wait until OTG_NRDY is 0 */
> +		value = readl(otg_regs + OTGSTS);
> +		ret = readl_poll_timeout(otg_regs + OTGSTS, value,
> +			(value & OTG_NRDY) != OTG_NRDY,
> +			10, 100000);
> +		if (ret)
> +			dev_warn(parent, "wait OTG ready timeout\n");
> +	}
> +
> +	return ret;
> +
> +}
> +
> +static int cdns_imx_resume(struct device *dev)
> +{
> +	struct cdns_imx *data = dev_get_drvdata(dev);
> +
> +	return clk_bulk_prepare_enable(data->num_clks, data->clks);

do you need to prepare and unprepare for every suspend/resume? Isn't
enable/disable enough?

> +static const struct dev_pm_ops cdns_imx_pm_ops = {
> +	SET_RUNTIME_PM_OPS(cdns_imx_suspend, cdns_imx_resume, NULL)

could the same be used for system sleep?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]

Re: [PATCH v7 1/3] usb: cdns3: introduce set_phy_power_on{off} APIs

[Peter Chen]

On 20-08-27 16:09:36, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
> 
> > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is
> > better we have unity APIs to handle both USB2 and USB3's power, it
> > could simplify code for error handling and further power management
> > implementation.
> >
> > Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> > Reviewed-by: Jun Li <jun.li@nxp.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/core.c | 43 ++++++++++++++++++++++++++--------------
> >  1 file changed, 28 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index 5c1586ec7824..e56dbb6a898c 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -371,6 +371,27 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
> >  	return ret;
> >  }
> >  
> > +static int set_phy_power_on(struct cdns3 *cdns)
> 
> care to keep the cdns3_ prefix? It's useful when you want to use
> e.g. ftrace and function_graph trace

I original added it, but Greg suggested it is not needed.

https://www.spinics.net/lists/linux-usb/msg197076.html

> 
> > +{
> > +	int ret;
> > +
> > +	ret = phy_power_on(cdns->usb2_phy);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = phy_power_on(cdns->usb3_phy);
> 
> let's say we fail here
> 
> > +	if (ret)
> > +		phy_power_off(cdns->usb2_phy);
> 
> usb2_phy will be powered off..

If usb3_phy's power_on has failed, it powers off usb2 phy's power which
is powered on before, and return fail.

> 
> > +	return ret;
> > +}
> > +
> > +static void set_phy_power_off(struct cdns3 *cdns)
> > +{
> > +	phy_power_off(cdns->usb3_phy);
> > +	phy_power_off(cdns->usb2_phy);
> 
> Won't this cause a problem?
> 

What's the problem you have seen?

-- 

Thanks,
Peter Chen

Re: [PATCH v7 2/3] usb: cdns3: add runtime PM support

[Peter Chen]

On 20-08-27 16:16:03, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen <peter.chen@nxp.com> writes:
> > Introduce runtime PM and wakeup interrupt handler for cdns3,
> > the runtime PM is default off since other cdns3 may not
> > implement glue layer support for runtime PM.
> >
> > One typical wakeup event use case is xHCI runtime suspend will clear
> > USBCMD.RS bit, after that the xHCI will not trigger any interrupts,
> > so its parent (cdns core device) needs to resume xHCI device when
> > any (wakeup) events occurs at host port.
> >
> > When the controller is in low power mode, the lpm flag will be set.
> > The interrupt triggered later than lpm flag is set considers as
> > wakeup interrupt and handled at cdns_wakeup_irq. Once the wakeup
> > occurs, it first disables interrupt to avoid later interrupt
> > occurrence since the controller is in low power mode at that
> > time, and access registers may be invalid at that time. At wakeup
> > handler, it will call pm_request_resume to wakeup xHCI device, and
> > at runtime resume handler, it will enable interrupt again.
> >
> > The API platform_suspend is introduced for glue layer to implement
> > platform specific PM sequence.
> 
> can't you rely on parent->child relationship here?

Sorry, what do you mean? It is the glue layer needed API for power
management, and pass through the platform_data when create the core
device.

> 
> > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
> > index e56dbb6a898c..faee5ec5fc20 100644
> > --- a/drivers/usb/cdns3/core.c
> > +++ b/drivers/usb/cdns3/core.c
> > @@ -392,6 +392,29 @@ static void set_phy_power_off(struct cdns3 *cdns)
> >  	phy_power_off(cdns->usb2_phy);
> >  }
> >  
> > +/**
> > + * cdns3_wakeup_irq - interrupt handler for wakeup events
> > + * @irq: irq number for cdns3 core device
> > + * @data: structure of cdns3
> > + *
> > + * Returns IRQ_HANDLED or IRQ_NONE
> > + */
> > +static irqreturn_t cdns3_wakeup_irq(int irq, void *data)
> > +{
> > +	struct cdns3 *cdns = data;
> > +
> > +	if (cdns->in_lpm) {
> > +		disable_irq_nosync(irq);
> 
> why do you need to call disable_irq_nosync()? interrupts are already
> disabled.

The interrupt is pending if it is not cleared, but clear the interrupt
status needs clock which will be done at .runtime_pm_resume and will
be scheduled later. Disable it could avoid the pending'ed interrupt
entering interrupt handler again.

We have similar design at: dwc3/gadget.c and chipidea/core.c

> 
> > +		cdns->wakeup_pending = true;
> > +		if ((cdns->role == USB_ROLE_HOST) && cdns->host_dev)
> > +			pm_request_resume(&cdns->host_dev->dev);
> 
> nothing for peripheral mode?

CDNS3 device is only at device mode when the connection is there, and if
the connection is there, it will not enter low power mode.

> 
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> >  /**
> >   * cdns3_probe - probe for cdns3 core device
> >   * @pdev: Pointer to cdns3 core platform device
> > @@ -418,6 +441,7 @@ static int cdns3_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	cdns->dev = dev;
> > +	cdns->pdata = dev_get_platdata(dev);
> >  
> >  	platform_set_drvdata(pdev, cdns);
> >  
> > @@ -466,6 +490,15 @@ static int cdns3_probe(struct platform_device *pdev)
> >  
> >  	cdns->otg_res = *res;
> >  
> > +	cdns->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> > +	if (cdns->wakeup_irq == -EPROBE_DEFER)
> > +		return cdns->wakeup_irq;
> > +
> > +	if (cdns->wakeup_irq < 0) {
> 
> should be <= 0, no?

Good catch, will change.

> 
> > @@ -502,6 +535,19 @@ static int cdns3_probe(struct platform_device *pdev)
> >  		goto err3;
> >  	}
> >  
> > +	if (cdns->wakeup_irq) {
> > +		ret = devm_request_threaded_irq(cdns->dev, cdns->wakeup_irq,
> > +						cdns3_wakeup_irq,
> > +						NULL,
> 
> if the thread handler is NULL, why don't you use devm_request_irq()?

Good catch, will change.

-- 

Thanks,
Peter Chen

Re: [PATCH v7 3/3] usb: cdns3: imx: add glue layer runtime pm implementation

[Peter Chen]

On 20-08-27 16:20:15, Felipe Balbi wrote:
> Peter Chen <peter.chen@nxp.com> writes:
> 
> > Add imx glue layer runtime pm implementation, and the runtime
> > pm is default off.
> >
> > Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/cdns3-imx.c | 203 ++++++++++++++++++++++++++++++++--
> >  1 file changed, 192 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/cdns3-imx.c b/drivers/usb/cdns3/cdns3-imx.c
> > index aba988e71958..a413df26e948 100644
> > --- a/drivers/usb/cdns3/cdns3-imx.c
> > +++ b/drivers/usb/cdns3/cdns3-imx.c
> > @@ -15,6 +15,8 @@
> >  #include <linux/io.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/pm_runtime.h>
> > +#include "core.h"
> >  
> >  #define USB3_CORE_CTRL1    0x00
> >  #define USB3_CORE_CTRL2    0x04
> > @@ -32,7 +34,7 @@
> >  /* Register bits definition */
> >  
> >  /* USB3_CORE_CTRL1 */
> > -#define SW_RESET_MASK	(0x3f << 26)
> > +#define SW_RESET_MASK	GENMASK(31, 26)
> 
> why is this part of adding imx runtime pm?

Sorry, will delete this improvement
> 
> > @@ -44,17 +46,17 @@
> >  #define OC_DISABLE	BIT(9)
> >  #define MDCTRL_CLK_SEL	BIT(7)
> >  #define MODE_STRAP_MASK	(0x7)
> > -#define DEV_MODE	(1 << 2)
> > -#define HOST_MODE	(1 << 1)
> > -#define OTG_MODE	(1 << 0)
> > +#define DEV_MODE	BIT(2)
> > +#define HOST_MODE	BIT(1)
> > +#define OTG_MODE	BIT(0)
> 
> and these?

Sorry, will delete this improvement

> 
> >  
> >  /* USB3_INT_REG */
> >  #define CLK_125_REQ	BIT(29)
> >  #define LPM_CLK_REQ	BIT(28)
> >  #define DEVU3_WAEKUP_EN	BIT(14)
> >  #define OTG_WAKEUP_EN	BIT(12)
> > -#define DEV_INT_EN (3 << 8) /* DEV INT b9:8 */
> > -#define HOST_INT1_EN (1 << 0) /* HOST INT b7:0 */
> > +#define DEV_INT_EN	GENMASK(9, 8) /* DEV INT b9:8 */
> > +#define HOST_INT1_EN	BIT(0) /* HOST INT b7:0 */
> 
Sorry, will delete this improvement

> 
> > @@ -62,15 +64,34 @@
> >  #define HOST_POWER_ON_READY	BIT(12)
> >  
> >  /* USB3_SSPHY_STATUS */
> > -#define CLK_VALID_MASK		(0x3f << 26)
> > -#define CLK_VALID_COMPARE_BITS	(0xf << 28)
> > -#define PHY_REFCLK_REQ		(1 << 0)
> > +#define CLK_VALID_MASK		GENMASK(31, 26)
> > +#define CLK_VALID_COMPARE_BITS	GENMASK(31, 28)
> > +#define PHY_REFCLK_REQ		BIT(0)
> 

Sorry, will delete this improvement

> 
> > +/* OTG registers definition */
> > +#define OTGSTS		0x4
> > +/* OTGSTS */
> > +#define OTG_NRDY	BIT(11)
> > +
> > +/* xHCI registers definition  */
> > +#define XECP_PM_PMCSR		0x8018
> > +#define XECP_AUX_CTRL_REG1	0x8120
> > +
> > +/* Register bits definition */
> > +/* XECP_AUX_CTRL_REG1 */
> > +#define CFG_RXDET_P3_EN		BIT(15)
> > +
> > +/* XECP_PM_PMCSR */
> > +#define PS_MASK			GENMASK(1, 0)
> > +#define PS_D0			0
> > +#define PS_D1			1
> 
> I think only these are part of $subject

Yes, you are right.

> 
> >  struct cdns_imx {
> >  	struct device *dev;
> >  	void __iomem *noncore;
> >  	struct clk_bulk_data *clks;
> >  	int num_clks;
> > +	struct platform_device *cdns3_pdev;
> >  };
> >  
> >  static inline u32 cdns_imx_readl(struct cdns_imx *data, u32 offset)
> > @@ -126,6 +147,20 @@ static int cdns_imx_noncore_init(struct cdns_imx *data)
> >  	return ret;
> >  }
> >  
> > +static int cdns_imx_platform_suspend(struct device *dev,
> > +	bool suspend, bool wakeup);
> > +static struct cdns3_platform_data cdns_imx_pdata = {
> 
> make it const?

Will change
> 
> > +	.platform_suspend = cdns_imx_platform_suspend,
> > +};
> > +
> > +static struct of_dev_auxdata cdns_imx_auxdata[] = {
> 
> const?

Will change

> 
> > +	{
> > +	.compatible = "cdns,usb3",
> > +	.platform_data = &cdns_imx_pdata,
> > +	},
> 
> bad indentation

Will change

> 
> > @@ -194,6 +233,147 @@ static int cdns_imx_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > +#ifdef CONFIG_PM
> > +static void cdns3_set_wakeup(struct cdns_imx *data, bool enable)
> > +{
> > +	u32 value;
> > +
> > +	value = cdns_imx_readl(data, USB3_INT_REG);
> > +	if (enable)
> > +		value |= OTG_WAKEUP_EN | DEVU3_WAEKUP_EN;
> > +	else
> > +		value &= ~(OTG_WAKEUP_EN | DEVU3_WAEKUP_EN);
> > +
> > +	cdns_imx_writel(data, USB3_INT_REG, value);
> > +}
> > +
> > +static int cdns_imx_platform_suspend(struct device *dev,
> > +		bool suspend, bool wakeup)
> > +{
> > +	struct cdns3 *cdns = dev_get_drvdata(dev);
> > +	struct device *parent = dev->parent;
> > +	struct cdns_imx *data = dev_get_drvdata(parent);
> > +	void __iomem *otg_regs = (void __iomem *)(cdns->otg_regs);
> 
> why the cast?

You mean __iomem?

The kernel test robot reports warning:

https://www.spinics.net/lists/linux-usb/msg199770.html

> > +
> > +static int cdns_imx_resume(struct device *dev)
> > +{
> > +	struct cdns_imx *data = dev_get_drvdata(dev);
> > +
> > +	return clk_bulk_prepare_enable(data->num_clks, data->clks);
> 
> do you need to prepare and unprepare for every suspend/resume? Isn't
> enable/disable enough?

Since runtime suspend/resume is not often, only at below situations,
we want the lowest power consumption:

- CDNS3 device is connected from the host
- There is no device on the CDNS3 host
- The device which supports runtime-suspend is suspended at 
CDNS3 host.

> 
> > +static const struct dev_pm_ops cdns_imx_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(cdns_imx_suspend, cdns_imx_resume, NULL)
> 
> could the same be used for system sleep?

The suspend is the same, but resume is not since the controller may lost
its power during system sleep, it needs to judge that power status and
restore the controller. I will submit these system PM supports later.

-- 

Thanks,
Peter Chen

Re: [PATCH v7 3/3] usb: cdns3: imx: add glue layer runtime pm implementation

[Peter Chen]

B
> >
> > > Add imx glue layer runtime pm implementation, and the runtime
> > > pm is default off.
> > >
> > > Reviewed-by: Pawel Laszczak <pawell@cadence.com>
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > >  drivers/usb/cdns3/cdns3-imx.c | 203 ++++++++++++++++++++++++++++++++--
> > >  1 file changed, 192 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/usb/cdns3/cdns3-imx.c b/drivers/usb/cdns3/cdns3-imx.c
> > > index aba988e71958..a413df26e948 100644
> > > --- a/drivers/usb/cdns3/cdns3-imx.c
> > > +++ b/drivers/usb/cdns3/cdns3-imx.c
> > > @@ -15,6 +15,8 @@
> > >  #include <linux/io.h>
> > >  #include <linux/of_platform.h>
> > >  #include <linux/iopoll.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include "core.h"
> > >
> > >  #define USB3_CORE_CTRL1    0x00
> > >  #define USB3_CORE_CTRL2    0x04
> > > @@ -32,7 +34,7 @@
> > >  /* Register bits definition */
> > >
> > >  /* USB3_CORE_CTRL1 */
> > > -#define SW_RESET_MASK      (0x3f << 26)
> > > +#define SW_RESET_MASK      GENMASK(31, 26)
> >
> > why is this part of adding imx runtime pm?
>
> Sorry, will delete this improvement
> >
> > > @@ -44,17 +46,17 @@
> > >  #define OC_DISABLE BIT(9)
> > >  #define MDCTRL_CLK_SEL     BIT(7)
> > >  #define MODE_STRAP_MASK    (0x7)
> > > -#define DEV_MODE   (1 << 2)
> > > -#define HOST_MODE  (1 << 1)
> > > -#define OTG_MODE   (1 << 0)
> > > +#define DEV_MODE   BIT(2)
> > > +#define HOST_MODE  BIT(1)
> > > +#define OTG_MODE   BIT(0)
> >
> > and these?
>
> Sorry, will delete this improvement
>
> >
> > >
> > >  /* USB3_INT_REG */
> > >  #define CLK_125_REQ        BIT(29)
> > >  #define LPM_CLK_REQ        BIT(28)
> > >  #define DEVU3_WAEKUP_EN    BIT(14)
> > >  #define OTG_WAKEUP_EN      BIT(12)
> > > -#define DEV_INT_EN (3 << 8) /* DEV INT b9:8 */
> > > -#define HOST_INT1_EN (1 << 0) /* HOST INT b7:0 */
> > > +#define DEV_INT_EN GENMASK(9, 8) /* DEV INT b9:8 */
> > > +#define HOST_INT1_EN       BIT(0) /* HOST INT b7:0 */
> >
> Sorry, will delete this improvement
>
> >
> > > @@ -62,15 +64,34 @@
> > >  #define HOST_POWER_ON_READY        BIT(12)
> > >
> > >  /* USB3_SSPHY_STATUS */
> > > -#define CLK_VALID_MASK             (0x3f << 26)
> > > -#define CLK_VALID_COMPARE_BITS     (0xf << 28)
> > > -#define PHY_REFCLK_REQ             (1 << 0)
> > > +#define CLK_VALID_MASK             GENMASK(31, 26)
> > > +#define CLK_VALID_COMPARE_BITS     GENMASK(31, 28)
> > > +#define PHY_REFCLK_REQ             BIT(0)
> >
>
> Sorry, will delete this improvement
>
> >
> > > +/* OTG registers definition */
> > > +#define OTGSTS             0x4
> > > +/* OTGSTS */
> > > +#define OTG_NRDY   BIT(11)
> > > +
> > > +/* xHCI registers definition  */
> > > +#define XECP_PM_PMCSR              0x8018
> > > +#define XECP_AUX_CTRL_REG1 0x8120
> > > +
> > > +/* Register bits definition */
> > > +/* XECP_AUX_CTRL_REG1 */
> > > +#define CFG_RXDET_P3_EN            BIT(15)
> > > +
> > > +/* XECP_PM_PMCSR */
> > > +#define PS_MASK                    GENMASK(1, 0)
> > > +#define PS_D0                      0
> > > +#define PS_D1                      1
> >
> > I think only these are part of $subject
>
> Yes, you are right.
>
> >
> > >  struct cdns_imx {
> > >     struct device *dev;
> > >     void __iomem *noncore;
> > >     struct clk_bulk_data *clks;
> > >     int num_clks;
> > > +   struct platform_device *cdns3_pdev;
> > >  };
> > >
> > >  static inline u32 cdns_imx_readl(struct cdns_imx *data, u32 offset)
> > > @@ -126,6 +147,20 @@ static int cdns_imx_noncore_init(struct cdns_imx *data)
> > >     return ret;
> > >  }
> > >
> > > +static int cdns_imx_platform_suspend(struct device *dev,
> > > +   bool suspend, bool wakeup);
> > > +static struct cdns3_platform_data cdns_imx_pdata = {
> >
> > make it const?
>
> Will change

After thinking more, the platform data may be different for
different glue layer platforms. I will keep this unchanging.

Peter