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 to
the CPU port the user port, which the frames are received from, is affine
to.
Currently, only the bit that corresponds to the first found CPU port is set
on the bitmap. When multiple CPU ports are being used, frames from the user
ports affine to the other CPU port which are set to be trapped will be
dropped as the affine CPU port is not set on the bitmap. Only the MT7531
switch is affected as there's only one port to be used as a CPU port on the
switch on the MT7988 SoC.
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 comments 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 <[email protected]>
---
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..8ab4718abb06 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. Any frames set for trapping to CPU port will be
+ * trapped to the CPU port the user port, which the frames are received
+ * from, is affine to.
+ */
+ 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 Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
> 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 to
> the CPU port the user port, which the frames are received from, is affine
> to.
I think you need to reword that, because at least I went "err what" -
especially the second sentence!
> Currently, only the bit that corresponds to the first found CPU port is set
> on the bitmap.
Ok.
> When multiple CPU ports are being used, frames from the user
> ports affine to the other CPU port which are set to be trapped will be
> dropped as the affine CPU port is not set on the bitmap.
Hmm. I think this is trying to say:
"When multiple CPU ports are being used, trapped frames from user ports
not affine to the first CPU port will be dropped we do not set these
ports as being affine to the second CPU port."
> Only the MT7531
> switch is affected as there's only one port to be used as a CPU port on the
> switch on the MT7988 SoC.
Erm, hang on. The previous bit indicated there was a problem when there
are multiple CPU ports, but here you're saying that only one switch is
affected - and that switch has only one CPU port. This at the very least
raises eyebrows, because it's just contradicted the first part
explaining when there's a problem.
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
> + * trapped to the CPU port the user port, which the frames are received
> + * from, is affine to.
Please reword the second sentence.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 11.06.2023 19:04, Russell King (Oracle) wrote:
> On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
>> 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 to
>> the CPU port the user port, which the frames are received from, is affine
>> to.
>
> I think you need to reword that, because at least I went "err what" -
> especially the second sentence!
Sure, how does this sound:
These switches trap frames 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.
>
> Ok.
>
>> When multiple CPU ports are being used, frames from the user
>> ports affine to the other CPU port which are set to be trapped will be
>> dropped as the affine CPU port is not set on the bitmap.
>
> Hmm. I think this is trying to say:
>
> "When multiple CPU ports are being used, trapped frames from user ports
> not affine to the first CPU port will be dropped we do not set these
> ports as being affine to the second CPU port."
Yes but it's not the affinity we set here. It's to enable the CPU port
for trapping.
>
>> Only the MT7531
>> switch is affected as there's only one port to be used as a CPU port on the
>> switch on the MT7988 SoC.
>
> Erm, hang on. The previous bit indicated there was a problem when there
> are multiple CPU ports, but here you're saying that only one switch is
> affected - and that switch has only one CPU port. This at the very least
> raises eyebrows, because it's just contradicted the first part
> explaining when there's a problem.
I meant to say, since I already explained at the start of the patch log
that this patch changes the bits of the CPU port bitmap for MT7531 and
the switch on the MT7988 SoC, only MT7531 is affected as there's only a
single CPU port on the switch on the MT7988 SoC. So the switch on the
MT7988 SoC cannot be affected.
>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
>> + * trapped to the CPU port the user port, which the frames are received
>> + * from, is affine to.
>
> Please reword the second sentence.
Any frames 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.
Arınç
On Mon, Jun 12, 2023 at 11:09:10AM +0100, Russell King (Oracle) wrote:
> > Yes but it's not the affinity we set here. It's to enable the CPU port for
> > trapping.
>
> In light of that, is the problem that we only enable one CPU port to
> receive trapped frames from their affine user ports?
The badly explained problem is that this driver is not coded up to handle
device trees with multiple CPU ports in the way that is desirable for Arınç.
Namely, when both CPU ports 5 and 6 are described in the device tree,
DSA currently chooses port 5 as the active and unchangeable CPU port.
That works, however it is not desirable for Arınç for performance reasons,
as explained in commit "net: dsa: introduce preferred_default_local_cpu_port
and use on MT7530" from this series.
So that change makes DSA choose port 6 as the active and unchangeable
CPU port. But as a preliminary change for that to work, one would need
to remove the current built-in assumption of the mt7530 driver: that the
active and unchangeable CPU port is also the first CPU port.
This change builds on the observation that there is no problem when all
CPU ports described in the device tree are set in the CPU port bitmap,
regardless of whether they are active or not. This is because packet
trapping on these switch sub-families follows the user to CPU port
affinity, and inactive CPU ports have no user ports affine to them.
On Mon, Jun 12, 2023 at 09:40:45AM +0300, Arınç ÜNAL wrote:
> On 11.06.2023 19:04, Russell King (Oracle) wrote:
> > On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
> > > 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 to
> > > the CPU port the user port, which the frames are received from, is affine
> > > to.
> >
> > I think you need to reword that, because at least I went "err what" -
> > especially the second sentence!
>
> Sure, how does this sound:
>
> These switches trap frames to the CPU port that is affine to the user port
> from which the frames are received.
"... to the inbound user port." I think that's a better way to describe
"user port from which the frames are received."
However, I'm still struggling to understand what the overall message for
this entire commit log actually is.
The actual affinity of the user ports seems to be not relevant, but this
commit is more about telling the switch which of its ports are CPU
ports.
So, if the problem is that we only end up with a single port set as a
CPU port when there are multiple, isn't it going to be better to say
something like:
"For MT7531 and the switch on MT7988, we are not correctly indicating
which ports are CPU ports when we have more than one CPU port. In order
to solve this, we need to set multiple bits in the XYZ register so the
switch will trap frames to the appropriate CPU port for frames received
on the inbound user port.
> > > Currently, only the bit that corresponds to the first found CPU port is set
> > > on the bitmap.
> >
> > Ok.
> >
> > > When multiple CPU ports are being used, frames from the user
> > > ports affine to the other CPU port which are set to be trapped will be
> > > dropped as the affine CPU port is not set on the bitmap.
> >
> > Hmm. I think this is trying to say:
> >
> > "When multiple CPU ports are being used, trapped frames from user ports
> > not affine to the first CPU port will be dropped we do not set these
> > ports as being affine to the second CPU port."
>
> Yes but it's not the affinity we set here. It's to enable the CPU port for
> trapping.
In light of that, is the problem that we only enable one CPU port to
receive trapped frames from their affine user ports?
> > > Only the MT7531
> > > switch is affected as there's only one port to be used as a CPU port on the
> > > switch on the MT7988 SoC.
> >
> > Erm, hang on. The previous bit indicated there was a problem when there
> > are multiple CPU ports, but here you're saying that only one switch is
> > affected - and that switch has only one CPU port. This at the very least
> > raises eyebrows, because it's just contradicted the first part
> > explaining when there's a problem.
>
> I meant to say, since I already explained at the start of the patch log that
> this patch changes the bits of the CPU port bitmap for MT7531 and the switch
> on the MT7988 SoC, only MT7531 is affected as there's only a single CPU port
> on the switch on the MT7988 SoC. So the switch on the MT7988 SoC cannot be
> affected.
>
> >
> > > diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> > > index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
> > > + * trapped to the CPU port the user port, which the frames are received
> > > + * from, is affine to.
> >
> > Please reword the second sentence.
>
> Any frames 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.
Too many "port"s. Would:
"Add this port to the CPU port bitmap for the MT7531 and switch on the
MT7988. Trapped frames will be sent to the CPU port that is affine to
the inbound user port."
explain it better?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 12.06.2023 13:09, Russell King (Oracle) wrote:
> On Mon, Jun 12, 2023 at 09:40:45AM +0300, Arınç ÜNAL wrote:
>> On 11.06.2023 19:04, Russell King (Oracle) wrote:
>>> On Sun, Jun 11, 2023 at 11:15:41AM +0300, Arınç ÜNAL wrote:
>>>> 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 to
>>>> the CPU port the user port, which the frames are received from, is affine
>>>> to.
>>>
>>> I think you need to reword that, because at least I went "err what" -
>>> especially the second sentence!
>>
>> Sure, how does this sound:
>>
>> These switches trap frames to the CPU port that is affine to the user port
>> from which the frames are received.
>
> "... to the inbound user port." I think that's a better way to describe
> "user port from which the frames are received."
Sounds good to me.
>
> However, I'm still struggling to understand what the overall message for
> this entire commit log actually is.
>
> The actual affinity of the user ports seems to be not relevant, but this
> commit is more about telling the switch which of its ports are CPU
> ports.
Yes, I also add a comment to explain how frame trapping works. The user
port - CPU port affinity is only relevant there.
>
> So, if the problem is that we only end up with a single port set as a
> CPU port when there are multiple, isn't it going to be better to say
> something like:
>
> "For MT7531 and the switch on MT7988, we are not correctly indicating
> which ports are CPU ports when we have more than one CPU port. In order
> to solve this, we need to set multiple bits in the XYZ register so the
> switch will trap frames to the appropriate CPU port for frames received
> on the inbound user port.
Yes, I'll replace this with the second paragraph.
>
>>>> Currently, only the bit that corresponds to the first found CPU port is set
>>>> on the bitmap.
>>>
>>> Ok.
>>>
>>>> When multiple CPU ports are being used, frames from the user
>>>> ports affine to the other CPU port which are set to be trapped will be
>>>> dropped as the affine CPU port is not set on the bitmap.
>>>
>>> Hmm. I think this is trying to say:
>>>
>>> "When multiple CPU ports are being used, trapped frames from user ports
>>> not affine to the first CPU port will be dropped we do not set these
>>> ports as being affine to the second CPU port."
>>
>> Yes but it's not the affinity we set here. It's to enable the CPU port for
>> trapping.
>
> In light of that, is the problem that we only enable one CPU port to
> receive trapped frames from their affine user ports?
Yes.
>
>>>> Only the MT7531
>>>> switch is affected as there's only one port to be used as a CPU port on the
>>>> switch on the MT7988 SoC.
>>>
>>> Erm, hang on. The previous bit indicated there was a problem when there
>>> are multiple CPU ports, but here you're saying that only one switch is
>>> affected - and that switch has only one CPU port. This at the very least
>>> raises eyebrows, because it's just contradicted the first part
>>> explaining when there's a problem.
>>
>> I meant to say, since I already explained at the start of the patch log that
>> this patch changes the bits of the CPU port bitmap for MT7531 and the switch
>> on the MT7988 SoC, only MT7531 is affected as there's only a single CPU port
>> on the switch on the MT7988 SoC. So the switch on the MT7988 SoC cannot be
>> affected.
>
>
>
>>
>>>
>>>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>>>> index 9bc54e1348cb..8ab4718abb06 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. Any frames set for trapping to CPU port will be
>>>> + * trapped to the CPU port the user port, which the frames are received
>>>> + * from, is affine to.
>>>
>>> Please reword the second sentence.
>>
>> Any frames 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.
>
> Too many "port"s. Would:
>
> "Add this port to the CPU port bitmap for the MT7531 and switch on the
> MT7988. Trapped frames will be sent to the CPU port that is affine to
> the inbound user port."
>
> explain it better?
Sounds good.
Arınç