Re: [PATCH PRELIMINARY 4/5] arm64: dts: qcom: shikra-cqs-evk: enable the first Ethernet port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Ethernet PHY remains permanently unpowered because the `vdda-2p5-supply` property is ignored and the regulator defaults to off.
--

commit b5b6a106e748058add8ac1a40dcbd3ae6ad2a0d0
Author: Mohd Ayaan Anwar <mohd.anwar@oss.qualcomm.com>

arm64: dts: qcom: shikra-cqs-evk: enable the first Ethernet port

This commit enables ethernet0 for the Shikra CQS EVK board with a DP83867
RGMII PHY. It also adds a fixed 2.5V regulator controlled by a GPIO to power
the PHY.

> diff --git a/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts b/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts
> index e62ba5aef71ff..7b30c67c68e8c 100644
> --- a/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts
> +++ b/arch/arm64/boot/dts/qcom/shikra-cqs-evk.dts
> @@ -21,6 +21,42 @@ aliases {
>  	chosen {
>  		stdout-path = "serial0:115200n8";
>  	};
> +
> +	rgmii_phy_supply: regulator-rgmii-phy {
> +		compatible = "regulator-fixed";
> +		regulator-name = "rgmii-phy-pwr";

[Severity: High]
Will this regulator ever be enabled by the core? 

Since no consumer driver will explicitly request it (see below), and the node 
is missing the regulator-always-on property, it appears the regulator core 
will leave it disabled at boot.

> +		regulator-min-microvolt = <2500000>;
> +		regulator-max-microvolt = <2500000>;
> +		gpio = <&tlmm 149 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		pinctrl-0 = <&rgmii_phy_pwr_default>;
> +		pinctrl-names = "default";
> +	};
> +};

[ ... ]

> +		rgmii_phy: ethernet-phy@7 {
> +			compatible = "ethernet-phy-id2000.a231";
> +			reg = <0x7>;
> +			reset-gpios = <&tlmm 135 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <10000>;
> +			reset-deassert-us = <50000>;
> +			vdda-2p5-supply = <&rgmii_phy_supply>;

[Severity: High]
Does the kernel actually use the vdda-2p5-supply property? 

It seems this custom property is completely ignored because no driver, 
including the DP83867 driver or the MDIO core, parses vdda-2p5-supply. 
Without a driver acting as a consumer, the PHY will fail to power on.

It looks like this same pattern might also be replicated in patch 5/5 for 
the second Ethernet port.

> +		};
> +	};
>  };

[ ... ]

> +&tlmm {
> +	rgmii_phy_pwr_default: rgmii-phy-pwr-default-state {
> +		pins = "gpio149";
> +		function = "gpio";
> +		drive-strength = <2>;
> +		bias-disable;
> +		output-low;

[Severity: High]
Is it intentional to drive the enable GPIO low here at boot? 

The regulator is defined as active-high above, so this default pinctrl 
state explicitly turns off the active-high regulator. Combined with the 
ignored supply property, this would leave the Ethernet PHY permanently 
unpowered and non-functional unconditionally at boot.

> +	};
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-shikra_ethernet_dts-v1-0-69c0c5c7c124@oss.qualcomm.com?part=4