[PATCH net-next v2 07/15] net: dsa: mt7530: move MT753X_MTRAP operations for MT7530

Arınç ÜNAL via B4 Relay posted 15 patches 1 year, 9 months ago
[PATCH net-next v2 07/15] net: dsa: mt7530: move MT753X_MTRAP operations for MT7530
Posted by Arınç ÜNAL via B4 Relay 1 year, 9 months ago
From: Arınç ÜNAL <arinc.unal@arinc9.com>

On MT7530, the media-independent interfaces of port 5 and 6 are controlled
by the MT7530_P5_DIS and MT7530_P6_DIS bits of the hardware trap. Deal with
these bits only when the relevant port is being enabled or disabled. This
ensures that these ports will be disabled when they are not in use.

Do not set MT7530_CHG_TRAP on mt7530_setup_port5() as that's already being
done on mt7530_setup().

Instead of globally setting MT7530_P5_MAC_SEL, clear it, then set it only
on the appropriate case.

If PHY muxing is detected, clear MT7530_P5_DIS before calling
mt7530_setup_port5().

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

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 606516206fb9..83436723cb16 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -880,8 +880,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 
 	val = mt7530_read(priv, MT753X_MTRAP);
 
-	val |= MT7530_CHG_TRAP | MT7530_P5_MAC_SEL | MT7530_P5_DIS;
-	val &= ~MT7530_P5_RGMII_MODE & ~MT7530_P5_PHY0_SEL;
+	val &= ~MT7530_P5_PHY0_SEL & ~MT7530_P5_MAC_SEL & ~MT7530_P5_RGMII_MODE;
 
 	switch (priv->p5_mode) {
 	/* MUX_PHY_P0: P0 -> P5 -> SoC MAC */
@@ -891,15 +890,13 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 
 	/* MUX_PHY_P4: P4 -> P5 -> SoC MAC */
 	case MUX_PHY_P4:
-		val &= ~MT7530_P5_MAC_SEL & ~MT7530_P5_DIS;
-
 		/* Setup the MAC by default for the cpu port */
 		mt7530_write(priv, MT753X_PMCR_P(5), 0x56300);
 		break;
 
 	/* GMAC5: P5 -> SoC MAC or external PHY */
 	default:
-		val &= ~MT7530_P5_DIS;
+		val |= MT7530_P5_MAC_SEL;
 		break;
 	}
 
@@ -1193,6 +1190,14 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
 
 	mutex_unlock(&priv->reg_mutex);
 
+	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
+		return 0;
+
+	if (port == 5)
+		mt7530_clear(priv, MT753X_MTRAP, MT7530_P5_DIS);
+	else if (port == 6)
+		mt7530_clear(priv, MT753X_MTRAP, MT7530_P6_DIS);
+
 	return 0;
 }
 
@@ -1211,6 +1216,14 @@ mt7530_port_disable(struct dsa_switch *ds, int port)
 		   PCR_MATRIX_CLR);
 
 	mutex_unlock(&priv->reg_mutex);
+
+	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
+		return;
+
+	if (port == 5)
+		mt7530_set(priv, MT753X_MTRAP, MT7530_P5_DIS);
+	else if (port == 6)
+		mt7530_set(priv, MT753X_MTRAP, MT7530_P6_DIS);
 }
 
 static int
@@ -2401,11 +2414,11 @@ mt7530_setup(struct dsa_switch *ds)
 		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
 			   RD_TAP_MASK, RD_TAP(16));
 
-	/* Enable port 6 */
-	val = mt7530_read(priv, MT753X_MTRAP);
-	val &= ~MT7530_P6_DIS & ~MT7530_PHY_INDIRECT_ACCESS;
-	val |= MT7530_CHG_TRAP;
-	mt7530_write(priv, MT753X_MTRAP, val);
+	/* Allow modifying the trap and directly access PHY registers via the
+	 * MDIO bus the switch is on.
+	 */
+	mt7530_rmw(priv, MT753X_MTRAP, MT7530_CHG_TRAP |
+		   MT7530_PHY_INDIRECT_ACCESS, MT7530_CHG_TRAP);
 
 	if ((val & MT7530_XTAL_MASK) == MT7530_XTAL_40MHZ)
 		mt7530_pll_setup(priv);
@@ -2488,8 +2501,11 @@ mt7530_setup(struct dsa_switch *ds)
 			break;
 		}
 
-		if (priv->p5_mode == MUX_PHY_P0 || priv->p5_mode == MUX_PHY_P4)
+		if (priv->p5_mode == MUX_PHY_P0 ||
+		    priv->p5_mode == MUX_PHY_P4) {
+			mt7530_clear(priv, MT753X_MTRAP, MT7530_P5_DIS);
 			mt7530_setup_port5(ds, interface);
+		}
 	}
 
 #ifdef CONFIG_GPIOLIB

-- 
2.40.1


Re: [PATCH net-next v2 07/15] net: dsa: mt7530: move MT753X_MTRAP operations for MT7530
Posted by Daniel Golle 1 year, 9 months ago
Hi Arınç,

On Mon, Apr 22, 2024 at 10:15:14AM +0300, Arınç ÜNAL via B4 Relay wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> On MT7530, the media-independent interfaces of port 5 and 6 are controlled
> by the MT7530_P5_DIS and MT7530_P6_DIS bits of the hardware trap. Deal with
> these bits only when the relevant port is being enabled or disabled. This
> ensures that these ports will be disabled when they are not in use.
> 
> Do not set MT7530_CHG_TRAP on mt7530_setup_port5() as that's already being
> done on mt7530_setup().

Multiple users reported ([1], [2]) that after I've imported the series
to OpenWrt they noticed that WAN connection on MT7621 boards using
PHY-muxing to hook up either port 0 or port 4 to GMAC1 no longer works.

The link still seems to come up, but no data flows. I went ahead and
confirmed the bug, then started bisecting the patches of this series,
and ended up identifying this very patch being the culprit.

I can't exclude that what ever the issue may be is caused by other
downstream patches we have, but can confirm that removing this patch of
your series [3] in OpenWrt fixes the issue. Please take a look and as
the cover letter states you have tested this on some MT7621 board,
please make sure traffic actually flows on the PHY-muxed port on that
board after this patch is applied, and if not, please figure out why and
repost a fixed version of this patch.


Cheers


Daniel

[1]: https://github.com/openwrt/openwrt/issues/15273
[2]: https://github.com/openwrt/openwrt/issues/15279
[3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=a8dde7e5bd6d289db6485cf57d3512ea62eaa827

> 
> Instead of globally setting MT7530_P5_MAC_SEL, clear it, then set it only
> on the appropriate case.
> 
> If PHY muxing is detected, clear MT7530_P5_DIS before calling
> mt7530_setup_port5().
> 
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
>  drivers/net/dsa/mt7530.c | 38 +++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 606516206fb9..83436723cb16 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -880,8 +880,7 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>  
>  	val = mt7530_read(priv, MT753X_MTRAP);
>  
> -	val |= MT7530_CHG_TRAP | MT7530_P5_MAC_SEL | MT7530_P5_DIS;
> -	val &= ~MT7530_P5_RGMII_MODE & ~MT7530_P5_PHY0_SEL;
> +	val &= ~MT7530_P5_PHY0_SEL & ~MT7530_P5_MAC_SEL & ~MT7530_P5_RGMII_MODE;
>  
>  	switch (priv->p5_mode) {
>  	/* MUX_PHY_P0: P0 -> P5 -> SoC MAC */
> @@ -891,15 +890,13 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
>  
>  	/* MUX_PHY_P4: P4 -> P5 -> SoC MAC */
>  	case MUX_PHY_P4:
> -		val &= ~MT7530_P5_MAC_SEL & ~MT7530_P5_DIS;
> -
>  		/* Setup the MAC by default for the cpu port */
>  		mt7530_write(priv, MT753X_PMCR_P(5), 0x56300);
>  		break;
>  
>  	/* GMAC5: P5 -> SoC MAC or external PHY */
>  	default:
> -		val &= ~MT7530_P5_DIS;
> +		val |= MT7530_P5_MAC_SEL;
>  		break;
>  	}
>  
> @@ -1193,6 +1190,14 @@ mt7530_port_enable(struct dsa_switch *ds, int port,
>  
>  	mutex_unlock(&priv->reg_mutex);
>  
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> +		return 0;
> +
> +	if (port == 5)
> +		mt7530_clear(priv, MT753X_MTRAP, MT7530_P5_DIS);
> +	else if (port == 6)
> +		mt7530_clear(priv, MT753X_MTRAP, MT7530_P6_DIS);
> +
>  	return 0;
>  }
>  
> @@ -1211,6 +1216,14 @@ mt7530_port_disable(struct dsa_switch *ds, int port)
>  		   PCR_MATRIX_CLR);
>  
>  	mutex_unlock(&priv->reg_mutex);
> +
> +	if (priv->id != ID_MT7530 && priv->id != ID_MT7621)
> +		return;
> +
> +	if (port == 5)
> +		mt7530_set(priv, MT753X_MTRAP, MT7530_P5_DIS);
> +	else if (port == 6)
> +		mt7530_set(priv, MT753X_MTRAP, MT7530_P6_DIS);
>  }
>  
>  static int
> @@ -2401,11 +2414,11 @@ mt7530_setup(struct dsa_switch *ds)
>  		mt7530_rmw(priv, MT7530_TRGMII_RD(i),
>  			   RD_TAP_MASK, RD_TAP(16));
>  
> -	/* Enable port 6 */
> -	val = mt7530_read(priv, MT753X_MTRAP);
> -	val &= ~MT7530_P6_DIS & ~MT7530_PHY_INDIRECT_ACCESS;
> -	val |= MT7530_CHG_TRAP;
> -	mt7530_write(priv, MT753X_MTRAP, val);
> +	/* Allow modifying the trap and directly access PHY registers via the
> +	 * MDIO bus the switch is on.
> +	 */
> +	mt7530_rmw(priv, MT753X_MTRAP, MT7530_CHG_TRAP |
> +		   MT7530_PHY_INDIRECT_ACCESS, MT7530_CHG_TRAP);
>  
>  	if ((val & MT7530_XTAL_MASK) == MT7530_XTAL_40MHZ)
>  		mt7530_pll_setup(priv);
> @@ -2488,8 +2501,11 @@ mt7530_setup(struct dsa_switch *ds)
>  			break;
>  		}
>  
> -		if (priv->p5_mode == MUX_PHY_P0 || priv->p5_mode == MUX_PHY_P4)
> +		if (priv->p5_mode == MUX_PHY_P0 ||
> +		    priv->p5_mode == MUX_PHY_P4) {
> +			mt7530_clear(priv, MT753X_MTRAP, MT7530_P5_DIS);
>  			mt7530_setup_port5(ds, interface);
> +		}
>  	}
>  
>  #ifdef CONFIG_GPIOLIB
> 
> -- 
> 2.40.1
> 
> 
Re: [PATCH net-next v2 07/15] net: dsa: mt7530: move MT753X_MTRAP operations for MT7530
Posted by Arınç ÜNAL 1 year, 9 months ago
On 27.04.2024 05:24, Daniel Golle wrote:
> Hi Arınç,
> 
> On Mon, Apr 22, 2024 at 10:15:14AM +0300, Arınç ÜNAL via B4 Relay wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> On MT7530, the media-independent interfaces of port 5 and 6 are controlled
>> by the MT7530_P5_DIS and MT7530_P6_DIS bits of the hardware trap. Deal with
>> these bits only when the relevant port is being enabled or disabled. This
>> ensures that these ports will be disabled when they are not in use.
>>
>> Do not set MT7530_CHG_TRAP on mt7530_setup_port5() as that's already being
>> done on mt7530_setup().
> 
> Multiple users reported ([1], [2]) that after I've imported the series
> to OpenWrt they noticed that WAN connection on MT7621 boards using
> PHY-muxing to hook up either port 0 or port 4 to GMAC1 no longer works.
> 
> The link still seems to come up, but no data flows. I went ahead and
> confirmed the bug, then started bisecting the patches of this series,
> and ended up identifying this very patch being the culprit.
> 
> I can't exclude that what ever the issue may be is caused by other
> downstream patches we have, but can confirm that removing this patch of
> your series [3] in OpenWrt fixes the issue. Please take a look and as
> the cover letter states you have tested this on some MT7621 board,
> please make sure traffic actually flows on the PHY-muxed port on that
> board after this patch is applied, and if not, please figure out why and
> repost a fixed version of this patch.
> 
> 
> Cheers
> 
> 
> Daniel
> 
> [1]: https://github.com/openwrt/openwrt/issues/15273
> [2]: https://github.com/openwrt/openwrt/issues/15279
> [3]: https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=a8dde7e5bd6d289db6485cf57d3512ea62eaa827

Thanks for reporting this Daniel. I am not happy that I've caused all this
fuss. My testing as described on the cover letter did not include the
hardware design with PHY muxing. Lesson learned; next time, I'll make sure
to test the specific hardware design when I work on the part of the code
that would affect that hardware design.

That said, I've submitted a patch that fixes this issue [1]. I have tested
the hardware design with PHY muxing with this fix applied and I don't
experience this issue anymore.

[1] https://lore.kernel.org/netdev/20240427-for-netnext-mt7530-do-not-disable-port5-when-phy-muxing-v1-1-793cdf9d7707@arinc9.com/

Arınç