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

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?

[ ... ]

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

[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?

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

[ ... ]

[ ... ]

[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?

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

[ ... ]

[ ... ]

[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?

[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?

+		plat_dat->pcs_init = rk_pcs_init;
+		plat_dat->pcs_exit = rk_pcs_exit;
+		plat_dat->select_pcs = rk_select_pcs;
+	}
+

[ ... ]