Re: [PATCH 3/4] dt-bindings: usb: add CIX Sky1 Cadence USB3 controller

[Krzysztof Kozlowski <krzk@kernel.org>]

On Mon, May 11, 2026 at 10:42:43AM +0800, Peter Chen wrote:
> Add a binding for the CIX Sky1 integration of the Cadence USBSSP DRD
> controller. The schema documents the glue register window, clocks,
> resets, interrupts and S5 system controller phandle.
> 
> Signed-off-by: Peter Chen <peter.chen@cixtech.com>
> ---
>  .../bindings/usb/cix,sky1-cdns3.yaml          | 151 ++++++++++++++++++

Why are you mixing USB patches with DTS in one patchset? Don't.

>  1 file changed, 151 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml b/Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml
> new file mode 100644
> index 000000000000..23d82d8cc9bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml

Complete mess of filename. There is no such compatible.

> @@ -0,0 +1,151 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/cix,sky1-cdns3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CIX Sky1 Cadence USB3 Controller
> +
> +maintainers:
> +  - Peter Chen <peter.chen@cixtech.com>
> +
> +description:
> +  The CIX Sky1 USB3 controller is based on the Cadence USBSSP DRD
> +  controller. The integration adds glue registers and mode strap controls
> +  in the Sky1 S5 system controller.
> +
> +allOf:
> +  - $ref: usb-drd.yaml#
> +  - $ref: usb-xhci.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: cix,sky1-usb3
> +      - const: cix,cdns-usb3

I don't understand the fallback compatible. You claim this device is
called EXACTLY like vendor cdns? Nope, you SoC specific compatibles.


> +
> +  reg:
> +    items:
> +      - description: OTG controller registers
> +      - description: Device controller registers
> +      - description: XHCI host controller registers
> +      - description: Sky1 USB glue registers
> +
> +  reg-names:
> +    items:
> +      - const: otg
> +      - const: dev
> +      - const: xhci

Wrong order, look at cdns,usb3 schema.

> +      - const: glue
> +
> +  interrupts:
> +    items:
> +      - description: XHCI host controller interrupt
> +      - description: Device controller interrupt
> +      - description: OTG/DRD controller interrupt
> +      - description: Wakeup interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: host
> +      - const: peripheral
> +      - const: otg
> +      - const: wakeup
> +
> +  clocks:
> +    items:
> +      - description: Start-of-frame clock
> +      - description: AXI bus clock
> +      - description: Low-power mode clock
> +      - description: APB register interface clock
> +
> +  clock-names:
> +    items:
> +      - const: sof
> +      - const: aclk
> +      - const: lpm
> +      - const: pclk
> +
> +  resets:
> +    items:
> +      - description: APB register reset
> +      - description: Controller reset
> +
> +  reset-names:
> +    items:
> +      - const: prst

apb

> +      - const: rst

controller or core

> +
> +  cix,syscon-usb:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the Sky1 S5 system controller used to program USB mode
> +      strap controls.
> +
> +  dma-coherent: true
> +
> +  maximum-speed:
> +    enum: [super-speed-plus, super-speed, high-speed, full-speed]

Why isn't this deducible from the compatible?

> +
> +  phys:
> +    minItems: 1
> +    maxItems: 2

No, this is not flexible.

> +
> +  phy-names:
> +    minItems: 1
> +    maxItems: 2
> +    items:
> +      anyOf:
> +        - const: cdns3,usb2-phy
> +        - const: cdns3,usb3-phy

Drop all this and define standard names.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - resets
> +  - reset-names
> +  - cix,syscon-usb

phys should be required, no?

> +
> +unevaluatedProperties: false

Best regards,
Krzysztof