[RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[arinc9.unal]

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

Move the PLL setup of the MT7530 switch out of the pad configuration of
port 6 to mt7530_setup, after reset.

This fixes the improper initialisation of the switch when only port 5 is
used as a CPU port.

Add supported phy modes of port 5 on the PLL setup.

Remove now incorrect comment regarding P5 as GMAC5.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---

I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
This is already the case for MT7530 and MT7531 on the MediaTek ethernet
driver on U-Boot [1] [2].

[0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
[1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
[2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903

There are some parts I couldn't figure out myself with my limited C
knowledge. I've pointed them out on the code. Vladimir, could you help?

There is a lot of code which is only needed for port 6 or trgmii on port 6,
but runs whether port 6 or trgmii is used or not. For now, the best I can
do is to fix port 5 so it works without port 6 being used.

The U-Boot driver seems to be much more organised so it could be taken as
a reference to sort out this DSA driver further.

Also, now that the pad setup for mt7530 is also moved somewhere else, can
we completely get rid of @pad_setup?

Arınç

---
 drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0e99de26d159..fb20ce4f443e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 
 /* Setup TX circuit including relevant PAD and driving */
 static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
 {
-	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, i, xtal;
 
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
@@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 		return -EINVAL;
 	}
 
+	/* Is setting trgint to 0 really needed for the !trgint check? */
+	trgint = 0;
+
+	/* FIXME: run this switch if p5 is defined on the devicetree */
+	/* and change interface to the phy-mode of port 5 */
+	switch (interface) {
+	case PHY_INTERFACE_MODE_GMII:
+		/* PLL frequency: 125MHz */
+		ncpo1 = 0x0c80;
+		break;
+	case PHY_INTERFACE_MODE_MII:
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+		/* PLL frequency: 125MHz */
+		ncpo1 = 0x0c80;
+		break;
+	default:
+		dev_err(priv->dev, "xMII interface %d not supported\n",
+			interface);
+		return -EINVAL;
+	}
+
+	/* FIXME: run this switch if p6 is defined on the devicetree */
+	/* and change interface to the phy-mode of port 6 */
 	switch (interface) {
 	case PHY_INTERFACE_MODE_RGMII:
-		trgint = 0;
 		/* PLL frequency: 125MHz */
 		ncpo1 = 0x0c80;
 		break;
@@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
 		     SYS_CTRL_REG_RST);
 
-	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
+	/* Setup switch core pll */
+	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
+	mt7530_pad_clk_setup(priv, interface);
+
+	/* Enable Port 6 */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
 	val |= MHWTRAP_MANUAL;
@@ -2491,11 +2517,9 @@ static void mt7531_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)
+mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
-	struct mt7530_priv *priv = ds->priv;
-
-	return priv->info->pad_setup(ds, state->interface);
+	return 0;
 }
 
 static int
@@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p6_interface == state->interface)
 			break;
 
-		mt753x_pad_setup(ds, state);
-
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
 
@@ -3187,7 +3209,7 @@ static 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,
+		.pad_setup = mt7530_pad_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
@@ -3199,7 +3221,7 @@ static 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,
+		.pad_setup = mt7530_pad_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
-- 
2.37.2

[RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[arinc9.unal]

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

Move the PLL setup of the MT7530 switch out of the pad configuration of
port 6 to mt7530_setup, after reset.

This fixes the improper initialisation of the switch when only port 5 is
used as a CPU port.

Add supported phy modes of port 5 on the PLL setup.

Remove now incorrect comment regarding P5 as GMAC5.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---

I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
This is already the case for MT7530 and MT7531 on the MediaTek ethernet
driver on U-Boot [1] [2].

[0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
[1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
[2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903

There are some parts I couldn't figure out myself with my limited C
knowledge. I've pointed them out on the code. Vladimir, could you help?

There is a lot of code which is only needed for port 6 or trgmii on port 6,
but runs whether port 6 or trgmii is used or not. For now, the best I can
do is to fix port 5 so it works without port 6 being used.

The U-Boot driver seems to be much more organised so it could be taken as
a reference to sort out this DSA driver further.

Also, now that the pad setup for mt7530 is also moved somewhere else, can
we completely get rid of @pad_setup?

Arınç

---
 drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0e99de26d159..fb20ce4f443e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 
 /* Setup TX circuit including relevant PAD and driving */
 static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
 {
-	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, i, xtal;
 
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
@@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 		return -EINVAL;
 	}
 
+	/* Is setting trgint to 0 really needed for the !trgint check? */
+	trgint = 0;
+
+	/* FIXME: run this switch if p5 is defined on the devicetree */
+	/* and change interface to the phy-mode of port 5 */
+	switch (interface) {
+	case PHY_INTERFACE_MODE_GMII:
+		/* PLL frequency: 125MHz */
+		ncpo1 = 0x0c80;
+		break;
+	case PHY_INTERFACE_MODE_MII:
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+		/* PLL frequency: 125MHz */
+		ncpo1 = 0x0c80;
+		break;
+	default:
+		dev_err(priv->dev, "xMII interface %d not supported\n",
+			interface);
+		return -EINVAL;
+	}
+
+	/* FIXME: run this switch if p6 is defined on the devicetree */
+	/* and change interface to the phy-mode of port 6 */
 	switch (interface) {
 	case PHY_INTERFACE_MODE_RGMII:
-		trgint = 0;
 		/* PLL frequency: 125MHz */
 		ncpo1 = 0x0c80;
 		break;
@@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
 		     SYS_CTRL_REG_RST);
 
-	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
+	/* Setup switch core pll */
+	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
+	mt7530_pad_clk_setup(priv, interface);
+
+	/* Enable Port 6 */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
 	val |= MHWTRAP_MANUAL;
@@ -2491,11 +2517,9 @@ static void mt7531_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)
+mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
-	struct mt7530_priv *priv = ds->priv;
-
-	return priv->info->pad_setup(ds, state->interface);
+	return 0;
 }
 
 static int
@@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p6_interface == state->interface)
 			break;
 
-		mt753x_pad_setup(ds, state);
-
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
 
@@ -3187,7 +3209,7 @@ static 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,
+		.pad_setup = mt7530_pad_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
@@ -3199,7 +3221,7 @@ static 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,
+		.pad_setup = mt7530_pad_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
-- 
2.37.2


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

[RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[arinc9.unal]

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

Move the PLL setup of the MT7530 switch out of the pad configuration of
port 6 to mt7530_setup, after reset.

This fixes the improper initialisation of the switch when only port 5 is
used as a CPU port.

Add supported phy modes of port 5 on the PLL setup.

Remove now incorrect comment regarding P5 as GMAC5.

Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---

I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
This is already the case for MT7530 and MT7531 on the MediaTek ethernet
driver on U-Boot [1] [2].

[0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
[1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
[2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903

There are some parts I couldn't figure out myself with my limited C
knowledge. I've pointed them out on the code. Vladimir, could you help?

There is a lot of code which is only needed for port 6 or trgmii on port 6,
but runs whether port 6 or trgmii is used or not. For now, the best I can
do is to fix port 5 so it works without port 6 being used.

The U-Boot driver seems to be much more organised so it could be taken as
a reference to sort out this DSA driver further.

Also, now that the pad setup for mt7530 is also moved somewhere else, can
we completely get rid of @pad_setup?

Arınç

---
 drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 0e99de26d159..fb20ce4f443e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 
 /* Setup TX circuit including relevant PAD and driving */
 static int
-mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
+mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
 {
-	struct mt7530_priv *priv = ds->priv;
 	u32 ncpo1, ssc_delta, trgint, i, xtal;
 
 	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
@@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 		return -EINVAL;
 	}
 
+	/* Is setting trgint to 0 really needed for the !trgint check? */
+	trgint = 0;
+
+	/* FIXME: run this switch if p5 is defined on the devicetree */
+	/* and change interface to the phy-mode of port 5 */
+	switch (interface) {
+	case PHY_INTERFACE_MODE_GMII:
+		/* PLL frequency: 125MHz */
+		ncpo1 = 0x0c80;
+		break;
+	case PHY_INTERFACE_MODE_MII:
+		break;
+	case PHY_INTERFACE_MODE_RGMII:
+		/* PLL frequency: 125MHz */
+		ncpo1 = 0x0c80;
+		break;
+	default:
+		dev_err(priv->dev, "xMII interface %d not supported\n",
+			interface);
+		return -EINVAL;
+	}
+
+	/* FIXME: run this switch if p6 is defined on the devicetree */
+	/* and change interface to the phy-mode of port 6 */
 	switch (interface) {
 	case PHY_INTERFACE_MODE_RGMII:
-		trgint = 0;
 		/* PLL frequency: 125MHz */
 		ncpo1 = 0x0c80;
 		break;
@@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
 		     SYS_CTRL_REG_RST);
 
-	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
+	/* Setup switch core pll */
+	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
+	mt7530_pad_clk_setup(priv, interface);
+
+	/* Enable Port 6 */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
 	val |= MHWTRAP_MANUAL;
@@ -2491,11 +2517,9 @@ static void mt7531_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)
+mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
 {
-	struct mt7530_priv *priv = ds->priv;
-
-	return priv->info->pad_setup(ds, state->interface);
+	return 0;
 }
 
 static int
@@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
 		if (priv->p6_interface == state->interface)
 			break;
 
-		mt753x_pad_setup(ds, state);
-
 		if (mt753x_mac_config(ds, port, mode, state) < 0)
 			goto unsupported;
 
@@ -3187,7 +3209,7 @@ static 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,
+		.pad_setup = mt7530_pad_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
@@ -3199,7 +3221,7 @@ static 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,
+		.pad_setup = mt7530_pad_setup,
 		.mac_port_get_caps = mt7530_mac_port_get_caps,
 		.mac_port_config = mt7530_mac_config,
 	},
-- 
2.37.2

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Russell King (Oracle)]

On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
> 
> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.

If this is what is necessary, you're taking some of the configuration
out of phylink's control, effectively making port 6 a fixed-interface
mode port. In that case, there should only ever be one interface
mode set in port 6's supported_interface mask, so that phylink knows
that no other interface modes can be selected for that port.

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Russell King (Oracle)]

On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
> 
> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.

If this is what is necessary, you're taking some of the configuration
out of phylink's control, effectively making port 6 a fixed-interface
mode port. In that case, there should only ever be one interface
mode set in port 6's supported_interface mask, so that phylink knows
that no other interface modes can be selected for that port.

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

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Russell King (Oracle)]

On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
> 
> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.

If this is what is necessary, you're taking some of the configuration
out of phylink's control, effectively making port 6 a fixed-interface
mode port. In that case, there should only ever be one interface
mode set in port 6's supported_interface mask, so that phylink knows
that no other interface modes can be selected for that port.

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[kernel test robot]

Hi,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on net/master]

url:    https://github.com/intel-lab-lkp/linux/commits/arinc9-unal-gmail-com/net-dsa-mt7530-move-PLL-setup-out-of-port-6-pad-configuration/20230304-205859
patch link:    https://lore.kernel.org/r/20230304125453.53476-1-arinc.unal%40arinc9.com
patch subject: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration
config: hexagon-randconfig-r041-20230302 (https://download.01.org/0day-ci/archive/20230305/202303050127.M8FXXFiW-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/e1d5d3c9e6556aea7296af456923103eaeeb7a22
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review arinc9-unal-gmail-com/net-dsa-mt7530-move-PLL-setup-out-of-port-6-pad-configuration/20230304-205859
        git checkout e1d5d3c9e6556aea7296af456923103eaeeb7a22
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/net/dsa/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303050127.M8FXXFiW-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/dsa/mt7530.c:6:
   In file included from include/linux/etherdevice.h:20:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/net/dsa/mt7530.c:6:
   In file included from include/linux/etherdevice.h:20:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/net/dsa/mt7530.c:6:
   In file included from include/linux/etherdevice.h:20:
   In file included from include/linux/if_ether.h:19:
   In file included from include/linux/skbuff.h:17:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:12:
   In file included from include/linux/hardirq.h:11:
   In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/hexagon/include/asm/io.h:334:
   include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
>> drivers/net/dsa/mt7530.c:2223:29: warning: variable 'interface' is uninitialized when used here [-Wuninitialized]
           mt7530_pad_clk_setup(priv, interface);
                                      ^~~~~~~~~
   drivers/net/dsa/mt7530.c:2144:2: note: variable 'interface' is declared here
           phy_interface_t interface;
           ^
   7 warnings generated.


vim +/interface +2223 drivers/net/dsa/mt7530.c

  2135	
  2136	static int
  2137	mt7530_setup(struct dsa_switch *ds)
  2138	{
  2139		struct mt7530_priv *priv = ds->priv;
  2140		struct device_node *dn = NULL;
  2141		struct device_node *phy_node;
  2142		struct device_node *mac_np;
  2143		struct mt7530_dummy_poll p;
  2144		phy_interface_t interface;
  2145		struct dsa_port *cpu_dp;
  2146		u32 id, val;
  2147		int ret, i;
  2148	
  2149		/* The parent node of master netdev which holds the common system
  2150		 * controller also is the container for two GMACs nodes representing
  2151		 * as two netdev instances.
  2152		 */
  2153		dsa_switch_for_each_cpu_port(cpu_dp, ds) {
  2154			dn = cpu_dp->master->dev.of_node->parent;
  2155			/* It doesn't matter which CPU port is found first,
  2156			 * their masters should share the same parent OF node
  2157			 */
  2158			break;
  2159		}
  2160	
  2161		if (!dn) {
  2162			dev_err(ds->dev, "parent OF node of DSA master not found");
  2163			return -EINVAL;
  2164		}
  2165	
  2166		ds->assisted_learning_on_cpu_port = true;
  2167		ds->mtu_enforcement_ingress = true;
  2168	
  2169		if (priv->id == ID_MT7530) {
  2170			regulator_set_voltage(priv->core_pwr, 1000000, 1000000);
  2171			ret = regulator_enable(priv->core_pwr);
  2172			if (ret < 0) {
  2173				dev_err(priv->dev,
  2174					"Failed to enable core power: %d\n", ret);
  2175				return ret;
  2176			}
  2177	
  2178			regulator_set_voltage(priv->io_pwr, 3300000, 3300000);
  2179			ret = regulator_enable(priv->io_pwr);
  2180			if (ret < 0) {
  2181				dev_err(priv->dev, "Failed to enable io pwr: %d\n",
  2182					ret);
  2183				return ret;
  2184			}
  2185		}
  2186	
  2187		/* Reset whole chip through gpio pin or memory-mapped registers for
  2188		 * different type of hardware
  2189		 */
  2190		if (priv->mcm) {
  2191			reset_control_assert(priv->rstc);
  2192			usleep_range(1000, 1100);
  2193			reset_control_deassert(priv->rstc);
  2194		} else {
  2195			gpiod_set_value_cansleep(priv->reset, 0);
  2196			usleep_range(1000, 1100);
  2197			gpiod_set_value_cansleep(priv->reset, 1);
  2198		}
  2199	
  2200		/* Waiting for MT7530 got to stable */
  2201		INIT_MT7530_DUMMY_POLL(&p, priv, MT7530_HWTRAP);
  2202		ret = readx_poll_timeout(_mt7530_read, &p, val, val != 0,
  2203					 20, 1000000);
  2204		if (ret < 0) {
  2205			dev_err(priv->dev, "reset timeout\n");
  2206			return ret;
  2207		}
  2208	
  2209		id = mt7530_read(priv, MT7530_CREV);
  2210		id >>= CHIP_NAME_SHIFT;
  2211		if (id != MT7530_ID) {
  2212			dev_err(priv->dev, "chip %x can't be supported\n", id);
  2213			return -ENODEV;
  2214		}
  2215	
  2216		/* Reset the switch through internal reset */
  2217		mt7530_write(priv, MT7530_SYS_CTRL,
  2218			     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
  2219			     SYS_CTRL_REG_RST);
  2220	
  2221		/* Setup switch core pll */
  2222		/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
> 2223		mt7530_pad_clk_setup(priv, interface);
  2224	
  2225		/* Enable Port 6 */
  2226		val = mt7530_read(priv, MT7530_MHWTRAP);
  2227		val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
  2228		val |= MHWTRAP_MANUAL;
  2229		mt7530_write(priv, MT7530_MHWTRAP, val);
  2230	
  2231		priv->p6_interface = PHY_INTERFACE_MODE_NA;
  2232	
  2233		/* Enable and reset MIB counters */
  2234		mt7530_mib_reset(ds);
  2235	
  2236		for (i = 0; i < MT7530_NUM_PORTS; i++) {
  2237			/* Disable forwarding by default on all ports */
  2238			mt7530_rmw(priv, MT7530_PCR_P(i), PCR_MATRIX_MASK,
  2239				   PCR_MATRIX_CLR);
  2240	
  2241			/* Disable learning by default on all ports */
  2242			mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
  2243	
  2244			if (dsa_is_cpu_port(ds, i)) {
  2245				ret = mt753x_cpu_port_enable(ds, i);
  2246				if (ret)
  2247					return ret;
  2248			} else {
  2249				mt7530_port_disable(ds, i);
  2250	
  2251				/* Set default PVID to 0 on all user ports */
  2252				mt7530_rmw(priv, MT7530_PPBV1_P(i), G0_PORT_VID_MASK,
  2253					   G0_PORT_VID_DEF);
  2254			}
  2255			/* Enable consistent egress tag */
  2256			mt7530_rmw(priv, MT7530_PVC_P(i), PVC_EG_TAG_MASK,
  2257				   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
  2258		}
  2259	
  2260		/* Setup VLAN ID 0 for VLAN-unaware bridges */
  2261		ret = mt7530_setup_vlan0(priv);
  2262		if (ret)
  2263			return ret;
  2264	
  2265		/* Setup port 5 */
  2266		priv->p5_intf_sel = P5_DISABLED;
  2267		interface = PHY_INTERFACE_MODE_NA;
  2268	
  2269		if (!dsa_is_unused_port(ds, 5)) {
  2270			priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
  2271			ret = of_get_phy_mode(dsa_to_port(ds, 5)->dn, &interface);
  2272			if (ret && ret != -ENODEV)
  2273				return ret;
  2274		} else {
  2275			/* Scan the ethernet nodes. look for GMAC1, lookup used phy */
  2276			for_each_child_of_node(dn, mac_np) {
  2277				if (!of_device_is_compatible(mac_np,
  2278							     "mediatek,eth-mac"))
  2279					continue;
  2280	
  2281				ret = of_property_read_u32(mac_np, "reg", &id);
  2282				if (ret < 0 || id != 1)
  2283					continue;
  2284	
  2285				phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
  2286				if (!phy_node)
  2287					continue;
  2288	
  2289				if (phy_node->parent == priv->dev->of_node->parent) {
  2290					ret = of_get_phy_mode(mac_np, &interface);
  2291					if (ret && ret != -ENODEV) {
  2292						of_node_put(mac_np);
  2293						of_node_put(phy_node);
  2294						return ret;
  2295					}
  2296					id = of_mdio_parse_addr(ds->dev, phy_node);
  2297					if (id == 0)
  2298						priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
  2299					if (id == 4)
  2300						priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
  2301				}
  2302				of_node_put(mac_np);
  2303				of_node_put(phy_node);
  2304				break;
  2305			}
  2306		}
  2307	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 4.03.2023 16:04, Russell King (Oracle) wrote:
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Move the PLL setup of the MT7530 switch out of the pad configuration of
>> port 6 to mt7530_setup, after reset.
>>
>> This fixes the improper initialisation of the switch when only port 5 is
>> used as a CPU port.
>>
>> Add supported phy modes of port 5 on the PLL setup.
>>
>> Remove now incorrect comment regarding P5 as GMAC5.
> 
> If this is what is necessary, you're taking some of the configuration
> out of phylink's control, effectively making port 6 a fixed-interface
> mode port. In that case, there should only ever be one interface
> mode set in port 6's supported_interface mask, so that phylink knows
> that no other interface modes can be selected for that port.

Thanks for the heads up Russell.

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 4.03.2023 16:04, Russell King (Oracle) wrote:
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Move the PLL setup of the MT7530 switch out of the pad configuration of
>> port 6 to mt7530_setup, after reset.
>>
>> This fixes the improper initialisation of the switch when only port 5 is
>> used as a CPU port.
>>
>> Add supported phy modes of port 5 on the PLL setup.
>>
>> Remove now incorrect comment regarding P5 as GMAC5.
> 
> If this is what is necessary, you're taking some of the configuration
> out of phylink's control, effectively making port 6 a fixed-interface
> mode port. In that case, there should only ever be one interface
> mode set in port 6's supported_interface mask, so that phylink knows
> that no other interface modes can be selected for that port.

Thanks for the heads up Russell.

Arınç

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 4.03.2023 16:04, Russell King (Oracle) wrote:
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Move the PLL setup of the MT7530 switch out of the pad configuration of
>> port 6 to mt7530_setup, after reset.
>>
>> This fixes the improper initialisation of the switch when only port 5 is
>> used as a CPU port.
>>
>> Add supported phy modes of port 5 on the PLL setup.
>>
>> Remove now incorrect comment regarding P5 as GMAC5.
> 
> If this is what is necessary, you're taking some of the configuration
> out of phylink's control, effectively making port 6 a fixed-interface
> mode port. In that case, there should only ever be one interface
> mode set in port 6's supported_interface mask, so that phylink knows
> that no other interface modes can be selected for that port.

Thanks for the heads up Russell.

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 4.03.2023 15:54, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
> 
> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> 
> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
> driver on U-Boot [1] [2].
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> 
> There are some parts I couldn't figure out myself with my limited C
> knowledge. I've pointed them out on the code. Vladimir, could you help?
> 
> There is a lot of code which is only needed for port 6 or trgmii on port 6,
> but runs whether port 6 or trgmii is used or not. For now, the best I can
> do is to fix port 5 so it works without port 6 being used.
> 
> The U-Boot driver seems to be much more organised so it could be taken as
> a reference to sort out this DSA driver further.
> 
> Also, now that the pad setup for mt7530 is also moved somewhere else, can
> we completely get rid of @pad_setup?
> 
> Arınç
> 
> ---
>   drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>   1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0e99de26d159..fb20ce4f443e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>   
>   /* Setup TX circuit including relevant PAD and driving */
>   static int
> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>   {
> -	struct mt7530_priv *priv = ds->priv;
>   	u32 ncpo1, ssc_delta, trgint, i, xtal;
>   
>   	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>   		return -EINVAL;
>   	}
>   
> +	/* Is setting trgint to 0 really needed for the !trgint check? */
> +	trgint = 0;
> +
> +	/* FIXME: run this switch if p5 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 5 */
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_GMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "xMII interface %d not supported\n",
> +			interface);
> +		return -EINVAL;
> +	}
> +
> +	/* FIXME: run this switch if p6 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 6 */
>   	switch (interface) {
>   	case PHY_INTERFACE_MODE_RGMII:
> -		trgint = 0;
>   		/* PLL frequency: 125MHz */
>   		ncpo1 = 0x0c80;
>   		break;
> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>   		     SYS_CTRL_REG_RST);
>   
> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
> +	/* Setup switch core pll */
> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
> +	mt7530_pad_clk_setup(priv, interface);

Unlike mt7531_pll_setup, there are return codes on mt7530_pad_clk_setup 
so we may need to check for the return code here.

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 4.03.2023 15:54, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
> 
> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> 
> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
> driver on U-Boot [1] [2].
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> 
> There are some parts I couldn't figure out myself with my limited C
> knowledge. I've pointed them out on the code. Vladimir, could you help?
> 
> There is a lot of code which is only needed for port 6 or trgmii on port 6,
> but runs whether port 6 or trgmii is used or not. For now, the best I can
> do is to fix port 5 so it works without port 6 being used.
> 
> The U-Boot driver seems to be much more organised so it could be taken as
> a reference to sort out this DSA driver further.
> 
> Also, now that the pad setup for mt7530 is also moved somewhere else, can
> we completely get rid of @pad_setup?
> 
> Arınç
> 
> ---
>   drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>   1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0e99de26d159..fb20ce4f443e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>   
>   /* Setup TX circuit including relevant PAD and driving */
>   static int
> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>   {
> -	struct mt7530_priv *priv = ds->priv;
>   	u32 ncpo1, ssc_delta, trgint, i, xtal;
>   
>   	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>   		return -EINVAL;
>   	}
>   
> +	/* Is setting trgint to 0 really needed for the !trgint check? */
> +	trgint = 0;
> +
> +	/* FIXME: run this switch if p5 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 5 */
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_GMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "xMII interface %d not supported\n",
> +			interface);
> +		return -EINVAL;
> +	}
> +
> +	/* FIXME: run this switch if p6 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 6 */
>   	switch (interface) {
>   	case PHY_INTERFACE_MODE_RGMII:
> -		trgint = 0;
>   		/* PLL frequency: 125MHz */
>   		ncpo1 = 0x0c80;
>   		break;
> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>   		     SYS_CTRL_REG_RST);
>   
> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
> +	/* Setup switch core pll */
> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
> +	mt7530_pad_clk_setup(priv, interface);

Unlike mt7531_pll_setup, there are return codes on mt7530_pad_clk_setup 
so we may need to check for the return code here.

Arınç

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 4.03.2023 15:54, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.
> 
> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> 
> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
> driver on U-Boot [1] [2].
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> 
> There are some parts I couldn't figure out myself with my limited C
> knowledge. I've pointed them out on the code. Vladimir, could you help?
> 
> There is a lot of code which is only needed for port 6 or trgmii on port 6,
> but runs whether port 6 or trgmii is used or not. For now, the best I can
> do is to fix port 5 so it works without port 6 being used.
> 
> The U-Boot driver seems to be much more organised so it could be taken as
> a reference to sort out this DSA driver further.
> 
> Also, now that the pad setup for mt7530 is also moved somewhere else, can
> we completely get rid of @pad_setup?
> 
> Arınç
> 
> ---
>   drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>   1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0e99de26d159..fb20ce4f443e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>   
>   /* Setup TX circuit including relevant PAD and driving */
>   static int
> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>   {
> -	struct mt7530_priv *priv = ds->priv;
>   	u32 ncpo1, ssc_delta, trgint, i, xtal;
>   
>   	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>   		return -EINVAL;
>   	}
>   
> +	/* Is setting trgint to 0 really needed for the !trgint check? */
> +	trgint = 0;
> +
> +	/* FIXME: run this switch if p5 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 5 */
> +	switch (interface) {
> +	case PHY_INTERFACE_MODE_GMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		break;
> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "xMII interface %d not supported\n",
> +			interface);
> +		return -EINVAL;
> +	}
> +
> +	/* FIXME: run this switch if p6 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 6 */
>   	switch (interface) {
>   	case PHY_INTERFACE_MODE_RGMII:
> -		trgint = 0;
>   		/* PLL frequency: 125MHz */
>   		ncpo1 = 0x0c80;
>   		break;
> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>   		     SYS_CTRL_REG_RST);
>   
> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
> +	/* Setup switch core pll */
> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
> +	mt7530_pad_clk_setup(priv, interface);

Unlike mt7531_pll_setup, there are return codes on mt7530_pad_clk_setup 
so we may need to check for the return code here.

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

Hi Arınç,

On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.

it would have been good if this patch had done that and only that, no?

> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> 
> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
> driver on U-Boot [1] [2].
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> 
> There are some parts I couldn't figure out myself with my limited C
> knowledge. I've pointed them out on the code. Vladimir, could you help?
> 
> There is a lot of code which is only needed for port 6 or trgmii on port 6,
> but runs whether port 6 or trgmii is used or not. For now, the best I can
> do is to fix port 5 so it works without port 6 being used.
> 
> The U-Boot driver seems to be much more organised so it could be taken as
> a reference to sort out this DSA driver further.
> 
> Also, now that the pad setup for mt7530 is also moved somewhere else, can
> we completely get rid of @pad_setup?
> 
> Arınç

I don't like the strategy you used in this patch here, and I don't want
to answer these questions just yet, because I'm not certain that my
answers will be useful in the end.

> 
> ---
>  drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0e99de26d159..fb20ce4f443e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>  
>  /* Setup TX circuit including relevant PAD and driving */
>  static int
> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>  {
> -	struct mt7530_priv *priv = ds->priv;
>  	u32 ncpo1, ssc_delta, trgint, i, xtal;
>  
>  	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>  		return -EINVAL;
>  	}
>  
> +	/* Is setting trgint to 0 really needed for the !trgint check? */
> +	trgint = 0;
> +
> +	/* FIXME: run this switch if p5 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 5 */
> +	switch (interface) {

so this should be something like priv->p5_interface (assuming this is
initialized by the time mt7530_pad_clk_setup() is called, which it isn't).

> +	case PHY_INTERFACE_MODE_GMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		break;

so with priv->p5_interface == "mii", ncpo1 is uninitialized (will be
potentially still initialized by the port 6 "switch" statement below).

> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "xMII interface %d not supported\n",
> +			interface);
> +		return -EINVAL;
> +	}

What method did you use to determine the ncpo1 values here? Are they
coordinated with what p6 wants? I would expect to see a table with

  priv->p5_interface        priv->p6_interface       ncpo1 value
      gmii                     rgmii                     ???
      mii                      rgmii                     ???
      rgmii                    rgmii                     ???
      gmii                     trgmii                    ???
      mii                      trgmii                    ???
      rgmii                    trgmii                    ???

right now, you let the p6_interface logic overwrite the ncpo1 selected
by the p5_interface logic like crazy, and it's not clear to me that this
is what you want.

This problem is way deeper than knowledge of the C language, your pseudo
code simply does not describe what should happen in all combinations.

From our private conversation dated Feb 08, I guess that what you're
doing is also a useless complication and not what you truly want. What
you truly want is for the CORE_GSWPLL_GRP2 register to be programmed
regardless of whether port 6 is used or not.

> +
> +	/* FIXME: run this switch if p6 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 6 */
>  	switch (interface) {

same comment as above, this should approximate priv->p6_interface.

>  	case PHY_INTERFACE_MODE_RGMII:
> -		trgint = 0;
>  		/* PLL frequency: 125MHz */
>  		ncpo1 = 0x0c80;
>  		break;
> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>  		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>  		     SYS_CTRL_REG_RST);
>  
> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
> +	/* Setup switch core pll */
> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
> +	mt7530_pad_clk_setup(priv, interface);

"interface" is completely uninitialized here.

Sorry, I'm not reviewing this any further, because it obviously doesn't work.

> +
> +	/* Enable Port 6 */
>  	val = mt7530_read(priv, MT7530_MHWTRAP);
>  	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>  	val |= MHWTRAP_MANUAL;
> @@ -2491,11 +2517,9 @@ static void mt7531_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)
> +mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>  {
> -	struct mt7530_priv *priv = ds->priv;
> -
> -	return priv->info->pad_setup(ds, state->interface);
> +	return 0;
>  }
>  
>  static int
> @@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		if (priv->p6_interface == state->interface)
>  			break;
>  
> -		mt753x_pad_setup(ds, state);
> -
>  		if (mt753x_mac_config(ds, port, mode, state) < 0)
>  			goto unsupported;
>  
> @@ -3187,7 +3209,7 @@ static 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,
> +		.pad_setup = mt7530_pad_setup,
>  		.mac_port_get_caps = mt7530_mac_port_get_caps,
>  		.mac_port_config = mt7530_mac_config,
>  	},
> @@ -3199,7 +3221,7 @@ static 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,
> +		.pad_setup = mt7530_pad_setup,

as a general rule, for bug fixes don't touch stuff that you don't need to touch

>  		.mac_port_get_caps = mt7530_mac_port_get_caps,
>  		.mac_port_config = mt7530_mac_config,
>  	},
> -- 
> 2.37.2
> 

Could you please let me know if this patch works, instead of what you've
posted above? It does what you *said* you tried to do, aka it performs
the initialization of CORE_GSWPLL_GRP2 independently of port 6 setup.
There is a slight reordering of register writes with MT7530_P6ECR, but
hopefully that doesn't bother anyone.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..a508402c4ecb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -393,6 +393,24 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
 }
 
+/* Set up switch core clock for MT7530 */
+static void mt7530_pll_setup(struct mt7530_priv *priv)
+{
+	/* Disable PLL */
+	core_write(priv, CORE_GSWPLL_GRP1, 0);
+
+	/* Set core clock into 500Mhz */
+	core_write(priv, CORE_GSWPLL_GRP2,
+		   RG_GSWPLL_POSDIV_500M(1) |
+		   RG_GSWPLL_FBKDIV_500M(25));
+
+	/* Enable PLL */
+	core_write(priv, CORE_GSWPLL_GRP1,
+		   RG_GSWPLL_EN_PRE |
+		   RG_GSWPLL_POSDIV_200M(2) |
+		   RG_GSWPLL_FBKDIV_200M(32));
+}
+
 /* Setup TX circuit including relevant PAD and driving */
 static int
 mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -453,21 +471,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 	core_clear(priv, CORE_TRGMII_GSW_CLK_CG,
 		   REG_GSWCK_EN | REG_TRGMIICK_EN);
 
-	/* Setup core clock for MT7530 */
-	/* Disable PLL */
-	core_write(priv, CORE_GSWPLL_GRP1, 0);
-
-	/* Set core clock into 500Mhz */
-	core_write(priv, CORE_GSWPLL_GRP2,
-		   RG_GSWPLL_POSDIV_500M(1) |
-		   RG_GSWPLL_FBKDIV_500M(25));
-
-	/* Enable PLL */
-	core_write(priv, CORE_GSWPLL_GRP1,
-		   RG_GSWPLL_EN_PRE |
-		   RG_GSWPLL_POSDIV_200M(2) |
-		   RG_GSWPLL_FBKDIV_200M(32));
-
 	/* Setup the MT7530 TRGMII Tx Clock */
 	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
 	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
@@ -2196,6 +2199,8 @@ mt7530_setup(struct dsa_switch *ds)
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
 		     SYS_CTRL_REG_RST);
 
+	mt7530_pll_setup(priv);
+
 	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
-- 
2.34.1

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

Hi Arınç,

On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.

it would have been good if this patch had done that and only that, no?

> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> 
> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
> driver on U-Boot [1] [2].
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> 
> There are some parts I couldn't figure out myself with my limited C
> knowledge. I've pointed them out on the code. Vladimir, could you help?
> 
> There is a lot of code which is only needed for port 6 or trgmii on port 6,
> but runs whether port 6 or trgmii is used or not. For now, the best I can
> do is to fix port 5 so it works without port 6 being used.
> 
> The U-Boot driver seems to be much more organised so it could be taken as
> a reference to sort out this DSA driver further.
> 
> Also, now that the pad setup for mt7530 is also moved somewhere else, can
> we completely get rid of @pad_setup?
> 
> Arınç

I don't like the strategy you used in this patch here, and I don't want
to answer these questions just yet, because I'm not certain that my
answers will be useful in the end.

> 
> ---
>  drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0e99de26d159..fb20ce4f443e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>  
>  /* Setup TX circuit including relevant PAD and driving */
>  static int
> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>  {
> -	struct mt7530_priv *priv = ds->priv;
>  	u32 ncpo1, ssc_delta, trgint, i, xtal;
>  
>  	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>  		return -EINVAL;
>  	}
>  
> +	/* Is setting trgint to 0 really needed for the !trgint check? */
> +	trgint = 0;
> +
> +	/* FIXME: run this switch if p5 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 5 */
> +	switch (interface) {

so this should be something like priv->p5_interface (assuming this is
initialized by the time mt7530_pad_clk_setup() is called, which it isn't).

> +	case PHY_INTERFACE_MODE_GMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		break;

so with priv->p5_interface == "mii", ncpo1 is uninitialized (will be
potentially still initialized by the port 6 "switch" statement below).

> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "xMII interface %d not supported\n",
> +			interface);
> +		return -EINVAL;
> +	}

What method did you use to determine the ncpo1 values here? Are they
coordinated with what p6 wants? I would expect to see a table with

  priv->p5_interface        priv->p6_interface       ncpo1 value
      gmii                     rgmii                     ???
      mii                      rgmii                     ???
      rgmii                    rgmii                     ???
      gmii                     trgmii                    ???
      mii                      trgmii                    ???
      rgmii                    trgmii                    ???

right now, you let the p6_interface logic overwrite the ncpo1 selected
by the p5_interface logic like crazy, and it's not clear to me that this
is what you want.

This problem is way deeper than knowledge of the C language, your pseudo
code simply does not describe what should happen in all combinations.

From our private conversation dated Feb 08, I guess that what you're
doing is also a useless complication and not what you truly want. What
you truly want is for the CORE_GSWPLL_GRP2 register to be programmed
regardless of whether port 6 is used or not.

> +
> +	/* FIXME: run this switch if p6 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 6 */
>  	switch (interface) {

same comment as above, this should approximate priv->p6_interface.

>  	case PHY_INTERFACE_MODE_RGMII:
> -		trgint = 0;
>  		/* PLL frequency: 125MHz */
>  		ncpo1 = 0x0c80;
>  		break;
> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>  		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>  		     SYS_CTRL_REG_RST);
>  
> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
> +	/* Setup switch core pll */
> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
> +	mt7530_pad_clk_setup(priv, interface);

"interface" is completely uninitialized here.

Sorry, I'm not reviewing this any further, because it obviously doesn't work.

> +
> +	/* Enable Port 6 */
>  	val = mt7530_read(priv, MT7530_MHWTRAP);
>  	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>  	val |= MHWTRAP_MANUAL;
> @@ -2491,11 +2517,9 @@ static void mt7531_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)
> +mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>  {
> -	struct mt7530_priv *priv = ds->priv;
> -
> -	return priv->info->pad_setup(ds, state->interface);
> +	return 0;
>  }
>  
>  static int
> @@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		if (priv->p6_interface == state->interface)
>  			break;
>  
> -		mt753x_pad_setup(ds, state);
> -
>  		if (mt753x_mac_config(ds, port, mode, state) < 0)
>  			goto unsupported;
>  
> @@ -3187,7 +3209,7 @@ static 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,
> +		.pad_setup = mt7530_pad_setup,
>  		.mac_port_get_caps = mt7530_mac_port_get_caps,
>  		.mac_port_config = mt7530_mac_config,
>  	},
> @@ -3199,7 +3221,7 @@ static 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,
> +		.pad_setup = mt7530_pad_setup,

as a general rule, for bug fixes don't touch stuff that you don't need to touch

>  		.mac_port_get_caps = mt7530_mac_port_get_caps,
>  		.mac_port_config = mt7530_mac_config,
>  	},
> -- 
> 2.37.2
> 

Could you please let me know if this patch works, instead of what you've
posted above? It does what you *said* you tried to do, aka it performs
the initialization of CORE_GSWPLL_GRP2 independently of port 6 setup.
There is a slight reordering of register writes with MT7530_P6ECR, but
hopefully that doesn't bother anyone.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..a508402c4ecb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -393,6 +393,24 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
 }
 
+/* Set up switch core clock for MT7530 */
+static void mt7530_pll_setup(struct mt7530_priv *priv)
+{
+	/* Disable PLL */
+	core_write(priv, CORE_GSWPLL_GRP1, 0);
+
+	/* Set core clock into 500Mhz */
+	core_write(priv, CORE_GSWPLL_GRP2,
+		   RG_GSWPLL_POSDIV_500M(1) |
+		   RG_GSWPLL_FBKDIV_500M(25));
+
+	/* Enable PLL */
+	core_write(priv, CORE_GSWPLL_GRP1,
+		   RG_GSWPLL_EN_PRE |
+		   RG_GSWPLL_POSDIV_200M(2) |
+		   RG_GSWPLL_FBKDIV_200M(32));
+}
+
 /* Setup TX circuit including relevant PAD and driving */
 static int
 mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -453,21 +471,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 	core_clear(priv, CORE_TRGMII_GSW_CLK_CG,
 		   REG_GSWCK_EN | REG_TRGMIICK_EN);
 
-	/* Setup core clock for MT7530 */
-	/* Disable PLL */
-	core_write(priv, CORE_GSWPLL_GRP1, 0);
-
-	/* Set core clock into 500Mhz */
-	core_write(priv, CORE_GSWPLL_GRP2,
-		   RG_GSWPLL_POSDIV_500M(1) |
-		   RG_GSWPLL_FBKDIV_500M(25));
-
-	/* Enable PLL */
-	core_write(priv, CORE_GSWPLL_GRP1,
-		   RG_GSWPLL_EN_PRE |
-		   RG_GSWPLL_POSDIV_200M(2) |
-		   RG_GSWPLL_FBKDIV_200M(32));
-
 	/* Setup the MT7530 TRGMII Tx Clock */
 	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
 	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
@@ -2196,6 +2199,8 @@ mt7530_setup(struct dsa_switch *ds)
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
 		     SYS_CTRL_REG_RST);
 
+	mt7530_pll_setup(priv);
+
 	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
-- 
2.34.1


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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

Hi Arınç,

On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Move the PLL setup of the MT7530 switch out of the pad configuration of
> port 6 to mt7530_setup, after reset.

it would have been good if this patch had done that and only that, no?

> This fixes the improper initialisation of the switch when only port 5 is
> used as a CPU port.
> 
> Add supported phy modes of port 5 on the PLL setup.
> 
> Remove now incorrect comment regarding P5 as GMAC5.
> 
> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> 
> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
> driver on U-Boot [1] [2].
> 
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> 
> There are some parts I couldn't figure out myself with my limited C
> knowledge. I've pointed them out on the code. Vladimir, could you help?
> 
> There is a lot of code which is only needed for port 6 or trgmii on port 6,
> but runs whether port 6 or trgmii is used or not. For now, the best I can
> do is to fix port 5 so it works without port 6 being used.
> 
> The U-Boot driver seems to be much more organised so it could be taken as
> a reference to sort out this DSA driver further.
> 
> Also, now that the pad setup for mt7530 is also moved somewhere else, can
> we completely get rid of @pad_setup?
> 
> Arınç

I don't like the strategy you used in this patch here, and I don't want
to answer these questions just yet, because I'm not certain that my
answers will be useful in the end.

> 
> ---
>  drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 0e99de26d159..fb20ce4f443e 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>  
>  /* Setup TX circuit including relevant PAD and driving */
>  static int
> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>  {
> -	struct mt7530_priv *priv = ds->priv;
>  	u32 ncpo1, ssc_delta, trgint, i, xtal;
>  
>  	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>  		return -EINVAL;
>  	}
>  
> +	/* Is setting trgint to 0 really needed for the !trgint check? */
> +	trgint = 0;
> +
> +	/* FIXME: run this switch if p5 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 5 */
> +	switch (interface) {

so this should be something like priv->p5_interface (assuming this is
initialized by the time mt7530_pad_clk_setup() is called, which it isn't).

> +	case PHY_INTERFACE_MODE_GMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	case PHY_INTERFACE_MODE_MII:
> +		break;

so with priv->p5_interface == "mii", ncpo1 is uninitialized (will be
potentially still initialized by the port 6 "switch" statement below).

> +	case PHY_INTERFACE_MODE_RGMII:
> +		/* PLL frequency: 125MHz */
> +		ncpo1 = 0x0c80;
> +		break;
> +	default:
> +		dev_err(priv->dev, "xMII interface %d not supported\n",
> +			interface);
> +		return -EINVAL;
> +	}

What method did you use to determine the ncpo1 values here? Are they
coordinated with what p6 wants? I would expect to see a table with

  priv->p5_interface        priv->p6_interface       ncpo1 value
      gmii                     rgmii                     ???
      mii                      rgmii                     ???
      rgmii                    rgmii                     ???
      gmii                     trgmii                    ???
      mii                      trgmii                    ???
      rgmii                    trgmii                    ???

right now, you let the p6_interface logic overwrite the ncpo1 selected
by the p5_interface logic like crazy, and it's not clear to me that this
is what you want.

This problem is way deeper than knowledge of the C language, your pseudo
code simply does not describe what should happen in all combinations.

From our private conversation dated Feb 08, I guess that what you're
doing is also a useless complication and not what you truly want. What
you truly want is for the CORE_GSWPLL_GRP2 register to be programmed
regardless of whether port 6 is used or not.

> +
> +	/* FIXME: run this switch if p6 is defined on the devicetree */
> +	/* and change interface to the phy-mode of port 6 */
>  	switch (interface) {

same comment as above, this should approximate priv->p6_interface.

>  	case PHY_INTERFACE_MODE_RGMII:
> -		trgint = 0;
>  		/* PLL frequency: 125MHz */
>  		ncpo1 = 0x0c80;
>  		break;
> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>  		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>  		     SYS_CTRL_REG_RST);
>  
> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
> +	/* Setup switch core pll */
> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
> +	mt7530_pad_clk_setup(priv, interface);

"interface" is completely uninitialized here.

Sorry, I'm not reviewing this any further, because it obviously doesn't work.

> +
> +	/* Enable Port 6 */
>  	val = mt7530_read(priv, MT7530_MHWTRAP);
>  	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>  	val |= MHWTRAP_MANUAL;
> @@ -2491,11 +2517,9 @@ static void mt7531_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)
> +mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>  {
> -	struct mt7530_priv *priv = ds->priv;
> -
> -	return priv->info->pad_setup(ds, state->interface);
> +	return 0;
>  }
>  
>  static int
> @@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>  		if (priv->p6_interface == state->interface)
>  			break;
>  
> -		mt753x_pad_setup(ds, state);
> -
>  		if (mt753x_mac_config(ds, port, mode, state) < 0)
>  			goto unsupported;
>  
> @@ -3187,7 +3209,7 @@ static 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,
> +		.pad_setup = mt7530_pad_setup,
>  		.mac_port_get_caps = mt7530_mac_port_get_caps,
>  		.mac_port_config = mt7530_mac_config,
>  	},
> @@ -3199,7 +3221,7 @@ static 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,
> +		.pad_setup = mt7530_pad_setup,

as a general rule, for bug fixes don't touch stuff that you don't need to touch

>  		.mac_port_get_caps = mt7530_mac_port_get_caps,
>  		.mac_port_config = mt7530_mac_config,
>  	},
> -- 
> 2.37.2
> 

Could you please let me know if this patch works, instead of what you've
posted above? It does what you *said* you tried to do, aka it performs
the initialization of CORE_GSWPLL_GRP2 independently of port 6 setup.
There is a slight reordering of register writes with MT7530_P6ECR, but
hopefully that doesn't bother anyone.

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3a15015bc409..a508402c4ecb 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -393,6 +393,24 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
 		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
 }
 
+/* Set up switch core clock for MT7530 */
+static void mt7530_pll_setup(struct mt7530_priv *priv)
+{
+	/* Disable PLL */
+	core_write(priv, CORE_GSWPLL_GRP1, 0);
+
+	/* Set core clock into 500Mhz */
+	core_write(priv, CORE_GSWPLL_GRP2,
+		   RG_GSWPLL_POSDIV_500M(1) |
+		   RG_GSWPLL_FBKDIV_500M(25));
+
+	/* Enable PLL */
+	core_write(priv, CORE_GSWPLL_GRP1,
+		   RG_GSWPLL_EN_PRE |
+		   RG_GSWPLL_POSDIV_200M(2) |
+		   RG_GSWPLL_FBKDIV_200M(32));
+}
+
 /* Setup TX circuit including relevant PAD and driving */
 static int
 mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
@@ -453,21 +471,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
 	core_clear(priv, CORE_TRGMII_GSW_CLK_CG,
 		   REG_GSWCK_EN | REG_TRGMIICK_EN);
 
-	/* Setup core clock for MT7530 */
-	/* Disable PLL */
-	core_write(priv, CORE_GSWPLL_GRP1, 0);
-
-	/* Set core clock into 500Mhz */
-	core_write(priv, CORE_GSWPLL_GRP2,
-		   RG_GSWPLL_POSDIV_500M(1) |
-		   RG_GSWPLL_FBKDIV_500M(25));
-
-	/* Enable PLL */
-	core_write(priv, CORE_GSWPLL_GRP1,
-		   RG_GSWPLL_EN_PRE |
-		   RG_GSWPLL_POSDIV_200M(2) |
-		   RG_GSWPLL_FBKDIV_200M(32));
-
 	/* Setup the MT7530 TRGMII Tx Clock */
 	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
 	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
@@ -2196,6 +2199,8 @@ mt7530_setup(struct dsa_switch *ds)
 		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
 		     SYS_CTRL_REG_RST);
 
+	mt7530_pll_setup(priv);
+
 	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
 	val = mt7530_read(priv, MT7530_MHWTRAP);
 	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
-- 
2.34.1

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 6.03.2023 18:45, Vladimir Oltean wrote:
> Hi Arınç,
> 
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Move the PLL setup of the MT7530 switch out of the pad configuration of
>> port 6 to mt7530_setup, after reset.
> 
> it would have been good if this patch had done that and only that, no?

Agreed.

> 
>> This fixes the improper initialisation of the switch when only port 5 is
>> used as a CPU port.
>>
>> Add supported phy modes of port 5 on the PLL setup.
>>
>> Remove now incorrect comment regarding P5 as GMAC5.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>
>> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
>> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
>> driver on U-Boot [1] [2].
>>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
>> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
>> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
>>
>> There are some parts I couldn't figure out myself with my limited C
>> knowledge. I've pointed them out on the code. Vladimir, could you help?
>>
>> There is a lot of code which is only needed for port 6 or trgmii on port 6,
>> but runs whether port 6 or trgmii is used or not. For now, the best I can
>> do is to fix port 5 so it works without port 6 being used.
>>
>> The U-Boot driver seems to be much more organised so it could be taken as
>> a reference to sort out this DSA driver further.
>>
>> Also, now that the pad setup for mt7530 is also moved somewhere else, can
>> we completely get rid of @pad_setup?
>>
>> Arınç
> 
> I don't like the strategy you used in this patch here, and I don't want
> to answer these questions just yet, because I'm not certain that my
> answers will be useful in the end.
> 
>>
>> ---
>>   drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>>   1 file changed, 34 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 0e99de26d159..fb20ce4f443e 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>>   
>>   /* Setup TX circuit including relevant PAD and driving */
>>   static int
>> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>>   {
>> -	struct mt7530_priv *priv = ds->priv;
>>   	u32 ncpo1, ssc_delta, trgint, i, xtal;
>>   
>>   	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
>> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Is setting trgint to 0 really needed for the !trgint check? */
>> +	trgint = 0;
>> +
>> +	/* FIXME: run this switch if p5 is defined on the devicetree */
>> +	/* and change interface to the phy-mode of port 5 */
>> +	switch (interface) {
> 
> so this should be something like priv->p5_interface (assuming this is
> initialized by the time mt7530_pad_clk_setup() is called, which it isn't).
> 
>> +	case PHY_INTERFACE_MODE_GMII:
>> +		/* PLL frequency: 125MHz */
>> +		ncpo1 = 0x0c80;
>> +		break;
>> +	case PHY_INTERFACE_MODE_MII:
>> +		break;
> 
> so with priv->p5_interface == "mii", ncpo1 is uninitialized (will be
> potentially still initialized by the port 6 "switch" statement below).
> 
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +		/* PLL frequency: 125MHz */
>> +		ncpo1 = 0x0c80;
>> +		break;
>> +	default:
>> +		dev_err(priv->dev, "xMII interface %d not supported\n",
>> +			interface);
>> +		return -EINVAL;
>> +	}
> 
> What method did you use to determine the ncpo1 values here? Are they
> coordinated with what p6 wants? I would expect to see a table with
> 
>    priv->p5_interface        priv->p6_interface       ncpo1 value
>        gmii                     rgmii                     ???
>        mii                      rgmii                     ???
>        rgmii                    rgmii                     ???
>        gmii                     trgmii                    ???
>        mii                      trgmii                    ???
>        rgmii                    trgmii                    ???

Looking at the Wikipedia page for Media-independent interface [0], the 
data interface must be clocked at 125 MHz for gigabit MIIs, which I 
believe what the "PLL" here refers to. trgmii needs higher frequency in 
some cases so if both CPU ports are enabled, the table would be:

     priv->p5_interface        priv->p6_interface       ncpo1 value
         gmii                     rgmii                     125MHz
         mii                      rgmii                     125MHz
         rgmii                    rgmii                     125MHz
         gmii                     trgmii                    125-250MHz
         mii                      trgmii                    125-250MHz
         rgmii                    trgmii                    125-250MHz

> 
> right now, you let the p6_interface logic overwrite the ncpo1 selected
> by the p5_interface logic like crazy, and it's not clear to me that this
> is what you want.

This seems to be fine as p6 sets the frequency either the same or higher.

> 
> This problem is way deeper than knowledge of the C language, your pseudo
> code simply does not describe what should happen in all combinations.
> 
>  From our private conversation dated Feb 08, I guess that what you're
> doing is also a useless complication and not what you truly want. What
> you truly want is for the CORE_GSWPLL_GRP2 register to be programmed
> regardless of whether port 6 is used or not.
> 
>> +
>> +	/* FIXME: run this switch if p6 is defined on the devicetree */
>> +	/* and change interface to the phy-mode of port 6 */
>>   	switch (interface) {
> 
> same comment as above, this should approximate priv->p6_interface.
> 
>>   	case PHY_INTERFACE_MODE_RGMII:
>> -		trgint = 0;
>>   		/* PLL frequency: 125MHz */
>>   		ncpo1 = 0x0c80;
>>   		break;
>> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>>   		     SYS_CTRL_REG_RST);
>>   
>> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
>> +	/* Setup switch core pll */
>> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
>> +	mt7530_pad_clk_setup(priv, interface);
> 
> "interface" is completely uninitialized here.
> 
> Sorry, I'm not reviewing this any further, because it obviously doesn't work.
> 
>> +
>> +	/* Enable Port 6 */
>>   	val = mt7530_read(priv, MT7530_MHWTRAP);
>>   	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>>   	val |= MHWTRAP_MANUAL;
>> @@ -2491,11 +2517,9 @@ static void mt7531_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)
>> +mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   {
>> -	struct mt7530_priv *priv = ds->priv;
>> -
>> -	return priv->info->pad_setup(ds, state->interface);
>> +	return 0;
>>   }
>>   
>>   static int
>> @@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>   		if (priv->p6_interface == state->interface)
>>   			break;
>>   
>> -		mt753x_pad_setup(ds, state);
>> -
>>   		if (mt753x_mac_config(ds, port, mode, state) < 0)
>>   			goto unsupported;
>>   
>> @@ -3187,7 +3209,7 @@ static 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,
>> +		.pad_setup = mt7530_pad_setup,
>>   		.mac_port_get_caps = mt7530_mac_port_get_caps,
>>   		.mac_port_config = mt7530_mac_config,
>>   	},
>> @@ -3199,7 +3221,7 @@ static 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,
>> +		.pad_setup = mt7530_pad_setup,
> 
> as a general rule, for bug fixes don't touch stuff that you don't need to touch
> 
>>   		.mac_port_get_caps = mt7530_mac_port_get_caps,
>>   		.mac_port_config = mt7530_mac_config,
>>   	},
>> -- 
>> 2.37.2
>>
> 
> Could you please let me know if this patch works, instead of what you've
> posted above? It does what you *said* you tried to do, aka it performs
> the initialization of CORE_GSWPLL_GRP2 independently of port 6 setup.
> There is a slight reordering of register writes with MT7530_P6ECR, but
> hopefully that doesn't bother anyone.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3a15015bc409..a508402c4ecb 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -393,6 +393,24 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>   		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
>   }
>   
> +/* Set up switch core clock for MT7530 */
> +static void mt7530_pll_setup(struct mt7530_priv *priv)
> +{
> +	/* Disable PLL */
> +	core_write(priv, CORE_GSWPLL_GRP1, 0);
> +
> +	/* Set core clock into 500Mhz */
> +	core_write(priv, CORE_GSWPLL_GRP2,
> +		   RG_GSWPLL_POSDIV_500M(1) |
> +		   RG_GSWPLL_FBKDIV_500M(25));
> +
> +	/* Enable PLL */
> +	core_write(priv, CORE_GSWPLL_GRP1,
> +		   RG_GSWPLL_EN_PRE |
> +		   RG_GSWPLL_POSDIV_200M(2) |
> +		   RG_GSWPLL_FBKDIV_200M(32));
> +}
> +
>   /* Setup TX circuit including relevant PAD and driving */
>   static int
>   mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> @@ -453,21 +471,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>   	core_clear(priv, CORE_TRGMII_GSW_CLK_CG,
>   		   REG_GSWCK_EN | REG_TRGMIICK_EN);
>   
> -	/* Setup core clock for MT7530 */
> -	/* Disable PLL */
> -	core_write(priv, CORE_GSWPLL_GRP1, 0);
> -
> -	/* Set core clock into 500Mhz */
> -	core_write(priv, CORE_GSWPLL_GRP2,
> -		   RG_GSWPLL_POSDIV_500M(1) |
> -		   RG_GSWPLL_FBKDIV_500M(25));
> -
> -	/* Enable PLL */
> -	core_write(priv, CORE_GSWPLL_GRP1,
> -		   RG_GSWPLL_EN_PRE |
> -		   RG_GSWPLL_POSDIV_200M(2) |
> -		   RG_GSWPLL_FBKDIV_200M(32));
> -
>   	/* Setup the MT7530 TRGMII Tx Clock */
>   	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
>   	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> @@ -2196,6 +2199,8 @@ mt7530_setup(struct dsa_switch *ds)
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>   		     SYS_CTRL_REG_RST);
>   
> +	mt7530_pll_setup(priv);
> +
>   	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
>   	val = mt7530_read(priv, MT7530_MHWTRAP);
>   	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;

This looks much better, thanks a lot! The only missing part is setting 
the PLL frequency when only port 5 is enabled.

I'll test it regardless.

[0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 6.03.2023 18:45, Vladimir Oltean wrote:
> Hi Arınç,
> 
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Move the PLL setup of the MT7530 switch out of the pad configuration of
>> port 6 to mt7530_setup, after reset.
> 
> it would have been good if this patch had done that and only that, no?

Agreed.

> 
>> This fixes the improper initialisation of the switch when only port 5 is
>> used as a CPU port.
>>
>> Add supported phy modes of port 5 on the PLL setup.
>>
>> Remove now incorrect comment regarding P5 as GMAC5.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>
>> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
>> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
>> driver on U-Boot [1] [2].
>>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
>> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
>> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
>>
>> There are some parts I couldn't figure out myself with my limited C
>> knowledge. I've pointed them out on the code. Vladimir, could you help?
>>
>> There is a lot of code which is only needed for port 6 or trgmii on port 6,
>> but runs whether port 6 or trgmii is used or not. For now, the best I can
>> do is to fix port 5 so it works without port 6 being used.
>>
>> The U-Boot driver seems to be much more organised so it could be taken as
>> a reference to sort out this DSA driver further.
>>
>> Also, now that the pad setup for mt7530 is also moved somewhere else, can
>> we completely get rid of @pad_setup?
>>
>> Arınç
> 
> I don't like the strategy you used in this patch here, and I don't want
> to answer these questions just yet, because I'm not certain that my
> answers will be useful in the end.
> 
>>
>> ---
>>   drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>>   1 file changed, 34 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 0e99de26d159..fb20ce4f443e 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>>   
>>   /* Setup TX circuit including relevant PAD and driving */
>>   static int
>> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>>   {
>> -	struct mt7530_priv *priv = ds->priv;
>>   	u32 ncpo1, ssc_delta, trgint, i, xtal;
>>   
>>   	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
>> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Is setting trgint to 0 really needed for the !trgint check? */
>> +	trgint = 0;
>> +
>> +	/* FIXME: run this switch if p5 is defined on the devicetree */
>> +	/* and change interface to the phy-mode of port 5 */
>> +	switch (interface) {
> 
> so this should be something like priv->p5_interface (assuming this is
> initialized by the time mt7530_pad_clk_setup() is called, which it isn't).
> 
>> +	case PHY_INTERFACE_MODE_GMII:
>> +		/* PLL frequency: 125MHz */
>> +		ncpo1 = 0x0c80;
>> +		break;
>> +	case PHY_INTERFACE_MODE_MII:
>> +		break;
> 
> so with priv->p5_interface == "mii", ncpo1 is uninitialized (will be
> potentially still initialized by the port 6 "switch" statement below).
> 
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +		/* PLL frequency: 125MHz */
>> +		ncpo1 = 0x0c80;
>> +		break;
>> +	default:
>> +		dev_err(priv->dev, "xMII interface %d not supported\n",
>> +			interface);
>> +		return -EINVAL;
>> +	}
> 
> What method did you use to determine the ncpo1 values here? Are they
> coordinated with what p6 wants? I would expect to see a table with
> 
>    priv->p5_interface        priv->p6_interface       ncpo1 value
>        gmii                     rgmii                     ???
>        mii                      rgmii                     ???
>        rgmii                    rgmii                     ???
>        gmii                     trgmii                    ???
>        mii                      trgmii                    ???
>        rgmii                    trgmii                    ???

Looking at the Wikipedia page for Media-independent interface [0], the 
data interface must be clocked at 125 MHz for gigabit MIIs, which I 
believe what the "PLL" here refers to. trgmii needs higher frequency in 
some cases so if both CPU ports are enabled, the table would be:

     priv->p5_interface        priv->p6_interface       ncpo1 value
         gmii                     rgmii                     125MHz
         mii                      rgmii                     125MHz
         rgmii                    rgmii                     125MHz
         gmii                     trgmii                    125-250MHz
         mii                      trgmii                    125-250MHz
         rgmii                    trgmii                    125-250MHz

> 
> right now, you let the p6_interface logic overwrite the ncpo1 selected
> by the p5_interface logic like crazy, and it's not clear to me that this
> is what you want.

This seems to be fine as p6 sets the frequency either the same or higher.

> 
> This problem is way deeper than knowledge of the C language, your pseudo
> code simply does not describe what should happen in all combinations.
> 
>  From our private conversation dated Feb 08, I guess that what you're
> doing is also a useless complication and not what you truly want. What
> you truly want is for the CORE_GSWPLL_GRP2 register to be programmed
> regardless of whether port 6 is used or not.
> 
>> +
>> +	/* FIXME: run this switch if p6 is defined on the devicetree */
>> +	/* and change interface to the phy-mode of port 6 */
>>   	switch (interface) {
> 
> same comment as above, this should approximate priv->p6_interface.
> 
>>   	case PHY_INTERFACE_MODE_RGMII:
>> -		trgint = 0;
>>   		/* PLL frequency: 125MHz */
>>   		ncpo1 = 0x0c80;
>>   		break;
>> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>>   		     SYS_CTRL_REG_RST);
>>   
>> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
>> +	/* Setup switch core pll */
>> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
>> +	mt7530_pad_clk_setup(priv, interface);
> 
> "interface" is completely uninitialized here.
> 
> Sorry, I'm not reviewing this any further, because it obviously doesn't work.
> 
>> +
>> +	/* Enable Port 6 */
>>   	val = mt7530_read(priv, MT7530_MHWTRAP);
>>   	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>>   	val |= MHWTRAP_MANUAL;
>> @@ -2491,11 +2517,9 @@ static void mt7531_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)
>> +mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   {
>> -	struct mt7530_priv *priv = ds->priv;
>> -
>> -	return priv->info->pad_setup(ds, state->interface);
>> +	return 0;
>>   }
>>   
>>   static int
>> @@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>   		if (priv->p6_interface == state->interface)
>>   			break;
>>   
>> -		mt753x_pad_setup(ds, state);
>> -
>>   		if (mt753x_mac_config(ds, port, mode, state) < 0)
>>   			goto unsupported;
>>   
>> @@ -3187,7 +3209,7 @@ static 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,
>> +		.pad_setup = mt7530_pad_setup,
>>   		.mac_port_get_caps = mt7530_mac_port_get_caps,
>>   		.mac_port_config = mt7530_mac_config,
>>   	},
>> @@ -3199,7 +3221,7 @@ static 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,
>> +		.pad_setup = mt7530_pad_setup,
> 
> as a general rule, for bug fixes don't touch stuff that you don't need to touch
> 
>>   		.mac_port_get_caps = mt7530_mac_port_get_caps,
>>   		.mac_port_config = mt7530_mac_config,
>>   	},
>> -- 
>> 2.37.2
>>
> 
> Could you please let me know if this patch works, instead of what you've
> posted above? It does what you *said* you tried to do, aka it performs
> the initialization of CORE_GSWPLL_GRP2 independently of port 6 setup.
> There is a slight reordering of register writes with MT7530_P6ECR, but
> hopefully that doesn't bother anyone.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3a15015bc409..a508402c4ecb 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -393,6 +393,24 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>   		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
>   }
>   
> +/* Set up switch core clock for MT7530 */
> +static void mt7530_pll_setup(struct mt7530_priv *priv)
> +{
> +	/* Disable PLL */
> +	core_write(priv, CORE_GSWPLL_GRP1, 0);
> +
> +	/* Set core clock into 500Mhz */
> +	core_write(priv, CORE_GSWPLL_GRP2,
> +		   RG_GSWPLL_POSDIV_500M(1) |
> +		   RG_GSWPLL_FBKDIV_500M(25));
> +
> +	/* Enable PLL */
> +	core_write(priv, CORE_GSWPLL_GRP1,
> +		   RG_GSWPLL_EN_PRE |
> +		   RG_GSWPLL_POSDIV_200M(2) |
> +		   RG_GSWPLL_FBKDIV_200M(32));
> +}
> +
>   /* Setup TX circuit including relevant PAD and driving */
>   static int
>   mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> @@ -453,21 +471,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>   	core_clear(priv, CORE_TRGMII_GSW_CLK_CG,
>   		   REG_GSWCK_EN | REG_TRGMIICK_EN);
>   
> -	/* Setup core clock for MT7530 */
> -	/* Disable PLL */
> -	core_write(priv, CORE_GSWPLL_GRP1, 0);
> -
> -	/* Set core clock into 500Mhz */
> -	core_write(priv, CORE_GSWPLL_GRP2,
> -		   RG_GSWPLL_POSDIV_500M(1) |
> -		   RG_GSWPLL_FBKDIV_500M(25));
> -
> -	/* Enable PLL */
> -	core_write(priv, CORE_GSWPLL_GRP1,
> -		   RG_GSWPLL_EN_PRE |
> -		   RG_GSWPLL_POSDIV_200M(2) |
> -		   RG_GSWPLL_FBKDIV_200M(32));
> -
>   	/* Setup the MT7530 TRGMII Tx Clock */
>   	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
>   	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> @@ -2196,6 +2199,8 @@ mt7530_setup(struct dsa_switch *ds)
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>   		     SYS_CTRL_REG_RST);
>   
> +	mt7530_pll_setup(priv);
> +
>   	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
>   	val = mt7530_read(priv, MT7530_MHWTRAP);
>   	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;

This looks much better, thanks a lot! The only missing part is setting 
the PLL frequency when only port 5 is enabled.

I'll test it regardless.

[0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII

Arınç

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 6.03.2023 18:45, Vladimir Oltean wrote:
> Hi Arınç,
> 
> On Sat, Mar 04, 2023 at 03:54:54PM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Move the PLL setup of the MT7530 switch out of the pad configuration of
>> port 6 to mt7530_setup, after reset.
> 
> it would have been good if this patch had done that and only that, no?

Agreed.

> 
>> This fixes the improper initialisation of the switch when only port 5 is
>> used as a CPU port.
>>
>> Add supported phy modes of port 5 on the PLL setup.
>>
>> Remove now incorrect comment regarding P5 as GMAC5.
>>
>> Fixes: b8f126a8d543 ("net-next: dsa: add dsa support for Mediatek MT7530 switch")
>> Fixes: 38f790a80560 ("net: dsa: mt7530: Add support for port 5")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
>>
>> I'm trying to mimic this change by Alexander [0] for the MT7530 switch.
>> This is already the case for MT7530 and MT7531 on the MediaTek ethernet
>> driver on U-Boot [1] [2].
>>
>> [0] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=42bc4fafe359ed6b73602b7a2dba0dd99588f8ce
>> [1] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L729
>> [2] https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
>>
>> There are some parts I couldn't figure out myself with my limited C
>> knowledge. I've pointed them out on the code. Vladimir, could you help?
>>
>> There is a lot of code which is only needed for port 6 or trgmii on port 6,
>> but runs whether port 6 or trgmii is used or not. For now, the best I can
>> do is to fix port 5 so it works without port 6 being used.
>>
>> The U-Boot driver seems to be much more organised so it could be taken as
>> a reference to sort out this DSA driver further.
>>
>> Also, now that the pad setup for mt7530 is also moved somewhere else, can
>> we completely get rid of @pad_setup?
>>
>> Arınç
> 
> I don't like the strategy you used in this patch here, and I don't want
> to answer these questions just yet, because I'm not certain that my
> answers will be useful in the end.
> 
>>
>> ---
>>   drivers/net/dsa/mt7530.c | 46 +++++++++++++++++++++++++++++-----------
>>   1 file changed, 34 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 0e99de26d159..fb20ce4f443e 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -395,9 +395,8 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>>   
>>   /* Setup TX circuit including relevant PAD and driving */
>>   static int
>> -mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>> +mt7530_pad_clk_setup(struct mt7530_priv *priv, phy_interface_t interface)
>>   {
>> -	struct mt7530_priv *priv = ds->priv;
>>   	u32 ncpo1, ssc_delta, trgint, i, xtal;
>>   
>>   	xtal = mt7530_read(priv, MT7530_MHWTRAP) & HWTRAP_XTAL_MASK;
>> @@ -409,9 +408,32 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   		return -EINVAL;
>>   	}
>>   
>> +	/* Is setting trgint to 0 really needed for the !trgint check? */
>> +	trgint = 0;
>> +
>> +	/* FIXME: run this switch if p5 is defined on the devicetree */
>> +	/* and change interface to the phy-mode of port 5 */
>> +	switch (interface) {
> 
> so this should be something like priv->p5_interface (assuming this is
> initialized by the time mt7530_pad_clk_setup() is called, which it isn't).
> 
>> +	case PHY_INTERFACE_MODE_GMII:
>> +		/* PLL frequency: 125MHz */
>> +		ncpo1 = 0x0c80;
>> +		break;
>> +	case PHY_INTERFACE_MODE_MII:
>> +		break;
> 
> so with priv->p5_interface == "mii", ncpo1 is uninitialized (will be
> potentially still initialized by the port 6 "switch" statement below).
> 
>> +	case PHY_INTERFACE_MODE_RGMII:
>> +		/* PLL frequency: 125MHz */
>> +		ncpo1 = 0x0c80;
>> +		break;
>> +	default:
>> +		dev_err(priv->dev, "xMII interface %d not supported\n",
>> +			interface);
>> +		return -EINVAL;
>> +	}
> 
> What method did you use to determine the ncpo1 values here? Are they
> coordinated with what p6 wants? I would expect to see a table with
> 
>    priv->p5_interface        priv->p6_interface       ncpo1 value
>        gmii                     rgmii                     ???
>        mii                      rgmii                     ???
>        rgmii                    rgmii                     ???
>        gmii                     trgmii                    ???
>        mii                      trgmii                    ???
>        rgmii                    trgmii                    ???

Looking at the Wikipedia page for Media-independent interface [0], the 
data interface must be clocked at 125 MHz for gigabit MIIs, which I 
believe what the "PLL" here refers to. trgmii needs higher frequency in 
some cases so if both CPU ports are enabled, the table would be:

     priv->p5_interface        priv->p6_interface       ncpo1 value
         gmii                     rgmii                     125MHz
         mii                      rgmii                     125MHz
         rgmii                    rgmii                     125MHz
         gmii                     trgmii                    125-250MHz
         mii                      trgmii                    125-250MHz
         rgmii                    trgmii                    125-250MHz

> 
> right now, you let the p6_interface logic overwrite the ncpo1 selected
> by the p5_interface logic like crazy, and it's not clear to me that this
> is what you want.

This seems to be fine as p6 sets the frequency either the same or higher.

> 
> This problem is way deeper than knowledge of the C language, your pseudo
> code simply does not describe what should happen in all combinations.
> 
>  From our private conversation dated Feb 08, I guess that what you're
> doing is also a useless complication and not what you truly want. What
> you truly want is for the CORE_GSWPLL_GRP2 register to be programmed
> regardless of whether port 6 is used or not.
> 
>> +
>> +	/* FIXME: run this switch if p6 is defined on the devicetree */
>> +	/* and change interface to the phy-mode of port 6 */
>>   	switch (interface) {
> 
> same comment as above, this should approximate priv->p6_interface.
> 
>>   	case PHY_INTERFACE_MODE_RGMII:
>> -		trgint = 0;
>>   		/* PLL frequency: 125MHz */
>>   		ncpo1 = 0x0c80;
>>   		break;
>> @@ -2172,7 +2194,11 @@ mt7530_setup(struct dsa_switch *ds)
>>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>>   		     SYS_CTRL_REG_RST);
>>   
>> -	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
>> +	/* Setup switch core pll */
>> +	/* FIXME: feed the phy-mode of port 5 and 6, if the ports are defined on the devicetree */
>> +	mt7530_pad_clk_setup(priv, interface);
> 
> "interface" is completely uninitialized here.
> 
> Sorry, I'm not reviewing this any further, because it obviously doesn't work.
> 
>> +
>> +	/* Enable Port 6 */
>>   	val = mt7530_read(priv, MT7530_MHWTRAP);
>>   	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;
>>   	val |= MHWTRAP_MANUAL;
>> @@ -2491,11 +2517,9 @@ static void mt7531_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)
>> +mt7530_pad_setup(struct dsa_switch *ds, phy_interface_t interface)
>>   {
>> -	struct mt7530_priv *priv = ds->priv;
>> -
>> -	return priv->info->pad_setup(ds, state->interface);
>> +	return 0;
>>   }
>>   
>>   static int
>> @@ -2769,8 +2793,6 @@ mt753x_phylink_mac_config(struct dsa_switch *ds, int port, unsigned int mode,
>>   		if (priv->p6_interface == state->interface)
>>   			break;
>>   
>> -		mt753x_pad_setup(ds, state);
>> -
>>   		if (mt753x_mac_config(ds, port, mode, state) < 0)
>>   			goto unsupported;
>>   
>> @@ -3187,7 +3209,7 @@ static 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,
>> +		.pad_setup = mt7530_pad_setup,
>>   		.mac_port_get_caps = mt7530_mac_port_get_caps,
>>   		.mac_port_config = mt7530_mac_config,
>>   	},
>> @@ -3199,7 +3221,7 @@ static 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,
>> +		.pad_setup = mt7530_pad_setup,
> 
> as a general rule, for bug fixes don't touch stuff that you don't need to touch
> 
>>   		.mac_port_get_caps = mt7530_mac_port_get_caps,
>>   		.mac_port_config = mt7530_mac_config,
>>   	},
>> -- 
>> 2.37.2
>>
> 
> Could you please let me know if this patch works, instead of what you've
> posted above? It does what you *said* you tried to do, aka it performs
> the initialization of CORE_GSWPLL_GRP2 independently of port 6 setup.
> There is a slight reordering of register writes with MT7530_P6ECR, but
> hopefully that doesn't bother anyone.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3a15015bc409..a508402c4ecb 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -393,6 +393,24 @@ mt7530_fdb_write(struct mt7530_priv *priv, u16 vid,
>   		mt7530_write(priv, MT7530_ATA1 + (i * 4), reg[i]);
>   }
>   
> +/* Set up switch core clock for MT7530 */
> +static void mt7530_pll_setup(struct mt7530_priv *priv)
> +{
> +	/* Disable PLL */
> +	core_write(priv, CORE_GSWPLL_GRP1, 0);
> +
> +	/* Set core clock into 500Mhz */
> +	core_write(priv, CORE_GSWPLL_GRP2,
> +		   RG_GSWPLL_POSDIV_500M(1) |
> +		   RG_GSWPLL_FBKDIV_500M(25));
> +
> +	/* Enable PLL */
> +	core_write(priv, CORE_GSWPLL_GRP1,
> +		   RG_GSWPLL_EN_PRE |
> +		   RG_GSWPLL_POSDIV_200M(2) |
> +		   RG_GSWPLL_FBKDIV_200M(32));
> +}
> +
>   /* Setup TX circuit including relevant PAD and driving */
>   static int
>   mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> @@ -453,21 +471,6 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>   	core_clear(priv, CORE_TRGMII_GSW_CLK_CG,
>   		   REG_GSWCK_EN | REG_TRGMIICK_EN);
>   
> -	/* Setup core clock for MT7530 */
> -	/* Disable PLL */
> -	core_write(priv, CORE_GSWPLL_GRP1, 0);
> -
> -	/* Set core clock into 500Mhz */
> -	core_write(priv, CORE_GSWPLL_GRP2,
> -		   RG_GSWPLL_POSDIV_500M(1) |
> -		   RG_GSWPLL_FBKDIV_500M(25));
> -
> -	/* Enable PLL */
> -	core_write(priv, CORE_GSWPLL_GRP1,
> -		   RG_GSWPLL_EN_PRE |
> -		   RG_GSWPLL_POSDIV_200M(2) |
> -		   RG_GSWPLL_FBKDIV_200M(32));
> -
>   	/* Setup the MT7530 TRGMII Tx Clock */
>   	core_write(priv, CORE_PLL_GROUP5, RG_LCDDS_PCW_NCPO1(ncpo1));
>   	core_write(priv, CORE_PLL_GROUP6, RG_LCDDS_PCW_NCPO0(0));
> @@ -2196,6 +2199,8 @@ mt7530_setup(struct dsa_switch *ds)
>   		     SYS_CTRL_PHY_RST | SYS_CTRL_SW_RST |
>   		     SYS_CTRL_REG_RST);
>   
> +	mt7530_pll_setup(priv);
> +
>   	/* Enable Port 6 only; P5 as GMAC5 which currently is not supported */
>   	val = mt7530_read(priv, MT7530_MHWTRAP);
>   	val &= ~MHWTRAP_P6_DIS & ~MHWTRAP_PHY_ACCESS;

This looks much better, thanks a lot! The only missing part is setting 
the PLL frequency when only port 5 is enabled.

I'll test it regardless.

[0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

On Mon, Mar 06, 2023 at 08:03:54PM +0300, Arınç ÜNAL wrote:
> Looking at the Wikipedia page for Media-independent interface [0], the data
> interface must be clocked at 125 MHz for gigabit MIIs, which I believe what
> the "PLL" here refers to. trgmii needs higher frequency in some cases so if
> both CPU ports are enabled, the table would be:
> 
>     priv->p5_interface        priv->p6_interface       ncpo1 value
>         gmii                     rgmii                     125MHz
>         mii                      rgmii                     125MHz
>         rgmii                    rgmii                     125MHz
>         gmii                     trgmii                    125-250MHz
>         mii                      trgmii                    125-250MHz
>         rgmii                    trgmii                    125-250MHz
> 
> [0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII

Wikipedia will only tell you what the frequency of the interface signals
needs to be. That is useful to keep in mind, but without information from
the datasheet regarding what the SoC's clock distribution tree looks like,
it's hard to know how that interface clock is derived from internal PLLs
and ultimately from the oscillators. I was hoping that was the kind of
information you could provide. The manuals I have access to, through charity,
don't say anything on that front.

Since I don't know what I'm commenting on, I'll stop commenting any further.

> > right now, you let the p6_interface logic overwrite the ncpo1 selected
> > by the p5_interface logic like crazy, and it's not clear to me that this
> > is what you want.
> 
> This seems to be fine as p6 sets the frequency either the same or higher.

(...)

> This looks much better, thanks a lot! The only missing part is setting the
> PLL frequency when only port 5 is enabled.

True. Although with the limited information I have, I'm not sure that
the ncpo1 value written into CORE_PLL_GROUP5 is needed by port5 either
way. The fact that you claim port5 works when ncpo1 ranges from 125 to
250 MHz tells me that it's either very tolerant of the ncpo1 value
(through mechanisms unknown to me), or simply unaffected by it (more
likely ATM). Since I don't have any details regarding the value, I'd
just like to treat the configuration procedure as plain code, and not
make any changes until there's a proof that they're needed.

> I'll test it regardless.

Thanks.

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

On Mon, Mar 06, 2023 at 08:03:54PM +0300, Arınç ÜNAL wrote:
> Looking at the Wikipedia page for Media-independent interface [0], the data
> interface must be clocked at 125 MHz for gigabit MIIs, which I believe what
> the "PLL" here refers to. trgmii needs higher frequency in some cases so if
> both CPU ports are enabled, the table would be:
> 
>     priv->p5_interface        priv->p6_interface       ncpo1 value
>         gmii                     rgmii                     125MHz
>         mii                      rgmii                     125MHz
>         rgmii                    rgmii                     125MHz
>         gmii                     trgmii                    125-250MHz
>         mii                      trgmii                    125-250MHz
>         rgmii                    trgmii                    125-250MHz
> 
> [0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII

Wikipedia will only tell you what the frequency of the interface signals
needs to be. That is useful to keep in mind, but without information from
the datasheet regarding what the SoC's clock distribution tree looks like,
it's hard to know how that interface clock is derived from internal PLLs
and ultimately from the oscillators. I was hoping that was the kind of
information you could provide. The manuals I have access to, through charity,
don't say anything on that front.

Since I don't know what I'm commenting on, I'll stop commenting any further.

> > right now, you let the p6_interface logic overwrite the ncpo1 selected
> > by the p5_interface logic like crazy, and it's not clear to me that this
> > is what you want.
> 
> This seems to be fine as p6 sets the frequency either the same or higher.

(...)

> This looks much better, thanks a lot! The only missing part is setting the
> PLL frequency when only port 5 is enabled.

True. Although with the limited information I have, I'm not sure that
the ncpo1 value written into CORE_PLL_GROUP5 is needed by port5 either
way. The fact that you claim port5 works when ncpo1 ranges from 125 to
250 MHz tells me that it's either very tolerant of the ncpo1 value
(through mechanisms unknown to me), or simply unaffected by it (more
likely ATM). Since I don't have any details regarding the value, I'd
just like to treat the configuration procedure as plain code, and not
make any changes until there's a proof that they're needed.

> I'll test it regardless.

Thanks.

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

On Mon, Mar 06, 2023 at 08:03:54PM +0300, Arınç ÜNAL wrote:
> Looking at the Wikipedia page for Media-independent interface [0], the data
> interface must be clocked at 125 MHz for gigabit MIIs, which I believe what
> the "PLL" here refers to. trgmii needs higher frequency in some cases so if
> both CPU ports are enabled, the table would be:
> 
>     priv->p5_interface        priv->p6_interface       ncpo1 value
>         gmii                     rgmii                     125MHz
>         mii                      rgmii                     125MHz
>         rgmii                    rgmii                     125MHz
>         gmii                     trgmii                    125-250MHz
>         mii                      trgmii                    125-250MHz
>         rgmii                    trgmii                    125-250MHz
> 
> [0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII

Wikipedia will only tell you what the frequency of the interface signals
needs to be. That is useful to keep in mind, but without information from
the datasheet regarding what the SoC's clock distribution tree looks like,
it's hard to know how that interface clock is derived from internal PLLs
and ultimately from the oscillators. I was hoping that was the kind of
information you could provide. The manuals I have access to, through charity,
don't say anything on that front.

Since I don't know what I'm commenting on, I'll stop commenting any further.

> > right now, you let the p6_interface logic overwrite the ncpo1 selected
> > by the p5_interface logic like crazy, and it's not clear to me that this
> > is what you want.
> 
> This seems to be fine as p6 sets the frequency either the same or higher.

(...)

> This looks much better, thanks a lot! The only missing part is setting the
> PLL frequency when only port 5 is enabled.

True. Although with the limited information I have, I'm not sure that
the ncpo1 value written into CORE_PLL_GROUP5 is needed by port5 either
way. The fact that you claim port5 works when ncpo1 ranges from 125 to
250 MHz tells me that it's either very tolerant of the ncpo1 value
(through mechanisms unknown to me), or simply unaffected by it (more
likely ATM). Since I don't have any details regarding the value, I'd
just like to treat the configuration procedure as plain code, and not
make any changes until there's a proof that they're needed.

> I'll test it regardless.

Thanks.

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 6.03.2023 23:19, Vladimir Oltean wrote:
> On Mon, Mar 06, 2023 at 08:03:54PM +0300, Arınç ÜNAL wrote:
>> Looking at the Wikipedia page for Media-independent interface [0], the data
>> interface must be clocked at 125 MHz for gigabit MIIs, which I believe what
>> the "PLL" here refers to. trgmii needs higher frequency in some cases so if
>> both CPU ports are enabled, the table would be:
>>
>>      priv->p5_interface        priv->p6_interface       ncpo1 value
>>          gmii                     rgmii                     125MHz
>>          mii                      rgmii                     125MHz
>>          rgmii                    rgmii                     125MHz
>>          gmii                     trgmii                    125-250MHz
>>          mii                      trgmii                    125-250MHz
>>          rgmii                    trgmii                    125-250MHz
>>
>> [0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII
> 
> Wikipedia will only tell you what the frequency of the interface signals
> needs to be. That is useful to keep in mind, but without information from
> the datasheet regarding what the SoC's clock distribution tree looks like,
> it's hard to know how that interface clock is derived from internal PLLs
> and ultimately from the oscillators. I was hoping that was the kind of
> information you could provide. The manuals I have access to, through charity,
> don't say anything on that front.

I wish I could. There are three people associated with MediaTek CC'd 
here. Maybe one will care to inform us.

> 
> Since I don't know what I'm commenting on, I'll stop commenting any further.
> 
>>> right now, you let the p6_interface logic overwrite the ncpo1 selected
>>> by the p5_interface logic like crazy, and it's not clear to me that this
>>> is what you want.
>>
>> This seems to be fine as p6 sets the frequency either the same or higher.
> 
> (...)

Sorry for the vague reply, my assumption was that interfaces with slower 
speeds, 100M, 1000M, etc. works fine at higher PLL frequency whilst they 
won't work the other way around.

> 
>> This looks much better, thanks a lot! The only missing part is setting the
>> PLL frequency when only port 5 is enabled.
> 
> True. Although with the limited information I have, I'm not sure that
> the ncpo1 value written into CORE_PLL_GROUP5 is needed by port5 either
> way. The fact that you claim port5 works when ncpo1 ranges from 125 to
> 250 MHz tells me that it's either very tolerant of the ncpo1 value
> (through mechanisms unknown to me), or simply unaffected by it (more
> likely ATM). Since I don't have any details regarding the value, I'd
> just like to treat the configuration procedure as plain code, and not
> make any changes until there's a proof that they're needed.
> 
>> I'll test it regardless.
> 
> Thanks.

Port 5 as CPU port works fine with this patch. I completely removed from 
port 6 phy modes.

With your patch on MT7621 (remember port 5 always worked on MT7623):

- Port 5 at rgmii as the only CPU port works, even though the PLL 
frequency won't be set. The download/upload speed is not affected.

- port 6 at trgmii mode won't work if the PLL frequency is not set. The 
SoC's MAC (gmac0) won't receive anything. It checks out since setting 
the PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" 
comment. So port 6 cannot properly transmit frames to the SoC's MAC.

- Port 6 at rgmii mode works without setting the PLL frequency. Speed is 
not affected.

I commented out core_write(priv, CORE_PLL_GROUP5, 
RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.

In conclusion, setting the PLL frequency is only needed for the trgmii 
mode, so I believe we can get rid of it on other cases.

One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and 
HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if 
xtal matching both of them is the expected behaviour.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fbf27d4ab5d9..12cea89ae0ac 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>  			/* PLL frequency: 150MHz: 1.2GBit */
>  			if (xtal == HWTRAP_XTAL_40MHZ)
>  				ncpo1 = 0x0780;
> +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
>  			if (xtal == HWTRAP_XTAL_25MHZ)
>  				ncpo1 = 0x0a00;
> +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
> +			if (xtal == HWTRAP_XTAL_20MHZ)
> +				dev_info(priv->dev, "XTAL is 20MHz too\n");
>  		} else { /* PLL frequency: 250MHz: 2.0Gbit */
>  			if (xtal == HWTRAP_XTAL_40MHZ)
>  				ncpo1 = 0x0c80;

> [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
> [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
> [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
> [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
> [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
> [    0.837007] DSA: tree 0 setup

Thunderbird limits lines to about 72 columns, so I'm pasting as 
quotation which seems to bypass that.

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 6.03.2023 23:19, Vladimir Oltean wrote:
> On Mon, Mar 06, 2023 at 08:03:54PM +0300, Arınç ÜNAL wrote:
>> Looking at the Wikipedia page for Media-independent interface [0], the data
>> interface must be clocked at 125 MHz for gigabit MIIs, which I believe what
>> the "PLL" here refers to. trgmii needs higher frequency in some cases so if
>> both CPU ports are enabled, the table would be:
>>
>>      priv->p5_interface        priv->p6_interface       ncpo1 value
>>          gmii                     rgmii                     125MHz
>>          mii                      rgmii                     125MHz
>>          rgmii                    rgmii                     125MHz
>>          gmii                     trgmii                    125-250MHz
>>          mii                      trgmii                    125-250MHz
>>          rgmii                    trgmii                    125-250MHz
>>
>> [0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII
> 
> Wikipedia will only tell you what the frequency of the interface signals
> needs to be. That is useful to keep in mind, but without information from
> the datasheet regarding what the SoC's clock distribution tree looks like,
> it's hard to know how that interface clock is derived from internal PLLs
> and ultimately from the oscillators. I was hoping that was the kind of
> information you could provide. The manuals I have access to, through charity,
> don't say anything on that front.

I wish I could. There are three people associated with MediaTek CC'd 
here. Maybe one will care to inform us.

> 
> Since I don't know what I'm commenting on, I'll stop commenting any further.
> 
>>> right now, you let the p6_interface logic overwrite the ncpo1 selected
>>> by the p5_interface logic like crazy, and it's not clear to me that this
>>> is what you want.
>>
>> This seems to be fine as p6 sets the frequency either the same or higher.
> 
> (...)

Sorry for the vague reply, my assumption was that interfaces with slower 
speeds, 100M, 1000M, etc. works fine at higher PLL frequency whilst they 
won't work the other way around.

> 
>> This looks much better, thanks a lot! The only missing part is setting the
>> PLL frequency when only port 5 is enabled.
> 
> True. Although with the limited information I have, I'm not sure that
> the ncpo1 value written into CORE_PLL_GROUP5 is needed by port5 either
> way. The fact that you claim port5 works when ncpo1 ranges from 125 to
> 250 MHz tells me that it's either very tolerant of the ncpo1 value
> (through mechanisms unknown to me), or simply unaffected by it (more
> likely ATM). Since I don't have any details regarding the value, I'd
> just like to treat the configuration procedure as plain code, and not
> make any changes until there's a proof that they're needed.
> 
>> I'll test it regardless.
> 
> Thanks.

Port 5 as CPU port works fine with this patch. I completely removed from 
port 6 phy modes.

With your patch on MT7621 (remember port 5 always worked on MT7623):

- Port 5 at rgmii as the only CPU port works, even though the PLL 
frequency won't be set. The download/upload speed is not affected.

- port 6 at trgmii mode won't work if the PLL frequency is not set. The 
SoC's MAC (gmac0) won't receive anything. It checks out since setting 
the PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" 
comment. So port 6 cannot properly transmit frames to the SoC's MAC.

- Port 6 at rgmii mode works without setting the PLL frequency. Speed is 
not affected.

I commented out core_write(priv, CORE_PLL_GROUP5, 
RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.

In conclusion, setting the PLL frequency is only needed for the trgmii 
mode, so I believe we can get rid of it on other cases.

One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and 
HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if 
xtal matching both of them is the expected behaviour.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fbf27d4ab5d9..12cea89ae0ac 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>  			/* PLL frequency: 150MHz: 1.2GBit */
>  			if (xtal == HWTRAP_XTAL_40MHZ)
>  				ncpo1 = 0x0780;
> +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
>  			if (xtal == HWTRAP_XTAL_25MHZ)
>  				ncpo1 = 0x0a00;
> +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
> +			if (xtal == HWTRAP_XTAL_20MHZ)
> +				dev_info(priv->dev, "XTAL is 20MHz too\n");
>  		} else { /* PLL frequency: 250MHz: 2.0Gbit */
>  			if (xtal == HWTRAP_XTAL_40MHZ)
>  				ncpo1 = 0x0c80;

> [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
> [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
> [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
> [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
> [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
> [    0.837007] DSA: tree 0 setup

Thunderbird limits lines to about 72 columns, so I'm pasting as 
quotation which seems to bypass that.

Arınç

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 6.03.2023 23:19, Vladimir Oltean wrote:
> On Mon, Mar 06, 2023 at 08:03:54PM +0300, Arınç ÜNAL wrote:
>> Looking at the Wikipedia page for Media-independent interface [0], the data
>> interface must be clocked at 125 MHz for gigabit MIIs, which I believe what
>> the "PLL" here refers to. trgmii needs higher frequency in some cases so if
>> both CPU ports are enabled, the table would be:
>>
>>      priv->p5_interface        priv->p6_interface       ncpo1 value
>>          gmii                     rgmii                     125MHz
>>          mii                      rgmii                     125MHz
>>          rgmii                    rgmii                     125MHz
>>          gmii                     trgmii                    125-250MHz
>>          mii                      trgmii                    125-250MHz
>>          rgmii                    trgmii                    125-250MHz
>>
>> [0] https://en.wikipedia.org/wiki/Media-independent_interface#GMII
> 
> Wikipedia will only tell you what the frequency of the interface signals
> needs to be. That is useful to keep in mind, but without information from
> the datasheet regarding what the SoC's clock distribution tree looks like,
> it's hard to know how that interface clock is derived from internal PLLs
> and ultimately from the oscillators. I was hoping that was the kind of
> information you could provide. The manuals I have access to, through charity,
> don't say anything on that front.

I wish I could. There are three people associated with MediaTek CC'd 
here. Maybe one will care to inform us.

> 
> Since I don't know what I'm commenting on, I'll stop commenting any further.
> 
>>> right now, you let the p6_interface logic overwrite the ncpo1 selected
>>> by the p5_interface logic like crazy, and it's not clear to me that this
>>> is what you want.
>>
>> This seems to be fine as p6 sets the frequency either the same or higher.
> 
> (...)

Sorry for the vague reply, my assumption was that interfaces with slower 
speeds, 100M, 1000M, etc. works fine at higher PLL frequency whilst they 
won't work the other way around.

> 
>> This looks much better, thanks a lot! The only missing part is setting the
>> PLL frequency when only port 5 is enabled.
> 
> True. Although with the limited information I have, I'm not sure that
> the ncpo1 value written into CORE_PLL_GROUP5 is needed by port5 either
> way. The fact that you claim port5 works when ncpo1 ranges from 125 to
> 250 MHz tells me that it's either very tolerant of the ncpo1 value
> (through mechanisms unknown to me), or simply unaffected by it (more
> likely ATM). Since I don't have any details regarding the value, I'd
> just like to treat the configuration procedure as plain code, and not
> make any changes until there's a proof that they're needed.
> 
>> I'll test it regardless.
> 
> Thanks.

Port 5 as CPU port works fine with this patch. I completely removed from 
port 6 phy modes.

With your patch on MT7621 (remember port 5 always worked on MT7623):

- Port 5 at rgmii as the only CPU port works, even though the PLL 
frequency won't be set. The download/upload speed is not affected.

- port 6 at trgmii mode won't work if the PLL frequency is not set. The 
SoC's MAC (gmac0) won't receive anything. It checks out since setting 
the PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" 
comment. So port 6 cannot properly transmit frames to the SoC's MAC.

- Port 6 at rgmii mode works without setting the PLL frequency. Speed is 
not affected.

I commented out core_write(priv, CORE_PLL_GROUP5, 
RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.

In conclusion, setting the PLL frequency is only needed for the trgmii 
mode, so I believe we can get rid of it on other cases.

One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and 
HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if 
xtal matching both of them is the expected behaviour.

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index fbf27d4ab5d9..12cea89ae0ac 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>  			/* PLL frequency: 150MHz: 1.2GBit */
>  			if (xtal == HWTRAP_XTAL_40MHZ)
>  				ncpo1 = 0x0780;
> +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
>  			if (xtal == HWTRAP_XTAL_25MHZ)
>  				ncpo1 = 0x0a00;
> +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
> +			if (xtal == HWTRAP_XTAL_20MHZ)
> +				dev_info(priv->dev, "XTAL is 20MHz too\n");
>  		} else { /* PLL frequency: 250MHz: 2.0Gbit */
>  			if (xtal == HWTRAP_XTAL_40MHZ)
>  				ncpo1 = 0x0c80;

> [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
> [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
> [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
> [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
> [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
> [    0.837007] DSA: tree 0 setup

Thunderbird limits lines to about 72 columns, so I'm pasting as 
quotation which seems to bypass that.

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
> Port 5 as CPU port works fine with this patch. I completely removed from
> port 6 phy modes.
> 
> With your patch on MT7621 (remember port 5 always worked on MT7623):
> 
> - Port 5 at rgmii as the only CPU port works, even though the PLL frequency
> won't be set. The download/upload speed is not affected.

That's good. Empirically it seems to prove that ncpo1 only affects p6,
which was my initial assumption.

> - port 6 at trgmii mode won't work if the PLL frequency is not set. The
> SoC's MAC (gmac0) won't receive anything. It checks out since setting the
> PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" comment.
> So port 6 cannot properly transmit frames to the SoC's MAC.
> 
> - Port 6 at rgmii mode works without setting the PLL frequency. Speed is not
> affected.
> 
> I commented out core_write(priv, CORE_PLL_GROUP5,
> RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.
> 
> In conclusion, setting the PLL frequency is only needed for the trgmii mode,
> so I believe we can get rid of it on other cases.

Got it, sounds expected, then? My patch can be submitted as-is, correct?

> One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and
> HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if
> xtal matching both of them is the expected behaviour.
> 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index fbf27d4ab5d9..12cea89ae0ac 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> >  			/* PLL frequency: 150MHz: 1.2GBit */
> >  			if (xtal == HWTRAP_XTAL_40MHZ)
> >  				ncpo1 = 0x0780;
> > +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");

In the C language, you need to put brackets { } around multi-statement
"if" blocks. Otherwise, despite the indentation, "dev_info" will be
executed unconditionally (unlike in Python for example). There should
also be a warning with newer gcc compilers about the misleading
indentation not leading to the expected code.

Like this:

			if (xtal == HWTRAP_XTAL_40MHZ) {
				ncpo1 = 0x0780;
				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
			}

> >  			if (xtal == HWTRAP_XTAL_25MHZ)
> >  				ncpo1 = 0x0a00;
> > +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
> > +			if (xtal == HWTRAP_XTAL_20MHZ)
> > +				dev_info(priv->dev, "XTAL is 20MHz too\n");
> >  		} else { /* PLL frequency: 250MHz: 2.0Gbit */
> >  			if (xtal == HWTRAP_XTAL_40MHZ)
> >  				ncpo1 = 0x0c80;
> 
> > [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> > [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
> > [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> > [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> > [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
> > [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
> > [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> > [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> > [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> > [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> > [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> > [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
> > [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
> > [    0.837007] DSA: tree 0 setup
> 
> Thunderbird limits lines to about 72 columns, so I'm pasting as quotation
> which seems to bypass that.

That seems to have worked, but shouldn't have been needed. I've uninstalled
Thunderbird in favor of mutt + vim for email editing.. although, isn't
there a Word Wrap option which you can just turn off?

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
> Port 5 as CPU port works fine with this patch. I completely removed from
> port 6 phy modes.
> 
> With your patch on MT7621 (remember port 5 always worked on MT7623):
> 
> - Port 5 at rgmii as the only CPU port works, even though the PLL frequency
> won't be set. The download/upload speed is not affected.

That's good. Empirically it seems to prove that ncpo1 only affects p6,
which was my initial assumption.

> - port 6 at trgmii mode won't work if the PLL frequency is not set. The
> SoC's MAC (gmac0) won't receive anything. It checks out since setting the
> PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" comment.
> So port 6 cannot properly transmit frames to the SoC's MAC.
> 
> - Port 6 at rgmii mode works without setting the PLL frequency. Speed is not
> affected.
> 
> I commented out core_write(priv, CORE_PLL_GROUP5,
> RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.
> 
> In conclusion, setting the PLL frequency is only needed for the trgmii mode,
> so I believe we can get rid of it on other cases.

Got it, sounds expected, then? My patch can be submitted as-is, correct?

> One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and
> HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if
> xtal matching both of them is the expected behaviour.
> 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index fbf27d4ab5d9..12cea89ae0ac 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> >  			/* PLL frequency: 150MHz: 1.2GBit */
> >  			if (xtal == HWTRAP_XTAL_40MHZ)
> >  				ncpo1 = 0x0780;
> > +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");

In the C language, you need to put brackets { } around multi-statement
"if" blocks. Otherwise, despite the indentation, "dev_info" will be
executed unconditionally (unlike in Python for example). There should
also be a warning with newer gcc compilers about the misleading
indentation not leading to the expected code.

Like this:

			if (xtal == HWTRAP_XTAL_40MHZ) {
				ncpo1 = 0x0780;
				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
			}

> >  			if (xtal == HWTRAP_XTAL_25MHZ)
> >  				ncpo1 = 0x0a00;
> > +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
> > +			if (xtal == HWTRAP_XTAL_20MHZ)
> > +				dev_info(priv->dev, "XTAL is 20MHz too\n");
> >  		} else { /* PLL frequency: 250MHz: 2.0Gbit */
> >  			if (xtal == HWTRAP_XTAL_40MHZ)
> >  				ncpo1 = 0x0c80;
> 
> > [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> > [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
> > [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> > [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> > [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
> > [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
> > [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> > [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> > [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> > [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> > [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> > [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
> > [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
> > [    0.837007] DSA: tree 0 setup
> 
> Thunderbird limits lines to about 72 columns, so I'm pasting as quotation
> which seems to bypass that.

That seems to have worked, but shouldn't have been needed. I've uninstalled
Thunderbird in favor of mutt + vim for email editing.. although, isn't
there a Word Wrap option which you can just turn off?

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Vladimir Oltean]

On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
> Port 5 as CPU port works fine with this patch. I completely removed from
> port 6 phy modes.
> 
> With your patch on MT7621 (remember port 5 always worked on MT7623):
> 
> - Port 5 at rgmii as the only CPU port works, even though the PLL frequency
> won't be set. The download/upload speed is not affected.

That's good. Empirically it seems to prove that ncpo1 only affects p6,
which was my initial assumption.

> - port 6 at trgmii mode won't work if the PLL frequency is not set. The
> SoC's MAC (gmac0) won't receive anything. It checks out since setting the
> PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" comment.
> So port 6 cannot properly transmit frames to the SoC's MAC.
> 
> - Port 6 at rgmii mode works without setting the PLL frequency. Speed is not
> affected.
> 
> I commented out core_write(priv, CORE_PLL_GROUP5,
> RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.
> 
> In conclusion, setting the PLL frequency is only needed for the trgmii mode,
> so I believe we can get rid of it on other cases.

Got it, sounds expected, then? My patch can be submitted as-is, correct?

> One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and
> HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if
> xtal matching both of them is the expected behaviour.
> 
> > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > index fbf27d4ab5d9..12cea89ae0ac 100644
> > --- a/drivers/net/dsa/mt7530.c
> > +++ b/drivers/net/dsa/mt7530.c
> > @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
> >  			/* PLL frequency: 150MHz: 1.2GBit */
> >  			if (xtal == HWTRAP_XTAL_40MHZ)
> >  				ncpo1 = 0x0780;
> > +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");

In the C language, you need to put brackets { } around multi-statement
"if" blocks. Otherwise, despite the indentation, "dev_info" will be
executed unconditionally (unlike in Python for example). There should
also be a warning with newer gcc compilers about the misleading
indentation not leading to the expected code.

Like this:

			if (xtal == HWTRAP_XTAL_40MHZ) {
				ncpo1 = 0x0780;
				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
			}

> >  			if (xtal == HWTRAP_XTAL_25MHZ)
> >  				ncpo1 = 0x0a00;
> > +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
> > +			if (xtal == HWTRAP_XTAL_20MHZ)
> > +				dev_info(priv->dev, "XTAL is 20MHz too\n");
> >  		} else { /* PLL frequency: 250MHz: 2.0Gbit */
> >  			if (xtal == HWTRAP_XTAL_40MHZ)
> >  				ncpo1 = 0x0c80;
> 
> > [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
> > [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
> > [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> > [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
> > [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
> > [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
> > [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
> > [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> > [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> > [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> > [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> > [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
> > [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
> > [    0.837007] DSA: tree 0 setup
> 
> Thunderbird limits lines to about 72 columns, so I'm pasting as quotation
> which seems to bypass that.

That seems to have worked, but shouldn't have been needed. I've uninstalled
Thunderbird in favor of mutt + vim for email editing.. although, isn't
there a Word Wrap option which you can just turn off?

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 7.03.2023 14:37, Vladimir Oltean wrote:
> On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
>> Port 5 as CPU port works fine with this patch. I completely removed from
>> port 6 phy modes.
>>
>> With your patch on MT7621 (remember port 5 always worked on MT7623):
>>
>> - Port 5 at rgmii as the only CPU port works, even though the PLL frequency
>> won't be set. The download/upload speed is not affected.
> 
> That's good. Empirically it seems to prove that ncpo1 only affects p6,
> which was my initial assumption.
> 
>> - port 6 at trgmii mode won't work if the PLL frequency is not set. The
>> SoC's MAC (gmac0) won't receive anything. It checks out since setting the
>> PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" comment.
>> So port 6 cannot properly transmit frames to the SoC's MAC.
>>
>> - Port 6 at rgmii mode works without setting the PLL frequency. Speed is not
>> affected.
>>
>> I commented out core_write(priv, CORE_PLL_GROUP5,
>> RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.
>>
>> In conclusion, setting the PLL frequency is only needed for the trgmii mode,
>> so I believe we can get rid of it on other cases.
> 
> Got it, sounds expected, then? My patch can be submitted as-is, correct?

Yup, your patch is a go! I can submit other patches to address the 
incorrect comment regarding port 5, and remove ncpo1 from rgmii mode for 
port 6.

> 
>> One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and
>> HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if
>> xtal matching both of them is the expected behaviour.
>>
>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>> index fbf27d4ab5d9..12cea89ae0ac 100644
>>> --- a/drivers/net/dsa/mt7530.c
>>> +++ b/drivers/net/dsa/mt7530.c
>>> @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>>>   			/* PLL frequency: 150MHz: 1.2GBit */
>>>   			if (xtal == HWTRAP_XTAL_40MHZ)
>>>   				ncpo1 = 0x0780;
>>> +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
> 
> In the C language, you need to put brackets { } around multi-statement
> "if" blocks. Otherwise, despite the indentation, "dev_info" will be
> executed unconditionally (unlike in Python for example). There should
> also be a warning with newer gcc compilers about the misleading
> indentation not leading to the expected code.
> 
> Like this:
> 
> 			if (xtal == HWTRAP_XTAL_40MHZ) {
> 				ncpo1 = 0x0780;
> 				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
> 			}
> 

Oh that figures, thanks a bunch. Clearly I've got a lot to learn. Now I 
see only 40MHz.

[    0.763772] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780

>>>   			if (xtal == HWTRAP_XTAL_25MHZ)
>>>   				ncpo1 = 0x0a00;
>>> +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
>>> +			if (xtal == HWTRAP_XTAL_20MHZ)
>>> +				dev_info(priv->dev, "XTAL is 20MHz too\n");
>>>   		} else { /* PLL frequency: 250MHz: 2.0Gbit */
>>>   			if (xtal == HWTRAP_XTAL_40MHZ)
>>>   				ncpo1 = 0x0c80;
>>
>>> [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
>>> [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
>>> [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
>>> [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
>>> [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
>>> [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
>>> [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
>>> [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
>>> [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
>>> [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
>>> [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
>>> [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
>>> [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
>>> [    0.837007] DSA: tree 0 setup
>>
>> Thunderbird limits lines to about 72 columns, so I'm pasting as quotation
>> which seems to bypass that.
> 
> That seems to have worked, but shouldn't have been needed. I've uninstalled
> Thunderbird in favor of mutt + vim for email editing.. although, isn't
> there a Word Wrap option which you can just turn off?

It turns out there is indeed:

https://support.mozilla.org/en-US/questions/1307935

Mutt seems to be too techy for my taste but one can't know until they 
try it, thanks for the info.

Arınç

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 7.03.2023 14:37, Vladimir Oltean wrote:
> On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
>> Port 5 as CPU port works fine with this patch. I completely removed from
>> port 6 phy modes.
>>
>> With your patch on MT7621 (remember port 5 always worked on MT7623):
>>
>> - Port 5 at rgmii as the only CPU port works, even though the PLL frequency
>> won't be set. The download/upload speed is not affected.
> 
> That's good. Empirically it seems to prove that ncpo1 only affects p6,
> which was my initial assumption.
> 
>> - port 6 at trgmii mode won't work if the PLL frequency is not set. The
>> SoC's MAC (gmac0) won't receive anything. It checks out since setting the
>> PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" comment.
>> So port 6 cannot properly transmit frames to the SoC's MAC.
>>
>> - Port 6 at rgmii mode works without setting the PLL frequency. Speed is not
>> affected.
>>
>> I commented out core_write(priv, CORE_PLL_GROUP5,
>> RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.
>>
>> In conclusion, setting the PLL frequency is only needed for the trgmii mode,
>> so I believe we can get rid of it on other cases.
> 
> Got it, sounds expected, then? My patch can be submitted as-is, correct?

Yup, your patch is a go! I can submit other patches to address the 
incorrect comment regarding port 5, and remove ncpo1 from rgmii mode for 
port 6.

> 
>> One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and
>> HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if
>> xtal matching both of them is the expected behaviour.
>>
>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>> index fbf27d4ab5d9..12cea89ae0ac 100644
>>> --- a/drivers/net/dsa/mt7530.c
>>> +++ b/drivers/net/dsa/mt7530.c
>>> @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>>>   			/* PLL frequency: 150MHz: 1.2GBit */
>>>   			if (xtal == HWTRAP_XTAL_40MHZ)
>>>   				ncpo1 = 0x0780;
>>> +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
> 
> In the C language, you need to put brackets { } around multi-statement
> "if" blocks. Otherwise, despite the indentation, "dev_info" will be
> executed unconditionally (unlike in Python for example). There should
> also be a warning with newer gcc compilers about the misleading
> indentation not leading to the expected code.
> 
> Like this:
> 
> 			if (xtal == HWTRAP_XTAL_40MHZ) {
> 				ncpo1 = 0x0780;
> 				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
> 			}
> 

Oh that figures, thanks a bunch. Clearly I've got a lot to learn. Now I 
see only 40MHz.

[    0.763772] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780

>>>   			if (xtal == HWTRAP_XTAL_25MHZ)
>>>   				ncpo1 = 0x0a00;
>>> +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
>>> +			if (xtal == HWTRAP_XTAL_20MHZ)
>>> +				dev_info(priv->dev, "XTAL is 20MHz too\n");
>>>   		} else { /* PLL frequency: 250MHz: 2.0Gbit */
>>>   			if (xtal == HWTRAP_XTAL_40MHZ)
>>>   				ncpo1 = 0x0c80;
>>
>>> [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
>>> [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
>>> [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
>>> [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
>>> [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
>>> [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
>>> [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
>>> [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
>>> [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
>>> [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
>>> [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
>>> [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
>>> [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
>>> [    0.837007] DSA: tree 0 setup
>>
>> Thunderbird limits lines to about 72 columns, so I'm pasting as quotation
>> which seems to bypass that.
> 
> That seems to have worked, but shouldn't have been needed. I've uninstalled
> Thunderbird in favor of mutt + vim for email editing.. although, isn't
> there a Word Wrap option which you can just turn off?

It turns out there is indeed:

https://support.mozilla.org/en-US/questions/1307935

Mutt seems to be too techy for my taste but one can't know until they 
try it, thanks for the info.

Arınç

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

Re: [RFC PATCH net] net: dsa: mt7530: move PLL setup out of port 6 pad configuration

[Arınç ÜNAL]

On 7.03.2023 14:37, Vladimir Oltean wrote:
> On Tue, Mar 07, 2023 at 02:26:08PM +0300, Arınç ÜNAL wrote:
>> Port 5 as CPU port works fine with this patch. I completely removed from
>> port 6 phy modes.
>>
>> With your patch on MT7621 (remember port 5 always worked on MT7623):
>>
>> - Port 5 at rgmii as the only CPU port works, even though the PLL frequency
>> won't be set. The download/upload speed is not affected.
> 
> That's good. Empirically it seems to prove that ncpo1 only affects p6,
> which was my initial assumption.
> 
>> - port 6 at trgmii mode won't work if the PLL frequency is not set. The
>> SoC's MAC (gmac0) won't receive anything. It checks out since setting the
>> PLL frequency is put under the "Setup the MT7530 TRGMII Tx Clock" comment.
>> So port 6 cannot properly transmit frames to the SoC's MAC.
>>
>> - Port 6 at rgmii mode works without setting the PLL frequency. Speed is not
>> affected.
>>
>> I commented out core_write(priv, CORE_PLL_GROUP5,
>> RG_LCDDS_PCW_NCPO1(ncpo1)); to stop setting the PLL frequency.
>>
>> In conclusion, setting the PLL frequency is only needed for the trgmii mode,
>> so I believe we can get rid of it on other cases.
> 
> Got it, sounds expected, then? My patch can be submitted as-is, correct?

Yup, your patch is a go! I can submit other patches to address the 
incorrect comment regarding port 5, and remove ncpo1 from rgmii mode for 
port 6.

> 
>> One more thing, on MT7621, xtal matches to both HWTRAP_XTAL_40MHZ and
>> HWTRAP_XTAL_25MHZ so the final value of ncpo1 is 0x0a00. I'm not sure if
>> xtal matching both of them is the expected behaviour.
>>
>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>> index fbf27d4ab5d9..12cea89ae0ac 100644
>>> --- a/drivers/net/dsa/mt7530.c
>>> +++ b/drivers/net/dsa/mt7530.c
>>> @@ -439,8 +439,12 @@ mt7530_pad_clk_setup(struct dsa_switch *ds, phy_interface_t interface)
>>>   			/* PLL frequency: 150MHz: 1.2GBit */
>>>   			if (xtal == HWTRAP_XTAL_40MHZ)
>>>   				ncpo1 = 0x0780;
>>> +				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
> 
> In the C language, you need to put brackets { } around multi-statement
> "if" blocks. Otherwise, despite the indentation, "dev_info" will be
> executed unconditionally (unlike in Python for example). There should
> also be a warning with newer gcc compilers about the misleading
> indentation not leading to the expected code.
> 
> Like this:
> 
> 			if (xtal == HWTRAP_XTAL_40MHZ) {
> 				ncpo1 = 0x0780;
> 				dev_info(priv->dev, "XTAL is 40MHz, ncpo1 is 0x0780\n");
> 			}
> 

Oh that figures, thanks a bunch. Clearly I've got a lot to learn. Now I 
see only 40MHz.

[    0.763772] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780

>>>   			if (xtal == HWTRAP_XTAL_25MHZ)
>>>   				ncpo1 = 0x0a00;
>>> +				dev_info(priv->dev, "XTAL is 25MHz, ncpo1 is 0x0a00\n");
>>> +			if (xtal == HWTRAP_XTAL_20MHZ)
>>> +				dev_info(priv->dev, "XTAL is 20MHz too\n");
>>>   		} else { /* PLL frequency: 250MHz: 2.0Gbit */
>>>   			if (xtal == HWTRAP_XTAL_40MHZ)
>>>   				ncpo1 = 0x0c80;
>>
>>> [    0.710455] mt7530 mdio-bus:1f: MT7530 adapts as multi-chip module
>>> [    0.734419] mt7530 mdio-bus:1f: configuring for fixed/rgmii link mode
>>> [    0.741766] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
>>> [    0.743647] mt7530 mdio-bus:1f: configuring for fixed/trgmii link mode
>>> [    0.755422] mt7530 mdio-bus:1f: XTAL is 40MHz, ncpo1 is 0x0780
>>> [    0.761250] mt7530 mdio-bus:1f: XTAL is 25MHz, ncpo1 is 0x0a00
>>> [    0.769414] mt7530 mdio-bus:1f: Link is Up - 1Gbps/Full - flow control rx/tx
>>> [    0.772067] mt7530 mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
>>> [    0.788647] mt7530 mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
>>> [    0.800354] mt7530 mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
>>> [    0.812031] mt7530 mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
>>> [    0.823418] mtk_soc_eth 1e100000.ethernet eth1: entered promiscuous mode
>>> [    0.830250] mtk_soc_eth 1e100000.ethernet eth0: entered promiscuous mode
>>> [    0.837007] DSA: tree 0 setup
>>
>> Thunderbird limits lines to about 72 columns, so I'm pasting as quotation
>> which seems to bypass that.
> 
> That seems to have worked, but shouldn't have been needed. I've uninstalled
> Thunderbird in favor of mutt + vim for email editing.. although, isn't
> there a Word Wrap option which you can just turn off?

It turns out there is indeed:

https://support.mozilla.org/en-US/questions/1307935

Mutt seems to be too techy for my taste but one can't know until they 
try it, thanks for the info.

Arınç