Re: [PATCH v4 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] The RGMII TX pads in `pinctrl_fec` are misconfigured with a value of `0x00`, resulting in a slow slew rate and minimal drive strength.
- [Low] The `reg_phy_vddio` node and label are redundantly redefined, which will cause a Device Tree Compiler duplicate label error.
- [Low] Potential missing RGMII delay configuration and misleading comment due to a blindly copy-pasted `phy-mode`.
--

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

arm64: dts: imx8mp-var-som-symphony: add second Ethernet port

This commit adds support for the second Ethernet interface on the Symphony
carrier board. It enables the FEC controller, configures the MaxLinear PHY
on the MDIO bus, and sets up 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 7ad57caa0f1c2..c5a38018d494b 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp-var-som-symphony.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mp-var-som-symphony.dts
> @@ -66,6 +66,24 @@ pcie0_refclk: pcie0-refclk {
>  		clock-frequency = <100000000>;
>  	};
>  
> +	reg_fec_phy: regulator-fec-phy {
> +		compatible = "regulator-fixed";
> +		regulator-name = "fec-phy";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +		regulator-enable-ramp-delay = <20000>;
> +		gpio = <&pca9534 7 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		regulator-always-on;
> +	};
> +
> +	reg_phy_vddio: regulator-phy-vddio {
> +		compatible = "regulator-fixed";
> +		regulator-name = "vddio-1v8";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +	};

[Severity: Low]
Will this cause a Device Tree Compiler duplicate label error?

The reg_phy_vddio label and its node appear to already be defined at the root
level in the included imx8mp-var-som.dtsi file. Redefining it directly here
in the symphony dts usually causes dtc to fail compilation.

[ ... ]
> @@ -106,6 +124,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>;
> +			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";

[Severity: Low]
Is this comment accurate for this interface, or was it copy-pasted from the
SOM eqos interface?

The comment states the delays are implemented via passive delay elements
on the SOM PCB, but this Ethernet PHY is located on the Symphony carrier
board.

If the carrier board lacks these passive delay elements, should phy-mode be
set to "rgmii-id" instead?

> +	phy-handle = <&ethphy1>;
> +	phy-supply = <&reg_fec_phy>;
> +	status = "okay";
> +};

[ ... ]
> @@ -363,6 +408,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]
Will configuring these RGMII TX pads with 0x00 degrade signal integrity?

In the i.MX8MP pad configuration, a value of 0x00 sets a Slow Slew Rate
(SRE=0) and the weakest Drive Strength (DSE=X1). RGMII interfaces run at
125 MHz DDR and typically require a fast slew rate and high drive strength
(such as 0x16 or 0x1f).

Does using 0x00 risk heavy distortion, TX packet loss, or a complete link
failure?

> +		>;
> +	};

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