| Message ID | 20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com |
|---|---|
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
On 10/17/23 05:11, Bjorn Andersson wrote: > In preparation for the introduction of a direct reference to the struct > dwc3 in the dwc3_qcom struct, rename the generically named "dwc3" to > reduce the risk for confusion. > > No functional change. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 10/17/23 05:11, Bjorn Andersson wrote: > In the case that the dwc3 core driver is instantiated from the same > memory resource information as the glue driver, the dwc_res memory > region will overlap with the memory region already mapped by the glue. > > As the DWC3 core driver already does math on the passed memory region to > exclude the XHCI region, also adjust the end address, to avoid having to > pass an adjusted region from the glue explicitly. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
On 10/17/23 05:11, Bjorn Andersson wrote: > The two constants for the offset and size of the qscratch block within > the DWC3 region has the same value in all supported variants, so they > don't necessarily need to be picked dynamically. > > By replacing the lookup with the constants it's possible to reuse the > same code path without the ACPI pdata structure in the upcoming commit. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- If we don't want more non-flagship-SoC-based post-sdm845 ACPI-supported platforms, using just one of N USB controllers which I am very happy that we don't so much so that I'd be even happy to drop this acpi thing altogether Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> Konrad
Hi Bjorn, On Mon, Oct 16, 2023, 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. > > 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. > > Furhtermore, the DeviceTree binding is a direct representation of the > Linux driver model, and doesn't necessarily describe "the USB IP-block". > > This series therefor attempts to flatten the driver split, and operate > the glue and core out of the same platform_device instance. And in order > to do this, the DeviceTree representation of the IP block is flattened. > > As a side effect, much of the ACPI integration code is dropped. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > Bjorn Andersson (12): > dt-bindings: usb: qcom,dwc3: Add qcom,sc8180x-dwc3 > usb: dwc3: qcom: Rename dwc3 platform_device reference > usb: dwc3: qcom: Merge resources from urs_usb device > usb: dwc3: Expose core driver as library > usb: dwc3: Override end of dwc3 memory resource > usb: dwc3: qcom: Add dwc3 core reference in driver state > usb: dwc3: qcom: Instantiate dwc3 core directly > usb: dwc3: qcom: Inline the qscratch constants > dt-bindings: usb: qcom,dwc3: Rename to "glue" > dt-bindings: usb: qcom,dwc3: Introduce flattened qcom,dwc3 binding > usb: dwc3: qcom: Flatten the Qualcomm dwc3 binding and implementation > arm64: dts: qcom: sc8180x: flatten usb_sec node > > .../devicetree/bindings/usb/qcom,dwc3-glue.yaml | 626 +++++++++++++++++++++ > .../devicetree/bindings/usb/qcom,dwc3.yaml | 321 ++++------- > .../devicetree/bindings/usb/snps,dwc3.yaml | 14 +- > .../arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts | 6 +- > arch/arm64/boot/dts/qcom/sc8180x-primus.dts | 6 +- > arch/arm64/boot/dts/qcom/sc8180x.dtsi | 34 +- > drivers/usb/dwc3/core.c | 136 +++-- > drivers/usb/dwc3/core.h | 10 + > drivers/usb/dwc3/dwc3-qcom.c | 328 ++++++----- > 9 files changed, 1057 insertions(+), 424 deletions(-) > --- > base-commit: 4d0515b235dec789578d135a5db586b25c5870cb > change-id: 20231016-dwc3-refactor-931e3b08a8b9 > > Best regards, > -- > Bjorn Andersson <quic_bjorande@quicinc.com> > First of all, thanks for the work. I just did a quick review through the changes, and I think it's great! (This will address some issues I also have with dwc3 currently too.) BR, Thinh
On Mon, Oct 16, 2023, Bjorn Andersson wrote: > In the case that the dwc3 core driver is instantiated from the same > memory resource information as the glue driver, the dwc_res memory > region will overlap with the memory region already mapped by the glue. > > As the DWC3 core driver already does math on the passed memory region to > exclude the XHCI region, also adjust the end address, to avoid having to > pass an adjusted region from the glue explicitly. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 71e376bebb16..5d86b803fab0 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1908,6 +1908,7 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev) > */ > dwc_res = *res; > dwc_res.start += DWC3_GLOBALS_REGS_START; > + dwc_res.end = res->start + DWC3_OTG_REGS_END; DWC3_OTG_REGS_END shouldn't really be the end. What offset does qcom start overlapping? Can the end be 0xda00 for now? (This can change) BR, Thinh > > if (dev->of_node) { > struct device_node *parent = of_get_parent(dev->of_node); > @@ -1915,6 +1916,7 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev) > if (of_device_is_compatible(parent, "realtek,rtd-dwc3")) { > dwc_res.start -= DWC3_GLOBALS_REGS_START; > dwc_res.start += DWC3_RTK_RTD_GLOBALS_REGS_START; > + dwc_res.end = dwc_res.start + DWC3_OTG_REGS_END; > } > > of_node_put(parent); > > -- > 2.25.1 >
On Mon, Oct 16, 2023, 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 platfrom_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 | 134 ++++++++++++++++++++++++++++++++---------------- > drivers/usb/dwc3/core.h | 10 ++++ > 2 files changed, 100 insertions(+), 44 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index d25490965b27..71e376bebb16 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > return 0; > } > > -static int dwc3_probe(struct platform_device *pdev) > +struct dwc3 *dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct resource *res, dwc_res; > @@ -1886,14 +1886,14 @@ 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; > + return ERR_PTR(-ENODEV); > } > > dwc->xhci_resources[0].start = res->start; > @@ -1922,7 +1922,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); > @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > - platform_set_drvdata(pdev, dwc); > dwc3_cache_hwparams(dwc); > > if (!dwc->sysdev_is_parent && > @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev) > > pm_runtime_put(dev); > > - return 0; > + return dwc; > > err_exit_debugfs: > dwc3_debugfs_exit(dwc); > @@ -2030,14 +2029,26 @@ 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 dwc3 *dwc; > + > + dwc = dwc3_probe(pdev); > + if (IS_ERR(dwc)) > + return PTR_ERR(dwc); > + > + platform_set_drvdata(pdev, dwc); > + > + return 0; > +} > > - pm_runtime_get_sync(&pdev->dev); > +void dwc3_remove(struct dwc3 *dwc) > +{ > + pm_runtime_get_sync(dwc->dev); > > dwc3_core_exit_mode(dwc); > dwc3_debugfs_exit(dwc); > @@ -2045,22 +2056,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) > @@ -2227,9 +2244,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)) > @@ -2241,10 +2257,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); > @@ -2261,15 +2277,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)) > @@ -2281,49 +2296,64 @@ 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); > > ret = dwc3_resume_common(dwc, PMSG_RESUME); > if (ret) > return ret; > > - pm_runtime_disable(dev); > - pm_runtime_set_active(dev); > - pm_runtime_enable(dev); > + pm_runtime_disable(dwc->dev); > + pm_runtime_set_active(dwc->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 && > @@ -2333,15 +2363,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 > @@ -2369,8 +2415,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 c6c87acbd376..f5e22559bb73 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1568,6 +1568,16 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc); > > int dwc3_core_soft_reset(struct dwc3 *dwc); > > +struct dwc3 *dwc3_probe(struct platform_device *pdev); > +void dwc3_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_suspend(struct dwc3 *dwc); > +int dwc3_resume(struct dwc3 *dwc); > +void dwc3_complete(struct dwc3 *dwc); > + Can we make this new interface clear? It's being buried under core.h this way. Perhaps add some comments and divider, or move to another header file? Thanks, Thinh > #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) > int dwc3_host_init(struct dwc3 *dwc); > void dwc3_host_exit(struct dwc3 *dwc); > > -- > 2.25.1 >
On Mon, Oct 16, 2023 at 08:11:08PM -0700, 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. > > 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. Nice to see this finally being worked on. It's not clear why mode-change notifications would be a problem though, as if you get such a notification, you know that core has been probed. > Furhtermore, the DeviceTree binding is a direct representation of the > Linux driver model, and doesn't necessarily describe "the USB IP-block". True. Johan
On Mon, Oct 16, 2023 at 08:11:10PM -0700, Bjorn Andersson wrote: > In preparation for the introduction of a direct reference to the struct > dwc3 in the dwc3_qcom struct, rename the generically named "dwc3" to > reduce the risk for confusion. > > No functional change. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 46 ++++++++++++++++++++++---------------------- > 1 file changed, 23 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 3de43df6bbe8..a31c3bc1f56e 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -67,7 +67,7 @@ struct dwc3_acpi_pdata { > struct dwc3_qcom { > struct device *dev; > void __iomem *qscratch_base; > - struct platform_device *dwc3; > + struct platform_device *dwc_dev; Since "dev" is so overloaded, please name this one "dwc_pdev" instead. > struct platform_device *urs_usb; > struct clk **clks; > int num_clocks; > static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) > { > - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc_dev); > struct usb_device *udev; > struct usb_hcd __maybe_unused *hcd; > > @@ -486,7 +486,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 = platform_get_drvdata(qcom->dwc_dev); Perhaps you can drop the tab while changing this line. Johan
On Mon, Oct 16, 2023 at 08:11:11PM -0700, Bjorn Andersson wrote: > With some ACPI DSDT tables, such as the one found in SC8180X devices, > the USB resources are split between the URSn and it's child USBn device > nodes, in particular the interrupts are placed in the child nodes. > > The solution that was chosen for handling this is to allocate a > platform_device from the child node and selectively pick interrupts > from the main platform_device, or from this created child device, when > creating the platform_device for the DWC3 core. > > This does however not work with the upcoming change where the DWC3 core > is instantiated from the same platform_device as the glue, as the DRD > and host code will attempt to resolve their interrupts from the shared > device, and not the child device. > > Work around this by merging the resources of the child device into the > glue device node, to present a single platform_device with all the > resources necessary. Nice approach. An alternative would be to drop ACPI support completely as Konrad suggested. Should simplify both this series and the multiport one. Is anyone really using the ACPI support here anymore? > -static struct platform_device * > -dwc3_qcom_create_urs_usb_platdev(struct device *dev) > +static int dwc3_qcom_acpi_merge_urs_resources(struct platform_device *pdev) > { > + struct device *dev = &pdev->dev; > + struct list_head resource_list; > + struct resource_entry *rentry; > + struct resource *resources; > struct fwnode_handle *fwh; > struct acpi_device *adev; > char name[8]; > + int count; > int ret; > int id; > + int i; > > /* Figure out device id */ > ret = sscanf(fwnode_get_name(dev->fwnode), "URS%d", &id); > if (!ret) > - return NULL; > + return -EINVAL; > > /* Find the child using name */ > snprintf(name, sizeof(name), "USB%d", id); > fwh = fwnode_get_named_child_node(dev->fwnode, name); > if (!fwh) > - return NULL; > + return 0; > > adev = to_acpi_device_node(fwh); > if (!adev) > - return NULL; > + return -EINVAL; This is currently leaking a reference to the fwnode, I fixed that up here: https://lore.kernel.org/linux-usb/20231117173650.21161-4-johan+linaro@kernel.org/ > + INIT_LIST_HEAD(&resource_list); > + > + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); > + if (count <= 0) > + return count; > + > + count += pdev->num_resources; > + > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > + if (!resources) { > + acpi_dev_free_resource_list(&resource_list); > + return -ENOMEM; > + } > + > + memcpy(resources, pdev->resource, sizeof(struct resource) * pdev->num_resources); > + count = pdev->num_resources; > + list_for_each_entry(rentry, &resource_list, node) { > + /* Avoid inserting duplicate entries, in case this is called more than once */ Either shorten this one or make it a multiline comment to stay within 80 chars. > + for (i = 0; i < count; i++) { Should this not be pdev->num_resources? > + if (resource_type(&resources[i]) == resource_type(rentry->res) && > + resources[i].start == rentry->res->start && > + resources[i].end == rentry->res->end) > + break; > + } > + > + if (i == count) Same here. > + resources[count++] = *rentry->res; > + } > > - return acpi_create_platform_device(adev, NULL); > + ret = platform_device_add_resources(pdev, resources, count); > + if (ret) > + dev_err(&pdev->dev, "failed to add resources\n"); > + > + acpi_dev_free_resource_list(&resource_list); > + kfree(resources); > + > + return ret; > } > > static int dwc3_qcom_probe(struct platform_device *pdev) > @@ -817,6 +853,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > dev_err(&pdev->dev, "no supporting ACPI device data\n"); > return -EINVAL; > } > + > + if (qcom->acpi_pdata->is_urs) { > + ret = dwc3_qcom_acpi_merge_urs_resources(pdev); > + if (ret < 0) > + goto clk_disable; The clocks have not been enabled here, just return ret. > + } > } > > qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); > @@ -857,18 +899,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > qcom->acpi_pdata->qscratch_base_offset; > parent_res->end = parent_res->start + > qcom->acpi_pdata->qscratch_base_size; > - > - if (qcom->acpi_pdata->is_urs) { > - qcom->urs_usb = dwc3_qcom_create_urs_usb_platdev(dev); > - if (IS_ERR_OR_NULL(qcom->urs_usb)) { > - dev_err(dev, "failed to create URS USB platdev\n"); > - if (!qcom->urs_usb) > - ret = -ENODEV; > - else > - ret = PTR_ERR(qcom->urs_usb); > - goto clk_disable; > - } > - } > } > > qcom->qscratch_base = devm_ioremap_resource(dev, parent_res); Johan
On Mon, Oct 16, 2023 at 08:11:12PM -0700, 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 platfrom_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 | 134 ++++++++++++++++++++++++++++++++---------------- > drivers/usb/dwc3/core.h | 10 ++++ > 2 files changed, 100 insertions(+), 44 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index d25490965b27..71e376bebb16 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > return 0; > } > > -static int dwc3_probe(struct platform_device *pdev) > +struct dwc3 *dwc3_probe(struct platform_device *pdev) Perhaps you should move allocation of struct dwc3 to the caller as well as you are going to need some way to pass in callback to core which need to be set before you register the xhci platform device. There could be other ways, like passing in a struct of callbacks, which can be added incrementally but it may be good think this through from the start. > { > struct device *dev = &pdev->dev; > struct resource *res, dwc_res; > @@ -1886,14 +1886,14 @@ 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; > + return ERR_PTR(-ENODEV); > } > > dwc->xhci_resources[0].start = res->start; > @@ -1922,7 +1922,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); > @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > - platform_set_drvdata(pdev, dwc); This is broken however as the pm ops access the data driver data and can be called as soon as you call pm_runtime_put() below. > dwc3_cache_hwparams(dwc); > > if (!dwc->sysdev_is_parent && > @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev) > > pm_runtime_put(dev); That is here. > - return 0; > + return dwc; > -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 dwc3 *dwc; > + > + dwc = dwc3_probe(pdev); > + if (IS_ERR(dwc)) > + return PTR_ERR(dwc); And that leaves a window, for example, here where you can hit a NULL pointer dereference. > + platform_set_drvdata(pdev, dwc); > + > + return 0; > +} Johan
On Mon, Oct 16, 2023 at 08:11:14PM -0700, Bjorn Andersson wrote: > In the coming changes the Qualcomm DWC3 glue will be able to either > manage the DWC3 core as a child platform_device, or directly instantiate > it within its own context. > > Introduce a reference to the dwc3 core state and make the driver > reference the dwc3 core either the child device or this new reference. > > As the new member isn't assigned, and qcom->dwc_dev is assigned in all > current cases, the change should have no functional impact. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++-------- > 1 file changed, 83 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 7c810712d246..901e5050363b 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -67,7 +67,8 @@ struct dwc3_acpi_pdata { > struct dwc3_qcom { > struct device *dev; > void __iomem *qscratch_base; > - struct platform_device *dwc_dev; > + struct platform_device *dwc_dev; /* only used when core is separate device */ > + struct dwc3 *dwc; /* not used when core is separate device */ Hmm. This quickly become really messy and hard to maintain. It may be fine as an intermediate step as part of this series, but why can't you do the conversion fully so that the Qualcomm glue driver never registers a core platform device? Is it just about where the core driver looks for DT properties? Johan
On Mon, Oct 16, 2023 at 08:11:15PM -0700, Bjorn Andersson wrote: > The Qualcomm DWC3 glue builds up a platform_device programmatically in > order to probe the DWC3 core when running off ACPI data. But with the > newly introduced support for instantiating the core directly from the > glue, this code can be replaced with a single function call. > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > --- > @@ -986,10 +933,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > interconnect_exit: > dwc3_qcom_interconnect_exit(qcom); > depopulate: > - if (np) > + if (qcom->dwc_dev) > of_platform_depopulate(&pdev->dev); > else > - platform_device_put(pdev); > + dwc3_remove(qcom->dwc); The current code was broken here too: https://lore.kernel.org/linux-usb/20231117173650.21161-2-johan+linaro@kernel.org/ Johan
On Wed, Nov 22, 2023 at 11:24:07AM +0100, Johan Hovold wrote: > On Mon, Oct 16, 2023 at 08:11:11PM -0700, Bjorn Andersson wrote: > > With some ACPI DSDT tables, such as the one found in SC8180X devices, > > the USB resources are split between the URSn and it's child USBn device > > nodes, in particular the interrupts are placed in the child nodes. > > > > The solution that was chosen for handling this is to allocate a > > platform_device from the child node and selectively pick interrupts > > from the main platform_device, or from this created child device, when > > creating the platform_device for the DWC3 core. > > > > This does however not work with the upcoming change where the DWC3 core > > is instantiated from the same platform_device as the glue, as the DRD > > and host code will attempt to resolve their interrupts from the shared > > device, and not the child device. > > > > Work around this by merging the resources of the child device into the > > glue device node, to present a single platform_device with all the > > resources necessary. > > Nice approach. > > An alternative would be to drop ACPI support completely as Konrad > suggested. Should simplify both this series and the multiport one. > > Is anyone really using the ACPI support here anymore? > At the introduction of SC8180X and the Lenovo Flex 5G we where able to run the Debian installer off the ACPI support in the kernel. Since then, at least the UFS support has regressed to the point that this would no longer be possible - without anyone noticing. I would like to see ACPI supported again in the future, but I can't really argue for its existence currently. In the end the new flattened code path is mostly shared with the ACPI path, so perhaps it makes sense to drop the support after this refactor, perhaps not. I will re-evaluate this. > > -static struct platform_device * > > -dwc3_qcom_create_urs_usb_platdev(struct device *dev) > > +static int dwc3_qcom_acpi_merge_urs_resources(struct platform_device *pdev) > > { > > + struct device *dev = &pdev->dev; > > + struct list_head resource_list; > > + struct resource_entry *rentry; > > + struct resource *resources; > > struct fwnode_handle *fwh; > > struct acpi_device *adev; > > char name[8]; > > + int count; > > int ret; > > int id; > > + int i; > > > > /* Figure out device id */ > > ret = sscanf(fwnode_get_name(dev->fwnode), "URS%d", &id); > > if (!ret) > > - return NULL; > > + return -EINVAL; > > > > /* Find the child using name */ > > snprintf(name, sizeof(name), "USB%d", id); > > fwh = fwnode_get_named_child_node(dev->fwnode, name); > > if (!fwh) > > - return NULL; > > + return 0; > > > > adev = to_acpi_device_node(fwh); > > if (!adev) > > - return NULL; > > + return -EINVAL; > > This is currently leaking a reference to the fwnode, I fixed that up > here: > > https://lore.kernel.org/linux-usb/20231117173650.21161-4-johan+linaro@kernel.org/ > > > + INIT_LIST_HEAD(&resource_list); > > + > > + count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL); > > + if (count <= 0) > > + return count; > > + > > + count += pdev->num_resources; > > + > > + resources = kcalloc(count, sizeof(*resources), GFP_KERNEL); > > + if (!resources) { > > + acpi_dev_free_resource_list(&resource_list); > > + return -ENOMEM; > > + } > > + > > + memcpy(resources, pdev->resource, sizeof(struct resource) * pdev->num_resources); > > + count = pdev->num_resources; > > + list_for_each_entry(rentry, &resource_list, node) { > > + /* Avoid inserting duplicate entries, in case this is called more than once */ > > Either shorten this one or make it a multiline comment to stay within 80 > chars. > > > + for (i = 0; i < count; i++) { > > Should this not be pdev->num_resources? > count is first used to denote the number of entries to allocate in the new list, it's then reset to pdev->num_resources 3 lines above this and after this list_for_each_entry() it would be the total number of resources in the new list (which could be less than the allocated number of items). I can avoid reusing the variable, to clarify this - if I choose to keep the ACPI support through the series. > > + if (resource_type(&resources[i]) == resource_type(rentry->res) && > > + resources[i].start == rentry->res->start && > > + resources[i].end == rentry->res->end) > > + break; > > + } > > + > > + if (i == count) > > Same here. > > > + resources[count++] = *rentry->res; > > + } > > > > - return acpi_create_platform_device(adev, NULL); > > + ret = platform_device_add_resources(pdev, resources, count); > > + if (ret) > > + dev_err(&pdev->dev, "failed to add resources\n"); > > + > > + acpi_dev_free_resource_list(&resource_list); > > + kfree(resources); > > + > > + return ret; > > } > > > > static int dwc3_qcom_probe(struct platform_device *pdev) > > @@ -817,6 +853,12 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > dev_err(&pdev->dev, "no supporting ACPI device data\n"); > > return -EINVAL; > > } > > + > > + if (qcom->acpi_pdata->is_urs) { > > + ret = dwc3_qcom_acpi_merge_urs_resources(pdev); > > + if (ret < 0) > > + goto clk_disable; > > The clocks have not been enabled here, just return ret. > Right. Thanks, Bjorn > > + } > > } > > > > qcom->resets = devm_reset_control_array_get_optional_exclusive(dev); > > @@ -857,18 +899,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > > qcom->acpi_pdata->qscratch_base_offset; > > parent_res->end = parent_res->start + > > qcom->acpi_pdata->qscratch_base_size; > > - > > - if (qcom->acpi_pdata->is_urs) { > > - qcom->urs_usb = dwc3_qcom_create_urs_usb_platdev(dev); > > - if (IS_ERR_OR_NULL(qcom->urs_usb)) { > > - dev_err(dev, "failed to create URS USB platdev\n"); > > - if (!qcom->urs_usb) > > - ret = -ENODEV; > > - else > > - ret = PTR_ERR(qcom->urs_usb); > > - goto clk_disable; > > - } > > - } > > } > > > > qcom->qscratch_base = devm_ioremap_resource(dev, parent_res); > > Johan
On Wed, Nov 22, 2023 at 12:57:37PM +0100, Johan Hovold wrote: > On Mon, Oct 16, 2023 at 08:11:12PM -0700, 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 platfrom_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 | 134 ++++++++++++++++++++++++++++++++---------------- > > drivers/usb/dwc3/core.h | 10 ++++ > > 2 files changed, 100 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index d25490965b27..71e376bebb16 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1876,7 +1876,7 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > > return 0; > > } > > > > -static int dwc3_probe(struct platform_device *pdev) > > +struct dwc3 *dwc3_probe(struct platform_device *pdev) > > Perhaps you should move allocation of struct dwc3 to the caller as well > as you are going to need some way to pass in callback to core which need > to be set before you register the xhci platform device. > > There could be other ways, like passing in a struct of callbacks, which > can be added incrementally but it may be good think this through from > the start. > My intention was to have callbacks and quirks passed through additional arguments in an incremental patch. IMHO passing such information through a pre-allocated and partially initialized struct is more obscure than passing that information as explicit parameters to the function... > > { > > struct device *dev = &pdev->dev; > > struct resource *res, dwc_res; > > @@ -1886,14 +1886,14 @@ 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; > > + return ERR_PTR(-ENODEV); > > } > > > > dwc->xhci_resources[0].start = res->start; > > @@ -1922,7 +1922,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); > > @@ -1953,7 +1953,6 @@ static int dwc3_probe(struct platform_device *pdev) > > goto err_disable_clks; > > } > > > > - platform_set_drvdata(pdev, dwc); > > This is broken however as the pm ops access the data driver data and can > be called as soon as you call pm_runtime_put() below. > You're right, thanks for spotting that. Regards, Bjorn > > dwc3_cache_hwparams(dwc); > > > > if (!dwc->sysdev_is_parent && > > @@ -2006,7 +2005,7 @@ static int dwc3_probe(struct platform_device *pdev) > > > > pm_runtime_put(dev); > > That is here. > > > - return 0; > > + return dwc; > > > -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 dwc3 *dwc; > > + > > + dwc = dwc3_probe(pdev); > > + if (IS_ERR(dwc)) > > + return PTR_ERR(dwc); > > And that leaves a window, for example, here where you can hit a NULL > pointer dereference. > > > + platform_set_drvdata(pdev, dwc); > > + > > + return 0; > > +} > > Johan >
On Wed, Nov 22, 2023 at 10:48:50AM +0100, Johan Hovold wrote: > On Mon, Oct 16, 2023 at 08:11:08PM -0700, 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. > > > > 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. > > Nice to see this finally being worked on. It's not clear why mode-change > notifications would be a problem though, as if you get such a > notification, you know that core has been probed. > The problem here is that the usb_role_switch is implemented in the core, but the glue needs to act upon the notification as well - and there's currently no way to have the core notify the glue about such changes. You can see this "solved" in the case of extcon, where both the Qualcomm glue and core listens to and act upon the extcon updates. This isn't pretty, but with the of-graph based role-switching description (and good judgement) it's not possible to replicate this on modern platforms. Which means that this leaves a TODO to investigate if we can drop the extcon support from dwc3-qcom.c Regards, Bjorn > > Furhtermore, the DeviceTree binding is a direct representation of the > > Linux driver model, and doesn't necessarily describe "the USB IP-block". > > True. > > Johan
On Wed, Nov 22, 2023 at 01:18:33PM +0100, Johan Hovold wrote: > On Mon, Oct 16, 2023 at 08:11:14PM -0700, Bjorn Andersson wrote: > > In the coming changes the Qualcomm DWC3 glue will be able to either > > manage the DWC3 core as a child platform_device, or directly instantiate > > it within its own context. > > > > Introduce a reference to the dwc3 core state and make the driver > > reference the dwc3 core either the child device or this new reference. > > > > As the new member isn't assigned, and qcom->dwc_dev is assigned in all > > current cases, the change should have no functional impact. > > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> > > --- > > drivers/usb/dwc3/dwc3-qcom.c | 100 +++++++++++++++++++++++++++++++++++-------- > > 1 file changed, 83 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index 7c810712d246..901e5050363b 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -67,7 +67,8 @@ struct dwc3_acpi_pdata { > > struct dwc3_qcom { > > struct device *dev; > > void __iomem *qscratch_base; > > - struct platform_device *dwc_dev; > > + struct platform_device *dwc_dev; /* only used when core is separate device */ > > + struct dwc3 *dwc; /* not used when core is separate device */ > > Hmm. This quickly become really messy and hard to maintain. It may be > fine as an intermediate step as part of this series, but why can't you > do the conversion fully so that the Qualcomm glue driver never registers > a core platform device? Is it just about where the core driver looks for > DT properties? > In the new driver model, pdev->dev.of_node needs to contain the resources for both the glue and the core. For most of the information, that's a matter of copying properties and child nodes from the child of_node, but e.g. reg and interrupts needs to be merged. As mentioned in my other reply, extcon is serviced to both nodes, so without the callbacks that will break, at least - and I'd have to check to see if the of_graphs can be handled... That said, part of the reason for doing this shuffle is to make sure that dwc is always a valid pointer, and while keeping this scheme of two modes we will not be able to assume this anywhere in the code - and hence continue to rely on luck. One way around this would be to follow the of_platform_populate() with a check to see if the core was registered and if so grab the dwc pointer, otherwise of_platform_depopulate() the core again and probe defer. It will come with a penalty for devices running on the old binding, and we don't protect ourselves from the core being unbound while we're holding a pointer to its internal data. But it looks like a much better position to me. (In this case I think dwc_dev becomes a local variable using during probe, and the rest of the code would operate on dwc) Regards, Bjorn
On 10/17/2023 8:41 AM, Bjorn Andersson wrote: > The USB block found in most Qualcomm platforms is modelled as three > different independent device drivers, and represented in DeviceTree as > two layered nodes. But as shown by the already existing layering > violations in the Qualcomm glue driver they can not be operated > independently. > > In the current model, the probing of the core is asynchronous, and in a > number of places there's risk that the driver dereferences NULL > pointers, as it peeks into the core's drvdata. > > There is also no way, in the current design to make the core notify the > glue upon DRD mode changes. Among the past proposals have been attempts > to provide a callback registration API, but as there is no way to know > when the core is probed this doesn't work. > > Based on the recent refactoring its now possible to instantiate the glue > and core from a single representation of the DWC3 IP-block. This will > also allow for the glue to pass a callback to be called for DRD mode > changes. > > The only overlapping handling between the Qualcomm glue and the core is > the release of reset, which is left to the core to handle. > Hi Bjorn, I think the reset has to be handled by glue itself. I was testing this series and found one issue: During suspend, we suspend core first which will assert the reset and then suspend the glue which will disable the clocks. This path doesn't seem to have a problem somehow even in flattened implementation. During resume, we resume the glue first and then resume the core. During resume of glue, we enable the clocks and at this point, the reset is still kept asserted causing the clocks to never turn ON leading to a crash. This is the case in flattened implementation only as in normal case, the reset is handled by glue and we never meddle with reset other than the time of probing. I tried to check if we explicitly de-assert the reset during start of resume sequence of glue (in addition to the de-assertion present in core) and things worked out fine. But if I try to balance the reset count and add an assert at end of suspend sequence of glue (in addition to the assertion present in core), then it crashes complaining a double assertion happened. So double de-asserting is not causing a problem but double asserting is causing an issue. Regards, Krishna,
On 10/17/2023 8:41 AM, Bjorn Andersson wrote: > The Qualcomm DWC3 glue builds up a platform_device programmatically in > order to probe the DWC3 core when running off ACPI data. But with the > newly introduced support for instantiating the core directly from the > glue, this code can be replaced with a single function call. > > @@ -942,7 +889,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > if (np) > ret = dwc3_qcom_of_register_core(pdev); > else > - ret = dwc3_qcom_acpi_register_core(pdev); > + ret = dwc3_qcom_probe_core(pdev, qcom); > > if (ret) { > dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret); > @@ -986,10 +933,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > interconnect_exit: > dwc3_qcom_interconnect_exit(qcom); > depopulate: > - if (np) > + if (qcom->dwc_dev) > of_platform_depopulate(&pdev->dev); > else > - platform_device_put(pdev); > + dwc3_remove(qcom->dwc); Hi Bjorn I was testing this patch and I suspect there is one issue. In the event dwc3_qcom_probe_core fails because dwc3_probe failed, then the variable (qcom->dwc) is NULL. We then exercise the depopulate path and we call dwc_remove(NULL) causing a crash. Regards, Krishna,
On Wed, Jan 10, 2024 at 08:43:23AM +0530, Krishna Kurapati PSSNV wrote: > > > On 10/17/2023 8:41 AM, Bjorn Andersson wrote: > > The USB block found in most Qualcomm platforms is modelled as three > > different independent device drivers, and represented in DeviceTree as > > two layered nodes. But as shown by the already existing layering > > violations in the Qualcomm glue driver they can not be operated > > independently. > > > > In the current model, the probing of the core is asynchronous, and in a > > number of places there's risk that the driver dereferences NULL > > pointers, as it peeks into the core's drvdata. > > > > There is also no way, in the current design to make the core notify the > > glue upon DRD mode changes. Among the past proposals have been attempts > > to provide a callback registration API, but as there is no way to know > > when the core is probed this doesn't work. > > > > Based on the recent refactoring its now possible to instantiate the glue > > and core from a single representation of the DWC3 IP-block. This will > > also allow for the glue to pass a callback to be called for DRD mode > > changes. > > > > The only overlapping handling between the Qualcomm glue and the core is > > the release of reset, which is left to the core to handle. > > > > Hi Bjorn, > > I think the reset has to be handled by glue itself. I was testing this > series and found one issue: > > During suspend, we suspend core first which will assert the reset and then > suspend the glue which will disable the clocks. This path doesn't seem to > have a problem somehow even in flattened implementation. > > During resume, we resume the glue first and then resume the core. During > resume of glue, we enable the clocks and at this point, the reset is still > kept asserted causing the clocks to never turn ON leading to a crash. This > is the case in flattened implementation only as in normal case, the reset is > handled by glue and we never meddle with reset other than the time of > probing. > > I tried to check if we explicitly de-assert the reset during start of resume > sequence of glue (in addition to the de-assertion present in core) and > things worked out fine. But if I try to balance the reset count and add an > assert at end of suspend sequence of glue (in addition to the assertion > present in core), then it crashes complaining a double assertion happened. > So double de-asserting is not causing a problem but double asserting is > causing an issue. > You're right. I looked at it briefly but ended up moving the reset handling in the wrong direction... I expect that in any scenario where a glue driver is used the core can not control the reset. So far we've dealt with this by just not telling the core about the reset. Thanks, Bjorn > Regards, > Krishna,
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. 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. Furhtermore, the DeviceTree binding is a direct representation of the Linux driver model, and doesn't necessarily describe "the USB IP-block". This series therefor attempts to flatten the driver split, and operate the glue and core out of the same platform_device instance. And in order to do this, the DeviceTree representation of the IP block is flattened. As a side effect, much of the ACPI integration code is dropped. Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com> --- Bjorn Andersson (12): dt-bindings: usb: qcom,dwc3: Add qcom,sc8180x-dwc3 usb: dwc3: qcom: Rename dwc3 platform_device reference usb: dwc3: qcom: Merge resources from urs_usb device usb: dwc3: Expose core driver as library usb: dwc3: Override end of dwc3 memory resource usb: dwc3: qcom: Add dwc3 core reference in driver state usb: dwc3: qcom: Instantiate dwc3 core directly usb: dwc3: qcom: Inline the qscratch constants dt-bindings: usb: qcom,dwc3: Rename to "glue" dt-bindings: usb: qcom,dwc3: Introduce flattened qcom,dwc3 binding usb: dwc3: qcom: Flatten the Qualcomm dwc3 binding and implementation arm64: dts: qcom: sc8180x: flatten usb_sec node .../devicetree/bindings/usb/qcom,dwc3-glue.yaml | 626 +++++++++++++++++++++ .../devicetree/bindings/usb/qcom,dwc3.yaml | 321 ++++------- .../devicetree/bindings/usb/snps,dwc3.yaml | 14 +- .../arm64/boot/dts/qcom/sc8180x-lenovo-flex-5g.dts | 6 +- arch/arm64/boot/dts/qcom/sc8180x-primus.dts | 6 +- arch/arm64/boot/dts/qcom/sc8180x.dtsi | 34 +- drivers/usb/dwc3/core.c | 136 +++-- drivers/usb/dwc3/core.h | 10 + drivers/usb/dwc3/dwc3-qcom.c | 328 ++++++----- 9 files changed, 1057 insertions(+), 424 deletions(-) --- base-commit: 4d0515b235dec789578d135a5db586b25c5870cb change-id: 20231016-dwc3-refactor-931e3b08a8b9 Best regards,