* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
@ 2025-07-15 20:50 ` Alex Elder
0 siblings, 0 replies; 22+ messages in thread
From: Alex Elder @ 2025-07-15 20:50 UTC (permalink / raw)
To: Ze Huang, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Yixun Lan, Thinh Nguyen, Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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");
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
2025-07-15 20:50 ` Alex Elder
@ 2025-07-20 6:34 ` Ze Huang
-1 siblings, 0 replies; 22+ messages in thread
From: Ze Huang @ 2025-07-20 6:34 UTC (permalink / raw)
To: Alex Elder, Ze Huang, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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");
> >
>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
@ 2025-07-20 6:34 ` Ze Huang
0 siblings, 0 replies; 22+ messages in thread
From: Ze Huang @ 2025-07-20 6:34 UTC (permalink / raw)
To: Alex Elder, Ze Huang, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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");
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
2025-07-20 6:34 ` Ze Huang
@ 2025-07-21 12:08 ` Ze Huang
-1 siblings, 0 replies; 22+ messages in thread
From: Ze Huang @ 2025-07-21 12:08 UTC (permalink / raw)
To: Alex Elder, Ze Huang, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
@ 2025-07-21 12:08 ` Ze Huang
0 siblings, 0 replies; 22+ messages in thread
From: Ze Huang @ 2025-07-21 12:08 UTC (permalink / raw)
To: Alex Elder, Ze Huang, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
2025-07-21 12:08 ` Ze Huang
@ 2025-07-22 0:34 ` Yao Zi
-1 siblings, 0 replies; 22+ messages in thread
From: Yao Zi @ 2025-07-22 0:34 UTC (permalink / raw)
To: Ze Huang, Alex Elder, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
@ 2025-07-22 0:34 ` Yao Zi
0 siblings, 0 replies; 22+ messages in thread
From: Yao Zi @ 2025-07-22 0:34 UTC (permalink / raw)
To: Ze Huang, Alex Elder, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
2025-07-22 0:34 ` Yao Zi
@ 2025-07-22 15:55 ` Ze Huang
-1 siblings, 0 replies; 22+ messages in thread
From: Ze Huang @ 2025-07-22 15:55 UTC (permalink / raw)
To: Yao Zi, Ze Huang, Alex Elder, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
@ 2025-07-22 15:55 ` Ze Huang
0 siblings, 0 replies; 22+ messages in thread
From: Ze Huang @ 2025-07-22 15:55 UTC (permalink / raw)
To: Yao Zi, Ze Huang, Alex Elder, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen,
Philipp Zabel
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
2025-07-15 20:50 ` Alex Elder
@ 2025-07-21 11:01 ` Philipp Zabel
-1 siblings, 0 replies; 22+ messages in thread
From: Philipp Zabel @ 2025-07-21 11:01 UTC (permalink / raw)
To: Alex Elder, Ze Huang, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH v6 2/2] usb: dwc3: add generic driver to support flattened
@ 2025-07-21 11:01 ` Philipp Zabel
0 siblings, 0 replies; 22+ messages in thread
From: Philipp Zabel @ 2025-07-21 11:01 UTC (permalink / raw)
To: Alex Elder, Ze Huang, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Yixun Lan, Thinh Nguyen
Cc: linux-usb, devicetree, linux-riscv, spacemit, linux-kernel
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
^ permalink raw reply [flat|nested] 22+ messages in thread