Re: [PATCH net-next v4 06/10] net: stmmac: mediatek: use standard internal delay properties

[sashiko-bot@kernel.org]

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

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

net: stmmac: mediatek: use standard internal delay properties

This commit updates the mediatek-dwmac driver to support the standard
rx-internal-delay-ps and tx-internal-delay-ps properties for RGMII internal
clock delays while falling back to the vendor mediatek,rx-delay-ps and
mediatek,tx-delay-ps properties when the standard ones are absent.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index cda3f3edde39f..b4abc20c83143 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

[ ... ]

> @@ -257,8 +259,8 @@ static const struct mediatek_dwmac_variant mt2712_gmac_variant = {
>  	.dwmac_set_delay = mt2712_set_delay,
>  	.clk_list = mt2712_dwmac_clk_l,
>  	.num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l),
> -	.rx_delay_max = MT2712_MII_DELAY_MAX,
> -	.tx_delay_max = MT2712_MII_DELAY_MAX,
> +	.mii_delay_max = MT2712_MII_DELAY_MAX,
> +	.rgmii_delay_max = MT2712_RGMII_DELAY_MAX,
>  	.dma_bit_mask = 33,
>  };

[ ... ]

> @@ -427,24 +432,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) {
> +		dev_err(plat->dev, "Invalid RGMII RX clock delay: %dps\n",
> +			rgmii_tx_delay_ps);
> +		return -EINVAL;
> +	}

[Severity: High]
In drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c, can
mediatek_dwmac_config_dt() reject valid MII and RMII delay values on MT2712?

For mt2712_gmac_variant, mii_delay_max is MT2712_MII_DELAY_MAX (17600 ps),
whereas rgmii_delay_max is MT2712_RGMII_DELAY_MAX (5440 ps).

When the interface is configured for MII or RMII, the RGMII-specific
tx-internal-delay-ps and rx-internal-delay-ps properties are absent from the
device tree, so rgmii_tx_delay_ps and rgmii_rx_delay_ps fall back to
tx_delay_ps and rx_delay_ps.

Because mediatek_dwmac_config_dt() then unconditionally checks
rgmii_tx_delay_ps and rgmii_rx_delay_ps against plat->variant->rgmii_delay_max
regardless of phy_mode, won't any MT2712 MII or RMII configuration with
mediatek,tx-delay-ps or mediatek,rx-delay-ps between 5440 ps and 17599 ps fail
probe with -EINVAL?

[Severity: Low]
Also in mediatek_dwmac_config_dt() in
drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c, when rgmii_rx_delay_ps
is greater than or equal to plat->variant->rgmii_delay_max, should the
dev_err() call log rgmii_rx_delay_ps instead of rgmii_tx_delay_ps?

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