Message-ID: <20250321214901.6bnjc2n76cknat2f@synopsys.com>
Date: Fri, 21 Mar 2025 21:49:03 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.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>, Frank Li <Frank.li@....com>,
        "linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 6/7] usb: dwc3: qcom: Transition to flattened model

On Tue, Mar 18, 2025, Bjorn Andersson wrote:
> The USB IP-block found in most Qualcomm platforms is modelled in the
> Linux kernel as 3 different independent device drivers, but as shown by
> the already existing layering violations in the Qualcomm glue driver
> they can not be operated independently.
> 
> With the current implementation, the glue driver registers the core and
> has no way to know when this is done. As a result, e.g. the suspend
> callbacks needs to guard against NULL pointer dereferences when trying
> to peek into the struct dwc3 found in the drvdata of the child.
> Even with these checks, there are no way to fully protect ourselves from
> the race conditions that occur if the DWC3 is unbound.
> 
> Missing from the upstream Qualcomm USB support is handling of role
> switching, in which the glue needs to be notified upon DRD mode changes.
> Several attempts has been made through the years to register callbacks
> etc, but they always fall short when it comes to handling of the core's
> probe deferral on resources etc.
> 
> Moving to a model where the DWC3 core is instantiated in a synchronous
> fashion avoids above described race conditions.
> 
> It is however not feasible to do so without also flattening the
> DeviceTree binding, as assumptions are made in the DWC3 core and
> frameworks used that the device's associated of_node will the that of
> the core. Furthermore, the DeviceTree binding is a direct
> representation of the Linux driver model, and doesn't necessarily
> describe "the USB IP-block".
> 
> The Qualcomm DWC3 glue driver is therefor transitioned to initialize and
> operate the DWC3 within the one device context, in synchronous fashion.
> 
> To provide a limited time backwards compatibility, a snapshot of the
> driver is retained in a previous commit. As such no care is taken in the
> dwc3-qcom driver for the qcom,dwc3 backwards compatibility.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 143 ++++++++++++++++++++++---------------------
>  1 file changed, 74 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 9d04c2457433bd6bcd96c445f59d7f2a3c6fdf24..49ad3fc8df33a354c7ab7cb0fbc47e47419fe689 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -4,7 +4,6 @@
>   * Inspired by dwc3-of-simple.c
>   */
>  
> -#include <linux/cleanup.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/clk.h>
> @@ -14,7 +13,6 @@
>  #include <linux/kernel.h>
>  #include <linux/extcon.h>
>  #include <linux/interconnect.h>
> -#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/phy/phy.h>
>  #include <linux/usb/of.h>
> @@ -23,6 +21,7 @@
>  #include <linux/usb/hcd.h>
>  #include <linux/usb.h>
>  #include "core.h"
> +#include "glue.h"
>  
>  /* USB QSCRATCH Hardware registers */
>  #define QSCRATCH_HS_PHY_CTRL			0x10
> @@ -73,7 +72,7 @@ struct dwc3_qcom_port {
>  struct dwc3_qcom {
>  	struct device		*dev;
>  	void __iomem		*qscratch_base;
> -	struct platform_device	*dwc3;
> +	struct dwc3		dwc;
>  	struct clk		**clks;
>  	int			num_clocks;
>  	struct reset_control	*resets;
> @@ -92,6 +91,8 @@ struct dwc3_qcom {
>  	struct icc_path		*icc_path_apps;
>  };
>  
> +#define to_dwc3_qcom(d) container_of((d), struct dwc3_qcom, dwc)
> +
>  static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>  {
>  	u32 reg;
> @@ -116,6 +117,11 @@ static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val)
>  	readl(base + offset);
>  }
>  
> +/*
> + * TODO: Make the in-core role switching code invoke dwc3_qcom_vbus_override_enable(),
> + * validate that the in-core extcon support is functional, and drop extcon
> + * handling from the glue
> + */
>  static void dwc3_qcom_vbus_override_enable(struct dwc3_qcom *qcom, bool enable)
>  {
>  	if (enable) {
> @@ -260,7 +266,7 @@ static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
>  		goto put_path_ddr;
>  	}
>  
> -	max_speed = usb_get_maximum_speed(&qcom->dwc3->dev);
> +	max_speed = usb_get_maximum_speed(qcom->dwc.dev);
>  	if (max_speed >= USB_SPEED_SUPER || max_speed == USB_SPEED_UNKNOWN) {
>  		ret = icc_set_bw(qcom->icc_path_ddr,
>  				USB_MEMORY_AVG_SS_BW, USB_MEMORY_PEAK_SS_BW);
> @@ -303,25 +309,14 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
>  /* Only usable in contexts where the role can not change. */
>  static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
>  {
> -	struct dwc3 *dwc;
> -
> -	/*
> -	 * FIXME: Fix this layering violation.
> -	 */
> -	dwc = platform_get_drvdata(qcom->dwc3);
> -
> -	/* Core driver may not have probed yet. */
> -	if (!dwc)
> -		return false;
> -
> -	return dwc->xhci;
> +	return qcom->dwc.xhci;
>  }
>  
>  static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom, int port_index)
>  {
> -	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>  	struct usb_device *udev;
>  	struct usb_hcd __maybe_unused *hcd;
> +	struct dwc3 *dwc = &qcom->dwc;
>  
>  	/*
>  	 * FIXME: Fix this layering violation.
> @@ -498,7 +493,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>  static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
>  {
>  	struct dwc3_qcom *qcom = data;
> -	struct dwc3	*dwc = platform_get_drvdata(qcom->dwc3);
> +	struct dwc3 *dwc = &qcom->dwc;
>  
>  	/* If pm_suspended then let pm_resume take care of resuming h/w */
>  	if (qcom->pm_suspended)
> @@ -700,40 +695,14 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
>  	return 0;
>  }
>  
> -static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev)
> -{
> -	struct device_node	*np = pdev->dev.of_node;
> -	struct device		*dev = &pdev->dev;
> -	int			ret;
> -
> -	struct device_node *dwc3_np __free(device_node) = of_get_compatible_child(np,
> -										  "snps,dwc3");
> -	if (!dwc3_np) {
> -		dev_err(dev, "failed to find dwc3 core child\n");
> -		return -ENODEV;
> -	}
> -
> -	ret = of_platform_populate(np, NULL, NULL, dev);
> -	if (ret) {
> -		dev_err(dev, "failed to register dwc3 core - %d\n", ret);
> -		return ret;
> -	}
> -
> -	qcom->dwc3 = of_find_device_by_node(dwc3_np);
> -	if (!qcom->dwc3) {
> -		ret = -ENODEV;
> -		dev_err(dev, "failed to get dwc3 platform device\n");
> -		of_platform_depopulate(dev);
> -	}
> -
> -	return ret;
> -}
> -
>  static int dwc3_qcom_probe(struct platform_device *pdev)
>  {
> +	struct dwc3_probe_data	probe_data = {};
>  	struct device_node	*np = pdev->dev.of_node;
>  	struct device		*dev = &pdev->dev;
>  	struct dwc3_qcom	*qcom;
> +	struct resource		res;
> +	struct resource		*r;
>  	int			ret, i;
>  	bool			ignore_pipe_clk;
>  	bool			wakeup_source;
> @@ -742,7 +711,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	if (!qcom)
>  		return -ENOMEM;
>  
> -	platform_set_drvdata(pdev, qcom);
>  	qcom->dev = &pdev->dev;
>  
>  	qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
> @@ -771,8 +739,15 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  		goto reset_assert;
>  	}
>  
> -	qcom->qscratch_base = devm_platform_ioremap_resource(pdev, 0);
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!r)
> +		goto clk_disable;
> +	res = *r;
> +	res.end = res.start + SDM845_QSCRATCH_BASE_OFFSET;
> +
> +	qcom->qscratch_base = devm_ioremap(dev, res.end, SDM845_QSCRATCH_SIZE);
>  	if (IS_ERR(qcom->qscratch_base)) {
> +		dev_err(dev, "failed to map qscratch region: %pe\n", qcom->qscratch_base);
>  		ret = PTR_ERR(qcom->qscratch_base);
>  		goto clk_disable;
>  	}
> @@ -792,17 +767,21 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	if (ignore_pipe_clk)
>  		dwc3_qcom_select_utmi_clk(qcom);
>  
> -	ret = dwc3_qcom_of_register_core(qcom, pdev);
> -	if (ret) {
> -		dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
> +	qcom->dwc.dev = dev;
> +	probe_data.dwc = &qcom->dwc;
> +	probe_data.res = &res;
> +	probe_data.ignore_clocks_and_resets = true;
> +	ret = dwc3_core_probe(&probe_data);
> +	if (ret)  {
> +		ret = dev_err_probe(dev, ret, "failed to register DWC3 Core\n");
>  		goto clk_disable;
>  	}
>  
>  	ret = dwc3_qcom_interconnect_init(qcom);
>  	if (ret)
> -		goto depopulate;
> +		goto remove_core;
>  
> -	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
> +	qcom->mode = usb_get_dr_mode(dev);
>  
>  	/* enable vbus override for device mode */
>  	if (qcom->mode != USB_DR_MODE_HOST)
> @@ -815,20 +794,15 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  
>  	wakeup_source = of_property_read_bool(dev->of_node, "wakeup-source");
>  	device_init_wakeup(&pdev->dev, wakeup_source);
> -	device_init_wakeup(&qcom->dwc3->dev, wakeup_source);
>  
>  	qcom->is_suspended = false;
> -	pm_runtime_set_active(dev);
> -	pm_runtime_enable(dev);
> -	pm_runtime_forbid(dev);
>  
>  	return 0;
>  
>  interconnect_exit:
>  	dwc3_qcom_interconnect_exit(qcom);
> -depopulate:
> -	of_platform_depopulate(&pdev->dev);
> -	platform_device_put(qcom->dwc3);
> +remove_core:
> +	dwc3_core_remove(&qcom->dwc);
>  clk_disable:
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
>  		clk_disable_unprepare(qcom->clks[i]);
> @@ -842,12 +816,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  
>  static void dwc3_qcom_remove(struct platform_device *pdev)
>  {
> -	struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> +	struct dwc3 *dwc = platform_get_drvdata(pdev);
> +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
>  	struct device *dev = &pdev->dev;
>  	int i;
>  
> -	of_platform_depopulate(&pdev->dev);
> -	platform_device_put(qcom->dwc3);
> +	dwc3_core_remove(&qcom->dwc);
>  
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
>  		clk_disable_unprepare(qcom->clks[i]);
> @@ -864,10 +838,15 @@ static void dwc3_qcom_remove(struct platform_device *pdev)
>  
>  static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
>  {
> -	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
>  	bool wakeup = device_may_wakeup(dev);
>  	int ret;
>  
> +	ret = dwc3_pm_suspend(&qcom->dwc);
> +	if (ret)
> +		return ret;
> +
>  	ret = dwc3_qcom_suspend(qcom, wakeup);
>  	if (ret)
>  		return ret;
> @@ -879,7 +858,8 @@ static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
>  
>  static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
>  {
> -	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
>  	bool wakeup = device_may_wakeup(dev);
>  	int ret;
>  
> @@ -889,30 +869,55 @@ static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
>  
>  	qcom->pm_suspended = false;
>  
> +	ret = dwc3_pm_resume(&qcom->dwc);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
>  static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
>  {
> -	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> +	int ret;
> +
> +	ret = dwc3_runtime_suspend(&qcom->dwc);
> +	if (ret)
> +		return ret;
>  
>  	return dwc3_qcom_suspend(qcom, true);
>  }
>  
> +static void __maybe_unused dwc3_qcom_complete(struct device *dev)
> +{
> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> +
> +	dwc3_pm_complete(dwc);
> +}
> +
>  static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
>  {
> -	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> +	struct dwc3_qcom *qcom = to_dwc3_qcom(dwc);
> +	int ret;
> +
> +	ret = dwc3_qcom_resume(qcom, true);
> +	if (ret)
> +		return ret;
>  
> -	return dwc3_qcom_resume(qcom, true);
> +	return dwc3_runtime_resume(&qcom->dwc);
>  }
>  
>  static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(dwc3_qcom_pm_suspend, dwc3_qcom_pm_resume)
>  	SET_RUNTIME_PM_OPS(dwc3_qcom_runtime_suspend, dwc3_qcom_runtime_resume,
>  			   NULL)
> +	.complete = dwc3_qcom_complete,
>  };
>  
>  static const struct of_device_id dwc3_qcom_of_match[] = {
> +	{ .compatible = "qcom,snps-dwc3" },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);
> 
> -- 
> 2.48.1
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@...opsys.com>

Thanks,
Thinh

Powered by blists - more mailing lists