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

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 5

- [Medium] SGMII is advertised to phylink for every RK3568 GMAC port, but
  the SGMII PCS glue and the GRF SGMII mode bit are only wired up when…
- [Low] Claim: setting plat_dat->default_an_inband = true and clearing
  plat_dat->set_clk_tx_rate does not enforce in-band SGMII;…
- [Low] Claim: the new "phy-mode = sgmii implies pcs-handle" conditional
  in Documentation/devicetree/bindings/net/rockchip-dwmac.yaml is not…
- [Low] The XPCS node added to arch/arm64/boot/dts/rockchip/rk3568.dtsi
  (ethernet-pcs@fda00000, lines 113-142) omits `phys` and `phy-names`,…
- [Low] DWMAC_ROCKCHIP now does `select PCS_XPCS_ROCKCHIP` while having
  no PM dependency, and PCS_XPCS_ROCKCHIP…
- [Low] Claim: rk3568_set_to_sgmii() and rk3568_set_to_rmii() discard the
  regmap_write() status, so a failure to switch GRF CON1 bit 7 leaves…

Pre-existing issues:
- [Medium] gmac_clk_enable(bsp_priv, true) leaks the bulk clock
  prepare/enable references when clk_prepare_enable(bsp_priv->clk_phy)…

--

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

This adds SGMII glue for the RK3568 GMACs: a set_to_sgmii()/set_to_rmii()
pair that programs GRF CON1 bit 7, pcs_init/pcs_exit/select_pcs callbacks
that create the Rockchip XPCS, and default_an_inband plus a NULL
set_clk_tx_rate for SGMII.  rk_gmac_powerup() is also reordered so the
clocks are enabled before the interface-specific configuration, with the
error unwinding folded into one 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.

[Severity: Low]

Can this select produce an unmet dependency warning when PM is disabled?

DWMAC_ROCKCHIP only carries:

	depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)

while the selected symbol in drivers/net/pcs/Kconfig is:

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

and in kernel/power/Kconfig:

config PM_GENERIC_DOMAINS
	bool
	depends on PM

With PM=n (for example a COMPILE_TEST build with OF=y and STMMAC_ETH=m),
DWMAC_ROCKCHIP now force-enables PCS_XPCS_ROCKCHIP, which in turn
force-enables PM_GENERIC_DOMAINS against its own depends on PM, so genpd
gets built into a PM=n kernel.  Would a conditional select, or a depends
on, be more appropriate here?

Related: PCS_XPCS_ROCKCHIP lives inside the "if PCS_XPCS" block in
drivers/net/pcs/Kconfig, so it also carries depends on PCS_XPCS.  Does
DWMAC_ROCKCHIP need to select PCS_XPCS too, to avoid the same kind of
warning when PCS_XPCS=n?

[ ... ]

[ ... ]

[Severity: Medium]

This isn't a bug introduced by this patch, but since the clock handling in
this function is being reorganised: does gmac_clk_enable(bsp_priv, true)
leak the bulk clock prepare/enable references when the phy clock fails?

static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool 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;

On the second failure the bulk clocks stay prepared/enabled and
bsp_priv->clk_enabled is still false, so neither the bare return here nor
the new clk_disable label (which is gated on clk_enabled) undoes them.  A
later successful gmac_clk_enable(true) would then call
clk_bulk_prepare_enable() a second time.  Is it worth rolling the bulk
clocks back inside gmac_clk_enable() while this area is being touched?

[ ... ]

[Severity: Medium]

Is SGMII advertised more widely than the PCS glue is actually installed?

rk_gmac_setup() derives the capability purely from the ops table:

	bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;

rk3568_ops now provides .set_to_sgmii, so supports_sgmii is true for every
rk3568 port and rk_get_interfaces() sets PHY_INTERFACE_MODE_SGMII in
config->supported_interfaces even for a port whose phy-mode is rgmii or
rmii.

The PCS side, however, is keyed off the probe-time phy-mode only (see the
rk_gmac_probe() hunk below), so on such a port priv->plat->select_pcs stays
NULL and no XPCS is ever created.  If phylink later picks SGMII out of
supported_interfaces (an SFP cage going through
phylink_sfp_select_interface(), or a PHY reporting
PHY_INTERFACE_MODE_SGMII), stmmac_mac_select_pcs() returns NULL:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_mac_select_pcs() {
	if (priv->plat->select_pcs) {
		pcs = priv->plat->select_pcs(priv, interface);
	...
}

so nothing rejects the selection.  Meanwhile rk_gmac_powerup() ran from the
probe-time bsp_priv->phy_iface, so GRF CON1 bit 7 was cleared by
rk3568_set_to_rgmii()/rk3568_set_to_rmii() and is never set, and
rk_set_clk_tx_rate() is still installed.  Would that leave the port with a
link that never comes up and no error reported?

Would gating the __set_bit(PHY_INTERFACE_MODE_SGMII, interfaces) on the same
condition that installs pcs_init/select_pcs, and programming the GRF mode
bit for the interface phylink actually selected, avoid this?

One more question, about the other patches in the same series rather than
this one:

[Severity: Low]

Does the xpcs node in arch/arm64/boot/dts/rockchip/rk3568.dtsi validate
against the new binding?  The binding
Documentation/devicetree/bindings/net/pcs/rockchip,rk3568-xpcs.yaml has:

required:
  - compatible
  - reg
  - "#address-cells"
  - "#size-cells"
  - clocks
  - clock-names
  - phys
  - phy-names
  - power-domains

while the SoC-level node has neither phys nor phy-names:

	xpcs: ethernet-pcs@fda00000 {
		compatible = "rockchip,rk3568-xpcs";
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <0x0 0xfda00000 0x0 0x200000>;
		clocks = <&cru PCLK_XPCS>, <&cru CLK_XPCS_EEE>;
		clock-names = "csr", "eee";
		power-domains = <&power RK3568_PD_PIPE>;
		status = "disabled";

Only rk3568-photonicat.dts adds them at board level, and dt-validate does
not skip nodes with status = "disabled", so would every other rk3568 board
report "'phys' is a required property" under CHECK_DTBS?  A board that
enables the node without adding both properties would also fail in
xpcs_rk_serdes_phy_init() -> devm_phy_get(dev, "serdes").

Would it be better to keep the PHY assignment optional in the binding, or to
move the properties into the SoC dtsi?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915123802.1561724-1-coiaprant%40gmail.com