Re: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568 - sashiko-bot

From: sashiko-bot@kernel.org
To: "Coia Prant" <coiaprant@gmail.com>
Cc: robh@kernel.org, devicetree@vger.kernel.org,
	neil.armstrong@linaro.org, olteanv@gmail.com,
	linux-phy@lists.infradead.org, vkoul@kernel.org,
	conor+dt@kernel.org
Subject: Re: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568
Date: Wed, 15 Jul 2026 19:14:55 +0000	[thread overview]
Message-ID: <20260715191455.D033A1F00ACA@smtp.kernel.org> (raw)
In-Reply-To: <20260714191341.690906-9-coiaprant@gmail.com>

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The SGMII initialization path inadvertently skips enabling the MAC clocks, leading to hardware hangs or faults when MAC registers are accessed.
--

--- Patch [8]: [RFC PATCH 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568 ---
commit 7be50db2ac8c40c8451407f345aaaaf2d0538799
Author: Coia Prant <coiaprant@gmail.com>

net: stmmac: dwmac-rk: add SGMII support for RK3568

The RK3568 SoC integrates a Synopsys DesignWare XPCS that can be connected
to GMAC0 or GMAC1 in SGMII mode. Add the necessary glue logic to support
this configuration.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 8d7042e689261..eca482b4b6bfc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
[ ... ]
> @@ -1379,6 +1441,9 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  	if (ret)
>  		return ret;
>  
> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> +		goto set_mode;
> +

[Severity: High]
Does this goto skip enabling the MAC clocks? By jumping directly to set_mode,
we bypass the call to gmac_clk_enable() further down in this function.

Can this cause a synchronous external abort when the MAC registers are accessed
subsequently while their clock domain remains disabled?

>  	ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
>  	if (ret < 0)
>  		return ret;
[ ... ]
> -	/*rmii or rgmii*/
> +set_mode:
> +	/* rmii, rgmii, sgmii */
>  	switch (bsp_priv->phy_iface) {
>  	case PHY_INTERFACE_MODE_RGMII:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714191341.690906-1-coiaprant@gmail.com?part=8

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy