Re: [PATCH 3/5] arm64: dts: allwinner: a523: Add EMAC0 ethernet MAC

[Andre Przywara <andre.przywara@arm.com>]

On Wed, 23 Apr 2025 22:03:24 +0800
Yixun Lan <dlan@gentoo.org> wrote:

Hi Yixun,

thanks for sending those patches!

> Add EMAC0 ethernet MAC support which found on A523 variant SoCs,
> including the A527/T527 chips.

maybe add here that MAC0 is compatible to the A64, and requires an
external PHY. And that we only add the RGMII pins for now.

> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  arch/arm64/boot/dts/allwinner/sun55i-a523.dtsi | 42 ++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun55i-a523.dtsi b/arch/arm64/boot/dts/allwinner/sun55i-a523.dtsi
> index ee485899ba0af69f32727a53de20051a2e31be1d..c3ba2146c4b45f72c2a5633ec434740d681a21fb 100644
> --- a/arch/arm64/boot/dts/allwinner/sun55i-a523.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun55i-a523.dtsi
> @@ -126,6 +126,17 @@ pio: pinctrl@2000000 {
>  			interrupt-controller;
>  			#interrupt-cells = <3>;
>  
> +			emac0_pins: emac0-pins {

Both the alias and the node name should contain rgmii instead of emac0,
as the other SoCs do, I think:
			rgmii0_pins: rgmii0-pins {

> +				pins = "PH0", "PH1", "PH2", "PH3",
> +					"PH4", "PH5", "PH6", "PH7",
> +					"PH9", "PH10","PH13","PH14",
> +					"PH15","PH16","PH17","PH18";

I think there should be a space behind each comma, and the
first quotation marks in each line should align.

PH13 is EPHY-25M, that's the (optional) 25 MHz output clock pin, for
PHYs without a crystal. That's not controlled by the MAC, so I would
leave it out of this list, as also both the Avaota and the Radxa don't
need it. If there will be a user, they can add this separately.

> +				allwinner,pinmux = <5>;
> +				function = "emac0";
> +				drive-strength = <40>;
> +				bias-pull-up;

Shouldn't this be push-pull, so no pull-up?

The rest looks correct, when compared to the A523 manual.

Cheers,
Andre

> +			};
> +
>  			mmc0_pins: mmc0-pins {
>  				pins = "PF0" ,"PF1", "PF2", "PF3", "PF4", "PF5";
>  				allwinner,pinmux = <2>;
> @@ -409,6 +420,15 @@ i2c5: i2c@2503400 {
>  			#size-cells = <0>;
>  		};
>  
> +		syscon: syscon@3000000 {
> +			compatible = "allwinner,sun55i-a523-system-control",
> +				     "allwinner,sun50i-a64-system-control";
> +			reg = <0x03000000 0x1000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			ranges;
> +		};
> +
>  		gic: interrupt-controller@3400000 {
>  			compatible = "arm,gic-v3";
>  			#address-cells = <1>;
> @@ -521,6 +541,28 @@ ohci1: usb@4200400 {
>  			status = "disabled";
>  		};
>  
> +		emac0: ethernet@4500000 {
> +			compatible = "allwinner,sun55i-a523-emac0",
> +				     "allwinner,sun50i-a64-emac";
> +			reg = <0x04500000 0x10000>;
> +			clocks = <&ccu CLK_BUS_EMAC0>;
> +			clock-names = "stmmaceth";
> +			resets = <&ccu RST_BUS_EMAC0>;
> +			reset-names = "stmmaceth";
> +			interrupts = <GIC_SPI 46 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "macirq";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&emac0_pins>;
> +			syscon = <&syscon>;
> +			status = "disabled";
> +
> +			mdio0: mdio {
> +				compatible = "snps,dwmac-mdio";
> +				#address-cells = <1>;
> +				#size-cells = <0>;
> +			};
> +		};
> +
>  		r_ccu: clock-controller@7010000 {
>  			compatible = "allwinner,sun55i-a523-r-ccu";
>  			reg = <0x7010000 0x250>;
>