Re: [PATCH v2 1/4] arm64: dts: ti: var-som-am62p: fix Ethernet PHY configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Converting the Ethernet PHY reset pin to an unclaimed `vdd-supply` regulator permanently breaks Ethernet.
- [Medium] Removal of hardware reset timing delays causes potential MDIO probe failures.
- [Medium] The `enet-phy-lane-no-swap` property is added but is not supported by the ADIN1300 PHY driver.
--

commit 3ee90bfeecffa9ab90466e7f1bf6b5a34e3bdc8e
Author: Stefano Radaelli <stefano.r@variscite.com>

arm64: dts: ti: var-som-am62p: fix Ethernet PHY configuration

This patch fixes the Ethernet device tree description on the VAR-SOM-AM62P.
It enables the CPSW Ethernet controller and corrects the Ethernet PHY
description by modelling the PHY power supply and adding board-specific
properties.

> diff --git a/arch/arm64/boot/dts/ti/k3-am62p5-var-som.dtsi b/arch/arm64/boot/dts/ti/k3-am62p5-var-som.dtsi
> index fc5a3942cde00..ca2483a04b9d9 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62p5-var-som.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62p5-var-som.dtsi
> @@ -113,6 +113,15 @@ reg_3v3_phy: regulator-3v3-phy {
>  		regulator-always-on;
>  	};
>  
> +	reg_eth_phy_vdd: regulator-eth-vdd {
> +		compatible = "regulator-fixed";
> +		regulator-name = "reg_eth_phy_vdd";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		gpio = <&main_gpio0 46 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +

[Severity: Medium]
Will this lack of startup delay cause MDIO probe failures?

The original PHY node explicitly defined reset-assert-us and reset-deassert-us
delays to satisfy the PHY's physical startup timing requirements. By shifting
the reset pin control to the regulator framework, these explicit delays were
lost. 

The new regulator definition lacks the equivalent startup-delay-us = <100000>
property, which means the kernel might immediately probe the MDIO bus without
waiting for the PHY to initialize.

[ ... ]

> @@ -159,9 +169,9 @@ &cpsw3g_mdio {
>  	cpsw3g_phy0: ethernet-phy@4 {
>  		compatible = "ethernet-phy-id0283.bc30";
>  		reg = <4>;
> -		reset-gpios = <&main_gpio0 46 GPIO_ACTIVE_LOW>;
> -		reset-assert-us = <10000>;
> -		reset-deassert-us = <100000>;
> +		bootph-all;
> +		enet-phy-lane-no-swap;

[Severity: Medium]
Does the ADIN1300 driver support the enet-phy-lane-no-swap property?

This property is added to the cpsw3g_phy0 node (an ADIN1300 PHY), but it
appears to only be parsed and implemented by specific drivers like the TI
DP83867 PHY driver. The ADIN1300 driver in drivers/net/phy/adin.c does not
seem to parse this property, so the lane-swap configuration might be silently
ignored.

> +		vdd-supply = <&reg_eth_phy_vdd>;

[Severity: High]
Could this permanently break Ethernet by holding the PHY in reset?

The commit moves control of the active-low PHY reset pin to a regulator-fixed
node assigned to vdd-supply. However, neither the Linux generic PHY framework
nor the ADIN1300 driver parses or claims vdd-supply.

During late initcall, the regulator framework automatically disables unclaimed
regulators that lack regulator-always-on. Disabling this active-high regulator
would drive GPIO 46 low, asserting reset on the PHY indefinitely.

>  	};
>  };

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