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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 06/11/2026 06:50, Hongliang Yang wrote:
> 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)

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