Re: [PATCH net-next v8 09/11] net: stmmac: dwmac-rk: add SGMII support for RK3568

[Maxime Chevallier]

Hi,

On 9/19/26 14:51, Coia Prant wrote:
> 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.
> 
> The current dwmac-rk driver does not support SGMII mode.  SGMII
> requires a PCS to handle auto-negotiation and link state reporting,
> but the existing driver only supports RGMII and RMII.
> 
> Add a set_to_sgmii() callback to configure the GMAC GRF register for
> SGMII mode (bit 7 set, interface selection bits 4:6 ignored when set).
> Also add a set_to_rmii() callback for rk3568 to explicitly clear bit 7,
> since the new SGMII path leaves it set and the RMII branch previously
> relied on the SoC reset value.
> 
> Provide pcs_init/pcs_exit callbacks to create/destroy the XPCS via
> xpcs_rk_create() from the Rockchip XPCS platform driver, and a
> select_pcs callback to return the XPCS to phylink. SGMII is not added
> to rk_get_interfaces(): it comes from the XPCS's own
> supported_interfaces, merged by stmmac_phylink_setup().
> 
> The SerDes PHY and the PD_PIPE power domain are owned by the XPCS
> driver rather than managed through the stmmac
> serdes_poweron/serdes_poweroff callbacks, which are legacy and meant
> for single-MAC platforms. On RK3568 the XPCS is the natural owner of
> the shared SerDes.
> 
> DWMAC_ROCKCHIP selects PCS_XPCS_ROCKCHIP. PCS_XPCS itself is already
> selected by STMMAC_ETH, and PM is selected by ARCH_ROCKCHIP, so no
> further selects are needed.
> 
> Reorder rk_gmac_powerup() so that gmac_clk_enable() is called before
> the SGMII check.  The SGMII path skips rk_get_phy_intf_sel(), so the
> clock must be enabled earlier to cover all register accesses in that
> path.  While at it, unify the error unwinding into a single
> clk_disable label and add error handling for the default (unhandled
> interface) case.
> 
> SGMII In-band vs Out-of-band
> ============================
> On RK3568, the MAC clock is fixed at 125 MHz and cannot be dynamically
> changed by the stmmac core's set_clk_tx_rate callback.  In-band mode
> works because the PCS handles rate adaptation internally.  Out-of-band
> mode does not work because the MAC would need to change the clock rate
> to 125/12.5/1.25 MHz for 1000/100/10 Mbps respectively, and the clock
> is fixed.
> 
> Enable default_an_inband for SGMII and disable the generic stmmac
> set_clk_tx_rate callback.  This forces phylink to use in-band mode,
> where the PCS is responsible for speed/duplex negotiation.
> 
> Note that default_an_inband can be overridden by a fixed-link node,
> and phylink may also fall back to out-of-band if the PHY does not
> support in-band signalling.  Out-of-band SGMII is not supported by
> this driver: the MAC clock would stay at 125 MHz for 10/100 Mbps,
> giving working TX but failing RX.  Boards must use in-band mode
> (managed = "in-band-status" or an in-band capable PHY).
> 
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
> Signed-off-by: Coia Prant <coiaprant@xxxxxxxxx>

Reviewed-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx

Maxime

> ---
>  drivers/net/ethernet/stmicro/stmmac/Kconfig   |   1 +
>  .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 130 +++++++++++++++---
>  2 files changed, 111 insertions(+), 20 deletions(-)
> 
> 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
>  	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..88f09014e3a69 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -20,6 +20,7 @@
>  #include <linux/delay.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/pcs/pcs-xpcs-rk.h>
>  #include <linux/pm_runtime.h>
>  
>  #include "stmmac_platform.h"
> @@ -47,6 +48,7 @@ struct rk_gmac_ops {
>  	void (*set_to_rgmii)(struct rk_priv_data *bsp_priv,
>  			     int tx_delay, int rx_delay);
>  	void (*set_to_rmii)(struct rk_priv_data *bsp_priv);
> +	void (*set_to_sgmii)(struct rk_priv_data *bsp_priv);
>  	int (*set_speed)(struct rk_priv_data *bsp_priv,
>  			 phy_interface_t interface, int speed);
>  	void (*integrated_phy_powerup)(struct rk_priv_data *bsp_priv);
> @@ -63,6 +65,7 @@ struct rk_gmac_ops {
>  	bool clock_grf_reg_in_php;
>  	bool supports_rgmii;
>  	bool supports_rmii;
> +	bool supports_sgmii;
>  	bool php_grf_required;
>  	bool regs_valid;
>  	u32 regs[];
> @@ -98,6 +101,7 @@ struct rk_priv_data {
>  	bool integrated_phy;
>  	bool supports_rgmii;
>  	bool supports_rmii;
> +	bool supports_sgmii;
>  
>  	struct clk_bulk_data *clks;
>  	int num_clks;
> @@ -809,6 +813,8 @@ static const struct rk_gmac_ops rk3528_ops = {
>  #define RK3568_GRF_GMAC1_CON1		0x038c
>  
>  /* RK3568_GRF_GMAC0_CON1 && RK3568_GRF_GMAC1_CON1 */
> +#define RK3568_GMAC_MODE_RMII_RGMII		GRF_CLR_BIT(7)
> +#define RK3568_GMAC_MODE_SGMII_QSGMII		GRF_BIT(7)
>  #define RK3568_GMAC_FLOW_CTRL			GRF_BIT(3)
>  #define RK3568_GMAC_FLOW_CTRL_CLR		GRF_CLR_BIT(3)
>  #define RK3568_GMAC_RXCLK_DLY_ENABLE		GRF_BIT(1)
> @@ -836,6 +842,16 @@ static int rk3568_init(struct rk_priv_data *bsp_priv)
>  	}
>  }
>  
> +static void rk3568_set_to_rmii(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_RMII_RGMII);
> +}
> +
>  static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
>  				int tx_delay, int rx_delay)
>  {
> @@ -851,19 +867,31 @@ 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);
> +}
> +
>  static const struct rk_gmac_ops rk3568_ops = {
>  	.init = rk3568_init,
> +	.set_to_rmii = rk3568_set_to_rmii,
>  	.set_to_rgmii = rk3568_set_to_rgmii,
> +	.set_to_sgmii = rk3568_set_to_sgmii,
> +
>  	.set_speed = rk_set_clk_mac_speed,
>  
>  	.gmac_phy_intf_sel_mask = GENMASK_U16(6, 4),
>  
> -	.supports_rmii = true,
> -
>  	.regs_valid = true,
>  	.regs = {
>  		0xfe2a0000, /* gmac0 */
> @@ -1208,6 +1236,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
>  		dev_err(bsp_priv->dev, "fail to disable phy-supply\n");
>  }
>  
> +static int rk_pcs_init(struct stmmac_priv *priv)
> +{
> +	struct device_node *np = priv->device->of_node;
> +	struct device_node *pcs_node;
> +	struct dw_xpcs *xpcs;
> +
> +	pcs_node = of_parse_phandle(np, "pcs-handle", 0);
> +	if (!pcs_node)
> +		return -ENODEV;
> +
> +	xpcs = xpcs_rk_create(priv->device, pcs_node);
> +	of_node_put(pcs_node);
> +	if (IS_ERR(xpcs))
> +		return PTR_ERR(xpcs);
> +
> +	priv->hw->xpcs = xpcs;
> +	return 0;
> +}
> +
> +static void rk_pcs_exit(struct stmmac_priv *priv)
> +{
> +	if (!priv->hw->xpcs)
> +		return;
> +
> +	xpcs_destroy(priv->hw->xpcs);
> +	priv->hw->xpcs = NULL;
> +}
> +
> +static struct phylink_pcs *rk_select_pcs(struct stmmac_priv *priv,
> +					 phy_interface_t interface)
> +{
> +	if (!priv->hw->xpcs)
> +		return NULL;
> +
> +	return xpcs_to_phylink_pcs(priv->hw->xpcs);
> +}
> +
>  static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>  					  struct plat_stmmacenet_data *plat,
>  					  const struct rk_gmac_ops *ops)
> @@ -1330,6 +1395,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>  
>  	bsp_priv->supports_rgmii = ops->supports_rgmii || !!ops->set_to_rgmii;
>  	bsp_priv->supports_rmii = ops->supports_rmii || !!ops->set_to_rmii;
> +	bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;
>  
>  	if (ops->init) {
>  		ret = ops->init(bsp_priv);
> @@ -1361,6 +1427,10 @@ static int rk_gmac_check_ops(struct rk_priv_data *bsp_priv)
>  		if (!bsp_priv->supports_rmii)
>  			return -EINVAL;
>  		break;
> +	case PHY_INTERFACE_MODE_SGMII:
> +		if (!bsp_priv->supports_sgmii)
> +			return -EINVAL;
> +		break;
>  	default:
>  		dev_err(bsp_priv->dev,
>  			"unsupported interface %d", bsp_priv->phy_iface);
> @@ -1379,16 +1449,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;
> +
>  	ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
>  	if (ret < 0)
> -		return ret;
> +		goto clk_disable;
>  
>  	intf = ret;
>  
> -	ret = gmac_clk_enable(bsp_priv, true);
> -	if (ret)
> -		return ret;
> -
>  	if (bsp_priv->gmac_phy_intf_sel_mask ||
>  	    bsp_priv->gmac_rmii_mode_mask) {
>  		/* If defined, encode the phy_intf_sel value */
> @@ -1399,10 +1472,8 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  				      bsp_priv->gmac_rmii_mode_mask);
>  
>  		ret = rk_write_gmac_grf_reg(bsp_priv, val);
> -		if (ret < 0) {
> -			gmac_clk_enable(bsp_priv, false);
> -			return ret;
> -		}
> +		if (ret < 0)
> +			goto clk_disable;
>  	}
>  
>  	if (bsp_priv->clock.rmii_mode_mask) {
> @@ -1410,13 +1481,12 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  				     bsp_priv->clock.rmii_mode_mask);
>  
>  		ret = rk_write_clock_grf_reg(bsp_priv, val);
> -		if (ret < 0) {
> -			gmac_clk_enable(bsp_priv, false);
> -			return ret;
> -		}
> +		if (ret < 0)
> +			goto clk_disable;
>  	}
>  
> -	/*rmii or rgmii*/
> +set_mode:
> +	/* rmii, rgmii, sgmii */
>  	switch (bsp_priv->phy_iface) {
>  	case PHY_INTERFACE_MODE_RGMII:
>  		dev_info(dev, "init for RGMII\n");
> @@ -1447,15 +1517,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 +1538,10 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  		bsp_priv->ops->integrated_phy_powerup(bsp_priv);
>  
>  	return 0;
> +
> +clk_disable:
> +	gmac_clk_enable(bsp_priv, false);
> +	return ret;
>  }
>  
>  static void rk_gmac_powerdown(struct rk_priv_data *gmac)
> @@ -1602,6 +1681,17 @@ static int rk_gmac_probe(struct platform_device *pdev)
>  	plat_dat->suspend = rk_gmac_suspend;
>  	plat_dat->resume = rk_gmac_resume;
>  
> +	if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +		/* SGMII clock always runs at 125 MHz */
> +		plat_dat->set_clk_tx_rate = NULL;
> +
> +		/* SGMII requires a PCS */
> +		plat_dat->default_an_inband = true;
> +		plat_dat->pcs_init = rk_pcs_init;
> +		plat_dat->pcs_exit = rk_pcs_exit;
> +		plat_dat->select_pcs = rk_select_pcs;
> +	}
> +
>  	plat_dat->bsp_priv = rk_gmac_setup(pdev, plat_dat, data);
>  	if (IS_ERR(plat_dat->bsp_priv))
>  		return PTR_ERR(plat_dat->bsp_priv);