Re: [PATCH v7 1/2] dt-bindings: usb: Add CIX Sky1 USBSSP controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] dt-bindings: usb: cix,sky1-usbssp: overlapping reg windows in
  RCSU block
- [Medium] dt-bindings: usb: cix,sky1-usbssp: redundant unprefixed
  disable properties
--

commit 96269a442880206984d4933991d01ca45a5cc83e
Author: Hongliang Yang <hongliang.yang@cixtech.com>

dt-bindings: usb: Add CIX Sky1 USBSSP controller

This commit adds the Device Tree binding schema for the CIX Sky1 Cadence
USBSSP controller wrapper and its child cdns,usb3 node.

> diff --git a/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml b/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
> new file mode 100644
> index 0000000000000..137ef9ac13356
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
> @@ -0,0 +1,154 @@
[ ... ]
> +  The register range of an instance is a window in the USB30 Type-A DRD
> +  RCSU block at 0x091c0000, starting at the AXI cache attribute strap
> +  register of the instance and ending after its AXI clock status
> +  register. The strap registers of the two controllers and of their PHYs
> +  are interleaved in that block, so the ranges of the two instances
> +  overlap.
[ ... ]
> +properties:
> +  compatible:
> +    const: cix,sky1-usbssp
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      Window from the AXI cache attribute strap register of this instance
> +      up to and including its AXI clock status register.

[Severity: Medium]
Should reg describe non-overlapping register regions (or should the shared
RCSU block at 0x091c0000 be modeled as a shared parent or syscon node)
instead of overlapping windows across sibling devices on the same bus?

Defining a single contiguous window from each controller's AXI cache
attribute strap register to its AXI clock status register makes the two
instances overlap in 0x091c0324..0x091c0403. In addition, the example
below specifies reg = <0x00 0x91c0314 0x00 0x100> for usb@91c0314, which
extends to 0x091c0414 and covers the second controller's STRAP_STATUS4
register at 0x091c0410 (an inclusive window ending after STRAP_STATUS0 at
0x091c0400 would be 0xf0 bytes). Because the two sibling nodes claim
overlapping MMIO ranges, standard exclusive resource reservation via
devm_platform_ioremap_resource() fails with -EBUSY on the second instance.

[ ... ]

> +  ssp-disable:
> +    type: boolean
> +    description: Disable SuperSpeed Plus mode of the controller.
> +
> +  u3-port-disable:
> +    type: boolean
> +    description: The board does not route the USB3 port of the controller.

[Severity: Medium]
Can the standard maximum-speed and phys properties in the child cdns,usb3
node be used instead of introducing ssp-disable and u3-port-disable?

Both properties lack a vendor prefix and duplicate standard USB bindings
already present on the child controller node: limiting the controller to
super-speed or high-speed is covered by maximum-speed, and an unrouted
USB3 port is represented by omitting cdns3,usb3-phy from phys/phy-names or
setting maximum-speed to "high-speed".

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106101210.1059256-1-hongliang.yang@cixtech.com?part=1