Re: [PATCH v1 1/4] dt-bindings: Add CIX Sky1 USB bindings

[Hongliang Yang <hongliang.yang@cixtech.com>]

Hi Krzysztof,

Thank you for the review. Most of the comments are already addressed in
v2 [1]; the remaining points are answered inline below.

[1] https://lore.kernel.org/all/20260902032134.1182634-1-hongliang.yang@cixtech.com/

On 01/09/2026 07:47, Krzysztof Kozlowski wrote:
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
> A nit, subject: drop second/last, redundant "bindings". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v7.1-rc7/source/Documentation/devicetree/bindings/submitting-patches.rst#L23
>
> And do not combine USB patches with other subsystems.

Done in v2: the combined bindings patch is split into separate patches
with "dt-bindings: phy:" and "dt-bindings: usb:" subject prefixes, one
schema per patch, e.g.

  dt-bindings: phy: Add CIX Sky1 USB3 PHY
  dt-bindings: usb: Add CIX Sky1 USBSSP controller

> Please read and follow DTS coding style.

Done in v2: the schemas and the new DTS regions now pass
dt-check-style in strict mode.

> > +unevaluatedProperties: false
>
> Which other schema do you want to reference?

None. Changed to additionalProperties: false in v2.

> > +  reg:
> > +    items:
> > +      - description: AXI property configuration register
> > +      - description: Controller status register
>
> Registers are not address spaces. Devices do not have address spaces of
> one register, so this is not a real device.

I understand the concern, and we looked at this again against the SoC
documentation. These two registers are RCSU (system configuration
unit) registers, but each of them is dedicated to one USB controller
and not shared with any other device:

 - The first register is the controller's Control Register for the AXI
   cache attributes. Its bit fields are awcache[3:0] (bits 3:0) and
   arcache[3:0] (bits 7:4), i.e. the AXI cache attributes of the
   controller's own AXI master interface. The glue driver programs it
   once during initialization.

 - The second register is a status register providing the AXI clock
   valid status the driver polls while enabling the controller.

The two instances submitted in this series illustrate the per-device
offsets: the USB4 controller uses 0x091c0314 (AXI cache attributes)
and 0x091c0400 (status), while USB5 uses 0x091c0324 and 0x091c0410.

The reason they end up in a separate node from the Cadence USBSSP core
is purely physical: on Sky1 the controller's control registers and the
Cadence USBSSP core registers are two disjoint regions in the address
map (e.g. 0x091c0314 vs 0x091d0000 for the USB4 controller). The glue
node therefore describes the Sky1-specific control registers, clocks
and resets, while the core node is the Cadence USBSSP core itself,
describing the actual register windows the cdns3 driver maps
("otg", "xhci", "dev").

That said, the register windows are admittedly tiny, so if you prefer
this modelled differently we are open to suggestions. One alternative
we considered was folding the control register file into the core node
and referencing the Sky1 glue properties from there, but that would
mix the Cadence core registers with Sky1-specific ones and lose the
1:1 mapping to the hardware topology.

> > +      - const: axi_property
>
> axi
>
> > +      - const: controller_status
>
> controller

Renamed in v2 (reg-names = "axi", "controller").

> > +  cix,usb_syscon:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      Phandle to the Sky1 S5 system controller used to program USB mode
> > +      strap controls.
>
> So this is part of that device most likely.

The OTG mode strap bits live in the S5 power-management controller,
which is a separate hardware block shared by several subsystems, so
they are not part of the USB controller's register file. That is why
the strap is accessed through the syscon phandle instead of a reg
entry, while the per-controller registers discussed above stay in the
USB node's own reg.

> > +  axi_bmax_value:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 0x7
> > +    description:
> > +      AXI bus maximum burst length configuration value.

Dropped in v2: the value is fixed for the SoC, so the driver now uses a
compile-time constant and the property is gone.

> > +patternProperties:
> > +  "^usb-controller@[0-9a-f]+$":
>
> Why using completely different names than entire Linux kernel?

Renamed to "usb@..." in v2, matching the cdns,usb3 nodes elsewhere.

> > +    $ref: cdns,usb3.yaml#
> > +    description:
> > +      The Cadence USBSSP core controller node. See cdns,usb3.yaml
> > +      for properties such as dr_mode, maximum-speed, phys, and interrupts.
>
> Redundant description.

Shortened in v2.

Best regards,
Hongliang Yang