Message ID | 20240321-for-net-mt7530-fix-eee-for-mt7531-mt7988-v2-1-9af9d5041bfe@arinc9.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | Fix EEE support for MT7531 and MT7988 SoC switch | expand |
On Thu, 2024-03-21 at 19:29 +0300, Arınç ÜNAL via B4 Relay wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > From: Arınç ÜNAL <arinc.unal@arinc9.com> > > The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE > features") > brought EEE support but did not enable EEE on MT7531 switch MACs. EEE > is > enabled on MT7531 switch MACs either by pulling the LAN2LED0 pin low > on the > board (bootstrapping), or unsetting the EEE_DIS bit on the trap > register. > > There are existing boards that were not designed to pull the pin low. > Therefore, unset the EEE_DIS bit on the trap register. > > Unlike MT7530, the modifiable trap register won't be populated > identical to > the trap status register after reset. Therefore, read from the trap > status > register, modify the bits, then write to the modifiable trap > register. > > My testing on MT7531 shows a certain amount of traffic loss when EEE > is > enabled. That said, I haven't come across a board that enables EEE. > So > enable EEE on the switch MACs but disable EEE advertisement on the > switch > PHYs. This way, we don't change the behaviour of the majority of the > boards > that have this switch. > > With this change, EEE can now be enabled using ethtool. > > The disable EEE bit on the trap pertains to the LAN2LED0 pin which is > usually used to control an LED. Once the bit is unset, the pin will > be low. > That will make the active low LED turn on. > > The pin is controlled by the switch PHY. It seems that the PHY > controls the > pin in the way that it inverts the pin state. That means depending on > the > wiring of the LED connected to LAN2LED0 on the board, the LED may be > on > without an active link. > > Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE > features") > Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > --- > drivers/net/dsa/mt7530.c | 14 ++++++++++++++ > drivers/net/dsa/mt7530.h | 1 + > 2 files changed, 15 insertions(+) > > 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); You may try to set bit 6 of CL45 register dev=0x1f, reg=0x403(PLL_GROUP_CTL_REG), which is an internal EEE switch called RG_SYSPLL_DMY2. Read it out first with "switch command", if you have it: root@OpenWrt:/# switch phy cl45 r 0 0x1f 0x403 Phy read dev_num=0x1f, reg=0x403, value=0x1091 And then set bit 6: root@OpenWrt:/# $ switch phy cl45 w 0 0x1f 0x403 0x10d1 root@OpenWrt:/# ethtool --set-eee lan1 eee on tx-lpi on tx-timer 0x1e advertise 0x28 root@OpenWrt:/# switch phy cl45 r 0 0x7 0x3c Phy read dev_num=0x7, reg=0x3c, value=0x6 Then you should be able to enable EEE via ethtool without clearing EEE_DIS bit of MT7530_HWTRAP. If above CL45 command is good for you, I think this can be moved to mtk_gephy_config_init() @ mediatek-ge.c, which will lead to cleaner implementation. > + > +/* 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); > +} Sorry, I still can't figure out why this is needed since we disable MDIO_AN_EEE_ADV in mediatek-ge.c after reading previous threads. Would you please provide something else (like dmesg log?) to show that settings in mtk_gephy_config_init() may fail? > + > 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_MBIT(7) > #define PHY_ENBIT(6) > #define CHG_STRAPBIT(8) > +#define EEE_DISBIT(4) > > /* Register for hw trap modification */ > #define MT7530_MHWTRAP0x7804 > > -- > 2.40.1 > >
Hello Huang. It's great to see someone from MediaTek providing information on this intellectual property. On 27.03.2024 12:51, SkyLake Huang (黃啟澤) wrote: > You may try to set bit 6 of CL45 register dev=0x1f, > reg=0x403(PLL_GROUP_CTL_REG), which is an internal EEE switch called > RG_SYSPLL_DMY2. > > Read it out first with "switch command", if you have it: > root@OpenWrt:/# switch phy cl45 r 0 0x1f 0x403 > Phy read dev_num=0x1f, reg=0x403, value=0x1091 > > And then set bit 6: > root@OpenWrt:/# $ switch phy cl45 w 0 0x1f 0x403 0x10d1 > root@OpenWrt:/# ethtool --set-eee lan1 eee on tx-lpi on tx-timer 0x1e > advertise 0x28 > root@OpenWrt:/# switch phy cl45 r 0 0x7 0x3c > Phy read dev_num=0x7, reg=0x3c, value=0x6 > > Then you should be able to enable EEE via ethtool without clearing > EEE_DIS bit of MT7530_HWTRAP. This won't interfere with the LEDs so it's better. I'll use this method, thank you! This is the current diff [1]. I suppose bit 6 is an internal EEE switch only on MT7531? I see that bit 6 of 0x403 is unset on MT7530 yet EEE is still enabled on the switch MACs. There's still packet loss when EEE is enabled by setting MT7531_RG_SYSPLL_DMY2. Please let me know if MT7531 switch PHYs need calibration for EEE like MT7986 and MT7988 SoC PHYs. > If above CL45 command is good for you, I think this can be moved to > mtk_gephy_config_init() @ mediatek-ge.c, which will lead to cleaner > implementation. I disagree. The operation concerns the switch MACs too, not just the switch PHYs. And the operation is not per switch PHY. >> + >> +/* 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); >> +} > Sorry, I still can't figure out why this is needed since we disable > MDIO_AN_EEE_ADV in mediatek-ge.c after reading previous threads. Would > you please provide something else (like dmesg log?) to show that > settings in mtk_gephy_config_init() may fail? Disabling EEE advertisement on the PHY driver doesn't fail. To confirm that and that MDIO_AN_EEE_ADV is populated after it is unset by the PHY driver, I've done some tests with this diff [2]. Currently: [ 2.189149] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000 [ 2.204792] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000 [ 2.221413] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=139) [ 2.233762] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000 [ 2.242555] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000 [ 2.259011] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=140) [ 2.271052] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000 [ 2.279842] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000 [ 2.296297] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=141) [ 2.308331] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000 [ 2.317134] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000 [ 2.333597] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=142) [ 2.345653] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000 [ 2.354423] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000 [ 2.370885] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=143) # mount -t debugfs none /sys/kernel/debug # echo go > /sys/kernel/debug/mt7530/eee_adv [ 33.055962] mt7530-mdio mdio-bus:1f: phy0: MDIO_AN_EEE_ADV is 00000000 [ 33.062943] mt7530-mdio mdio-bus:1f: phy1: MDIO_AN_EEE_ADV is 00000000 [ 33.069909] mt7530-mdio mdio-bus:1f: phy2: MDIO_AN_EEE_ADV is 00000000 [ 33.076871] mt7530-mdio mdio-bus:1f: phy3: MDIO_AN_EEE_ADV is 00000000 [ 33.083822] mt7530-mdio mdio-bus:1f: phy4: MDIO_AN_EEE_ADV is 00000000 # ethtool --show-eee lan0 EEE settings for lan0: EEE status: not supported --- If EEE_DIS is unset or MT7531_RG_SYSPLL_DMY2 is set, MDIO_AN_EEE_ADV is populated. This also happens on the MT7530 switch: [ 2.254291] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000006 [ 2.270015] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000 [ 2.286572] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=139) [ 2.298921] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000006 [ 2.307712] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000 [ 2.324162] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=140) [ 2.336204] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000006 [ 2.344974] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000 [ 2.361445] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=141) [ 2.373475] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000006 [ 2.382273] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000 [ 2.398728] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=142) [ 2.410780] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000006 [ 2.419571] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000 [ 2.436028] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=143) # mount -t debugfs none /sys/kernel/debug # echo go > /sys/kernel/debug/mt7530/eee_adv [ 8.731911] mt7530-mdio mdio-bus:1f: phy0: MDIO_AN_EEE_ADV is 00000006 [ 8.738908] mt7530-mdio mdio-bus:1f: phy1: MDIO_AN_EEE_ADV is 00000006 [ 8.745861] mt7530-mdio mdio-bus:1f: phy2: MDIO_AN_EEE_ADV is 00000006 [ 8.752816] mt7530-mdio mdio-bus:1f: phy3: MDIO_AN_EEE_ADV is 00000006 [ 8.759776] mt7530-mdio mdio-bus:1f: phy4: MDIO_AN_EEE_ADV is 00000006 # ethtool --show-eee lan0 EEE settings for lan0: EEE status: enabled - inactive Tx LPI: disabled Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: 100baseT/Full 1000baseT/Full Link partner advertised EEE link modes: Not reported --- If MDIO_AN_EEE_ADV is unset before the PHY driver kicks in, it stays unset: [ 2.266561] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000 [ 2.282268] MediaTek MT7531 PHY mt7530-0:00: MDIO_AN_EEE_ADV is 00000000 [ 2.298832] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7531 PHY] (irq=139) [ 2.311187] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000 [ 2.319976] MediaTek MT7531 PHY mt7530-0:01: MDIO_AN_EEE_ADV is 00000000 [ 2.336443] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7531 PHY] (irq=140) [ 2.348488] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000 [ 2.357280] MediaTek MT7531 PHY mt7530-0:02: MDIO_AN_EEE_ADV is 00000000 [ 2.373728] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7531 PHY] (irq=141) [ 2.385780] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000 [ 2.394551] MediaTek MT7531 PHY mt7530-0:03: MDIO_AN_EEE_ADV is 00000000 [ 2.411011] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7531 PHY] (irq=142) [ 2.423047] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000 [ 2.431849] MediaTek MT7531 PHY mt7530-0:04: MDIO_AN_EEE_ADV is 00000000 [ 2.448324] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7531 PHY] (irq=143) # mount -t debugfs none /sys/kernel/debug # echo go > /sys/kernel/debug/mt7530/eee_adv [ 33.906140] mt7530-mdio mdio-bus:1f: phy0: MDIO_AN_EEE_ADV is 00000000 [ 33.913121] mt7530-mdio mdio-bus:1f: phy1: MDIO_AN_EEE_ADV is 00000000 [ 33.920094] mt7530-mdio mdio-bus:1f: phy2: MDIO_AN_EEE_ADV is 00000000 [ 33.927054] mt7530-mdio mdio-bus:1f: phy3: MDIO_AN_EEE_ADV is 00000000 [ 33.934005] mt7530-mdio mdio-bus:1f: phy4: MDIO_AN_EEE_ADV is 00000000 # ethtool --show-eee lan0 EEE settings for lan0: EEE status: disabled Tx LPI: disabled Supported EEE link modes: 100baseT/Full 1000baseT/Full Advertised EEE link modes: Not reported Link partner advertised EEE link modes: Not reported [1] diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index fb3d2a53bbd9..9a351d11f349 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2630,18 +2630,25 @@ mt7531_setup(struct dsa_switch *ds) mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK, MT7531_GPIO0_INTERRUPT); - /* Enable PHY core PLL, since phy_device has not yet been created - * provided for phy_[read,write]_mmd_indirect is called, we provide - * our own mt7531_ind_mmd_phy_[read,write] to complete this - * function. + /* Enable Energy-Efficient Ethernet (EEE) and PHY core PLL, since + * phy_device has not yet been created provided for + * phy_[read,write]_mmd_indirect is called, we provide our own + * mt7531_ind_mmd_phy_[read,write] to complete this function. */ val = mt7531_ind_c45_phy_read(priv, MT753X_CTRL_PHY_ADDR, MDIO_MMD_VEND2, CORE_PLL_GROUP4); - val |= MT7531_PHY_PLL_BYPASS_MODE; + val |= MT7531_RG_SYSPLL_DMY2 | MT7531_PHY_PLL_BYPASS_MODE; val &= ~MT7531_PHY_PLL_OFF; mt7531_ind_c45_phy_write(priv, MT753X_CTRL_PHY_ADDR, MDIO_MMD_VEND2, CORE_PLL_GROUP4, 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); + } + mt7531_setup_common(ds); /* Setup VLAN ID 0 for VLAN-unaware bridges */ diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h index d17b318e6ee4..9439db495001 100644 --- a/drivers/net/dsa/mt7530.h +++ b/drivers/net/dsa/mt7530.h @@ -616,6 +616,7 @@ enum mt7531_clk_skew { #define RG_SYSPLL_DDSFBK_EN BIT(12) #define RG_SYSPLL_BIAS_EN BIT(11) #define RG_SYSPLL_BIAS_LPF_EN BIT(10) +#define MT7531_RG_SYSPLL_DMY2 BIT(6) #define MT7531_PHY_PLL_OFF BIT(5) #define MT7531_PHY_PLL_BYPASS_MODE BIT(4) [2] diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 35906e669259..fb3d2a53bbd9 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -845,6 +845,22 @@ static ssize_t reg_value_write(struct file *filp, const char __user *buffer, return count; } +static ssize_t eee_adv_write(struct file *filp, const char __user *buffer, + size_t count, loff_t *ppos) +{ + struct mt7530_priv *priv = filp->private_data; + int i; + + for (i = MT753X_CTRL_PHY_ADDR; + i < MT753X_CTRL_PHY_ADDR + MT7530_NUM_PHYS; i++) { + dev_info(priv->dev, "phy%d: MDIO_AN_EEE_ADV is %08x", i, + mt7531_ind_c45_phy_read(priv, i, MDIO_MMD_AN, + MDIO_AN_EEE_ADV)); + } + + return count; +} + static const struct file_operations reg_address_fops = { .owner = THIS_MODULE, .open = simple_open, @@ -859,6 +875,12 @@ static const struct file_operations reg_value_fops = { .write = reg_value_write, }; +static const struct file_operations eee_adv_fops = { + .owner = THIS_MODULE, + .open = simple_open, + .write = eee_adv_write, +}; + static void mt7530_debugfs_init(struct mt7530_priv *priv) { struct dentry *dir; @@ -869,6 +891,7 @@ static void mt7530_debugfs_init(struct mt7530_priv *priv) debugfs_create_file("reg_address", 0644, dir, priv, ®_address_fops); debugfs_create_file("reg_value", 0644, dir, priv, ®_value_fops); + debugfs_create_file("eee_adv", 0644, dir, priv, &eee_adv_fops); } static void diff --git a/drivers/net/phy/mediatek-ge.c b/drivers/net/phy/mediatek-ge.c index e8822adbf3bc..aacef7091d8f 100644 --- a/drivers/net/phy/mediatek-ge.c +++ b/drivers/net/phy/mediatek-ge.c @@ -71,9 +71,15 @@ static int mtk_gephy_write_page(struct phy_device *phydev, int page) static void mtk_gephy_config_init(struct phy_device *phydev) { + dev_info(&phydev->mdio.dev, "MDIO_AN_EEE_ADV is %08x", + phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV)); + /* Disable EEE */ phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0); + dev_info(&phydev->mdio.dev, "MDIO_AN_EEE_ADV is %08x", + phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV)); + /* Enable HW auto downshift */ phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED, 0x14, 0, BIT(4)); Arınç
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