Re: [PATCH net-next v10 10/11] arm64: dts: rockchip: rk3568-photonicat: enable SGMII LAN port

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message of "arm64: dts: rockchip: rk3568-photonicat:
  enable SGMII LAN port" describes the combphy2, xpcs/xpcs_mii0 and…

--

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

This commit enables the SGMII LAN port on the Ariaboard Photonicat. It
routes combphy2 to GMAC0, enables the xpcs node, configures gmac0 for
SGMII and adds the YT8521SC PHY on mdio0. It also adds LED nodes to both
the new SGMII PHY and the existing RGMII WAN PHY on mdio1.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> index 58c1052ba8ef3..fdaa4a2a4328b 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-photonicat.dts

[ ... ]

> @@ -260,9 +262,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_mii0>;
> +	phy-handle = <&sgmii_phy>;
> +	phy-mode = "sgmii";
> +	phy-supply = <&vcc_3v3>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gmac0_miim>;
> +	status = "okay";
>  };

[Severity: Low]
This isn't a bug, but the commit message describes the combphy2,
xpcs/xpcs_mii0 and mdio0 changes and not this &gmac0 rework. The gmac0
rework is the main functional change in the patch. Could the message
describe it too?

In this hunk, status = "disabled" becomes a full SGMII configuration
that:

  - reparents SCLK_GMAC0_RX_TX to the 125 MHz clk_gmac0_xpcs_mii clock
    through assigned-clock-parents
  - sets managed = "in-band-status". The commit "net: stmmac: dwmac-rk:
    add SGMII support for RK3568" says boards must use it because
    out-of-band SGMII breaks RX at 10/100
  - adds pcs-handle = <&xpcs_mii0>, phy-handle, phy-supply and the
    gmac0_miim pinctrl

The commit message also says:

  "Also add LED configuration for the existing RGMII PHY on mdio1 for
  consistency."

Those LED_FUNCTION_WAN nodes go on rgmii_phy, which drives the separate
WAN port that already works. Could that go in its own patch? As it
stands, reverting or bisecting the SGMII LAN enablement also drops the
WAN LED change.

[ ... ]

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