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

[Coia Prant]

Jakub Kicinski <kuba@xxxxxxxxxx> 于2026年9月13日周日 04:13写道：
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: stmmac: dwmac-rk: add SGMII support for RK3568
>
> This adds SGMII glue for RK3568: a set_to_sgmii() callback that programs
> GMAC CON1 bit 7, pcs_init/pcs_exit/select_pcs callbacks that create the
> Rockchip XPCS from the "pcs-handle" phandle, and a rework of the
> rk_gmac_powerup() error unwinding into a single clk_disable label.
>
> Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)
>
> A few questions below.
>
> > While at it, fix the clock enable ordering in rk_gmac_powerup():
> > gmac_clk_enable() is now called before any register access, including
> > the SGMII mode setup path.  Previously SGMII mode would bypass the
> > clock enable via a goto, which could cause synchronous external abort
> > when accessing MAC registers with the clock domain disabled.
>
> [Severity: Low]
> Does this paragraph describe the baseline?  At 879e280b8486 there is no
> SGMII case and no goto in rk_gmac_powerup(), and the order is already:
>
>         ret = rk_get_phy_intf_sel(bsp_priv->phy_iface);
>         if (ret < 0)
>                 return ret;
>         intf = ret;
>
>         ret = gmac_clk_enable(bsp_priv, true);
>         if (ret)
>                 return ret;
>         ...
>                 ret = rk_write_gmac_grf_reg(bsp_priv, val);
>
> so gmac_clk_enable() already preceded every register access and there was
> nothing that could bypass it.  The change here is a pure reorder ahead of
> rk_get_phy_intf_sel(), which touches no registers, plus the new unified
> unwind label.
>
> Would it be more accurate to describe this as preparing the ordering for
> the new SGMII path rather than as a fix for a synchronous external abort?

You're right. The baseline already had gmac_clk_enable() before any
register access. This change is a reorder to prepare for the new SGMII
path that skips rk_get_phy_intf_sel(), not a fix for a pre-existing
SError. I'll correct the wording.

> > 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
>
> [ ... ]
>
> > @@ -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)
> > @@ -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);
> > +}
> > +
> >  static const struct rk_gmac_ops rk3568_ops = {
> >       .init = rk3568_init,
> >       .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,
> > +     .supports_sgmii = true,
>
> [Severity: Medium]
> Who clears CON1 bit 7 for RMII on rk3568?
>
> rk3568_set_to_rgmii() now explicitly writes RK3568_GMAC_MODE_RMII_RGMII,
> which shows bit 7 has to be programmed by the driver.  rk3568_ops declares
> supports_rmii = true but has no .set_to_rmii, so the RMII branch of
> rk_gmac_powerup() is a no-op:
>
>         case PHY_INTERFACE_MODE_RMII:
>                 dev_info(dev, "init for RMII\n");
>                 if (bsp_priv->ops->set_to_rmii)
>                         bsp_priv->ops->set_to_rmii(bsp_priv);
>                 break;
>
> and the generic GRF write uses gmac_phy_intf_sel_mask = GENMASK_U16(6, 4),
> whose write-enable half-word never covers bit 7.
>
> Since rk3568_set_to_sgmii() leaves bit 7 set persistently and nothing in
> rk_gmac_powerdown() restores it, can an RMII port come up muxed to the
> SerDes when bit 7 was left set by firmware or by a previous SGMII
> configuration of the same MAC (kexec, warm reboot)?  The probe would report
> success while the link never works.

Good catch. rk3568_ops declares supports_rmii but has no .set_to_rmii,
so the RMII branch is a no-op. If bit 7 was left set by SGMII or
firmware, an RMII port would be muxed to the SerDes.

I'll add rk3568_set_to_rmii() that explicitly clears bit 7, and wire it
up in rk3568_ops.

Since we set set_to_rmii and set_to_sgmii, supports_rmii and supports_sgmii
will be omitted.

> > +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;
>
> [Severity: Low]
> Should "pcs-handle" be documented in the Rockchip MAC binding?
>
> This makes the property effectively mandatory for phy-mode = "sgmii" on
> RK3568: a missing phandle returns -ENODEV, and stmmac_pcs_setup() turns
> that into a probe failure:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c:stmmac_pcs_setup() {
>         ret = priv->plat->pcs_init(priv);
>         if (ret)
>                 return dev_err_probe(priv->device, ret, "No xPCS found\n");
> }
>
> Documentation/devicetree/bindings/net/rockchip-dwmac.yaml does not mention
> pcs-handle at all - it is neither described nor conditionally required for
> the SGMII-capable compatibles - while the new
> Documentation/devicetree/bindings/net/pcs/rockchip-dwxpcs.yaml says the
> port "is linked to an Ethernet MAC controller via the pcs-handle property
> in the MAC's device tree node".  Other stmmac glue bindings that parse this
> property describe it (renesas,rzn1-gmac.yaml, renesas,rzv2h-gbeth.yaml).
> Could the MAC binding be updated to match?

You're right, rockchip-dwmac.yaml should describe pcs-handle, since the
driver now requires it for phy-mode = "sgmii". I'll add it to the MAC
binding.

> > @@ -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;
>
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the bare return here cannot
> unwind a partially enabled clock domain.  gmac_clk_enable() leaks the bulk
> clock references when the PHY clock fails:
>
> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:gmac_clk_enable() {
>                         ret = clk_bulk_prepare_enable(bsp_priv->num_clks,
>                                                       bsp_priv->clks);
>                         if (ret)
>                                 return ret;
>
>                         ret = clk_prepare_enable(bsp_priv->clk_phy);
>                         if (ret)
>                                 return ret;
>         ...
>                         bsp_priv->clk_enabled = true;
> }
>
> The second failure path returns without clk_bulk_disable_unprepare() and
> without setting clk_enabled, so the new clk_disable label,
> rk_gmac_powerdown() and the suspend path are all no-ops afterwards because
> the disable branch is gated on clk_enabled.  Does this leave the bulk
> clocks prepared and enabled for the lifetime of the system?

This is a pre-existing bug, not introduced by this patch. The second
failure path returns without clk_bulk_disable_unprepare() and without
setting clk_enabled, so the new clk_disable label cannot unwind it.
It should fix it as a separate small patch.

> > @@ -1486,6 +1557,9 @@ static void rk_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
> >
> >       if (rk->supports_rmii)
> >               __set_bit(PHY_INTERFACE_MODE_RMII, interfaces);
> > +
> > +     if (rk->supports_sgmii)
> > +             __set_bit(PHY_INTERFACE_MODE_SGMII, interfaces);
> >  }
>
> [Severity: Medium]
> Is the SGMII advertisement here consistent with where the PCS is installed?
>
> supports_sgmii is true for every rk3568 instance regardless of DT, so SGMII
> always lands in phylink_config.supported_interfaces.  The PCS plumbing,
> however, is only installed when the DT phy-mode is exactly SGMII (see the
> rk_gmac_probe() hunk below).
>
> On an RK3568 board with phy-mode = "rgmii"/"rmii", phylink can still pick
> SGMII - phylink_validate_phy() intersects phy->possible_interfaces with
> config->supported_interfaces - and then:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_select_pcs() {
>         if (priv->plat->select_pcs) {
>                 pcs = priv->plat->select_pcs(priv, interface);
>                 if (!IS_ERR(pcs))
>                         return pcs;
>         }
> }
>
> returns NULL because select_pcs is unset, no XPCS was ever created
> (pcs_init is NULL), and rk_gmac_powerup() has already programmed the GRF
> once from the DT-derived bsp_priv->phy_iface, so rk3568_set_to_sgmii() never
> ran.  Would gating the SGMII bit on the same condition that installs
> select_pcs/pcs_init keep the two in agreement?

The phy-mode is fixed by the device tree and does not change at runtime.

rk_gmac_powerup() programs the GRF according to bsp_priv->phy_iface, which
is derived from phy-mode, not from supported_interfaces. So advertising
SGMII in supported_interfaces does not cause the MAC to switch to SGMII on
an RGMII board.

This is the same as the existing handling for RGMII and RMII, which are
also advertised unconditionally when the SoC supports them. There is no
inconsistency introduced by this patch.

> >  static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
> > @@ -1602,6 +1676,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;
>
> [Severity: Medium]
> Does default_an_inband guarantee in-band mode here?
>
> include/linux/phylink.h describes it as a default that "a fixed-link
> specification will override", and phylink has two ways out of in-band:
>
> drivers/net/phy/phylink.c:phylink_parse_mode() {
>         dn = fwnode_get_named_child_node(fwnode, "fixed-link");
>         if (dn || fwnode_property_present(fwnode, "fixed-link"))
>                 pl->cfg_link_an_mode = MLO_AN_FIXED;
> }
>
> drivers/net/phy/phylink.c:phylink_pcs_neg_mode() {
>                         neg_mode = PHYLINK_PCS_NEG_OUTBAND;
>                         if (pl->phydev)
>                                 mode = MLO_AN_PHY;
> }
>
> With set_clk_tx_rate cleared, stmmac_mac_link_up() then programs no rate at
> all:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_link_up() {
>         if (priv->plat->set_clk_tx_rate) {
>                 ret = priv->plat->set_clk_tx_rate(priv->plat->bsp_priv,
>                                                 priv->plat->clk_tx_i,
>                                                 interface, speed);
> }
>
> So for phy-mode = "sgmii" with a fixed-link node, or with a PHY that cannot
> do in-band signalling, the fixed 125 MHz clock is used at 10/100 Mbps -
> which the commit message says gives working TX and failing RX.  Should the
> driver reject out-of-band SGMII explicitly instead of relying on the
> default?

You're right. default_an_inband can be overridden by fixed-link, and
phylink can fall back to outband if the PHY cannot do in-band. With
set_clk_tx_rate = NULL, the MAC clock must stay at 125 MHz for 10/100
Mbps, otherwise the TX-works-RX-fails behaviour described in the
changelog occurs.

I will document the in-band-only limitation in the changelog.

Thanks,
Coia