[RFC PATCH net-next 00/22] net: dsa: MT7530, MT7531, and MT7988 improvements

[RFC PATCH net-next 00/22] net: dsa: MT7530, MT7531, and MT7988 improvements

[arinc9.unal]

Hello!

This patch series is focused on simplifying the code, and improving the
logic of the support for MT7530, MT7531, and MT7988 SoC switches.

There's also a fix for the switch on the MT7988 SoC.

Daniel, can you test this series on the MT7531 and MT7988 SoC switch?

The only missing piece to properly support multiple ports as CPU ports is
the fixes [0] [1] [2] from Richard.

I've got questions regarding patch 13 and 19.

For patch 19:

Daniel, does the switch on the MT7988 SoC require the register to fire
interrupts properly? The code uses an inclusive check which was untouched
when the MT7988 SoC switch support was added.

For patch 13:

Do I need to protect the register from being accessed by processes while
this operation is being done? I don't see this on mt7530_setup() but it's
being done on mt7530_setup_port5().

I have very thoroughly tested the patch series with every possible
configuration on the MCM and standalone MT7530 switch. I'll let the name of
the dtb files speak for themselves.

MT7621 Unielec:

only-gmac0-mt7621-unielec-u7621-06-16m.dtb
rgmii-only-gmac0-mt7621-unielec-u7621-06-16m.dtb
only-gmac1-mt7621-unielec-u7621-06-16m.dtb
gmac0-and-gmac1-mt7621-unielec-u7621-06-16m.dtb
phy0-muxing-mt7621-unielec-u7621-06-16m.dtb
phy4-muxing-mt7621-unielec-u7621-06-16m.dtb
port5-as-user-mt7621-unielec-u7621-06-16m.dtb

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

MT7623 Bananapi:

only-gmac0-mt7623n-bananapi-bpi-r2.dtb
rgmii-only-gmac0-mt7623n-bananapi-bpi-r2.dtb
only-gmac1-mt7623n-bananapi-bpi-r2.dtb
gmac0-and-gmac1-mt7623n-bananapi-bpi-r2.dtb
phy0-muxing-mt7623n-bananapi-bpi-r2.dtb
phy4-muxing-mt7623n-bananapi-bpi-r2.dtb
port5-as-user-mt7623n-bananapi-bpi-r2.dtb

tftpboot 0x80008000 arm-uImage; tftpboot 0x83000000 arm-rootfs.cpio.uboot; tftpboot 0x83f00000 $dtb; bootm 0x80008000 0x83000000 0x83f00000

[0] https://lore.kernel.org/netdev/20230212213949.672443-1-richard@routerhints.com/
[1] https://lore.kernel.org/netdev/20230212215152.673221-1-richard@routerhints.com/
[2] https://lore.kernel.org/netdev/20230212214027.672501-1-richard@routerhints.com/

Arınç

[RFC PATCH net-next 01/22] net: dsa: mt7530: add missing @p5_interface to mt7530_priv description

[arinc9.unal]

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

Add the missing p5_interface field to the mt7530_priv description. Sort out
the description in the process.

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

diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 5084f48a8869..845f5dd16d83 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -746,7 +746,8 @@ struct mt753x_info {
  * @ports:		Holding the state among ports
  * @reg_mutex:		The lock for protecting among process accessing
  *			registers
- * @p6_interface	Holding the current port 6 interface
+ * @p6_interface:	Holding the current port 6 interface
+ * @p5_interface:	Holding the current port 5 interface
  * @p5_intf_sel:	Holding the current port 5 interface select
  * @irq:		IRQ number of the switch
  * @irq_domain:		IRQ domain of the switch irq_chip
-- 
2.37.2

[RFC PATCH net-next 02/22] net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel

[arinc9.unal]

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

Use the p5_interface_select enumeration as the data type for the
p5_intf_sel field. This ensures p5_intf_sel can only take the values
defined in the p5_interface_select enumeration.

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

diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 845f5dd16d83..703f8a528317 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -674,13 +674,13 @@ struct mt7530_port {
 };
 
 /* Port 5 interface select definitions */
-enum p5_interface_select {
-	P5_DISABLED = 0,
+typedef enum {
+	P5_DISABLED,
 	P5_INTF_SEL_PHY_P0,
 	P5_INTF_SEL_PHY_P4,
 	P5_INTF_SEL_GMAC5,
 	P5_INTF_SEL_GMAC5_SGMII,
-};
+} p5_interface_select;
 
 struct mt7530_priv;
 
@@ -768,7 +768,7 @@ struct mt7530_priv {
 	bool			mcm;
 	phy_interface_t		p6_interface;
 	phy_interface_t		p5_interface;
-	unsigned int		p5_intf_sel;
+	p5_interface_select	p5_intf_sel;
 	u8			mirror_rx;
 	u8			mirror_tx;
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
-- 
2.37.2

[RFC PATCH net-next 03/22] net: dsa: mt7530: properly support MT7531AE and MT7531BE

[arinc9.unal]

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

Introduce the p5_sgmii pointer to store the information for whether port 5
has got SGMII or not. Print "found MT7531AE" if it's got it, print "found
MT7531BE" if it hasn't.

Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
switch is identified.

Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
information. Address the code where mt7531_dual_sgmii_supported() is used.

Get rid of mt7531_is_rgmii_port() which just prints the opposite of
priv->p5_sgmii.

Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
represent the mode that port 5 is used in, not the hardware information of
port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if port 5 is not
dsa_is_unused_port().

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530-mdio.c |  7 ++---
 drivers/net/dsa/mt7530.c      | 49 +++++++++++++++--------------------
 drivers/net/dsa/mt7530.h      |  6 +++--
 3 files changed, 27 insertions(+), 35 deletions(-)

diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
index 088533663b83..fa3ee85a99c1 100644
--- a/drivers/net/dsa/mt7530-mdio.c
+++ b/drivers/net/dsa/mt7530-mdio.c
@@ -81,17 +81,14 @@ static const struct regmap_bus mt7530_regmap_bus = {
 };
 
 static int
-mt7531_create_sgmii(struct mt7530_priv *priv, bool dual_sgmii)
+mt7531_create_sgmii(struct mt7530_priv *priv)
 {
 	struct regmap_config *mt7531_pcs_config[2] = {};
 	struct phylink_pcs *pcs;
 	struct regmap *regmap;
 	int i, ret = 0;
 
-	/* MT7531AE has two SGMII units for port 5 and port 6
-	 * MT7531BE has only one SGMII unit for port 6
-	 */
-	for (i = dual_sgmii ? 0 : 1; i < 2; i++) {
+	for (i = priv->p5_sgmii ? 0 : 1; i < 2; i++) {
 		mt7531_pcs_config[i] = devm_kzalloc(priv->dev,
 						    sizeof(struct regmap_config),
 						    GFP_KERNEL);
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index c680873819b0..edc34be745b2 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -473,15 +473,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 	return 0;
 }
 
-static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
-{
-	u32 val;
-
-	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
-
-	return (val & PAD_DUAL_SGMII_EN) != 0;
-}
-
 static int
 mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
@@ -496,7 +487,7 @@ mt7531_pll_setup(struct mt7530_priv *priv)
 	u32 xtal;
 	u32 val;
 
-	if (mt7531_dual_sgmii_supported(priv))
+	if (priv->p5_sgmii)
 		return;
 
 	val = mt7530_read(priv, MT7531_CREV);
@@ -907,8 +898,6 @@ static const char *p5_intf_modes(unsigned int p5_interface)
 		return "PHY P4";
 	case P5_INTF_SEL_GMAC5:
 		return "GMAC5";
-	case P5_INTF_SEL_GMAC5_SGMII:
-		return "GMAC5_SGMII";
 	default:
 		return "unknown";
 	}
@@ -2440,6 +2429,18 @@ mt7531_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	/* MT7531AE has got two SGMII units. One for port 5, one for port 6.
+	 * MT7531BE has got only one SGMII unit which is for port 6.
+	 */
+	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
+
+	if ((val & PAD_DUAL_SGMII_EN) != 0) {
+		priv->p5_sgmii = true;
+		dev_info(priv->dev, "found MT7531AE\n");
+	} else {
+		dev_info(priv->dev, "found MT7531BE\n");
+	}
+
 	/* all MACs must be forced link-down before sw reset */
 	for (i = 0; i < MT7530_NUM_PORTS; i++)
 		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
@@ -2451,19 +2452,16 @@ mt7531_setup(struct dsa_switch *ds)
 
 	mt7531_pll_setup(priv);
 
-	if (mt7531_dual_sgmii_supported(priv)) {
-		priv->p5_intf_sel = P5_INTF_SEL_GMAC5_SGMII;
-
+	if (priv->p5_sgmii) {
 		/* Let ds->slave_mii_bus be able to access external phy. */
 		mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO11_RG_RXD2_MASK,
 			   MT7531_EXT_P_MDC_11);
 		mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO12_RG_RXD3_MASK,
 			   MT7531_EXT_P_MDIO_12);
-	} else {
-		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
 	}
-	dev_dbg(ds->dev, "P5 support %s interface\n",
-		p5_intf_modes(priv->p5_intf_sel));
+
+	if (!dsa_is_unused_port(ds, 5))
+		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
 
 	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
 		   MT7531_GPIO0_INTERRUPT);
@@ -2523,11 +2521,6 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
 	}
 }
 
-static bool mt7531_is_rgmii_port(struct mt7530_priv *priv, u32 port)
-{
-	return (port == 5) && (priv->p5_intf_sel != P5_INTF_SEL_GMAC5_SGMII);
-}
-
 static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
 				     struct phylink_config *config)
 {
@@ -2540,7 +2533,7 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
 		break;
 
 	case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
-		if (mt7531_is_rgmii_port(priv, port)) {
+		if (!priv->p5_sgmii) {
 			phy_interface_set_rgmii(config->supported_interfaces);
 			break;
 		}
@@ -2607,7 +2600,7 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
 {
 	u32 val;
 
-	if (!mt7531_is_rgmii_port(priv, port)) {
+	if (priv->p5_sgmii) {
 		dev_err(priv->dev, "RGMII mode is not available for port %d\n",
 			port);
 		return -EINVAL;
@@ -2860,7 +2853,7 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
 
 	switch (port) {
 	case 5:
-		if (mt7531_is_rgmii_port(priv, port))
+		if (!priv->p5_sgmii)
 			interface = PHY_INTERFACE_MODE_RGMII;
 		else
 			interface = PHY_INTERFACE_MODE_2500BASEX;
@@ -3019,7 +3012,7 @@ mt753x_setup(struct dsa_switch *ds)
 		mt7530_free_irq_common(priv);
 
 	if (priv->create_sgmii) {
-		ret = priv->create_sgmii(priv, mt7531_dual_sgmii_supported(priv));
+		ret = priv->create_sgmii(priv);
 		if (ret && priv->irq)
 			mt7530_free_irq(priv);
 	}
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 703f8a528317..f58828577520 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -679,7 +679,6 @@ typedef enum {
 	P5_INTF_SEL_PHY_P0,
 	P5_INTF_SEL_PHY_P4,
 	P5_INTF_SEL_GMAC5,
-	P5_INTF_SEL_GMAC5_SGMII,
 } p5_interface_select;
 
 struct mt7530_priv;
@@ -749,6 +748,8 @@ struct mt753x_info {
  * @p6_interface:	Holding the current port 6 interface
  * @p5_interface:	Holding the current port 5 interface
  * @p5_intf_sel:	Holding the current port 5 interface select
+ * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
+ *			has got SGMII
  * @irq:		IRQ number of the switch
  * @irq_domain:		IRQ domain of the switch irq_chip
  * @irq_enable:		IRQ enable bits, synced to SYS_INT_EN
@@ -769,6 +770,7 @@ struct mt7530_priv {
 	phy_interface_t		p6_interface;
 	phy_interface_t		p5_interface;
 	p5_interface_select	p5_intf_sel;
+	bool			p5_sgmii;
 	u8			mirror_rx;
 	u8			mirror_tx;
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
@@ -778,7 +780,7 @@ struct mt7530_priv {
 	int irq;
 	struct irq_domain *irq_domain;
 	u32 irq_enable;
-	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
+	int (*create_sgmii)(struct mt7530_priv *priv);
 };
 
 struct mt7530_hw_vlan_entry {
-- 
2.37.2

[RFC PATCH net-next 04/22] net: dsa: mt7530: improve comments regarding port 5 and 6

[arinc9.unal]

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

There's no logic to numerically order the CPU ports. State the port number
and its capability of being used as a CPU port instead.

Remove the irrelevant PHY muxing information from
mt7530_mac_port_get_caps(). Explain the supported MII modes instead.

Remove the out of place PHY muxing information from
mt753x_phylink_mac_config(). The function is for both the MT7530 and MT7531
switches but there's no PHY muxing on MT7531.

These comments were gradually introduced with the commits below.
ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK API")
38f790a80560 ("net: dsa: mt7530: Add support for port 5")
88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
hardware")
c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index edc34be745b2..e956ffa1eea8 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2504,7 +2504,9 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
 			  config->supported_interfaces);
 		break;
 
-	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+	case 5: /* Port 5 which can be used as a CPU port supports rgmii with
+		 * delays, mii, and gmii.
+		 */
 		phy_interface_set_rgmii(config->supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_MII,
 			  config->supported_interfaces);
@@ -2512,7 +2514,9 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
 			  config->supported_interfaces);
 		break;
 
-	case 6: /* 1st cpu port */
+	case 6: /* Port 6 which can be used as a CPU port supports rgmii and
+		 * trgmii.
+		 */
 		__set_bit(PHY_INTERFACE_MODE_RGMII,
 			  config->supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_TRGMII,
@@ -2532,14 +2536,17 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
 			  config->supported_interfaces);
 		break;
 
-	case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
+	case 5: /* Port 5 which can be used as a CPU port supports rgmii with
+		 * delays on MT7531BE, sgmii/802.3z on MT7531AE.
+		 */
 		if (!priv->p5_sgmii) {
 			phy_interface_set_rgmii(config->supported_interfaces);
 			break;
 		}
 		fallthrough;
 
-	case 6: /* 1st cpu port supports sgmii/8023z only */
+	case 6: /* Port 6 which can be used as a CPU port supports sgmii/802.3z.
+		 */
 		__set_bit(PHY_INTERFACE_MODE_SGMII,
 			  config->supported_interfaces);
 		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
@@ -2731,7 +2738,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
 			goto unsupported;
 		break;
-	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+	case 5: /* Port 5, can be used as a CPU port. */
 		if (priv->p5_interface == state->interface)
 			break;
 
@@ -2741,7 +2748,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p5_intf_sel != P5_DISABLED)
 			priv->p5_interface = state->interface;
 		break;
-	case 6: /* 1st cpu port */
+	case 6: /* Port 6, can be used as a CPU port. */
 		if (priv->p6_interface == state->interface)
 			break;
 
-- 
2.37.2

[RFC PATCH net-next 05/22] net: dsa: mt7530: read XTAL value from correct register

[arinc9.unal]

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

On commit 7ef6f6f8d237 ("net: dsa: mt7530: Add MT7621 TRGMII mode support")
bit mask macros were added under the MT7530_HWTRAP register to read the
crystal frequency. However, the value given to the xtal variable on
mt7530_pad_clk_setup() is read from the MT7530_MHWTRAP register instead.

It doesn't seem to matter as my testing on MCM and standalone MT7530 shows
the value is correctly read from both registers but change it anyway.

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 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 e956ffa1eea8..30553044d4b7 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -406,7 +406,7 @@ mt7530_pad_clk_setup(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;
+	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
 
 	if (xtal == HWTRAP_XTAL_20MHZ) {
 		dev_err(priv->dev,
-- 
2.37.2

[RFC PATCH net-next 06/22] net: dsa: mt7530: improve code path for setting up port 5

[arinc9.unal]

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()

Currently mt7530_setup_port5() from mt7530_setup() always runs. If port 5
is used as a CPU, DSA, or user port, mt7530_setup_port5() from
mt753x_phylink_mac_config() won't run. That is because priv->p5_interface
set on mt7530_setup_port5() will match state->interface on
mt753x_phylink_mac_config() which will stop running mt7530_setup_port5()
again.

mt7530_setup_port5() from mt753x_phylink_mac_config() won't run when port 5
is disabled or used for PHY muxing as port 5 won't be defined on the
devicetree.

Therefore, mt7530_setup_port5() will never run from
mt753x_phylink_mac_config().

Address this by not running mt7530_setup_port5() from mt7530_setup() if
port 5 is used as a CPU, DSA, or user port. For the cases of PHY muxing or
the port being disabled, call mt7530_setup_port5() from mt7530_setup().

Do not set priv->p5_interface on mt7530_setup_port5(). There won't be a
case where mt753x_phylink_mac_config() runs after mt7530_setup_port5()
anymore.

Do not set priv->p5_intf_sel to P5_DISABLED. It is already set to that when
"priv" is allocated.

Move setting the interface to a more specific location. It's supposed to be
overwritten if PHY muxing is detected.

Improve the comment which explain the process.

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 30553044d4b7..591df09c8bb5 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -968,8 +968,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));
 
-	priv->p5_interface = interface;
-
 unlock_exit:
 	mutex_unlock(&priv->reg_mutex);
 }
@@ -2277,16 +2275,15 @@ mt7530_setup(struct dsa_switch *ds)
 		return ret;
 
 	/* Setup port 5 */
-	priv->p5_intf_sel = P5_DISABLED;
-	interface = PHY_INTERFACE_MODE_NA;
-
 	if (!dsa_is_unused_port(ds, 5)) {
 		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
-		ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
-		if (ret && ret != -ENODEV)
-			return ret;
 	} else {
-		/* Scan the ethernet nodes. look for GMAC1, lookup used phy */
+		/* Scan the ethernet nodes. Look for GMAC1, lookup the used PHY.
+		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
+		 * is detected.
+		 */
+		interface = PHY_INTERFACE_MODE_NA;
+
 		for_each_child_of_node(dn, mac_np) {
 			if (!of_device_is_compatible(mac_np,
 						     "mediatek,eth-mac"))
@@ -2317,6 +2314,8 @@ mt7530_setup(struct dsa_switch *ds)
 			of_node_put(phy_node);
 			break;
 		}
+
+		mt7530_setup_port5(ds, interface);
 	}
 
 #ifdef CONFIG_GPIOLIB
@@ -2327,8 +2326,6 @@ mt7530_setup(struct dsa_switch *ds)
 	}
 #endif /* CONFIG_GPIOLIB */
 
-	mt7530_setup_port5(ds, interface);
-
 	/* Flush the FDB table */
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
 	if (ret < 0)
-- 
2.37.2

[RFC PATCH net-next 07/22] net: dsa: mt7530: do not run mt7530_setup_port5() if port 5 is disabled

[arinc9.unal]

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

There's no need to run all the code on mt7530_setup_port5() if port 5 is
disabled. The only case for calling mt7530_setup_port5() from
mt7530_setup() is when PHY muxing is enabled. That is because port 5 is not
defined as a port on the devicetree, therefore, it cannot be controlled by
phylink.

Because of this, run mt7530_setup_port5() if priv->p5_intf_sel is
P5_INTF_SEL_PHY_P0 or P5_INTF_SEL_PHY_P4. Remove the P5_DISABLED case from
mt7530_setup_port5().

Stop initialising the interface variable as the remaining cases will always
call mt7530_setup_port5() with it initialised.

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 591df09c8bb5..bac2388319a3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -932,9 +932,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
 		val &= ~MHWTRAP_P5_DIS;
 		break;
-	case P5_DISABLED:
-		interface = PHY_INTERFACE_MODE_NA;
-		break;
 	default:
 		dev_err(ds->dev, "Unsupported p5_intf_sel %d\n",
 			priv->p5_intf_sel);
@@ -2282,8 +2279,6 @@ mt7530_setup(struct dsa_switch *ds)
 		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
 		 * is detected.
 		 */
-		interface = PHY_INTERFACE_MODE_NA;
-
 		for_each_child_of_node(dn, mac_np) {
 			if (!of_device_is_compatible(mac_np,
 						     "mediatek,eth-mac"))
@@ -2315,7 +2310,9 @@ mt7530_setup(struct dsa_switch *ds)
 			break;
 		}
 
-		mt7530_setup_port5(ds, interface);
+		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
+		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
+			mt7530_setup_port5(ds, interface);
 	}
 
 #ifdef CONFIG_GPIOLIB
-- 
2.37.2

[RFC PATCH net-next 08/22] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

[arinc9.unal]

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

The idea of p5_interface and p6_interface pointers is to prevent
mt753x_mac_config() from running twice for MT7531, as it's already run with
mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is used as
a CPU port.

Change p5_interface and p6_interface to p5_configured and p6_configured.
Make them boolean.

Do not set them for any other reason.

The priv->p5_intf_sel check is useless as in this code path, it will always
be P5_INTF_SEL_GMAC5.

There was also no need to set priv->p5_interface and priv->p6_interface to
PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
already be set to that when "priv" is allocated. The pointers were of the
phy_interface_t enumeration type, and the first element of the enum is
PHY_INTERFACE_MODE_NA. There was nothing in between that would change this
beforehand.

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 19 ++++---------------
 drivers/net/dsa/mt7530.h | 10 ++++++----
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index bac2388319a3..2f670e512415 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2237,8 +2237,6 @@ mt7530_setup(struct dsa_switch *ds)
 	val |= MHWTRAP_MANUAL;
 	mt7530_write(priv, MT7530_MHWTRAP, val);
 
-	priv->p6_interface = PHY_INTERFACE_MODE_NA;
-
 	/* Enable and reset MIB counters */
 	mt7530_mib_reset(ds);
 
@@ -2460,10 +2458,6 @@ mt7531_setup(struct dsa_switch *ds)
 	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
 		   MT7531_GPIO0_INTERRUPT);
 
-	/* Let phylink decide the interface later. */
-	priv->p5_interface = PHY_INTERFACE_MODE_NA;
-	priv->p6_interface = PHY_INTERFACE_MODE_NA;
-
 	/* Enable PHY core PLL, since phy_device has not yet been created
 	 * provided for phy_[read,write]_mmd_indirect is called, we provide
 	 * our own mt7531_ind_mmd_phy_[read,write] to complete this
@@ -2733,25 +2727,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 			goto unsupported;
 		break;
 	case 5: /* Port 5, can be used as a CPU port. */
-		if (priv->p5_interface == state->interface)
+		if (priv->p5_configured)
 			break;
 
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
-
-		if (priv->p5_intf_sel != P5_DISABLED)
-			priv->p5_interface = state->interface;
 		break;
 	case 6: /* Port 6, can be used as a CPU port. */
-		if (priv->p6_interface == state->interface)
+		if (priv->p6_configured)
 			break;
 
 		mt753x_pad_setup(ds, state);
 
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
-
-		priv->p6_interface = state->interface;
 		break;
 	default:
 unsupported:
@@ -2859,12 +2848,12 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
 		else
 			interface = PHY_INTERFACE_MODE_2500BASEX;
 
-		priv->p5_interface = interface;
+		priv->p5_configured = true;
 		break;
 	case 6:
 		interface = PHY_INTERFACE_MODE_2500BASEX;
 
-		priv->p6_interface = interface;
+		priv->p6_configured = true;
 		break;
 	default:
 		return -EINVAL;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index f58828577520..c3a37a0f4843 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -745,8 +745,10 @@ struct mt753x_info {
  * @ports:		Holding the state among ports
  * @reg_mutex:		The lock for protecting among process accessing
  *			registers
- * @p6_interface:	Holding the current port 6 interface
- * @p5_interface:	Holding the current port 5 interface
+ * @p6_configured:	Flag for distinguishing if port 6 of the MT7531 switch
+ *			is already configured
+ * @p5_configured:	Flag for distinguishing if port 5 of the MT7531 switch
+ *			is already configured
  * @p5_intf_sel:	Holding the current port 5 interface select
  * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
  *			has got SGMII
@@ -767,8 +769,8 @@ struct mt7530_priv {
 	const struct mt753x_info *info;
 	unsigned int		id;
 	bool			mcm;
-	phy_interface_t		p6_interface;
-	phy_interface_t		p5_interface;
+	bool			p6_configured;
+	bool			p5_configured;
 	p5_interface_select	p5_intf_sel;
 	bool			p5_sgmii;
 	u8			mirror_rx;
-- 
2.37.2

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

[arinc9.unal]

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.

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.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 2f670e512415..be143da94add 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -933,9 +933,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 */
@@ -965,7 +963,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.37.2

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

[arinc9.unal]

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 call 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()

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.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 be143da94add..58eff6568d4c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -401,7 +401,7 @@ static void mt7530_pll_setup(struct mt7530_priv *priv)
 
 /* 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;
@@ -473,6 +473,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)
 {
@@ -2576,12 +2582,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.37.2

[RFC PATCH net-next 11/22] net: dsa: mt7530: remove pad_setup function pointer

[arinc9.unal]

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.

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
---
 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 58eff6568d4c..a1627e20675d 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -473,18 +473,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)
 {
@@ -2569,14 +2557,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)
@@ -2743,8 +2723,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p6_configured)
 			break;
 
-		mt753x_pad_setup(ds, state);
-
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
 		break;
@@ -3046,11 +3024,6 @@ static int mt753x_set_mac_eee(struct dsa_switch *ds, int port,
 	return 0;
 }
 
-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;
@@ -3112,7 +3085,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,
 	},
@@ -3124,7 +3096,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,
 	},
@@ -3136,7 +3107,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,
@@ -3149,7 +3119,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,
@@ -3179,9 +3148,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 c3a37a0f4843..cad9115de22b 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -696,8 +696,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
@@ -718,7 +716,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.37.2

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

[arinc9.unal]

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

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a1627e20675d..eaa36d41e8b9 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -408,13 +408,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 
 	xtal = mt7530_read(priv, MT7530_HWTRAP) & 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;
@@ -2136,7 +2129,7 @@ mt7530_setup(struct dsa_switch *ds)
 	struct mt7530_dummy_poll p;
 	phy_interface_t interface;
 	struct dsa_port *cpu_dp;
-	u32 id, val;
+	u32 id, val, xtal;
 	int ret, i;
 
 	/* The parent node of master netdev which holds the common system
@@ -2206,6 +2199,15 @@ mt7530_setup(struct dsa_switch *ds)
 		return -ENODEV;
 	}
 
+	xtal = mt7530_read(priv, MT7530_HWTRAP) & 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;
+	}
+
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
-- 
2.37.2

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

[arinc9.unal]

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

Enable port 6 only when port 6 is being used. Update the comment on
mt7530_setup() with a better explanation.

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
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 eaa36d41e8b9..8fe9b1e6932c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -404,7 +404,11 @@ static int
 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, trgint, xtal, val;
+
+	val = mt7530_read(priv, MT7530_MHWTRAP);
+	val &= ~MHWTRAP_P6_DIS;
+	mt7530_write(priv, MT7530_MHWTRAP, val);
 
 	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
 
@@ -2224,9 +2228,9 @@ mt7530_setup(struct dsa_switch *ds)
 		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
 			   RD_TAP_MASK, RD_TAP(16));
 
-	/* Enable port 6 */
+	/* Enable PHY access and operate in manual mode */
 	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.37.2

[RFC PATCH net-next 14/22] net: dsa: mt7530: switch to if/else statements on mt7530_setup_port6()

[arinc9.unal]

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/else statements which simplifies the code.

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

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8fe9b1e6932c..610828b56eac 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -400,11 +400,11 @@ static void mt7530_pll_setup(struct mt7530_priv *priv)
 }
 
 /* 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, val;
+	u32 ncpo1, ssc_delta, xtal, val;
 
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS;
@@ -412,16 +412,18 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 
 	xtal = mt7530_read(priv, MT7530_HWTRAP) & HWTRAP_XTAL_MASK;
 
-	switch (interface) {
-	case PHY_INTERFACE_MODE_RGMII:
-		trgint = 0;
-		break;
-	case PHY_INTERFACE_MODE_TRGMII:
-		trgint = 1;
+	if (interface == PHY_INTERFACE_MODE_RGMII) {
+		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
+			   P6_INTF_MODE(0));
+	} else {
+		mt7530_rmw(priv, MT7530_P6ECR, P6_INTF_MODE_MASK,
+			   P6_INTF_MODE(1));
+
 		if (xtal == HWTRAP_XTAL_25MHZ)
 			ssc_delta = 0x57;
 		else
 			ssc_delta = 0x87;
+
 		if (priv->id == ID_MT7621) {
 			/* PLL frequency: 150MHz: 1.2GBit */
 			if (xtal == HWTRAP_XTAL_40MHZ)
@@ -434,17 +436,7 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 			if (xtal == HWTRAP_XTAL_25MHZ)
 				ncpo1 = 0x1400;
 		}
-		break;
-	default:
-		dev_err(priv->dev, "xMII interface %d not supported\n",
-			interface);
-		return -EINVAL;
-	}
 
-	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);
 
@@ -466,8 +458,6 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 		/* Enable the MT7530 TRGMII clocks */
 		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
 	}
-
-	return 0;
 }
 
 static void
@@ -2568,14 +2558,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) {
 		mt7530_setup_port5(priv->ds, interface);
 	} else if (port == 6) {
-		ret = mt7530_setup_port6(priv->ds, interface);
-		if (ret)
-			return ret;
+		mt7530_setup_port6(priv->ds, interface);
 	}
 
 	return 0;
-- 
2.37.2

[RFC PATCH net-next 15/22] net: dsa: mt7530: set TRGMII RD TAP if trgmii is being used

[arinc9.unal]

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

This code sets the Read Data (RD) TAP value to 16 for all TRGMII control
registers.

The for loop iterates over all the TRGMII control registers, and
mt7530_rmw() function is used to perform a read-modify-write operation on
each register's RD_TAP field to set its value to 16.

This operation is used to tune the timing of the read data signal in
TRGMII to match the TX signal of the link partner.

Run this if trgmii is being used. Since this code doesn't lower the
driving, there's no apparent benefit to run this if trgmii is not being
used.

Add a comment to explain the code.

Thanks to 趙皎宏 (Landen Chao) for pointing out what the code does.

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 610828b56eac..029d3129bb8b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -404,7 +404,7 @@ static void
 mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
-	u32 ncpo1, ssc_delta, xtal, val;
+	u32 ncpo1, ssc_delta, i, xtal, val;
 
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS;
@@ -457,6 +457,11 @@ mt7530_setup_port6(struct dsa_switch *ds, phy_interface_t interface)
 
 		/* Enable the MT7530 TRGMII clocks */
 		core_set(priv, CORE_TRGMII_GSW_CLK_CG, REG_TRGMIICK_EN);
+
+		/* Set the Read Data TAP value of the MT7530 TRGMII */
+		for (i = 0; i < NUM_TRGMII_CTRL; i++)
+			mt7530_rmw(priv, MT7530_TRGMII_RD(i),
+				   RD_TAP_MASK, RD_TAP(16));
 	}
 }
 
@@ -2214,10 +2219,6 @@ mt7530_setup(struct dsa_switch *ds)
 		mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
 			     TD_DM_DRVP(8) | TD_DM_DRVN(8));
 
-	for (i = 0; i < NUM_TRGMII_CTRL; i++)
-		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
-			   RD_TAP_MASK, RD_TAP(16));
-
 	/* Enable PHY access and operate in manual mode */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_PHY_ACCESS;
-- 
2.37.2

[RFC PATCH net-next 16/22] net: dsa: mt7530: move lowering port 5 RGMII driving to mt7530_setup()

[arinc9.unal]

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

Move lowering Tx driving of rgmii on port 5 to right before lowering of Tx
driving of trgmii on port 6 on mt7530_setup().

This way, the switch should consume less power regardless of port 5 being
used.

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 029d3129bb8b..5466259fd99b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -938,10 +938,6 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 		/* P5 RGMII TX Clock Control: delay x */
 		mt7530_write(priv, MT7530_P5RGMIITXCR,
 			     CSR_RGMII_TXC_CFG(0x10 + tx_delay));
-
-		/* reduce P5 RGMII Tx driving, 8mA */
-		mt7530_write(priv, MT7530_IO_DRV_CR,
-			     P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
 	}
 
 	mt7530_write(priv, MT7530_MHWTRAP, val);
@@ -2214,6 +2210,10 @@ mt7530_setup(struct dsa_switch *ds)
 
 	mt7530_pll_setup(priv);
 
+	/* Lower P5 RGMII Tx driving, 8mA */
+	mt7530_write(priv, MT7530_IO_DRV_CR,
+			P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
+
 	/* Lower Tx driving for TRGMII path */
 	for (i = 0; i < NUM_TRGMII_CTRL; i++)
 		mt7530_write(priv, MT7530_TRGMII_TD_ODT(i),
-- 
2.37.2

[RFC PATCH net-next 17/22] net: dsa: mt7530: fix port capabilities for MT7988

[arinc9.unal]

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

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

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.

Fixes: 110c18bfed41 ("net: dsa: mt7530: introduce driver for MT7988 built-in switch")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Acked-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 5466259fd99b..899fc52193e1 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2538,10 +2538,8 @@ 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) {
-	case 0 ... 4: /* Internal phy */
+	case 0 ... 3: /* Internal phy */
 		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
 			  config->supported_interfaces);
 		break;
-- 
2.37.2

[RFC PATCH net-next 18/22] net: dsa: mt7530: remove .mac_port_config for MT7988 and make it optional

[arinc9.unal]

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

For the switch on the MT7988 SoC, the code in mac_port_config for MT7988 is
not needed as the interface of the CPU port is already handled on
mt7988_mac_port_get_caps().

Make .mac_port_config optional. Before calling
priv->info->mac_port_config(), if there's no mac_port_config member in the
priv->info table, exit mt753x_mac_config() successfully.

Remove mac_port_config from the sanity check as the sanity check requires a
pointer to a mac_port_config function to be non-NULL. This will fail for
MT7988 as mac_port_config won't be a member of its info table.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Co-authored-by: Daniel Golle <daniel@makrotopia.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 899fc52193e1..a66a762cb5db 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2619,17 +2619,6 @@ static bool mt753x_is_mac_port(u32 port)
 	return (port == 5 || port == 6);
 }
 
-static int
-mt7988_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
-		  phy_interface_t interface)
-{
-	if (dsa_is_cpu_port(ds, port) &&
-	    interface == PHY_INTERFACE_MODE_INTERNAL)
-		return 0;
-
-	return -EINVAL;
-}
-
 static int
 mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
@@ -2670,6 +2659,9 @@ mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 {
 	struct mt7530_priv *priv = ds->priv;
 
+	if (!priv->info->mac_port_config)
+		return 0;
+
 	return priv->info->mac_port_config(ds, port, mode, state->interface);
 }
 
@@ -3113,7 +3105,6 @@ const struct mt753x_info mt753x_table[] = {
 		.phy_write_c45 = mt7531_ind_c45_phy_write,
 		.cpu_port_config = mt7988_cpu_port_config,
 		.mac_port_get_caps = mt7988_mac_port_get_caps,
-		.mac_port_config = mt7988_mac_config,
 	},
 };
 EXPORT_SYMBOL_GPL(mt753x_table);
@@ -3141,8 +3132,7 @@ mt7530_probe_common(struct mt7530_priv *priv)
 	 * properly.
 	 */
 	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)
+	    !priv->info->phy_write_c22 || !priv->info->mac_port_get_caps)
 		return -EINVAL;
 
 	priv->id = priv->info->id;
-- 
2.37.2

[RFC PATCH net-next 19/22] net: dsa: mt7530: set interrupt register only for MT7530

[arinc9.unal]

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

Setting this register related to interrupts is only needed for the MT7530
switch. Make an exclusive check to ensure this.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 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 a66a762cb5db..ac1e3c58aaac 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2034,7 +2034,7 @@ mt7530_setup_irq(struct mt7530_priv *priv)
 	}
 
 	/* This register must be set for MT7530 to properly fire interrupts */
-	if (priv->id != ID_MT7531)
+	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
 		mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
 
 	ret = request_threaded_irq(priv->irq, NULL, mt7530_irq_thread_fn,
-- 
2.37.2

[RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

[arinc9.unal]

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

Force link-down on all MACs before internal reset. Let's follow suit commit
728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
reset").

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index ac1e3c58aaac..8ece3d0d820c 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
 		return -EINVAL;
 	}
 
+	/* Force link-down on all MACs before internal reset */
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
+
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
-- 
2.37.2

[RFC PATCH net-next 21/22] net: dsa: mt7530: get rid of useless error returns on phylink code path

[arinc9.unal]

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

Remove error returns on the cases where they are already handled with the
function the mac_port_get_caps member points to.

mt7531_mac_config() is also called from mt7531_cpu_port_config() outside of
phylink but the port and interface modes are already handled there.

Change the functions and the mac_port_config function pointer to void now
that there're no error returns anymore.

Remove mt753x_is_mac_port() that used to help the said error returns.

On mt7531_mac_config(), switch to if statements to simplify the code.

Remove internal phy cases from mt753x_phylink_mac_config() as there is no
configuration to be done for them. There's also no need to check the
interface mode as that's already handled with the function the
mac_port_get_caps member points to.

Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 81 ++++++++--------------------------------
 drivers/net/dsa/mt7530.h |  2 +-
 2 files changed, 17 insertions(+), 66 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8ece3d0d820c..3d19e06061cb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2556,7 +2556,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
 	}
 }
 
-static int
+static void
 mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
 {
@@ -2567,22 +2567,14 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	} else if (port == 6) {
 		mt7530_setup_port6(priv->ds, interface);
 	}
-
-	return 0;
 }
 
-static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
-			      phy_interface_t interface,
-			      struct phy_device *phydev)
+static void mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
+			       phy_interface_t interface,
+			       struct phy_device *phydev)
 {
 	u32 val;
 
-	if (priv->p5_sgmii) {
-		dev_err(priv->dev, "RGMII mode is not available for port %d\n",
-			port);
-		return -EINVAL;
-	}
-
 	val = mt7530_read(priv, MT7531_CLKGEN_CTRL);
 	val |= GP_CLK_EN;
 	val &= ~GP_MODE_MASK;
@@ -2610,20 +2602,14 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
 		case PHY_INTERFACE_MODE_RGMII_ID:
 			break;
 		default:
-			return -EINVAL;
+			break;
 		}
 	}
-	mt7530_write(priv, MT7531_CLKGEN_CTRL, val);
 
-	return 0;
-}
-
-static bool mt753x_is_mac_port(u32 port)
-{
-	return (port == 5 || port == 6);
+	mt7530_write(priv, MT7531_CLKGEN_CTRL, val);
 }
 
-static int
+static void
 mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  phy_interface_t interface)
 {
@@ -2631,42 +2617,21 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	struct phy_device *phydev;
 	struct dsa_port *dp;
 
-	if (!mt753x_is_mac_port(port)) {
-		dev_err(priv->dev, "port %d is not a MAC port\n", port);
-		return -EINVAL;
-	}
-
-	switch (interface) {
-	case PHY_INTERFACE_MODE_RGMII:
-	case PHY_INTERFACE_MODE_RGMII_ID:
-	case PHY_INTERFACE_MODE_RGMII_RXID:
-	case PHY_INTERFACE_MODE_RGMII_TXID:
+	if (phy_interface_mode_is_rgmii(interface)) {
 		dp = dsa_to_port(ds, port);
 		phydev = dp->slave->phydev;
-		return mt7531_rgmii_setup(priv, port, interface, phydev);
-	case PHY_INTERFACE_MODE_SGMII:
-	case PHY_INTERFACE_MODE_NA:
-	case PHY_INTERFACE_MODE_1000BASEX:
-	case PHY_INTERFACE_MODE_2500BASEX:
-		/* handled in SGMII PCS driver */
-		return 0;
-	default:
-		return -EINVAL;
+		mt7531_rgmii_setup(priv, port, interface, phydev);
 	}
-
-	return -EINVAL;
 }
 
-static int
+static void
 mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		  const struct phylink_link_state *state)
 {
 	struct mt7530_priv *priv = ds->priv;
 
-	if (!priv->info->mac_port_config)
-		return 0;
-
-	return priv->info->mac_port_config(ds, port, mode, state->interface);
+	if (priv->info->mac_port_config)
+		priv->info->mac_port_config(ds, port, mode, state->interface);
 }
 
 static struct phylink_pcs *
@@ -2695,30 +2660,18 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 	u32 mcr_cur, mcr_new;
 
 	switch (port) {
-	case 0 ... 4: /* Internal phy */
-		if (state->interface != PHY_INTERFACE_MODE_GMII &&
-		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
-			goto unsupported;
-		break;
 	case 5: /* Port 5, can be used as a CPU port. */
 		if (priv->p5_configured)
 			break;
 
-		if (mt753x_mac_config(ds, port, mode, state) < 0)
-			goto unsupported;
+		mt753x_mac_config(ds, port, mode, state);
 		break;
 	case 6: /* Port 6, can be used as a CPU port. */
 		if (priv->p6_configured)
 			break;
 
-		if (mt753x_mac_config(ds, port, mode, state) < 0)
-			goto unsupported;
+		mt753x_mac_config(ds, port, mode, state);
 		break;
-	default:
-unsupported:
-		dev_err(ds->dev, "%s: unsupported %s port: %i\n",
-			__func__, phy_modes(state->interface), port);
-		return;
 	}
 
 	mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));
@@ -2811,7 +2764,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
 	struct mt7530_priv *priv = ds->priv;
 	phy_interface_t interface;
 	int speed;
-	int ret;
 
 	switch (port) {
 	case 5:
@@ -2836,9 +2788,8 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
 	else
 		speed = SPEED_1000;
 
-	ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
-	if (ret)
-		return ret;
+	mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
+
 	mt7530_write(priv, MT7530_PMCR_P(port),
 		     PMCR_CPU_PORT_SETTING(priv->id));
 	mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED,
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index cad9115de22b..ee2b3d2d6258 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -722,7 +722,7 @@ struct mt753x_info {
 	void (*mac_port_validate)(struct dsa_switch *ds, int port,
 				  phy_interface_t interface,
 				  unsigned long *supported);
-	int (*mac_port_config)(struct dsa_switch *ds, int port,
+	void (*mac_port_config)(struct dsa_switch *ds, int port,
 			       unsigned int mode,
 			       phy_interface_t interface);
 };
-- 
2.37.2

[RFC PATCH net-next 22/22] net: dsa: mt7530: rename p5_intf_sel and use only for MT7530 switch

[arinc9.unal]

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

The p5_intf_sel pointer is used to store the information of whether PHY
muxing is used or not. PHY muxing is a feature specific to port 5 of the
MT7530 switch. Do not use it for other switch models.

Rename the pointer to p5_mode to store the mode the port is being used in.
Rename the p5_interface_select enum to mt7530_p5_mode, the string
representation to mt7530_p5_mode_str, and the enum elements.

If PHY muxing is not detected, the default mode, GMAC5, will be used.

Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 61 ++++++++++++++++------------------------
 drivers/net/dsa/mt7530.h | 15 +++++-----
 2 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3d19e06061cb..63b108ef5e0e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -873,19 +873,15 @@ mt7530_set_ageing_time(struct dsa_switch *ds, unsigned int msecs)
 	return 0;
 }
 
-static const char *p5_intf_modes(unsigned int p5_interface)
-{
-	switch (p5_interface) {
-	case P5_DISABLED:
-		return "DISABLED";
-	case P5_INTF_SEL_PHY_P0:
-		return "PHY P0";
-	case P5_INTF_SEL_PHY_P4:
-		return "PHY P4";
-	case P5_INTF_SEL_GMAC5:
-		return "GMAC5";
+static const char *mt7530_p5_mode_str(unsigned int mode)
+{
+	switch (mode) {
+	case MUX_PHY_P0:
+		return "MUX PHY P0";
+	case MUX_PHY_P4:
+		return "MUX PHY P4";
 	default:
-		return "unknown";
+		return "GMAC5";
 	}
 }
 
@@ -902,23 +898,21 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 	val |= MHWTRAP_MANUAL | MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
 	val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
 
-	switch (priv->p5_intf_sel) {
-	case P5_INTF_SEL_PHY_P0:
-		/* MT7530_P5_MODE_GPHY_P0: 2nd GMAC -> P5 -> P0 */
+	switch (priv->p5_mode) {
+	case MUX_PHY_P0:
+		/* MUX_PHY_P0: P0 -> P5 -> SoC MAC */
 		val |= MHWTRAP_PHY0_SEL;
 		fallthrough;
-	case P5_INTF_SEL_PHY_P4:
-		/* MT7530_P5_MODE_GPHY_P4: 2nd GMAC -> P5 -> P4 */
+	case MUX_PHY_P4:
+		/* MUX_PHY_P4: P4 -> P5 -> SoC MAC */
 		val &= ~MHWTRAP_P5_MAC_SEL & ~MHWTRAP_P5_DIS;
 
 		/* Setup the MAC by default for the cpu port */
 		mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
 		break;
-	case P5_INTF_SEL_GMAC5:
-		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
-		val &= ~MHWTRAP_P5_DIS;
-		break;
 	default:
+		/* GMAC5: P5 -> SoC MAC or external PHY */
+		val &= ~MHWTRAP_P5_DIS;
 		break;
 	}
 
@@ -942,8 +936,8 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 
 	mt7530_write(priv, MT7530_MHWTRAP, val);
 
-	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));
+	dev_dbg(ds->dev, "Setup P5, HWTRAP=0x%x, mode=%s, phy-mode=%s\n", val,
+		mt7530_p5_mode_str(priv->p5_mode), phy_modes(interface));
 
 	mutex_unlock(&priv->reg_mutex);
 }
@@ -2261,13 +2255,11 @@ mt7530_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
-	/* Setup port 5 */
-	if (!dsa_is_unused_port(ds, 5)) {
-		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
-	} else {
+	/* Check for PHY muxing on port 5 */
+	if (dsa_is_unused_port(ds, 5)) {
 		/* Scan the ethernet nodes. Look for GMAC1, lookup the used PHY.
-		 * Set priv->p5_intf_sel to the appropriate value if PHY muxing
-		 * is detected.
+		 * Set priv->p5_mode to the appropriate value if PHY muxing is
+		 * detected.
 		 */
 		for_each_child_of_node(dn, mac_np) {
 			if (!of_device_is_compatible(mac_np,
@@ -2291,17 +2283,17 @@ mt7530_setup(struct dsa_switch *ds)
 				}
 				id = of_mdio_parse_addr(ds->dev, phy_node);
 				if (id == 0)
-					priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
+					priv->p5_mode = MUX_PHY_P0;
 				if (id == 4)
-					priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
+					priv->p5_mode = MUX_PHY_P4;
 			}
 			of_node_put(mac_np);
 			of_node_put(phy_node);
 			break;
 		}
 
-		if (priv->p5_intf_sel == P5_INTF_SEL_PHY_P0 ||
-		    priv->p5_intf_sel == P5_INTF_SEL_PHY_P4)
+		if (priv->p5_mode == MUX_PHY_P0 ||
+		    priv->p5_mode == MUX_PHY_P4)
 			mt7530_setup_port5(ds, interface);
 	}
 
@@ -2444,9 +2436,6 @@ mt7531_setup(struct dsa_switch *ds)
 			   MT7531_EXT_P_MDIO_12);
 	}
 
-	if (!dsa_is_unused_port(ds, 5))
-		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
-
 	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
 		   MT7531_GPIO0_INTERRUPT);
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index ee2b3d2d6258..8187d77603f8 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -673,13 +673,12 @@ struct mt7530_port {
 	struct phylink_pcs *sgmii_pcs;
 };
 
-/* Port 5 interface select definitions */
+/* Port 5 mode definitions of the MT7530 switch */
 typedef enum {
-	P5_DISABLED,
-	P5_INTF_SEL_PHY_P0,
-	P5_INTF_SEL_PHY_P4,
-	P5_INTF_SEL_GMAC5,
-} p5_interface_select;
+	GMAC5,
+	MUX_PHY_P0,
+	MUX_PHY_P4,
+} mt7530_p5_mode;
 
 struct mt7530_priv;
 
@@ -746,7 +745,7 @@ struct mt753x_info {
  *			is already configured
  * @p5_configured:	Flag for distinguishing if port 5 of the MT7531 switch
  *			is already configured
- * @p5_intf_sel:	Holding the current port 5 interface select
+ * @p5_mode:		Holding the current port 5 mode of the MT7530 switch
  * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
  *			has got SGMII
  * @irq:		IRQ number of the switch
@@ -768,7 +767,7 @@ struct mt7530_priv {
 	bool			mcm;
 	bool			p6_configured;
 	bool			p5_configured;
-	p5_interface_select	p5_intf_sel;
+	mt7530_p5_mode		p5_mode;
 	bool			p5_sgmii;
 	u8			mirror_rx;
 	u8			mirror_tx;
-- 
2.37.2

Re: [RFC PATCH net-next 03/22] net: dsa: mt7530: properly support MT7531AE and MT7531BE

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:29PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Introduce the p5_sgmii pointer to store the information for whether port 5
> has got SGMII or not. Print "found MT7531AE" if it's got it, print "found
> MT7531BE" if it hasn't.
> 
> Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
> switch is identified.
> 
> Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
> information. Address the code where mt7531_dual_sgmii_supported() is used.
> 
> Get rid of mt7531_is_rgmii_port() which just prints the opposite of
> priv->p5_sgmii.
> 
> Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
> represent the mode that port 5 is used in, not the hardware information of
> port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if port 5 is not
> dsa_is_unused_port().
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530-mdio.c |  7 ++---
>  drivers/net/dsa/mt7530.c      | 49 +++++++++++++++--------------------
>  drivers/net/dsa/mt7530.h      |  6 +++--
>  3 files changed, 27 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
> index 088533663b83..fa3ee85a99c1 100644
> --- a/drivers/net/dsa/mt7530-mdio.c
> +++ b/drivers/net/dsa/mt7530-mdio.c
> @@ -81,17 +81,14 @@ static const struct regmap_bus mt7530_regmap_bus = {
>  };
>  
>  static int
> -mt7531_create_sgmii(struct mt7530_priv *priv, bool dual_sgmii)
> +mt7531_create_sgmii(struct mt7530_priv *priv)
>  {
>  	struct regmap_config *mt7531_pcs_config[2] = {};
>  	struct phylink_pcs *pcs;
>  	struct regmap *regmap;
>  	int i, ret = 0;
>  
> -	/* MT7531AE has two SGMII units for port 5 and port 6
> -	 * MT7531BE has only one SGMII unit for port 6
> -	 */
> -	for (i = dual_sgmii ? 0 : 1; i < 2; i++) {
> +	for (i = priv->p5_sgmii ? 0 : 1; i < 2; i++) {
>  		mt7531_pcs_config[i] = devm_kzalloc(priv->dev,
>  						    sizeof(struct regmap_config),
>  						    GFP_KERNEL);
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index c680873819b0..edc34be745b2 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -473,15 +473,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>  	return 0;
>  }
>  
> -static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
> -{
> -	u32 val;
> -
> -	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
> -
> -	return (val & PAD_DUAL_SGMII_EN) != 0;
> -}
> -
>  static int
>  mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>  {
> @@ -496,7 +487,7 @@ mt7531_pll_setup(struct mt7530_priv *priv)
>  	u32 xtal;
>  	u32 val;
>  
> -	if (mt7531_dual_sgmii_supported(priv))
> +	if (priv->p5_sgmii)
>  		return;
>  
>  	val = mt7530_read(priv, MT7531_CREV);
> @@ -907,8 +898,6 @@ static const char *p5_intf_modes(unsigned int p5_interface)
>  		return "PHY P4";
>  	case P5_INTF_SEL_GMAC5:
>  		return "GMAC5";
> -	case P5_INTF_SEL_GMAC5_SGMII:
> -		return "GMAC5_SGMII";
>  	default:
>  		return "unknown";
>  	}
> @@ -2440,6 +2429,18 @@ mt7531_setup(struct dsa_switch *ds)
>  		return -ENODEV;
>  	}
>  
> +	/* MT7531AE has got two SGMII units. One for port 5, one for port 6.
> +	 * MT7531BE has got only one SGMII unit which is for port 6.
> +	 */
> +	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
> +
> +	if ((val & PAD_DUAL_SGMII_EN) != 0) {
> +		priv->p5_sgmii = true;
> +		dev_info(priv->dev, "found MT7531AE\n");
> +	} else {
> +		dev_info(priv->dev, "found MT7531BE\n");

I don't think dev_info is useful here for regular users.
If you really want this output, use dev_dbg to reduce log pollution.
Imho completely removing the else branch and only silently
setting priv->p5_sgmii is sufficient, as users can also turn on
dyndbg for drivers/net/pcs/pcs-mtk-lynxi.c and will then be informed
about the created SGMII/1000Base-X/2500Base-X PCS instances.


> +	}
> +
>  	/* all MACs must be forced link-down before sw reset */
>  	for (i = 0; i < MT7530_NUM_PORTS; i++)
>  		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
> @@ -2451,19 +2452,16 @@ mt7531_setup(struct dsa_switch *ds)
>  
>  	mt7531_pll_setup(priv);
>  
> -	if (mt7531_dual_sgmii_supported(priv)) {
> -		priv->p5_intf_sel = P5_INTF_SEL_GMAC5_SGMII;
> -
> +	if (priv->p5_sgmii) {
>  		/* Let ds->slave_mii_bus be able to access external phy. */
>  		mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO11_RG_RXD2_MASK,
>  			   MT7531_EXT_P_MDC_11);
>  		mt7530_rmw(priv, MT7531_GPIO_MODE1, MT7531_GPIO12_RG_RXD3_MASK,
>  			   MT7531_EXT_P_MDIO_12);
> -	} else {
> -		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
>  	}
> -	dev_dbg(ds->dev, "P5 support %s interface\n",
> -		p5_intf_modes(priv->p5_intf_sel));
> +
> +	if (!dsa_is_unused_port(ds, 5))
> +		priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
>  
>  	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
>  		   MT7531_GPIO0_INTERRUPT);
> @@ -2523,11 +2521,6 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
>  	}
>  }
>  
> -static bool mt7531_is_rgmii_port(struct mt7530_priv *priv, u32 port)
> -{
> -	return (port == 5) && (priv->p5_intf_sel != P5_INTF_SEL_GMAC5_SGMII);
> -}
> -
>  static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
>  				     struct phylink_config *config)
>  {
> @@ -2540,7 +2533,7 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
>  		break;
>  
>  	case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
> -		if (mt7531_is_rgmii_port(priv, port)) {
> +		if (!priv->p5_sgmii) {
>  			phy_interface_set_rgmii(config->supported_interfaces);
>  			break;
>  		}
> @@ -2607,7 +2600,7 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
>  {
>  	u32 val;
>  
> -	if (!mt7531_is_rgmii_port(priv, port)) {
> +	if (priv->p5_sgmii) {
>  		dev_err(priv->dev, "RGMII mode is not available for port %d\n",
>  			port);
>  		return -EINVAL;
> @@ -2860,7 +2853,7 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
>  
>  	switch (port) {
>  	case 5:
> -		if (mt7531_is_rgmii_port(priv, port))
> +		if (!priv->p5_sgmii)
>  			interface = PHY_INTERFACE_MODE_RGMII;
>  		else
>  			interface = PHY_INTERFACE_MODE_2500BASEX;
> @@ -3019,7 +3012,7 @@ mt753x_setup(struct dsa_switch *ds)
>  		mt7530_free_irq_common(priv);
>  
>  	if (priv->create_sgmii) {
> -		ret = priv->create_sgmii(priv, mt7531_dual_sgmii_supported(priv));
> +		ret = priv->create_sgmii(priv);
>  		if (ret && priv->irq)
>  			mt7530_free_irq(priv);
>  	}
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 703f8a528317..f58828577520 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -679,7 +679,6 @@ typedef enum {
>  	P5_INTF_SEL_PHY_P0,
>  	P5_INTF_SEL_PHY_P4,
>  	P5_INTF_SEL_GMAC5,
> -	P5_INTF_SEL_GMAC5_SGMII,
>  } p5_interface_select;
>  
>  struct mt7530_priv;
> @@ -749,6 +748,8 @@ struct mt753x_info {
>   * @p6_interface:	Holding the current port 6 interface
>   * @p5_interface:	Holding the current port 5 interface
>   * @p5_intf_sel:	Holding the current port 5 interface select
> + * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
> + *			has got SGMII
>   * @irq:		IRQ number of the switch
>   * @irq_domain:		IRQ domain of the switch irq_chip
>   * @irq_enable:		IRQ enable bits, synced to SYS_INT_EN
> @@ -769,6 +770,7 @@ struct mt7530_priv {
>  	phy_interface_t		p6_interface;
>  	phy_interface_t		p5_interface;
>  	p5_interface_select	p5_intf_sel;
> +	bool			p5_sgmii;
>  	u8			mirror_rx;
>  	u8			mirror_tx;
>  	struct mt7530_port	ports[MT7530_NUM_PORTS];
> @@ -778,7 +780,7 @@ struct mt7530_priv {
>  	int irq;
>  	struct irq_domain *irq_domain;
>  	u32 irq_enable;
> -	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
> +	int (*create_sgmii)(struct mt7530_priv *priv);
>  };
>  
>  struct mt7530_hw_vlan_entry {
> -- 
> 2.37.2
> 

Re: [RFC PATCH net-next 01/22] net: dsa: mt7530: add missing @p5_interface to mt7530_priv description

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:27PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Add the missing p5_interface field to the mt7530_priv description. Sort out
> the description in the process.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

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

> ---
>  drivers/net/dsa/mt7530.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 5084f48a8869..845f5dd16d83 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -746,7 +746,8 @@ struct mt753x_info {
>   * @ports:		Holding the state among ports
>   * @reg_mutex:		The lock for protecting among process accessing
>   *			registers
> - * @p6_interface	Holding the current port 6 interface
> + * @p6_interface:	Holding the current port 6 interface
> + * @p5_interface:	Holding the current port 5 interface
>   * @p5_intf_sel:	Holding the current port 5 interface select
>   * @irq:		IRQ number of the switch
>   * @irq_domain:		IRQ domain of the switch irq_chip
> -- 
> 2.37.2
> 

Re: [RFC PATCH net-next 02/22] net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:28PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Use the p5_interface_select enumeration as the data type for the
> p5_intf_sel field. This ensures p5_intf_sel can only take the values
> defined in the p5_interface_select enumeration.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index 845f5dd16d83..703f8a528317 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -674,13 +674,13 @@ struct mt7530_port {
>  };
>  
>  /* Port 5 interface select definitions */
> -enum p5_interface_select {
> -	P5_DISABLED = 0,
> +typedef enum {

We usually avoid adding typedef in kernel code. If the purpose is
just to be more verbose in the struct definition, you can as well
also just use 'enum p5_interface_select as type in the struct.

> +	P5_DISABLED,
>  	P5_INTF_SEL_PHY_P0,
>  	P5_INTF_SEL_PHY_P4,
>  	P5_INTF_SEL_GMAC5,
>  	P5_INTF_SEL_GMAC5_SGMII,
> -};
> +} p5_interface_select;
>  
>  struct mt7530_priv;
>  
> @@ -768,7 +768,7 @@ struct mt7530_priv {
>  	bool			mcm;
>  	phy_interface_t		p6_interface;
>  	phy_interface_t		p5_interface;
> -	unsigned int		p5_intf_sel;
> +	p5_interface_select	p5_intf_sel;

enum p5_interface_select	p5_intf_sel;

>  	u8			mirror_rx;
>  	u8			mirror_tx;
>  	struct mt7530_port	ports[MT7530_NUM_PORTS];
> -- 
> 2.37.2
> 

Re: [RFC PATCH net-next 04/22] net: dsa: mt7530: improve comments regarding port 5 and 6

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:30PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> There's no logic to numerically order the CPU ports. State the port number
> and its capability of being used as a CPU port instead.
> 
> Remove the irrelevant PHY muxing information from
> mt7530_mac_port_get_caps(). Explain the supported MII modes instead.
> 
> Remove the out of place PHY muxing information from
> mt753x_phylink_mac_config(). The function is for both the MT7530 and MT7531
> switches but there's no PHY muxing on MT7531.
> 
> These comments were gradually introduced with the commits below.
> ca366d6c889b ("net: dsa: mt7530: Convert to PHYLINK API")
> 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> 88bdef8be9f6 ("net: dsa: mt7530: Extend device data ready for adding a new
> hardware")
> c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

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

> ---
>  drivers/net/dsa/mt7530.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index edc34be745b2..e956ffa1eea8 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2504,7 +2504,9 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
>  			  config->supported_interfaces);
>  		break;
>  
> -	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
> +	case 5: /* Port 5 which can be used as a CPU port supports rgmii with
> +		 * delays, mii, and gmii.
> +		 */
>  		phy_interface_set_rgmii(config->supported_interfaces);
>  		__set_bit(PHY_INTERFACE_MODE_MII,
>  			  config->supported_interfaces);
> @@ -2512,7 +2514,9 @@ static void mt7530_mac_port_get_caps(struct dsa_switch *ds, int port,
>  			  config->supported_interfaces);
>  		break;
>  
> -	case 6: /* 1st cpu port */
> +	case 6: /* Port 6 which can be used as a CPU port supports rgmii and
> +		 * trgmii.
> +		 */
>  		__set_bit(PHY_INTERFACE_MODE_RGMII,
>  			  config->supported_interfaces);
>  		__set_bit(PHY_INTERFACE_MODE_TRGMII,
> @@ -2532,14 +2536,17 @@ static void mt7531_mac_port_get_caps(struct dsa_switch *ds, int port,
>  			  config->supported_interfaces);
>  		break;
>  
> -	case 5: /* 2nd cpu port supports either rgmii or sgmii/8023z */
> +	case 5: /* Port 5 which can be used as a CPU port supports rgmii with
> +		 * delays on MT7531BE, sgmii/802.3z on MT7531AE.
> +		 */
>  		if (!priv->p5_sgmii) {
>  			phy_interface_set_rgmii(config->supported_interfaces);
>  			break;
>  		}
>  		fallthrough;
>  
> -	case 6: /* 1st cpu port supports sgmii/8023z only */
> +	case 6: /* Port 6 which can be used as a CPU port supports sgmii/802.3z.
> +		 */
>  		__set_bit(PHY_INTERFACE_MODE_SGMII,
>  			  config->supported_interfaces);
>  		__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> @@ -2731,7 +2738,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
>  			goto unsupported;
>  		break;
> -	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
> +	case 5: /* Port 5, can be used as a CPU port. */
>  		if (priv->p5_interface == state->interface)
>  			break;
>  
> @@ -2741,7 +2748,7 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		if (priv->p5_intf_sel != P5_DISABLED)
>  			priv->p5_interface = state->interface;
>  		break;
> -	case 6: /* 1st cpu port */
> +	case 6: /* Port 6, can be used as a CPU port. */
>  		if (priv->p6_interface == state->interface)
>  			break;
>  
> -- 
> 2.37.2
> 

Re: [RFC PATCH net-next 08/22] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> The idea of p5_interface and p6_interface pointers is to prevent
> mt753x_mac_config() from running twice for MT7531, as it's already run with
> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is used as
> a CPU port.
> 
> Change p5_interface and p6_interface to p5_configured and p6_configured.
> Make them boolean.
> 
> Do not set them for any other reason.
> 
> The priv->p5_intf_sel check is useless as in this code path, it will always
> be P5_INTF_SEL_GMAC5.
> 
> There was also no need to set priv->p5_interface and priv->p6_interface to
> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
> already be set to that when "priv" is allocated. The pointers were of the
> phy_interface_t enumeration type, and the first element of the enum is
> PHY_INTERFACE_MODE_NA. There was nothing in between that would change this
> beforehand.
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

NACK. This assumes that a user port is configured exactly once.
However, interface mode may change because of mode-changing PHYs (e.g.
often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
2500M, ie. depending on actual link speed).

Also when using SFP modules (which can be hotplugged) the interface
mode may change after initially setting up the driver, e.g. when SFP
driver is loaded or a module is plugged or replaced.

> ---
>  drivers/net/dsa/mt7530.c | 19 ++++---------------
>  drivers/net/dsa/mt7530.h | 10 ++++++----
>  2 files changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index bac2388319a3..2f670e512415 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2237,8 +2237,6 @@ mt7530_setup(struct dsa_switch *ds)
>  	val |= MHWTRAP_MANUAL;
>  	mt7530_write(priv, MT7530_MHWTRAP, val);
>  
> -	priv->p6_interface = PHY_INTERFACE_MODE_NA;
> -
>  	/* Enable and reset MIB counters */
>  	mt7530_mib_reset(ds);
>  
> @@ -2460,10 +2458,6 @@ mt7531_setup(struct dsa_switch *ds)
>  	mt7530_rmw(priv, MT7531_GPIO_MODE0, MT7531_GPIO0_MASK,
>  		   MT7531_GPIO0_INTERRUPT);
>  
> -	/* Let phylink decide the interface later. */
> -	priv->p5_interface = PHY_INTERFACE_MODE_NA;
> -	priv->p6_interface = PHY_INTERFACE_MODE_NA;
> -
>  	/* Enable PHY core PLL, since phy_device has not yet been created
>  	 * provided for phy_[read,write]_mmd_indirect is called, we provide
>  	 * our own mt7531_ind_mmd_phy_[read,write] to complete this
> @@ -2733,25 +2727,20 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  			goto unsupported;
>  		break;
>  	case 5: /* Port 5, can be used as a CPU port. */
> -		if (priv->p5_interface == state->interface)
> +		if (priv->p5_configured)
>  			break;
>  
>  		if (mt753x_mac_config(ds, port, mode, state) < 0)
>  			goto unsupported;
> -
> -		if (priv->p5_intf_sel != P5_DISABLED)
> -			priv->p5_interface = state->interface;
>  		break;
>  	case 6: /* Port 6, can be used as a CPU port. */
> -		if (priv->p6_interface == state->interface)
> +		if (priv->p6_configured)
>  			break;
>  
>  		mt753x_pad_setup(ds, state);
>  
>  		if (mt753x_mac_config(ds, port, mode, state) < 0)
>  			goto unsupported;
> -
> -		priv->p6_interface = state->interface;
>  		break;
>  	default:
>  unsupported:
> @@ -2859,12 +2848,12 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
>  		else
>  			interface = PHY_INTERFACE_MODE_2500BASEX;
>  
> -		priv->p5_interface = interface;
> +		priv->p5_configured = true;
>  		break;
>  	case 6:
>  		interface = PHY_INTERFACE_MODE_2500BASEX;
>  
> -		priv->p6_interface = interface;
> +		priv->p6_configured = true;
>  		break;
>  	default:
>  		return -EINVAL;
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index f58828577520..c3a37a0f4843 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -745,8 +745,10 @@ struct mt753x_info {
>   * @ports:		Holding the state among ports
>   * @reg_mutex:		The lock for protecting among process accessing
>   *			registers
> - * @p6_interface:	Holding the current port 6 interface
> - * @p5_interface:	Holding the current port 5 interface
> + * @p6_configured:	Flag for distinguishing if port 6 of the MT7531 switch
> + *			is already configured
> + * @p5_configured:	Flag for distinguishing if port 5 of the MT7531 switch
> + *			is already configured
>   * @p5_intf_sel:	Holding the current port 5 interface select
>   * @p5_sgmii:		Flag for distinguishing if port 5 of the MT7531 switch
>   *			has got SGMII
> @@ -767,8 +769,8 @@ struct mt7530_priv {
>  	const struct mt753x_info *info;
>  	unsigned int		id;
>  	bool			mcm;
> -	phy_interface_t		p6_interface;
> -	phy_interface_t		p5_interface;
> +	bool			p6_configured;
> +	bool			p5_configured;
>  	p5_interface_select	p5_intf_sel;
>  	bool			p5_sgmii;
>  	u8			mirror_rx;
> -- 
> 2.37.2
> 

Re: [RFC PATCH net-next 08/22] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

[Arınç ÜNAL]

On 21.04.2023 20:28, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> The idea of p5_interface and p6_interface pointers is to prevent
>> mt753x_mac_config() from running twice for MT7531, as it's already run with
>> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is used as
>> a CPU port.
>>
>> Change p5_interface and p6_interface to p5_configured and p6_configured.
>> Make them boolean.
>>
>> Do not set them for any other reason.
>>
>> The priv->p5_intf_sel check is useless as in this code path, it will always
>> be P5_INTF_SEL_GMAC5.
>>
>> There was also no need to set priv->p5_interface and priv->p6_interface to
>> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
>> already be set to that when "priv" is allocated. The pointers were of the
>> phy_interface_t enumeration type, and the first element of the enum is
>> PHY_INTERFACE_MODE_NA. There was nothing in between that would change this
>> beforehand.
>>
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> NACK. This assumes that a user port is configured exactly once.
> However, interface mode may change because of mode-changing PHYs (e.g.
> often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
> 2500M, ie. depending on actual link speed).
> 
> Also when using SFP modules (which can be hotplugged) the interface
> mode may change after initially setting up the driver, e.g. when SFP
> driver is loaded or a module is plugged or replaced.

I'm not sure I understand. pX_configured would be set to true only when 
the port is used as a CPU port. mt753x_mac_config() should run for user 
or DSA ports more than once, if needed.

Arınç

Re: [RFC PATCH net-next 08/22] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

[Arınç ÜNAL]

On 21.04.2023 21:17, Arınç ÜNAL wrote:
> On 21.04.2023 20:28, Daniel Golle wrote:
>> On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>
>>> The idea of p5_interface and p6_interface pointers is to prevent
>>> mt753x_mac_config() from running twice for MT7531, as it's already 
>>> run with
>>> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is 
>>> used as
>>> a CPU port.
>>>
>>> Change p5_interface and p6_interface to p5_configured and p6_configured.
>>> Make them boolean.
>>>
>>> Do not set them for any other reason.
>>>
>>> The priv->p5_intf_sel check is useless as in this code path, it will 
>>> always
>>> be P5_INTF_SEL_GMAC5.
>>>
>>> There was also no need to set priv->p5_interface and 
>>> priv->p6_interface to
>>> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they would
>>> already be set to that when "priv" is allocated. The pointers were of 
>>> the
>>> phy_interface_t enumeration type, and the first element of the enum is
>>> PHY_INTERFACE_MODE_NA. There was nothing in between that would change 
>>> this
>>> beforehand.
>>>
>>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> NACK. This assumes that a user port is configured exactly once.
>> However, interface mode may change because of mode-changing PHYs (e.g.
>> often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
>> 2500M, ie. depending on actual link speed).
>>
>> Also when using SFP modules (which can be hotplugged) the interface
>> mode may change after initially setting up the driver, e.g. when SFP
>> driver is loaded or a module is plugged or replaced.
> 
> I'm not sure I understand. pX_configured would be set to true only when 
> the port is used as a CPU port. mt753x_mac_config() should run for user 
> or DSA ports more than once, if needed.

Looking at this again, once pX_interface is true, the check will prevent 
even user or DSA ports to be configured again. What about setting 
pX_interface to false after mt753x_mac_config() is run?

Arınç

Re: [RFC PATCH net-next 08/22] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

[Arınç ÜNAL]

On 21.04.2023 21:20, Arınç ÜNAL wrote:
> On 21.04.2023 21:17, Arınç ÜNAL wrote:
>> On 21.04.2023 20:28, Daniel Golle wrote:
>>> On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
>>>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>>
>>>> The idea of p5_interface and p6_interface pointers is to prevent
>>>> mt753x_mac_config() from running twice for MT7531, as it's already 
>>>> run with
>>>> mt753x_cpu_port_enable() from mt7531_setup_common(), if the port is 
>>>> used as
>>>> a CPU port.
>>>>
>>>> Change p5_interface and p6_interface to p5_configured and 
>>>> p6_configured.
>>>> Make them boolean.
>>>>
>>>> Do not set them for any other reason.
>>>>
>>>> The priv->p5_intf_sel check is useless as in this code path, it will 
>>>> always
>>>> be P5_INTF_SEL_GMAC5.
>>>>
>>>> There was also no need to set priv->p5_interface and 
>>>> priv->p6_interface to
>>>> PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup() as they 
>>>> would
>>>> already be set to that when "priv" is allocated. The pointers were 
>>>> of the
>>>> phy_interface_t enumeration type, and the first element of the enum is
>>>> PHY_INTERFACE_MODE_NA. There was nothing in between that would 
>>>> change this
>>>> beforehand.
>>>>
>>>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>>>
>>> NACK. This assumes that a user port is configured exactly once.
>>> However, interface mode may change because of mode-changing PHYs (e.g.
>>> often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
>>> 2500M, ie. depending on actual link speed).
>>>
>>> Also when using SFP modules (which can be hotplugged) the interface
>>> mode may change after initially setting up the driver, e.g. when SFP
>>> driver is loaded or a module is plugged or replaced.
>>
>> I'm not sure I understand. pX_configured would be set to true only 
>> when the port is used as a CPU port. mt753x_mac_config() should run 
>> for user or DSA ports more than once, if needed.
> 
> Looking at this again, once pX_interface is true, the check will prevent 
> even user or DSA ports to be configured again. What about setting 
> pX_interface to false after mt753x_mac_config() is run?

On a third thought, pX_interface will never be true for the port if it's 
a user or DSA port so this should not be a problem at all.

Arınç

Re: [RFC PATCH net-next 02/22] net: dsa: mt7530: use p5_interface_select as data type for p5_intf_sel

[Arınç ÜNAL]

On 21.04.2023 19:23, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:28PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Use the p5_interface_select enumeration as the data type for the
>> p5_intf_sel field. This ensures p5_intf_sel can only take the values
>> defined in the p5_interface_select enumeration.
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index 845f5dd16d83..703f8a528317 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -674,13 +674,13 @@ struct mt7530_port {
>>   };
>>   
>>   /* Port 5 interface select definitions */
>> -enum p5_interface_select {
>> -	P5_DISABLED = 0,
>> +typedef enum {
> 
> We usually avoid adding typedef in kernel code. If the purpose is
> just to be more verbose in the struct definition, you can as well
> also just use 'enum p5_interface_select as type in the struct.
> 
>> +	P5_DISABLED,
>>   	P5_INTF_SEL_PHY_P0,
>>   	P5_INTF_SEL_PHY_P4,
>>   	P5_INTF_SEL_GMAC5,
>>   	P5_INTF_SEL_GMAC5_SGMII,
>> -};
>> +} p5_interface_select;
>>   
>>   struct mt7530_priv;
>>   
>> @@ -768,7 +768,7 @@ struct mt7530_priv {
>>   	bool			mcm;
>>   	phy_interface_t		p6_interface;
>>   	phy_interface_t		p5_interface;
>> -	unsigned int		p5_intf_sel;
>> +	p5_interface_select	p5_intf_sel;
> 
> enum p5_interface_select	p5_intf_sel;

Will do, thanks.

Arınç

Re: [RFC PATCH net-next 03/22] net: dsa: mt7530: properly support MT7531AE and MT7531BE

[Arınç ÜNAL]

On 21.04.2023 18:22, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:29PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Introduce the p5_sgmii pointer to store the information for whether port 5
>> has got SGMII or not. Print "found MT7531AE" if it's got it, print "found
>> MT7531BE" if it hasn't.
>>
>> Move the comment about MT7531AE and MT7531BE to mt7531_setup(), where the
>> switch is identified.
>>
>> Get rid of mt7531_dual_sgmii_supported() now that priv->p5_sgmii stores the
>> information. Address the code where mt7531_dual_sgmii_supported() is used.
>>
>> Get rid of mt7531_is_rgmii_port() which just prints the opposite of
>> priv->p5_sgmii.
>>
>> Remove P5_INTF_SEL_GMAC5_SGMII. The p5_interface_select enum is supposed to
>> represent the mode that port 5 is used in, not the hardware information of
>> port 5. Set p5_intf_sel to P5_INTF_SEL_GMAC5 instead, if port 5 is not
>> dsa_is_unused_port().
>>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530-mdio.c |  7 ++---
>>   drivers/net/dsa/mt7530.c      | 49 +++++++++++++++--------------------
>>   drivers/net/dsa/mt7530.h      |  6 +++--
>>   3 files changed, 27 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
>> index 088533663b83..fa3ee85a99c1 100644
>> --- a/drivers/net/dsa/mt7530-mdio.c
>> +++ b/drivers/net/dsa/mt7530-mdio.c
>> @@ -81,17 +81,14 @@ static const struct regmap_bus mt7530_regmap_bus = {
>>   };
>>   
>>   static int
>> -mt7531_create_sgmii(struct mt7530_priv *priv, bool dual_sgmii)
>> +mt7531_create_sgmii(struct mt7530_priv *priv)
>>   {
>>   	struct regmap_config *mt7531_pcs_config[2] = {};
>>   	struct phylink_pcs *pcs;
>>   	struct regmap *regmap;
>>   	int i, ret = 0;
>>   
>> -	/* MT7531AE has two SGMII units for port 5 and port 6
>> -	 * MT7531BE has only one SGMII unit for port 6
>> -	 */
>> -	for (i = dual_sgmii ? 0 : 1; i < 2; i++) {
>> +	for (i = priv->p5_sgmii ? 0 : 1; i < 2; i++) {
>>   		mt7531_pcs_config[i] = devm_kzalloc(priv->dev,
>>   						    sizeof(struct regmap_config),
>>   						    GFP_KERNEL);
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index c680873819b0..edc34be745b2 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -473,15 +473,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   	return 0;
>>   }
>>   
>> -static bool mt7531_dual_sgmii_supported(struct mt7530_priv *priv)
>> -{
>> -	u32 val;
>> -
>> -	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
>> -
>> -	return (val & PAD_DUAL_SGMII_EN) != 0;
>> -}
>> -
>>   static int
>>   mt7531_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   {
>> @@ -496,7 +487,7 @@ mt7531_pll_setup(struct mt7530_priv *priv)
>>   	u32 xtal;
>>   	u32 val;
>>   
>> -	if (mt7531_dual_sgmii_supported(priv))
>> +	if (priv->p5_sgmii)
>>   		return;
>>   
>>   	val = mt7530_read(priv, MT7531_CREV);
>> @@ -907,8 +898,6 @@ static const char *p5_intf_modes(unsigned int p5_interface)
>>   		return "PHY P4";
>>   	case P5_INTF_SEL_GMAC5:
>>   		return "GMAC5";
>> -	case P5_INTF_SEL_GMAC5_SGMII:
>> -		return "GMAC5_SGMII";
>>   	default:
>>   		return "unknown";
>>   	}
>> @@ -2440,6 +2429,18 @@ mt7531_setup(struct dsa_switch *ds)
>>   		return -ENODEV;
>>   	}
>>   
>> +	/* MT7531AE has got two SGMII units. One for port 5, one for port 6.
>> +	 * MT7531BE has got only one SGMII unit which is for port 6.
>> +	 */
>> +	val = mt7530_read(priv, MT7531_TOP_SIG_SR);
>> +
>> +	if ((val & PAD_DUAL_SGMII_EN) != 0) {
>> +		priv->p5_sgmii = true;
>> +		dev_info(priv->dev, "found MT7531AE\n");
>> +	} else {
>> +		dev_info(priv->dev, "found MT7531BE\n");
> 
> I don't think dev_info is useful here for regular users.
> If you really want this output, use dev_dbg to reduce log pollution.
> Imho completely removing the else branch and only silently
> setting priv->p5_sgmii is sufficient, as users can also turn on
> dyndbg for drivers/net/pcs/pcs-mtk-lynxi.c and will then be informed
> about the created SGMII/1000Base-X/2500Base-X PCS instances.

Sounds good, I'll drop it.

Arınç

Re: [RFC PATCH net-next 19/22] net: dsa: mt7530: set interrupt register only for MT7530

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:45PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Setting this register related to interrupts is only needed for the MT7530
> switch. Make an exclusive check to ensure this.
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Acked-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Daniel Golle <daniel@makrotopia.org>
(on MT7988 which is the relevant hardware regarding this change,
interrupts still work fine)


> ---
>  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 a66a762cb5db..ac1e3c58aaac 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2034,7 +2034,7 @@ mt7530_setup_irq(struct mt7530_priv *priv)
>  	}
>  
>  	/* This register must be set for MT7530 to properly fire interrupts */
> -	if (priv->id != ID_MT7531)
> +	if (priv->id == ID_MT7530 || priv->id == ID_MT7621)
>  		mt7530_set(priv, MT7530_TOP_SIG_CTRL, TOP_SIG_CTRL_NORMAL);
>  
>  	ret = request_threaded_irq(priv->irq, NULL, mt7530_irq_thread_fn,
> -- 
> 2.37.2
> 

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Force link-down on all MACs before internal reset. Let's follow suit commit
> 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
> reset").
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index ac1e3c58aaac..8ece3d0d820c 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
>  		return -EINVAL;
>  	}
>  
> +	/* Force link-down on all MACs before internal reset */
> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> +

Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
is probably better. Though it isn't documented I assume that the requirement
to have the ports in force-link-down may also apply to MT7988, and for sure
it doesn't do any harm.

Hence I suggest to squash this change:
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index a2cb7e296165e..998c4e8930cd3 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -2203,10 +2203,6 @@ mt7530_setup(struct dsa_switch *ds)
 		return -EINVAL;
 	}
 
-	/* Force link-down on all MACs before internal reset */
-	for (i = 0; i < MT7530_NUM_PORTS; i++)
-		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
-
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
@@ -2423,10 +2419,6 @@ mt7531_setup(struct dsa_switch *ds)
 		dev_info(priv->dev, "found MT7531BE\n");
 	}
 
-	/* all MACs must be forced link-down before sw reset */
-	for (i = 0; i < MT7530_NUM_PORTS; i++)
-		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
-
 	/* Reset the switch through internal reset */
 	mt7530_write(priv, MT7530_SYS_CTRL,
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
@@ -2907,6 +2899,10 @@ mt753x_setup(struct dsa_switch *ds)
 		priv->pcs[i].port = i;
 	}
 
+	/* Force link-down on all MACs before setup */
+	for (i = 0; i < MT7530_NUM_PORTS; i++)
+		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
+
 	ret = priv->info->sw_setup(ds);
 	if (ret)
 		return ret;

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

[Arınç ÜNAL]

On 21.04.2023 21:42, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Force link-down on all MACs before internal reset. Let's follow suit commit
>> 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
>> reset").
>>
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ac1e3c58aaac..8ece3d0d820c 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Force link-down on all MACs before internal reset */
>> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
>> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
>> +
> 
> Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
> is probably better. Though it isn't documented I assume that the requirement
> to have the ports in force-link-down may also apply to MT7988, and for sure
> it doesn't do any harm.
> 
> Hence I suggest to squash this change:
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index a2cb7e296165e..998c4e8930cd3 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2203,10 +2203,6 @@ mt7530_setup(struct dsa_switch *ds)
>   		return -EINVAL;
>   	}
>   
> -	/* Force link-down on all MACs before internal reset */
> -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> -		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> -
>   	/* Reset the switch through internal reset */
>   	mt7530_write(priv, MT7530_SYS_CTRL,
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> @@ -2423,10 +2419,6 @@ mt7531_setup(struct dsa_switch *ds)
>   		dev_info(priv->dev, "found MT7531BE\n");
>   	}
>   
> -	/* all MACs must be forced link-down before sw reset */
> -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> -		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
> -
>   	/* Reset the switch through internal reset */
>   	mt7530_write(priv, MT7530_SYS_CTRL,
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> @@ -2907,6 +2899,10 @@ mt753x_setup(struct dsa_switch *ds)
>   		priv->pcs[i].port = i;
>   	}
>   
> +	/* Force link-down on all MACs before setup */
> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);

MT7531 has got a different bit on the register for this, 
MT7531_FORCE_LNK. Are you sure PMCR_FORCE_LNK would work for MT7531 too?

Arınç

Re: [RFC PATCH net-next 21/22] net: dsa: mt7530: get rid of useless error returns on phylink code path

[Daniel Golle]

On Fri, Apr 21, 2023 at 05:36:47PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Remove error returns on the cases where they are already handled with the
> function the mac_port_get_caps member points to.
> 
> mt7531_mac_config() is also called from mt7531_cpu_port_config() outside of
> phylink but the port and interface modes are already handled there.
> 
> Change the functions and the mac_port_config function pointer to void now
> that there're no error returns anymore.
> 
> Remove mt753x_is_mac_port() that used to help the said error returns.
> 
> On mt7531_mac_config(), switch to if statements to simplify the code.
> 
> Remove internal phy cases from mt753x_phylink_mac_config() as there is no
> configuration to be done for them. There's also no need to check the
> interface mode as that's already handled with the function the
> mac_port_get_caps member points to.
> 
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>

Acked-by: Daniel Golle <daniel@makrotopia.org>
Tested-by: Daniel Golle <daniel@makrotopia.org>
(on BPi-R3 MT7986A+MT7531AE, BPi-R64 MT7622+MT7531BE and MT7988A rfb)

> ---
>  drivers/net/dsa/mt7530.c | 81 ++++++++--------------------------------
>  drivers/net/dsa/mt7530.h |  2 +-
>  2 files changed, 17 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 8ece3d0d820c..3d19e06061cb 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2556,7 +2556,7 @@ static void mt7988_mac_port_get_caps(struct dsa_switch *ds, int port,
>  	}
>  }
>  
> -static int
> +static void
>  mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		  phy_interface_t interface)
>  {
> @@ -2567,22 +2567,14 @@ mt7530_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  	} else if (port == 6) {
>  		mt7530_setup_port6(priv->ds, interface);
>  	}
> -
> -	return 0;
>  }
>  
> -static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
> -			      phy_interface_t interface,
> -			      struct phy_device *phydev)
> +static void mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
> +			       phy_interface_t interface,
> +			       struct phy_device *phydev)
>  {
>  	u32 val;
>  
> -	if (priv->p5_sgmii) {
> -		dev_err(priv->dev, "RGMII mode is not available for port %d\n",
> -			port);
> -		return -EINVAL;
> -	}
> -
>  	val = mt7530_read(priv, MT7531_CLKGEN_CTRL);
>  	val |= GP_CLK_EN;
>  	val &= ~GP_MODE_MASK;
> @@ -2610,20 +2602,14 @@ static int mt7531_rgmii_setup(struct mt7530_priv *priv, u32 port,
>  		case PHY_INTERFACE_MODE_RGMII_ID:
>  			break;
>  		default:
> -			return -EINVAL;
> +			break;
>  		}
>  	}
> -	mt7530_write(priv, MT7531_CLKGEN_CTRL, val);
>  
> -	return 0;
> -}
> -
> -static bool mt753x_is_mac_port(u32 port)
> -{
> -	return (port == 5 || port == 6);
> +	mt7530_write(priv, MT7531_CLKGEN_CTRL, val);
>  }
>  
> -static int
> +static void
>  mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		  phy_interface_t interface)
>  {
> @@ -2631,42 +2617,21 @@ mt7531_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  	struct phy_device *phydev;
>  	struct dsa_port *dp;
>  
> -	if (!mt753x_is_mac_port(port)) {
> -		dev_err(priv->dev, "port %d is not a MAC port\n", port);
> -		return -EINVAL;
> -	}
> -
> -	switch (interface) {
> -	case PHY_INTERFACE_MODE_RGMII:
> -	case PHY_INTERFACE_MODE_RGMII_ID:
> -	case PHY_INTERFACE_MODE_RGMII_RXID:
> -	case PHY_INTERFACE_MODE_RGMII_TXID:
> +	if (phy_interface_mode_is_rgmii(interface)) {
>  		dp = dsa_to_port(ds, port);
>  		phydev = dp->slave->phydev;
> -		return mt7531_rgmii_setup(priv, port, interface, phydev);
> -	case PHY_INTERFACE_MODE_SGMII:
> -	case PHY_INTERFACE_MODE_NA:
> -	case PHY_INTERFACE_MODE_1000BASEX:
> -	case PHY_INTERFACE_MODE_2500BASEX:
> -		/* handled in SGMII PCS driver */
> -		return 0;
> -	default:
> -		return -EINVAL;
> +		mt7531_rgmii_setup(priv, port, interface, phydev);
>  	}
> -
> -	return -EINVAL;
>  }
>  
> -static int
> +static void
>  mt753x_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		  const struct phylink_link_state *state)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  
> -	if (!priv->info->mac_port_config)
> -		return 0;
> -
> -	return priv->info->mac_port_config(ds, port, mode, state->interface);
> +	if (priv->info->mac_port_config)
> +		priv->info->mac_port_config(ds, port, mode, state->interface);
>  }
>  
>  static struct phylink_pcs *
> @@ -2695,30 +2660,18 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  	u32 mcr_cur, mcr_new;
>  
>  	switch (port) {
> -	case 0 ... 4: /* Internal phy */
> -		if (state->interface != PHY_INTERFACE_MODE_GMII &&
> -		    state->interface != PHY_INTERFACE_MODE_INTERNAL)
> -			goto unsupported;
> -		break;
>  	case 5: /* Port 5, can be used as a CPU port. */
>  		if (priv->p5_configured)
>  			break;
>  
> -		if (mt753x_mac_config(ds, port, mode, state) < 0)
> -			goto unsupported;
> +		mt753x_mac_config(ds, port, mode, state);
>  		break;
>  	case 6: /* Port 6, can be used as a CPU port. */
>  		if (priv->p6_configured)
>  			break;
>  
> -		if (mt753x_mac_config(ds, port, mode, state) < 0)
> -			goto unsupported;
> +		mt753x_mac_config(ds, port, mode, state);
>  		break;
> -	default:
> -unsupported:
> -		dev_err(ds->dev, "%s: unsupported %s port: %i\n",
> -			__func__, phy_modes(state->interface), port);
> -		return;
>  	}
>  
>  	mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));
> @@ -2811,7 +2764,6 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
>  	struct mt7530_priv *priv = ds->priv;
>  	phy_interface_t interface;
>  	int speed;
> -	int ret;
>  
>  	switch (port) {
>  	case 5:
> @@ -2836,9 +2788,8 @@ mt7531_cpu_port_config(struct dsa_switch *ds, int port)
>  	else
>  		speed = SPEED_1000;
>  
> -	ret = mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
> -	if (ret)
> -		return ret;
> +	mt7531_mac_config(ds, port, MLO_AN_FIXED, interface);
> +
>  	mt7530_write(priv, MT7530_PMCR_P(port),
>  		     PMCR_CPU_PORT_SETTING(priv->id));
>  	mt753x_phylink_pcs_link_up(&priv->pcs[port].pcs, MLO_AN_FIXED,
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index cad9115de22b..ee2b3d2d6258 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -722,7 +722,7 @@ struct mt753x_info {
>  	void (*mac_port_validate)(struct dsa_switch *ds, int port,
>  				  phy_interface_t interface,
>  				  unsigned long *supported);
> -	int (*mac_port_config)(struct dsa_switch *ds, int port,
> +	void (*mac_port_config)(struct dsa_switch *ds, int port,
>  			       unsigned int mode,
>  			       phy_interface_t interface);
>  };
> -- 
> 2.37.2
> 

Re: [RFC PATCH net-next 08/22] net: dsa: mt7530: change p{5,6}_interface to p{5,6}_configured

[Daniel Golle]

On Fri, Apr 21, 2023 at 09:25:39PM +0300, Arınç ÜNAL wrote:
> 
> 
> On 21.04.2023 21:20, Arınç ÜNAL wrote:
> > On 21.04.2023 21:17, Arınç ÜNAL wrote:
> > > On 21.04.2023 20:28, Daniel Golle wrote:
> > > > On Fri, Apr 21, 2023 at 05:36:34PM +0300, arinc9.unal@gmail.com wrote:
> > > > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > 
> > > > > The idea of p5_interface and p6_interface pointers is to prevent
> > > > > mt753x_mac_config() from running twice for MT7531, as it's
> > > > > already run with
> > > > > mt753x_cpu_port_enable() from mt7531_setup_common(), if the
> > > > > port is used as
> > > > > a CPU port.
> > > > > 
> > > > > Change p5_interface and p6_interface to p5_configured and
> > > > > p6_configured.
> > > > > Make them boolean.
> > > > > 
> > > > > Do not set them for any other reason.
> > > > > 
> > > > > The priv->p5_intf_sel check is useless as in this code path,
> > > > > it will always
> > > > > be P5_INTF_SEL_GMAC5.
> > > > > 
> > > > > There was also no need to set priv->p5_interface and
> > > > > priv->p6_interface to
> > > > > PHY_INTERFACE_MODE_NA on mt7530_setup() and mt7531_setup()
> > > > > as they would
> > > > > already be set to that when "priv" is allocated. The
> > > > > pointers were of the
> > > > > phy_interface_t enumeration type, and the first element of the enum is
> > > > > PHY_INTERFACE_MODE_NA. There was nothing in between that
> > > > > would change this
> > > > > beforehand.
> > > > > 
> > > > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > > 
> > > > NACK. This assumes that a user port is configured exactly once.
> > > > However, interface mode may change because of mode-changing PHYs (e.g.
> > > > often using Cisco SGMII for 10M/100M/1000M but using 2500Base-X for
> > > > 2500M, ie. depending on actual link speed).
> > > > 
> > > > Also when using SFP modules (which can be hotplugged) the interface
> > > > mode may change after initially setting up the driver, e.g. when SFP
> > > > driver is loaded or a module is plugged or replaced.
> > > 
> > > I'm not sure I understand. pX_configured would be set to true only
> > > when the port is used as a CPU port. mt753x_mac_config() should run
> > > for user or DSA ports more than once, if needed.
> > 
> > Looking at this again, once pX_interface is true, the check will prevent
> > even user or DSA ports to be configured again. What about setting
> > pX_interface to false after mt753x_mac_config() is run?
> 
> On a third thought, pX_interface will never be true for the port if it's a
> user or DSA port so this should not be a problem at all.

I also followed the individual codepaths and conclude that you are
right. I've also tested this by now on several boards with MT753x,
incl. BPi-R3 and SerDes interface mode switching is anyway handled in
the PCS driver (I should have rembered that...)

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

> 
> Arınç

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

[Daniel Golle]

On Fri, Apr 21, 2023 at 09:47:16PM +0300, Arınç ÜNAL wrote:
> On 21.04.2023 21:42, Daniel Golle wrote:
> > On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > Force link-down on all MACs before internal reset. Let's follow suit commit
> > > 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
> > > reset").
> > > 
> > > Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > >   drivers/net/dsa/mt7530.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index ac1e3c58aaac..8ece3d0d820c 100644
> > > --- a/drivers/net/dsa/mt7530.c
> > > +++ b/drivers/net/dsa/mt7530.c
> > > @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
> > >   		return -EINVAL;
> > >   	}
> > > +	/* Force link-down on all MACs before internal reset */
> > > +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > > +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> > > +
> > 
> > Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
> > is probably better. Though it isn't documented I assume that the requirement
> > to have the ports in force-link-down may also apply to MT7988, and for sure
> > it doesn't do any harm.
> > 
> > Hence I suggest to squash this change:
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index a2cb7e296165e..998c4e8930cd3 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -2203,10 +2203,6 @@ mt7530_setup(struct dsa_switch *ds)
> >   		return -EINVAL;
> >   	}
> > -	/* Force link-down on all MACs before internal reset */
> > -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > -		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> > -
> >   	/* Reset the switch through internal reset */
> >   	mt7530_write(priv, MT7530_SYS_CTRL,
> >   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> > @@ -2423,10 +2419,6 @@ mt7531_setup(struct dsa_switch *ds)
> >   		dev_info(priv->dev, "found MT7531BE\n");
> >   	}
> > -	/* all MACs must be forced link-down before sw reset */
> > -	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > -		mt7530_write(priv, MT7530_PMCR_P(i), MT7531_FORCE_LNK);
> > -
> >   	/* Reset the switch through internal reset */
> >   	mt7530_write(priv, MT7530_SYS_CTRL,
> >   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
> > @@ -2907,6 +2899,10 @@ mt753x_setup(struct dsa_switch *ds)
> >   		priv->pcs[i].port = i;
> >   	}
> > +	/* Force link-down on all MACs before setup */
> > +	for (i = 0; i < MT7530_NUM_PORTS; i++)
> > +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
> 
> MT7531 has got a different bit on the register for this, MT7531_FORCE_LNK.
> Are you sure PMCR_FORCE_LNK would work for MT7531 too?

No, I had overlooked that. As the effects of not doing the
force-link-down before the reset are subtle and depend on the
link-partners I may not have cought them in my tests.


> 
> Arınç

Re: [RFC PATCH net-next 20/22] net: dsa: mt7530: force link-down on MACs before reset on MT7530

[Arınç ÜNAL]

On 21.04.2023 21:42, Daniel Golle wrote:
> On Fri, Apr 21, 2023 at 05:36:46PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Force link-down on all MACs before internal reset. Let's follow suit commit
>> 728c2af6ad8c ("net: mt7531: ensure all MACs are powered down before
>> reset").
>>
>> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index ac1e3c58aaac..8ece3d0d820c 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -2203,6 +2203,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Force link-down on all MACs before internal reset */
>> +	for (i = 0; i < MT7530_NUM_PORTS; i++)
>> +		mt7530_write(priv, MT7530_PMCR_P(i), PMCR_FORCE_LNK);
>> +
> 
> Moving this part to mt753x_setup just before calling priv->info->sw_setup(ds);
> is probably better. Though it isn't documented I assume that the requirement
> to have the ports in force-link-down may also apply to MT7988, and for sure
> it doesn't do any harm.

Now that I'm reading through the programming guide for MT7531 [0] and 
MT7530 [1], I see that the SW_PHY_RST bit on the SYS_CTRL register 
doesn't exist for MT7531. This is likely why forcing link-down on the 
MACs is necessary for MT7531.

I didn't come across any documentation for the switch on the MT7988 SoC. 
Should I assume the registers are identical with MT7531 or have you got 
a document I can look at?

You also don't do system or register reset for the switch on the MT7988 
SoC, what's up with that?

I'm not going to do this change for MT7530 as I think SW_PHY_RST is 
sufficient, and there's no mention to this like on MT7531.

[0] 
https://drive.google.com/file/d/1aVdQz3rbKWjkvdga8-LQ-VFXjmHR8yf9/view?usp=sharing
[1] 
https://github.com/vschagen/documents/blob/main/MT7621_ProgrammingGuide_GSW_v0_3.pdf

Arınç