Re: [PATCH v2 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:
- [High] rk3568_set_to_sgmii fails to clear interface selection bits 4:6, leading to potential hardware misconfiguration.
- [Low] DWMAC_ROCKCHIP selects PCS_XPCS_ROCKCHIP without inheriting its PM dependency.
--

--- Patch [8]: [PATCH v2 08/10] net: stmmac: dwmac-rk: add SGMII support for RK3568 ---
commit 53e13be00ad70a538e5aaf2d42200c1ae27d3eef
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/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index e3dd5adda5aca..5088acc06982e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -170,6 +170,7 @@ config DWMAC_ROCKCHIP
>  	default ARCH_ROCKCHIP
>  	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
>  	select MFD_SYSCON
> +	select PCS_XPCS_ROCKCHIP

[Severity: Low]
Could selecting PCS_XPCS_ROCKCHIP here cause a Kconfig unmet dependency
warning or build failure if PM is disabled? 

PCS_XPCS_ROCKCHIP depends on PM || COMPILE_TEST, but DWMAC_ROCKCHIP lacks
this dependency.

>  	help
>  	  Support for Ethernet controller on Rockchip RK3288 SoC.
>  

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 8d7042e689261..e47ca1bec5b8b 100644
> --- 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]
Does this actually clear the interface selection bits 4:6 as mentioned in the
commit message? 

It looks like this only writes RK3568_GMAC_MODE_SGMII_QSGMII (which sets
bit 7) but doesn't supply a write mask or data to clear bits 4:6. If the
bootloader left bits 4:6 set to a non-zero value, could this lead to an
incorrect hardware configuration?

[ ... ]
> @@ -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 of the goto here, the standard clearing of interface selection bits
using bsp_priv->gmac_phy_intf_sel_mask that happens below is bypassed for
SGMII mode. 

Should rk3568_set_to_sgmii() handle clearing those bits instead?

>  	ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
>  	if (ret < 0)
> -		return ret;
> +		goto clk_disable;

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

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