Re: [PATCH net-next v3 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568

[sashiko-bot@kernel.org]

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

New issues:
- [High] Hardware initialization failure: The interface selection bits 4:6 are not cleared for SGMII mode, contradicting the commit message and leaving the MAC in an invalid state.

Pre-existing issues:
- [High] Unhandled Error: pm_runtime_get_sync() return value is ignored, potentially causing hardware access without proper resume.
--

commit 5a7c3b38ba957e74b26623c8e3e94e4c77b594de
Author: Coia Prant <coiaprant@gmail.com>

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

This commit adds support for SGMII mode on the RK3568 SoC by providing the
necessary glue logic and XPCS configuration. It introduces callbacks to set
up the GMAC GRF register and handle PCS initialization, while also fixing
clock enable ordering in the powerup sequence.

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
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
[ ... ]
> @@ -851,18 +857,32 @@ static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
>  		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
>  
>  	regmap_write(bsp_priv->grf, con1,
> +		     RK3568_GMAC_MODE_RMII_RGMII |
>  		     RK3568_GMAC_RXCLK_DLY_ENABLE |
>  		     RK3568_GMAC_TXCLK_DLY_ENABLE);
>  }
>  
> +static void rk3568_set_to_sgmii(struct rk_priv_data *bsp_priv)
> +{
> +	u32 con1;
> +
> +	con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
> +				     RK3568_GRF_GMAC0_CON1;
> +
> +	regmap_write(bsp_priv->grf, con1, RK3568_GMAC_MODE_SGMII_QSGMII);
> +}

[Severity: High]
The commit message mentions "bit 7 set, interface selection bits 4:6 cleared",
but this write only targets bit 7 using RK3568_GMAC_MODE_SGMII_QSGMII (which
is GRF_BIT(7)). Does this leave the interface selection bits 4:6 in their
previous state instead of clearing them? Could this cause the SGMII interface
to fail to initialize properly?

[ ... ]
> @@ -1379,16 +1441,19 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  	if (ret)
>  		return ret;
>  
> +	ret = gmac_clk_enable(bsp_priv, true);
> +	if (ret)
> +		return ret;
> +
> +	if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_SGMII)
> +		goto set_mode;

[Severity: High]
Because PHY_INTERFACE_MODE_SGMII takes this goto set_mode, it skips the
generic GRF interface selection mask configuration right below it. Since
rk3568_set_to_sgmii() also doesn't clear bits 4:6, is there anywhere that the
clearing mask is actually applied for SGMII?

> +
>  	ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
>  	if (ret < 0)
> -		return ret;
> +		goto clk_disable;
>  
>  	intf = ret;
>  
[ ... ]
> @@ -1447,15 +1509,20 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  		if (bsp_priv->ops->set_to_rmii)
>  			bsp_priv->ops->set_to_rmii(bsp_priv);
>  		break;
> +	case PHY_INTERFACE_MODE_SGMII:
> +		dev_info(dev, "init for SGMII\n");
> +		if (bsp_priv->ops->set_to_sgmii)
> +			bsp_priv->ops->set_to_sgmii(bsp_priv);
> +		break;
>  	default:
>  		dev_err(dev, "NO interface defined!\n");
> +		ret = -EINVAL;
> +		goto clk_disable;
>  	}
>  
>  	ret = rk_phy_powerup(bsp_priv);
> -	if (ret) {
> -		gmac_clk_enable(bsp_priv, false);
> -		return ret;
> -	}
> +	if (ret)
> +		goto clk_disable;
>  
>  	pm_runtime_get_sync(dev);
>  
> @@ -1463,6 +1530,10 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  		bsp_priv->ops->integrated_phy_powerup(bsp_priv);
>  
>  	return 0;

[Severity: High]
This is a pre-existing issue, but the return value of pm_runtime_get_sync()
is ignored here. If the device power domain fails to resume, would the
subsequent integrated_phy_powerup() hardware access trigger a synchronous
external abort or cause silent data corruption?

> +
> +clk_disable:
> +	gmac_clk_enable(bsp_priv, false);
> +	return ret;
>  }

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

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