diff mbox series

[net-next,15/15] net: mscc: ocelot: don't hardcode the number of the CPU port

Message ID 20191109130301.13716-16-olteanv@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series Accomodate DSA front-end into Ocelot | expand

Commit Message

Vladimir Oltean Nov. 9, 2019, 1:03 p.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

VSC7514 is a 10-port switch with 2 extra "CPU ports" (targets in the
queuing subsystem for terminating traffic locally).

There are 2 issues with hardcoding the CPU port as #10:
- It is not clear which snippets of the code are configuring something
  for one of the CPU ports, and which snippets are just doing something
  related to the number of physical ports.
- Actually any physical port can act as a CPU port connected to an
  external CPU (in addition to the local CPU). This is called NPI mode
  (Node Processor Interface) and is the way that the 6-port VSC9959
  (Felix) switch is integrated inside NXP LS1028A (the "local management
  CPU" functionality is not used there).

This patch makes it clear that the ocelot_bridge_stp_state_set function
operates on the CPU port (by making it an implicit member of the
bridging domain), and at the same time adds logic for the NPI port (aka
a physical port) to play the role of a CPU port (it shouldn't be part of
bridge_fwd_mask, as it's not explicitly enslaved to a bridge).

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Andrew Lunn Nov. 10, 2019, 4:50 p.m. UTC | #1 
On Sat, Nov 09, 2019 at 03:03:01PM +0200, Vladimir Oltean wrote:
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> VSC7514 is a 10-port switch with 2 extra "CPU ports" (targets in the
> queuing subsystem for terminating traffic locally).

So maybe that answers my last question.
 
> There are 2 issues with hardcoding the CPU port as #10:
> - It is not clear which snippets of the code are configuring something
>   for one of the CPU ports, and which snippets are just doing something
>   related to the number of physical ports.
> - Actually any physical port can act as a CPU port connected to an
>   external CPU (in addition to the local CPU). This is called NPI mode
>   (Node Processor Interface) and is the way that the 6-port VSC9959
>   (Felix) switch is integrated inside NXP LS1028A (the "local management
>   CPU" functionality is not used there).

So i'm having trouble reading this and spotting the difference between
the DSA concept of a CPU port and the two extra "CPU ports". Maybe
using the concept of virtual ports would help?

Are the physical ports number 0-9, and so port #10 is the first extra
"CPU port", aka a virtual port? And so that would not work for DSA,
where you need a physical port.

      Andrew
Vladimir Oltean Nov. 10, 2019, 5 p.m. UTC | #2 
On Sun, 10 Nov 2019 at 18:50, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sat, Nov 09, 2019 at 03:03:01PM +0200, Vladimir Oltean wrote:
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > VSC7514 is a 10-port switch with 2 extra "CPU ports" (targets in the
> > queuing subsystem for terminating traffic locally).
>
> So maybe that answers my last question.
>
> > There are 2 issues with hardcoding the CPU port as #10:
> > - It is not clear which snippets of the code are configuring something
> >   for one of the CPU ports, and which snippets are just doing something
> >   related to the number of physical ports.
> > - Actually any physical port can act as a CPU port connected to an
> >   external CPU (in addition to the local CPU). This is called NPI mode
> >   (Node Processor Interface) and is the way that the 6-port VSC9959
> >   (Felix) switch is integrated inside NXP LS1028A (the "local management
> >   CPU" functionality is not used there).
>
> So i'm having trouble reading this and spotting the difference between
> the DSA concept of a CPU port and the two extra "CPU ports". Maybe
> using the concept of virtual ports would help?
>
> Are the physical ports number 0-9, and so port #10 is the first extra
> "CPU port", aka a virtual port? And so that would not work for DSA,
> where you need a physical port.
>
>       Andrew

Right. See my other answer which links to Ocelot documentation. The
3.14 chapter "CPU Port Module" should clarify. The switch core has a
number of CPU ports (typically 2) which are to be integrated with
SoC-specific frame transfer abilities, typically DMA. The way this was
integrated in LS1028A is described by: "It is also possible to use a
regular front port as a CPU port. This is known as a Node Processor
Interface (NPI)." So the embedded switch and the rest of the system
are strangers and talk over Ethernet (the 2 "virtual" CPU ports are
not used), hence the reason why the "normal" (virtual, etc) CPU ports
are better modelled as switchdev and the "NPI" CPU port is better
modelled as DSA.
Andrew Lunn Nov. 10, 2019, 5:12 p.m. UTC | #3 
On Sun, Nov 10, 2019 at 07:00:33PM +0200, Vladimir Oltean wrote:
> On Sun, 10 Nov 2019 at 18:50, Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Sat, Nov 09, 2019 at 03:03:01PM +0200, Vladimir Oltean wrote:
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > >
> > > VSC7514 is a 10-port switch with 2 extra "CPU ports" (targets in the
> > > queuing subsystem for terminating traffic locally).
> >
> > So maybe that answers my last question.
> >
> > > There are 2 issues with hardcoding the CPU port as #10:
> > > - It is not clear which snippets of the code are configuring something
> > >   for one of the CPU ports, and which snippets are just doing something
> > >   related to the number of physical ports.
> > > - Actually any physical port can act as a CPU port connected to an
> > >   external CPU (in addition to the local CPU). This is called NPI mode
> > >   (Node Processor Interface) and is the way that the 6-port VSC9959
> > >   (Felix) switch is integrated inside NXP LS1028A (the "local management
> > >   CPU" functionality is not used there).
> >
> > So i'm having trouble reading this and spotting the difference between
> > the DSA concept of a CPU port and the two extra "CPU ports". Maybe
> > using the concept of virtual ports would help?
> >
> > Are the physical ports number 0-9, and so port #10 is the first extra
> > "CPU port", aka a virtual port? And so that would not work for DSA,
> > where you need a physical port.
> >
> >       Andrew
> 
> Right. See my other answer which links to Ocelot documentation.

Yes, i'm getting the picture now.

The basic problem is that in the Linux kernel CPU port has a specific
meaning, and it is clashing with the meaning used in the datasheet. So
maybe in the driver, we need to refer to these two ports as 'local
ports'?

The mv88e6xxx driver has a similar problem. Some of the switches have
a Z80 embedded in them. And this Z80 has an ethernet interface
connected to the switch core as port 12. So far we don't support it,
but if we ever do, i'm sure we will end up calling it the z80 port,
not the cpu port.

    Andrew
Vladimir Oltean Nov. 10, 2019, 5:33 p.m. UTC | #4 
On Sun, 10 Nov 2019 at 19:12, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Sun, Nov 10, 2019 at 07:00:33PM +0200, Vladimir Oltean wrote:
> > On Sun, 10 Nov 2019 at 18:50, Andrew Lunn <andrew@lunn.ch> wrote:
> > >
> > > On Sat, Nov 09, 2019 at 03:03:01PM +0200, Vladimir Oltean wrote:
> > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > >
> > > > VSC7514 is a 10-port switch with 2 extra "CPU ports" (targets in the
> > > > queuing subsystem for terminating traffic locally).
> > >
> > > So maybe that answers my last question.
> > >
> > > > There are 2 issues with hardcoding the CPU port as #10:
> > > > - It is not clear which snippets of the code are configuring something
> > > >   for one of the CPU ports, and which snippets are just doing something
> > > >   related to the number of physical ports.
> > > > - Actually any physical port can act as a CPU port connected to an
> > > >   external CPU (in addition to the local CPU). This is called NPI mode
> > > >   (Node Processor Interface) and is the way that the 6-port VSC9959
> > > >   (Felix) switch is integrated inside NXP LS1028A (the "local management
> > > >   CPU" functionality is not used there).
> > >
> > > So i'm having trouble reading this and spotting the difference between
> > > the DSA concept of a CPU port and the two extra "CPU ports". Maybe
> > > using the concept of virtual ports would help?
> > >
> > > Are the physical ports number 0-9, and so port #10 is the first extra
> > > "CPU port", aka a virtual port? And so that would not work for DSA,
> > > where you need a physical port.
> > >
> > >       Andrew
> >
> > Right. See my other answer which links to Ocelot documentation.
>
> Yes, i'm getting the picture now.
>
> The basic problem is that in the Linux kernel CPU port has a specific
> meaning, and it is clashing with the meaning used in the datasheet. So
> maybe in the driver, we need to refer to these two ports as 'local
> ports'?
>

Hmm, I don't know. Both types of CPU ports lead to management CPUs,
but to different types of them. I understand the clash with the DSA
meaning, but even if I rename it I would have to provide an
explanation relative to the datasheet definitions (and I already
explain that the NPI mode is the DSA type of CPU port). I'm not sure
there is a net gain.

> The mv88e6xxx driver has a similar problem. Some of the switches have
> a Z80 embedded in them. And this Z80 has an ethernet interface
> connected to the switch core as port 12. So far we don't support it,
> but if we ever do, i'm sure we will end up calling it the z80 port,
> not the cpu port.
>
>     Andrew
Florian Fainelli Nov. 10, 2019, 8:54 p.m. UTC | #5 
On 11/10/2019 9:33 AM, Vladimir Oltean wrote:
> On Sun, 10 Nov 2019 at 19:12, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Sun, Nov 10, 2019 at 07:00:33PM +0200, Vladimir Oltean wrote:
>>> On Sun, 10 Nov 2019 at 18:50, Andrew Lunn <andrew@lunn.ch> wrote:
>>>>
>>>> On Sat, Nov 09, 2019 at 03:03:01PM +0200, Vladimir Oltean wrote:
>>>>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
>>>>>
>>>>> VSC7514 is a 10-port switch with 2 extra "CPU ports" (targets in the
>>>>> queuing subsystem for terminating traffic locally).
>>>>
>>>> So maybe that answers my last question.
>>>>
>>>>> There are 2 issues with hardcoding the CPU port as #10:
>>>>> - It is not clear which snippets of the code are configuring something
>>>>>   for one of the CPU ports, and which snippets are just doing something
>>>>>   related to the number of physical ports.
>>>>> - Actually any physical port can act as a CPU port connected to an
>>>>>   external CPU (in addition to the local CPU). This is called NPI mode
>>>>>   (Node Processor Interface) and is the way that the 6-port VSC9959
>>>>>   (Felix) switch is integrated inside NXP LS1028A (the "local management
>>>>>   CPU" functionality is not used there).
>>>>
>>>> So i'm having trouble reading this and spotting the difference between
>>>> the DSA concept of a CPU port and the two extra "CPU ports". Maybe
>>>> using the concept of virtual ports would help?
>>>>
>>>> Are the physical ports number 0-9, and so port #10 is the first extra
>>>> "CPU port", aka a virtual port? And so that would not work for DSA,
>>>> where you need a physical port.
>>>>
>>>>       Andrew
>>>
>>> Right. See my other answer which links to Ocelot documentation.
>>
>> Yes, i'm getting the picture now.
>>
>> The basic problem is that in the Linux kernel CPU port has a specific
>> meaning, and it is clashing with the meaning used in the datasheet. So
>> maybe in the driver, we need to refer to these two ports as 'local
>> ports'?
>>
> 
> Hmm, I don't know. Both types of CPU ports lead to management CPUs,
> but to different types of them. I understand the clash with the DSA
> meaning, but even if I rename it I would have to provide an
> explanation relative to the datasheet definitions (and I already
> explain that the NPI mode is the DSA type of CPU port). I'm not sure
> there is a net gain.

Maybe we need to agree on renaming DSA's CPU port to "mgmt_port" or
something that indicates that there is in-band signaling to help support
the function of managing the switch, incidentally Broadcom switches call
their ports In-Band Management Port (IMP) which is clearer IMHO.
Vladimir Oltean Nov. 12, 2019, 12:53 a.m. UTC | #6 
On Sun, 10 Nov 2019 at 22:54, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 11/10/2019 9:33 AM, Vladimir Oltean wrote:
> > On Sun, 10 Nov 2019 at 19:12, Andrew Lunn <andrew@lunn.ch> wrote:
> >>
> >> On Sun, Nov 10, 2019 at 07:00:33PM +0200, Vladimir Oltean wrote:
> >>> On Sun, 10 Nov 2019 at 18:50, Andrew Lunn <andrew@lunn.ch> wrote:
> >>>>
> >>>> On Sat, Nov 09, 2019 at 03:03:01PM +0200, Vladimir Oltean wrote:
> >>>>> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> >>>>>
> >>>>> VSC7514 is a 10-port switch with 2 extra "CPU ports" (targets in the
> >>>>> queuing subsystem for terminating traffic locally).
> >>>>
> >>>> So maybe that answers my last question.
> >>>>
> >>>>> There are 2 issues with hardcoding the CPU port as #10:
> >>>>> - It is not clear which snippets of the code are configuring something
> >>>>>   for one of the CPU ports, and which snippets are just doing something
> >>>>>   related to the number of physical ports.
> >>>>> - Actually any physical port can act as a CPU port connected to an
> >>>>>   external CPU (in addition to the local CPU). This is called NPI mode
> >>>>>   (Node Processor Interface) and is the way that the 6-port VSC9959
> >>>>>   (Felix) switch is integrated inside NXP LS1028A (the "local management
> >>>>>   CPU" functionality is not used there).
> >>>>
> >>>> So i'm having trouble reading this and spotting the difference between
> >>>> the DSA concept of a CPU port and the two extra "CPU ports". Maybe
> >>>> using the concept of virtual ports would help?
> >>>>
> >>>> Are the physical ports number 0-9, and so port #10 is the first extra
> >>>> "CPU port", aka a virtual port? And so that would not work for DSA,
> >>>> where you need a physical port.
> >>>>
> >>>>       Andrew
> >>>
> >>> Right. See my other answer which links to Ocelot documentation.
> >>
> >> Yes, i'm getting the picture now.
> >>
> >> The basic problem is that in the Linux kernel CPU port has a specific
> >> meaning, and it is clashing with the meaning used in the datasheet. So
> >> maybe in the driver, we need to refer to these two ports as 'local
> >> ports'?
> >>
> >
> > Hmm, I don't know. Both types of CPU ports lead to management CPUs,
> > but to different types of them. I understand the clash with the DSA
> > meaning, but even if I rename it I would have to provide an
> > explanation relative to the datasheet definitions (and I already
> > explain that the NPI mode is the DSA type of CPU port). I'm not sure
> > there is a net gain.
>
> Maybe we need to agree on renaming DSA's CPU port to "mgmt_port" or
> something that indicates that there is in-band signaling to help support
> the function of managing the switch, incidentally Broadcom switches call
> their ports In-Band Management Port (IMP) which is clearer IMHO.
> --
> Florian

In the hardware conceptions that I float in, a "management port" has
the connotation of "exclusively management" (link-local multicast plus
user-defined trapping rules). While I completely understand that this
model is something that doesn't help the Linux abstraction at all, it
is something that apparently enough people in NXP have thought of as
being a good idea since they actually put it in practice in designs.
Just something to keep in mind.

Andrew, is the Z80 embedded CPU able to run Linux? If not, then from
what perspective are you saying you're going to call it "the z80 port"
instead of "CPU port", and why would you add support for it?
The current ocelot driver runs on the little CPU and doesn't support
external management, and the downstream felix driver runs on the big
CPU and doesn't support "local" I/O (DMA, PIO), so there are both at
the extremes. I don't know of any kernel driver that sets up the
switch for a remote DSA master, but I'd be curious to see what is the
terminology there.

But otherwise, I don't know whether there's anything really actionable
here. What the ocelot driver calls a CPU port is always a "port
towards the CPU running Linux and managing the switch", so the CPU
port is always local by definition, no matter whether the CPU is
connected over DMA or over Ethernet (aka NPI mode or not).

Thanks,
-Vladimir
Andrew Lunn Nov. 12, 2019, 2:53 a.m. UTC | #7 
> Andrew, is the Z80 embedded CPU able to run Linux?

No. 

If not, then from
> what perspective are you saying you're going to call it "the z80 port"
> instead of "CPU port", and why would you add support for it?

I've wanted to do a Hello World, but never got around to it.  I have
seen uses cases where it is possible to hot {un}plug the host. The
switch keeps on running. While the host is missing, STP packets are no
longer sent, but the switch keeps on switching. At some point the
other switches in the net are going to do something and STP will break
down, either partitioning the net, or causing loops. You could have
the Z80 monitor for the host going away, and either taking over the
STP, or cleanly shutting the switch down.

But Marvells real use case for the Z80 is for it to manage the switch,
no DSA at all. Just a dumb unmanaged, so very simple managed switch.

> But otherwise, I don't know whether there's anything really actionable
> here. What the ocelot driver calls a CPU port is always a "port
> towards the CPU running Linux and managing the switch", so the CPU
> port is always local by definition, no matter whether the CPU is
> connected over DMA or over Ethernet (aka NPI mode or not).

Well, it got me confused, but i think i have it now. It is more about
new people getting up to speed on the driver, especially if they have
experience with other DSA drivers, and suddenly the CPU port can mean
something different.

	  Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index bba6d60dc5a8..3e7a2796c37d 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1383,7 +1383,7 @@  static void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port,
 	 * a source for the other ports.
 	 */
 	for (p = 0; p < ocelot->num_phys_ports; p++) {
-		if (ocelot->bridge_fwd_mask & BIT(p)) {
+		if (p == ocelot->cpu || (ocelot->bridge_fwd_mask & BIT(p))) {
 			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(p);
 
 			for (i = 0; i < ocelot->num_phys_ports; i++) {
@@ -1398,15 +1398,18 @@  static void ocelot_bridge_stp_state_set(struct ocelot *ocelot, int port,
 				}
 			}
 
-			ocelot_write_rix(ocelot,
-					 BIT(ocelot->num_phys_ports) | mask,
+			/* Avoid the NPI port from looping back to itself */
+			if (p != ocelot->cpu)
+				mask |= BIT(ocelot->cpu);
+
+			ocelot_write_rix(ocelot, mask,
 					 ANA_PGID_PGID, PGID_SRC + p);
 		} else {
 			/* Only the CPU port, this is compatible with link
 			 * aggregation.
 			 */
 			ocelot_write_rix(ocelot,
-					 BIT(ocelot->num_phys_ports),
+					 BIT(ocelot->cpu),
 					 ANA_PGID_PGID, PGID_SRC + p);
 		}
 	}