Re: [PATCH v1 04/15] arm64: dts: freescale: imx8mn-var-som: Update FEC support with MaxLinear PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying the shared SOM `.dtsi` and carrier `.dts` in-place to support the latest hardware revision breaks backwards compatibility with older deployed SOMs and carrier boards.
- [Medium] The commit message claims to add a pinctrl sleep state for low-power operation, but the patch modifies the default active pinctrl state instead, and the sleep state already existed.
--

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

arm64: dts: freescale: imx8mn-var-som: Update FEC support with MaxLinear PHY

Update the FEC Ethernet controller on the i.MX8MN VAR-SOM to match the
latest SOM hardware revision using the integrated MaxLinear MXL86110 PHY.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts b/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts
> index 7308613a48b2b..520aff23e5872 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-var-som-symphony.dts
[ ... ]
> @@ -74,7 +75,24 @@ reg_per_3v3: regulator-peripheral-3v3 {
>  };
>  
>  &ethphy {
> -	reset-gpios = <&pca9534 5 GPIO_ACTIVE_HIGH>;
> +	leds {
> +		#address-cells = <1>;

[Severity: High]
Does removing the carrier board reset GPIO override break backwards
compatibility with older deployed SOMs and carrier boards?

Booting a new kernel on older hardware (using AR8033 or ADIN1300 PHYs) will
use the new MaxLinear PHY configuration and drop the carrier board's pca9534
reset GPIO line.

While later patches in this series add a legacy DTB to restore legacy Wi-Fi
configuration, it includes imx8mn-var-som-symphony.dts without overriding
the Ethernet configuration. Legacy boards might still receive the broken
Ethernet bindings and lose networking.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-var-som.dtsi b/arch/arm64/boot/dts/freescale/imx8mn-var-som.dtsi
> index 71a2f0866822f..09146ab763858 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mn-var-som.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-var-som.dtsi
[ ... ]
> @@ -96,24 +103,19 @@ &fec1 {
>  	phy-mode = "rgmii";
>  	phy-handle = <&ethphy>;
>  	phy-supply = <&reg_eth_phy>;
> -	fsl,magic-packet;
>  	status = "okay";
>  
>  	mdio {
>  		#address-cells = <1>;
>  		#size-cells = <0>;
>  
> -		ethphy: ethernet-phy@4 { /* AR8033 or ADIN1300 */
> +		ethphy: ethernet-phy@4 {
>  			compatible = "ethernet-phy-ieee802.3-c22";
>  			reg = <4>;
>  			reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
>  			reset-assert-us = <10000>;
> -			/*
> -			 * Deassert delay:
> -			 * ADIN1300 requires 5ms.
> -			 * AR8033   requires 1ms.
> -			 */
> -			reset-deassert-us = <20000>;
> +			reset-deassert-us = <100000>;
> +			vddio-supply = <&reg_phy_vddio>;
>  		};
>  	};
>  };

[Severity: High]
Similarly, overwriting the shared SOM description in-place for the new
hardware revision effectively drops Ethernet support for older models.

Will older revisions of the SOM that use the AR8033 or ADIN1300 PHYs fail
to probe the Ethernet PHY after this change?

[ ... ]
> @@ -342,7 +344,7 @@ MX8MN_IOMUXC_ENET_TXC_ENET1_RGMII_TXC		0x1f
>  			MX8MN_IOMUXC_ENET_RXC_ENET1_RGMII_RXC		0x91
>  			MX8MN_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL	0x91
>  			MX8MN_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL	0x1f
> -			MX8MN_IOMUXC_GPIO1_IO09_GPIO1_IO9		0x159
> +			MX8MN_IOMUXC_GPIO1_IO09_GPIO1_IO9		0x19
>  		>;
>  	};

[Severity: Medium]
The commit message states this patch will "add a pinctrl sleep state for
low-power operation." Is this description accurate?

The patch modifies the default active pinctrl state, rather than adding or
updating a sleep state, which already existed. Could the commit message be
clarified to reflect that the pull-up configuration on the reset pin in the
default state is being altered?

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