Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
@ 2021-04-08  9:35 Chunfeng Yun
  2021-04-08  9:35 ` [PATCH 2/6] usb: xhci-mtk: check return value in suspend/resume hooks Chunfeng Yun
                   ` (6 more replies)
  0 siblings, 7 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-08  9:35 UTC (permalink / raw)
  To: Rob Herring, Mathias Nyman
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
	linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung, Ikjoon Jang,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/6] usb: xhci-mtk: check return value in suspend/resume hooks
  2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
@ 2021-04-08  9:35 ` Chunfeng Yun
  2021-04-08  9:35 ` [PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt Chunfeng Yun
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-08  9:35 UTC (permalink / raw)
  To: Rob Herring, Mathias Nyman
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
	linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung, Ikjoon Jang,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt
  2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
  2021-04-08  9:35 ` [PATCH 2/6] usb: xhci-mtk: check return value in suspend/resume hooks Chunfeng Yun
@ 2021-04-08  9:35 ` Chunfeng Yun
  2021-04-09 18:41   ` Rob Herring
  2021-04-08  9:35 ` [PATCH 4/6] usb: xhci-mtk: add support runtime PM Chunfeng Yun
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-08  9:35 UTC (permalink / raw)
  To: Rob Herring, Mathias Nyman
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
	linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung, Ikjoon Jang,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 4/6] usb: xhci-mtk: add support runtime PM
  2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
  2021-04-08  9:35 ` [PATCH 2/6] usb: xhci-mtk: check return value in suspend/resume hooks Chunfeng Yun
  2021-04-08  9:35 ` [PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt Chunfeng Yun
@ 2021-04-08  9:35 ` Chunfeng Yun
  2021-04-08  9:51   ` Chunfeng Yun
  2021-04-09  5:45   ` Ikjoon Jang
  2021-04-08  9:35 ` [PATCH 5/6] usb: xhci-mtk: use clock bulk to get clocks Chunfeng Yun
                   ` (3 subsequent siblings)
  6 siblings, 2 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-08  9:35 UTC (permalink / raw)
  To: Rob Herring, Mathias Nyman
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
	linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung, Ikjoon Jang,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 5/6] usb: xhci-mtk: use clock bulk to get clocks
  2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
                   ` (2 preceding siblings ...)
  2021-04-08  9:35 ` [PATCH 4/6] usb: xhci-mtk: add support runtime PM Chunfeng Yun
@ 2021-04-08  9:35 ` Chunfeng Yun
  2021-04-08  9:35 ` [PATCH 6/6] usb: xhci-mtk: remove unused members Chunfeng Yun
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-08  9:35 UTC (permalink / raw)
  To: Rob Herring, Mathias Nyman
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
	linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung, Ikjoon Jang,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 6/6] usb: xhci-mtk: remove unused members
  2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
                   ` (3 preceding siblings ...)
  2021-04-08  9:35 ` [PATCH 5/6] usb: xhci-mtk: use clock bulk to get clocks Chunfeng Yun
@ 2021-04-08  9:35 ` Chunfeng Yun
  2021-04-08 17:41 ` [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Rafael J. Wysocki
  2021-04-09  5:32 ` Ikjoon Jang
  6 siblings, 0 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-08  9:35 UTC (permalink / raw)
  To: Rob Herring, Mathias Nyman
  Cc: Chunfeng Yun, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
	linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung, Ikjoon Jang,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM
  2021-04-08  9:35 ` [PATCH 4/6] usb: xhci-mtk: add support runtime PM Chunfeng Yun
@ 2021-04-08  9:51   ` Chunfeng Yun
  2021-04-09  5:45   ` Ikjoon Jang
  1 sibling, 0 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-08  9:51 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	linux-arm-kernel, linux-mediatek, devicetree, linux-kernel,
	linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
                   ` (4 preceding siblings ...)
  2021-04-08  9:35 ` [PATCH 6/6] usb: xhci-mtk: remove unused members Chunfeng Yun
@ 2021-04-08 17:41 ` Rafael J. Wysocki
  2021-04-09  1:53   ` Chunfeng Yun
  2021-04-09  5:32 ` Ikjoon Jang
  6 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-04-08 17:41 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Linux ARM, moderated list:ARM/Mediatek SoC...,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Linux PM, Tony Lindgren,
	Tianping Fang, Eddie Hung, Ikjoon Jang, Nicolas Boichat

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
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-08 17:41 ` [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Rafael J. Wysocki
@ 2021-04-09  1:53   ` Chunfeng Yun
  2021-04-09  5:39     ` Tony Lindgren
  0 siblings, 1 reply; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-09  1:53 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Linux ARM, moderated list:ARM/Mediatek SoC...,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Linux PM, Tony Lindgren,
	Tianping Fang, Eddie Hung, Ikjoon Jang, Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
                   ` (5 preceding siblings ...)
  2021-04-08 17:41 ` [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Rafael J. Wysocki
@ 2021-04-09  5:32 ` Ikjoon Jang
  2021-04-09  5:40   ` Tony Lindgren
  2021-04-09  8:43   ` Chunfeng Yun
  6 siblings, 2 replies; 21+ messages in thread
From: Ikjoon Jang @ 2021-04-09  5:32 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung,
	Nicolas Boichat

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
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-09  1:53   ` Chunfeng Yun
@ 2021-04-09  5:39     ` Tony Lindgren
  2021-04-09  8:36       ` Chunfeng Yun
  0 siblings, 1 reply; 21+ messages in thread
From: Tony Lindgren @ 2021-04-09  5:39 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Rafael J. Wysocki, Rob Herring, Mathias Nyman,
	Greg Kroah-Hartman, Matthias Brugger, Rafael J. Wysocki,
	Len Brown, Pavel Machek,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Linux ARM, moderated list:ARM/Mediatek SoC...,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Linux PM, Tianping Fang, Eddie Hung,
	Ikjoon Jang, Nicolas Boichat

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-09  5:32 ` Ikjoon Jang
@ 2021-04-09  5:40   ` Tony Lindgren
  2021-04-09  8:43   ` Chunfeng Yun
  1 sibling, 0 replies; 21+ messages in thread
From: Tony Lindgren @ 2021-04-09  5:40 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Chunfeng Yun, Rob Herring, Mathias Nyman, Greg Kroah-Hartman,
	Matthias Brugger, Rafael J. Wysocki, Len Brown, Pavel Machek,
	linux-usb, moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, linux-pm, Tianping Fang, Eddie Hung, Nicolas Boichat

* 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM
  2021-04-08  9:35 ` [PATCH 4/6] usb: xhci-mtk: add support runtime PM Chunfeng Yun
  2021-04-08  9:51   ` Chunfeng Yun
@ 2021-04-09  5:45   ` Ikjoon Jang
  2021-04-09  8:54     ` Chunfeng Yun
  1 sibling, 1 reply; 21+ messages in thread
From: Ikjoon Jang @ 2021-04-09  5:45 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung,
	Nicolas Boichat

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
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-09  5:39     ` Tony Lindgren
@ 2021-04-09  8:36       ` Chunfeng Yun
  2021-04-09 11:14         ` Rafael J. Wysocki
  0 siblings, 1 reply; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-09  8:36 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Rafael J. Wysocki, Rob Herring, Mathias Nyman,
	Greg Kroah-Hartman, Matthias Brugger, Rafael J. Wysocki,
	Len Brown, Pavel Machek,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Linux ARM, moderated list:ARM/Mediatek SoC...,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Linux PM, Tianping Fang, Eddie Hung,
	Ikjoon Jang, Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-09  5:32 ` Ikjoon Jang
  2021-04-09  5:40   ` Tony Lindgren
@ 2021-04-09  8:43   ` Chunfeng Yun
  1 sibling, 0 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-09  8:43 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM
  2021-04-09  5:45   ` Ikjoon Jang
@ 2021-04-09  8:54     ` Chunfeng Yun
  2021-04-12  5:14       ` Ikjoon Jang
  0 siblings, 1 reply; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-09  8:54 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-09  8:36       ` Chunfeng Yun
@ 2021-04-09 11:14         ` Rafael J. Wysocki
  2021-04-10  1:44           ` Chunfeng Yun
  0 siblings, 1 reply; 21+ messages in thread
From: Rafael J. Wysocki @ 2021-04-09 11:14 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Tony Lindgren, Rafael J. Wysocki, Rob Herring, Mathias Nyman,
	Greg Kroah-Hartman, Matthias Brugger, Rafael J. Wysocki,
	Len Brown, Pavel Machek,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Linux ARM, moderated list:ARM/Mediatek SoC...,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Linux PM, Tianping Fang, Eddie Hung,
	Ikjoon Jang, Nicolas Boichat

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?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt
  2021-04-08  9:35 ` [PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt Chunfeng Yun
@ 2021-04-09 18:41   ` Rob Herring
  0 siblings, 0 replies; 21+ messages in thread
From: Rob Herring @ 2021-04-09 18:41 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Mathias Nyman, Rafael J. Wysocki, linux-mediatek,
	Greg Kroah-Hartman, linux-pm, linux-usb, Len Brown, Rob Herring,
	devicetree, Eddie Hung, Tianping Fang, Ikjoon Jang,
	Nicolas Boichat, Tony Lindgren, linux-kernel, Pavel Machek,
	Matthias Brugger, linux-arm-kernel

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>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called
  2021-04-09 11:14         ` Rafael J. Wysocki
@ 2021-04-10  1:44           ` Chunfeng Yun
  0 siblings, 0 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-10  1:44 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Tony Lindgren, Rob Herring, Mathias Nyman, Greg Kroah-Hartman,
	Matthias Brugger, Rafael J. Wysocki, Len Brown, Pavel Machek,
	open list:ULTRA-WIDEBAND (UWB) SUBSYSTEM:,
	Linux ARM, moderated list:ARM/Mediatek SoC...,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Linux Kernel Mailing List, Linux PM, Tianping Fang, Eddie Hung,
	Ikjoon Jang, Nicolas Boichat

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




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM
  2021-04-09  8:54     ` Chunfeng Yun
@ 2021-04-12  5:14       ` Ikjoon Jang
  2021-04-13  3:35         ` Chunfeng Yun
  0 siblings, 1 reply; 21+ messages in thread
From: Ikjoon Jang @ 2021-04-12  5:14 UTC (permalink / raw)
  To: Chunfeng Yun
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung,
	Nicolas Boichat

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/6] usb: xhci-mtk: add support runtime PM
  2021-04-12  5:14       ` Ikjoon Jang
@ 2021-04-13  3:35         ` Chunfeng Yun
  0 siblings, 0 replies; 21+ messages in thread
From: Chunfeng Yun @ 2021-04-13  3:35 UTC (permalink / raw)
  To: Ikjoon Jang
  Cc: Rob Herring, Mathias Nyman, Greg Kroah-Hartman, Matthias Brugger,
	Rafael J. Wysocki, Len Brown, Pavel Machek, linux-usb,
	moderated list:ARM/Mediatek SoC support,
	moderated list:ARM/Mediatek SoC support,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list, linux-pm, Tony Lindgren, Tianping Fang, Eddie Hung,
	Nicolas Boichat

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


^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-08  9:35 [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Chunfeng Yun
2021-04-08  9:35 ` [PATCH 2/6] usb: xhci-mtk: check return value in suspend/resume hooks Chunfeng Yun
2021-04-08  9:35 ` [PATCH 3/6] dt-bindings: usb: mtk-xhci: add wakeup interrupt Chunfeng Yun
2021-04-09 18:41   ` Rob Herring
2021-04-08  9:35 ` [PATCH 4/6] usb: xhci-mtk: add support runtime PM Chunfeng Yun
2021-04-08  9:51   ` Chunfeng Yun
2021-04-09  5:45   ` Ikjoon Jang
2021-04-09  8:54     ` Chunfeng Yun
2021-04-12  5:14       ` Ikjoon Jang
2021-04-13  3:35         ` Chunfeng Yun
2021-04-08  9:35 ` [PATCH 5/6] usb: xhci-mtk: use clock bulk to get clocks Chunfeng Yun
2021-04-08  9:35 ` [PATCH 6/6] usb: xhci-mtk: remove unused members Chunfeng Yun
2021-04-08 17:41 ` [PATCH 1/6] PM: runtime: enable wake irq after runtime_suspend hook called Rafael J. Wysocki
2021-04-09  1:53   ` Chunfeng Yun
2021-04-09  5:39     ` Tony Lindgren
2021-04-09  8:36       ` Chunfeng Yun
2021-04-09 11:14         ` Rafael J. Wysocki
2021-04-10  1:44           ` Chunfeng Yun
2021-04-09  5:32 ` Ikjoon Jang
2021-04-09  5:40   ` Tony Lindgren
2021-04-09  8:43   ` Chunfeng Yun

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git