mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v5 1/2] dt-bindings: usb: snps,dwc3: tighten the two-connector shared HS bus
@ 2026-11-02  5:41 Qianhui Xu
  2026-11-02  8:07 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 3+ messages in thread
From: Qianhui Xu @ 2026-11-02  5:41 UTC (permalink / raw)
  To: devicetree
  Cc: Rob Herring, Conor Dooley, Krzysztof Kozlowski,
	Greg Kroah-Hartman, linux-usb, linux-arm-msm, linux-kernel

Resending only 1/2 of this series.

The earlier post of this patch (Message-ID
<20261031150154.1106455-2-xiaqinghuan@mail.ustc.edu.cn>, 0/2 cover
<20261031150154.1106455-1-xiaqinghuan@mail.ustc.edu.cn>, 2026-10-31) used the
wrong device-tree list address: it was sent to
linux-devicetree@vger.kernel.org, which vger rejects with

  550 5.1.1 <linux-devicetree@vger.kernel.org>: Recipient address rejected:
  User unknown in virtual alias table

so it never reached devicetree@vger.kernel.org.  The patch text below is
unchanged, byte for byte, and is the same text as in the 0/2 cover.  The
sibling 2/2 went to linux-usb and linux-arm-msm and is archived there; this
resend is only about putting 1/2 into the devicetree archive.  The subject is
kept as "[PATCH v5 1/2]" so it still lines up with that cover.

AI-assistance disclosure: this patch is drafted with an AI assistant and
edited by me; the author reviewed, built and tested it, and the content and
the measurements are the author's own.

Two connectors behind one controller only take the mux path when the
controller node itself carries "usb-role-switch": dwc3_get_dr_mode()
otherwise forces dr_mode back to peripheral, dwc3_get_extcon() skips the
role-switch branch and dwc3_drd_init() never calls dwc3_role_mux_init().
The binding accepted such a DT, so a DT that passed the schema check did
not describe a DT the driver could act on.  Require the property whenever
port-select-gpios is present.

Two more holes in the endpoint schema are fixed at the same time:

  - the hs-port patternProperties only covered endpoint@0 and endpoint@1,
    so an unnumbered endpoint or endpoint@2+ escaped the
    unevaluatedProperties: false rule of the graph endpoint.  Use the same
    endpoint name pattern as graph.yaml so every endpoint is validated.

  - the mux-state complementarity was expressed with two independent "if
    const: 0" blocks, which only rejected two zeroes; two endpoints both
    claiming mux-state = <1> passed validation.  Express the pair as oneOf
    so exactly one of the two assignments is accepted.

Signed-off-by: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>
---
 .../bindings/usb/snps,dwc3-common.yaml        | 105 +++++++++---------
 1 file changed, 55 insertions(+), 50 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
index 6b06b0027b..b23d5d8d74 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
@@ -31,7 +31,9 @@ $defs:
         enum: [0, 1]
         description:
           Logical level of the port-select GPIO which connects this
-          connector to the shared High Speed data bus.
+          connector to the shared High Speed data bus.  The two connectors of
+          one controller must use different levels; the driver rejects a
+          device tree where two connectors claim the same one.
 
     dependentRequired:
       usb-role-switch: [mux-state]
@@ -42,7 +44,7 @@ $defs:
     unevaluatedProperties: false
 
     patternProperties:
-      '^endpoint@[01]$':
+      '^endpoint(@[0-9a-f]+)?$':
         $ref: '#/$defs/hs-endpoint'
 
 allOf:
@@ -61,70 +63,74 @@ allOf:
   - if:
       required: [port-select-gpios]
     then:
-      required: [dr_mode, ports]
+      # The mux path is only taken when the controller itself carries
+      # "usb-role-switch": dwc3_get_dr_mode() forces dr_mode back to
+      # peripheral, dwc3_get_extcon() skips the role-switch branch and
+      # dwc3_drd_init() never calls dwc3_role_mux_init() without it.
+      required: [dr_mode, ports, usb-role-switch]
       properties:
         dr_mode:
           const: otg
         ports:
-          required: [port@0]
+          # Each connector is one port with a single endpoint: the mux-state
+          # property on that endpoint says which level of the port-select GPIO
+          # reaches it, so two connectors are two ports, not two endpoints of
+          # one port.  port@1 is the SuperSpeed lane of the PHY and is already
+          # described by snps,dwc3.yaml, so the second High Speed connector
+          # lands on port@2.
+          required: [port@0, port@2]
           properties:
             port@0:
-              required: ['#address-cells', '#size-cells', endpoint@0, endpoint@1]
+              $ref: '#/$defs/hs-port'
+            port@2:
+              $ref: '#/$defs/hs-port'
+
+          # The two connectors must claim opposite levels of the port-select
+          # GPIO; requiring both ports in every branch is what makes the two
+          # cases mutually exclusive for oneOf.
+          oneOf:
+            - required: [port@0, port@2]
               properties:
-                endpoint@0:
-                  required: [usb-role-switch, mux-state]
-                endpoint@1:
-                  required: [usb-role-switch, mux-state]
-    else:
-      properties:
-        ports:
-          properties:
-            port@0:
-              patternProperties:
-                '^endpoint@[01]$':
+                port@0:
+                  $ref: '#/$defs/hs-port'
                   properties:
-                    usb-role-switch: false
-                    mux-state: false
-  - if:
-      properties:
-        ports:
-          properties:
-            port@0:
-              properties:
-                endpoint@0:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 0
+                port@2:
+                  $ref: '#/$defs/hs-port'
                   properties:
-                    mux-state:
-                      const: 0
-    then:
-      properties:
-        ports:
-          properties:
-            port@0:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 1
+            - required: [port@0, port@2]
               properties:
-                endpoint@1:
+                port@0:
+                  $ref: '#/$defs/hs-port'
                   properties:
-                    mux-state:
-                      const: 1
-  - if:
-      properties:
-        ports:
-          properties:
-            port@0:
-              properties:
-                endpoint@1:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 1
+                port@2:
+                  $ref: '#/$defs/hs-port'
                   properties:
-                    mux-state:
-                      const: 0
-    then:
+                    endpoint:
+                      properties:
+                        mux-state:
+                          const: 0
+    else:
       properties:
         ports:
           properties:
             port@0:
-              properties:
-                endpoint@0:
+              patternProperties:
+                '^endpoint(@[0-9a-f]+)?$':
                   properties:
-                    mux-state:
-                      const: 1
+                    usb-role-switch: false
+                    mux-state: false
 
 properties:
   port-select-gpios:
@@ -536,4 +542,3 @@ required:
 
 additionalProperties: true
 ...
-
base-commit: fd69bd54e328c407b1853378b25d15803d3453fa
-- 
2.53.0


^ permalink raw reply	[flat|nested] 3+ messages in thread
* [PATCH v5 0/2] usb: dwc3: fixes for two connectors sharing one HS bus
@ 2026-10-31 15:01 Qianhui Xu
  2026-10-31 15:01 ` [PATCH v5 1/2] dt-bindings: usb: snps,dwc3: tighten the two-connector shared " Qianhui Xu
  0 siblings, 1 reply; 3+ messages in thread
From: Qianhui Xu @ 2026-10-31 15:01 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Thinh Nguyen
  Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-usb,
	linux-devicetree, linux-arm-msm, linux-kernel

*** AI-assistance disclosure ***
This cover letter and the accompanying patches were prepared with the help
of an LLM coding assistant; the author reviewed, tested and takes full
responsibility for the content. All measurements cited below come from real
hardware runs and are reproducible with the commands referenced in our
public reports.

Greetings,

This is v5.  It is a fixup series that applies *on top of* v4 [1] -- it
does not replace v4 1/3..3/3, it fixes them in place:

  - 1/2 tightens the binding of v4 1/3 (snps,dwc3-common.yaml),
  - 2/2 fixes the driver of v4 3/3 (usb: dwc3: support two connectors
    sharing one HS data bus) and also corrects
    usb_role_switch_at_endpoint() introduced by v4 2/3.

Base: v4 3/3 (fd69bd54e328, "usb: dwc3: support two connectors sharing
one HS data bus").  Please apply v4 1/3..3/3 from lore [1] before this
series; the `base-commit:` trailer in this posting is informational only,
since it names a commit that is not in any public tree yet.

Hardware: Lenovo Legion Y700 (4th gen) tablet, SM8750.  Both High Speed
connectors share one DWC3 controller; a port-select GPIO picks which
connector owns the bus, and each connector has its own typec port with its
own role switch, described per graph endpoint.

Changes since v4 (all points are replies to the automated review by
Sashiko [2][3][4]; no human maintainer replied in the v4 thread yet):

Binding (Sashiko on v4 1/3, three Medium findings [2]):

  * "hs-port patternProperties does not match what the driver expects /
    escapes unevaluatedProperties".  Fixed: the endpoint name pattern is
    now the same one graph.yaml uses, so an unnumbered endpoint or
    endpoint@2+ is validated too instead of silently escaping the
    unevaluatedProperties: false rule (v5 1/2).

  * "usb-role-switch should not be required only indirectly".  Fixed: the
    controller node must now carry "usb-role-switch" whenever
    port-select-gpios is present.  This is a driver-fact fix, not just
    schema cosmetics: without that property dwc3_get_dr_mode() forces
    dr_mode back to peripheral and dwc3_drd_init() never runs, so a DT
    that passed the old schema did not describe a DT the driver could act
    on (v5 1/2).

  * "mux-state complementarity is not expressed".  Fixed: the two
    independent `if: const: 0` blocks only rejected two zeroes; two
    endpoints both claiming mux-state = <1> passed validation.  The pair
    is now a single oneOf so exactly one assignment is accepted (v5 1/2).

  * On the standing question whether this should use the standard
    mux-controls/mux-states properties instead of per-endpoint role
    switches: kept the graph-endpoint shape for now.  Reason: the
    arbitration is "earliest request wins" between two *typec connectors*
    that must each be a usb role switch consumer, and the endpoint is the
    fwnode the connector is attached to; a mux-control consumer does not
    give us that per-connector identity.  drivers/usb/ has no
    mux_control consumer today to follow as precedent.  Happy to reshape
    if the DT maintainers prefer the standard property.

usb: roles (Sashiko on v4 2/3, one High finding [3]):

  * "-EPROBE_DEFER is swallowed by !IS_ERR_OR_NULL(sw)".  Fixed: pass the
    error up, as suggested (v5 2/2, last hunk).  This was also the root
    cause we hit on real hardware: the UCSI glue kept a NULL switch and
    the connectors never bound.

usb: dwc3 (Sashiko on v4 3/3, five High + three Medium findings [4]):

  * "dwc3_set_mode() under dwc->lock self-deadlocks".  Fixed: request
    decision and issue are now paired by mux->lock; dwc->lock is only held
    while updating the individual fields (v5 2/2).

  * "glue layer is never notified".  Fixed: dwc3_pre_set_role() is called
    with the effective role, outside spinlocks, before dwc3_set_mode(),
    mirroring the single-connector path (v5 2/2).

  * "same-role request that moves the data path to the other connector".
    Fixed: dwc3_set_prtcap() only clears SUSPHY when PRTCAP changes, so
    that case now clears it explicitly (v5 2/2).

  * "registration earlier than the initial mode request races with a
    connector driver".  Fixed: register and request the initial mode under
    mux->lock, tear down in reverse order on failure (v5 2/2).

  * "fwnode references leak on error paths".  Fixed: dwc->role_mux is
    published before the error paths that used to leak the endpoint
    fwnodes, so dwc3_role_mux_exit() can actually unwind (v5 2/2).

  * "sticky mux->error is reported to the next caller after it already
    committed its own request".  Fixed: the sticky value is dropped; a
    failed data-path switch is logged and counted per connector (v5 2/2).

  * (found during on-device testing, same area) dwc3_mux_role_get()
    returned USB_ROLE_NONE for a connector that did not own the data path,
    hiding its pending request; the connector driver then never sent the
    set_role(NONE) that would clear it, and the stale request kept winning
    arbitration after unplug.  get() now reports the requested role
    (v5 2/2).

Test evidence (all on the SM8750 tablet, kernel built from this series):

  - `git am --3way` of the whole series on top of v4 3/3 succeeds;
    checkpatch --strict reports 0 errors / 0 warnings / 0 checks for both
    patches.
  - `dt-doc-validate` on snps,dwc3-common.yaml: clean.
  - `make W=1 drivers/usb/dwc3/ drivers/usb/roles/` (arm64): 0 warnings.
  - On device: after the -EPROBE_DEFER fix, /sys/class/typec shows port0,
    port1 and a bound partner; with function_graph tracing, 12 consecutive
    plug/unplug cycles each went usb_role_switch_set_role ->
    dwc3_mux_role_set [dwc3] -> glue notifier -> dwc3_set_mode with zero
    WARNING/BUG/lockdep splats, USB data and charging both stable
    throughout.
  - The binding topology (two HS connectors on port@0 and port@2 of the
    controller's port node, one endpoint each, mux-state on the endpoint)
    matches what the board DTS uses; port@1 is occupied by the SuperSpeed
    path to the QMP PHY, which is why the two HS connectors are not
    siblings under one port.

Known limitation, stated up front: the role *arbitration* detail (which
connector wins a simultaneous request) is verified only by code reading and
the stale-request test above; a dedicated per-port role-value trace is
instrumented but its data set is not yet complete.  Everything else listed
above has been exercised on hardware.

Thanks,
Qianhui Xu

[1] v4: https://lore.kernel.org/linux-usb/20261026123211.243224-1-xiaqinghuan@mail.ustc.edu.cn/
[2] Sashiko on v4 1/3: <sashiko-outbox-185247@kernel.org>
[3] Sashiko on v4 2/3: <sashiko-outbox-185254@kernel.org>
[4] Sashiko on v4 3/3: <sashiko-outbox-185299@kernel.org>

Qianhui Xu (2):
  dt-bindings: usb: snps,dwc3: tighten the two-connector shared HS bus
  usb: dwc3: fix the two-connector shared HS bus role handling

 .../bindings/usb/snps,dwc3-common.yaml        | 105 +++++++++---------
 drivers/usb/dwc3/core.c                       |  36 ++++--
 drivers/usb/dwc3/core.h                       |   4 +-
 drivers/usb/dwc3/drd.c                        |  65 ++++++++---
 drivers/usb/roles/class.c                     |   7 +-
 5 files changed, 140 insertions(+), 77 deletions(-)


base-commit: fd69bd54e328c407b1853378b25d15803d3453fa
-- 
2.53.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2026-11-02  8:07 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-02  5:41 [PATCH v5 1/2] dt-bindings: usb: snps,dwc3: tighten the two-connector shared HS bus Qianhui Xu
2026-11-02  8:07 ` Krzysztof Kozlowski
  -- strict thread matches above, loose matches on Subject: below --
2026-10-31 15:01 [PATCH v5 0/2] usb: dwc3: fixes for two connectors sharing one " Qianhui Xu
2026-10-31 15:01 ` [PATCH v5 1/2] dt-bindings: usb: snps,dwc3: tighten the two-connector shared " Qianhui Xu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®