Re: [PATCH v3 3/3] arm64: dts: rockchip: Add Axelera Embedded 111c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] arm64: dts: rockchip: missing domain-supply on pd_gpu in 111c
- [Medium] arm64: dts: rockchip: tsadc left disabled in 111c with active
  OPPs
- [Low] arm64: dts: rockchip: invalid phy@ node names in 111c mdio nodes
- [Low] arm64: dts: rockchip: unreferenced vcc3v3_m2 and pcie avdd
  regulators in 111c
--

commit 607ecf41e5900d1d5b78865cbefcac07bdaf3309
Author: Patrick Barsanti <patrick.barsanti@amarulasolutions.com>

arm64: dts: rockchip: Add Axelera Embedded 111c

This commit adds the initial device tree for the RK3588-based Axelera
Embedded 111c board.

Link: https://axelera.ai/axelera-products/embedded/111c

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-axelera-embedded-111c.dts b/arch/arm64/boot/dts/rockchip/rk3588-axelera-embedded-111c.dts
> new file mode 100644
> index 0000000000000..0d760ee0f4e61
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-axelera-embedded-111c.dts
> @@ -0,0 +1,810 @@

[ ... ]

> +	pcie20_avdd0v85: regulator-pcie20-avdd0v85 {
> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <850000>;
> +		regulator-max-microvolt = <850000>;
> +		regulator-name = "pcie20_avdd0v85";
> +		vin-supply = <&vdda_0v85_s0>;
> +	};
> +
> +	pcie20_avdd1v8: regulator-pcie20-avdd1v8 {
> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-name = "pcie20_avdd1v8";
> +		vin-supply = <&vcca_1v8_s0>;
> +	};
> +
> +	pcie30_avdd0v75: regulator-pcie30-avdd0v75 {
> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <750000>;
> +		regulator-max-microvolt = <750000>;
> +		regulator-name = "pcie30_avdd0v75";
> +		vin-supply = <&hdmi_vdda0v85_s0>;
> +	};
> +
> +	pcie30_avdd1v8: regulator-pcie30-avdd1v8 {
> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-name = "pcie30_avdd1v8";
> +		vin-supply = <&vcca_1v8_s0>;
> +	};

[ ... ]

> +	vcc3v3_m2: regulator-vcc3v3-m2 {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PC4 GPIO_ACTIVE_HIGH>; /* PCIE_PWREN_H */
> +		pinctrl-0 = <&pcie_pwren_h>;
> +		pinctrl-names = "default";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-name = "3v_m2";
> +		vin-supply = <&vcc3v3_sus>;
> +	};
> +
> +	vcc3v3_sus: regulator-vcc3v3-sus {
> +		compatible = "regulator-fixed";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-name = "3vsus";
> +		vin-supply = <&vcc12v_dcin>;
> +	};

[Severity: Low]
Are vcc3v3_m2, vcc3v3_sus, pcie20_avdd0v85, pcie20_avdd1v8,
pcie30_avdd0v75, and pcie30_avdd1v8 still needed here?  None of these
regulators (nor the pcie_pwren_h pinctrl group) are referenced anywhere in
this device tree, and vcc3v3_m2 unconditionally claims GPIO0_C4 and drives
the M.2 power rail high at boot even though only &pcie3x4 (supplied by
vcc_1v8_pcie) is enabled.

[ ... ]

> +&gpu {
> +	mali-supply = <&vdd_gpu_s0>;
> +	status = "okay";
> +};

[Severity: High]
Does &pd_gpu also need domain-supply = <&vdd_gpu_s0>?

In drivers/pmdomain/rockchip/pm-domains.c, RK3588_PD_GPU sets
need_regulator = true so rockchip_pd_regulator_enable() looks up
domain-supply before powering on the hardware domain.  Because vdd_gpu_s0
is only regulator-boot-on, if panthor probes as a module after unused
regulators are disabled or if the GPU domain is cycled across runtime PM,
genpd will substitute a dummy regulator and power on RK3588_PD_GPU while
vdd_gpu_s0 is off, hanging the system as described in f94500eb7328
("arm64: dts: rockchip: Add GPU power domain regulator dependency for
RK3588").

[ ... ]

> +&mdio0 {
> +	reset-gpios = <&gpio4 RK_PB3 GPIO_ACTIVE_LOW>; /* GMAC0_RST_N */
> +	reset-delay-us = <20000>;
> +	reset-post-delay-us = <100000>;
> +
> +	rgmii_phy0: phy@1 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <0x1>;
> +	};
> +};
> +
> +&mdio1 {
> +	reset-gpios = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>; /* GMAC1_RST_N */
> +	reset-delay-us = <20000>;
> +	reset-post-delay-us = <100000>;
> +
> +	rgmii_phy1: phy@3 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <0x3>;
> +	};
> +};

[Severity: Low]
Should phy@1 and phy@3 be named ethernet-phy@1 and ethernet-phy@3?
Documentation/devicetree/bindings/net/ethernet-phy.yaml selects nodes by
matching $nodename against ^ethernet-phy(@[a-f0-9]+)?$, so make dtbs_check
fails to match these nodes against the binding.

[ ... ]

> +&saradc {
> +	vref-supply = <&vcca_1v8_s0>;
> +	status = "okay";
> +};

[Severity: Medium]
Should &tsadc also be enabled here?

This board enables CPU and GPU voltage supplies with rk3588.dtsi OPPs,
allowing DVFS scaling up to the maximum turbo frequencies, while &tsadc
remains disabled by default in rk3588-base.dtsi.  Without &tsadc enabled,
all seven SoC thermal zones and hardware over-temperature shutdown remain
inactive.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113113808.1779860-1-patrick.barsanti@amarulasolutions.com?part=3