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

[Daniel Golle <daniel@makrotopia.org>]

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