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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

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] The series reverses the MT2712 delay-stage dividers compared
  with the baseline driver and the binding.

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but in
  mediatek_dwmac_clks_config(), if clk_bulk_prepare_enable() succeeds…

--

net: stmmac: mediatek: add support for MT8189 SoC

This patch adds the mediatek,mt8189-gmac compatible to dwmac-mediatek,
along with a clock list and variant platform data. The new variant reuses
the MT8195 callbacks with a 0x270 pericfg offset and 180ps RX/TX delay
stage dividers, and it enables the TX clock output.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index 147e217b86620..d480c339a16d0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> @@ -45,8 +45,15 @@
>  #define MT2712_RGMII_DLY_TXC_MAX	17600 /* 32 x 550ps */

[Severity: Medium]
The context here shows the MT2712 defines added earlier in this series. Are
the MT2712 RGMII and RMII stage dividers swapped?

The defines currently read:

#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_RXC_MAX	17600 /* 32 x 550ps */
#define MT2712_RGMII_DLY_TXC_STAGE_DIV	550 /* 550ps per stage */

Before the series, mt2712_delay_ps2stage() divided RGMII delays by 170 and
MII/RMII delays by 550. The mediatek-dwmac.yaml binding says the same:

  For MT2712 RGMII interface, Allowed value need to be a multiple of 170,
  ...
  For MT2712 RMII/MII interface, Allowed value need to be a multiple of 550,

mt2712_gmac_variant then uses:

	.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,

mt2712_set_delay() picks these dividers based on the phy mode. RGMII TX
still divides by 170, but only because it uses the RMII define. RGMII RX
now divides by 550 instead of 170. MII/RMII TX and RX now divide by 170
instead of 550.

For example, an RGMII mediatek,rx-delay-ps = <1530> used to program 9
stages and now programs 2. An MII/RMII delay of 1650 used to program 3
stages and now programs 9.

This came from two earlier patches in this series, not from this one:
"net: stmmac: mediatek: simplify TX/RX delay handling in mt2712_set_delay"
and "net: stmmac: mediatek: add RX/TX delay stage divider in platform
data". The first one's commit message also describes the old code as
"550 for RGMII, 170 for MII/RMII".

Changing only tx_rgmii_delay_stage_div to MT2712_RGMII_DLY_TXC_STAGE_DIV
would make things worse. mt2712-evb.dts (phy-mode "rgmii-rxid",
mediatek,tx-delay-ps = <1530>) would drop from 9 stages to 2.

Could the define values be swapped instead (RGMII = 170, RMII = 550), with
tx_rgmii_delay_stage_div then using the RGMII define?

[ ... ]

> @@ -751,6 +777,8 @@ static void mediatek_dwmac_remove(struct platform_device *pdev)
>  static const struct of_device_id mediatek_dwmac_match[] = {
>  	{ .compatible = "mediatek,mt2712-gmac",
>  	  .data = &mt2712_gmac_variant },
> +	{ .compatible = "mediatek,mt8189-gmac",
> +	  .data = &mt8189_gmac_variant },

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
MT8189 now uses the same probe path. In mediatek_dwmac_clks_config(), if
clk_bulk_prepare_enable() succeeds and clk_prepare_enable() then fails on
plat->rmii_internal_clk, do the bulk clocks stay enabled?

drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c:mediatek_dwmac_clks_config() {
    ...
	ret = clk_bulk_prepare_enable(variant->num_clks, plat->clks);
	...
	ret = clk_prepare_enable(plat->rmii_internal_clk);
	if (ret) {
		dev_err(plat->dev, "failed to enable rmii internal clk, err = %d\n", ret);
		return ret;
	}
    ...
}

This path never calls clk_bulk_disable_unprepare(), and
mediatek_dwmac_probe() returns the error directly:

	ret = mediatek_dwmac_clks_config(priv_plat, true);
	if (ret)
		return ret;

The resume path behaves the same way.

For MT8189, rmii_internal_clk is only obtained when
mediatek,rmii-clk-from-mac is present, and the MT8189 binding forbids that
property. A binding-compliant MT8189 DT therefore gets a NULL clock and
cannot hit this path, but the driver does not enforce the binding.

>  	{ .compatible = "mediatek,mt8195-gmac",
>  	  .data = &mt8195_gmac_variant },
>  	{ }

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