Re: [PATCH v2 2/2] arm64: dts: allwinner: a523: x96qpro+: enable Ethernet support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: misplaced PHY clock and pinctrl prevents PHY initialization
- [Medium] arm64: dts: incorrect regulator assigned to phy-supply
- [Low] arm64: dts: redundant clocks property in gmac1 node
--

commit 96eb11f77a00f58bb3ba2189cfd9ad8f4d670b8d
Author: Andre Przywara <andre.przywara@arm.com>

arm64: dts: allwinner: a523: x96qpro+: enable Ethernet support

The X96QPro+ TV Box features a Gigabit Ethernet capable port, which is
driven by the secondary EMAC, and supported by a Maxio MAE0621A PHY.
This commit adds the required devicetree nodes to enable that port.

> diff --git a/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts b/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts
> index eb9fd0a65b41c..8015473ecb7ae 100644
> --- a/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun55i-h728-x96qpro+.dts
[ ... ]
> @@ -72,11 +73,38 @@ &ehci1 {
>  	status = "okay";
>  };
>  
> +&gmac1 {
> +	clocks = <&ccu CLK_BUS_EMAC1>, <&ccu CLK_MBUS_EMAC1>;

[Severity: Low]
Are these clock definitions needed here? It looks like they exactly
duplicate the clocks property already defined for gmac1 in the base
sun55i-a523.dtsi without changing the values.

> +	pinctrl-names = "default";
> +	pinctrl-0 = <&rgmii1_pins>;
> +	phy-mode = "rgmii-id";
> +	phy-handle = <&ext_rgmii_phy>;
> +	phy-supply = <&reg_cldo3>;

[Severity: Medium]
Should this use reg_cldo4 instead of reg_cldo3?

Other sibling A523 boards use reg_cldo4 for gmac1, and reg_cldo4 is
explicitly named vcc-eth-phy in the PMIC definitions. If reg_cldo4 is
the actual PHY power rail, it will be disabled late in the boot process
by the regulator core since it is unreferenced, which would cause the PHY
to lose power.

> +
> +	tx-internal-delay-ps = <200>;
> +	rx-internal-delay-ps = <500>;
> +
> +	status = "okay";
> +};
> +
[ ... ]
> +&mdio1 {
> +	ext_rgmii_phy: ethernet-phy@1 {
> +		compatible = "ethernet-phy-id7b74.4411";
> +		reg = <1>;
> +		clocks = <&ccu CLK_EMAC1_25M>;

[Severity: High]
Since there is no specific Maxio MAE0621A PHY driver, won't this PHY
bind to the generic PHY driver (genphy) which does not parse or enable
clocks?

If so, the 25MHz reference clock will never be enabled by the kernel,
which would prevent the Ethernet PHY from initializing.

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ephy1_25m_pin>;
> +		reset-gpios = <&pio 9 16 GPIO_ACTIVE_LOW>;

[Severity: High]
Does placing the pinctrl here alongside the reset-gpios cause the hardware
reset to execute before the clock pin is muxed?

The MDIO bus core (mdiobus_register_device()) pulses reset-gpios before
device_add() and driver binding. Because pinctrl is applied during driver
probe, the reset is pulsed before the clock pin is muxed and active, meaning
the PHY undergoes hardware reset without its clock running.

Could this leave the PHY in an undefined state?

> +		reset-assert-us = <10000>;
> +		reset-deassert-us = <150000>;
> +	};
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926232142.5244-1-andre.przywara@arm.com?part=2