| Message ID | 20190503115928.27662-1-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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 = <®_aldo2>; /* VCC3V3-MAC: GMAC-3V */ > + }; > +}; > + > +&emac { > + pinctrl-names = "default"; > + pinctrl-0 = <&ext_rgmii_pins>; > + phy-mode = "rgmii"; > + phy-handle = <&ext_rgmii_phy>; > + phy-supply = <®_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
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 = <®_aldo2>; /* VCC3V3-MAC: GMAC-3V */ > > + }; > > +}; > > + > > +&emac { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&ext_rgmii_pins>; > > + phy-mode = "rgmii"; > > + phy-handle = <&ext_rgmii_phy>; > > + phy-supply = <®_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?
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 = <®_aldo2>; /* VCC3V3-MAC: GMAC-3V */ > > > + }; > > > +}; > > > + > > > +&emac { > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&ext_rgmii_pins>; > > > + phy-mode = "rgmii"; > > > + phy-handle = <&ext_rgmii_phy>; > > > + phy-supply = <®_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.
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
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 = <®_aldo2>; /* VCC3V3-MAC: GMAC-3V */ > > > > + }; > > > > +}; > > > > + > > > > +&emac { > > > > + pinctrl-names = "default"; > > > > + pinctrl-0 = <&ext_rgmii_pins>; > > > > + phy-mode = "rgmii"; > > > > + phy-handle = <&ext_rgmii_phy>; > > > > + phy-supply = <®_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
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 = <®_aldo2>; /* VCC3V3-MAC: GMAC-3V */ > > > > + }; > > > > +}; > > > > + > > > > +&emac { > > > > + pinctrl-names = "default"; > > > > + pinctrl-0 = <&ext_rgmii_pins>; > > > > + phy-mode = "rgmii"; > > > > + phy-handle = <&ext_rgmii_phy>; > > > > + phy-supply = <®_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.
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.
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 = <®_aldo2>; /* VCC3V3-MAC: GMAC-3V */ + }; +}; + +&emac { + pinctrl-names = "default"; + pinctrl-0 = <&ext_rgmii_pins>; + phy-mode = "rgmii"; + phy-handle = <&ext_rgmii_phy>; + phy-supply = <®_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 {
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(+)