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

[off-list ref] 于2026年9月15日周二 04:24写道：

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 2 (1) · Low: 3

- [Medium] DWMAC_ROCKCHIP gains an unconditional `select
  PCS_XPCS_ROCKCHIP` without inheriting the selected symbol's…
- [Medium] For SGMII the driver removes every speed-dependent clock
  programming path (rk_gmac_probe() sets plat_dat->set_clk_tx_rate =…
- [Low] rk_get_interfaces() (dwmac-rk.c:1570-1571) now sets
  PHY_INTERFACE_MODE_SGMII in phylink's supported_interfaces for every…
- [Low] The RK3568 XPCS SoC node added by the series ('arm64: dts:
  rockchip: rk3568: add XPCS and fixed-clock nodes') omits properties…
- [Low] rk_select_pcs() (dwmac-rk.c:1268-1275) ignores its `interface`
  argument and returns the SGMII XPCS for every interface mode,…

Pre-existing issues:
- [Medium] DISCARDED (valid but pre-existing and below the reporting
  threshold for pre-existing defects).

--

net: stmmac: dwmac-rk: add SGMII support for RK3568

Adds a set_to_sgmii()/set_to_rmii() pair for rk3568 to program GRF bit 7,
wires up pcs_init/pcs_exit/select_pcs so the Rockchip XPCS is used as the
PCS, and reorders rk_gmac_powerup() so the clocks are enabled before the
interface-specific configuration.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part1%20V1.1-20210301.pdf (Page 386)

A few questions below.

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
index ab3c4cf964239..c6559e749c0ce 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: Medium]
Does this select produce unmet dependency warnings?  PCS_XPCS_ROCKCHIP in
drivers/net/pcs/Kconfig carries three dependency lines:

  config PCS_XPCS_ROCKCHIP
        tristate "Rockchip XPCS controller support"
        default ARCH_ROCKCHIP
        depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
        depends on GENERIC_PHY || COMPILE_TEST
        depends on PM_GENERIC_DOMAINS || COMPILE_TEST

while DWMAC_ROCKCHIP only has "depends on OF && (ARCH_ROCKCHIP ||
COMPILE_TEST)".  ARCH_ROCKCHIP in arch/arm64/Kconfig.platforms selects PM
but neither GENERIC_PHY nor PM_GENERIC_DOMAINS, so a config with
ARCH_ROCKCHIP=y, COMPILE_TEST=n and GENERIC_PHY=n (or PM_GENERIC_DOMAINS=n)
force-enables PCS_XPCS_ROCKCHIP with its own depends unsatisfied:

  WARNING: unmet direct dependencies detected for PCS_XPCS_ROCKCHIP

pcs-xpcs-rk.o is then built against the phy stubs where devm_phy_get()
returns ERR_PTR(-ENOSYS), so the probe cannot succeed.

The select is also unconditional, so every Rockchip dwmac build
(RK3288/RK3328/RK3399/RK3588 ...) force-enables the RK356x-only XPCS
driver.  Would inheriting the constraints, or making the selection
conditional, be preferable here?  Note that simply adding the two depends
lines to DWMAC_ROCKCHIP would turn off Ethernet on existing RK3288/RK3399
configurations, so the choice probably needs to be explicit.

The Kconfig change is also not mentioned anywhere in the commit message.

You're right. I'll move the dependencies into PCS_XPCS_ROCKCHIP as
selects instead of depends on:

    config PCS_XPCS_ROCKCHIP
        tristate "Rockchip XPCS controller support"
        default ARCH_ROCKCHIP
        depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
        select GENERIC_PHY
        select PM_GENERIC_DOMAINS

This way the symbol pulls in its own dependencies when it is selected
by DWMAC_ROCKCHIP, so the unconditional select in dwmac-rk does not
produce an unmet-dependency warning.

quoted

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 8d7042e689261..5d501ed8d1429 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

[ ... ]

quoted

@@ -1208,6 +1237,43 @@ static void rk_phy_powerdown(struct rk_priv_data *bsp_priv)
+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);
+}

[Severity: Low]
Should rk_select_pcs() look at its interface argument?  The mac_select_pcs
contract in include/linux/phylink.h is to return the PCS for the specified
interface, or NULL when none is required, but this returns the SGMII XPCS
for any mode.

On an RK3568 port probed as SGMII, rk_get_interfaces() also advertises the
RGMII variants and RMII, and xpcs->pcs.supported_interfaces is non-empty
(filled in by xpcs_get_interfaces()).  When phylink validates one of those
non-SGMII interfaces, phylink_validate_mac_and_pcs() hits:

        if (!phy_interface_empty(pcs->supported_interfaces) &&
            !test_bit(state->interface, pcs->supported_interfaces)) {
                phylink_err(pl, "MAC returned PCS which does not support %s\n",
                            phy_modes(state->interface));
                return -EINVAL;
        }

Would returning the XPCS only for PHY_INTERFACE_MODE_SGMII and NULL
otherwise avoid that error path?

plat_dat->select_pcs is only assigned inside the
if (phy_interface == PHY_INTERFACE_MODE_SGMII) branch of
rk_gmac_probe(). For RGMII/RMII ports the callback is NULL, so
stmmac_mac_select_pcs() returns NULL before reaching rk_select_pcs().

On an SGMII port, phylink_validate() is called with
state->interface = PHY_INTERFACE_MODE_SGMII, so only SGMII is
validated against the PCS. The RGMII/RMII bits in supported_interfaces
are not iterated in this path.

quoted

@@ -1379,16 +1450,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 now that gmac_clk_enable()
is the first resource acquisition in rk_gmac_powerup(), its partial-failure
path becomes more visible.  Do the bulk clocks leak when the clk_phy enable
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;
        ...
}

The second failure returns without calling clk_bulk_disable_unprepare() and
without setting bsp_priv->clk_enabled, so the later gmac_clk_enable(bsp_priv,
false) in rk_gmac_powerdown() and at the new clk_disable label is a no-op
because it is gated on clk_enabled.  This only triggers for integrated-PHY
setups where clk_phy is non-NULL.

This is pre-existing and unrelated to SGMII.

quoted

+     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;

[ ... ]

quoted

@@ -1486,6 +1566,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: Low]
Is SGMII safe to advertise on a port that was not probed as SGMII?
rk3568_ops populates set_to_sgmii unconditionally, and rk_gmac_setup()
derives supports_sgmii from it, so every RK3568 port now advertises SGMII
in phylink's supported_interfaces (stmmac_phy_setup() copies the
get_interfaces() output verbatim into config->supported_interfaces).

All the SGMII setup, though, is keyed off the probe-time DT phy-mode:

  - rk_gmac_powerup() writes RK3568_GMAC_MODE_SGMII_QSGMII only in the
    "case PHY_INTERFACE_MODE_SGMII:" branch, which runs once at init
  - rk_gmac_probe() installs pcs_init/pcs_exit/select_pcs,
    default_an_inband and clears set_clk_tx_rate only under
    "if (plat_dat->phy_interface == PHY_INTERFACE_MODE_SGMII)"

So on a port probed as rgmii*/rmii, if phylink later selects SGMII (SFP
cage, or a PHY reporting SGMII as its host interface), GRF bit 7 stays
cleared by rk3568_set_to_rgmii()/rk3568_set_to_rmii(), plat->select_pcs is
NULL so stmmac_mac_select_pcs() returns NULL and there is no PCS to run
autonegotiation.  Before this patch SGMII was not in supported_interfaces
and such a configuration was rejected during validation.  Would gating the
__set_bit() on the probe-time interface be closer to what the hardware can
actually do?

phylink_create() receives the DT phy-mode as iface, and
phylink_validate() validates that fixed interface, not every bit in
supported_interfaces. The GRF is programmed from bsp_priv->phy_iface
in rk_gmac_powerup(), which is also the DT phy-mode. Advertising SGMII
in supported_interfaces does not change the interface mode at runtime.

quoted

@@ -1602,6 +1685,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]
The commit message says:

  "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."

Is "forces" accurate?  default_an_inband is only a default, and the next
paragraph of the message says so itself.  In phylink_parse_mode():

        if (pl->config->default_an_inband)
                pl->cfg_link_an_mode = MLO_AN_INBAND;

        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;

and xpcs_inband_caps() advertises out-of-band as acceptable:

        case DW_AN_C37_SGMII:
        case DW_AN_C37_1000BASEX:
                return LINK_INBAND_DISABLE | LINK_INBAND_ENABLE;

so phylink_pcs_neg_mode() can settle on PHYLINK_PCS_NEG_OUTBAND for a
fixed-link node or a PHY without in-band support.  With set_clk_tx_rate set
to NULL, rk3568_ops.set_speed = rk_set_clk_mac_speed also becomes
unreachable, so a 10/100 Mbps link is brought up with the clocking left at
the 1 Gbps setting and nothing warns or fails.  Would rejecting a
non-inband neg_mode (for example in mac_config or pcs_validate), or failing
probe when a fixed-link node is present for SGMII, match what the message
describes?

The v5 commit message already notes that default_an_inband can be
overridden by a fixed-link node, and that out-of-band SGMII is not
supported by this driver.

One more observation on the series rather than on this patch: the new
binding Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml
lists phys and phy-names in its unconditional required list, but the
ethernet-pcs@fda00000 node added to arch/arm64/boot/dts/rockchip/rk3568.dtsi
supplies only reg, clocks, clock-names and power-domains.  Only
rk3568-photonicat.dts adds them:

  &xpcs {
        phys = <&combphy2 PHY_TYPE_SGMII>;
        phy-names = "serdes";
        status = "okay";
  };

[Severity: Low]
Does dtbs_check report "phys is a required property" for the other RK3568
board DTBs?  dtschema validates nodes regardless of status = "disabled".
The neighbouring sata0 node in the same dtsi does carry phys/phy-names at
SoC level, so would moving them into rk3568.dtsi be an option?

As replied earlier, the SerDes connection is board-specific and cannot
move into the SoC dtsi. dtbs_check skips required-property validation
for status = "disabled" nodes.