| Message ID | 20240811-dwc3-refactor-v2-4-91f370d61ad2@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
Hi Bjorn, kernel test robot noticed the following build errors: [auto build test ERROR on 864b1099d16fc7e332c3ad7823058c65f890486c] url: https://github.com/intel-lab-lkp/linux/commits/Bjorn-Andersson/dt-bindings-usb-snps-dwc3-Split-core-description/20240812-111102 base: 864b1099d16fc7e332c3ad7823058c65f890486c patch link: https://lore.kernel.org/r/20240811-dwc3-refactor-v2-4-91f370d61ad2%40quicinc.com patch subject: [PATCH v2 4/7] usb: dwc3: core: Expose core driver as library config: i386-buildonly-randconfig-005-20240812 (https://download.01.org/0day-ci/archive/20240812/202408122011.adSQGOUp-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240812/202408122011.adSQGOUp-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202408122011.adSQGOUp-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/usb/dwc3/core.c:39:10: fatal error: glue.h: No such file or directory 39 | #include "glue.h" | ^~~~~~~~ compilation terminated. vim +39 drivers/usb/dwc3/core.c 36 37 #include "core.h" 38 #include "gadget.h" > 39 #include "glue.h" 40 #include "io.h" 41
On Sun, Aug 11, 2024 at 08:12:01PM -0700, Bjorn Andersson wrote: > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > 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 <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/core.c | 169 +++++++++++++++++++++++++++++++----------------- > drivers/usb/dwc3/core.h | 3 + > 2 files changed, 114 insertions(+), 58 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 734de2a8bd21..6addb3c367e6 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" > @@ -2076,10 +2077,11 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) > return 0; > } > > -static int dwc3_probe(struct platform_device *pdev) > +struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res, > + bool ignore_clocks_and_resets, void *glue) > { > struct device *dev = &pdev->dev; > - struct resource *res, dwc_res; > + struct resource dwc_res; > unsigned int hw_mode; > void __iomem *regs; > struct dwc3 *dwc; > @@ -2087,15 +2089,10 @@ 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; > - } > + dwc->glue = glue; > > dwc->xhci_resources[0].start = res->start; > dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > @@ -2123,7 +2120,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); > @@ -2132,15 +2129,17 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc3_get_software_properties(dwc); > > - dwc->reset = devm_reset_control_array_get_optional_shared(dev); > - if (IS_ERR(dwc->reset)) { > - ret = PTR_ERR(dwc->reset); > - goto err_put_psy; > - } > + if (!ignore_clocks_and_resets) { > + dwc->reset = devm_reset_control_array_get_optional_shared(dev); > + if (IS_ERR(dwc->reset)) { > + ret = PTR_ERR(dwc->reset); > + goto err_put_psy; > + } > > - ret = dwc3_get_clocks(dwc); > - if (ret) > - goto err_put_psy; > + ret = dwc3_get_clocks(dwc); > + if (ret) > + goto err_put_psy; > + } > > ret = reset_control_deassert(dwc->reset); > if (ret) > @@ -2225,7 +2224,7 @@ static int dwc3_probe(struct platform_device *pdev) > > dma_set_max_seg_size(dev, UINT_MAX); > > - return 0; > + return dwc; > > err_exit_debugfs: > dwc3_debugfs_exit(dwc); > @@ -2249,14 +2248,33 @@ 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 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; > + } > > - pm_runtime_get_sync(&pdev->dev); > + dwc = dwc3_probe(pdev, res, false, NULL); > + if (IS_ERR(dwc)) > + return PTR_ERR(dwc); > + > + platform_set_drvdata(pdev, dwc); > + > + return 0; > +} > + > +void dwc3_remove(struct dwc3 *dwc) > +{ > + pm_runtime_get_sync(dwc->dev); > > dwc3_core_exit_mode(dwc); > dwc3_debugfs_exit(dwc); > @@ -2264,22 +2282,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) > @@ -2450,9 +2474,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)) > @@ -2464,10 +2487,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); > @@ -2484,15 +2507,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)) > @@ -2504,52 +2526,67 @@ 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); > > - 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); > return ret; > } > > - pm_runtime_enable(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 && > @@ -2559,15 +2596,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) since you touch this line, suggest use new SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() help macro. also CONFIG_PM_SLEEP can be removed. Frank > + .complete = dwc3_plat_complete, > + SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume, > + dwc3_plat_runtime_idle) > }; > > #ifdef CONFIG_OF > @@ -2595,8 +2648,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 1e561fd8b86e..4a0ee9ef72e2 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1160,6 +1160,7 @@ struct dwc3_scratchpad_array { > * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO, > * DATWRREQINFO, and DESWRREQINFO value passed from > * glue driver. > + * @glue: private reference to any glue context > */ > struct dwc3 { > struct work_struct drd_work; > @@ -1388,6 +1389,8 @@ struct dwc3 { > int num_ep_resized; > struct dentry *debug_root; > u32 gsbuscfg0_reqinfo; > + > + void *glue; > }; > > #define INCRX_BURST_MODE 0 > > -- > 2.45.2 >
On Sun, Aug 11, 2024, Bjorn Andersson wrote: > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > 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 <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/core.c | 169 +++++++++++++++++++++++++++++++----------------- > drivers/usb/dwc3/core.h | 3 + > 2 files changed, 114 insertions(+), 58 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 734de2a8bd21..6addb3c367e6 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" > @@ -2076,10 +2077,11 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) > return 0; > } > > -static int dwc3_probe(struct platform_device *pdev) > +struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res, > + bool ignore_clocks_and_resets, void *glue) Perhaps create a wrapper struct to hold the parameters above. I can see we may expand this in the future. > { > struct device *dev = &pdev->dev; > - struct resource *res, dwc_res; > + struct resource dwc_res; > unsigned int hw_mode; > void __iomem *regs; > struct dwc3 *dwc; > @@ -2087,15 +2089,10 @@ 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; > - } > + dwc->glue = glue; > > dwc->xhci_resources[0].start = res->start; > dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > @@ -2123,7 +2120,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); > @@ -2132,15 +2129,17 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc3_get_software_properties(dwc); > > - dwc->reset = devm_reset_control_array_get_optional_shared(dev); > - if (IS_ERR(dwc->reset)) { > - ret = PTR_ERR(dwc->reset); > - goto err_put_psy; > - } > + if (!ignore_clocks_and_resets) { This seems to be specific change for your platform. Let's keep this change separated from this patch. Thanks, Thinh > + dwc->reset = devm_reset_control_array_get_optional_shared(dev); > + if (IS_ERR(dwc->reset)) { > + ret = PTR_ERR(dwc->reset); > + goto err_put_psy; > + } > > - ret = dwc3_get_clocks(dwc); > - if (ret) > - goto err_put_psy; > + ret = dwc3_get_clocks(dwc); > + if (ret) > + goto err_put_psy; > + } > > ret = reset_control_deassert(dwc->reset); > if (ret) > @@ -2225,7 +2224,7 @@ static int dwc3_probe(struct platform_device *pdev) > > dma_set_max_seg_size(dev, UINT_MAX); > > - return 0; > + return dwc; > > err_exit_debugfs: > dwc3_debugfs_exit(dwc); > @@ -2249,14 +2248,33 @@ 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 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; > + } > > - pm_runtime_get_sync(&pdev->dev); > + dwc = dwc3_probe(pdev, res, false, NULL); > + if (IS_ERR(dwc)) > + return PTR_ERR(dwc); > + > + platform_set_drvdata(pdev, dwc); > + > + return 0; > +} > + > +void dwc3_remove(struct dwc3 *dwc) > +{ > + pm_runtime_get_sync(dwc->dev); > > dwc3_core_exit_mode(dwc); > dwc3_debugfs_exit(dwc); > @@ -2264,22 +2282,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) > @@ -2450,9 +2474,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)) > @@ -2464,10 +2487,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); > @@ -2484,15 +2507,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)) > @@ -2504,52 +2526,67 @@ 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); > > - 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); > return ret; > } > > - pm_runtime_enable(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 && > @@ -2559,15 +2596,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 > @@ -2595,8 +2648,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 1e561fd8b86e..4a0ee9ef72e2 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1160,6 +1160,7 @@ struct dwc3_scratchpad_array { > * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO, > * DATWRREQINFO, and DESWRREQINFO value passed from > * glue driver. > + * @glue: private reference to any glue context > */ > struct dwc3 { > struct work_struct drd_work; > @@ -1388,6 +1389,8 @@ struct dwc3 { > int num_ep_resized; > struct dentry *debug_root; > u32 gsbuscfg0_reqinfo; > + > + void *glue; > }; > > #define INCRX_BURST_MODE 0 > > -- > 2.45.2 >
On Tue, Aug 13, 2024, Frank Li wrote: > On Sun, Aug 11, 2024 at 08:12:01PM -0700, Bjorn Andersson wrote: > > From: Bjorn Andersson <quic_bjorande@quicinc.com> ... > > +{ > > + 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) > > since you touch this line, > suggest use new SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() help macro. > also CONFIG_PM_SLEEP can be removed. > If we want to make that change, please keep that change separate from this patch/series. Thanks, Thinh
On 8/12/2024 8:42 AM, Bjorn Andersson wrote: > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > 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 <quic_bjorande@quicinc.com> ... > -static int dwc3_probe(struct platform_device *pdev) > +struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res, > + bool ignore_clocks_and_resets, void *glue) > { > struct device *dev = &pdev->dev; > - struct resource *res, dwc_res; > + struct resource dwc_res; > unsigned int hw_mode; > void __iomem *regs; > struct dwc3 *dwc; > @@ -2087,15 +2089,10 @@ 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; > - } ... > +static int dwc3_plat_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; > + } > > - pm_runtime_get_sync(&pdev->dev); > + dwc = dwc3_probe(pdev, res, false, NULL); > + if (IS_ERR(dwc)) > + return PTR_ERR(dwc); > + > + platform_set_drvdata(pdev, dwc); This setting of platform drvdata is redundant I believe. We already do it in dwc3_probe. > + > + return 0; > +} > +
On Wed, Aug 14, 2024 at 12:56:44AM +0000, Thinh Nguyen wrote: > On Sun, Aug 11, 2024, Bjorn Andersson wrote: > > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c [..] > > @@ -2076,10 +2077,11 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) > > return 0; > > } > > > > -static int dwc3_probe(struct platform_device *pdev) > > +struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res, > > + bool ignore_clocks_and_resets, void *glue) > > Perhaps create a wrapper struct to hold the parameters above. I can see > we may expand this in the future. > Sounds good. There are a few cases where e.g. the Qualcomm glue needs to react to role switching in the core, so we already know that we want to add some callbacks here. > > { > > struct device *dev = &pdev->dev; > > - struct resource *res, dwc_res; > > + struct resource dwc_res; > > unsigned int hw_mode; > > void __iomem *regs; > > struct dwc3 *dwc; > > @@ -2087,15 +2089,10 @@ 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; > > - } > > + dwc->glue = glue; > > > > dwc->xhci_resources[0].start = res->start; > > dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > > @@ -2123,7 +2120,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); > > @@ -2132,15 +2129,17 @@ static int dwc3_probe(struct platform_device *pdev) > > > > dwc3_get_software_properties(dwc); > > > > - dwc->reset = devm_reset_control_array_get_optional_shared(dev); > > - if (IS_ERR(dwc->reset)) { > > - ret = PTR_ERR(dwc->reset); > > - goto err_put_psy; > > - } > > + if (!ignore_clocks_and_resets) { > > This seems to be specific change for your platform. Let's keep this > change separated from this patch. > The primary need here is that the glue code needs to be able to access the hardware before the core's resume and after suspend. I'd expect other glue implementations will have the same need. But moving this change to a separate change allows us to reason about that separately, so I like the suggestion. Thanks, Bjorn > Thanks, > Thinh >
On Tue, Aug 13, 2024 at 02:15:35PM -0400, Frank Li wrote: > On Sun, Aug 11, 2024 at 08:12:01PM -0700, Bjorn Andersson wrote: > > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c [..] > > 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) > > since you touch this line, > suggest use new SYSTEM_SLEEP_PM_OPS() and RUNTIME_PM_OPS() help macro. > also CONFIG_PM_SLEEP can be removed. > I'd be happy to follow up with such cleanups after we've concluded this, I did spot a few other things that would benefit from some TLC. Regards, Bjorn
On Mon, Aug 19, 2024 at 11:18:15PM +0530, Krishna Kurapati wrote: > > > On 8/12/2024 8:42 AM, Bjorn Andersson wrote: > > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > > > 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 <quic_bjorande@quicinc.com> > > ... > > > -static int dwc3_probe(struct platform_device *pdev) > > +struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res, > > + bool ignore_clocks_and_resets, void *glue) > > { > > struct device *dev = &pdev->dev; > > - struct resource *res, dwc_res; > > + struct resource dwc_res; > > unsigned int hw_mode; > > void __iomem *regs; > > struct dwc3 *dwc; > > @@ -2087,15 +2089,10 @@ 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; > > - } > > ... > > > +static int dwc3_plat_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; > > + } > > - pm_runtime_get_sync(&pdev->dev); > > + dwc = dwc3_probe(pdev, res, false, NULL); > > + if (IS_ERR(dwc)) > > + return PTR_ERR(dwc); > > + > > + platform_set_drvdata(pdev, dwc); > > This setting of platform drvdata is redundant I believe. We already do it in > dwc3_probe. > Good catch, you're certainly right. Regards, Bjorn > > + > > + return 0; > > +} > > +
On Mon, Aug 12, 2024 at 12:07 PM Bjorn Andersson <andersson@kernel.org> wrote: > > From: Bjorn Andersson <quic_bjorande@quicinc.com> > > 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 <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/core.c | 169 +++++++++++++++++++++++++++++++----------------- > drivers/usb/dwc3/core.h | 3 + > 2 files changed, 114 insertions(+), 58 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 734de2a8bd21..6addb3c367e6 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" > @@ -2076,10 +2077,11 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) > return 0; > } > > -static int dwc3_probe(struct platform_device *pdev) > +struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res, > + bool ignore_clocks_and_resets, void *glue) > { > struct device *dev = &pdev->dev; > - struct resource *res, dwc_res; > + struct resource dwc_res; > unsigned int hw_mode; > void __iomem *regs; > struct dwc3 *dwc; > @@ -2087,15 +2089,10 @@ 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; > - } > + dwc->glue = glue; > > dwc->xhci_resources[0].start = res->start; > dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + > @@ -2123,7 +2120,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); > @@ -2132,15 +2129,17 @@ static int dwc3_probe(struct platform_device *pdev) > > dwc3_get_software_properties(dwc); > > - dwc->reset = devm_reset_control_array_get_optional_shared(dev); > - if (IS_ERR(dwc->reset)) { > - ret = PTR_ERR(dwc->reset); > - goto err_put_psy; > - } > + if (!ignore_clocks_and_resets) { > + dwc->reset = devm_reset_control_array_get_optional_shared(dev); > + if (IS_ERR(dwc->reset)) { > + ret = PTR_ERR(dwc->reset); > + goto err_put_psy; > + } > > - ret = dwc3_get_clocks(dwc); > - if (ret) > - goto err_put_psy; > + ret = dwc3_get_clocks(dwc); > + if (ret) > + goto err_put_psy; > + } > > ret = reset_control_deassert(dwc->reset); > if (ret) > @@ -2225,7 +2224,7 @@ static int dwc3_probe(struct platform_device *pdev) > > dma_set_max_seg_size(dev, UINT_MAX); > > - return 0; > + return dwc; > > err_exit_debugfs: > dwc3_debugfs_exit(dwc); > @@ -2249,14 +2248,33 @@ 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 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; > + } > > - pm_runtime_get_sync(&pdev->dev); > + dwc = dwc3_probe(pdev, res, false, NULL); > + if (IS_ERR(dwc)) > + return PTR_ERR(dwc); > + > + platform_set_drvdata(pdev, dwc); > + > + return 0; > +} > + > +void dwc3_remove(struct dwc3 *dwc) > +{ > + pm_runtime_get_sync(dwc->dev); > > dwc3_core_exit_mode(dwc); > dwc3_debugfs_exit(dwc); > @@ -2264,22 +2282,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) > @@ -2450,9 +2474,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)) > @@ -2464,10 +2487,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); > @@ -2484,15 +2507,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)) > @@ -2504,52 +2526,67 @@ 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); > > - 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); > return ret; > } > > - pm_runtime_enable(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 && > @@ -2559,15 +2596,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 > @@ -2595,8 +2648,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 1e561fd8b86e..4a0ee9ef72e2 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1160,6 +1160,7 @@ struct dwc3_scratchpad_array { > * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO, > * DATWRREQINFO, and DESWRREQINFO value passed from > * glue driver. > + * @glue: private reference to any glue context > */ > struct dwc3 { > struct work_struct drd_work; > @@ -1388,6 +1389,8 @@ struct dwc3 { > int num_ep_resized; > struct dentry *debug_root; > u32 gsbuscfg0_reqinfo; > + > + void *glue; This is ugly. 'struct dwc3_qcom' can wrap 'struct dwc3' instead of having two separate structures pointing at each other. You can use container_of() to convert (struct dwc3 *) to (struct dwc3_qcom *). I attached a diff to delete the reverse ->glue pointer. I did not compile-test it (I cannot due to missing glue.h anyway), but you will understand my suggestion.
On Fri, Sep 13, 2024 at 07:21:35AM +0900, Masahiro Yamada wrote: > On Mon, Aug 12, 2024 at 12:07 PM Bjorn Andersson <andersson@kernel.org> wrote: [..] > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 1e561fd8b86e..4a0ee9ef72e2 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -1160,6 +1160,7 @@ struct dwc3_scratchpad_array { > > * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO, > > * DATWRREQINFO, and DESWRREQINFO value passed from > > * glue driver. > > + * @glue: private reference to any glue context > > */ > > struct dwc3 { > > struct work_struct drd_work; > > @@ -1388,6 +1389,8 @@ struct dwc3 { > > int num_ep_resized; > > struct dentry *debug_root; > > u32 gsbuscfg0_reqinfo; > > + > > + void *glue; > > > This is ugly. > > > 'struct dwc3_qcom' can wrap 'struct dwc3' > instead of having two separate structures > pointing at each other. > > > You can use container_of() to > convert (struct dwc3 *) to (struct dwc3_qcom *). > > > I attached a diff to delete the reverse ->glue pointer. > > I did not compile-test it (I cannot due to missing glue.h > anyway), but you will understand my suggestion. > Thanks for your suggestion, Yamada-san. I agree, that will look better. Regards, Bjorn
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 734de2a8bd21..6addb3c367e6 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" @@ -2076,10 +2077,11 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) return 0; } -static int dwc3_probe(struct platform_device *pdev) +struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res, + bool ignore_clocks_and_resets, void *glue) { struct device *dev = &pdev->dev; - struct resource *res, dwc_res; + struct resource dwc_res; unsigned int hw_mode; void __iomem *regs; struct dwc3 *dwc; @@ -2087,15 +2089,10 @@ 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; - } + dwc->glue = glue; dwc->xhci_resources[0].start = res->start; dwc->xhci_resources[0].end = dwc->xhci_resources[0].start + @@ -2123,7 +2120,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); @@ -2132,15 +2129,17 @@ static int dwc3_probe(struct platform_device *pdev) dwc3_get_software_properties(dwc); - dwc->reset = devm_reset_control_array_get_optional_shared(dev); - if (IS_ERR(dwc->reset)) { - ret = PTR_ERR(dwc->reset); - goto err_put_psy; - } + if (!ignore_clocks_and_resets) { + dwc->reset = devm_reset_control_array_get_optional_shared(dev); + if (IS_ERR(dwc->reset)) { + ret = PTR_ERR(dwc->reset); + goto err_put_psy; + } - ret = dwc3_get_clocks(dwc); - if (ret) - goto err_put_psy; + ret = dwc3_get_clocks(dwc); + if (ret) + goto err_put_psy; + } ret = reset_control_deassert(dwc->reset); if (ret) @@ -2225,7 +2224,7 @@ static int dwc3_probe(struct platform_device *pdev) dma_set_max_seg_size(dev, UINT_MAX); - return 0; + return dwc; err_exit_debugfs: dwc3_debugfs_exit(dwc); @@ -2249,14 +2248,33 @@ 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 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; + } - pm_runtime_get_sync(&pdev->dev); + dwc = dwc3_probe(pdev, res, false, NULL); + if (IS_ERR(dwc)) + return PTR_ERR(dwc); + + platform_set_drvdata(pdev, dwc); + + return 0; +} + +void dwc3_remove(struct dwc3 *dwc) +{ + pm_runtime_get_sync(dwc->dev); dwc3_core_exit_mode(dwc); dwc3_debugfs_exit(dwc); @@ -2264,22 +2282,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) @@ -2450,9 +2474,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)) @@ -2464,10 +2487,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); @@ -2484,15 +2507,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)) @@ -2504,52 +2526,67 @@ 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); - 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); return ret; } - pm_runtime_enable(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 && @@ -2559,15 +2596,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 @@ -2595,8 +2648,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 1e561fd8b86e..4a0ee9ef72e2 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1160,6 +1160,7 @@ struct dwc3_scratchpad_array { * @gsbuscfg0_reqinfo: store GSBUSCFG0.DATRDREQINFO, DESRDREQINFO, * DATWRREQINFO, and DESWRREQINFO value passed from * glue driver. + * @glue: private reference to any glue context */ struct dwc3 { struct work_struct drd_work; @@ -1388,6 +1389,8 @@ struct dwc3 { int num_ep_resized; struct dentry *debug_root; u32 gsbuscfg0_reqinfo; + + void *glue; }; #define INCRX_BURST_MODE 0