Re: MT7530 bug, forward broadcast and unknown frames to the correct CPU port

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

On 29.04.2023 20:35, Vladimir Oltean wrote:
> On Sat, Apr 29, 2023 at 04:03:57PM +0300, Arınç ÜNAL wrote:
>> This is the final diff I'm going to submit to net.
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 4d5c5820e461..cc5fa641b026 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -1008,9 +1008,9 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
>>   	mt7530_write(priv, MT7530_PVC_P(port),
>>   		     PORT_SPEC_TAG);
>> -	/* Disable flooding by default */
>> -	mt7530_rmw(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK,
>> -		   BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) | UNU_FFP(BIT(port)));
>> +	/* Enable flooding on the CPU port */
>> +	mt7530_set(priv, MT7530_MFC, BC_FFP(BIT(port)) | UNM_FFP(BIT(port)) |
>> +		   UNU_FFP(BIT(port)));
>>   	/* Set CPU port number */
>>   	if (priv->id == ID_MT7621)
>> @@ -2225,6 +2225,10 @@ mt7530_setup(struct dsa_switch *ds)
>>   		/* Disable learning by default on all ports */
>>   		mt7530_set(priv, MT7530_PSC_P(i), SA_DIS);
>> +		/* Disable flooding on all ports */
>> +		mt7530_clear(priv, MT7530_MFC, BC_FFP(BIT(i)) | UNM_FFP(BIT(i)) |
>> +			     UNU_FFP(BIT(i)));
>> +
>>   		if (dsa_is_cpu_port(ds, i)) {
>>   			ret = mt753x_cpu_port_enable(ds, i);
>>   			if (ret)
>> @@ -2412,6 +2416,10 @@ mt7531_setup(struct dsa_switch *ds)
>>   		mt7530_set(priv, MT7531_DBG_CNT(i), MT7531_DIS_CLR);
>> +		/* Disable flooding on all ports */
>> +		mt7530_clear(priv, MT7530_MFC, BC_FFP(BIT(i)) | UNM_FFP(BIT(i)) |
>> +			     UNU_FFP(BIT(i)));
>> +
>>   		if (dsa_is_cpu_port(ds, i)) {
>>   			ret = mt753x_cpu_port_enable(ds, i);
>>   			if (ret)
> 
> Looks ok, but considering that the register is the same for all ports,
> then instead of accessing the hardware one by one for each port, you
> could issue a single:
> 
> 	mt7530_clear(priv, MT7530_MFC, BC_FFP_MASK | UNM_FFP_MASK | UNU_FFP_MASK);
> 
> before the per-port for loop.

Will do, thanks.

The preferred port operation should be in the clear after this diff:

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index cb0f138d39eb..3a69ef68ceae 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -967,6 +967,10 @@ mt753x_cpu_port_enable(struct dsa_switch *ds, int port)
  	if (priv->id == ID_MT7621)
  		mt7530_rmw(priv, MT7530_MFC, CPU_MASK, CPU_EN | CPU_PORT(port));
  
+	/* Set the CPU port for MT7531 and switch on MT7988 SoC */
+	if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
+		mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK, BIT(port));
+
  	/* CPU port gets connected to all user ports of
  	 * the switch.
  	 */
@@ -2321,15 +2325,6 @@ mt7531_setup_common(struct dsa_switch *ds)
  	struct dsa_port *cpu_dp;
  	int ret, i;
  
-	/* BPDU to CPU port */
-	dsa_switch_for_each_cpu_port(cpu_dp, ds) {
-		mt7530_rmw(priv, MT7531_CFC, MT7531_CPU_PMAP_MASK,
-			   BIT(cpu_dp->index));
-		break;
-	}
-	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
-		   MT753X_BPDU_CPU_ONLY);
-
  	/* Enable and reset MIB counters */
  	mt7530_mib_reset(ds);
  
@@ -2360,6 +2355,10 @@ mt7531_setup_common(struct dsa_switch *ds)
  			   PVC_EG_TAG(MT7530_VLAN_EG_CONSISTENT));
  	}
  
+	/* Trap BPDUs to the CPU port */
+	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
+		   MT753X_BPDU_CPU_ONLY);
+
  	/* Flush the FDB table */
  	ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
  	if (ret < 0)

The MT7531 manual states that the CPU_PMAP bits are unset after reset so
no need to clear it beforehand.

Are you fine with the preferred port patch now that I mentioned port 6
would be preferred for MT7531BE since it's got 2.5G whilst port 5 has
got 1G? Would you like to submit it or leave it to me to send the diff
above and this?

Arınç