Re: [PATCH 3/3] arm64: dts: rockchip: Add Axelera AI metis-sbc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] The `&hdmi0` node mixes HDMI M0 and M1 pinmux groups for its pins, which will cause the HDMI controller to miss Hot-Plug Detection (HPD) signals.
- [Medium] The `&sdhci` (eMMC) node specifies `full-pwr-cycle-in-suspend` but lacks the required `vmmc-supply` and `vqmmc-supply` regulator assignments, and the underlying PMIC supplies are not configured to turn off in suspend.
- [Medium] The `pmic@0` (RK806) node lacks a `pinctrl` assignment for its interrupt pin.
- [Medium] The `vcc3v3_hubreset` regulator is used as a hack to release a USB hub reset GPIO, and is incorrectly passed as the `phy-supply` to `&usbdp_phy1` while being marked as `regulator-always-on` and `regulator-boot-on`.
- [Low] The `vbus5v0_typec` and `vcc_3v3_sd_s0` regulators are defined but never referenced by any consumer node in the device tree, and lack the `regulator-always-on` property.
--

commit 4aee50a229d0828f7f68b373236eeefdfcd2b1b8
Author: Patrick Barsanti <patrick.barsanti@amarulasolutions.com>

arm64: dts: rockchip: Add Axelera AI metis-sbc

Add minimal device tree for the Axelera AI Metis Compute Board
(rk3588-metis-sbc).

Link: https://axelera.ai/evaluation-systems/metis-compute-board

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-metis-sbc.dts b/arch/arm64/boot/dts/rockchip/rk3588-metis-sbc.dts
> new file mode 100644
> index 0000000000000..56d734f9f25bc
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-metis-sbc.dts
> @@ -0,0 +1,840 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/soc/rockchip,vop2.h>
> +#include "rk3588.dtsi"
> +
> +/ {
> +	model = "Axelera AI Metis Compute Board";
> +	compatible = "axelera,metis-sbc", "rockchip,rk3588";

[ ... ]

> +	vbus5v0_typec: vbus5v0-typec {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio4 RK_PB4 GPIO_ACTIVE_HIGH>; /* TYPEC5V_PWREN */
> +		pinctrl-0 = <&typec5v_pwren>;
> +		pinctrl-names = "default";
> +		regulator-name = "usbc_ss_vbus";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		vin-supply = <&vcc5v0_sus>;
> +	};

[Severity: Low]
Are this regulator and vcc_3v3_sd_s0 actually used anywhere?

They lack the regulator-always-on property and are never referenced by any
consumer node in the device tree, which means the regulator framework will
never enable them.

[ ... ]

> +	vcc_3v3_sd_s0: vcc-3v3-sd-s0 {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio0 RK_PC5 GPIO_ACTIVE_HIGH>; /* SDMMC_PWREN */
> +		pinctrl-0 = <&sdmmc_pwren>;
> +		pinctrl-names = "default";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-name = "vsd_3v3";
> +		startup-delay-us = <1000000>;
> +		vin-supply = <&vcc_3v3_s3>;
> +	};

[Severity: Low]
Similar to vbus5v0_typec, is this regulator actually utilized?

It appears to be unreferenced dead code since it is not marked as always
on and is not linked to any consumer node.

[ ... ]

> +	vcc3v3_hubreset: vcc3v3-hubreset {
> +		compatible = "regulator-fixed";
> +		enable-active-high;
> +		gpio = <&gpio3 RK_PB2 GPIO_ACTIVE_HIGH>; /* USB_HUB_RST_N */
> +		pinctrl-0 = <&usb_hub_rst_n>;
> +		pinctrl-names = "default";
> +		regulator-always-on;
> +		regulator-boot-on;
> +		regulator-name = "vcc3v3_hubreset";
> +		vin-supply = <&vcc_3v3_s3>;
> +	};

[Severity: Medium]
Does marking this regulator with regulator-always-on and regulator-boot-on
defeat the intended reset sequencing mechanism?

It looks like this regulator is used to control a reset line for the
downstream hub and is later referenced as phy-supply in &usbdp_phy1 to allow
the PHY driver to sequence the reset. However, keeping it always on forces
the reset line to be de-asserted immediately at boot before the PHY driver
is ready, which can cause the hub to fail enumeration.

[ ... ]

> +&hdmi0 {
> +	pinctrl-0 = <&hdmim0_tx0_cec &hdmim0_tx0_hpd_sbc &hdmim0_tx0_scl &hdmim0_tx0_sda>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +};

[ ... ]

> +	hdmi {
> +		hdmim0_tx0_hpd_sbc: hdmim0-tx0-hpd-sbc {
> +			rockchip,pins = <3 RK_PD4 3 &pcfg_pull_none>;
> +		};
> +	};

[Severity: High]
Does mixing HDMI M0 and M1 pinmux groups break display hot-plug detection?

The hdmi0 node uses the M0 hardware pinmux group for CEC, SCL, and SDA, but
hdmim0_tx0_hpd_sbc is mapped to 3 RK_PD4 3, which corresponds to the M1 group.

Since Rockchip SoCs configure IOMUX routing globally for a peripheral via GRF,
and the controller is routed to the M0 group for CEC/I2C, won't the M1 HPD pin
be electrically ignored?

[ ... ]

> +&sdhci {
> +	bus-width = <8>;
> +	max-frequency = <200000000>;
> +	mmc-hs400-1_8v;
> +	mmc-hs400-enhanced-strobe;
> +	no-sd;
> +	no-sdio;
> +	non-removable;
> +	full-pwr-cycle-in-suspend;
> +	status = "okay";
> +};

[Severity: Medium]
Without explicit vmmc-supply and vqmmc-supply regulator assignments, can this
full-pwr-cycle-in-suspend property cause eMMC resume failures?

Because the underlying vcc_3v3_s3 regulator is configured with
regulator-on-in-suspend, power will not actually be removed during suspend.
Upon resume, the MMC core will attempt a full re-initialization expecting the
card to be idle, but since the card never lost power, the re-initialization
might fail.

[ ... ]

> +&spi2 {
> +	assigned-clock-rates = <200000000>;
> +	assigned-clocks = <&cru CLK_SPI2>;
> +	num-cs = <1>;
> +	pinctrl-0 = <&spi2m2_cs0 &spi2m2_pins>;
> +	pinctrl-names = "default";
> +	status = "okay";
> +
> +	pmic@0 {
> +		compatible = "rockchip,rk806";
> +		reg = <0x0>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <RK_PA7 IRQ_TYPE_LEVEL_LOW>; /* PMIC_INT_L */
> +		spi-max-frequency = <1000000>;
> +		system-power-controller;

[Severity: Medium]
Should there be a pinctrl assignment for the PMIC interrupt pin?

Relying entirely on the bootloader's default pin configuration could result
in the pin being left floating or misconfigured as an output, which can lead
to missed or spurious PMIC interrupts and power management failures.

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