LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
@ 2019-05-03 11:59 Jagan Teki
  2019-05-03 14:46 ` Maxime Ripard
  0 siblings, 1 reply; 8+ messages in thread
From: Jagan Teki @ 2019-05-03 11:59 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Mark Rutland
  Cc: Michael Trimarchi, devicetree, linux-arm-kernel, linux-kernel,
	linux-amarula, linux-sunxi, Jagan Teki

Add Ethernet support for orangepi-one-plus board,

- Ethernet port connected via RTL8211E PHY
- PHY suppiled with
  GMAC-2V5, fixed regulator with GMAC_EN pin via PD6
  GMAC-3V, which is supplied by VCC3V3-MAC via aldo2
- RGMII-RESET pin connected via PD14

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- emac changes on top of https://patchwork.kernel.org/cover/10899529/
  series

 .../allwinner/sun50i-h6-orangepi-one-plus.dts |  8 ++++
 .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 42 +++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
index 12e17567ab56..9e8ed1053715 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
@@ -9,4 +9,12 @@
 / {
 	model = "OrangePi One Plus";
 	compatible = "xunlong,orangepi-one-plus", "allwinner,sun50i-h6";
+
+	aliases {
+		ethernet0 = &emac;
+	};
+};
+
+&emac {
+	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
index 62e27948a3fa..c48e24acaf8a 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
@@ -45,6 +45,48 @@
 		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-3V (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>; /* GMAC_EN: 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>; /* VCC3V3-MAC: GMAC-3V */
+	};
+};
+
+&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>;
+};
+
+&mdio {
+	ext_rgmii_phy: ethernet-phy@1 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <1>;
+
+		reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* RGMII-RESET: PD14 */
+		reset-assert-us = <15000>;
+		reset-deassert-us = <40000>;
+	};
 };
 
 &ehci0 {
-- 
2.18.0.321.gffc6fa0e3


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
  2019-05-03 11:59 [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support Jagan Teki
@ 2019-05-03 14:46 ` Maxime Ripard
  2019-05-06  9:33   ` Jagan Teki
  0 siblings, 1 reply; 8+ messages in thread
From: Maxime Ripard @ 2019-05-03 14:46 UTC (permalink / raw)
  To: Jagan Teki
  Cc: Chen-Yu Tsai, Rob Herring, Mark Rutland, Michael Trimarchi,
	devicetree, linux-arm-kernel, linux-kernel, linux-amarula,
	linux-sunxi

[-- Attachment #1: Type: text/plain, Size: 3224 bytes --]

On Fri, May 03, 2019 at 05:29:28PM +0530, Jagan Teki wrote:
> Add Ethernet support for orangepi-one-plus board,
>
> - Ethernet port connected via RTL8211E PHY
> - PHY suppiled with
>   GMAC-2V5, fixed regulator with GMAC_EN pin via PD6
>   GMAC-3V, which is supplied by VCC3V3-MAC via aldo2
> - RGMII-RESET pin connected via PD14
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

Your commit log should be improved. We can get those informations from
the patch itself...

> ---
> Changes for v2:
> - emac changes on top of https://patchwork.kernel.org/cover/10899529/
>   series
>
>  .../allwinner/sun50i-h6-orangepi-one-plus.dts |  8 ++++
>  .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 42 +++++++++++++++++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> index 12e17567ab56..9e8ed1053715 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> @@ -9,4 +9,12 @@
>  / {
>  	model = "OrangePi One Plus";
>  	compatible = "xunlong,orangepi-one-plus", "allwinner,sun50i-h6";
> +
> +	aliases {
> +		ethernet0 = &emac;
> +	};
> +};
> +
> +&emac {
> +	status = "okay";
>  };
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> index 62e27948a3fa..c48e24acaf8a 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> @@ -45,6 +45,48 @@
>  		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-3V (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>; /* GMAC_EN: 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>; /* VCC3V3-MAC: GMAC-3V */
> +	};
> +};
> +
> +&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>;
> +};
> +
> +&mdio {
> +	ext_rgmii_phy: ethernet-phy@1 {
> +		compatible = "ethernet-phy-ieee802.3-c22";
> +		reg = <1>;
> +
> +		reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* RGMII-RESET: PD14 */
> +		reset-assert-us = <15000>;
> +		reset-deassert-us = <40000>;
> +	};
>  };

... however, at no point in time you explain why you made that switch,
and while most of the definition of the EMAC nodes is in the DTSI, you
only enable it in one DTS.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
  2019-05-03 14:46 ` Maxime Ripard
@ 2019-05-06  9:33   ` Jagan Teki
  2019-05-06 13:31     ` [linux-sunxi] " Ondřej Jirman
  2019-05-07  7:06     ` Maxime Ripard
  0 siblings, 2 replies; 8+ messages in thread
From: Jagan Teki @ 2019-05-06  9:33 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Chen-Yu Tsai, Rob Herring, Mark Rutland, Michael Trimarchi,
	devicetree, linux-arm-kernel, linux-kernel, linux-amarula,
	linux-sunxi

On Fri, May 3, 2019 at 8:16 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Fri, May 03, 2019 at 05:29:28PM +0530, Jagan Teki wrote:
> > Add Ethernet support for orangepi-one-plus board,
> >
> > - Ethernet port connected via RTL8211E PHY
> > - PHY suppiled with
> >   GMAC-2V5, fixed regulator with GMAC_EN pin via PD6
> >   GMAC-3V, which is supplied by VCC3V3-MAC via aldo2
> > - RGMII-RESET pin connected via PD14
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>
> Your commit log should be improved. We can get those informations from
> the patch itself...

Thought it was a clear commit log :)  will update anyway.

>
> > ---
> > Changes for v2:
> > - emac changes on top of https://patchwork.kernel.org/cover/10899529/
> >   series
> >
> >  .../allwinner/sun50i-h6-orangepi-one-plus.dts |  8 ++++
> >  .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 42 +++++++++++++++++++
> >  2 files changed, 50 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > index 12e17567ab56..9e8ed1053715 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > @@ -9,4 +9,12 @@
> >  / {
> >       model = "OrangePi One Plus";
> >       compatible = "xunlong,orangepi-one-plus", "allwinner,sun50i-h6";
> > +
> > +     aliases {
> > +             ethernet0 = &emac;
> > +     };
> > +};
> > +
> > +&emac {
> > +     status = "okay";
> >  };
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > index 62e27948a3fa..c48e24acaf8a 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > @@ -45,6 +45,48 @@
> >               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-3V (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>; /* GMAC_EN: 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>; /* VCC3V3-MAC: GMAC-3V */
> > +     };
> > +};
> > +
> > +&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>;
> > +};
> > +
> > +&mdio {
> > +     ext_rgmii_phy: ethernet-phy@1 {
> > +             compatible = "ethernet-phy-ieee802.3-c22";
> > +             reg = <1>;
> > +
> > +             reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* RGMII-RESET: PD14 */
> > +             reset-assert-us = <15000>;
> > +             reset-deassert-us = <40000>;
> > +     };
> >  };
>
> ... however, at no point in time you explain why you made that switch,
> and while most of the definition of the EMAC nodes is in the DTSI, you
> only enable it in one DTS.

The dtsi is shared b/w 1+ and lite2 and 1+ has emac, so I enabled the
status directly on dts and keeping the relevant nodes on dtsi just
like SoC dtsi does. do I need to mention this in commit log?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [linux-sunxi] Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
  2019-05-06  9:33   ` Jagan Teki
@ 2019-05-06 13:31     ` Ondřej Jirman
  2019-05-07  7:06       ` Maxime Ripard
  2019-05-07 12:57       ` Jagan Teki
  2019-05-07  7:06     ` Maxime Ripard
  1 sibling, 2 replies; 8+ messages in thread
From: Ondřej Jirman @ 2019-05-06 13:31 UTC (permalink / raw)
  To: Jagan Teki
  Cc: Maxime Ripard, Chen-Yu Tsai, Rob Herring, Mark Rutland,
	Michael Trimarchi, devicetree, linux-arm-kernel, linux-kernel,
	linux-amarula, linux-sunxi

On Mon, May 06, 2019 at 03:03:15PM +0530, Jagan Teki wrote:
> On Fri, May 3, 2019 at 8:16 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Fri, May 03, 2019 at 05:29:28PM +0530, Jagan Teki wrote:
> > > Add Ethernet support for orangepi-one-plus board,
> > >
> > > - Ethernet port connected via RTL8211E PHY
> > > - PHY suppiled with
> > >   GMAC-2V5, fixed regulator with GMAC_EN pin via PD6
> > >   GMAC-3V, which is supplied by VCC3V3-MAC via aldo2
> > > - RGMII-RESET pin connected via PD14
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > Your commit log should be improved. We can get those informations from
> > the patch itself...
> 
> Thought it was a clear commit log :)  will update anyway.
> 
> >
> > > ---
> > > Changes for v2:
> > > - emac changes on top of https://patchwork.kernel.org/cover/10899529/
> > >   series
> > >
> > >  .../allwinner/sun50i-h6-orangepi-one-plus.dts |  8 ++++
> > >  .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 42 +++++++++++++++++++
> > >  2 files changed, 50 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > index 12e17567ab56..9e8ed1053715 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > @@ -9,4 +9,12 @@
> > >  / {
> > >       model = "OrangePi One Plus";
> > >       compatible = "xunlong,orangepi-one-plus", "allwinner,sun50i-h6";
> > > +
> > > +     aliases {
> > > +             ethernet0 = &emac;
> > > +     };
> > > +};
> > > +
> > > +&emac {
> > > +     status = "okay";
> > >  };
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > index 62e27948a3fa..c48e24acaf8a 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > @@ -45,6 +45,48 @@
> > >               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-3V (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>; /* GMAC_EN: 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>; /* VCC3V3-MAC: GMAC-3V */
> > > +     };
> > > +};
> > > +
> > > +&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>;
> > > +};
> > > +
> > > +&mdio {
> > > +     ext_rgmii_phy: ethernet-phy@1 {
> > > +             compatible = "ethernet-phy-ieee802.3-c22";
> > > +             reg = <1>;
> > > +
> > > +             reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* RGMII-RESET: PD14 */
> > > +             reset-assert-us = <15000>;
> > > +             reset-deassert-us = <40000>;
> > > +     };
> > >  };
> >
> > ... however, at no point in time you explain why you made that switch,
> > and while most of the definition of the EMAC nodes is in the DTSI, you
> > only enable it in one DTS.
> 
> The dtsi is shared b/w 1+ and lite2 and 1+ has emac, so I enabled the
> status directly on dts and keeping the relevant nodes on dtsi just
> like SoC dtsi does. do I need to mention this in commit log?

Lite 2 doesn't have reg_gmac_2v5 and it also doesn't have the external phy.
But with this patch, reg_gmac_2v5 will also show up in the Lite 2's final
DTB.

Comapred to SoC dtsi, the SoC always has things that are in the dtsi, they
are just not enabled/used by the board, but they are present on the chip.

So this comes down to what the meaning of board-level dtsi should be. I
doubt we want it to mean "a collection of stuff that may or may not be
present on the boards that depend on it".

regards,
	o.

> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
  2019-05-06  9:33   ` Jagan Teki
  2019-05-06 13:31     ` [linux-sunxi] " Ondřej Jirman
@ 2019-05-07  7:06     ` Maxime Ripard
  2019-05-07 12:58       ` Jagan Teki
  1 sibling, 1 reply; 8+ messages in thread
From: Maxime Ripard @ 2019-05-07  7:06 UTC (permalink / raw)
  To: Jagan Teki
  Cc: Chen-Yu Tsai, Rob Herring, Mark Rutland, Michael Trimarchi,
	devicetree, linux-arm-kernel, linux-kernel, linux-amarula,
	linux-sunxi

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]

On Mon, May 06, 2019 at 03:03:15PM +0530, Jagan Teki wrote:
> On Fri, May 3, 2019 at 8:16 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> >
> > On Fri, May 03, 2019 at 05:29:28PM +0530, Jagan Teki wrote:
> > > Add Ethernet support for orangepi-one-plus board,
> > >
> > > - Ethernet port connected via RTL8211E PHY
> > > - PHY suppiled with
> > >   GMAC-2V5, fixed regulator with GMAC_EN pin via PD6
> > >   GMAC-3V, which is supplied by VCC3V3-MAC via aldo2
> > > - RGMII-RESET pin connected via PD14
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >
> > Your commit log should be improved. We can get those informations from
> > the patch itself...
>
> Thought it was a clear commit log :)  will update anyway.

Well, yes and no. The commit log is clear indeed, but it doesn't
provide what it's supposed to provide.

You shouldn't put *what* is being done by the patch. That's pretty
easy to figure out by reading the patch itself. You have to explain
why and how you did it, which is lacking in that case.

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [linux-sunxi] Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
  2019-05-06 13:31     ` [linux-sunxi] " Ondřej Jirman
@ 2019-05-07  7:06       ` Maxime Ripard
  2019-05-07 12:57       ` Jagan Teki
  1 sibling, 0 replies; 8+ messages in thread
From: Maxime Ripard @ 2019-05-07  7:06 UTC (permalink / raw)
  To: Jagan Teki, Chen-Yu Tsai, Rob Herring, Mark Rutland,
	Michael Trimarchi, devicetree, linux-arm-kernel, linux-kernel,
	linux-amarula, linux-sunxi

[-- Attachment #1: Type: text/plain, Size: 4439 bytes --]

On Mon, May 06, 2019 at 03:31:01PM +0200, Ondřej Jirman wrote:
> > > > ---
> > > > Changes for v2:
> > > > - emac changes on top of https://patchwork.kernel.org/cover/10899529/
> > > >   series
> > > >
> > > >  .../allwinner/sun50i-h6-orangepi-one-plus.dts |  8 ++++
> > > >  .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 42 +++++++++++++++++++
> > > >  2 files changed, 50 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > > index 12e17567ab56..9e8ed1053715 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > > @@ -9,4 +9,12 @@
> > > >  / {
> > > >       model = "OrangePi One Plus";
> > > >       compatible = "xunlong,orangepi-one-plus", "allwinner,sun50i-h6";
> > > > +
> > > > +     aliases {
> > > > +             ethernet0 = &emac;
> > > > +     };
> > > > +};
> > > > +
> > > > +&emac {
> > > > +     status = "okay";
> > > >  };
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > > index 62e27948a3fa..c48e24acaf8a 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > > @@ -45,6 +45,48 @@
> > > >               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-3V (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>; /* GMAC_EN: 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>; /* VCC3V3-MAC: GMAC-3V */
> > > > +     };
> > > > +};
> > > > +
> > > > +&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>;
> > > > +};
> > > > +
> > > > +&mdio {
> > > > +     ext_rgmii_phy: ethernet-phy@1 {
> > > > +             compatible = "ethernet-phy-ieee802.3-c22";
> > > > +             reg = <1>;
> > > > +
> > > > +             reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* RGMII-RESET: PD14 */
> > > > +             reset-assert-us = <15000>;
> > > > +             reset-deassert-us = <40000>;
> > > > +     };
> > > >  };
> > >
> > > ... however, at no point in time you explain why you made that switch,
> > > and while most of the definition of the EMAC nodes is in the DTSI, you
> > > only enable it in one DTS.
> >
> > The dtsi is shared b/w 1+ and lite2 and 1+ has emac, so I enabled the
> > status directly on dts and keeping the relevant nodes on dtsi just
> > like SoC dtsi does. do I need to mention this in commit log?
>
> Lite 2 doesn't have reg_gmac_2v5 and it also doesn't have the external phy.
> But with this patch, reg_gmac_2v5 will also show up in the Lite 2's final
> DTB.
>
> Comapred to SoC dtsi, the SoC always has things that are in the dtsi, they
> are just not enabled/used by the board, but they are present on the chip.
>
> So this comes down to what the meaning of board-level dtsi should be. I
> doubt we want it to mean "a collection of stuff that may or may not be
> present on the boards that depend on it".

Agreed.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [linux-sunxi] Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
  2019-05-06 13:31     ` [linux-sunxi] " Ondřej Jirman
  2019-05-07  7:06       ` Maxime Ripard
@ 2019-05-07 12:57       ` Jagan Teki
  1 sibling, 0 replies; 8+ messages in thread
From: Jagan Teki @ 2019-05-07 12:57 UTC (permalink / raw)
  To: Jagan Teki, Maxime Ripard, Chen-Yu Tsai, Rob Herring,
	Mark Rutland, Michael Trimarchi, devicetree, linux-arm-kernel,
	linux-kernel, linux-amarula, linux-sunxi

On Mon, May 6, 2019 at 7:01 PM Ondřej Jirman <megous@megous.com> wrote:
>
> On Mon, May 06, 2019 at 03:03:15PM +0530, Jagan Teki wrote:
> > On Fri, May 3, 2019 at 8:16 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Fri, May 03, 2019 at 05:29:28PM +0530, Jagan Teki wrote:
> > > > Add Ethernet support for orangepi-one-plus board,
> > > >
> > > > - Ethernet port connected via RTL8211E PHY
> > > > - PHY suppiled with
> > > >   GMAC-2V5, fixed regulator with GMAC_EN pin via PD6
> > > >   GMAC-3V, which is supplied by VCC3V3-MAC via aldo2
> > > > - RGMII-RESET pin connected via PD14
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > Your commit log should be improved. We can get those informations from
> > > the patch itself...
> >
> > Thought it was a clear commit log :)  will update anyway.
> >
> > >
> > > > ---
> > > > Changes for v2:
> > > > - emac changes on top of https://patchwork.kernel.org/cover/10899529/
> > > >   series
> > > >
> > > >  .../allwinner/sun50i-h6-orangepi-one-plus.dts |  8 ++++
> > > >  .../dts/allwinner/sun50i-h6-orangepi.dtsi     | 42 +++++++++++++++++++
> > > >  2 files changed, 50 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > > index 12e17567ab56..9e8ed1053715 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi-one-plus.dts
> > > > @@ -9,4 +9,12 @@
> > > >  / {
> > > >       model = "OrangePi One Plus";
> > > >       compatible = "xunlong,orangepi-one-plus", "allwinner,sun50i-h6";
> > > > +
> > > > +     aliases {
> > > > +             ethernet0 = &emac;
> > > > +     };
> > > > +};
> > > > +
> > > > +&emac {
> > > > +     status = "okay";
> > > >  };
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > > index 62e27948a3fa..c48e24acaf8a 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-orangepi.dtsi
> > > > @@ -45,6 +45,48 @@
> > > >               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-3V (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>; /* GMAC_EN: 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>; /* VCC3V3-MAC: GMAC-3V */
> > > > +     };
> > > > +};
> > > > +
> > > > +&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>;
> > > > +};
> > > > +
> > > > +&mdio {
> > > > +     ext_rgmii_phy: ethernet-phy@1 {
> > > > +             compatible = "ethernet-phy-ieee802.3-c22";
> > > > +             reg = <1>;
> > > > +
> > > > +             reset-gpios = <&pio 3 14 GPIO_ACTIVE_LOW>; /* RGMII-RESET: PD14 */
> > > > +             reset-assert-us = <15000>;
> > > > +             reset-deassert-us = <40000>;
> > > > +     };
> > > >  };
> > >
> > > ... however, at no point in time you explain why you made that switch,
> > > and while most of the definition of the EMAC nodes is in the DTSI, you
> > > only enable it in one DTS.
> >
> > The dtsi is shared b/w 1+ and lite2 and 1+ has emac, so I enabled the
> > status directly on dts and keeping the relevant nodes on dtsi just
> > like SoC dtsi does. do I need to mention this in commit log?
>
> Lite 2 doesn't have reg_gmac_2v5 and it also doesn't have the external phy.
> But with this patch, reg_gmac_2v5 will also show up in the Lite 2's final
> DTB.
>
> Comapred to SoC dtsi, the SoC always has things that are in the dtsi, they
> are just not enabled/used by the board, but they are present on the chip.
>
> So this comes down to what the meaning of board-level dtsi should be. I
> doubt we want it to mean "a collection of stuff that may or may not be
> present on the boards that depend on it".

Was thinking in another direction, thanks for the info. agreed.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support
  2019-05-07  7:06     ` Maxime Ripard
@ 2019-05-07 12:58       ` Jagan Teki
  0 siblings, 0 replies; 8+ messages in thread
From: Jagan Teki @ 2019-05-07 12:58 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Chen-Yu Tsai, Rob Herring, Mark Rutland, Michael Trimarchi,
	devicetree, linux-arm-kernel, linux-kernel, linux-amarula,
	linux-sunxi

On Tue, May 7, 2019 at 12:36 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Mon, May 06, 2019 at 03:03:15PM +0530, Jagan Teki wrote:
> > On Fri, May 3, 2019 at 8:16 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > >
> > > On Fri, May 03, 2019 at 05:29:28PM +0530, Jagan Teki wrote:
> > > > Add Ethernet support for orangepi-one-plus board,
> > > >
> > > > - Ethernet port connected via RTL8211E PHY
> > > > - PHY suppiled with
> > > >   GMAC-2V5, fixed regulator with GMAC_EN pin via PD6
> > > >   GMAC-3V, which is supplied by VCC3V3-MAC via aldo2
> > > > - RGMII-RESET pin connected via PD14
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > >
> > > Your commit log should be improved. We can get those informations from
> > > the patch itself...
> >
> > Thought it was a clear commit log :)  will update anyway.
>
> Well, yes and no. The commit log is clear indeed, but it doesn't
> provide what it's supposed to provide.
>
> You shouldn't put *what* is being done by the patch. That's pretty
> easy to figure out by reading the patch itself. You have to explain
> why and how you did it, which is lacking in that case.

Make sense, will rework.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-05-07 12:58 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-03 11:59 [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support Jagan Teki
2019-05-03 14:46 ` Maxime Ripard
2019-05-06  9:33   ` Jagan Teki
2019-05-06 13:31     ` [linux-sunxi] " Ondřej Jirman
2019-05-07  7:06       ` Maxime Ripard
2019-05-07 12:57       ` Jagan Teki
2019-05-07  7:06     ` Maxime Ripard
2019-05-07 12:58       ` Jagan Teki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).