Re: [PATCH v5 13/14] arm64: dts: imx8mp-var-som-symphony: add second Ethernet port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Cross-MAC PHY topology causes a probe failure because the PHY's power regulator is bound to the consumer MAC and defaults to off during MDIO bus scanning.
- [High] RGMII TX pads for `fec` are configured with a value of `0x00`, resulting in weak drive strength and slow slew rate, which is inadequate for Gigabit Ethernet.
- [High] The PHY hardware ID will be read before its `reset-gpios` is deasserted, causing MDIO detection to fail if the PHY boots in reset.
--

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

arm64: dts: imx8mp-var-som-symphony: add second Ethernet port
    
Add support for the second Ethernet interface available on the Symphony
carrier board. Enable the FEC controller, add the MaxLinear MXL86110
Gigabit Ethernet PHY on the MDIO bus, describe its reset and power
configuration, and add the required RGMII pinctrl settings.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-var-som-symphony.dts b/arch/arm64/boot/dts/freescale/imx8mp-var-som-symphony.dts
> index 93dee9ec0b9bd..e928a3d44b1d5 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-var-som-symphony.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-var-som-symphony.dts
[ ... ]
> @@ -106,6 +116,33 @@ &ecspi2 {
>  	status = "okay";
>  };
>  
> +&eqos {
> +	mdio {
> +		ethphy1: ethernet-phy@5 {
> +			compatible = "ethernet-phy-ieee802.3-c22";
> +			reg = <5>;
> +			reset-gpios = <&pca9534 5 GPIO_ACTIVE_LOW>;

[Severity: High]
Will the MDIO core be able to detect this PHY if it boots in reset?

Because the specific PHY ID isn't provided in the compatible string, the Linux
MDIO core will attempt to read the hardware ID over the bus before creating
the PHY device structure.

However, the reset-gpios line is only acquired and deasserted later during
phy_device_register(). If the PHY powers up held in reset by the GPIO
expander, the initial MDIO read will fail.

Would it be better to specify the PHY ID in the compatible string
(e.g., ethernet-phy-idXXXX.YYYY) so the core knows to skip the ID read?

> +			reset-assert-us = <10000>;
> +			reset-deassert-us = <20000>;
> +			vddio-supply = <&reg_phy_vddio>;
> +		};
> +	};
> +};
> +
> +&fec {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_fec>;
> +	/*
> +	 * The required RGMII TX and RX 2ns delays are implemented directly
> +	 * in hardware via passive delay elements on the SOM PCB.
> +	 * No delay configuration is needed in software via PHY driver.
> +	 */
> +	phy-mode = "rgmii";
> +	phy-handle = <&ethphy1>;
> +	phy-supply = <&reg_fec_phy>;

[Severity: High]
Could there be a probe failure due to the PHY being unpowered during the MDIO
bus scan?

The ethphy1 node is attached to the &eqos MDIO bus, but its power regulator
reg_fec_phy is provided via phy-supply in the &fec MAC node.

Since the regulator lacks regulator-always-on, it defaults to off. When &eqos
probes and registers its MDIO bus, it will scan for ethphy1, but the PHY will
be unpowered because the &fec MAC hasn't probed yet.

Should phy-supply be moved to the ethphy1 node instead?

> +	status = "okay";
> +};
[ ... ]
> @@ -363,6 +400,23 @@ MX8MP_IOMUXC_ECSPI2_SS0__GPIO5_IO13				0x12
>  		>;
>  	};
>  
> +	pinctrl_fec: fecgrp {
> +		fsl,pins = <
> +			MX8MP_IOMUXC_SAI1_RXD4__ENET1_RGMII_RD0				0x90
> +			MX8MP_IOMUXC_SAI1_RXD5__ENET1_RGMII_RD1				0x90
> +			MX8MP_IOMUXC_SAI1_RXD6__ENET1_RGMII_RD2				0x90
> +			MX8MP_IOMUXC_SAI1_RXD7__ENET1_RGMII_RD3				0x90
> +			MX8MP_IOMUXC_SAI1_TXC__ENET1_RGMII_RXC				0x90
> +			MX8MP_IOMUXC_SAI1_TXFS__ENET1_RGMII_RX_CTL			0x90
> +			MX8MP_IOMUXC_SAI1_TXD0__ENET1_RGMII_TD0				0x00
> +			MX8MP_IOMUXC_SAI1_TXD1__ENET1_RGMII_TD1				0x00
> +			MX8MP_IOMUXC_SAI1_TXD2__ENET1_RGMII_TD2				0x00
> +			MX8MP_IOMUXC_SAI1_TXD3__ENET1_RGMII_TD3				0x00
> +			MX8MP_IOMUXC_SAI1_TXD4__ENET1_RGMII_TX_CTL			0x00
> +			MX8MP_IOMUXC_SAI1_TXD5__ENET1_RGMII_TXC				0x00

[Severity: High]
Does a pad value of 0x00 provide adequate signal integrity for Gigabit
Ethernet?

On the i.MX8MP pad controller, a value of 0x00 configures the Slew Rate to
slow and Drive Strength to its weakest setting.

Since Gigabit RGMII requires a 125 MHz clock with tight timing margins, could
this weak drive strength and slow slew rate cause unpredictable packet loss or
link failures?

> +		>;
> +	};

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