On 06/11/2026 06:50, Hongliang Yang wrote:
On Thu, Nov 05, 2026 at 08:49:05AM +0100, Krzysztof Kozlowski wrote:
quoted
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)
So instead of replying under each comment, you selected a few of them
and expect me to map/figure out which ones you ignored?
OK, then find me where did you reply to this:
"And do not combine USB patches with other subsystems."
"I do not see how you addressed my feedback from v1. I asked, not to
combine USB patches with other subsystems, within one patchset. These
are separate patchsets."
Either here below or in original threads (more than one):
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:
Where is the reply?
Best regards,
Krzysztof