Re: [PATCH v1 4/4] arm64: dts: cix: Add USB4/USB5 nodes for Sky1

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The parent USB wrapper nodes (`usb4`, `usb5`) and the required USB PHY node (`usb3_phy4`) are left disabled in the board device tree, preventing the USB controllers from probing.
- [Low] The unit addresses for the `usb4` and `usb5` nodes do not match their first `reg` address, violating the Device Tree specification.
- [Low] The `usb-port` child nodes inside `usb3_phy4` use unit addresses but lack the required `reg` properties.
- [Low] Incorrect `reg-names` order for the `cdns,usb3` controllers violates the DT schema binding.
--

commit fb5456e6e85188c30dcbeb49499209cebdd39407
Author: Hongliang Yang <hongliang.yang@cixtech.com>

arm64: dts: cix: Add USB4/USB5 nodes for Sky1
    
Add USB4 and USB5 controller nodes and their associated USB3 PHY nodes for
the CIX Sky1 SoC. Enable USB4 and USB5 in host mode on the Radxa Orion O6
board.

> diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> index e39c87774c128..33d3ffdc3f9e0 100644
> --- a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
> +++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
[ ... ]
> @@ -117,3 +133,23 @@ &s5_gpio2 {
>  &uart2 {
>  	status = "okay";
>  };
> +
> +&usb4 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_usb4>;
> +};

[Severity: High]
Are the usb4 and usb5 parent nodes intentionally left disabled in the board
configuration?

The parent nodes usb4 and usb5 are marked as status = "disabled" in the
DTSI. Although this patch adds pin control configurations for them, it
omits status = "okay";. The Linux DT core will not instantiate disabled
platform devices, which would prevent their children (usbss_4, usbss_5)
from probing.

> +
> +&usbss_4 {
> +	status = "okay";
> +	dr_mode = "host";
> +};
> +
> +&usb5 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_usb5>;
> +};
> +
> +&usbss_5 {
> +	status = "okay";
> +	dr_mode = "host";
> +};

[Severity: High]
Is the required PHY node (usb3_phy4) missing its enablement in this board
file?

The PHY is marked disabled by default in sky1.dtsi, but it is completely
omitted from the board file. This could cause PHY consumer requests to fail
with -ENODEV even if the controllers did probe, preventing USB4 and USB5
from functioning.

> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index a77c52296ebd3..44736c0715732 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
[ ... ]
> @@ -574,6 +581,132 @@ mbox_ap2sfh: mailbox@80a0000 {
>  			cix,mbox-dir = "tx";
>  		};
>  
> +		usb4: usb@91c0300 {
> +			compatible = "cix,sky1-usbssp";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +
> +			reg = <0x00 0x091c0314 0x00 0x4>,

[Severity: Low]
Does the unit address @91c0300 need to be updated to match the first reg
address 0x091c0314? The DeviceTree specification mandates that the unit
address must exactly match the first address in the node's reg property,
which could trigger validation warnings.

> +				<0x00 0x091c0400 0x00 0x4>;
> +			reg-names = "axi_property", "controller_status";
> +
[ ... ]
> +			usbss_4: usb-controller@91d0000 {
> +				compatible = "cdns,usb3";
> +				reg = <0x00 0x91d0000 0x00 0x4000>,
> +					<0x00 0x91d4000 0x00 0x4000>,
> +					<0x00 0x91d8000 0x00 0x8000>;
> +				reg-names = "otg", "dev", "xhci";

[Severity: Low]
Is the order of the reg-names correct here? 

The device tree binding for Cadence USB3 controllers requires the reg-names
property to be strictly ordered as "otg", "xhci", "dev". Do the names and
the corresponding reg tuples need to be reordered so that the "xhci" memory
region matches the required order to avoid schema validation errors?

> +
> +				interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,
[ ... ]
> +		usb5: usb@91c0304 {
> +			compatible = "cix,sky1-usbssp";
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges;
> +
> +			reg = <0x00 0x091c0324 0x00 0x4>,

[Severity: Low]
Similarly, does the unit address @91c0304 need to be updated to match the
first reg address 0x091c0324 to avoid validation warnings?

> +				<0x00 0x091c0410 0x00 0x4>;
> +			reg-names = "axi_property", "controller_status";
> +
[ ... ]
> +			usbss_5: usb-controller@91e0000 {
> +				compatible = "cdns,usb3";
> +				reg = <0x00 0x91e0000 0x00 0x4000>,
> +					<0x00 0x91e4000 0x00 0x4000>,
> +					<0x00 0x91e8000 0x00 0x8000>;
> +				reg-names = "otg", "dev", "xhci";

[Severity: Low]
Does the order of the reg-names and corresponding reg tuples for usbss_5
also need to be adjusted to "otg", "xhci", "dev" to match the binding
schema?

> +
> +				interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>,
[ ... ]
> +		usb3_phy4: usb-phy@9210000 {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			compatible = "cix,sky1-usb3-phy";
> +			reg = <0x00 0x09210000 0x00 0x40000>;
> +			status = "disabled";
> +
> +			resets = <&s5_syscon SKY1_USBPHY_SS_RST_N>,
> +				<&s5_syscon SKY1_USBPHY_SS_PST_N>;
> +			reset-names = "reset", "preset";
> +
> +			clocks = <&scmi_clk CLK_TREE_USB3A_PHY3_GATE>,
> +				<&scmi_clk CLK_TREE_USB3A_PHY_x2_REF>;
> +			clock-names = "apb_clk", "ref_clk";
> +
> +			usb3_phy4_0: usb-port@0 {
> +				#phy-cells = <0>;
> +				status = "disabled";
> +			};

[Severity: Low]
Since this node has a unit-address (@0) and the parent has
#address-cells = <1>;, should it include a reg = <0>; property?

The DeviceTree specification states that if a node has a unit-address in
its name, it must have a corresponding reg property.

> +
> +			usb3_phy4_1: usb-port@1 {
> +				#phy-cells = <0>;
> +				status = "disabled";
> +			};

[Severity: Low]
Should this node also include a reg = <1>; property for the unit-address @1?

> +		};

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