[PATCH v6 0/2] Add SpacemiT K1 USB3.0 host controller support

[PATCH v6 0/2] Add SpacemiT K1 USB3.0 host controller support

[Ze Huang]

The USB 3.0 controller found in the SpacemiT K1 SoC[1] supports both USB3.0
Host and USB2.0 Dual-Role Device (DRD). 

This controller is compatible with DesignWare Core USB 3 (DWC3) driver.
However, constraints in the `snps,dwc3` bindings limit the ability to describe
hardware-specific features in a clean and maintainable way. While
`dwc3-of-simple` still serves as a glue layer for many platforms, it requires a
split device tree node structure, which is less desirable in newer platforms.

To promote a transition toward a flattened `dwc` node structure, this series
introduces `dwc3-generic-plat`, building upon prior efforts that exposed the
DWC3 core driver [2].

The device tree support for SpacemiT K1 will be submitted separately when the
associated PHY driver is ready.

This series is based on 6.16-rc1 and has been tested on BananaPi development
boards.

Link: https://developer.spacemit.com/documentation?token=AjHDwrW78igAAEkiHracBI9HnTb [1]
Link: https://lore.kernel.org/all/20250414-dwc3-refactor-v7-3-f015b358722d@oss.qualcomm.com [2]

Signed-off-by: Ze Huang <huang.ze@linux.dev>
---
Changes in v6:
- replace SET_RUNTIME_PM_OPS/SET_SYSTEM_SLEEP_PM_OPS with RUNTIME_PM_OPS/SYSTEM_SLEEP_PM_OPS
- Link to v5: https://lore.kernel.org/r/20250705-dwc3_generic-v5-0-9dbc53ea53d2@linux.dev

Changes in v5:
- drop DTS patch (will submit when PHY driver is ready)
- drop interconnects and update resets property in dt-bindings
- remove unnecessary __maybe_unused attribute and PM guards
- switch to devres APIs for reset and clock management
- Link to v4: https://lore.kernel.org/all/20250526-b4-k1-dwc3-v3-v4-0-63e4e525e5cb@whut.edu.cn/

Changes in v4:
- dt-bindings spacemit,k1-dwc:
  - reorder properties
  - add properties of phys & phy-names
  - add usb hub nodes in example dt
- add support for spacemit,k1-mbus
- dwc3 generic plat driver:
  - rename dwc3-common.c to dwc3-generic-plat.c
  - use SYSTEM_SLEEP_PM_OPS macros and drop PM guards
- dts:
  - reorder dts properties of usb dwc3 node
  - move "dr_mode" of dwc3 from dtsi to dts
- Link to v3: https://lore.kernel.org/r/20250518-b4-k1-dwc3-v3-v3-0-7609c8baa2a6@whut.edu.cn

Changes in v3:
- introduce dwc3-common for generic dwc3 hardware
- fix warnings in usb host dt-bindings
- fix errors in dts
- Link to v2: https://lore.kernel.org/r/20250428-b4-k1-dwc3-v2-v1-0-7cb061abd619@whut.edu.cn

Changes in v2:
- dt-bindings:
  - add missing 'maxItems'
  - remove 'status' property in exmaple
  - fold dwc3 node into parent
- drop dwc3 glue driver and use snps,dwc3 driver directly
- rename dts nodes and reorder properties to fit coding style
- Link to v1: https://lore.kernel.org/all/20250407-b4-k1-usb3-v3-2-v1-0-bf0bcc41c9ba@whut.edu.cn

---
Ze Huang (2):
      dt-bindings: usb: dwc3: add support for SpacemiT K1
      usb: dwc3: add generic driver to support flattened

 .../devicetree/bindings/usb/spacemit,k1-dwc3.yaml  | 107 ++++++++++++
 drivers/usb/dwc3/Kconfig                           |  11 ++
 drivers/usb/dwc3/Makefile                          |   1 +
 drivers/usb/dwc3/dwc3-generic-plat.c               | 182 +++++++++++++++++++++
 4 files changed, 301 insertions(+)
---
base-commit: d9946fe286439c2aeaa7953b8c316efe5b83d515
change-id: 20250705-dwc3_generic-8d02859722c3

Best regards,
-- 
Ze Huang <huang.ze@linux.dev>

[PATCH v6 1/2] dt-bindings: usb: dwc3: add support for SpacemiT K1

[Ze Huang]

Add support for the USB 3.0 Dual-Role Device (DRD) controller embedded
in the SpacemiT K1 SoC. The controller is based on the Synopsys
DesignWare Core USB 3 (DWC3) IP, supporting USB3.0 host mode and USB 2.0
DRD mode.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Ze Huang <huang.ze@linux.dev>
---
 .../devicetree/bindings/usb/spacemit,k1-dwc3.yaml  | 107 +++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml b/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..c967ad6aae50199127a4f8a17d53fc34e8d9480b
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml
@@ -0,0 +1,107 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/spacemit,k1-dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SpacemiT K1 SuperSpeed DWC3 USB SoC Controller
+
+maintainers:
+  - Ze Huang <huang.ze@linux.dev>
+
+description: |
+  The SpacemiT K1 embeds a DWC3 USB IP Core which supports Host functions
+  for USB 3.0 and DRD for USB 2.0.
+
+  Key features:
+  - USB3.0 SuperSpeed and USB2.0 High/Full/Low-Speed support
+  - Supports low-power modes (USB2.0 suspend, USB3.0 U1/U2/U3)
+  - Internal DMA controller and flexible endpoint FIFO sizing
+
+  Communication Interface:
+  - Use of PIPE3 (125MHz) interface for USB3.0 PHY
+  - Use of UTMI+ (30/60MHz) interface for USB2.0 PHY
+
+allOf:
+  - $ref: snps,dwc3-common.yaml#
+
+properties:
+  compatible:
+    const: spacemit,k1-dwc3
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: usbdrd30
+
+  interrupts:
+    maxItems: 1
+
+  phys:
+    items:
+      - description: phandle to USB2/HS PHY
+      - description: phandle to USB3/SS PHY
+
+  phy-names:
+    items:
+      - const: usb2-phy
+      - const: usb3-phy
+
+  resets:
+    items:
+      - description: USB3.0 AHB reset line
+      - description: USB3.0 VCC reset line
+      - description: USB3.0 PHY reset line
+
+  vbus-supply:
+    description: A phandle to the regulator supplying the VBUS voltage.
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - phys
+  - phy-names
+  - resets
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    usb@c0a00000 {
+        compatible = "spacemit,k1-dwc3";
+        reg = <0xc0a00000 0x10000>;
+        clocks = <&syscon_apmu 16>;
+        clock-names = "usbdrd30";
+        interrupts = <125>;
+        phys = <&usb2phy>, <&usb3phy>;
+        phy-names = "usb2-phy", "usb3-phy";
+        resets = <&syscon_apmu 8>,
+                 <&syscon_apmu 9>,
+                 <&syscon_apmu 10>;
+        vbus-supply = <&usb3_vbus>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        hub_2_0: hub@1 {
+            compatible = "usb2109,2817";
+            reg = <1>;
+            vdd-supply = <&usb3_vhub>;
+            peer-hub = <&hub_3_0>;
+            reset-gpios = <&gpio 3 28 1>;
+        };
+
+        hub_3_0: hub@2 {
+            compatible = "usb2109,817";
+            reg = <2>;
+            vdd-supply = <&usb3_vhub>;
+            peer-hub = <&hub_2_0>;
+            reset-gpios = <&gpio 3 28 1>;
+        };
+    };

-- 
2.50.1

[PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Ze Huang]

To support flattened dwc3 dt model and drop the glue layer, introduce the
`dwc3-generic` driver. This enables direct binding of the DWC3 core driver
and offers an alternative to the existing glue driver `dwc3-of-simple`.

Signed-off-by: Ze Huang <huang.ze@linux.dev>
---
 drivers/usb/dwc3/Kconfig             |  11 +++
 drivers/usb/dwc3/Makefile            |   1 +
 drivers/usb/dwc3/dwc3-generic-plat.c | 182 +++++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 310d182e10b50b253d7e5a51674806e6ec442a2a..4925d15084f816d3ff92059b476ebcc799b56b51 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -189,4 +189,15 @@ config USB_DWC3_RTK
 	  or dual-role mode.
 	  Say 'Y' or 'M' if you have such device.
 
+config USB_DWC3_GENERIC_PLAT
+	tristate "DWC3 Generic Platform Driver"
+	depends on OF && COMMON_CLK
+	default USB_DWC3
+	help
+	  Support USB3 functionality in simple SoC integrations.
+	  Currently supports SpacemiT DWC USB3. Platforms using
+	  dwc3-of-simple can easily switch to dwc3-generic by flattening
+	  the dwc3 child node in the device tree.
+	  Say 'Y' or 'M' here if your platform integrates DWC3 in a similar way.
+
 endif
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index 830e6c9e5fe073c1f662ce34b6a4a2da34c407a2..96469e48ff9d189cc8d0b65e65424eae2158bcfe 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_USB_DWC3_IMX8MP)		+= dwc3-imx8mp.o
 obj-$(CONFIG_USB_DWC3_XILINX)		+= dwc3-xilinx.o
 obj-$(CONFIG_USB_DWC3_OCTEON)		+= dwc3-octeon.o
 obj-$(CONFIG_USB_DWC3_RTK)		+= dwc3-rtk.o
+obj-$(CONFIG_USB_DWC3_GENERIC_PLAT)	+= dwc3-generic-plat.o
diff --git a/drivers/usb/dwc3/dwc3-generic-plat.c b/drivers/usb/dwc3/dwc3-generic-plat.c
new file mode 100644
index 0000000000000000000000000000000000000000..766f4cf17b6909793956f44660d3b3febad50a23
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-generic-plat.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * dwc3-generic-plat.c - DesignWare USB3 generic platform driver
+ *
+ * Copyright (C) 2025 Ze Huang <huang.ze@linux.dev>
+ *
+ * Inspired by dwc3-qcom.c and dwc3-of-simple.c
+ */
+
+#include <linux/clk.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include "glue.h"
+
+struct dwc3_generic {
+	struct device		*dev;
+	struct dwc3		dwc;
+	struct clk_bulk_data	*clks;
+	int			num_clocks;
+	struct reset_control	*resets;
+};
+
+static void dwc3_generic_reset_control_assert(void *data)
+{
+	reset_control_assert(data);
+}
+
+static void dwc3_generic_clk_bulk_disable_unprepare(void *data)
+{
+	struct dwc3_generic *dwc3 = data;
+
+	clk_bulk_disable_unprepare(dwc3->num_clocks, dwc3->clks);
+}
+
+static int dwc3_generic_probe(struct platform_device *pdev)
+{
+	struct dwc3_probe_data probe_data = {};
+	struct device *dev = &pdev->dev;
+	struct dwc3_generic *dwc3;
+	struct resource *res;
+	int ret;
+
+	dwc3 = devm_kzalloc(dev, sizeof(*dwc3), GFP_KERNEL);
+	if (!dwc3)
+		return -ENOMEM;
+
+	dwc3->dev = dev;
+
+	platform_set_drvdata(pdev, dwc3);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "missing memory resource\n");
+		return -ENODEV;
+	}
+
+	dwc3->resets = devm_reset_control_array_get_optional_exclusive(dev);
+	if (IS_ERR(dwc3->resets))
+		return dev_err_probe(dev, PTR_ERR(dwc3->resets), "failed to get resets\n");
+
+	ret = reset_control_assert(dwc3->resets);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to assert resets\n");
+
+	ret = devm_add_action_or_reset(dev, dwc3_generic_reset_control_assert, dwc3->resets);
+	if (ret)
+		return ret;
+
+	usleep_range(10, 1000);
+
+	ret = reset_control_deassert(dwc3->resets);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to deassert resets\n");
+
+	ret = devm_clk_bulk_get_all(dwc3->dev, &dwc3->clks);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to get clocks\n");
+
+	dwc3->num_clocks = ret;
+
+	ret = clk_bulk_prepare_enable(dwc3->num_clocks, dwc3->clks);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable clocks\n");
+
+	ret = devm_add_action_or_reset(dev, dwc3_generic_clk_bulk_disable_unprepare, dwc3);
+	if (ret)
+		return ret;
+
+	dwc3->dwc.dev = dev;
+	probe_data.dwc = &dwc3->dwc;
+	probe_data.res = res;
+	probe_data.ignore_clocks_and_resets = true;
+	ret = dwc3_core_probe(&probe_data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
+
+	return 0;
+}
+
+static void dwc3_generic_remove(struct platform_device *pdev)
+{
+	struct dwc3_generic *dwc3 = platform_get_drvdata(pdev);
+
+	dwc3_core_remove(&dwc3->dwc);
+}
+
+static int dwc3_generic_suspend(struct device *dev)
+{
+	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
+	int ret;
+
+	ret = dwc3_pm_suspend(&dwc3->dwc);
+	if (ret)
+		return ret;
+
+	clk_bulk_disable_unprepare(dwc3->num_clocks, dwc3->clks);
+
+	return 0;
+}
+
+static int dwc3_generic_resume(struct device *dev)
+{
+	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_bulk_prepare_enable(dwc3->num_clocks, dwc3->clks);
+	if (ret)
+		return ret;
+
+	ret = dwc3_pm_resume(&dwc3->dwc);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int dwc3_generic_runtime_suspend(struct device *dev)
+{
+	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
+
+	return dwc3_runtime_suspend(&dwc3->dwc);
+}
+
+static int dwc3_generic_runtime_resume(struct device *dev)
+{
+	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
+
+	return dwc3_runtime_resume(&dwc3->dwc);
+}
+
+static int dwc3_generic_runtime_idle(struct device *dev)
+{
+	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
+
+	return dwc3_runtime_idle(&dwc3->dwc);
+}
+
+static const struct dev_pm_ops dwc3_generic_dev_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(dwc3_generic_suspend, dwc3_generic_resume)
+	RUNTIME_PM_OPS(dwc3_generic_runtime_suspend, dwc3_generic_runtime_resume,
+		       dwc3_generic_runtime_idle)
+};
+
+static const struct of_device_id dwc3_generic_of_match[] = {
+	{ .compatible = "spacemit,k1-dwc3", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, dwc3_generic_of_match);
+
+static struct platform_driver dwc3_generic_driver = {
+	.probe		= dwc3_generic_probe,
+	.remove		= dwc3_generic_remove,
+	.driver		= {
+		.name	= "dwc3-generic-plat",
+		.of_match_table = dwc3_generic_of_match,
+		.pm	= pm_ptr(&dwc3_generic_dev_pm_ops),
+	},
+};
+module_platform_driver(dwc3_generic_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("DesignWare USB3 generic platform driver");

-- 
2.50.1

Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Alex Elder]

On 7/12/25 2:49 AM, Ze Huang wrote:
> To support flattened dwc3 dt model and drop the glue layer, introduce the
> `dwc3-generic` driver. This enables direct binding of the DWC3 core driver
> and offers an alternative to the existing glue driver `dwc3-of-simple`.

I'm not familiar with dwc-of-simple.c, and won't comment on
how this differs from that (or does not).

Given you're implementing an alternative though, can you explain
in a little more detail what's different between the two?  Why
would someone choose to use this driver rather than the other one?

> 
> Signed-off-by: Ze Huang <huang.ze@linux.dev>
> ---
>   drivers/usb/dwc3/Kconfig             |  11 +++
>   drivers/usb/dwc3/Makefile            |   1 +
>   drivers/usb/dwc3/dwc3-generic-plat.c | 182 +++++++++++++++++++++++++++++++++++
>   3 files changed, 194 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 310d182e10b50b253d7e5a51674806e6ec442a2a..4925d15084f816d3ff92059b476ebcc799b56b51 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -189,4 +189,15 @@ config USB_DWC3_RTK
>   	  or dual-role mode.
>   	  Say 'Y' or 'M' if you have such device.
>   
> +config USB_DWC3_GENERIC_PLAT
> +	tristate "DWC3 Generic Platform Driver"
> +	depends on OF && COMMON_CLK
> +	default USB_DWC3
> +	help
> +	  Support USB3 functionality in simple SoC integrations.
> +	  Currently supports SpacemiT DWC USB3. Platforms using
> +	  dwc3-of-simple can easily switch to dwc3-generic by flattening
> +	  the dwc3 child node in the device tree.
> +	  Say 'Y' or 'M' here if your platform integrates DWC3 in a similar way.
> +
>   endif
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index 830e6c9e5fe073c1f662ce34b6a4a2da34c407a2..96469e48ff9d189cc8d0b65e65424eae2158bcfe 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_USB_DWC3_IMX8MP)		+= dwc3-imx8mp.o
>   obj-$(CONFIG_USB_DWC3_XILINX)		+= dwc3-xilinx.o
>   obj-$(CONFIG_USB_DWC3_OCTEON)		+= dwc3-octeon.o
>   obj-$(CONFIG_USB_DWC3_RTK)		+= dwc3-rtk.o
> +obj-$(CONFIG_USB_DWC3_GENERIC_PLAT)	+= dwc3-generic-plat.o
> diff --git a/drivers/usb/dwc3/dwc3-generic-plat.c b/drivers/usb/dwc3/dwc3-generic-plat.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..766f4cf17b6909793956f44660d3b3febad50a23
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-generic-plat.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * dwc3-generic-plat.c - DesignWare USB3 generic platform driver
> + *
> + * Copyright (C) 2025 Ze Huang <huang.ze@linux.dev>
> + *
> + * Inspired by dwc3-qcom.c and dwc3-of-simple.c
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include "glue.h"
> +
> +struct dwc3_generic {
> +	struct device		*dev;
> +	struct dwc3		dwc;
> +	struct clk_bulk_data	*clks;
> +	int			num_clocks;
> +	struct reset_control	*resets;
> +};
> +
> +static void dwc3_generic_reset_control_assert(void *data)
> +{
> +	reset_control_assert(data);
> +}
> +

The next function can go away.  (See below.)

> +static void dwc3_generic_clk_bulk_disable_unprepare(void *data)
> +{
> +	struct dwc3_generic *dwc3 = data;
> +
> +	clk_bulk_disable_unprepare(dwc3->num_clocks, dwc3->clks);
> +}
> +
> +static int dwc3_generic_probe(struct platform_device *pdev)
> +{
> +	struct dwc3_probe_data probe_data = {};
> +	struct device *dev = &pdev->dev;
> +	struct dwc3_generic *dwc3;
> +	struct resource *res;
> +	int ret;
> +
> +	dwc3 = devm_kzalloc(dev, sizeof(*dwc3), GFP_KERNEL);
> +	if (!dwc3)
> +		return -ENOMEM;
> +
> +	dwc3->dev = dev;
> +
> +	platform_set_drvdata(pdev, dwc3);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "missing memory resource\n");
> +		return -ENODEV;
> +	}
> +
> +	dwc3->resets = devm_reset_control_array_get_optional_exclusive(dev);
> +	if (IS_ERR(dwc3->resets))
> +		return dev_err_probe(dev, PTR_ERR(dwc3->resets), "failed to get resets\n");
> +

It isn't enforced on exclusive resets, but I'm pretty sure
resets are assumed to be asserted initially.

> +	ret = reset_control_assert(dwc3->resets);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to assert resets\n");
> +
> +	ret = devm_add_action_or_reset(dev, dwc3_generic_reset_control_assert, dwc3->resets);
> +	if (ret)
> +		return ret;

The re-assert shouldn't be set up unless the deassert below
succeeds.

> +	usleep_range(10, 1000);

This seems like a large range.  You could just do msleep(1);
Also, can you add a comment explaining why a delay is needed,
and why 1 millisecond is the right amount of time to sleep?

> +	ret = reset_control_deassert(dwc3->resets);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to deassert resets\n");
> +
> +	ret = devm_clk_bulk_get_all(dwc3->dev, &dwc3->clks);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed to get clocks\n");

Call devm_clk_bulk_get_all_enabled() instead of doing the two
steps separately here.

					-Alex

> +	dwc3->num_clocks = ret;
> +
> +	ret = clk_bulk_prepare_enable(dwc3->num_clocks, dwc3->clks);
> +	if (ret)
j> +		return dev_err_probe(dev, ret, "failed to enable clocks\n");
> +
> +	ret = devm_add_action_or_reset(dev, dwc3_generic_clk_bulk_disable_unprepare, dwc3);
> +	if (ret)
> +		return ret;
> +
> +	dwc3->dwc.dev = dev;
> +	probe_data.dwc = &dwc3->dwc;
> +	probe_data.res = res;
> +	probe_data.ignore_clocks_and_resets = true;
> +	ret = dwc3_core_probe(&probe_data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
> +
> +	return 0;
> +}
> +
> +static void dwc3_generic_remove(struct platform_device *pdev)
> +{
> +	struct dwc3_generic *dwc3 = platform_get_drvdata(pdev);
> +
> +	dwc3_core_remove(&dwc3->dwc);
> +}
> +
> +static int dwc3_generic_suspend(struct device *dev)
> +{
> +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = dwc3_pm_suspend(&dwc3->dwc);
> +	if (ret)
> +		return ret;
> +
> +	clk_bulk_disable_unprepare(dwc3->num_clocks, dwc3->clks);
> +
> +	return 0;
> +}
> +
> +static int dwc3_generic_resume(struct device *dev)
> +{
> +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_bulk_prepare_enable(dwc3->num_clocks, dwc3->clks);
> +	if (ret)
> +		return ret;
> +
> +	ret = dwc3_pm_resume(&dwc3->dwc);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dwc3_generic_runtime_suspend(struct device *dev)
> +{
> +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> +
> +	return dwc3_runtime_suspend(&dwc3->dwc);
> +}
> +
> +static int dwc3_generic_runtime_resume(struct device *dev)
> +{
> +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> +
> +	return dwc3_runtime_resume(&dwc3->dwc);
> +}
> +
> +static int dwc3_generic_runtime_idle(struct device *dev)
> +{
> +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> +
> +	return dwc3_runtime_idle(&dwc3->dwc);
> +}
> +
> +static const struct dev_pm_ops dwc3_generic_dev_pm_ops = {
> +	SYSTEM_SLEEP_PM_OPS(dwc3_generic_suspend, dwc3_generic_resume)
> +	RUNTIME_PM_OPS(dwc3_generic_runtime_suspend, dwc3_generic_runtime_resume,
> +		       dwc3_generic_runtime_idle)
> +};
> +
> +static const struct of_device_id dwc3_generic_of_match[] = {
> +	{ .compatible = "spacemit,k1-dwc3", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, dwc3_generic_of_match);
> +
> +static struct platform_driver dwc3_generic_driver = {
> +	.probe		= dwc3_generic_probe,
> +	.remove		= dwc3_generic_remove,
> +	.driver		= {
> +		.name	= "dwc3-generic-plat",
> +		.of_match_table = dwc3_generic_of_match,
> +		.pm	= pm_ptr(&dwc3_generic_dev_pm_ops),
> +	},
> +};
> +module_platform_driver(dwc3_generic_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("DesignWare USB3 generic platform driver");
> 

Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Ze Huang]

On Tue, Jul 15, 2025 at 03:50:54PM -0500, Alex Elder wrote:
> On 7/12/25 2:49 AM, Ze Huang wrote:
> > To support flattened dwc3 dt model and drop the glue layer, introduce the
> > `dwc3-generic` driver. This enables direct binding of the DWC3 core driver
> > and offers an alternative to the existing glue driver `dwc3-of-simple`.
> 
> I'm not familiar with dwc-of-simple.c, and won't comment on
> how this differs from that (or does not).
> 
> Given you're implementing an alternative though, can you explain
> in a little more detail what's different between the two?  Why
> would someone choose to use this driver rather than the other one?

They are basically the same.

dwc-generic use a plain dt node while dwc-of-simple will nest the dwc3
node as its child.

Both will use dwc3_core_probe() to finish the probe process. But now we
can simplify the process by just calling it, instead of calling
of_platform_populate() and create another snps,dwc3 device driver.

> 
> > 
> > Signed-off-by: Ze Huang <huang.ze@linux.dev>
> > ---
> >   drivers/usb/dwc3/Kconfig             |  11 +++
> >   drivers/usb/dwc3/Makefile            |   1 +
> >   drivers/usb/dwc3/dwc3-generic-plat.c | 182 +++++++++++++++++++++++++++++++++++
> >   3 files changed, 194 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > index 310d182e10b50b253d7e5a51674806e6ec442a2a..4925d15084f816d3ff92059b476ebcc799b56b51 100644
> > --- a/drivers/usb/dwc3/Kconfig
> > +++ b/drivers/usb/dwc3/Kconfig
> > @@ -189,4 +189,15 @@ config USB_DWC3_RTK
> >   	  or dual-role mode.
> >   	  Say 'Y' or 'M' if you have such device.
> > +config USB_DWC3_GENERIC_PLAT
> > +	tristate "DWC3 Generic Platform Driver"
> > +	depends on OF && COMMON_CLK
> > +	default USB_DWC3
> > +	help
> > +	  Support USB3 functionality in simple SoC integrations.
> > +	  Currently supports SpacemiT DWC USB3. Platforms using
> > +	  dwc3-of-simple can easily switch to dwc3-generic by flattening
> > +	  the dwc3 child node in the device tree.
> > +	  Say 'Y' or 'M' here if your platform integrates DWC3 in a similar way.
> > +
> >   endif
> > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> > index 830e6c9e5fe073c1f662ce34b6a4a2da34c407a2..96469e48ff9d189cc8d0b65e65424eae2158bcfe 100644
> > --- a/drivers/usb/dwc3/Makefile
> > +++ b/drivers/usb/dwc3/Makefile
> > @@ -57,3 +57,4 @@ obj-$(CONFIG_USB_DWC3_IMX8MP)		+= dwc3-imx8mp.o
> >   obj-$(CONFIG_USB_DWC3_XILINX)		+= dwc3-xilinx.o
> >   obj-$(CONFIG_USB_DWC3_OCTEON)		+= dwc3-octeon.o
> >   obj-$(CONFIG_USB_DWC3_RTK)		+= dwc3-rtk.o
> > +obj-$(CONFIG_USB_DWC3_GENERIC_PLAT)	+= dwc3-generic-plat.o
> > diff --git a/drivers/usb/dwc3/dwc3-generic-plat.c b/drivers/usb/dwc3/dwc3-generic-plat.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..766f4cf17b6909793956f44660d3b3febad50a23
> > --- /dev/null
> > +++ b/drivers/usb/dwc3/dwc3-generic-plat.c
> > @@ -0,0 +1,182 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * dwc3-generic-plat.c - DesignWare USB3 generic platform driver
> > + *
> > + * Copyright (C) 2025 Ze Huang <huang.ze@linux.dev>
> > + *
> > + * Inspired by dwc3-qcom.c and dwc3-of-simple.c
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include "glue.h"
> > +
> > +struct dwc3_generic {
> > +	struct device		*dev;
> > +	struct dwc3		dwc;
> > +	struct clk_bulk_data	*clks;
> > +	int			num_clocks;
> > +	struct reset_control	*resets;
> > +};
> > +
> > +static void dwc3_generic_reset_control_assert(void *data)
> > +{
> > +	reset_control_assert(data);
> > +}
> > +
> 
> The next function can go away.  (See below.)
> 

OK

> > +static void dwc3_generic_clk_bulk_disable_unprepare(void *data)
> > +{
> > +	struct dwc3_generic *dwc3 = data;
> > +
> > +	clk_bulk_disable_unprepare(dwc3->num_clocks, dwc3->clks);
> > +}
> > +
> > +static int dwc3_generic_probe(struct platform_device *pdev)
> > +{
> > +	struct dwc3_probe_data probe_data = {};
> > +	struct device *dev = &pdev->dev;
> > +	struct dwc3_generic *dwc3;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	dwc3 = devm_kzalloc(dev, sizeof(*dwc3), GFP_KERNEL);
> > +	if (!dwc3)
> > +		return -ENOMEM;
> > +
> > +	dwc3->dev = dev;
> > +
> > +	platform_set_drvdata(pdev, dwc3);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "missing memory resource\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	dwc3->resets = devm_reset_control_array_get_optional_exclusive(dev);
> > +	if (IS_ERR(dwc3->resets))
> > +		return dev_err_probe(dev, PTR_ERR(dwc3->resets), "failed to get resets\n");
> > +
> 
> It isn't enforced on exclusive resets, but I'm pretty sure
> resets are assumed to be asserted initially.
> 

I'd like to keep it. We cannot guarantee what environment was passed
in (from bootloader), right?

> > +	ret = reset_control_assert(dwc3->resets);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to assert resets\n");
> > +
> > +	ret = devm_add_action_or_reset(dev, dwc3_generic_reset_control_assert, dwc3->resets);
> > +	if (ret)
> > +		return ret;
> 
> The re-assert shouldn't be set up unless the deassert below
> succeeds.
> 

Will move behind the deassert.

> > +	usleep_range(10, 1000);
> 
> This seems like a large range.  You could just do msleep(1);
> Also, can you add a comment explaining why a delay is needed,
> and why 1 millisecond is the right amount of time to sleep?
> 

I will check the range with spacemit and reply soon.

> > +	ret = reset_control_deassert(dwc3->resets);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to deassert resets\n");
> > +
> > +	ret = devm_clk_bulk_get_all(dwc3->dev, &dwc3->clks);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> 
> Call devm_clk_bulk_get_all_enabled() instead of doing the two
> steps separately here.
> 

Will do, thanks.

> 					-Alex
> 
> > +	dwc3->num_clocks = ret;
> > +
> > +	ret = clk_bulk_prepare_enable(dwc3->num_clocks, dwc3->clks);
> > +	if (ret)
> j> +		return dev_err_probe(dev, ret, "failed to enable clocks\n");
> > +
> > +	ret = devm_add_action_or_reset(dev, dwc3_generic_clk_bulk_disable_unprepare, dwc3);
> > +	if (ret)
> > +		return ret;
> > +
> > +	dwc3->dwc.dev = dev;
> > +	probe_data.dwc = &dwc3->dwc;
> > +	probe_data.res = res;
> > +	probe_data.ignore_clocks_and_resets = true;
> > +	ret = dwc3_core_probe(&probe_data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static void dwc3_generic_remove(struct platform_device *pdev)
> > +{
> > +	struct dwc3_generic *dwc3 = platform_get_drvdata(pdev);
> > +
> > +	dwc3_core_remove(&dwc3->dwc);
> > +}
> > +
> > +static int dwc3_generic_suspend(struct device *dev)
> > +{
> > +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = dwc3_pm_suspend(&dwc3->dwc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	clk_bulk_disable_unprepare(dwc3->num_clocks, dwc3->clks);
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc3_generic_resume(struct device *dev)
> > +{
> > +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = clk_bulk_prepare_enable(dwc3->num_clocks, dwc3->clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = dwc3_pm_resume(&dwc3->dwc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +static int dwc3_generic_runtime_suspend(struct device *dev)
> > +{
> > +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> > +
> > +	return dwc3_runtime_suspend(&dwc3->dwc);
> > +}
> > +
> > +static int dwc3_generic_runtime_resume(struct device *dev)
> > +{
> > +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> > +
> > +	return dwc3_runtime_resume(&dwc3->dwc);
> > +}
> > +
> > +static int dwc3_generic_runtime_idle(struct device *dev)
> > +{
> > +	struct dwc3_generic *dwc3 = dev_get_drvdata(dev);
> > +
> > +	return dwc3_runtime_idle(&dwc3->dwc);
> > +}
> > +
> > +static const struct dev_pm_ops dwc3_generic_dev_pm_ops = {
> > +	SYSTEM_SLEEP_PM_OPS(dwc3_generic_suspend, dwc3_generic_resume)
> > +	RUNTIME_PM_OPS(dwc3_generic_runtime_suspend, dwc3_generic_runtime_resume,
> > +		       dwc3_generic_runtime_idle)
> > +};
> > +
> > +static const struct of_device_id dwc3_generic_of_match[] = {
> > +	{ .compatible = "spacemit,k1-dwc3", },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, dwc3_generic_of_match);
> > +
> > +static struct platform_driver dwc3_generic_driver = {
> > +	.probe		= dwc3_generic_probe,
> > +	.remove		= dwc3_generic_remove,
> > +	.driver		= {
> > +		.name	= "dwc3-generic-plat",
> > +		.of_match_table = dwc3_generic_of_match,
> > +		.pm	= pm_ptr(&dwc3_generic_dev_pm_ops),
> > +	},
> > +};
> > +module_platform_driver(dwc3_generic_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("DesignWare USB3 generic platform driver");
> > 
> 

Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Philipp Zabel]

On Di, 2025-07-15 at 15:50 -0500, Alex Elder wrote:
> On 7/12/25 2:49 AM, Ze Huang wrote:
[...]
> > +static int dwc3_generic_probe(struct platform_device *pdev)
> > +{
> > +	struct dwc3_probe_data probe_data = {};
> > +	struct device *dev = &pdev->dev;
> > +	struct dwc3_generic *dwc3;
> > +	struct resource *res;
> > +	int ret;
> > +
> > +	dwc3 = devm_kzalloc(dev, sizeof(*dwc3), GFP_KERNEL);
> > +	if (!dwc3)
> > +		return -ENOMEM;
> > +
> > +	dwc3->dev = dev;
> > +
> > +	platform_set_drvdata(pdev, dwc3);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "missing memory resource\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	dwc3->resets = devm_reset_control_array_get_optional_exclusive(dev);
> > +	if (IS_ERR(dwc3->resets))
> > +		return dev_err_probe(dev, PTR_ERR(dwc3->resets), "failed to get resets\n");
> > +
> 
> It isn't enforced on exclusive resets, but I'm pretty sure
> resets are assumed to be asserted initially.

The reset controller API doesn't guarantee this. Whether reset controls
are initially asserted depends on the specific SoC/reset controller and
also on what the bootloader did before.

For example, there are self-deasserting reset controls that start out
deasserted and can only ever be asserted for a short pulse [1]. Even
the shared reset API only assumes that the reset line may have been
asserted at some point before the first assert() [2].

[1] https://docs.kernel.org/driver-api/reset.html#triggering
[2] https://docs.kernel.org/driver-api/reset.html#assertion-and-deassertion

Whether an explicit reset_control_assert() in the probe function is
needed depends on which assumptions the driver can make on its own (on
all platforms it is used on).
For example, for some devices it may be enough to assume that the
device has been reset at some point between power-on and probe.


regards
Philipp

Re: [PATCH v6 1/2] dt-bindings: usb: dwc3: add support for SpacemiT K1

[Philipp Zabel]

On Sa, 2025-07-12 at 15:49 +0800, Ze Huang wrote:
> Add support for the USB 3.0 Dual-Role Device (DRD) controller embedded
> in the SpacemiT K1 SoC. The controller is based on the Synopsys
> DesignWare Core USB 3 (DWC3) IP, supporting USB3.0 host mode and USB 2.0
> DRD mode.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Ze Huang <huang.ze@linux.dev>
> ---
>  .../devicetree/bindings/usb/spacemit,k1-dwc3.yaml  | 107 +++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml b/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..c967ad6aae50199127a4f8a17d53fc34e8d9480b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml
> @@ -0,0 +1,107 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/spacemit,k1-dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SpacemiT K1 SuperSpeed DWC3 USB SoC Controller
> +
> +maintainers:
> +  - Ze Huang <huang.ze@linux.dev>
> +
> +description: |
> +  The SpacemiT K1 embeds a DWC3 USB IP Core which supports Host functions
> +  for USB 3.0 and DRD for USB 2.0.
> +
> +  Key features:
> +  - USB3.0 SuperSpeed and USB2.0 High/Full/Low-Speed support
> +  - Supports low-power modes (USB2.0 suspend, USB3.0 U1/U2/U3)
> +  - Internal DMA controller and flexible endpoint FIFO sizing
> +
> +  Communication Interface:
> +  - Use of PIPE3 (125MHz) interface for USB3.0 PHY
> +  - Use of UTMI+ (30/60MHz) interface for USB2.0 PHY
> +
> +allOf:
> +  - $ref: snps,dwc3-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: spacemit,k1-dwc3
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    const: usbdrd30
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  phys:
> +    items:
> +      - description: phandle to USB2/HS PHY
> +      - description: phandle to USB3/SS PHY
> +
> +  phy-names:
> +    items:
> +      - const: usb2-phy
> +      - const: usb3-phy
> +
> +  resets:
> +    items:
> +      - description: USB3.0 AHB reset line
> +      - description: USB3.0 VCC reset line
> +      - description: USB3.0 PHY reset line

Are we sure all resets will only ever need to be triggered together?
Otherwise it might be safer to add a reset-names property.

regards
Philipp

Re: [PATCH v6 1/2] dt-bindings: usb: dwc3: add support for SpacemiT K1

[Ze Huang]

On Mon, Jul 21, 2025 at 01:02:54PM +0200, Philipp Zabel wrote:
> On Sa, 2025-07-12 at 15:49 +0800, Ze Huang wrote:
> > Add support for the USB 3.0 Dual-Role Device (DRD) controller embedded
> > in the SpacemiT K1 SoC. The controller is based on the Synopsys
> > DesignWare Core USB 3 (DWC3) IP, supporting USB3.0 host mode and USB 2.0
> > DRD mode.
> > 
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Signed-off-by: Ze Huang <huang.ze@linux.dev>
> > ---
> >  .../devicetree/bindings/usb/spacemit,k1-dwc3.yaml  | 107 +++++++++++++++++++++
> >  1 file changed, 107 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml b/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..c967ad6aae50199127a4f8a17d53fc34e8d9480b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/spacemit,k1-dwc3.yaml
> > @@ -0,0 +1,107 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/spacemit,k1-dwc3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: SpacemiT K1 SuperSpeed DWC3 USB SoC Controller
> > +
> > +maintainers:
> > +  - Ze Huang <huang.ze@linux.dev>
> > +
> > +description: |
> > +  The SpacemiT K1 embeds a DWC3 USB IP Core which supports Host functions
> > +  for USB 3.0 and DRD for USB 2.0.
> > +
> > +  Key features:
> > +  - USB3.0 SuperSpeed and USB2.0 High/Full/Low-Speed support
> > +  - Supports low-power modes (USB2.0 suspend, USB3.0 U1/U2/U3)
> > +  - Internal DMA controller and flexible endpoint FIFO sizing
> > +
> > +  Communication Interface:
> > +  - Use of PIPE3 (125MHz) interface for USB3.0 PHY
> > +  - Use of UTMI+ (30/60MHz) interface for USB2.0 PHY
> > +
> > +allOf:
> > +  - $ref: snps,dwc3-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: spacemit,k1-dwc3
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +  clock-names:
> > +    const: usbdrd30
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  phys:
> > +    items:
> > +      - description: phandle to USB2/HS PHY
> > +      - description: phandle to USB3/SS PHY
> > +
> > +  phy-names:
> > +    items:
> > +      - const: usb2-phy
> > +      - const: usb3-phy
> > +
> > +  resets:
> > +    items:
> > +      - description: USB3.0 AHB reset line
> > +      - description: USB3.0 VCC reset line
> > +      - description: USB3.0 PHY reset line
> 
> Are we sure all resets will only ever need to be triggered together?
>
> Otherwise it might be safer to add a reset-names property.
>

Yeah, that's helpful. Will add reset-names property in next version.

> 
> regards
> Philipp

Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Ze Huang]

On Sun, Jul 20, 2025 at 02:34:07PM +0800, Ze Huang wrote:
> On Tue, Jul 15, 2025 at 03:50:54PM -0500, Alex Elder wrote:
> > On 7/12/25 2:49 AM, Ze Huang wrote:
> > > To support flattened dwc3 dt model and drop the glue layer, introduce the
> > > `dwc3-generic` driver. This enables direct binding of the DWC3 core driver
> > > and offers an alternative to the existing glue driver `dwc3-of-simple`.
> > 
> > I'm not familiar with dwc-of-simple.c, and won't comment on
> > how this differs from that (or does not).
> > 
> > Given you're implementing an alternative though, can you explain
> > in a little more detail what's different between the two?  Why
> > would someone choose to use this driver rather than the other one?
> 
> They are basically the same.
> 
> dwc-generic use a plain dt node while dwc-of-simple will nest the dwc3
> node as its child.
> 
> Both will use dwc3_core_probe() to finish the probe process. But now we
> can simplify the process by just calling it, instead of calling
> of_platform_populate() and create another snps,dwc3 device driver.

[...]

> > > +	ret = reset_control_assert(dwc3->resets);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "failed to assert resets\n");
> > > +
> > > +	ret = devm_add_action_or_reset(dev, dwc3_generic_reset_control_assert, dwc3->resets);
> > > +	if (ret)
> > > +		return ret;
> > 
> > The re-assert shouldn't be set up unless the deassert below
> > succeeds.
> > 
> 
> Will move behind the deassert.
> 
> > > +	usleep_range(10, 1000);
> > 
> > This seems like a large range.  You could just do msleep(1);
> > Also, can you add a comment explaining why a delay is needed,
> > and why 1 millisecond is the right amount of time to sleep?
> > 
> 
> I will check the range with spacemit and reply soon.
> 

the resets are asynchronous with no strict timing. But to be safe, each
reset should stay active for at least 1 µs. I’ll switch to a udelay(2)
and add comment accordingly.

> > > +	ret = reset_control_deassert(dwc3->resets);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "failed to deassert resets\n");
> > > +
> > > +	ret = devm_clk_bulk_get_all(dwc3->dev, &dwc3->clks);
> > > +	if (ret < 0)
> > > +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> > 
> > Call devm_clk_bulk_get_all_enabled() instead of doing the two
> > steps separately here.
> > 
> 
> Will do, thanks.
> 
> > 					-Alex

Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Yao Zi]

On Mon, Jul 21, 2025 at 08:08:06PM +0800, Ze Huang wrote:
> On Sun, Jul 20, 2025 at 02:34:07PM +0800, Ze Huang wrote:
> > On Tue, Jul 15, 2025 at 03:50:54PM -0500, Alex Elder wrote:
> > > On 7/12/25 2:49 AM, Ze Huang wrote:
> > > > To support flattened dwc3 dt model and drop the glue layer, introduce the
> > > > `dwc3-generic` driver. This enables direct binding of the DWC3 core driver
> > > > and offers an alternative to the existing glue driver `dwc3-of-simple`.
> > > 
> > > I'm not familiar with dwc-of-simple.c, and won't comment on
> > > how this differs from that (or does not).
> > > 
> > > Given you're implementing an alternative though, can you explain
> > > in a little more detail what's different between the two?  Why
> > > would someone choose to use this driver rather than the other one?
> > 
> > They are basically the same.
> > 
> > dwc-generic use a plain dt node while dwc-of-simple will nest the dwc3
> > node as its child.
> > 
> > Both will use dwc3_core_probe() to finish the probe process. But now we
> > can simplify the process by just calling it, instead of calling
> > of_platform_populate() and create another snps,dwc3 device driver.
> 
> [...]
> 
> > > > +	ret = reset_control_assert(dwc3->resets);
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "failed to assert resets\n");
> > > > +
> > > > +	ret = devm_add_action_or_reset(dev, dwc3_generic_reset_control_assert, dwc3->resets);
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > The re-assert shouldn't be set up unless the deassert below
> > > succeeds.
> > > 
> > 
> > Will move behind the deassert.
> > 
> > > > +	usleep_range(10, 1000);
> > > 
> > > This seems like a large range.  You could just do msleep(1);
> > > Also, can you add a comment explaining why a delay is needed,
> > > and why 1 millisecond is the right amount of time to sleep?
> > > 
> > 
> > I will check the range with spacemit and reply soon.
> > 
> 
> the resets are asynchronous with no strict timing. But to be safe, each
> reset should stay active for at least 1 µs. I’ll switch to a udelay(2)
> and add comment accordingly.

This may be a little farsight: do you think it's better to make the
reset timing part of the of_match_data? This is more flexible and
reduces future burden when introducing a new platform that comes with a
different reset timing, which is a very likely case we'll face since
it's a "generic" driver.

> > > > +	ret = reset_control_deassert(dwc3->resets);
> > > > +	if (ret)
> > > > +		return dev_err_probe(dev, ret, "failed to deassert resets\n");
> > > > +
> > > > +	ret = devm_clk_bulk_get_all(dwc3->dev, &dwc3->clks);
> > > > +	if (ret < 0)
> > > > +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> > > 
> > > Call devm_clk_bulk_get_all_enabled() instead of doing the two
> > > steps separately here.
> > > 
> > 
> > Will do, thanks.
> > 
> > > 					-Alex
> 

Regards,
Yao Zi

Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened

[Ze Huang]

On Tue, Jul 22, 2025 at 12:34:46AM +0000, Yao Zi wrote:
> On Mon, Jul 21, 2025 at 08:08:06PM +0800, Ze Huang wrote:
> > On Sun, Jul 20, 2025 at 02:34:07PM +0800, Ze Huang wrote:
> > > On Tue, Jul 15, 2025 at 03:50:54PM -0500, Alex Elder wrote:
> > > > On 7/12/25 2:49 AM, Ze Huang wrote:
> > > > > To support flattened dwc3 dt model and drop the glue layer, introduce the
> > > > > `dwc3-generic` driver. This enables direct binding of the DWC3 core driver
> > > > > and offers an alternative to the existing glue driver `dwc3-of-simple`.
> > > > 
> > > > I'm not familiar with dwc-of-simple.c, and won't comment on
> > > > how this differs from that (or does not).
> > > > 
> > > > Given you're implementing an alternative though, can you explain
> > > > in a little more detail what's different between the two?  Why
> > > > would someone choose to use this driver rather than the other one?
> > > 
> > > They are basically the same.
> > > 
> > > dwc-generic use a plain dt node while dwc-of-simple will nest the dwc3
> > > node as its child.
> > > 
> > > Both will use dwc3_core_probe() to finish the probe process. But now we
> > > can simplify the process by just calling it, instead of calling
> > > of_platform_populate() and create another snps,dwc3 device driver.
> > 
> > [...]
> > 
> > > > > +	ret = reset_control_assert(dwc3->resets);
> > > > > +	if (ret)
> > > > > +		return dev_err_probe(dev, ret, "failed to assert resets\n");
> > > > > +
> > > > > +	ret = devm_add_action_or_reset(dev, dwc3_generic_reset_control_assert, dwc3->resets);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > 
> > > > The re-assert shouldn't be set up unless the deassert below
> > > > succeeds.
> > > > 
> > > 
> > > Will move behind the deassert.
> > > 
> > > > > +	usleep_range(10, 1000);
> > > > 
> > > > This seems like a large range.  You could just do msleep(1);
> > > > Also, can you add a comment explaining why a delay is needed,
> > > > and why 1 millisecond is the right amount of time to sleep?
> > > > 
> > > 
> > > I will check the range with spacemit and reply soon.
> > > 
> > 
> > the resets are asynchronous with no strict timing. But to be safe, each
> > reset should stay active for at least 1 µs. I’ll switch to a udelay(2)
> > and add comment accordingly.
> 
> This may be a little farsight: do you think it's better to make the
> reset timing part of the of_match_data? This is more flexible and
> reduces future burden when introducing a new platform that comes with a
> different reset timing, which is a very likely case we'll face since
> it's a "generic" driver.
> 

Hi Zi Yao, thanks for the suggestion.

The delay is only for safety. I think there will not be much device require
this setting. I'd prefer to keep the logic simple and just cover the
currently known compatible devices.

If we encounter future platforms that require different timing constraints,
we can revisit this and introduce of_match_data as needed.

> > > > > +	ret = reset_control_deassert(dwc3->resets);
> > > > > +	if (ret)
> > > > > +		return dev_err_probe(dev, ret, "failed to deassert resets\n");
> > > > > +
> > > > > +	ret = devm_clk_bulk_get_all(dwc3->dev, &dwc3->clks);
> > > > > +	if (ret < 0)
> > > > > +		return dev_err_probe(dev, ret, "failed to get clocks\n");
> > > > 
> > > > Call devm_clk_bulk_get_all_enabled() instead of doing the two
> > > > steps separately here.
> > > > 
> > > 
> > > Will do, thanks.
> > > 
> > > > 					-Alex
> > 
> 
> Regards,
> Yao Zi