Re: [net-next,03/21] net: stmmac: rk: convert rk3328 to use bsp_priv->id

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
net: stmmac: rk: convert to mask-based interface mode configuration

This commit converts many Rockchip GMAC implementations to use a new
centralized mask-based infrastructure for configuring PHY interface
mode. The PHY_INTF_SEL and RMII mode configuration is moved from
individual set_to_rgmii()/set_to_rmii() functions to rk_gmac_powerup().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 4117f6863ff0..48fc5b58092f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

[ ... ]

> @@ -1198,9 +1225,6 @@ static const struct rk_gmac_ops rk3576_ops = {
>  #define RK3588_GRF_GMAC_CON0			0X0008
>  #define RK3588_GRF_CLK_CON1			0X0070

The comment above this definition says "/* php_grf */" indicating that
RK3588_GRF_GMAC_CON0 is in the php_grf address space.

> -#define RK3588_GMAC_PHY_INTF_SEL(id, val)	\
> -	(GRF_FIELD(5, 3, val) << ((id) * 6))
> -

[ ... ]

> +static int rk3588_init(struct rk_priv_data *bsp_priv)
> +{
> +	switch (bsp_priv->id) {
> +	case 0:
> +		bsp_priv->gmac_phy_intf_sel_mask = GENMASK_U16(5, 3);
> +		return 0;
> +
> +	case 1:
> +		bsp_priv->gmac_phy_intf_sel_mask = GENMASK_U16(11, 9);
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

[ ... ]

>  static void rk3588_set_to_rgmii(struct rk_priv_data *bsp_priv,
>  				int tx_delay, int rx_delay)
>  {
>  	u32 offset_con, id = bsp_priv->id;
>
>  	offset_con = bsp_priv->id == 1 ? RK3588_GRF_GMAC_CON9 :
>  					 RK3588_GRF_GMAC_CON8;
>
> -	regmap_write(bsp_priv->php_grf, RK3588_GRF_GMAC_CON0,
> -		     RK3588_GMAC_PHY_INTF_SEL(id, PHY_INTF_SEL_RGMII));

This removed code wrote to php_grf, but the new centralized code in
rk_gmac_powerup() writes to grf instead. Is this intentional?

[ ... ]

>  static void rk3588_set_to_rmii(struct rk_priv_data *bsp_priv)
>  {
> -	regmap_write(bsp_priv->php_grf, RK3588_GRF_GMAC_CON0,
> -		     RK3588_GMAC_PHY_INTF_SEL(bsp_priv->id, PHY_INTF_SEL_RMII));

Same here - the original wrote to php_grf.

[ ... ]

>  static const struct rk_gmac_ops rk3588_ops = {
> +	.init = rk3588_init,
>  	.set_to_rgmii = rk3588_set_to_rgmii,
>  	.set_to_rmii = rk3588_set_to_rmii,
>  	.set_speed = rk3588_set_gmac_speed,
>  	.set_clock_selection = rk3588_set_clock_selection,
> +
> +	.gmac_grf_reg = RK3588_GRF_GMAC_CON0,

This sets gmac_grf_reg to RK3588_GRF_GMAC_CON0, which is then used by
rk_gmac_powerup() to write to bsp_priv->grf.

[ ... ]

> @@ -1654,6 +1696,7 @@ static int rk_gmac_check_ops(struct rk_priv_data *bsp_priv)
>  static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  {
>  	struct device *dev = bsp_priv->dev;
> +	u32 val;
>  	int ret;

[ ... ]

> +	if (bsp_priv->gmac_phy_intf_sel_mask ||
> +	    bsp_priv->gmac_rmii_mode_mask) {
> +		/* If defined, encode the phy_intf_sel value */
> +		val = rk_encode_wm16(ret, bsp_priv->gmac_phy_intf_sel_mask);
> +
> +		/* If defined, encode the RMII mode mask setting. */
> +		val |= rk_encode_wm16(ret == PHY_INTF_SEL_RMII,
> +				      bsp_priv->gmac_rmii_mode_mask);
> +
> +		ret = regmap_write(bsp_priv->grf, bsp_priv->gmac_grf_reg, val);
                                 ^^^^^^^^^^^^

For RK3588, this writes to bsp_priv->grf at offset RK3588_GRF_GMAC_CON0
(0x0008). However, the original code wrote to bsp_priv->php_grf at the
same offset. These are different regmaps obtained from different device
tree phandles (rockchip,grf vs rockchip,php-grf).

Does RK3588 need a mechanism to write the PHY_INTF_SEL to php_grf rather
than grf? The clock register already has clock_grf_reg_in_php support
to select which regmap to use, but there does not appear to be a similar
mechanism for gmac_grf_reg.
-- 
pw-bot: cr