Re: [PATCH v8 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 disable
  properties
--

commit e68eb2e5dd3e2c328c5e798e76df60be2d4b127a
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 Cadence USBSSP DRD
controller wrapper on the CIX Sky1 SoC.

> 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..994c805d30da2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/cix,sky1-usbssp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CIX Sky1 Cadence USBSSP Controller
> +
> +maintainers:
> +  - Hongliang Yang <hongliang.yang@cixtech.com>
> +
> +description: |
> +  The CIX Sky1 USBSSP controller is based on the Cadence USBSSP DRD
> +  controller. Each instance wraps one of the two USB3 Type-A DRD
> +  controllers of the SoC.
> +
> +  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.
> +
> +  The Cadence USBSSP core itself is described by a child node at its own
> +  address. Mode strap controls are programmed through the Sky1 S5 system
> +  controller referenced by the cix,usb-syscon phandle, whose arguments
> +  give the register offset and the bit position of the strap field.
> +
> +  The USB2 PHY of the controller needs no configuration. It is described
> +  as a usb-nop-xceiv device and referenced by the core node like any
> +  other PHY.
> +
> +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 each instance describe its disjoint strap and status registers as two
separate reg entries (like rcsu_strap and rcsu_status in
cix,sky1-pcie-host.yaml) or model the RCSU block as a shared parent node
rather than a single overlapping reg window?

Because the RCSU block places all strap registers in 0x091c0300..0x091c03ff
and all status registers in 0x091c0400..0x091c04ff, combining both into one
contiguous range causes the two sibling nodes on the same bus (usb@91c0314
and usb@91c0324) to overlap across 0x091c0324..0x091c0403 and the PHY's
strap register at 0x091c0328.  Overlapping reg ranges prevent standard
exclusive MMIO resource reservation via devm_platform_ioremap_resource(), as
the second instance fails probe with -EBUSY.

In addition, the example below uses reg = <0x00 0x91c0314 0x00 0x100>, which
extends to 0x091c0413 (including the second controller's status register at
0x091c0410) instead of 0xf0 bytes ending after this instance's status
register at 0x091c0400.

[ ... ]

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

[Severity: Medium]
Are cix,ssp-disable and cix,u3-port-disable redundant with the standard USB
properties already present on the child cdns,usb3 node?

Limiting the controller to Gen1 SuperSpeed is already expressed by setting
maximum-speed = "super-speed" on the child node, and an unrouted USB3 port
is expressed by maximum-speed = "high-speed" along with omitting
cdns3,usb3-phy from phys and phy-names.  Keeping separate disable properties
on the parent wrapper also allows contradictory configuration between the
glue driver and cdnsp_run() in the Cadence gadget driver, which reads
maximum-speed from the child node and programs the same 3xPort mode
register.

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