Re: [PATCH net-next v4 10/10] net: stmmac: mediatek: add support for MT8189 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] net: stmmac: mediatek: rgmii_delay_max applied to MII/RMII fallback delays
- [Low] net: stmmac: mediatek: wrong delay logged in mediatek_dwmac_config_dt()
--

commit 05a1a870b144ad38c76a76fcfec8bbabb47b798b
Author: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

net: stmmac: mediatek: add support for MT8189 SoC

This series refactors internal RX/TX clock delay handling in the MediaTek
DWMAC driver to support standard RGMII delay properties and adds support for
the MT8189 SoC.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index 30ae0dba7fffd..07d0c186ab655 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

[ ... ]

> @@ -434,24 +492,56 @@ static int mediatek_dwmac_config_dt(struct mediatek_dwmac_plat_data *plat)
>  		return PTR_ERR(plat->peri_regmap);
>  	}
>  
> -	if (!of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps)) {
> -		if (tx_delay_ps < plat->variant->tx_delay_max) {
> -			mac_delay->tx_delay = tx_delay_ps;
> -		} else {
> -			dev_err(plat->dev, "Invalid TX clock delay: %dps\n", tx_delay_ps);
> -			return -EINVAL;
> -		}
> +	of_property_read_u32(plat->np, "mediatek,tx-delay-ps", &tx_delay_ps);
> +	if (tx_delay_ps >= plat->variant->mii_delay_max) {
> +		dev_err(plat->dev, "Invalid TX clock delay: %dps\n",
> +			tx_delay_ps);
> +		return -EINVAL;
>  	}
>  
> -	if (!of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps)) {
> -		if (rx_delay_ps < plat->variant->rx_delay_max) {
> -			mac_delay->rx_delay = rx_delay_ps;
> -		} else {
> -			dev_err(plat->dev, "Invalid RX clock delay: %dps\n", rx_delay_ps);
> -			return -EINVAL;
> -		}
> +	/* Read RGMII TX internal delay standard property */
> +	if (of_property_read_u32(plat->np, "tx-internal-delay-ps",
> +				 &rgmii_tx_delay_ps)) {
> +		/*
> +		 * standard property value not available, fallback on vendor
> +		 * value instead
> +		 */
> +		rgmii_tx_delay_ps = tx_delay_ps;
>  	}
>  
> +	if (rgmii_tx_delay_ps >= plat->variant->rgmii_delay_max) {
> +		dev_err(plat->dev, "Invalid RGMII TX clock delay: %dps\n",
> +			rgmii_tx_delay_ps);
> +		return -EINVAL;
> +	}
> +
> +	of_property_read_u32(plat->np, "mediatek,rx-delay-ps", &rx_delay_ps);
> +	if (rx_delay_ps >= plat->variant->mii_delay_max) {
> +		dev_err(plat->dev, "Invalid RX clock delay: %dps\n",
> +			rx_delay_ps);
> +		return -EINVAL;
> +	}
> +
> +	/* Read RGMII RX internal delay standard property */
> +	if (of_property_read_u32(plat->np, "rx-internal-delay-ps",
> +				 &rgmii_rx_delay_ps)) {
> +		/*
> +		 * standard property value not available, fallback on vendor
> +		 * value instead
> +		 */
> +		rgmii_rx_delay_ps = rx_delay_ps;
> +	}
> +
> +	if (rgmii_rx_delay_ps >= plat->variant->rgmii_delay_max) {

[Severity: Medium]
In drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c, when
mediatek_dwmac_probe() calls mediatek_dwmac_config_dt(), will checking
rgmii_tx_delay_ps and rgmii_rx_delay_ps against
plat->variant->rgmii_delay_max unconditionally reject valid MII and RMII
configurations on SoCs where mii_delay_max is larger than rgmii_delay_max?

On MT2712, MT2712_MII_DELAY_MAX is 17600ps while MT2712_RGMII_DELAY_MAX is
5440ps:

drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c:
#define MT2712_MII_DLY_STAGE_DIV	550 /* 550ps per stage */
#define MT2712_MII_DELAY_MAX		17600 /* 32 x 550ps */
#define MT2712_RGMII_DLY_STAGE_DIV	170 /* 170ps per stage */
#define MT2712_RGMII_DELAY_MAX		5440 /* 32 x 170ps */

When a board uses MII or RMII mode and specifies mediatek,tx-delay-ps or
mediatek,rx-delay-ps in the range [5440, 17600) without the RGMII-specific
tx-internal-delay-ps or rx-internal-delay-ps properties, rgmii_tx_delay_ps
and rgmii_rx_delay_ps fall back to tx_delay_ps and rx_delay_ps and then fail
the plat->variant->rgmii_delay_max check with -EINVAL.

Should the rgmii_delay_max checks only be enforced when the PHY interface
is in RGMII mode?

> +		dev_err(plat->dev, "Invalid RGMII RX clock delay: %dps\n",
> +			rgmii_tx_delay_ps);
> +		return -EINVAL;
> +	}

[Severity: Low]
In mediatek_dwmac_config_dt(), when rgmii_rx_delay_ps >=
plat->variant->rgmii_delay_max, dev_err() passes rgmii_tx_delay_ps instead
of rgmii_rx_delay_ps. Should this log rgmii_rx_delay_ps instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-dwmac-mediatek-mt8189-v4-0-c86c14fc50f4@collabora.com?part=10