Re: [PATCH v2 2/2] arm64: dts: allwinner: h6: Add Orange Pi 3 LTS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] arm64: dts: allwinner: per-PHY reset-gpios and c22 compatible break YT8531C
- [Medium] arm64: dts: allwinner: orangepi-3-lts: swapped LED default-state values
--

commit e75611867d87d079b20765ad9c1ddf5482d2d71a
Author: Jernej Skrabec <jernej.skrabec@gmail.com>

arm64: dts: allwinner: h6: Add Orange Pi 3 LTS

This commit adds a device tree for the Allwinner H6-based Orange Pi 3 LTS
board, describing its regulators, storage, wireless module, LEDs, USB ports,
HDMI, and Motorcomm YT8531C Gigabit Ethernet PHY.

Link: https://lore.kernel.org/linux-sunxi/20250413134318.66681-3-jernej.skrabec@gmail.com/

> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3-lts.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3-lts.dts
> new file mode 100644
> index 0000000000000..e6aa0b2e17ebe
> --- /dev/null
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3-lts.dts
[ ... ]
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		led-0 {
> +			function = LED_FUNCTION_STATUS;
> +			color = <LED_COLOR_ID_RED>;
> +			gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; /* PL4 */
> +			default-state = "on";
> +		};
> +
> +		led-1 {
> +			function = LED_FUNCTION_POWER;
> +			color = <LED_COLOR_ID_GREEN>;
> +			gpios = <&r_pio 0 7 GPIO_ACTIVE_HIGH>; /* PL7 */
> +			default-state = "off";
> +		};
> +	};

[Severity: Medium]
The commit message notes that the green PL7 LED is used as the power
indicator. When gpio_led_probe() -> create_gpio_led() in
drivers/leds/leds-gpio.c parses the leds node, led-0 (red PL4,
LED_FUNCTION_STATUS) is turned on and led-1 (green PL7, LED_FUNCTION_POWER)
is turned off.

When the LED functions were swapped relative to sun50i-h6-orangepi-3.dts,
were the default-state values on led-0 and led-1 accidentally left unswapped?

> +
> +	reg_gmac_3v3: gmac-3v3 {
> +		compatible = "regulator-fixed";
> +		regulator-name = "gmac-3v3";
> +		vin-supply = <&reg_vcc5v>;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		startup-delay-us = <150000>;
> +		enable-active-high;
> +		gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> +	};
[ ... ]
> +&mdio {
> +	ext_rgmii_phy: ethernet-phy@1 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <1>;
> +
> +		motorcomm,clk-out-frequency-hz = <125000000>;
> +		rx-internal-delay-ps = <1950>;
> +		tx-internal-delay-ps = <1950>;
> +
> +		reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* PD14 */
> +		reset-assert-us = <15000>;
> +		reset-deassert-us = <100000>;
> +	};
> +};

[Severity: High]
Will the YT8531C PHY fail to probe or lose its 125 MHz clock output with
compatible = "ethernet-phy-ieee802.3-c22" and reset-gpios placed on the
ethernet-phy@1 node?

First, during MDIO bus registration:

  sun8i_dwmac_init()
    regulator_enable(gmac->regulator) /* enables reg_gmac_3v3 (PD6) */
  stmmac_mdio_register()
    of_mdiobus_register()
      __of_mdiobus_parse_phys()
        fwnode_mdiobus_register_phy()

In drivers/net/mdio/fwnode_mdio.c:fwnode_mdiobus_register_phy():

	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);

Because compatible is not "ethernet-phy-id4f51.e91b" (which is also required
by Documentation/devicetree/bindings/net/motorcomm,yt8xxx.yaml),
fwnode_get_phy_id() fails and get_phy_device() -> get_phy_c22_id() reads
MII_PHYSID1 and MII_PHYSID2 over MDIO before
fwnode_mdiobus_phy_device_register() -> phy_device_register() registers and
deasserts the per-PHY reset-gpios (PD14):

  phy_device_register()
    mdiobus_register_device()
      mdio_device_register_reset()
    phy_device_reset(phydev, 0)

If PD14 is low when the kernel boots (for example, when U-Boot has not
initialized Ethernet) or on driver rebind after phy_device_remove() asserts
reset, the PHY is still held in reset during get_phy_c22_id(), causing MDIO
reads to return 0xffff and PHY registration to fail with -ENODEV.

Second, because reset-gpios is attached to the PHY node rather than the MDIO
bus, bringing the network interface down calls:

  stmmac_release()
    phylink_disconnect_phy()
      phy_disconnect()
        phy_detach()
          phy_device_reset(phydev, 1)

This asserts hardware reset on PD14, resetting YTPHY_SYNCE_CFG_REG back to
its 25 MHz default. When the interface is brought back up via stmmac_open()
-> stmmac_init_phy() -> phylink_fwnode_phy_connect() -> phy_attach_direct()
-> phy_init_hw(), PD14 is deasserted and yt8531_config_init() runs, but
motorcomm,clk-out-frequency-hz is only applied once in yt8531_probe() in
drivers/net/phy/motorcomm.c:

	if (device_property_read_u32(dev, "motorcomm,clk-out-frequency-hz", &freq))
		freq = YTPHY_DTS_OUTPUT_CLK_DIS;
	...
	return ytphy_modify_ext_with_lock(phydev, YTPHY_SYNCE_CFG_REG, mask,
					  val);

Should ethernet-phy@1 use compatible = "ethernet-phy-id4f51.e91b" (or move
the reset handling so the PHY is out of reset before ID reads and not hard
reset across interface down/up cycles)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022-sunxi-orangepi-3-lts-v2-0-70b899252c6a@gmail.com?part=2