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

[netdev-bot+sashiko@kernel.org]

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.

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

>  	help
>  	  Support for Ethernet controller on Rockchip RK3288 SoC.
>  
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 8d7042e689261..b3d746dd966ff 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

[ ... ]

> @@ -1330,6 +1395,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>  
>  	bsp_priv->supports_rgmii = ops->supports_rgmii || !!ops->set_to_rgmii;
>  	bsp_priv->supports_rmii = ops->supports_rmii || !!ops->set_to_rmii;
> +	bsp_priv->supports_sgmii = ops->supports_sgmii || !!ops->set_to_sgmii;
>  
>  	if (ops->init) {
>  		ret = ops->init(bsp_priv);

[ ... ]

> @@ -1379,16 +1449,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 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?

> +
> +	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;
>  
> -	ret = gmac_clk_enable(bsp_priv, true);
> -	if (ret)
> -		return ret;
> -
>  	if (bsp_priv->gmac_phy_intf_sel_mask ||
>  	    bsp_priv->gmac_rmii_mode_mask) {
>  		/* If defined, encode the phy_intf_sel value */

[ ... ]

> @@ -1447,15 +1517,20 @@ static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
>  		if (bsp_priv->ops->set_to_rmii)
>  			bsp_priv->ops->set_to_rmii(bsp_priv);
>  		break;
> +	case PHY_INTERFACE_MODE_SGMII:
> +		dev_info(dev, "init for SGMII\n");
> +		if (bsp_priv->ops->set_to_sgmii)
> +			bsp_priv->ops->set_to_sgmii(bsp_priv);
> +		break;
>  	default:
>  		dev_err(dev, "NO interface defined!\n");
> +		ret = -EINVAL;
> +		goto clk_disable;
>  	}
>  
>  	ret = rk_phy_powerup(bsp_priv);
> -	if (ret) {
> -		gmac_clk_enable(bsp_priv, false);
> -		return ret;
> -	}
> +	if (ret)
> +		goto clk_disable;
>  
>  	pm_runtime_get_sync(dev);
>  
> @@ -1486,6 +1565,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 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?

>  
>  static int rk_set_clk_tx_rate(void *bsp_priv_, struct clk *clk_tx_i,
> @@ -1602,6 +1684,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;
> +		plat_dat->pcs_init = rk_pcs_init;
> +		plat_dat->pcs_exit = rk_pcs_exit;
> +		plat_dat->select_pcs = rk_select_pcs;
> +	}
> +
>  	plat_dat->bsp_priv = rk_gmac_setup(pdev, plat_dat, data);
>  	if (IS_ERR(plat_dat->bsp_priv))
>  		return PTR_ERR(plat_dat->bsp_priv);

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