[BPi-R3-Mini] Airoha EN8811H 2.5G PHY driver

[dangowrt (Daniel Golle)]

TX rate adaption for 1000M link partners is not working that great apparently

iperf3 from 1G-connected host to R3-mini via 2.5G-capable switch (link speed on R3-mini is 2.5G)

Connecting to host 192.168.5.169, port 5201
[  5] local 192.168.1.136 port 38688 connected to 192.168.5.169 port 5201
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   115 MBytes   962 Mbits/sec   59    889 KBytes       
[  5]   1.00-2.00   sec   113 MBytes   945 Mbits/sec    0    981 KBytes       
[  5]   2.00-3.00   sec   112 MBytes   941 Mbits/sec    0   1.04 MBytes       
[  5]   3.00-4.00   sec   113 MBytes   946 Mbits/sec    9    806 KBytes       
[  5]   4.00-5.00   sec   112 MBytes   941 Mbits/sec    0    912 KBytes       
[  5]   5.00-6.00   sec   112 MBytes   937 Mbits/sec    0    994 KBytes       
[  5]   6.00-7.00   sec   112 MBytes   936 Mbits/sec    0   1.05 MBytes       
[  5]   7.00-8.00   sec   113 MBytes   951 Mbits/sec    9    822 KBytes       
[  5]   8.00-9.00   sec   111 MBytes   931 Mbits/sec    0    921 KBytes       
[  5]   9.00-10.00  sec   112 MBytes   942 Mbits/sec    0   1011 KBytes       
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec  1.10 GBytes   943 Mbits/sec   77             sender
[  5]   0.00-10.01  sec  1.10 GBytes   941 Mbits/sec                  receiver

iperf Done.

iperf3 from host to R3-mini connect direct to the R3-mini with 1G

[root@box filogic]# iperf3 -R -c 192.168.1.1
Connecting to host 192.168.1.1, port 5201
Reverse mode, remote host 192.168.1.1 is sending
[  5] local 192.168.1.254 port 53862 connected to 192.168.1.1 port 5201
[ ID] Interval           Transfer     Bitrate
[  5]   0.00-1.00   sec  92.1 MBytes   772 Mbits/sec                  
[  5]   1.00-2.00   sec  94.8 MBytes   795 Mbits/sec                  
[  5]   2.00-3.00   sec  91.0 MBytes   763 Mbits/sec                  
[  5]   3.00-4.00   sec  95.6 MBytes   802 Mbits/sec                  
[  5]   4.00-5.00   sec  85.0 MBytes   713 Mbits/sec                  
[  5]   5.00-6.00   sec  88.5 MBytes   742 Mbits/sec                  
[  5]   6.00-7.00   sec  84.6 MBytes   710 Mbits/sec                  
[  5]   7.00-8.00   sec  90.0 MBytes   755 Mbits/sec                  
[  5]   8.00-9.00   sec  98.4 MBytes   825 Mbits/sec                  
[  5]   9.00-10.00  sec  90.9 MBytes   762 Mbits/sec                  
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-10.00  sec   911 MBytes   764 Mbits/sec  4537             sender
[  5]   0.00-10.00  sec   911 MBytes   764 Mbits/sec                  receiver

iperf Done.

So 764 MBit/s instead of 942 MBit/s just because the EN8811H is sending a few too many pause frames…

[ericwoud (Eric W.)]

@dangowrt

the missing 2500M link-partner advertisement bit which we were promised to have in some custom register. So that didn’t happen…?

This part it apparently is not possible to fix.

[frank-w]

You do not use the uboot driver from immortalwrt/my tree as base? This has firmware builtin via header file, not good?

But we should discuss this in the uboot thread

[ericwoud (Eric W.)]

@dangowrt

From what I can tell the one issue which is still unresolved is having to go through the whole reset → load-firmware → … cycle every time the link is set down

I will have to look in to this, I was not aware of this until yesterday. So I need to check this with original driver.

[ericwoud (Eric W.)]

@ericwoud

I will have to look in to this, I was not aware of this until yesterday. So I need to check this with original driver.

I tracked this all the way back to my first version (almost same as original) and with older firmware, still this issue.

Edit: So I was too hasty with my last post and deleted it.

[ericwoud (Eric W.)]

@dangowrt:

It may not be the most elegant solution, but it seems to work. Based on the load firmware routine, found that the ‘host’ can be restarted like this:

static int en8811h_restart_host(struct phy_device *phydev)
{
	int ret;

	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
				     EN8811H_FW_CTRL_1_START);
	if (ret < 0)
		return ret;

	return air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
				     EN8811H_FW_CTRL_1_FINISH);
}

So added:

	if (!priv->fw_loaded) {
		ret = en8811h_load_firmware(phydev);
		if (ret < 0)
			phydev_err(phydev, "Load firmware failed: %d\n", ret);
		else
			priv->fw_loaded = true;
	} else {
		ret = en8811h_restart_host(phydev);
	}
	if (ret < 0)
		return ret;

But then still need to do the entire setup, but it is a lot quicker without loading the firmware files.

Edit:

So I cleaned up a bit, so that the code in en8811h_config_init() is clean:

	if (!priv->fw_loaded)
		ret = en8811h_load_firmware(phydev);
	else
		ret = en8811h_restart_host(phydev);
	if (ret < 0)
		return ret;

A few lines moved to: en8811h_load_firmware().

[frank-w]

@ericwoud

Maybe this is interesting:

https://git01.mediatek.com/plugins/gitiles/openwrt/feeds/mtk-openwrt-feeds/+/cf03c993d54465eedcfc0f2a533b0239756497f7

[dangowrt (Daniel Golle)]

Updated firmware was posted linux-firmware

https://lore.kernel.org/linux-firmware/[email protected]/T/#u

[dangowrt (Daniel Golle)]

Ok, tested it (eh8811h firmware 24011202) and now indeed 2500M LPA gets set if the link partner advertises 2500M. However, it doesn’t get cleared if the link partner no longer advertises it or even the cable is unplugged and a different link-partner is plugged in, then we see

[   56.198597] Airoha EN8811H mdio-bus:0f: Downshift occurred from negotiated speed 2.5Gbps to actual speed 1Gbps, check cabling!
[   56.209986] mtk_soc_eth 15100000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx

I guess that will need another firmware fix…

[ericwoud (Eric W.)]

@frank-w

Maybe this is interesting:

Thanks for the tip…

[ericwoud (Eric W.)]

@dangowrt

However, it doesn’t get cleared if the link partner no longer advertises it

@dangowrt

I guess that will need another firmware fix…

Or… we clear the bit:

	if (pbus_value & EN8811H_2P5G_LPA_2P5G)
		linkmode_set_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
				 phydev->lp_advertising);
	else
		linkmode_clear_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
				 phydev->lp_advertising);

Edit:

Better to use linkmode_mod_bit()

	linkmode_mod_bit(ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
			 phydev->lp_advertising,
			 pbus_value & EN8811H_2P5G_LPA_2P5G);

[ericwoud (Eric W.)]

@dangowrt @frank-w @castiel652

So I guess it is about ready for rfc patch v2, if there are no further comments here…

[dangowrt (Daniel Golle)]

@ericwoud

Or… we clear the bit:

Well, did you actually test that?

I’ve tried that and added some debugging code to actually validate the content of the register. The full output including my debugging printout which I added to the (pbus_value & EN8811H_2P5G_LPA_2P5G) condition:

[   56.191775] Airoha EN8811H mdio-bus:0f: EN8811H_2P5G_LPA_2P5G bit was set
[   56.198597] Airoha EN8811H mdio-bus:0f: Downshift occurred from negotiated speed 2.5Gbps to actual speed 1Gbps, check cabling!
[   56.209986] mtk_soc_eth 15100000.ethernet eth1: Link is Up - 1Gbps/Full - flow control rx/tx

(link partner is NetXtreme BCM5762 Gigabit Ethernet PCIe which doesn’t use 2.5G/5G/10G advertisement bits at all. But before another link partner with RTL8221BV PHY which does support and did advertise 2.5G was plugged.)

[dangowrt (Daniel Golle)]

@ericwoud

if there are no further comments here…

One comment is that we can simplify things a bit by storing the firmware version as u32 instead of bool firmware_loaded. All memory in the priv struct gets zerod on allocation anyway, and we should only set firmware_version once we read it and it was non-zero. Then we can use the firmware_version member just like the firmware_loaded member before (ie. decide whether to call en8811h_load_firmware or en8811h_restart_host) and we could even use work-arounds for known firmware bugs (such as missing 2.5G LPA…) if the version u32 (which luckily can be compared as in unsigned int in a meaningful way as in 0x24011202 > 0x23112102).

Maybe diff is better than English to explain what I mean

fixes to be squashed into the previous commit

[ericwoud (Eric W.)]

@dangowrt

Well, did you actually test that?

Yes it works fine here.

The original code only setting the bit had genphy_c45_read_lpa() but this was changed. There the bit was always cleared. So this is why my old code was only setting the bit.

mdio.h - include/linux/mdio.h - Linux source code (v6.7.3) - Bootlin

I was also thinking about using the firmware version… before firmware load it always reads a different number, but always 0x8bxxxxxx. Always negative then it would always be smaller than loaded firmware version.

I will look into your suggestions this weekend.

Edit:

@dangowrt I was thinking to drop the dirty bug fix lpa2500 totally If by any chance someone is still using old firmware, there is no big deal, only missing the lpa2500 bit…

[ericwoud (Eric W.)]

Actually, my first thought of using the firmware version register, to use it to check if firmware is loaded or not. But I am not sure if the register value reliably can be used before any firmware is loaded (to determine it is not loaded). It is then always 0x8bxxxxxx, but I am but sure this will always in the future be the case …

[dangowrt (Daniel Golle)]

@ericwoud

Yes it works fine here.

I’m testing with your latest driver now and it does not work for me when switching cables from 2500M-capable link partner to Gigabit Ethernet link partner without any 802.3bz capabilities. See email for details and pull-request for a working work-around.

[dangowrt (Daniel Golle)]

@ericwoud

If by any chance someone is still using old firmware

It doesn’t hurt to include it at this point, also because linux-next and linux-firmware are asynchronous trees side-by-side and linux-firmware is updated in distros (and OpenWrt) independenly of the Linux kernel.

Hence, as I want to also merge this in OpenWrt asap it will have to still support the old firmware until the new one got merged to linux-firmware and a new linux-firmware release came out (because that’s what we are packaging in OpenWrt – doing a git checkout of linux-firmware is quite insane as you might now, so we rely on the released tarballs).

So please keep it, so we can have the identical driver to have users of OpenWrt and vanilla kernel with your driver test the same thing (and eventually submit bug reports for the same code).

[ericwoud (Eric W.)]

@dangowrt @frank-w

I just added hwmon temperature readout, in a new a patch in net-next-airoha branch.

I do not have any info on the units read from the register (only register address number, I do not have datasheet), so I think only thing to check is if this is the correct units.

If this is ok, I can just squash it to the first patch version (then I do not need a new patch for it).

[ericwoud (Eric W.)]

The hwmon temperature code is practically the same as the one in ‘mxl-gpy.c’, so I guess this would be acceptable code. I’ll squash it.