[PATCH 0/5] allwinner: Add EMAC0 support to A523 variant SoC

[PATCH 0/5] allwinner: Add EMAC0 support to A523 variant SoC

[Yixun Lan]

This patch series is trying to add EMAC0 ethernet MAC support
to the A523 variant SoCs, including A523, A527/T527 chips.

This MAC0 is compatible to previous A64 SoC, so introduce a new DT
compatible but make it as a fallback to A64's compatible.

In this version, the PHYRSTB pin which routed to external phy
has not been populated in DT. It's kind of optional for now,
but we probably should handle it well later.

I've tested only on Radxa A5E board.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
Yixun Lan (5):
      dt-bindings: sram: sunxi-sram: Add A523 compatible
      dt-bindings: arm: sunxi: Add A523 EMAC0 compatible
      arm64: dts: allwinner: a523: Add EMAC0 ethernet MAC
      arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board
      arm64: dts: allwinner: t527: add EMAC0 to Avaoto-A1 board

 .../bindings/net/allwinner,sun8i-a83t-emac.yaml    |  1 +
 .../sram/allwinner,sun4i-a10-system-control.yaml   |  1 +
 arch/arm64/boot/dts/allwinner/sun55i-a523.dtsi     | 42 ++++++++++++++++++++++
 .../boot/dts/allwinner/sun55i-a527-radxa-a5e.dts   | 17 +++++++++
 .../boot/dts/allwinner/sun55i-t527-avaota-a1.dts   | 17 +++++++++
 5 files changed, 78 insertions(+)
---
base-commit: 69714722df19a7d9e81b7e8f208ca8f325af4502
change-id: 20250423-01-sun55i-emac0-5e395a80f6bf

Best regards,
-- 
Yixun Lan

[PATCH 1/5] dt-bindings: sram: sunxi-sram: Add A523 compatible

[Yixun Lan]

Add new compatible for A527/T527 chips which using same die
as the A523 SoC.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 .../devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml     | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
index a7236f7db4ec34d44c4e2268f76281ef8ed83189..e7f7cf72719ea884d48fff69620467ff2834913b 100644
--- a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
+++ b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
@@ -50,6 +50,7 @@ properties:
           - enum:
               - allwinner,sun50i-a100-system-control
               - allwinner,sun50i-h6-system-control
+              - allwinner,sun55i-a523-system-control
           - const: allwinner,sun50i-a64-system-control
 
   reg:

-- 
2.49.0

Re: [PATCH 1/5] dt-bindings: sram: sunxi-sram: Add A523 compatible

[Andre Przywara]

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

Hi,

> Add new compatible for A527/T527 chips which using same die
> as the A523 SoC.

this reads a bit confusing, what about:
The Allwinner A523 family of SoCs have their "system control" registers
compatible to the A64 SoC, so just add the new SoC specific compatible
string.

Regardless:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  .../devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml     | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
> index a7236f7db4ec34d44c4e2268f76281ef8ed83189..e7f7cf72719ea884d48fff69620467ff2834913b 100644
> --- a/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
> +++ b/Documentation/devicetree/bindings/sram/allwinner,sun4i-a10-system-control.yaml
> @@ -50,6 +50,7 @@ properties:
>            - enum:
>                - allwinner,sun50i-a100-system-control
>                - allwinner,sun50i-h6-system-control
> +              - allwinner,sun55i-a523-system-control
>            - const: allwinner,sun50i-a64-system-control
>  
>    reg:
> 

[PATCH 2/5] dt-bindings: arm: sunxi: Add A523 EMAC0 compatible

[Yixun Lan]

Allwinner A523 SoC variant (A527/T527) contains an "EMAC0" Ethernet
MAC compatible to the A64 version.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
index 7fe0352dff0f8d74a08f3f6aac5450ad685e6a08..7b6a2fde8175353621367c8d8f7a956e4aac7177 100644
--- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
+++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
@@ -23,6 +23,7 @@ properties:
               - allwinner,sun20i-d1-emac
               - allwinner,sun50i-h6-emac
               - allwinner,sun50i-h616-emac0
+              - allwinner,sun55i-a523-emac0
           - const: allwinner,sun50i-a64-emac
 
   reg:

-- 
2.49.0

Re: [PATCH 2/5] dt-bindings: arm: sunxi: Add A523 EMAC0 compatible

[Andre Przywara]

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

> Allwinner A523 SoC variant (A527/T527) contains an "EMAC0" Ethernet
> MAC compatible to the A64 version.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>

I can confirm that the register and DMA descriptor layout in the
manual looks the same as for the A64, and the driver also works, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> index 7fe0352dff0f8d74a08f3f6aac5450ad685e6a08..7b6a2fde8175353621367c8d8f7a956e4aac7177 100644
> --- a/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-a83t-emac.yaml
> @@ -23,6 +23,7 @@ properties:
>                - allwinner,sun20i-d1-emac
>                - allwinner,sun50i-h6-emac
>                - allwinner,sun50i-h616-emac0
> +              - allwinner,sun55i-a523-emac0
>            - const: allwinner,sun50i-a64-emac
>  
>    reg:
> 

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

[Yixun Lan]

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

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 {
+				pins = "PH0", "PH1", "PH2", "PH3",
+					"PH4", "PH5", "PH6", "PH7",
+					"PH9", "PH10","PH13","PH14",
+					"PH15","PH16","PH17","PH18";
+				allwinner,pinmux = <5>;
+				function = "emac0";
+				drive-strength = <40>;
+				bias-pull-up;
+			};
+
 			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>;

-- 
2.49.0

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

[Andre Przywara]

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

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

[Yixun Lan]

Hi Andre,

On 01:43 Thu 24 Apr     , Andre Przywara wrote:
> 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.
> 
ok

> > 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 {
> 
ok
> > +				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.
> 
will do

> 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.
> 
make sense

> > +				allwinner,pinmux = <5>;
> > +				function = "emac0";
> > +				drive-strength = <40>;
> > +				bias-pull-up;
> 
> Shouldn't this be push-pull, so no pull-up?
> 
will drop

> The rest looks correct, when compared to the A523 manual.
> 
thanks for review

-- 
Yixun Lan (dlan)

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

[Chen-Yu Tsai]

On Thu, Apr 24, 2025 at 11:28 AM Yixun Lan <dlan@gentoo.org> wrote:
>
> Hi Andre,
>
> On 01:43 Thu 24 Apr     , Andre Przywara wrote:
> > 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.
> >
> ok
>
> > > 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 {
> >
> ok
> > > +                           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.
> >
> will do
>
> > 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.
> >
> make sense
>
> > > +                           allwinner,pinmux = <5>;
> > > +                           function = "emac0";
> > > +                           drive-strength = <40>;
> > > +                           bias-pull-up;
> >
> > Shouldn't this be push-pull, so no pull-up?
> >
> will drop

It would be better to have an explicit "bias-disable" here.
That way you are not depending on the bootloader setting a
certain state, or depending on the reset default.

ChenYu

> > The rest looks correct, when compared to the A523 manual.
> >
> thanks for review
>
> --
> Yixun Lan (dlan)

[PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Yixun Lan]

On Radxa A5E board, the EMAC0 connect to an external YT8531C PHY,
which features a 25MHz crystal, and using PH8 pin as PHY reset.

Tested on A5E board with schematic V1.20.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
 arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts b/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
index 912e1bda974ce5f64c425e371357b1a78b7c13dd..b3399a28badb5172801e47b8a45d5b753fc56ef1 100644
--- a/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
+++ b/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
@@ -54,6 +54,23 @@ &ehci1 {
 	status = "okay";
 };
 
+&emac0 {
+	phy-mode = "rgmii";
+	phy-handle = <&ext_rgmii_phy>;
+
+	allwinner,tx-delay-ps = <300>;
+	allwinner,rx-delay-ps = <400>;
+
+	status = "okay";
+};
+
+&mdio0 {
+	ext_rgmii_phy: ethernet-phy@1 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <1>;
+	};
+};
+
 &mmc0 {
 	vmmc-supply = <&reg_cldo3>;
 	cd-gpios = <&pio 5 6 (GPIO_ACTIVE_LOW | GPIO_PULL_DOWN)>; /* PF6 */

-- 
2.49.0

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andrew Lunn]

> +&emac0 {
> +	phy-mode = "rgmii";

Does the PCB have extra long clock lines in order to provide the
needed 2ns delay? I guess not, so this should be rgmii-id.

> +	phy-handle = <&ext_rgmii_phy>;
> +
> +	allwinner,tx-delay-ps = <300>;
> +	allwinner,rx-delay-ps = <400>;

These are rather low delays, since the standard requires 2ns. Anyway,
once you change phy-mode, you probably don't need these.

     Andrew

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

On Wed, 23 Apr 2025 18:58:37 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

Hi,

> > +&emac0 {
> > +	phy-mode = "rgmii";  
> 
> Does the PCB have extra long clock lines in order to provide the
> needed 2ns delay? I guess not, so this should be rgmii-id.

That's a good point, and it probably true.

> 
> > +	phy-handle = <&ext_rgmii_phy>;
> > +
> > +	allwinner,tx-delay-ps = <300>;
> > +	allwinner,rx-delay-ps = <400>;  
> 
> These are rather low delays, since the standard requires 2ns. Anyway,
> once you change phy-mode, you probably don't need these.

Those go on top of the main 2ns delay, I guess to accommodate some skew
between the RX and TX lines, or to account for extra some PCB delay
between clock and data? The vendor BSP kernels/DTs program those board
specific values, so we have been following suit for a while, for the
previous SoCs as well.
I just tried, it also works with some variations of those values, but
setting tx-delay to 0 stops communication.

Cheers,
Andre

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Yixun Lan]

Hi Andrew, Andre,

On 01:42 Thu 24 Apr     , Andre Przywara wrote:
> On Wed, 23 Apr 2025 18:58:37 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> Hi,
> 
> > > +&emac0 {
> > > +	phy-mode = "rgmii";  
> > 
> > Does the PCB have extra long clock lines in order to provide the
> > needed 2ns delay? I guess not, so this should be rgmii-id.
> 
> That's a good point, and it probably true.
> 
> > 
> > > +	phy-handle = <&ext_rgmii_phy>;
> > > +
> > > +	allwinner,tx-delay-ps = <300>;
> > > +	allwinner,rx-delay-ps = <400>;  
> > 
> > These are rather low delays, since the standard requires 2ns. Anyway,
> > once you change phy-mode, you probably don't need these.
> 
As I tested, drop these two properties making ethernet unable to work,
there might be some space to improve, but currently I'd leave it for now

> Those go on top of the main 2ns delay, I guess to accommodate some skew
> between the RX and TX lines, or to account for extra some PCB delay
> between clock and data? The vendor BSP kernels/DTs program those board
> specific values, so we have been following suit for a while, for the
> previous SoCs as well.
> I just tried, it also works with some variations of those values, but
> setting tx-delay to 0 stops communication.
> 
I'd not bother to try other combinations, and just stick to vendor's settings

thanks

-- 
Yixun Lan (dlan)

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

On Thu, 24 Apr 2025 10:05:14 +0000
Yixun Lan <dlan@gentoo.org> wrote:

Hi,

> Hi Andrew, Andre,
> 
> On 01:42 Thu 24 Apr     , Andre Przywara wrote:
> > On Wed, 23 Apr 2025 18:58:37 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > Hi,
> >   
> > > > +&emac0 {
> > > > +	phy-mode = "rgmii";    
> > > 
> > > Does the PCB have extra long clock lines in order to provide the
> > > needed 2ns delay? I guess not, so this should be rgmii-id.  
> > 
> > That's a good point, and it probably true.
> >   
> > >   
> > > > +	phy-handle = <&ext_rgmii_phy>;
> > > > +
> > > > +	allwinner,tx-delay-ps = <300>;
> > > > +	allwinner,rx-delay-ps = <400>;    
> > > 
> > > These are rather low delays, since the standard requires 2ns. Anyway,
> > > once you change phy-mode, you probably don't need these.  
> >   
> As I tested, drop these two properties making ethernet unable to work,
> there might be some space to improve, but currently I'd leave it for now

Yes, I think we need those delays to be programmed into the syscon clock
register, and have been doing so for years.

> > Those go on top of the main 2ns delay, I guess to accommodate some skew
> > between the RX and TX lines, or to account for extra some PCB delay
> > between clock and data? The vendor BSP kernels/DTs program those board
> > specific values, so we have been following suit for a while, for the
> > previous SoCs as well.
> > I just tried, it also works with some variations of those values, but
> > setting tx-delay to 0 stops communication.
> >   
> I'd not bother to try other combinations, and just stick to vendor's settings

I learned to not rely too much on Allwinner BSP settings ;-)

And it's quite easy to experiment, actually: you can write directly to
0x3000030, for instance using devmem or peekpoke, at Linux runtime. Run
some tests or benchmarks, then try a new setting, without rebooting.

Cheers,
Andre.

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andrew Lunn]

> I'd not bother to try other combinations, and just stick to vendor's
> settings

Vendors get stuff wrong all the time. Just because it works does not
mean it is correct. And RGMII delays are very frequently wrong because
there are multiple ways to get a link which works, but don't follow
the DT binding.

	Andrew

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

On Thu, 24 Apr 2025 14:19:59 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

Hi Andrew,

> > I'd not bother to try other combinations, and just stick to vendor's
> > settings  
> 
> Vendors get stuff wrong all the time. Just because it works does not
> mean it is correct. And RGMII delays are very frequently wrong because
> there are multiple ways to get a link which works, but don't follow
> the DT binding.

Speaking of which: do you know of a good method to verify the delay
timing? Is there *something* which is sensitive to those timings and which
can be easily checked and qualified?
I just tried iperf3 yesterday, but didn't spot any real change in the
numbers when toying with those delay values.
As mentioned in the other email, we can easily hack the values at runtime,
so if there is a way to get some "quality" value, this could even be
automated.

Thanks,
Andre

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andrew Lunn]

> Speaking of which: do you know of a good method to verify the delay
> timing? Is there *something* which is sensitive to those timings and which
> can be easily checked and qualified?

Not really. If you are on the edge, you might get more ethernet FCS
errors. For iperf3 in UDP mode, that would translate to packet loss.
Or ethtool -S might report checksum errors, depending on the driver.

From what i've heard from others there is a pretty sharp cut off from
broken to working. Just stay away from the cliff and it should be
O.K. for −40°C to 85°C, or whatever the temperate range is supposed to
be.

	Andrew

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andrew Lunn]

On Thu, Apr 24, 2025 at 01:42:41AM +0100, Andre Przywara wrote:
> On Wed, 23 Apr 2025 18:58:37 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> Hi,
> 
> > > +&emac0 {
> > > +	phy-mode = "rgmii";  
> > 
> > Does the PCB have extra long clock lines in order to provide the
> > needed 2ns delay? I guess not, so this should be rgmii-id.
> 
> That's a good point, and it probably true.
> 
> > 
> > > +	phy-handle = <&ext_rgmii_phy>;
> > > +
> > > +	allwinner,tx-delay-ps = <300>;
> > > +	allwinner,rx-delay-ps = <400>;  
> > 
> > These are rather low delays, since the standard requires 2ns. Anyway,
> > once you change phy-mode, you probably don't need these.
> 
> Those go on top of the main 2ns delay

Which 2ns delay? "rgmii" means don't add 2ns delay, the PCB is doing
it. So if there is a 2ns delay, something is broken by not respecting
"rgmii".

> I just tried, it also works with some variations of those values, but
> setting tx-delay to 0 stops communication.

Just to be clear, you tried it with "rgmii-id" and the same <300> and
<400> values?

	Andrew

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

On Thu, 24 Apr 2025 14:16:16 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> On Thu, Apr 24, 2025 at 01:42:41AM +0100, Andre Przywara wrote:
> > On Wed, 23 Apr 2025 18:58:37 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > Hi,
> >   
> > > > +&emac0 {
> > > > +	phy-mode = "rgmii";    
> > > 
> > > Does the PCB have extra long clock lines in order to provide the
> > > needed 2ns delay? I guess not, so this should be rgmii-id.  
> > 
> > That's a good point, and it probably true.
> >   
> > >   
> > > > +	phy-handle = <&ext_rgmii_phy>;
> > > > +
> > > > +	allwinner,tx-delay-ps = <300>;
> > > > +	allwinner,rx-delay-ps = <400>;    
> > > 
> > > These are rather low delays, since the standard requires 2ns. Anyway,
> > > once you change phy-mode, you probably don't need these.  
> > 
> > Those go on top of the main 2ns delay  
> 
> Which 2ns delay? "rgmii" means don't add 2ns delay, the PCB is doing
> it. So if there is a 2ns delay, something is broken by not respecting
> "rgmii".
> 
> > I just tried, it also works with some variations of those values, but
> > setting tx-delay to 0 stops communication.  
> 
> Just to be clear, you tried it with "rgmii-id" and the same <300> and
> <400> values?

Yes, sorry, I wasn't clear: I used rgmii-id, then experimented with those
values. I briefly tried "rgmii", and I couldn't get a lease, so I quite
confident it's rgmii-id, as you said. The vendor DTs just use "rgmii", but
they might hack the delay up another way (and I cannot be asked to look at
that awful code).

Cheers,
Andre

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andrew Lunn]

> > Just to be clear, you tried it with "rgmii-id" and the same <300> and
> > <400> values?
> 
> Yes, sorry, I wasn't clear: I used rgmii-id, then experimented with those
> values.

O.K, great.

I do suspect the delays are not actually in pico seconds. But without
a data sheet, it is hard to know.

       if (!of_property_read_u32(node, "allwinner,rx-delay-ps", &val)) {
                if (val % 100) {
                        dev_err(dev, "rx-delay must be a multiple of 100\n");
                        return -EINVAL;
                }
                val /= 100;
                dev_dbg(dev, "set rx-delay to %x\n", val);
                if (val <= gmac->variant->rx_delay_max) {
                        reg &= ~(gmac->variant->rx_delay_max <<
                                 SYSCON_ERXDC_SHIFT);
                        reg |= (val << SYSCON_ERXDC_SHIFT);

So the code divides by 100 and writes it to a register. But:

static const struct emac_variant emac_variant_h3 = {
        .rx_delay_max = 31,


static const struct emac_variant emac_variant_r40 = {
        .rx_delay_max = 7,
};

With the change from 7 to 31, did the range get extended by a factor
of 4, or did the step go down by a factor of 4, and the / 100 should
be / 25? I suppose the git history might have the answer in the commit
message, but i'm too lazy to go look.

	Andrew



I briefly tried "rgmii", and I couldn't get a lease, so I quite
> confident it's rgmii-id, as you said. The vendor DTs just use "rgmii", but
> they might hack the delay up another way (and I cannot be asked to look at
> that awful code).
> 
> Cheers,
> Andre

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

On Thu, 24 Apr 2025 14:57:27 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

Hi Andrew,

> > > Just to be clear, you tried it with "rgmii-id" and the same <300> and
> > > <400> values?  
> > 
> > Yes, sorry, I wasn't clear: I used rgmii-id, then experimented with those
> > values.  
> 
> O.K, great.
> 
> I do suspect the delays are not actually in pico seconds. But without
> a data sheet, it is hard to know.
> 
>        if (!of_property_read_u32(node, "allwinner,rx-delay-ps", &val)) {
>                 if (val % 100) {
>                         dev_err(dev, "rx-delay must be a multiple of 100\n");
>                         return -EINVAL;
>                 }
>                 val /= 100;
>                 dev_dbg(dev, "set rx-delay to %x\n", val);
>                 if (val <= gmac->variant->rx_delay_max) {
>                         reg &= ~(gmac->variant->rx_delay_max <<
>                                  SYSCON_ERXDC_SHIFT);
>                         reg |= (val << SYSCON_ERXDC_SHIFT);
> 
> So the code divides by 100 and writes it to a register. But:
> 
> static const struct emac_variant emac_variant_h3 = {
>         .rx_delay_max = 31,
> 
> 
> static const struct emac_variant emac_variant_r40 = {
>         .rx_delay_max = 7,
> };
> 
> With the change from 7 to 31, did the range get extended by a factor
> of 4, or did the step go down by a factor of 4, and the / 100 should
> be / 25? I suppose the git history might have the answer in the commit
> message, but i'm too lazy to go look.

IIRC this picosecond mapping was somewhat made up, to match common DT
properties. The manual just says:
====================
12:10  R/W  default: 0x0 ETXDC: Configure EMAC Transmit Clock Delay Chain.
====================

So the unit is really unknown, but is probably some kind of internal cycle count.
The change from 7 to 31 is purely because the bitfield grew from 3 to 5
bits. We don't know if the underlying unit changed on the way.
Those values are just copied from whatever the board vendor came up with,
we then multiply them by 100 and put them in the mainline DT. Welcome to
the world of Allwinner ;-)

And git history doesn't help, it's all already in the first commit for this
driver. I remember some discussions on the mailing list, almost 10 years
ago, but this requires even more digging ...

Cheers,
Andre



> 
> I briefly tried "rgmii", and I couldn't get a lease, so I quite
> > confident it's rgmii-id, as you said. The vendor DTs just use "rgmii", but
> > they might hack the delay up another way (and I cannot be asked to look at
> > that awful code).
> > 
> > Cheers,
> > Andre  

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Jernej Škrabec]

cc: Corentin LABBE

Dne četrtek, 24. april 2025 ob 16:00:37 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> On Thu, 24 Apr 2025 14:57:27 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> Hi Andrew,
> 
> > > > Just to be clear, you tried it with "rgmii-id" and the same <300> and
> > > > <400> values?  
> > > 
> > > Yes, sorry, I wasn't clear: I used rgmii-id, then experimented with those
> > > values.  
> > 
> > O.K, great.
> > 
> > I do suspect the delays are not actually in pico seconds. But without
> > a data sheet, it is hard to know.
> > 
> >        if (!of_property_read_u32(node, "allwinner,rx-delay-ps", &val)) {
> >                 if (val % 100) {
> >                         dev_err(dev, "rx-delay must be a multiple of 100\n");
> >                         return -EINVAL;
> >                 }
> >                 val /= 100;
> >                 dev_dbg(dev, "set rx-delay to %x\n", val);
> >                 if (val <= gmac->variant->rx_delay_max) {
> >                         reg &= ~(gmac->variant->rx_delay_max <<
> >                                  SYSCON_ERXDC_SHIFT);
> >                         reg |= (val << SYSCON_ERXDC_SHIFT);
> > 
> > So the code divides by 100 and writes it to a register. But:
> > 
> > static const struct emac_variant emac_variant_h3 = {
> >         .rx_delay_max = 31,
> > 
> > 
> > static const struct emac_variant emac_variant_r40 = {
> >         .rx_delay_max = 7,
> > };
> > 
> > With the change from 7 to 31, did the range get extended by a factor
> > of 4, or did the step go down by a factor of 4, and the / 100 should
> > be / 25? I suppose the git history might have the answer in the commit
> > message, but i'm too lazy to go look.
> 
> IIRC this picosecond mapping was somewhat made up, to match common DT
> properties. The manual just says:
> ====================
> 12:10  R/W  default: 0x0 ETXDC: Configure EMAC Transmit Clock Delay Chain.
> ====================
> 
> So the unit is really unknown, but is probably some kind of internal cycle count.
> The change from 7 to 31 is purely because the bitfield grew from 3 to 5
> bits. We don't know if the underlying unit changed on the way.
> Those values are just copied from whatever the board vendor came up with,
> we then multiply them by 100 and put them in the mainline DT. Welcome to
> the world of Allwinner ;-)

IIRC Corentin asked Allwinner about units and their response was in 100 ps.

In my experience, vendor DT has proper delays specified, just 7 instead of
700, for example. What they get wrong, or better said, don't care, is phy
mode. It's always set to rgmii because phy driver most of the time ignores
this value and phy IC just uses mode set using resistors. Proper way here
would be to check schematic and set phy mode according to that. This method
always works, except for one board, which had resistors set wrong and
phy mode configured over phy driver was actually fix for it.

Best regards,
Jernej

> 
> And git history doesn't help, it's all already in the first commit for this
> driver. I remember some discussions on the mailing list, almost 10 years
> ago, but this requires even more digging ...
> 
> Cheers,
> Andre
> 
> 
> 
> > 
> > I briefly tried "rgmii", and I couldn't get a lease, so I quite
> > > confident it's rgmii-id, as you said. The vendor DTs just use "rgmii", but
> > > they might hack the delay up another way (and I cannot be asked to look at
> > > that awful code).
> > > 
> > > Cheers,
> > > Andre  
> 
> 

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andrew Lunn]

> In my experience, vendor DT has proper delays specified, just 7 instead of
> 700, for example. What they get wrong, or better said, don't care, is phy
> mode. It's always set to rgmii because phy driver most of the time ignores
> this value and phy IC just uses mode set using resistors. Proper way here
> would be to check schematic and set phy mode according to that. This method
> always works, except for one board, which had resistors set wrong and
> phy mode configured over phy driver was actually fix for it.

What PHY driver is this? If it is ignoring the mode, it is broken.

We have had problems in the past in this respect. A PHY driver which
ignored the RGMII modes, and strapping was used. That 'worked' until
somebody built a board with broken strapping and added code to respect
the RGMII mode, overriding the strapping. It made that board work, but
broke lots of others which had the wrong RGMII mode....

If we have this again, i would like to know so we can try to get in
front of the problem, before we have lots of broken boards...

	Andrew

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Chen-Yu Tsai]

On Fri, Apr 25, 2025 at 3:02 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > In my experience, vendor DT has proper delays specified, just 7 instead of
> > 700, for example. What they get wrong, or better said, don't care, is phy
> > mode. It's always set to rgmii because phy driver most of the time ignores
> > this value and phy IC just uses mode set using resistors. Proper way here
> > would be to check schematic and set phy mode according to that. This method
> > always works, except for one board, which had resistors set wrong and
> > phy mode configured over phy driver was actually fix for it.
>
> What PHY driver is this? If it is ignoring the mode, it is broken.
>
> We have had problems in the past in this respect. A PHY driver which
> ignored the RGMII modes, and strapping was used. That 'worked' until
> somebody built a board with broken strapping and added code to respect
> the RGMII mode, overriding the strapping. It made that board work, but
> broke lots of others which had the wrong RGMII mode....
>
> If we have this again, i would like to know so we can try to get in
> front of the problem, before we have lots of broken boards...

I think the incident you are referring to is exactly the one that Jernej
mentioned.

And regarding the bad PHY driver, it could simply be that the PHY driver
was not built or not loaded, hence the kernel falling back to the generic
one, which of course doesn't know how to set the modes.

ChenYu

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Jernej Škrabec]

Dne četrtek, 24. april 2025 ob 21:05:17 Srednjeevropski poletni čas je Chen-Yu Tsai napisal(a):
> On Fri, Apr 25, 2025 at 3:02 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > In my experience, vendor DT has proper delays specified, just 7 instead of
> > > 700, for example. What they get wrong, or better said, don't care, is phy
> > > mode. It's always set to rgmii because phy driver most of the time ignores
> > > this value and phy IC just uses mode set using resistors. Proper way here
> > > would be to check schematic and set phy mode according to that. This method
> > > always works, except for one board, which had resistors set wrong and
> > > phy mode configured over phy driver was actually fix for it.
> >
> > What PHY driver is this? If it is ignoring the mode, it is broken.
> >
> > We have had problems in the past in this respect. A PHY driver which
> > ignored the RGMII modes, and strapping was used. That 'worked' until
> > somebody built a board with broken strapping and added code to respect
> > the RGMII mode, overriding the strapping. It made that board work, but
> > broke lots of others which had the wrong RGMII mode....
> >
> > If we have this again, i would like to know so we can try to get in
> > front of the problem, before we have lots of broken boards...
> 
> I think the incident you are referring to is exactly the one that Jernej
> mentioned.
> 
> And regarding the bad PHY driver, it could simply be that the PHY driver
> was not built or not loaded, hence the kernel falling back to the generic
> one, which of course doesn't know how to set the modes.

Mainline is sorted out as far as I'm aware. Broken PHY drivers are part
of BSP code drops, from where these values are taken from. So, for sure
I wouldn't trust phy mode set in BSP code, but allwinner,tx-delay-ps and
allwinner,rx-delay-ps are usually trustworthy.

Best regards,
Jernej

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

On Thu, 24 Apr 2025 20:38:34 +0200
Jernej Škrabec <jernej.skrabec@gmail.com> wrote:

> cc: Corentin LABBE
> 
> Dne četrtek, 24. april 2025 ob 16:00:37 Srednjeevropski poletni čas je Andre Przywara napisal(a):
> > On Thu, 24 Apr 2025 14:57:27 +0200
> > Andrew Lunn <andrew@lunn.ch> wrote:
> > 
> > Hi Andrew,
> >   
> > > > > Just to be clear, you tried it with "rgmii-id" and the same <300> and
> > > > > <400> values?    
> > > > 
> > > > Yes, sorry, I wasn't clear: I used rgmii-id, then experimented with those
> > > > values.    
> > > 
> > > O.K, great.
> > > 
> > > I do suspect the delays are not actually in pico seconds. But without
> > > a data sheet, it is hard to know.
> > > 
> > >        if (!of_property_read_u32(node, "allwinner,rx-delay-ps", &val)) {
> > >                 if (val % 100) {
> > >                         dev_err(dev, "rx-delay must be a multiple of 100\n");
> > >                         return -EINVAL;
> > >                 }
> > >                 val /= 100;
> > >                 dev_dbg(dev, "set rx-delay to %x\n", val);
> > >                 if (val <= gmac->variant->rx_delay_max) {
> > >                         reg &= ~(gmac->variant->rx_delay_max <<
> > >                                  SYSCON_ERXDC_SHIFT);
> > >                         reg |= (val << SYSCON_ERXDC_SHIFT);
> > > 
> > > So the code divides by 100 and writes it to a register. But:
> > > 
> > > static const struct emac_variant emac_variant_h3 = {
> > >         .rx_delay_max = 31,
> > > 
> > > 
> > > static const struct emac_variant emac_variant_r40 = {
> > >         .rx_delay_max = 7,
> > > };
> > > 
> > > With the change from 7 to 31, did the range get extended by a factor
> > > of 4, or did the step go down by a factor of 4, and the / 100 should
> > > be / 25? I suppose the git history might have the answer in the commit
> > > message, but i'm too lazy to go look.  
> > 
> > IIRC this picosecond mapping was somewhat made up, to match common DT
> > properties. The manual just says:
> > ====================
> > 12:10  R/W  default: 0x0 ETXDC: Configure EMAC Transmit Clock Delay Chain.
> > ====================
> > 
> > So the unit is really unknown, but is probably some kind of internal cycle count.
> > The change from 7 to 31 is purely because the bitfield grew from 3 to 5
> > bits. We don't know if the underlying unit changed on the way.
> > Those values are just copied from whatever the board vendor came up with,
> > we then multiply them by 100 and put them in the mainline DT. Welcome to
> > the world of Allwinner ;-)  
> 
> IIRC Corentin asked Allwinner about units and their response was in 100 ps.
> 
> In my experience, vendor DT has proper delays specified, just 7 instead of
> 700, for example. What they get wrong, or better said, don't care, is phy
> mode. It's always set to rgmii because phy driver most of the time ignores
> this value and phy IC just uses mode set using resistors.

Ah, right, I dimly remembered there was some hardware setting, but your
mentioning of those strap resistors now tickled my memory!

So according to the Radxa board schematic, RGMII0-RXD0/RXDLY is pulled
up to VCCIO via 4.7K, while RGMII0-RXD1/TXDLY is pulled to GND (also via
4K7). According to the Motorcom YT8531 datasheet this means that RX
delay is enabled, but TX delay is not.
The Avaota board uses the same setup, albeit with an RTL8211F-CG PHY,
but its datasheet confirms it uses the same logic.

So does this mean we should say rgmii-rxid, so that the MAC adds the TX
delay? Does the stmmac driver actually support this? I couldn't find
this part by quickly checking the code.

Cheers,
Andre

> Proper way here
> would be to check schematic and set phy mode according to that. This method
> always works, except for one board, which had resistors set wrong and
> phy mode configured over phy driver was actually fix for it.
> 
> Best regards,
> Jernej
> 
> > 
> > And git history doesn't help, it's all already in the first commit for this
> > driver. I remember some discussions on the mailing list, almost 10 years
> > ago, but this requires even more digging ...
> > 
> > Cheers,
> > Andre
> > 
> > 
> >   
> > > 
> > > I briefly tried "rgmii", and I couldn't get a lease, so I quite  
> > > > confident it's rgmii-id, as you said. The vendor DTs just use "rgmii", but
> > > > they might hack the delay up another way (and I cannot be asked to look at
> > > > that awful code).
> > > > 
> > > > Cheers,
> > > > Andre    
> > 
> >   
> 
> 
> 
> 
> 

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andrew Lunn]

> Ah, right, I dimly remembered there was some hardware setting, but your
> mentioning of those strap resistors now tickled my memory!
> 
> So according to the Radxa board schematic, RGMII0-RXD0/RXDLY is pulled
> up to VCCIO via 4.7K, while RGMII0-RXD1/TXDLY is pulled to GND (also via
> 4K7). According to the Motorcom YT8531 datasheet this means that RX
> delay is enabled, but TX delay is not.
> The Avaota board uses the same setup, albeit with an RTL8211F-CG PHY,
> but its datasheet confirms it uses the same logic.
> 
> So does this mean we should say rgmii-rxid, so that the MAC adds the TX
> delay? Does the stmmac driver actually support this? I couldn't find
> this part by quickly checking the code.

No. It is what the PCB provides which matters. A very small number of
PCB have extra long clock lines to add the 2ns delay. Those boards
should use 'rgmii'. All other boards should use rgmii-id, meaning the
delays need to be provided somewhere else. Typically it is the PHY
which adds the delays.

The strapping should not matter, the PHY driver will override that. So
'rgmii-id' should result in the PHY doing the basis 2ns in both
directions. The MAC DT properties then add additional delays, which i
consider fine tuning. Most systems don't actually need fine tuning,
but the YT8531 is funky, it often does need it for some reason.

	Andrew

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

On Fri, 25 Apr 2025 04:01:30 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> > Ah, right, I dimly remembered there was some hardware setting, but your
> > mentioning of those strap resistors now tickled my memory!
> > 
> > So according to the Radxa board schematic, RGMII0-RXD0/RXDLY is pulled
> > up to VCCIO via 4.7K, while RGMII0-RXD1/TXDLY is pulled to GND (also via
> > 4K7). According to the Motorcom YT8531 datasheet this means that RX
> > delay is enabled, but TX delay is not.
> > The Avaota board uses the same setup, albeit with an RTL8211F-CG PHY,
> > but its datasheet confirms it uses the same logic.
> > 
> > So does this mean we should say rgmii-rxid, so that the MAC adds the TX
> > delay? Does the stmmac driver actually support this? I couldn't find
> > this part by quickly checking the code.  
> 
> No. It is what the PCB provides which matters. A very small number of
> PCB have extra long clock lines to add the 2ns delay. Those boards
> should use 'rgmii'. All other boards should use rgmii-id, meaning the
> delays need to be provided somewhere else. Typically it is the PHY
> which adds the delays.
> 
> The strapping should not matter, the PHY driver will override that. So
> 'rgmii-id' should result in the PHY doing the basis 2ns in both
> directions. The MAC DT properties then add additional delays, which i
> consider fine tuning. Most systems don't actually need fine tuning,
> but the YT8531 is funky, it often does need it for some reason.

Ah, many thanks for the explanation, that clears that up! I read something
about the MAC adding delays, which confused me, but what you say now makes
sense.

Thanks!
Andre.

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Andre Przywara]

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

Hi Yixun,

> On Radxa A5E board, the EMAC0 connect to an external YT8531C PHY,
> which features a 25MHz crystal, and using PH8 pin as PHY reset.
> 
> Tested on A5E board with schematic V1.20.

Can you please add a name to the /aliases node, to make U-Boot add a
MAC address?
	ethernet0 = &emac0;

> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
>  arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts b/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
> index 912e1bda974ce5f64c425e371357b1a78b7c13dd..b3399a28badb5172801e47b8a45d5b753fc56ef1 100644
> --- a/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
> @@ -54,6 +54,23 @@ &ehci1 {
>  	status = "okay";
>  };
>  
> +&emac0 {
> +	phy-mode = "rgmii";
> +	phy-handle = <&ext_rgmii_phy>;

Can you please add the phy-supply here, which should be CLDO3? It's
referenced by other nodes, so would be enabled already,but each node
should be self-contained.

Cheers,
Andre

> +
> +	allwinner,tx-delay-ps = <300>;
> +	allwinner,rx-delay-ps = <400>;
> +
> +	status = "okay";
> +};
> +
> +&mdio0 {
> +	ext_rgmii_phy: ethernet-phy@1 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <1>;
> +	};
> +};
> +
>  &mmc0 {
>  	vmmc-supply = <&reg_cldo3>;
>  	cd-gpios = <&pio 5 6 (GPIO_ACTIVE_LOW | GPIO_PULL_DOWN)>; /* PF6 */
> 

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Yixun Lan]

Hi Andre,

On 01:43 Thu 24 Apr     , Andre Przywara wrote:
> On Wed, 23 Apr 2025 22:03:25 +0800
> Yixun Lan <dlan@gentoo.org> wrote:
> 
> Hi Yixun,
> 
> > On Radxa A5E board, the EMAC0 connect to an external YT8531C PHY,
> > which features a 25MHz crystal, and using PH8 pin as PHY reset.
> > 
> > Tested on A5E board with schematic V1.20.
> 
> Can you please add a name to the /aliases node, to make U-Boot add a
> MAC address?
> 	ethernet0 = &emac0;
> 
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> >  arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts b/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
> > index 912e1bda974ce5f64c425e371357b1a78b7c13dd..b3399a28badb5172801e47b8a45d5b753fc56ef1 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun55i-a527-radxa-a5e.dts
> > @@ -54,6 +54,23 @@ &ehci1 {
> >  	status = "okay";
> >  };
> >  
> > +&emac0 {
> > +	phy-mode = "rgmii";
> > +	phy-handle = <&ext_rgmii_phy>;
> 
> Can you please add the phy-supply here, which should be CLDO3? It's
> referenced by other nodes, so would be enabled already,but each node
> should be self-contained.
right, I was about to check and then forgot.. will add it back next version

-- 
Yixun Lan (dlan)

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Chukun Pan]

Hi,

> On Radxa A5E board, the EMAC0 connect to an external YT8531C PHY,
> which features a 25MHz crystal, and using PH8 pin as PHY reset.
> 
> Tested on A5E board with schematic V1.20.

Although the schematic says it is YT8531C, the PHY on the V1.20 board
is Maxio MAE0621A. The article of cnx-software also mentioned this:
https://www.cnx-software.com/2025/01/04/radxa-cubie-a5e-allwinner-a527-t527-sbc-with-hdmi-2-0-dual-gbe-wifi-6-bluetooth-5-4/

Thanks,
Chukun

--
2.25.1

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Yixun Lan]

Hi Chukun,

On 11:30 Fri 25 Apr     , Chukun Pan wrote:
> Hi,
> 
> > On Radxa A5E board, the EMAC0 connect to an external YT8531C PHY,
> > which features a 25MHz crystal, and using PH8 pin as PHY reset.
> > 
> > Tested on A5E board with schematic V1.20.
> 
> Although the schematic says it is YT8531C, the PHY on the V1.20 board
> is Maxio MAE0621A. The article of cnx-software also mentioned this:
> https://www.cnx-software.com/2025/01/04/radxa-cubie-a5e-allwinner-a527-t527-sbc-with-hdmi-2-0-dual-gbe-wifi-6-bluetooth-5-4/
> 
IMO, then the schematic should be updated, I could definitely adjust
the commit message to reflect this change, but don't know if further
action need to take, like writing a new phy driver, I guess a fallback
to generic phy just works?
(google says, the MAE0621A is a pin-to-pin chip to RTL8211F..)

-- 
Yixun Lan (dlan)

Re: [PATCH 4/5] arm64: dts: allwinner: a527: add EMAC0 to Radxa A5E board

[Chukun Pan]

Hi,

> > > On Radxa A5E board, the EMAC0 connect to an external YT8531C PHY,
> > > which features a 25MHz crystal, and using PH8 pin as PHY reset.
> > >
> > > Tested on A5E board with schematic V1.20.
> >
> > Although the schematic says it is YT8531C, the PHY on the V1.20 board
> > is Maxio MAE0621A. The article of cnx-software also mentioned this:
>
> IMO, then the schematic should be updated, I could definitely adjust
> the commit message to reflect this change, but don't know if further
> action need to take, like writing a new phy driver, I guess a fallback
> to generic phy just works?

The schematic on the radxa website is still V1.10. [1]
So how did you test it on the A5E board? Both PHYs
on the board (V1.20) are Maxio MAE0621A.

dmesg should show the PHY driver used:
dwmac-sun8i ... eth0: PHY [stmmac-0:01] driver [YT8531 Gigabit Ethernet] (irq=POLL)

[1] https://radxa.com/products/cubie/a5e/#downloads

Thanks,
Chukun

--
2.25.1

[PATCH 5/5] arm64: dts: allwinner: t527: add EMAC0 to Avaoto-A1 board

[Yixun Lan]

On Avaoto A1 board, the EMAC0 connect to an external RTL8211F-CG PHY,
which features a 25MHz crystal, and using PH8 pin as PHY reset.

Signed-off-by: Yixun Lan <dlan@gentoo.org>
---
I don't own this board, only compose this patch according to the
schematics. Let me know if it works.
---
 arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts b/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
index 85a546aecdbe149d6bad10327fca1fb7dafff6ad..23ab89c742c679fb274babbb0205f119eb2c9baa 100644
--- a/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
@@ -64,6 +64,23 @@ &ehci1 {
 	status = "okay";
 };
 
+&emac0 {
+	phy-mode = "rgmii";
+	phy-handle = <&ext_rgmii_phy>;
+
+	allwinner,tx-delay-ps = <100>;
+	allwinner,rx-delay-ps = <300>;
+
+	status = "okay";
+};
+
+&mdio0 {
+	ext_rgmii_phy: ethernet-phy@1 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <1>;
+	};
+};
+
 &mmc0 {
 	vmmc-supply = <&reg_cldo3>;
 	cd-gpios = <&pio 5 6 (GPIO_ACTIVE_LOW | GPIO_PULL_DOWN)>; /* PF6 */

-- 
2.49.0

Re: [PATCH 5/5] arm64: dts: allwinner: t527: add EMAC0 to Avaoto-A1 board

[Andrew Lunn]

> +&emac0 {
> +	phy-mode = "rgmii";
> +	phy-handle = <&ext_rgmii_phy>;
> +
> +	allwinner,tx-delay-ps = <100>;
> +	allwinner,rx-delay-ps = <300>;

Same issue here.

     Andrew

Re: [PATCH 5/5] arm64: dts: allwinner: t527: add EMAC0 to Avaoto-A1 board

[Andre Przywara]

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

Hi,

> On Avaoto A1 board, the EMAC0 connect to an external RTL8211F-CG PHY,

The name would be "Avaota" A1 board.

> which features a 25MHz crystal, and using PH8 pin as PHY reset.
> 
> Signed-off-by: Yixun Lan <dlan@gentoo.org>
> ---
> I don't own this board, only compose this patch according to the
> schematics. Let me know if it works.
> ---
>  arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts b/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
> index 85a546aecdbe149d6bad10327fca1fb7dafff6ad..23ab89c742c679fb274babbb0205f119eb2c9baa 100644
> --- a/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
> @@ -64,6 +64,23 @@ &ehci1 {
>  	status = "okay";
>  };

As for the Radxa board, we need an alias for ethernet0.

>  
> +&emac0 {
> +	phy-mode = "rgmii";

As Andrew mentioned, this should probably be "rgmii-id".

> +	phy-handle = <&ext_rgmii_phy>;

Can you please add the phy-supply here, it's reg_dcdc4.

Cheers,
Andre

> +
> +	allwinner,tx-delay-ps = <100>;
> +	allwinner,rx-delay-ps = <300>;
> +
> +	status = "okay";
> +};
> +
> +&mdio0 {
> +	ext_rgmii_phy: ethernet-phy@1 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <1>;
> +	};
> +};
> +
>  &mmc0 {
>  	vmmc-supply = <&reg_cldo3>;
>  	cd-gpios = <&pio 5 6 (GPIO_ACTIVE_LOW | GPIO_PULL_DOWN)>; /* PF6 */
> 

Re: [PATCH 5/5] arm64: dts: allwinner: t527: add EMAC0 to Avaoto-A1 board

[Yixun Lan]

hi Andre,

On 02:17 Thu 24 Apr     , Andre Przywara wrote:
> On Wed, 23 Apr 2025 22:03:26 +0800
> Yixun Lan <dlan@gentoo.org> wrote:
> 
> Hi,
> 
> > On Avaoto A1 board, the EMAC0 connect to an external RTL8211F-CG PHY,
> 
> The name would be "Avaota" A1 board.
> 
> > which features a 25MHz crystal, and using PH8 pin as PHY reset.
> > 
> > Signed-off-by: Yixun Lan <dlan@gentoo.org>
> > ---
> > I don't own this board, only compose this patch according to the
> > schematics. Let me know if it works.
> > ---
> >  arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts b/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
> > index 85a546aecdbe149d6bad10327fca1fb7dafff6ad..23ab89c742c679fb274babbb0205f119eb2c9baa 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun55i-t527-avaota-a1.dts
> > @@ -64,6 +64,23 @@ &ehci1 {
> >  	status = "okay";
> >  };
> 
> As for the Radxa board, we need an alias for ethernet0.
> 
> >  
> > +&emac0 {
> > +	phy-mode = "rgmii";
> 
> As Andrew mentioned, this should probably be "rgmii-id".
> 
> > +	phy-handle = <&ext_rgmii_phy>;
> 
> Can you please add the phy-supply here, it's reg_dcdc4.
> 
> Cheers,
> Andre
> 
> > +
> > +	allwinner,tx-delay-ps = <100>;
> > +	allwinner,rx-delay-ps = <300>;
> > +
> > +	status = "okay";
> > +};
> > +
> > +&mdio0 {
> > +	ext_rgmii_phy: ethernet-phy@1 {
> > +		compatible = "ethernet-phy-ieee802.3-c22";
> > +		reg = <1>;
> > +	};
> > +};
> > +
> >  &mmc0 {
> >  	vmmc-supply = <&reg_cldo3>;
> >  	cd-gpios = <&pio 5 6 (GPIO_ACTIVE_LOW | GPIO_PULL_DOWN)>; /* PF6 */
> > 
> 

all above comments make sense, will address in next version
-- 
Yixun Lan (dlan)