lkml.org 
[lkml]   [2023]   [Oct]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 04/12] usb: dwc3: Expose core driver as library
    Date
    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
    >
    \
     
     \ /
      Last update: 2023-10-21 00:19    [W:4.640 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site