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

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

On 29.04.2023 21:39, Arınç ÜNAL wrote:
> 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));

Should be 'mt7530_set(priv, MT7531_CFC, 
MT7531_CPU_PMAP_MASK(BIT(port)));' instead.

> +
>       /* 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ç