[PATCH net-next v2 0/7] MT7530 DSA Subdriver Improvements Act II

[PATCH net-next v2 0/7] MT7530 DSA Subdriver Improvements Act II

[Arınç ÜNAL via B4 Relay]

Hello!

This is the second patch series with the goal of simplifying the MT7530 DSA
subdriver and improving support for MT7530, MT7531, and the switch on the
MT7988 SoC.

I have done a simple ping test to confirm basic communication on all switch
ports on MCM and standalone MT7530, and MT7531 switch with this patch
series applied.

MT7621 Unielec, MCM MT7530:

rgmii-only-gmac0-mt7621-unielec-u7621-06-16m.dtb
gmac0-and-gmac1-mt7621-unielec-u7621-06-16m.dtb

tftpboot 0x80008000 mips-uzImage.bin; tftpboot 0x83000000 mips-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000

MT7622 Bananapi, MT7531:

gmac0-and-gmac1-mt7622-bananapi-bpi-r64.dtb

tftpboot 0x40000000 arm64-Image; tftpboot 0x45000000 arm64-rootfs.cpio.uboot; tftpboot 0x4a000000 $dtb; booti 0x40000000 0x45000000 0x4a000000

MT7623 Bananapi, standalone MT7530:

rgmii-only-gmac0-mt7623n-bananapi-bpi-r2.dtb
gmac0-and-gmac1-mt7623n-bananapi-bpi-r2.dtb

tftpboot 0x80008000 arm-zImage; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootz 0x80008000 0x83000000 0x83f00000

This patch series is the continuation of the patch series linked below.

https://lore.kernel.org/r/20230522121532.86610-1-arinc.unal@arinc9.com

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
Changes in v2:
- Update the patches with the latest received trailers.
- Remove 'net: dsa: mt7530: move enabling port 6 to mt7530_setup_port6()'
  which was patch 5. I will bring a more appropriate change with a later
  patch series.
- Patch 5
  - Set P6_INTF_MODE(0) and explain why on the patch log.
- Patch 6
  - Mention the MT7988 document and explain more on the patch log.
- Patch 7
  - Explain more on the patch log.
- Link to v1: https://lore.kernel.org/r/20240113102529.80371-1-arinc.unal@arinc9.com

---
Arınç ÜNAL (7):
      net: dsa: mt7530: empty default case on mt7530_setup_port5()
      net: dsa: mt7530: call port 6 setup from mt7530_mac_config()
      net: dsa: mt7530: remove pad_setup function pointer
      net: dsa: mt7530: move XTAL check to mt7530_setup()
      net: dsa: mt7530: simplify mt7530_setup_port6() and change to void
      net: dsa: mt7530: correct port capabilities of MT7988
      net: dsa: mt7530: do not clear config->supported_interfaces

 drivers/net/dsa/mt7530.c | 154 +++++++++++++++++------------------------------
 drivers/net/dsa/mt7530.h |   3 -
 2 files changed, 54 insertions(+), 103 deletions(-)
---
base-commit: 4acf4e62cd572b0c806035046b3698f5585ab821
change-id: 20240121-for-netnext-mt7530-improvements-2-b4f43661b485

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

[PATCH net-next v2 1/7] net: dsa: mt7530: empty default case on mt7530_setup_port5()

[Arınç ÜNAL via B4 Relay]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

There're two code paths for setting up port 5:

mt7530_setup()
-> mt7530_setup_port5()

mt753x_phylink_mac_config()
-> mt753x_mac_config()
   -> mt7530_mac_config()
      -> mt7530_setup_port5()

On the first code path, priv->p5_intf_sel is either set to
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4 when mt7530_setup_port5() is run.

On the second code path, priv->p5_intf_sel is set to P5_INTF_SEL_GMAC5 when
mt7530_setup_port5() is run.

Empty the default case which will never run but is needed nonetheless to
handle all the remaining enumeration values.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 68be38ae66e0..330e22abc076 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -943,9 +943,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 		val &= ~MHWTRAP_P5_DIS;
 		break;
 	default:
-		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
-			priv->p5_intf_sel);
-		goto unlock_exit;
+		break;
 	}
 
 	/* Setup RGMII settings */
@@ -975,7 +973,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, intf_sel=%s, phy-mode=%s\n",
 		val, p5_intf_modes(priv->p5_intf_sel), phy_modes(interface));
 
-unlock_exit:
 	mutex_unlock(&priv->reg_mutex);
 }
 

-- 
2.40.1

[PATCH net-next v2 2/7] net: dsa: mt7530: call port 6 setup from mt7530_mac_config()

[Arınç ÜNAL via B4 Relay]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

mt7530_pad_clk_setup() is called if port 6 is enabled. It used to do more
things than setting up port 6. That part was moved to more appropriate
locations, mt7530_setup() and mt7530_pll_setup().

Now that all it does is set up port 6, rename it to mt7530_setup_port6(),
and move it to a more appropriate location, under mt7530_mac_config().

Leave an empty mt7530_pad_clk_setup() to satisfy the pad_setup function
pointer.

This is the code path for setting up the ports before:

mt753x_phylink_mac_config()
-> mt753x_mac_config()
   -> mt7530_mac_config()
      -> mt7530_setup_port5()
-> mt753x_pad_setup()
   -> mt7530_pad_clk_setup()

This is after:

mt753x_phylink_mac_config()
-> mt753x_mac_config()
   -> mt7530_mac_config()
      -> mt7530_setup_port5()
      -> mt7530_setup_port6()

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 330e22abc076..2d468a5f2e70 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -415,7 +415,7 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
 
 /* Setup port 6 interface mode and TRGMII TX circuit */
 static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, xtal;
@@ -487,6 +487,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 	return 0;
 }
 
+static int
+mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+{
+	return 0;
+}
+
 static int
 mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
@@ -2624,12 +2630,15 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
+	int ret;
 
-	/* Only need to setup port5. */
-	if (port != 5)
-		return 0;
-
-	mt7530_setup_port5(priv->ds, interface);
+	if (port == 5) {
+		mt7530_setup_port5(priv->ds, interface);
+	} else if (port == 6) {
+		ret = mt7530_setup_port6(priv->ds, interface);
+		if (ret)
+			return ret;
+	}
 
 	return 0;
 }

-- 
2.40.1

[PATCH net-next v2 3/7] net: dsa: mt7530: remove pad_setup function pointer

[Arınç ÜNAL via B4 Relay]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

The pad_setup function pointer was introduced with 88bdef8be9f6 ("net: dsa:
mt7530: Extend device data ready for adding a new hardware"). It was being
used to set up the core clock and port 6 of the MT7530 switch, and pll of
the MT7531 switch.

All of these were moved to more appropriate locations, and it was never
used for the switch on the MT7988 SoC. Therefore, this function pointer
hasn't got a use anymore. Remove it.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 36 ++----------------------------------
 drivers/net/dsa/mt7530.h |  3 ---
 2 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 2d468a5f2e70..fdaf65b58b72 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -487,18 +487,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 	return 0;
 }
 
-static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
-	return 0;
-}
-
-static int
-mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
-	return 0;
-}
-
 static void
 mt7531_pll_setup(struct mt7530_priv *priv)
 {
@@ -2617,14 +2605,6 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 	}
 }
 
-static int
-mt753x_pad_setup(struct dsa_switch *ds, const struct phylink_link_state *state)
-{
-	struct mt7530_priv *priv = ds->priv;
-
-	return priv->info->pad_setup(ds, state->interface);
-}
-
 static int
 mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
@@ -2794,8 +2774,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p6_interface == state->interface)
 			break;
 
-		mt753x_pad_setup(ds, state);
-
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
 
@@ -3113,11 +3091,6 @@ mt753x_conduit_state_change(struct dsa_switch *ds,
 	mt7530_rmw(priv, MT7530_MFC, CPU_EN | CPU_PORT_MASK, val);
 }
 
-static int mt7988_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
-{
-	return 0;
-}
-
 static int mt7988_setup(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
@@ -3181,7 +3154,6 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c22 = mt7530_phy_write_c22,
 		.phy_read_c45 = mt7530_phy_read_c45,
 		.phy_write_c45 = mt7530_phy_write_c45,
-		.pad_setup = mt7530_pad_clk_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
@@ -3193,7 +3165,6 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c22 = mt7530_phy_write_c22,
 		.phy_read_c45 = mt7530_phy_read_c45,
 		.phy_write_c45 = mt7530_phy_write_c45,
-		.pad_setup = mt7530_pad_clk_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
@@ -3205,7 +3176,6 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c22 = mt7531_ind_c22_phy_write,
 		.phy_read_c45 = mt7531_ind_c45_phy_read,
 		.phy_write_c45 = mt7531_ind_c45_phy_write,
-		.pad_setup = mt7531_pad_setup,
 		.cpu_port_config = mt7531_cpu_port_config,
 		.mac_port_get_caps = mt7531_mac_port_get_caps,
 		.mac_port_config = mt7531_mac_config,
@@ -3218,7 +3188,6 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c22 = mt7531_ind_c22_phy_write,
 		.phy_read_c45 = mt7531_ind_c45_phy_read,
 		.phy_write_c45 = mt7531_ind_c45_phy_write,
-		.pad_setup = mt7988_pad_setup,
 		.cpu_port_config = mt7988_cpu_port_config,
 		.mac_port_get_caps = mt7988_mac_port_get_caps,
 		.mac_port_config = mt7988_mac_config,
@@ -3248,9 +3217,8 @@ mt7530_probe_common(struct mt7530_priv *priv)
 	/* Sanity check if these required device operations are filled
 	 * properly.
 	 */
-	if (!priv->info->sw_setup || !priv->info->pad_setup ||
-	    !priv->info->phy_read_c22 || !priv->info->phy_write_c22 ||
-	    !priv->info->mac_port_get_caps ||
+	if (!priv->info->sw_setup || !priv->info->phy_read_c22 ||
+	    !priv->info->phy_write_c22 || !priv->info->mac_port_get_caps ||
 	    !priv->info->mac_port_config)
 		return -EINVAL;
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 80060cc740d2..26a6d2160c08 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -704,8 +704,6 @@ struct mt753x_pcs {
  * @phy_write_c22:	Holding the way writing PHY port using C22
  * @phy_read_c45:	Holding the way reading PHY port using C45
  * @phy_write_c45:	Holding the way writing PHY port using C45
- * @pad_setup:		Holding the way setting up the bus pad for a certain
- *			MAC port
  * @phy_mode_supported:	Check if the PHY type is being supported on a certain
  *			port
  * @mac_port_validate:	Holding the way to set addition validate type for a
@@ -726,7 +724,6 @@ struct mt753x_info {
 			    int regnum);
 	int (*phy_write_c45)(struct mt7530_priv *priv, int port, int devad,
 			     int regnum, u16 val);
-	int (*pad_setup)(struct dsa_switch *ds, phy_interface_t interface);
 	int (*cpu_port_config)(struct dsa_switch *ds, int port);
 	void (*mac_port_get_caps)(struct dsa_switch *ds, int port,
 				  struct phylink_config *config);

-- 
2.40.1

[PATCH net-next v2 4/7] net: dsa: mt7530: move XTAL check to mt7530_setup()

[Arınç ÜNAL via B4 Relay]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

The crystal frequency concerns the switch core. The frequency should be
checked when the switch is being set up so the driver can reject the
unsupported hardware earlier and without requiring port 6 to be used.

Move it to mt7530_setup(). Drop the unnecessary function printing.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index fdaf65b58b72..c4d492e29fdf 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -422,13 +422,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
 
-	if (xtal == HWTRAP_XTAL_20MHZ) {
-		dev_err(priv->dev,
-			"%s: MT7530 with a 20MHz XTAL is not supported!\n",
-			__func__);
-		return -EINVAL;
-	}
-
 	switch (interface) {
 	case PHY_INTERFACE_MODE_RGMII:
 		trgint = 0;
@@ -2253,6 +2246,12 @@ mt7530_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	if ((val & HWTRAP_XTAL_MASK) == HWTRAP_XTAL_20MHZ) {
+		dev_err(priv->dev,
+			"MT7530 with a 20MHz XTAL is not supported!\n");
+		return -EINVAL;
+	}
+
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |

-- 
2.40.1

[PATCH net-next v2 5/7] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL via B4 Relay]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

This code is from before this driver was converted to phylink API. Phylink
deals with the unsupported interface cases before mt7530_setup_port6() is
run. Therefore, the default case would never run. However, it must be
defined nonetheless to handle all the remaining enumeration values, the
phy-modes.

Switch to if statement for RGMII and return which simplifies the code and
saves an indent.

Set P6_INTF_MODE, which is the the three least significant bits of the
MT7530_P6ECR register, to 0 for RGMII even though it will already be 0
after reset. This is to keep supporting dynamic reconfiguration of the port
in the case the interface changes from TRGMII to RGMII. The core operations
for TRGMII does not interfere with RGMII so no need to undo them.

Read XTAL after checking for RGMII as it's only needed for the TRGMII
interface mode.

Change mt7530_setup_port6() to void now that there're no error cases left.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 103 ++++++++++++++++++++---------------------------
 1 file changed, 43 insertions(+), 60 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index c4d492e29fdf..36dc2bbcf3b6 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -414,70 +414,57 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
 }
 
 /* Setup port 6 interface mode and TRGMII TX circuit */
-static int
+static void
 mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
-	u32 ncpo1, ssc_delta, trgint, xtal;
-
-	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
+	u32 ncpo1, ssc_delta, xtal;
 
-	switch (interface) {
-	case PHY_INTERFACE_MODE_RGMII:
-		trgint = 0;
-		break;
-	case PHY_INTERFACE_MODE_TRGMII:
-		trgint = 1;
-		if (xtal == HWTRAP_XTAL_25MHZ)
-			ssc_delta = 0x57;
-		else
-			ssc_delta = 0x87;
-		if (priv->id == ID_MT7621) {
-			/* PLL frequency: 125MHz: 1.0GBit */
-			if (xtal == HWTRAP_XTAL_40MHZ)
-				ncpo1 = 0x0640;
-			if (xtal == HWTRAP_XTAL_25MHZ)
-				ncpo1 = 0x0a00;
-		} else { /* PLL frequency: 250MHz: 2.0Gbit */
-			if (xtal == HWTRAP_XTAL_40MHZ)
-				ncpo1 = 0x0c80;
-			if (xtal == HWTRAP_XTAL_25MHZ)
-				ncpo1 = 0x1400;
-		}
-		break;
-	default:
-		dev_err(priv->dev, "xMII interface %d not supported\n",
-			interface);
-		return -EINVAL;
+	if (interface == PHY_INTERFACE_MODE_RGMII) {
+		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
+			   P6_INTF_MODE(0));
+		return;
 	}
 
-	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
-		   P6_INTF_MODE(trgint));
+	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));
 
-	if (trgint) {
-		/* Disable the MT7530 TRGMII clocks */
-		core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
+	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
 
-		/* Setup the MT7530 TRGMII Tx Clock */
-		core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
-		core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
-		core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
-		core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
-		core_write(priv, CORE_PLL_GROUP4,
-			   RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
-			   RG_SYSPLL_BIAS_LPF_EN);
-		core_write(priv, CORE_PLL_GROUP2,
-			   RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
-			   RG_SYSPLL_POSDIV(1));
-		core_write(priv, CORE_PLL_GROUP7,
-			   RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
-			   RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
+	if (xtal == HWTRAP_XTAL_25MHZ)
+		ssc_delta = 0x57;
+	else
+		ssc_delta = 0x87;
 
-		/* Enable the MT7530 TRGMII clocks */
-		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
+	if (priv->id == ID_MT7621) {
+		/* PLL frequency: 125MHz: 1.0GBit */
+		if (xtal == HWTRAP_XTAL_40MHZ)
+			ncpo1 = 0x0640;
+		if (xtal == HWTRAP_XTAL_25MHZ)
+			ncpo1 = 0x0a00;
+	} else { /* PLL frequency: 250MHz: 2.0Gbit */
+		if (xtal == HWTRAP_XTAL_40MHZ)
+			ncpo1 = 0x0c80;
+		if (xtal == HWTRAP_XTAL_25MHZ)
+			ncpo1 = 0x1400;
 	}
 
-	return 0;
+	/* Disable the MT7530 TRGMII clocks */
+	core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
+
+	/* Setup the MT7530 TRGMII Tx Clock */
+	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
+	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
+	core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
+	core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
+	core_write(priv, CORE_PLL_GROUP4, RG_SYSPLL_DDSFBK_EN |
+		   RG_SYSPLL_BIAS_EN | RG_SYSPLL_BIAS_LPF_EN);
+	core_write(priv, CORE_PLL_GROUP2, RG_SYSPLL_EN_NORMAL |
+		   RG_SYSPLL_VODEN | RG_SYSPLL_POSDIV(1));
+	core_write(priv, CORE_PLL_GROUP7, RG_LCDDS_PCW_NCPO_CHG |
+		   RG_LCCDS_C(3) | RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
+
+	/* Enable the MT7530 TRGMII clocks */
+	core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
 }
 
 static void
@@ -2609,15 +2596,11 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
-	int ret;
 
-	if (port == 5) {
+	if (port == 5)
 		mt7530_setup_port5(priv->ds, interface);
-	} else if (port == 6) {
-		ret = mt7530_setup_port6(priv->ds, interface);
-		if (ret)
-			return ret;
-	}
+	else if (port == 6)
+		mt7530_setup_port6(priv->ds, interface);
 
 	return 0;
 }

-- 
2.40.1

Re: [PATCH net-next v2 5/7] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Daniel Golle]

On Tue, Jan 30, 2024 at 06:20:51PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> This code is from before this driver was converted to phylink API. Phylink
> deals with the unsupported interface cases before mt7530_setup_port6() is
> run. Therefore, the default case would never run. However, it must be
> defined nonetheless to handle all the remaining enumeration values, the
> phy-modes.
> 
> Switch to if statement for RGMII and return which simplifies the code and
> saves an indent.
> 
> Set P6_INTF_MODE, which is the the three least significant bits of the
> MT7530_P6ECR register, to 0 for RGMII even though it will already be 0
> after reset. This is to keep supporting dynamic reconfiguration of the port
> in the case the interface changes from TRGMII to RGMII. The core operations
> for TRGMII does not interfere with RGMII so no need to undo them.

That last sentence doesn't parse English gramar.
"operations": plural
"does": singular

Should probably be either "The core operation for TRGMII does not..."
or "The core operations for TRGMII do not..."

As you are mentioning it, I'm now curious if you consider to
dynamically reconfiguring TRGIII<->RGMII on port 6 depending on
whether there is more then 1 GBit/s possible bandwidth needed between
port 6 and the remaining ports? That could make sense for power
management, but then we should at least again switch off the TRGMII
clocks in the RGMII case before returning, see my suggestion inline
below.

> 
> Read XTAL after checking for RGMII as it's only needed for the TRGMII
> interface mode.
> 
> Change mt7530_setup_port6() to void now that there're no error cases left.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

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

> ---
>  drivers/net/dsa/mt7530.c | 103 ++++++++++++++++++++---------------------------
>  1 file changed, 43 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index c4d492e29fdf..36dc2bbcf3b6 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -414,70 +414,57 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
>  }
>  
>  /* Setup port 6 interface mode and TRGMII TX circuit */
> -static int
> +static void
>  mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>  {
>  	struct mt7530_priv *priv = ds->priv;
> -	u32 ncpo1, ssc_delta, trgint, xtal;
> -
> -	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> +	u32 ncpo1, ssc_delta, xtal;
>  
> -	switch (interface) {
> -	case PHY_INTERFACE_MODE_RGMII:
> -		trgint = 0;
> -		break;
> -	case PHY_INTERFACE_MODE_TRGMII:
> -		trgint = 1;
> -		if (xtal == HWTRAP_XTAL_25MHZ)
> -			ssc_delta = 0x57;
> -		else
> -			ssc_delta = 0x87;
> -		if (priv->id == ID_MT7621) {
> -			/* PLL frequency: 125MHz: 1.0GBit */
> -			if (xtal == HWTRAP_XTAL_40MHZ)
> -				ncpo1 = 0x0640;
> -			if (xtal == HWTRAP_XTAL_25MHZ)
> -				ncpo1 = 0x0a00;
> -		} else { /* PLL frequency: 250MHz: 2.0Gbit */
> -			if (xtal == HWTRAP_XTAL_40MHZ)
> -				ncpo1 = 0x0c80;
> -			if (xtal == HWTRAP_XTAL_25MHZ)
> -				ncpo1 = 0x1400;
> -		}
> -		break;
> -	default:
> -		dev_err(priv->dev, "xMII interface %d not supported\n",
> -			interface);
> -		return -EINVAL;
> +	if (interface == PHY_INTERFACE_MODE_RGMII) {
> +		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
> +			   P6_INTF_MODE(0));

Maybe at least switch off TRGMIICK here because we are sure we don't need it
in the RGMII case, ie:
core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);

And that then is another line of code already present just below which
means you could keep variable trgint as it was and return after
switching off TRGMIICK below anyway...

> +		return;
>  	}
>  
> -	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
> -		   P6_INTF_MODE(trgint));
> +	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));
>  
> -	if (trgint) {
> -		/* Disable the MT7530 TRGMII clocks */
> -		core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
> +	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
>  
> -		/* Setup the MT7530 TRGMII Tx Clock */
> -		core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
> -		core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> -		core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
> -		core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
> -		core_write(priv, CORE_PLL_GROUP4,
> -			   RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
> -			   RG_SYSPLL_BIAS_LPF_EN);
> -		core_write(priv, CORE_PLL_GROUP2,
> -			   RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
> -			   RG_SYSPLL_POSDIV(1));
> -		core_write(priv, CORE_PLL_GROUP7,
> -			   RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
> -			   RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
> +	if (xtal == HWTRAP_XTAL_25MHZ)
> +		ssc_delta = 0x57;
> +	else
> +		ssc_delta = 0x87;
>  
> -		/* Enable the MT7530 TRGMII clocks */
> -		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
> +	if (priv->id == ID_MT7621) {
> +		/* PLL frequency: 125MHz: 1.0GBit */
> +		if (xtal == HWTRAP_XTAL_40MHZ)
> +			ncpo1 = 0x0640;
> +		if (xtal == HWTRAP_XTAL_25MHZ)
> +			ncpo1 = 0x0a00;
> +	} else { /* PLL frequency: 250MHz: 2.0Gbit */
> +		if (xtal == HWTRAP_XTAL_40MHZ)
> +			ncpo1 = 0x0c80;
> +		if (xtal == HWTRAP_XTAL_25MHZ)
> +			ncpo1 = 0x1400;
>  	}
>  
> -	return 0;
> +	/* Disable the MT7530 TRGMII clocks */
> +	core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);

... by moving this line up and letting it happen unconditionally for
both RGMII and TRGMII (in case that works and doesn't break the RGMII
case, but I assume it doesn't)

> +
> +	/* Setup the MT7530 TRGMII Tx Clock */
> +	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
> +	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> +	core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
> +	core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
> +	core_write(priv, CORE_PLL_GROUP4, RG_SYSPLL_DDSFBK_EN |
> +		   RG_SYSPLL_BIAS_EN | RG_SYSPLL_BIAS_LPF_EN);
> +	core_write(priv, CORE_PLL_GROUP2, RG_SYSPLL_EN_NORMAL |
> +		   RG_SYSPLL_VODEN | RG_SYSPLL_POSDIV(1));
> +	core_write(priv, CORE_PLL_GROUP7, RG_LCDDS_PCW_NCPO_CHG |
> +		   RG_LCCDS_C(3) | RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
> +
> +	/* Enable the MT7530 TRGMII clocks */
> +	core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
>  }
>  
>  static void
> @@ -2609,15 +2596,11 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		  phy_interface_t interface)
>  {
>  	struct mt7530_priv *priv = ds->priv;
> -	int ret;
>  
> -	if (port == 5) {
> +	if (port == 5)
>  		mt7530_setup_port5(priv->ds, interface);
> -	} else if (port == 6) {
> -		ret = mt7530_setup_port6(priv->ds, interface);
> -		if (ret)
> -			return ret;
> -	}
> +	else if (port == 6)
> +		mt7530_setup_port6(priv->ds, interface);
>  
>  	return 0;
>  }
> 
> -- 
> 2.40.1
> 
> 

Re: [PATCH net-next v2 5/7] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL]

On 30.01.2024 18:59, Daniel Golle wrote:
> On Tue, Jan 30, 2024 at 06:20:51PM +0300, Arınç ÜNAL via B4 Relay wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> This code is from before this driver was converted to phylink API. Phylink
>> deals with the unsupported interface cases before mt7530_setup_port6() is
>> run. Therefore, the default case would never run. However, it must be
>> defined nonetheless to handle all the remaining enumeration values, the
>> phy-modes.
>>
>> Switch to if statement for RGMII and return which simplifies the code and
>> saves an indent.
>>
>> Set P6_INTF_MODE, which is the the three least significant bits of the
>> MT7530_P6ECR register, to 0 for RGMII even though it will already be 0
>> after reset. This is to keep supporting dynamic reconfiguration of the port
>> in the case the interface changes from TRGMII to RGMII. The core operations
>> for TRGMII does not interfere with RGMII so no need to undo them.
> 
> That last sentence doesn't parse English gramar.
> "operations": plural
> "does": singular
> 
> Should probably be either "The core operation for TRGMII does not..."
> or "The core operations for TRGMII do not..."

I'll use the latter, thanks.

> 
> As you are mentioning it, I'm now curious if you consider to
> dynamically reconfiguring TRGIII<->RGMII on port 6 depending on
> whether there is more then 1 GBit/s possible bandwidth needed between
> port 6 and the remaining ports? That could make sense for power
> management, but then we should at least again switch off the TRGMII
> clocks in the RGMII case before returning, see my suggestion inline
> below.

Turning off the TRGMII clocks for RGMII makes sense to me. But I don't see
any cases where dynamic interface change between TRGMII and RGMII would
ever occur. Speed too. No PHYs support TRGMII, only some MediaTek SoC MACs
do. That means TRGMII would only be used in fixed links which there is no
dynamic reconfiguration. My patch is about simplifying the code so I don't
want to change the dynamic reconfiguration behaviour.

That said, last year, I have very thoroughly tested this "turbo" RGMII
interface between MT7530 standalone switch and MT7623NI SoC, which would
supposedly achieve 2 Gbps TX & 2 Gbps RX. The performance was as if the
link was regular RGMII. Unless the MediaTek SoC ethernet driver somehow
caps TRGMII to 1 Gbps, I consider this whole TRGMII shenanigans a scam, to
be extremely blunt. I'll give this a one last shot sometime before I push
for the removal of TRGMII from Linux altogether and default to RGMII where
it's used. Because of this, I don't want to spend too much time on this
patch as it's potentially wasted effort.

> 
>>
>> Read XTAL after checking for RGMII as it's only needed for the TRGMII
>> interface mode.
>>
>> Change mt7530_setup_port6() to void now that there're no error cases left.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Reviewed-by: Daniel Golle <daniel@makrotopia.org>
> 
>> ---
>>   drivers/net/dsa/mt7530.c | 103 ++++++++++++++++++++---------------------------
>>   1 file changed, 43 insertions(+), 60 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index c4d492e29fdf..36dc2bbcf3b6 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -414,70 +414,57 @@ mt753x_preferred_default_local_cpu_port(struct dsa_switch *ds)
>>   }
>>   
>>   /* Setup port 6 interface mode and TRGMII TX circuit */
>> -static int
>> +static void
>>   mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
>>   {
>>   	struct mt7530_priv *priv = ds->priv;
>> -	u32 ncpo1, ssc_delta, trgint, xtal;
>> -
>> -	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
>> +	u32 ncpo1, ssc_delta, xtal;
>>   
>> -	switch (interface) {
>> -	case PHY_INTERFACE_MODE_RGMII:
>> -		trgint = 0;
>> -		break;
>> -	case PHY_INTERFACE_MODE_TRGMII:
>> -		trgint = 1;
>> -		if (xtal == HWTRAP_XTAL_25MHZ)
>> -			ssc_delta = 0x57;
>> -		else
>> -			ssc_delta = 0x87;
>> -		if (priv->id == ID_MT7621) {
>> -			/* PLL frequency: 125MHz: 1.0GBit */
>> -			if (xtal == HWTRAP_XTAL_40MHZ)
>> -				ncpo1 = 0x0640;
>> -			if (xtal == HWTRAP_XTAL_25MHZ)
>> -				ncpo1 = 0x0a00;
>> -		} else { /* PLL frequency: 250MHz: 2.0Gbit */
>> -			if (xtal == HWTRAP_XTAL_40MHZ)
>> -				ncpo1 = 0x0c80;
>> -			if (xtal == HWTRAP_XTAL_25MHZ)
>> -				ncpo1 = 0x1400;
>> -		}
>> -		break;
>> -	default:
>> -		dev_err(priv->dev, "xMII interface %d not supported\n",
>> -			interface);
>> -		return -EINVAL;
>> +	if (interface == PHY_INTERFACE_MODE_RGMII) {
>> +		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
>> +			   P6_INTF_MODE(0));
> 
> Maybe at least switch off TRGMIICK here because we are sure we don't need it
> in the RGMII case, ie:
> core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
> 
> And that then is another line of code already present just below which
> means you could keep variable trgint as it was and return after
> switching off TRGMIICK below anyway...
> 
>> +		return;
>>   	}
>>   
>> -	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
>> -		   P6_INTF_MODE(trgint));
>> +	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));
>>   
>> -	if (trgint) {
>> -		/* Disable the MT7530 TRGMII clocks */
>> -		core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
>> +	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
>>   
>> -		/* Setup the MT7530 TRGMII Tx Clock */
>> -		core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
>> -		core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
>> -		core_write(priv, CORE_PLL_GROUP10, RG_LCDDS_SSC_DELTA(ssc_delta));
>> -		core_write(priv, CORE_PLL_GROUP11, RG_LCDDS_SSC_DELTA1(ssc_delta));
>> -		core_write(priv, CORE_PLL_GROUP4,
>> -			   RG_SYSPLL_DDSFBK_EN | RG_SYSPLL_BIAS_EN |
>> -			   RG_SYSPLL_BIAS_LPF_EN);
>> -		core_write(priv, CORE_PLL_GROUP2,
>> -			   RG_SYSPLL_EN_NORMAL | RG_SYSPLL_VODEN |
>> -			   RG_SYSPLL_POSDIV(1));
>> -		core_write(priv, CORE_PLL_GROUP7,
>> -			   RG_LCDDS_PCW_NCPO_CHG | RG_LCCDS_C(3) |
>> -			   RG_LCDDS_PWDB | RG_LCDDS_ISO_EN);
>> +	if (xtal == HWTRAP_XTAL_25MHZ)
>> +		ssc_delta = 0x57;
>> +	else
>> +		ssc_delta = 0x87;
>>   
>> -		/* Enable the MT7530 TRGMII clocks */
>> -		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
>> +	if (priv->id == ID_MT7621) {
>> +		/* PLL frequency: 125MHz: 1.0GBit */
>> +		if (xtal == HWTRAP_XTAL_40MHZ)
>> +			ncpo1 = 0x0640;
>> +		if (xtal == HWTRAP_XTAL_25MHZ)
>> +			ncpo1 = 0x0a00;
>> +	} else { /* PLL frequency: 250MHz: 2.0Gbit */
>> +		if (xtal == HWTRAP_XTAL_40MHZ)
>> +			ncpo1 = 0x0c80;
>> +		if (xtal == HWTRAP_XTAL_25MHZ)
>> +			ncpo1 = 0x1400;
>>   	}
>>   
>> -	return 0;
>> +	/* Disable the MT7530 TRGMII clocks */
>> +	core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
> 
> ... by moving this line up and letting it happen unconditionally for
> both RGMII and TRGMII (in case that works and doesn't break the RGMII
> case, but I assume it doesn't)

I've just tested this, works fine. This looks simpler than bringing back
the trgint variable.

static void
mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
{
	struct mt7530_priv *priv = ds->priv;
	u32 ncpo1, ssc_delta, xtal;

	/* Disable the MT7530 TRGMII clocks */
	core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);

	if (interface == PHY_INTERFACE_MODE_RGMII) {
		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
			   P6_INTF_MODE(0));
		return;
	}

	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));

...

Arınç

Re: [PATCH net-next v2 5/7] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Vladimir Oltean]

On Tue, Jan 30, 2024 at 08:46:04PM +0300, Arınç ÜNAL wrote:
> would supposedly achieve 2 Gbps TX & 2 Gbps RX

Source? Commit 8efaa653a8a5 ("net: ethernet: mediatek: Add MT7621 TRGMII
mode support") says "TRGMII speed is 1200MBit.".

> Unless the MediaTek SoC ethernet driver somehow caps TRGMII to 1 Gbps,
> I consider this whole TRGMII shenanigans a scam

I laughed :)

You have to see whether the CPU isn't in fact at 100% already, becoming
a bottleneck before the interface speed does.

Also, mtk_eth_soc.c has an interesting comment "TRGMII is not permitted
on MT7621 if using DDR2" - not sure if applicable to your setup or not.

I just got myself an ASUS RT-AX1800U (uses the mt7621_asus_rt-ax53u.dts
device tree AFAICT) which I'll be setting up with OpenWrt in the weeks
to come, and on which I might also be able to run some tests from time
to time.

Re: [PATCH net-next v2 5/7] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL]

On 2.02.2024 02:57, Vladimir Oltean wrote:
> On Tue, Jan 30, 2024 at 08:46:04PM +0300, Arınç ÜNAL wrote:
>> would supposedly achieve 2 Gbps TX & 2 Gbps RX
> 
> Source? Commit 8efaa653a8a5 ("net: ethernet: mediatek: Add MT7621 TRGMII
> mode support") says "TRGMII speed is 1200MBit.".

That is for MT7621. It's claimed that TRGMII on MT7621 can only handle that
much. I already told you I'm doing the test on MT7623NI SoC.

MT7623 is ARM and more powerful. On that one, the PLL frequency can be set
all the way up to 362.5 MHz to provide 2900 Mbps (allegedly).

You can check the repository that the commit above links to for more
details:

https://github.com/BPI-SINOVOIP/BPI-R2-bsp/blob/591910e127cd9c811fe9e811ddb6c7278d8ed934/linux-mt/drivers/net/ethernet/raeth/Kconfig#L141
https://github.com/BPI-SINOVOIP/BPI-R2-bsp/blob/591910e127cd9c811fe9e811ddb6c7278d8ed934/linux-mt/drivers/net/ethernet/raeth/raeth_config.h#L201
https://github.com/BPI-SINOVOIP/BPI-R2-bsp/blob/591910e127cd9c811fe9e811ddb6c7278d8ed934/u-boot-mt/drivers/net/rt2880_eth.c#L2178

> 
>> Unless the MediaTek SoC ethernet driver somehow caps TRGMII to 1 Gbps,
>> I consider this whole TRGMII shenanigans a scam
> 
> I laughed :)
> 
> You have to see whether the CPU isn't in fact at 100% already, becoming
> a bottleneck before the interface speed does.

I'm happy I'm entertaining you but you've got to give me a little credit.
:)

MT7621 won't even handle 1 Gbps RX & 1 Gbps TX. But if the IP traffic is
offloaded to the packet processing engine
(drivers/net/ethernet/mediatek/mtk_ppe_offload.c), there won't be any load
on the CPU.

table ip global {
	flowtable f {
		hook ingress priority 0
		devices = { wan, lan1, lan2, lan3, lan4 }
		flags offload
	}

	chain forward {
		type filter hook forward priority 0
		ip protocol { tcp, udp } flow offload @f
	}

	chain postrouting {
		type nat hook postrouting priority 0
		oifname "wan" masquerade
	}
}

MT7623 can handle 1 Gbps RX & 1 Gbps without much CPU load. It performs the
same with or without hardware flow offloading, unlike MT7621.

The way I test this:

I do the test on a single computer. I have two gigabit ports on my
motherboard. I isolate a port by putting it on another network namespace to
do the test.

Client Network
iperf client: 192.168.2.2/24
router: 192.168.2.1/24

Server Network
router: 192.168.3.2/24
iperf server: 192.168.3.1/24

iperf Client
ip a add 192.168.2.2/24 dev enp9s0
ip l set up enp9s0
ip route add 192.168.3.1 via 192.168.2.1
iperf3 -c 192.168.3.1 --bidir -t 20

iperf Server
ip netns add iperfserver
ip link set dev eno1 netns iperfserver
ip netns exec iperfserver ip a add 192.168.3.1/24 dev eno1
ip netns exec iperfserver ip l set up eno1
ip netns exec iperfserver iperf3 -s

I did say I've done thorough testing.

> 
> Also, mtk_eth_soc.c has an interesting comment "TRGMII is not permitted
> on MT7621 if using DDR2" - not sure if applicable to your setup or not.

My device has DDR3 memory. Also, with a device tree defining trgmii on a
link of MediaTek SoC MAC, that check should prevent the mtk_eth_soc driver
from configuring the MAC if the device has DDR2 memory, no?

> 
> I just got myself an ASUS RT-AX1800U (uses the mt7621_asus_rt-ax53u.dts
> device tree AFAICT) which I'll be setting up with OpenWrt in the weeks
> to come, and on which I might also be able to run some tests from time
> to time.

Doing tests on MT7621 will be useless without utilising the PPE. To use it,
you can add these to /etc/config/firewall:

config defaults
	...
	option flow_offloading '1'
	option flow_offloading_hw '1'

Or enable software flow offloading and hardware flow offloading options if
using LuCI. When both options are enabled, hardware flow offloading will be
used.

Make sure to change the PLL frequency on the MT7530 side to 150 MHz. It
operates at the standard RGMII frequency since commit 37c218d8021e ("net:
dsa: mt7530: fix corrupt frames using trgmii on 40 MHz XTAL MT7621").

For 40MHz XTAL:
0x0640 x 0d1,2 = 0x0780

For 25MHz XTAL:
0x0a00 x 0d1,2 = 0x0c00

Arınç

[PATCH net-next v2 6/7] net: dsa: mt7530: correct port capabilities of MT7988

[Arınç ÜNAL via B4 Relay]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

On the switch on the MT7988 SoC, as shown in Block Diagram 8.1.1.3 on page
125 of "MT7988A Wi-Fi 7 Generation Router Platform: Datasheet (Open
Version) v0.1", there are only 4 PHYs. That's port 0 to 3. Set the case for
ports which connect to switch PHYs to '0 ... 3'.

Port 4 and 5 are not used at all in this design.

Link: https://wiki.banana-pi.org/Banana_Pi_BPI-R4#Documents [1]
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 36dc2bbcf3b6..638cd3f2a495 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2577,7 +2577,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 
 	switch (port) {
 	/* Ports which are connected to switch PHYs. There is no MII pinout. */
-	case 0 ... 4:
+	case 0 ... 3:
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;

-- 
2.40.1

Re: [PATCH net-next v2 6/7] net: dsa: mt7530: correct port capabilities of MT7988

[Vladimir Oltean]

On Tue, Jan 30, 2024 at 06:20:52PM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> On the switch on the MT7988 SoC, as shown in Block Diagram 8.1.1.3 on page
> 125 of "MT7988A Wi-Fi 7 Generation Router Platform: Datasheet (Open
> Version) v0.1", there are only 4 PHYs. That's port 0 to 3. Set the case for
> ports which connect to switch PHYs to '0 ... 3'.
> 
> Port 4 and 5 are not used at all in this design.
> 
> Link: https://wiki.banana-pi.org/Banana_Pi_BPI-R4#Documents [1]
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Acked-by: Daniel Golle <daniel@makrotopia.org>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next v2 7/7] net: dsa: mt7530: do not clear config->supported_interfaces

[Arınç ÜNAL via B4 Relay]

From: Arınç ÜNAL <arinc.unal@arinc9.com>

There's no need to clear the config->supported_interfaces bitmap before
reporting the supported interfaces as all bits in the bitmap will already
be initialized to zero when the phylink_config structure is allocated. The
"config" pointer points to &dp->phylink_config, and "dp" is allocated by
dsa_port_touch() with kzalloc(), so all its fields are filled with zeroes.

There's no code that would change the bitmap beforehand. Remove it.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
---
 drivers/net/dsa/mt7530.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 638cd3f2a495..c6b40ca277f5 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2573,8 +2573,6 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
 static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 				     struct phylink_config *config)
 {
-	phy_interface_zero(config->supported_interfaces);
-
 	switch (port) {
 	/* Ports which are connected to switch PHYs. There is no MII pinout. */
 	case 0 ... 3:

-- 
2.40.1