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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] arm64: dts: rockchip: rk3588-embedded-111c: invalid phy@ node names in mdio
- [Low] arm64: dts: rockchip: unreferenced vcc3v3_m2 and pcie avdd regulators in 111c
--

commit 057af7753f22c58a1c6fea7ae240d761d88b6fe7
Author: Patrick Barsanti <patrick.barsanti@amarulasolutions.com>

arm64: dts: rockchip: Add Axelera Embedded 111c

This commit adds a minimal device tree for the RK3588-based Axelera Embedded
111c board featuring an on-board Axelera AI Metis AIPU.

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

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

[ ... ]

> +	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]
The v2 changelog mentions dropping regulators that were defined but never
referenced along with their corresponding pinctrls.

However, vcc3v3_m2, its parent supply vcc3v3_sus, and the four
pcie20_avdd0v85, pcie20_avdd1v8, pcie30_avdd0v75, and pcie30_avdd1v8
regulators are not referenced anywhere in rk3588-embedded-111c.dts (the only
enabled PCIe controller is &pcie3x4, which uses vpcie3v3-supply =
<&vcc_1v8_pcie>).

Because vcc3v3_m2 specifies regulator-always-on and regulator-boot-on, the
fixed-regulator driver will still claim gpio0 RK_PC4 (using pinctrl
pcie_pwren_h) and drive it high at boot even though no M.2 PCIe controller is
enabled.

Should vcc3v3_m2 (along with pcie_pwren_h and vcc3v3_sus) and the unreferenced
pcie20/pcie30 avdd regulators be dropped as well?

[ ... ]

> +&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]
Will make dtbs_check fail to match the rgmii_phy0 and rgmii_phy1 nodes against
Documentation/devicetree/bindings/net/ethernet-phy.yaml?

In Documentation/devicetree/bindings/net/ethernet-phy.yaml, the select block
only matches nodes whose name matches "^ethernet-phy(@[a-f0-9]+)?$":

select:
  properties:
    $nodename:
      pattern: "^ethernet-phy(@[a-f0-9]+)?$"

  required:
    - $nodename

Because the nodes under &mdio0 and &mdio1 are named phy@1 and phy@3 with
compatible = "ethernet-phy-ieee802.3-c22", dtbs_check reports that no schema
matches that compatible string. Should these nodes be named ethernet-phy@1 and
ethernet-phy@3 instead?

[ ... ]

> +	pci {
> +		pcie_pwren_h: pcie-pwren-h {
> +			rockchip,pins = <0 RK_PC4 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};

[ ... ]

> +	};

[ ... ]

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