RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible string

[Jun Li <jun.li@nxp.com>]

Hi,
> -----Original Message-----
> From: Felipe Balbi <balbi@kernel.org>
> Sent: Tuesday, May 11, 2021 5:27 PM
> To: Jun Li <jun.li@nxp.com>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>
> Cc: gregkh@linuxfoundation.org; shawnguo@kernel.org; dl-linux-imx
> <linux-imx@nxp.com>; thunder.leizhen@huawei.com;
> linux-usb@vger.kernel.org
> Subject: RE: [PATCH] usb: dwc3: imx8mp: detect dwc3 core node via compatible
> string
> 
> 
> Hi,
> 
> Jun Li <jun.li@nxp.com> writes:
> >> > > > Good suggestion, but if extcon notifier listener can't work for
> >> > > > me, my understanding is this *teach* in glue layer driver still
> >> > > > need access
> >> > > > dwc3 core instance struct, right?
> >> > >
> >> > > for now, maybe. But it may be better to implement a notifier
> >> > > method in role switch class.
> >> >
> >> > I am not sure if introduce notifier in role switch class is a good
> >> > idea, I had the impression extcon is not encouraged to use if possible.
> >>
> >> I'm not against role switch notifiers. They were proposed before
> >> already couple of years ago, but at that time there just were no
> >> users them notifier, so the patch was just dropped from the series [1].
> >> But I don't think anybody was against the idea. Please feel free to
> >> add them to the class if you have use for them.
> >
> > So I had the incorrect impression.
> >
> > Yes, that's the Felipe was referring to, I think.
> >
> > I will pick up [1] and improve my driver as Felipe suggested.
> 
> sounds great, thahnks Li Jun

Got chance to check this, but it turns out this glue driver
still has to be care of the dwc3 core and its probe completion,
because the role switch class is created by dwc3 core in its
probe.

dev_pm_set_dedicated_wake_irq() is a good solution for case
a sperate/glue driver is not required, in my imx8mp case, I
need a glue driver anyway, so this seems can't make my driver
much simpler. Rough change please see below:  

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 9f7c7f587d38..bcb63bcf27be 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -874,7 +874,6 @@ usb3_0: usb@32f10100 {
 			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
 				 <&clk IMX8MP_CLK_USB_ROOT>;
 			clock-names = "hsio", "suspend";
-			interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			dma-ranges = <0x40000000 0x40000000 0xc0000000>;
@@ -891,7 +890,9 @@ usb_dwc3_0: usb@38100000 {
 				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
 				assigned-clock-rates = <500000000>;
-				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
+							<GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "irq", "wakeup";
 				phys = <&usb3_phy0>, <&usb3_phy0>;
 				phy-names = "usb2-phy", "usb3-phy";
 				snps,dis-u2-freeclk-exists-quirk;
@@ -915,7 +916,6 @@ usb3_1: usb@32f10108 {
 			clocks = <&clk IMX8MP_CLK_HSIO_ROOT>,
 				 <&clk IMX8MP_CLK_USB_ROOT>;
 			clock-names = "hsio", "suspend";
-			interrupts = <GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
 			#address-cells = <1>;
 			#size-cells = <1>;
 			dma-ranges = <0x40000000 0x40000000 0xc0000000>;
@@ -932,7 +932,9 @@ usb_dwc3_1: usb@38200000 {
 				assigned-clocks = <&clk IMX8MP_CLK_HSIO_AXI>;
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_500M>;
 				assigned-clock-rates = <500000000>;
-				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>;
+				interrupts = <GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
+							<GIC_SPI 149 IRQ_TYPE_LEVEL_HIGH>;
+				interrupt-names = "irq", "wakeup";
 				phys = <&usb3_phy1>, <&usb3_phy1>;
 				phy-names = "usb2-phy", "usb3-phy";
 				snps,dis-u2-freeclk-exists-quirk;
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b6871ee5e8ca..4dac7cd98a31 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1615,6 +1615,12 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	dwc->wakeup_irq = platform_get_irq_byname_optional(pdev, "wakeup");
+	if (dwc->wakeup_irq == -EPROBE_DEFER)
+		goto assert_reset;
+	else
+		dev_err(dwc->dev, "the wakeup irq No. is %d\n", dwc->wakeup_irq);
+
 	ret = clk_bulk_prepare_enable(dwc->num_clks, dwc->clks);
 	if (ret)
 		goto assert_reset;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1c85f40a8dd8..264ae7def903 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -32,6 +32,7 @@
 #include <linux/phy/phy.h>
 
 #include <linux/power_supply.h>
+#include <linux/pm_wakeirq.h>
 
 #define DWC3_MSG_MAX	500
 
@@ -1295,6 +1296,7 @@ struct dwc3 {
 	int			max_cfg_eps;
 	int			last_fifo_depth;
 	int			num_ep_resized;
+	int			wakeup_irq;
 };
 
 #define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/dwc3-imx8mp.c b/drivers/usb/dwc3/dwc3-imx8mp.c
index 756faa46d33a..76f83fe64e4b 100644
--- a/drivers/usb/dwc3/dwc3-imx8mp.c
+++ b/drivers/usb/dwc3/dwc3-imx8mp.c
@@ -38,29 +38,24 @@
 
 struct dwc3_imx8mp {
 	struct device			*dev;
-	struct platform_device		*dwc3;
 	void __iomem			*glue_base;
 	struct clk			*hsio_clk;
 	struct clk			*suspend_clk;
-	int				irq;
+	enum usb_role			role;
+	struct notifier_block		notifier;
 	bool				pm_suspended;
-	bool				wakeup_pending;
 };
 
 static void dwc3_imx8mp_wakeup_enable(struct dwc3_imx8mp *dwc3_imx)
 {
-	struct dwc3	*dwc3 = platform_get_drvdata(dwc3_imx->dwc3);
-	u32		val;
-
-	if (!dwc3)
-		return;
+	u32 val;
 
 	val = readl(dwc3_imx->glue_base + USB_WAKEUP_CTRL);
 
-	if ((dwc3->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc3->xhci)
+	if (dwc3_imx->role == USB_ROLE_HOST)
 		val |= USB_WAKEUP_EN | USB_WAKEUP_SS_CONN |
 		       USB_WAKEUP_U3_EN | USB_WAKEUP_DPDM_EN;
-	else if (dwc3->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
+	else if (dwc3_imx->role == USB_ROLE_DEVICE)
 		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
 		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
 
@@ -76,23 +71,16 @@ static void dwc3_imx8mp_wakeup_disable(struct dwc3_imx8mp *dwc3_imx)
 	writel(val, dwc3_imx->glue_base + USB_WAKEUP_CTRL);
 }
 
-static irqreturn_t dwc3_imx8mp_interrupt(int irq, void *_dwc3_imx)
+static int dwc3_imx8mp_role_event(struct notifier_block *nb,
+				  unsigned long event, void *ptr)
 {
-	struct dwc3_imx8mp	*dwc3_imx = _dwc3_imx;
-	struct dwc3		*dwc = platform_get_drvdata(dwc3_imx->dwc3);
+	struct dwc3_imx8mp *dwc3_imx;
 
-	if (!dwc3_imx->pm_suspended)
-		return IRQ_HANDLED;
-
-	disable_irq_nosync(dwc3_imx->irq);
-	dwc3_imx->wakeup_pending = true;
+	dwc3_imx = container_of(nb, struct dwc3_imx8mp, notifier);
 
-	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci)
-		pm_runtime_resume(&dwc->xhci->dev);
-	else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE)
-		pm_runtime_get(dwc->dev);
+	dwc3_imx->role = event;
 
-	return IRQ_HANDLED;
+	return NOTIFY_DONE;
 }
 
 static int dwc3_imx8mp_probe(struct platform_device *pdev)
@@ -100,7 +88,7 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 	struct device		*dev = &pdev->dev;
 	struct device_node	*dwc3_np, *node = dev->of_node;
 	struct dwc3_imx8mp	*dwc3_imx;
-	int			err, irq;
+	int			err;
 
 	if (!node) {
 		dev_err(dev, "device node not found\n");
@@ -145,20 +133,6 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 		goto disable_hsio_clk;
 	}
 
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		err = irq;
-		goto disable_clks;
-	}
-	dwc3_imx->irq = irq;
-
-	err = devm_request_threaded_irq(dev, irq, NULL, dwc3_imx8mp_interrupt,
-					IRQF_ONESHOT, dev_name(dev), dwc3_imx);
-	if (err) {
-		dev_err(dev, "failed to request IRQ #%d --> %d\n", irq, err);
-		goto disable_clks;
-	}
-
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	err = pm_runtime_get_sync(dev);
@@ -177,13 +151,6 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to create dwc3 core\n");
 		goto err_node_put;
 	}
-
-	dwc3_imx->dwc3 = of_find_device_by_node(dwc3_np);
-	if (!dwc3_imx->dwc3) {
-		dev_err(dev, "failed to get dwc3 platform device\n");
-		err = -ENODEV;
-		goto depopulate;
-	}
 	of_node_put(dwc3_np);
 
 	device_set_wakeup_capable(dev, true);
@@ -191,14 +158,11 @@ static int dwc3_imx8mp_probe(struct platform_device *pdev)
 
 	return 0;
 
-depopulate:
-	of_platform_depopulate(dev);
 err_node_put:
 	of_node_put(dwc3_np);
 disable_rpm:
 	pm_runtime_disable(dev);
 	pm_runtime_put_noidle(dev);
-disable_clks:
 	clk_disable_unprepare(dwc3_imx->suspend_clk);
 disable_hsio_clk:
 	clk_disable_unprepare(dwc3_imx->hsio_clk);
@@ -224,12 +188,54 @@ static int dwc3_imx8mp_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int dwc3_imx8mp_register_notifier(struct dwc3_imx8mp *dwc3_imx)
+{
+	struct device_node	*dwc3_np, *node = dwc3_imx->dev->of_node;
+	struct platform_device	*dwc3_pdev;
+	struct dwc3		*dwc3;
+	int			err;
+
+	dwc3_np = of_get_compatible_child(node, "snps,dwc3");
+	if (!dwc3_np) {
+		err = -ENODEV;
+		dev_err(dwc3_imx->dev, "failed to find dwc3 core child\n");
+		return err;
+	}
+
+	dwc3_pdev = of_find_device_by_node(dwc3_np);
+	if (!dwc3_pdev) {
+		dev_err(dwc3_imx->dev, "failed to get dwc3 platform device\n");
+		err = -ENODEV;
+		return err;
+	}
+	of_node_put(dwc3_np);
+
+	dwc3 = platform_get_drvdata(dwc3_pdev);
+	if (!dwc3)
+		return 0;
+
+	if (!IS_ERR_OR_NULL(dwc3->role_sw)) {
+		dwc3_imx->notifier.notifier_call = dwc3_imx8mp_role_event;
+		err = usb_role_switch_register_notifier(dwc3->role_sw,
+							&dwc3_imx->notifier);
+		if (err < 0) {
+			dev_err(dwc3_imx->dev, "failed to register notifier\n");
+			return err;
+		}
+	}
+
+	return 0;
+}
+
 static int __maybe_unused dwc3_imx8mp_suspend(struct dwc3_imx8mp *dwc3_imx,
 					      pm_message_t msg)
 {
 	if (dwc3_imx->pm_suspended)
 		return 0;
 
+	if (!dwc3_imx->notifier.notifier_call)
+		dwc3_imx8mp_register_notifier(dwc3_imx);
+
 	/* Wakeup enable */
 	if (PMSG_IS_AUTO(msg) || device_may_wakeup(dwc3_imx->dev))
 		dwc3_imx8mp_wakeup_enable(dwc3_imx);
@@ -242,31 +248,18 @@ static int __maybe_unused dwc3_imx8mp_suspend(struct dwc3_imx8mp *dwc3_imx,
 static int __maybe_unused dwc3_imx8mp_resume(struct dwc3_imx8mp *dwc3_imx,
 					     pm_message_t msg)
 {
-	struct dwc3	*dwc = platform_get_drvdata(dwc3_imx->dwc3);
 	int ret = 0;
 
 	if (!dwc3_imx->pm_suspended)
 		return 0;
 
+	if (!dwc3_imx->notifier.notifier_call)
+		dwc3_imx8mp_register_notifier(dwc3_imx);
+
 	/* Wakeup disable */
 	dwc3_imx8mp_wakeup_disable(dwc3_imx);
 	dwc3_imx->pm_suspended = false;
 
-	if (dwc3_imx->wakeup_pending) {
-		dwc3_imx->wakeup_pending = false;
-		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
-			pm_runtime_mark_last_busy(dwc->dev);
-			pm_runtime_put_autosuspend(dwc->dev);
-		} else {
-			/*
-			 * Add wait for xhci switch from suspend
-			 * clock to normal clock to detect connection.
-			 */
-			usleep_range(9000, 10000);
-		}
-		enable_irq(dwc3_imx->irq);
-	}
-
 	return ret;
 }
 
@@ -277,9 +270,7 @@ static int __maybe_unused dwc3_imx8mp_pm_suspend(struct device *dev)
 
 	ret = dwc3_imx8mp_suspend(dwc3_imx, PMSG_SUSPEND);
 
-	if (device_may_wakeup(dwc3_imx->dev))
-		enable_irq_wake(dwc3_imx->irq);
-	else
+	if (!device_may_wakeup(dwc3_imx->dev))
 		clk_disable_unprepare(dwc3_imx->suspend_clk);
 
 	clk_disable_unprepare(dwc3_imx->hsio_clk);
@@ -293,9 +284,7 @@ static int __maybe_unused dwc3_imx8mp_pm_resume(struct device *dev)
 	struct dwc3_imx8mp *dwc3_imx = dev_get_drvdata(dev);
 	int ret;
 
-	if (device_may_wakeup(dwc3_imx->dev)) {
-		disable_irq_wake(dwc3_imx->irq);
-	} else {
+	if (!device_may_wakeup(dwc3_imx->dev)) {
 		ret = clk_prepare_enable(dwc3_imx->suspend_clk);
 		if (ret)
 			return ret;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e56f1a6db2de..4ac9d262e608 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2666,6 +2666,16 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 	dwc->gadget_driver	= driver;
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
+	if (dwc->wakeup_irq > 0) {
+		ret = dev_pm_set_dedicated_wake_irq(dwc->dev,
+						    dwc->wakeup_irq);
+		if (ret) {
+			dev_err(dwc->dev, "set wakeup irq %d failed\n",
+				dwc->wakeup_irq);
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -2681,6 +2691,7 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 	struct dwc3		*dwc = gadget_to_dwc(g);
 	unsigned long		flags;
 
+	dev_pm_clear_wake_irq(dwc->dev);
 	spin_lock_irqsave(&dwc->lock, flags);
 	dwc->gadget_driver	= NULL;
 	dwc->max_cfg_eps = 0;
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index f29a264635aa..3df0523906e5 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -121,6 +121,17 @@ int dwc3_host_init(struct dwc3 *dwc)
 		goto err;
 	}
 
+	if (dwc->wakeup_irq > 0) {
+		ret = dev_pm_set_dedicated_wake_irq(&xhci->dev,
+						    dwc->wakeup_irq);
+		if (ret) {
+			dev_err(dwc->dev, "set wakeup irq %d failed\n",
+				dwc->wakeup_irq);
+			goto err;
+		}
+		dev_err(&xhci->dev, "wakeup irq of xhci set OK.\n");
+	}
+
 	return 0;
 err:
 	platform_device_put(xhci);
@@ -129,5 +140,6 @@ int dwc3_host_init(struct dwc3 *dwc)
 
 void dwc3_host_exit(struct dwc3 *dwc)
 {
+	dev_pm_clear_wake_irq(&dwc->xhci->dev);
 	platform_device_unregister(dwc->xhci);
 }
-- 
2.25.1


Thanks
Li Jun
> 
> --
> balbi