[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[kevans at FreeBSD.org]

The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
significant packet loss on Gigabit links. This patch sets the magical bits
in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
undocumented stuff.

[1] https://forum.pine64.org/showthread.php?tid=835&pid=19704#pid19704

Kyle Evans (2):
  net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys
  Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX

 configs/pine64_plus_defconfig |  2 ++
 drivers/net/phy/Kconfig       | 10 ++++++++++
 drivers/net/phy/realtek.c     | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)

-- 
2.15.1

[U-Boot] [PATCH 1/2] net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys

[kevans at FreeBSD.org]

Setting PHY_RTL8211E_PINE64_GIGABIT_FIX forces internal rx/tx delays off
on the PHY, as well as flipping some magical undocumented bits. The
magic number comes from the Pine64 engineering team, presumably as a
proxy from Realtek. This configuration fixes the throughput on some
Pine64 models. Packet loss of up to 60-70% has been observed without
this.

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
---
 drivers/net/phy/Kconfig   | 10 ++++++++++
 drivers/net/phy/realtek.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index e32f1eb1c0..ad648a889d 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -114,6 +114,16 @@ config PHY_NATSEMI
 config PHY_REALTEK
 	bool "Realtek Ethernet PHYs support"
 
+config RTL8211E_PINE64_GIGABIT_FIX
+	bool "Fix gigabit throughput on some Pine64+ models"
+	depends on PHY_REALTEK
+	help
+	  Configure the Realtek RTL8211E found on some Pine64+ models differently to
+	  fix throughput on Gigabit links, turning off all internal delays in the
+	  process. The settings that this touches are not documented in the CONFREG
+	  section of the RTL8211E datasheet, but come from Realtek by way of the
+	  Pine64 engineering team.
+
 config RTL8211X_PHY_FORCE_MASTER
 	bool "Ethernet PHY RTL8211x: force 1000BASE-T master mode"
 	depends on PHY_REALTEK
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 6d917f86f4..d5c2a46c67 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -13,6 +13,7 @@
 #include <phy.h>
 
 #define PHY_RTL8211x_FORCE_MASTER BIT(1)
+#define PHY_RTL8211E_PINE64_GIGABIT_FIX BIT(2)
 
 #define PHY_AUTONEGOTIATE_TIMEOUT 5000
 
@@ -47,6 +48,13 @@
 #define MIIM_RTL8211F_PHYSTAT_SPDDONE  0x0800
 #define MIIM_RTL8211F_PHYSTAT_LINK     0x0004
 
+#define MIIM_RTL8211E_CONFREG           0x1c
+#define MIIM_RTL8211E_CONFREG_TXD		0x0002
+#define MIIM_RTL8211E_CONFREG_RXD		0x0004
+#define MIIM_RTL8211E_CONFREG_MAGIC		0xb400	/* Undocumented */
+
+#define MIIM_RTL8211E_EXT_PAGE_SELECT  0x1e
+
 #define MIIM_RTL8211F_PAGE_SELECT      0x1f
 #define MIIM_RTL8211F_TX_DELAY		0x100
 #define MIIM_RTL8211F_LCR		0x10
@@ -60,6 +68,15 @@ static int rtl8211b_probe(struct phy_device *phydev)
 	return 0;
 }
 
+static int rtl8211e_probe(struct phy_device *phydev)
+{
+#ifdef CONFIG_RTL8211E_PINE64_GIGABIT_FIX
+	phydev->flags |= PHY_RTL8211E_PINE64_GIGABIT_FIX;
+#endif
+
+	return 0;
+}
+
 /* RealTek RTL8211x */
 static int rtl8211x_config(struct phy_device *phydev)
 {
@@ -81,6 +98,22 @@ static int rtl8211x_config(struct phy_device *phydev)
 		reg |= MIIM_RTL8211x_CTRL1000T_MASTER;
 		phy_write(phydev, MDIO_DEVAD_NONE, MII_CTRL1000, reg);
 	}
+	if (phydev->flags & PHY_RTL8211E_PINE64_GIGABIT_FIX) {
+		unsigned int reg;
+
+		phy_write(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211F_PAGE_SELECT,
+			  7);
+		phy_write(phydev, MDIO_DEVAD_NONE,
+			  MIIM_RTL8211E_EXT_PAGE_SELECT, 0xa4);
+		reg = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211E_CONFREG);
+		/* Ensure both internal delays are turned off */
+		reg &= ~(MIIM_RTL8211E_CONFREG_TXD | MIIM_RTL8211E_CONFREG_RXD);
+		/* Flip the magic undocumented bits */
+		reg |= MIIM_RTL8211E_CONFREG_MAGIC;
+		phy_write(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211E_CONFREG, reg);
+		phy_write(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211F_PAGE_SELECT,
+			  0);
+	}
 	/* read interrupt status just to clear it */
 	phy_read(phydev, MDIO_DEVAD_NONE, MIIM_RTL8211x_PHY_INER);
 
@@ -279,6 +312,7 @@ static struct phy_driver RTL8211E_driver = {
 	.uid = 0x1cc915,
 	.mask = 0xffffff,
 	.features = PHY_GBIT_FEATURES,
+	.probe = &rtl8211e_probe,
 	.config = &rtl8211x_config,
 	.startup = &rtl8211e_startup,
 	.shutdown = &genphy_shutdown,
-- 
2.15.1

[U-Boot] [PATCH 2/2] Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX

[kevans at FreeBSD.org]

The Pine64+ uses a generic PHY driver, so flip it over to using the Realtek PHY
driver to actually apply the RTL8211e fix.

Signed-off-by: Kyle Evans <kevans@FreeBSD.org>
---
 configs/pine64_plus_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/configs/pine64_plus_defconfig b/configs/pine64_plus_defconfig
index 01ed23844b..149311f2c4 100644
--- a/configs/pine64_plus_defconfig
+++ b/configs/pine64_plus_defconfig
@@ -14,3 +14,5 @@ CONFIG_OF_LIST="sun50i-a64-pine64 sun50i-a64-pine64-plus"
 CONFIG_SUN8I_EMAC=y
 CONFIG_USB_EHCI_HCD=y
 CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE=y
+CONFIG_PHY_REALTEK=y
+CONFIG_RTL8211E_PINE64_GIGABIT_FIX=y
-- 
2.15.1

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[André Przywara]

Hi,

thanks for picking this up!

(CC:ing Icenowy, who was engaged in a Linux fix for this issue last year
[2][3]. It's Chinese New Year though, so not sure how quickly she will
answer).

On 14/02/18 23:02, kevans at FreeBSD.org wrote:
> The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
> significant packet loss on Gigabit links.

Do you have a faulty board at hand? What is the actual effect in U-Boot?
IIRC the bug "just" causes a slower connection in Gigabit mode, I am not
sure we care so much in U-Boot? Or is it actually packages dropped,
which is much more annoying without TCP covering up for this?

> This patch sets the magical bits
> in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
> undocumented stuff.

So if I remember the discussion correctly, this workaround affects the
performance of the "good" boards. Have you checked this?
There was a discussion last year [2][3] about how to fix this in Linux,
which definitely involved some DT property (ideally in the PHY node).
This would allow people to turn this on and off depending on their
particular board.
I am not sure this discussion lead anywhere, though, it might be a good
idea to warm this up again.

Cheers,
Andre.

[2] https://marc.info/?l=devicetree&m=149281711105621
[3] https://marc.info/?l=linux-netdev&m=150337466923103

> [1] https://forum.pine64.org/showthread.php?tid=835&pid=19704#pid19704
> 
> Kyle Evans (2):
>   net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys
>   Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX
> 
>  configs/pine64_plus_defconfig |  2 ++
>  drivers/net/phy/Kconfig       | 10 ++++++++++
>  drivers/net/phy/realtek.c     | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
> 

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[Kyle Evans]

On Thu, Feb 15, 2018 at 3:32 PM, André Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> thanks for picking this up!
>
> (CC:ing Icenowy, who was engaged in a Linux fix for this issue last year
> [2][3]. It's Chinese New Year though, so not sure how quickly she will
> answer).
>
> On 14/02/18 23:02, kevans at FreeBSD.org wrote:
>> The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
>> significant packet loss on Gigabit links.
>
> Do you have a faulty board at hand? What is the actual effect in U-Boot?
> IIRC the bug "just" causes a slower connection in Gigabit mode, I am not
> sure we care so much in U-Boot? Or is it actually packages dropped,
> which is much more annoying without TCP covering up for this?

I do. =) My board in particular sees 60-70% packet loss without this,
making netboot incredibly unreliable.

>
>> This patch sets the magical bits
>> in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
>> undocumented stuff.
>
> So if I remember the discussion correctly, this workaround affects the
> performance of the "good" boards. Have you checked this?
> There was a discussion last year [2][3] about how to fix this in Linux,
> which definitely involved some DT property (ideally in the PHY node).
> This would allow people to turn this on and off depending on their
> particular board.
> I am not sure this discussion lead anywhere, though, it might be a good
> idea to warm this up again.
>

Unfortunately, I do not have a good board to test with, so I've no
idea what it ends up doing to performance. for them.

I think rgmii-txid is actually not right for this. As an aside, I've
pulled this value from [1]. The value they ended up going with (as
seen in this patch and in [1]) turns off both TXID and RXID (IIRC,
from the realtek documentation for this PHY), leaving us with just
plain ol' "rgmii" with no internal delay at all. [1] also applies it
unconditionally for pine64.

[1] https://github.com/longsleep/linux-pine64/commit/ffe3ca5be7682bbeb0fdadede29acd4a3c888015

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[Peter Robinson]

On Thu, Feb 15, 2018 at 9:32 PM, André Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> thanks for picking this up!
>
> (CC:ing Icenowy, who was engaged in a Linux fix for this issue last year
> [2][3]. It's Chinese New Year though, so not sure how quickly she will
> answer).
>
> On 14/02/18 23:02, kevans at FreeBSD.org wrote:
>> The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
>> significant packet loss on Gigabit links.
>
> Do you have a faulty board at hand? What is the actual effect in U-Boot?
> IIRC the bug "just" causes a slower connection in Gigabit mode, I am not
> sure we care so much in U-Boot? Or is it actually packages dropped,
> which is much more annoying without TCP covering up for this?
>
>> This patch sets the magical bits
>> in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
>> undocumented stuff.
>
> So if I remember the discussion correctly, this workaround affects the
> performance of the "good" boards. Have you checked this?
> There was a discussion last year [2][3] about how to fix this in Linux,
> which definitely involved some DT property (ideally in the PHY node).
> This would allow people to turn this on and off depending on their
> particular board.

So I have a "bad" board, the fix upstream took it from "basically not
working at all" to vaguely but not with great speed be able to
transfer data.

I've just tested this patch series on that board and was getting
around 7MBps on Linux too. So presumably what ever bits that are being
tweaked by this patch in u-boot aren't reset when the linux driver
loads and it improved performance on Linux for me too.

From a netboot (untested) I would think this would be useful too.

Either way for the series:
Tested-by: Peter Robinson <pbrobinson@gmail.com>

> [2] https://marc.info/?l=devicetree&m=149281711105621
> [3] https://marc.info/?l=linux-netdev&m=150337466923103
>
>> [1] https://forum.pine64.org/showthread.php?tid=835&pid=19704#pid19704
>>
>> Kyle Evans (2):
>>   net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys
>>   Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX
>>
>>  configs/pine64_plus_defconfig |  2 ++
>>  drivers/net/phy/Kconfig       | 10 ++++++++++
>>  drivers/net/phy/realtek.c     | 34 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 46 insertions(+)
>>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH 1/2] net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys

[Joe Hershberger]

On Wed, Feb 14, 2018 at 5:02 PM,  <kevans@freebsd.org> wrote:
> Setting PHY_RTL8211E_PINE64_GIGABIT_FIX forces internal rx/tx delays off
> on the PHY, as well as flipping some magical undocumented bits. The
> magic number comes from the Pine64 engineering team, presumably as a
> proxy from Realtek. This configuration fixes the throughput on some
> Pine64 models. Packet loss of up to 60-70% has been observed without
> this.
>
> Signed-off-by: Kyle Evans <kevans@FreeBSD.org>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] [PATCH 2/2] Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX

[Joe Hershberger]

On Wed, Feb 14, 2018 at 5:02 PM,  <kevans@freebsd.org> wrote:
> The Pine64+ uses a generic PHY driver, so flip it over to using the Realtek PHY
> driver to actually apply the RTL8211e fix.
>
> Signed-off-by: Kyle Evans <kevans@FreeBSD.org>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

[U-Boot] net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys

[Joe Hershberger]

Hi Kyle,

https://patchwork.ozlabs.org/patch/873752/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX

[Joe Hershberger]

Hi Kyle,

https://patchwork.ozlabs.org/patch/873751/ was applied to http://git.denx.de/?p=u-boot/u-boot-net.git

Thanks!
-Joe

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[Kyle Evans]

On Wed, Feb 14, 2018 at 5:02 PM,  <kevans@freebsd.org> wrote:
> The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
> significant packet loss on Gigabit links. This patch sets the magical bits
> in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
> undocumented stuff.
>
> [1] https://forum.pine64.org/showthread.php?tid=835&pid=19704#pid19704
>
> Kyle Evans (2):
>   net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys
>   Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX
>
>  configs/pine64_plus_defconfig |  2 ++
>  drivers/net/phy/Kconfig       | 10 ++++++++++
>  drivers/net/phy/realtek.c     | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
>

Hi,

I'd like to dredge this one up again, since it's been a couple months
and I still run this patch locally. =)

It does hurt measured performance, but for the broken set of boards
this is absolutely necessary to function on a Gigabit link and it
doesn't look like this broken set is really a minority given the
number of complaints I've seen.

Thanks,

Kyle Evans

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[Peter Robinson]

On Wed, Apr 18, 2018 at 2:16 PM, Kyle Evans <kevans@freebsd.org> wrote:
> On Wed, Feb 14, 2018 at 5:02 PM,  <kevans@freebsd.org> wrote:
>> The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
>> significant packet loss on Gigabit links. This patch sets the magical bits
>> in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
>> undocumented stuff.
>>
>> [1] https://forum.pine64.org/showthread.php?tid=835&pid=19704#pid19704
>>
>> Kyle Evans (2):
>>   net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys
>>   Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX
>>
>>  configs/pine64_plus_defconfig |  2 ++
>>  drivers/net/phy/Kconfig       | 10 ++++++++++
>>  drivers/net/phy/realtek.c     | 34 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 46 insertions(+)
>>
>
> Hi,
>
> I'd like to dredge this one up again, since it's been a couple months
> and I still run this patch locally. =)
>
> It does hurt measured performance, but for the broken set of boards
> this is absolutely necessary to function on a Gigabit link and it
> doesn't look like this broken set is really a minority given the
> number of complaints I've seen.

I see it upstream in master:
http://git.denx.de/?p=u-boot.git;a=commit;h=dfa1a74045c930ec3935f748b0969f9d76352e13
http://git.denx.de/?p=u-boot.git;a=commit;h=66526e70381dbaad58533cfbd7bce07c668205c6

What else is needed?

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[Kyle Evans]

On Wed, Apr 18, 2018 at 8:55 AM, Peter Robinson <pbrobinson@gmail.com> wrote:
> On Wed, Apr 18, 2018 at 2:16 PM, Kyle Evans <kevans@freebsd.org> wrote:
>> On Wed, Feb 14, 2018 at 5:02 PM,  <kevans@freebsd.org> wrote:
>>> The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
>>> significant packet loss on Gigabit links. This patch sets the magical bits
>>> in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
>>> undocumented stuff.
>>>
>>> [1] https://forum.pine64.org/showthread.php?tid=835&pid=19704#pid19704
>>>
>>> Kyle Evans (2):
>>>   net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys
>>>   Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX
>>>
>>>  configs/pine64_plus_defconfig |  2 ++
>>>  drivers/net/phy/Kconfig       | 10 ++++++++++
>>>  drivers/net/phy/realtek.c     | 34 ++++++++++++++++++++++++++++++++++
>>>  3 files changed, 46 insertions(+)
>>>
>>
>> Hi,
>>
>> I'd like to dredge this one up again, since it's been a couple months
>> and I still run this patch locally. =)
>>
>> It does hurt measured performance, but for the broken set of boards
>> this is absolutely necessary to function on a Gigabit link and it
>> doesn't look like this broken set is really a minority given the
>> number of complaints I've seen.
>
> I see it upstream in master:
> http://git.denx.de/?p=u-boot.git;a=commit;h=dfa1a74045c930ec3935f748b0969f9d76352e13
> http://git.denx.de/?p=u-boot.git;a=commit;h=66526e70381dbaad58533cfbd7bce07c668205c6
>
> What else is needed?

Ah, sorry about this, then. =) Clearly I need to grow a new pair of
eyeballs and stop browsing out-of-date source trees.

[U-Boot] [PATCH 0/2] Add fix for Pine64 gigabit throughput issues

[Peter Robinson]

On Wed, Apr 18, 2018 at 2:58 PM, Kyle Evans <kevans@freebsd.org> wrote:
> On Wed, Apr 18, 2018 at 8:55 AM, Peter Robinson <pbrobinson@gmail.com> wrote:
>> On Wed, Apr 18, 2018 at 2:16 PM, Kyle Evans <kevans@freebsd.org> wrote:
>>> On Wed, Feb 14, 2018 at 5:02 PM,  <kevans@freebsd.org> wrote:
>>>> The Pine64 has a known issue on gigabit links (see [1]); some boards suffer
>>>> significant packet loss on Gigabit links. This patch sets the magical bits
>>>> in CONFREG on the RTL8211E PHY to turn off the internal delay and do some other
>>>> undocumented stuff.
>>>>
>>>> [1] https://forum.pine64.org/showthread.php?tid=835&pid=19704#pid19704
>>>>
>>>> Kyle Evans (2):
>>>>   net: phy: Add PHY_RTL8211E_PINE64_GIGABIT_FIX for realtek phys
>>>>   Configs: Use the newly added PHY_RTL8211E_PINE64_GIGABIT_FIX
>>>>
>>>>  configs/pine64_plus_defconfig |  2 ++
>>>>  drivers/net/phy/Kconfig       | 10 ++++++++++
>>>>  drivers/net/phy/realtek.c     | 34 ++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 46 insertions(+)
>>>>
>>>
>>> Hi,
>>>
>>> I'd like to dredge this one up again, since it's been a couple months
>>> and I still run this patch locally. =)
>>>
>>> It does hurt measured performance, but for the broken set of boards
>>> this is absolutely necessary to function on a Gigabit link and it
>>> doesn't look like this broken set is really a minority given the
>>> number of complaints I've seen.
>>
>> I see it upstream in master:
>> http://git.denx.de/?p=u-boot.git;a=commit;h=dfa1a74045c930ec3935f748b0969f9d76352e13
>> http://git.denx.de/?p=u-boot.git;a=commit;h=66526e70381dbaad58533cfbd7bce07c668205c6
>>
>> What else is needed?
>
> Ah, sorry about this, then. =) Clearly I need to grow a new pair of
> eyeballs and stop browsing out-of-date source trees.

was in at least 2018.05-rc2