Re: [PATCH 2/3] arm64: dts: rockchip: rk3588s-nanopi: switch to generic eth phy compatible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] arm64: dts: rockchip: per-PHY reset-gpios not deasserted before C22 ID read
--

commit 743d7222bfed74bfab2871d223949437c61ab32b
Author: Joachim Eastwood <joachim.eastwood@gmail.com>

arm64: dts: rockchip: rk3588s-nanopi: switch to generic eth phy compatible

This commit switches the rgmii_phy1 Ethernet PHY node in
rk3588s-nanopi.dtsi from the explicit RTL8211F PHY ID compatible
("ethernet-phy-id001c.c916") to the generic Clause 22 compatible
("ethernet-phy-ieee802.3-c22") to support the MotorComm YT8531C PHY on
NanoPi M6V2.

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-nanopi.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s-nanopi.dtsi
> index 22b0cdee22b40..4977ccb668f39 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s-nanopi.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-nanopi.dtsi
> @@ -269,7 +269,7 @@ &i2s5_8ch {
>  
>  &mdio1 {
>  	rgmii_phy1: ethernet-phy@1 {
> -		compatible = "ethernet-phy-id001c.c916";
> +		compatible = "ethernet-phy-ieee802.3-c22";

[Severity: High]
Will this cause PHY registration to fail with -ENODEV when the reset GPIO is
still asserted during MDIO probe?

In arch/arm64/boot/dts/rockchip/rk3588s-nanopi.dtsi, rgmii_phy1 defines
per-PHY reset properties on the ethernet-phy@1 child node:

	rgmii_phy1: ethernet-phy@1 {
		compatible = "ethernet-phy-ieee802.3-c22";
		reg = <0x1>;
		pinctrl-names = "default";
		pinctrl-0 = <&rtl8211f_rst>;
		reset-assert-us = <20000>;
		reset-deassert-us = <100000>;
		reset-gpios = <&gpio3 RK_PB7 GPIO_ACTIVE_LOW>;
	};

With "ethernet-phy-ieee802.3-c22", fwnode_get_phy_id() returns -EINVAL, so
fwnode_mdiobus_register_phy() in drivers/net/mdio/fwnode_mdio.c calls
get_phy_device() -> get_phy_c22_id() to read MII_PHYSID1 and MII_PHYSID2 over
MDIO before fwnode_mdiobus_phy_device_register() associates the DT node:

	is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
	if (is_c45 || fwnode_get_phy_id(child, &phy_id))
		phy = get_phy_device(bus, addr, is_c45);
	else
		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
	if (IS_ERR(phy)) {
		rc = PTR_ERR(phy);
		goto clean_mii_ts;
	}
	...
	} else if (is_of_node(child)) {
		rc = fwnode_mdiobus_phy_device_register(bus, phy, child, addr);

Per-PHY reset-gpios and pinctrl-0 are only acquired and deasserted later via
fwnode_mdiobus_phy_device_register() -> phy_device_register() in
drivers/net/phy/phy_device.c:

	err = mdiobus_register_device(&phydev->mdio);
	if (err)
		return err;

	/* Deassert the reset signal */
	phy_device_reset(phydev, 0);

If the bootloader leaves gpio3 RK_PB7 asserted at boot, or if the driver is
unbound and rebound (since phy_device_remove() calls phy_device_reset(phydev,
1) to assert the reset line), get_phy_c22_id() reads the PHY ID registers
while the PHY is held in hardware reset and fails with -ENODEV before
reset-gpios can ever be deasserted.

Note that the Rockchip vendor DTS only works with "ethernet-phy-ieee802.3-c22"
because it places legacy snps,reset-gpio on the gmac node, which
stmmac_mdio_reset() deasserts before scanning the MDIO bus.

As documented in Documentation/devicetree/bindings/net/ethernet-phy.yaml,
when a PHY requires a reset to be deasserted before its ID registers can be
read, shouldn't the compatible string keep the explicit "ethernet-phy-id..."
form (or be overridden in the M6V2 board DTS), or should the reset properties
be moved to the mdio1 bus node?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101-nanopi-m6v2-v1-0-e401ea64b394@gmail.com?part=2