| Message ID | 20250113-dwc3-refactor-v3-8-d1722075df7b@oss.qualcomm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
On Mon, Jan 13, 2025 at 09:11:41PM -0800, Bjorn Andersson wrote: > The DWC3 IP block is handled by three distinct device drivers: XHCI, > DWC3 core and a platform specific (optional) DWC3 glue driver. > > This has resulted in, at least in the case of the Qualcomm glue, the > presence of a number of layering violations, where the glue code either > can't handle, or has to work around, the fact that core might not probe > deterministically. > > An example of this is that the suspend path should operate slightly > different depending on the device operating in host or peripheral mode, > and the only way to determine the operating state is to peek into the > core's drvdata. > > The Qualcomm glue driver is expected to make updates in the qscratch > register region (the "glue" region) during role switch events, but with > the glue and core split using the driver model, there is no reasonable > way to introduce listeners for mode changes. > > Split the dwc3 core platform_driver callbacks and their implementation > and export the implementation, to make it possible to deterministically > instantiate the dwc3 core as part of the dwc3 glue drivers and to > allow flattening of the DeviceTree representation. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> > --- > drivers/usb/dwc3/core.c | 149 ++++++++++++++++++++++++++++++------------------ > drivers/usb/dwc3/glue.h | 22 +++++++ > 2 files changed, 117 insertions(+), 54 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index c22b8678e02e..930d812a9fbb 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -36,6 +36,7 @@ > > #include "core.h" > #include "gadget.h" > +#include "glue.h" > #include "io.h" > > #include "debug.h" > @@ -2129,27 +2130,14 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) > return 0; > } > > -static int dwc3_probe(struct platform_device *pdev) > +int dwc3_init(struct dwc3 *dwc, struct resource *res) Maybe need data struct, which included res. It may need pass down more information in future. > { > - struct device *dev = &pdev->dev; > - struct resource *res, dwc_res; > + struct device *dev = dwc->dev; > + struct resource dwc_res; > unsigned int hw_mode; > void __iomem *regs; > - struct dwc3 *dwc; > int ret; > ... > +++ b/drivers/usb/dwc3/glue.h > @@ -0,0 +1,22 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * glue.h - DesignWare USB3 DRD glue header > + */ > + > +#ifndef __DRIVERS_USB_DWC3_GLUE_H > +#define __DRIVERS_USB_DWC3_GLUE_H > + > +#include <linux/types.h> > +#include "core.h" > + > +int dwc3_init(struct dwc3 *dwc, struct resource *res); > +void dwc3_uninit(struct dwc3 *dwc); > + > +int dwc3_runtime_suspend(struct dwc3 *dwc); > +int dwc3_runtime_resume(struct dwc3 *dwc); > +int dwc3_runtime_idle(struct dwc3 *dwc); > +int dwc3_suspend(struct dwc3 *dwc); > +int dwc3_resume(struct dwc3 *dwc); > +void dwc3_complete(struct dwc3 *dwc); dwc3_usb_*()? There may be name polution in future. There are many IPs created by dwc. Frank > + > +#endif > > -- > 2.45.2 >
On Tue, Jan 14, 2025 at 02:43:59PM -0500, Frank Li wrote: > On Mon, Jan 13, 2025 at 09:11:41PM -0800, Bjorn Andersson wrote: > > The DWC3 IP block is handled by three distinct device drivers: XHCI, > > DWC3 core and a platform specific (optional) DWC3 glue driver. > > > > This has resulted in, at least in the case of the Qualcomm glue, the > > presence of a number of layering violations, where the glue code either > > can't handle, or has to work around, the fact that core might not probe > > deterministically. > > > > An example of this is that the suspend path should operate slightly > > different depending on the device operating in host or peripheral mode, > > and the only way to determine the operating state is to peek into the > > core's drvdata. > > > > The Qualcomm glue driver is expected to make updates in the qscratch > > register region (the "glue" region) during role switch events, but with > > the glue and core split using the driver model, there is no reasonable > > way to introduce listeners for mode changes. > > > > Split the dwc3 core platform_driver callbacks and their implementation > > and export the implementation, to make it possible to deterministically > > instantiate the dwc3 core as part of the dwc3 glue drivers and to > > allow flattening of the DeviceTree representation. > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> > > --- > > drivers/usb/dwc3/core.c | 149 ++++++++++++++++++++++++++++++------------------ > > drivers/usb/dwc3/glue.h | 22 +++++++ > > 2 files changed, 117 insertions(+), 54 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index c22b8678e02e..930d812a9fbb 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -36,6 +36,7 @@ > > > > #include "core.h" > > #include "gadget.h" > > +#include "glue.h" > > #include "io.h" > > > > #include "debug.h" > > @@ -2129,27 +2130,14 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) > > return 0; > > } > > > > -static int dwc3_probe(struct platform_device *pdev) > > +int dwc3_init(struct dwc3 *dwc, struct resource *res) > > Maybe need data struct, which included res. It may need pass down more > information in future. > Per this series we're resolving the race conditions stemming from dwc3-qcom and core having different lifetimes, but more importantly it will allow us to propose a mechanism for invoking role-switch events in the glue code from the core - i.e. we are planning to add at least one more argument here; so this proposal makes sense. > > { > > - struct device *dev = &pdev->dev; > > - struct resource *res, dwc_res; > > + struct device *dev = dwc->dev; > > + struct resource dwc_res; > > unsigned int hw_mode; > > void __iomem *regs; > > - struct dwc3 *dwc; > > int ret; > > > ... > > > +++ b/drivers/usb/dwc3/glue.h > > @@ -0,0 +1,22 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * glue.h - DesignWare USB3 DRD glue header > > + */ > > + > > +#ifndef __DRIVERS_USB_DWC3_GLUE_H > > +#define __DRIVERS_USB_DWC3_GLUE_H > > + > > +#include <linux/types.h> > > +#include "core.h" > > + > > +int dwc3_init(struct dwc3 *dwc, struct resource *res); > > +void dwc3_uninit(struct dwc3 *dwc); > > + > > +int dwc3_runtime_suspend(struct dwc3 *dwc); > > +int dwc3_runtime_resume(struct dwc3 *dwc); > > +int dwc3_runtime_idle(struct dwc3 *dwc); > > +int dwc3_suspend(struct dwc3 *dwc); > > +int dwc3_resume(struct dwc3 *dwc); > > +void dwc3_complete(struct dwc3 *dwc); > > dwc3_usb_*()? There may be name polution in future. There are many IPs > created by dwc. > I thought dwc3 was uniquely associated with USB, but I don't have any objections to your proposal. Thanks, Bjorn > Frank > > > + > > +#endif > > > > -- > > 2.45.2 > >
On Tue, Jan 14, 2025, Bjorn Andersson wrote: > On Tue, Jan 14, 2025 at 02:43:59PM -0500, Frank Li wrote: > > ... > > > > > +++ b/drivers/usb/dwc3/glue.h > > > @@ -0,0 +1,22 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* > > > + * glue.h - DesignWare USB3 DRD glue header > > > + */ > > > + > > > +#ifndef __DRIVERS_USB_DWC3_GLUE_H > > > +#define __DRIVERS_USB_DWC3_GLUE_H > > > + > > > +#include <linux/types.h> > > > +#include "core.h" > > > + > > > +int dwc3_init(struct dwc3 *dwc, struct resource *res); > > > +void dwc3_uninit(struct dwc3 *dwc); > > > + > > > +int dwc3_runtime_suspend(struct dwc3 *dwc); > > > +int dwc3_runtime_resume(struct dwc3 *dwc); > > > +int dwc3_runtime_idle(struct dwc3 *dwc); > > > +int dwc3_suspend(struct dwc3 *dwc); > > > +int dwc3_resume(struct dwc3 *dwc); > > > +void dwc3_complete(struct dwc3 *dwc); > > > > dwc3_usb_*()? There may be name polution in future. There are many IPs > > created by dwc. > > > > I thought dwc3 was uniquely associated with USB, but I don't have any > objections to your proposal. > > Thanks, > Bjorn > Regarding this naming, let's keep them as what Bjorn has as they are more consistent to what we're doing in the driver. We can revise in the future should we need to. A couple of things I would like to rename is the "dwc3_uninit" to perhaps use dwc3_exit or dwc3_cleanup instead; the other is the dwc3_complete(), perhaps dwc3_pm_complete() for clarity. Also, should we add CONFIG_PM_SLEEP guards for these exported pm ops? Thanks, Thinh
On Tue, Jan 14, 2025, Thinh Nguyen wrote: > On Tue, Jan 14, 2025, Bjorn Andersson wrote: > > Also, should we add CONFIG_PM_SLEEP guards for these exported pm ops? Ignore this. I thought I saw the code outside of the guards. BR, Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index c22b8678e02e..930d812a9fbb 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -36,6 +36,7 @@ #include "core.h" #include "gadget.h" +#include "glue.h" #include "io.h" #include "debug.h" @@ -2129,27 +2130,14 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) return 0; } -static int dwc3_probe(struct platform_device *pdev) +int dwc3_init(struct dwc3 *dwc, struct resource *res) { - struct device *dev = &pdev->dev; - struct resource *res, dwc_res; + struct device *dev = dwc->dev; + struct resource dwc_res; unsigned int hw_mode; void __iomem *regs; - struct dwc3 *dwc; int ret; - dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); - if (!dwc) - return -ENOMEM; - - dwc->dev = dev; - - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - dev_err(dev, "missing memory resource\n"); - return -ENODEV; - } - dwc->xhci_resources[0].start = res->start; dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + DWC3_XHCI_REGS_END; @@ -2209,7 +2197,7 @@ static int dwc3_probe(struct platform_device *pdev) goto err_disable_clks; } - platform_set_drvdata(pdev, dwc); + dev_set_drvdata(dev, dwc); dwc3_cache_hwparams(dwc); if (!dwc->sysdev_is_parent && @@ -2304,12 +2292,31 @@ static int dwc3_probe(struct platform_device *pdev) return ret; } +EXPORT_SYMBOL_GPL(dwc3_init); -static void dwc3_remove(struct platform_device *pdev) +static int dwc3_probe(struct platform_device *pdev) { - struct dwc3 *dwc = platform_get_drvdata(pdev); + struct resource *res; + struct dwc3 *dwc; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pdev->dev, "missing memory resource\n"); + return -ENODEV; + } + + dwc = devm_kzalloc(&pdev->dev, sizeof(*dwc), GFP_KERNEL); + if (!dwc) + return -ENOMEM; - pm_runtime_get_sync(&pdev->dev); + dwc->dev = &pdev->dev; + + return dwc3_init(dwc, res); +} + +void dwc3_uninit(struct dwc3 *dwc) +{ + pm_runtime_get_sync(dwc->dev); dwc3_core_exit_mode(dwc); dwc3_debugfs_exit(dwc); @@ -2317,22 +2324,28 @@ static void dwc3_remove(struct platform_device *pdev) dwc3_core_exit(dwc); dwc3_ulpi_exit(dwc); - pm_runtime_allow(&pdev->dev); - pm_runtime_disable(&pdev->dev); - pm_runtime_dont_use_autosuspend(&pdev->dev); - pm_runtime_put_noidle(&pdev->dev); + pm_runtime_allow(dwc->dev); + pm_runtime_disable(dwc->dev); + pm_runtime_dont_use_autosuspend(dwc->dev); + pm_runtime_put_noidle(dwc->dev); /* * HACK: Clear the driver data, which is currently accessed by parent * glue drivers, before allowing the parent to suspend. */ - platform_set_drvdata(pdev, NULL); - pm_runtime_set_suspended(&pdev->dev); + dev_set_drvdata(dwc->dev, NULL); + pm_runtime_set_suspended(dwc->dev); dwc3_free_event_buffers(dwc); if (dwc->usb_psy) power_supply_put(dwc->usb_psy); } +EXPORT_SYMBOL_GPL(dwc3_uninit); + +static void dwc3_remove(struct platform_device *pdev) +{ + dwc3_uninit(platform_get_drvdata(pdev)); +} #ifdef CONFIG_PM static int dwc3_core_init_for_resume(struct dwc3 *dwc) @@ -2521,9 +2534,8 @@ static int dwc3_runtime_checks(struct dwc3 *dwc) return 0; } -static int dwc3_runtime_suspend(struct device *dev) +int dwc3_runtime_suspend(struct dwc3 *dwc) { - struct dwc3 *dwc = dev_get_drvdata(dev); int ret; if (dwc3_runtime_checks(dwc)) @@ -2535,10 +2547,10 @@ static int dwc3_runtime_suspend(struct device *dev) return 0; } +EXPORT_SYMBOL_GPL(dwc3_runtime_suspend); -static int dwc3_runtime_resume(struct device *dev) +int dwc3_runtime_resume(struct dwc3 *dwc) { - struct dwc3 *dwc = dev_get_drvdata(dev); int ret; ret = dwc3_resume_common(dwc, PMSG_AUTO_RESUME); @@ -2559,15 +2571,14 @@ static int dwc3_runtime_resume(struct device *dev) break; } - pm_runtime_mark_last_busy(dev); + pm_runtime_mark_last_busy(dwc->dev); return 0; } +EXPORT_SYMBOL_GPL(dwc3_runtime_resume); -static int dwc3_runtime_idle(struct device *dev) +int dwc3_runtime_idle(struct dwc3 *dwc) { - struct dwc3 *dwc = dev_get_drvdata(dev); - switch (dwc->current_dr_role) { case DWC3_GCTL_PRTCAP_DEVICE: if (dwc3_runtime_checks(dwc)) @@ -2579,50 +2590,65 @@ static int dwc3_runtime_idle(struct device *dev) break; } - pm_runtime_mark_last_busy(dev); - pm_runtime_autosuspend(dev); + pm_runtime_mark_last_busy(dwc->dev); + pm_runtime_autosuspend(dwc->dev); return 0; } +EXPORT_SYMBOL_GPL(dwc3_runtime_idle); + +static int dwc3_plat_runtime_suspend(struct device *dev) +{ + return dwc3_runtime_suspend(dev_get_drvdata(dev)); +} + +static int dwc3_plat_runtime_resume(struct device *dev) +{ + return dwc3_runtime_resume(dev_get_drvdata(dev)); +} + +static int dwc3_plat_runtime_idle(struct device *dev) +{ + return dwc3_runtime_idle(dev_get_drvdata(dev)); +} #endif /* CONFIG_PM */ #ifdef CONFIG_PM_SLEEP -static int dwc3_suspend(struct device *dev) +int dwc3_suspend(struct dwc3 *dwc) { - struct dwc3 *dwc = dev_get_drvdata(dev); int ret; ret = dwc3_suspend_common(dwc, PMSG_SUSPEND); if (ret) return ret; - pinctrl_pm_select_sleep_state(dev); + pinctrl_pm_select_sleep_state(dwc->dev); return 0; } +EXPORT_SYMBOL_GPL(dwc3_suspend); -static int dwc3_resume(struct device *dev) +int dwc3_resume(struct dwc3 *dwc) { - struct dwc3 *dwc = dev_get_drvdata(dev); int ret = 0; - pinctrl_pm_select_default_state(dev); + pinctrl_pm_select_default_state(dwc->dev); - pm_runtime_disable(dev); - pm_runtime_set_active(dev); + pm_runtime_disable(dwc->dev); + pm_runtime_set_active(dwc->dev); ret = dwc3_resume_common(dwc, PMSG_RESUME); if (ret) - pm_runtime_set_suspended(dev); + pm_runtime_set_suspended(dwc->dev); - pm_runtime_enable(dev); + pm_runtime_enable(dwc->dev); return ret; } +EXPORT_SYMBOL_GPL(dwc3_resume); -static void dwc3_complete(struct device *dev) +void dwc3_complete(struct dwc3 *dwc) { - struct dwc3 *dwc = dev_get_drvdata(dev); u32 reg; if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST && @@ -2632,21 +2658,36 @@ static void dwc3_complete(struct device *dev) dwc3_writel(dwc->regs, DWC3_GUCTL3, reg); } } +EXPORT_SYMBOL_GPL(dwc3_complete); + +static int dwc3_plat_suspend(struct device *dev) +{ + return dwc3_suspend(dev_get_drvdata(dev)); +} + +static int dwc3_plat_resume(struct device *dev) +{ + return dwc3_resume(dev_get_drvdata(dev)); +} + +static void dwc3_plat_complete(struct device *dev) +{ + dwc3_complete(dev_get_drvdata(dev)); +} #else -#define dwc3_complete NULL +#define dwc3_plat_complete NULL #endif /* CONFIG_PM_SLEEP */ static const struct dev_pm_ops dwc3_dev_pm_ops = { - SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume) - .complete = dwc3_complete, - + SET_SYSTEM_SLEEP_PM_OPS(dwc3_plat_suspend, dwc3_plat_resume) + .complete = dwc3_plat_complete, /* * Runtime suspend halts the controller on disconnection. It relies on * platforms with custom connection notification to start the controller * again. */ - SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume, - dwc3_runtime_idle) + SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume, + dwc3_plat_runtime_idle) }; #ifdef CONFIG_OF diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h new file mode 100644 index 000000000000..f6a513e43f53 --- /dev/null +++ b/drivers/usb/dwc3/glue.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * glue.h - DesignWare USB3 DRD glue header + */ + +#ifndef __DRIVERS_USB_DWC3_GLUE_H +#define __DRIVERS_USB_DWC3_GLUE_H + +#include <linux/types.h> +#include "core.h" + +int dwc3_init(struct dwc3 *dwc, struct resource *res); +void dwc3_uninit(struct dwc3 *dwc); + +int dwc3_runtime_suspend(struct dwc3 *dwc); +int dwc3_runtime_resume(struct dwc3 *dwc); +int dwc3_runtime_idle(struct dwc3 *dwc); +int dwc3_suspend(struct dwc3 *dwc); +int dwc3_resume(struct dwc3 *dwc); +void dwc3_complete(struct dwc3 *dwc); + +#endif
The DWC3 IP block is handled by three distinct device drivers: XHCI, DWC3 core and a platform specific (optional) DWC3 glue driver. This has resulted in, at least in the case of the Qualcomm glue, the presence of a number of layering violations, where the glue code either can't handle, or has to work around, the fact that core might not probe deterministically. An example of this is that the suspend path should operate slightly different depending on the device operating in host or peripheral mode, and the only way to determine the operating state is to peek into the core's drvdata. The Qualcomm glue driver is expected to make updates in the qscratch register region (the "glue" region) during role switch events, but with the glue and core split using the driver model, there is no reasonable way to introduce listeners for mode changes. Split the dwc3 core platform_driver callbacks and their implementation and export the implementation, to make it possible to deterministically instantiate the dwc3 core as part of the dwc3 glue drivers and to allow flattening of the DeviceTree representation. Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> --- drivers/usb/dwc3/core.c | 149 ++++++++++++++++++++++++++++++------------------ drivers/usb/dwc3/glue.h | 22 +++++++ 2 files changed, 117 insertions(+), 54 deletions(-)