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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
Thanks,
Thinh
