From: Arınç ÜNAL <arinc.unal@arinc9.com>

Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
SoC represents a CPU port to trap frames to. These switches trap frames
received from a user port to the CPU port that is affine to the user port
from which the frames are received.

Currently, only the bit that corresponds to the first found CPU port is set
on the bitmap. When multiple CPU ports are being used, the trapped frames
from the user ports not affine to the first CPU port will be dropped as the
other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
not affected as there's only one port to be used as a CPU port.

To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
for each CPU port.

Add a comment to explain frame trapping for these switches.

According to the document MT7531 Reference Manual for Development Board
v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
beforehand. Since there's currently no public document for the switch on
the MT7988 SoC, I assume this is also the case for this switch.

Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
---
 drivers/net/dsa/mt7530.c | 16 +++++++++-------
 drivers/net/dsa/mt7530.h |  1 +
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9bc54e1348cb..b1657679e69d 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -1010,6 +1010,14 @@ 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));
 
+	/* Add the CPU port to the CPU port bitmap for MT7531 and the switch on
+	 * the MT7988 SoC. Frames received from a user port which are set for
+	 * trapping to CPU port will be trapped to the CPU port that is affine
+	 * to the user port from which the frames are received.
+	 */
+	if (priv->id == ID_MT7531 || priv->id == ID_MT7988)
+		mt7530_set(priv, MT7531_CFC, MT7531_CPU_PMAP(BIT(port)));
+
 	/* CPU port gets connected to all user ports of
 	 * the switch.
 	 */
@@ -2352,15 +2360,9 @@ static int
 mt7531_setup_common(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
-	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;
-	}
+	/* Trap BPDUs to the CPU port(s) */
 	mt7530_rmw(priv, MT753X_BPC, MT753X_BPDU_PORT_FW_MASK,
 		   MT753X_BPDU_CPU_ONLY);
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 5084f48a8869..e590cf43f3ae 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -54,6 +54,7 @@ enum mt753x_id {
 #define  MT7531_MIRROR_PORT_GET(x)	(((x) >> 16) & MIRROR_MASK)
 #define  MT7531_MIRROR_PORT_SET(x)	(((x) & MIRROR_MASK) << 16)
 #define  MT7531_CPU_PMAP_MASK		GENMASK(7, 0)
+#define  MT7531_CPU_PMAP(x)		FIELD_PREP(MT7531_CPU_PMAP_MASK, x)
 
 #define MT753X_MIRROR_REG(id)		((((id) == ID_MT7531) || ((id) == ID_MT7988)) ?	\
 					 MT7531_CFC : MT7530_MFC)
-- 
2.39.2

On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
> From: Arınç ÜNAL <arinc.unal@arinc9.com>
> 
> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
> SoC represents a CPU port to trap frames to. These switches trap frames
> received from a user port to the CPU port that is affine to the user port
> from which the frames are received.
> 
> Currently, only the bit that corresponds to the first found CPU port is set
> on the bitmap. When multiple CPU ports are being used, the trapped frames
> from the user ports not affine to the first CPU port will be dropped as the
> other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
> not affected as there's only one port to be used as a CPU port.
> 
> To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
> bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
> switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
> for each CPU port.
> 
> Add a comment to explain frame trapping for these switches.
> 
> According to the document MT7531 Reference Manual for Development Board
> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
> beforehand. Since there's currently no public document for the switch on
> the MT7988 SoC, I assume this is also the case for this switch.
> 
> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---

Would you agree that this is just preparatory work for change "net: dsa:
introduce preferred_default_local_cpu_port and use on MT7530" and not a
fix to an existing problem in the code base?

On 14.06.2023 22:43, Vladimir Oltean wrote:
> On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
>> From: Arınç ÜNAL <arinc.unal@arinc9.com>
>>
>> Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>> SoC represents a CPU port to trap frames to. These switches trap frames
>> received from a user port to the CPU port that is affine to the user port
>> from which the frames are received.
>>
>> Currently, only the bit that corresponds to the first found CPU port is set
>> on the bitmap. When multiple CPU ports are being used, the trapped frames
>> from the user ports not affine to the first CPU port will be dropped as the
>> other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
>> not affected as there's only one port to be used as a CPU port.
>>
>> To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
>> bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
>> switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
>> for each CPU port.
>>
>> Add a comment to explain frame trapping for these switches.
>>
>> According to the document MT7531 Reference Manual for Development Board
>> v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
>> beforehand. Since there's currently no public document for the switch on
>> the MT7988 SoC, I assume this is also the case for this switch.
>>
>> Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
>> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> ---
> 
> Would you agree that this is just preparatory work for change "net: dsa:
> introduce preferred_default_local_cpu_port and use on MT7530" and not a
> fix to an existing problem in the code base?

Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a 
case where there's a user port affine to a CPU port that is not enabled 
on the CPU port bitmap. So yeah, this is just preparatory work for "net: 
dsa: introduce preferred_default_local_cpu_port and use on MT7530".

So how do I change the patch to reflect this?

Arınç

On Wed, Jun 14, 2023 at 11:56:44PM +0300, Arınç ÜNAL wrote:
> On 14.06.2023 22:43, Vladimir Oltean wrote:
> > On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > 
> > > Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
> > > SoC represents a CPU port to trap frames to. These switches trap frames
> > > received from a user port to the CPU port that is affine to the user port
> > > from which the frames are received.
> > > 
> > > Currently, only the bit that corresponds to the first found CPU port is set
> > > on the bitmap. When multiple CPU ports are being used, the trapped frames
> > > from the user ports not affine to the first CPU port will be dropped as the
> > > other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
> > > not affected as there's only one port to be used as a CPU port.
> > > 
> > > To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
> > > bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
> > > switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
> > > for each CPU port.
> > > 
> > > Add a comment to explain frame trapping for these switches.
> > > 
> > > According to the document MT7531 Reference Manual for Development Board
> > > v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
> > > beforehand. Since there's currently no public document for the switch on
> > > the MT7988 SoC, I assume this is also the case for this switch.
> > > 
> > > Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> > > ---
> > 
> > Would you agree that this is just preparatory work for change "net: dsa:
> > introduce preferred_default_local_cpu_port and use on MT7530" and not a
> > fix to an existing problem in the code base?
> 
> Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a case
> where there's a user port affine to a CPU port that is not enabled on the
> CPU port bitmap. So yeah, this is just preparatory work for "net: dsa:
> introduce preferred_default_local_cpu_port and use on MT7530".
> 
> So how do I change the patch to reflect this?
> 
> Arınç

net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP

MT7531_CPU_PMAP represents the destination port mask for trapped-to-CPU
frames (further restricted by PCR_MATRIX).

Currently the driver sets the first CPU port as the single port in this
bit mask, which works fine regardless of whether the device tree defines
port 5, 6 or 5+6 as CPU ports. This is because the logic coincides with
DSA's logic of picking the first CPU port as the CPU port that all user
ports are affine to, by default.

An upcoming change would like to influence DSA's selection of the
default CPU port to no longer be the first one, and in that case, this
logic needs adaptation.

Since there is no observed leakage or duplication of frames if all CPU
ports are defined in this bit mask, simply include them all.

Note that there is no Fixes tag

On 15 June 2023 00:13:52 EEST, Vladimir Oltean <olteanv@gmail.com> wrote:
>On Wed, Jun 14, 2023 at 11:56:44PM +0300, Arınç ÜNAL wrote:
>> On 14.06.2023 22:43, Vladimir Oltean wrote:
>> > On Mon, Jun 12, 2023 at 10:59:39AM +0300, arinc9.unal@gmail.com wrote:
>> > > From: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > 
>> > > Every bit of the CPU port bitmap for MT7531 and the switch on the MT7988
>> > > SoC represents a CPU port to trap frames to. These switches trap frames
>> > > received from a user port to the CPU port that is affine to the user port
>> > > from which the frames are received.
>> > > 
>> > > Currently, only the bit that corresponds to the first found CPU port is set
>> > > on the bitmap. When multiple CPU ports are being used, the trapped frames
>> > > from the user ports not affine to the first CPU port will be dropped as the
>> > > other CPU port is not set on the bitmap. The switch on the MT7988 SoC is
>> > > not affected as there's only one port to be used as a CPU port.
>> > > 
>> > > To fix this, introduce the MT7531_CPU_PMAP macro to individually set the
>> > > bits of the CPU port bitmap. Set the CPU port bitmap for MT7531 and the
>> > > switch on the MT7988 SoC on mt753x_cpu_port_enable() which runs on a loop
>> > > for each CPU port.
>> > > 
>> > > Add a comment to explain frame trapping for these switches.
>> > > 
>> > > According to the document MT7531 Reference Manual for Development Board
>> > > v1.0, the MT7531_CPU_PMAP bits are unset after reset so no need to clear it
>> > > beforehand. Since there's currently no public document for the switch on
>> > > the MT7988 SoC, I assume this is also the case for this switch.
>> > > 
>> > > Fixes: c288575f7810 ("net: dsa: mt7530: Add the support of MT7531 switch")
>> > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com>
>> > > ---
>> > 
>> > Would you agree that this is just preparatory work for change "net: dsa:
>> > introduce preferred_default_local_cpu_port and use on MT7530" and not a
>> > fix to an existing problem in the code base?
>> 
>> Makes sense. Pre-preferred_default_local_cpu_port patch, there isn't a case
>> where there's a user port affine to a CPU port that is not enabled on the
>> CPU port bitmap. So yeah, this is just preparatory work for "net: dsa:
>> introduce preferred_default_local_cpu_port and use on MT7530".
>> 
>> So how do I change the patch to reflect this?
>> 
>> Arınç
>
>net: dsa: mt7530: set all CPU ports in MT7531_CPU_PMAP
>
>MT7531_CPU_PMAP represents the destination port mask for trapped-to-CPU
>frames (further restricted by PCR_MATRIX).
>
>Currently the driver sets the first CPU port as the single port in this
>bit mask, which works fine regardless of whether the device tree defines
>port 5, 6 or 5+6 as CPU ports. This is because the logic coincides with
>DSA's logic of picking the first CPU port as the CPU port that all user
>ports are affine to, by default.
>
>An upcoming change would like to influence DSA's selection of the
>default CPU port to no longer be the first one, and in that case, this
>logic needs adaptation.
>
>Since there is no observed leakage or duplication of frames if all CPU
>ports are defined in this bit mask, simply include them all.
>
>Note that there is no Fixes tag

Thanks a lot for making it easier for me.

Arınç