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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] At 1Gbps on RGMII, mt8195_fix_mac_speed()
  (drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c) treats the DT…
- [Medium] At 1Gbps, mt8195_fix_mac_speed()
  (drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c) silently clears…

--

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

This commit adds a fix_mac_speed callback to the MediaTek dwmac glue and
implements it for MT8195. When an RGMII link comes up at 1Gbps and the DT
requests a TX delay, the callback turns on the fixed 2ns TX clock phase
shift (RGMII_TXC_PHASE_CTRL). At other speeds it falls back to the coarse
delay macro circuit.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index 7dd46cd5a49e1..4b2d5280f7030 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

[ ... ]

> @@ -430,9 +433,65 @@ 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)
> +{

[ ... ]

> +	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]
At 1Gbps, does this treat mediatek,tx-delay-ps as an on/off switch rather
than as a delay amount?

The gate checks the raw picosecond value in mac_delay->tx_delay.
mediatek_dwmac_config_dt() stores that value without rounding:

    if (tx_delay_ps < plat->variant->tx_delay_max) {
        mac_delay->tx_delay = tx_delay_ps;

Both mt8195_set_delay() and the new 10/100 branch round the value down to
290ps stages (180ps on MT8189). So a value of 1..289ps means no delay at
probe time and at 10/100. At 1G, the same DT value now gets the full ~2ns
phase shift.

For every other value (290ps, 870ps, 8990ps, ...), the configured amount is
replaced by a fixed ~2ns at 1G. GTXC_ENABLE and GTXC_STAGES are in the
update mask, so the coarse delay is also cleared, with no warning.

The same DT setting therefore gives a different MAC TX delay depending on
link speed. Could boards that tuned tx-delay-ps to their PCB skew end up
with the wrong total skew at gigabit?

The binding in Documentation/devicetree/bindings/net/mediatek-dwmac.yaml
still describes the property as:

      For MT8188/MT8195 RGMII/RMII/MII interface, Allowed value need to be a
      multiple of 290, or will round down. Range 0~31*290.

The commit message also doesn't say that the configured value is ignored at
1G.

None of the in-tree boards seem to hit this. mt8395-genio-common and
mt8395-radxa-nio-12l use rgmii-rxid with 2030ps. mt8390-grinn-genio-sbc uses
30ps, but its phy-mode is rgmii-id, which skips this path.

The later commit "net: stmmac: mediatek: add support for MT8189 SoC" also
wires mt8195_fix_mac_speed() into the MT8189 variant, so MT8189 gets the
same behaviour.

> +	} else {
> +		if (variant->tx_rgmii_delay_stage_div) {
> +			tx_delay_stage_val = mac_delay->tx_delay /
> +					     variant->tx_rgmii_delay_stage_div;
> +		} else {
> +			tx_delay_stage_val = 0;
> +		}
> +
> +		reg_val |= FIELD_PREP(MT8195_DLY_GTXC_ENABLE,
> +				      !!tx_delay_stage_val);
> +		reg_val |= FIELD_PREP(MT8195_DLY_GTXC_STAGES,
> +				      tx_delay_stage_val);
> +		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);
> +}

[Severity: Medium]
Does the 1G path clear the TX clock inversion requested by
mediatek,txc-inverse?

MT8195_DLY_GTXC_INV is in the regmap_update_bits() mask. The SPEED_1000
branch, however, sets only MT8195_RGMII_TXC_PHASE_CTRL in reg_val. GTXC_INV
is therefore written as 0 whatever mac_delay->tx_inv says.

The 10/100 branch above keeps the inversion. mt8195_set_delay() also
applies it for all RGMII modes at probe and resume:

    gtxc_delay_val |= FIELD_PREP(MT8195_DLY_GTXC_INV, mac_delay->tx_inv);

The 1G case is also inconsistent with itself. With tx-delay-ps = 0, the
early return leaves the inversion in place. With any non-zero tx-delay-ps,
it is cleared.

The binding describes mediatek,txc-inverse as "tx clock will be inversed in
MII/RGMII case", with no exception for any speed. Should the SPEED_1000
branch also include FIELD_PREP(MT8195_DLY_GTXC_INV, mac_delay->tx_inv)?

No in-tree MediaTek board uses txc-inverse today. MT8189 is affected too,
once "net: stmmac: mediatek: add support for MT8189 SoC" wires in the same
callback.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016-dwmac-mediatek-mt8189-v3-0-25daf2bbcf35%40collabora.com