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

Arınç ÜNAL
2023-04-29T18:40:27Z

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ç