[PATCH v6 0/3] usb: cdns3: Add CIX Sky1 USBSSP controller support

[PATCH v6 0/3] usb: cdns3: Add CIX Sky1 USBSSP controller support

[Hongliang Yang (杨宏亮)]

This series adds support for the USBSSP controllers USB4 and USB5 of
the CIX Sky1 SoC.

Patch 1 adds the device tree binding, patch 2 the glue driver, and
patch 3 the SoC nodes plus the board enablement.

The controllers are Cadence USBSSP instances. The CIX integration adds
per-controller glue registers in the SoC RCSU, an AXI bus attribute
configuration register and a controller status register, and mode strap
controls in the S5 system controller. The glue driver manages the
clocks, the resets, the strap controls and the glue registers, and then
populates the Cadence USBSSP core, which is described as a child node
at the core address.

Tested on Radxa Orion O6 (Sky1) in host mode, together with the CIX
USB3 PHY series posted separately.

The series applies on top of v7.4-rc1.

Changes in v6:
- Dropped the simple-bus RCSU container. Each controller node is now
  placed at the address of its glue registers and the Cadence USBSSP
  core is a child node at its own address.
- Replaced the usbN aliases and the undocumented id property with the
  register offset and bit position arguments of the cix,usb-syscon
  phandle.
- Documented the cix,usb-syscon arguments and allowed #syscon-cells on
  the S5 system controller binding.
- Converted the probe error paths to dev_err_probe().
- Removed the unused over-current GPIO handling and the unused dr_base
  and cdnsp_pdev fields.
- Do not disable the clocks before they have been acquired.
- Dropped the redundant depends on USB_CDNS3 and fixed the compatible
  name in the Kconfig help text.
- Reference the shared USB3 PHY through its single phandle.
- Fix the 125us prescaler value: the hardware defines it as
  time * frequency - 1.
- Make the AXI halt writes read-modify-write so the preserved and
  RW1C bits of XEC_AXI_CAP keep their state.
- Corrected the mail sender name: v5 was sent with a non-ASCII
  display name in the From header, v6 uses the plain ASCII name
  that matches the Signed-off-by.

Changes in v5:
- Added the MAINTAINERS entry for the glue driver.
- Fixed continuation line indentation reported by checkpatch.


Hongliang Yang (3):
  dt-bindings: usb: Add CIX Sky1 USBSSP controller
  usb: cdns3: Add CIX Sky1 glue driver
  arm64: dts: cix: Add USB4/USB5 nodes for Sky1

 .../soc/cix/cix,sky1-system-control.yaml      |   3 +
 .../bindings/usb/cix,sky1-usbssp.yaml         | 155 ++++
 MAINTAINERS                                   |   2 +
 arch/arm64/boot/dts/cix/sky1-orion-o6.dts     |  42 ++
 arch/arm64/boot/dts/cix/sky1.dtsi             |  85 +++
 drivers/usb/cdns3/Kconfig                     |  13 +
 drivers/usb/cdns3/Makefile                    |   1 +
 drivers/usb/cdns3/cdnsp-sky1.c                | 680 ++++++++++++++++++
 drivers/usb/cdns3/cdnsp-sky1.h                | 100 +++
 9 files changed, 1081 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
 create mode 100644 drivers/usb/cdns3/cdnsp-sky1.c
 create mode 100644 drivers/usb/cdns3/cdnsp-sky1.h


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
prerequisite-patch-id: 2f1459f75e92d7faeb93a1e2f39d27901bea3e39
prerequisite-patch-id: 39e78ce1767908db1293c4868bc320abedacfebf
prerequisite-patch-id: 0f0fd45ae0d7cc57eb8c899a2ecb9f1efa6ebbfc

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

[Hongliang Yang (杨宏亮)]

Add device tree binding for the CIX Sky1 Cadence USBSSP controller.
Each controller has an AXI bus attribute configuration register and a
controller status register inside the SoC RCSU, mapped by the reg
property at their real addresses, and contains the Cadence USBSSP core
as a child node at the core 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.
Allow the #syscon-cells property on the system controller binding for
that.

Signed-off-by: Hongliang Yang <hongliang.yang@cixtech.com>
---
Changes in v6:
- Describe the glue registers at their real addresses instead of
  wrapping them in a simple-bus container with translated offsets.
- Document the register offset and bit position arguments of the
  cix,usb-syscon phandle and allow #syscon-cells on the S5 system
  controller.
- Document the ssp-disable and u3-port-disable properties read by the
  glue driver.
- Fix the property order reported by sashiko.
---
 .../soc/cix/cix,sky1-system-control.yaml      |   3 +
 .../bindings/usb/cix,sky1-usbssp.yaml         | 155 ++++++++++++++++++
 MAINTAINERS                                   |   2 +
 3 files changed, 160 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml

diff --git a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
index a01a515222c6..d19ef4e3cac0 100644
--- a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
+++ b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
@@ -27,6 +27,9 @@ properties:
   '#reset-cells':
     const: 1
 
+  '#syscon-cells':
+    const: 2
+
 required:
   - compatible
   - reg
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 000000000000..621526d32d7c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
@@ -0,0 +1,155 @@
+# 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. The integration adds per-controller glue registers in the
+  SoC RCSU (reset control and status unit), an AXI bus attribute
+  configuration register and a controller status register, both mapped
+  by the reg property. 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.
+
+properties:
+  compatible:
+    const: cix,sky1-usbssp
+
+  reg:
+    items:
+      - description: AXI bus attribute configuration register
+      - description: Controller status register
+
+  reg-names:
+    items:
+      - const: axi
+      - const: status
+
+  ranges: true
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 2
+
+  clocks:
+    items:
+      - description: Start-of-frame clock
+      - description: AXI bus clock
+      - description: Low-power mode clock
+      - description: APB register interface clock
+
+  clock-names:
+    items:
+      - const: sof
+      - const: axi
+      - const: lpm
+      - const: apb
+
+  resets:
+    items:
+      - description: APB register reset
+      - description: Controller reset
+
+  reset-names:
+    items:
+      - const: apb
+      - const: core
+
+  ssp-disable:
+    type: boolean
+    description: Disable SuperSpeed Plus mode of the controller.
+
+  u3-port-disable:
+    type: boolean
+    description: The board does not route the USB3 port of the controller.
+
+  cix,usb-syscon:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Phandle to the Sky1 S5 system controller followed by the register
+      offset and the bit position of the USB mode strap field.
+    items:
+      - items:
+          - description: Phandle to the Sky1 S5 system controller
+          - description: Register offset of the USB mode strap field
+          - description: Bit position of the USB mode strap field
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - ranges
+  - "#address-cells"
+  - "#size-cells"
+  - clocks
+  - clock-names
+  - resets
+  - reset-names
+  - cix,usb-syscon
+
+patternProperties:
+  "^usb@[0-9a-f]+$":
+    $ref: cdns,usb3.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/cix,sky1.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/reset/cix,sky1-s5-system-control.h>
+
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        usb@91c0314 {
+            compatible = "cix,sky1-usbssp";
+            reg = <0x00 0x91c0314 0x00 0x10>,
+                  <0x00 0x91c0400 0x00 0x10>;
+            reg-names = "axi", "status";
+            ranges;
+            #address-cells = <2>;
+            #size-cells = <2>;
+
+            clocks = <&scmi_clk CLK_TREE_USB3A_H0_CLK_SOF>,
+                     <&scmi_clk CLK_TREE_USB3A_0_AXI_GATE>,
+                     <&scmi_clk CLK_TREE_USB3A_H0_CLK_LPM>,
+                     <&scmi_clk CLK_TREE_USB3A_0_APB_GATE>;
+            clock-names = "sof", "axi", "lpm", "apb";
+            resets = <&src SKY1_USBC_SS2_PRST_N>,
+                     <&src SKY1_USBC_SS2_RST_N>;
+            reset-names = "apb", "core";
+            cix,usb-syscon = <&src 0x424 8>;
+
+            usb@91d0000 {
+                compatible = "cdns,usb3";
+                reg = <0x00 0x91d0000 0x00 0x4000>,
+                      <0x00 0x91d8000 0x00 0x8000>,
+                      <0x00 0x91d4000 0x00 0x4000>;
+                reg-names = "otg", "xhci", "dev";
+                dr_mode = "otg";
+
+                interrupts = <GIC_SPI 168 IRQ_TYPE_LEVEL_HIGH>,
+                             <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>,
+                             <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>,
+                             <GIC_SPI 171 IRQ_TYPE_LEVEL_HIGH>;
+                interrupt-names = "host", "peripheral", "otg", "wakeup";
+                maximum-speed = "super-speed-plus";
+
+                phys = <&usb3_phy4_0>, <&usb2_phy6>;
+                phy-names = "cdns3,usb3-phy", "cdns3,usb2-phy";
+            };
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index b857e529e5e2..92ff4ccff791 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2904,9 +2904,11 @@ T:	git https://github.com/cixtech/linux-mainline.git
 F:	Documentation/devicetree/bindings/arm/cix.yaml
 F:	Documentation/devicetree/bindings/mailbox/cix,sky1-mbox.yaml
 F:	Documentation/devicetree/bindings/phy/cix,sky1-usb3-phy.yaml
+F:	Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
 F:	arch/arm64/boot/dts/cix/
 F:	drivers/mailbox/cix-mailbox.c
 F:	drivers/phy/cix/
+F:	drivers/usb/cdns3/cdnsp-sky1.*
 K:	\bcix\b
 
 ARM/CLKDEV SUPPORT

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] dt-bindings: soc: cix: non-standard #syscon-cells property in system-control
- [Low] dt-bindings: usb: cix,sky1-usbssp: redundant unprefixed disable properties
--

commit 6118aaded5d0bbade230e227b29df8b8fcf97408
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 CIX Sky1 Cadence
USBSSP controller and updates the CIX Sky1 system controller binding and
MAINTAINERS file.

> diff --git a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
> index a01a515222c6d..d19ef4e3cac02 100644
> --- a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
> +++ b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-control.yaml
> @@ -27,6 +27,9 @@ properties:
>    '#reset-cells':
>      const: 1
>  
> +  '#syscon-cells':
> +    const: 2
> +

[Severity: Low]
Should #syscon-cells be dropped from cix,sky1-system-control.yaml?

A syscon node represents a miscellaneous collection of registers rather than a
provider class with a fixed specifier cell count, and different consumers of
 the same syscon node may take 0, 1, 2, or more arguments. In
drivers/mfd/syscon.c, syscon_regmap_lookup_by_phandle_args() parses phandle
arguments with a fixed count passed by the caller rather than querying a
#syscon-cells property on the provider node:

drivers/mfd/syscon.c:syscon_regmap_lookup_by_phandle_args() {
	...
	rc = of_parse_phandle_with_fixed_args(np, property, arg_count,
			0, &args);
	...
}

No other binding under Documentation/devicetree/bindings/ defines
#syscon-cells, and cix,usb-syscon in cix,sky1-usbssp.yaml already specifies
and constrains its two arguments via its phandle-array items schema.

> 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..621526d32d7c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/cix,sky1-usbssp.yaml
[ ... ]
> +  reset-names:
> +    items:
> +      - const: apb
> +      - const: core
> +
> +  ssp-disable:
> +    type: boolean
> +    description: Disable SuperSpeed Plus mode of the controller.
> +
> +  u3-port-disable:
> +    type: boolean
> +    description: The board does not route the USB3 port of the controller.

[Severity: Low]
Do ssp-disable and u3-port-disable on the parent glue node duplicate standard
USB properties that are already available on the child cdns,usb3 controller
node?

In Documentation/devicetree/bindings/usb/cdns,usb3.yaml (and usb.yaml), speed
limits and available PHYs are already described on the child node:

  maximum-speed:
    enum: [super-speed-plus, super-speed, high-speed, full-speed]

  phys:
    minItems: 1
    maxItems: 2

The example in this binding already sets maximum-speed and phys on the child
usb@91d0000 node. Adding custom unprefixed ssp-disable and u3-port-disable
properties on the wrapper node creates two places to describe the same
hardware configuration. Could the glue driver inspect the child node's
maximum-speed and phys properties instead of introducing these two properties?

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

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

[Krzysztof Kozlowski]

On 05/11/2026 02:18, Hongliang Yang (杨宏亮) wrote:
> Add device tree binding for the CIX Sky1 Cadence USBSSP controller.
> Each controller has an AXI bus attribute configuration register and a
> controller status register inside the SoC RCSU, mapped by the reg
> property at their real addresses, and contains the Cadence USBSSP core
> as a child node at the core 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.
> Allow the #syscon-cells property on the system controller binding for
> that.
> 
> Signed-off-by: Hongliang Yang <hongliang.yang@cixtech.com>
> ---
> Changes in v6:

NAK, you did not respond, did not implement feedback or part of the
feedback.

Best regards,
Krzysztof

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

[Hongliang Yang]

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

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

[Krzysztof Kozlowski]

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

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

[Hongliang Yang]

On 06/11/2026 08:01, Krzysztof Kozlowski wrote:
> So instead of replying under each comment, you selected a few of them
> and expect me to map/figure out which ones you ignored?

No. Compressing eight separate comments into one mail was the wrong way
to answer a review, and it is what made you match numbered paragraphs to
quotes yourself. I will answer under the individual messages instead.

> 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."

The first one, in the v1 thread:

  https://lore.kernel.org/all/20260902032306.1182941-1-hongliang.yang@cixtech.com/

where I wrote "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". That answer was only half true: v2
split the bindings into separate patches, but the PHY and the USB series
were still submitted together. The series were actually split in v4, and
have been separate since:

  [PATCH v4 0/3] Add CIX Sky1 USB3 PHY support
    <20260906032431.470744-1-hongliang.yang@cixtech.com>
  [PATCH v4 0/4] Add CIX Sky1 USBSSP controller support
    <20260906032440.470788-1-hongliang.yang@cixtech.com>

The second one, on the v3 cover (6ca4ba08): I did not reply to it. The
split went into v4 with no mail saying so. There is no reply I can point
you to for that message, and I am not going to claim there is.

What remains are the per-patch comments on the binding that my previous
mail answered in summary form. I will reply to those under your original
messages, one at a time, rather than restate them here.

Hongliang

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

[Krzysztof Kozlowski]

On 06/11/2026 08:28, Hongliang Yang wrote:
> On 06/11/2026 08:01, Krzysztof Kozlowski wrote:
>> So instead of replying under each comment, you selected a few of them
>> and expect me to map/figure out which ones you ignored?
> 
> No. Compressing eight separate comments into one mail was the wrong way
> to answer a review, and it is what made you match numbered paragraphs to
> quotes yourself. I will answer under the individual messages instead.
> 
>> 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."
> 
> The first one, in the v1 thread:
> 
>   https://lore.kernel.org/all/20260902032306.1182941-1-hongliang.yang@cixtech.com/
> 
> where I wrote "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". That answer was only half true: v2
> split the bindings into separate patches, but the PHY and the USB series
> were still submitted together. The series were actually split in v4, and
> have been separate since:
> 
>   [PATCH v4 0/3] Add CIX Sky1 USB3 PHY support
>     <20260906032431.470744-1-hongliang.yang@cixtech.com>
>   [PATCH v4 0/4] Add CIX Sky1 USBSSP controller support
>     <20260906032440.470788-1-hongliang.yang@cixtech.com>
> 
> The second one, on the v3 cover (6ca4ba08): I did not reply to it. The
> split went into v4 with no mail saying so. There is no reply I can point
> you to for that message, and I am not going to claim there is.
> 
> What remains are the per-patch comments on the binding that my previous
> mail answered in summary form. I will reply to those under your original
> messages, one at a time, rather than restate them here.

You still combine USB with other subsystem. Identify subsystems here,
please.
1. USB
2. SoC, no?

DTS cannot be placed here.


Best regards,
Krzysztof

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

[Hongliang Yang]

On 06/11/2026 08:39, Krzysztof Kozlowski wrote:
> You still combine USB with other subsystem. Identify subsystems here,
> please.
> 1. USB
> 2. SoC, no?
> DTS cannot be placed here.

Yes:
1. USB - the binding and the glue driver.
2. SoC - the arm64 device tree.

You are right, the dts does not belong in a USB series. v7 will be the
two USB patches only:

  dt-bindings: usb: Add CIX Sky1 USBSSP controller
  usb: cdns3: Add CIX Sky1 glue driver

and the arm64 dts will be sent separately once the binding is accepted,
so that it is not mixed into this series and does not hold it up.

Hongliang

[PATCH v6 2/3] usb: cdns3: Add CIX Sky1 glue driver

[Hongliang Yang (杨宏亮)]

Add a glue layer driver for the Cadence USBSSP controller integrated in
the CIX Sky1 SoC. The driver manages the controller clocks, resets, and
the mode strap controls located in the Sky1 S5 system controller. The
per-controller glue registers, the AXI bus attribute configuration
register and the controller status register, are mapped directly
through the reg property of the controller node.

Signed-off-by: Hongliang Yang <hongliang.yang@cixtech.com>
---
Changes in v6:
- Read the USB mode strap register offset and bit position from the
  cix,usb-syscon phandle arguments instead of relying on the usbN
  aliases and the undocumented id property.
- Use dev_err_probe() in the probe error paths.
- Drop the unused over-current GPIO handling and the unused dr_base and
  cdnsp_pdev fields.
- Do not disable the clocks before they have been acquired.
- Drop the redundant depends on USB_CDNS3 and fix the compatible name
  in the Kconfig help text.
---
 drivers/usb/cdns3/Kconfig      |  13 +
 drivers/usb/cdns3/Makefile     |   1 +
 drivers/usb/cdns3/cdnsp-sky1.c | 680 +++++++++++++++++++++++++++++++++
 drivers/usb/cdns3/cdnsp-sky1.h | 100 +++++
 4 files changed, 794 insertions(+)
 create mode 100644 drivers/usb/cdns3/cdnsp-sky1.c
 create mode 100644 drivers/usb/cdns3/cdnsp-sky1.h

diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
index 39ad23d1ada8..678cedd8de97 100644
--- a/drivers/usb/cdns3/Kconfig
+++ b/drivers/usb/cdns3/Kconfig
@@ -77,6 +77,19 @@ config USB_CDNSP_PCI
 	  If you choose to build this driver as module it will
 	  be dynamically linked and module will be called cdnsp-pci.ko
 
+config USB_CDNSP_SKY1
+	tristate "Cadence USB3 support on CIX Sky1 SoC platforms"
+	depends on ARCH_CIX || COMPILE_TEST
+	default USB_CDNS3
+	help
+	  Glue driver for the Cadence USB dual-role controllers on CIX Sky1
+	  (device tree compatible cix,sky1-usbssp). It enables clocks and
+	  resets from the SoC, then uses the shared cdns3 platform core
+	  (cdns.ko).
+
+	  If built as a module, the module is named cdnsp-sky1.ko and must be
+	  loaded after the cdns core module when both are loadable modules.
+
 config USB_CDNS3_TI
 	tristate "Cadence USB3 support on TI platforms"
 	depends on ARCH_K3 || COMPILE_TEST
diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
index b2e4ba6a49a3..ab813aaf9940 100644
--- a/drivers/usb/cdns3/Makefile
+++ b/drivers/usb/cdns3/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_USB_CDNSP_PCI)			+= cdnsp-pci.o
 obj-$(CONFIG_USB_CDNS3_TI)			+= cdns3-ti.o
 obj-$(CONFIG_USB_CDNS3_IMX)			+= cdns3-imx.o
 obj-$(CONFIG_USB_CDNS3_STARFIVE)		+= cdns3-starfive.o
+obj-$(CONFIG_USB_CDNSP_SKY1)			+= cdnsp-sky1.o
diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
new file mode 100644
index 000000000000..ca1a37c61baa
--- /dev/null
+++ b/drivers/usb/cdns3/cdnsp-sky1.c
@@ -0,0 +1,680 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cdnsp-sky1.c - CIX Sky1 glue for Cadence USBSSP DRD controller
+ *
+ * Copyright (C) 2026 CIX Technology Group Co., Ltd.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+#include "cdnsp-sky1.h"
+#include "core.h"
+
+static const char *cix_usb_clk_names[CIX_USB_CLK_NUM] = {
+	"sof",
+	"axi",
+	"lpm",
+	"apb",
+};
+
+static int cdnsp_sky1_set_mode(struct device *dev, int mode)
+{
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+
+	return regmap_update_bits(data->usb_syscon, data->strap_offset,
+				  GENMASK(data->strap_bit + 1, data->strap_bit),
+				  mode << data->strap_bit);
+}
+
+/**
+ * cdnsp_sky1_clk_enable_all() - enable all clocks for usb controller
+ * @dev:	Pointer to the device of platform_device
+ *
+ */
+
+static int cdnsp_sky1_clk_enable_all(struct device *dev)
+{
+	int i, ret = 0;
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+	struct clk **cix_usb_clks = data->cix_usb_clks;
+
+	for (i = 0; i < CIX_USB_CLK_NUM; i++) {
+		cix_usb_clks[i] = devm_clk_get(dev, cix_usb_clk_names[i]);
+		if (IS_ERR(cix_usb_clks[i])) {
+			ret = dev_err_probe(dev, PTR_ERR(cix_usb_clks[i]),
+					    "could not get %s clock\n",
+					    cix_usb_clk_names[i]);
+			goto err_usb_clks;
+		}
+		ret = clk_prepare_enable(cix_usb_clks[i]);
+		if (ret) {
+			dev_err(dev, "%s enable failed:%d\n", cix_usb_clk_names[i], ret);
+			goto err_usb_clks;
+		}
+	}
+	dev_dbg(dev, "enable sky1 USB clock done\n");
+	return ret;
+
+err_usb_clks:
+	while (--i >= 0)
+		clk_disable_unprepare(cix_usb_clks[i]);
+	return ret;
+};
+
+/**
+ * cdnsp_sky1_clk_disable_all() - disable all clocks for usb controller
+ * @dev:	Pointer to the device of platform_device
+ *
+ */
+
+static void cdnsp_sky1_clk_disable_all(struct device *dev)
+{
+	int i;
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+	struct clk **cix_usb_clks = data->cix_usb_clks;
+
+	for (i = 0; i < CIX_USB_CLK_NUM; i++)
+		clk_disable_unprepare(cix_usb_clks[i]);
+};
+
+/**
+ * cdnsp_sky1_clk_enable_resume() - enable the clocks that are turned
+ * off while suspend
+ * @dev:	Pointer to the device of platform_device
+ *
+ */
+
+static int cdnsp_sky1_clk_enable_resume(struct device *dev)
+{
+	int i, ret;
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+	struct clk **cix_usb_clks = data->cix_usb_clks;
+
+	for (i = 0; i < CIX_USB_CLK_OFF_NUM; i++) {
+		ret = clk_prepare_enable(cix_usb_clks[i]);
+		if (ret) {
+			dev_err(dev, "failed to enable clock %s: %d\n",
+				cix_usb_clk_names[i], ret);
+			goto err_usb_clks;
+		}
+	}
+	return 0;
+
+err_usb_clks:
+	while (--i >= 0)
+		clk_disable_unprepare(cix_usb_clks[i]);
+	return ret;
+};
+
+/**
+ * cdnsp_sky1_clk_disable_suspend() - disable the clocks which are not
+ * needed when suspend
+ * @dev:	Pointer to the device of platform_device
+ *
+ */
+
+static void cdnsp_sky1_clk_disable_suspend(struct device *dev)
+{
+	int i;
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+	struct clk **cix_usb_clks = data->cix_usb_clks;
+
+	for (i = 0; i < CIX_USB_CLK_OFF_NUM; i++)
+		clk_disable_unprepare(cix_usb_clks[i]);
+};
+
+static int sky1_rcsu_handshake(struct cdnsp_sky1 *data, u32 mask, u32 done,
+			       u64 timeout_us)
+{
+	u32	result;
+	int	ret;
+
+	ret = readl_poll_timeout(data->rcsu_ctst_base, result,
+				 (result & mask) == done ||
+				 result == U32_MAX,
+				 10, timeout_us);
+	if (result == U32_MAX)		/* card removed */
+		return -ENODEV;
+	return ret;
+}
+
+static int cdns_sky1_platform_suspend(struct device *dev,
+				      bool suspend, bool wakeup)
+{
+	struct cdns *cdns = dev_get_drvdata(dev);
+	struct platform_device *xhci_dev = cdns->host_dev;
+	struct usb_hcd  *hcd;
+	struct device *parent = cdns->dev->parent;
+	struct cdnsp_sky1 *data = dev_get_drvdata(parent);
+	u32 value;
+	int poll;
+	int ret = 0;
+	int count = 3;
+
+	data->wakeup = wakeup;
+
+	if (cdns->role != USB_ROLE_HOST)
+		return 0;
+
+	if (!xhci_dev) {
+		dev_dbg(dev, "host controller has not been registered\n");
+		return 0;
+	}
+
+	hcd = dev_get_drvdata(&xhci_dev->dev);
+	if (!hcd) {
+		dev_dbg(dev, "host controller have not registered\n");
+		return 0;
+	}
+
+	if (suspend) {
+		while (count--) {
+			value = readl(hcd->regs + XECP_PM_PMCSR);
+			value &= ~PS_MASK;
+			value |= PS_D3 | PS_PME_En;
+			writel(value, hcd->regs + XECP_PM_PMCSR);
+			/* After controller enters D3, disable AXI and SOF
+			 * until AXI valid flag changes to 0.
+			 */
+			if (sky1_rcsu_handshake(data, AXI_CLOCK_VALID, 0,
+						100ULL * 1000)) {
+				u32 ctst;
+
+				ctst = readl(data->rcsu_ctst_base);
+				dev_dbg(dev, "enter D3 failed, register value:%x\n",
+					ctst);
+			} else {
+				break;
+			}
+		}
+		if (count < 0) {
+			u32 ctst;
+
+			ctst = readl(data->rcsu_ctst_base);
+			dev_err(dev, "enter D3 failed after retries, register value:%x\n",
+				ctst);
+		}
+	} else {
+		while (count--) {
+			value = readl(hcd->regs + XECP_PM_PMCSR);
+			value &= ~PS_MASK;
+			value |= PS_D0;
+			value &= ~PS_PME_En;
+			writel(value, hcd->regs + XECP_PM_PMCSR);
+			/* Wait power state back to D0 */
+			poll = readl_poll_timeout(hcd->regs + XECP_PM_PMCSR,
+						  value,
+						  (value & PS_MASK) == 0 ||
+						  value == U32_MAX,
+						  10, 100ULL * 1000);
+			if (poll) {
+				dev_dbg(dev, "exit D3 timeout, power state=0x%lx\n",
+					value & PS_MASK);
+			} else {
+				break;
+			}
+		}
+		if (count < 0) {
+			dev_err(dev, "exit D3 timeout after retries, power state=0x%lx\n",
+				value & PS_MASK);
+		}
+	}
+
+	return ret;
+}
+
+static void cdnsp_sky1_configure_controller(struct cdnsp_sky1 *data)
+{
+	int clk;
+	int v0, v1, v2, v3;
+	u32 dev_cap, xhci_cap;
+	u32 val = 0;
+
+	if (data->u3_disable) {
+		dev_dbg(data->dev, "disable u3 port\n");
+		writel(D_XEC_CFG_3XPORT_MODE_VALUE, data->device_base
+			+ D_XEC_CFG_3XPORT_MODE);
+	}
+	dev_cap = readl(data->device_base + D_XEC_AXI_CAP);
+	xhci_cap = readl(data->xhci_base + D_XEC_AXI_CAP);
+	writel(dev_cap | AXI_HALT, data->device_base + D_XEC_AXI_CAP);
+	writel(xhci_cap | AXI_HALT, data->xhci_base + D_XEC_AXI_CAP);
+	writel(data->axi_bmax_value, data->device_base + D_XEC_AXI_CTRL0);
+	writel(data->axi_bmax_value, data->xhci_base + D_XEC_AXI_CTRL0);
+	writel(dev_cap & ~AXI_HALT, data->device_base + D_XEC_AXI_CAP);
+	writel(xhci_cap & ~AXI_HALT, data->xhci_base + D_XEC_AXI_CAP);
+	clk = data->sof_clk_freq;
+	v0 =  25 * clk / 100000000;
+	v1 = clk / 10000;
+	v2 = clk / 10;
+	v3 = 125 * clk / 1000000;
+	writel(((v0 > 1) ? v0 - 1 : 1), data->device_base
+		+ D_XEC_PRE_REG_250NS);
+	writel((unsigned int)((v1 / 100 > 1) > 0 ? (v1 / 100) - 1 : 1),
+	       data->device_base + D_XEC_PRE_REG_1US);
+	writel((unsigned int)((v1 / 10 > 1) > 0 ? (v1 / 10) - 1 : 1),
+	       data->device_base + D_XEC_PRE_REG_10US);
+	writel(((v1) > 1 ? v1 - 1 : 1), data->device_base
+		+ D_XEC_PRE_REG_100US);
+	writel((unsigned int)(v3 > 1 ? v3 - 1 : 1),
+	       data->device_base + D_XEC_PRE_REG_125US);
+	writel(((v2 / 100 > 1) ? (v2 / 100) - 1 : 1), data->device_base
+		+ D_XEC_PRE_REG_1MS);
+	writel(((v2 / 10 > 1) ? (v2 / 10) - 1 : 1), data->device_base
+		+ D_XEC_PRE_REG_10MS);
+	writel((v2 > 1 ? v2 - 1 : 1), data->device_base
+		+ D_XEC_PRE_REG_100MS);
+	dev_dbg(data->dev, "readl:%x, %x ,%x, %x, %x, %x, %x, %x\n",
+		readl(data->device_base + D_XEC_PRE_REG_250NS),
+		readl(data->device_base + D_XEC_PRE_REG_1US),
+		readl(data->device_base + D_XEC_PRE_REG_10US),
+		readl(data->device_base + D_XEC_PRE_REG_100US),
+		readl(data->device_base + D_XEC_PRE_REG_125US),
+		readl(data->device_base + D_XEC_PRE_REG_1MS),
+		readl(data->device_base + D_XEC_PRE_REG_10MS),
+		readl(data->device_base + D_XEC_PRE_REG_100MS));
+	clk = data->lpm_clk_freq;
+	v0 =  25 * clk / 100000000;
+	v1 = clk / 10000;
+	v2 = clk / 10;
+	v3 = 125 * clk / 1000000;
+	writel(((v0 > 1) ? v0 - 1 : 1), data->device_base
+		+ D_XEC_LPM_PRE_REG_250NS);
+	writel((unsigned int)((v1 / 100 > 1) > 0 ? (v1 / 100) - 1 : 1), data->device_base
+		+ D_XEC_LPM_PRE_REG_1US);
+	writel((unsigned int)((v1 / 10 > 1) > 0 ? (v1 / 10) - 1 : 1), data->device_base
+		+ D_XEC_LPM_PRE_REG_10US);
+	writel(((v1) > 1 ? v1 - 1 : 1), data->device_base
+		+ D_XEC_LPM_PRE_REG_100US);
+	writel((unsigned int)(v3 > 1 ? v3 - 1 : 1),
+	       data->device_base + D_XEC_LPM_PRE_REG_125US);
+	writel(((v2 / 100 > 1) ? (v2 / 100) - 1 : 1), data->device_base
+		+ D_XEC_LPM_PRE_REG_1MS);
+	writel(((v2 / 10 > 1) ? (v2 / 10) - 1 : 1), data->device_base
+		+ D_XEC_LPM_PRE_REG_10MS);
+	writel((v2 > 1 ? v2 - 1 : 1), data->device_base
+		+ D_XEC_LPM_PRE_REG_100MS);
+	v0 = readl(data->xhci_base + XEC_USBSSP_CHICKEN_BITS_3);
+	v0 &= ~(CFG_APB_TIMEOUT_PSLVERR_EN | CFG_APB_PSLVERR_EN);
+	writel(v0, data->xhci_base + XEC_USBSSP_CHICKEN_BITS_3);
+	if (data->u3_disable) {
+		dev_dbg(data->dev, "disable u3 port\n");
+		writel(XEC_CFG_3XPORT_MODE_VALUE, data->xhci_base
+			+ XEC_CFG_3XPORT_MODE);
+	} else if (data->ssp_disable) {
+		dev_dbg(data->dev, "disable ssp\n");
+		v0 = readl(data->xhci_base + XEC_CFG_3XPORT_MODE);
+		writel(v0  & CFG_3XPORT_MODE_DIS_SSP, data->xhci_base
+			+ XEC_CFG_3XPORT_MODE);
+	}
+	clk = data->sof_clk_freq;
+	v0 =  25 * clk / 100000000;
+	v1 = clk / 10000;
+	v2 = clk / 10;
+	v3 = 125 * clk / 1000000;
+	writel(((v0 > 1) ? v0 - 1 : 0), data->xhci_base
+		+ XEC_PRE_REG_250NS);
+	writel((unsigned int)((v1 / 100 > 1) > 0 ? (v1 / 100) - 1 : 0), data->xhci_base
+		+ XEC_PRE_REG_1US);
+	writel((unsigned int)((v1 / 10 > 1) > 0 ? (v1 / 10) - 1 : 0), data->xhci_base
+		+ XEC_PRE_REG_10US);
+	writel(((v1) > 1 ? v1 - 1 : 0), data->xhci_base
+		+ XEC_PRE_REG_100US);
+	writel((unsigned int)(v3 > 1 ? v3 - 1 : 0),
+	       data->xhci_base + XEC_PRE_REG_125US);
+	writel(((v2 / 100 > 1) ? (v2 / 100) - 1 : 0), data->xhci_base
+		+ XEC_PRE_REG_1MS);
+	writel(((v2 / 10 > 1) ? (v2 / 10) - 1 : 0), data->xhci_base
+		+ XEC_PRE_REG_10MS);
+	writel((v2 > 1 ? v2 - 1 : 0), data->xhci_base
+		+ XEC_PRE_REG_100MS);
+	clk = data->lpm_clk_freq;
+	v0 =  25 * clk / 100000000;
+	v1 = clk / 10000;
+	v2 = clk / 10;
+	v3 = 125 * clk / 1000000;
+	writel(((v0 > 1) ? v0 - 1 : 0), data->xhci_base
+		+ XEC_LPM_PRE_REG_250NS);
+	writel((unsigned int)((v1 / 100 > 1) > 0 ? (v1 / 100) - 1 : 0), data->xhci_base
+		+ XEC_LPM_PRE_REG_1US);
+	writel((unsigned int)((v1 / 10 > 1) > 0 ? (v1 / 10) - 1 : 0), data->xhci_base
+		+ XEC_LPM_PRE_REG_10US);
+	writel(((v1) > 1 ? v1 - 1 : 0), data->xhci_base
+		+ XEC_LPM_PRE_REG_100US);
+	writel((unsigned int)(v3 > 1 ? v3 - 1 : 0),
+	       data->xhci_base + XEC_LPM_PRE_REG_125US);
+	writel(((v2 / 100 > 1) ? (v2 / 100) - 1 : 0), data->xhci_base
+		+ XEC_LPM_PRE_REG_1MS);
+	writel(((v2 / 10 > 1) ? (v2 / 10) - 1 : 0), data->xhci_base
+		+ XEC_LPM_PRE_REG_10MS);
+	writel((v2 > 1 ? v2 - 1 : 0), data->xhci_base
+		+ XEC_LPM_PRE_REG_100MS);
+	val = readl(data->xhci_base + XEC_USBSSP_CLK_GATING_CTRL);
+	val |= HOST20_ACLK_GATING_DISABLE | HOST20_UTMI_GATING_DISABLE;
+	writel(val, data->xhci_base + XEC_USBSSP_CLK_GATING_CTRL);
+}
+
+static int cdnsp_sky1_drd_init(struct cdnsp_sky1 *data)
+{
+	int ret = 0;
+
+	reset_control_assert(data->reset);
+	reset_control_assert(data->preset);
+	ret = cdnsp_sky1_clk_enable_all(data->dev);
+	if (ret)
+		return ret;
+	writel(CIX_USB_AXI_WR_CACHE_VALUE, data->rcsu_axi_base);
+	cdnsp_sky1_set_mode(data->dev, MODE_STRAP_OTG);
+	reset_control_deassert(data->preset);
+	cdnsp_sky1_configure_controller(data);
+	reset_control_deassert(data->reset);
+	return ret;
+}
+
+static void __iomem *sky1_of_get_addr_by_name(struct device_node *parent, const char *name)
+{
+	void __iomem *addr = NULL;
+	struct device_node *node;
+	int index;
+
+	for_each_child_of_node(parent, node) {
+		index = of_property_match_string(node, "reg-names", name);
+		if (index >= 0) {
+			addr = of_iomap(node, index);
+			of_node_put(node);
+			break;
+		}
+	}
+
+	return addr;
+}
+
+static void __iomem *sky1_get_addr_by_name(struct device *dev, const char *name)
+{
+	return sky1_of_get_addr_by_name(dev->of_node, name);
+}
+
+static void sky1_put_addr(void __iomem *regs)
+{
+	if (regs)
+		iounmap(regs);
+}
+
+static int cdnsp_sky1_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node = dev->of_node;
+	struct of_dev_auxdata cdns_sky1_auxdata[] = {
+		{ .compatible = "cdns,usb3", },
+		{},
+	};
+	struct cdnsp_sky1 *data;
+	struct cdns3_platform_data *cdns_sky1_pdata;
+	unsigned int args[2];
+	int ret = 0;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	data->rcsu_axi_base = devm_platform_ioremap_resource_byname(pdev, "axi");
+	if (IS_ERR(data->rcsu_axi_base))
+		return PTR_ERR(data->rcsu_axi_base);
+	data->rcsu_ctst_base = devm_platform_ioremap_resource_byname(pdev, "status");
+	if (IS_ERR(data->rcsu_ctst_base))
+		return PTR_ERR(data->rcsu_ctst_base);
+	data->reset = devm_reset_control_get_exclusive(&pdev->dev, "core");
+	if (IS_ERR(data->reset))
+		return dev_err_probe(dev, PTR_ERR(data->reset),
+				     "failed to get core reset\n");
+	data->preset = devm_reset_control_get_exclusive(&pdev->dev, "apb");
+	if (IS_ERR(data->preset))
+		return dev_err_probe(dev, PTR_ERR(data->preset),
+				     "failed to get APB reset\n");
+	platform_set_drvdata(pdev, data);
+	data->dev = dev;
+	data->usb_syscon =
+		syscon_regmap_lookup_by_phandle_args(node, "cix,usb-syscon",
+						     2, args);
+	if (IS_ERR(data->usb_syscon))
+		return dev_err_probe(dev, PTR_ERR(data->usb_syscon),
+				     "failed to get S5 system controller\n");
+	data->strap_offset = args[0];
+	data->strap_bit = args[1];
+	data->u3_disable = device_property_read_bool(dev, "u3-port-disable");
+	data->ssp_disable = device_property_read_bool(dev, "ssp-disable");
+	data->sof_clk_freq = CIX_USB_CLK_8M;
+	data->lpm_clk_freq = CIX_USB_CLK_32K;
+	data->axi_bmax_value = AXI_BMAX_VALUE_DEFAULT;
+	data->xhci_base = sky1_get_addr_by_name(dev, "xhci");
+	if (!data->xhci_base) {
+		ret = -ENODEV;
+		goto err_put_xhci;
+	}
+	data->device_base = sky1_get_addr_by_name(dev, "dev");
+	if (!data->device_base) {
+		ret = -ENODEV;
+		goto err_put_addrs;
+	}
+	ret = cdnsp_sky1_drd_init(data);
+	if (ret == -ETIMEDOUT)
+		ret = -EPROBE_DEFER;
+	if (ret) {
+		/*
+		 * drd_init() leaves the clocks disabled and the resets
+		 * asserted on failure; only the address mappings are
+		 * left to release.
+		 */
+		goto err_put_addrs;
+	}
+	/*
+	 * The platform data is devm-allocated, so it stays alive as long as
+	 * this device; the child devices are depopulated before it is freed.
+	 */
+	cdns_sky1_pdata = devm_kzalloc(dev, sizeof(*cdns_sky1_pdata), GFP_KERNEL);
+	if (!cdns_sky1_pdata) {
+		ret = -ENOMEM;
+		goto err_quiesce;
+	}
+	cdns_sky1_pdata->platform_suspend = cdns_sky1_platform_suspend;
+	cdns_sky1_pdata->quirks = CDNS3_DEFAULT_PM_RUNTIME_ALLOW;
+	cdns_sky1_auxdata[0].platform_data = cdns_sky1_pdata;
+	ret = of_platform_populate(node, NULL, cdns_sky1_auxdata, dev);
+	if (ret) {
+		dev_err(dev, "failed to create children: %d\n", ret);
+		goto err_quiesce;
+	}
+	device_set_wakeup_capable(dev, true);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+	return 0;
+
+err_quiesce:
+	reset_control_assert(data->reset);
+	reset_control_assert(data->preset);
+	cdnsp_sky1_clk_disable_all(dev);
+err_put_addrs:
+	sky1_put_addr(data->device_base);
+err_put_xhci:
+	sky1_put_addr(data->xhci_base);
+	return ret;
+}
+
+static void cdnsp_sky1_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(dev);
+	of_platform_depopulate(dev);
+	sky1_put_addr(data->xhci_base);
+	sky1_put_addr(data->device_base);
+	reset_control_assert(data->reset);
+	reset_control_assert(data->preset);
+	cdnsp_sky1_clk_disable_all(dev);
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_put_noidle(dev);
+}
+
+/*
+ * Because the wake-up interrupt and host interrupt are the same interrupt,
+ * closing the axi and sof clock will result in the inability to generate
+ * port status change interrupt.
+ */
+static int cdnsp_sky1_system_suspend(struct device *dev)
+{
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+
+	if (!data->wakeup) {
+		reset_control_assert(data->reset);
+		reset_control_assert(data->preset);
+	}
+	cdnsp_sky1_clk_disable_suspend(dev);
+	return 0;
+}
+
+static int cdnsp_sky1_system_resume(struct device *dev)
+{
+	int ret = 0;
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+
+	ret = cdnsp_sky1_clk_enable_resume(dev);
+	if (ret)
+		return ret;
+	if (!data->wakeup) {
+		writel(CIX_USB_AXI_WR_CACHE_VALUE, data->rcsu_axi_base);
+		cdnsp_sky1_set_mode(data->dev, MODE_STRAP_OTG);
+		reset_control_deassert(data->preset);
+		cdnsp_sky1_configure_controller(data);
+		reset_control_deassert(data->reset);
+	}
+	return 0;
+}
+
+static const struct dev_pm_ops cdnsp_sky1_pm_ops = {
+	SYSTEM_SLEEP_PM_OPS(cdnsp_sky1_system_suspend, cdnsp_sky1_system_resume)
+};
+
+static const struct of_device_id cdns_sky1_of_match[] = {
+	{ .compatible = "cix,sky1-usbssp", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cdns_sky1_of_match);
+
+static int cdnsp_sky1_find_cdns_dev(struct device *dev, void *data)
+{
+	struct device **cdns_dev = data;
+
+	if (dev->of_node && of_device_is_compatible(dev->of_node, "cdns,usb3")) {
+		/* keep a reference: the pointer is used after the iteration */
+		*cdns_dev = get_device(dev);
+		return 1;
+	}
+	return 0;
+}
+
+static int cdnsp_sky1_find_gadget_match(struct device *dev, void *data)
+{
+	struct device **gadget_dev = data;
+	const char *name = dev_name(dev);
+	static const char gadget_prefix[] = "gadget.";
+
+	/*
+	 * The gadget device is registered on the gadget bus with name
+	 * "gadget.%d" (see usb_add_gadget_udc -> dev_set_name).
+	 * It sits on the gadget bus and has the function driver bound to it.
+	 * Verify the device is on the gadget bus by checking the bus name.
+	 * This prevents matching devices that happen to have "gadget." prefix
+	 * in their name but are not real gadget devices, and also avoids
+	 * NULL pointer dereference when device bus is being removed.
+	 */
+	if (name && dev->bus && !strcmp(dev->bus->name, "gadget") &&
+	    !strncmp(name, gadget_prefix, sizeof(gadget_prefix) - 1)) {
+		/* keep a reference: the pointer is used after the iteration */
+		*gadget_dev = get_device(dev);
+		return 1;
+	}
+	return 0;
+}
+
+static void cdnsp_sky1_shutdown(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
+	struct device *cdns_dev = NULL;
+	struct device *gadget_dev = NULL;
+	struct cdns *cdns = NULL;
+
+	if (!device_may_wakeup(dev)) {
+		/*
+		 * Find the cdns3 child device, then find its gadget device
+		 * and release the function driver before disabling clocks.
+		 * This ensures all register accesses in gadget_unbind_driver
+		 * complete before clocks are turned off.
+		 */
+		device_for_each_child(dev, &cdns_dev, cdnsp_sky1_find_cdns_dev);
+		if (cdns_dev)
+			cdns = dev_get_drvdata(cdns_dev);
+		if (cdns)
+			device_for_each_child(cdns_dev, &gadget_dev,
+					      cdnsp_sky1_find_gadget_match);
+		if (gadget_dev)
+			device_release_driver(gadget_dev);
+		if (cdns && cdns->host_dev) {
+			struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
+
+			if (hcd && hcd->irq > 0) {
+				/*
+				 * Clear HCD_FLAG_HW_ACCESSIBLE before disable_irq.
+				 * This prevents usb_hcd_irq from calling xhci_irq
+				 * (which reads USBSTS) after clocks are disabled.
+				 */
+				clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+				disable_irq(hcd->irq);
+				synchronize_irq(hcd->irq);
+			}
+		}
+		reset_control_assert(data->reset);
+		reset_control_assert(data->preset);
+		cdnsp_sky1_clk_disable_all(dev);
+		put_device(gadget_dev);
+		put_device(cdns_dev);
+	}
+}
+
+static struct platform_driver cdnsp_sky1_driver = {
+	.probe		= cdnsp_sky1_probe,
+	.remove		= cdnsp_sky1_remove,
+	.shutdown	= cdnsp_sky1_shutdown,
+	.driver		= {
+		.name	= "cdnsp-sky1",
+		.of_match_table	= cdns_sky1_of_match,
+		.pm	= pm_sleep_ptr(&cdnsp_sky1_pm_ops),
+	},
+};
+
+module_platform_driver(cdnsp_sky1_driver);
+
+MODULE_ALIAS("platform:cdnsp-sky1");
+MODULE_DESCRIPTION("CIX Sky1 Cadence USBSSP DRD glue driver");
+MODULE_AUTHOR("Hongliang Yang <hongliang.yang@cixtech.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/usb/cdns3/cdnsp-sky1.h b/drivers/usb/cdns3/cdnsp-sky1.h
new file mode 100644
index 000000000000..3de99fbbbe03
--- /dev/null
+++ b/drivers/usb/cdns3/cdnsp-sky1.h
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _CDNSP_SKY1_H
+#define _CDNSP_SKY1_H
+#include <linux/clk.h>
+#include <linux/reset.h>
+#include <linux/types.h>
+
+#define MODE_STRAP_OTG                  0
+
+#define AXI_HALT                        BIT(31)
+#define AXI_BMAX_VALUE_DEFAULT          0x7
+
+#define D_XEC_CFG_3XPORT_MODE           0x2040
+#define D_XEC_AXI_CAP                   0x2174
+#define D_XEC_AXI_CTRL0                 0x217C
+#define D_XEC_PRE_REG_250NS             0x21E8
+#define D_XEC_PRE_REG_1US               0x21EC
+#define D_XEC_PRE_REG_10US              0x21F0
+#define D_XEC_PRE_REG_100US             0x21F4
+#define D_XEC_PRE_REG_125US             0x21F8
+#define D_XEC_PRE_REG_1MS               0x21FC
+#define D_XEC_PRE_REG_10MS              0x2200
+#define D_XEC_PRE_REG_100MS             0x2204
+#define D_XEC_LPM_PRE_REG_250NS         0x2208
+#define D_XEC_LPM_PRE_REG_1US           0x220C
+#define D_XEC_LPM_PRE_REG_10US          0x2210
+#define D_XEC_LPM_PRE_REG_100US         0x2214
+#define D_XEC_LPM_PRE_REG_125US         0x2218
+#define D_XEC_LPM_PRE_REG_1MS           0x221C
+#define D_XEC_LPM_PRE_REG_10MS          0x2220
+#define D_XEC_LPM_PRE_REG_100MS         0x2224
+
+#define XEC_CFG_3XPORT_MODE             0x2040
+#define XEC_PRE_REG_250NS               0x21E8
+#define XEC_PRE_REG_1US                 0x21EC
+#define XEC_PRE_REG_10US                0x21F0
+#define XEC_PRE_REG_100US               0x21F4
+#define XEC_PRE_REG_125US               0x21F8
+#define XEC_PRE_REG_1MS                 0x21FC
+#define XEC_PRE_REG_10MS                0x2200
+#define XEC_PRE_REG_100MS               0x2204
+#define XEC_LPM_PRE_REG_250NS           0x2208
+#define XEC_LPM_PRE_REG_1US             0x220C
+#define XEC_LPM_PRE_REG_10US            0x2210
+#define XEC_LPM_PRE_REG_100US           0x2214
+#define XEC_LPM_PRE_REG_125US           0x2218
+#define XEC_LPM_PRE_REG_1MS             0x221C
+#define XEC_LPM_PRE_REG_10MS            0x2220
+#define XEC_LPM_PRE_REG_100MS           0x2224
+#define XEC_USBSSP_CHICKEN_BITS_3       0x2230
+#define XEC_USBSSP_CLK_GATING_CTRL      0x2234
+#define XECP_PM_PMCSR                   0x2240
+
+/* XEC_USBSSP_CLK_GATING_CTRL */
+#define HOST20_ACLK_GATING_DISABLE      BIT(4)
+#define HOST20_UTMI_GATING_DISABLE      BIT(5)
+
+/* XECP_PM_PMCSR */
+#define PS_MASK                         GENMASK(1, 0)
+#define PS_D0                           0
+#define PS_D3                           3
+#define PS_PME_En                       BIT(8)
+
+#define D_XEC_CFG_3XPORT_MODE_VALUE     0xa0031e07
+#define XEC_CFG_3XPORT_MODE_VALUE       0xa0031e07
+#define CFG_3XPORT_MODE_DIS_SSP         (~BIT(31))
+
+#define CFG_APB_TIMEOUT_PSLVERR_EN      BIT(22)
+#define CFG_APB_PSLVERR_EN              BIT(23)
+
+#define CIX_USB_CLK_NUM                 (4)
+#define CIX_USB_CLK_OFF_NUM             (2)
+#define CIX_USB_AXI_WR_CACHE_VALUE      0x33
+#define CIX_USB_CLK_32K                 32000
+#define CIX_USB_CLK_8M                  8000000
+
+#define AXI_CLOCK_VALID                 BIT(1)
+
+struct cdnsp_sky1 {
+	struct device *dev;
+	void __iomem *rcsu_axi_base;
+	void __iomem *rcsu_ctst_base;
+	void __iomem *xhci_base;
+	void __iomem *device_base;
+	struct reset_control *reset;
+	struct reset_control *preset;
+	struct clk *cix_usb_clks[CIX_USB_CLK_NUM];
+	unsigned int strap_offset;
+	unsigned int strap_bit;
+	struct regmap	*usb_syscon;
+	int lpm_clk_freq;
+	int sof_clk_freq;
+	bool u3_disable;
+	bool ssp_disable;
+	int axi_bmax_value;
+	bool wakeup;
+};
+
+#endif /* _CDNSP_SKY1_H */

Re: [PATCH v6 2/3] usb: cdns3: Add CIX Sky1 glue driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] usb: cdns3: unclocked MMIO access and host_dev race in cdnsp_sky1_shutdown()
- [High] usb: cdns3: controller reset in cdnsp_sky1_system_suspend() breaks resume
- [Medium] usb: cdns3: ignored D3 errors and bad D0 poll in cdns_sky1_platform_suspend()
- [Medium] usb: cdns3: wakeup clocks disabled on suspend in cdnsp_sky1_system_suspend()
- [Medium] usb: cdns3: inconsistent wakeup checks between parent and child in cdnsp-sky1
- [Medium] usb: cdns3: clock disable underflow after failed cdnsp_sky1_system_resume()
- [Low] usb: cdns3: missing Return sections in cdnsp-sky1 kernel-doc comments
--

commit 0410eea3b97afaa677cd0ee5ca265bc4f7a11f31
Author: Hongliang Yang (杨宏亮) <hongliang.yang@cixtech.com>

usb: cdns3: Add CIX Sky1 glue driver

This commit adds a glue driver for the Cadence USBSSP dual-role controller
on the CIX Sky1 SoC, managing clocks, resets, mode strap controls in the
S5 system controller, and per-controller glue registers.

> diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> new file mode 100644
> index 0000000000000..ca1a37c61baa6
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c
> @@ -0,0 +1,680 @@

[ ... ]

> +/**
> + * cdnsp_sky1_clk_enable_all() - enable all clocks for usb controller
> + * @dev:	Pointer to the device of platform_device
> + *
> + */
> +
> +static int cdnsp_sky1_clk_enable_all(struct device *dev)
> +{

[ ... ]

> +};
> +
> +/**
> + * cdnsp_sky1_clk_disable_all() - disable all clocks for usb controller
> + * @dev:	Pointer to the device of platform_device
> + *
> + */
> +
> +static void cdnsp_sky1_clk_disable_all(struct device *dev)
> +{
> +	int i;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct clk **cix_usb_clks = data->cix_usb_clks;
> +
> +	for (i = 0; i < CIX_USB_CLK_NUM; i++)
> +		clk_disable_unprepare(cix_usb_clks[i]);
> +};
> +
> +/**
> + * cdnsp_sky1_clk_enable_resume() - enable the clocks that are turned
> + * off while suspend
> + * @dev:	Pointer to the device of platform_device
> + *
> + */
> +
> +static int cdnsp_sky1_clk_enable_resume(struct device *dev)

[Severity: Low]
This isn't a bug, but both cdnsp_sky1_clk_enable_all() and
cdnsp_sky1_clk_enable_resume() have kernel-doc comments without a Return:
section. Will scripts/kernel-doc emit warnings here when building with
W=1?

[ ... ]

> +static int cdns_sky1_platform_suspend(struct device *dev,
> +				      bool suspend, bool wakeup)
> +{
> +	struct cdns *cdns = dev_get_drvdata(dev);
> +	struct platform_device *xhci_dev = cdns->host_dev;
> +	struct usb_hcd  *hcd;
> +	struct device *parent = cdns->dev->parent;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(parent);
> +	u32 value;
> +	int poll;
> +	int ret = 0;
> +	int count = 3;
> +
> +	data->wakeup = wakeup;
> +
> +	if (cdns->role != USB_ROLE_HOST)
> +		return 0;
> +
> +	if (!xhci_dev) {
> +		dev_dbg(dev, "host controller has not been registered\n");
> +		return 0;
> +	}
> +
> +	hcd = dev_get_drvdata(&xhci_dev->dev);
> +	if (!hcd) {
> +		dev_dbg(dev, "host controller have not registered\n");
> +		return 0;
> +	}
> +
> +	if (suspend) {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D3 | PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* After controller enters D3, disable AXI and SOF
> +			 * until AXI valid flag changes to 0.
> +			 */
> +			if (sky1_rcsu_handshake(data, AXI_CLOCK_VALID, 0,
> +						100ULL * 1000)) {
> +				u32 ctst;
> +
> +				ctst = readl(data->rcsu_ctst_base);
> +				dev_dbg(dev, "enter D3 failed, register value:%x\n",
> +					ctst);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			u32 ctst;
> +
> +			ctst = readl(data->rcsu_ctst_base);
> +			dev_err(dev, "enter D3 failed after retries, register value:%x\n",
> +				ctst);
> +		}
> +	} else {
> +		while (count--) {
> +			value = readl(hcd->regs + XECP_PM_PMCSR);
> +			value &= ~PS_MASK;
> +			value |= PS_D0;
> +			value &= ~PS_PME_En;
> +			writel(value, hcd->regs + XECP_PM_PMCSR);
> +			/* Wait power state back to D0 */
> +			poll = readl_poll_timeout(hcd->regs + XECP_PM_PMCSR,
> +						  value,
> +						  (value & PS_MASK) == 0 ||
> +						  value == U32_MAX,
> +						  10, 100ULL * 1000);
> +			if (poll) {
> +				dev_dbg(dev, "exit D3 timeout, power state=0x%lx\n",
> +					value & PS_MASK);
> +			} else {
> +				break;
> +			}
> +		}
> +		if (count < 0) {
> +			dev_err(dev, "exit D3 timeout after retries, power state=0x%lx\n",
> +				value & PS_MASK);
> +		}
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [1], are there several issues in
cdns_sky1_platform_suspend()?

First, ret is initialized to 0 and never updated on failure. When
sky1_rcsu_handshake() fails all retries on suspend (count < 0) or
readl_poll_timeout() fails all retries on resume (count < 0),
cdns_sky1_platform_suspend() logs an error via dev_err() but still returns
0. On suspend, won't callers then proceed to power off PHYs and disable the
AXI and SOF clocks in cdnsp_sky1_system_suspend() while AXI_CLOCK_VALID is
still asserted?

Second, in the resume (!suspend) branch, readl_poll_timeout() polls
hcd->regs + XECP_PM_PMCSR with the condition (value & PS_MASK) == 0 ||
value == U32_MAX. Unlike sky1_rcsu_handshake(), which checks if
result == U32_MAX and returns -ENODEV, this loop only checks if (poll) and
breaks out when poll == 0. If reading XECP_PM_PMCSR returns U32_MAX (where
value & PS_MASK is PS_D3), won't readl_poll_timeout() return 0 and cause
the loop to exit immediately as if the controller had reached D0?

Third, in the suspend branch, value |= PS_D3 | PS_PME_En sets PS_PME_En
unconditionally even when wakeup is false.

Fourth, data->wakeup = wakeup is assigned before returning early when
cdns->role != USB_ROLE_HOST. In peripheral or idle OTG role, the
controller is not placed into D3 and AXI_CLOCK_VALID is not cleared, yet
if wakeup is true, cdnsp_sky1_system_suspend() skips asserting resets while
disabling the sof and axi clocks with the controller still active in D0.

[ ... ]

> +static void cdnsp_sky1_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	pm_runtime_get_sync(dev);
> +	of_platform_depopulate(dev);
> +	sky1_put_addr(data->xhci_base);
> +	sky1_put_addr(data->device_base);
> +	reset_control_assert(data->reset);
> +	reset_control_assert(data->preset);
> +	cdnsp_sky1_clk_disable_all(dev);
> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_put_noidle(dev);
> +}
> +
> +/*
> + * Because the wake-up interrupt and host interrupt are the same interrupt,
> + * closing the axi and sof clock will result in the inability to generate
> + * port status change interrupt.
> + */
> +static int cdnsp_sky1_system_suspend(struct device *dev)
> +{
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	if (!data->wakeup) {
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +	}
> +	cdnsp_sky1_clk_disable_suspend(dev);
> +	return 0;
> +}

[Severity: Medium]
As reported by Sashiko on v5 [1], the comment above
cdnsp_sky1_system_suspend() notes that closing the axi and sof clocks
prevents generating a port status change wakeup interrupt. However,
cdnsp_sky1_system_suspend() only guards reset_control_assert() with
if (!data->wakeup) and then unconditionally calls
cdnsp_sky1_clk_disable_suspend(dev), which disables the first
CIX_USB_CLK_OFF_NUM clocks (sof and axi).

Doesn't this disable the sof and axi clocks even when data->wakeup is
true, preventing wakeup interrupts during system sleep?

> +
> +static int cdnsp_sky1_system_resume(struct device *dev)
> +{
> +	int ret = 0;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +
> +	ret = cdnsp_sky1_clk_enable_resume(dev);
> +	if (ret)
> +		return ret;
> +	if (!data->wakeup) {
> +		writel(CIX_USB_AXI_WR_CACHE_VALUE, data->rcsu_axi_base);
> +		cdnsp_sky1_set_mode(data->dev, MODE_STRAP_OTG);
> +		reset_control_deassert(data->preset);
> +		cdnsp_sky1_configure_controller(data);
> +		reset_control_deassert(data->reset);
> +	}
> +	return 0;
> +}

[Severity: High]
As reported by Sashiko on v5 [1], how does system resume recover after
cdnsp_sky1_system_suspend() asserts data->reset and data->preset when
!data->wakeup?

Asserting data->reset and data->preset resets the entire Cadence USBSSP
controller, clearing all OTG/DRD, xHCI, and gadget registers. On system
resume, cdnsp_sky1_system_resume() deasserts preset, runs
cdnsp_sky1_configure_controller(), and deasserts reset, leaving the DRD
block in its post-reset state (OTGCMD_HOST_BUS_REQ is not set,
OTGSTS_CDNSP_XHCI_READY is 0, OVERRIDE_IDPULLUP is cleared, and OTG
interrupts in otg_cdnsp_regs->ien are disabled).

When the child device then resumes in
cdns3_plat_resume() -> cdns3_controller_resume():

drivers/usb/cdns3/cdns3-plat.c:cdns3_controller_resume() {
	...
	cdns3_set_platform_suspend(cdns->dev, false, false);

	spin_lock_irqsave(&cdns->lock, flags);
	cdns_resume(cdns);
	cdns->in_lpm = false;
	spin_unlock_irqrestore(&cdns->lock, flags);
	...
}

First, cdns3_set_platform_suspend() invokes cdns_sky1_platform_suspend()
before cdns_resume() runs, reading and writing hcd->regs + XECP_PM_PMCSR
before cdns_drd_host_on() has re-enabled the host controller in the DRD
block.

Second, cdns3_controller_resume() calls cdns_resume(cdns) while holding
spin_lock_irqsave(&cdns->lock, flags). In cdns_resume(),
cdns_power_is_lost(cdns) is checked (which for CDNSP_CONTROLLER_V2 reads
&cdns->otg_v1_regs->simulate at offset 0x40 rather than simulate at 0x28
in struct cdnsp_otg_regs, and simulate is never written in cdns_drd_init()
for CDNSP_CONTROLLER_V2). If cdns_power_is_lost() returns true (after the
reset or during runtime resume enabled by CDNS3_DEFAULT_PM_RUNTIME_ALLOW),
cdns_resume() calls cdns_hw_role_switch() (which calls
pm_runtime_get_sync() and mutex_lock(&cdns->mutex)) or cdns_drd_host_on() /
cdns_drd_gadget_on() (which call phy_set_mode() -> mutex_lock(&phy->mutex))
under the spinlock, sleeping in atomic context. If cdns_power_is_lost()
returns false, cdns_drd_host_on() and xHCI restore are skipped, and
neither cdnsp_sky1_system_resume() nor cdns_resume() calls
cdns_drd_update_mode() to restore OVERRIDE_IDPULLUP and
otg_irq_regs->ien.

[Severity: Medium]
As reported by Sashiko on v5 [1], what happens if
cdnsp_sky1_clk_enable_resume() fails in cdnsp_sky1_system_resume()?

During system suspend, cdnsp_sky1_clk_disable_suspend() disables and
unprepares the first CIX_USB_CLK_OFF_NUM clocks (sof and axi). If
cdnsp_sky1_clk_enable_resume() fails during resume, it unwinds any clock
it just enabled and returns the error, leaving sof and axi disabled
without tracking that those clocks remain off. When cdnsp_sky1_remove() or
cdnsp_sky1_shutdown() later runs, it unconditionally calls
cdnsp_sky1_clk_disable_all(dev) across all CIX_USB_CLK_NUM clocks. Won't
that underflow the enable and prepare counts for sof and axi?

[ ... ]

> +static void cdnsp_sky1_shutdown(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct cdnsp_sky1 *data = dev_get_drvdata(dev);
> +	struct device *cdns_dev = NULL;
> +	struct device *gadget_dev = NULL;
> +	struct cdns *cdns = NULL;
> +
> +	if (!device_may_wakeup(dev)) {

[Severity: Medium]
As reported by Sashiko on v5 [1], is the wakeup state checked consistently
between the parent glue device and the child cdns,usb3 device?

cdnsp_sky1_probe() calls device_set_wakeup_capable(dev, true) on the
parent glue device, while cdns3_plat_probe() also marks the child
cdns,usb3 device wakeup-capable.

During system suspend and resume, cdnsp_sky1_system_suspend() and
cdnsp_sky1_system_resume() check data->wakeup (populated from the child
device in cdns_sky1_platform_suspend()), ignoring device_may_wakeup(dev) on
the parent. Conversely, cdnsp_sky1_shutdown() checks
if (!device_may_wakeup(dev)) on the parent glue device, ignoring the child
device's wakeup setting.

Also, during runtime suspend (PMSG_AUTO_SUSPEND),
cdns3_controller_suspend() passes wakeup = true to
cdns_sky1_platform_suspend(), setting data->wakeup = true; if the child
device remains in cdns->in_lpm when system suspend runs,
cdns3_controller_suspend() returns early without updating data->wakeup to
device_may_wakeup(cdns->dev).

> +		/*
> +		 * Find the cdns3 child device, then find its gadget device
> +		 * and release the function driver before disabling clocks.
> +		 * This ensures all register accesses in gadget_unbind_driver
> +		 * complete before clocks are turned off.
> +		 */
> +		device_for_each_child(dev, &cdns_dev, cdnsp_sky1_find_cdns_dev);
> +		if (cdns_dev)
> +			cdns = dev_get_drvdata(cdns_dev);
> +		if (cdns)
> +			device_for_each_child(cdns_dev, &gadget_dev,
> +					      cdnsp_sky1_find_gadget_match);
> +		if (gadget_dev)
> +			device_release_driver(gadget_dev);
> +		if (cdns && cdns->host_dev) {
> +			struct usb_hcd *hcd = platform_get_drvdata(cdns->host_dev);
> +
> +			if (hcd && hcd->irq > 0) {
> +				/*
> +				 * Clear HCD_FLAG_HW_ACCESSIBLE before disable_irq.
> +				 * This prevents usb_hcd_irq from calling xhci_irq
> +				 * (which reads USBSTS) after clocks are disabled.
> +				 */
> +				clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
> +				disable_irq(hcd->irq);
> +				synchronize_irq(hcd->irq);
> +			}
> +		}
> +		reset_control_assert(data->reset);
> +		reset_control_assert(data->preset);
> +		cdnsp_sky1_clk_disable_all(dev);
> +		put_device(gadget_dev);
> +		put_device(cdns_dev);
> +	}
> +}

[Severity: High]
As reported by Sashiko on v5 [1], can cdnsp_sky1_shutdown() trigger
unclocked MMIO accesses or race with role switching?

1. Because cdnsp_sky1_probe() sets CDNS3_DEFAULT_PM_RUNTIME_ALLOW, the
child cdns,usb3 device autosuspends when idle or in peripheral mode.
During device_shutdown(), the driver core only calls
pm_runtime_get_noresume(), not pm_runtime_get_sync(). When cdns->host_dev
is not active, usb_hcd_platform_shutdown() does not run to runtime-resume
cdns_dev. Calling device_release_driver(gadget_dev) without first calling
pm_runtime_get_sync(cdns_dev) causes
gadget_unbind_driver() -> cdnsp_gadget_udc_stop() -> cdnsp_stop() to access
controller MMIO registers while the controller is in low-power state.

2. cdnsp_sky1_shutdown() reads cdns->host_dev and accesses its drvdata
struct usb_hcd without holding cdns->mutex or disabling cdns->otg_irq. A
concurrent OTG role switch via cdns_drd_thread_irq() or cdns_role_set()
calls cdns_host_exit() under cdns->mutex:

drivers/usb/cdns3/host.c:cdns_host_exit() {
	kfree(cdns->xhci_plat_data);
	platform_device_unregister(cdns->host_dev);
	cdns->host_dev = NULL;
	cdns_drd_host_off(cdns);
}

which unregisters and frees cdns->host_dev and struct usb_hcd before
setting cdns->host_dev = NULL, racing with cdnsp_sky1_shutdown().

3. Calling device_release_driver(gadget_dev) only unbinds the gadget
function driver on the gadget bus and does not tear down the gadget or DRD
drivers. Both cdns->otg_irq (requested with IRQF_SHARED for
cdns_drd_irq()) and cdns->dev_irq (requested with IRQF_SHARED for
cdnsp_irq_handler(), and not disabled via hcd->irq when cdns->host_dev is
NULL in device mode) remain enabled when cdnsp_sky1_shutdown() asserts
data->reset and data->preset and disables all clocks via
cdnsp_sky1_clk_disable_all(dev). If a shared or pending interrupt fires on
otg_irq or dev_irq afterward, cdns_drd_irq() or cdnsp_irq_handler() will
read controller MMIO registers while resets are asserted and clocks are
disabled.

[1] https://lore.kernel.org/r/20261103074929.1971195-3-hongliang.yang@cixtech.com

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

[PATCH v6 3/3] arm64: dts: cix: Add USB4/USB5 nodes for Sky1

[Hongliang Yang (杨宏亮)]

Add USB4 and USB5 controller nodes for the CIX Sky1 SoC. Enable USB4
and USB5 in host mode on the Radxa Orion O6 board.

Each controller node is placed at the address of its glue registers in
the RCSU and contains the Cadence USBSSP core child node at the core
address. The mode strap register offset and bit position are passed as
arguments of the cix,usb-syscon phandle.

Signed-off-by: Hongliang Yang <hongliang.yang@cixtech.com>
---
Changes in v6:
- Drop the simple-bus RCSU container and the translated register
  offsets; place each controller node at its real address.
- Drop the usb4/usb5 aliases and pass the mode strap register offset
  and bit position through the cix,usb-syscon phandle.
- Reference the shared USB3 PHY through its single phandle.
---
 arch/arm64/boot/dts/cix/sky1-orion-o6.dts | 42 +++++++++++
 arch/arm64/boot/dts/cix/sky1.dtsi         | 85 +++++++++++++++++++++++
 2 files changed, 127 insertions(+)

diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
index 5d57dd9ea299..d07fad302050 100644
--- a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
+++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
@@ -80,6 +80,22 @@ pins {
 
 		};
 	};
+
+	pinctrl_usb4: usb4-power-on-cfg {
+		pins {
+			bias-pull-down;
+			drive-strength = <8>;
+			pinmux = <CIX_PAD_GPIO041_FUNC_USB_DRIVE_VBUS4>;
+		};
+	};
+
+	pinctrl_usb5: usb5-power-on-cfg {
+		pins {
+			bias-pull-down;
+			drive-strength = <8>;
+			pinmux = <CIX_PAD_GPIO042_FUNC_USB_DRIVE_VBUS5>;
+		};
+	};
 };
 
 &pcie_x8_rc {
@@ -118,6 +134,32 @@ &uart2 {
 	status = "okay";
 };
 
+&usb4 {
+	pinctrl-0 = <&pinctrl_usb4>;
+	pinctrl-names = "default";
+
+	status = "okay";
+};
+
+&usbss_4 {
+	dr_mode = "host";
+
+	status = "okay";
+};
+
+&usb5 {
+	pinctrl-0 = <&pinctrl_usb5>;
+	pinctrl-names = "default";
+
+	status = "okay";
+};
+
+&usbss_5 {
+	dr_mode = "host";
+
+	status = "okay";
+};
+
 &usb3_phy4 {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
index c081b63a9848..ae66dca53894 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -606,6 +606,90 @@ mbox_ap2sfh: mailbox@80a0000 {
 			cix,mbox-dir = "tx";
 		};
 
+		usb4: usb@91c0314 {
+			compatible = "cix,sky1-usbssp";
+			reg = <0x00 0x091c0314 0x00 0x10>,
+			      <0x00 0x091c0400 0x00 0x10>;
+			reg-names = "axi", "status";
+			ranges;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			clocks = <&scmi_clk CLK_TREE_USB3A_H0_CLK_SOF>,
+				<&scmi_clk CLK_TREE_USB3A_0_AXI_GATE>,
+				<&scmi_clk CLK_TREE_USB3A_H0_CLK_LPM>,
+				<&scmi_clk CLK_TREE_USB3A_0_APB_GATE>;
+			clock-names = "sof", "axi", "lpm", "apb";
+			resets = <&s5_syscon SKY1_USBC_SS2_PRST_N>,
+				<&s5_syscon SKY1_USBC_SS2_RST_N>;
+			reset-names = "apb", "core";
+			/* USB3 Type-A controller 0 mode strap */
+			cix,usb-syscon = <&s5_syscon 0x424 8>;
+
+			status = "disabled";
+
+			usbss_4: usb@91d0000 {
+				compatible = "cdns,usb3";
+				reg = <0x00 0x91d0000 0x00 0x4000>,
+				      <0x00 0x91d8000 0x00 0x8000>,
+				      <0x00 0x91d4000 0x00 0x4000>;
+				reg-names = "otg", "xhci", "dev";
+				dr_mode = "otg";
+				interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,
+					<GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,
+					<GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>,
+					<GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>;
+				interrupt-names = "host", "peripheral",
+					"otg", "wakeup";
+				maximum-speed = "super-speed-plus";
+				phys = <&usb3_phy4>;
+				phy-names = "cdns3,usb3-phy";
+
+				status = "disabled";
+			};
+		};
+
+		usb5: usb@91c0324 {
+			compatible = "cix,sky1-usbssp";
+			reg = <0x00 0x091c0324 0x00 0x10>,
+			      <0x00 0x091c0410 0x00 0x10>;
+			reg-names = "axi", "status";
+			ranges;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			clocks = <&scmi_clk CLK_TREE_USB3A_H1_CLK_SOF>,
+				<&scmi_clk CLK_TREE_USB3A_1_AXI_GATE>,
+				<&scmi_clk CLK_TREE_USB3A_H1_CLK_LPM>,
+				<&scmi_clk CLK_TREE_USB3A_1_APB_GATE>;
+			clock-names = "sof", "axi", "lpm", "apb";
+			resets = <&s5_syscon SKY1_USBC_SS3_PRST_N>,
+				<&s5_syscon SKY1_USBC_SS3_RST_N>;
+			reset-names = "apb", "core";
+			/* USB3 Type-A controller 1 mode strap */
+			cix,usb-syscon = <&s5_syscon 0x424 10>;
+
+			status = "disabled";
+
+			usbss_5: usb@91e0000 {
+				compatible = "cdns,usb3";
+				reg = <0x00 0x91e0000 0x00 0x4000>,
+				      <0x00 0x91e8000 0x00 0x8000>,
+				      <0x00 0x91e4000 0x00 0x4000>;
+				reg-names = "otg", "xhci", "dev";
+				dr_mode = "otg";
+				interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>,
+					<GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>,
+					<GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH 0>,
+					<GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>;
+				interrupt-names = "host", "peripheral",
+					"otg", "wakeup";
+				maximum-speed = "super-speed-plus";
+				phys = <&usb3_phy4>;
+				phy-names = "cdns3,usb3-phy";
+
+				status = "disabled";
+			};
+		};
+
 		usb3_phy4: usb-phy@9210000 {
 			compatible = "cix,sky1-usb3-phy";
 			reg = <0x00 0x09210000 0x00 0x40000>;
@@ -780,6 +864,7 @@ s5_syscon: syscon@16000000 {
 			compatible = "cix,sky1-s5-system-control", "syscon";
 			reg = <0x0 0x16000000 0x0 0x1000>;
 			#reset-cells = <1>;
+			#syscon-cells = <2>;
 		};
 
 		s5_gpio0: gpio-controller@16004000 {

Re: [PATCH v6 3/3] arm64: dts: cix: Add USB4/USB5 nodes for Sky1

[sashiko-bot]

> Add USB4 and USB5 controller nodes for the CIX Sky1 SoC. Enable USB4
> and USB5 in host mode on the Radxa Orion O6 board.
> 
> Each controller node is placed at the address of its glue registers in
> the RCSU and contains the Cadence USBSSP core child node at the core
> address. The mode strap register offset and bit position are passed as
> arguments of the cix,usb-syscon phandle.
> 
> Signed-off-by: Hongliang Yang <hongliang.yang@cixtech.com>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH v6 3/3] arm64: dts: cix: Add USB4/USB5 nodes for Sky1

[Krzysztof Kozlowski]

On 05/11/2026 02:18, Hongliang Yang (杨宏亮) wrote:
> Add USB4 and USB5 controller nodes for the CIX Sky1 SoC. Enable USB4
> and USB5 in host mode on the Radxa Orion O6 board.
> 
> Each controller node is placed at the address of its glue registers in
> the RCSU and contains the Cadence USBSSP core child node at the core
> address. The mode strap register offset and bit position are passed as
> arguments of the cix,usb-syscon phandle.
> 
> Signed-off-by: Hongliang Yang <hongliang.yang@cixtech.com>
> ---
> Changes in v6:

So you keep ignoring review or parts of review and never responding to it.

NAK last time, I will be NAKing this.

Best regards,
Krzysztof

Re: [PATCH v6 3/3] arm64: dts: cix: Add USB4/USB5 nodes for Sky1

[Hongliang Yang]

On Thu, Nov 05, 2026 at 08:48:27AM +0100, Krzysztof Kozlowski wrote:
> So you keep ignoring review or parts of review and never responding to it.
> NAK last time, I will be NAKing this.

Fair, and it is the same mistake as on the binding: I answered the v5
cover letter and treated that as answering the review. The full
point-by-point answer is in the reply to [PATCH v6 1/3]; this one covers
only what is specific to the dts.

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

Gone in v6, together with the ranges translation. v5 had a
usb-rcsu@91c0300 node that was a bus for a block of registers; v6 has
neither the node nor the translation.

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

That was the consequence of the container: a node at a translated
offset 0x14 with a child at an absolute address. In v7 the unit address
of every node is the first address of its own reg, so nothing is
translated and nothing is invented.

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

The usbN aliases and the id property are gone since v6. The mode strap
is now an argument of the syscon phandle, and it points at the S5 system
controller, which is where that register is:

	cix,usb-syscon = <&s5_syscon 0x424 8>;   /* USB4 */
	cix,usb-syscon = <&s5_syscon 0x424 10>;  /* USB5 */

Controller 0 of the Type-A block is used by USB4, controller 1 is used
by USB5.

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

Correct for the register model; v6 changed the container and the aliases
but left the two 16-byte register slots, which was not what you asked
for. v7 changes that part:

	usb4: usb@91c0314 {
		compatible = "cix,sky1-usbssp";
		reg = <0x00 0x091c0314 0x00 0x100>;
		...

		usbss_4: usb@91d0000 {
			compatible = "cdns,usb3";
			...
		};
	};

	usb5: usb@91c0324 {
		compatible = "cix,sky1-usbssp";
		reg = <0x00 0x091c0324 0x00 0x100>;
		...

		usbss_5: usb@91e0000 {
			compatible = "cdns,usb3";
			...
		};
	};

One range per controller, from its AXI cache attribute register to its
AXI clock status register, instead of two register slots. The two ranges
overlap, because the two controllers' registers are interleaved in the
shared 0x091c0000 block and 0x0328 inside the second range belongs to
controller 0. I would rather have the dts say that than pretend each
controller has a private block; the reason the driver uses
devm_ioremap() instead of devm_platform_ioremap_resource() is the same
overlap.

5) The PHY is a separate series since v4 ("do not combine USB patches
   with other subsystems", 6ca4ba08), and v7 touches only bindings/usb/
   and this dtsi. v6 also modified the S5 system control binding to add
   #syscon-cells, which is not a property that exists in the kernel; that
   hunk is dropped and the changelog no longer claims it.

6) "Odd style, please read git commit help page" (0df645f6): the commit
   messages are what git format-patch produced, nothing hand-edited.

As in the binding reply, I am implementing v7 now instead of asking you
another question and waiting.

Thanks,
Hongliang