On Mon, Oct 16, 2023, 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 platfrom_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 <quic_bjorande@xxxxxxxxxxx> > --- > drivers/usb/dwc3/core.c | 134 ++++++++++++++++++++++++++++++++---------------- > drivers/usb/dwc3/core.h | 10 ++++ > 2 files changed, 100 insertions(+), 44 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index d25490965b27..71e376bebb16 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > return 0; > } > > -static int dwc3_probe(struct platform_device *pdev) > +struct dwc3 *dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct resource *res, dwc_res; > @@ -1886,14 +1886,14 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > if (!dwc) > - return -ENOMEM; > + return ERR_PTR(-ENOMEM); > > dwc->dev = dev; > > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > dev_err(dev, "missing memory resource\n"); > - return -ENODEV; > + return ERR_PTR(-ENODEV); > } > > dwc->xhci_resources[0].start = res->start; > @@ -1922,7 +1922,7 @@ static int dwc3_probe(struct platform_device *pdev) > > regs = devm_ioremap_resource(dev, &dwc_res); > if (IS_ERR(regs)) > - return PTR_ERR(regs); > + return ERR_CAST(regs); > > dwc->regs = regs; > dwc->regs_size = resource_size(&dwc_res); > @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > - platform_set_drvdata(pdev, dwc); > dwc3_cache_hwparams(dwc); > > if (!dwc->sysdev_is_parent && > @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev) > > pm_runtime_put(dev); > > - return 0; > + return dwc; > > err_exit_debugfs: > dwc3_debugfs_exit(dwc); > @@ -2030,14 +2029,26 @@ static int dwc3_probe(struct platform_device *pdev) > if (dwc->usb_psy) > power_supply_put(dwc->usb_psy); > > - return ret; > + return ERR_PTR(ret); > } > +EXPORT_SYMBOL_GPL(dwc3_probe); > > -static void dwc3_remove(struct platform_device *pdev) > +static int dwc3_plat_probe(struct platform_device *pdev) > { > - struct dwc3 *dwc = platform_get_drvdata(pdev); > + struct dwc3 *dwc; > + > + dwc = dwc3_probe(pdev); > + if (IS_ERR(dwc)) > + return PTR_ERR(dwc); > + > + platform_set_drvdata(pdev, dwc); > + > + return 0; > +} > > - pm_runtime_get_sync(&pdev->dev); > +void dwc3_remove(struct dwc3 *dwc) > +{ > + pm_runtime_get_sync(dwc->dev); > > dwc3_core_exit_mode(dwc); > dwc3_debugfs_exit(dwc); > @@ -2045,22 +2056,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_remove); > + > +static void dwc3_plat_remove(struct platform_device *pdev) > +{ > + dwc3_remove(platform_get_drvdata(pdev)); > +} > > #ifdef CONFIG_PM > static int dwc3_core_init_for_resume(struct dwc3 *dwc) > @@ -2227,9 +2244,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)) > @@ -2241,10 +2257,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); > @@ -2261,15 +2277,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)) > @@ -2281,49 +2296,64 @@ 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; > > - pinctrl_pm_select_default_state(dev); > + pinctrl_pm_select_default_state(dwc->dev); > > ret = dwc3_resume_common(dwc, PMSG_RESUME); > if (ret) > return ret; > > - pm_runtime_disable(dev); > - pm_runtime_set_active(dev); > - pm_runtime_enable(dev); > + pm_runtime_disable(dwc->dev); > + pm_runtime_set_active(dwc->dev); > + pm_runtime_enable(dwc->dev); > > return 0; > } > +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 && > @@ -2333,15 +2363,31 @@ 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_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume, > - dwc3_runtime_idle) > + SET_SYSTEM_SLEEP_PM_OPS(dwc3_plat_suspend, dwc3_plat_resume) > + .complete = dwc3_plat_complete, > + SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume, > + dwc3_plat_runtime_idle) > }; > > #ifdef CONFIG_OF > @@ -2369,8 +2415,8 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match); > #endif > > static struct platform_driver dwc3_driver = { > - .probe = dwc3_probe, > - .remove_new = dwc3_remove, > + .probe = dwc3_plat_probe, > + .remove_new = dwc3_plat_remove, > .driver = { > .name = "dwc3", > .of_match_table = of_match_ptr(of_dwc3_match), > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index c6c87acbd376..f5e22559bb73 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1568,6 +1568,16 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc); > > int dwc3_core_soft_reset(struct dwc3 *dwc); > > +struct dwc3 *dwc3_probe(struct platform_device *pdev); > +void dwc3_remove(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); > + Can we make this new interface clear? It's being buried under core.h this way. Perhaps add some comments and divider, or move to another header file? Thanks, Thinh > #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) > int dwc3_host_init(struct dwc3 *dwc); > void dwc3_host_exit(struct dwc3 *dwc); > > -- > 2.25.1 >