Message-ID: <syv3mma7nsll7lioso4dhkhrk2udvleaaef5mmuws4fjyjfa2v@awjqx6uqt6lj>
Date: Wed, 4 Feb 2026 14:30:27 +0800
From: Xu Yang <xu.yang_2@....com>
To: Peng Fan <peng.fan@....nxp.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Shawn Guo <shawnguo@...nel.org>, 
	Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team <kernel@...gutronix.de>, 
	Fabio Estevam <festevam@...il.com>, Frank Li <frank.li@....com>, Li Jun <jun.li@....com>, 
	Thinh Nguyen <Thinh.Nguyen@...opsys.com>, linux-usb@...r.kernel.org, devicetree@...r.kernel.org, 
	imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] usb: dwc3: introduce flatten model driver of i.MX Soc

On Tue, Feb 03, 2026 at 09:38:54AM +0800, Peng Fan wrote:
> On Mon, Feb 02, 2026 at 06:27:47PM +0800, Xu Yang wrote:
> >To support flatten dwc3 devicetree model, introduce a new driver.
> >
> >Signed-off-by: Xu Yang <xu.yang_2@....com>
> >---
> > drivers/usb/dwc3/Kconfig    |  12 ++
> > drivers/usb/dwc3/Makefile   |   1 +
> > drivers/usb/dwc3/dwc3-imx.c | 428 ++++++++++++++++++++++++++++++++++++++++++++
> ...
> >diff --git a/drivers/usb/dwc3/dwc3-imx.c b/drivers/usb/dwc3/dwc3-imx.c
> >new file mode 100644
> >index 0000000000000000000000000000000000000000..57b175e929d7e163df5af7e2265ab7117fa1dc9a
> >--- /dev/null
> >+++ b/drivers/usb/dwc3/dwc3-imx.c
> >@@ -0,0 +1,428 @@
> >+// SPDX-License-Identifier: GPL-2.0
> >+/*
> >+ * dwc3-imx.c - NXP i.MX Soc USB3 Specific Glue layer
> >+ *
> >+ * Copyright 2026 NXP
> >+ */
> >+
> >+#include <linux/clk.h>
> >+#include <linux/interrupt.h>
> >+#include <linux/io.h>
> >+#include <linux/kernel.h>
> >+#include <linux/module.h>
> >+#include <linux/of_platform.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/pm_runtime.h>
> >+
> >+#include "core.h"
> >+#include "glue.h"
> >+
> >+/* USB wakeup registers */
> >+#define USB_WAKEUP_CTRL			0x00
> >+
> >+/* Global wakeup interrupt enable, also used to clear interrupt */
> >+#define USB_WAKEUP_EN			BIT(31)
> >+/* Wakeup from connect or disconnect, only for superspeed */
> >+#define USB_WAKEUP_SS_CONN		BIT(5)
> >+/* 0 select vbus_valid, 1 select sessvld */
> >+#define USB_WAKEUP_VBUS_SRC_SESS_VAL	BIT(4)
> >+/* Enable signal for wake up from u3 state */
> >+#define USB_WAKEUP_U3_EN		BIT(3)
> >+/* Enable signal for wake up from id change */
> >+#define USB_WAKEUP_ID_EN		BIT(2)
> >+/* Enable signal for wake up from vbus change */
> >+#define	USB_WAKEUP_VBUS_EN		BIT(1)
> >+/* Enable signal for wake up from dp/dm change */
> >+#define USB_WAKEUP_DPDM_EN		BIT(0)
> >+
> >+#define USB_WAKEUP_EN_MASK		GENMASK(5, 0)
> >+
> >+/* USB glue registers */
> >+#define USB_CTRL0		0x00
> >+#define USB_CTRL1		0x04
> >+
> >+#define USB_CTRL0_PORTPWR_EN	BIT(12) /* 1 - PPC enabled (default) */
> >+#define USB_CTRL0_USB3_FIXED	BIT(22) /* 1 - USB3 permanent attached */
> >+#define USB_CTRL0_USB2_FIXED	BIT(23) /* 1 - USB2 permanent attached */
> >+
> >+#define USB_CTRL1_OC_POLARITY	BIT(16) /* 0 - HIGH / 1 - LOW */
> >+#define USB_CTRL1_PWR_POLARITY	BIT(17) /* 0 - HIGH / 1 - LOW */
> >+
> >+struct dwc3_imx {
> >+	struct dwc3	dwc;
> >+	struct device	*dev;
> >+	void __iomem	*blkctl_base;
> >+	void __iomem	*glue_base;
> >+	struct clk	*hsio_clk;
> >+	struct clk	*suspend_clk;
> >+	int		irq;
> >+	bool		pm_suspended;
> >+	bool		wakeup_pending;
> >+};
> >+
> >+#define to_dwc3_imx(d) container_of((d), struct dwc3_imx, dwc)
> >+
> >+static void dwc3_imx_configure_glue(struct dwc3_imx *dwc_imx)
> >+{
> >+	struct device *dev = dwc_imx->dev;
> >+	u32 value;
> >+
> >+	if (!dwc_imx->glue_base)
> >+		return;
> >+
> >+	value = readl(dwc_imx->glue_base + USB_CTRL0);
> >+
> >+	if (device_property_read_bool(dev, "fsl,permanently-attached"))
> >+		value |= (USB_CTRL0_USB2_FIXED | USB_CTRL0_USB3_FIXED);
> 
> No need parentheses.

OK.

> 
> >+	else
> >+		value &= ~(USB_CTRL0_USB2_FIXED | USB_CTRL0_USB3_FIXED);
> >+
> >+	if (device_property_read_bool(dev, "fsl,disable-port-power-control"))
> >+		value &= ~(USB_CTRL0_PORTPWR_EN);
> 
> Ditto.

OK.

> 
> >+	else
> >+		value |= USB_CTRL0_PORTPWR_EN;
> >+
> >+	writel(value, dwc_imx->glue_base + USB_CTRL0);
> >+
> >+	value = readl(dwc_imx->glue_base + USB_CTRL1);
> >+	if (device_property_read_bool(dev, "fsl,over-current-active-low"))
> >+		value |= USB_CTRL1_OC_POLARITY;
> >+	else
> >+		value &= ~USB_CTRL1_OC_POLARITY;
> >+
> >+	if (device_property_read_bool(dev, "fsl,power-active-low"))
> >+		value |= USB_CTRL1_PWR_POLARITY;
> >+	else
> >+		value &= ~USB_CTRL1_PWR_POLARITY;
> >+
> >+	writel(value, dwc_imx->glue_base + USB_CTRL1);
> >+}
> >+
> >+static void dwc3_imx_wakeup_enable(struct dwc3_imx *dwc_imx, pm_message_t msg)
> >+{
> >+	struct dwc3	*dwc = &dwc_imx->dwc;
> >+	u32		val;
> 
> In dwc3_imx_configure_glue, space is used. while here, tab is used.

OK.

> 
> >+
> >+	val = readl(dwc_imx->blkctl_base + USB_WAKEUP_CTRL);
> >+
> >+	if ((dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST) && dwc->xhci) {
> >+		val |= USB_WAKEUP_EN | USB_WAKEUP_DPDM_EN;
> >+		if (PMSG_IS_AUTO(msg))
> >+			val |= USB_WAKEUP_SS_CONN | USB_WAKEUP_U3_EN;
> >+	} else {
> >+		val |= USB_WAKEUP_EN | USB_WAKEUP_VBUS_EN |
> >+		       USB_WAKEUP_VBUS_SRC_SESS_VAL;
> >+	}
> >+
> >+	writel(val, dwc_imx->blkctl_base + USB_WAKEUP_CTRL);
> >+}
> >+
> >+static void dwc3_imx_wakeup_disable(struct dwc3_imx *dwc_imx)
> >+{
> >+	u32	val;
> 
> Ditto.
> 
> >+
> >+	val = readl(dwc_imx->blkctl_base + USB_WAKEUP_CTRL);
> >+	val &= ~(USB_WAKEUP_EN | USB_WAKEUP_EN_MASK);
> >+	writel(val, dwc_imx->blkctl_base + USB_WAKEUP_CTRL);
> >+}
> >+
> >+static irqreturn_t dwc3_imx_interrupt(int irq, void *data)
> >+{
> >+	struct dwc3_imx		*dwc_imx = data;
> >+	struct dwc3		*dwc = &dwc_imx->dwc;
> 
> Ditto.
> 
> >+
> >+	if (!dwc_imx->pm_suspended)
> >+		return IRQ_HANDLED;
> >+
> >+	disable_irq_nosync(dwc_imx->irq);
> >+	dwc_imx->wakeup_pending = true;
> >+
> >+	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);
> >+
> >+	return IRQ_HANDLED;
> >+}
> >+
> >+static void dwc3_imx_pre_set_role(struct dwc3 *dwc, enum usb_role role)
> >+{
> >+	if (role == USB_ROLE_HOST)
> >+		/*
> >+		 * For xhci host, we need disable dwc core auto
> >+		 * suspend, because during this auto suspend delay(5s),
> >+		 * xhci host RUN_STOP is cleared and wakeup is not
> >+		 * enabled, if device is inserted, xhci host can't
> >+		 * response the connection.
> >+		 */
> >+		pm_runtime_dont_use_autosuspend(dwc->dev);
> >+	else
> >+		pm_runtime_use_autosuspend(dwc->dev);
> >+}
> >+
> >+static struct dwc3_glue_ops dwc3_imx_glue_ops = {
> >+	.pre_set_role   = dwc3_imx_pre_set_role,
> >+};
> >+
> >+static const struct property_entry dwc3_imx_properties[] = {
> >+	PROPERTY_ENTRY_BOOL("xhci-missing-cas-quirk"),
> >+	PROPERTY_ENTRY_BOOL("xhci-skip-phy-init-quirk"),
> >+	{},
> >+};
> >+
> >+static const struct software_node dwc3_imx_swnode = {
> >+	.properties = dwc3_imx_properties,
> >+};
> >+
> >+static int dwc3_imx_probe(struct platform_device *pdev)
> >+{
> >+	struct device		*dev = &pdev->dev;
> >+	struct dwc3_imx		*dwc_imx;
> >+	struct dwc3		*dwc;
> >+	struct resource		*res;
> >+	const char		*irq_name;
> >+	struct dwc3_probe_data	probe_data = {};
> >+	int			ret, irq;
> 
> As written above, unify the format.
> 
> >+
> >+	dwc_imx = devm_kzalloc(dev, sizeof(*dwc_imx), GFP_KERNEL);
> >+	if (!dwc_imx)
> >+		return -ENOMEM;
> >+
> >+	platform_set_drvdata(pdev, dwc_imx);
> >+	dwc_imx->dev = dev;
> >+
> >+	dwc_imx->blkctl_base = devm_platform_ioremap_resource_byname(pdev, "blkctl");
> >+	if (IS_ERR(dwc_imx->blkctl_base))
> >+		return PTR_ERR(dwc_imx->blkctl_base);
> >+
> >+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "glue");
> >+	if (!res) {
> >+		dev_warn(dev, "Base address for glue layer missing\n");
> >+	} else {
> >+		dwc_imx->glue_base = devm_ioremap_resource(dev, res);
> >+		if (IS_ERR(dwc_imx->glue_base))
> >+			return PTR_ERR(dwc_imx->glue_base);
> >+	}
> >+
> >+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
> >+	if (!res)
> >+		return dev_err_probe(dev, -ENODEV, "missing core memory resource\n");
> 
> -ENOENT should be better.

As Thinh's comment, will keep ENODEV.

> 
> >+
> >+	dwc_imx->hsio_clk = devm_clk_get_enabled(dev, "hsio");
> >+	if (IS_ERR(dwc_imx->hsio_clk))
> >+		return dev_err_probe(dev, PTR_ERR(dwc_imx->hsio_clk),
> >+				     "Failed to get hsio clk\n");
> >+
> >+	dwc_imx->suspend_clk = devm_clk_get_enabled(dev, "suspend");
> >+	if (IS_ERR(dwc_imx->suspend_clk))
> >+		return dev_err_probe(dev, PTR_ERR(dwc_imx->suspend_clk),
> >+				     "Failed to get suspend clk\n");
> >+
> >+	irq = platform_get_irq_byname(pdev, "wakeup");
> >+	if (irq < 0)
> >+		return irq;
> >+	dwc_imx->irq = irq;
> >+
> >+	irq_name = devm_kasprintf(dev, GFP_KERNEL, "%s:wakeup", dev_name(dev));
> >+	if (!irq_name)
> >+		return dev_err_probe(dev, -ENOMEM, "failed to create irq_name\n");
> >+
> >+	ret = devm_request_threaded_irq(dev, irq, NULL, dwc3_imx_interrupt,
> >+					IRQF_ONESHOT, irq_name, dwc_imx);
> >+	if (ret)
> >+		return dev_err_probe(dev, ret, "failed to request IRQ #%d\n", irq);
> 
> Should this be moved to end after all are initialized?

Thanks for the suggestion.

I prefer to setup all glue layer setting before initialize core parts. So
the logic will be a bit clear. To avoid accident, I will disable wakeup
irq by default and enable it during system suspend. So IRQF_NO_AUTOEN
will be added in v2.

> 
> >+
> >+	ret = device_add_software_node(dev, &dwc3_imx_swnode);
> >+	if (ret)
> >+		return dev_err_probe(dev, ret, "failed to add software node\n");
> >+
> >+	dwc = &dwc_imx->dwc;
> >+	dwc->dev = dev;
> >+	dwc->glue_ops = &dwc3_imx_glue_ops;
> >+
> >+	probe_data.res = res;
> >+	probe_data.dwc = dwc;
> >+	probe_data.core_may_lose_power = true;
> >+
> >+	ret = dwc3_core_probe(&probe_data);
> >+	if (ret) {
> >+		device_remove_software_node(dev);
> >+		return ret;
> >+	}
> >+
> >+	device_set_wakeup_capable(dev, true);
> >+	return 0;
> >+}
> >+
> >+static void dwc3_imx_remove(struct platform_device *pdev)
> >+{
> >+	struct device	*dev = &pdev->dev;
> >+	struct dwc3	*dwc = dev_get_drvdata(dev);
> >+
> >+	if (pm_runtime_resume_and_get(dev) < 0)
> 
> print a error message.
> 
> when this fail return, there is no chance to probe success again I think,
> because resource not freed.

I just double check the driver logic here, dwc3_core_remove() will do all
above pm_runtime_* things. So these runtime PM operations are redundent
here. I will remove them in v2.

Thanks,
Xu Yang

Powered by blists - more mailing lists