Re: [PATCH net-next v2 5/7] net: stmmac: mediatek: use TX clock phase shift in RGMII mode with 1Gbps speed

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] net: stmmac: mediatek: uninitialized variable use in mt8195_fix_mac_speed()
- [Medium] net: stmmac: mediatek: MT8195_DLY_GTXC_INV is unconditionally cleared at 1Gbps
- [Medium] net: stmmac: mediatek: GTXC delay macro enabled with 0 stages at 10/100Mbps
--

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

net: stmmac: mediatek: use TX clock phase shift in RGMII mode with 1Gbps speed

This patch introduces the dwmac_fix_mac_speed callback for the MT8195
MAC to configure the TX clock phase shift after link speed negotiation.
It prefers a 2ns fixed delay at 1Gbps speed and falls back to the HW
delay macro circuit for 10/100Mbps speeds.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index dee12cfa437d0..6123efbbaf026 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

[ ... ]

> @@ -433,9 +436,62 @@ static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  	return 0;
>  }
>  
> +static void mt8195_fix_mac_speed(void *priv, phy_interface_t interface,
> +				 int speed, unsigned int mode)
> +{
> +	struct mediatek_dwmac_plat_data *priv_plat = priv;
> +	const struct mediatek_dwmac_variant *variant;
> +	struct mac_delay_struct *mac_delay;
> +	u32 tx_delay_stage_val, reg_offset;
> +	u32 reg_val = 0;
> +
> +	if (!priv_plat)
> +		return;
> +
> +	mac_delay = &priv_plat->mac_delay;
> +	variant = priv_plat->variant;
> +
> +	if (!mac_delay->tx_delay ||
> +	    (interface != PHY_INTERFACE_MODE_RGMII &&
> +	     interface != PHY_INTERFACE_MODE_RGMII_RXID))
> +		return;
> +
> +	/*
> +	 * When link speed is 1Gbps with RGMII interface, and a TX internal
> +	 * delay needs to be applied on MAC, prefer to override the delay
> +	 * settings with a 2ns fixed delay which is controlled by
> +	 * RGMII_TXC_PHASE_CTRL. Otherwise, fallback to HW delay macro circuit
> +	 * for 10/100Mbps link speeds.
> +	 */
> +	if (speed == SPEED_1000) {
> +		reg_val = MT8195_RGMII_TXC_PHASE_CTRL;

[Severity: Medium]
Will this assignment cause the MT8195_DLY_GTXC_INV setting to be lost when
the link speed negotiates to 1Gbps? The subsequent regmap_update_bits() includes
MT8195_DLY_GTXC_INV in its mask, which appears to clear the inversion bit in
hardware if it was previously set.

> +	} else {
> +		if (variant->tx_delay_stage_div)
> +			tx_delay_stage_val = mac_delay->tx_delay /
> +					     variant->tx_delay_stage_div;
> +
> +		reg_val |= FIELD_PREP(MT8195_DLY_GTXC_ENABLE,
> +				      !!mac_delay->tx_delay);

[Severity: Medium]
Is it intentional to set the enable bit based on the raw mac_delay->tx_delay
instead of the computed tx_delay_stage_val? If the raw delay is smaller than
the divisor, tx_delay_stage_val truncates to 0, which would enable the GTXC
delay macro with 0 stages.

> +		reg_val |= FIELD_PREP(MT8195_DLY_GTXC_STAGES,
> +				      tx_delay_stage_val);

[Severity: Medium]
What happens if variant->tx_delay_stage_div is 0? The explicit check suggests
it could be 0, but without an else branch, tx_delay_stage_val remains
uninitialized and its stack memory would be written to the hardware register.

> +		reg_val |= FIELD_PREP(MT8195_DLY_GTXC_INV,
> +				      mac_delay->tx_inv);
> +	}
> +
> +	reg_offset = variant->peri_eth_ctrl_offset + MT8195_PERI_ETH_CTRL0;
> +	regmap_update_bits(priv_plat->peri_regmap,
> +			   reg_offset,
> +			   MT8195_RGMII_TXC_PHASE_CTRL |
> +			   MT8195_DLY_GTXC_ENABLE |
> +			   MT8195_DLY_GTXC_INV |
> +			   MT8195_DLY_GTXC_STAGES,
> +			   reg_val);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-dwmac-mediatek-mt8189-v2-0-430bd74d5ef9@collabora.com?part=5