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 4 potential
issues to consider:
- [Medium] dt-bindings: usb: snps,dwc3: missing required on endpoint and
  mux-state
- [Medium] dt-bindings: usb: snps,dwc3: hs-port endpoint pattern
  mismatches oneOf
- [Medium] dt-bindings: usb: snps,dwc3: port@2 missing from top-level
  ports schema
- [Low] dt-bindings: usb: snps,dwc3: shared HS mux modeled as port@2
  with Linux refs
--

commit f8622d01bfd3dd39d739606bdf63957e847faee6
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 connectors sharing
a single High Speed data bus selected via port-select-gpios.

> 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'

[Severity: Medium]
Does patternProperties here mismatch the oneOf check below, which only
inspects the unnumbered endpoint node name?

Because hs-port matches '^endpoint(@[0-9a-f]+)?$', it accepts multiple
endpoints in one port as well as numbered endpoints like endpoint@0.  If
both port@0 and port@2 use endpoint@0 with opposite mux-state values,
neither port has an unnumbered endpoint property, so both oneOf branches
below match vacuously and validation fails.  Conversely, if port@0 uses
endpoint and port@2 uses endpoint@0, both connectors can claim the same
mux-state value and still pass oneOf.

>  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]

[Severity: Low]
Should a shared High Speed bus routed through an external GPIO mux be
modeled as multiple endpoints under port@0 (or using standard mux bindings
such as gpio-mux.yaml and mux-states) rather than as a second controller
port@2 with custom port-select-gpios and mux-state properties?  In OF graph
bindings, ports under the controller node represent the controller's own
physical buses, and the controller still has only one High Speed bus at
port@0.

Also, the property descriptions and comments above reference Linux driver
internals (usb_role_switch_at_endpoint(), dwc3_get_dr_mode(),
dwc3_get_extcon(), dwc3_drd_init(), and dwc3_role_mux_init()) to justify
binding constraints, and note that port@1 is described by snps,dwc3.yaml
when port@1 is defined in snps,dwc3-common.yaml.

> +          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]
Can a device tree omit endpoint or mux-state on one of the ports and still
pass this oneOf check?

In hs-endpoint, mux-state and usb-role-switch are only linked via
dependentRequired, and neither hs-port nor the oneOf branches list endpoint
or mux-state in required.  In JSON Schema, properties constraints succeed
when the named property is absent, so if port@0 has an endpoint with
mux-state = <0> and usb-role-switch while port@2 omits mux-state and
usb-role-switch (or omits endpoint altogether), the first oneOf branch
matches and dtbs_check passes a device tree that dwc3_role_mux_init() will
reject at runtime with -EINVAL.

> +    else:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              patternProperties:
> +                '^endpoint(@[0-9a-f]+)?$':
> +                  properties:
> +                    usb-role-switch: false
> +                    mux-state: false

[ ... ]

> @@ -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]
Does omitting port@2 from the top-level ports properties cause dtbs_check
to reject port@2 as an unevaluated property?

Top-level properties.ports references /schemas/graph.yaml#/properties/ports
and only declares port@0 and port@1, so dtschema's graph fixup on
/properties/ports rejects port@2 even when port-select-gpios is present.
Defining port@2 under top-level properties.ports.properties and setting
port@2: false in the else branch above when port-select-gpios is absent
would allow it only when the GPIO mux is used.

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