Message-ID: <aYFRrguYW8Ps8Nth@shlinux89>
Date: Tue, 3 Feb 2026 09:38:54 +0800
From: Peng Fan <peng.fan@....nxp.com>
To: Xu Yang <xu.yang_2@....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 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.

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

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

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

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

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

>+		return;
>+
>+	dwc3_core_remove(dwc);
>+	device_remove_software_node(dev);
>+	pm_runtime_put_noidle(dev);
>+}
>+

Regards
Peng

Powered by blists - more mailing lists