Re: [PATCH v3 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] RGMII TX pads are configured with lowest drive strength and slow slew rate, degrading the 125MHz Gigabit Ethernet transmission.
- [High] Race condition in PHY power initialization due to misplaced `phy-supply`.
- [Medium] The `vddio-regulator` subnode inside `ethphy1` lacks a `compatible` string and will be ignored by the regulator core.
--

commit fb47dd91efd03a99780078ebbf8ddbd840e641f1
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 6b2aaf6bae071..f4926ce0aec7e 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 +117,38 @@ &ecspi2 {
>  	status = "okay";
>  };
>  
> +&eqos {
> +	mdio {
> +		ethphy1: ethernet-phy@5 {
> +			compatible = "ethernet-phy-ieee802.3-c22";
> +			reg = <5>;
> +			reset-gpios = <&pca9534 5 GPIO_ACTIVE_LOW>;
> +			reset-assert-us = <10000>;
> +			reset-deassert-us = <20000>;
> +			vddio-supply = <&vddio1>;
> +
> +			vddio1: vddio-regulator {
> +				regulator-min-microvolt = <1800000>;
> +				regulator-max-microvolt = <1800000>;
> +			};

[Severity: Medium]
Does this subnode need a compatible string?

Without a string like "regulator-fixed", the Linux regulator framework might
not instantiate a regulator device for this node, rendering the vddio-supply
reference dead.

> +		};
> +	};
> +};
> +
> +&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]
Can this cause a race condition during PHY power initialization?

Since the phy-supply is assigned to the fec MAC node while the PHY is on the
eqos MDIO bus, eqos could probe and scan the MDIO bus before reg_fec_phy
powers up the PHY. Should phy-supply be placed inside the ethphy1 node to
ensure proper probe ordering?

> +	status = "okay";
> +};

[ ... ]

> @@ -363,6 +406,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]
Are these pad control values correct?

On the i.MX8MP, 0x00 maps to DSE_X1 (lowest drive strength) and FSEL=0 (slow
slew rate). For 125 MHz Gigabit Ethernet, could this cause severe signal edge
rounding and potential failure to negotiate a link?

> +		>;
> +	};

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