[RFC PATCH net-next 0/8] MT7530 DSA Subdriver Improvements Act II

[RFC PATCH net-next 0/8] MT7530 DSA Subdriver Improvements Act II

[Arınç ÜNAL]

Hi.

I'm submitting this to receive reviews while net-next is closed.

Arınç

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

[Arınç ÜNAL]

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 7f9d63b61168..88ab926ce19e 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

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

[Arınç ÜNAL]

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 88ab926ce19e..434028e9667d 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)
 {
@@ -2608,12 +2614,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

[RFC PATCH net-next 3/8] net: dsa: mt7530: remove pad_setup function pointer

[Arınç ÜNAL]

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 434028e9667d..823dc3ab15c8 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)
 {
@@ -2601,14 +2589,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)
@@ -2778,8 +2758,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;
 
@@ -3097,11 +3075,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;
@@ -3165,7 +3138,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,
 	},
@@ -3177,7 +3149,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,
 	},
@@ -3189,7 +3160,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,
@@ -3202,7 +3172,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,
@@ -3232,9 +3201,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

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

[Arınç ÜNAL]

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>
---
 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 823dc3ab15c8..d3e1e31ac8d0 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;
@@ -2235,6 +2228,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

Re: [RFC PATCH net-next 4/8] net: dsa: mt7530: move XTAL check to mt7530_setup()

[Vladimir Oltean]

On Sat, Jan 13, 2024 at 01:25:25PM +0300, Arınç ÜNAL wrote:
> 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>

[RFC PATCH net-next 5/8] net: dsa: mt7530: move enabling port 6 to mt7530_setup_port6()

[Arınç ÜNAL]

Enable port 6 only when port 6 is being used. Update the comment on
mt7530_setup() with a better explanation. Do not set MHWTRAP_MANUAL on
mt7530_setup_port5() as it's already done on mt7530_setup() beforehand.

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index d3e1e31ac8d0..3ce4e0bb04dd 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -420,6 +420,8 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, xtal;
 
+	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
+
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
 
 	switch (interface) {
@@ -910,7 +912,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 
-	val |= MHWTRAP_MANUAL | MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
+	val |= MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
 	val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
 
 	switch (priv->p5_intf_sel) {
@@ -2250,9 +2252,11 @@ mt7530_setup(struct dsa_switch *ds)
 		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
 			   RD_TAP_MASK, RD_TAP(16));
 
-	/* Enable port 6 */
+	/* Directly access the PHY registers via C_MDC/C_MDIO. The bit that
+	 * enables modifying the hardware trap must be set for this.
+	 */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
-	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
+	val &= ~MHWTRAP_PHY_ACCESS;
 	val |= MHWTRAP_MANUAL;
 	mt7530_write(priv, MT7530_MHWTRAP, val);
 
-- 
2.40.1

Re: [RFC PATCH net-next 5/8] net: dsa: mt7530: move enabling port 6 to mt7530_setup_port6()

[Vladimir Oltean]

On Sat, Jan 13, 2024 at 01:25:26PM +0300, Arınç ÜNAL wrote:
> Enable port 6 only when port 6 is being used. Update the comment on
> mt7530_setup() with a better explanation. Do not set MHWTRAP_MANUAL on
> mt7530_setup_port5() as it's already done on mt7530_setup() beforehand.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

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

[RFC PATCH net-next 6/8] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL]

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.

Do not set P6_INTF_MODE, which is the the three least significant bits of
the MT7530_P6ECR register, to 0 for RGMII as it will already be 0 after
reset.

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 | 100 ++++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 60 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3ce4e0bb04dd..3a02308763ca 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -414,72 +414,56 @@ 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;
+	u32 ncpo1, ssc_delta, xtal;
 
 	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
 
+	if (interface == PHY_INTERFACE_MODE_RGMII)
+		return;
+
+	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK, P6_INTF_MODE(1));
+
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
 
-	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)
-			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;
+			ncpo1 = 0x0a00;
+	} else { /* PLL frequency: 250MHz: 2.0Gbit */
+		if (xtal == HWTRAP_XTAL_40MHZ)
+			ncpo1 = 0x0c80;
+		if (xtal == HWTRAP_XTAL_25MHZ)
+			ncpo1 = 0x1400;
 	}
 
-	mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
-		   P6_INTF_MODE(trgint));
-
-	if (trgint) {
-		/* 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);
+	/* Disable the MT7530 TRGMII clocks */
+	core_clear(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
 
-		/* Enable the MT7530 TRGMII clocks */
-		core_set(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);
 
-	return 0;
+	/* Enable the MT7530 TRGMII clocks */
+	core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
 }
 
 static void
@@ -2597,15 +2581,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: [RFC PATCH net-next 6/8] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Vladimir Oltean]

On Sat, Jan 13, 2024 at 01:25:27PM +0300, Arınç ÜNAL wrote:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3ce4e0bb04dd..3a02308763ca 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -414,72 +414,56 @@ 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;
> +	u32 ncpo1, ssc_delta, xtal;
>  
>  	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
>  
> +	if (interface == PHY_INTERFACE_MODE_RGMII)
> +		return;

It would be good to add a comment here which states that the port comes
out of reset with values good for RGMII.

Also, there's a built-in assumption in this patch, that dynamically
switching between RGMII and TRGMII is not possible. This is because
phylink mac_config() is not necesarily called only once immediately
after reset, but after each major_config().

The fact that the driver sets both PHY_INTERFACE_MODE_RGMII and
PHY_INTERFACE_MODE_TRGMII at once in config->supported_interfaces does
not disprove that dynamic reconfiguration is possible. Normally,
interfaces for which it doesn't make sense to dynamically reconfigure
(they are wired to fixed pinout) have a single bit set in
supported_interfaces. Is this switching something that makes any sense
at all, given that port 6 is internal? It's not something that phylink
knows to do today, but if this is theoretically possible and remotely
useful, someone might end up wanting, in the future, to revert this patch.

Re: [RFC PATCH net-next 6/8] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL]

On 16.01.2024 00:37, Vladimir Oltean wrote:
> On Sat, Jan 13, 2024 at 01:25:27PM +0300, Arınç ÜNAL wrote:
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3ce4e0bb04dd..3a02308763ca 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -414,72 +414,56 @@ 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;
>> +	u32 ncpo1, ssc_delta, xtal;
>>   
>>   	mt7530_clear(priv, MT7530_MHWTRAP, MHWTRAP_P6_DIS);
>>   
>> +	if (interface == PHY_INTERFACE_MODE_RGMII)
>> +		return;
> 
> It would be good to add a comment here which states that the port comes
> out of reset with values good for RGMII.
> 
> Also, there's a built-in assumption in this patch, that dynamically
> switching between RGMII and TRGMII is not possible. This is because
> phylink mac_config() is not necesarily called only once immediately
> after reset, but after each major_config().
> 
> The fact that the driver sets both PHY_INTERFACE_MODE_RGMII and
> PHY_INTERFACE_MODE_TRGMII at once in config->supported_interfaces does
> not disprove that dynamic reconfiguration is possible. Normally,
> interfaces for which it doesn't make sense to dynamically reconfigure
> (they are wired to fixed pinout) have a single bit set in
> supported_interfaces. Is this switching something that makes any sense
> at all, given that port 6 is internal? It's not something that phylink
> knows to do today, but if this is theoretically possible and remotely
> useful, someone might end up wanting, in the future, to revert this patch.

Do you mean by internal port that the port does not have MII pinout? Port 6
of the MT7530 switch do. It is possible to have an external PHY wired to
it. So it would make sense to design mt7530_setup_port6() in the sense that
dynamic reconfiguration is possible.

I've tested to see that the core operations for TRGMII does not interfere
so no need to undo them when the interface changes from TRGMII to RGMII.

I'll do below on this patch:

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

Arınç

Re: [RFC PATCH net-next 6/8] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Vladimir Oltean]

On Tue, Jan 16, 2024 at 04:09:18PM +0300, Arınç ÜNAL wrote:
> Do you mean by internal port that the port does not have MII pinout? Port 6
> of the MT7530 switch do. It is possible to have an external PHY wired to it.

Yes, this is what I meant by internal port. It seems I was wrong to
assume it is always connected to GMAC0.

How is the selection done between internal and external wiring?

If external wiring to a PHY is possible, shouldn't the driver accept all
4 RGMII variants with phy_interface_mode_is_rgmii(), because the delays
specified in "rgmii-txid", "rgmii-rxid", "rgmii-id" always pertain to
the PHY, and thus it doesn't make sense for the MAC to not allow the use
of the full spectrum?

> So it would make sense to design mt7530_setup_port6() in the sense that
> dynamic reconfiguration is possible.

Ok, you mean to keep the dynamic reconfiguration possible rather than
redesign to disallow it.

> I've tested to see that the core operations for TRGMII does not interfere
> so no need to undo them when the interface changes from TRGMII to RGMII.
> 
> I'll do below on this patch:
> 
> 	if (interface == PHY_INTERFACE_MODE_RGMII) {
> 		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
> 			   P6_INTF_MODE(0));
> 		return;
> 	}

Ok.

Re: [RFC PATCH net-next 6/8] net: dsa: mt7530: simplify mt7530_setup_port6() and change to void

[Arınç ÜNAL]

On 16.01.2024 16:47, Vladimir Oltean wrote:
> On Tue, Jan 16, 2024 at 04:09:18PM +0300, Arınç ÜNAL wrote:
>> Do you mean by internal port that the port does not have MII pinout? Port 6
>> of the MT7530 switch do. It is possible to have an external PHY wired to it.
> 
> Yes, this is what I meant by internal port. It seems I was wrong to
> assume it is always connected to GMAC0.
> 
> How is the selection done between internal and external wiring?

There are two variants of MT7530. One is standalone, the other comes with
the SoC as a part of the multi-chip module. More information at
mediatek,mt7530.yaml.

The standalone one is straightforward. A MAC or a PHY can be wired to the
MII pinouts of port 5 and 6 on the PCB, just as the relevant MII standard
describes.

On the MT7621 SoCs which include the switch IC, port 6 is wired to GMAC0,
port 5 is wired to GMAC1. I assume you mean internal and external wiring in
reference to this case. This is the internal wiring.

The external wiring works by wiring the PHY or MAC to the MII pinouts of
the SoC's two MACs. Since RX of the switch MAC is wired to TX of the SoC
MAC and vice versa, the external PHY or MAC must be wired TX to TX and RX
to RX. Ubiquiti EdgeRouter X SFP is wired this way. The wiring for clock
pins may need to be mirrored too, I haven't studied the RGMII specification
that much in detail. This works by not enabling the SoC MAC.

> 
> If external wiring to a PHY is possible, shouldn't the driver accept all
> 4 RGMII variants with phy_interface_mode_is_rgmii(), because the delays
> specified in "rgmii-txid", "rgmii-rxid", "rgmii-id" always pertain to
> the PHY, and thus it doesn't make sense for the MAC to not allow the use
> of the full spectrum?

Great point. I think delays are not supported on port 6. There's only the
"MT7530 Giga Switch programming guide v0.1" document mentioning setting
delays on page 8, and it's only for port 5. It is also implemented on
mt7530_setup_port5():

/* P5 RGMII RX Clock Control: delay setting for 1000M */
mt7530_write(priv, MT7530_P5RGMIIRXCR, CSR_RGMII_EDGE_ALIGN);

/* Don't set delay in DSA mode */
if (!dsa_is_dsa_port(priv->ds, 5) &&
     (interface == PHY_INTERFACE_MODE_RGMII_TXID ||
      interface == PHY_INTERFACE_MODE_RGMII_ID))
	tx_delay = 4; /* n * 0.5 ns */

/* P5 RGMII TX Clock Control: delay x */
mt7530_write(priv, MT7530_P5RGMIITXCR,
	     CSR_RGMII_TXC_CFG(0x10 + tx_delay));

There's only the TX driving mentioned for port 6 on the document. I'm
guessing port 6 was intended to connect to another TRGMII capable MAC so
delays were out of the question. TRGMII is just overclocked RGMII to
provide up to 2Gbps TX/RX, at least in theory. The whole existence of the
TRGMII interface on Linux is only being used by the MT7621 and MT7623 SoC
MACs, and port 6 MAC of the MT7530 switch.

> 
>> So it would make sense to design mt7530_setup_port6() in the sense that
>> dynamic reconfiguration is possible.
> 
> Ok, you mean to keep the dynamic reconfiguration possible rather than
> redesign to disallow it.

Very much so.

Arınç

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

[Arınç ÜNAL]

On the switch on the MT7988 SoC, there are only 4 PHYs. That's port 0 to 3.
Set the internal phy cases to '0 ... 3'.

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 3a02308763ca..e7e7e89d8eca 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2562,7 +2562,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 
 	switch (port) {
 	/* Internal PHY */
-	case 0 ... 4:
+	case 0 ... 3:
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;
-- 
2.40.1

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

[Vladimir Oltean]

On Sat, Jan 13, 2024 at 01:25:28PM +0300, Arınç ÜNAL wrote:
> On the switch on the MT7988 SoC, there are only 4 PHYs. That's port 0 to 3.
> Set the internal phy cases to '0 ... 3'.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Acked-by: Daniel Golle <daniel@makrotopia.org>
> ---

For this patch to be obviously correct, please make a statement in the
commit message about port 4. It doesn't exist at all?

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

[Daniel Golle]

On Mon, Jan 15, 2024 at 11:42:38PM +0200, Vladimir Oltean wrote:
> On Sat, Jan 13, 2024 at 01:25:28PM +0300, Arınç ÜNAL wrote:
> > On the switch on the MT7988 SoC, there are only 4 PHYs. That's port 0 to 3.
> > Set the internal phy cases to '0 ... 3'.
> > 
> > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > Acked-by: Daniel Golle <daniel@makrotopia.org>
> > ---
> 
> For this patch to be obviously correct, please make a statement in the
> commit message about port 4. It doesn't exist at all?

Correct. As shown in Block Diagram 8.1.1.3 on page 125 of
MT7988A Wi-Fi 7 Generation Router Platform: Datasheet (Open Version)
Version: 0.1
Release Date: 2023-10-18
which is available publicly on BananaPi's Wiki[1].

Port 0~3 are TP user ports, Port 6 is internally linked to XGMII of MAC 0.
Port 4 and 5 are not used at all in this design.

[1]: https://wiki.banana-pi.org/Banana_Pi_BPI-R4#Documents

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

[Arınç ÜNAL]

On 16.01.2024 01:09, Daniel Golle wrote:
> On Mon, Jan 15, 2024 at 11:42:38PM +0200, Vladimir Oltean wrote:
>> On Sat, Jan 13, 2024 at 01:25:28PM +0300, Arınç ÜNAL wrote:
>>> On the switch on the MT7988 SoC, there are only 4 PHYs. That's port 0 to 3.
>>> Set the internal phy cases to '0 ... 3'.
>>>
>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>> Acked-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>
>> For this patch to be obviously correct, please make a statement in the
>> commit message about port 4. It doesn't exist at all?
> 
> Correct. As shown in Block Diagram 8.1.1.3 on page 125 of
> MT7988A Wi-Fi 7 Generation Router Platform: Datasheet (Open Version)
> Version: 0.1
> Release Date: 2023-10-18
> which is available publicly on BananaPi's Wiki[1].
> 
> Port 0~3 are TP user ports, Port 6 is internally linked to XGMII of MAC 0.
> Port 4 and 5 are not used at all in this design.
> 
> [1]: https://wiki.banana-pi.org/Banana_Pi_BPI-R4#Documents

Thanks for chiming in Daniel, I will include this on the patch log.

Arınç

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

[Arınç ÜNAL]

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.
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>
---
 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 e7e7e89d8eca..361a9cda48eb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2558,8 +2558,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) {
 	/* Internal PHY */
 	case 0 ... 3:
-- 
2.40.1

Re: [RFC PATCH net-next 8/8] net: dsa: mt7530: do not clear config->supported_interfaces

[Vladimir Oltean]

On Sat, Jan 13, 2024 at 01:25:29PM +0300, Arınç ÜNAL wrote:
> 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.
> 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>
> ---

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

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