[07/12] arm64: dts: allwinner: orange-pi-3: Enable ethernet
diff mbox series

Message ID 20190405234514.6183-8-megous@megous.com
State Not Applicable
Delegated to: Johannes Berg
Headers show
Series
  • Add support for Orange Pi 3
Related show

Commit Message

Ondřej Jirman April 5, 2019, 11:45 p.m. UTC
From: Ondrej Jirman <megous@megous.com>

Orange Pi 3 has two regulators that power the Realtek RTL8211E.
According to the phy datasheet, both regulators need to be enabled
at the same time, but we can only specify a single phy-supply in
the DT.

This can be achieved by making one regulator depedning on the
other via vin-supply. While it's not a technically correct
description of the hardware, it achieves the purpose.

All values of RX/TX delay were tested exhaustively and a middle
one of the working values was chosen.

Signed-off-by: Ondrej Jirman <megous@megous.com>
---
 .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Jagan Teki April 8, 2019, 6:11 a.m. UTC | #1
On Sat, Apr 6, 2019 at 5:15 AM <megous@megous.com> wrote:
>
> From: Ondrej Jirman <megous@megous.com>
>
> Orange Pi 3 has two regulators that power the Realtek RTL8211E.
> According to the phy datasheet, both regulators need to be enabled
> at the same time, but we can only specify a single phy-supply in
> the DT.
>
> This can be achieved by making one regulator depedning on the
> other via vin-supply. While it's not a technically correct
> description of the hardware, it achieves the purpose.
>
> All values of RX/TX delay were tested exhaustively and a middle
> one of the working values was chosen.
>
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
>  .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)

This was in ML[1], I thought this change would already merged. I
remember Chen-Yu applied and revert due to emac node not mainlined at
that time.

[1] https://patchwork.kernel.org/patch/10558281/
Maxime Ripard April 8, 2019, 7:40 a.m. UTC | #2
On Sat, Apr 06, 2019 at 01:45:09AM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> Orange Pi 3 has two regulators that power the Realtek RTL8211E.
> According to the phy datasheet, both regulators need to be enabled
> at the same time, but we can only specify a single phy-supply in
> the DT.
>
> This can be achieved by making one regulator depedning on the
> other via vin-supply. While it's not a technically correct
> description of the hardware, it achieves the purpose.
>
> All values of RX/TX delay were tested exhaustively and a middle
> one of the working values was chosen.
>
> Signed-off-by: Ondrej Jirman <megous@megous.com>
> ---
>  .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> index 644946749088..5270142527f5 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> @@ -15,6 +15,7 @@
>
>  	aliases {
>  		serial0 = &uart0;
> +		ethernet0 = &emac;
>  	};
>
>  	chosen {
> @@ -64,6 +65,27 @@
>  		regulator-max-microvolt = <5000000>;
>  		regulator-always-on;
>  	};
> +
> +	/*
> +	 * The board uses 2.5V RGMII signalling. Power sequence
> +	 * to enable the phy is to enable GMAC-2V5 and GMAC-3V3 (aldo2)
> +	 * power rails at the same time and to wait 100ms.
> +	 */
> +	reg_gmac_2v5: gmac-2v5 {
> +                compatible = "regulator-fixed";
> +                regulator-name = "gmac-2v5";
> +                regulator-min-microvolt = <2500000>;
> +                regulator-max-microvolt = <2500000>;
> +                startup-delay-us = <100000>;
> +                enable-active-high;
> +                gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */

Is enable-active-high still needed? It's redundant with the
GPIO_ACTIVE_HIGH flag.

The indentation is wrong here as well.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Ondřej Jirman April 8, 2019, 10:26 p.m. UTC | #3
On Mon, Apr 08, 2019 at 11:41:38AM +0530, Jagan Teki wrote:
> On Sat, Apr 6, 2019 at 5:15 AM <megous@megous.com> wrote:
> >
> > From: Ondrej Jirman <megous@megous.com>
> >
> > Orange Pi 3 has two regulators that power the Realtek RTL8211E.
> > According to the phy datasheet, both regulators need to be enabled
> > at the same time, but we can only specify a single phy-supply in
> > the DT.
> >
> > This can be achieved by making one regulator depedning on the
> > other via vin-supply. While it's not a technically correct
> > description of the hardware, it achieves the purpose.
> >
> > All values of RX/TX delay were tested exhaustively and a middle
> > one of the working values was chosen.
> >
> > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > ---
> >  .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> 
> This was in ML[1], I thought this change would already merged. I
> remember Chen-Yu applied and revert due to emac node not mainlined at
> that time.
> 
> [1] https://patchwork.kernel.org/patch/10558281/

The patch was not merged at the time, and bitrotted a bit. Armbian folks
were applying the bitortted patch out-of-the mainline and were experiencing
NPEs. I fixed the patch, and it is also part of this series. It's patch 5.

	o.
Ondřej Jirman April 8, 2019, 11:22 p.m. UTC | #4
On Mon, Apr 08, 2019 at 09:40:42AM +0200, Maxime Ripard wrote:
> On Sat, Apr 06, 2019 at 01:45:09AM +0200, megous@megous.com wrote:
> > From: Ondrej Jirman <megous@megous.com>
> >
> > Orange Pi 3 has two regulators that power the Realtek RTL8211E.
> > According to the phy datasheet, both regulators need to be enabled
> > at the same time, but we can only specify a single phy-supply in
> > the DT.
> >
> > This can be achieved by making one regulator depedning on the
> > other via vin-supply. While it's not a technically correct
> > description of the hardware, it achieves the purpose.
> >
> > All values of RX/TX delay were tested exhaustively and a middle
> > one of the working values was chosen.
> >
> > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > ---
> >  .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 44 +++++++++++++++++++
> >  1 file changed, 44 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> > index 644946749088..5270142527f5 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> > @@ -15,6 +15,7 @@
> >
> >  	aliases {
> >  		serial0 = &uart0;
> > +		ethernet0 = &emac;
> >  	};
> >
> >  	chosen {
> > @@ -64,6 +65,27 @@
> >  		regulator-max-microvolt = <5000000>;
> >  		regulator-always-on;
> >  	};
> > +
> > +	/*
> > +	 * The board uses 2.5V RGMII signalling. Power sequence
> > +	 * to enable the phy is to enable GMAC-2V5 and GMAC-3V3 (aldo2)
> > +	 * power rails at the same time and to wait 100ms.
> > +	 */
> > +	reg_gmac_2v5: gmac-2v5 {
> > +                compatible = "regulator-fixed";
> > +                regulator-name = "gmac-2v5";
> > +                regulator-min-microvolt = <2500000>;
> > +                regulator-max-microvolt = <2500000>;
> > +                startup-delay-us = <100000>;
> > +                enable-active-high;
> > +                gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> 
> Is enable-active-high still needed? It's redundant with the
> GPIO_ACTIVE_HIGH flag.

Looking at the code, use/non-use of enable-active-high inhibits
flags specified in gpio property. So the GPIO_ACTIVE_HIGH flag
is ignored here (had GPIO_ACTIVE_LOW been used, the kernel would
ignore it too and print a warning).

So enable-active-high is still necessary here.

See comment in gpiolib-of.c where this is handled:

/*
 * The regulator GPIO handles are specified such that the
 * presence or absence of "enable-active-high" solely controls
 * the polarity of the GPIO line. Any phandle flags must
 * be actively ignored.
 */

regards,
	o.

> The indentation is wrong here as well.
> 
> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Maxime Ripard April 9, 2019, 7:23 a.m. UTC | #5
On Tue, Apr 09, 2019 at 01:22:32AM +0200, Ondřej Jirman wrote:
> On Mon, Apr 08, 2019 at 09:40:42AM +0200, Maxime Ripard wrote:
> > On Sat, Apr 06, 2019 at 01:45:09AM +0200, megous@megous.com wrote:
> > > From: Ondrej Jirman <megous@megous.com>
> > >
> > > Orange Pi 3 has two regulators that power the Realtek RTL8211E.
> > > According to the phy datasheet, both regulators need to be enabled
> > > at the same time, but we can only specify a single phy-supply in
> > > the DT.
> > >
> > > This can be achieved by making one regulator depedning on the
> > > other via vin-supply. While it's not a technically correct
> > > description of the hardware, it achieves the purpose.
> > >
> > > All values of RX/TX delay were tested exhaustively and a middle
> > > one of the working values was chosen.
> > >
> > > Signed-off-by: Ondrej Jirman <megous@megous.com>
> > > ---
> > >  .../dts/allwinner/sun50i-h6-orangepi-3.dts    | 44 +++++++++++++++++++
> > >  1 file changed, 44 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> > > index 644946749088..5270142527f5 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
> > > @@ -15,6 +15,7 @@
> > >
> > >  	aliases {
> > >  		serial0 = &uart0;
> > > +		ethernet0 = &emac;
> > >  	};
> > >
> > >  	chosen {
> > > @@ -64,6 +65,27 @@
> > >  		regulator-max-microvolt = <5000000>;
> > >  		regulator-always-on;
> > >  	};
> > > +
> > > +	/*
> > > +	 * The board uses 2.5V RGMII signalling. Power sequence
> > > +	 * to enable the phy is to enable GMAC-2V5 and GMAC-3V3 (aldo2)
> > > +	 * power rails at the same time and to wait 100ms.
> > > +	 */
> > > +	reg_gmac_2v5: gmac-2v5 {
> > > +                compatible = "regulator-fixed";
> > > +                regulator-name = "gmac-2v5";
> > > +                regulator-min-microvolt = <2500000>;
> > > +                regulator-max-microvolt = <2500000>;
> > > +                startup-delay-us = <100000>;
> > > +                enable-active-high;
> > > +                gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> >
> > Is enable-active-high still needed? It's redundant with the
> > GPIO_ACTIVE_HIGH flag.
>
> Looking at the code, use/non-use of enable-active-high inhibits
> flags specified in gpio property. So the GPIO_ACTIVE_HIGH flag
> is ignored here (had GPIO_ACTIVE_LOW been used, the kernel would
> ignore it too and print a warning).
>
> So enable-active-high is still necessary here.

Too bad :/

> See comment in gpiolib-of.c where this is handled:
>
> /*
>  * The regulator GPIO handles are specified such that the
>  * presence or absence of "enable-active-high" solely controls
>  * the polarity of the GPIO line. Any phandle flags must
>  * be actively ignored.
>  */

Thanks for digging this out

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Linus Walleij April 9, 2019, 7:35 a.m. UTC | #6
On Tue, Apr 9, 2019 at 1:22 AM Ondřej Jirman <megous@megous.com> wrote:

> > > +                enable-active-high;
> > > +                gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
> >
> > Is enable-active-high still needed? It's redundant with the
> > GPIO_ACTIVE_HIGH flag.
>
> Looking at the code, use/non-use of enable-active-high inhibits
> flags specified in gpio property. So the GPIO_ACTIVE_HIGH flag
> is ignored here (had GPIO_ACTIVE_LOW been used, the kernel would
> ignore it too and print a warning).
>
> So enable-active-high is still necessary here.
>
> See comment in gpiolib-of.c where this is handled:
>
> /*
>  * The regulator GPIO handles are specified such that the
>  * presence or absence of "enable-active-high" solely controls
>  * the polarity of the GPIO line. Any phandle flags must
>  * be actively ignored.
>  */

Yeah this caused me special headache in the current merge
window because of buggy code on my part.

This is an effect of this flag being defined for powerpc
ages before we properly implemented generic GPIO
bindings. We just have to respect it.

See:
https://marc.info/?l=linux-gpio&m=155417774822532&w=2

Yours,
Linus Walleij

Patch
diff mbox series

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
index 644946749088..5270142527f5 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-3.dts
@@ -15,6 +15,7 @@ 
 
 	aliases {
 		serial0 = &uart0;
+		ethernet0 = &emac;
 	};
 
 	chosen {
@@ -64,6 +65,27 @@ 
 		regulator-max-microvolt = <5000000>;
 		regulator-always-on;
 	};
+
+	/*
+	 * The board uses 2.5V RGMII signalling. Power sequence
+	 * to enable the phy is to enable GMAC-2V5 and GMAC-3V3 (aldo2)
+	 * power rails at the same time and to wait 100ms.
+	 */
+	reg_gmac_2v5: gmac-2v5 {
+                compatible = "regulator-fixed";
+                regulator-name = "gmac-2v5";
+                regulator-min-microvolt = <2500000>;
+                regulator-max-microvolt = <2500000>;
+                startup-delay-us = <100000>;
+                enable-active-high;
+                gpio = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
+
+                /* The real parent of gmac-2v5 is reg_vcc5v, but we need
+                 * to enable two regulators to power the phy. This is one
+                 * way to achieve that.
+                 */
+                vin-supply = <&reg_aldo2>; /* GMAC-3V3 */
+        };
 };
 
 &cpu0 {
@@ -82,6 +104,17 @@ 
 	status = "okay";
 };
 
+&emac {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ext_rgmii_pins>;
+	phy-mode = "rgmii";
+	phy-handle = <&ext_rgmii_phy>;
+	phy-supply = <&reg_gmac_2v5>;
+	allwinner,rx-delay-ps = <1500>;
+	allwinner,tx-delay-ps = <700>;
+	status = "okay";
+};
+
 &hdmi {
 	ddc-supply = <&reg_ddc>;
 	status = "okay";
@@ -93,6 +126,17 @@ 
 	};
 };
 
+&mdio {
+	ext_rgmii_phy: ethernet-phy@1 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <1>;
+
+		reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* PD14 */
+		reset-assert-us = <15000>;
+		reset-deassert-us = <40000>;
+	};
+};
+
 &mmc0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&mmc0_pins>;