[PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Chunfeng Yun]

When the dedicated wake irq is level trigger, enable it before
calling runtime_suspend, will trigger an interrupt.

e.g.
for a low level trigger type, it's low level at running time (0),
and becomes high level when enters suspend (runtime_suspend (1) is
called), a wakeup signal at (2) make it become low level, wake irq
will be triggered.

                ------------------
               |           ^     ^|
----------------           |     | --------------
 |<---(0)--->|<--(1)--|   (3)   (2)    (4)

if we enable the wake irq before calling runtime_suspend during (0),
an interrupt will arise, it causes resume immediately;
enable wake irq after calling runtime_suspend, e.g. at (3) or (4),
will works.

This patch seems no side effect on edge trigger wake irq.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/base/power/runtime.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index a46a7e30881b..796739a015a5 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -619,12 +619,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
 	callback = RPM_GET_CALLBACK(dev, runtime_suspend);
-
-	dev_pm_enable_wake_irq_check(dev, true);
 	retval = rpm_callback(callback, dev);
 	if (retval)
 		goto fail;
 
+	dev_pm_enable_wake_irq_check(dev, true);
+
  no_callback:
 	__update_runtime_status(dev, RPM_SUSPENDED);
 	pm_runtime_deactivate_timer(dev);
@@ -659,7 +659,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	return retval;
 
  fail:
-	dev_pm_disable_wake_irq_check(dev);
 	__update_runtime_status(dev, RPM_ACTIVE);
 	dev->power.deferred_resume = false;
 	wake_up_all(&dev->power.wait_queue);
-- 
2.18.0

[PATCH 2/6] usb: xhci-mtk: check return value in suspend/resume hooks

[Chunfeng Yun]

Return error number if encounter errors during suspend and resume.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index c1bc40289833..a74764ab914a 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -630,18 +630,12 @@ static int xhci_mtk_remove(struct platform_device *dev)
 	return 0;
 }
 
-/*
- * if ip sleep fails, and all clocks are disabled, access register will hang
- * AHB bus, so stop polling roothubs to avoid regs access on bus suspend.
- * and no need to check whether ip sleep failed or not; this will cause SPM
- * to wake up system immediately after system suspend complete if ip sleep
- * fails, it is what we wanted.
- */
 static int __maybe_unused xhci_mtk_suspend(struct device *dev)
 {
 	struct xhci_hcd_mtk *mtk = dev_get_drvdata(dev);
 	struct usb_hcd *hcd = mtk->hcd;
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	int ret;
 
 	xhci_dbg(xhci, "%s: stop port polling\n", __func__);
 	clear_bit(HCD_FLAG_POLL_RH, &hcd->flags);
@@ -649,10 +643,21 @@ static int __maybe_unused xhci_mtk_suspend(struct device *dev)
 	clear_bit(HCD_FLAG_POLL_RH, &xhci->shared_hcd->flags);
 	del_timer_sync(&xhci->shared_hcd->rh_timer);
 
-	xhci_mtk_host_disable(mtk);
+	ret = xhci_mtk_host_disable(mtk);
+	if (ret)
+		goto restart_poll_rh;
+
 	xhci_mtk_clks_disable(mtk);
 	usb_wakeup_set(mtk, true);
 	return 0;
+
+restart_poll_rh:
+	xhci_dbg(xhci, "%s: restart port polling\n", __func__);
+	set_bit(HCD_FLAG_POLL_RH, &xhci->shared_hcd->flags);
+	usb_hcd_poll_rh_status(xhci->shared_hcd);
+	set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
+	usb_hcd_poll_rh_status(hcd);
+	return ret;
 }
 
 static int __maybe_unused xhci_mtk_resume(struct device *dev)
@@ -660,10 +665,16 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
 	struct xhci_hcd_mtk *mtk = dev_get_drvdata(dev);
 	struct usb_hcd *hcd = mtk->hcd;
 	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+	int ret;
 
 	usb_wakeup_set(mtk, false);
-	xhci_mtk_clks_enable(mtk);
-	xhci_mtk_host_enable(mtk);
+	ret = xhci_mtk_clks_enable(mtk);
+	if (ret)
+		goto enable_wakeup;
+
+	ret = xhci_mtk_host_enable(mtk);
+	if (ret)
+		goto disable_clks;
 
 	xhci_dbg(xhci, "%s: restart port polling\n", __func__);
 	set_bit(HCD_FLAG_POLL_RH, &xhci->shared_hcd->flags);
@@ -671,6 +682,12 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
 	set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
 	usb_hcd_poll_rh_status(hcd);
 	return 0;
+
+disable_clks:
+	xhci_mtk_clks_disable(mtk);
+enable_wakeup:
+	usb_wakeup_set(mtk, true);
+	return ret;
 }
 
 static const struct dev_pm_ops xhci_mtk_pm_ops = {
-- 
2.18.0

[PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt

[Chunfeng Yun]

Add an interrupt which is EINT usually to support runtime PM,
meanwhile add "interrupt-names" property, for backward
compatibility, it's optional and used when wakeup interrupt
exists

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml  | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
index 45bf4ea00c9e..4fe8a301d03f 100644
--- a/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
+++ b/Documentation/devicetree/bindings/usb/mediatek,mtk-xhci.yaml
@@ -46,7 +46,18 @@ properties:
       - const: ippc  # optional, only needed for case 1.
 
   interrupts:
-    maxItems: 1
+    description:
+      use "interrupts-extended" when the interrupts are connected to the
+      separate interrupt controllers
+    minItems: 1
+    items:
+      - description: xHCI host controller interrupt
+      - description: optional, wakeup interrupt used to support runtime PM
+
+  interrupt-names:
+    items:
+      - const: host
+      - const: wakeup
 
   power-domains:
     description: A phandle to USB power domain node to control USB's MTCMOS
-- 
2.18.0

[PATCH 4/6] usb: xhci-mtk: add support runtime PM

[Chunfeng Yun]

A dedicated wakeup irq will be used to handle runtime suspend/resume,
we use dev_pm_set_dedicated_wake_irq API to take care of requesting
and attaching wakeup irq, then the suspend/resume framework will help
to enable/disable wakeup irq.

The runtime PM is default off since some platforms may not support it.
users can enable it via power/control (set "auto") in sysfs.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk.c | 140 +++++++++++++++++++++++++++++++-----
 1 file changed, 124 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index a74764ab914a..30927f4064d4 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
@@ -358,7 +359,6 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
 			mtk->uwk_reg_base, mtk->uwk_vers);
 
 	return PTR_ERR_OR_ZERO(mtk->uwk);
-
 }
 
 static void usb_wakeup_set(struct xhci_hcd_mtk *mtk, bool enable)
@@ -458,6 +458,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct usb_hcd *hcd;
 	int ret = -ENODEV;
+	int wakeup_irq;
 	int irq;
 
 	if (usb_disabled())
@@ -485,6 +486,21 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	irq = platform_get_irq_byname_optional(pdev, "host");
+	if (irq < 0) {
+		if (irq == -EPROBE_DEFER)
+			return irq;
+
+		/* for backward compatibility */
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0)
+			return irq;
+	}
+
+	wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
+	if (wakeup_irq == -EPROBE_DEFER)
+		return wakeup_irq;
+
 	mtk->lpm_support = of_property_read_bool(node, "usb3-lpm-capable");
 	/* optional property, ignore the error if it does not exist */
 	of_property_read_u32(node, "mediatek,u3p-dis-msk",
@@ -496,9 +512,11 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	pm_runtime_set_active(dev);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_autosuspend_delay(dev, 4000);
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
-	device_enable_async_suspend(dev);
 
 	ret = xhci_mtk_ldos_enable(mtk);
 	if (ret)
@@ -508,12 +526,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_ldos;
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		ret = irq;
-		goto disable_clk;
-	}
-
 	hcd = usb_create_hcd(driver, dev, dev_name(dev));
 	if (!hcd) {
 		ret = -ENOMEM;
@@ -579,8 +591,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	if (ret)
 		goto dealloc_usb2_hcd;
 
+	if (wakeup_irq > 0) {
+		ret = dev_pm_set_dedicated_wake_irq(dev, wakeup_irq);
+		if (ret) {
+			dev_err(dev, "set wakeup irq %d failed\n", wakeup_irq);
+			goto dealloc_usb3_hcd;
+		}
+		dev_info(dev, "wakeup irq %d\n", wakeup_irq);
+	}
+
+	device_enable_async_suspend(dev);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+	pm_runtime_forbid(dev);
+
 	return 0;
 
+dealloc_usb3_hcd:
+	usb_remove_hcd(xhci->shared_hcd);
+	xhci->shared_hcd = NULL;
+
 dealloc_usb2_hcd:
 	usb_remove_hcd(hcd);
 
@@ -601,25 +631,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	xhci_mtk_ldos_disable(mtk);
 
 disable_pm:
-	pm_runtime_put_sync(dev);
+	pm_runtime_put_sync_autosuspend(dev);
 	pm_runtime_disable(dev);
 	return ret;
 }
 
-static int xhci_mtk_remove(struct platform_device *dev)
+static int xhci_mtk_remove(struct platform_device *pdev)
 {
-	struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
+	struct xhci_hcd_mtk *mtk = platform_get_drvdata(pdev);
 	struct usb_hcd	*hcd = mtk->hcd;
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	struct usb_hcd  *shared_hcd = xhci->shared_hcd;
+	struct device *dev = &pdev->dev;
 
-	pm_runtime_put_noidle(&dev->dev);
-	pm_runtime_disable(&dev->dev);
+	pm_runtime_get_sync(dev);
+	xhci->xhc_state |= XHCI_STATE_REMOVING;
+	dev_pm_clear_wake_irq(dev);
+	device_init_wakeup(dev, false);
 
 	usb_remove_hcd(shared_hcd);
 	xhci->shared_hcd = NULL;
-	device_init_wakeup(&dev->dev, false);
-
 	usb_remove_hcd(hcd);
 	usb_put_hcd(shared_hcd);
 	usb_put_hcd(hcd);
@@ -627,6 +658,10 @@ static int xhci_mtk_remove(struct platform_device *dev)
 	xhci_mtk_clks_disable(mtk);
 	xhci_mtk_ldos_disable(mtk);
 
+	pm_runtime_disable(dev);
+	pm_runtime_put_noidle(dev);
+	pm_runtime_set_suspended(dev);
+
 	return 0;
 }
 
@@ -690,10 +725,83 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
 	return ret;
 }
 
+static int check_rhub_status(struct xhci_hcd *xhci, struct xhci_hub *rhub)
+{
+	u32 suspended_ports;
+	u32 status;
+	int num_ports;
+	int i;
+
+	num_ports = rhub->num_ports;
+	suspended_ports = rhub->bus_state.suspended_ports;
+	for (i = 0; i < num_ports; i++) {
+		if (!(suspended_ports & BIT(i))) {
+			status = readl(rhub->ports[i]->addr);
+			if (status & PORT_CONNECT)
+				return -EBUSY;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * check the bus whether it could suspend or not
+ * the bus will suspend if the downstream ports are already suspended,
+ * or no devices connected.
+ */
+static int check_bus_status(struct xhci_hcd *xhci)
+{
+	int ret;
+
+	ret = check_rhub_status(xhci, &xhci->usb3_rhub);
+	if (ret)
+		return ret;
+
+	return check_rhub_status(xhci, &xhci->usb2_rhub);
+}
+
+static int __maybe_unused xhci_mtk_runtime_suspend(struct device *dev)
+{
+	struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
+	struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
+	int ret = 0;
+
+	if (xhci->xhc_state)
+		return -ESHUTDOWN;
+
+	if (device_may_wakeup(dev)) {
+		ret = check_bus_status(xhci);
+		if (!ret)
+			ret = xhci_mtk_suspend(dev);
+	}
+
+	/* -EBUSY: let PM automatically reschedule another autosuspend */
+	return ret ? -EBUSY : 0;
+}
+
+static int __maybe_unused xhci_mtk_runtime_resume(struct device *dev)
+{
+	struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
+	struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
+	int ret = 0;
+
+	if (xhci->xhc_state)
+		return -ESHUTDOWN;
+
+	if (device_may_wakeup(dev))
+		ret = xhci_mtk_resume(dev);
+
+	return ret;
+}
+
 static const struct dev_pm_ops xhci_mtk_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(xhci_mtk_suspend, xhci_mtk_resume)
+	SET_RUNTIME_PM_OPS(xhci_mtk_runtime_suspend,
+			   xhci_mtk_runtime_resume, NULL)
 };
-#define DEV_PM_OPS IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL
+
+#define DEV_PM_OPS (IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL)
 
 static const struct of_device_id mtk_xhci_of_match[] = {
 	{ .compatible = "mediatek,mt8173-xhci"},
-- 
2.18.0

[PATCH 5/6] usb: xhci-mtk: use clock bulk to get clocks

[Chunfeng Yun]

Use clock bulk helpers to get/enable/disable clocks, meanwhile
make sys_ck optional, then will be easier to handle clocks.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk.c | 109 +++++++-----------------------------
 drivers/usb/host/xhci-mtk.h |  10 ++--
 2 files changed, 24 insertions(+), 95 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 30927f4064d4..d4c455eecb8d 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -7,7 +7,6 @@
  *  Chunfeng Yun <chunfeng.yun@mediatek.com>
  */
 
-#include <linux/clk.h>
 #include <linux/dma-mapping.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -220,89 +219,6 @@ static int xhci_mtk_ssusb_config(struct xhci_hcd_mtk *mtk)
 	return xhci_mtk_host_enable(mtk);
 }
 
-static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
-{
-	struct device *dev = mtk->dev;
-
-	mtk->sys_clk = devm_clk_get(dev, "sys_ck");
-	if (IS_ERR(mtk->sys_clk)) {
-		dev_err(dev, "fail to get sys_ck\n");
-		return PTR_ERR(mtk->sys_clk);
-	}
-
-	mtk->xhci_clk = devm_clk_get_optional(dev, "xhci_ck");
-	if (IS_ERR(mtk->xhci_clk))
-		return PTR_ERR(mtk->xhci_clk);
-
-	mtk->ref_clk = devm_clk_get_optional(dev, "ref_ck");
-	if (IS_ERR(mtk->ref_clk))
-		return PTR_ERR(mtk->ref_clk);
-
-	mtk->mcu_clk = devm_clk_get_optional(dev, "mcu_ck");
-	if (IS_ERR(mtk->mcu_clk))
-		return PTR_ERR(mtk->mcu_clk);
-
-	mtk->dma_clk = devm_clk_get_optional(dev, "dma_ck");
-	return PTR_ERR_OR_ZERO(mtk->dma_clk);
-}
-
-static int xhci_mtk_clks_enable(struct xhci_hcd_mtk *mtk)
-{
-	int ret;
-
-	ret = clk_prepare_enable(mtk->ref_clk);
-	if (ret) {
-		dev_err(mtk->dev, "failed to enable ref_clk\n");
-		goto ref_clk_err;
-	}
-
-	ret = clk_prepare_enable(mtk->sys_clk);
-	if (ret) {
-		dev_err(mtk->dev, "failed to enable sys_clk\n");
-		goto sys_clk_err;
-	}
-
-	ret = clk_prepare_enable(mtk->xhci_clk);
-	if (ret) {
-		dev_err(mtk->dev, "failed to enable xhci_clk\n");
-		goto xhci_clk_err;
-	}
-
-	ret = clk_prepare_enable(mtk->mcu_clk);
-	if (ret) {
-		dev_err(mtk->dev, "failed to enable mcu_clk\n");
-		goto mcu_clk_err;
-	}
-
-	ret = clk_prepare_enable(mtk->dma_clk);
-	if (ret) {
-		dev_err(mtk->dev, "failed to enable dma_clk\n");
-		goto dma_clk_err;
-	}
-
-	return 0;
-
-dma_clk_err:
-	clk_disable_unprepare(mtk->mcu_clk);
-mcu_clk_err:
-	clk_disable_unprepare(mtk->xhci_clk);
-xhci_clk_err:
-	clk_disable_unprepare(mtk->sys_clk);
-sys_clk_err:
-	clk_disable_unprepare(mtk->ref_clk);
-ref_clk_err:
-	return ret;
-}
-
-static void xhci_mtk_clks_disable(struct xhci_hcd_mtk *mtk)
-{
-	clk_disable_unprepare(mtk->dma_clk);
-	clk_disable_unprepare(mtk->mcu_clk);
-	clk_disable_unprepare(mtk->xhci_clk);
-	clk_disable_unprepare(mtk->sys_clk);
-	clk_disable_unprepare(mtk->ref_clk);
-}
-
 /* only clocks can be turn off for ip-sleep wakeup mode */
 static void usb_wakeup_ip_sleep_set(struct xhci_hcd_mtk *mtk, bool enable)
 {
@@ -367,6 +283,19 @@ static void usb_wakeup_set(struct xhci_hcd_mtk *mtk, bool enable)
 		usb_wakeup_ip_sleep_set(mtk, enable);
 }
 
+static int xhci_mtk_clks_get(struct xhci_hcd_mtk *mtk)
+{
+	struct clk_bulk_data *clks = mtk->clks;
+
+	clks[0].id = "sys_ck";
+	clks[1].id = "xhci_ck";
+	clks[2].id = "ref_ck";
+	clks[3].id = "mcu_ck";
+	clks[4].id = "dma_ck";
+
+	return devm_clk_bulk_get_optional(mtk->dev, BULK_CLKS_NUM, clks);
+}
+
 static int xhci_mtk_ldos_enable(struct xhci_hcd_mtk *mtk)
 {
 	int ret;
@@ -522,7 +451,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	if (ret)
 		goto disable_pm;
 
-	ret = xhci_mtk_clks_enable(mtk);
+	ret = clk_bulk_prepare_enable(BULK_CLKS_NUM, mtk->clks);
 	if (ret)
 		goto disable_ldos;
 
@@ -625,7 +554,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	usb_put_hcd(hcd);
 
 disable_clk:
-	xhci_mtk_clks_disable(mtk);
+	clk_bulk_disable_unprepare(BULK_CLKS_NUM, mtk->clks);
 
 disable_ldos:
 	xhci_mtk_ldos_disable(mtk);
@@ -655,7 +584,7 @@ static int xhci_mtk_remove(struct platform_device *pdev)
 	usb_put_hcd(shared_hcd);
 	usb_put_hcd(hcd);
 	xhci_mtk_sch_exit(mtk);
-	xhci_mtk_clks_disable(mtk);
+	clk_bulk_disable_unprepare(BULK_CLKS_NUM, mtk->clks);
 	xhci_mtk_ldos_disable(mtk);
 
 	pm_runtime_disable(dev);
@@ -682,7 +611,7 @@ static int __maybe_unused xhci_mtk_suspend(struct device *dev)
 	if (ret)
 		goto restart_poll_rh;
 
-	xhci_mtk_clks_disable(mtk);
+	clk_bulk_disable_unprepare(BULK_CLKS_NUM, mtk->clks);
 	usb_wakeup_set(mtk, true);
 	return 0;
 
@@ -703,7 +632,7 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
 	int ret;
 
 	usb_wakeup_set(mtk, false);
-	ret = xhci_mtk_clks_enable(mtk);
+	ret = clk_bulk_prepare_enable(BULK_CLKS_NUM, mtk->clks);
 	if (ret)
 		goto enable_wakeup;
 
@@ -719,7 +648,7 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
 	return 0;
 
 disable_clks:
-	xhci_mtk_clks_disable(mtk);
+	clk_bulk_disable_unprepare(BULK_CLKS_NUM, mtk->clks);
 enable_wakeup:
 	usb_wakeup_set(mtk, true);
 	return ret;
diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index 621ec1a85009..11996edc1967 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -9,8 +9,12 @@
 #ifndef _XHCI_MTK_H_
 #define _XHCI_MTK_H_
 
+#include <linux/clk.h>
+
 #include "xhci.h"
 
+#define BULK_CLKS_NUM	5
+
 /**
  * To simplify scheduler algorithm, set a upper limit for ESIT,
  * if a synchromous ep's ESIT is larger than @XHCI_MTK_MAX_ESIT,
@@ -140,11 +144,7 @@ struct xhci_hcd_mtk {
 	int u3p_dis_msk;
 	struct regulator *vusb33;
 	struct regulator *vbus;
-	struct clk *sys_clk;	/* sys and mac clock */
-	struct clk *xhci_clk;
-	struct clk *ref_clk;
-	struct clk *mcu_clk;
-	struct clk *dma_clk;
+	struct clk_bulk_data clks[BULK_CLKS_NUM];
 	struct regmap *pericfg;
 	struct phy **phys;
 	int num_phys;
-- 
2.18.0

[PATCH 6/6] usb: xhci-mtk: remove unused members

[Chunfeng Yun]

Now some members about phys and wakeup are not used anymore,
remove them.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
index 11996edc1967..7940593a3445 100644
--- a/drivers/usb/host/xhci-mtk.h
+++ b/drivers/usb/host/xhci-mtk.h
@@ -145,9 +145,6 @@ struct xhci_hcd_mtk {
 	struct regulator *vusb33;
 	struct regulator *vbus;
 	struct clk_bulk_data clks[BULK_CLKS_NUM];
-	struct regmap *pericfg;
-	struct phy **phys;
-	int num_phys;
 	bool lpm_support;
 	/* usb remote wakeup */
 	bool uwk_en;
-- 
2.18.0

Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM

[Chunfeng Yun]

Hi Ikjoon,

On Thu, 2021-04-08 at 17:35 +0800, Chunfeng Yun wrote:
> A dedicated wakeup irq will be used to handle runtime suspend/resume,
> we use dev_pm_set_dedicated_wake_irq API to take care of requesting
> and attaching wakeup irq, then the suspend/resume framework will help
> to enable/disable wakeup irq.
> 
> The runtime PM is default off since some platforms may not support it.
> users can enable it via power/control (set "auto") in sysfs.
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  drivers/usb/host/xhci-mtk.c | 140 +++++++++++++++++++++++++++++++-----
>  1 file changed, 124 insertions(+), 16 deletions(-)

Please help to test the series on mt8192 chromebook, thanks a lot

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Rafael J. Wysocki]

On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> When the dedicated wake irq is level trigger, enable it before
> calling runtime_suspend, will trigger an interrupt.
>
> e.g.
> for a low level trigger type, it's low level at running time (0),
> and becomes high level when enters suspend (runtime_suspend (1) is
> called), a wakeup signal at (2) make it become low level, wake irq
> will be triggered.
>
>                 ------------------
>                |           ^     ^|
> ----------------           |     | --------------
>  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
>
> if we enable the wake irq before calling runtime_suspend during (0),
> an interrupt will arise, it causes resume immediately;

But that's necessary to avoid missing a wakeup interrupt, isn't it?

> enable wake irq after calling runtime_suspend, e.g. at (3) or (4),
> will works.
>
> This patch seems no side effect on edge trigger wake irq.
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  drivers/base/power/runtime.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index a46a7e30881b..796739a015a5 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -619,12 +619,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>         __update_runtime_status(dev, RPM_SUSPENDING);
>
>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> -
> -       dev_pm_enable_wake_irq_check(dev, true);
>         retval = rpm_callback(callback, dev);
>         if (retval)
>                 goto fail;
>
> +       dev_pm_enable_wake_irq_check(dev, true);
> +
>   no_callback:
>         __update_runtime_status(dev, RPM_SUSPENDED);
>         pm_runtime_deactivate_timer(dev);
> @@ -659,7 +659,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>         return retval;
>
>   fail:
> -       dev_pm_disable_wake_irq_check(dev);
>         __update_runtime_status(dev, RPM_ACTIVE);
>         dev->power.deferred_resume = false;
>         wake_up_all(&dev->power.wait_queue);
> --
> 2.18.0
>

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Chunfeng Yun]

On Thu, 2021-04-08 at 19:41 +0200, Rafael J. Wysocki wrote:
> On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > When the dedicated wake irq is level trigger, enable it before
> > calling runtime_suspend, will trigger an interrupt.
> >
> > e.g.
> > for a low level trigger type, it's low level at running time (0),
> > and becomes high level when enters suspend (runtime_suspend (1) is
> > called), a wakeup signal at (2) make it become low level, wake irq
> > will be triggered.
> >
> >                 ------------------
> >                |           ^     ^|
> > ----------------           |     | --------------
> >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> >
> > if we enable the wake irq before calling runtime_suspend during (0),
> > an interrupt will arise, it causes resume immediately;
> 
> But that's necessary to avoid missing a wakeup interrupt, isn't it?
That's also what I worry about.
It may happen if the trigger level only keeps a very short time, and the
interrupt controller can't process it timely, but I don't think it
follow the level trigger mechanism, the HW should latch it until the ISR
is called. right?

> 
> > enable wake irq after calling runtime_suspend, e.g. at (3) or (4),
> > will works.
> >
> > This patch seems no side effect on edge trigger wake irq.
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  drivers/base/power/runtime.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index a46a7e30881b..796739a015a5 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -619,12 +619,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >         __update_runtime_status(dev, RPM_SUSPENDING);
> >
> >         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> > -
> > -       dev_pm_enable_wake_irq_check(dev, true);
> >         retval = rpm_callback(callback, dev);
> >         if (retval)
> >                 goto fail;
> >
> > +       dev_pm_enable_wake_irq_check(dev, true);
> > +
> >   no_callback:
> >         __update_runtime_status(dev, RPM_SUSPENDED);
> >         pm_runtime_deactivate_timer(dev);
> > @@ -659,7 +659,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >         return retval;
> >
> >   fail:
> > -       dev_pm_disable_wake_irq_check(dev);
> >         __update_runtime_status(dev, RPM_ACTIVE);
> >         dev->power.deferred_resume = false;
> >         wake_up_all(&dev->power.wait_queue);
> > --
> > 2.18.0
> >

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Ikjoon Jang]

Hi Chunfeng,

On Thu, Apr 8, 2021 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> When the dedicated wake irq is level trigger, enable it before
> calling runtime_suspend, will trigger an interrupt.
>
> e.g.
> for a low level trigger type, it's low level at running time (0),
> and becomes high level when enters suspend (runtime_suspend (1) is
> called), a wakeup signal at (2) make it become low level, wake irq
> will be triggered.
>
>                 ------------------
>                |           ^     ^|
> ----------------           |     | --------------
>  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
>

Can't we just use a falling edge type for this irq line?

> if we enable the wake irq before calling runtime_suspend during (0),
> an interrupt will arise, it causes resume immediately;
> enable wake irq after calling runtime_suspend, e.g. at (3) or (4),
> will works.
>
> This patch seems no side effect on edge trigger wake irq.
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  drivers/base/power/runtime.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index a46a7e30881b..796739a015a5 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -619,12 +619,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>         __update_runtime_status(dev, RPM_SUSPENDING);
>
>         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> -
> -       dev_pm_enable_wake_irq_check(dev, true);
>         retval = rpm_callback(callback, dev);
>         if (retval)
>                 goto fail;
>
> +       dev_pm_enable_wake_irq_check(dev, true);
> +
>   no_callback:
>         __update_runtime_status(dev, RPM_SUSPENDED);
>         pm_runtime_deactivate_timer(dev);
> @@ -659,7 +659,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>         return retval;
>
>   fail:
> -       dev_pm_disable_wake_irq_check(dev);
>         __update_runtime_status(dev, RPM_ACTIVE);
>         dev->power.deferred_resume = false;
>         wake_up_all(&dev->power.wait_queue);
> --
> 2.18.0
>

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Tony Lindgren]

* Chunfeng Yun <chunfeng.yun@mediatek.com> [210409 01:54]:
> On Thu, 2021-04-08 at 19:41 +0200, Rafael J. Wysocki wrote:
> > On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> > >
> > > When the dedicated wake irq is level trigger, enable it before
> > > calling runtime_suspend, will trigger an interrupt.
> > >
> > > e.g.
> > > for a low level trigger type, it's low level at running time (0),
> > > and becomes high level when enters suspend (runtime_suspend (1) is
> > > called), a wakeup signal at (2) make it become low level, wake irq
> > > will be triggered.
> > >
> > >                 ------------------
> > >                |           ^     ^|
> > > ----------------           |     | --------------
> > >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> > >
> > > if we enable the wake irq before calling runtime_suspend during (0),
> > > an interrupt will arise, it causes resume immediately;
> > 
> > But that's necessary to avoid missing a wakeup interrupt, isn't it?
> That's also what I worry about.

Yeah sounds like this patch will lead into missed wakeirqs.

Regards,

Tony

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Tony Lindgren]

* Ikjoon Jang <ikjn@chromium.org> [210409 05:33]:
> Hi Chunfeng,
> 
> On Thu, Apr 8, 2021 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > When the dedicated wake irq is level trigger, enable it before
> > calling runtime_suspend, will trigger an interrupt.
> >
> > e.g.
> > for a low level trigger type, it's low level at running time (0),
> > and becomes high level when enters suspend (runtime_suspend (1) is
> > called), a wakeup signal at (2) make it become low level, wake irq
> > will be triggered.
> >
> >                 ------------------
> >                |           ^     ^|
> > ----------------           |     | --------------
> >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> >
> 
> Can't we just use a falling edge type for this irq line?

Sounds reasonable to me :)

Tony

Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM

[Ikjoon Jang]

On Thu, Apr 8, 2021 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> A dedicated wakeup irq will be used to handle runtime suspend/resume,
> we use dev_pm_set_dedicated_wake_irq API to take care of requesting
> and attaching wakeup irq, then the suspend/resume framework will help
> to enable/disable wakeup irq.
>
> The runtime PM is default off since some platforms may not support it.
> users can enable it via power/control (set "auto") in sysfs.
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  drivers/usb/host/xhci-mtk.c | 140 +++++++++++++++++++++++++++++++-----
>  1 file changed, 124 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> index a74764ab914a..30927f4064d4 100644
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -16,6 +16,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/pm_wakeirq.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>
> @@ -358,7 +359,6 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
>                         mtk->uwk_reg_base, mtk->uwk_vers);
>
>         return PTR_ERR_OR_ZERO(mtk->uwk);
> -
>  }
>
>  static void usb_wakeup_set(struct xhci_hcd_mtk *mtk, bool enable)
> @@ -458,6 +458,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>         struct resource *res;
>         struct usb_hcd *hcd;
>         int ret = -ENODEV;
> +       int wakeup_irq;
>         int irq;
>
>         if (usb_disabled())
> @@ -485,6 +486,21 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>
> +       irq = platform_get_irq_byname_optional(pdev, "host");
> +       if (irq < 0) {
> +               if (irq == -EPROBE_DEFER)
> +                       return irq;
> +
> +               /* for backward compatibility */
> +               irq = platform_get_irq(pdev, 0);
> +               if (irq < 0)
> +                       return irq;
> +       }
> +
> +       wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> +       if (wakeup_irq == -EPROBE_DEFER)
> +               return wakeup_irq;
> +
>         mtk->lpm_support = of_property_read_bool(node, "usb3-lpm-capable");
>         /* optional property, ignore the error if it does not exist */
>         of_property_read_u32(node, "mediatek,u3p-dis-msk",
> @@ -496,9 +512,11 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>                 return ret;
>         }
>
> +       pm_runtime_set_active(dev);
> +       pm_runtime_use_autosuspend(dev);
> +       pm_runtime_set_autosuspend_delay(dev, 4000);
>         pm_runtime_enable(dev);
>         pm_runtime_get_sync(dev);
> -       device_enable_async_suspend(dev);
>
>         ret = xhci_mtk_ldos_enable(mtk);
>         if (ret)
> @@ -508,12 +526,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>         if (ret)
>                 goto disable_ldos;
>
> -       irq = platform_get_irq(pdev, 0);
> -       if (irq < 0) {
> -               ret = irq;
> -               goto disable_clk;
> -       }
> -
>         hcd = usb_create_hcd(driver, dev, dev_name(dev));
>         if (!hcd) {
>                 ret = -ENOMEM;
> @@ -579,8 +591,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>         if (ret)
>                 goto dealloc_usb2_hcd;
>
> +       if (wakeup_irq > 0) {
> +               ret = dev_pm_set_dedicated_wake_irq(dev, wakeup_irq);
> +               if (ret) {
> +                       dev_err(dev, "set wakeup irq %d failed\n", wakeup_irq);
> +                       goto dealloc_usb3_hcd;
> +               }
> +               dev_info(dev, "wakeup irq %d\n", wakeup_irq);
> +       }
> +
> +       device_enable_async_suspend(dev);
> +       pm_runtime_mark_last_busy(dev);
> +       pm_runtime_put_autosuspend(dev);
> +       pm_runtime_forbid(dev);
> +
>         return 0;
>
> +dealloc_usb3_hcd:
> +       usb_remove_hcd(xhci->shared_hcd);
> +       xhci->shared_hcd = NULL;
> +
>  dealloc_usb2_hcd:
>         usb_remove_hcd(hcd);
>
> @@ -601,25 +631,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
>         xhci_mtk_ldos_disable(mtk);
>
>  disable_pm:
> -       pm_runtime_put_sync(dev);
> +       pm_runtime_put_sync_autosuspend(dev);
>         pm_runtime_disable(dev);
>         return ret;
>  }
>
> -static int xhci_mtk_remove(struct platform_device *dev)
> +static int xhci_mtk_remove(struct platform_device *pdev)
>  {
> -       struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
> +       struct xhci_hcd_mtk *mtk = platform_get_drvdata(pdev);
>         struct usb_hcd  *hcd = mtk->hcd;
>         struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>         struct usb_hcd  *shared_hcd = xhci->shared_hcd;
> +       struct device *dev = &pdev->dev;
>
> -       pm_runtime_put_noidle(&dev->dev);
> -       pm_runtime_disable(&dev->dev);
> +       pm_runtime_get_sync(dev);
> +       xhci->xhc_state |= XHCI_STATE_REMOVING;
> +       dev_pm_clear_wake_irq(dev);
> +       device_init_wakeup(dev, false);
>
>         usb_remove_hcd(shared_hcd);
>         xhci->shared_hcd = NULL;
> -       device_init_wakeup(&dev->dev, false);
> -
>         usb_remove_hcd(hcd);
>         usb_put_hcd(shared_hcd);
>         usb_put_hcd(hcd);
> @@ -627,6 +658,10 @@ static int xhci_mtk_remove(struct platform_device *dev)
>         xhci_mtk_clks_disable(mtk);
>         xhci_mtk_ldos_disable(mtk);
>
> +       pm_runtime_disable(dev);
> +       pm_runtime_put_noidle(dev);
> +       pm_runtime_set_suspended(dev);
> +
>         return 0;
>  }
>
> @@ -690,10 +725,83 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
>         return ret;
>  }
>
> +static int check_rhub_status(struct xhci_hcd *xhci, struct xhci_hub *rhub)
> +{
> +       u32 suspended_ports;
> +       u32 status;
> +       int num_ports;
> +       int i;
> +
> +       num_ports = rhub->num_ports;
> +       suspended_ports = rhub->bus_state.suspended_ports;
> +       for (i = 0; i < num_ports; i++) {
> +               if (!(suspended_ports & BIT(i))) {
> +                       status = readl(rhub->ports[i]->addr);
> +                       if (status & PORT_CONNECT)

So this pm_runtime support is activated only when there's no devices
connected at all?
I think this will always return -EBUSY with my board having an on-board hub
connected to both rhubs.

> +                               return -EBUSY;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * check the bus whether it could suspend or not
> + * the bus will suspend if the downstream ports are already suspended,
> + * or no devices connected.
> + */
> +static int check_bus_status(struct xhci_hcd *xhci)
> +{
> +       int ret;
> +
> +       ret = check_rhub_status(xhci, &xhci->usb3_rhub);
> +       if (ret)
> +               return ret;
> +
> +       return check_rhub_status(xhci, &xhci->usb2_rhub);
> +}
> +
> +static int __maybe_unused xhci_mtk_runtime_suspend(struct device *dev)
> +{
> +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> +       int ret = 0;
> +
> +       if (xhci->xhc_state)
> +               return -ESHUTDOWN;
> +
> +       if (device_may_wakeup(dev)) {
> +               ret = check_bus_status(xhci);
> +               if (!ret)
> +                       ret = xhci_mtk_suspend(dev);
> +       }
> +
> +       /* -EBUSY: let PM automatically reschedule another autosuspend */
> +       return ret ? -EBUSY : 0;
> +}
> +
> +static int __maybe_unused xhci_mtk_runtime_resume(struct device *dev)
> +{
> +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> +       int ret = 0;
> +
> +       if (xhci->xhc_state)
> +               return -ESHUTDOWN;
> +
> +       if (device_may_wakeup(dev))
> +               ret = xhci_mtk_resume(dev);
> +
> +       return ret;
> +}
> +
>  static const struct dev_pm_ops xhci_mtk_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(xhci_mtk_suspend, xhci_mtk_resume)
> +       SET_RUNTIME_PM_OPS(xhci_mtk_runtime_suspend,
> +                          xhci_mtk_runtime_resume, NULL)
>  };
> -#define DEV_PM_OPS IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL
> +
> +#define DEV_PM_OPS (IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL)
>
>  static const struct of_device_id mtk_xhci_of_match[] = {
>         { .compatible = "mediatek,mt8173-xhci"},
> --
> 2.18.0
>

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Chunfeng Yun]

On Fri, 2021-04-09 at 08:39 +0300, Tony Lindgren wrote:
> * Chunfeng Yun <chunfeng.yun@mediatek.com> [210409 01:54]:
> > On Thu, 2021-04-08 at 19:41 +0200, Rafael J. Wysocki wrote:
> > > On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> > > >
> > > > When the dedicated wake irq is level trigger, enable it before
> > > > calling runtime_suspend, will trigger an interrupt.
> > > >
> > > > e.g.
> > > > for a low level trigger type, it's low level at running time (0),
> > > > and becomes high level when enters suspend (runtime_suspend (1) is
> > > > called), a wakeup signal at (2) make it become low level, wake irq
> > > > will be triggered.
> > > >
> > > >                 ------------------
> > > >                |           ^     ^|
> > > > ----------------           |     | --------------
> > > >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> > > >
> > > > if we enable the wake irq before calling runtime_suspend during (0),
> > > > an interrupt will arise, it causes resume immediately;
> > > 
> > > But that's necessary to avoid missing a wakeup interrupt, isn't it?
> > That's also what I worry about.
> 
> Yeah sounds like this patch will lead into missed wakeirqs.
If miss level trigger wakeirqs, that means HW doesn't latch it? is it HW
limitation?
> 
> Regards,
> 
> Tony

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Chunfeng Yun]

On Fri, 2021-04-09 at 13:32 +0800, Ikjoon Jang wrote:
> Hi Chunfeng,
> 
> On Thu, Apr 8, 2021 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > When the dedicated wake irq is level trigger, enable it before
> > calling runtime_suspend, will trigger an interrupt.
> >
> > e.g.
> > for a low level trigger type, it's low level at running time (0),
> > and becomes high level when enters suspend (runtime_suspend (1) is
> > called), a wakeup signal at (2) make it become low level, wake irq
> > will be triggered.
> >
> >                 ------------------
> >                |           ^     ^|
> > ----------------           |     | --------------
> >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> >
> 
> Can't we just use a falling edge type for this irq line?
I'll try it, but the original code still doesn't process above mentioned
case.

> 
> > if we enable the wake irq before calling runtime_suspend during (0),
> > an interrupt will arise, it causes resume immediately;
> > enable wake irq after calling runtime_suspend, e.g. at (3) or (4),
> > will works.
> >
> > This patch seems no side effect on edge trigger wake irq.
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  drivers/base/power/runtime.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> > index a46a7e30881b..796739a015a5 100644
> > --- a/drivers/base/power/runtime.c
> > +++ b/drivers/base/power/runtime.c
> > @@ -619,12 +619,12 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >         __update_runtime_status(dev, RPM_SUSPENDING);
> >
> >         callback = RPM_GET_CALLBACK(dev, runtime_suspend);
> > -
> > -       dev_pm_enable_wake_irq_check(dev, true);
> >         retval = rpm_callback(callback, dev);
> >         if (retval)
> >                 goto fail;
> >
> > +       dev_pm_enable_wake_irq_check(dev, true);
> > +
> >   no_callback:
> >         __update_runtime_status(dev, RPM_SUSPENDED);
> >         pm_runtime_deactivate_timer(dev);
> > @@ -659,7 +659,6 @@ static int rpm_suspend(struct device *dev, int rpmflags)
> >         return retval;
> >
> >   fail:
> > -       dev_pm_disable_wake_irq_check(dev);
> >         __update_runtime_status(dev, RPM_ACTIVE);
> >         dev->power.deferred_resume = false;
> >         wake_up_all(&dev->power.wait_queue);
> > --
> > 2.18.0
> >

Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM

[Chunfeng Yun]

On Fri, 2021-04-09 at 13:45 +0800, Ikjoon Jang wrote:
> On Thu, Apr 8, 2021 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > A dedicated wakeup irq will be used to handle runtime suspend/resume,
> > we use dev_pm_set_dedicated_wake_irq API to take care of requesting
> > and attaching wakeup irq, then the suspend/resume framework will help
> > to enable/disable wakeup irq.
> >
> > The runtime PM is default off since some platforms may not support it.
> > users can enable it via power/control (set "auto") in sysfs.
> >
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> >  drivers/usb/host/xhci-mtk.c | 140 +++++++++++++++++++++++++++++++-----
> >  1 file changed, 124 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> > index a74764ab914a..30927f4064d4 100644
> > --- a/drivers/usb/host/xhci-mtk.c
> > +++ b/drivers/usb/host/xhci-mtk.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/pm_wakeirq.h>
> >  #include <linux/regmap.h>
> >  #include <linux/regulator/consumer.h>
> >
> > @@ -358,7 +359,6 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
> >                         mtk->uwk_reg_base, mtk->uwk_vers);
> >
> >         return PTR_ERR_OR_ZERO(mtk->uwk);
> > -
> >  }
> >
> >  static void usb_wakeup_set(struct xhci_hcd_mtk *mtk, bool enable)
> > @@ -458,6 +458,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> >         struct resource *res;
> >         struct usb_hcd *hcd;
> >         int ret = -ENODEV;
> > +       int wakeup_irq;
> >         int irq;
> >
> >         if (usb_disabled())
> > @@ -485,6 +486,21 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> >         if (ret)
> >                 return ret;
> >
> > +       irq = platform_get_irq_byname_optional(pdev, "host");
> > +       if (irq < 0) {
> > +               if (irq == -EPROBE_DEFER)
> > +                       return irq;
> > +
> > +               /* for backward compatibility */
> > +               irq = platform_get_irq(pdev, 0);
> > +               if (irq < 0)
> > +                       return irq;
> > +       }
> > +
> > +       wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> > +       if (wakeup_irq == -EPROBE_DEFER)
> > +               return wakeup_irq;
> > +
> >         mtk->lpm_support = of_property_read_bool(node, "usb3-lpm-capable");
> >         /* optional property, ignore the error if it does not exist */
> >         of_property_read_u32(node, "mediatek,u3p-dis-msk",
> > @@ -496,9 +512,11 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> >                 return ret;
> >         }
> >
> > +       pm_runtime_set_active(dev);
> > +       pm_runtime_use_autosuspend(dev);
> > +       pm_runtime_set_autosuspend_delay(dev, 4000);
> >         pm_runtime_enable(dev);
> >         pm_runtime_get_sync(dev);
> > -       device_enable_async_suspend(dev);
> >
> >         ret = xhci_mtk_ldos_enable(mtk);
> >         if (ret)
> > @@ -508,12 +526,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto disable_ldos;
> >
> > -       irq = platform_get_irq(pdev, 0);
> > -       if (irq < 0) {
> > -               ret = irq;
> > -               goto disable_clk;
> > -       }
> > -
> >         hcd = usb_create_hcd(driver, dev, dev_name(dev));
> >         if (!hcd) {
> >                 ret = -ENOMEM;
> > @@ -579,8 +591,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto dealloc_usb2_hcd;
> >
> > +       if (wakeup_irq > 0) {
> > +               ret = dev_pm_set_dedicated_wake_irq(dev, wakeup_irq);
> > +               if (ret) {
> > +                       dev_err(dev, "set wakeup irq %d failed\n", wakeup_irq);
> > +                       goto dealloc_usb3_hcd;
> > +               }
> > +               dev_info(dev, "wakeup irq %d\n", wakeup_irq);
> > +       }
> > +
> > +       device_enable_async_suspend(dev);
> > +       pm_runtime_mark_last_busy(dev);
> > +       pm_runtime_put_autosuspend(dev);
> > +       pm_runtime_forbid(dev);
> > +
> >         return 0;
> >
> > +dealloc_usb3_hcd:
> > +       usb_remove_hcd(xhci->shared_hcd);
> > +       xhci->shared_hcd = NULL;
> > +
> >  dealloc_usb2_hcd:
> >         usb_remove_hcd(hcd);
> >
> > @@ -601,25 +631,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> >         xhci_mtk_ldos_disable(mtk);
> >
> >  disable_pm:
> > -       pm_runtime_put_sync(dev);
> > +       pm_runtime_put_sync_autosuspend(dev);
> >         pm_runtime_disable(dev);
> >         return ret;
> >  }
> >
> > -static int xhci_mtk_remove(struct platform_device *dev)
> > +static int xhci_mtk_remove(struct platform_device *pdev)
> >  {
> > -       struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
> > +       struct xhci_hcd_mtk *mtk = platform_get_drvdata(pdev);
> >         struct usb_hcd  *hcd = mtk->hcd;
> >         struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> >         struct usb_hcd  *shared_hcd = xhci->shared_hcd;
> > +       struct device *dev = &pdev->dev;
> >
> > -       pm_runtime_put_noidle(&dev->dev);
> > -       pm_runtime_disable(&dev->dev);
> > +       pm_runtime_get_sync(dev);
> > +       xhci->xhc_state |= XHCI_STATE_REMOVING;
> > +       dev_pm_clear_wake_irq(dev);
> > +       device_init_wakeup(dev, false);
> >
> >         usb_remove_hcd(shared_hcd);
> >         xhci->shared_hcd = NULL;
> > -       device_init_wakeup(&dev->dev, false);
> > -
> >         usb_remove_hcd(hcd);
> >         usb_put_hcd(shared_hcd);
> >         usb_put_hcd(hcd);
> > @@ -627,6 +658,10 @@ static int xhci_mtk_remove(struct platform_device *dev)
> >         xhci_mtk_clks_disable(mtk);
> >         xhci_mtk_ldos_disable(mtk);
> >
> > +       pm_runtime_disable(dev);
> > +       pm_runtime_put_noidle(dev);
> > +       pm_runtime_set_suspended(dev);
> > +
> >         return 0;
> >  }
> >
> > @@ -690,10 +725,83 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
> >         return ret;
> >  }
> >
> > +static int check_rhub_status(struct xhci_hcd *xhci, struct xhci_hub *rhub)
> > +{
> > +       u32 suspended_ports;
> > +       u32 status;
> > +       int num_ports;
> > +       int i;
> > +
> > +       num_ports = rhub->num_ports;
> > +       suspended_ports = rhub->bus_state.suspended_ports;
> > +       for (i = 0; i < num_ports; i++) {
> > +               if (!(suspended_ports & BIT(i))) {
> > +                       status = readl(rhub->ports[i]->addr);
> > +                       if (status & PORT_CONNECT)
> 
> So this pm_runtime support is activated only when there's no devices
> connected at all?
No, if the connected devices also support runtime suspend, it will enter
suspend mode when no data transfer, then the controller can enter
suspend too
> I think this will always return -EBUSY with my board having an on-board hub
> connected to both rhubs.
the on-board hub supports runtime suspend by default, so if no devices
connected, it will enter suspend

> 
> > +                               return -EBUSY;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +/*
> > + * check the bus whether it could suspend or not
> > + * the bus will suspend if the downstream ports are already suspended,
> > + * or no devices connected.
> > + */
> > +static int check_bus_status(struct xhci_hcd *xhci)
> > +{
> > +       int ret;
> > +
> > +       ret = check_rhub_status(xhci, &xhci->usb3_rhub);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return check_rhub_status(xhci, &xhci->usb2_rhub);
> > +}
> > +
> > +static int __maybe_unused xhci_mtk_runtime_suspend(struct device *dev)
> > +{
> > +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> > +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> > +       int ret = 0;
> > +
> > +       if (xhci->xhc_state)
> > +               return -ESHUTDOWN;
> > +
> > +       if (device_may_wakeup(dev)) {
> > +               ret = check_bus_status(xhci);
> > +               if (!ret)
> > +                       ret = xhci_mtk_suspend(dev);
> > +       }
> > +
> > +       /* -EBUSY: let PM automatically reschedule another autosuspend */
> > +       return ret ? -EBUSY : 0;
> > +}
> > +
> > +static int __maybe_unused xhci_mtk_runtime_resume(struct device *dev)
> > +{
> > +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> > +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> > +       int ret = 0;
> > +
> > +       if (xhci->xhc_state)
> > +               return -ESHUTDOWN;
> > +
> > +       if (device_may_wakeup(dev))
> > +               ret = xhci_mtk_resume(dev);
> > +
> > +       return ret;
> > +}
> > +
> >  static const struct dev_pm_ops xhci_mtk_pm_ops = {
> >         SET_SYSTEM_SLEEP_PM_OPS(xhci_mtk_suspend, xhci_mtk_resume)
> > +       SET_RUNTIME_PM_OPS(xhci_mtk_runtime_suspend,
> > +                          xhci_mtk_runtime_resume, NULL)
> >  };
> > -#define DEV_PM_OPS IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL
> > +
> > +#define DEV_PM_OPS (IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL)
> >
> >  static const struct of_device_id mtk_xhci_of_match[] = {
> >         { .compatible = "mediatek,mt8173-xhci"},
> > --
> > 2.18.0
> >

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Rafael J. Wysocki]

On Fri, Apr 9, 2021 at 10:36 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> On Fri, 2021-04-09 at 08:39 +0300, Tony Lindgren wrote:
> > * Chunfeng Yun <chunfeng.yun@mediatek.com> [210409 01:54]:
> > > On Thu, 2021-04-08 at 19:41 +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> > > > >
> > > > > When the dedicated wake irq is level trigger, enable it before
> > > > > calling runtime_suspend, will trigger an interrupt.
> > > > >
> > > > > e.g.
> > > > > for a low level trigger type, it's low level at running time (0),
> > > > > and becomes high level when enters suspend (runtime_suspend (1) is
> > > > > called), a wakeup signal at (2) make it become low level, wake irq
> > > > > will be triggered.
> > > > >
> > > > >                 ------------------
> > > > >                |           ^     ^|
> > > > > ----------------           |     | --------------
> > > > >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> > > > >
> > > > > if we enable the wake irq before calling runtime_suspend during (0),
> > > > > an interrupt will arise, it causes resume immediately;
> > > >
> > > > But that's necessary to avoid missing a wakeup interrupt, isn't it?
> > > That's also what I worry about.
> >
> > Yeah sounds like this patch will lead into missed wakeirqs.
> If miss level trigger wakeirqs, that means HW doesn't latch it? is it HW
> limitation?

If it's level-triggered, it won't be missed, but then it is just
pointless to suspend the device when wakeup is being signaled in the
first place.

I'm not sure if I understand the underlying problem correctly.  Is it
about addressing spurious wakeups?

Re: [PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt

[Rob Herring]

On Thu, 08 Apr 2021 17:35:11 +0800, Chunfeng Yun wrote:
> Add an interrupt which is EINT usually to support runtime PM,
> meanwhile add "interrupt-names" property, for backward
> compatibility, it's optional and used when wakeup interrupt
> exists
> 
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
>  .../devicetree/bindings/usb/mediatek,mtk-xhci.yaml  | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called

[Chunfeng Yun]

On Fri, 2021-04-09 at 13:14 +0200, Rafael J. Wysocki wrote:
> On Fri, Apr 9, 2021 at 10:36 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > On Fri, 2021-04-09 at 08:39 +0300, Tony Lindgren wrote:
> > > * Chunfeng Yun <chunfeng.yun@mediatek.com> [210409 01:54]:
> > > > On Thu, 2021-04-08 at 19:41 +0200, Rafael J. Wysocki wrote:
> > > > > On Thu, Apr 8, 2021 at 11:35 AM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> > > > > >
> > > > > > When the dedicated wake irq is level trigger, enable it before
> > > > > > calling runtime_suspend, will trigger an interrupt.
> > > > > >
> > > > > > e.g.
> > > > > > for a low level trigger type, it's low level at running time (0),
> > > > > > and becomes high level when enters suspend (runtime_suspend (1) is
> > > > > > called), a wakeup signal at (2) make it become low level, wake irq
> > > > > > will be triggered.
> > > > > >
> > > > > >                 ------------------
> > > > > >                |           ^     ^|
> > > > > > ----------------           |     | --------------
> > > > > >  |<---(0)--->|<--(1)--|   (3)   (2)    (4)
> > > > > >
> > > > > > if we enable the wake irq before calling runtime_suspend during (0),
> > > > > > an interrupt will arise, it causes resume immediately;
> > > > >
> > > > > But that's necessary to avoid missing a wakeup interrupt, isn't it?
> > > > That's also what I worry about.
> > >
> > > Yeah sounds like this patch will lead into missed wakeirqs.
> > If miss level trigger wakeirqs, that means HW doesn't latch it? is it HW
> > limitation?
> 
> If it's level-triggered, it won't be missed, but then it is just
> pointless to suspend the device when wakeup is being signaled in the
> first place.
Got it
> 
> I'm not sure if I understand the underlying problem correctly.  Is it
> about addressing spurious wakeups?
In fact, it's default value is the same as the wakeup signal, maybe the
above case, using level trigger, should be avoided, it is not clear and
causes confusion, as Ikjoon and Tony suggested, using falling edge type
is better.

Thanks a lot

Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM

[Ikjoon Jang]

On Fri, Apr 9, 2021 at 4:54 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
>
> On Fri, 2021-04-09 at 13:45 +0800, Ikjoon Jang wrote:
> > On Thu, Apr 8, 2021 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> > >
> > > A dedicated wakeup irq will be used to handle runtime suspend/resume,
> > > we use dev_pm_set_dedicated_wake_irq API to take care of requesting
> > > and attaching wakeup irq, then the suspend/resume framework will help
> > > to enable/disable wakeup irq.
> > >
> > > The runtime PM is default off since some platforms may not support it.
> > > users can enable it via power/control (set "auto") in sysfs.
> > >
> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > ---
> > >  drivers/usb/host/xhci-mtk.c | 140 +++++++++++++++++++++++++++++++-----
> > >  1 file changed, 124 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> > > index a74764ab914a..30927f4064d4 100644
> > > --- a/drivers/usb/host/xhci-mtk.c
> > > +++ b/drivers/usb/host/xhci-mtk.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/pm_wakeirq.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/regulator/consumer.h>
> > >
> > > @@ -358,7 +359,6 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
> > >                         mtk->uwk_reg_base, mtk->uwk_vers);
> > >
> > >         return PTR_ERR_OR_ZERO(mtk->uwk);
> > > -
> > >  }
> > >
> > >  static void usb_wakeup_set(struct xhci_hcd_mtk *mtk, bool enable)
> > > @@ -458,6 +458,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> > >         struct resource *res;
> > >         struct usb_hcd *hcd;
> > >         int ret = -ENODEV;
> > > +       int wakeup_irq;
> > >         int irq;
> > >
> > >         if (usb_disabled())
> > > @@ -485,6 +486,21 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       irq = platform_get_irq_byname_optional(pdev, "host");
> > > +       if (irq < 0) {
> > > +               if (irq == -EPROBE_DEFER)
> > > +                       return irq;
> > > +
> > > +               /* for backward compatibility */
> > > +               irq = platform_get_irq(pdev, 0);
> > > +               if (irq < 0)
> > > +                       return irq;
> > > +       }
> > > +
> > > +       wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
> > > +       if (wakeup_irq == -EPROBE_DEFER)
> > > +               return wakeup_irq;
> > > +
> > >         mtk->lpm_support = of_property_read_bool(node, "usb3-lpm-capable");
> > >         /* optional property, ignore the error if it does not exist */
> > >         of_property_read_u32(node, "mediatek,u3p-dis-msk",
> > > @@ -496,9 +512,11 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> > >                 return ret;
> > >         }
> > >
> > > +       pm_runtime_set_active(dev);
> > > +       pm_runtime_use_autosuspend(dev);
> > > +       pm_runtime_set_autosuspend_delay(dev, 4000);
> > >         pm_runtime_enable(dev);
> > >         pm_runtime_get_sync(dev);
> > > -       device_enable_async_suspend(dev);
> > >
> > >         ret = xhci_mtk_ldos_enable(mtk);
> > >         if (ret)
> > > @@ -508,12 +526,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> > >         if (ret)
> > >                 goto disable_ldos;
> > >
> > > -       irq = platform_get_irq(pdev, 0);
> > > -       if (irq < 0) {
> > > -               ret = irq;
> > > -               goto disable_clk;
> > > -       }
> > > -
> > >         hcd = usb_create_hcd(driver, dev, dev_name(dev));
> > >         if (!hcd) {
> > >                 ret = -ENOMEM;
> > > @@ -579,8 +591,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> > >         if (ret)
> > >                 goto dealloc_usb2_hcd;
> > >
> > > +       if (wakeup_irq > 0) {
> > > +               ret = dev_pm_set_dedicated_wake_irq(dev, wakeup_irq);
> > > +               if (ret) {
> > > +                       dev_err(dev, "set wakeup irq %d failed\n", wakeup_irq);
> > > +                       goto dealloc_usb3_hcd;
> > > +               }
> > > +               dev_info(dev, "wakeup irq %d\n", wakeup_irq);
> > > +       }
> > > +
> > > +       device_enable_async_suspend(dev);
> > > +       pm_runtime_mark_last_busy(dev);
> > > +       pm_runtime_put_autosuspend(dev);
> > > +       pm_runtime_forbid(dev);
> > > +
> > >         return 0;
> > >
> > > +dealloc_usb3_hcd:
> > > +       usb_remove_hcd(xhci->shared_hcd);
> > > +       xhci->shared_hcd = NULL;
> > > +
> > >  dealloc_usb2_hcd:
> > >         usb_remove_hcd(hcd);
> > >
> > > @@ -601,25 +631,26 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> > >         xhci_mtk_ldos_disable(mtk);
> > >
> > >  disable_pm:
> > > -       pm_runtime_put_sync(dev);
> > > +       pm_runtime_put_sync_autosuspend(dev);
> > >         pm_runtime_disable(dev);
> > >         return ret;
> > >  }
> > >
> > > -static int xhci_mtk_remove(struct platform_device *dev)
> > > +static int xhci_mtk_remove(struct platform_device *pdev)
> > >  {
> > > -       struct xhci_hcd_mtk *mtk = platform_get_drvdata(dev);
> > > +       struct xhci_hcd_mtk *mtk = platform_get_drvdata(pdev);
> > >         struct usb_hcd  *hcd = mtk->hcd;
> > >         struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> > >         struct usb_hcd  *shared_hcd = xhci->shared_hcd;
> > > +       struct device *dev = &pdev->dev;
> > >
> > > -       pm_runtime_put_noidle(&dev->dev);
> > > -       pm_runtime_disable(&dev->dev);
> > > +       pm_runtime_get_sync(dev);
> > > +       xhci->xhc_state |= XHCI_STATE_REMOVING;
> > > +       dev_pm_clear_wake_irq(dev);
> > > +       device_init_wakeup(dev, false);
> > >
> > >         usb_remove_hcd(shared_hcd);
> > >         xhci->shared_hcd = NULL;
> > > -       device_init_wakeup(&dev->dev, false);
> > > -
> > >         usb_remove_hcd(hcd);
> > >         usb_put_hcd(shared_hcd);
> > >         usb_put_hcd(hcd);
> > > @@ -627,6 +658,10 @@ static int xhci_mtk_remove(struct platform_device *dev)
> > >         xhci_mtk_clks_disable(mtk);
> > >         xhci_mtk_ldos_disable(mtk);
> > >
> > > +       pm_runtime_disable(dev);
> > > +       pm_runtime_put_noidle(dev);
> > > +       pm_runtime_set_suspended(dev);
> > > +
> > >         return 0;
> > >  }
> > >
> > > @@ -690,10 +725,83 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
> > >         return ret;
> > >  }
> > >
> > > +static int check_rhub_status(struct xhci_hcd *xhci, struct xhci_hub *rhub)
> > > +{
> > > +       u32 suspended_ports;
> > > +       u32 status;
> > > +       int num_ports;
> > > +       int i;
> > > +
> > > +       num_ports = rhub->num_ports;
> > > +       suspended_ports = rhub->bus_state.suspended_ports;
> > > +       for (i = 0; i < num_ports; i++) {
> > > +               if (!(suspended_ports & BIT(i))) {
> > > +                       status = readl(rhub->ports[i]->addr);
> > > +                       if (status & PORT_CONNECT)
> >
> > So this pm_runtime support is activated only when there's no devices
> > connected at all?
> No, if the connected devices also support runtime suspend, it will enter
> suspend mode when no data transfer, then the controller can enter
> suspend too
> > I think this will always return -EBUSY with my board having an on-board hub
> > connected to both rhubs.
> the on-board hub supports runtime suspend by default, so if no devices
> connected, it will enter suspend

Sorry, you're correct. I was confused that the condition was
(suspended && connect)
My on-board hub connected to rhub is always in a suspended state
whenever it's called.

However, I don't think this could return -EBUSY
rpm_suspend() only be called when all the descendants are in sleep already.
Did you see any cases of this function returning -EBUSY or any concerns on here?


>
> >
> > > +                               return -EBUSY;
> > > +               }
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +/*
> > > + * check the bus whether it could suspend or not
> > > + * the bus will suspend if the downstream ports are already suspended,
> > > + * or no devices connected.
> > > + */
> > > +static int check_bus_status(struct xhci_hcd *xhci)
> > > +{
> > > +       int ret;
> > > +
> > > +       ret = check_rhub_status(xhci, &xhci->usb3_rhub);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       return check_rhub_status(xhci, &xhci->usb2_rhub);
> > > +}
> > > +
> > > +static int __maybe_unused xhci_mtk_runtime_suspend(struct device *dev)
> > > +{
> > > +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> > > +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> > > +       int ret = 0;
> > > +
> > > +       if (xhci->xhc_state)
> > > +               return -ESHUTDOWN;
> > > +
> > > +       if (device_may_wakeup(dev)) {
> > > +               ret = check_bus_status(xhci);
> > > +               if (!ret)
> > > +                       ret = xhci_mtk_suspend(dev);
> > > +       }
> > > +
> > > +       /* -EBUSY: let PM automatically reschedule another autosuspend */
> > > +       return ret ? -EBUSY : 0;
> > > +}
> > > +
> > > +static int __maybe_unused xhci_mtk_runtime_resume(struct device *dev)
> > > +{
> > > +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> > > +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> > > +       int ret = 0;
> > > +
> > > +       if (xhci->xhc_state)
> > > +               return -ESHUTDOWN;
> > > +
> > > +       if (device_may_wakeup(dev))
> > > +               ret = xhci_mtk_resume(dev);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  static const struct dev_pm_ops xhci_mtk_pm_ops = {
> > >         SET_SYSTEM_SLEEP_PM_OPS(xhci_mtk_suspend, xhci_mtk_resume)
> > > +       SET_RUNTIME_PM_OPS(xhci_mtk_runtime_suspend,
> > > +                          xhci_mtk_runtime_resume, NULL)
> > >  };
> > > -#define DEV_PM_OPS IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL
> > > +
> > > +#define DEV_PM_OPS (IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL)
> > >
> > >  static const struct of_device_id mtk_xhci_of_match[] = {
> > >         { .compatible = "mediatek,mt8173-xhci"},
> > > --
> > > 2.18.0
> > >
>

Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM

[Chunfeng Yun]

On Mon, 2021-04-12 at 13:14 +0800, Ikjoon Jang wrote:
> On Fri, Apr 9, 2021 at 4:54 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> >
> > On Fri, 2021-04-09 at 13:45 +0800, Ikjoon Jang wrote:
> > > On Thu, Apr 8, 2021 at 5:35 PM Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> > > >
> > > > A dedicated wakeup irq will be used to handle runtime suspend/resume,
> > > > we use dev_pm_set_dedicated_wake_irq API to take care of requesting
> > > > and attaching wakeup irq, then the suspend/resume framework will help
> > > > to enable/disable wakeup irq.
> > > >
> > > > The runtime PM is default off since some platforms may not support it.
> > > > users can enable it via power/control (set "auto") in sysfs.
> > > >
> > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > > ---
> > > >  drivers/usb/host/xhci-mtk.c | 140 +++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 124 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> > > > index a74764ab914a..30927f4064d4 100644
> > > > --- a/drivers/usb/host/xhci-mtk.c
> > > > +++ b/drivers/usb/host/xhci-mtk.c
[...]
> > > >
> > > > +static int check_rhub_status(struct xhci_hcd *xhci, struct xhci_hub *rhub)
> > > > +{
> > > > +       u32 suspended_ports;
> > > > +       u32 status;
> > > > +       int num_ports;
> > > > +       int i;
> > > > +
> > > > +       num_ports = rhub->num_ports;
> > > > +       suspended_ports = rhub->bus_state.suspended_ports;
> > > > +       for (i = 0; i < num_ports; i++) {
> > > > +               if (!(suspended_ports & BIT(i))) {
> > > > +                       status = readl(rhub->ports[i]->addr);
> > > > +                       if (status & PORT_CONNECT)
> > >
> > > So this pm_runtime support is activated only when there's no devices
> > > connected at all?
> > No, if the connected devices also support runtime suspend, it will enter
> > suspend mode when no data transfer, then the controller can enter
> > suspend too
> > > I think this will always return -EBUSY with my board having an on-board hub
> > > connected to both rhubs.
> > the on-board hub supports runtime suspend by default, so if no devices
> > connected, it will enter suspend
> 
> Sorry, you're correct. I was confused that the condition was
> (suspended && connect)
> My on-board hub connected to rhub is always in a suspended state
> whenever it's called.
> 
> However, I don't think this could return -EBUSY
> rpm_suspend() only be called when all the descendants are in sleep already.
You mean we can drop the bus check? 
If PM already takes care of children count, I think no need check it
anymore.
> Did you see any cases of this function returning -EBUSY or any concerns on here?
No, I didn't see it before.

Thanks

> 
> 
> >
> > >
> > > > +                               return -EBUSY;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * check the bus whether it could suspend or not
> > > > + * the bus will suspend if the downstream ports are already suspended,
> > > > + * or no devices connected.
> > > > + */
> > > > +static int check_bus_status(struct xhci_hcd *xhci)
> > > > +{
> > > > +       int ret;
> > > > +
> > > > +       ret = check_rhub_status(xhci, &xhci->usb3_rhub);
> > > > +       if (ret)
> > > > +               return ret;
> > > > +
> > > > +       return check_rhub_status(xhci, &xhci->usb2_rhub);
> > > > +}
> > > > +
> > > > +static int __maybe_unused xhci_mtk_runtime_suspend(struct device *dev)
> > > > +{
> > > > +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> > > > +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> > > > +       int ret = 0;
> > > > +
> > > > +       if (xhci->xhc_state)
> > > > +               return -ESHUTDOWN;
> > > > +
> > > > +       if (device_may_wakeup(dev)) {
> > > > +               ret = check_bus_status(xhci);
> > > > +               if (!ret)
> > > > +                       ret = xhci_mtk_suspend(dev);
> > > > +       }
> > > > +
> > > > +       /* -EBUSY: let PM automatically reschedule another autosuspend */
> > > > +       return ret ? -EBUSY : 0;
> > > > +}
> > > > +
> > > > +static int __maybe_unused xhci_mtk_runtime_resume(struct device *dev)
> > > > +{
> > > > +       struct xhci_hcd_mtk  *mtk = dev_get_drvdata(dev);
> > > > +       struct xhci_hcd *xhci = hcd_to_xhci(mtk->hcd);
> > > > +       int ret = 0;
> > > > +
> > > > +       if (xhci->xhc_state)
> > > > +               return -ESHUTDOWN;
> > > > +
> > > > +       if (device_may_wakeup(dev))
> > > > +               ret = xhci_mtk_resume(dev);
> > > > +
> > > > +       return ret;
> > > > +}
> > > > +
> > > >  static const struct dev_pm_ops xhci_mtk_pm_ops = {
> > > >         SET_SYSTEM_SLEEP_PM_OPS(xhci_mtk_suspend, xhci_mtk_resume)
> > > > +       SET_RUNTIME_PM_OPS(xhci_mtk_runtime_suspend,
> > > > +                          xhci_mtk_runtime_resume, NULL)
> > > >  };
> > > > -#define DEV_PM_OPS IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL
> > > > +
> > > > +#define DEV_PM_OPS (IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL)
> > > >
> > > >  static const struct of_device_id mtk_xhci_of_match[] = {
> > > >         { .compatible = "mediatek,mt8173-xhci"},
> > > > --
> > > > 2.18.0
> > > >
> >