[PATCH v2 0/9] usb: some PM changes for cdns3 and xhci-plat

[PATCH v2 0/9] usb: some PM changes for cdns3 and xhci-plat

[Peter Chen]

Hi Felipe & Mathias,

In this series, it adds cdns3 runtime PM support as well as some
xhci-plat changes, and verified at NXP i.MX8QM and i.MX8QXP platforms.

Patch 1-3: add main runtime PM support for cdns3 core and imx glue layer
Patch 4-9: add platform data support and two quirks for xhci-plat.c.
- .suspend_quirk for platform special sequence between xhci_bus_suspend
and xhci_suspend.
- skip_phy_initialization for skip PHY management from HCD core.

Changes for v1:
- 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.

Peter Chen (9):
  usb: cdns3: introduce cdns3_set_phy_power API
  usb: cdns3: add runtime PM support
  usb: cdns3: imx: add glue layer runtime pm implementation
  usb: host: xhci-plat: add platform data support
  usb: host: xhci-plat: add .suspend_quirk for struct xhci_plat_priv
  usb: host: xhci-plat: delete the unnecessary code
  usb: host: xhci-plat: add priv flag for skip_phy_initialization
  usb: cdns3: host: add .suspend_quirk for xhci-plat.c
  usb: cdns3: host: add xhci_plat_priv's flag skip_phy_initialization

 drivers/usb/cdns3/cdns3-imx.c   | 179 +++++++++++++++++++++++++++++++-
 drivers/usb/cdns3/core.c        | 163 +++++++++++++++++++++++------
 drivers/usb/cdns3/core.h        |  14 +++
 drivers/usb/cdns3/drd.c         |   7 ++
 drivers/usb/cdns3/gadget.c      |   4 +
 drivers/usb/cdns3/host-export.h |   6 ++
 drivers/usb/cdns3/host.c        |  48 +++++++++
 drivers/usb/host/xhci-plat.c    |  36 ++++++-
 drivers/usb/host/xhci-plat.h    |   2 +
 9 files changed, 420 insertions(+), 39 deletions(-)

-- 
2.17.1

[PATCH v2 1/9] usb: cdns3: introduce cdns3_set_phy_power API

[Peter Chen]

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

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index 19bbb5b7e6b6..bfd09aa98c12 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role)
 	return ret;
 }
 
+static int cdns3_set_phy_power(struct cdns3 *cdns, bool on)
+{
+	int ret = 0;
+
+	if (on) {
+		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;
+		}
+	} else {
+		phy_power_off(cdns->usb3_phy);
+		phy_power_off(cdns->usb2_phy);
+	}
+
+	return 0;
+}
+
 /**
  * cdns3_probe - probe for cdns3 core device
  * @pdev: Pointer to cdns3 core platform device
@@ -477,14 +499,10 @@ static int cdns3_probe(struct platform_device *pdev)
 	if (ret)
 		goto err1;
 
-	ret = phy_power_on(cdns->usb2_phy);
+	ret = cdns3_set_phy_power(cdns, true);
 	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;
@@ -496,16 +514,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);
@@ -522,14 +540,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);
+	cdns3_set_phy_power(cdns, false);
 err2:
 	phy_exit(cdns->usb3_phy);
 err1:
@@ -553,8 +568,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);
+	cdns3_set_phy_power(cdns, false);
 	phy_exit(cdns->usb2_phy);
 	phy_exit(cdns->usb3_phy);
 	return 0;
-- 
2.17.1

RE: [PATCH v2 1/9] usb: cdns3: introduce cdns3_set_phy_power API

[Jun Li]

Hi,
> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月24日 7:23
> To: balbi@kernel.org; mathias.nyman@intel.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun Li
> <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> Subject: [PATCH v2 1/9] usb: cdns3: introduce cdns3_set_phy_power API
> 
> Since we have both USB2 and USB3 PHYs for cdns3 controller, it is better we have
> a unity API to handle both USB2 and USB3's power, it could simplify code for error
> handling and further power management implementation.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

Reviewed-by: Jun Li <jun.li@nxp.com>

> ---
>  drivers/usb/cdns3/core.c | 44 ++++++++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index
> 19bbb5b7e6b6..bfd09aa98c12 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -384,6 +384,28 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum
> usb_role role)
>  	return ret;
>  }
> 
> +static int cdns3_set_phy_power(struct cdns3 *cdns, bool on) {
> +	int ret = 0;
> +
> +	if (on) {
> +		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;
> +		}
> +	} else {
> +		phy_power_off(cdns->usb3_phy);
> +		phy_power_off(cdns->usb2_phy);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * cdns3_probe - probe for cdns3 core device
>   * @pdev: Pointer to cdns3 core platform device @@ -477,14 +499,10 @@ static int
> cdns3_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err1;
> 
> -	ret = phy_power_on(cdns->usb2_phy);
> +	ret = cdns3_set_phy_power(cdns, true);
>  	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; @@ -496,16 +514,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);
> @@ -522,14 +540,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);
> +	cdns3_set_phy_power(cdns, false);
>  err2:
>  	phy_exit(cdns->usb3_phy);
>  err1:
> @@ -553,8 +568,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);
> +	cdns3_set_phy_power(cdns, false);
>  	phy_exit(cdns->usb2_phy);
>  	phy_exit(cdns->usb3_phy);
>  	return 0;
> --
> 2.17.1

[PATCH v2 2/9] 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 has not implemented
glue layer support for runtime PM.

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_drd_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_runtime_get to wakeup controller, 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.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/core.c   | 119 +++++++++++++++++++++++++++++++------
 drivers/usb/cdns3/core.h   |  14 +++++
 drivers/usb/cdns3/drd.c    |   7 +++
 drivers/usb/cdns3/gadget.c |   4 ++
 drivers/usb/cdns3/host.c   |   7 +++
 5 files changed, 134 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c
index bfd09aa98c12..5221c1306e64 100644
--- a/drivers/usb/cdns3/core.c
+++ b/drivers/usb/cdns3/core.c
@@ -432,6 +432,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);
 
@@ -525,9 +526,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,
@@ -574,52 +577,134 @@ 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;
+	unsigned long flags;
+
+	spin_lock_irqsave(&cdns->lock, flags);
+	if (cdns->pdata && cdns->pdata->platform_suspend)
+		ret = cdns->pdata->platform_suspend(dev, suspend, wakeup);
+
+	spin_unlock_irqrestore(&cdns->lock, flags);
+
+	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 = false;
 
-	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);
+	cdns3_set_phy_power(cdns, false);
+	spin_lock_irqsave(&cdns->lock, flags);
+	cdns->in_lpm = true;
+	spin_unlock_irqrestore(&cdns->lock, flags);
 
 	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 = cdns3_set_phy_power(cdns, true);
+	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_int) {
+		cdns->wakeup_int = false;
+		if (cdns->role == USB_ROLE_HOST) {
+			/* Trigger xhci-plat.c runtime runtime */
+			pm_runtime_get(&cdns->host_dev->dev);
+			pm_runtime_mark_last_busy(&cdns->host_dev->dev);
+			pm_runtime_put_autosuspend(&cdns->host_dev->dev);
+			/* balence the pm_runtime_get at cdns3_drd_irq */
+			pm_runtime_mark_last_busy(cdns->dev);
+			pm_runtime_put_autosuspend(cdns->dev);
+		}
+
+		enable_irq(cdns->otg_irq);
+	}
+
+	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..9eb8aeea2d20 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
@@ -62,6 +68,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_int: wakeup interrupt occurs
+ * @pdata: platform data from glue layer
+ * @lock: spinlock structure
  */
 struct cdns3 {
 	struct device			*dev;
@@ -89,6 +99,10 @@ struct cdns3 {
 	struct mutex			mutex;
 	enum usb_dr_mode		dr_mode;
 	struct usb_role_switch		*role_sw;
+	bool				in_lpm;
+	bool				wakeup_int;
+	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 58089841ed52..292ea248c0ec 100644
--- a/drivers/usb/cdns3/drd.c
+++ b/drivers/usb/cdns3/drd.c
@@ -278,6 +278,13 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
 	struct cdns3 *cdns = data;
 	u32 reg;
 
+	if (cdns->in_lpm) {
+		disable_irq_nosync(irq);
+		cdns->wakeup_int = true;
+		pm_runtime_get(cdns->dev);
+		return IRQ_HANDLED;
+	}
+
 	if (cdns->dr_mode != USB_DR_MODE_OTG)
 		return ret;
 
diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c
index 559e5c01c2b0..ccb257d9daa6 100644
--- a/drivers/usb/cdns3/gadget.c
+++ b/drivers/usb/cdns3/gadget.c
@@ -1766,9 +1766,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 ad788bf3fe4f..b579ef15f4e0 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_switch_host(cdns, 1);
 
@@ -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

RE: [PATCH v2 2/9] usb: cdns3: add runtime PM support

[Jun Li]

Hi,

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月24日 7:23
> To: balbi@kernel.org; mathias.nyman@intel.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun Li
> <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> Subject: [PATCH v2 2/9] usb: cdns3: add runtime PM support
> 
> Introduce runtime PM and wakeup interrupt handler for cdns3, the runtime PM is default
> off since other cdns3 has not implemented glue layer support for runtime PM.
> 
> 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_drd_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_runtime_get to wakeup controller, 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.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/core.c   | 119 +++++++++++++++++++++++++++++++------
>  drivers/usb/cdns3/core.h   |  14 +++++
>  drivers/usb/cdns3/drd.c    |   7 +++
>  drivers/usb/cdns3/gadget.c |   4 ++
>  drivers/usb/cdns3/host.c   |   7 +++
>  5 files changed, 134 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index
> bfd09aa98c12..5221c1306e64 100644
> --- a/drivers/usb/cdns3/core.c
> +++ b/drivers/usb/cdns3/core.c
> @@ -432,6 +432,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);
> 
> @@ -525,9 +526,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, @@ -574,52
> +577,134 @@ static int cdns3_remove(struct platform_device *pdev)
>  	return 0;
>  }
> 
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM

__maybe_unused

> 
> -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;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cdns->lock, flags);
> +	if (cdns->pdata && cdns->pdata->platform_suspend)
> +		ret = cdns->pdata->platform_suspend(dev, suspend, wakeup);
> +
> +	spin_unlock_irqrestore(&cdns->lock, flags);
> +
> +	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 = false;

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);
> +	cdns3_set_phy_power(cdns, false);
> +	spin_lock_irqsave(&cdns->lock, flags);
> +	cdns->in_lpm = true;
> +	spin_unlock_irqrestore(&cdns->lock, flags);
> 
>  	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 = cdns3_set_phy_power(cdns, true);
> +	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_int) {
> +		cdns->wakeup_int = false;
> +		if (cdns->role == USB_ROLE_HOST) {
> +			/* Trigger xhci-plat.c runtime runtime */
> +			pm_runtime_get(&cdns->host_dev->dev);
> +			pm_runtime_mark_last_busy(&cdns->host_dev->dev);
> +			pm_runtime_put_autosuspend(&cdns->host_dev->dev);
> +			/* balence the pm_runtime_get at cdns3_drd_irq */

%s/balence/balance

> +			pm_runtime_mark_last_busy(cdns->dev);
> +			pm_runtime_put_autosuspend(cdns->dev);

A simple
if (cdns->role == USB_ROLE_HOST)
      pm_runtime_resume(&cdns->host_dev->dev);
in low power event handler can't work?

> +		}
> +
> +		enable_irq(cdns->otg_irq);
> +	}
> +
> +	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);

Seems this hasn't been used, I did not find the implementation.
this role->suspend() is only to be used in system suspend but not in runtime suspend?

> +		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..9eb8aeea2d20 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 @@ -62,6 +68,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_int: wakeup interrupt occurs
> + * @pdata: platform data from glue layer
> + * @lock: spinlock structure
>   */
>  struct cdns3 {
>  	struct device			*dev;
> @@ -89,6 +99,10 @@ struct cdns3 {
>  	struct mutex			mutex;
>  	enum usb_dr_mode		dr_mode;
>  	struct usb_role_switch		*role_sw;
> +	bool				in_lpm;
> +	bool				wakeup_int;
> +	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
> 58089841ed52..292ea248c0ec 100644
> --- a/drivers/usb/cdns3/drd.c
> +++ b/drivers/usb/cdns3/drd.c
> @@ -278,6 +278,13 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
>  	struct cdns3 *cdns = data;
>  	u32 reg;
> 
> +	if (cdns->in_lpm) {
> +		disable_irq_nosync(irq);
> +		cdns->wakeup_int = true;
> +		pm_runtime_get(cdns->dev);
> +		return IRQ_HANDLED;
> +	}

All low power events go through otg irq?

Li Jun

> +
>  	if (cdns->dr_mode != USB_DR_MODE_OTG)
>  		return ret;
> 
> diff --git a/drivers/usb/cdns3/gadget.c b/drivers/usb/cdns3/gadget.c index
> 559e5c01c2b0..ccb257d9daa6 100644
> --- a/drivers/usb/cdns3/gadget.c
> +++ b/drivers/usb/cdns3/gadget.c
> @@ -1766,9 +1766,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
> ad788bf3fe4f..b579ef15f4e0 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_switch_host(cdns, 1);
> 
> @@ -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

RE: [PATCH v2 2/9] usb: cdns3: add runtime PM support

[Jun Li]

> -----Original Message-----
> From: Jun Li <jun.li@nxp.com>
> Sent: 2020年5月24日 13:58
> To: Peter Chen <peter.chen@nxp.com>; balbi@kernel.org; mathias.nyman@intel.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org
> Subject: RE: [PATCH v2 2/9] usb: cdns3: add runtime PM support
...

> > +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);
> 
> Seems this hasn't been used, I did not find the implementation.

Ah, see gadget defined it.

> this role->suspend() is only to be used in system suspend but not in runtime suspend?
> 
> > +		spin_unlock_irqrestore(&cdns->lock, flags);
> >  	}
> >

Re: [PATCH v2 2/9] usb: cdns3: add runtime PM support

[Peter Chen]

On 20-05-24 05:57:53, Jun Li wrote:
> > 
> > -#ifdef CONFIG_PM_SLEEP
> > +#ifdef CONFIG_PM
> 
> __maybe_unused

Since there are several functions will be used in PM routine,
it needs to add '__maybe_unused' for every functions, So, I choose
to use MACRO directly.

> 
> > 
> > -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;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&cdns->lock, flags);
> > +	if (cdns->pdata && cdns->pdata->platform_suspend)
> > +		ret = cdns->pdata->platform_suspend(dev, suspend, wakeup);
> > +
> > +	spin_unlock_irqrestore(&cdns->lock, flags);
> > +
> > +	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 = false;
> 
> wakeup = device_may_wakeup(dev)?

No, it is for wakeup indicator for both runtime and system. At runtime,
the USB wakeup needs to be enabled all time, not depends on
/sys/.../power/wakeup.

> 
> > 
> > -	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);
> > +	cdns3_set_phy_power(cdns, false);
> > +	spin_lock_irqsave(&cdns->lock, flags);
> > +	cdns->in_lpm = true;
> > +	spin_unlock_irqrestore(&cdns->lock, flags);
> > 
> >  	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 = cdns3_set_phy_power(cdns, true);
> > +	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_int) {
> > +		cdns->wakeup_int = false;
> > +		if (cdns->role == USB_ROLE_HOST) {
> > +			/* Trigger xhci-plat.c runtime runtime */
> > +			pm_runtime_get(&cdns->host_dev->dev);
> > +			pm_runtime_mark_last_busy(&cdns->host_dev->dev);
> > +			pm_runtime_put_autosuspend(&cdns->host_dev->dev);
> > +			/* balence the pm_runtime_get at cdns3_drd_irq */
> 
> %s/balence/balance

Ok

> 
> > +			pm_runtime_mark_last_busy(cdns->dev);
> > +			pm_runtime_put_autosuspend(cdns->dev);
> 
> A simple
> if (cdns->role == USB_ROLE_HOST)
>       pm_runtime_resume(&cdns->host_dev->dev);
> in low power event handler can't work?

I tried, the pm_runtime_resume is sync pm request, it will cause
rpm_resume nested because the cdns->dev runtime_status is resuming
(it is during the runtime resume stage).

Using pm_request_resume could work since it is async pm, but below
are still needed to balance pm_runtime_get at wakeup interrupt handler.

+			pm_runtime_mark_last_busy(cdns->dev);
+			pm_runtime_put_autosuspend(cdns->dev);

> 
> > +		}
> > +
> > +		enable_irq(cdns->otg_irq);
> > +	}
> > +
> > +	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);
> 
> Seems this hasn't been used, I did not find the implementation.
> this role->suspend() is only to be used in system suspend but not in runtime suspend?

I don't change it in this series, it is used for system PM.

> >  int cdns3_hw_role_switch(struct cdns3 *cdns); diff --git
> > a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c index
> > 58089841ed52..292ea248c0ec 100644
> > --- a/drivers/usb/cdns3/drd.c
> > +++ b/drivers/usb/cdns3/drd.c
> > @@ -278,6 +278,13 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
> >  	struct cdns3 *cdns = data;
> >  	u32 reg;
> > 
> > +	if (cdns->in_lpm) {
> > +		disable_irq_nosync(irq);
> > +		cdns->wakeup_int = true;
> > +		pm_runtime_get(cdns->dev);
> > +		return IRQ_HANDLED;
> > +	}
> 
> All low power events go through otg irq?

drd.c is built into core, so no matter, peripheral(host)-only or 
drd configuration, this drd interrupt handler are all called firstly.

One thing needs to change at next revision is I only consider
the interrupt number are the same for all three modules
(peripheral/host/otg), I will cover the different interrupt
number use case.

Peter

-- 

Thanks,
Peter Chen

RE: [PATCH v2 2/9] usb: cdns3: add runtime PM support

[Jun Li]

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月25日 10:57
> To: Jun Li <jun.li@nxp.com>
> Cc: balbi@kernel.org; mathias.nyman@intel.com; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; pawell@cadence.com; rogerq@ti.com;
> gregkh@linuxfoundation.org
> Subject: Re: [PATCH v2 2/9] usb: cdns3: add runtime PM support
> 
> On 20-05-24 05:57:53, Jun Li wrote:
> > >
> > > -#ifdef CONFIG_PM_SLEEP
> > > +#ifdef CONFIG_PM
> >
> > __maybe_unused
> 
> Since there are several functions will be used in PM routine, it needs to add
> '__maybe_unused' for every functions, So, I choose to use MACRO directly.
> 
> >
> > >
> > > -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;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&cdns->lock, flags);
> > > +	if (cdns->pdata && cdns->pdata->platform_suspend)
> > > +		ret = cdns->pdata->platform_suspend(dev, suspend, wakeup);
> > > +
> > > +	spin_unlock_irqrestore(&cdns->lock, flags);
> > > +
> > > +	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 = false;
> >
> > wakeup = device_may_wakeup(dev)?
> 
> No, it is for wakeup indicator for both runtime and system. At runtime, the USB
> wakeup needs to be enabled all time, not depends on /sys/.../power/wakeup.

I mean for system pm:
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);
> > > +	cdns3_set_phy_power(cdns, false);
> > > +	spin_lock_irqsave(&cdns->lock, flags);
> > > +	cdns->in_lpm = true;
> > > +	spin_unlock_irqrestore(&cdns->lock, flags);
> > >
> > >  	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 = cdns3_set_phy_power(cdns, true);
> > > +	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_int) {
> > > +		cdns->wakeup_int = false;
> > > +		if (cdns->role == USB_ROLE_HOST) {
> > > +			/* Trigger xhci-plat.c runtime runtime */
> > > +			pm_runtime_get(&cdns->host_dev->dev);
> > > +			pm_runtime_mark_last_busy(&cdns->host_dev->dev);
> > > +			pm_runtime_put_autosuspend(&cdns->host_dev->dev);
> > > +			/* balence the pm_runtime_get at cdns3_drd_irq */
> >
> > %s/balence/balance
> 
> Ok
> 
> >
> > > +			pm_runtime_mark_last_busy(cdns->dev);
> > > +			pm_runtime_put_autosuspend(cdns->dev);
> >
> > A simple
> > if (cdns->role == USB_ROLE_HOST)
> >       pm_runtime_resume(&cdns->host_dev->dev);
> > in low power event handler can't work?
> 
> I tried, the pm_runtime_resume is sync pm request, it will cause rpm_resume nested
> because the cdns->dev runtime_status is resuming (it is during the runtime resume
> stage).

if you only resume the cdns->host_dev->dev, I think there is no
nested resume, the sequence is
cdns-imx resume -> cdns->dev resume -> cdns->host_dev->dev resume.

> 
> Using pm_request_resume could work since it is async pm, but below are still needed
> to balance pm_runtime_get at wakeup interrupt handler.
> 
> +			pm_runtime_mark_last_busy(cdns->dev);
> +			pm_runtime_put_autosuspend(cdns->dev);
> 
> >
> > > +		}
> > > +
> > > +		enable_irq(cdns->otg_irq);
> > > +	}
> > > +
> > > +	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);
> >
> > Seems this hasn't been used, I did not find the implementation.
> > this role->suspend() is only to be used in system suspend but not in runtime suspend?
> 
> I don't change it in this series, it is used for system PM.

OK, understand this is the original code.
For imx runtime PM of this patch set, do you still expect this?
(i.e cdns3_gadget_suspend() will not be called for gadget runtime
suspend).

Li Jun

> 
> > >  int cdns3_hw_role_switch(struct cdns3 *cdns); diff --git
> > > a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c index
> > > 58089841ed52..292ea248c0ec 100644
> > > --- a/drivers/usb/cdns3/drd.c
> > > +++ b/drivers/usb/cdns3/drd.c
> > > @@ -278,6 +278,13 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
> > >  	struct cdns3 *cdns = data;
> > >  	u32 reg;
> > >
> > > +	if (cdns->in_lpm) {
> > > +		disable_irq_nosync(irq);
> > > +		cdns->wakeup_int = true;
> > > +		pm_runtime_get(cdns->dev);
> > > +		return IRQ_HANDLED;
> > > +	}
> >
> > All low power events go through otg irq?
> 
> drd.c is built into core, so no matter, peripheral(host)-only or drd configuration,
> this drd interrupt handler are all called firstly.
> 
> One thing needs to change at next revision is I only consider the interrupt number
> are the same for all three modules (peripheral/host/otg), I will cover the different
> interrupt number use case.

For shared irq with the same number, disable_irq_nosync(irq)
can really disable the irq?

Li Jun
> 
> Peter
> 
> --
> 
> Thanks,
> Peter Chen

Re: [PATCH v2 2/9] usb: cdns3: add runtime PM support

[Peter Chen]

On 20-05-25 12:23:36, Jun Li wrote:
> > > > +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 = false;
> > >
> > > wakeup = device_may_wakeup(dev)?
> > 
> > No, it is for wakeup indicator for both runtime and system. At runtime, the USB
> > wakeup needs to be enabled all time, not depends on /sys/.../power/wakeup.
> 
> I mean for system pm:
> if (PMSG_IS_AUTO(msg))
> 	wakeup = true;
> else
> 	wakeup = device_may_wakeup(dev);

Ok, I will change. In this patch series, it only supports
wakeup for runtime PM. For system PM wakeup support, I plan
to support later.

> > > > +	if (cdns->wakeup_int) {
> > > > +		cdns->wakeup_int = false;
> > > > +		if (cdns->role == USB_ROLE_HOST) {
> > > > +			/* Trigger xhci-plat.c runtime runtime */
> > > > +			pm_runtime_get(&cdns->host_dev->dev);
> > > > +			pm_runtime_mark_last_busy(&cdns->host_dev->dev);
> > > > +			pm_runtime_put_autosuspend(&cdns->host_dev->dev);
> > > > +			/* balence the pm_runtime_get at cdns3_drd_irq */
> > >
> > > %s/balence/balance
> > 
> > Ok
> > 
> > >
> > > > +			pm_runtime_mark_last_busy(cdns->dev);
> > > > +			pm_runtime_put_autosuspend(cdns->dev);
> > >
> > > A simple
> > > if (cdns->role == USB_ROLE_HOST)
> > >       pm_runtime_resume(&cdns->host_dev->dev);
> > > in low power event handler can't work?
> > 
> > I tried, the pm_runtime_resume is sync pm request, it will cause rpm_resume nested
> > because the cdns->dev runtime_status is resuming (it is during the runtime resume
> > stage).
> 
> if you only resume the cdns->host_dev->dev, I think there is no
> nested resume, the sequence is
> cdns-imx resume -> cdns->dev resume -> cdns->host_dev->dev resume.
> 

I only resume the cdns->host_dev->dev like you suggested for testing,
There is one more rpm resume for cdns->dev, see below:

cdns-imx resume (1) -> cdns->dev resume(2) -> cdns->host_dev->dev
resume(3) -> cdns->dev resume (4)

Trace log and rpm status like below:

root@imx8qmmek:~# cat /sys/kernel/debug/tracing/trace | grep xhci-hcd.1.auto
     kworker/0:0-5     [000] d..1   144.830873: rpm_resume: xhci-hcd.1.auto flags-0 cnt-0  dep-0  auto-1 p-0 irq-0 chi
ld-0
root@imx8qmmek:~# cat /sys/kernel/debug/tracing/trace | grep 5b130000.cdns3 
          <idle>-0     [000] d.h2   144.830155: rpm_resume: 5b130000.cdns3 flags-5 cnt-1  dep-0  auto-1 p-0 irq-0 chil
d-0
          <idle>-0     [000] dNh2   144.830166: rpm_return_int: rpm_resume+0x114/0x758:5b130000.cdns3 ret=0
     kworker/0:0-5     [000] d..1   144.830189: rpm_resume: 5b130000.cdns3 flags-2 cnt-1  dep-0  auto-1 p-0 irq-0 chil
d-0
     kworker/0:0-5     [000] d..1   144.830875: rpm_resume: 5b130000.cdns3 flags-0 cnt-2  dep-0  auto-1 p-0 irq-0 chil
d-0

imx8qmmek:~# cat /sys/bus/platform/devices/5b130000.cdns3/power/runtime_status
resuming

If one device is resuming, the rpm_resume will be scheduled, and wait
the previous resume finish.

> > > > +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);
> > >
> > > Seems this hasn't been used, I did not find the implementation.
> > > this role->suspend() is only to be used in system suspend but not in runtime suspend?
> > 
> > I don't change it in this series, it is used for system PM.
> 
> OK, understand this is the original code.
> For imx runtime PM of this patch set, do you still expect this?
> (i.e cdns3_gadget_suspend() will not be called for gadget runtime
> suspend).
> 

This code may need to purify in future for current mainline code.
The main purpose of this code was to disconnect host and re-connect
host after resume during the system suspend/resume. At runtime,
we don't need such operation since the runtime suspend doesn't be
called if the connection is there.

> 
> > 
> > > >  int cdns3_hw_role_switch(struct cdns3 *cdns); diff --git
> > > > a/drivers/usb/cdns3/drd.c b/drivers/usb/cdns3/drd.c index
> > > > 58089841ed52..292ea248c0ec 100644
> > > > --- a/drivers/usb/cdns3/drd.c
> > > > +++ b/drivers/usb/cdns3/drd.c
> > > > @@ -278,6 +278,13 @@ static irqreturn_t cdns3_drd_irq(int irq, void *data)
> > > >  	struct cdns3 *cdns = data;
> > > >  	u32 reg;
> > > >
> > > > +	if (cdns->in_lpm) {
> > > > +		disable_irq_nosync(irq);
> > > > +		cdns->wakeup_int = true;
> > > > +		pm_runtime_get(cdns->dev);
> > > > +		return IRQ_HANDLED;
> > > > +	}
> > >
> > > All low power events go through otg irq?
> > 
> > drd.c is built into core, so no matter, peripheral(host)-only or drd configuration,
> > this drd interrupt handler are all called firstly.
> > 
> > One thing needs to change at next revision is I only consider the interrupt number
> > are the same for all three modules (peripheral/host/otg), I will cover the different
> > interrupt number use case.
> 
> For shared irq with the same number, disable_irq_nosync(irq)
> can really disable the irq?
> 

When the controller is in low power mode, the normal controller logic
(peripheral/host/otg) will not trigger interrupt. The
disable_irq_nosync here is mainly to disable interrupt for wakeup
interrupt, and wait async runtime resume to clear the status, then
re-enable the interrupt.

-- 

Thanks,
Peter Chen

Re: [PATCH v2 2/9] usb: cdns3: add runtime PM support

[Chunfeng Yun]

Hi Peter,
On Sun, 2020-05-24 at 07:22 +0800, Peter Chen wrote:
> Introduce runtime PM and wakeup interrupt handler for cdns3,
> the runtime PM is default off since other cdns3 has not implemented
> glue layer support for runtime PM.
> 
> 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_drd_irq. 
Wonder to know that CPUs may be powered off in this case? 
Do you consider the case when usb controller is in low power mode, and
also power off CPUs, then how to wakeup usb controller?

Thanks

> 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_runtime_get to wakeup controller, 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.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/core.c   | 119 +++++++++++++++++++++++++++++++------
>  drivers/usb/cdns3/core.h   |  14 +++++
>  drivers/usb/cdns3/drd.c    |   7 +++
>  drivers/usb/cdns3/gadget.c |   4 ++
>  drivers/usb/cdns3/host.c   |   7 +++
>  5 files changed, 134 insertions(+), 17 deletions(-)

Re: [PATCH v2 2/9] 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 has not implemented
> > glue layer support for runtime PM.
> >
> > 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_drd_irq.
> Wonder to know that CPUs may be powered off in this case?

Yes

> Do you consider the case when usb controller is in low power mode, and
> also power off CPUs, then how to wakeup usb controller?
>

About USB wakeup for system suspend, there are still some works need
to do for this driver, will do later.

For system wakeup, it is platform specific. If the main cores are
powered off totally,
and do not respond any wakeup events, it needs the other core to get this wakeup
event, and power on main cores. After main cores are powered on, the
wakeup interrupt
status are there (if controller is not powered off), the controller
driver could handle
this wakeup event.

Peter

> Thanks
>
> > 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_runtime_get to wakeup controller, 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.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/cdns3/core.c   | 119 +++++++++++++++++++++++++++++++------
> >  drivers/usb/cdns3/core.h   |  14 +++++
> >  drivers/usb/cdns3/drd.c    |   7 +++
> >  drivers/usb/cdns3/gadget.c |   4 ++
> >  drivers/usb/cdns3/host.c   |   7 +++
> >  5 files changed, 134 insertions(+), 17 deletions(-)
>

[PATCH v2 3/9] 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.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/cdns3-imx.c | 179 +++++++++++++++++++++++++++++++++-
 1 file changed, 177 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-imx.c b/drivers/usb/cdns3/cdns3-imx.c
index aba988e71958..9d247de8e7eb 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
@@ -66,11 +68,30 @@
 #define CLK_VALID_COMPARE_BITS	(0xf << 28)
 #define PHY_REFCLK_REQ		(1 << 0)
 
+/* OTG registers definition */
+#define OTGSTS		0x4
+/* OTGSTS */
+#define OTG_NRDY	(1 << 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		(1 << 15)
+
+/* XECP_PM_PMCSR */
+#define PS_MASK			(3 << 0)
+#define PS_D0			0
+#define PS_D1			(1 << 0)
+
 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,141 @@ 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 = 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_atomic(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_atomic(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_atomic(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 {
+		/* wait CLK_125_REQ to be 1 */
+		value = cdns_imx_readl(data, USB3_INT_REG);
+		ret = readl_poll_timeout_atomic(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");
+
+		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 for mdctrl_clk_status is cleared */
+		value = cdns_imx_readl(data, USB3_CORE_STATUS);
+		ret = readl_poll_timeout_atomic(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_atomic(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;
+}
+
+#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 +380,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 v2 3/9] usb: cdns3: imx: add glue layer runtime pm implementation

[Jun Li]

Hi

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月24日 7:23
> To: balbi@kernel.org; mathias.nyman@intel.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun Li
> <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> Subject: [PATCH v2 3/9] usb: cdns3: imx: add glue layer runtime pm implementation
> 
> Add imx glue layer runtime pm implementation, and the runtime pm is default off.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/cdns3/cdns3-imx.c | 179 +++++++++++++++++++++++++++++++++-
>  1 file changed, 177 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-imx.c b/drivers/usb/cdns3/cdns3-imx.c index
> aba988e71958..9d247de8e7eb 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
> @@ -66,11 +68,30 @@
>  #define CLK_VALID_COMPARE_BITS	(0xf << 28)

GENMASK(31, 28)

>  #define PHY_REFCLK_REQ		(1 << 0)

BIT(0), also for others

> 
> +/* OTG registers definition */
> +#define OTGSTS		0x4
> +/* OTGSTS */
> +#define OTG_NRDY	(1 << 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		(1 << 15)
> +
> +/* XECP_PM_PMCSR */
> +#define PS_MASK			(3 << 0)
> +#define PS_D0			0
> +#define PS_D1			(1 << 0)
> +
>  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,141 @@ 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 = 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_atomic(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_atomic(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_atomic(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 {
> +		/* wait CLK_125_REQ to be 1 */
> +		value = cdns_imx_readl(data, USB3_INT_REG);
> +		ret = readl_poll_timeout_atomic(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");
> +
> +		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 for mdctrl_clk_status is cleared */
> +		value = cdns_imx_readl(data, USB3_CORE_STATUS);
> +		ret = readl_poll_timeout_atomic(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_atomic(otg_regs + OTGSTS, value,
> +			(value & OTG_NRDY) != OTG_NRDY,
> +			10, 100000);
> +		if (ret)
> +			dev_warn(parent, "wait OTG ready timeout\n");

Make sense to move forward if any of above timeout happens?

Li Jun

> +	}
> +
> +	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;
> +}
> +
> +#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 +380,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 v2 3/9] usb: cdns3: imx: add glue layer runtime pm implementation

[Peter Chen]

On 20-05-24 06:55:02, Jun Li wrote:
> > 
> >  #define USB3_CORE_CTRL1    0x00
> >  #define USB3_CORE_CTRL2    0x04
> > @@ -66,11 +68,30 @@
> >  #define CLK_VALID_COMPARE_BITS	(0xf << 28)
> 
> GENMASK(31, 28)
> 
> >  #define PHY_REFCLK_REQ		(1 << 0)
> 
> BIT(0), also for others

Will change for all.

> 
> > 
> > +/* OTG registers definition */
> > +#define OTGSTS		0x4
> > +/* OTGSTS */
> > +#define OTG_NRDY	(1 << 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		(1 << 15)
> > +
> > +/* XECP_PM_PMCSR */
> > +#define PS_MASK			(3 << 0)
> > +#define PS_D0			0
> > +#define PS_D1			(1 << 0)
> > +
> > +		/* wait for mdctrl_clk_status is cleared */
> > +		value = cdns_imx_readl(data, USB3_CORE_STATUS);
> > +		ret = readl_poll_timeout_atomic(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_atomic(otg_regs + OTGSTS, value,
> > +			(value & OTG_NRDY) != OTG_NRDY,
> > +			10, 100000);
> > +		if (ret)
> > +			dev_warn(parent, "wait OTG ready timeout\n");
> 
> Make sense to move forward if any of above timeout happens?

I will return -ETIMEOUT for all timeout case, thanks.

Peter
> 
> Li Jun
> 
> > +	}
> > +
> > +	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;
> > +}
> > +
> > +#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 +380,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
> 

-- 

Thanks,
Peter Chen

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

[kbuild test robot]

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

Hi Peter,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on balbi-usb/testing/next next-20200522]
[cannot apply to shawnguo/for-next v5.7-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Peter-Chen/usb-some-PM-changes-for-cdns3-and-xhci-plat/20200524-072451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: s390-randconfig-s002-20200524 (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-240-gf0fe1cd9-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 ARCH=s390 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/usb/cdns3/cdns3-imx.c:150:12: warning: 'cdns_imx_platform_suspend' used but never defined
150 | static int cdns_imx_platform_suspend(struct device *dev,
|            ^~~~~~~~~~~~~~~~~~~~~~~~~

vim +/cdns_imx_platform_suspend +150 drivers/usb/cdns3/cdns3-imx.c

   149	
 > 150	static int cdns_imx_platform_suspend(struct device *dev,
   151		bool suspend, bool wakeup);
   152	static struct cdns3_platform_data cdns_imx_pdata = {
   153		.platform_suspend = cdns_imx_platform_suspend,
   154	};
   155	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33792 bytes --]

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

[Peter Chen]

On 20-05-25 12:53:11, kbuild test robot wrote:
> Hi Peter,
> 
> I love your patch! Perhaps something to improve:
> 
> [auto build test WARNING on usb/usb-testing]
> [also build test WARNING on balbi-usb/testing/next next-20200522]
> [cannot apply to shawnguo/for-next v5.7-rc7]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fstackoverflow.com%2Fa%2F37406982&data=02%7C01%7Cpeter.chen%40nxp.com%7Cd98349851abc4e0abf1408d8006799d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637259792390096024&sdata=yRFR26%2FjlA8dg0wS4xGplfPvXtG8tYa1qb2lroSsq0Y%3D&reserved=0]
> 
> url:    https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2F0day-ci%2Flinux%2Fcommits%2FPeter-Chen%2Fusb-some-PM-changes-for-cdns3-and-xhci-plat%2F20200524-072451&data=02%7C01%7Cpeter.chen%40nxp.com%7Cd98349851abc4e0abf1408d8006799d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637259792390096024&sdata=mJmxpr0897Sk415jiIGMg0Q7jIxAO93t%2FvUoJxAOXIg%3D&reserved=0
> base:   https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fgregkh%2Fusb.git&data=02%7C01%7Cpeter.chen%40nxp.com%7Cd98349851abc4e0abf1408d8006799d7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637259792390096024&sdata=nrCKZGGd4%2FFUKaIGm1PTn2OzeJ3XbSZvb9sn7GHgFBw%3D&reserved=0 usb-testing
> config: s390-randconfig-s002-20200524 (attached as .config)
> compiler: s390-linux-gcc (GCC) 9.3.0
> reproduce:
>         # apt-get install sparse
>         # sparse version: v0.6.1-240-gf0fe1cd9-dirty
>         # save the attached .config to linux build tree
>         make W=1 C=1 ARCH=s390 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kbuild test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>, old ones prefixed by <<):
> 
> >> drivers/usb/cdns3/cdns3-imx.c:150:12: warning: 'cdns_imx_platform_suspend' used but never defined

Will fix it at next revision, thanks.

Peter
> 150 | static int cdns_imx_platform_suspend(struct device *dev,
> |            ^~~~~~~~~~~~~~~~~~~~~~~~~
> 
> vim +/cdns_imx_platform_suspend +150 drivers/usb/cdns3/cdns3-imx.c
> 
>    149	
>  > 150	static int cdns_imx_platform_suspend(struct device *dev,
>    151		bool suspend, bool wakeup);
>    152	static struct cdns3_platform_data cdns_imx_pdata = {
>    153		.platform_suspend = cdns_imx_platform_suspend,
>    154	};
>    155	
> 


-- 

Thanks,
Peter Chen

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

[kbuild test robot]

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

Hi Peter,

I love your patch! Yet something to improve:

[auto build test ERROR on usb/usb-testing]
[also build test ERROR on balbi-usb/testing/next next-20200522]
[cannot apply to shawnguo/for-next v5.7-rc7]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Peter-Chen/usb-some-PM-changes-for-cdns3-and-xhci-plat/20200524-072451
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: parisc-allyesconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> hppa-linux-ld: drivers/usb/cdns3/cdns3-imx.o:(.data+0x88): undefined reference to `cdns_imx_platform_suspend'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 61601 bytes --]

[PATCH v2 4/9] usb: host: xhci-plat: add platform data support

[Peter Chen]

Some xhci hosts (eg dwc3 and cdns3) do not use OF to create
platform device, they create xhci-plat platform device runtime.
And these platforms may also have quirks, and the quirks could
be supplied by their parent device through platform data.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index f6b4089bfc4a..38bea2b6a17d 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -264,7 +264,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_reg_clk;
 
-	priv_match = of_device_get_match_data(&pdev->dev);
+	if (pdev->dev.of_node)
+		priv_match = of_device_get_match_data(&pdev->dev);
+	else
+		priv_match = dev_get_platdata(&pdev->dev);
+
 	if (priv_match) {
 		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
 
-- 
2.17.1

Re: [PATCH v2 4/9] usb: host: xhci-plat: add platform data support

[Mathias Nyman]

On 24.5.2020 2.22, Peter Chen wrote:
> Some xhci hosts (eg dwc3 and cdns3) do not use OF to create
> platform device, they create xhci-plat platform device runtime.
> And these platforms may also have quirks, and the quirks could
> be supplied by their parent device through platform data.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/host/xhci-plat.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index f6b4089bfc4a..38bea2b6a17d 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -264,7 +264,11 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto disable_reg_clk;
>  
> -	priv_match = of_device_get_match_data(&pdev->dev);
> +	if (pdev->dev.of_node)
> +		priv_match = of_device_get_match_data(&pdev->dev);
> +	else
> +		priv_match = dev_get_platdata(&pdev->dev);
> +
>  	if (priv_match) {
>  		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
>  
> 

Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>

[PATCH v2 5/9] usb: host: xhci-plat: add .suspend_quirk for struct xhci_plat_priv

[Peter Chen]

Some platforms (eg cdns3) may have special sequences between
xhci_bus_suspend and xhci_suspend, add .suspend_quick for it.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 19 +++++++++++++++++++
 drivers/usb/host/xhci-plat.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 38bea2b6a17d..51e20464fe0a 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -54,6 +54,16 @@ static int xhci_priv_init_quirk(struct usb_hcd *hcd)
 	return priv->init_quirk(hcd);
 }
 
+static int xhci_priv_suspend_quirk(struct usb_hcd *hcd)
+{
+	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
+
+	if (!priv->suspend_quirk)
+		return 0;
+
+	return priv->suspend_quirk(hcd);
+}
+
 static int xhci_priv_resume_quirk(struct usb_hcd *hcd)
 {
 	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
@@ -401,7 +411,11 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 {
 	struct usb_hcd	*hcd = dev_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	int ret;
 
+	ret = xhci_priv_suspend_quirk(hcd);
+	if (ret)
+		return ret;
 	/*
 	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
 	 * to do wakeup during suspend. Since xhci_plat_suspend is currently
@@ -438,6 +452,11 @@ static int __maybe_unused xhci_plat_runtime_suspend(struct device *dev)
 {
 	struct usb_hcd  *hcd = dev_get_drvdata(dev);
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	int ret;
+
+	ret = xhci_priv_suspend_quirk(hcd);
+	if (ret)
+		return ret;
 
 	return xhci_suspend(xhci, true);
 }
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index b49f6447bd3a..1fb149d1fbce 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -15,6 +15,7 @@ struct xhci_plat_priv {
 	unsigned long long quirks;
 	void (*plat_start)(struct usb_hcd *);
 	int (*init_quirk)(struct usb_hcd *);
+	int (*suspend_quirk)(struct usb_hcd *);
 	int (*resume_quirk)(struct usb_hcd *);
 };
 
-- 
2.17.1

Re: [PATCH v2 5/9] usb: host: xhci-plat: add .suspend_quirk for struct xhci_plat_priv

[Mathias Nyman]

On 24.5.2020 2.23, Peter Chen wrote:
> Some platforms (eg cdns3) may have special sequences between
> xhci_bus_suspend and xhci_suspend, add .suspend_quick for it.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/host/xhci-plat.c | 19 +++++++++++++++++++
>  drivers/usb/host/xhci-plat.h |  1 +
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 38bea2b6a17d..51e20464fe0a 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -54,6 +54,16 @@ static int xhci_priv_init_quirk(struct usb_hcd *hcd)
>  	return priv->init_quirk(hcd);
>  }
>  
> +static int xhci_priv_suspend_quirk(struct usb_hcd *hcd)
> +{
> +	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
> +
> +	if (!priv->suspend_quirk)
> +		return 0;
> +
> +	return priv->suspend_quirk(hcd);
> +}
> +
>  static int xhci_priv_resume_quirk(struct usb_hcd *hcd)
>  {
>  	struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
> @@ -401,7 +411,11 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
>  {
>  	struct usb_hcd	*hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
> +	int ret;
>  
> +	ret = xhci_priv_suspend_quirk(hcd);
> +	if (ret)
> +		return ret;
>  	/*
>  	 * xhci_suspend() needs `do_wakeup` to know whether host is allowed
>  	 * to do wakeup during suspend. Since xhci_plat_suspend is currently
> @@ -438,6 +452,11 @@ static int __maybe_unused xhci_plat_runtime_suspend(struct device *dev)
>  {
>  	struct usb_hcd  *hcd = dev_get_drvdata(dev);
>  	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	int ret;
> +
> +	ret = xhci_priv_suspend_quirk(hcd);
> +	if (ret)
> +		return ret;
>  
>  	return xhci_suspend(xhci, true);
>  }
> diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
> index b49f6447bd3a..1fb149d1fbce 100644
> --- a/drivers/usb/host/xhci-plat.h
> +++ b/drivers/usb/host/xhci-plat.h
> @@ -15,6 +15,7 @@ struct xhci_plat_priv {
>  	unsigned long long quirks;
>  	void (*plat_start)(struct usb_hcd *);
>  	int (*init_quirk)(struct usb_hcd *);
> +	int (*suspend_quirk)(struct usb_hcd *);
>  	int (*resume_quirk)(struct usb_hcd *);
>  };
>  
> 

Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>

[PATCH v2 6/9] usb: host: xhci-plat: delete the unnecessary code

[Peter Chen]

The if {} condition is duplicated with outer if {} condition.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 51e20464fe0a..03d6bbe51919 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -283,8 +283,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
 
 		/* Just copy data for now */
-		if (priv_match)
-			*priv = *priv_match;
+		*priv = *priv_match;
 	}
 
 	device_wakeup_enable(hcd->self.controller);
-- 
2.17.1

RE: [PATCH v2 6/9] usb: host: xhci-plat: delete the unnecessary code

[Jun Li]

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月24日 7:23
> To: balbi@kernel.org; mathias.nyman@intel.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun Li
> <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> Subject: [PATCH v2 6/9] usb: host: xhci-plat: delete the unnecessary code
> 
> The if {} condition is duplicated with outer if {} condition.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

Reviewed-by: Jun Li <jun.li@nxp.com>

> ---
>  drivers/usb/host/xhci-plat.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index
> 51e20464fe0a..03d6bbe51919 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -283,8 +283,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
> 
>  		/* Just copy data for now */
> -		if (priv_match)
> -			*priv = *priv_match;
> +		*priv = *priv_match;
>  	}
> 
>  	device_wakeup_enable(hcd->self.controller);
> --
> 2.17.1

Re: [PATCH v2 6/9] usb: host: xhci-plat: delete the unnecessary code

[Mathias Nyman]

On 24.5.2020 2.23, Peter Chen wrote:
> The if {} condition is duplicated with outer if {} condition.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>

Acked-by: Mathias Nyman <mathias.nyman@linux.intel.com>

> ---
>  drivers/usb/host/xhci-plat.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 51e20464fe0a..03d6bbe51919 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -283,8 +283,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
>  
>  		/* Just copy data for now */
> -		if (priv_match)
> -			*priv = *priv_match;
> +		*priv = *priv_match;
>  	}
>  
>  	device_wakeup_enable(hcd->self.controller);
> 

[PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization

[Peter Chen]

Some DRD controllers (eg, dwc3 & cdns3) have PHY management at
their own driver to cover both device and host mode, so add one
priv flag for such users to skip PHY management from HCD core.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/host/xhci-plat.c | 8 ++++++--
 drivers/usb/host/xhci-plat.h | 1 +
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 03d6bbe51919..a3d6cb464186 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -183,6 +183,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct usb_hcd		*hcd;
 	int			ret;
 	int			irq;
+	struct xhci_plat_priv	*priv = NULL;
+
 
 	if (usb_disabled())
 		return -ENODEV;
@@ -280,8 +282,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 		priv_match = dev_get_platdata(&pdev->dev);
 
 	if (priv_match) {
-		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
-
+		priv = hcd_to_xhci_priv(hcd);
 		/* Just copy data for now */
 		*priv = *priv_match;
 	}
@@ -329,6 +330,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 
 	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
 	xhci->shared_hcd->tpl_support = hcd->tpl_support;
+	if (priv && priv->skip_phy_initialization)
+		hcd->skip_phy_initialization = 1;
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto disable_usb_phy;
diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
index 1fb149d1fbce..8825e8eb28d6 100644
--- a/drivers/usb/host/xhci-plat.h
+++ b/drivers/usb/host/xhci-plat.h
@@ -13,6 +13,7 @@
 struct xhci_plat_priv {
 	const char *firmware_name;
 	unsigned long long quirks;
+	unsigned int skip_phy_initialization:1;
 	void (*plat_start)(struct usb_hcd *);
 	int (*init_quirk)(struct usb_hcd *);
 	int (*suspend_quirk)(struct usb_hcd *);
-- 
2.17.1

RE: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization

[Jun Li]

Hi

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月24日 7:23
> To: balbi@kernel.org; mathias.nyman@intel.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun Li
> <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> Subject: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for
> skip_phy_initialization
> 
> Some DRD controllers (eg, dwc3 & cdns3) have PHY management at their own driver
> to cover both device and host mode, so add one priv flag for such users to skip
> PHY management from HCD core.

Can this flag be set directly in __cdns3_host_init()?

Li Jun
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/host/xhci-plat.c | 8 ++++++--  drivers/usb/host/xhci-plat.h | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index
> 03d6bbe51919..a3d6cb464186 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -183,6 +183,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct usb_hcd		*hcd;
>  	int			ret;
>  	int			irq;
> +	struct xhci_plat_priv	*priv = NULL;
> +
> 
>  	if (usb_disabled())
>  		return -ENODEV;
> @@ -280,8 +282,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  		priv_match = dev_get_platdata(&pdev->dev);
> 
>  	if (priv_match) {
> -		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
> -
> +		priv = hcd_to_xhci_priv(hcd);
>  		/* Just copy data for now */
>  		*priv = *priv_match;
>  	}
> @@ -329,6 +330,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> 
>  	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
>  	xhci->shared_hcd->tpl_support = hcd->tpl_support;
> +	if (priv && priv->skip_phy_initialization)
> +		hcd->skip_phy_initialization = 1;
> +
>  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>  	if (ret)
>  		goto disable_usb_phy;
> diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h index
> 1fb149d1fbce..8825e8eb28d6 100644
> --- a/drivers/usb/host/xhci-plat.h
> +++ b/drivers/usb/host/xhci-plat.h
> @@ -13,6 +13,7 @@
>  struct xhci_plat_priv {
>  	const char *firmware_name;
>  	unsigned long long quirks;
> +	unsigned int skip_phy_initialization:1;
>  	void (*plat_start)(struct usb_hcd *);
>  	int (*init_quirk)(struct usb_hcd *);
>  	int (*suspend_quirk)(struct usb_hcd *);
> --
> 2.17.1

Re: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization

[Peter Chen]

On 20-05-24 07:40:34, Jun Li wrote:
> Hi
> 
> > -----Original Message-----
> > From: Peter Chen <peter.chen@nxp.com>
> > Sent: 2020年5月24日 7:23
> > To: balbi@kernel.org; mathias.nyman@intel.com
> > Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun Li
> > <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> > Subject: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for
> > skip_phy_initialization
> > 
> > Some DRD controllers (eg, dwc3 & cdns3) have PHY management at their own driver
> > to cover both device and host mode, so add one priv flag for such users to skip
> > PHY management from HCD core.
> 
> Can this flag be set directly in __cdns3_host_init()?

No, since both HCD creation and usb_add_hcd invoking are at
xhci_plat_probe, glue layer has no chance to change hcd flags.

Peter
> 
> Li Jun
> > 
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 8 ++++++--  drivers/usb/host/xhci-plat.h | 1 +
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index
> > 03d6bbe51919..a3d6cb464186 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -183,6 +183,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct usb_hcd		*hcd;
> >  	int			ret;
> >  	int			irq;
> > +	struct xhci_plat_priv	*priv = NULL;
> > +
> > 
> >  	if (usb_disabled())
> >  		return -ENODEV;
> > @@ -280,8 +282,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  		priv_match = dev_get_platdata(&pdev->dev);
> > 
> >  	if (priv_match) {
> > -		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
> > -
> > +		priv = hcd_to_xhci_priv(hcd);
> >  		/* Just copy data for now */
> >  		*priv = *priv_match;
> >  	}
> > @@ -329,6 +330,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > 
> >  	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
> >  	xhci->shared_hcd->tpl_support = hcd->tpl_support;
> > +	if (priv && priv->skip_phy_initialization)
> > +		hcd->skip_phy_initialization = 1;
> > +
> >  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> >  	if (ret)
> >  		goto disable_usb_phy;
> > diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h index
> > 1fb149d1fbce..8825e8eb28d6 100644
> > --- a/drivers/usb/host/xhci-plat.h
> > +++ b/drivers/usb/host/xhci-plat.h
> > @@ -13,6 +13,7 @@
> >  struct xhci_plat_priv {
> >  	const char *firmware_name;
> >  	unsigned long long quirks;
> > +	unsigned int skip_phy_initialization:1;
> >  	void (*plat_start)(struct usb_hcd *);
> >  	int (*init_quirk)(struct usb_hcd *);
> >  	int (*suspend_quirk)(struct usb_hcd *);
> > --
> > 2.17.1
> 

-- 

Thanks,
Peter Chen

RE: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization

[Jun Li]

> -----Original Message-----
> From: Peter Chen <peter.chen@nxp.com>
> Sent: 2020年5月25日 11:04
> To: Jun Li <jun.li@nxp.com>
> Cc: balbi@kernel.org; mathias.nyman@intel.com; linux-usb@vger.kernel.org;
> dl-linux-imx <linux-imx@nxp.com>; pawell@cadence.com; rogerq@ti.com;
> gregkh@linuxfoundation.org
> Subject: Re: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for
> skip_phy_initialization
> 
> On 20-05-24 07:40:34, Jun Li wrote:
> > Hi
> >
> > > -----Original Message-----
> > > From: Peter Chen <peter.chen@nxp.com>
> > > Sent: 2020年5月24日 7:23
> > > To: balbi@kernel.org; mathias.nyman@intel.com
> > > Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun
> > > Li <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> > > Subject: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for
> > > skip_phy_initialization
> > >
> > > Some DRD controllers (eg, dwc3 & cdns3) have PHY management at their
> > > own driver to cover both device and host mode, so add one priv flag
> > > for such users to skip PHY management from HCD core.
> >
> > Can this flag be set directly in __cdns3_host_init()?
> 
> No, since both HCD creation and usb_add_hcd invoking are at xhci_plat_probe, glue
> layer has no chance to change hcd flags.

I see after patch[2/9],

@@ -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;
+

You already can access hcd directly.

Li Jun
> 
> Peter
> >
> > Li Jun
> > >
> > > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > > ---
> > >  drivers/usb/host/xhci-plat.c | 8 ++++++--
> > > drivers/usb/host/xhci-plat.h | 1 +
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-plat.c
> > > b/drivers/usb/host/xhci-plat.c index
> > > 03d6bbe51919..a3d6cb464186 100644
> > > --- a/drivers/usb/host/xhci-plat.c
> > > +++ b/drivers/usb/host/xhci-plat.c
> > > @@ -183,6 +183,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > >  	struct usb_hcd		*hcd;
> > >  	int			ret;
> > >  	int			irq;
> > > +	struct xhci_plat_priv	*priv = NULL;
> > > +
> > >
> > >  	if (usb_disabled())
> > >  		return -ENODEV;
> > > @@ -280,8 +282,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > >  		priv_match = dev_get_platdata(&pdev->dev);
> > >
> > >  	if (priv_match) {
> > > -		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);
> > > -
> > > +		priv = hcd_to_xhci_priv(hcd);
> > >  		/* Just copy data for now */
> > >  		*priv = *priv_match;
> > >  	}
> > > @@ -329,6 +330,9 @@ static int xhci_plat_probe(struct
> > > platform_device *pdev)
> > >
> > >  	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
> > >  	xhci->shared_hcd->tpl_support = hcd->tpl_support;
> > > +	if (priv && priv->skip_phy_initialization)
> > > +		hcd->skip_phy_initialization = 1;
> > > +
> > >  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> > >  	if (ret)
> > >  		goto disable_usb_phy;
> > > diff --git a/drivers/usb/host/xhci-plat.h
> > > b/drivers/usb/host/xhci-plat.h index
> > > 1fb149d1fbce..8825e8eb28d6 100644
> > > --- a/drivers/usb/host/xhci-plat.h
> > > +++ b/drivers/usb/host/xhci-plat.h
> > > @@ -13,6 +13,7 @@
> > >  struct xhci_plat_priv {
> > >  	const char *firmware_name;
> > >  	unsigned long long quirks;
> > > +	unsigned int skip_phy_initialization:1;
> > >  	void (*plat_start)(struct usb_hcd *);
> > >  	int (*init_quirk)(struct usb_hcd *);
> > >  	int (*suspend_quirk)(struct usb_hcd *);
> > > --
> > > 2.17.1
> >
> 
> --
> 
> Thanks,
> Peter Chen

Re: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization

[Peter Chen]

On 20-05-25 12:30:49, Jun Li wrote:
> 
> 
> > -----Original Message-----
> > From: Peter Chen <peter.chen@nxp.com>
> > Sent: 2020年5月25日 11:04
> > To: Jun Li <jun.li@nxp.com>
> > Cc: balbi@kernel.org; mathias.nyman@intel.com; linux-usb@vger.kernel.org;
> > dl-linux-imx <linux-imx@nxp.com>; pawell@cadence.com; rogerq@ti.com;
> > gregkh@linuxfoundation.org
> > Subject: Re: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for
> > skip_phy_initialization
> > 
> > On 20-05-24 07:40:34, Jun Li wrote:
> > > Hi
> > >
> > > > -----Original Message-----
> > > > From: Peter Chen <peter.chen@nxp.com>
> > > > Sent: 2020年5月24日 7:23
> > > > To: balbi@kernel.org; mathias.nyman@intel.com
> > > > Cc: linux-usb@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>;
> > > > pawell@cadence.com; rogerq@ti.com; gregkh@linuxfoundation.org; Jun
> > > > Li <jun.li@nxp.com>; Peter Chen <peter.chen@nxp.com>
> > > > Subject: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for
> > > > skip_phy_initialization
> > > >
> > > > Some DRD controllers (eg, dwc3 & cdns3) have PHY management at their
> > > > own driver to cover both device and host mode, so add one priv flag
> > > > for such users to skip PHY management from HCD core.
> > >
> > > Can this flag be set directly in __cdns3_host_init()?
> > 
> > No, since both HCD creation and usb_add_hcd invoking are at xhci_plat_probe, glue
> > layer has no chance to change hcd flags.
> 
> I see after patch[2/9],
> 
> @@ -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;
> +
> 
> You already can access hcd directly.
> 

At that time, usb_add_hcd was called, and the power was powered on if
the skip flat was not set.

-- 

Thanks,
Peter Chen

Re: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization

[Mathias Nyman]

On 24.5.2020 2.23, Peter Chen wrote:
> Some DRD controllers (eg, dwc3 & cdns3) have PHY management at
> their own driver to cover both device and host mode, so add one
> priv flag for such users to skip PHY management from HCD core.
> 
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> ---
>  drivers/usb/host/xhci-plat.c | 8 ++++++--
>  drivers/usb/host/xhci-plat.h | 1 +
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 03d6bbe51919..a3d6cb464186 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -183,6 +183,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct usb_hcd		*hcd;
>  	int			ret;
>  	int			irq;
> +	struct xhci_plat_priv	*priv = NULL;
> +
>  
>  	if (usb_disabled())
>  		return -ENODEV;
> @@ -280,8 +282,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  		priv_match = dev_get_platdata(&pdev->dev);
>  
>  	if (priv_match) {
> -		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);h
> -
> +		priv = hcd_to_xhci_priv(hcd);
>  		/* Just copy data for now */
>  		*priv = *priv_match;
>  	}
> @@ -329,6 +330,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  
>  	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
>  	xhci->shared_hcd->tpl_support = hcd->tpl_support;
> +	if (priv && priv->skip_phy_initialization)
> +		hcd->skip_phy_initialization = 1;
> +
>  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>  	if (ret)
>  		goto disable_usb_phy;
> diff --git a/drivers/usb/host/xhci-plat.h b/drivers/usb/host/xhci-plat.h
> index 1fb149d1fbce..8825e8eb28d6 100644
> --- a/drivers/usb/host/xhci-plat.h
> +++ b/drivers/usb/host/xhci-plat.h
> @@ -13,6 +13,7 @@
>  struct xhci_plat_priv {
>  	const char *firmware_name;
>  	unsigned long long quirks;
> +	unsigned int skip_phy_initialization:1;

Any specific reason why this approach was chosen instead of adding
a new flag to the "long long quirks" above?

-Mathias 

RE: [PATCH v2 7/9] usb: host: xhci-plat: add priv flag for skip_phy_initialization

[Peter Chen]

 
> 
> On 24.5.2020 2.23, Peter Chen wrote:
> > Some DRD controllers (eg, dwc3 & cdns3) have PHY management at their
> > own driver to cover both device and host mode, so add one priv flag
> > for such users to skip PHY management from HCD core.
> >
> > Signed-off-by: Peter Chen <peter.chen@nxp.com>
> > ---
> >  drivers/usb/host/xhci-plat.c | 8 ++++++--
> > drivers/usb/host/xhci-plat.h | 1 +
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c
> > b/drivers/usb/host/xhci-plat.c index 03d6bbe51919..a3d6cb464186 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -183,6 +183,8 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct usb_hcd		*hcd;
> >  	int			ret;
> >  	int			irq;
> > +	struct xhci_plat_priv	*priv = NULL;
> > +
> >
> >  	if (usb_disabled())
> >  		return -ENODEV;
> > @@ -280,8 +282,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  		priv_match = dev_get_platdata(&pdev->dev);
> >
> >  	if (priv_match) {
> > -		struct xhci_plat_priv *priv = hcd_to_xhci_priv(hcd);h
> > -
> > +		priv = hcd_to_xhci_priv(hcd);
> >  		/* Just copy data for now */
> >  		*priv = *priv_match;
> >  	}
> > @@ -329,6 +330,9 @@ static int xhci_plat_probe(struct platform_device
> > *pdev)
> >
> >  	hcd->tpl_support = of_usb_host_tpl_support(sysdev->of_node);
> >  	xhci->shared_hcd->tpl_support = hcd->tpl_support;
> > +	if (priv && priv->skip_phy_initialization)
> > +		hcd->skip_phy_initialization = 1;
> > +
> >  	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> >  	if (ret)
> >  		goto disable_usb_phy;
> > diff --git a/drivers/usb/host/xhci-plat.h
> > b/drivers/usb/host/xhci-plat.h index 1fb149d1fbce..8825e8eb28d6 100644
> > --- a/drivers/usb/host/xhci-plat.h
> > +++ b/drivers/usb/host/xhci-plat.h
> > @@ -13,6 +13,7 @@
> >  struct xhci_plat_priv {
> >  	const char *firmware_name;
> >  	unsigned long long quirks;
> > +	unsigned int skip_phy_initialization:1;
> 
> Any specific reason why this approach was chosen instead of adding a new flag to
> the "long long quirks" above?

I thought the quirks are for hardware, but it seems it could be used for software too.
I will change it by using quirks, thanks.

Peter 

[PATCH v2 8/9] usb: cdns3: host: add .suspend_quirk for xhci-plat.c

[Peter Chen]

cdns3 has some special PM sequence between xhci_bus_suspend and
xhci_suspend, add quirk to implement it.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/host-export.h |  6 +++++
 drivers/usb/cdns3/host.c        | 40 +++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/usb/cdns3/host-export.h b/drivers/usb/cdns3/host-export.h
index ae11810f8826..26041718a086 100644
--- a/drivers/usb/cdns3/host-export.h
+++ b/drivers/usb/cdns3/host-export.h
@@ -9,9 +9,11 @@
 #ifndef __LINUX_CDNS3_HOST_EXPORT
 #define __LINUX_CDNS3_HOST_EXPORT
 
+struct usb_hcd;
 #ifdef CONFIG_USB_CDNS3_HOST
 
 int cdns3_host_init(struct cdns3 *cdns);
+int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd);
 
 #else
 
@@ -21,6 +23,10 @@ static inline int cdns3_host_init(struct cdns3 *cdns)
 }
 
 static inline void cdns3_host_exit(struct cdns3 *cdns) { }
+static inline int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd)
+{
+	return 0;
+}
 
 #endif /* CONFIG_USB_CDNS3_HOST */
 
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index b579ef15f4e0..030d6421abd3 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -14,6 +14,18 @@
 #include "drd.h"
 #include "host-export.h"
 #include <linux/usb/hcd.h>
+#include "../host/xhci.h"
+#include "../host/xhci-plat.h"
+
+#define XECP_PORT_CAP_REG	0x8000
+#define XECP_AUX_CTRL_REG1	0x8120
+
+#define CFG_RXDET_P3_EN		(1 << 15)
+#define LPM_2_STB_SWITCH_EN	(1 << 25)
+
+static const struct xhci_plat_priv xhci_plat_cdns3_xhci = {
+	.suspend_quirk = xhci_cdns3_suspend_quirk,
+};
 
 static int __cdns3_host_init(struct cdns3 *cdns)
 {
@@ -39,6 +51,11 @@ static int __cdns3_host_init(struct cdns3 *cdns)
 		goto err1;
 	}
 
+	ret = platform_device_add_data(xhci, &xhci_plat_cdns3_xhci,
+			sizeof(struct xhci_plat_priv));
+	if (ret)
+		goto err1;
+
 	ret = platform_device_add(xhci);
 	if (ret) {
 		dev_err(cdns->dev, "failed to register xHCI device\n");
@@ -56,6 +73,29 @@ static int __cdns3_host_init(struct cdns3 *cdns)
 	return ret;
 }
 
+int xhci_cdns3_suspend_quirk(struct usb_hcd *hcd)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	u32 value;
+
+	/* set usbcmd.EU3S */
+	value = readl(&xhci->op_regs->command);
+	value |= CMD_PM_INDEX;
+	writel(value, &xhci->op_regs->command);
+
+	if (hcd->regs) {
+		value = readl(hcd->regs + XECP_AUX_CTRL_REG1);
+		value |= CFG_RXDET_P3_EN;
+		writel(value, hcd->regs + XECP_AUX_CTRL_REG1);
+
+		value = readl(hcd->regs + XECP_PORT_CAP_REG);
+		value |= LPM_2_STB_SWITCH_EN;
+		writel(value, hcd->regs + XECP_PORT_CAP_REG);
+	}
+
+	return 0;
+}
+
 static void cdns3_host_exit(struct cdns3 *cdns)
 {
 	platform_device_unregister(cdns->host_dev);
-- 
2.17.1

[PATCH v2 9/9] usb: cdns3: host: add xhci_plat_priv's flag skip_phy_initialization

[Peter Chen]

cdns3 manages PHY by own DRD driver, so skip the management by
HCD core.

Signed-off-by: Peter Chen <peter.chen@nxp.com>
---
 drivers/usb/cdns3/host.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 030d6421abd3..d681c3db25cd 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -24,6 +24,7 @@
 #define LPM_2_STB_SWITCH_EN	(1 << 25)
 
 static const struct xhci_plat_priv xhci_plat_cdns3_xhci = {
+	.skip_phy_initialization = 1,
 	.suspend_quirk = xhci_cdns3_suspend_quirk,
 };
 
-- 
2.17.1

Re: [PATCH v2 0/9] usb: some PM changes for cdns3 and xhci-plat

[Peter Chen]

On 20-05-24 07:22:55, Peter Chen wrote:

Hi Mathias,

Would I get any comments for xhci-plat.c changes?

Peter

> Hi Felipe & Mathias,
> 
> In this series, it adds cdns3 runtime PM support as well as some
> xhci-plat changes, and verified at NXP i.MX8QM and i.MX8QXP platforms.
> 
> Patch 1-3: add main runtime PM support for cdns3 core and imx glue layer
> Patch 4-9: add platform data support and two quirks for xhci-plat.c.
> - .suspend_quirk for platform special sequence between xhci_bus_suspend
> and xhci_suspend.
> - skip_phy_initialization for skip PHY management from HCD core.
> 
> Changes for v1:
> - 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.
> 
> Peter Chen (9):
>   usb: cdns3: introduce cdns3_set_phy_power API
>   usb: cdns3: add runtime PM support
>   usb: cdns3: imx: add glue layer runtime pm implementation
>   usb: host: xhci-plat: add platform data support
>   usb: host: xhci-plat: add .suspend_quirk for struct xhci_plat_priv
>   usb: host: xhci-plat: delete the unnecessary code
>   usb: host: xhci-plat: add priv flag for skip_phy_initialization
>   usb: cdns3: host: add .suspend_quirk for xhci-plat.c
>   usb: cdns3: host: add xhci_plat_priv's flag skip_phy_initialization
> 
>  drivers/usb/cdns3/cdns3-imx.c   | 179 +++++++++++++++++++++++++++++++-
>  drivers/usb/cdns3/core.c        | 163 +++++++++++++++++++++++------
>  drivers/usb/cdns3/core.h        |  14 +++
>  drivers/usb/cdns3/drd.c         |   7 ++
>  drivers/usb/cdns3/gadget.c      |   4 +
>  drivers/usb/cdns3/host-export.h |   6 ++
>  drivers/usb/cdns3/host.c        |  48 +++++++++
>  drivers/usb/host/xhci-plat.c    |  36 ++++++-
>  drivers/usb/host/xhci-plat.h    |   2 +
>  9 files changed, 420 insertions(+), 39 deletions(-)
> 
> -- 
> 2.17.1
> 

-- 

Thanks,
Peter Chen