Re: [PATCH 2/2] arm64: dts: rockchip: Add ArmSoM CM5 and CM5-IO

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Type-C DP Altmode configuration errors (SVID format, missing mode-switch, and misplaced SVID)
- [High] Wireless (SYN43752 WiFi/BT) missing child nodes and unreferenced pinctrls
- [Medium] ES8388 audio is advertised in the commit message but functionally missing
- [Medium] Missing vmmc-supply and vqmmc-supply for the eMMC
- [Low] The gmac0_rst pinctrl is defined but omitted from &gmac0
--

--- Patch [2]: [PATCH 2/2] arm64: dts: rockchip: Add ArmSoM CM5 and CM5-IO ---
commit 0ff6fbd7a765dd8a49516e69aa071bce9122a7bd
Author: Jiaxing Hu <huhuvmb88@gmail.com>

arm64: dts: rockchip: Add ArmSoM CM5 and CM5-IO
    
Adds the RK3576-based ArmSoM CM5 compute module and its CM5-IO carrier
board. This includes support for the RK806 PMIC, HYM8563 RTC, eMMC,
GMAC, SYN43752 WiFi/BT, HDMI, PCIe, and USB.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5-io.dts b/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5-io.dts
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5-io.dts

[ ... ]

> +&i2c0 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c0m1_xfer>;
> +
> +	es8388: es8388@10 {
> +		#sound-dai-cells = <0>;
> +		compatible = "everest,es8388", "everest,es8323";
> +		reg = <0x10>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&sai1m0_mclk>;
> +		status = "disabled";
> +	};

[Severity: Medium]
The commit message mentions adding ES8388 audio support, but the node is
explicitly set to disabled here. 

Is this board missing a sound card node (like simple-audio-card) or I2S
integration to make the audio codec functionally available?

[ ... ]

> +				altmodes {
> +					#address-cells = <1>;
> +					#size-cells = <0>;
> +
> +					altmode@0 {
> +						reg = <0>;
> +						svid = <0xff01>;

[Severity: High]
Does this property format result in a 32-bit property being created?

When fwnode_property_read_u16() reads this, it will extract the first two
bytes, which might evaluate to an invalid SVID of 0 rather than 0xff01.

Should this be specified as a 16-bit property, such as:
svid = /bits/ 16 <0xff01>;

> +						vdo = <0xffffffff>;
> +					};
> +				};

[ ... ]

> +&usbdp_phy {
> +	status = "okay";
> +	orientation-switch;
> +	svid = <0xff01>;
> +	sbu1-dc-gpios = <&gpio4 RK_PC4 GPIO_ACTIVE_HIGH>;
> +	sbu2-dc-gpios = <&gpio4 RK_PC5 GPIO_ACTIVE_HIGH>;

[Severity: High]
Does the Rockchip USB/DP PHY driver expect an svid property at this level?

Also, is this node missing the mode-switch; boolean property? Without it,
the PHY driver might fail to register as a Type-C mux, preventing DP Altmode
from negotiating successfully.

> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;

[ ... ]

> diff --git a/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5.dtsi b/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5.dtsi
> new file mode 100644
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5.dtsi

[ ... ]

> +&gmac0 {
> +	phy-mode = "rgmii-rxid";
> +	clock_in_out = "output";
> +	clocks = <&cru CLK_GMAC0_125M_SRC>, <&cru CLK_GMAC0_RMII_CRU>,
> +		 <&cru PCLK_GMAC0>, <&cru ACLK_GMAC0>,
> +		 <&cru CLK_GMAC0_PTP_REF>, <&cru REFCLKO25M_GMAC0_OUT>;
> +	clock-names = "stmmaceth", "clk_mac_ref", "pclk_mac", "aclk_mac",
> +		      "ptp_ref", "clk_mac_refout";
> +	tx_delay = <0x21>;
> +	rx_delay = <0x00>;
> +	phy-handle = <&rgmii_phy0>;
> +	phy-supply = <&vcc_3v3_s3>;
> +	snps,reset-gpios = <&gpio2 RK_PB3 GPIO_ACTIVE_LOW>;
> +	snps,reset-delays-us = <0 20000 100000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&eth0m0_miim
> +		     &eth0m0_tx_bus2
> +		     &eth0m0_rx_bus2
> +		     &eth0m0_rgmii_clk
> +		     &eth0m0_rgmii_bus
> +		     &ethm0_clk0_25m_out>;

[Severity: Low]
Is &gmac0_rst missing from this pinctrl-0 list? 

The gmac0-rst pinctrl is defined later in this file to configure the PHY reset
pin without pull resistors. Although snps,reset-gpios drives the pin, missing
the pinctrl assignment leaves the configuration as dead code and might result
in undefined pin states during boot or sleep.

> +	status = "okay";
> +};

[ ... ]

> +/* eMMC: on-module, HS400 200 MHz with enhanced strobe. */
> +&sdhci {
> +	bus-width = <8>;
> +	full-pwr-cycle-in-suspend;
> +	max-frequency = <200000000>;

[Severity: Medium]
Are vmmc-supply and vqmmc-supply required for this node?

By specifying full-pwr-cycle-in-suspend without defining the regulator supplies,
the MMC subsystem cannot actively perform the requested power cycle. This might
fall back to a dummy regulator and cause improper suspend/resume behavior.

> +	mmc-hs400-1_8v;
> +	mmc-hs400-enhanced-strobe;
> +	no-sdio;
> +	no-sd;
> +	non-removable;
> +	/delete-property/ supports-cqe;
> +	status = "okay";
> +};
> +
> +/* SDIO: SYN43752 WiFi, power-sequenced by sdio_pwrseq, clock from HYM8563. */
> +&sdio {
> +	bus-width = <4>;
> +	cap-sdio-irq;

[Severity: High]
Is a WiFi child node missing here to configure out-of-band interrupts?

Without the child node, the wifi_host_wake_irq pinctrl defined later is
unreferenced, which might break host-wake interrupt functionality.

> +	disable-wp;
> +	keep-power-in-suspend;
> +	mmc-pwrseq = <&sdio_pwrseq>;
> +	no-sd;
> +	no-mmc;
> +	non-removable;
> +	sd-uhs-sdr50;
> +	sd-uhs-sdr104;
> +	vmmc-supply = <&vcc_3v3_s3>;
> +	vqmmc-supply = <&vcc_1v8_s3>;
> +	wakeup-source;
> +	status = "okay";
> +};
> +
> +/* UART4: Bluetooth (SYN43752). */
> +&uart4 {
> +	pinctrl-0 = <&uart4m1_xfer &uart4m1_ctsn &uart4m1_rtsn>;
> +	pinctrl-names = "default";
> +	uart-has-rtscts;
> +	status = "okay";
> +};

[Severity: High]
Should there be a bluetooth child node added to this UART configuration?

Without it, the Bluetooth driver cannot probe, and the bt_reg_on pinctrl
defined below remains unused, preventing the chip from being taken out of
reset.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716012838.2353939-1-huhuvmb88@gmail.com?part=2