diff mbox series

[net,v3] net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards

Message ID 20240408-for-net-mt7530-fix-eee-for-mt7531-mt7988-v3-1-84fdef1f008b@arinc9.com (mailing list archive)
State Accepted
Commit 06dfcd4098cfdc4d4577d94793a4f9125386da8b
Delegated to: Netdev Maintainers
Headers show
Series [net,v3] net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 942 this patch: 942
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 16 of 16 maintainers
netdev/build_clang success Errors and warnings before: 953 this patch: 953
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 953 this patch: 953
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 37 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-04-10--06-00 (tests: 962)

Commit Message

Arınç ÜNAL via B4 Relay April 8, 2024, 7:08 a.m. UTC 
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 by pulling the LAN2LED0 pin low on the board
(bootstrapping), unsetting the EEE_DIS bit on the trap register, or setting
the internal EEE switch bit on the CORE_PLL_GROUP4 register. Thanks to
SkyLake Huang (黃啟澤) from MediaTek for providing information on the
internal EEE switch bit.

There are existing boards that were not designed to pull the pin low.
Because of that, the EEE status currently depends on the board design.

The EEE_DIS 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.

To not cause this unwanted behaviour whilst enabling EEE on all boards, set
the internal EEE switch bit on the CORE_PLL_GROUP4 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. The mediatek-ge PHY driver already disables EEE
advertisement on the switch PHYs but my testing shows that it is somehow
enabled afterwards. Disabling EEE advertisement before the PHY driver
initialises keeps it off.

With this change, EEE can now be enabled using ethtool.

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>
---
Here's some information for the record. EEE could not be enabled on MT7531
on most boards using ethtool before this. On MT7988 SoC switch, EEE is
disabled by default but can be turned on normally using ethtool. EEE is
enabled by default on MT7530 and there's no need to make changes on the DSA
subdriver for it.
---
Changes in v3:
- Remove patch 2, it was revealed that it doesn't fix a bug.
- Patch 1
  - Use the internal EEE switch bit provided by SkyLake Huang (黃啟澤). It
    is a better method compared to unsetting the EEE_DIS bit of the trap as
    the latter method causes unwanted behaviour on the LED connected to the
    pin that pertains to the EEE_DIS bit.
- Link to v2: https://lore.kernel.org/r/20240321-for-net-mt7530-fix-eee-for-mt7531-mt7988-v2-0-9af9d5041bfe@arinc9.com

Changes in v2:
- Delegate the patch to the net tree.
- Remove patch 3, it was revealed that it doesn't fix a bug.
- Patch 1
  - Disable EEE advertisement on MT7531 by default.
- Link to v1: https://lore.kernel.org/r/20240318-for-net-mt7530-fix-eee-for-mt7531-mt7988-v1-0-3f17226344e8@arinc9.com
---
 drivers/net/dsa/mt7530.c | 17 ++++++++++++-----
 drivers/net/dsa/mt7530.h |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)


---
base-commit: bccb798e07f8bb8b91212fe8ed1e421685449076
change-id: 20240317-for-net-mt7530-fix-eee-for-mt7531-mt7988-a5c5453cc0e8

Best regards,

Comments

Paolo Abeni April 9, 2024, 11:58 a.m. UTC | #1 
On Mon, 2024-04-08 at 10:08 +0300, Arınç ÜNAL via B4 Relay wrote:
> 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  MACs. EEE is
> enabled on MT7531 switch MACs by pulling the LAN2LED0 pin low on the board
> (bootstrapping), unsetting the EEE_DIS bit on the trap register, or setting
> the internal EEE switch bit on the CORE_PLL_GROUP4 register. Thanks to
> SkyLake Huang (黃啟澤) from MediaTek for providing information on the
> internal EEE switch bit.
> 
> There are existing boards that were not designed to pull the pin low.
> Because of that, the EEE status currently depends on the board design.
> 
> The EEE_DIS 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.
> 
> To not cause this unwanted behaviour whilst enabling EEE on all boards, set
> the internal EEE switch bit on the CORE_PLL_GROUP4 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. The mediatek-ge PHY driver already disables EEE
> advertisement on the switch PHYs but my testing shows that it is somehow
> enabled afterwards. Disabling EEE advertisement before the PHY driver
> initialises keeps it off.
> 
> With this change, EEE can now be enabled using ethtool.
> 
> 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>
> ---
> Here's some information for the record. EEE could not be enabled on MT7531
> on most boards using ethtool before this. On MT7988 SoC switch, EEE is
> disabled by default but can be turned on normally using ethtool. EEE is
> enabled by default on MT7530 and there's no need to make changes on the DSA
> subdriver for it.
> ---
> Changes in v3:
> - Remove patch 2, it was revealed that it doesn't fix a bug.
> - Patch 1
>   - Use the internal EEE switch bit provided by SkyLake Huang (黃啟澤). It
>     is a better method compared to unsetting the EEE_DIS bit of the trap as
>     the latter method causes unwanted behaviour on the LED connected to the
>     pin that pertains to the EEE_DIS bit.

Since this leverages something relatively obscure, it would be great if
someone in the CC list could independently test it. Let's wait a bit
more.

Cheers,

Paolo
Daniel Golle April 10, 2024, 6:52 p.m. UTC | #2 
On Tue, Apr 09, 2024 at 01:58:56PM +0200, Paolo Abeni wrote:
> On Mon, 2024-04-08 at 10:08 +0300, Arınç ÜNAL via B4 Relay wrote:
> > 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  MACs. EEE is
> > enabled on MT7531 switch MACs by pulling the LAN2LED0 pin low on the board
> > (bootstrapping), unsetting the EEE_DIS bit on the trap register, or setting
> > the internal EEE switch bit on the CORE_PLL_GROUP4 register. Thanks to
> > SkyLake Huang (黃啟澤) from MediaTek for providing information on the
> > internal EEE switch bit.
> > 
> > There are existing boards that were not designed to pull the pin low.
> > Because of that, the EEE status currently depends on the board design.
> > 
> > The EEE_DIS 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.
> > 
> > To not cause this unwanted behaviour whilst enabling EEE on all boards, set
> > the internal EEE switch bit on the CORE_PLL_GROUP4 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. The mediatek-ge PHY driver already disables EEE
> > advertisement on the switch PHYs but my testing shows that it is somehow
> > enabled afterwards. Disabling EEE advertisement before the PHY driver
> > initialises keeps it off.
> > 
> > With this change, EEE can now be enabled using ethtool.
> > 
> > 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>
> > ---
> > Here's some information for the record. EEE could not be enabled on MT7531
> > on most boards using ethtool before this. On MT7988 SoC switch, EEE is
> > disabled by default but can be turned on normally using ethtool. EEE is
> > enabled by default on MT7530 and there's no need to make changes on the DSA
> > subdriver for it.
> > ---
> > Changes in v3:
> > - Remove patch 2, it was revealed that it doesn't fix a bug.
> > - Patch 1
> >   - Use the internal EEE switch bit provided by SkyLake Huang (黃啟澤). It
> >     is a better method compared to unsetting the EEE_DIS bit of the trap as
> >     the latter method causes unwanted behaviour on the LED connected to the
> >     pin that pertains to the EEE_DIS bit.
> 
> Since this leverages something relatively obscure, it would be great if
> someone in the CC list could independently test it. Let's wait a bit
> more.

I've excessively tested this patch on MT7531 today, and reviewed it
today and yesterday. I've also picked it as downstream patch[1] to
OpenWrt, so an even wider audience will have the pleasure of working
EEE on those switch ICs and in-SoC switches.

Tested-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Daniel Golle <daniel@makrotopia.org>

[1]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=98f9154316fe8371c709bd11ae8f263e22075ec6
patchwork-bot+netdevbpf@kernel.org April 11, 2024, 2 a.m. UTC | #3 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 08 Apr 2024 10:08:53 +0300 you wrote:
> 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 by pulling the LAN2LED0 pin low on the board
> (bootstrapping), unsetting the EEE_DIS bit on the trap register, or setting
> the internal EEE switch bit on the CORE_PLL_GROUP4 register. Thanks to
> SkyLake Huang (黃啟澤) from MediaTek for providing information on the
> internal EEE switch bit.
> 
> [...]

Here is the summary with links:
  - [net,v3] net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards
    https://git.kernel.org/netdev/net/c/06dfcd4098cf

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1035820c2377..451ff0620c2e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2505,18 +2505,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)