Re: [PATCH 0/3] Fix EEE support for MT7531 and MT7988 SoC switch

From: Arınç ÜNAL
Date: Thu Mar 21 2024 - 12:10:27 EST

On 20.03.2024 00:31, Florian Fainelli wrote:
On 3/19/24 13:26, Daniel Golle wrote:
On Tue, Mar 19, 2024 at 08:38:03PM +0100, Andrew Lunn wrote:
I would argue that EEE advertisement on the PHY should be enabled by
default.

That is an open question at the moment. For some use cases, it can add
extra delay and jitter which can cause problems. I've heard people
doing PTP don't like EEE for example.

MediaTek consumer-grade hardware doesn't support PTP and hence that
quite certainly won't ever be an issue with all switch ICs supported
by the mt7530 driver.

I'd rather first change the (configuration) default in OpenWrt (which
is arguable the way most people are using this hardware), also because
that will be more visible/obvious for users. Or even just make EEE
configurable in the LuCI web-UI as a first step so users start playing
with it.

After all, I also have a hard time imagining that MediaTek disabled
EEE in their downstream driver for no reason:

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

EEE tends to be an interoperability trap and typically results in unexplained link drops with different link partners which are difficult to debug and root cause. It would be great to have more context as to why it was disabled in the downstream tree to know what we are up against, though I would not be surprised if there had been a number of issues reported.

I have started testing MT7531 with EEE enabled and immediately experienced
frames that wouldn't egress the switch or improperly received on the link
partner.

SoC MAC <-EEE off-> MT7531 P6 MAC (acting as PHY)
MT7531 P0 MAC <-EEE on -> MT7531 P0 PHY
MT7531 P0 PHY <-EEE on -> Computer connected with twisted pair

I've tested pinging from the SoC's CPU. Packet capturing on the twisted
pair computer showed very few frames were being received.

# ping 192.168.2.2
PING 192.168.2.2 (192.168.2.2): 56 data bytes
64 bytes from 192.168.2.2: seq=36 ttl=64 time=0.486 ms
^C
--- 192.168.2.2 ping statistics ---
64 packets transmitted, 1 packets received, 98% packet loss
round-trip min/avg/max = 0.486/0.486/0.486 ms

It seems there's less loss when frames are passed more frequently.

# ping 192.168.2.2 -i 0.06
PING 192.168.2.2 (192.168.2.2): 56 data bytes
64 bytes from 192.168.2.2: seq=5 ttl=64 time=0.285 ms
64 bytes from 192.168.2.2: seq=6 ttl=64 time=0.155 ms
64 bytes from 192.168.2.2: seq=7 ttl=64 time=0.243 ms
64 bytes from 192.168.2.2: seq=8 ttl=64 time=0.139 ms
64 bytes from 192.168.2.2: seq=9 ttl=64 time=0.224 ms
64 bytes from 192.168.2.2: seq=68 ttl=64 time=0.350 ms
64 bytes from 192.168.2.2: seq=69 ttl=64 time=0.242 ms
64 bytes from 192.168.2.2: seq=70 ttl=64 time=0.230 ms
64 bytes from 192.168.2.2: seq=71 ttl=64 time=0.242 ms
64 bytes from 192.168.2.2: seq=72 ttl=64 time=0.276 ms
64 bytes from 192.168.2.2: seq=101 ttl=64 time=0.224 ms
64 bytes from 192.168.2.2: seq=102 ttl=64 time=0.238 ms
64 bytes from 192.168.2.2: seq=103 ttl=64 time=0.240 ms
..
--- 192.168.2.2 ping statistics ---
214 packets transmitted, 32 packets received, 85% packet loss
round-trip min/avg/max = 0.099/0.225/0.350 ms

# ping 192.168.2.2 -i 0.05
PING 192.168.2.2 (192.168.2.2): 56 data bytes
64 bytes from 192.168.2.2: seq=1 ttl=64 time=0.277 ms
64 bytes from 192.168.2.2: seq=2 ttl=64 time=0.240 ms
64 bytes from 192.168.2.2: seq=3 ttl=64 time=0.133 ms
64 bytes from 192.168.2.2: seq=4 ttl=64 time=0.233 ms
64 bytes from 192.168.2.2: seq=5 ttl=64 time=0.223 ms
64 bytes from 192.168.2.2: seq=6 ttl=64 time=0.228 ms
64 bytes from 192.168.2.2: seq=7 ttl=64 time=0.236 ms
64 bytes from 192.168.2.2: seq=8 ttl=64 time=0.150 ms
..
--- 192.168.2.2 ping statistics ---
41 packets transmitted, 40 packets received, 2% packet loss
round-trip min/avg/max = 0.112/0.206/0.277 ms


That said as an user, if someone has a well controlled environment, they should absolutely be able to turn on EEE and see how stable it holds in their environment.

Looks like this is the way to go. I'm planning to submit v2 with patch 1
as:

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 678b51f9cea6..6aa99b590329 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2458,6 +2458,20 @@ mt7531_setup(struct dsa_switch *ds)
/* Reset the switch through internal reset */
mt7530_write(priv, MT7530_SYS_CTRL, SYS_CTRL_SW_RST | SYS_CTRL_REG_RST);
+ /* Allow modifying the trap and enable Energy-Efficient Ethernet (EEE).
+ */
+ val = mt7530_read(priv, MT7531_HWTRAP);
+ val |= CHG_STRAP;
+ val &= ~EEE_DIS;
+ mt7530_write(priv, MT7530_MHWTRAP, val);
+
+ /* Disable EEE advertisement on the switch PHYs. */
+ for (i = MT753X_CTRL_PHY_ADDR;
+ i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) {
+ mt7531_ind_c45_phy_write(priv, i, MDIO_MMD_AN, MDIO_AN_EEE_ADV,
+ 0);
+ }
+
if (!priv->p5_sgmii) {
mt7531_pll_setup(priv);
} else {
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index a71166e0a7fc..509ed5362236 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -457,6 +457,7 @@ enum mt7531_clk_skew {
#define XTAL_FSEL_M BIT(7)
#define PHY_EN BIT(6)
#define CHG_STRAP BIT(8)
+#define EEE_DIS BIT(4)
/* Register for hw trap modification */
#define MT7530_MHWTRAP 0x7804

Arınç