Re: [net-next,03/21] net: stmmac: rk: convert rk3328 to use bsp_priv->id
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-01-25 22:16:36
Also in:
linux-rockchip, netdev
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().
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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