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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Contradictory suspend state configuration for `vdd_cpu_lit_s0` and `vdd_gpu_s0` regulators.
- [Medium] `full-pwr-cycle-in-suspend` specified for eMMC without a `vmmc-supply`.
- [Low] Missing ES8388 audio codec device node on I2C0 bus.
- [Low] Use of deprecated `gpio` property instead of `gpios` in `vcc_3v3_pcie` fixed regulator.
--

--- Patch [2]: [PATCH v2 2/2] arm64: dts: rockchip: Add ArmSoM CM5 and CM5-IO ---
commit cd4f511d2817d5912abf4ba85345906a62079b4e
Author: Jiaxing Hu <gahing@gahingwoo.com>

arm64: dts: rockchip: Add ArmSoM CM5 and CM5-IO

Add the ArmSoM CM5, an RK3576 compute module (rk3576-armsom-cm5.dtsi),
and its CM5-IO carrier (rk3576-armsom-cm5-io.dts). The module has the
RK806 PMIC, HYM8563 RTC, eMMC, GMAC0 with an on-module MotorComm YT8531
PHY, and the SDIO bus for the on-module WiFi.

> 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
> index 0000000000000..79a903585197c
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5-io.dts
> @@ -0,0 +1,401 @@

[ ... ]

> +	/* PCIe 3.3 V for the M.2 slot, gated by GPIO0 PC3. */
> +	vcc_3v3_pcie: regulator-vcc-3v3-pcie {
> +		compatible = "regulator-fixed";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pcie_pwr_en>;
> +		regulator-name = "vcc_3v3_pcie";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PC3 GPIO_ACTIVE_HIGH>;

[Severity: Low]
Does this regulator intentionally use the deprecated gpio property instead
of gpios? Using gpios here would match standard devicetree bindings and
the style used by other regulators in this file.

> +		startup-delay-us = <5000>;
> +		vin-supply = <&vcc_5v0_sys>;
> +	};

[ ... ]

> +/*
> + * I2C0: ES8388 audio codec @ 0x10, FUSB302 USB-C PD controller @ 0x22.
> + * Pinctrl i2c0m1 (GPIO0 PC4 SCL / GPIO0 PC5 SDA).
> + */
> +&i2c0 {
> +	status = "okay";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&i2c0m1_xfer>;
> +
> +	usbc0: usb-typec@22 {

[Severity: Low]
The comment above mentions the ES8388 audio codec at 0x10, but is the device
node for it missing here in &i2c0?

> 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
> index 0000000000000..4612b7e1d6e8a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3576-armsom-cm5.dtsi
> @@ -0,0 +1,564 @@

[ ... ]

> +			vdd_cpu_lit_s0: dcdc-reg3 {
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <550000>;
> +				regulator-max-microvolt = <950000>;
> +				regulator-ramp-delay = <12500>;
> +				regulator-name = "vdd_cpu_lit_s0";
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +					regulator-suspend-microvolt = <750000>;

[Severity: Medium]
Can a regulator be simultaneously turned off and maintain a specific suspend
voltage? Specifying both regulator-off-in-suspend and
regulator-suspend-microvolt appears contradictory and may cause unexpected
behavior during suspend.

> +				};
> +			};

[ ... ]

> +			vdd_gpu_s0: dcdc-reg5 {
> +				regulator-boot-on;
> +				regulator-min-microvolt = <550000>;
> +				regulator-max-microvolt = <900000>;
> +				regulator-ramp-delay = <12500>;
> +				regulator-name = "vdd_gpu_s0";
> +				regulator-enable-ramp-delay = <400>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +					regulator-suspend-microvolt = <850000>;

[Severity: Medium]
Similar to vdd_cpu_lit_s0, is it correct to include both
regulator-off-in-suspend and regulator-suspend-microvolt here for the GPU
regulator?

> +				};
> +			};

[ ... ]

> +/* eMMC: on-module, HS400 200 MHz with enhanced strobe. */
> +&sdhci {
> +	bus-width = <8>;
> +	full-pwr-cycle-in-suspend;
> +	max-frequency = <200000000>;
> +	mmc-hs400-1_8v;
> +	mmc-hs400-enhanced-strobe;
> +	no-sdio;
> +	no-sd;
> +	non-removable;
> +	/delete-property/ supports-cqe;
> +	status = "okay";
> +};

[Severity: Medium]
Will full-pwr-cycle-in-suspend have any effect if vmmc-supply is not
specified? Without a vmmc-supply to control the power, the MMC core may not
be able to actually fully power down the device, potentially leading to state
mismatches on resume.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716065548.2466198-1-gahing@gahingwoo.com?part=2