| Message ID | 20250318-dwc3-refactor-v5-0-90ea6e5b3ba4@oss.qualcomm.com |
|---|---|
| Headers | show |
| Series | usb: dwc3: qcom: Flatten dwc3 structure | expand |
On Tue, 18 Mar 2025 14:05:00 -0500, 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 proper 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. > > Departing from previous versions' attempts at runtime-convert the > Devicetree representation is swapped out and instead a snapshot of the > current dwc3-qcom driver is proposed to be carried for a limited time. > > A patch to convert a single platform - sc8280xp - is included in the > series. The broader conversion will be submitted in a follow up series. > > --- > Changes in v5: > - Moved the snapshot commit first, to get a clean copy > - Add missing kernel-doc in glue.h > - Used a local "struct device" variable in PM functions to reduce the > patch size > - Replaced initialization with default values with zero-initalizing the > dwc3_probe_data in dwc3_probe() > - Add TODO about extcon, as a role-switch callback needs to be > implemented > - Corrected &usb_2 mmio region length > - Changes the timeline expressed in the commit message to suggest the > legacy driver to be dropped after next LTS > - Integrated qcom,dwc3.yaml changes since v4 > - Link to v4: https://lore.kernel.org/r/20250226-dwc3-refactor-v4-0-4415e7111e49@oss.qualcomm.com > > Changes in v4: > - dwc3_{init,uninit}() renamed to dwc3_core_probe() and dwc3_core_remove() > - dwc3_{suspend, resume, complete}() changed to dwc3_pm_*() > - Arguments to dwc3_core_probe() are wrapped in a struct to better > handle the expected growing list of parameters. > - Add the lost call to dwc3_core_remove() from the Qualcomm glue driver > - Removed now unused cleanup.h, of_address.h, and of_irq.h includes from > dwc3-qcom.c > - Link to v3: https://lore.kernel.org/r/20250113-dwc3-refactor-v3-0-d1722075df7b@oss.qualcomm.com > > Changes in v3: > - Replaced the handcoded migration logic of compatible, reg, interrupts, > phys with overlays. > - Move the migration logic (and overlays) to a new drivers/of/overlays > directory and apply this at postcore, so that it takes effect prior to > the relevant platform_devices are created > - struct dwc3 is embedded in the glue context, rather than having a > separate object allocated > - The hack of using of_address_to_resource() to avoid platform_resource > being stale is removed (thanks to applying migration at postcore) > - Link to v2: https://lore.kernel.org/r/20240811-dwc3-refactor-v2-0-91f370d61ad2@quicinc.com > > Changes in v2: > - Rewrite after ACPI removal, multiport support and interrupt fixes > - Completely changed strategy for DeviceTree binding, as previous idea > of using snps,dwc3 as a generic fallback required unreasonable changes > to that binding. > - Abandoned idea of supporting both flattened and unflattened device > model in the one driver. As Johan pointed out, it will leave the race > condition holes and will make the code harder to understand. > Furthermore, the role switching logic that we intend to introduce > following this would have depended on the user updating their > DeviceTree blobs. > - Per above, introduced the dynamic DeviceTree rewrite > - Link to v1: https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/ > > --- > Bjorn Andersson (7): > usb: dwc3: qcom: Snapshot driver for backwards compatibilty > dt-bindings: usb: Introduce qcom,snps-dwc3 > usb: dwc3: core: Expose core driver as library > usb: dwc3: core: Don't touch resets and clocks > usb: dwc3: qcom: Don't rely on drvdata during probe > usb: dwc3: qcom: Transition to flattened model > arm64: dts: qcom: sc8280x: Flatten the USB nodes > > .../devicetree/bindings/usb/qcom,dwc3.yaml | 13 +- > .../devicetree/bindings/usb/qcom,snps-dwc3.yaml | 620 ++++++++++++++ > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 12 +- > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 5 +- > arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 12 +- > .../boot/dts/qcom/sc8280xp-huawei-gaokun3.dts | 10 +- > .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 11 +- > .../boot/dts/qcom/sc8280xp-microsoft-arcata.dts | 10 +- > .../boot/dts/qcom/sc8280xp-microsoft-blackrock.dts | 18 +- > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 157 ++-- > drivers/usb/dwc3/Makefile | 1 + > drivers/usb/dwc3/core.c | 160 ++-- > drivers/usb/dwc3/dwc3-qcom-legacy.c | 935 +++++++++++++++++++++ > drivers/usb/dwc3/dwc3-qcom.c | 157 ++-- > drivers/usb/dwc3/glue.h | 35 + > 15 files changed, 1885 insertions(+), 271 deletions(-) > --- > base-commit: c4d4884b67802c41fd67399747165d65c770621a > change-id: 20231016-dwc3-refactor-931e3b08a8b9 > > Best regards, > -- > Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> > > > My bot found new DTB warnings on the .dts files added or changed in this series. Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings are fixed by another series. Ultimately, it is up to the platform maintainer whether these warnings are acceptable or not. No need to reply unless the platform maintainer has comments. If you already ran DT checks and didn't see these error(s), then make sure dt-schema is up to date: pip3 install dtschema --upgrade New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/qcom/' for 20250318-dwc3-refactor-v5-0-90ea6e5b3ba4@oss.qualcomm.com: arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a800000: #address-cells: 1 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a800000: #size-cells: 0 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a800000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'dr_mode', 'phy-names', 'phys', 'ports', 'ranges', 'snps,dis-u1-entry-quirk', 'snps,dis-u2-entry-quirk' were unexpected) from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dtb: usb@a800000: #address-cells: 1 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dtb: usb@a800000: #size-cells: 0 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dtb: usb@a800000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'dr_mode', 'phy-names', 'phys', 'ports', 'ranges', 'snps,dis-u1-entry-quirk', 'snps,dis-u2-entry-quirk' were unexpected) from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sa8295p-adp.dtb: usb@a800000: #address-cells: 1 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sa8295p-adp.dtb: usb@a800000: #size-cells: 0 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sa8295p-adp.dtb: usb@a800000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'dr_mode', 'phy-names', 'phys', 'ports', 'ranges', 'snps,dis-u1-entry-quirk', 'snps,dis-u2-entry-quirk' were unexpected) from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-huawei-gaokun3.dtb: usb@a800000: #address-cells: 1 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-huawei-gaokun3.dtb: usb@a800000: #size-cells: 0 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-huawei-gaokun3.dtb: usb@a800000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'dr_mode', 'phy-names', 'phys', 'ports', 'ranges', 'snps,dis-u1-entry-quirk', 'snps,dis-u2-entry-quirk' were unexpected) from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-microsoft-blackrock.dtb: usb@a800000: #address-cells: 1 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-microsoft-blackrock.dtb: usb@a800000: #size-cells: 0 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-microsoft-blackrock.dtb: usb@a800000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'dr_mode', 'phy-names', 'phys', 'ports', 'ranges', 'snps,dis-u1-entry-quirk', 'snps,dis-u2-entry-quirk' were unexpected) from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-crd.dtb: usb@a800000: #address-cells: 1 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-crd.dtb: usb@a800000: #size-cells: 0 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sc8280xp-crd.dtb: usb@a800000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'dr_mode', 'phy-names', 'phys', 'ports', 'ranges', 'snps,dis-u1-entry-quirk', 'snps,dis-u2-entry-quirk' were unexpected) from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: usb@a800000: #address-cells: 1 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# arch/arm64/boot/dts/qcom/sa8540p-ride.dtb: usb@a800000: #size-cells: 0 was expected from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml#
On Tue, Mar 18, 2025 at 05:26:02PM -0500, Rob Herring (Arm) wrote: > New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/qcom/' for 20250318-dwc3-refactor-v5-0-90ea6e5b3ba4@oss.qualcomm.com: > > arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a800000: #address-cells: 1 was expected > from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# > arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a800000: #size-cells: 0 was expected > from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# > arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dtb: usb@a800000: Unevaluated properties are not allowed ('#address-cells', '#size-cells', 'dr_mode', 'phy-names', 'phys', 'ports', 'ranges', 'snps,dis-u1-entry-quirk', 'snps,dis-u2-entry-quirk' were unexpected) > from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# I lost the hunk that removes #address-cells, #size-cells, and ranges from &usb_1 in patch 7. So, this error report is correct, but does not affect the implementation. Thanks for catching this, Rob. Regards, Bjorn
On 18/03/2025 20:05, Bjorn Andersson wrote: > When the core is integrated with glue, it's reasonable to assume that > the glue driver will have to touch the IP before/after the core takes > the hardware out and into reset. As such the glue must own these > resources and be allowed to turn them on/off outside the core's > handling. > > Allow the platform or glue layer to indicate if the core logic for > clocks and resets should be skipped to deal with this. > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> > --- > drivers/usb/dwc3/core.c | 20 +++++++++++--------- > drivers/usb/dwc3/glue.h | 3 +++ > 2 files changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index b428b6fc3d0a55811e2f75d33d79df4b0c67dcac..77a9848a0ac70fbe563988cecbe489130989aadc 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -2198,15 +2198,17 @@ int dwc3_core_probe(const struct dwc3_probe_data *data) > if (IS_ERR(dwc->usb_psy)) > return dev_err_probe(dev, PTR_ERR(dwc->usb_psy), "couldn't get usb power supply\n"); > > - 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 (!data->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) > @@ -2321,7 +2323,7 @@ EXPORT_SYMBOL_GPL(dwc3_core_probe); > > static int dwc3_probe(struct platform_device *pdev) > { > - struct dwc3_probe_data probe_data; > + struct dwc3_probe_data probe_data = {}; This should go in the previous patch Neil > struct resource *res; > struct dwc3 *dwc; > > diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h > index e73cfc466012f07214291abe56454934ab014013..703d40c189565b1e28ae28afb8728b78f4cd2fca 100644 > --- a/drivers/usb/dwc3/glue.h > +++ b/drivers/usb/dwc3/glue.h > @@ -13,10 +13,13 @@ > * dwc3_probe_data: Initialization parameters passed to dwc3_core_probe() > * @dwc: Reference to dwc3 context structure > * @res: resource for the DWC3 core mmio region > + * @ignore_clocks_and_resets: clocks and resets defined for the device should > + * be ignored by the DWC3 core, as they are managed by the glue > */ > struct dwc3_probe_data { > struct dwc3 *dwc; > struct resource *res; > + bool ignore_clocks_and_resets; > }; > > int dwc3_core_probe(const struct dwc3_probe_data *data); >
Hi, On 18/03/2025 20:05, Bjorn Andersson wrote: > The Qualcomm USB glue is not separate of the Synopsys DWC3 core and > several of the snps,dwc3 properties (such as clocks and reset) conflicts > in expectation with the Qualcomm integration. > > Using the newly split out Synopsys DWC3 core properties, describe the > Qualcomm USB block in a single block. The new binding is a copy of > qcom,dwc3 with the needed modifications. > > It would have been convenient to retain the two structures with the same > compatibles, but as there exist no way to select a binding based on the > absence of a subnode/patternProperty, a new generic compatible is > introduced to describe this binding. > > To avoid redefining all the platform-specific compatibles, "select" is > used to tell the DeviceTree validator which binding to use solely on the > generic compatible. (Otherwise if the specific compatible matches during > validation, the generic one must match as well) > > Mark qcom,dwc3 deprecated, to favor expressing future platforms using > the new combined binding. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com> > --- > .../devicetree/bindings/usb/qcom,dwc3.yaml | 13 +- > .../devicetree/bindings/usb/qcom,snps-dwc3.yaml | 620 +++++++++++++++++++++ > 2 files changed, 632 insertions(+), 1 deletion(-) > When converting sm8550 and sm8650, I get: usb@a600000: Unevaluated properties are not allowed ('dma-coherent' was unexpected) from schema $id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml# So I guess it should be added. Neil
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 proper 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. Departing from previous versions' attempts at runtime-convert the Devicetree representation is swapped out and instead a snapshot of the current dwc3-qcom driver is proposed to be carried for a limited time. A patch to convert a single platform - sc8280xp - is included in the series. The broader conversion will be submitted in a follow up series. --- Changes in v5: - Moved the snapshot commit first, to get a clean copy - Add missing kernel-doc in glue.h - Used a local "struct device" variable in PM functions to reduce the patch size - Replaced initialization with default values with zero-initalizing the dwc3_probe_data in dwc3_probe() - Add TODO about extcon, as a role-switch callback needs to be implemented - Corrected &usb_2 mmio region length - Changes the timeline expressed in the commit message to suggest the legacy driver to be dropped after next LTS - Integrated qcom,dwc3.yaml changes since v4 - Link to v4: https://lore.kernel.org/r/20250226-dwc3-refactor-v4-0-4415e7111e49@oss.qualcomm.com Changes in v4: - dwc3_{init,uninit}() renamed to dwc3_core_probe() and dwc3_core_remove() - dwc3_{suspend, resume, complete}() changed to dwc3_pm_*() - Arguments to dwc3_core_probe() are wrapped in a struct to better handle the expected growing list of parameters. - Add the lost call to dwc3_core_remove() from the Qualcomm glue driver - Removed now unused cleanup.h, of_address.h, and of_irq.h includes from dwc3-qcom.c - Link to v3: https://lore.kernel.org/r/20250113-dwc3-refactor-v3-0-d1722075df7b@oss.qualcomm.com Changes in v3: - Replaced the handcoded migration logic of compatible, reg, interrupts, phys with overlays. - Move the migration logic (and overlays) to a new drivers/of/overlays directory and apply this at postcore, so that it takes effect prior to the relevant platform_devices are created - struct dwc3 is embedded in the glue context, rather than having a separate object allocated - The hack of using of_address_to_resource() to avoid platform_resource being stale is removed (thanks to applying migration at postcore) - Link to v2: https://lore.kernel.org/r/20240811-dwc3-refactor-v2-0-91f370d61ad2@quicinc.com Changes in v2: - Rewrite after ACPI removal, multiport support and interrupt fixes - Completely changed strategy for DeviceTree binding, as previous idea of using snps,dwc3 as a generic fallback required unreasonable changes to that binding. - Abandoned idea of supporting both flattened and unflattened device model in the one driver. As Johan pointed out, it will leave the race condition holes and will make the code harder to understand. Furthermore, the role switching logic that we intend to introduce following this would have depended on the user updating their DeviceTree blobs. - Per above, introduced the dynamic DeviceTree rewrite - Link to v1: https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/ --- Bjorn Andersson (7): usb: dwc3: qcom: Snapshot driver for backwards compatibilty dt-bindings: usb: Introduce qcom,snps-dwc3 usb: dwc3: core: Expose core driver as library usb: dwc3: core: Don't touch resets and clocks usb: dwc3: qcom: Don't rely on drvdata during probe usb: dwc3: qcom: Transition to flattened model arm64: dts: qcom: sc8280x: Flatten the USB nodes .../devicetree/bindings/usb/qcom,dwc3.yaml | 13 +- .../devicetree/bindings/usb/qcom,snps-dwc3.yaml | 620 ++++++++++++++ arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 12 +- arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 5 +- arch/arm64/boot/dts/qcom/sc8280xp-crd.dts | 12 +- .../boot/dts/qcom/sc8280xp-huawei-gaokun3.dts | 10 +- .../dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 11 +- .../boot/dts/qcom/sc8280xp-microsoft-arcata.dts | 10 +- .../boot/dts/qcom/sc8280xp-microsoft-blackrock.dts | 18 +- arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 157 ++-- drivers/usb/dwc3/Makefile | 1 + drivers/usb/dwc3/core.c | 160 ++-- drivers/usb/dwc3/dwc3-qcom-legacy.c | 935 +++++++++++++++++++++ drivers/usb/dwc3/dwc3-qcom.c | 157 ++-- drivers/usb/dwc3/glue.h | 35 + 15 files changed, 1885 insertions(+), 271 deletions(-) --- base-commit: c4d4884b67802c41fd67399747165d65c770621a change-id: 20231016-dwc3-refactor-931e3b08a8b9 Best regards,