Re: [RFC PATCH v3 1/2] 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: hs-port rejects unnumbered endpoint in port@0
- [Medium] dt-bindings: usb: snps,dwc3: mux-state conditionals allow duplicate state 1
- [Low] dt-bindings: usb: custom mux properties and usb_role_switch_at_endpoint() ref
--

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

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

This commit updates Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
to describe two connectors sharing the High Speed bus of a single DWC3
controller via a GPIO selector and numbered endpoints under port@0.

> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> index 6c0b8b6538246..6b06b0027b393 100644
> --- a/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3-common.yaml
> @@ -13,6 +13,38 @@ 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.

[Severity: Low]
In $defs/hs-endpoint, the usb-role-switch description references the Linux
kernel internal function usb_role_switch_at_endpoint(). Since
Documentation/devicetree/bindings/writing-bindings.rst advises against
referring to Linux or driver implementation details in hardware bindings,
should this function reference be removed from the description?

Also, the GPIO-controlled switch routing the shared High-Speed D+/D- pair
between two connectors appears to be an external board-level multiplexer.
Could this use the standard multiplexer bindings from
Documentation/devicetree/bindings/mux/gpio-mux.yaml and
Documentation/devicetree/bindings/mux/mux-consumer.yaml (such as mux-controls
and mux-states) rather than introducing custom port-select-gpios and singular
mux-state properties on the DWC3 controller and its endpoints?

> +
> +    dependentRequired:
> +      usb-role-switch: [mux-state]
> +      mux-state: [usb-role-switch]
> +
> +  hs-port:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    unevaluatedProperties: false
> +
> +    patternProperties:
> +      '^endpoint@[01]$':
> +        $ref: '#/$defs/hs-endpoint'

[Severity: Medium]
Does replacing /schemas/graph.yaml#/properties/port with #/$defs/hs-port on
ports/properties/port@0 cause a dtbs_check validation regression for existing
single-connector DWC3 device trees?

Previously, port@0 referenced /schemas/graph.yaml#/properties/port, which
allows both an unnumbered endpoint node and numbered endpoint@N child nodes.
Here, $defs/hs-port references /schemas/graph.yaml#/$defs/port-base with
unevaluatedProperties: false and only matches ^endpoint@[01]$ in
patternProperties.

When a single-connector DWC3 node without port-select-gpios defines an
unnumbered endpoint child node under port@0 (such as in
arch/arm64/boot/dts/qcom/sm8550.dtsi,
arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts, or
arch/arm64/boot/dts/apple/t8103-jxxx.dtsi), won't that unnumbered endpoint
node now be rejected as an unevaluated property?

> +
>  allOf:
>    - $ref: usb-drd.yaml#
>    - if:
> @@ -26,8 +58,83 @@ allOf:
>        $ref: usb.yaml#
>      else:
>        $ref: usb-xhci.yaml#
> +  - if:
> +      required: [port-select-gpios]
> +    then:
> +      required: [dr_mode, ports]
> +      properties:
> +        dr_mode:
> +          const: otg
> +        ports:
> +          required: [port@0]
> +          properties:
> +            port@0:
> +              required: ['#address-cells', '#size-cells', endpoint@0, endpoint@1]
> +              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]$':
> +                  properties:
> +                    usb-role-switch: false
> +                    mux-state: false
> +  - if:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@0:
> +                  properties:
> +                    mux-state:
> +                      const: 0
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@1:
> +                  properties:
> +                    mux-state:
> +                      const: 1
> +  - if:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@1:
> +                  properties:
> +                    mux-state:
> +                      const: 0
> +    then:
> +      properties:
> +        ports:
> +          properties:
> +            port@0:
> +              properties:
> +                endpoint@0:
> +                  properties:
> +                    mux-state:
> +                      const: 1

[Severity: Medium]
Do these two if-then blocks in allOf allow both endpoint@0 and endpoint@1 to
set mux-state = <1> at the same time?

Both if conditions test for const: 0 (on endpoint@0 and endpoint@1
respectively). When both endpoints set mux-state = <1>, neither if condition
matches const: 0, so neither then branch is enforced and the duplicate
mux-state = <1> configuration passes validation.

Also, because the if blocks do not include required constraints at each nested
level (ports, port@0, endpoint@0/endpoint@1, and mux-state), won't the if
conditions match vacuously when those nodes or properties are absent?

[ ... ]

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

[ ... ]

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