Re: [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Missing `status = "disabled";` for `usb4` and `usb5` device nodes in the SoC-level device tree.
- [Medium] Invalid double-vendor prefix in compatible string and squashing of SoC-specific glue layer with generic IP core.
- [Low] Unnecessary inclusion of `<dt-bindings/phy/phy.h>` in `sky1.dtsi` without associated PHY definitions.
- [Low] Redundant/duplicate interrupt mappings for the `wakeup` interrupt.
--

commit a270480764c96a27fe2e14e30bfb46b99527fcfb
Author: Peter Chen <peter.chen@cixtech.com>

arm64: dts: cix: add Sky1 USB4 and USB5 controllers

Add the Sky1 USB4 and USB5 Cadence USB3 controller nodes with their
registers, interrupts, clocks, resets and S5 syscon control. Enable both
ports on the Orion O6 board in host mode with the required VBUS pinctrl.

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index bb5cfb1f21133..9f7d9ad6586c3 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -6,6 +6,8 @@
>  
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/clock/cix,sky1.h>
> +#include <dt-bindings/phy/phy.h>

Since there are no phys properties or macros from this header used in the
patch, is this inclusion needed?

> +#include <dt-bindings/reset/cix,sky1-s5-system-control.h>
>  #include "sky1-power.h"
>  
>  / {

[ ... ]

> @@ -504,6 +506,72 @@ mbox_ap2sfh: mailbox@80a0000 {
>  			cix,mbox-dir = "tx";
>  		};
>  
> +		usb4: usb@91d0000 {
> +			compatible = "cix,sky1-usb3", "cix,cdns-usb3";

Does the "cix,cdns-usb3" string incorrectly combine two vendor prefixes?
Should the generic fallback be "cdns,usb3"?

> +			reg = <0x00 0x91d0000 0x00 0x4000>,
> +					<0x00 0x91d4000 0x00 0x4000>,
> +					<0x00 0x91d8000 0x00 0x8000>,
> +					<0x00 0x91c0314 0x00 0x4>;
> +			reg-names = "otg", "dev", "xhci", "glue";

Does adding the "glue" register and cix,syscon-usb to the generic core
break the generic upstream schema which expects only three reg entries?

Should the SoC-specific glue logic be implemented as a parent wrapper node
with a child node for the generic Cadence USB3 core instead?

> +
> +			interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,	/* host irq */
> +					<GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,	/* peripheral irq */
> +					<GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>,	/* otgirq */
> +					<GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>;	/* wakeup irq */

The wakeup interrupt uses the same GIC SPI line as the main host and
peripheral interrupts. Is this redundant, and could it lead to conflicting
interrupt registration?

[ ... ]

> +			cix,syscon-usb = <&s5_syscon>;
> +			dma-coherent;
> +			maximum-speed = "super-speed-plus";
> +			dr_mode = "otg";
> +		};

Should this node include a status = "disabled" property so the USB drivers do
not probe unconditionally on all boards using this SoC?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511024244.981941-1-peter.chen@cixtech.com?part=4