Re: [PATCH v11 3/3] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: rockchip: gmac0 SGMII probe failure in rk_gmac_powerup()
- [High] arm64: dts: rockchip: missing driver and DT binding for rk3568-xpcs
--

commit aa6d6fc31ff0de23e03fbaf6018d2105111972d7
Author: Coia Prant <coiaprant@gmail.com>

arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

This commit enables the gmac0 and xpcs nodes on the Ariaboard Photonicat
board to support its Motorcomm YT8521SC Ethernet PHY over SGMII via
combphy2, and adds the PHY and LED definitions under mdio0.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> index 12ae9159bea87..d9015dbfb9f5c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> @@ -261,9 +261,18 @@ &cpu3 {
>  	cpu-supply = <&vdd_cpu>;
>  };
>  
> -/* Motorcomm YT8521SC LAN port (require SGMII) */
> +/* Motorcomm YT8521SC LAN port */
>  &gmac0 {
> -	status = "disabled";
> +	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>;
> +	assigned-clock-parents = <&clk_gmac0_xpcs_mii>;
> +	managed = "in-band-status";
> +	pcs-handle = <&xpcs>;
> +	phy-handle = <&sgmii_phy>;
> +	phy-mode = "sgmii";
> +	phy-supply = <&vcc_3v3>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gmac0_miim>;
> +	status = "okay";
>  };

[Severity: High]
When gmac0 probes with phy-mode = "sgmii", the call chain
rk_gmac_probe() -> devm_stmmac_pltfr_probe() -> stmmac_dvr_probe() ->
rk_gmac_init() -> rk_gmac_powerup() in
drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c validates the PHY interface
through rk_gmac_check_ops() and rk_get_phy_intf_sel():

drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_gmac_check_ops() {
	switch (bsp_priv->phy_iface) {
	case PHY_INTERFACE_MODE_RGMII:
	case PHY_INTERFACE_MODE_RGMII_ID:
	case PHY_INTERFACE_MODE_RGMII_RXID:
	case PHY_INTERFACE_MODE_RGMII_TXID:
		if (!bsp_priv->supports_rgmii)
			return -EINVAL;
		break;
	case PHY_INTERFACE_MODE_RMII:
		if (!bsp_priv->supports_rmii)
			return -EINVAL;
		break;
	default:
		dev_err(bsp_priv->dev,
			"unsupported interface %d", bsp_priv->phy_iface);
	}
	return 0;
}

drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c:rk_get_phy_intf_sel() {
	int ret = stmmac_get_phy_intf_sel(interface);

	/* Only RGMII and RMII are supported */
	if (ret != PHY_INTF_SEL_RGMII && ret != PHY_INTF_SEL_RMII)
		ret = -EINVAL;

	return ret;
}

In addition, rk_get_interfaces() only sets RGMII and RMII in phylink's
supported_interfaces, and dwmac-rk.c does not parse pcs-handle or populate
plat_dat->pcs_init / plat_dat->select_pcs to attach an XPCS instance.

Will enabling gmac0 with phy-mode = "sgmii" and pcs-handle = <&xpcs> cause
rk_gmac_powerup() to fail probe with -EINVAL before mdio0 or the network
interface can be registered?

[ ... ]

> @@ -606,3 +645,8 @@ &xin32k {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&clk32k_out1>;
>  };
> +
> +&xpcs {
> +	phys = <&combphy2 PHY_TYPE_SGMII>;
> +	status = "okay";
> +};

[Severity: High]
In arch/arm64/boot/dts/rockchip/rk3568.dtsi, the xpcs node is defined as:

	xpcs: ethernet-pcs@fda00000 {
		compatible = "rockchip,rk3568-xpcs";
		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";
	};

Looking at xpcs_of_ids[] in drivers/net/pcs/pcs-xpcs-plat.c, only
"snps,dw-xpcs*" compatibles are matched, and no driver in the tree matches
"rockchip,rk3568-xpcs" or handles its "eee" clock and phys properties.

When rockchip_combphy_probe() in
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c probes combphy2, it
asserts the PHY reset line:

	ret = reset_control_assert(priv->phy_rst);
	if (ret) {
		dev_err(dev, "failed to reset phy\n");
		return ret;
	}

That reset is only deasserted when a consumer driver calls phy_init() ->
rockchip_combphy_init(). Furthermore, Documentation/devicetree/bindings/
(including Documentation/devicetree/bindings/net/pcs/snps,dw-xpcs.yaml)
does not document "rockchip,rk3568-xpcs" or the phys, power-domains, and
"eee" clock properties on this node.

Could enabling &xpcs before the driver and binding schema for
"rockchip,rk3568-xpcs" are present leave combphy2 held in hardware reset and
introduce dtbs_check regressions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022025844.150320-1-coiaprant@gmail.com?part=3