LinuxLists.cc - [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

2019-05-03 12:11:18

Subject: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

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 <[email protected]>
---
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

2019-05-03 14:48:36

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

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 <[email protected]>

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

Attachments:

(No filename) (3.25 kB)
signature.asc (235.00 B)
Download all attachments

2019-05-06 09:34:36

by Jagan Teki

[permalink] [raw]

Subject: Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

On Fri, May 3, 2019 at 8:16 PM Maxime Ripard <[email protected]> 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 <[email protected]>
>
> 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?

2019-05-06 13:32:20

by Ondřej Jirman

[permalink] [raw]

Subject: Re: [linux-sunxi] Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

On Mon, May 06, 2019 at 03:03:15PM +0530, Jagan Teki wrote:
> On Fri, May 3, 2019 at 8:16 PM Maxime Ripard <[email protected]> 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 <[email protected]>
> >
> > 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 [email protected].
> For more options, visit https://groups.google.com/d/optout.

2019-05-07 07:09:40

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

Attachments:

(No filename) (1.12 kB)
signature.asc (235.00 B)
Download all attachments

2019-05-07 07:09:48

by Maxime Ripard

[permalink] [raw]

Subject: Re: [linux-sunxi] Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

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

Attachments:

(No filename) (4.33 kB)
signature.asc (235.00 B)
Download all attachments

2019-05-07 13:00:22

by Jagan Teki

[permalink] [raw]

Subject: Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

On Tue, May 7, 2019 at 12:36 PM Maxime Ripard <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >
> > > 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.

2019-05-07 13:00:28

by Jagan Teki

[permalink] [raw]

Subject: Re: [linux-sunxi] Re: [PATCH v2] arm64: allwinner: h6: orangepi-one-plus: Add Ethernet support

On Mon, May 6, 2019 at 7:01 PM Ondřej Jirman <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > >
> > > 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.