Re: [PATCH net-next v3 5/9] net: stmmac: mediatek: add RX/TX delay stage divider in platform data

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] In the variant data this patch adds for MT2712, the delay-stage
  dividers are the wrong way round for the hardware, and the RGMII TX…

--

net: stmmac: mediatek: add RX/TX delay stage divider in platform data

This commit adds RMII and RGMII RX/TX delay stage divider fields to
struct mediatek_dwmac_variant. It makes mt2712_set_delay() and
mt8195_set_delay() read the dividers from the variant data instead of
hardcoded constants, and it guards the divisions against a zero divider.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index 596411d4a5b2b..cb5bda53edb94 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

[ ... ]

> @@ -258,6 +270,10 @@ static const struct mediatek_dwmac_variant mt2712_gmac_variant = {
>  	.num_clks = ARRAY_SIZE(mt2712_dwmac_clk_l),
>  	.rx_delay_max = MT2712_RGMII_DLY_RXC_MAX,
>  	.tx_delay_max = MT2712_RGMII_DLY_TXC_MAX,
> +	.rx_rmii_delay_stage_div = MT2712_RMII_DLY_RXC_STAGE_DIV,
> +	.tx_rmii_delay_stage_div = MT2712_RMII_DLY_TXC_STAGE_DIV,
> +	.rx_rgmii_delay_stage_div = MT2712_RGMII_DLY_RXC_STAGE_DIV,
> +	.tx_rgmii_delay_stage_div = MT2712_RMII_DLY_TXC_STAGE_DIV,
                                    ^^^^

[Severity: High]

Should this be MT2712_RGMII_DLY_TXC_STAGE_DIV? The RX line above uses
the RGMII constant, and after this line MT2712_RGMII_DLY_TXC_STAGE_DIV
has no users.

Compared with the parent commit, this changes the RGMII TX (GTXC)
divider in mt2712_set_delay() from 550 to 170. The commit message
doesn't mention that change.

There also seems to be a wider problem with the values. The previous
patch in this series defined:

#define MT2712_RMII_DLY_RXC_STAGE_DIV	170 /* 170ps per stage */
#define MT2712_RMII_DLY_TXC_STAGE_DIV	170 /* 170ps per stage */

#define MT2712_RGMII_DLY_RXC_STAGE_DIV	550 /* 550ps per stage */
[ ... ]
#define MT2712_RGMII_DLY_TXC_STAGE_DIV	550 /* 550ps per stage */

The original mt2712_delay_ps2stage() used the opposite mapping:

	case PHY_INTERFACE_MODE_MII:
	case PHY_INTERFACE_MODE_RMII:
		/* 550ps per stage for MII/RMII */
		mac_delay->tx_delay /= 550;
		mac_delay->rx_delay /= 550;
		break;
	[ ... ]
		/* 170ps per stage for RGMII */
		mac_delay->tx_delay /= 170;
		mac_delay->rx_delay /= 170;

The binding in mediatek-dwmac.yaml matches the original code:

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

The commit message of that earlier patch also seems to describe the
original code the wrong way round (550 for RGMII, 170 for MII/RMII).

With those values now stored in mt2712_gmac_variant, mt2712_set_delay()
divides by:

  RGMII RX:        550 (originally 170)
  RGMII TX:        170 (correct only because of the constant above)
  MII/RMII RX/TX:  170 (originally 550)

Wouldn't this write the wrong stage counts to PERI_ETH_DLY?

For example, RGMII with mediatek,rx-delay-ps = <1700> gives 3 RXC
stages instead of 10. MII with 1100 ps gives 6 stages instead of 2.

RMII with 5500 ps is inside the binding's 0~31*550 range, but
5500 / 170 = 32. FIELD_PREP() masks that to 0 in the 5-bit
ETH_DLY_*_STAGES field, while the ENABLE bit is still set from
!!rx_delay_stage_val.

The in-tree mt2712-evb (rgmii-rxid, tx-delay-ps = 1530) gets the
expected 9 stages only because of the RMII constant on this line. The
parent commit gave it 2.

This still looks present at the end of the series. Should both parts
be fixed together? That would mean swapping the macro values back
(RGMII = 170, MII/RMII = 550) and using MT2712_RGMII_DLY_TXC_STAGE_DIV
here. Fixing only one of them still leaves RGMII TX at 550.

>  	.dma_bit_mask = 33,
>  };

[ ... ]

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