Message-ID: <Z8C6V8SADaUr0n+n@lizhi-Precision-Tower-5810>
Date: Thu, 27 Feb 2025 14:17:43 -0500
From: Frank Li <Frank.li@....com>
To: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Felipe Balbi <balbi@...nel.org>,
	Wesley Cheng <quic_wcheng@...cinc.com>,
	Saravana Kannan <saravanak@...gle.com>,
	Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Bjorn Andersson <andersson@...nel.org>,
	Konrad Dybcio <konradybcio@...nel.org>,
	linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 2/7] usb: dwc3: core: Expose core driver as library

On Wed, Feb 26, 2025 at 04:17:49PM -0800, 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 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 <bjorn.andersson@....qualcomm.com>
> ---
>  drivers/usb/dwc3/core.c | 155 +++++++++++++++++++++++++++++++-----------------
>  drivers/usb/dwc3/glue.h |  32 ++++++++++
>  2 files changed, 133 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index dfa1b5fe48dc..d9f0a6782d36 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"
> @@ -2137,27 +2138,16 @@ static struct power_supply *dwc3_get_usb_power_supply(struct dwc3 *dwc)
>  	return usb_psy;
>  }
>
> -static int dwc3_probe(struct platform_device *pdev)
> +int dwc3_core_probe(const struct dwc3_probe_data *data)
>  {
> -	struct device		*dev = &pdev->dev;
> -	struct resource		*res, dwc_res;
> +	struct dwc3		*dwc = data->dwc;
> +	struct device		*dev = dwc->dev;
> +	struct resource		dwc_res;
>  	unsigned int		hw_mode;
>  	void __iomem		*regs;
> -	struct dwc3		*dwc;
> +	struct resource		*res = data->res;
>  	int			ret;
>
> -	dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> -	if (!dwc)
> -		return -ENOMEM;
> -
> -	dwc->dev = dev;
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		dev_err(dev, "missing memory resource\n");
> -		return -ENODEV;
> -	}
> -
>  	dwc->xhci_resources[0].start = res->start;
>  	dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
>  					DWC3_XHCI_REGS_END;
> @@ -2221,7 +2211,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  		goto err_disable_clks;
>  	}
>
> -	platform_set_drvdata(pdev, dwc);
> +	dev_set_drvdata(dev, dwc);
>  	dwc3_cache_hwparams(dwc);
>
>  	if (!dwc->sysdev_is_parent &&
> @@ -2316,12 +2306,35 @@ static int dwc3_probe(struct platform_device *pdev)
>
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(dwc3_core_probe);
>
> -static void dwc3_remove(struct platform_device *pdev)
> +static int dwc3_probe(struct platform_device *pdev)
>  {
> -	struct dwc3	*dwc = platform_get_drvdata(pdev);
> +	struct dwc3_probe_data probe_data;
> +	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;
> +	}
> +
> +	dwc = devm_kzalloc(&pdev->dev, sizeof(*dwc), GFP_KERNEL);
> +	if (!dwc)
> +		return -ENOMEM;
>
> -	pm_runtime_get_sync(&pdev->dev);
> +	dwc->dev = &pdev->dev;
> +
> +	probe_data.dwc = dwc;
> +	probe_data.res = res;
> +
> +	return dwc3_core_probe(&probe_data);
> +}
> +
> +void dwc3_core_remove(struct dwc3 *dwc)
> +{
> +	pm_runtime_get_sync(dwc->dev);
>
>  	dwc3_core_exit_mode(dwc);
>  	dwc3_debugfs_exit(dwc);
> @@ -2329,22 +2342,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_core_remove);
> +
> +static void dwc3_remove(struct platform_device *pdev)
> +{
> +	dwc3_core_remove(platform_get_drvdata(pdev));
> +}
>
>  #ifdef CONFIG_PM
>  static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> @@ -2533,9 +2552,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))
> @@ -2547,10 +2565,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);
> @@ -2571,15 +2589,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))
> @@ -2591,53 +2608,68 @@ 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_pm_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_pm_suspend);
>
> -static int dwc3_resume(struct device *dev)
> +int dwc3_pm_resume(struct dwc3 *dwc)
>  {
> -	struct dwc3	*dwc = dev_get_drvdata(dev);

Can you add varible dev to reduce difference.

	struct device *dev = dwc->dev;

also for other pm functions.

Frank
>  	int		ret = 0;
>
> -	pinctrl_pm_select_default_state(dev);
> +	pinctrl_pm_select_default_state(dwc->dev);
>
> -	pm_runtime_disable(dev);
> -	ret = pm_runtime_set_active(dev);
> +	pm_runtime_disable(dwc->dev);
> +	ret = pm_runtime_set_active(dwc->dev);
>  	if (ret)
>  		goto out;
>
>  	ret = dwc3_resume_common(dwc, PMSG_RESUME);
>  	if (ret)
> -		pm_runtime_set_suspended(dev);
> +		pm_runtime_set_suspended(dwc->dev);
>
>  out:
> -	pm_runtime_enable(dev);
> +	pm_runtime_enable(dwc->dev);
>
>  	return ret;
>  }
> +EXPORT_SYMBOL_GPL(dwc3_pm_resume);
>
> -static void dwc3_complete(struct device *dev)
> +void dwc3_pm_complete(struct dwc3 *dwc)
>  {
> -	struct dwc3	*dwc = dev_get_drvdata(dev);
>  	u32		reg;
>
>  	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
> @@ -2647,21 +2679,36 @@ static void dwc3_complete(struct device *dev)
>  		dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
>  	}
>  }
> +EXPORT_SYMBOL_GPL(dwc3_pm_complete);
> +
> +static int dwc3_plat_suspend(struct device *dev)
> +{
> +	return dwc3_pm_suspend(dev_get_drvdata(dev));
> +}
> +
> +static int dwc3_plat_resume(struct device *dev)
> +{
> +	return dwc3_pm_resume(dev_get_drvdata(dev));
> +}
> +
> +static void dwc3_plat_complete(struct device *dev)
> +{
> +	dwc3_pm_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_SYSTEM_SLEEP_PM_OPS(dwc3_plat_suspend, dwc3_plat_resume)
> +	.complete = dwc3_plat_complete,
>  	/*
>  	 * Runtime suspend halts the controller on disconnection. It relies on
>  	 * platforms with custom connection notification to start the controller
>  	 * again.
>  	 */
> -	SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
> -			dwc3_runtime_idle)
> +	SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume,
> +			   dwc3_plat_runtime_idle)
>  };
>
>  #ifdef CONFIG_OF
> diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
> new file mode 100644
> index 000000000000..e73cfc466012
> --- /dev/null
> +++ b/drivers/usb/dwc3/glue.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * glue.h - DesignWare USB3 DRD glue header
> + */
> +
> +#ifndef __DRIVERS_USB_DWC3_GLUE_H
> +#define __DRIVERS_USB_DWC3_GLUE_H
> +
> +#include <linux/types.h>
> +#include "core.h"
> +
> +/**
> + * dwc3_probe_data: Initialization parameters passed to dwc3_core_probe()
> + * @dwc: Reference to dwc3 context structure
> + * @res: resource for the DWC3 core mmio region
> + */
> +struct dwc3_probe_data {
> +	struct dwc3 *dwc;
> +	struct resource *res;
> +};
> +
> +int dwc3_core_probe(const struct dwc3_probe_data *data);
> +void dwc3_core_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_pm_suspend(struct dwc3 *dwc);
> +int dwc3_pm_resume(struct dwc3 *dwc);
> +void dwc3_pm_complete(struct dwc3 *dwc);
> +
> +#endif
>
> --
> 2.45.2
>

Powered by blists - more mailing lists