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

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

On Thu, Nov 05, 2026 at 08:49:05AM +0100, Krzysztof Kozlowski wrote:
> NAK, you did not respond, did not implement feedback or part of the
> feedback.

You are right, and I owe you a proper answer instead of another version.
I replied to the cover letter of v5 and to nothing else, so the per-patch
comments on v5 (b28d5657) and on v4 (a5586b54) were never answered.
Answering a cover letter is not answering a review. Below is the
point-by-point answer, including what I got wrong and what I have not
implemented.

1) "No MMIO here? ... If you have here MMIO for some glue over IP block,
   then it is unlikely to be a syscon." (v4, 7c52bcf5)

We do have MMIO. Each controller owns registers in the USB30 Type-A
DRD0/1 RCSU block at 0x091c0000. "What is the actual memory map there?"
(791fe5ba) - this is the block:

  offset   register        contents                                  belongs to
  0x0300   STRAP_PIN0      stream ID (SID/SSID), controller 0        USB4
  0x0304   STRAP_PIN1      stream ID (SID/SSID), controller 1        USB5
  0x030c   STRAP_PIN3      clock enables, controllers and PHYs       USB4, USB5
  0x0314   STRAP_PIN5      awcache[3:0], arcache[7:4], controller 0  USB4
  0x0324   STRAP_PIN9      awcache[3:0], arcache[7:4], controller 1  USB5
  0x0328   STRAP_PIN10     USB2 PLL controls, controller 0           USB4
  0x0400   STRAP_STATUS0   AXI clock en/valid/req, controller 0      USB4
  0x0410   STRAP_STATUS4   AXI clock en/valid/req, controller 1      USB5

Controller 0 of the block is used by USB4, controller 1 is used by USB5.
The driver writes the AXI cache attributes of its own controller and
polls its own AXI clock valid bit. By the rule you stated, this is a
device with MMIO, so a device node, not a syscon, and the
cix,sky1-usbss-rcsu syscon binding from v4 is gone.

2) "Individual registers are not a device." (791fe5ba)
   "You need to describe the address ranges, hierarchy, not the
   registers." (291fc3be)

Agreed, and v6 still got this wrong: two reg entries of 0x10 bytes each
are just a register with padding. What I intend to send as v7 is one
range per controller, starting at its AXI cache attribute register and
covering the register file up to its status register:

  usb@91c0314  reg = <0x0 0x91c0314 0x0 0x100>   /* USB4, controller 0 */
  usb@91c0324  reg = <0x0 0x91c0324 0x0 0x100>   /* USB5, controller 1 */

0x100 is the same size as the glue window of ti,j721e-usb in
Documentation/devicetree/bindings/usb/cdns3-ti.yaml, so the range is
comparable to what the other Cadence users describe for glue registers.

I want to be explicit about the hardware rather than let the DT imply
otherwise: the strap and status registers of the two controllers are
interleaved in this one block, and 0x0328 inside controller 1's range
belongs to controller 0. There is no per-controller contiguous range
that contains only that controller's registers, so the two ranges
overlap, and the binding will say so. That is also why the driver maps
the range with devm_ioremap() and not devm_platform_ioremap_resource():
request_mem_region() would make one of the two controllers fail with
-EBUSY for registers the hardware shares.

To be clear that this is the controller's register file and not an
arbitrary register: the same register set exists as a dedicated block per
controller on the four Type-C instances of this SoC, at 0x09000000,
0x09070000, 0x090e0000 and 0x09150000. There the block is not shared, and
the cache attribute register is at 0x310 instead of 0x314 - which is why
the status offset comes from per-compatible driver data. Only the two
Type-A controllers share one block.

3) "Shared region is not a simple bus." / "Drop node. Not relevant."
   (b28d5657)

Done. usb-rcsu@91c0300 and its ranges translation are removed; v6 and
v7 use absolute addresses with nothing translated.

4) "So this is @14 / But this is at @91d0000 / You have completely fake
   representation of hardware." (b28d5657)

That was the v5 container: a node at a translated offset 0x14 with a
child at an absolute address. In v7 the unit address of each node is the
first address in its reg, so usb@91c0314 and usb@91c0324 are where those
registers actually are, and the Cadence core children usb@91d0000 and
usb@91e0000 are at their own addresses.

5) "Syscon is not a USB device. This is not supposed to be in USB."
   (f76cef66)

The RCSU binding is gone since v5. The only syscon reference left is
cix,usb-syscon, which points at the S5 system controller for the USB
mode strap at 0x424, bit 8 for USB4 and bit 10 for USB5. That register
is in the system control module, not in the USB controller.

6) "NAK, undocumented ABI. Drop entire alias and do not sneak such stuff
   in the future." (6029d402)

Done in v6: of_alias_get_id() and the "id" property are gone, the driver
no longer derives an index from DT. axi_bmax_value is no longer a DT
property either, it is a driver constant, which also answers Conor's
"Why is this not a fixed value per SoC?".

7) "In the description you describe hardware, not binding." (f76cef66)

I extended the description because I did not know how else to answer
"What is the actual memory map there?" in the patch. In v7 the
description is back to what the block is and which registers it holds;
the map above lives in the commit message, where it belongs.

8) "No improvements, since v2." (7c35c198)

You were right about the register model, and that is what v7 changes.
The rest changed in v2..v6 but the changelog never said so - it only
ever listed the previous revision, and v5's text was a stale copy of
v2's. Complete one:

Changes in v7:
- Describe the RCSU registers of each controller as one address range
  starting at its AXI cache attribute register, instead of two 16-byte
  register slots.
- Map that range with devm_ioremap(); request_mem_region() cannot be
  used because the two Type-A controllers share the block and their
  ranges overlap.
- Take the offset of the AXI clock status register from per-compatible
  driver data (0xec on Type-A, 0xf0 on Type-C), instead of two
  reg-names.
- Dropped #syscon-cells from the S5 system controller binding: the
  property does not exist in the kernel, and the series no longer
  touches bindings/soc/cix.

Changes in v6:
- Dropped the simple-bus RCSU container; each node is placed at the
  address of its glue registers and the core is a child at its own
  address.
- Replaced the usbN aliases and the undocumented id property with the
  register offset and bit position arguments of cix,usb-syscon.
- Converted probe error paths to dev_err_probe().
- Removed the unused over-current GPIO handling and the unused dr_base
  and cdnsp_pdev fields.
- Fixed the 125us prescaler value: time * frequency - 1.
- Made the AXI halt writes read-modify-write so the preserved and RW1C
  bits of XEC_AXI_CAP keep their state.

Changes in v5:
- Dropped the cix,sky1-usbss-rcsu syscon binding and the cix,usb-rcsu
  phandle; the glue registers are described in the controller node.
- Added the MAINTAINERS entry for the glue driver.

Changes in v4:
- Split the combined PHY/USBSSP series into two independent series.
- Restructured the RCSU registers as a shared syscon region with offsets
  (reverted in v5).
- Fixed the indentation reported by checkpatch.

Changes in v3:
- Fixed the interrupt specifiers of the core nodes: the Sky1 GICv3 uses
  four cells, so every entry needs the fourth (empty) cell.
- PHY driver: assert the resets before registering the PHY provider,
  disable clocks in LIFO order, and fix the error unroll path so clocks
  that were never enabled are not disabled.

Changes in v2:
- Split the single binding into a PHY binding and a USBSSP binding.
- clock-names: apb/ref for the PHY, sof/axi/lpm/apb for the controller.
- reset-names: core/apb instead of reset/preset.
- Dropped axi_bmax_value from DT; it is a driver constant.
- Added the MAINTAINERS entries.

Changes in v1: initial version.

9) The one thing I am still unsure about is the wrapper. You wrote, for
   the case without MMIO, "you should have one node only, just like we
   did for DWC". We do have MMIO, so I kept the wrapper and the child
   core the way fsl,imx8mp-dwc3.yaml does it, because the glue and the
   core are driven by two separate drivers, and folding them into one
   node would need a single driver owning five reg ranges across two
   unrelated address regions. If you would rather have one node, one
   line is enough and I will restructure before posting.

I am implementing v7 now (the reg change, devm_ioremap(), the per-family
status offset, and the trimmed description) and will post it in the next
few days rather than ask you another question and wait. If the shape
above is still wrong, replying "no" to this mail is enough and I will not
send it.

Thanks,
Hongliang