Re: [PATCH v1 2/3] arm64: dts: ti: Add support for Variscite VAR-SOM-AM62

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `reg_vdd_mmc2` (wifi_power) regulator is missing its `gpio` property and incorrectly configures Wi-Fi/BT enable pins as floating inputs.
- [High] Race condition in Ethernet PHY initialization due to incorrect pinmux placement.
- [Medium] Blind copy-paste of WM8904 codec hardware configuration from bindings.
- [Low] Usage of the incorrect `AM62PX_MCU_IOPAD` macro instead of `AM62X_MCU_IOPAD` in an AM625 device tree.
- [Low] Copy-paste artifact in the OPP table comment referencing the TI `am625-sk` evaluation board.
--

--- Patch [2]: [PATCH v1 2/3] arm64: dts: ti: Add support for Variscite VAR-SOM-AM62 ---
commit f91b9416e264c64f84899f5bbd68c1deb9597b14
Author: Stefano Radaelli <stefano.r@variscite.com>

arm64: dts: ti: Add support for Variscite VAR-SOM-AM62

This commit adds device tree support for the Variscite VAR-SOM-AM62 system
on module. It enables default SOM-specific peripherals like MMC, Ethernet,
Audio, and Wi-Fi.

Link: https://variscite.com/system-on-module-som/ti-sitara-am62x/var-som-am62/

> diff --git a/arch/arm64/boot/dts/ti/k3-am625-var-som.dtsi b/arch/arm64/boot/dts/ti/k3-am625-var-som.dtsi
> new file mode 100644
> index 0000000000000..95dd96fe4c66d
> --- /dev/null
> +++ b/arch/arm64/boot/dts/ti/k3-am625-var-som.dtsi
> @@ -0,0 +1,491 @@
[ ... ]
> +	opp-table {
> +		/* Add 1.4GHz OPP for am625-sk board. Requires VDD_CORE to be at 0.85V */
> +		opp-1400000000 {
> +			opp-hz = /bits/ 64 <1400000000>;

[Severity: Low]
Is this comment leftover from a copy-paste? It references the am625-sk
evaluation board rather than the var-som board.

> [ ... ]
> +	reg_3v3_phy: regulator-3v3-phy {
> +		compatible = "regulator-fixed";
> +		regulator-name = "On-module +V3.3_PHY";
> +		gpios = <&main_gpio0 45 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		regulator-always-on;
> +	};

[Severity: High]
Can this cause a race condition during Ethernet PHY initialization?

The PHY regulator reg_3v3_phy is set to always-on and asserts GPIO 45
in software. However, the physical pinmux for power (GPIO0_45) and reset
(GPIO0_46) is tied to the Ethernet MAC (cpsw3g) later in the file:

	pinctrl_rgmii1: main-rgmii1-default-pins {
		bootph-all;
		pinctrl-single,pins = <
			...
			AM62X_IOPAD(0x00bc, PIN_INPUT, 7) /* (V24) VOUT0_DATA1.GPIO0_46 */
			AM62X_IOPAD(0x00b8, PIN_INPUT, 7) /* (U22) VOUT0_DATA0.GPIO0_45 */
		>;
	};

When the MAC probes, it applies the pinmux, causing the PHY to receive power
instantly. Since the PHY lacks a phy-supply link to this regulator, there is a
risk of a 0ms power-on delay before the MAC registers the MDIO bus and probes
the PHY, violating its power-on and reset timing requirements.

> +
> +	reg_vdd_mmc2: regulator-vdd-wifi {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_vdd_mmc2>;
> +		compatible = "regulator-fixed";
> +		regulator-name = "wifi_power";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-always-on;
> +	};

[Severity: High]
Does this regulator fail to power on the Wi-Fi/BT module?

This wifi_power fixed regulator applies pinctrl_vdd_mmc2 but lacks a
gpio = <...> property, so it acts as a dummy software regulator without
actively toggling any pins.

Furthermore, in pinctrl_vdd_mmc2 further down:

	pinctrl_vdd_mmc2: main-vdd-mmc2-default-pins {
		pinctrl-single,pins = <
			AM62X_IOPAD(0x00d8, PIN_INPUT, 7) /* (V21) VOUT0_DATA8.GPIO0_53 */
			AM62X_IOPAD(0x00dc, PIN_INPUT, 7) /* (W21) VOUT0_DATA9.GPIO0_54 */
			AM62X_IOPAD(0x00f0, PIN_INPUT, 7) /* (Y22) VOUT0_DATA14.GPIO0_59 */
			AM62X_IOPAD(0x00f4, PIN_INPUT, 7) /* (AA21) VOUT0_DATA15.GPIO0_60 */
		>;
	};

These four GPIOs are configured as PIN_INPUT (floating). If any of these are
hardware enable pins (like WL_EN), won't they remain unasserted and leave the
module powered off?

> [ ... ]
> +	wm8904: audio-codec@1a {
> +		compatible = "wlf,wm8904";
> [ ... ]
> +		wlf,drc-cfg-regs = /bits/ 16 <0x01af 0x3248 0x0000 0x0000>,
> +				   /bits/ 16 <0x04af 0x324b 0x0010 0x0408>,
> +				   /bits/ 16 <0x04af 0x324b 0x0028 0x0704>,
> +				   /bits/ 16 <0x04af 0x324b 0x0018 0x078c>,
> +				   /bits/ 16 <0x04af 0x324b 0x0010 0x050e>;
> +		/* GPIO1 = DMIC_CLK, don't touch others */
> +		wlf,gpio-cfg = <0x0018>, <0xffff>, <0xffff>, <0xffff>;
> +	};

[Severity: Medium]
Are these DRC and GPIO configurations intended for this specific board?

This looks like a verbatim copy-paste from the device tree bindings example. It
forces GPIO1 to be driven as an output clock (wlf,gpio-cfg = <0x0018>). Could
this cause electrical contention if GPIO1 is wired differently on the
VAR-SOM-AM62?

> [ ... ]
> +&mcu_pmx0 {
> +	pinctrl_wkup_clkout0: wkup-clkout0-default-pins {
> +		pinctrl-single,pins = <
> +			AM62PX_MCU_IOPAD(0x0084, PIN_OUTPUT, 0) /* (F13) WKUP_CLKOUT0 */
> +		>;
> +	};
> +};

[Severity: Low]
Should this use the AM62X_MCU_IOPAD macro instead of AM62PX_MCU_IOPAD?

The device tree is for an AM625, but this uses a macro intended for the AM62P
SoC family.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783545241.git.stefano.r@variscite.com?part=2