* [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure
@ 2025-02-27 0:17 Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3 Bjorn Andersson
` (6 more replies)
0 siblings, 7 replies; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
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 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):
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: Snapshot driver for backwards compatibilty
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 | 619 ++++++++++++++
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 | 174 ++--
drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 +++++++++++++++++++++
drivers/usb/dwc3/dwc3-qcom.c | 152 ++--
drivers/usb/dwc3/glue.h | 33 +
15 files changed, 1881 insertions(+), 280 deletions(-)
---
base-commit: c1136f35c7d1ce2517e875884644a44da6121c35
change-id: 20231016-dwc3-refactor-931e3b08a8b9
Best regards,
--
Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
@ 2025-02-27 0:17 ` Bjorn Andersson
2025-02-28 22:36 ` Rob Herring
2025-02-27 0:17 ` [PATCH v4 2/7] usb: dwc3: core: Expose core driver as library Bjorn Andersson
` (5 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
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 | 619 +++++++++++++++++++++
2 files changed, 631 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index a2b3cf625e5b..6d818e6dddbc 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -4,11 +4,22 @@
$id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
$schema: http://devicetree.org/meta-schemas/core.yaml#
-title: Qualcomm SuperSpeed DWC3 USB SoC controller
+title: Legacy Qualcomm SuperSpeed DWC3 USB SoC controller
maintainers:
- Wesley Cheng <quic_wcheng@quicinc.com>
+# Use the combined qcom,snps-dwc3 instead
+deprecated: true
+
+select:
+ properties:
+ compatible:
+ contains:
+ const: qcom,dwc3
+ required:
+ - compatible
+
properties:
compatible:
items:
diff --git a/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
new file mode 100644
index 000000000000..37af52e01803
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
@@ -0,0 +1,619 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SuperSpeed DWC3 USB SoC controller
+
+maintainers:
+ - Wesley Cheng <quic_wcheng@quicinc.com>
+
+description:
+ Describes the Qualcomm USB block, based on Synopsys DWC3.
+
+select:
+ properties:
+ compatible:
+ contains:
+ const: qcom,snps-dwc3
+ required:
+ - compatible
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - qcom,ipq4019-dwc3
+ - qcom,ipq5018-dwc3
+ - qcom,ipq5332-dwc3
+ - qcom,ipq5424-dwc3
+ - qcom,ipq6018-dwc3
+ - qcom,ipq8064-dwc3
+ - qcom,ipq8074-dwc3
+ - qcom,ipq9574-dwc3
+ - qcom,msm8953-dwc3
+ - qcom,msm8994-dwc3
+ - qcom,msm8996-dwc3
+ - qcom,msm8998-dwc3
+ - qcom,qcm2290-dwc3
+ - qcom,qcs404-dwc3
+ - qcom,qcs615-dwc3
+ - qcom,qcs8300-dwc3
+ - qcom,qdu1000-dwc3
+ - qcom,sa8775p-dwc3
+ - qcom,sar2130p-dwc3
+ - qcom,sc7180-dwc3
+ - qcom,sc7280-dwc3
+ - qcom,sc8180x-dwc3
+ - qcom,sc8180x-dwc3-mp
+ - qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
+ - qcom,sdm660-dwc3
+ - qcom,sdm670-dwc3
+ - qcom,sdm845-dwc3
+ - qcom,sdx55-dwc3
+ - qcom,sdx65-dwc3
+ - qcom,sdx75-dwc3
+ - qcom,sm4250-dwc3
+ - qcom,sm6115-dwc3
+ - qcom,sm6125-dwc3
+ - qcom,sm6350-dwc3
+ - qcom,sm6375-dwc3
+ - qcom,sm8150-dwc3
+ - qcom,sm8250-dwc3
+ - qcom,sm8350-dwc3
+ - qcom,sm8450-dwc3
+ - qcom,sm8550-dwc3
+ - qcom,sm8650-dwc3
+ - qcom,x1e80100-dwc3
+ - const: qcom,snps-dwc3
+
+ reg:
+ description: Offset and length of register set for QSCRATCH wrapper
+ maxItems: 1
+
+ power-domains:
+ description: specifies a phandle to PM domain provider node
+ maxItems: 1
+
+ required-opps:
+ maxItems: 1
+
+ clocks:
+ description: |
+ Several clocks are used, depending on the variant. Typical ones are::
+ - cfg_noc:: System Config NOC clock.
+ - core:: Master/Core clock, has to be >= 125 MHz for SS operation and >=
+ 60MHz for HS operation.
+ - iface:: System bus AXI clock.
+ - sleep:: Sleep clock, used for wakeup when USB3 core goes into low
+ power mode (U3).
+ - mock_utmi:: Mock utmi clock needed for ITP/SOF generation in host
+ mode. Its frequency should be 19.2MHz.
+ minItems: 1
+ maxItems: 9
+
+ clock-names:
+ minItems: 1
+ maxItems: 9
+
+ iommus:
+ maxItems: 1
+
+ resets:
+ maxItems: 1
+
+ interconnects:
+ maxItems: 2
+
+ interconnect-names:
+ items:
+ - const: usb-ddr
+ - const: apps-usb
+
+ interrupts:
+ description: |
+ Different types of interrupts are used based on HS PHY used on target:
+ - dwc_usb3: Core DWC3 interrupt
+ - pwr_event: Used for wakeup based on other power events.
+ - hs_phy_irq: Apart from DP/DM/QUSB2 PHY interrupts, there is
+ hs_phy_irq which is not triggered by default and its
+ functionality is mutually exclusive to that of
+ {dp/dm}_hs_phy_irq and qusb2_phy_irq.
+ - qusb2_phy: SoCs with QUSB2 PHY do not have separate DP/DM IRQs and
+ expose only a single IRQ whose behavior can be modified
+ by the QUSB2PHY_INTR_CTRL register. The required DPSE/
+ DMSE configuration is done in QUSB2PHY_INTR_CTRL register
+ of PHY address space.
+ - {dp/dm}_hs_phy_irq: These IRQ's directly reflect changes on the DP/
+ DM pads of the SoC. These are used for wakeup
+ only on SoCs with non-QUSB2 targets with
+ exception of SDM670/SDM845/SM6350.
+ - ss_phy_irq: Used for remote wakeup in Super Speed mode of operation.
+ minItems: 3
+ maxItems: 19
+
+ interrupt-names:
+ minItems: 3
+ maxItems: 19
+
+ qcom,select-utmi-as-pipe-clk:
+ description:
+ If present, disable USB3 pipe_clk requirement.
+ Used when dwc3 operates without SSPHY and only
+ HS/FS/LS modes are supported.
+ type: boolean
+
+ wakeup-source: true
+
+# Required child node:
+
+required:
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+ - interrupts
+ - interrupt-names
+
+allOf:
+ - $ref: snps,dwc3-common.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq4019-dwc3
+ - qcom,ipq5332-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 3
+ clock-names:
+ items:
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq8064-dwc3
+ then:
+ properties:
+ clocks:
+ items:
+ - description: Master/Core clock, has to be >= 125 MHz
+ for SS operation and >= 60MHz for HS operation.
+ clock-names:
+ items:
+ - const: core
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq9574-dwc3
+ - qcom,msm8953-dwc3
+ - qcom,msm8996-dwc3
+ - qcom,msm8998-dwc3
+ - qcom,qcs8300-dwc3
+ - qcom,sa8775p-dwc3
+ - qcom,sc7180-dwc3
+ - qcom,sc7280-dwc3
+ - qcom,sdm670-dwc3
+ - qcom,sdm845-dwc3
+ - qcom,sdx55-dwc3
+ - qcom,sdx65-dwc3
+ - qcom,sdx75-dwc3
+ - qcom,sm6350-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 5
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq6018-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 3
+ maxItems: 4
+ clock-names:
+ oneOf:
+ - items:
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+ - items:
+ - const: cfg_noc
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq8074-dwc3
+ - qcom,qdu1000-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 4
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq5018-dwc3
+ - qcom,msm8994-dwc3
+ - qcom,qcs404-dwc3
+ then:
+ properties:
+ clocks:
+ maxItems: 4
+ clock-names:
+ items:
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sc8280xp-dwc3
+ - qcom,sc8280xp-dwc3-mp
+ - qcom,x1e80100-dwc3
+ - qcom,x1e80100-dwc3-mp
+ then:
+ properties:
+ clocks:
+ maxItems: 9
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - const: noc_aggr
+ - const: noc_aggr_north
+ - const: noc_aggr_south
+ - const: noc_sys
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sdm660-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 4
+ maxItems: 5
+ clock-names:
+ oneOf:
+ - items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - items:
+ - const: cfg_noc
+ - const: core
+ - const: sleep
+ - const: mock_utmi
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,qcm2290-dwc3
+ - qcom,qcs615-dwc3
+ - qcom,sar2130p-dwc3
+ - qcom,sc8180x-dwc3
+ - qcom,sc8180x-dwc3-mp
+ - qcom,sm6115-dwc3
+ - qcom,sm6125-dwc3
+ - qcom,sm8150-dwc3
+ - qcom,sm8250-dwc3
+ - qcom,sm8450-dwc3
+ - qcom,sm8550-dwc3
+ - qcom,sm8650-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 6
+ clock-names:
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - const: xo
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sm8350-dwc3
+ then:
+ properties:
+ clocks:
+ minItems: 5
+ maxItems: 6
+ clock-names:
+ minItems: 5
+ items:
+ - const: cfg_noc
+ - const: core
+ - const: iface
+ - const: sleep
+ - const: mock_utmi
+ - const: xo
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq5018-dwc3
+ - qcom,ipq6018-dwc3
+ - qcom,ipq8074-dwc3
+ - qcom,msm8953-dwc3
+ - qcom,msm8998-dwc3
+ then:
+ properties:
+ interrupts:
+ minItems: 3
+ maxItems: 4
+ interrupt-names:
+ items:
+ - const: dwc_usb3
+ - const: pwr_event
+ - const: qusb2_phy
+ - const: ss_phy_irq
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,msm8996-dwc3
+ - qcom,qcs404-dwc3
+ - qcom,sdm660-dwc3
+ - qcom,sm6115-dwc3
+ - qcom,sm6125-dwc3
+ then:
+ properties:
+ interrupts:
+ minItems: 4
+ maxItems: 5
+ interrupt-names:
+ items:
+ - const: dwc_usb3
+ - const: pwr_event
+ - const: qusb2_phy
+ - const: hs_phy_irq
+ - const: ss_phy_irq
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq5332-dwc3
+ then:
+ properties:
+ interrupts:
+ maxItems: 4
+ interrupt-names:
+ items:
+ - const: dwc_usb3
+ - const: pwr_event
+ - const: dp_hs_phy_irq
+ - const: dm_hs_phy_irq
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,x1e80100-dwc3
+ then:
+ properties:
+ interrupts:
+ maxItems: 5
+ interrupt-names:
+ items:
+ - const: dwc_usb3
+ - const: pwr_event
+ - const: dp_hs_phy_irq
+ - const: dm_hs_phy_irq
+ - const: ss_phy_irq
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,ipq4019-dwc3
+ - qcom,ipq8064-dwc3
+ - qcom,msm8994-dwc3
+ - qcom,qcs615-dwc3
+ - qcom,qcs8300-dwc3
+ - qcom,qdu1000-dwc3
+ - qcom,sa8775p-dwc3
+ - qcom,sc7180-dwc3
+ - qcom,sc7280-dwc3
+ - qcom,sc8180x-dwc3
+ - qcom,sc8280xp-dwc3
+ - qcom,sdm670-dwc3
+ - qcom,sdm845-dwc3
+ - qcom,sdx55-dwc3
+ - qcom,sdx65-dwc3
+ - qcom,sdx75-dwc3
+ - qcom,sm4250-dwc3
+ - qcom,sm6350-dwc3
+ - qcom,sm8150-dwc3
+ - qcom,sm8250-dwc3
+ - qcom,sm8350-dwc3
+ - qcom,sm8450-dwc3
+ - qcom,sm8550-dwc3
+ - qcom,sm8650-dwc3
+ then:
+ properties:
+ interrupts:
+ minItems: 5
+ maxItems: 6
+ interrupt-names:
+ items:
+ - const: dwc_usb3
+ - const: pwr_event
+ - const: hs_phy_irq
+ - const: dp_hs_phy_irq
+ - const: dm_hs_phy_irq
+ - const: ss_phy_irq
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sc8180x-dwc3-mp
+ - qcom,x1e80100-dwc3-mp
+ then:
+ properties:
+ interrupts:
+ minItems: 11
+ maxItems: 11
+ interrupt-names:
+ items:
+ - const: dwc_usb3
+ - const: pwr_event_1
+ - const: pwr_event_2
+ - const: hs_phy_1
+ - const: hs_phy_2
+ - const: dp_hs_phy_1
+ - const: dm_hs_phy_1
+ - const: dp_hs_phy_2
+ - const: dm_hs_phy_2
+ - const: ss_phy_1
+ - const: ss_phy_2
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - qcom,sc8280xp-dwc3-mp
+ then:
+ properties:
+ interrupts:
+ minItems: 19
+ maxItems: 19
+ interrupt-names:
+ items:
+ - const: dwc_usb3
+ - const: pwr_event_1
+ - const: pwr_event_2
+ - const: pwr_event_3
+ - const: pwr_event_4
+ - const: hs_phy_1
+ - const: hs_phy_2
+ - const: hs_phy_3
+ - const: hs_phy_4
+ - const: dp_hs_phy_1
+ - const: dm_hs_phy_1
+ - const: dp_hs_phy_2
+ - const: dm_hs_phy_2
+ - const: dp_hs_phy_3
+ - const: dm_hs_phy_3
+ - const: dp_hs_phy_4
+ - const: dm_hs_phy_4
+ - const: ss_phy_1
+ - const: ss_phy_2
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/qcom,gcc-sdm845.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/interrupt-controller/irq.h>
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ usb@a600000 {
+ compatible = "qcom,sdm845-dwc3", "qcom,snps-dwc3";
+ reg = <0 0x0a600000 0 0x100000>;
+
+ clocks = <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
+ <&gcc GCC_USB30_PRIM_MASTER_CLK>,
+ <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>,
+ <&gcc GCC_USB30_PRIM_SLEEP_CLK>,
+ <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>;
+ clock-names = "cfg_noc",
+ "core",
+ "iface",
+ "sleep",
+ "mock_utmi";
+
+ assigned-clocks = <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
+ <&gcc GCC_USB30_PRIM_MASTER_CLK>;
+ assigned-clock-rates = <19200000>, <150000000>;
+
+ interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 131 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 489 IRQ_TYPE_EDGE_BOTH>,
+ <GIC_SPI 488 IRQ_TYPE_EDGE_BOTH>,
+ <GIC_SPI 486 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "dwc_usb3", "pwr_event", "hs_phy_irq",
+ "dp_hs_phy_irq", "dm_hs_phy_irq", "ss_phy_irq";
+
+ power-domains = <&gcc USB30_PRIM_GDSC>;
+
+ resets = <&gcc GCC_USB30_PRIM_BCR>;
+
+ iommus = <&apps_smmu 0x740 0>;
+ snps,dis_u2_susphy_quirk;
+ snps,dis_enblslpm_quirk;
+ phys = <&usb_1_hsphy>, <&usb_1_ssphy>;
+ phy-names = "usb2-phy", "usb3-phy";
+ };
+ };
+...
--
2.45.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 2/7] usb: dwc3: core: Expose core driver as library
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3 Bjorn Andersson
@ 2025-02-27 0:17 ` Bjorn Andersson
2025-02-27 19:17 ` Frank Li
2025-02-27 0:17 ` [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks Bjorn Andersson
` (4 subsequent siblings)
6 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
The DWC3 IP block is handled by three distinct device drivers: XHCI,
DWC3 core and a platform specific (optional) DWC3 glue driver.
This has resulted in, at least in the case of the Qualcomm glue, the
presence of a number of layering violations, where the glue code either
can't handle, or has to work around, the fact that core might not probe
deterministically.
An example of this is that the suspend path should operate slightly
different depending on the device operating in host or peripheral mode,
and the only way to determine the operating state is to peek into the
core's drvdata.
The Qualcomm glue driver is expected to make updates in the qscratch
register region (the "glue" region) during role switch events, but with
the glue and core split using the driver model, there is no reasonable
way to introduce listeners for mode changes.
Split the dwc3 core platform_driver callbacks and their implementation
and export the implementation, to make it possible to deterministically
instantiate the dwc3 core as part of the dwc3 glue drivers and to
allow flattening of the DeviceTree representation.
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
drivers/usb/dwc3/core.c | 155 +++++++++++++++++++++++++++++++-----------------
drivers/usb/dwc3/glue.h | 32 ++++++++++
2 files changed, 133 insertions(+), 54 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index dfa1b5fe48dc..d9f0a6782d36 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -36,6 +36,7 @@
#include "core.h"
#include "gadget.h"
+#include "glue.h"
#include "io.h"
#include "debug.h"
@@ -2137,27 +2138,16 @@ static struct power_supply *dwc3_get_usb_power_supply(struct dwc3 *dwc)
return usb_psy;
}
-static int dwc3_probe(struct platform_device *pdev)
+int dwc3_core_probe(const struct dwc3_probe_data *data)
{
- struct device *dev = &pdev->dev;
- struct resource *res, dwc_res;
+ struct dwc3 *dwc = data->dwc;
+ struct device *dev = dwc->dev;
+ struct resource dwc_res;
unsigned int hw_mode;
void __iomem *regs;
- struct dwc3 *dwc;
+ struct resource *res = data->res;
int ret;
- dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
- if (!dwc)
- return -ENOMEM;
-
- dwc->dev = dev;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(dev, "missing memory resource\n");
- return -ENODEV;
- }
-
dwc->xhci_resources[0].start = res->start;
dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
DWC3_XHCI_REGS_END;
@@ -2221,7 +2211,7 @@ static int dwc3_probe(struct platform_device *pdev)
goto err_disable_clks;
}
- platform_set_drvdata(pdev, dwc);
+ dev_set_drvdata(dev, dwc);
dwc3_cache_hwparams(dwc);
if (!dwc->sysdev_is_parent &&
@@ -2316,12 +2306,35 @@ static int dwc3_probe(struct platform_device *pdev)
return ret;
}
+EXPORT_SYMBOL_GPL(dwc3_core_probe);
-static void dwc3_remove(struct platform_device *pdev)
+static int dwc3_probe(struct platform_device *pdev)
{
- struct dwc3 *dwc = platform_get_drvdata(pdev);
+ struct dwc3_probe_data probe_data;
+ struct resource *res;
+ struct dwc3 *dwc;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "missing memory resource\n");
+ return -ENODEV;
+ }
+
+ dwc = devm_kzalloc(&pdev->dev, sizeof(*dwc), GFP_KERNEL);
+ if (!dwc)
+ return -ENOMEM;
- pm_runtime_get_sync(&pdev->dev);
+ dwc->dev = &pdev->dev;
+
+ probe_data.dwc = dwc;
+ probe_data.res = res;
+
+ return dwc3_core_probe(&probe_data);
+}
+
+void dwc3_core_remove(struct dwc3 *dwc)
+{
+ pm_runtime_get_sync(dwc->dev);
dwc3_core_exit_mode(dwc);
dwc3_debugfs_exit(dwc);
@@ -2329,22 +2342,28 @@ static void dwc3_remove(struct platform_device *pdev)
dwc3_core_exit(dwc);
dwc3_ulpi_exit(dwc);
- pm_runtime_allow(&pdev->dev);
- pm_runtime_disable(&pdev->dev);
- pm_runtime_dont_use_autosuspend(&pdev->dev);
- pm_runtime_put_noidle(&pdev->dev);
+ pm_runtime_allow(dwc->dev);
+ pm_runtime_disable(dwc->dev);
+ pm_runtime_dont_use_autosuspend(dwc->dev);
+ pm_runtime_put_noidle(dwc->dev);
/*
* HACK: Clear the driver data, which is currently accessed by parent
* glue drivers, before allowing the parent to suspend.
*/
- platform_set_drvdata(pdev, NULL);
- pm_runtime_set_suspended(&pdev->dev);
+ dev_set_drvdata(dwc->dev, NULL);
+ pm_runtime_set_suspended(dwc->dev);
dwc3_free_event_buffers(dwc);
if (dwc->usb_psy)
power_supply_put(dwc->usb_psy);
}
+EXPORT_SYMBOL_GPL(dwc3_core_remove);
+
+static void dwc3_remove(struct platform_device *pdev)
+{
+ dwc3_core_remove(platform_get_drvdata(pdev));
+}
#ifdef CONFIG_PM
static int dwc3_core_init_for_resume(struct dwc3 *dwc)
@@ -2533,9 +2552,8 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
return 0;
}
-static int dwc3_runtime_suspend(struct device *dev)
+int dwc3_runtime_suspend(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
if (dwc3_runtime_checks(dwc))
@@ -2547,10 +2565,10 @@ static int dwc3_runtime_suspend(struct device *dev)
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_runtime_suspend);
-static int dwc3_runtime_resume(struct device *dev)
+int dwc3_runtime_resume(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
ret = dwc3_resume_common(dwc, PMSG_AUTO_RESUME);
@@ -2571,15 +2589,14 @@ static int dwc3_runtime_resume(struct device *dev)
break;
}
- pm_runtime_mark_last_busy(dev);
+ pm_runtime_mark_last_busy(dwc->dev);
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_runtime_resume);
-static int dwc3_runtime_idle(struct device *dev)
+int dwc3_runtime_idle(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
-
switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
if (dwc3_runtime_checks(dwc))
@@ -2591,53 +2608,68 @@ static int dwc3_runtime_idle(struct device *dev)
break;
}
- pm_runtime_mark_last_busy(dev);
- pm_runtime_autosuspend(dev);
+ pm_runtime_mark_last_busy(dwc->dev);
+ pm_runtime_autosuspend(dwc->dev);
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_runtime_idle);
+
+static int dwc3_plat_runtime_suspend(struct device *dev)
+{
+ return dwc3_runtime_suspend(dev_get_drvdata(dev));
+}
+
+static int dwc3_plat_runtime_resume(struct device *dev)
+{
+ return dwc3_runtime_resume(dev_get_drvdata(dev));
+}
+
+static int dwc3_plat_runtime_idle(struct device *dev)
+{
+ return dwc3_runtime_idle(dev_get_drvdata(dev));
+}
#endif /* CONFIG_PM */
#ifdef CONFIG_PM_SLEEP
-static int dwc3_suspend(struct device *dev)
+int dwc3_pm_suspend(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret;
ret = dwc3_suspend_common(dwc, PMSG_SUSPEND);
if (ret)
return ret;
- pinctrl_pm_select_sleep_state(dev);
+ pinctrl_pm_select_sleep_state(dwc->dev);
return 0;
}
+EXPORT_SYMBOL_GPL(dwc3_pm_suspend);
-static int dwc3_resume(struct device *dev)
+int dwc3_pm_resume(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
int ret = 0;
- pinctrl_pm_select_default_state(dev);
+ pinctrl_pm_select_default_state(dwc->dev);
- pm_runtime_disable(dev);
- ret = pm_runtime_set_active(dev);
+ pm_runtime_disable(dwc->dev);
+ ret = pm_runtime_set_active(dwc->dev);
if (ret)
goto out;
ret = dwc3_resume_common(dwc, PMSG_RESUME);
if (ret)
- pm_runtime_set_suspended(dev);
+ pm_runtime_set_suspended(dwc->dev);
out:
- pm_runtime_enable(dev);
+ pm_runtime_enable(dwc->dev);
return ret;
}
+EXPORT_SYMBOL_GPL(dwc3_pm_resume);
-static void dwc3_complete(struct device *dev)
+void dwc3_pm_complete(struct dwc3 *dwc)
{
- struct dwc3 *dwc = dev_get_drvdata(dev);
u32 reg;
if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
@@ -2647,21 +2679,36 @@ static void dwc3_complete(struct device *dev)
dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
}
}
+EXPORT_SYMBOL_GPL(dwc3_pm_complete);
+
+static int dwc3_plat_suspend(struct device *dev)
+{
+ return dwc3_pm_suspend(dev_get_drvdata(dev));
+}
+
+static int dwc3_plat_resume(struct device *dev)
+{
+ return dwc3_pm_resume(dev_get_drvdata(dev));
+}
+
+static void dwc3_plat_complete(struct device *dev)
+{
+ dwc3_pm_complete(dev_get_drvdata(dev));
+}
#else
-#define dwc3_complete NULL
+#define dwc3_plat_complete NULL
#endif /* CONFIG_PM_SLEEP */
static const struct dev_pm_ops dwc3_dev_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
- .complete = dwc3_complete,
-
+ SET_SYSTEM_SLEEP_PM_OPS(dwc3_plat_suspend, dwc3_plat_resume)
+ .complete = dwc3_plat_complete,
/*
* Runtime suspend halts the controller on disconnection. It relies on
* platforms with custom connection notification to start the controller
* again.
*/
- SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
- dwc3_runtime_idle)
+ SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume,
+ dwc3_plat_runtime_idle)
};
#ifdef CONFIG_OF
diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
new file mode 100644
index 000000000000..e73cfc466012
--- /dev/null
+++ b/drivers/usb/dwc3/glue.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * glue.h - DesignWare USB3 DRD glue header
+ */
+
+#ifndef __DRIVERS_USB_DWC3_GLUE_H
+#define __DRIVERS_USB_DWC3_GLUE_H
+
+#include <linux/types.h>
+#include "core.h"
+
+/**
+ * dwc3_probe_data: Initialization parameters passed to dwc3_core_probe()
+ * @dwc: Reference to dwc3 context structure
+ * @res: resource for the DWC3 core mmio region
+ */
+struct dwc3_probe_data {
+ struct dwc3 *dwc;
+ struct resource *res;
+};
+
+int dwc3_core_probe(const struct dwc3_probe_data *data);
+void dwc3_core_remove(struct dwc3 *dwc);
+
+int dwc3_runtime_suspend(struct dwc3 *dwc);
+int dwc3_runtime_resume(struct dwc3 *dwc);
+int dwc3_runtime_idle(struct dwc3 *dwc);
+int dwc3_pm_suspend(struct dwc3 *dwc);
+int dwc3_pm_resume(struct dwc3 *dwc);
+void dwc3_pm_complete(struct dwc3 *dwc);
+
+#endif
--
2.45.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3 Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 2/7] usb: dwc3: core: Expose core driver as library Bjorn Andersson
@ 2025-02-27 0:17 ` Bjorn Andersson
2025-02-27 4:24 ` Dmitry Baryshkov
2025-02-27 19:22 ` Frank Li
2025-02-27 0:17 ` [PATCH v4 4/7] usb: dwc3: qcom: Don't rely on drvdata during probe Bjorn Andersson
` (3 subsequent siblings)
6 siblings, 2 replies; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
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.
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
drivers/usb/dwc3/core.c | 19 +++++++++++--------
drivers/usb/dwc3/glue.h | 1 +
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index d9f0a6782d36..aecdde8dc999 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -2187,15 +2187,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)
@@ -2328,6 +2330,7 @@ static int dwc3_probe(struct platform_device *pdev)
probe_data.dwc = dwc;
probe_data.res = res;
+ probe_data.ignore_clocks_and_resets = false;
return dwc3_core_probe(&probe_data);
}
diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
index e73cfc466012..1ddb451bdbd0 100644
--- a/drivers/usb/dwc3/glue.h
+++ b/drivers/usb/dwc3/glue.h
@@ -17,6 +17,7 @@
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);
--
2.45.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 4/7] usb: dwc3: qcom: Don't rely on drvdata during probe
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
` (2 preceding siblings ...)
2025-02-27 0:17 ` [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks Bjorn Andersson
@ 2025-02-27 0:17 ` Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty Bjorn Andersson
` (2 subsequent siblings)
6 siblings, 0 replies; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
With the upcoming transition to a model where DWC3 core and glue operate
on a single struct device the drvdata datatype will change to be owned
by the core.
The drvdata is however used by the Qualcomm DWC3 glue to pass the qcom
glue context around before the core is allocated.
Remove this problem, and clean up the code, by passing the dwc3_qcom
struct around during probe, instead of acquiring it from the drvdata.
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
drivers/usb/dwc3/dwc3-qcom.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 58683bb672e9..50b1da845113 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -547,9 +547,10 @@ static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
return ret;
}
-static int dwc3_qcom_setup_port_irq(struct platform_device *pdev, int port_index, bool is_multiport)
+static int dwc3_qcom_setup_port_irq(struct dwc3_qcom *qcom,
+ struct platform_device *pdev,
+ int port_index, bool is_multiport)
{
- struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
const char *irq_name;
int irq;
int ret;
@@ -634,9 +635,8 @@ static int dwc3_qcom_find_num_ports(struct platform_device *pdev)
return DWC3_QCOM_MAX_PORTS;
}
-static int dwc3_qcom_setup_irq(struct platform_device *pdev)
+static int dwc3_qcom_setup_irq(struct dwc3_qcom *qcom, struct platform_device *pdev)
{
- struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
bool is_multiport;
int ret;
int i;
@@ -645,7 +645,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
is_multiport = (qcom->num_ports > 1);
for (i = 0; i < qcom->num_ports; i++) {
- ret = dwc3_qcom_setup_port_irq(pdev, i, is_multiport);
+ ret = dwc3_qcom_setup_port_irq(qcom, pdev, i, is_multiport);
if (ret)
return ret;
}
@@ -700,9 +700,8 @@ static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
return 0;
}
-static int dwc3_qcom_of_register_core(struct platform_device *pdev)
+static int dwc3_qcom_of_register_core(struct dwc3_qcom *qcom, struct platform_device *pdev)
{
- struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
struct device_node *np = pdev->dev.of_node;
struct device *dev = &pdev->dev;
int ret;
@@ -778,7 +777,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
goto clk_disable;
}
- ret = dwc3_qcom_setup_irq(pdev);
+ ret = dwc3_qcom_setup_irq(qcom, pdev);
if (ret) {
dev_err(dev, "failed to setup IRQs, err=%d\n", ret);
goto clk_disable;
@@ -793,7 +792,7 @@ 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(pdev);
+ ret = dwc3_qcom_of_register_core(qcom, pdev);
if (ret) {
dev_err(dev, "failed to register DWC3 Core, err=%d\n", ret);
goto clk_disable;
--
2.45.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
` (3 preceding siblings ...)
2025-02-27 0:17 ` [PATCH v4 4/7] usb: dwc3: qcom: Don't rely on drvdata during probe Bjorn Andersson
@ 2025-02-27 0:17 ` Bjorn Andersson
2025-03-04 0:05 ` Thinh Nguyen
2025-02-27 0:17 ` [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 7/7] arm64: dts: qcom: sc8280x: Flatten the USB nodes Bjorn Andersson
6 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
In order to more tightly integrate the Qualcomm glue driver with the
dwc3 core the driver is redesigned to avoid splitting the implementation
using the driver model. But due to the strong coupling to the Devicetree
binding needs to be updated as well.
Various ways to provide backwards compatibility with existing Devicetree
blobs has been explored, but migrating the Devicetree information
between the old and the new binding is non-trivial.
For the vast majority of boards out there, the kernel and Devicetree are
generated and handled together, which in practice means that backwards
compatibility needs to be managed across about 1 kernel release.
For some though, such as the various Snapdragon laptops, the Devicetree
blobs live a life separate of the kernel. In each one of these, with the
continued extension of new features, it's recommended that users would
upgrade their Devicetree somewhat frequently.
With this in mind, simply carrying a snapshot/copy of the current driver
is simpler than creating and maintaining the migration code.
The driver is kept under the same Kconfig option, to ensure that Linux
distributions doesn't drop USB support on these platforms.
The driver, which is going to be refactored to handle the newly
introduced qcom,snps-dwc3 compatible, is updated to temporarily not
match against any compatible.
This driver should be removed after 2 LTS releases.
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
drivers/usb/dwc3/Makefile | 1 +
drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
drivers/usb/dwc3/dwc3-qcom.c | 1 -
3 files changed, 935 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index 124eda2522d9..830e6c9e5fe0 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_USB_DWC3_MESON_G12A) += dwc3-meson-g12a.o
obj-$(CONFIG_USB_DWC3_OF_SIMPLE) += dwc3-of-simple.o
obj-$(CONFIG_USB_DWC3_ST) += dwc3-st.o
obj-$(CONFIG_USB_DWC3_QCOM) += dwc3-qcom.o
+obj-$(CONFIG_USB_DWC3_QCOM) += dwc3-qcom-legacy.o
obj-$(CONFIG_USB_DWC3_IMX8MP) += dwc3-imx8mp.o
obj-$(CONFIG_USB_DWC3_XILINX) += dwc3-xilinx.o
obj-$(CONFIG_USB_DWC3_OCTEON) += dwc3-octeon.o
diff --git a/drivers/usb/dwc3/dwc3-qcom-legacy.c b/drivers/usb/dwc3/dwc3-qcom-legacy.c
new file mode 100644
index 000000000000..3f7a4c4d8eb7
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-qcom-legacy.c
@@ -0,0 +1,934 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ * Inspired by dwc3-of-simple.c
+ */
+
+#include <linux/cleanup.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/clk.h>
+#include <linux/irq.h>
+#include <linux/of_clk.h>
+#include <linux/module.h>
+#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>
+#include <linux/reset.h>
+#include <linux/iopoll.h>
+#include <linux/usb/hcd.h>
+#include <linux/usb.h>
+#include "core.h"
+
+/* USB QSCRATCH Hardware registers */
+#define QSCRATCH_HS_PHY_CTRL 0x10
+#define UTMI_OTG_VBUS_VALID BIT(20)
+#define SW_SESSVLD_SEL BIT(28)
+
+#define QSCRATCH_SS_PHY_CTRL 0x30
+#define LANE0_PWR_PRESENT BIT(24)
+
+#define QSCRATCH_GENERAL_CFG 0x08
+#define PIPE_UTMI_CLK_SEL BIT(0)
+#define PIPE3_PHYSTATUS_SW BIT(3)
+#define PIPE_UTMI_CLK_DIS BIT(8)
+
+#define PWR_EVNT_LPM_IN_L2_MASK BIT(4)
+#define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)
+
+#define SDM845_QSCRATCH_BASE_OFFSET 0xf8800
+#define SDM845_QSCRATCH_SIZE 0x400
+#define SDM845_DWC3_CORE_SIZE 0xcd00
+
+/* Interconnect path bandwidths in MBps */
+#define USB_MEMORY_AVG_HS_BW MBps_to_icc(240)
+#define USB_MEMORY_PEAK_HS_BW MBps_to_icc(700)
+#define USB_MEMORY_AVG_SS_BW MBps_to_icc(1000)
+#define USB_MEMORY_PEAK_SS_BW MBps_to_icc(2500)
+#define APPS_USB_AVG_BW 0
+#define APPS_USB_PEAK_BW MBps_to_icc(40)
+
+/* Qualcomm SoCs with multiport support has up to 4 ports */
+#define DWC3_QCOM_MAX_PORTS 4
+
+static const u32 pwr_evnt_irq_stat_reg[DWC3_QCOM_MAX_PORTS] = {
+ 0x58,
+ 0x1dc,
+ 0x228,
+ 0x238,
+};
+
+struct dwc3_qcom_port {
+ int qusb2_phy_irq;
+ int dp_hs_phy_irq;
+ int dm_hs_phy_irq;
+ int ss_phy_irq;
+ enum usb_device_speed usb2_speed;
+};
+
+struct dwc3_qcom {
+ struct device *dev;
+ void __iomem *qscratch_base;
+ struct platform_device *dwc3;
+ struct clk **clks;
+ int num_clocks;
+ struct reset_control *resets;
+ struct dwc3_qcom_port ports[DWC3_QCOM_MAX_PORTS];
+ u8 num_ports;
+
+ struct extcon_dev *edev;
+ struct extcon_dev *host_edev;
+ struct notifier_block vbus_nb;
+ struct notifier_block host_nb;
+
+ enum usb_dr_mode mode;
+ bool is_suspended;
+ bool pm_suspended;
+ struct icc_path *icc_path_ddr;
+ struct icc_path *icc_path_apps;
+};
+
+static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
+{
+ u32 reg;
+
+ reg = readl(base + offset);
+ reg |= val;
+ writel(reg, base + offset);
+
+ /* ensure that above write is through */
+ readl(base + offset);
+}
+
+static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val)
+{
+ u32 reg;
+
+ reg = readl(base + offset);
+ reg &= ~val;
+ writel(reg, base + offset);
+
+ /* ensure that above write is through */
+ readl(base + offset);
+}
+
+static void dwc3_qcom_vbus_override_enable(struct dwc3_qcom *qcom, bool enable)
+{
+ if (enable) {
+ dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_SS_PHY_CTRL,
+ LANE0_PWR_PRESENT);
+ dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_HS_PHY_CTRL,
+ UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL);
+ } else {
+ dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_SS_PHY_CTRL,
+ LANE0_PWR_PRESENT);
+ dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_HS_PHY_CTRL,
+ UTMI_OTG_VBUS_VALID | SW_SESSVLD_SEL);
+ }
+}
+
+static int dwc3_qcom_vbus_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, vbus_nb);
+
+ /* enable vbus override for device mode */
+ dwc3_qcom_vbus_override_enable(qcom, event);
+ qcom->mode = event ? USB_DR_MODE_PERIPHERAL : USB_DR_MODE_HOST;
+
+ return NOTIFY_DONE;
+}
+
+static int dwc3_qcom_host_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, host_nb);
+
+ /* disable vbus override in host mode */
+ dwc3_qcom_vbus_override_enable(qcom, !event);
+ qcom->mode = event ? USB_DR_MODE_HOST : USB_DR_MODE_PERIPHERAL;
+
+ return NOTIFY_DONE;
+}
+
+static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
+{
+ struct device *dev = qcom->dev;
+ struct extcon_dev *host_edev;
+ int ret;
+
+ if (!of_property_present(dev->of_node, "extcon"))
+ return 0;
+
+ qcom->edev = extcon_get_edev_by_phandle(dev, 0);
+ if (IS_ERR(qcom->edev))
+ return dev_err_probe(dev, PTR_ERR(qcom->edev),
+ "Failed to get extcon\n");
+
+ qcom->vbus_nb.notifier_call = dwc3_qcom_vbus_notifier;
+
+ qcom->host_edev = extcon_get_edev_by_phandle(dev, 1);
+ if (IS_ERR(qcom->host_edev))
+ qcom->host_edev = NULL;
+
+ ret = devm_extcon_register_notifier(dev, qcom->edev, EXTCON_USB,
+ &qcom->vbus_nb);
+ if (ret < 0) {
+ dev_err(dev, "VBUS notifier register failed\n");
+ return ret;
+ }
+
+ if (qcom->host_edev)
+ host_edev = qcom->host_edev;
+ else
+ host_edev = qcom->edev;
+
+ qcom->host_nb.notifier_call = dwc3_qcom_host_notifier;
+ ret = devm_extcon_register_notifier(dev, host_edev, EXTCON_USB_HOST,
+ &qcom->host_nb);
+ if (ret < 0) {
+ dev_err(dev, "Host notifier register failed\n");
+ return ret;
+ }
+
+ /* Update initial VBUS override based on extcon state */
+ if (extcon_get_state(qcom->edev, EXTCON_USB) ||
+ !extcon_get_state(host_edev, EXTCON_USB_HOST))
+ dwc3_qcom_vbus_notifier(&qcom->vbus_nb, true, qcom->edev);
+ else
+ dwc3_qcom_vbus_notifier(&qcom->vbus_nb, false, qcom->edev);
+
+ return 0;
+}
+
+static int dwc3_qcom_interconnect_enable(struct dwc3_qcom *qcom)
+{
+ int ret;
+
+ ret = icc_enable(qcom->icc_path_ddr);
+ if (ret)
+ return ret;
+
+ ret = icc_enable(qcom->icc_path_apps);
+ if (ret)
+ icc_disable(qcom->icc_path_ddr);
+
+ return ret;
+}
+
+static int dwc3_qcom_interconnect_disable(struct dwc3_qcom *qcom)
+{
+ int ret;
+
+ ret = icc_disable(qcom->icc_path_ddr);
+ if (ret)
+ return ret;
+
+ ret = icc_disable(qcom->icc_path_apps);
+ if (ret)
+ icc_enable(qcom->icc_path_ddr);
+
+ return ret;
+}
+
+/**
+ * dwc3_qcom_interconnect_init() - Get interconnect path handles
+ * and set bandwidth.
+ * @qcom: Pointer to the concerned usb core.
+ *
+ */
+static int dwc3_qcom_interconnect_init(struct dwc3_qcom *qcom)
+{
+ enum usb_device_speed max_speed;
+ struct device *dev = qcom->dev;
+ int ret;
+
+ qcom->icc_path_ddr = of_icc_get(dev, "usb-ddr");
+ if (IS_ERR(qcom->icc_path_ddr)) {
+ return dev_err_probe(dev, PTR_ERR(qcom->icc_path_ddr),
+ "failed to get usb-ddr path\n");
+ }
+
+ qcom->icc_path_apps = of_icc_get(dev, "apps-usb");
+ if (IS_ERR(qcom->icc_path_apps)) {
+ ret = dev_err_probe(dev, PTR_ERR(qcom->icc_path_apps),
+ "failed to get apps-usb path\n");
+ goto put_path_ddr;
+ }
+
+ max_speed = usb_get_maximum_speed(&qcom->dwc3->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);
+ } else {
+ ret = icc_set_bw(qcom->icc_path_ddr,
+ USB_MEMORY_AVG_HS_BW, USB_MEMORY_PEAK_HS_BW);
+ }
+ if (ret) {
+ dev_err(dev, "failed to set bandwidth for usb-ddr path: %d\n", ret);
+ goto put_path_apps;
+ }
+
+ ret = icc_set_bw(qcom->icc_path_apps, APPS_USB_AVG_BW, APPS_USB_PEAK_BW);
+ if (ret) {
+ dev_err(dev, "failed to set bandwidth for apps-usb path: %d\n", ret);
+ goto put_path_apps;
+ }
+
+ return 0;
+
+put_path_apps:
+ icc_put(qcom->icc_path_apps);
+put_path_ddr:
+ icc_put(qcom->icc_path_ddr);
+ return ret;
+}
+
+/**
+ * dwc3_qcom_interconnect_exit() - Release interconnect path handles
+ * @qcom: Pointer to the concerned usb core.
+ *
+ * This function is used to release interconnect path handle.
+ */
+static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
+{
+ icc_put(qcom->icc_path_ddr);
+ icc_put(qcom->icc_path_apps);
+}
+
+/* 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;
+}
+
+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;
+
+ /*
+ * FIXME: Fix this layering violation.
+ */
+ hcd = platform_get_drvdata(dwc->xhci);
+
+#ifdef CONFIG_USB
+ udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
+#else
+ udev = NULL;
+#endif
+ if (!udev)
+ return USB_SPEED_UNKNOWN;
+
+ return udev->speed;
+}
+
+static void dwc3_qcom_enable_wakeup_irq(int irq, unsigned int polarity)
+{
+ if (!irq)
+ return;
+
+ if (polarity)
+ irq_set_irq_type(irq, polarity);
+
+ enable_irq(irq);
+ enable_irq_wake(irq);
+}
+
+static void dwc3_qcom_disable_wakeup_irq(int irq)
+{
+ if (!irq)
+ return;
+
+ disable_irq_wake(irq);
+ disable_irq_nosync(irq);
+}
+
+static void dwc3_qcom_disable_port_interrupts(struct dwc3_qcom_port *port)
+{
+ dwc3_qcom_disable_wakeup_irq(port->qusb2_phy_irq);
+
+ if (port->usb2_speed == USB_SPEED_LOW) {
+ dwc3_qcom_disable_wakeup_irq(port->dm_hs_phy_irq);
+ } else if ((port->usb2_speed == USB_SPEED_HIGH) ||
+ (port->usb2_speed == USB_SPEED_FULL)) {
+ dwc3_qcom_disable_wakeup_irq(port->dp_hs_phy_irq);
+ } else {
+ dwc3_qcom_disable_wakeup_irq(port->dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(port->dm_hs_phy_irq);
+ }
+
+ dwc3_qcom_disable_wakeup_irq(port->ss_phy_irq);
+}
+
+static void dwc3_qcom_enable_port_interrupts(struct dwc3_qcom_port *port)
+{
+ dwc3_qcom_enable_wakeup_irq(port->qusb2_phy_irq, 0);
+
+ /*
+ * Configure DP/DM line interrupts based on the USB2 device attached to
+ * the root hub port. When HS/FS device is connected, configure the DP line
+ * as falling edge to detect both disconnect and remote wakeup scenarios. When
+ * LS device is connected, configure DM line as falling edge to detect both
+ * disconnect and remote wakeup. When no device is connected, configure both
+ * DP and DM lines as rising edge to detect HS/HS/LS device connect scenario.
+ */
+
+ if (port->usb2_speed == USB_SPEED_LOW) {
+ dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
+ IRQ_TYPE_EDGE_FALLING);
+ } else if ((port->usb2_speed == USB_SPEED_HIGH) ||
+ (port->usb2_speed == USB_SPEED_FULL)) {
+ dwc3_qcom_enable_wakeup_irq(port->dp_hs_phy_irq,
+ IRQ_TYPE_EDGE_FALLING);
+ } else {
+ dwc3_qcom_enable_wakeup_irq(port->dp_hs_phy_irq,
+ IRQ_TYPE_EDGE_RISING);
+ dwc3_qcom_enable_wakeup_irq(port->dm_hs_phy_irq,
+ IRQ_TYPE_EDGE_RISING);
+ }
+
+ dwc3_qcom_enable_wakeup_irq(port->ss_phy_irq, 0);
+}
+
+static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
+{
+ int i;
+
+ for (i = 0; i < qcom->num_ports; i++)
+ dwc3_qcom_disable_port_interrupts(&qcom->ports[i]);
+}
+
+static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
+{
+ int i;
+
+ for (i = 0; i < qcom->num_ports; i++)
+ dwc3_qcom_enable_port_interrupts(&qcom->ports[i]);
+}
+
+static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
+{
+ u32 val;
+ int i, ret;
+
+ if (qcom->is_suspended)
+ return 0;
+
+ for (i = 0; i < qcom->num_ports; i++) {
+ val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg[i]);
+ if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
+ dev_err(qcom->dev, "port-%d HS-PHY not in L2\n", i + 1);
+ }
+
+ for (i = qcom->num_clocks - 1; i >= 0; i--)
+ clk_disable_unprepare(qcom->clks[i]);
+
+ ret = dwc3_qcom_interconnect_disable(qcom);
+ if (ret)
+ dev_warn(qcom->dev, "failed to disable interconnect: %d\n", ret);
+
+ /*
+ * The role is stable during suspend as role switching is done from a
+ * freezable workqueue.
+ */
+ if (dwc3_qcom_is_host(qcom) && wakeup) {
+ for (i = 0; i < qcom->num_ports; i++)
+ qcom->ports[i].usb2_speed = dwc3_qcom_read_usb2_speed(qcom, i);
+ dwc3_qcom_enable_interrupts(qcom);
+ }
+
+ qcom->is_suspended = true;
+
+ return 0;
+}
+
+static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
+{
+ int ret;
+ int i;
+
+ if (!qcom->is_suspended)
+ return 0;
+
+ if (dwc3_qcom_is_host(qcom) && wakeup)
+ dwc3_qcom_disable_interrupts(qcom);
+
+ for (i = 0; i < qcom->num_clocks; i++) {
+ ret = clk_prepare_enable(qcom->clks[i]);
+ if (ret < 0) {
+ while (--i >= 0)
+ clk_disable_unprepare(qcom->clks[i]);
+ return ret;
+ }
+ }
+
+ ret = dwc3_qcom_interconnect_enable(qcom);
+ if (ret)
+ dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
+
+ /* Clear existing events from PHY related to L2 in/out */
+ for (i = 0; i < qcom->num_ports; i++) {
+ dwc3_qcom_setbits(qcom->qscratch_base,
+ pwr_evnt_irq_stat_reg[i],
+ PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
+ }
+
+ qcom->is_suspended = false;
+
+ return 0;
+}
+
+static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
+{
+ struct dwc3_qcom *qcom = data;
+ struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
+
+ /* If pm_suspended then let pm_resume take care of resuming h/w */
+ if (qcom->pm_suspended)
+ return IRQ_HANDLED;
+
+ /*
+ * This is safe as role switching is done from a freezable workqueue
+ * and the wakeup interrupts are disabled as part of resume.
+ */
+ if (dwc3_qcom_is_host(qcom))
+ pm_runtime_resume(&dwc->xhci->dev);
+
+ return IRQ_HANDLED;
+}
+
+static void dwc3_qcom_select_utmi_clk(struct dwc3_qcom *qcom)
+{
+ /* Configure dwc3 to use UTMI clock as PIPE clock not present */
+ dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+ PIPE_UTMI_CLK_DIS);
+
+ usleep_range(100, 1000);
+
+ dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+ PIPE_UTMI_CLK_SEL | PIPE3_PHYSTATUS_SW);
+
+ usleep_range(100, 1000);
+
+ dwc3_qcom_clrbits(qcom->qscratch_base, QSCRATCH_GENERAL_CFG,
+ PIPE_UTMI_CLK_DIS);
+}
+
+static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
+ const char *name)
+{
+ int ret;
+
+ /* Keep wakeup interrupts disabled until suspend */
+ ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
+ qcom_dwc3_resume_irq,
+ IRQF_ONESHOT | IRQF_NO_AUTOEN,
+ name, qcom);
+ if (ret)
+ dev_err(qcom->dev, "failed to request irq %s: %d\n", name, ret);
+
+ return ret;
+}
+
+static int dwc3_qcom_setup_port_irq(struct dwc3_qcom *qcom,
+ struct platform_device *pdev,
+ int port_index, bool is_multiport)
+{
+ const char *irq_name;
+ int irq;
+ int ret;
+
+ if (is_multiport)
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_index + 1);
+ else
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_irq");
+ if (!irq_name)
+ return -ENOMEM;
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
+ if (irq > 0) {
+ ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
+ if (ret)
+ return ret;
+ qcom->ports[port_index].dp_hs_phy_irq = irq;
+ }
+
+ if (is_multiport)
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_%d", port_index + 1);
+ else
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_irq");
+ if (!irq_name)
+ return -ENOMEM;
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
+ if (irq > 0) {
+ ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
+ if (ret)
+ return ret;
+ qcom->ports[port_index].dm_hs_phy_irq = irq;
+ }
+
+ if (is_multiport)
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_%d", port_index + 1);
+ else
+ irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_irq");
+ if (!irq_name)
+ return -ENOMEM;
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
+ if (irq > 0) {
+ ret = dwc3_qcom_request_irq(qcom, irq, irq_name);
+ if (ret)
+ return ret;
+ qcom->ports[port_index].ss_phy_irq = irq;
+ }
+
+ if (is_multiport)
+ return 0;
+
+ irq = platform_get_irq_byname_optional(pdev, "qusb2_phy");
+ if (irq > 0) {
+ ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy");
+ if (ret)
+ return ret;
+ qcom->ports[port_index].qusb2_phy_irq = irq;
+ }
+
+ return 0;
+}
+
+static int dwc3_qcom_find_num_ports(struct platform_device *pdev)
+{
+ char irq_name[14];
+ int port_num;
+ int irq;
+
+ irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_1");
+ if (irq <= 0)
+ return 1;
+
+ for (port_num = 2; port_num <= DWC3_QCOM_MAX_PORTS; port_num++) {
+ sprintf(irq_name, "dp_hs_phy_%d", port_num);
+
+ irq = platform_get_irq_byname_optional(pdev, irq_name);
+ if (irq <= 0)
+ return port_num - 1;
+ }
+
+ return DWC3_QCOM_MAX_PORTS;
+}
+
+static int dwc3_qcom_setup_irq(struct dwc3_qcom *qcom, struct platform_device *pdev)
+{
+ bool is_multiport;
+ int ret;
+ int i;
+
+ qcom->num_ports = dwc3_qcom_find_num_ports(pdev);
+ is_multiport = (qcom->num_ports > 1);
+
+ for (i = 0; i < qcom->num_ports; i++) {
+ ret = dwc3_qcom_setup_port_irq(qcom, pdev, i, is_multiport);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
+{
+ struct device *dev = qcom->dev;
+ struct device_node *np = dev->of_node;
+ int i;
+
+ if (!np || !count)
+ return 0;
+
+ if (count < 0)
+ return count;
+
+ qcom->num_clocks = count;
+
+ qcom->clks = devm_kcalloc(dev, qcom->num_clocks,
+ sizeof(struct clk *), GFP_KERNEL);
+ if (!qcom->clks)
+ return -ENOMEM;
+
+ for (i = 0; i < qcom->num_clocks; i++) {
+ struct clk *clk;
+ int ret;
+
+ clk = of_clk_get(np, i);
+ if (IS_ERR(clk)) {
+ while (--i >= 0)
+ clk_put(qcom->clks[i]);
+ return PTR_ERR(clk);
+ }
+
+ ret = clk_prepare_enable(clk);
+ if (ret < 0) {
+ while (--i >= 0) {
+ clk_disable_unprepare(qcom->clks[i]);
+ clk_put(qcom->clks[i]);
+ }
+ clk_put(clk);
+
+ return ret;
+ }
+
+ qcom->clks[i] = clk;
+ }
+
+ 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 device_node *np = pdev->dev.of_node;
+ struct device *dev = &pdev->dev;
+ struct dwc3_qcom *qcom;
+ int ret, i;
+ bool ignore_pipe_clk;
+ bool wakeup_source;
+
+ qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
+ if (!qcom)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, qcom);
+ qcom->dev = &pdev->dev;
+
+ qcom->resets = devm_reset_control_array_get_optional_exclusive(dev);
+ if (IS_ERR(qcom->resets)) {
+ return dev_err_probe(&pdev->dev, PTR_ERR(qcom->resets),
+ "failed to get resets\n");
+ }
+
+ ret = reset_control_assert(qcom->resets);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to assert resets, err=%d\n", ret);
+ return ret;
+ }
+
+ usleep_range(10, 1000);
+
+ ret = reset_control_deassert(qcom->resets);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to deassert resets, err=%d\n", ret);
+ goto reset_assert;
+ }
+
+ ret = dwc3_qcom_clk_init(qcom, of_clk_get_parent_count(np));
+ if (ret) {
+ dev_err_probe(dev, ret, "failed to get clocks\n");
+ goto reset_assert;
+ }
+
+ qcom->qscratch_base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(qcom->qscratch_base)) {
+ ret = PTR_ERR(qcom->qscratch_base);
+ goto clk_disable;
+ }
+
+ ret = dwc3_qcom_setup_irq(qcom, pdev);
+ if (ret) {
+ dev_err(dev, "failed to setup IRQs, err=%d\n", ret);
+ goto clk_disable;
+ }
+
+ /*
+ * Disable pipe_clk requirement if specified. Used when dwc3
+ * operates without SSPHY and only HS/FS/LS modes are supported.
+ */
+ ignore_pipe_clk = device_property_read_bool(dev,
+ "qcom,select-utmi-as-pipe-clk");
+ 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);
+ goto clk_disable;
+ }
+
+ ret = dwc3_qcom_interconnect_init(qcom);
+ if (ret)
+ goto depopulate;
+
+ qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
+
+ /* enable vbus override for device mode */
+ if (qcom->mode != USB_DR_MODE_HOST)
+ dwc3_qcom_vbus_override_enable(qcom, true);
+
+ /* register extcon to override sw_vbus on Vbus change later */
+ ret = dwc3_qcom_register_extcon(qcom);
+ if (ret)
+ goto interconnect_exit;
+
+ 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);
+clk_disable:
+ for (i = qcom->num_clocks - 1; i >= 0; i--) {
+ clk_disable_unprepare(qcom->clks[i]);
+ clk_put(qcom->clks[i]);
+ }
+reset_assert:
+ reset_control_assert(qcom->resets);
+
+ return ret;
+}
+
+static void dwc3_qcom_remove(struct platform_device *pdev)
+{
+ struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
+ struct device *dev = &pdev->dev;
+ int i;
+
+ of_platform_depopulate(&pdev->dev);
+ platform_device_put(qcom->dwc3);
+
+ for (i = qcom->num_clocks - 1; i >= 0; i--) {
+ clk_disable_unprepare(qcom->clks[i]);
+ clk_put(qcom->clks[i]);
+ }
+ qcom->num_clocks = 0;
+
+ dwc3_qcom_interconnect_exit(qcom);
+ reset_control_assert(qcom->resets);
+
+ pm_runtime_allow(dev);
+ pm_runtime_disable(dev);
+}
+
+static int __maybe_unused dwc3_qcom_pm_suspend(struct device *dev)
+{
+ struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+ bool wakeup = device_may_wakeup(dev);
+ int ret;
+
+ ret = dwc3_qcom_suspend(qcom, wakeup);
+ if (ret)
+ return ret;
+
+ qcom->pm_suspended = true;
+
+ return 0;
+}
+
+static int __maybe_unused dwc3_qcom_pm_resume(struct device *dev)
+{
+ struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+ bool wakeup = device_may_wakeup(dev);
+ int ret;
+
+ ret = dwc3_qcom_resume(qcom, wakeup);
+ if (ret)
+ return ret;
+
+ qcom->pm_suspended = false;
+
+ return 0;
+}
+
+static int __maybe_unused dwc3_qcom_runtime_suspend(struct device *dev)
+{
+ struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+
+ return dwc3_qcom_suspend(qcom, true);
+}
+
+static int __maybe_unused dwc3_qcom_runtime_resume(struct device *dev)
+{
+ struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+
+ return dwc3_qcom_resume(qcom, true);
+}
+
+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)
+};
+
+static const struct of_device_id dwc3_qcom_of_match[] = {
+ { .compatible = "qcom,dwc3" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);
+
+static struct platform_driver dwc3_qcom_driver = {
+ .probe = dwc3_qcom_probe,
+ .remove = dwc3_qcom_remove,
+ .driver = {
+ .name = "dwc3-qcom",
+ .pm = &dwc3_qcom_dev_pm_ops,
+ .of_match_table = dwc3_qcom_of_match,
+ },
+};
+
+module_platform_driver(dwc3_qcom_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("DesignWare DWC3 QCOM legacy glue Driver");
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 50b1da845113..9d04c2457433 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -913,7 +913,6 @@ static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
};
static const struct of_device_id dwc3_qcom_of_match[] = {
- { .compatible = "qcom,dwc3" },
{ }
};
MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match);
--
2.45.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
` (4 preceding siblings ...)
2025-02-27 0:17 ` [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty Bjorn Andersson
@ 2025-02-27 0:17 ` Bjorn Andersson
2025-03-07 6:41 ` Dmitry Baryshkov
2025-02-27 0:17 ` [PATCH v4 7/7] arm64: dts: qcom: sc8280x: Flatten the USB nodes Bjorn Andersson
6 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
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@oss.qualcomm.com>
---
drivers/usb/dwc3/dwc3-qcom.c | 138 +++++++++++++++++++++----------------------
1 file changed, 69 insertions(+), 69 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 9d04c2457433..63e60f15ceaa 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;
@@ -260,7 +261,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 +304,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 +488,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 +690,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 +706,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 +734,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 +762,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 +789,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 +811,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 +833,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 +853,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 +864,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.45.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 7/7] arm64: dts: qcom: sc8280x: Flatten the USB nodes
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
` (5 preceding siblings ...)
2025-02-27 0:17 ` [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model Bjorn Andersson
@ 2025-02-27 0:17 ` Bjorn Andersson
2025-03-08 17:58 ` Konrad Dybcio
6 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 0:17 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel,
Bjorn Andersson
Transition the three USB controllers found in sc8280xp to the newly
introduced, flattened representation of the Qualcomm USB block, i.e.
qcom,snps-dwc3, to show the end result.
The reg and interrupts properties from the usb child node are merged
with their counterpart in the outer node, remaining properties and child
nodes are simply moved.
Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
---
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 ++++++++++-----------
8 files changed, 95 insertions(+), 140 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
index 2fd1dafe63ce..3d84cbf5af31 100644
--- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
@@ -631,12 +631,10 @@ &ufs_card_phy {
};
&usb_0 {
- status = "okay";
-};
-
-&usb_0_dwc3 {
/* TODO: Define USB-C connector properly */
dr_mode = "peripheral";
+
+ status = "okay";
};
&usb_0_hsphy {
@@ -655,12 +653,10 @@ &usb_0_qmpphy {
};
&usb_1 {
- status = "okay";
-};
-
-&usb_1_dwc3 {
/* TODO: Define USB-C connector properly */
dr_mode = "host";
+
+ status = "okay";
};
&usb_1_hsphy {
diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
index 177b9dad6ff7..7be803fb7cbe 100644
--- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
@@ -466,11 +466,8 @@ &ufs_mem_phy {
};
&usb_0 {
- status = "okay";
-};
-
-&usb_0_dwc3 {
dr_mode = "peripheral";
+ status = "okay";
};
&usb_0_hsphy {
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index 75adaa19d1c3..05fe5793f1f1 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -795,11 +795,9 @@ &ufs_mem_phy {
};
&usb_0 {
- status = "okay";
-};
-
-&usb_0_dwc3 {
dr_mode = "host";
+
+ status = "okay";
};
&usb_0_dwc3_hs {
@@ -832,11 +830,9 @@ &usb_0_qmpphy_out {
};
&usb_1 {
- status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "host";
+
+ status = "okay";
};
&usb_1_dwc3_hs {
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-huawei-gaokun3.dts b/arch/arm64/boot/dts/qcom/sc8280xp-huawei-gaokun3.dts
index 09b95f89ee58..300c7dc999a4 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-huawei-gaokun3.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-huawei-gaokun3.dts
@@ -997,11 +997,8 @@ bluetooth {
};
&usb_0 {
- status = "okay";
-};
-
-&usb_0_dwc3 {
dr_mode = "host";
+ status = "okay";
};
&usb_0_hsphy {
@@ -1026,11 +1023,8 @@ &usb_0_qmpphy_dp_in {
};
&usb_1 {
- status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "host";
+ status = "okay";
};
&usb_1_hsphy {
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index f3190f408f4b..1b9501cc82f4 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -1343,11 +1343,9 @@ bluetooth {
};
&usb_0 {
- status = "okay";
-};
-
-&usb_0_dwc3 {
dr_mode = "host";
+
+ status = "okay";
};
&usb_0_dwc3_hs {
@@ -1380,11 +1378,8 @@ &usb_0_qmpphy_out {
};
&usb_1 {
- status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "host";
+ status = "okay";
};
&usb_1_dwc3_hs {
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
index ae5daeac8fe2..82672f441ea2 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-arcata.dts
@@ -749,11 +749,8 @@ embedded-controller {
};
&usb_0 {
- status = "okay";
-};
-
-&usb_0_dwc3 {
dr_mode = "host";
+ status = "okay";
};
&usb_0_dwc3_hs {
@@ -786,11 +783,8 @@ &usb_0_qmpphy_out {
};
&usb_1 {
- status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "host";
+ status = "okay";
};
&usb_1_dwc3_hs {
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-blackrock.dts b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-blackrock.dts
index fa9d94105052..eeb69cfd4422 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-blackrock.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-microsoft-blackrock.dts
@@ -982,11 +982,9 @@ bluetooth {
};
&usb_0 {
- status = "okay";
-};
-
-&usb_0_dwc3 {
dr_mode = "host";
+
+ status = "okay";
};
&usb_0_dwc3_hs {
@@ -1019,11 +1017,9 @@ &usb_0_qmpphy_out {
};
&usb_1 {
- status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "host";
+
+ status = "okay";
};
&usb_1_dwc3_hs {
@@ -1059,12 +1055,10 @@ &usb_2 {
pinctrl-0 = <&usb2_en_state>;
pinctrl-names = "default";
- status = "okay";
-};
-
-&usb_2_dwc3 {
phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
phy-names = "usb2-0", "usb3-0";
+
+ status = "okay";
};
&usb_2_hsphy0 {
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 01501acb1790..3dea86b0e13d 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3457,12 +3457,9 @@ system-cache-controller@9200000 {
interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
};
- usb_2: usb@a4f8800 {
- compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
- reg = <0 0x0a4f8800 0 0x400>;
- #address-cells = <2>;
- #size-cells = <2>;
- ranges;
+ usb_2: usb@a400000 {
+ compatible = "qcom,sc8280xp-dwc3-mp", "qcom,snps-dwc3";
+ reg = <0 0x0a400000 0 0x10000>;
clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>,
<&gcc GCC_USB30_MP_MASTER_CLK>,
@@ -3480,7 +3477,8 @@ usb_2: usb@a4f8800 {
<&gcc GCC_USB30_MP_MASTER_CLK>;
assigned-clock-rates = <19200000>, <200000000>;
- interrupts-extended = <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+ interrupts-extended = <&intc GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
<&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
<&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
<&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>,
@@ -3499,7 +3497,8 @@ usb_2: usb@a4f8800 {
<&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
<&pdc 17 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "pwr_event_1", "pwr_event_2",
+ interrupt-names = "dwc_usb3",
+ "pwr_event_1", "pwr_event_2",
"pwr_event_3", "pwr_event_4",
"hs_phy_1", "hs_phy_2",
"hs_phy_3", "hs_phy_4",
@@ -3509,6 +3508,7 @@ usb_2: usb@a4f8800 {
"dp_hs_phy_4", "dm_hs_phy_4",
"ss_phy_1", "ss_phy_2";
+ iommus = <&apps_smmu 0x800 0x0>;
power-domains = <&gcc USB30_MP_GDSC>;
required-opps = <&rpmhpd_opp_nom>;
@@ -3518,35 +3518,28 @@ usb_2: usb@a4f8800 {
<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_MP 0>;
interconnect-names = "usb-ddr", "apps-usb";
+ phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
+ <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
+ <&usb_2_hsphy2>,
+ <&usb_2_hsphy3>;
+ phy-names = "usb2-0", "usb3-0",
+ "usb2-1", "usb3-1",
+ "usb2-2",
+ "usb2-3";
+
wakeup-source;
+ dr_mode = "host";
+
+ snps,dis-u1-entry-quirk;
+ snps,dis-u2-entry-quirk;
+
status = "disabled";
+ };
- usb_2_dwc3: usb@a400000 {
- compatible = "snps,dwc3";
- reg = <0 0x0a400000 0 0xcd00>;
- interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
- iommus = <&apps_smmu 0x800 0x0>;
- phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
- <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
- <&usb_2_hsphy2>,
- <&usb_2_hsphy3>;
- phy-names = "usb2-0", "usb3-0",
- "usb2-1", "usb3-1",
- "usb2-2",
- "usb2-3";
- dr_mode = "host";
- snps,dis-u1-entry-quirk;
- snps,dis-u2-entry-quirk;
- };
- };
-
- usb_0: usb@a6f8800 {
- compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
- reg = <0 0x0a6f8800 0 0x400>;
- #address-cells = <2>;
- #size-cells = <2>;
- ranges;
+ usb_0: usb@a600000 {
+ compatible = "qcom,sc8280xp-dwc3", "qcom,snps-dwc3";
+ reg = <0 0x0a600000 0 0x20000>;
clocks = <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
<&gcc GCC_USB30_PRIM_MASTER_CLK>,
@@ -3564,17 +3557,20 @@ usb_0: usb@a6f8800 {
<&gcc GCC_USB30_PRIM_MASTER_CLK>;
assigned-clock-rates = <19200000>, <200000000>;
- interrupts-extended = <&intc GIC_SPI 804 IRQ_TYPE_LEVEL_HIGH>,
+ interrupts-extended = <&intc GIC_SPI 803 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 804 IRQ_TYPE_LEVEL_HIGH>,
<&intc GIC_SPI 805 IRQ_TYPE_LEVEL_HIGH>,
<&pdc 14 IRQ_TYPE_EDGE_BOTH>,
<&pdc 15 IRQ_TYPE_EDGE_BOTH>,
<&pdc 138 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "pwr_event",
+ interrupt-names = "dwc_usb3",
+ "pwr_event",
"hs_phy_irq",
"dp_hs_phy_irq",
"dm_hs_phy_irq",
"ss_phy_irq";
+ iommus = <&apps_smmu 0x820 0x0>;
power-domains = <&gcc USB30_PRIM_GDSC>;
required-opps = <&rpmhpd_opp_nom>;
@@ -3584,45 +3580,40 @@ usb_0: usb@a6f8800 {
<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_0 0>;
interconnect-names = "usb-ddr", "apps-usb";
+ phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
+ phy-names = "usb2-phy", "usb3-phy";
+
wakeup-source;
- status = "disabled";
+ snps,dis-u1-entry-quirk;
+ snps,dis-u2-entry-quirk;
- usb_0_dwc3: usb@a600000 {
- compatible = "snps,dwc3";
- reg = <0 0x0a600000 0 0xcd00>;
- interrupts = <GIC_SPI 803 IRQ_TYPE_LEVEL_HIGH>;
- iommus = <&apps_smmu 0x820 0x0>;
- phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
- phy-names = "usb2-phy", "usb3-phy";
- snps,dis-u1-entry-quirk;
- snps,dis-u2-entry-quirk;
+ status = "disabled";
- ports {
- #address-cells = <1>;
- #size-cells = <0>;
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
- port@0 {
- reg = <0>;
+ port@0 {
+ reg = <0>;
- usb_0_dwc3_hs: endpoint {
- };
+ usb_0_dwc3_hs: endpoint {
};
+ };
- port@1 {
- reg = <1>;
+ port@1 {
+ reg = <1>;
- usb_0_dwc3_ss: endpoint {
- remote-endpoint = <&usb_0_qmpphy_usb_ss_in>;
- };
+ usb_0_dwc3_ss: endpoint {
+ remote-endpoint = <&usb_0_qmpphy_usb_ss_in>;
};
};
};
};
- usb_1: usb@a8f8800 {
- compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
- reg = <0 0x0a8f8800 0 0x400>;
+ usb_1: usb@a800000 {
+ compatible = "qcom,sc8280xp-dwc3", "qcom,snps-dwc3";
+ reg = <0 0x0a800000 0 0x10000>;
#address-cells = <2>;
#size-cells = <2>;
ranges;
@@ -3643,17 +3634,20 @@ usb_1: usb@a8f8800 {
<&gcc GCC_USB30_SEC_MASTER_CLK>;
assigned-clock-rates = <19200000>, <200000000>;
- interrupts-extended = <&intc GIC_SPI 811 IRQ_TYPE_LEVEL_HIGH>,
+ interrupts-extended = <&intc GIC_SPI 810 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 811 IRQ_TYPE_LEVEL_HIGH>,
<&intc GIC_SPI 790 IRQ_TYPE_LEVEL_HIGH>,
<&pdc 12 IRQ_TYPE_EDGE_BOTH>,
<&pdc 13 IRQ_TYPE_EDGE_BOTH>,
<&pdc 136 IRQ_TYPE_LEVEL_HIGH>;
- interrupt-names = "pwr_event",
+ interrupt-names = "dwc_usb3",
+ "pwr_event",
"hs_phy_irq",
"dp_hs_phy_irq",
"dm_hs_phy_irq",
"ss_phy_irq";
+ iommus = <&apps_smmu 0x860 0x0>;
power-domains = <&gcc USB30_SEC_GDSC>;
required-opps = <&rpmhpd_opp_nom>;
@@ -3663,37 +3657,32 @@ usb_1: usb@a8f8800 {
<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_1 0>;
interconnect-names = "usb-ddr", "apps-usb";
+ phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>;
+ phy-names = "usb2-phy", "usb3-phy";
+
wakeup-source;
- status = "disabled";
+ snps,dis-u1-entry-quirk;
+ snps,dis-u2-entry-quirk;
- usb_1_dwc3: usb@a800000 {
- compatible = "snps,dwc3";
- reg = <0 0x0a800000 0 0xcd00>;
- interrupts = <GIC_SPI 810 IRQ_TYPE_LEVEL_HIGH>;
- iommus = <&apps_smmu 0x860 0x0>;
- phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>;
- phy-names = "usb2-phy", "usb3-phy";
- snps,dis-u1-entry-quirk;
- snps,dis-u2-entry-quirk;
+ status = "disabled";
- ports {
- #address-cells = <1>;
- #size-cells = <0>;
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
- port@0 {
- reg = <0>;
+ port@0 {
+ reg = <0>;
- usb_1_dwc3_hs: endpoint {
- };
+ usb_1_dwc3_hs: endpoint {
};
+ };
- port@1 {
- reg = <1>;
+ port@1 {
+ reg = <1>;
- usb_1_dwc3_ss: endpoint {
- remote-endpoint = <&usb_1_qmpphy_usb_ss_in>;
- };
+ usb_1_dwc3_ss: endpoint {
+ remote-endpoint = <&usb_1_qmpphy_usb_ss_in>;
};
};
};
--
2.45.2
^ permalink raw reply related [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks
2025-02-27 0:17 ` [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks Bjorn Andersson
@ 2025-02-27 4:24 ` Dmitry Baryshkov
2025-02-27 15:55 ` Bjorn Andersson
2025-02-27 19:22 ` Frank Li
1 sibling, 1 reply; 30+ messages in thread
From: Dmitry Baryshkov @ 2025-02-27 4:24 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li, linux-arm-msm, linux-usb, devicetree, linux-kernel
On Wed, Feb 26, 2025 at 04:17:50PM -0800, 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.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/core.c | 19 +++++++++++--------
> drivers/usb/dwc3/glue.h | 1 +
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index d9f0a6782d36..aecdde8dc999 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2328,6 +2330,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> probe_data.dwc = dwc;
> probe_data.res = res;
> + probe_data.ignore_clocks_and_resets = false;
Isn't it a default value?
>
> return dwc3_core_probe(&probe_data);
> }
> diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
> index e73cfc466012..1ddb451bdbd0 100644
> --- a/drivers/usb/dwc3/glue.h
> +++ b/drivers/usb/dwc3/glue.h
> @@ -17,6 +17,7 @@
> 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);
>
> --
> 2.45.2
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks
2025-02-27 4:24 ` Dmitry Baryshkov
@ 2025-02-27 15:55 ` Bjorn Andersson
2025-02-27 16:07 ` Dmitry Baryshkov
0 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-02-27 15:55 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Thinh Nguyen, Philipp Zabel, Konrad Dybcio,
Frank Li, linux-arm-msm, linux-usb, devicetree, linux-kernel
On Thu, Feb 27, 2025 at 06:24:17AM +0200, Dmitry Baryshkov wrote:
> On Wed, Feb 26, 2025 at 04:17:50PM -0800, 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.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> > drivers/usb/dwc3/core.c | 19 +++++++++++--------
> > drivers/usb/dwc3/glue.h | 1 +
> > 2 files changed, 12 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index d9f0a6782d36..aecdde8dc999 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -2328,6 +2330,7 @@ static int dwc3_probe(struct platform_device *pdev)
> >
> > probe_data.dwc = dwc;
> > probe_data.res = res;
> > + probe_data.ignore_clocks_and_resets = false;
>
> Isn't it a default value?
>
I like the false because it's explicit, but I have no strong attachment
to it.
That said, it's not the default value, because probe_data isn't
zero-initialized. That would however make sense to do, in order to avoid
surprises in the future when probe_data grows.
Regards,
Bjorn
> >
> > return dwc3_core_probe(&probe_data);
> > }
> > diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
> > index e73cfc466012..1ddb451bdbd0 100644
> > --- a/drivers/usb/dwc3/glue.h
> > +++ b/drivers/usb/dwc3/glue.h
> > @@ -17,6 +17,7 @@
> > 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);
> >
> > --
> > 2.45.2
> >
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks
2025-02-27 15:55 ` Bjorn Andersson
@ 2025-02-27 16:07 ` Dmitry Baryshkov
0 siblings, 0 replies; 30+ messages in thread
From: Dmitry Baryshkov @ 2025-02-27 16:07 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Thinh Nguyen, Philipp Zabel, Konrad Dybcio,
Frank Li, linux-arm-msm, linux-usb, devicetree, linux-kernel
On Thu, Feb 27, 2025 at 09:55:29AM -0600, Bjorn Andersson wrote:
> On Thu, Feb 27, 2025 at 06:24:17AM +0200, Dmitry Baryshkov wrote:
> > On Wed, Feb 26, 2025 at 04:17:50PM -0800, 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.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > ---
> > > drivers/usb/dwc3/core.c | 19 +++++++++++--------
> > > drivers/usb/dwc3/glue.h | 1 +
> > > 2 files changed, 12 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index d9f0a6782d36..aecdde8dc999 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > > @@ -2328,6 +2330,7 @@ static int dwc3_probe(struct platform_device *pdev)
> > >
> > > probe_data.dwc = dwc;
> > > probe_data.res = res;
> > > + probe_data.ignore_clocks_and_resets = false;
> >
> > Isn't it a default value?
> >
>
> I like the false because it's explicit, but I have no strong attachment
> to it.
I'm more biased to the 'make unusal cases stand out', which means the
defaults can go away to highlight non-default cases.
>
> That said, it's not the default value, because probe_data isn't
> zero-initialized. That would however make sense to do, in order to avoid
> surprises in the future when probe_data grows.
:-)
>
> Regards,
> Bjorn
>
> > >
> > > return dwc3_core_probe(&probe_data);
> > > }
> > > diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
> > > index e73cfc466012..1ddb451bdbd0 100644
> > > --- a/drivers/usb/dwc3/glue.h
> > > +++ b/drivers/usb/dwc3/glue.h
> > > @@ -17,6 +17,7 @@
> > > 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);
> > >
> > > --
> > > 2.45.2
> > >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 2/7] usb: dwc3: core: Expose core driver as library
2025-02-27 0:17 ` [PATCH v4 2/7] usb: dwc3: core: Expose core driver as library Bjorn Andersson
@ 2025-02-27 19:17 ` Frank Li
0 siblings, 0 replies; 30+ messages in thread
From: Frank Li @ 2025-02-27 19:17 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
linux-arm-msm, linux-usb, devicetree, linux-kernel
On Wed, Feb 26, 2025 at 04:17:49PM -0800, Bjorn Andersson wrote:
> The DWC3 IP block is handled by three distinct device drivers: XHCI,
> DWC3 core and a platform specific (optional) DWC3 glue driver.
>
> This has resulted in, at least in the case of the Qualcomm glue, the
> presence of a number of layering violations, where the glue code either
> can't handle, or has to work around, the fact that core might not probe
> deterministically.
>
> An example of this is that the suspend path should operate slightly
> different depending on the device operating in host or peripheral mode,
> and the only way to determine the operating state is to peek into the
> core's drvdata.
>
> The Qualcomm glue driver is expected to make updates in the qscratch
> register region (the "glue" region) during role switch events, but with
> the glue and core split using the driver model, there is no reasonable
> way to introduce listeners for mode changes.
>
> Split the dwc3 core platform_driver callbacks and their implementation
> and export the implementation, to make it possible to deterministically
> instantiate the dwc3 core as part of the dwc3 glue drivers and to
> allow flattening of the DeviceTree representation.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/core.c | 155 +++++++++++++++++++++++++++++++-----------------
> drivers/usb/dwc3/glue.h | 32 ++++++++++
> 2 files changed, 133 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index dfa1b5fe48dc..d9f0a6782d36 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -36,6 +36,7 @@
>
> #include "core.h"
> #include "gadget.h"
> +#include "glue.h"
> #include "io.h"
>
> #include "debug.h"
> @@ -2137,27 +2138,16 @@ static struct power_supply *dwc3_get_usb_power_supply(struct dwc3 *dwc)
> return usb_psy;
> }
>
> -static int dwc3_probe(struct platform_device *pdev)
> +int dwc3_core_probe(const struct dwc3_probe_data *data)
> {
> - struct device *dev = &pdev->dev;
> - struct resource *res, dwc_res;
> + struct dwc3 *dwc = data->dwc;
> + struct device *dev = dwc->dev;
> + struct resource dwc_res;
> unsigned int hw_mode;
> void __iomem *regs;
> - struct dwc3 *dwc;
> + struct resource *res = data->res;
> int ret;
>
> - dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> - if (!dwc)
> - return -ENOMEM;
> -
> - dwc->dev = dev;
> -
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(dev, "missing memory resource\n");
> - return -ENODEV;
> - }
> -
> dwc->xhci_resources[0].start = res->start;
> dwc->xhci_resources[0].end = dwc->xhci_resources[0].start +
> DWC3_XHCI_REGS_END;
> @@ -2221,7 +2211,7 @@ static int dwc3_probe(struct platform_device *pdev)
> goto err_disable_clks;
> }
>
> - platform_set_drvdata(pdev, dwc);
> + dev_set_drvdata(dev, dwc);
> dwc3_cache_hwparams(dwc);
>
> if (!dwc->sysdev_is_parent &&
> @@ -2316,12 +2306,35 @@ static int dwc3_probe(struct platform_device *pdev)
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(dwc3_core_probe);
>
> -static void dwc3_remove(struct platform_device *pdev)
> +static int dwc3_probe(struct platform_device *pdev)
> {
> - struct dwc3 *dwc = platform_get_drvdata(pdev);
> + struct dwc3_probe_data probe_data;
> + struct resource *res;
> + struct dwc3 *dwc;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "missing memory resource\n");
> + return -ENODEV;
> + }
> +
> + dwc = devm_kzalloc(&pdev->dev, sizeof(*dwc), GFP_KERNEL);
> + if (!dwc)
> + return -ENOMEM;
>
> - pm_runtime_get_sync(&pdev->dev);
> + dwc->dev = &pdev->dev;
> +
> + probe_data.dwc = dwc;
> + probe_data.res = res;
> +
> + return dwc3_core_probe(&probe_data);
> +}
> +
> +void dwc3_core_remove(struct dwc3 *dwc)
> +{
> + pm_runtime_get_sync(dwc->dev);
>
> dwc3_core_exit_mode(dwc);
> dwc3_debugfs_exit(dwc);
> @@ -2329,22 +2342,28 @@ static void dwc3_remove(struct platform_device *pdev)
> dwc3_core_exit(dwc);
> dwc3_ulpi_exit(dwc);
>
> - pm_runtime_allow(&pdev->dev);
> - pm_runtime_disable(&pdev->dev);
> - pm_runtime_dont_use_autosuspend(&pdev->dev);
> - pm_runtime_put_noidle(&pdev->dev);
> + pm_runtime_allow(dwc->dev);
> + pm_runtime_disable(dwc->dev);
> + pm_runtime_dont_use_autosuspend(dwc->dev);
> + pm_runtime_put_noidle(dwc->dev);
> /*
> * HACK: Clear the driver data, which is currently accessed by parent
> * glue drivers, before allowing the parent to suspend.
> */
> - platform_set_drvdata(pdev, NULL);
> - pm_runtime_set_suspended(&pdev->dev);
> + dev_set_drvdata(dwc->dev, NULL);
> + pm_runtime_set_suspended(dwc->dev);
>
> dwc3_free_event_buffers(dwc);
>
> if (dwc->usb_psy)
> power_supply_put(dwc->usb_psy);
> }
> +EXPORT_SYMBOL_GPL(dwc3_core_remove);
> +
> +static void dwc3_remove(struct platform_device *pdev)
> +{
> + dwc3_core_remove(platform_get_drvdata(pdev));
> +}
>
> #ifdef CONFIG_PM
> static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> @@ -2533,9 +2552,8 @@ static int dwc3_runtime_checks(struct dwc3 *dwc)
> return 0;
> }
>
> -static int dwc3_runtime_suspend(struct device *dev)
> +int dwc3_runtime_suspend(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> if (dwc3_runtime_checks(dwc))
> @@ -2547,10 +2565,10 @@ static int dwc3_runtime_suspend(struct device *dev)
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_runtime_suspend);
>
> -static int dwc3_runtime_resume(struct device *dev)
> +int dwc3_runtime_resume(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> ret = dwc3_resume_common(dwc, PMSG_AUTO_RESUME);
> @@ -2571,15 +2589,14 @@ static int dwc3_runtime_resume(struct device *dev)
> break;
> }
>
> - pm_runtime_mark_last_busy(dev);
> + pm_runtime_mark_last_busy(dwc->dev);
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_runtime_resume);
>
> -static int dwc3_runtime_idle(struct device *dev)
> +int dwc3_runtime_idle(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> -
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> if (dwc3_runtime_checks(dwc))
> @@ -2591,53 +2608,68 @@ static int dwc3_runtime_idle(struct device *dev)
> break;
> }
>
> - pm_runtime_mark_last_busy(dev);
> - pm_runtime_autosuspend(dev);
> + pm_runtime_mark_last_busy(dwc->dev);
> + pm_runtime_autosuspend(dwc->dev);
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_runtime_idle);
> +
> +static int dwc3_plat_runtime_suspend(struct device *dev)
> +{
> + return dwc3_runtime_suspend(dev_get_drvdata(dev));
> +}
> +
> +static int dwc3_plat_runtime_resume(struct device *dev)
> +{
> + return dwc3_runtime_resume(dev_get_drvdata(dev));
> +}
> +
> +static int dwc3_plat_runtime_idle(struct device *dev)
> +{
> + return dwc3_runtime_idle(dev_get_drvdata(dev));
> +}
> #endif /* CONFIG_PM */
>
> #ifdef CONFIG_PM_SLEEP
> -static int dwc3_suspend(struct device *dev)
> +int dwc3_pm_suspend(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> int ret;
>
> ret = dwc3_suspend_common(dwc, PMSG_SUSPEND);
> if (ret)
> return ret;
>
> - pinctrl_pm_select_sleep_state(dev);
> + pinctrl_pm_select_sleep_state(dwc->dev);
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(dwc3_pm_suspend);
>
> -static int dwc3_resume(struct device *dev)
> +int dwc3_pm_resume(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
Can you add varible dev to reduce difference.
struct device *dev = dwc->dev;
also for other pm functions.
Frank
> int ret = 0;
>
> - pinctrl_pm_select_default_state(dev);
> + pinctrl_pm_select_default_state(dwc->dev);
>
> - pm_runtime_disable(dev);
> - ret = pm_runtime_set_active(dev);
> + pm_runtime_disable(dwc->dev);
> + ret = pm_runtime_set_active(dwc->dev);
> if (ret)
> goto out;
>
> ret = dwc3_resume_common(dwc, PMSG_RESUME);
> if (ret)
> - pm_runtime_set_suspended(dev);
> + pm_runtime_set_suspended(dwc->dev);
>
> out:
> - pm_runtime_enable(dev);
> + pm_runtime_enable(dwc->dev);
>
> return ret;
> }
> +EXPORT_SYMBOL_GPL(dwc3_pm_resume);
>
> -static void dwc3_complete(struct device *dev)
> +void dwc3_pm_complete(struct dwc3 *dwc)
> {
> - struct dwc3 *dwc = dev_get_drvdata(dev);
> u32 reg;
>
> if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
> @@ -2647,21 +2679,36 @@ static void dwc3_complete(struct device *dev)
> dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
> }
> }
> +EXPORT_SYMBOL_GPL(dwc3_pm_complete);
> +
> +static int dwc3_plat_suspend(struct device *dev)
> +{
> + return dwc3_pm_suspend(dev_get_drvdata(dev));
> +}
> +
> +static int dwc3_plat_resume(struct device *dev)
> +{
> + return dwc3_pm_resume(dev_get_drvdata(dev));
> +}
> +
> +static void dwc3_plat_complete(struct device *dev)
> +{
> + dwc3_pm_complete(dev_get_drvdata(dev));
> +}
> #else
> -#define dwc3_complete NULL
> +#define dwc3_plat_complete NULL
> #endif /* CONFIG_PM_SLEEP */
>
> static const struct dev_pm_ops dwc3_dev_pm_ops = {
> - SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
> - .complete = dwc3_complete,
> -
> + SET_SYSTEM_SLEEP_PM_OPS(dwc3_plat_suspend, dwc3_plat_resume)
> + .complete = dwc3_plat_complete,
> /*
> * Runtime suspend halts the controller on disconnection. It relies on
> * platforms with custom connection notification to start the controller
> * again.
> */
> - SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
> - dwc3_runtime_idle)
> + SET_RUNTIME_PM_OPS(dwc3_plat_runtime_suspend, dwc3_plat_runtime_resume,
> + dwc3_plat_runtime_idle)
> };
>
> #ifdef CONFIG_OF
> diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
> new file mode 100644
> index 000000000000..e73cfc466012
> --- /dev/null
> +++ b/drivers/usb/dwc3/glue.h
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * glue.h - DesignWare USB3 DRD glue header
> + */
> +
> +#ifndef __DRIVERS_USB_DWC3_GLUE_H
> +#define __DRIVERS_USB_DWC3_GLUE_H
> +
> +#include <linux/types.h>
> +#include "core.h"
> +
> +/**
> + * dwc3_probe_data: Initialization parameters passed to dwc3_core_probe()
> + * @dwc: Reference to dwc3 context structure
> + * @res: resource for the DWC3 core mmio region
> + */
> +struct dwc3_probe_data {
> + struct dwc3 *dwc;
> + struct resource *res;
> +};
> +
> +int dwc3_core_probe(const struct dwc3_probe_data *data);
> +void dwc3_core_remove(struct dwc3 *dwc);
> +
> +int dwc3_runtime_suspend(struct dwc3 *dwc);
> +int dwc3_runtime_resume(struct dwc3 *dwc);
> +int dwc3_runtime_idle(struct dwc3 *dwc);
> +int dwc3_pm_suspend(struct dwc3 *dwc);
> +int dwc3_pm_resume(struct dwc3 *dwc);
> +void dwc3_pm_complete(struct dwc3 *dwc);
> +
> +#endif
>
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks
2025-02-27 0:17 ` [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks Bjorn Andersson
2025-02-27 4:24 ` Dmitry Baryshkov
@ 2025-02-27 19:22 ` Frank Li
1 sibling, 0 replies; 30+ messages in thread
From: Frank Li @ 2025-02-27 19:22 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
linux-arm-msm, linux-usb, devicetree, linux-kernel
On Wed, Feb 26, 2025 at 04:17:50PM -0800, 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.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/core.c | 19 +++++++++++--------
> drivers/usb/dwc3/glue.h | 1 +
> 2 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index d9f0a6782d36..aecdde8dc999 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -2187,15 +2187,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)
> @@ -2328,6 +2330,7 @@ static int dwc3_probe(struct platform_device *pdev)
>
> probe_data.dwc = dwc;
> probe_data.res = res;
> + probe_data.ignore_clocks_and_resets = false;
>
> return dwc3_core_probe(&probe_data);
> }
> diff --git a/drivers/usb/dwc3/glue.h b/drivers/usb/dwc3/glue.h
> index e73cfc466012..1ddb451bdbd0 100644
> --- a/drivers/usb/dwc3/glue.h
> +++ b/drivers/usb/dwc3/glue.h
> @@ -17,6 +17,7 @@
> struct dwc3_probe_data {
> struct dwc3 *dwc;
> struct resource *res;
> + bool ignore_clocks_and_resets;
Nit: suggest add kernel doc comments for each fields
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> };
>
> int dwc3_core_probe(const struct dwc3_probe_data *data);
>
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3
2025-02-27 0:17 ` [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3 Bjorn Andersson
@ 2025-02-28 22:36 ` Rob Herring
2025-03-03 22:11 ` Bjorn Andersson
0 siblings, 1 reply; 30+ messages in thread
From: Rob Herring @ 2025-02-28 22:36 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Conor Dooley,
Felipe Balbi, Wesley Cheng, Saravana Kannan, Thinh Nguyen,
Philipp Zabel, Bjorn Andersson, Konrad Dybcio, Frank Li,
linux-arm-msm, linux-usb, devicetree, linux-kernel
On Wed, Feb 26, 2025 at 04:17:48PM -0800, 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 | 619 +++++++++++++++++++++
> 2 files changed, 631 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index a2b3cf625e5b..6d818e6dddbc 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -4,11 +4,22 @@
> $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
> $schema: http://devicetree.org/meta-schemas/core.yaml#
>
> -title: Qualcomm SuperSpeed DWC3 USB SoC controller
> +title: Legacy Qualcomm SuperSpeed DWC3 USB SoC controller
>
> maintainers:
> - Wesley Cheng <quic_wcheng@quicinc.com>
>
> +# Use the combined qcom,snps-dwc3 instead
> +deprecated: true
> +
> +select:
> + properties:
> + compatible:
> + contains:
> + const: qcom,dwc3
> + required:
> + - compatible
> +
> properties:
> compatible:
> items:
> diff --git a/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
> new file mode 100644
> index 000000000000..37af52e01803
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
> @@ -0,0 +1,619 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SuperSpeed DWC3 USB SoC controller
> +
> +maintainers:
> + - Wesley Cheng <quic_wcheng@quicinc.com>
> +
> +description:
> + Describes the Qualcomm USB block, based on Synopsys DWC3.
> +
> +select:
> + properties:
> + compatible:
> + contains:
> + const: qcom,snps-dwc3
> + required:
> + - compatible
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - qcom,ipq4019-dwc3
> + - qcom,ipq5018-dwc3
> + - qcom,ipq5332-dwc3
> + - qcom,ipq5424-dwc3
> + - qcom,ipq6018-dwc3
> + - qcom,ipq8064-dwc3
> + - qcom,ipq8074-dwc3
> + - qcom,ipq9574-dwc3
> + - qcom,msm8953-dwc3
> + - qcom,msm8994-dwc3
> + - qcom,msm8996-dwc3
> + - qcom,msm8998-dwc3
> + - qcom,qcm2290-dwc3
> + - qcom,qcs404-dwc3
> + - qcom,qcs615-dwc3
> + - qcom,qcs8300-dwc3
> + - qcom,qdu1000-dwc3
> + - qcom,sa8775p-dwc3
> + - qcom,sar2130p-dwc3
> + - qcom,sc7180-dwc3
> + - qcom,sc7280-dwc3
> + - qcom,sc8180x-dwc3
> + - qcom,sc8180x-dwc3-mp
> + - qcom,sc8280xp-dwc3
> + - qcom,sc8280xp-dwc3-mp
> + - qcom,sdm660-dwc3
> + - qcom,sdm670-dwc3
> + - qcom,sdm845-dwc3
> + - qcom,sdx55-dwc3
> + - qcom,sdx65-dwc3
> + - qcom,sdx75-dwc3
> + - qcom,sm4250-dwc3
> + - qcom,sm6115-dwc3
> + - qcom,sm6125-dwc3
> + - qcom,sm6350-dwc3
> + - qcom,sm6375-dwc3
> + - qcom,sm8150-dwc3
> + - qcom,sm8250-dwc3
> + - qcom,sm8350-dwc3
> + - qcom,sm8450-dwc3
> + - qcom,sm8550-dwc3
> + - qcom,sm8650-dwc3
> + - qcom,x1e80100-dwc3
> + - const: qcom,snps-dwc3
> +
> + reg:
> + description: Offset and length of register set for QSCRATCH wrapper
I think you want to drop this. Or do you need 2 regions? The wrapper
regs and the DWC3 regs? Probably worth describing separately even if
they are adjacent currently.
> + maxItems: 1
> +
> + power-domains:
> + description: specifies a phandle to PM domain provider node
Drop the description.
Otherwise, looks good.
Rob
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3
2025-02-28 22:36 ` Rob Herring
@ 2025-03-03 22:11 ` Bjorn Andersson
0 siblings, 0 replies; 30+ messages in thread
From: Bjorn Andersson @ 2025-03-03 22:11 UTC (permalink / raw)
To: Rob Herring
Cc: Bjorn Andersson, Greg Kroah-Hartman, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm, linux-usb, devicetree, linux-kernel
On Fri, Feb 28, 2025 at 04:36:14PM -0600, Rob Herring wrote:
> On Wed, Feb 26, 2025 at 04:17:48PM -0800, 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 | 619 +++++++++++++++++++++
> > 2 files changed, 631 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > index a2b3cf625e5b..6d818e6dddbc 100644
> > --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> > @@ -4,11 +4,22 @@
> > $id: http://devicetree.org/schemas/usb/qcom,dwc3.yaml#
> > $schema: http://devicetree.org/meta-schemas/core.yaml#
> >
> > -title: Qualcomm SuperSpeed DWC3 USB SoC controller
> > +title: Legacy Qualcomm SuperSpeed DWC3 USB SoC controller
> >
> > maintainers:
> > - Wesley Cheng <quic_wcheng@quicinc.com>
> >
> > +# Use the combined qcom,snps-dwc3 instead
> > +deprecated: true
> > +
> > +select:
> > + properties:
> > + compatible:
> > + contains:
> > + const: qcom,dwc3
> > + required:
> > + - compatible
> > +
> > properties:
> > compatible:
> > items:
> > diff --git a/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
> > new file mode 100644
> > index 000000000000..37af52e01803
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/qcom,snps-dwc3.yaml
> > @@ -0,0 +1,619 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/usb/qcom,snps-dwc3.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm SuperSpeed DWC3 USB SoC controller
> > +
> > +maintainers:
> > + - Wesley Cheng <quic_wcheng@quicinc.com>
> > +
> > +description:
> > + Describes the Qualcomm USB block, based on Synopsys DWC3.
> > +
> > +select:
> > + properties:
> > + compatible:
> > + contains:
> > + const: qcom,snps-dwc3
> > + required:
> > + - compatible
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - qcom,ipq4019-dwc3
> > + - qcom,ipq5018-dwc3
> > + - qcom,ipq5332-dwc3
> > + - qcom,ipq5424-dwc3
> > + - qcom,ipq6018-dwc3
> > + - qcom,ipq8064-dwc3
> > + - qcom,ipq8074-dwc3
> > + - qcom,ipq9574-dwc3
> > + - qcom,msm8953-dwc3
> > + - qcom,msm8994-dwc3
> > + - qcom,msm8996-dwc3
> > + - qcom,msm8998-dwc3
> > + - qcom,qcm2290-dwc3
> > + - qcom,qcs404-dwc3
> > + - qcom,qcs615-dwc3
> > + - qcom,qcs8300-dwc3
> > + - qcom,qdu1000-dwc3
> > + - qcom,sa8775p-dwc3
> > + - qcom,sar2130p-dwc3
> > + - qcom,sc7180-dwc3
> > + - qcom,sc7280-dwc3
> > + - qcom,sc8180x-dwc3
> > + - qcom,sc8180x-dwc3-mp
> > + - qcom,sc8280xp-dwc3
> > + - qcom,sc8280xp-dwc3-mp
> > + - qcom,sdm660-dwc3
> > + - qcom,sdm670-dwc3
> > + - qcom,sdm845-dwc3
> > + - qcom,sdx55-dwc3
> > + - qcom,sdx65-dwc3
> > + - qcom,sdx75-dwc3
> > + - qcom,sm4250-dwc3
> > + - qcom,sm6115-dwc3
> > + - qcom,sm6125-dwc3
> > + - qcom,sm6350-dwc3
> > + - qcom,sm6375-dwc3
> > + - qcom,sm8150-dwc3
> > + - qcom,sm8250-dwc3
> > + - qcom,sm8350-dwc3
> > + - qcom,sm8450-dwc3
> > + - qcom,sm8550-dwc3
> > + - qcom,sm8650-dwc3
> > + - qcom,x1e80100-dwc3
> > + - const: qcom,snps-dwc3
> > +
> > + reg:
> > + description: Offset and length of register set for QSCRATCH wrapper
>
> I think you want to drop this. Or do you need 2 regions? The wrapper
> regs and the DWC3 regs? Probably worth describing separately even if
> they are adjacent currently.
>
There's now a single node, with a single "reg" indended to cover DWC3,
XHCI, and Qualcomm glue (qscratch).
I contemplated describing the separate parts in reg, but if so it would
make sense to also split the dwc3-region into it's dwc3 and xhci parts.
I think it makes sense to go the other direction and just describe all
three in one.
That said, I obviously failed to update the description to match my
intentions here. It seems reasonable to me to just drop it.
Regards,
Bjorn
> > + maxItems: 1
> > +
> > + power-domains:
> > + description: specifies a phandle to PM domain provider node
>
> Drop the description.
>
> Otherwise, looks good.
>
> Rob
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-02-27 0:17 ` [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty Bjorn Andersson
@ 2025-03-04 0:05 ` Thinh Nguyen
2025-03-04 0:39 ` Thinh Nguyen
0 siblings, 1 reply; 30+ messages in thread
From: Thinh Nguyen @ 2025-03-04 0:05 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li, linux-arm-msm@vger.kernel.org,
linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> In order to more tightly integrate the Qualcomm glue driver with the
> dwc3 core the driver is redesigned to avoid splitting the implementation
> using the driver model. But due to the strong coupling to the Devicetree
> binding needs to be updated as well.
>
> Various ways to provide backwards compatibility with existing Devicetree
> blobs has been explored, but migrating the Devicetree information
> between the old and the new binding is non-trivial.
>
> For the vast majority of boards out there, the kernel and Devicetree are
> generated and handled together, which in practice means that backwards
> compatibility needs to be managed across about 1 kernel release.
>
> For some though, such as the various Snapdragon laptops, the Devicetree
> blobs live a life separate of the kernel. In each one of these, with the
> continued extension of new features, it's recommended that users would
> upgrade their Devicetree somewhat frequently.
>
> With this in mind, simply carrying a snapshot/copy of the current driver
> is simpler than creating and maintaining the migration code.
>
> The driver is kept under the same Kconfig option, to ensure that Linux
> distributions doesn't drop USB support on these platforms.
>
> The driver, which is going to be refactored to handle the newly
> introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> match against any compatible.
>
> This driver should be removed after 2 LTS releases.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/Makefile | 1 +
> drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> drivers/usb/dwc3/dwc3-qcom.c | 1 -
> 3 files changed, 935 insertions(+), 1 deletion(-)
>
This is a bit concerning if there's no matching compatible string. ie.
we don't have user for the new driver without downstream dependencies
(or some workaround in the driver binding).
While I understand the intention, I'm afraid we may have to support and
maintain this much longer than the proposed 2 LTS releases (as seen with
anything tagged with "legacy" in the upstream kernel). If possible, I'd
prefer the complications of maintenance of the migration code be handled
downstream.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-04 0:05 ` Thinh Nguyen
@ 2025-03-04 0:39 ` Thinh Nguyen
2025-03-04 3:13 ` Bjorn Andersson
0 siblings, 1 reply; 30+ messages in thread
From: Thinh Nguyen @ 2025-03-04 0:39 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Philipp Zabel, Bjorn Andersson, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > In order to more tightly integrate the Qualcomm glue driver with the
> > dwc3 core the driver is redesigned to avoid splitting the implementation
> > using the driver model. But due to the strong coupling to the Devicetree
> > binding needs to be updated as well.
> >
> > Various ways to provide backwards compatibility with existing Devicetree
> > blobs has been explored, but migrating the Devicetree information
> > between the old and the new binding is non-trivial.
> >
> > For the vast majority of boards out there, the kernel and Devicetree are
> > generated and handled together, which in practice means that backwards
> > compatibility needs to be managed across about 1 kernel release.
> >
> > For some though, such as the various Snapdragon laptops, the Devicetree
> > blobs live a life separate of the kernel. In each one of these, with the
> > continued extension of new features, it's recommended that users would
> > upgrade their Devicetree somewhat frequently.
> >
> > With this in mind, simply carrying a snapshot/copy of the current driver
> > is simpler than creating and maintaining the migration code.
> >
> > The driver is kept under the same Kconfig option, to ensure that Linux
> > distributions doesn't drop USB support on these platforms.
> >
> > The driver, which is going to be refactored to handle the newly
> > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > match against any compatible.
> >
> > This driver should be removed after 2 LTS releases.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > ---
> > drivers/usb/dwc3/Makefile | 1 +
> > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > 3 files changed, 935 insertions(+), 1 deletion(-)
> >
>
> This is a bit concerning if there's no matching compatible string. ie.
> we don't have user for the new driver without downstream dependencies
> (or some workaround in the driver binding).
Ignore the comment above, I missed the "temporarily" in your log
above. However, the comment below still stands.
>
> While I understand the intention, I'm afraid we may have to support and
> maintain this much longer than the proposed 2 LTS releases (as seen with
> anything tagged with "legacy" in the upstream kernel). If possible, I'd
> prefer the complications of maintenance of the migration code be handled
> downstream.
>
Thanks,
Thinh
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-04 0:39 ` Thinh Nguyen
@ 2025-03-04 3:13 ` Bjorn Andersson
2025-03-05 0:31 ` Thinh Nguyen
0 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-03-04 3:13 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > In order to more tightly integrate the Qualcomm glue driver with the
> > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > using the driver model. But due to the strong coupling to the Devicetree
> > > binding needs to be updated as well.
> > >
> > > Various ways to provide backwards compatibility with existing Devicetree
> > > blobs has been explored, but migrating the Devicetree information
> > > between the old and the new binding is non-trivial.
> > >
> > > For the vast majority of boards out there, the kernel and Devicetree are
> > > generated and handled together, which in practice means that backwards
> > > compatibility needs to be managed across about 1 kernel release.
> > >
> > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > blobs live a life separate of the kernel. In each one of these, with the
> > > continued extension of new features, it's recommended that users would
> > > upgrade their Devicetree somewhat frequently.
> > >
> > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > is simpler than creating and maintaining the migration code.
> > >
> > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > distributions doesn't drop USB support on these platforms.
> > >
> > > The driver, which is going to be refactored to handle the newly
> > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > match against any compatible.
> > >
> > > This driver should be removed after 2 LTS releases.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > ---
> > > drivers/usb/dwc3/Makefile | 1 +
> > > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > > 3 files changed, 935 insertions(+), 1 deletion(-)
> > >
> >
> > This is a bit concerning if there's no matching compatible string. ie.
> > we don't have user for the new driver without downstream dependencies
> > (or some workaround in the driver binding).
>
> Ignore the comment above, I missed the "temporarily" in your log
> above. However, the comment below still stands.
>
> >
> > While I understand the intention, I'm afraid we may have to support and
> > maintain this much longer than the proposed 2 LTS releases (as seen with
> > anything tagged with "legacy" in the upstream kernel).
There are no products shipping today using dwc3-qcom where Devicetree is
considered firmware. The primary audience for a longer transition is
users of the various laptops with Qualcomm-chip in them. But given the
rapid development in a variety of functional areas, these users will be
highly compelled to update their DTBs within 2 years.
The other obvious user group is to make sure us upstream developers
don't loose USB when things get out of sync.
That said, if the model defined here is to be followed in other cases
(or my other vendors) where Devicetree is treated as firmware, your
concerns are valid - and it might be worth taking the cost of managing
the live-migration code.
> > If possible, I'd
> > prefer the complications of maintenance of the migration code be handled
> > downstream.
> >
I'm sorry, but here it sounds like you're saying that you don't want any
migration code upstream at all? This is not possible, as this will break
USB for developers and users short term. We can of course discuss the 2
LTS though, if you want a shorter life span for this migration.
In my view, setting a flag date when the dwc3-qcom-legacy.c will be
removed will provide upstream users a transition period, at a very low
additional cost (934 lines of already tested code). If someone
downstream after that flag date wants to retain support for qcom,dwc3
they can just revert the removal of dwc3-qcom-legacy.c.
The alternative is that I try to get the migration code suggested in v3
to a state where it can be merged (right now it's 6x larger) and then
keep investing indefinitely in making sure it's not bit-rotting
(although Rob Herring did request a flag date of the migration code in
v3 as well...).
Regards,
Bjorn
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-04 3:13 ` Bjorn Andersson
@ 2025-03-05 0:31 ` Thinh Nguyen
2025-03-06 22:49 ` Bjorn Andersson
0 siblings, 1 reply; 30+ messages in thread
From: Thinh Nguyen @ 2025-03-05 0:31 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Thinh Nguyen, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Mon, Mar 03, 2025, Bjorn Andersson wrote:
> On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> > On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > > In order to more tightly integrate the Qualcomm glue driver with the
> > > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > > using the driver model. But due to the strong coupling to the Devicetree
> > > > binding needs to be updated as well.
> > > >
> > > > Various ways to provide backwards compatibility with existing Devicetree
> > > > blobs has been explored, but migrating the Devicetree information
> > > > between the old and the new binding is non-trivial.
> > > >
> > > > For the vast majority of boards out there, the kernel and Devicetree are
> > > > generated and handled together, which in practice means that backwards
> > > > compatibility needs to be managed across about 1 kernel release.
> > > >
> > > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > > blobs live a life separate of the kernel. In each one of these, with the
> > > > continued extension of new features, it's recommended that users would
> > > > upgrade their Devicetree somewhat frequently.
> > > >
> > > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > > is simpler than creating and maintaining the migration code.
> > > >
> > > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > > distributions doesn't drop USB support on these platforms.
> > > >
> > > > The driver, which is going to be refactored to handle the newly
> > > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > > match against any compatible.
> > > >
> > > > This driver should be removed after 2 LTS releases.
> > > >
> > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > > ---
> > > > drivers/usb/dwc3/Makefile | 1 +
> > > > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > > > 3 files changed, 935 insertions(+), 1 deletion(-)
> > > >
> > >
> > > This is a bit concerning if there's no matching compatible string. ie.
> > > we don't have user for the new driver without downstream dependencies
> > > (or some workaround in the driver binding).
> >
> > Ignore the comment above, I missed the "temporarily" in your log
> > above. However, the comment below still stands.
> >
> > >
> > > While I understand the intention, I'm afraid we may have to support and
> > > maintain this much longer than the proposed 2 LTS releases (as seen with
> > > anything tagged with "legacy" in the upstream kernel).
>
> There are no products shipping today using dwc3-qcom where Devicetree is
> considered firmware. The primary audience for a longer transition is
> users of the various laptops with Qualcomm-chip in them. But given the
> rapid development in a variety of functional areas, these users will be
> highly compelled to update their DTBs within 2 years.
>
> The other obvious user group is to make sure us upstream developers
> don't loose USB when things get out of sync.
>
>
> That said, if the model defined here is to be followed in other cases
> (or my other vendors) where Devicetree is treated as firmware, your
> concerns are valid - and it might be worth taking the cost of managing
> the live-migration code.
>
> > > If possible, I'd
> > > prefer the complications of maintenance of the migration code be handled
> > > downstream.
> > >
>
> I'm sorry, but here it sounds like you're saying that you don't want any
> migration code upstream at all? This is not possible, as this will break
> USB for developers and users short term. We can of course discuss the 2
> LTS though, if you want a shorter life span for this migration.
>
My first concern is now we have a legacy driver that should not be
continued to be developed while we also need to address any
regression/fixes found in the future from the legacy driver. While I
would encourage users to start migrating to the new driver, I won't
reject fixes to the legacy driver either. In the next 2 years+, my
other concern is that I'm not confident that we can easily remove the
legacy driver and the DTS then.
Code can break, and that's not unexpected. If 2 LTS releases later and
we remove the dwc3-qcom-legacy, things can break then too. This may just
as be painful if we need fixes to the legacy driver due to some previous
regression. Also, I'm sure your team did a fair share of testing the new
driver right? Is there some major concern in the new driver that we
haven't addressed?
>
> In my view, setting a flag date when the dwc3-qcom-legacy.c will be
> removed will provide upstream users a transition period, at a very low
> additional cost (934 lines of already tested code). If someone
> downstream after that flag date wants to retain support for qcom,dwc3
> they can just revert the removal of dwc3-qcom-legacy.c.
The same can be said that they can revert the update (or apply fixes)
should they found issue with the new change.
>
> The alternative is that I try to get the migration code suggested in v3
> to a state where it can be merged (right now it's 6x larger) and then
> keep investing indefinitely in making sure it's not bit-rotting
> (although Rob Herring did request a flag date of the migration code in
> v3 as well...).
>
All that said, if you believe that this transition will be quite
disruptive without preserving the legacy driver/dts, then we will do so.
Can I request that you make this snapshot as one of the first patches in
the series so reverts/git-blames can easily be traced?
BR,
Thinh
Side question: for Snapdragon laptops, without the corresponding kernel
and DTS updates, don't things break easily?
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-05 0:31 ` Thinh Nguyen
@ 2025-03-06 22:49 ` Bjorn Andersson
2025-03-07 16:17 ` Frank Li
2025-03-07 23:00 ` Thinh Nguyen
0 siblings, 2 replies; 30+ messages in thread
From: Bjorn Andersson @ 2025-03-06 22:49 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, Mar 05, 2025 at 12:31:49AM +0000, Thinh Nguyen wrote:
> On Mon, Mar 03, 2025, Bjorn Andersson wrote:
> > On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> > > On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > > > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > > > In order to more tightly integrate the Qualcomm glue driver with the
> > > > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > > > using the driver model. But due to the strong coupling to the Devicetree
> > > > > binding needs to be updated as well.
> > > > >
> > > > > Various ways to provide backwards compatibility with existing Devicetree
> > > > > blobs has been explored, but migrating the Devicetree information
> > > > > between the old and the new binding is non-trivial.
> > > > >
> > > > > For the vast majority of boards out there, the kernel and Devicetree are
> > > > > generated and handled together, which in practice means that backwards
> > > > > compatibility needs to be managed across about 1 kernel release.
> > > > >
> > > > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > > > blobs live a life separate of the kernel. In each one of these, with the
> > > > > continued extension of new features, it's recommended that users would
> > > > > upgrade their Devicetree somewhat frequently.
> > > > >
> > > > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > > > is simpler than creating and maintaining the migration code.
> > > > >
> > > > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > > > distributions doesn't drop USB support on these platforms.
> > > > >
> > > > > The driver, which is going to be refactored to handle the newly
> > > > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > > > match against any compatible.
> > > > >
> > > > > This driver should be removed after 2 LTS releases.
> > > > >
> > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > > > ---
> > > > > drivers/usb/dwc3/Makefile | 1 +
> > > > > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > > > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > > > > 3 files changed, 935 insertions(+), 1 deletion(-)
> > > > >
> > > >
> > > > This is a bit concerning if there's no matching compatible string. ie.
> > > > we don't have user for the new driver without downstream dependencies
> > > > (or some workaround in the driver binding).
> > >
> > > Ignore the comment above, I missed the "temporarily" in your log
> > > above. However, the comment below still stands.
> > >
> > > >
> > > > While I understand the intention, I'm afraid we may have to support and
> > > > maintain this much longer than the proposed 2 LTS releases (as seen with
> > > > anything tagged with "legacy" in the upstream kernel).
> >
> > There are no products shipping today using dwc3-qcom where Devicetree is
> > considered firmware. The primary audience for a longer transition is
> > users of the various laptops with Qualcomm-chip in them. But given the
> > rapid development in a variety of functional areas, these users will be
> > highly compelled to update their DTBs within 2 years.
> >
> > The other obvious user group is to make sure us upstream developers
> > don't loose USB when things get out of sync.
> >
> >
> > That said, if the model defined here is to be followed in other cases
> > (or my other vendors) where Devicetree is treated as firmware, your
> > concerns are valid - and it might be worth taking the cost of managing
> > the live-migration code.
> >
> > > > If possible, I'd
> > > > prefer the complications of maintenance of the migration code be handled
> > > > downstream.
> > > >
> >
> > I'm sorry, but here it sounds like you're saying that you don't want any
> > migration code upstream at all? This is not possible, as this will break
> > USB for developers and users short term. We can of course discuss the 2
> > LTS though, if you want a shorter life span for this migration.
> >
>
> My first concern is now we have a legacy driver that should not be
> continued to be developed while we also need to address any
> regression/fixes found in the future from the legacy driver. While I
> would encourage users to start migrating to the new driver, I won't
> reject fixes to the legacy driver either. In the next 2 years+, my
> other concern is that I'm not confident that we can easily remove the
> legacy driver and the DTS then.
>
The problem at hand is that the driver _needs_ a bunch of work.
Role-switching only works sometimes, extcon is (for older platforms)
duplicated in both glue and core - with the hope that each part does its
thing in a suitable fashion, the layering violations can trigger
NULL-pointer dereferences or use-after-free, PM runtime is marked
forbidden...
We've looked at these problems for a few years now, without coming up
with any solution to address these issues within the current design.
Following this refactor, we will be able to work on these improvements.
For this to happen, I intend to transition all the
arch/*/boot/dts/qcom/* platforms to the new binding as soon as possible.
Looking ahead, when we hit the point of deprecating the dwc3-qcom-legacy
driver:
The upstream-based product we have today do ship Devicetree in
combination with the kernel, so they would upgrade both together and get
the new driver.
The other group would be kernel developers, enthusiasts, specific users
who for some reason is upgrading their kernel but not their Devicetree.
These users will want the new features and stability we're bringing.
> Code can break, and that's not unexpected. If 2 LTS releases later and
> we remove the dwc3-qcom-legacy, things can break then too. This may just
> as be painful if we need fixes to the legacy driver due to some previous
> regression. Also, I'm sure your team did a fair share of testing the new
> driver right? Is there some major concern in the new driver that we
> haven't addressed?
>
The new and old drivers are mostly identical at this point, and expected
to diverge from here.
The one thing I have identified to differ is that the "legacy" driver
supports 2 extcon handles in the glue, but this is not considered
acceptable by the binding so I haven't found anyone actually exercising
this code path - then again extcon and usb_role_switch is one of the
things this enables us to clean up.
That said, while this model seems suitable for Qualcomm, due to the
current state of things, I don't know if the same is true for Frank Li,
perhaps NXP has a broader user base and need the migration logic.
> >
> > In my view, setting a flag date when the dwc3-qcom-legacy.c will be
> > removed will provide upstream users a transition period, at a very low
> > additional cost (934 lines of already tested code). If someone
> > downstream after that flag date wants to retain support for qcom,dwc3
> > they can just revert the removal of dwc3-qcom-legacy.c.
>
> The same can be said that they can revert the update (or apply fixes)
> should they found issue with the new change.
>
We're changing the Devicetree binding, which gives us two problems:
1) Devicetree source code and DWC3 driver code are merged through
different trees.
2) The compiled Devicetree (.dtb) and kernel image are in some cases
separate software deliverables.
So we absolutely need some migration mechanism to not just break USB for
everyone for the coming 1-2 releases - at least.
That said, the "2 LTS" is completely arbitrary. If you prefer to limit
that, we can certainly have that discussion! E.g. I wouldn't argue
against setting the flag-date by the end of this year.
> >
> > The alternative is that I try to get the migration code suggested in v3
> > to a state where it can be merged (right now it's 6x larger) and then
> > keep investing indefinitely in making sure it's not bit-rotting
> > (although Rob Herring did request a flag date of the migration code in
> > v3 as well...).
> >
>
> All that said, if you believe that this transition will be quite
> disruptive without preserving the legacy driver/dts, then we will do so.
>
We absolutely need a transition period, per above reasons. The length of
it is an open question.
> Can I request that you make this snapshot as one of the first patches in
> the series so reverts/git-blames can easily be traced?
>
Absolutely.
> BR,
> Thinh
>
> Side question: for Snapdragon laptops, without the corresponding kernel
> and DTS updates, don't things break easily?
It certainly happens, but maintaining backwards compatibility is
something we're striving for. As the Devicetree bindings mature, the
easier this is though.
One example where this is a problem will be clear here, that users
attempting to boot today's kernel with tomorrows Devicetree blobs will
not get USB - because today's kernel doesn't know how to make of the
information in that description.
This is true for any hardware or firmware interface though, so there's
only so much one can do about that (and whatever that is, we're trying
to do - for the sake of user friendliness).
Regards,
Bjorn
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model
2025-02-27 0:17 ` [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model Bjorn Andersson
@ 2025-03-07 6:41 ` Dmitry Baryshkov
2025-03-11 2:46 ` Bjorn Andersson
0 siblings, 1 reply; 30+ messages in thread
From: Dmitry Baryshkov @ 2025-03-07 6:41 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Felipe Balbi, Wesley Cheng, Saravana Kannan,
Thinh Nguyen, Philipp Zabel, Bjorn Andersson, Konrad Dybcio,
Frank Li, linux-arm-msm, linux-usb, devicetree, linux-kernel
On Wed, Feb 26, 2025 at 04:17:53PM -0800, 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@oss.qualcomm.com>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 138 +++++++++++++++++++++----------------------
> 1 file changed, 69 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 9d04c2457433..63e60f15ceaa 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>
As a heads up, would it make sense to also drop extcon support while we
are transitioning to the new driver / DT bindings?
> #include <linux/interconnect.h>
> -#include <linux/of_platform.h>
> #include <linux/platform_device.h>
> #include <linux/phy/phy.h>
> #include <linux/usb/of.h>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-06 22:49 ` Bjorn Andersson
@ 2025-03-07 16:17 ` Frank Li
2025-03-07 23:00 ` Thinh Nguyen
1 sibling, 0 replies; 30+ messages in thread
From: Frank Li @ 2025-03-07 16:17 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Thinh Nguyen, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Thu, Mar 06, 2025 at 04:49:28PM -0600, Bjorn Andersson wrote:
> On Wed, Mar 05, 2025 at 12:31:49AM +0000, Thinh Nguyen wrote:
> > On Mon, Mar 03, 2025, Bjorn Andersson wrote:
> > > On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> > > > On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > > > > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > > > > In order to more tightly integrate the Qualcomm glue driver with the
> > > > > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > > > > using the driver model. But due to the strong coupling to the Devicetree
> > > > > > binding needs to be updated as well.
> > > > > >
> > > > > > Various ways to provide backwards compatibility with existing Devicetree
> > > > > > blobs has been explored, but migrating the Devicetree information
> > > > > > between the old and the new binding is non-trivial.
> > > > > >
> > > > > > For the vast majority of boards out there, the kernel and Devicetree are
> > > > > > generated and handled together, which in practice means that backwards
> > > > > > compatibility needs to be managed across about 1 kernel release.
> > > > > >
> > > > > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > > > > blobs live a life separate of the kernel. In each one of these, with the
> > > > > > continued extension of new features, it's recommended that users would
> > > > > > upgrade their Devicetree somewhat frequently.
> > > > > >
> > > > > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > > > > is simpler than creating and maintaining the migration code.
> > > > > >
> > > > > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > > > > distributions doesn't drop USB support on these platforms.
> > > > > >
> > > > > > The driver, which is going to be refactored to handle the newly
> > > > > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > > > > match against any compatible.
> > > > > >
> > > > > > This driver should be removed after 2 LTS releases.
> > > > > >
> > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > > > > ---
> > > > > > drivers/usb/dwc3/Makefile | 1 +
> > > > > > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > > > > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > > > > > 3 files changed, 935 insertions(+), 1 deletion(-)
> > > > > >
> > > > >
> > > > > This is a bit concerning if there's no matching compatible string. ie.
> > > > > we don't have user for the new driver without downstream dependencies
> > > > > (or some workaround in the driver binding).
> > > >
> > > > Ignore the comment above, I missed the "temporarily" in your log
> > > > above. However, the comment below still stands.
> > > >
> > > > >
> > > > > While I understand the intention, I'm afraid we may have to support and
> > > > > maintain this much longer than the proposed 2 LTS releases (as seen with
> > > > > anything tagged with "legacy" in the upstream kernel).
> > >
> > > There are no products shipping today using dwc3-qcom where Devicetree is
> > > considered firmware. The primary audience for a longer transition is
> > > users of the various laptops with Qualcomm-chip in them. But given the
> > > rapid development in a variety of functional areas, these users will be
> > > highly compelled to update their DTBs within 2 years.
> > >
> > > The other obvious user group is to make sure us upstream developers
> > > don't loose USB when things get out of sync.
> > >
> > >
> > > That said, if the model defined here is to be followed in other cases
> > > (or my other vendors) where Devicetree is treated as firmware, your
> > > concerns are valid - and it might be worth taking the cost of managing
> > > the live-migration code.
> > >
> > > > > If possible, I'd
> > > > > prefer the complications of maintenance of the migration code be handled
> > > > > downstream.
> > > > >
> > >
> > > I'm sorry, but here it sounds like you're saying that you don't want any
> > > migration code upstream at all? This is not possible, as this will break
> > > USB for developers and users short term. We can of course discuss the 2
> > > LTS though, if you want a shorter life span for this migration.
> > >
> >
> > My first concern is now we have a legacy driver that should not be
> > continued to be developed while we also need to address any
> > regression/fixes found in the future from the legacy driver. While I
> > would encourage users to start migrating to the new driver, I won't
> > reject fixes to the legacy driver either. In the next 2 years+, my
> > other concern is that I'm not confident that we can easily remove the
> > legacy driver and the DTS then.
> >
>
> The problem at hand is that the driver _needs_ a bunch of work.
> Role-switching only works sometimes, extcon is (for older platforms)
> duplicated in both glue and core - with the hope that each part does its
> thing in a suitable fashion, the layering violations can trigger
> NULL-pointer dereferences or use-after-free, PM runtime is marked
> forbidden...
>
> We've looked at these problems for a few years now, without coming up
> with any solution to address these issues within the current design.
>
> Following this refactor, we will be able to work on these improvements.
> For this to happen, I intend to transition all the
> arch/*/boot/dts/qcom/* platforms to the new binding as soon as possible.
>
>
> Looking ahead, when we hit the point of deprecating the dwc3-qcom-legacy
> driver:
>
> The upstream-based product we have today do ship Devicetree in
> combination with the kernel, so they would upgrade both together and get
> the new driver.
>
> The other group would be kernel developers, enthusiasts, specific users
> who for some reason is upgrading their kernel but not their Devicetree.
> These users will want the new features and stability we're bringing.
>
> > Code can break, and that's not unexpected. If 2 LTS releases later and
> > we remove the dwc3-qcom-legacy, things can break then too. This may just
> > as be painful if we need fixes to the legacy driver due to some previous
> > regression. Also, I'm sure your team did a fair share of testing the new
> > driver right? Is there some major concern in the new driver that we
> > haven't addressed?
> >
>
> The new and old drivers are mostly identical at this point, and expected
> to diverge from here.
>
> The one thing I have identified to differ is that the "legacy" driver
> supports 2 extcon handles in the glue, but this is not considered
> acceptable by the binding so I haven't found anyone actually exercising
> this code path - then again extcon and usb_role_switch is one of the
> things this enables us to clean up.
>
>
> That said, while this model seems suitable for Qualcomm, due to the
> current state of things, I don't know if the same is true for Frank Li,
> perhaps NXP has a broader user base and need the migration logic.
So far, we use only one extcon.
Frank
>
> > >
> > > In my view, setting a flag date when the dwc3-qcom-legacy.c will be
> > > removed will provide upstream users a transition period, at a very low
> > > additional cost (934 lines of already tested code). If someone
> > > downstream after that flag date wants to retain support for qcom,dwc3
> > > they can just revert the removal of dwc3-qcom-legacy.c.
> >
> > The same can be said that they can revert the update (or apply fixes)
> > should they found issue with the new change.
> >
>
> We're changing the Devicetree binding, which gives us two problems:
> 1) Devicetree source code and DWC3 driver code are merged through
> different trees.
>
> 2) The compiled Devicetree (.dtb) and kernel image are in some cases
> separate software deliverables.
>
> So we absolutely need some migration mechanism to not just break USB for
> everyone for the coming 1-2 releases - at least.
>
> That said, the "2 LTS" is completely arbitrary. If you prefer to limit
> that, we can certainly have that discussion! E.g. I wouldn't argue
> against setting the flag-date by the end of this year.
>
> > >
> > > The alternative is that I try to get the migration code suggested in v3
> > > to a state where it can be merged (right now it's 6x larger) and then
> > > keep investing indefinitely in making sure it's not bit-rotting
> > > (although Rob Herring did request a flag date of the migration code in
> > > v3 as well...).
> > >
> >
> > All that said, if you believe that this transition will be quite
> > disruptive without preserving the legacy driver/dts, then we will do so.
> >
>
> We absolutely need a transition period, per above reasons. The length of
> it is an open question.
>
> > Can I request that you make this snapshot as one of the first patches in
> > the series so reverts/git-blames can easily be traced?
> >
>
> Absolutely.
>
> > BR,
> > Thinh
> >
> > Side question: for Snapdragon laptops, without the corresponding kernel
> > and DTS updates, don't things break easily?
>
> It certainly happens, but maintaining backwards compatibility is
> something we're striving for. As the Devicetree bindings mature, the
> easier this is though.
>
> One example where this is a problem will be clear here, that users
> attempting to boot today's kernel with tomorrows Devicetree blobs will
> not get USB - because today's kernel doesn't know how to make of the
> information in that description.
>
> This is true for any hardware or firmware interface though, so there's
> only so much one can do about that (and whatever that is, we're trying
> to do - for the sake of user friendliness).
>
> Regards,
> Bjorn
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-06 22:49 ` Bjorn Andersson
2025-03-07 16:17 ` Frank Li
@ 2025-03-07 23:00 ` Thinh Nguyen
2025-03-07 23:16 ` Thinh Nguyen
2025-03-11 3:06 ` Bjorn Andersson
1 sibling, 2 replies; 30+ messages in thread
From: Thinh Nguyen @ 2025-03-07 23:00 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Thinh Nguyen, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Thu, Mar 06, 2025, Bjorn Andersson wrote:
> On Wed, Mar 05, 2025 at 12:31:49AM +0000, Thinh Nguyen wrote:
> > On Mon, Mar 03, 2025, Bjorn Andersson wrote:
> > > On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> > > > On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > > > > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > > > > In order to more tightly integrate the Qualcomm glue driver with the
> > > > > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > > > > using the driver model. But due to the strong coupling to the Devicetree
> > > > > > binding needs to be updated as well.
> > > > > >
> > > > > > Various ways to provide backwards compatibility with existing Devicetree
> > > > > > blobs has been explored, but migrating the Devicetree information
> > > > > > between the old and the new binding is non-trivial.
> > > > > >
> > > > > > For the vast majority of boards out there, the kernel and Devicetree are
> > > > > > generated and handled together, which in practice means that backwards
> > > > > > compatibility needs to be managed across about 1 kernel release.
> > > > > >
> > > > > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > > > > blobs live a life separate of the kernel. In each one of these, with the
> > > > > > continued extension of new features, it's recommended that users would
> > > > > > upgrade their Devicetree somewhat frequently.
> > > > > >
> > > > > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > > > > is simpler than creating and maintaining the migration code.
> > > > > >
> > > > > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > > > > distributions doesn't drop USB support on these platforms.
> > > > > >
> > > > > > The driver, which is going to be refactored to handle the newly
> > > > > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > > > > match against any compatible.
> > > > > >
> > > > > > This driver should be removed after 2 LTS releases.
> > > > > >
> > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > > > > ---
> > > > > > drivers/usb/dwc3/Makefile | 1 +
> > > > > > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > > > > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > > > > > 3 files changed, 935 insertions(+), 1 deletion(-)
> > > > > >
> > > > >
> > > > > This is a bit concerning if there's no matching compatible string. ie.
> > > > > we don't have user for the new driver without downstream dependencies
> > > > > (or some workaround in the driver binding).
> > > >
> > > > Ignore the comment above, I missed the "temporarily" in your log
> > > > above. However, the comment below still stands.
> > > >
> > > > >
> > > > > While I understand the intention, I'm afraid we may have to support and
> > > > > maintain this much longer than the proposed 2 LTS releases (as seen with
> > > > > anything tagged with "legacy" in the upstream kernel).
> > >
> > > There are no products shipping today using dwc3-qcom where Devicetree is
> > > considered firmware. The primary audience for a longer transition is
> > > users of the various laptops with Qualcomm-chip in them. But given the
> > > rapid development in a variety of functional areas, these users will be
> > > highly compelled to update their DTBs within 2 years.
> > >
> > > The other obvious user group is to make sure us upstream developers
> > > don't loose USB when things get out of sync.
> > >
> > >
> > > That said, if the model defined here is to be followed in other cases
> > > (or my other vendors) where Devicetree is treated as firmware, your
> > > concerns are valid - and it might be worth taking the cost of managing
> > > the live-migration code.
> > >
> > > > > If possible, I'd
> > > > > prefer the complications of maintenance of the migration code be handled
> > > > > downstream.
> > > > >
> > >
> > > I'm sorry, but here it sounds like you're saying that you don't want any
> > > migration code upstream at all? This is not possible, as this will break
> > > USB for developers and users short term. We can of course discuss the 2
> > > LTS though, if you want a shorter life span for this migration.
> > >
> >
> > My first concern is now we have a legacy driver that should not be
> > continued to be developed while we also need to address any
> > regression/fixes found in the future from the legacy driver. While I
> > would encourage users to start migrating to the new driver, I won't
> > reject fixes to the legacy driver either. In the next 2 years+, my
> > other concern is that I'm not confident that we can easily remove the
> > legacy driver and the DTS then.
> >
>
> The problem at hand is that the driver _needs_ a bunch of work.
> Role-switching only works sometimes, extcon is (for older platforms)
> duplicated in both glue and core - with the hope that each part does its
> thing in a suitable fashion, the layering violations can trigger
> NULL-pointer dereferences or use-after-free, PM runtime is marked
> forbidden...
>
> We've looked at these problems for a few years now, without coming up
> with any solution to address these issues within the current design.
That's understood, and that's the incentive for your work here.
>
> Following this refactor, we will be able to work on these improvements.
> For this to happen, I intend to transition all the
> arch/*/boot/dts/qcom/* platforms to the new binding as soon as possible.
>
>
> Looking ahead, when we hit the point of deprecating the dwc3-qcom-legacy
> driver:
>
> The upstream-based product we have today do ship Devicetree in
> combination with the kernel, so they would upgrade both together and get
> the new driver.
>
> The other group would be kernel developers, enthusiasts, specific users
> who for some reason is upgrading their kernel but not their Devicetree.
> These users will want the new features and stability we're bringing.
>
> > Code can break, and that's not unexpected. If 2 LTS releases later and
> > we remove the dwc3-qcom-legacy, things can break then too. This may just
> > as be painful if we need fixes to the legacy driver due to some previous
> > regression. Also, I'm sure your team did a fair share of testing the new
> > driver right? Is there some major concern in the new driver that we
> > haven't addressed?
> >
>
> The new and old drivers are mostly identical at this point, and expected
This was my expectation, that the new and old drivers are mostly
identical at this point. This is one of the reasons why I suggested to
directly switch to using the new driver at this point.
> to diverge from here.
This is what I want to avoid.
>
> The one thing I have identified to differ is that the "legacy" driver
> supports 2 extcon handles in the glue, but this is not considered
> acceptable by the binding so I haven't found anyone actually exercising
> this code path - then again extcon and usb_role_switch is one of the
> things this enables us to clean up.
>
>
> That said, while this model seems suitable for Qualcomm, due to the
> current state of things, I don't know if the same is true for Frank Li,
> perhaps NXP has a broader user base and need the migration logic.
>
This was not expected. If the new driver doesn't support certain devices
with extcon, how can we expect to remove/deprecate the legacy driver
without dropping support to these devices.
> > >
> > > In my view, setting a flag date when the dwc3-qcom-legacy.c will be
> > > removed will provide upstream users a transition period, at a very low
> > > additional cost (934 lines of already tested code). If someone
> > > downstream after that flag date wants to retain support for qcom,dwc3
> > > they can just revert the removal of dwc3-qcom-legacy.c.
> >
> > The same can be said that they can revert the update (or apply fixes)
> > should they found issue with the new change.
> >
>
> We're changing the Devicetree binding, which gives us two problems:
> 1) Devicetree source code and DWC3 driver code are merged through
> different trees.
>
> 2) The compiled Devicetree (.dtb) and kernel image are in some cases
> separate software deliverables.
>
> So we absolutely need some migration mechanism to not just break USB for
> everyone for the coming 1-2 releases - at least.
>
> That said, the "2 LTS" is completely arbitrary. If you prefer to limit
> that, we can certainly have that discussion! E.g. I wouldn't argue
> against setting the flag-date by the end of this year.
>
I don't know enough about the timeline so suggest a different number.
> > >
> > > The alternative is that I try to get the migration code suggested in v3
> > > to a state where it can be merged (right now it's 6x larger) and then
> > > keep investing indefinitely in making sure it's not bit-rotting
> > > (although Rob Herring did request a flag date of the migration code in
> > > v3 as well...).
> > >
> >
> > All that said, if you believe that this transition will be quite
> > disruptive without preserving the legacy driver/dts, then we will do so.
> >
>
> We absolutely need a transition period, per above reasons. The length of
> it is an open question.
Ok, but before we merge the new driver, do we have any plan to support
devices that use extcon in the new driver?
(Apologies if I had missed this discussion prior.)
>
> > Can I request that you make this snapshot as one of the first patches in
> > the series so reverts/git-blames can easily be traced?
> >
>
> Absolutely.
Thanks.
>
> > BR,
> > Thinh
> >
> > Side question: for Snapdragon laptops, without the corresponding kernel
> > and DTS updates, don't things break easily?
>
> It certainly happens, but maintaining backwards compatibility is
> something we're striving for. As the Devicetree bindings mature, the
> easier this is though.
>
> One example where this is a problem will be clear here, that users
> attempting to boot today's kernel with tomorrows Devicetree blobs will
> not get USB - because today's kernel doesn't know how to make of the
> information in that description.
>
> This is true for any hardware or firmware interface though, so there's
> only so much one can do about that (and whatever that is, we're trying
> to do - for the sake of user friendliness).
>
Right, for those that migrate the DTS and kernel separately, breakage
should not come as a surprise. As you said, there's only so much we can
do. I would expect for them to also have certain protocol to handle this
when it happens.
BR,
Thinh
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-07 23:00 ` Thinh Nguyen
@ 2025-03-07 23:16 ` Thinh Nguyen
2025-03-11 3:06 ` Bjorn Andersson
1 sibling, 0 replies; 30+ messages in thread
From: Thinh Nguyen @ 2025-03-07 23:16 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Fri, Mar 07, 2025, Thinh Nguyen wrote:
> On Thu, Mar 06, 2025, Bjorn Andersson wrote:
> >
> > That said, the "2 LTS" is completely arbitrary. If you prefer to limit
> > that, we can certainly have that discussion! E.g. I wouldn't argue
> > against setting the flag-date by the end of this year.
> >
>
> I don't know enough about the timeline so suggest a different number.
>
Sorry fix typo:
I don't know enough about the timeline to* suggest a different number.
Thinh
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 7/7] arm64: dts: qcom: sc8280x: Flatten the USB nodes
2025-02-27 0:17 ` [PATCH v4 7/7] arm64: dts: qcom: sc8280x: Flatten the USB nodes Bjorn Andersson
@ 2025-03-08 17:58 ` Konrad Dybcio
0 siblings, 0 replies; 30+ messages in thread
From: Konrad Dybcio @ 2025-03-08 17:58 UTC (permalink / raw)
To: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Thinh Nguyen, Philipp Zabel, Bjorn Andersson,
Konrad Dybcio, Frank Li
Cc: linux-arm-msm, linux-usb, devicetree, linux-kernel
On 27.02.2025 1:17 AM, Bjorn Andersson wrote:
> Transition the three USB controllers found in sc8280xp to the newly
> introduced, flattened representation of the Qualcomm USB block, i.e.
> qcom,snps-dwc3, to show the end result.
>
> The reg and interrupts properties from the usb child node are merged
> with their counterpart in the outer node, remaining properties and child
> nodes are simply moved.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Konrad
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model
2025-03-07 6:41 ` Dmitry Baryshkov
@ 2025-03-11 2:46 ` Bjorn Andersson
2025-03-11 19:02 ` Dmitry Baryshkov
0 siblings, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-03-11 2:46 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Thinh Nguyen, Philipp Zabel, Konrad Dybcio,
Frank Li, linux-arm-msm, linux-usb, devicetree, linux-kernel
On Fri, Mar 07, 2025 at 08:41:33AM +0200, Dmitry Baryshkov wrote:
> On Wed, Feb 26, 2025 at 04:17:53PM -0800, 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@oss.qualcomm.com>
> > ---
> > drivers/usb/dwc3/dwc3-qcom.c | 138 +++++++++++++++++++++----------------------
> > 1 file changed, 69 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 9d04c2457433..63e60f15ceaa 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>
>
> As a heads up, would it make sense to also drop extcon support while we
> are transitioning to the new driver / DT bindings?
>
Yes, I believe this code should be cleaned out in favor of relying on
the core's implementation thereof and callbacks into the qcom-code for
configuration the glue hardware - which looks to be the same callback we
want to add for the usb_role_switch.
Regards,
Bjorn
> > #include <linux/interconnect.h>
> > -#include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > #include <linux/phy/phy.h>
> > #include <linux/usb/of.h>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-07 23:00 ` Thinh Nguyen
2025-03-07 23:16 ` Thinh Nguyen
@ 2025-03-11 3:06 ` Bjorn Andersson
2025-03-11 23:59 ` Thinh Nguyen
1 sibling, 1 reply; 30+ messages in thread
From: Bjorn Andersson @ 2025-03-11 3:06 UTC (permalink / raw)
To: Thinh Nguyen
Cc: Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Fri, Mar 07, 2025 at 11:00:32PM +0000, Thinh Nguyen wrote:
> On Thu, Mar 06, 2025, Bjorn Andersson wrote:
> > On Wed, Mar 05, 2025 at 12:31:49AM +0000, Thinh Nguyen wrote:
> > > On Mon, Mar 03, 2025, Bjorn Andersson wrote:
> > > > On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> > > > > On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > > > > > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > > > > > In order to more tightly integrate the Qualcomm glue driver with the
> > > > > > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > > > > > using the driver model. But due to the strong coupling to the Devicetree
> > > > > > > binding needs to be updated as well.
> > > > > > >
> > > > > > > Various ways to provide backwards compatibility with existing Devicetree
> > > > > > > blobs has been explored, but migrating the Devicetree information
> > > > > > > between the old and the new binding is non-trivial.
> > > > > > >
> > > > > > > For the vast majority of boards out there, the kernel and Devicetree are
> > > > > > > generated and handled together, which in practice means that backwards
> > > > > > > compatibility needs to be managed across about 1 kernel release.
> > > > > > >
> > > > > > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > > > > > blobs live a life separate of the kernel. In each one of these, with the
> > > > > > > continued extension of new features, it's recommended that users would
> > > > > > > upgrade their Devicetree somewhat frequently.
> > > > > > >
> > > > > > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > > > > > is simpler than creating and maintaining the migration code.
> > > > > > >
> > > > > > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > > > > > distributions doesn't drop USB support on these platforms.
> > > > > > >
> > > > > > > The driver, which is going to be refactored to handle the newly
> > > > > > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > > > > > match against any compatible.
> > > > > > >
> > > > > > > This driver should be removed after 2 LTS releases.
> > > > > > >
> > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > > > > > ---
> > > > > > > drivers/usb/dwc3/Makefile | 1 +
> > > > > > > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > > > > > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > > > > > > 3 files changed, 935 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > >
> > > > > > This is a bit concerning if there's no matching compatible string. ie.
> > > > > > we don't have user for the new driver without downstream dependencies
> > > > > > (or some workaround in the driver binding).
> > > > >
> > > > > Ignore the comment above, I missed the "temporarily" in your log
> > > > > above. However, the comment below still stands.
> > > > >
> > > > > >
> > > > > > While I understand the intention, I'm afraid we may have to support and
> > > > > > maintain this much longer than the proposed 2 LTS releases (as seen with
> > > > > > anything tagged with "legacy" in the upstream kernel).
> > > >
> > > > There are no products shipping today using dwc3-qcom where Devicetree is
> > > > considered firmware. The primary audience for a longer transition is
> > > > users of the various laptops with Qualcomm-chip in them. But given the
> > > > rapid development in a variety of functional areas, these users will be
> > > > highly compelled to update their DTBs within 2 years.
> > > >
> > > > The other obvious user group is to make sure us upstream developers
> > > > don't loose USB when things get out of sync.
> > > >
> > > >
> > > > That said, if the model defined here is to be followed in other cases
> > > > (or my other vendors) where Devicetree is treated as firmware, your
> > > > concerns are valid - and it might be worth taking the cost of managing
> > > > the live-migration code.
> > > >
> > > > > > If possible, I'd
> > > > > > prefer the complications of maintenance of the migration code be handled
> > > > > > downstream.
> > > > > >
> > > >
> > > > I'm sorry, but here it sounds like you're saying that you don't want any
> > > > migration code upstream at all? This is not possible, as this will break
> > > > USB for developers and users short term. We can of course discuss the 2
> > > > LTS though, if you want a shorter life span for this migration.
> > > >
> > >
> > > My first concern is now we have a legacy driver that should not be
> > > continued to be developed while we also need to address any
> > > regression/fixes found in the future from the legacy driver. While I
> > > would encourage users to start migrating to the new driver, I won't
> > > reject fixes to the legacy driver either. In the next 2 years+, my
> > > other concern is that I'm not confident that we can easily remove the
> > > legacy driver and the DTS then.
> > >
> >
> > The problem at hand is that the driver _needs_ a bunch of work.
> > Role-switching only works sometimes, extcon is (for older platforms)
> > duplicated in both glue and core - with the hope that each part does its
> > thing in a suitable fashion, the layering violations can trigger
> > NULL-pointer dereferences or use-after-free, PM runtime is marked
> > forbidden...
> >
> > We've looked at these problems for a few years now, without coming up
> > with any solution to address these issues within the current design.
>
> That's understood, and that's the incentive for your work here.
>
> >
> > Following this refactor, we will be able to work on these improvements.
> > For this to happen, I intend to transition all the
> > arch/*/boot/dts/qcom/* platforms to the new binding as soon as possible.
> >
> >
> > Looking ahead, when we hit the point of deprecating the dwc3-qcom-legacy
> > driver:
> >
> > The upstream-based product we have today do ship Devicetree in
> > combination with the kernel, so they would upgrade both together and get
> > the new driver.
> >
> > The other group would be kernel developers, enthusiasts, specific users
> > who for some reason is upgrading their kernel but not their Devicetree.
> > These users will want the new features and stability we're bringing.
> >
> > > Code can break, and that's not unexpected. If 2 LTS releases later and
> > > we remove the dwc3-qcom-legacy, things can break then too. This may just
> > > as be painful if we need fixes to the legacy driver due to some previous
> > > regression. Also, I'm sure your team did a fair share of testing the new
> > > driver right? Is there some major concern in the new driver that we
> > > haven't addressed?
> > >
> >
> > The new and old drivers are mostly identical at this point, and expected
>
> This was my expectation, that the new and old drivers are mostly
> identical at this point. This is one of the reasons why I suggested to
> directly switch to using the new driver at this point.
>
I do suggest that we directly switch all targets to use the new
implementation.
But that switch happens in Devicetree blobs (.dtb files) that are:
1) Entering the upstream kernel through a different maintainer
2) In some cases managed as through a separate software delivery
mechanism - so it's possible that users run v6.15 with a DTB from v6.13.
> > to diverge from here.
>
> This is what I want to avoid.
>
Understood.
> >
> > The one thing I have identified to differ is that the "legacy" driver
> > supports 2 extcon handles in the glue, but this is not considered
> > acceptable by the binding so I haven't found anyone actually exercising
> > this code path - then again extcon and usb_role_switch is one of the
> > things this enables us to clean up.
> >
> >
> > That said, while this model seems suitable for Qualcomm, due to the
> > current state of things, I don't know if the same is true for Frank Li,
> > perhaps NXP has a broader user base and need the migration logic.
> >
>
> This was not expected. If the new driver doesn't support certain devices
> with extcon, how can we expect to remove/deprecate the legacy driver
> without dropping support to these devices.
>
The opposite will be the case. The new driver will support extcon and
usb_role_switch, while the legacy one can't support usb_role_switch. The
layering violations can't be resolved in the old driver, so the issues
stemming from this can't be fixed.
> > > >
> > > > In my view, setting a flag date when the dwc3-qcom-legacy.c will be
> > > > removed will provide upstream users a transition period, at a very low
> > > > additional cost (934 lines of already tested code). If someone
> > > > downstream after that flag date wants to retain support for qcom,dwc3
> > > > they can just revert the removal of dwc3-qcom-legacy.c.
> > >
> > > The same can be said that they can revert the update (or apply fixes)
> > > should they found issue with the new change.
> > >
> >
> > We're changing the Devicetree binding, which gives us two problems:
> > 1) Devicetree source code and DWC3 driver code are merged through
> > different trees.
> >
> > 2) The compiled Devicetree (.dtb) and kernel image are in some cases
> > separate software deliverables.
> >
> > So we absolutely need some migration mechanism to not just break USB for
> > everyone for the coming 1-2 releases - at least.
> >
> > That said, the "2 LTS" is completely arbitrary. If you prefer to limit
> > that, we can certainly have that discussion! E.g. I wouldn't argue
> > against setting the flag-date by the end of this year.
> >
>
> I don't know enough about the timeline so suggest a different number.
>
If we pick this up soon, and then take one release to convert all the
Devicetree source, we should be ready to drop the legacy implementation
after next LTS, i.e. by the end of this year.
> > > >
> > > > The alternative is that I try to get the migration code suggested in v3
> > > > to a state where it can be merged (right now it's 6x larger) and then
> > > > keep investing indefinitely in making sure it's not bit-rotting
> > > > (although Rob Herring did request a flag date of the migration code in
> > > > v3 as well...).
> > > >
> > >
> > > All that said, if you believe that this transition will be quite
> > > disruptive without preserving the legacy driver/dts, then we will do so.
> > >
> >
> > We absolutely need a transition period, per above reasons. The length of
> > it is an open question.
>
> Ok, but before we merge the new driver, do we have any plan to support
> devices that use extcon in the new driver?
>
> (Apologies if I had missed this discussion prior.)
>
Yes, my intention is to transition all devices to the new binding asap -
including pre-type-C ones that uses extcon.
> >
> > > Can I request that you make this snapshot as one of the first patches in
> > > the series so reverts/git-blames can easily be traced?
> > >
> >
> > Absolutely.
>
> Thanks.
>
> >
> > > BR,
> > > Thinh
> > >
> > > Side question: for Snapdragon laptops, without the corresponding kernel
> > > and DTS updates, don't things break easily?
> >
> > It certainly happens, but maintaining backwards compatibility is
> > something we're striving for. As the Devicetree bindings mature, the
> > easier this is though.
> >
> > One example where this is a problem will be clear here, that users
> > attempting to boot today's kernel with tomorrows Devicetree blobs will
> > not get USB - because today's kernel doesn't know how to make of the
> > information in that description.
> >
> > This is true for any hardware or firmware interface though, so there's
> > only so much one can do about that (and whatever that is, we're trying
> > to do - for the sake of user friendliness).
> >
>
> Right, for those that migrate the DTS and kernel separately, breakage
> should not come as a surprise. As you said, there's only so much we can
> do. I would expect for them to also have certain protocol to handle this
> when it happens.
>
No, there isn't really a protocol, given that in the Devicetree
community we give a promise to the users that such breakage shouldn't
happen. (In the same way as you shouldn't have to update BIOS on your PC
to boot a new version of Linux)
That said, I'm arguing in favor of such breakage, as the problem is
limited (and we give a little bit of grace period) and the fact that
the current implementation is a dead end.
Regards,
Bjorn
> BR,
> Thinh
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model
2025-03-11 2:46 ` Bjorn Andersson
@ 2025-03-11 19:02 ` Dmitry Baryshkov
2025-03-18 19:12 ` Bjorn Andersson
0 siblings, 1 reply; 30+ messages in thread
From: Dmitry Baryshkov @ 2025-03-11 19:02 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Dmitry Baryshkov, Bjorn Andersson, Greg Kroah-Hartman,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Felipe Balbi,
Wesley Cheng, Saravana Kannan, Thinh Nguyen, Philipp Zabel,
Konrad Dybcio, Frank Li, linux-arm-msm, linux-usb, devicetree,
linux-kernel
On Mon, Mar 10, 2025 at 09:46:24PM -0500, Bjorn Andersson wrote:
> On Fri, Mar 07, 2025 at 08:41:33AM +0200, Dmitry Baryshkov wrote:
> > On Wed, Feb 26, 2025 at 04:17:53PM -0800, 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@oss.qualcomm.com>
> > > ---
> > > drivers/usb/dwc3/dwc3-qcom.c | 138 +++++++++++++++++++++----------------------
> > > 1 file changed, 69 insertions(+), 69 deletions(-)
> > >
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index 9d04c2457433..63e60f15ceaa 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>
> >
> > As a heads up, would it make sense to also drop extcon support while we
> > are transitioning to the new driver / DT bindings?
> >
>
> Yes, I believe this code should be cleaned out in favor of relying on
> the core's implementation thereof and callbacks into the qcom-code for
> configuration the glue hardware - which looks to be the same callback we
> want to add for the usb_role_switch.
Hmm. So do you want to clean it later? Or do you plan to drop extcon
from the next iteration?
>
> Regards,
> Bjorn
>
> > > #include <linux/interconnect.h>
> > > -#include <linux/of_platform.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/phy/phy.h>
> > > #include <linux/usb/of.h>
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty
2025-03-11 3:06 ` Bjorn Andersson
@ 2025-03-11 23:59 ` Thinh Nguyen
0 siblings, 0 replies; 30+ messages in thread
From: Thinh Nguyen @ 2025-03-11 23:59 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Thinh Nguyen, Bjorn Andersson, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Felipe Balbi, Wesley Cheng,
Saravana Kannan, Philipp Zabel, Konrad Dybcio, Frank Li,
linux-arm-msm@vger.kernel.org, linux-usb@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Mon, Mar 10, 2025, Bjorn Andersson wrote:
> On Fri, Mar 07, 2025 at 11:00:32PM +0000, Thinh Nguyen wrote:
> > On Thu, Mar 06, 2025, Bjorn Andersson wrote:
> > > On Wed, Mar 05, 2025 at 12:31:49AM +0000, Thinh Nguyen wrote:
> > > > On Mon, Mar 03, 2025, Bjorn Andersson wrote:
> > > > > On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> > > > > > On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > > > > > > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > > > > > > In order to more tightly integrate the Qualcomm glue driver with the
> > > > > > > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > > > > > > using the driver model. But due to the strong coupling to the Devicetree
> > > > > > > > binding needs to be updated as well.
> > > > > > > >
> > > > > > > > Various ways to provide backwards compatibility with existing Devicetree
> > > > > > > > blobs has been explored, but migrating the Devicetree information
> > > > > > > > between the old and the new binding is non-trivial.
> > > > > > > >
> > > > > > > > For the vast majority of boards out there, the kernel and Devicetree are
> > > > > > > > generated and handled together, which in practice means that backwards
> > > > > > > > compatibility needs to be managed across about 1 kernel release.
> > > > > > > >
> > > > > > > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > > > > > > blobs live a life separate of the kernel. In each one of these, with the
> > > > > > > > continued extension of new features, it's recommended that users would
> > > > > > > > upgrade their Devicetree somewhat frequently.
> > > > > > > >
> > > > > > > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > > > > > > is simpler than creating and maintaining the migration code.
> > > > > > > >
> > > > > > > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > > > > > > distributions doesn't drop USB support on these platforms.
> > > > > > > >
> > > > > > > > The driver, which is going to be refactored to handle the newly
> > > > > > > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > > > > > > match against any compatible.
> > > > > > > >
> > > > > > > > This driver should be removed after 2 LTS releases.
> > > > > > > >
> > > > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@oss.qualcomm.com>
> > > > > > > > ---
> > > > > > > > drivers/usb/dwc3/Makefile | 1 +
> > > > > > > > drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > > > > > > drivers/usb/dwc3/dwc3-qcom.c | 1 -
> > > > > > > > 3 files changed, 935 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > >
> > > > > > > This is a bit concerning if there's no matching compatible string. ie.
> > > > > > > we don't have user for the new driver without downstream dependencies
> > > > > > > (or some workaround in the driver binding).
> > > > > >
> > > > > > Ignore the comment above, I missed the "temporarily" in your log
> > > > > > above. However, the comment below still stands.
> > > > > >
> > > > > > >
> > > > > > > While I understand the intention, I'm afraid we may have to support and
> > > > > > > maintain this much longer than the proposed 2 LTS releases (as seen with
> > > > > > > anything tagged with "legacy" in the upstream kernel).
> > > > >
> > > > > There are no products shipping today using dwc3-qcom where Devicetree is
> > > > > considered firmware. The primary audience for a longer transition is
> > > > > users of the various laptops with Qualcomm-chip in them. But given the
> > > > > rapid development in a variety of functional areas, these users will be
> > > > > highly compelled to update their DTBs within 2 years.
> > > > >
> > > > > The other obvious user group is to make sure us upstream developers
> > > > > don't loose USB when things get out of sync.
> > > > >
> > > > >
> > > > > That said, if the model defined here is to be followed in other cases
> > > > > (or my other vendors) where Devicetree is treated as firmware, your
> > > > > concerns are valid - and it might be worth taking the cost of managing
> > > > > the live-migration code.
> > > > >
> > > > > > > If possible, I'd
> > > > > > > prefer the complications of maintenance of the migration code be handled
> > > > > > > downstream.
> > > > > > >
> > > > >
> > > > > I'm sorry, but here it sounds like you're saying that you don't want any
> > > > > migration code upstream at all? This is not possible, as this will break
> > > > > USB for developers and users short term. We can of course discuss the 2
> > > > > LTS though, if you want a shorter life span for this migration.
> > > > >
> > > >
> > > > My first concern is now we have a legacy driver that should not be
> > > > continued to be developed while we also need to address any
> > > > regression/fixes found in the future from the legacy driver. While I
> > > > would encourage users to start migrating to the new driver, I won't
> > > > reject fixes to the legacy driver either. In the next 2 years+, my
> > > > other concern is that I'm not confident that we can easily remove the
> > > > legacy driver and the DTS then.
> > > >
> > >
> > > The problem at hand is that the driver _needs_ a bunch of work.
> > > Role-switching only works sometimes, extcon is (for older platforms)
> > > duplicated in both glue and core - with the hope that each part does its
> > > thing in a suitable fashion, the layering violations can trigger
> > > NULL-pointer dereferences or use-after-free, PM runtime is marked
> > > forbidden...
> > >
> > > We've looked at these problems for a few years now, without coming up
> > > with any solution to address these issues within the current design.
> >
> > That's understood, and that's the incentive for your work here.
> >
> > >
> > > Following this refactor, we will be able to work on these improvements.
> > > For this to happen, I intend to transition all the
> > > arch/*/boot/dts/qcom/* platforms to the new binding as soon as possible.
> > >
> > >
> > > Looking ahead, when we hit the point of deprecating the dwc3-qcom-legacy
> > > driver:
> > >
> > > The upstream-based product we have today do ship Devicetree in
> > > combination with the kernel, so they would upgrade both together and get
> > > the new driver.
> > >
> > > The other group would be kernel developers, enthusiasts, specific users
> > > who for some reason is upgrading their kernel but not their Devicetree.
> > > These users will want the new features and stability we're bringing.
> > >
> > > > Code can break, and that's not unexpected. If 2 LTS releases later and
> > > > we remove the dwc3-qcom-legacy, things can break then too. This may just
> > > > as be painful if we need fixes to the legacy driver due to some previous
> > > > regression. Also, I'm sure your team did a fair share of testing the new
> > > > driver right? Is there some major concern in the new driver that we
> > > > haven't addressed?
> > > >
> > >
> > > The new and old drivers are mostly identical at this point, and expected
> >
> > This was my expectation, that the new and old drivers are mostly
> > identical at this point. This is one of the reasons why I suggested to
> > directly switch to using the new driver at this point.
> >
>
> I do suggest that we directly switch all targets to use the new
> implementation.
>
> But that switch happens in Devicetree blobs (.dtb files) that are:
> 1) Entering the upstream kernel through a different maintainer
> 2) In some cases managed as through a separate software delivery
> mechanism - so it's possible that users run v6.15 with a DTB from v6.13.
>
> > > to diverge from here.
> >
> > This is what I want to avoid.
> >
>
> Understood.
>
> > >
> > > The one thing I have identified to differ is that the "legacy" driver
> > > supports 2 extcon handles in the glue, but this is not considered
> > > acceptable by the binding so I haven't found anyone actually exercising
> > > this code path - then again extcon and usb_role_switch is one of the
> > > things this enables us to clean up.
> > >
> > >
> > > That said, while this model seems suitable for Qualcomm, due to the
> > > current state of things, I don't know if the same is true for Frank Li,
> > > perhaps NXP has a broader user base and need the migration logic.
> > >
> >
> > This was not expected. If the new driver doesn't support certain devices
> > with extcon, how can we expect to remove/deprecate the legacy driver
> > without dropping support to these devices.
> >
>
> The opposite will be the case. The new driver will support extcon and
> usb_role_switch, while the legacy one can't support usb_role_switch. The
> layering violations can't be resolved in the old driver, so the issues
> stemming from this can't be fixed.
Ah.. then this should be fine.
>
> > > > >
> > > > > In my view, setting a flag date when the dwc3-qcom-legacy.c will be
> > > > > removed will provide upstream users a transition period, at a very low
> > > > > additional cost (934 lines of already tested code). If someone
> > > > > downstream after that flag date wants to retain support for qcom,dwc3
> > > > > they can just revert the removal of dwc3-qcom-legacy.c.
> > > >
> > > > The same can be said that they can revert the update (or apply fixes)
> > > > should they found issue with the new change.
> > > >
> > >
> > > We're changing the Devicetree binding, which gives us two problems:
> > > 1) Devicetree source code and DWC3 driver code are merged through
> > > different trees.
> > >
> > > 2) The compiled Devicetree (.dtb) and kernel image are in some cases
> > > separate software deliverables.
> > >
> > > So we absolutely need some migration mechanism to not just break USB for
> > > everyone for the coming 1-2 releases - at least.
> > >
> > > That said, the "2 LTS" is completely arbitrary. If you prefer to limit
> > > that, we can certainly have that discussion! E.g. I wouldn't argue
> > > against setting the flag-date by the end of this year.
> > >
> >
> > I don't know enough about the timeline so suggest a different number.
> >
>
> If we pick this up soon, and then take one release to convert all the
> Devicetree source, we should be ready to drop the legacy implementation
> after next LTS, i.e. by the end of this year.
Ok.
>
> > > > >
> > > > > The alternative is that I try to get the migration code suggested in v3
> > > > > to a state where it can be merged (right now it's 6x larger) and then
> > > > > keep investing indefinitely in making sure it's not bit-rotting
> > > > > (although Rob Herring did request a flag date of the migration code in
> > > > > v3 as well...).
> > > > >
> > > >
> > > > All that said, if you believe that this transition will be quite
> > > > disruptive without preserving the legacy driver/dts, then we will do so.
> > > >
> > >
> > > We absolutely need a transition period, per above reasons. The length of
> > > it is an open question.
> >
> > Ok, but before we merge the new driver, do we have any plan to support
> > devices that use extcon in the new driver?
> >
> > (Apologies if I had missed this discussion prior.)
> >
>
> Yes, my intention is to transition all devices to the new binding asap -
> including pre-type-C ones that uses extcon.
Ok.
>
> > >
> > > > Can I request that you make this snapshot as one of the first patches in
> > > > the series so reverts/git-blames can easily be traced?
> > > >
> > >
> > > Absolutely.
> >
> > Thanks.
> >
> > >
> > > > BR,
> > > > Thinh
> > > >
> > > > Side question: for Snapdragon laptops, without the corresponding kernel
> > > > and DTS updates, don't things break easily?
> > >
> > > It certainly happens, but maintaining backwards compatibility is
> > > something we're striving for. As the Devicetree bindings mature, the
> > > easier this is though.
> > >
> > > One example where this is a problem will be clear here, that users
> > > attempting to boot today's kernel with tomorrows Devicetree blobs will
> > > not get USB - because today's kernel doesn't know how to make of the
> > > information in that description.
> > >
> > > This is true for any hardware or firmware interface though, so there's
> > > only so much one can do about that (and whatever that is, we're trying
> > > to do - for the sake of user friendliness).
> > >
> >
> > Right, for those that migrate the DTS and kernel separately, breakage
> > should not come as a surprise. As you said, there's only so much we can
> > do. I would expect for them to also have certain protocol to handle this
> > when it happens.
> >
>
> No, there isn't really a protocol, given that in the Devicetree
> community we give a promise to the users that such breakage shouldn't
> happen. (In the same way as you shouldn't have to update BIOS on your PC
> to boot a new version of Linux)
I don't think that's a fair comparison. It's equivalent to comparing
uboot to Linux. It's unusual for uboot to not use a corresponding DTS
for uboot. Regardless..
>
> That said, I'm arguing in favor of such breakage, as the problem is
> limited (and we give a little bit of grace period) and the fact that
> the current implementation is a dead end.
>
I really like your rework of the dwc3 here. We'll go with your
suggestion. I'll look forward to your next version.
Thanks,
Thinh
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model
2025-03-11 19:02 ` Dmitry Baryshkov
@ 2025-03-18 19:12 ` Bjorn Andersson
0 siblings, 0 replies; 30+ messages in thread
From: Bjorn Andersson @ 2025-03-18 19:12 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Dmitry Baryshkov, Bjorn Andersson, Greg Kroah-Hartman,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Felipe Balbi,
Wesley Cheng, Saravana Kannan, Thinh Nguyen, Philipp Zabel,
Konrad Dybcio, Frank Li, linux-arm-msm, linux-usb, devicetree,
linux-kernel
On Tue, Mar 11, 2025 at 09:02:03PM +0200, Dmitry Baryshkov wrote:
> On Mon, Mar 10, 2025 at 09:46:24PM -0500, Bjorn Andersson wrote:
> > On Fri, Mar 07, 2025 at 08:41:33AM +0200, Dmitry Baryshkov wrote:
> > > On Wed, Feb 26, 2025 at 04:17:53PM -0800, 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@oss.qualcomm.com>
> > > > ---
> > > > drivers/usb/dwc3/dwc3-qcom.c | 138 +++++++++++++++++++++----------------------
> > > > 1 file changed, 69 insertions(+), 69 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > > index 9d04c2457433..63e60f15ceaa 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>
> > >
> > > As a heads up, would it make sense to also drop extcon support while we
> > > are transitioning to the new driver / DT bindings?
> > >
> >
> > Yes, I believe this code should be cleaned out in favor of relying on
> > the core's implementation thereof and callbacks into the qcom-code for
> > configuration the glue hardware - which looks to be the same callback we
> > want to add for the usb_role_switch.
>
> Hmm. So do you want to clean it later? Or do you plan to drop extcon
> from the next iteration?
>
It turns out that this was cleaner to do as a separate commit, but as we
need a role-switch callback from core to glue (Krishna has a patch ready
to go for this) and I don't have any extcon hardware at my desk right
now I decided to leave a TODO in the code for now.
This needs to be resolved before we transition the extcon-based dts
files.
Regards,
Bjorn
> >
> > Regards,
> > Bjorn
> >
> > > > #include <linux/interconnect.h>
> > > > -#include <linux/of_platform.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/phy/phy.h>
> > > > #include <linux/usb/of.h>
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2025-03-18 19:12 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-27 0:17 [PATCH v4 0/7] usb: dwc3: qcom: Flatten dwc3 structure Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 1/7] dt-bindings: usb: Introduce qcom,snps-dwc3 Bjorn Andersson
2025-02-28 22:36 ` Rob Herring
2025-03-03 22:11 ` Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 2/7] usb: dwc3: core: Expose core driver as library Bjorn Andersson
2025-02-27 19:17 ` Frank Li
2025-02-27 0:17 ` [PATCH v4 3/7] usb: dwc3: core: Don't touch resets and clocks Bjorn Andersson
2025-02-27 4:24 ` Dmitry Baryshkov
2025-02-27 15:55 ` Bjorn Andersson
2025-02-27 16:07 ` Dmitry Baryshkov
2025-02-27 19:22 ` Frank Li
2025-02-27 0:17 ` [PATCH v4 4/7] usb: dwc3: qcom: Don't rely on drvdata during probe Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards compatibilty Bjorn Andersson
2025-03-04 0:05 ` Thinh Nguyen
2025-03-04 0:39 ` Thinh Nguyen
2025-03-04 3:13 ` Bjorn Andersson
2025-03-05 0:31 ` Thinh Nguyen
2025-03-06 22:49 ` Bjorn Andersson
2025-03-07 16:17 ` Frank Li
2025-03-07 23:00 ` Thinh Nguyen
2025-03-07 23:16 ` Thinh Nguyen
2025-03-11 3:06 ` Bjorn Andersson
2025-03-11 23:59 ` Thinh Nguyen
2025-02-27 0:17 ` [PATCH v4 6/7] usb: dwc3: qcom: Transition to flattened model Bjorn Andersson
2025-03-07 6:41 ` Dmitry Baryshkov
2025-03-11 2:46 ` Bjorn Andersson
2025-03-11 19:02 ` Dmitry Baryshkov
2025-03-18 19:12 ` Bjorn Andersson
2025-02-27 0:17 ` [PATCH v4 7/7] arm64: dts: qcom: sc8280x: Flatten the USB nodes Bjorn Andersson
2025-03-08 17:58 ` Konrad Dybcio
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).