Re: [PATCH v3 2/2] arm64: dts: rockchip: Add Vicharak Vaaman board

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Medium] The SD card interface specifies `max-frequency = <150000000>;` without hardware capability to switch to 1.8V.
- [Medium] The `phy-supply` property is incorrectly placed in the MAC node instead of the PHY node.
- [Low] Mismatch between v2 changelog claims and implemented fixed regulator naming.
- [Low] Logically contradictory suspend states in PMIC regulators.
- [Low] Invalid `pwm-supply` property used in the PWM regulator `vdd_log`.
--

--- Patch [2]: [PATCH v3 2/2] arm64: dts: rockchip: Add Vicharak Vaaman board ---
commit 2ba03c57ee758ea3f8ebf1be9be041bcd351ec42
Author: Hrushiraj Gandhi <hrushirajg23@gmail.com>

arm64: dts: rockchip: Add Vicharak Vaaman board

Add initial devicetree support for the Vicharak Vaaman, an RK3399-based
single-board computer.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-vicharak-vaaman.dts b/arch/arm64/boot/dts/rockchip/rk3399-vicharak-vaaman.dts
> new file mode 100644
> index 0000000000000..01b8c1536c4cf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-vicharak-vaaman.dts

[ ... ]

> +	vcc5v0_sys: regulator-vcc-sys {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc5v0_sys";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc12v_dcin>;
> +	};

[Severity: Low]
This isn't a bug, but the v2 changelog claims fixed-regulator node names
were updated to use a regulator-[voltage] prefix consistently. Does this
node name intentionally omit the voltage?

[ ... ]

> +	vdd_log: regulator-vdd-log {
> +		compatible = "pwm-regulator";
> +		pwms = <&pwm2 0 25000 1>;
> +		pwm-supply = <&vcc5v0_sys>;

[Severity: Low]
Does the generic PWM regulator binding support pwm-supply? It seems the
proper property for an input supply would be vin-supply or power-supply,
which could cause the regulator core to ignore this supply dependency.

> +		regulator-name = "vdd_log";
> +		regulator-always-on;

[ ... ]

> +&gmac {
> +	assigned-clocks = <&cru SCLK_RMII_SRC>;
> +	assigned-clock-parents = <&clkin_gmac>;
> +	clock_in_out = "input";
> +	phy-supply = <&vcc3v3_lan>;

[Severity: Medium]
Should this phy-supply property be placed inside the child ethernet-phy
node (rtl8211e) rather than the MAC node? Standard ethernet PHY bindings
typically require the supply in the PHY node, which could prevent the PHY
driver from finding its supply and managing its power state.

> +	phy-mode = "rgmii";
> +	phy-handle = <&rtl8211e>;

[ ... ]

> +			vdd_center: DCDC_REG1 {
> +				regulator-name = "vdd_center";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <750000>;
> +				regulator-max-microvolt = <1350000>;
> +				regulator-ramp-delay = <6001>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +					regulator-suspend-microvolt = <900000>;

[Severity: Low]
Is it semantically valid to specify both regulator-off-in-suspend and
a specific regulator-suspend-microvolt? Requesting a voltage during
suspend while turning the regulator off seems contradictory.

> +				};
> +			};

[ ... ]

> +			vcca0v9_hdmi: LDO_REG7 {
> +				regulator-name = "vcca0v9_hdmi";
> +				regulator-always-on;
> +				regulator-boot-on;
> +				regulator-min-microvolt = <900000>;
> +				regulator-max-microvolt = <900000>;
> +				regulator-state-mem {
> +					regulator-off-in-suspend;
> +					regulator-suspend-microvolt = <900000>;

[Severity: Low]
Similar to vdd_center, this specifies both regulator-off-in-suspend and
a specific suspend voltage.

> +				};
> +			};

[ ... ]

> +&sdmmc {
> +	bus-width = <4>;
> +	cap-mmc-highspeed;
> +	cap-sd-highspeed;
> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
> +	disable-wp;
> +	max-frequency = <150000000>;

[Severity: Medium]
Will this 150MHz (UHS-I) configuration cause SD card initialization to fail?

The SDIO supply (vcc_sdio) is fixed exactly at 3.0V without a vqmmc-supply
provided, preventing the required switch to 1.8V for speeds above 50MHz:

	vcc_sdio: LDO_REG4 {
		...
		regulator-min-microvolt = <3000000>;
		regulator-max-microvolt = <3000000>;
		...
	};

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cd &sdmmc_cmd &sdmmc_bus4>;
> +	status = "okay";
> +};

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