| Message ID | 20231016-dwc3-refactor-v1-4-ab4a84165470@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
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@quicinc.com> > --- > 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 >
On Mon, Oct 16, 2023 at 08:11:12PM -0700, 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@quicinc.com> > --- > 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) Perhaps you should move allocation of struct dwc3 to the caller as well as you are going to need some way to pass in callback to core which need to be set before you register the xhci platform device. There could be other ways, like passing in a struct of callbacks, which can be added incrementally but it may be good think this through from the start. > { > 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); This is broken however as the pm ops access the data driver data and can be called as soon as you call pm_runtime_put() below. > 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); That is here. > - return 0; > + return dwc; > -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); And that leaves a window, for example, here where you can hit a NULL pointer dereference. > + platform_set_drvdata(pdev, dwc); > + > + return 0; > +} Johan
On Wed, Nov 22, 2023 at 12:57:37PM +0100, Johan Hovold wrote: > On Mon, Oct 16, 2023 at 08:11:12PM -0700, 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@quicinc.com> > > --- > > 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) > > Perhaps you should move allocation of struct dwc3 to the caller as well > as you are going to need some way to pass in callback to core which need > to be set before you register the xhci platform device. > > There could be other ways, like passing in a struct of callbacks, which > can be added incrementally but it may be good think this through from > the start. > My intention was to have callbacks and quirks passed through additional arguments in an incremental patch. IMHO passing such information through a pre-allocated and partially initialized struct is more obscure than passing that information as explicit parameters to the function... > > { > > 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); > > This is broken however as the pm ops access the data driver data and can > be called as soon as you call pm_runtime_put() below. > You're right, thanks for spotting that. Regards, Bjorn > > 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); > > That is here. > > > - return 0; > > + return dwc; > > > -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); > > And that leaves a window, for example, here where you can hit a NULL > pointer dereference. > > > + platform_set_drvdata(pdev, dwc); > > + > > + return 0; > > +} > > Johan >
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); + #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);
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@quicinc.com> --- drivers/usb/dwc3/core.c | 134 ++++++++++++++++++++++++++++++++---------------- drivers/usb/dwc3/core.h | 10 ++++ 2 files changed, 100 insertions(+), 44 deletions(-)