Re: [PATCH net-next v2 7/7] 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 (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] On MT8189, the driver and the DT binding disagree about MAC-
  sourced RMII.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  mediatek_dwmac_probe() ignores the return value of…
- [Medium] This problem wasn't introduced by this patch, but
  mediatek_dwmac_clks_config(enabled=true) leaks clock enables.

--

net: stmmac: mediatek: add support for MT8189 SoC

This adds MT8189 support to the MediaTek dwmac glue driver. MT8189 reuses
the MT8195 pericfg programming callbacks, with a different register
offset, 180ps delay stage dividers, a shorter clock list and the TX clock
output enable feature.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index 36599dc33b14..f07c331b029f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c

[ ... ]

> @@ -124,6 +131,10 @@ static const char * const mt8195_dwmac_clk_l[] = {
>  	"axi", "apb", "mac_cg", "mac_main", "ptp_ref"
>  };
>  
> +static const char * const mt8189_dwmac_clk_l[] = {
> +	"mac_main", "ptp_ref"
> +};

[Severity: Medium]
How is RMII with the MAC providing the reference clock meant to work on
MT8189?

The MT8189 binding added earlier in this series ("dt-bindings: net:
mediatek-dwmac: add support for MT8189 SoC") limits clock-names to exactly
these two entries:

    clock-names:
      items:
        - const: mac_main
        - const: ptp_ref

However, it still allows mediatek,rmii-clk-from-mac for this compatible.
The tx/rx-delay-ps descriptions also mention the MT8189 RMII interface.

If a board sets mediatek,rmii-clk-from-mac, mediatek_dwmac_clk_init()
always requests a clock that the binding does not permit:

drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c:mediatek_dwmac_clk_init() {
    ...
	if (plat->rmii_clk_from_mac) {
		plat->rmii_internal_clk = devm_clk_get(plat->dev, "rmii_internal");
		if (IS_ERR(plat->rmii_internal_clk))
			ret = PTR_ERR(plat->rmii_internal_clk);
    ...
}

So a DT that passes schema validation fails to probe. A DT that supplies
rmii_internal, and so would probe, fails schema validation.

The new variant also reuses mt8195_set_interface() and mt8195_set_delay().
These program MT8195_RMII_CLK_SRC_INTERNAL and the RMII_TXC/RMII_RXC delay
path, so the driver seems to treat this mode as supported.

Should the MT8189 binding gain an optional rmii_internal clock? Or should
the binding and the driver reject mediatek,rmii-clk-from-mac for MT8189
with a clear error?

[ ... ]

> @@ -495,6 +506,21 @@ static void mt8195_fix_mac_speed(void *priv, phy_interface_t interface,
>  			   reg_val);
>  }
>  
> +static const struct mediatek_dwmac_variant mt8189_gmac_variant = {
> +	.dwmac_set_phy_interface = mt8195_set_interface,
> +	.dwmac_set_delay = mt8195_set_delay,

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Now that
MT8189 uses mt8195_set_delay(), can it cause trouble that
mediatek_dwmac_probe() ignores the return value of mediatek_dwmac_init()?

drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c:mediatek_dwmac_probe() {
    ...
	mediatek_dwmac_common_data(pdev, plat_dat, priv_plat);
	mediatek_dwmac_init(&pdev->dev, priv_plat);

	ret = mediatek_dwmac_clks_config(priv_plat, true);
    ...
}

With phy-mode = "gmii", stmmac_get_phy_intf_sel() returns
PHY_INTF_SEL_GMII_MII. That passes the check in mediatek_dwmac_init(), and
mt8195_set_interface() writes CTRL0. mt8195_set_delay() then reaches:

	default:
		dev_err(plat->dev, "phy interface not supported\n");
		return -EINVAL;

CTRL1 and CTRL2 are never written, but probe still goes on into
stmmac_dvr_probe() and succeeds.

The error is also ignored for phy modes that mediatek_dwmac_init() rejects
up front. In that case the device probes with the pericfg interface
registers never programmed.

Should probe fail when mediatek_dwmac_init() returns an error?

> +	.dwmac_fix_mac_speed = mt8195_fix_mac_speed,
> +	.clk_list = mt8189_dwmac_clk_l,
> +	.num_clks = ARRAY_SIZE(mt8189_dwmac_clk_l),

[Severity: Medium]
This isn't a bug introduced by this patch, but does
mediatek_dwmac_clks_config() leak the bulk clock enables when the
rmii_internal clock fails to enable?

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;
		}
    ...
}

That path has no clk_bulk_disable_unprepare(). mediatek_dwmac_probe() also
returns the error directly, without reaching err_drv_probe:

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

On MT8189 this leaves mac_main and ptp_ref prepared and enabled. The devm
bulk get cleanup only drops the clock references. The same leak can
happen through the clks_config callback on resume.

On MT8189 this needs a DT that sets mediatek,rmii-clk-from-mac and also
supplies an rmii_internal clock, which the MT8189 binding does not allow.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260924-dwmac-mediatek-mt8189-v2-0-430bd74d5ef9%40collabora.com