* [PATCH net] net: dsa: mv88e6xxx: Preserve priority went setting CPU port.
@ 2020-01-04 16:13 Andrew Lunn
2020-01-04 18:56 ` Vladimir Oltean
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Lunn @ 2020-01-04 16:13 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Vivien Didelot, Chris Healy, Andrew Lunn
The 6390 family uses an extended register to set the port connected to
the CPU. The lower 5 bits indicate the port, the upper three bits are
the priority of the frames as they pass through the switch, what
egress queue they should use, etc. Since frames being set to the CPU
are typically management frames, BPDU, IGMP, ARP, etc set the priority
to 7, the reset default, and the highest.
Fixes: 33641994a676 ("net: dsa: mv88e6xxx: Monitor and Management tables")
Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
drivers/net/dsa/mv88e6xxx/global1.c | 5 +++++
drivers/net/dsa/mv88e6xxx/global1.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
index 120a65d3e3ef..ce03f155e9fb 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.c
+++ b/drivers/net/dsa/mv88e6xxx/global1.c
@@ -360,6 +360,11 @@ int mv88e6390_g1_set_cpu_port(struct mv88e6xxx_chip *chip, int port)
{
u16 ptr = MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST;
+ /* Use the default high priority for manegement frames sent to
+ * the CPU.
+ */
+ port |= MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI;
+
return mv88e6390_g1_monitor_write(chip, ptr, port);
}
diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
index bc5a6b2bb1e4..5324c6f4ae90 100644
--- a/drivers/net/dsa/mv88e6xxx/global1.h
+++ b/drivers/net/dsa/mv88e6xxx/global1.h
@@ -211,6 +211,7 @@
#define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_INGRESS_DEST 0x2000
#define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_EGRESS_DEST 0x2100
#define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST 0x3000
+#define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI 0x00e0
#define MV88E6390_G1_MONITOR_MGMT_CTL_DATA_MASK 0x00ff
/* Offset 0x1C: Global Control 2 */
--
2.24.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: dsa: mv88e6xxx: Preserve priority went setting CPU port.
2020-01-04 16:13 [PATCH net] net: dsa: mv88e6xxx: Preserve priority went setting CPU port Andrew Lunn
@ 2020-01-04 18:56 ` Vladimir Oltean
2020-01-04 22:04 ` Andrew Lunn
0 siblings, 1 reply; 4+ messages in thread
From: Vladimir Oltean @ 2020-01-04 18:56 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, netdev, Vivien Didelot, Chris Healy
Hi Andrew,
Is there a typo in the commit message? (went -> when)
On Sat, 4 Jan 2020 at 18:16, Andrew Lunn <andrew@lunn.ch> wrote:
>
> The 6390 family uses an extended register to set the port connected to
> the CPU. The lower 5 bits indicate the port, the upper three bits are
> the priority of the frames as they pass through the switch, what
> egress queue they should use, etc. Since frames being set to the CPU
> are typically management frames, BPDU, IGMP, ARP, etc set the priority
> to 7, the reset default, and the highest.
>
> Fixes: 33641994a676 ("net: dsa: mv88e6xxx: Monitor and Management tables")
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
Offtopic: Does the switch look at VLAN PCP for these frames at all, or
is the priority fixed to the value from this register?
> drivers/net/dsa/mv88e6xxx/global1.c | 5 +++++
> drivers/net/dsa/mv88e6xxx/global1.h | 1 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
> index 120a65d3e3ef..ce03f155e9fb 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1.c
> @@ -360,6 +360,11 @@ int mv88e6390_g1_set_cpu_port(struct mv88e6xxx_chip *chip, int port)
> {
> u16 ptr = MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST;
>
> + /* Use the default high priority for manegement frames sent to
management
> + * the CPU.
> + */
> + port |= MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI;
> +
> return mv88e6390_g1_monitor_write(chip, ptr, port);
> }
>
> diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
> index bc5a6b2bb1e4..5324c6f4ae90 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1.h
> +++ b/drivers/net/dsa/mv88e6xxx/global1.h
> @@ -211,6 +211,7 @@
> #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_INGRESS_DEST 0x2000
> #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_EGRESS_DEST 0x2100
> #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST 0x3000
> +#define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI 0x00e0
I suppose this could be more nicely expressed as
MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI(x) ((x) << 5 &
GENMASK(7, 5)), in case somebody wants to change it from 7?
> #define MV88E6390_G1_MONITOR_MGMT_CTL_DATA_MASK 0x00ff
>
> /* Offset 0x1C: Global Control 2 */
> --
> 2.24.0
>
Regards,
-Vladimir
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: dsa: mv88e6xxx: Preserve priority went setting CPU port.
2020-01-04 18:56 ` Vladimir Oltean
@ 2020-01-04 22:04 ` Andrew Lunn
2020-01-04 22:27 ` Vladimir Oltean
0 siblings, 1 reply; 4+ messages in thread
From: Andrew Lunn @ 2020-01-04 22:04 UTC (permalink / raw)
To: Vladimir Oltean; +Cc: David Miller, netdev, Vivien Didelot, Chris Healy
On Sat, Jan 04, 2020 at 08:56:12PM +0200, Vladimir Oltean wrote:
> Hi Andrew,
>
> Is there a typo in the commit message? (went -> when)
Yep. Thanks for pointing it out.
>
> On Sat, 4 Jan 2020 at 18:16, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > The 6390 family uses an extended register to set the port connected to
> > the CPU. The lower 5 bits indicate the port, the upper three bits are
> > the priority of the frames as they pass through the switch, what
> > egress queue they should use, etc. Since frames being set to the CPU
> > are typically management frames, BPDU, IGMP, ARP, etc set the priority
> > to 7, the reset default, and the highest.
> >
> > Fixes: 33641994a676 ("net: dsa: mv88e6xxx: Monitor and Management tables")
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > ---
>
> Offtopic: Does the switch look at VLAN PCP for these frames at all, or
> is the priority fixed to the value from this register?
I _think_ it is fixed. But this is just for "management"
frames. Normal data frames heading to the CPU because of MAC address
learning should have all the normal QoS operations the switch supports
to determining their priority.
> > drivers/net/dsa/mv88e6xxx/global1.c | 5 +++++
> > drivers/net/dsa/mv88e6xxx/global1.h | 1 +
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
> > index 120a65d3e3ef..ce03f155e9fb 100644
> > --- a/drivers/net/dsa/mv88e6xxx/global1.c
> > +++ b/drivers/net/dsa/mv88e6xxx/global1.c
> > @@ -360,6 +360,11 @@ int mv88e6390_g1_set_cpu_port(struct mv88e6xxx_chip *chip, int port)
> > {
> > u16 ptr = MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST;
> >
> > + /* Use the default high priority for manegement frames sent to
>
> management
Humm. What happened to my spell checker?
> > + * the CPU.
> > + */
> > + port |= MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI;
> > +
> > return mv88e6390_g1_monitor_write(chip, ptr, port);
> > }
> >
> > diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
> > index bc5a6b2bb1e4..5324c6f4ae90 100644
> > --- a/drivers/net/dsa/mv88e6xxx/global1.h
> > +++ b/drivers/net/dsa/mv88e6xxx/global1.h
> > @@ -211,6 +211,7 @@
> > #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_INGRESS_DEST 0x2000
> > #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_EGRESS_DEST 0x2100
> > #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST 0x3000
> > +#define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI 0x00e0
>
> I suppose this could be more nicely expressed as
> MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI(x) ((x) << 5 &
> GENMASK(7, 5)), in case somebody wants to change it from 7?
It could be. But i'm not aware of any suitable existing API to
configure this. So i went KISS and used the hard coded value.
Andrew
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] net: dsa: mv88e6xxx: Preserve priority went setting CPU port.
2020-01-04 22:04 ` Andrew Lunn
@ 2020-01-04 22:27 ` Vladimir Oltean
0 siblings, 0 replies; 4+ messages in thread
From: Vladimir Oltean @ 2020-01-04 22:27 UTC (permalink / raw)
To: Andrew Lunn; +Cc: David Miller, netdev, Vivien Didelot, Chris Healy
On Sun, 5 Jan 2020 at 00:05, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, Jan 04, 2020 at 08:56:12PM +0200, Vladimir Oltean wrote:
> > Hi Andrew,
> >
> > Is there a typo in the commit message? (went -> when)
>
> Yep. Thanks for pointing it out.
>
> >
> > On Sat, 4 Jan 2020 at 18:16, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > The 6390 family uses an extended register to set the port connected to
> > > the CPU. The lower 5 bits indicate the port, the upper three bits are
> > > the priority of the frames as they pass through the switch, what
> > > egress queue they should use, etc. Since frames being set to the CPU
> > > are typically management frames, BPDU, IGMP, ARP, etc set the priority
> > > to 7, the reset default, and the highest.
> > >
> > > Fixes: 33641994a676 ("net: dsa: mv88e6xxx: Monitor and Management tables")
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > > ---
> >
> > Offtopic: Does the switch look at VLAN PCP for these frames at all, or
> > is the priority fixed to the value from this register?
>
> I _think_ it is fixed. But this is just for "management"
> frames. Normal data frames heading to the CPU because of MAC address
> learning should have all the normal QoS operations the switch supports
> to determining their priority.
>
> > > drivers/net/dsa/mv88e6xxx/global1.c | 5 +++++
> > > drivers/net/dsa/mv88e6xxx/global1.h | 1 +
> > > 2 files changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/net/dsa/mv88e6xxx/global1.c b/drivers/net/dsa/mv88e6xxx/global1.c
> > > index 120a65d3e3ef..ce03f155e9fb 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/global1.c
> > > +++ b/drivers/net/dsa/mv88e6xxx/global1.c
> > > @@ -360,6 +360,11 @@ int mv88e6390_g1_set_cpu_port(struct mv88e6xxx_chip *chip, int port)
> > > {
> > > u16 ptr = MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST;
> > >
> > > + /* Use the default high priority for manegement frames sent to
> >
> > management
>
> Humm. What happened to my spell checker?
>
> > > + * the CPU.
> > > + */
> > > + port |= MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI;
> > > +
> > > return mv88e6390_g1_monitor_write(chip, ptr, port);
> > > }
> > >
> > > diff --git a/drivers/net/dsa/mv88e6xxx/global1.h b/drivers/net/dsa/mv88e6xxx/global1.h
> > > index bc5a6b2bb1e4..5324c6f4ae90 100644
> > > --- a/drivers/net/dsa/mv88e6xxx/global1.h
> > > +++ b/drivers/net/dsa/mv88e6xxx/global1.h
> > > @@ -211,6 +211,7 @@
> > > #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_INGRESS_DEST 0x2000
> > > #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_EGRESS_DEST 0x2100
> > > #define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST 0x3000
> > > +#define MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI 0x00e0
> >
> > I suppose this could be more nicely expressed as
> > MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI(x) ((x) << 5 &
> > GENMASK(7, 5)), in case somebody wants to change it from 7?
>
> It could be. But i'm not aware of any suitable existing API to
> configure this. So i went KISS and used the hard coded value.
>
No, I mean it's absolutely reasonable for the default value of the
traffic class for management traffic to be 7 rather than 0, if it's
fixed (and it's not surprising that this is the out-of-reset default).
I was just making an argument for that "7" to be more explicit for
code lurkers who don't have access to Marvell documentation...
According to your answer above, the sja1105 hardware is very similar
in behavior (its knob is called HOSTPRIO and that driver hardcodes it
to 7 too, which makes sense). It's just that the similarity is far
from obvious if you define
MV88E6390_G1_MONITOR_MGMT_CTL_PTR_CPU_DEST_MGMTPRI as 0x00e0 (my 2
cents). Had I known that Peridot does behave so similar in this
regard, I would have thought twice about attempting to create a
driver-specific devlink param for this setting [0]:
https://www.spinics.net/lists/netdev/msg614039.html
> Andrew
Regards,
-Vladimir
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-04 22:32 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-04 16:13 [PATCH net] net: dsa: mv88e6xxx: Preserve priority went setting CPU port Andrew Lunn
2020-01-04 18:56 ` Vladimir Oltean
2020-01-04 22:04 ` Andrew Lunn
2020-01-04 22:27 ` Vladimir Oltean
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).