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

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

[Arınç ÜNAL]

Hi.

This patch series fixes EEE support for MT7531 and the switch on the MT7988
SoC. EEE did not work on MT7531 on most boards before this, it is unclear
what's the status on MT7988 SoC switch as I don't have the hardware.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
Arınç ÜNAL (3):
      net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards
      net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988
      net: phy: mediatek-ge: do not disable EEE advertisement

 drivers/net/dsa/mt7530.c      | 7 +++++++
 drivers/net/dsa/mt7530.h      | 7 ++++++-
 drivers/net/phy/mediatek-ge.c | 3 ---
 3 files changed, 13 insertions(+), 4 deletions(-)
---
base-commit: ea80e3ed09ab2c2b75724faf5484721753e92c31
change-id: 20240317-for-net-mt7530-fix-eee-for-mt7531-mt7988-a5c5453cc0e8

Best regards,
-- 
Arınç ÜNAL <arinc.unal@arinc9.com>

[PATCH 1/3] net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards

[Arınç ÜNAL]

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.

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")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 7 +++++++
 drivers/net/dsa/mt7530.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 678b51f9cea6..d741d03969eb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2458,6 +2458,13 @@ 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);
+
 	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

-- 
2.40.1

[PATCH 2/3] net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988

[Arınç ÜNAL]

The MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 bits let the
PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits determine the 1G/100 EEE
abilities of the MAC. If MT7531_FORCE_EEE1G and MT7531_FORCE_EEE100 are
unset, the abilities are left to be determined by PHY auto polling.

The commit 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
made it so that the PMCR_FORCE_EEE1G and PMCR_FORCE_EEE100 bits are set on
mt753x_phylink_mac_link_up(). But it did not set the MT7531_FORCE_EEE1G and
MT7531_FORCE_EEE100 bits. Because of this, EEE will be enabled on the
switch MACs by polling the PHY, regardless of the result of phy_init_eee().

Define these bits and add them to MT7531_FORCE_MODE which is being used by
the subdriver. With this, EEE will be prevented from being enabled on the
switch MACs when phy_init_eee() fails.

Fixes: 40b5d2f15c09 ("net: dsa: mt7530: Add support for EEE features")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 509ed5362236..5b99aeca34b4 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -299,11 +299,15 @@ enum mt7530_vlan_port_acc_frm {
 #define  MT7531_FORCE_DPX		BIT(29)
 #define  MT7531_FORCE_RX_FC		BIT(28)
 #define  MT7531_FORCE_TX_FC		BIT(27)
+#define  MT7531_FORCE_EEE100		BIT(26)
+#define  MT7531_FORCE_EEE1G		BIT(25)
 #define  MT7531_FORCE_MODE		(MT7531_FORCE_LNK | \
 					 MT7531_FORCE_SPD | \
 					 MT7531_FORCE_DPX | \
 					 MT7531_FORCE_RX_FC | \
-					 MT7531_FORCE_TX_FC)
+					 MT7531_FORCE_TX_FC | \
+					 MT7531_FORCE_EEE100 | \
+					 MT7531_FORCE_EEE1G)
 #define  PMCR_LINK_SETTINGS_MASK	(PMCR_TX_EN | PMCR_FORCE_SPEED_1000 | \
 					 PMCR_RX_EN | PMCR_FORCE_SPEED_100 | \
 					 PMCR_TX_FC_EN | PMCR_RX_FC_EN | \

-- 
2.40.1

[PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Arınç ÜNAL]

There's no need to disable Energy-Efficient Ethernet (EEE) advertisement on
the MT7530 and MT7531 switch PHYs. EEE works fine on MT7530 and MT7531
switch PHYs. Remove the code where EEE advertisement is disabled.

This is a bugfix because there's a possible race condition where the
mediatek-ge driver would kick in after the MT7530 DSA subdriver which would
have EEE disabled until manually enabled.

Fixes: e40d2cca0189 ("net: phy: add MediaTek Gigabit Ethernet PHY driver")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/phy/mediatek-ge.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/phy/mediatek-ge.c b/drivers/net/phy/mediatek-ge.c
index a493ae01b267..54ea64a37ab3 100644
--- a/drivers/net/phy/mediatek-ge.c
+++ b/drivers/net/phy/mediatek-ge.c
@@ -23,9 +23,6 @@ static int mtk_gephy_write_page(struct phy_device *phydev, int page)
 
 static void mtk_gephy_config_init(struct phy_device *phydev)
 {
-	/* Disable EEE */
-	phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, 0);
-
 	/* Enable HW auto downshift */
 	phy_modify_paged(phydev, MTK_PHY_PAGE_EXTENDED, 0x14, 0, BIT(4));
 

-- 
2.40.1

Re: [PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Arınç ÜNAL]

On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> There's no need to disable Energy-Efficient Ethernet (EEE) advertisement on
> the MT7530 and MT7531 switch PHYs. EEE works fine on MT7530 and MT7531
> switch PHYs. Remove the code where EEE advertisement is disabled.
> 
> This is a bugfix because there's a possible race condition where the
> mediatek-ge driver would kick in after the MT7530 DSA subdriver which would
> have EEE disabled until manually enabled.

Can I get an opinion on this? Is it actually possible that the PHY driver
would start probing after the DSA subdriver? On the console logs for the
DSA subdriver, I can see that the name of the PHY driver will appear, which
makes me believe the PHY driver would actually never probe after the DSA
subdriver.

[    4.402641] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.420392] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.437791] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.455096] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
[    4.472422] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)

I don't want to submit a bugfix to the net tree if the bug won't ever
happen in real life.

Arınç

Re: [PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Russell King (Oracle)]

On Wed, Mar 20, 2024 at 11:40:56PM +0300, Arınç ÜNAL wrote:
> On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > There's no need to disable Energy-Efficient Ethernet (EEE) advertisement on
> > the MT7530 and MT7531 switch PHYs. EEE works fine on MT7530 and MT7531
> > switch PHYs. Remove the code where EEE advertisement is disabled.
> > 
> > This is a bugfix because there's a possible race condition where the
> > mediatek-ge driver would kick in after the MT7530 DSA subdriver which would
> > have EEE disabled until manually enabled.
> 
> Can I get an opinion on this? Is it actually possible that the PHY driver
> would start probing after the DSA subdriver? On the console logs for the
> DSA subdriver, I can see that the name of the PHY driver will appear, which
> makes me believe the PHY driver would actually never probe after the DSA
> subdriver.
> 
> [    4.402641] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.420392] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.437791] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.455096] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.472422] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> 
> I don't want to submit a bugfix to the net tree if the bug won't ever
> happen in real life.

It would be really great if you could tell us which bug fixes you're
submitting are for a real problem that you or a user have encountered,
and which are down to essentially code inspection and things that
"aren't correct". Basically, don't do this.

It isn't true that the PHY specific driver will be probed before DSA
initialises - consider the case where the DSA driver is built-in but
the PHY specific driver is modular and on the not-yet-mounted rootfs.
That would result in the generic PHY driver being used even when the
PHY specific driver were to be loaded later - and thus only basic
standard 802.3 PHY behaviour will be supported.

That's not specific to mt7530, it applies to everything that uses
phylib. It isn't something that really warrants "bug fixing" in each
and every driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Russell King (Oracle)]

On Wed, Mar 20, 2024 at 11:40:56PM +0300, Arınç ÜNAL wrote:
> On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
> > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > 
> > There's no need to disable Energy-Efficient Ethernet (EEE) advertisement on
> > the MT7530 and MT7531 switch PHYs. EEE works fine on MT7530 and MT7531
> > switch PHYs. Remove the code where EEE advertisement is disabled.
> > 
> > This is a bugfix because there's a possible race condition where the
> > mediatek-ge driver would kick in after the MT7530 DSA subdriver which would
> > have EEE disabled until manually enabled.
> 
> Can I get an opinion on this? Is it actually possible that the PHY driver
> would start probing after the DSA subdriver? On the console logs for the
> DSA subdriver, I can see that the name of the PHY driver will appear, which
> makes me believe the PHY driver would actually never probe after the DSA
> subdriver.
> 
> [    4.402641] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.420392] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.437791] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.455096] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    4.472422] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> 
> I don't want to submit a bugfix to the net tree if the bug won't ever
> happen in real life.

It would be really great if you could tell us which bug fixes you're
submitting are for a real problem that you or a user have encountered,
and which are down to essentially code inspection and things that
"aren't correct". Basically, don't do this.

It isn't true that the PHY specific driver will be probed before DSA
initialises - consider the case where the DSA driver is built-in but
the PHY specific driver is modular and on the not-yet-mounted rootfs.
That would result in the generic PHY driver being used even when the
PHY specific driver were to be loaded later - and thus only basic
standard 802.3 PHY behaviour will be supported.

That's not specific to mt7530, it applies to everything that uses
phylib. It isn't something that really warrants "bug fixing" in each
and every driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Arınç ÜNAL]

On 20.03.2024 23:53, Russell King (Oracle) wrote:
> On Wed, Mar 20, 2024 at 11:40:56PM +0300, Arınç ÜNAL wrote:
>> On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
>> Can I get an opinion on this? Is it actually possible that the PHY driver
>> would start probing after the DSA subdriver? On the console logs for the
>> DSA subdriver, I can see that the name of the PHY driver will appear, which
>> makes me believe the PHY driver would actually never probe after the DSA
>> subdriver.
>>
>> [    4.402641] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.420392] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.437791] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.455096] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
>> [    4.472422] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
>>
>> I don't want to submit a bugfix to the net tree if the bug won't ever
>> happen in real life.
> 
> It would be really great if you could tell us which bug fixes you're
> submitting are for a real problem that you or a user have encountered,
> and which are down to essentially code inspection and things that
> "aren't correct". Basically, don't do this.

I agree. Patch 1 fixes a real problem, patch 2 "fixes" a problem found with
code inspection. Though, it would be great if you could review patch 2.

> 
> It isn't true that the PHY specific driver will be probed before DSA
> initialises - consider the case where the DSA driver is built-in but
> the PHY specific driver is modular and on the not-yet-mounted rootfs.
> That would result in the generic PHY driver being used even when the
> PHY specific driver were to be loaded later - and thus only basic
> standard 802.3 PHY behaviour will be supported.
> 
> That's not specific to mt7530, it applies to everything that uses
> phylib. It isn't something that really warrants "bug fixing" in each
> and every driver.

That makes sense. But there's a special case with the MT7530 DSA subdriver
and mediatek-ge driver. The PHY driver is needed for the PHYs to function
properly. So the DSA subdriver forces mediatek-ge to be selected [1]. So
the PHY driver could only be compiled as a module when the DSA subdriver is
also compiled so. And that designates mediatek-ge as a dependency for the
DSA subdriver, if I understand correctly.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=fb4bb62aaac715e50c7c007714af19a2698db88b

Re: [PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Florian Fainelli]

On 3/18/2024 12:46 AM, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> There's no need to disable Energy-Efficient Ethernet (EEE) advertisement on
> the MT7530 and MT7531 switch PHYs. EEE works fine on MT7530 and MT7531
> switch PHYs. Remove the code where EEE advertisement is disabled.
> 
> This is a bugfix because there's a possible race condition where the
> mediatek-ge driver would kick in after the MT7530 DSA subdriver which would
> have EEE disabled until manually enabled.
> 
> Fixes: e40d2cca0189 ("net: phy: add MediaTek Gigabit Ethernet PHY driver")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Could we get Sean to chime in, maybe there was a valid reason to disable 
EEE advertisement in the first place?
-- 
Florian

Re: [PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Florian Fainelli]

On 3/18/2024 12:46 AM, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> There's no need to disable Energy-Efficient Ethernet (EEE) advertisement on
> the MT7530 and MT7531 switch PHYs. EEE works fine on MT7530 and MT7531
> switch PHYs. Remove the code where EEE advertisement is disabled.
> 
> This is a bugfix because there's a possible race condition where the
> mediatek-ge driver would kick in after the MT7530 DSA subdriver which would
> have EEE disabled until manually enabled.
> 
> Fixes: e40d2cca0189 ("net: phy: add MediaTek Gigabit Ethernet PHY driver")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Could we get Sean to chime in, maybe there was a valid reason to disable 
EEE advertisement in the first place?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards

[Florian Fainelli]

On 3/18/2024 12:46 AM, 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 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.
> 
> 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")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

Re: [PATCH 1/3] net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards

[Florian Fainelli]

On 3/18/2024 12:46 AM, 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 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.
> 
> 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")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
> Hi.
> 
> This patch series fixes EEE support for MT7531 and the switch on the MT7988
> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
> what's the status on MT7988 SoC switch as I don't have the hardware.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

I see the state of this patch series is deferred on patchwork. I see that I
forgot to delegate this to the net tree. As I don't see any objections in
this series, I'll send v2 with it tomorrow.

Arınç

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

[Daniel Golle]

On Wed, Mar 20, 2024 at 11:10:19AM +0300, Arınç ÜNAL wrote:
> On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
> > Hi.
> > 
> > This patch series fixes EEE support for MT7531 and the switch on the MT7988
> > SoC. EEE did not work on MT7531 on most boards before this, it is unclear
> > what's the status on MT7988 SoC switch as I don't have the hardware.
> > 
> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> I see the state of this patch series is deferred on patchwork. I see that I
> forgot to delegate this to the net tree. As I don't see any objections in
> this series, I'll send v2 with it tomorrow.

Sorry, but imho it should go to net-next, and you might have overlook
it but there have been some concerns.

For sure it should not go to net tree as you are enabling a new feature
and not fixing anything which is broken. EEE on MT7988 works fine as
of now (see my reply), EEE on MT7531 was supposedly intentionally
disabled for reasons we should ask MTK people about.

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

[Daniel Golle]

On Wed, Mar 20, 2024 at 11:10:19AM +0300, Arınç ÜNAL wrote:
> On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
> > Hi.
> > 
> > This patch series fixes EEE support for MT7531 and the switch on the MT7988
> > SoC. EEE did not work on MT7531 on most boards before this, it is unclear
> > what's the status on MT7988 SoC switch as I don't have the hardware.
> > 
> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> I see the state of this patch series is deferred on patchwork. I see that I
> forgot to delegate this to the net tree. As I don't see any objections in
> this series, I'll send v2 with it tomorrow.

Sorry, but imho it should go to net-next, and you might have overlook
it but there have been some concerns.

For sure it should not go to net tree as you are enabling a new feature
and not fixing anything which is broken. EEE on MT7988 works fine as
of now (see my reply), EEE on MT7531 was supposedly intentionally
disabled for reasons we should ask MTK people about.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 20.03.2024 14:08, Daniel Golle wrote:
> On Wed, Mar 20, 2024 at 11:10:19AM +0300, Arınç ÜNAL wrote:
>> On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
>>> Hi.
>>>
>>> This patch series fixes EEE support for MT7531 and the switch on the MT7988
>>> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
>>> what's the status on MT7988 SoC switch as I don't have the hardware.
>>>
>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> I see the state of this patch series is deferred on patchwork. I see that I
>> forgot to delegate this to the net tree. As I don't see any objections in
>> this series, I'll send v2 with it tomorrow.
> 
> Sorry, but imho it should go to net-next, and you might have overlook
> it but there have been some concerns.

I don't believe I overlooked anything. I've come to this conclusion after
reading every response in this thread.

> 
> For sure it should not go to net tree as you are enabling a new feature
> and not fixing anything which is broken. EEE on MT7988 works fine as

EEE support exists since the commit which I've mentioned on my patches
here. I am fixing it. I thought I had explained this clearly on the
patches.

> of now (see my reply), EEE on MT7531 was supposedly intentionally
> disabled for reasons we should ask MTK people about.

Are you talking about the EEE_DIS bit on the trap? There's no default
setting there, it's up to the board vendor to enable/disable EEE by pulling
the affine pin low or high. So there's no intentional disabling by MediaTek
there. I see no need to ask the corporate regarding this.

This patch series does not in any way enable EEE on the switch PHYs and
MACs when it's disabled by default.

Arınç

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

[Arınç ÜNAL via B4 Relay]

Hi.

This patch series fixes EEE support for MT7531 and the switch on the MT7988
SoC. EEE did not work on MT7531 on most boards before this, it is unclear
what's the status on MT7988 SoC switch as I don't have the hardware.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
Arınç ÜNAL (3):
      net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards
      net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988
      net: phy: mediatek-ge: do not disable EEE advertisement

 drivers/net/dsa/mt7530.c      | 7 +++++++
 drivers/net/dsa/mt7530.h      | 7 ++++++-
 drivers/net/phy/mediatek-ge.c | 3 ---
 3 files changed, 13 insertions(+), 4 deletions(-)
---
base-commit: ea80e3ed09ab2c2b75724faf5484721753e92c31
change-id: 20240317-for-net-mt7530-fix-eee-for-mt7531-mt7988-a5c5453cc0e8

Best regards,
-- 
Arınç ÜNAL <arinc.unal@arinc9.com>

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

[Arınç ÜNAL via B4 Relay]

Hi.

This patch series fixes EEE support for MT7531 and the switch on the MT7988
SoC. EEE did not work on MT7531 on most boards before this, it is unclear
what's the status on MT7988 SoC switch as I don't have the hardware.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
Arınç ÜNAL (3):
      net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards
      net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988
      net: phy: mediatek-ge: do not disable EEE advertisement

 drivers/net/dsa/mt7530.c      | 7 +++++++
 drivers/net/dsa/mt7530.h      | 7 ++++++-
 drivers/net/phy/mediatek-ge.c | 3 ---
 3 files changed, 13 insertions(+), 4 deletions(-)
---
base-commit: ea80e3ed09ab2c2b75724faf5484721753e92c31
change-id: 20240317-for-net-mt7530-fix-eee-for-mt7531-mt7988-a5c5453cc0e8

Best regards,
-- 
Arınç ÜNAL <arinc.unal@arinc9.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Florian Fainelli]

On 3/18/2024 12:46 AM, Arınç ÜNAL via B4 Relay wrote:
> Hi.
> 
> This patch series fixes EEE support for MT7531 and the switch on the MT7988
> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
> what's the status on MT7988 SoC switch as I don't have the hardware.

We've received your patch series 4 times and this was the same thing 
with your previous b4 submission, can you find out what happened? Thanks.
-- 
Florian

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

[Florian Fainelli]

On 3/18/2024 12:46 AM, Arınç ÜNAL via B4 Relay wrote:
> Hi.
> 
> This patch series fixes EEE support for MT7531 and the switch on the MT7988
> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
> what's the status on MT7988 SoC switch as I don't have the hardware.

We've received your patch series 4 times and this was the same thing 
with your previous b4 submission, can you find out what happened? Thanks.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 18.03.2024 15:57, Florian Fainelli wrote:
> 
> 
> On 3/18/2024 12:46 AM, Arınç ÜNAL via B4 Relay wrote:
>> Hi.
>>
>> This patch series fixes EEE support for MT7531 and the switch on the MT7988
>> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
>> what's the status on MT7988 SoC switch as I don't have the hardware.
> 
> We've received your patch series 4 times and this was the same thing with your previous b4 submission, can you find out what happened? Thanks.

It looks like my branch name was too long again. b4 0.13.0 cannot handle
branch names that are too long. I'll keep it shorter for future
submissions. It'd be great if Konstantin could provide a specific limit.

https://lore.kernel.org/all/20240205-silky-sensible-puffin-8e23ee@lemur/

Arınç

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

[Konstantin Ryabitsev]

On Mon, Mar 18, 2024 at 05:03:29PM +0300, Arınç ÜNAL wrote:
> > We've received your patch series 4 times and this was the same thing with your previous b4 submission, can you find out what happened? Thanks.
> 
> It looks like my branch name was too long again. b4 0.13.0 cannot handle
> branch names that are too long. I'll keep it shorter for future
> submissions. It'd be great if Konstantin could provide a specific limit.

It's not really b4, it's the web endpoint and the version of python it's
running. I hope to fix it soon by applying the same workarounds as we ended up
doing for b4 itself.

-K

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

[Konstantin Ryabitsev]

On Mon, Mar 18, 2024 at 05:03:29PM +0300, Arınç ÜNAL wrote:
> > We've received your patch series 4 times and this was the same thing with your previous b4 submission, can you find out what happened? Thanks.
> 
> It looks like my branch name was too long again. b4 0.13.0 cannot handle
> branch names that are too long. I'll keep it shorter for future
> submissions. It'd be great if Konstantin could provide a specific limit.

It's not really b4, it's the web endpoint and the version of python it's
running. I hope to fix it soon by applying the same workarounds as we ended up
doing for b4 itself.

-K

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Konstantin Ryabitsev]

On Mon, Mar 18, 2024 at 02:01:58PM -0400, Konstantin Ryabitsev wrote:
> > It looks like my branch name was too long again. b4 0.13.0 cannot handle
> > branch names that are too long. I'll keep it shorter for future
> > submissions. It'd be great if Konstantin could provide a specific limit.
> 
> It's not really b4, it's the web endpoint and the version of python it's
> running. I hope to fix it soon by applying the same workarounds as we ended up
> doing for b4 itself.

I've deployed the version of endpoint that works around this bug. Hopefully,
you won't see it again regardless of the message-id length.

-K

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

[Konstantin Ryabitsev]

On Mon, Mar 18, 2024 at 02:01:58PM -0400, Konstantin Ryabitsev wrote:
> > It looks like my branch name was too long again. b4 0.13.0 cannot handle
> > branch names that are too long. I'll keep it shorter for future
> > submissions. It'd be great if Konstantin could provide a specific limit.
> 
> It's not really b4, it's the web endpoint and the version of python it's
> running. I hope to fix it soon by applying the same workarounds as we ended up
> doing for b4 itself.

I've deployed the version of endpoint that works around this bug. Hopefully,
you won't see it again regardless of the message-id length.

-K

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 19.03.2024 19:03, Konstantin Ryabitsev wrote:
> On Mon, Mar 18, 2024 at 02:01:58PM -0400, Konstantin Ryabitsev wrote:
>>> It looks like my branch name was too long again. b4 0.13.0 cannot handle
>>> branch names that are too long. I'll keep it shorter for future
>>> submissions. It'd be great if Konstantin could provide a specific limit.
>>
>> It's not really b4, it's the web endpoint and the version of python it's
>> running. I hope to fix it soon by applying the same workarounds as we ended up
>> doing for b4 itself.
> 
> I've deployed the version of endpoint that works around this bug. Hopefully,
> you won't see it again regardless of the message-id length.

Thank you very much!

Arınç

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

[Daniel Golle]

On Mon, Mar 18, 2024 at 10:46:22AM +0300, Arınç ÜNAL via B4 Relay wrote:
> Hi.
> 
> This patch series fixes EEE support for MT7531 and the switch on the MT7988
> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
> what's the status on MT7988 SoC switch as I don't have the hardware.

EEE seems to already work just fine on the MT7988 built-in switch, at least
on the BPI-R4. I don't think the SoC has bootstrap pins related to EEE like
stand-alone MT753x may have.

root@bpi-r4:~# ethtool --show-eee lan1
EEE settings for lan1:
    EEE status: disabled
    Tx LPI: 30 (us)
    Supported EEE link modes:  100baseT/Full
                               1000baseT/Full
    Advertised EEE link modes:  Not reported
    Link partner advertised EEE link modes:  100baseT/Full
                                             1000baseT/Full

root@bpi-r4:~# ethtool --set-eee lan1 eee on
root@bpi-r4:~# ethtool --show-eee lan1
EEE settings for lan1:
    EEE status: enabled - inactive
    Tx LPI: 30 (us)
    Supported EEE link modes:  100baseT/Full
                               1000baseT/Full
    Advertised EEE link modes:  100baseT/Full
                                1000baseT/Full
    Link partner advertised EEE link modes:  Not reported
root@bpi-r4:~# ethtool --show-eee lan1
EEE settings for lan1:
    EEE status: enabled - active
    Tx LPI: 30 (us)
    Supported EEE link modes:  100baseT/Full
                               1000baseT/Full
    Advertised EEE link modes:  100baseT/Full
                                1000baseT/Full
    Link partner advertised EEE link modes:  100baseT/Full
                                             1000baseT/Full

So don't fix if it ain't broken maybe...?

> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> Arınç ÜNAL (3):
>       net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards
>       net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988
>       net: phy: mediatek-ge: do not disable EEE advertisement
> 
>  drivers/net/dsa/mt7530.c      | 7 +++++++
>  drivers/net/dsa/mt7530.h      | 7 ++++++-
>  drivers/net/phy/mediatek-ge.c | 3 ---
>  3 files changed, 13 insertions(+), 4 deletions(-)
> ---
> base-commit: ea80e3ed09ab2c2b75724faf5484721753e92c31
> change-id: 20240317-for-net-mt7530-fix-eee-for-mt7531-mt7988-a5c5453cc0e8
> 
> Best regards,
> -- 
> Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> 

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

[Daniel Golle]

On Mon, Mar 18, 2024 at 10:46:22AM +0300, Arınç ÜNAL via B4 Relay wrote:
> Hi.
> 
> This patch series fixes EEE support for MT7531 and the switch on the MT7988
> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
> what's the status on MT7988 SoC switch as I don't have the hardware.

EEE seems to already work just fine on the MT7988 built-in switch, at least
on the BPI-R4. I don't think the SoC has bootstrap pins related to EEE like
stand-alone MT753x may have.

root@bpi-r4:~# ethtool --show-eee lan1
EEE settings for lan1:
    EEE status: disabled
    Tx LPI: 30 (us)
    Supported EEE link modes:  100baseT/Full
                               1000baseT/Full
    Advertised EEE link modes:  Not reported
    Link partner advertised EEE link modes:  100baseT/Full
                                             1000baseT/Full

root@bpi-r4:~# ethtool --set-eee lan1 eee on
root@bpi-r4:~# ethtool --show-eee lan1
EEE settings for lan1:
    EEE status: enabled - inactive
    Tx LPI: 30 (us)
    Supported EEE link modes:  100baseT/Full
                               1000baseT/Full
    Advertised EEE link modes:  100baseT/Full
                                1000baseT/Full
    Link partner advertised EEE link modes:  Not reported
root@bpi-r4:~# ethtool --show-eee lan1
EEE settings for lan1:
    EEE status: enabled - active
    Tx LPI: 30 (us)
    Supported EEE link modes:  100baseT/Full
                               1000baseT/Full
    Advertised EEE link modes:  100baseT/Full
                                1000baseT/Full
    Link partner advertised EEE link modes:  100baseT/Full
                                             1000baseT/Full

So don't fix if it ain't broken maybe...?

> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> Arınç ÜNAL (3):
>       net: dsa: mt7530: fix enabling EEE on MT7531 switch on all boards
>       net: dsa: mt7530: fix disabling EEE on failure on MT7531 and MT7988
>       net: phy: mediatek-ge: do not disable EEE advertisement
> 
>  drivers/net/dsa/mt7530.c      | 7 +++++++
>  drivers/net/dsa/mt7530.h      | 7 ++++++-
>  drivers/net/phy/mediatek-ge.c | 3 ---
>  3 files changed, 13 insertions(+), 4 deletions(-)
> ---
> base-commit: ea80e3ed09ab2c2b75724faf5484721753e92c31
> change-id: 20240317-for-net-mt7530-fix-eee-for-mt7531-mt7988-a5c5453cc0e8
> 
> Best regards,
> -- 
> Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 19.03.2024 21:25, Daniel Golle wrote:
> On Mon, Mar 18, 2024 at 10:46:22AM +0300, Arınç ÜNAL via B4 Relay wrote:
>> Hi.
>>
>> This patch series fixes EEE support for MT7531 and the switch on the MT7988
>> SoC. EEE did not work on MT7531 on most boards before this, it is unclear
>> what's the status on MT7988 SoC switch as I don't have the hardware.
> 
> EEE seems to already work just fine on the MT7988 built-in switch, at least
> on the BPI-R4. I don't think the SoC has bootstrap pins related to EEE like
> stand-alone MT753x may have.
> 
> root@bpi-r4:~# ethtool --show-eee lan1
> EEE settings for lan1:
>      EEE status: disabled
>      Tx LPI: 30 (us)
>      Supported EEE link modes:  100baseT/Full
>                                 1000baseT/Full
>      Advertised EEE link modes:  Not reported
>      Link partner advertised EEE link modes:  100baseT/Full
>                                               1000baseT/Full
> 
> root@bpi-r4:~# ethtool --set-eee lan1 eee on
> root@bpi-r4:~# ethtool --show-eee lan1
> EEE settings for lan1:
>      EEE status: enabled - inactive
>      Tx LPI: 30 (us)
>      Supported EEE link modes:  100baseT/Full
>                                 1000baseT/Full
>      Advertised EEE link modes:  100baseT/Full
>                                  1000baseT/Full
>      Link partner advertised EEE link modes:  Not reported
> root@bpi-r4:~# ethtool --show-eee lan1
> EEE settings for lan1:
>      EEE status: enabled - active
>      Tx LPI: 30 (us)
>      Supported EEE link modes:  100baseT/Full
>                                 1000baseT/Full
>      Advertised EEE link modes:  100baseT/Full
>                                  1000baseT/Full
>      Link partner advertised EEE link modes:  100baseT/Full
>                                               1000baseT/Full
> 
> So don't fix if it ain't broken maybe...?

I would argue that EEE advertisement on the PHY should be enabled by
default. I guess we're supposed to supply that on the PHY driver. Can you
test with this diff applied and see if it works without manually enabling
EEE using ethtool?

diff --git a/drivers/net/phy/mediatek-ge-soc.c b/drivers/net/phy/mediatek-ge-soc.c
index 0f3a1538a8b8..5f482c12018a 100644
--- a/drivers/net/phy/mediatek-ge-soc.c
+++ b/drivers/net/phy/mediatek-ge-soc.c
@@ -978,6 +978,9 @@ static void mt798x_phy_eee(struct phy_device *phydev)
  		       MTK_PHY_RG_LPI_PCS_DSP_CTRL_REG122,
  		       MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK,
  		       FIELD_PREP(MTK_PHY_LPI_NORM_MSE_HI_THRESH1000_MASK, 0xff));
+
+	phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_EEE_ADV, MDIO_EEE_100TX |
+		      MDIO_EEE_1000T);
  }
  
  static int cal_sw(struct phy_device *phydev, enum CAL_ITEM cal_item,

Arınç

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

[Andrew Lunn]

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

The current phylib core code leaves the PHY advertisement whatever its
reset default is. So we leave it to the manufacture to decide if it
should be enabled or disabled by default. It is policy, so it should
really be down to user space to configure EEE how it wants it.

       Andrew

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

[Andrew Lunn]

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

The current phylib core code leaves the PHY advertisement whatever its
reset default is. So we leave it to the manufacture to decide if it
should be enabled or disabled by default. It is policy, so it should
really be down to user space to configure EEE how it wants it.

       Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 19.03.2024 22:38, 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.
> 
> The current phylib core code leaves the PHY advertisement whatever its
> reset default is. So we leave it to the manufacture to decide if it
> should be enabled or disabled by default. It is policy, so it should
> really be down to user space to configure EEE how it wants it.

That's fine by me. Then my patch series is okay as it is.

Arınç

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

[Daniel Golle]

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


> 
> The current phylib core code leaves the PHY advertisement whatever its
> reset default is. So we leave it to the manufacture to decide if it
> should be enabled or disabled by default. It is policy, so it should
> really be down to user space to configure EEE how it wants it.

I very much agree with that policy, changing the default definitely
feels like something which could affect quite a lot of people and
should be done very carefully, if at all.

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

[Daniel Golle]

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


> 
> The current phylib core code leaves the PHY advertisement whatever its
> reset default is. So we leave it to the manufacture to decide if it
> should be enabled or disabled by default. It is policy, so it should
> really be down to user space to configure EEE how it wants it.

I very much agree with that policy, changing the default definitely
feels like something which could affect quite a lot of people and
should be done very carefully, if at all.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 19.03.2024 23: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

Are you saying this to indicate that we shouldn't remove that from
mediatek-ge? If so, I've already explained that there'd be no practical
change in removing it as both MT7530 and MT7531 switches enable EEE
advertisement after mediatek-ge.

Arınç

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

[Florian Fainelli]

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.

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.
-- 
Florian

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

[Florian Fainelli]

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.

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.
-- 
Florian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

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ç

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

[Florian Fainelli]

On 3/21/24 09:09, Arınç ÜNAL wrote:
> 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

OK, so this is intended to describe that the SoC's Ethernet MAC link to 
the integrated switch did not use EEE only the user-facing ports. That 
makes sense because it's all digital logic and you are not going to be 
seeing much power saving from having EEE enabled between the SoC's 
Ethernet MAC and CPU port of the switch, that said, however, I wonder if 
this has an impact on any form of flow control within the switch that is 
reacting to LPI and you need EEE to be enabled end-to-end?

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

That would point to an issue getting in and out of LPI, do you see these 
packet losses even with different LPI timeouts?

> 
> # 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ç

-- 
Florian

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

[Florian Fainelli]

On 3/21/24 09:09, Arınç ÜNAL wrote:
> 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

OK, so this is intended to describe that the SoC's Ethernet MAC link to 
the integrated switch did not use EEE only the user-facing ports. That 
makes sense because it's all digital logic and you are not going to be 
seeing much power saving from having EEE enabled between the SoC's 
Ethernet MAC and CPU port of the switch, that said, however, I wonder if 
this has an impact on any form of flow control within the switch that is 
reacting to LPI and you need EEE to be enabled end-to-end?

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

That would point to an issue getting in and out of LPI, do you see these 
packet losses even with different LPI timeouts?

> 
> # 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ç

-- 
Florian


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 21/03/2024 18:31, Florian Fainelli wrote:
> On 3/21/24 09:09, Arınç ÜNAL wrote:
>> 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
> 
> OK, so this is intended to describe that the SoC's Ethernet MAC link to the integrated switch did not use EEE only the user-facing ports. That makes sense because it's all digital logic and you are not going to be seeing much power saving from having EEE enabled between the SoC's Ethernet MAC and CPU port of the switch, that said, however, I wonder if this has an impact on any form of flow control within the switch that is reacting to LPI and you need EEE to be enabled end-to-end?

I've tested pinging between my computers with EEE enabled interfaces. The
behaviour is identical.

> 
>>
>> 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.
> 
> That would point to an issue getting in and out of LPI, do you see these packet losses even with different LPI timeouts?

The NICs on my computers don't seem to allow changing the tx-lpi and
tx-timer options.

Computer 1 (Intel I219-V, driver: e1000e):

$ sudo ethtool --set-eee eno1 tx-timer 15
netlink error: Invalid argument

$ sudo ethtool --show-eee eno1
EEE settings for eno1:
	EEE status: enabled - active
	Tx LPI: 17 (us)
	Supported EEE link modes:  100baseT/Full
	                           1000baseT/Full
	Advertised EEE link modes:  100baseT/Full
	                            1000baseT/Full
	Link partner advertised EEE link modes:  100baseT/Full
	                                         1000baseT/Full

Computer 2 (Realtek RTL8111H, driver: r8169):

$ sudo ethtool --set-eee eno1 tx-lpi on

$ sudo ethtool --show-eee eno1
EEE settings for eno1:
	EEE status: enabled - active
	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:  100baseT/Full
	                                         1000baseT/Full

I've tested with switch ports interfaces' tx-timer from 0 to 40, same
tx-timer for both interfaces. Loss is still there.

I suppose the MT7531 switch PHYs need calibration for EEE that is currently
missing from the mediatek-ge driver.

Arınç

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

[Russell King (Oracle)]

On Sun, Mar 24, 2024 at 12:47:08PM +0300, Arınç ÜNAL wrote:
> On 21/03/2024 18:31, Florian Fainelli wrote:
> > On 3/21/24 09:09, Arınç ÜNAL wrote:
> > > 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
> > 
> > OK, so this is intended to describe that the SoC's Ethernet MAC link to the integrated switch did not use EEE only the user-facing ports. That makes sense because it's all digital logic and you are not going to be seeing much power saving from having EEE enabled between the SoC's Ethernet MAC and CPU port of the switch, that said, however, I wonder if this has an impact on any form of flow control within the switch that is reacting to LPI and you need EEE to be enabled end-to-end?
> 
> I've tested pinging between my computers with EEE enabled interfaces. The
> behaviour is identical.
> 
> > 
> > > 
> > > 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.
> > 
> > That would point to an issue getting in and out of LPI, do you see these packet losses even with different LPI timeouts?
> 
> The NICs on my computers don't seem to allow changing the tx-lpi and
> tx-timer options.
> 
> Computer 1 (Intel I219-V, driver: e1000e):
> 
> $ sudo ethtool --set-eee eno1 tx-timer 15
> netlink error: Invalid argument
> 
> $ sudo ethtool --show-eee eno1
> EEE settings for eno1:
> 	EEE status: enabled - active
> 	Tx LPI: 17 (us)
> 	Supported EEE link modes:  100baseT/Full
> 	                           1000baseT/Full
> 	Advertised EEE link modes:  100baseT/Full
> 	                            1000baseT/Full
> 	Link partner advertised EEE link modes:  100baseT/Full
> 	                                         1000baseT/Full
> 
> Computer 2 (Realtek RTL8111H, driver: r8169):
> 
> $ sudo ethtool --set-eee eno1 tx-lpi on
> 
> $ sudo ethtool --show-eee eno1
> EEE settings for eno1:
> 	EEE status: enabled - active
> 	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:  100baseT/Full
> 	                                         1000baseT/Full
> 
> I've tested with switch ports interfaces' tx-timer from 0 to 40, same
> tx-timer for both interfaces. Loss is still there.

EEE implementations tend to be a mess in the way drivers implement the
API, so one can't at the moment rely on what ethtool says about the
status. Sadly, this is what happens when driver authors are left to
their own ends. :(

> I suppose the MT7531 switch PHYs need calibration for EEE that is currently
> missing from the mediatek-ge driver.

EEE is quite simple from the software point of view. There is software
negotiation of the modules that EEE supports, and then there is are
one or more timers that affect the behaviour of EEE. The LPI timer is
"how long the link needs to be idle for before _this_ end signals that
it _can_ enter low power state". The link only enters low power state
when *both* ends of the link signal that they can enter low power
state.

What calibration would be necessary?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Sun, Mar 24, 2024 at 12:47:08PM +0300, Arınç ÜNAL wrote:
> On 21/03/2024 18:31, Florian Fainelli wrote:
> > On 3/21/24 09:09, Arınç ÜNAL wrote:
> > > 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
> > 
> > OK, so this is intended to describe that the SoC's Ethernet MAC link to the integrated switch did not use EEE only the user-facing ports. That makes sense because it's all digital logic and you are not going to be seeing much power saving from having EEE enabled between the SoC's Ethernet MAC and CPU port of the switch, that said, however, I wonder if this has an impact on any form of flow control within the switch that is reacting to LPI and you need EEE to be enabled end-to-end?
> 
> I've tested pinging between my computers with EEE enabled interfaces. The
> behaviour is identical.
> 
> > 
> > > 
> > > 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.
> > 
> > That would point to an issue getting in and out of LPI, do you see these packet losses even with different LPI timeouts?
> 
> The NICs on my computers don't seem to allow changing the tx-lpi and
> tx-timer options.
> 
> Computer 1 (Intel I219-V, driver: e1000e):
> 
> $ sudo ethtool --set-eee eno1 tx-timer 15
> netlink error: Invalid argument
> 
> $ sudo ethtool --show-eee eno1
> EEE settings for eno1:
> 	EEE status: enabled - active
> 	Tx LPI: 17 (us)
> 	Supported EEE link modes:  100baseT/Full
> 	                           1000baseT/Full
> 	Advertised EEE link modes:  100baseT/Full
> 	                            1000baseT/Full
> 	Link partner advertised EEE link modes:  100baseT/Full
> 	                                         1000baseT/Full
> 
> Computer 2 (Realtek RTL8111H, driver: r8169):
> 
> $ sudo ethtool --set-eee eno1 tx-lpi on
> 
> $ sudo ethtool --show-eee eno1
> EEE settings for eno1:
> 	EEE status: enabled - active
> 	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:  100baseT/Full
> 	                                         1000baseT/Full
> 
> I've tested with switch ports interfaces' tx-timer from 0 to 40, same
> tx-timer for both interfaces. Loss is still there.

EEE implementations tend to be a mess in the way drivers implement the
API, so one can't at the moment rely on what ethtool says about the
status. Sadly, this is what happens when driver authors are left to
their own ends. :(

> I suppose the MT7531 switch PHYs need calibration for EEE that is currently
> missing from the mediatek-ge driver.

EEE is quite simple from the software point of view. There is software
negotiation of the modules that EEE supports, and then there is are
one or more timers that affect the behaviour of EEE. The LPI timer is
"how long the link needs to be idle for before _this_ end signals that
it _can_ enter low power state". The link only enters low power state
when *both* ends of the link signal that they can enter low power
state.

What calibration would be necessary?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arınç ÜNAL]

On 24.03.2024 14:39, Russell King (Oracle) wrote:
> On Sun, Mar 24, 2024 at 12:47:08PM +0300, Arınç ÜNAL wrote:
>> I've tested with switch ports interfaces' tx-timer from 0 to 40, same
>> tx-timer for both interfaces. Loss is still there.
> 
> EEE implementations tend to be a mess in the way drivers implement the
> API, so one can't at the moment rely on what ethtool says about the
> status. Sadly, this is what happens when driver authors are left to
> their own ends. :(
> 
>> I suppose the MT7531 switch PHYs need calibration for EEE that is currently
>> missing from the mediatek-ge driver.
> 
> EEE is quite simple from the software point of view. There is software
> negotiation of the modules that EEE supports, and then there is are
> one or more timers that affect the behaviour of EEE. The LPI timer is
> "how long the link needs to be idle for before _this_ end signals that
> it _can_ enter low power state". The link only enters low power state
> when *both* ends of the link signal that they can enter low power
> state.
> 
> What calibration would be necessary?

Check out mt798x_phy_eee() on drivers/net/phy/mediatek-ge-soc.c.

Arınç