Re: [PATCH v2 4/7] usb: dwc3: core: Expose core driver as library
From: Masahiro Yamada
Date: Thu Sep 12 2024 - 18:22:28 EST
On Mon, Aug 12, 2024 at 12:07 PM Bjorn Andersson <andersson@xxxxxxxxxx> wrote:
>
> From: Bjorn Andersson <quic_bjorande@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> ---
> 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.
--
Best Regards
Masahiro Yamada
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5b5f6831d9f3..9d1f7db937d0 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2085,23 +2085,14 @@ static int dwc3_get_num_ports(struct dwc3 *dwc)
return 0;
}
-struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res,
- bool ignore_clocks_and_resets, void *glue)
+int dwc3_init(struct dwc3 *dwc, struct resource *res, bool ignore_clocks_and_resets)
{
- struct device *dev = &pdev->dev;
+ struct device *dev = dwc->dev;
struct resource dwc_res;
unsigned int hw_mode;
void __iomem *regs;
- struct dwc3 *dwc;
int ret;
- dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
- if (!dwc)
- return ERR_PTR(-ENOMEM);
-
- dwc->dev = dev;
- dwc->glue = glue;
-
dwc->xhci_resources[0].start = res->start;
dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
DWC3_XHCI_REGS_END;
@@ -2163,7 +2154,7 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev, struct resource *res,
goto err_disable_clks;
}
- platform_set_drvdata(pdev, dwc);
+ dev_set_drvdata(dev, dwc);
dwc3_cache_hwparams(dwc);
if (!dwc->sysdev_is_parent &&
@@ -2271,16 +2262,20 @@ static int dwc3_plat_probe(struct platform_device *pdev)
return -ENODEV;
}
- dwc = dwc3_probe(pdev, res, false, NULL);
- if (IS_ERR(dwc))
- return PTR_ERR(dwc);
+ dwc = devm_kzalloc(&pdev->dev, sizeof(*dwc), GFP_KERNEL);
+ if (!dwc)
+ return ERR_PTR(-ENOMEM);
- platform_set_drvdata(pdev, dwc);
+ dwc->dev = &pdev->dev;
+
+ ret = dwc3_init(dwc, res, false);
+ if (ret)
+ return ret;
return 0;
}
-void dwc3_remove(struct dwc3 *dwc)
+void dwc3_uninit(struct dwc3 *dwc)
{
pm_runtime_get_sync(dwc->dev);
@@ -2306,11 +2301,11 @@ void dwc3_remove(struct dwc3 *dwc)
if (dwc->usb_psy)
power_supply_put(dwc->usb_psy);
}
-EXPORT_SYMBOL_GPL(dwc3_remove);
+EXPORT_SYMBOL_GPL(dwc3_uninit);
-static void dwc3_plat_remove(struct platform_device *pdev)
+static void dwc3_remove(struct platform_device *pdev)
{
- dwc3_remove(platform_get_drvdata(pdev));
+ dwc3_uninit(platform_get_drvdata(pdev));
}
#ifdef CONFIG_PM
@@ -2656,8 +2651,8 @@ MODULE_DEVICE_TABLE(acpi, dwc3_acpi_match);
#endif
static struct platform_driver dwc3_driver = {
- .probe = dwc3_plat_probe,
- .remove_new = dwc3_plat_remove,
+ .probe = dwc3_probe,
+ .remove_new = dwc3_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 4a0ee9ef72e2..1e561fd8b86e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1160,7 +1160,6 @@ 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;
@@ -1389,8 +1388,6 @@ struct dwc3 {
int num_ep_resized;
struct dentry *debug_root;
u32 gsbuscfg0_reqinfo;
-
- void *glue;
};
#define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 27b5013cd411..e9088a8873a8 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -76,7 +76,7 @@ struct dwc3_qcom_port {
struct dwc3_qcom {
struct device *dev;
void __iomem *qscratch_base;
- struct dwc3 *dwc;
+ struct dwc3 dwc;
struct clk **clks;
int num_clocks;
struct reset_control *resets;
@@ -964,9 +964,11 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ignore_pipe_clk)
dwc3_qcom_select_utmi_clk(qcom);
- qcom->dwc = dwc3_probe(pdev, &res, true, qcom);
- if (IS_ERR(qcom->dwc)) {
- ret = dev_err_probe(dev, PTR_ERR(qcom->dwc), "failed to register DWC3 Core\n");
+ qcom->dwc.dev = dev;
+
+ ret = dwc3_init(&qcom->dwc, &res, true);
+ if (ret) {
+ ret = dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
goto clk_disable;
}
@@ -995,7 +997,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
interconnect_exit:
dwc3_qcom_interconnect_exit(qcom);
remove_core:
- dwc3_remove(qcom->dwc);
+ dwc3_uninit(qcom->dwc);
clk_disable:
for (i = qcom->num_clocks - 1; i >= 0; i--) {
clk_disable_unprepare(qcom->clks[i]);
@@ -1010,7 +1012,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
static void dwc3_qcom_remove(struct platform_device *pdev)
{
struct dwc3 *dwc = platform_get_drvdata(pdev);
- struct dwc3_qcom *qcom = dwc->glue;
+ struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
struct device *dev = &pdev->dev;
int i;
@@ -1030,7 +1032,7 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
{
struct dwc3 *dwc = dev_get_drvdata(dev);
- struct dwc3_qcom *qcom = dwc->glue;
+ struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
bool wakeup = device_may_wakeup(dev);
int ret;
@@ -1050,7 +1052,7 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
{
struct dwc3 *dwc = dev_get_drvdata(dev);
- struct dwc3_qcom *qcom = dwc->glue;
+ struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
bool wakeup = device_may_wakeup(dev);
int ret;
@@ -1070,7 +1072,7 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
{
struct dwc3 *dwc = dev_get_drvdata(dev);
- struct dwc3_qcom *qcom = dwc->glue;
+ struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
int ret;
ret = dwc3_runtime_suspend(qcom->dwc);
@@ -1090,7 +1092,7 @@ static void __maybe_unused dwc3_qcom_complete(struct device *dev)
static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
{
struct dwc3 *dwc = dev_get_drvdata(dev);
- struct dwc3_qcom *qcom = dwc->glue;
+ struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
int ret;
ret = dwc3_qcom_resume(qcom, true);