Re: [PATCH 1/3] dt-bindings: usb: snps,dwc3: describe two connectors sharing one HS bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: usb: snps,dwc3: missing endpoint/mux-state required in oneOf
- [Medium] dt-bindings: usb: snps,dwc3: port@2 missing from top-level ports schema
- [Medium] dt-bindings: usb: snps,dwc3: Linux-specific mux binding and port@2 split
--

commit f3f5f5bb87ea01fcf8b40062734fa5217caa615a
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

dt-bindings: usb: snps,dwc3: describe two connectors sharing one HS bus

This commit extends snps,dwc3-common.yaml to describe two USB connectors
sharing a single High-Speed bus via a port-select-gpios multiplexer, using
port@0 and port@2 with per-endpoint mux-state and usb-role-switch
properties.

> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> index 6c0b8b6538246..0be2a6e83ee48 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> @@ -13,6 +13,40 @@ description:
>    Defines the properties of the DWC3 core as being embedded in either an
>    vendor-specific implementation or as a standalone component.
>  
> +$defs:
> +  hs-endpoint:
> +    $ref: /schemas/graph.yaml#/$defs/endpoint-base
> +    unevaluatedProperties: false
> +
> +    properties:
> +      usb-role-switch:
> +        type: boolean
> +        description:
> +          This connector endpoint has its own USB role switch. The switch is
> +          registered against this endpoint and looked up with
> +          usb_role_switch_at_endpoint().
> +
> +      mux-state:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1]
> +        description:
> +          Logical level of the port-select GPIO which connects this
> +          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]
> +      mux-state: [usb-role-switch]
> +
> +  hs-port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    unevaluatedProperties: false
> +
> +    patternProperties:
> +      '^endpoint(@[0-9a-f]+)?$':
> +        $ref: '#/$defs/hs-endpoint'
> +
>  allOf:
>    - $ref: usb-drd.yaml#
>    - if:
> @@ -26,8 +60,87 @@ allOf:
>        $ref: usb.yaml#
>      else:
>        $ref: usb-xhci.yaml#
> +  - if:
> +      required: [port-select-gpios]
> +    then:
> +      # 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:
> +          # 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:
> +              $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:
> +                port@0:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 0
> +                port@2:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 1
> +            - required: [port@0, port@2]
> +              properties:
> +                port@0:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 1
> +                port@2:
> +                  $ref: '#/$defs/hs-port'
> +                  properties:
> +                    endpoint:
> +                      properties:
> +                        mux-state:
> +                          const: 0

[Severity: Medium]
Does the oneOf block under allOf allow incomplete or duplicate mux-state
configurations to pass schema validation while rejecting numbered
endpoint@0 nodes?

In $defs/hs-endpoint, usb-role-switch and mux-state are only constrained
with dependentRequired, so an endpoint omitting both properties satisfies
hs-endpoint. In addition, neither port@0 nor port@2 in the allOf then
branch specifies required: [endpoint], and the endpoint subschemas inside
the two oneOf branches only specify properties: { mux-state: { const: ... } }
without required: [mux-state].

In JSON Schema, the properties keyword evaluates to true when the target
property is absent. If port@0 has an endpoint with mux-state = <0> and
usb-role-switch, while port@2 omits endpoint (or its endpoint omits both
mux-state and usb-role-switch), the first branch of oneOf evaluates to true
and the second branch evaluates to false, allowing an incomplete device
tree that fails at runtime in dwc3_role_mux_init() to pass validation.

Furthermore, $defs/hs-port defines patternProperties matching
^endpoint(@[0-9a-f]+)?$, whereas the oneOf branches only check the literal
unnumbered property name endpoint:

1. If port@0 uses endpoint with mux-state = <0> and port@2 uses endpoint@0
   with mux-state = <0>, port@2 has no unnumbered endpoint property and
   vacuously matches const: 1 in the first oneOf branch, allowing duplicate
   mux-state levels to pass.
2. If both port@0 and port@2 use endpoint@0 with opposite mux-state values
   0 and 1, neither port has an unnumbered endpoint property, so both
   branches of oneOf evaluate to true and oneOf fails.
3. If a port defines both endpoint and endpoint@1, hs-port accepts both and
   oneOf ignores endpoint@1, even though the commit message states that a
   third endpoint is rejected.

Should port@0 and port@2 require a single unnumbered endpoint (or constrain
endpoint count and naming consistently between hs-port and oneOf) and
require mux-state and usb-role-switch on each endpoint when
port-select-gpios is present?

> +    else:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              patternProperties:
> +                '^endpoint(@[0-9a-f]+)?$':
> +                  properties:
> +                    usb-role-switch: false
> +                    mux-state: false
>  
>  properties:
> +  port-select-gpios:
> +    maxItems: 1
> +    description:
> +      Selects which of the two connectors drives the controller's shared High
> +      Speed data bus.  Each connector is described by "port@0" or "port@2",
> +      and the endpoint of that port carries the connector's own role-switch
> +      input and the "mux-state" level selecting it.

[Severity: Medium]
Is defining custom port-select-gpios and mux-state properties on the DWC3
controller and splitting the shared High-Speed bus across port@0 and port@2
the right hardware representation for this board topology?

There are three concerns with this binding design in
Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml:

1. The DWC3 controller itself has a single High-Speed D+/D- pair routed
   through an external GPIO-controlled multiplexer to two connectors.
   Instead of introducing custom port-select-gpios and endpoint mux-state
   properties on the DWC3 binding, could this use the existing multiplexer
   bindings in Documentation/devicetree/bindings/mux/gpio-mux.yaml and
   Documentation/devicetree/bindings/mux/mux-consumer.yaml (such as
   mux-controls or mux-states)?
2. Under ports in snps,dwc3-common.yaml, port@0 represents the controller's
   High-Speed data bus and port@1 represents the SuperSpeed data bus. Since
   the controller still has only one physical High-Speed bus, why is the
   second connector modeled as a separate controller port (port@2) rather
   than two endpoints (endpoint@0 and endpoint@1) under port@0 or via an
   intermediate mux node?
3. Documentation/devicetree/bindings/writing-bindings.rst states that
   bindings should describe hardware rather than Linux driver
   implementation details. Here, $defs/hs-endpoint and the allOf conditional
   for port-select-gpios reference Linux kernel functions
   (usb_role_switch_at_endpoint(), dwc3_get_dr_mode(), dwc3_get_extcon(),
   dwc3_drd_init(), and dwc3_role_mux_init()) and force dr_mode and
   controller-level usb-role-switch into required to satisfy current driver
   control flow, even though usb-drd.yaml already defines dr_mode as
   optional with a default of otg.

> @@ -411,7 +524,7 @@ properties:
>  
>      properties:
>        port@0:
> -        $ref: /schemas/graph.yaml#/properties/port
> +        $ref: '#/$defs/hs-port'
>          description: High Speed (HS) data bus.
>  
>        port@1:

[Severity: Medium]
Will dtschema reject every device tree that sets port-select-gpios and
defines port@2 because port@2 is missing from the top-level
properties/ports block in snps,dwc3-common.yaml?

In snps,dwc3-common.yaml, the top-level properties/ports schema references
/schemas/graph.yaml#/properties/ports and only lists port@0 and port@1:

    ports:
      $ref: /schemas/graph.yaml#/properties/ports
      ...
      properties:
        port@0:
          $ref: '#/$defs/hs-port'
          description: High Speed (HS) data bus.

        port@1:
          $ref: /schemas/graph.yaml#/properties/port
          description: Super Speed (SS) data bus.

(Note also that the comment in allOf and the commit message state that
port@1 is described by snps,dwc3.yaml, whereas port@1 is defined here in
snps,dwc3-common.yaml.)

When dtschema processes a node referencing graph.yaml#/properties/ports
that defines properties, it automatically injects unevaluatedProperties:
false into that properties/ports subschema. In JSON Schema,
unevaluatedProperties: false on the top-level properties/ports block does
not see port@2 evaluated inside the separate allOf then branch, so any
device tree containing port@2 is rejected as having an unevaluated property.

Conversely, in the allOf else branch when port-select-gpios is absent, only
port@0 is restricted and port@2 is not disallowed with port@2: false.

Should port@2 be declared under the top-level properties/ports block and
disallowed with port@2: false in the else branch when port-select-gpios is
absent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104032706.210042-1-xiaqinghuan@mail.ustc.edu.cn?part=1