LAN937x switch series support cascade mode of operation, in which two
switch can be connected to work like a single switch having the
advantage of increased number of ports. Two switches can be connected
using SPI, and a dedicated port from each switch will be inter-connected
forming a data path between two switches, known as cascaded port.
This patch series add support for cascade mode of operation using SPI
protocol and configures cascaded ports from each switches based on the
requirement.
Patch series tested on LAN9373 Dual Board, which is a custom board
with two LAN9373 switches connected in cascaded mode, and PORT 4
used as cascaded port from each switch.
Rakesh Sankaranarayanan (11):
net: dsa: microchip: lan937x: add cascade tailtag
net: dsa: microchip: lan937x: update SMI index
net: dsa: microchip: lan937x: enable cascade port
net: dsa: microchip: lan937x: update port number for LAN9373
net: dsa: microchip: lan937x: add shared global interrupt
net: dsa: microchip: lan937x: get cascade tag protocol
net: dsa: microchip: lan937x: update switch register
net: dsa: microchip: lan937x: avoid mib read for cascaded port
net: dsa: microchip: lan937x: update port membership with dsa port
net: dsa: microchip: lan937x: update vlan untag membership
net: dsa: microchip: lan937x: update multicast table
drivers/net/dsa/microchip/ksz9477.c | 8 ++-
drivers/net/dsa/microchip/ksz_common.c | 47 ++++++++++----
drivers/net/dsa/microchip/ksz_common.h | 3 +
drivers/net/dsa/microchip/lan937x.h | 1 +
drivers/net/dsa/microchip/lan937x_main.c | 33 +++++++++-
drivers/net/dsa/microchip/lan937x_reg.h | 3 +
include/net/dsa.h | 17 +++++
net/dsa/tag_ksz.c | 80 ++++++++++++++++++++++--
8 files changed, 173 insertions(+), 19 deletions(-)
--
2.34.1
cascade tailtag contains 3 bytes of information, it includes
additional bytes for accomodating port number in second switch.
Destination port bitmap on first switch is at bit position 7:0 and
of second switch is at bit position 15:8, add new tailtag xmit and
rcv functions for cascade with proper formatting. Add new tag protocol
for cascading and link with new xmit and rcv functions.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
include/net/dsa.h | 2 ++
net/dsa/tag_ksz.c | 80 ++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 77 insertions(+), 5 deletions(-)
@@ -16,6 +16,7 @@#define KSZ9477_NAME "ksz9477"#define KSZ9893_NAME "ksz9893"#define LAN937X_NAME "lan937x"+#define LAN937X_CASCADE_NAME "lan937x_cascade"/* Typically only one byte is used for tail tag. */#define KSZ_PTP_TAG_LEN 4
@@ -24,6 +25,9 @@#define KSZ_HWTS_EN 0+#define SWITCH_0 0+#define SWITCH_1 1+structksz_tagger_private{structksz_tagger_datadata;/* Must be first */unsignedlongstate;
@@ -84,10 +88,10 @@ static int ksz_connect(struct dsa_switch *ds)}staticstructsk_buff*ksz_common_rcv(structsk_buff*skb,-structnet_device*dev,-unsignedintport,unsignedintlen)+structnet_device*dev,unsignedintport,+unsignedintlen,u8device){-skb->dev=dsa_master_find_slave(dev,0,port);+skb->dev=dsa_master_find_slave(dev,device,port);if(!skb->dev)returnNULL;
Get index of cascaded port (if any) from device tree and enable
the feature. These ports referenced as dev->dsa_port and will be
used for processing further based on cascaded connection.
For the second switch in cascaded connection, no dev->cpu_port will
be assigned, and same happens for dev->dsa_port variable for switches
without cascading. For the single switch design, there is no way
dev->cpu_port will be unassigned. But coming to cascaded connection,
it can be unassigned, and they will be having value zero. Keeping the
initial value as zero will create error in other features like port
forwarding since DSA will misunderstood these as port index zero. So
keep the default values as 0xFF which is of invalid value so that if
nothing assigned, taking bitmap of the cpu_port or dsa_port will not
cause any harm.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz_common.c | 4 +++
drivers/net/dsa/microchip/ksz_common.h | 2 ++
drivers/net/dsa/microchip/lan937x.h | 1 +
drivers/net/dsa/microchip/lan937x_main.c | 31 ++++++++++++++++++++++++
drivers/net/dsa/microchip/lan937x_reg.h | 3 +++
5 files changed, 41 insertions(+)
@@ -147,6 +147,7 @@ struct ksz_device {u32chip_id;u8chip_rev;intcpu_port;/* port connected to CPU */+intdsa_port;/* Port used as cascaded port */u32smi_index;intphy_port_cnt;phy_interface_tcompat_interface;
@@ -205,11 +205,42 @@ void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port)dev->dev_ops->cfg_port_member(dev,port,member);}+voidlan937x_config_dsa_port(structdsa_switch*ds)+{+structksz_device*dev=ds->priv;+structdsa_port*dp;++dev->dsa_port=0xFF;++dsa_switch_for_each_port(dp,ds){+if(dsa_is_dsa_port(ds,dp->index)){+ksz_rmw32(dev,REG_SW_CASCADE_MODE_CTL,+CASCADE_PORT_SEL,dp->index);+dev->dsa_port=dp->index;++/* Tail tag should be enabled for switch 0+*incascadedconnection.+*/+if(dev->smi_index==0){+lan937x_port_cfg(dev,dp->index,REG_PORT_CTRL_0,+PORT_TAIL_TAG_ENABLE,true);+}++/* Frame check length should be disabled for cascaded ports */+lan937x_port_cfg(dev,dp->index,REG_PORT_MAC_CTRL_0,+PORT_CHECK_LENGTH,false);+}+}+}+voidlan937x_config_cpu_port(structdsa_switch*ds){structksz_device*dev=ds->priv;structdsa_port*dp;+/* Initializing cpu_port parameter into invalid value */+dev->cpu_port=0xFF;+dsa_switch_for_each_cpu_port(dp,ds){if(dev->info->cpu_ports&(1<<dp->index)){dev->cpu_port=dp->index;
Current DSA driver register mdio interface for a port in the
format of SMI-switch_index:port_number, switch_index is derived
using variable ds->index. For a single switch ds->index will be
always zero, and for cascaded switch, ds->index should be one.
But it is found that ds->index is getting updated only after
mdio_register stage. Update mdio_register to use variable directly
from device tree using "dsa,member" identifier.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz_common.c | 6 +++++-
drivers/net/dsa/microchip/ksz_common.h | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
@@ -147,6 +147,7 @@ struct ksz_device {u32chip_id;u8chip_rev;intcpu_port;/* port connected to CPU */+u32smi_index;intphy_port_cnt;phy_interface_tcompat_interface;boolsynclko_125;
@@ -1490,7 +1490,7 @@ const struct ksz_chip_data ksz_switch_chips[] = {.num_alus=1024,.num_statics=256,.cpu_ports=0x38,/* can be configured as cpu port */-.port_cnt=5,/* total physical port count */+.port_cnt=8,/* total physical port count */.port_nirqs=6,.num_tx_queues=8,.tc_cbs_supported=true,
In cascade mode interrupt line is shared among both switches.
Update global interrupt flag for shared interrupt, otherwise second
switch probe will fail with busy status.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz_common.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
Update ksz_get_tag_protocol to return separate tag protocol if
switch is connected in cascade mode. Variable ds->dst->last_switch
will contain total number of switches registered. For cascaded
connection alone, this will be more than zero.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz_common.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
Second switch in cascaded connection doesn't have port with macb
interface. dsa_switch_register returns error if macb interface is
not up. Due to this reason, second switch in cascaded connection will
not report error during dsa_switch_register and mib thread work will be
invoked even if actual switch register is not done. This will lead to
kernel warning and it can be avoided by checking device tree setup
status. This will return true only after actual switch register is done.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz_common.c | 10 ++++++++++
1 file changed, 10 insertions(+)
@@ -3213,6 +3213,7 @@ int ksz_switch_register(struct ksz_device *dev){conststructksz_chip_data*info;structdevice_node*port,*ports;+structdsa_switch_tree*dst;phy_interface_tinterface;unsignedintport_num;intret;
@@ -3330,6 +3331,15 @@ int ksz_switch_register(struct ksz_device *dev)returnret;}+/* Do not proceed further if device tree setup is not done.+*dsa_register_switch()willnotreporterrorincaseof+*cascadedswitch.Thiswillleadtoschedulingmibread+*workandkernelwarning.+*/+dst=dev->ds->dst;+if(!dst->setup)+return0;+/* Read MIB counters every 30 seconds to avoid overflow. */dev->mib_read_interval=msecs_to_jiffies(5000);
Cascaded port need not be involved in mib read process. Unlike cpu port,
mib read function will be called for all other ports. Add check to skip
function if port is of type DSA_PORT_TYPE_DSA.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz_common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Like cpu port, cascaded port will act as host port in second switch. And
all ports from both switches should be able to forward packets to cascaded
ports. Add cascaded port (dev->dsa_port) to each port membership.
Current design add bit map of user ports as cpu port membership. Include
cascaded port index as well to this group.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz_common.c | 7 ++++---
drivers/net/dsa/microchip/lan937x_main.c | 2 +-
include/net/dsa.h | 15 +++++++++++++++
3 files changed, 20 insertions(+), 4 deletions(-)
@@ -617,6 +621,17 @@ static inline u32 dsa_cpu_ports(struct dsa_switch *ds)returnmask;}+staticinlineu32dsa_dsa_ports(structdsa_switch*ds)+{+structdsa_port*dsa_dp;+u32mask=0;++dsa_switch_for_each_dsa_port(dsa_dp,ds)+mask|=BIT(dsa_dp->index);++returnmask;+}+/* Return the local port used to reach an arbitrary switch device */staticinlineunsignedintdsa_routing_port(structdsa_switch*ds,intdevice){
Exclude cascaded port from vlan untag membership table since it will be
the host port for second switch. Here setting 1 means, port will be
capable of receiving tagged frames and 0 means, port can not receive
tagged frames.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz9477.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -399,7 +399,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port,vlan_table[1]|=BIT(port);elsevlan_table[1]&=~BIT(port);-vlan_table[1]&=~(BIT(dev->cpu_port));+vlan_table[1]&=~(BIT(dev->cpu_port)|BIT(dev->dsa_port));vlan_table[2]|=BIT(port)|BIT(dev->cpu_port);
Program multicast table for cascaded port in second switch with
default port forward value since it is the host port for second switch.
Current driver program the same for cpu port in first switch.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz9477.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
@@ -1116,18 +1116,22 @@ void ksz9477_config_cpu_port(struct dsa_switch *ds)intksz9477_enable_stp_addr(structksz_device*dev){+u32fwd_port=BIT(dev->cpu_port);constu32*masks;u32data;intret;masks=dev->info->masks;+if(dev->ds->index==1)+fwd_port=BIT(dev->dsa_port);+/* Enable Reserved multicast table */ksz_cfg(dev,REG_SW_LUE_CTRL_0,SW_RESV_MCAST_ENABLE,true);/* Set the Override bit for forwarding BPDU packet to CPU */ret=ksz_write32(dev,REG_SW_ALU_VAL_B,-ALU_V_OVERRIDE|BIT(dev->cpu_port));+ALU_V_OVERRIDE|fwd_port);if(ret<0)returnret;
From: Andrew Lunn <andrew@lunn.ch> Date: 2023-02-02 15:46:12
On Thu, Feb 02, 2023 at 06:29:27PM +0530, Rakesh Sankaranarayanan wrote:
Cascaded port need not be involved in mib read process. Unlike cpu port,
mib read function will be called for all other ports. Add check to skip
function if port is of type DSA_PORT_TYPE_DSA.
I would actually read the statistics. Having debugged D in DSA
systems, it is useful to know if packets are making it from one switch
to the other, etc.
The problem is getting the information out of the kernel. For
mv88e6xxx we have had an out of tree patch which exposes this
information in debugfs.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2023-02-02 15:46:35
On Thu, Feb 02, 2023 at 06:29:26PM +0530, Rakesh Sankaranarayanan wrote:
Second switch in cascaded connection doesn't have port with macb
interface. dsa_switch_register returns error if macb interface is
not up. Due to this reason, second switch in cascaded connection will
not report error during dsa_switch_register and mib thread work will be
invoked even if actual switch register is not done. This will lead to
kernel warning and it can be avoided by checking device tree setup
status. This will return true only after actual switch register is done.
What i think you need to do is move the code into ksz_setup().
With a D in DSA setup, dsa_switch_register() adds the switch to the
list of switches, and then a check is performed to see if all switches
in the cluster have been registered. If not, it just returns. If all
switches have been registered, it then iterates over all the switches
can calls dsa_switch_ops.setup().
By moving the start of the MIB counter into setup(), it will only be
started once all the switches are present, and it means you don't need
to look at DSA core internal state.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2023-02-02 15:47:00
On Thu, Feb 02, 2023 at 06:29:24PM +0530, Rakesh Sankaranarayanan wrote:
In cascade mode interrupt line is shared among both switches.
I assume this is specific to the board you are using. Other boards
could have two interrupts. It should not cause a problem marking it a
shared, but please update the commit message to indicate that the
interrupts don't need to be shared.
Andrew
Hi Andrew,
Thanks for the comment, I will change and test the code as you
explained and update the patch in next revision.
Thanks,
Rakesh S.
On Thu, 2023-02-02 at 16:40 +0100, Andrew Lunn wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe
On Thu, Feb 02, 2023 at 06:29:26PM +0530, Rakesh Sankaranarayanan
wrote:
quoted
Second switch in cascaded connection doesn't have port with macb
interface. dsa_switch_register returns error if macb interface is
not up. Due to this reason, second switch in cascaded connection
will
not report error during dsa_switch_register and mib thread work
will be
invoked even if actual switch register is not done. This will lead
to
kernel warning and it can be avoided by checking device tree setup
status. This will return true only after actual switch register is
done.
What i think you need to do is move the code into ksz_setup().
With a D in DSA setup, dsa_switch_register() adds the switch to the
list of switches, and then a check is performed to see if all
switches
in the cluster have been registered. If not, it just returns. If all
switches have been registered, it then iterates over all the switches
can calls dsa_switch_ops.setup().
By moving the start of the MIB counter into setup(), it will only be
started once all the switches are present, and it means you don't
need
to look at DSA core internal state.
Andrew
From: Vladimir Oltean <olteanv@gmail.com> Date: 2023-02-03 23:10:16
On Thu, Feb 02, 2023 at 06:29:20PM +0530, Rakesh Sankaranarayanan wrote:
quoted hunk
cascade tailtag contains 3 bytes of information, it includes
additional bytes for accomodating port number in second switch.
Destination port bitmap on first switch is at bit position 7:0 and
of second switch is at bit position 15:8, add new tailtag xmit and
rcv functions for cascade with proper formatting. Add new tag protocol
for cascading and link with new xmit and rcv functions.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
include/net/dsa.h | 2 ++
net/dsa/tag_ksz.c | 80 ++++++++++++++++++++++++++++++++++++++++++++---
2 files changed, 77 insertions(+), 5 deletions(-)
Nope, no new lan937x_cascade tagging protocol.
If you read Documentation/networking/dsa/dsa.rst, it says:
| From the perspective of the network stack, all switches within the same DSA
| switch tree use the same tagging protocol.
Unless you are prepared to remove this assumption from the DSA framework,
you need to fold the cascade tag handling into the regular lan937x
tagging protocol (and declare the larger needed_tailroom to cover the
tail tag case).
You can look at dp->ds->index when figuring out the length of the tail
tag that should be inserted.
There's a lot of consolidation that could (and should) be done first
between lan937x_xmit() and ksz9477_xmit(), prior to adding the support
for cascade tagging.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2023-02-03 23:18:27
On Thu, Feb 02, 2023 at 06:29:21PM +0530, Rakesh Sankaranarayanan wrote:
Current DSA driver register mdio interface for a port in the
format of SMI-switch_index:port_number, switch_index is derived
using variable ds->index. For a single switch ds->index will be
always zero, and for cascaded switch, ds->index should be one.
But it is found that ds->index is getting updated only after
mdio_register stage. Update mdio_register to use variable directly
from device tree using "dsa,member" identifier.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
Impossible, check again.
The call path is:
ksz_switch_register()
-> dsa_register_switch()
-> dsa_switch_probe()
-> dsa_switch_parse_of()
-> dsa_switch_parse_member_of()
-> sets ds->index
-> dsa_tree_setup()
-> dsa_tree_setup_switches()
-> dsa_switch_setup()
-> ksz_setup()
-> ksz_mdio_register()
-> you claim ds->index isn't set
You don't even need to be an expert on the code path, you can grep for
"ds->index = ", put a dump_stack() where it's set and one where you need
it, and compare in the stack trace which functions are common and where
they diverge.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2023-02-03 23:31:06
On Thu, Feb 02, 2023 at 06:29:25PM +0530, Rakesh Sankaranarayanan wrote:
Update ksz_get_tag_protocol to return separate tag protocol if
switch is connected in cascade mode. Variable ds->dst->last_switch
will contain total number of switches registered. For cascaded
connection alone, this will be more than zero.
Nope, last_switch does not contain the total number of switches
registered, but the index of the last switch in this tree. DSA does not
assume that the indices are consecutive.
If you make any assumption in the driver regarding switch numbering in a
cascade setup, it is an assumption that a device tree writer who is not
you needs to know about. So you must document it in
Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2023-02-03 23:37:28
On Thu, Feb 02, 2023 at 04:40:36PM +0100, Andrew Lunn wrote:
On Thu, Feb 02, 2023 at 06:29:26PM +0530, Rakesh Sankaranarayanan wrote:
quoted
Second switch in cascaded connection doesn't have port with macb
interface. dsa_switch_register returns error if macb interface is
not up. Due to this reason, second switch in cascaded connection will
not report error during dsa_switch_register and mib thread work will be
invoked even if actual switch register is not done. This will lead to
kernel warning and it can be avoided by checking device tree setup
status. This will return true only after actual switch register is done.
What i think you need to do is move the code into ksz_setup().
With a D in DSA setup, dsa_switch_register() adds the switch to the
list of switches, and then a check is performed to see if all switches
in the cluster have been registered. If not, it just returns. If all
switches have been registered, it then iterates over all the switches
can calls dsa_switch_ops.setup().
By moving the start of the MIB counter into setup(), it will only be
started once all the switches are present, and it means you don't need
to look at DSA core internal state.
+1
Also there's a bug in its own right in ksz_mib_read_work(), here:
if (!netif_carrier_ok(dp->slave))
mib->cnt_ptr = dev->info->reg_mib_cnt;
The code accesses dp->slave, so naturally it kicks the bucket for
cascade ports.
It doesn't crash with CPU ports because dp->slave is in a union with
dp->master, which is also a struct net_device * with its own carrier:
struct dsa_port {
/* A CPU port is physically connected to a master device.
* A user port exposed to userspace has a slave device.
*/
union {
struct net_device *master;
struct net_device *slave;
};
This needs to be fixed, since accessing the DSA master through a
dp->slave pointer is dangerous and unintended.
Easiest thing to do would be to only check link state if (dsa_port_is_user(dp)).
For other ports always read all MIB counters.
From: Vladimir Oltean <olteanv@gmail.com> Date: 2023-02-03 23:49:06
On Thu, Feb 02, 2023 at 06:29:29PM +0530, Rakesh Sankaranarayanan wrote:
quoted hunk
Exclude cascaded port from vlan untag membership table since it will be
the host port for second switch. Here setting 1 means, port will be
capable of receiving tagged frames and 0 means, port can not receive
tagged frames.
Signed-off-by: Rakesh Sankaranarayanan <redacted>
---
drivers/net/dsa/microchip/ksz9477.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -399,7 +399,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port,vlan_table[1]|=BIT(port);elsevlan_table[1]&=~BIT(port);-vlan_table[1]&=~(BIT(dev->cpu_port));+vlan_table[1]&=~(BIT(dev->cpu_port)|BIT(dev->dsa_port));
Hi Vladimir,
On Sat, 2023-02-04 at 01:26 +0200, Vladimir Oltean wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe
On Fri, Feb 03, 2023 at 10:43:40AM +0000,
Rakesh.Sankaranarayanan@microchip.com wrote:
quoted
Hi Andrew,
On Thu, 2023-02-02 at 16:19 +0100, Andrew Lunn wrote:
quoted
quoted
LAN9373 have total 8 physical ports. Update port_cnt member in
ksz_chip_data structure.
This seems more like a fix. Should it be applied to net, not net-
next,
and have Fixes: tag?
Andrew
Yes, I will update and send it as separate net patch with fixes
tag.
What's the story here? Arun must have surely known this isn't a 5
port switch?
It was my mistake during replicating the structure for LAN9370 and
LAN9373. I tested the basic switch functionality on LAN9370 and LAN9374
but not LAN9373. LAN9373 Evaluation board available in cascading setup.
When Rakesh brought up the board for cascading, he found out there is
bug. I should have double checked all the structure member before
submitting the patch but I overlooked it.