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
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 <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 6 +++++-
drivers/net/dsa/microchip/ksz_common.h | 1 +
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 46becc0382d6..d2ec5acd7b17 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1882,7 +1882,7 @@ static int ksz_mdio_register(struct ksz_device *dev)
bus->read = ksz_sw_mdio_read;
bus->write = ksz_sw_mdio_write;
bus->name = "ksz slave smi";
- snprintf(bus->id, MII_BUS_ID_SIZE, "SMI-%d", ds->index);
+ snprintf(bus->id, MII_BUS_ID_SIZE, "SMI-%d", dev->smi_index);
bus->parent = ds->dev;
bus->phy_mask = ~ds->phys_mii_mask;
@@ -3136,6 +3136,7 @@ struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
{
struct dsa_switch *ds;
struct ksz_device *swdev;
+ u32 sw_idx[2];
ds = devm_kzalloc(base, sizeof(*ds), GFP_KERNEL);
if (!ds)
@@ -3155,6 +3156,9 @@ struct ksz_device *ksz_switch_alloc(struct device *base, void *priv)
swdev->ds = ds;
swdev->priv = priv;
+ of_property_read_variable_u32_array(base->of_node, "dsa,member", sw_idx, 2, 2);
+ swdev->smi_index = sw_idx[1];
+
return swdev;
}
EXPORT_SYMBOL(ksz_switch_alloc);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index d2d5761d58e9..aab60f2587bf 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -147,6 +147,7 @@ struct ksz_device {
u32 chip_id;
u8 chip_rev;
int cpu_port; /* port connected to CPU */
+ u32 smi_index;
int phy_port_cnt;
phy_interface_t compat_interface;
bool synclko_125;
--
2.34.1
LAN9373 have total 8 physical ports. Update port_cnt member in
ksz_chip_data structure.
Signed-off-by: Rakesh Sankaranarayanan <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index ada673b6efc6..7062ce1749fb 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -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,
--
2.34.1
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 <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 7062ce1749fb..adf8391dd29f 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2009,7 +2009,8 @@ static irqreturn_t ksz_irq_thread_fn(int irq, void *dev_id)
return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
}
-static int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq)
+static int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq,
+ unsigned long flag)
{
int ret, n;
@@ -2025,7 +2026,7 @@ static int ksz_irq_common_setup(struct ksz_device *dev, struct ksz_irq *kirq)
irq_create_mapping(kirq->domain, n);
ret = request_threaded_irq(kirq->irq_num, NULL, ksz_irq_thread_fn,
- IRQF_ONESHOT, kirq->name, kirq);
+ flag, kirq->name, kirq);
if (ret)
goto out;
@@ -2048,7 +2049,7 @@ static int ksz_girq_setup(struct ksz_device *dev)
girq->irq_num = dev->irq;
- return ksz_irq_common_setup(dev, girq);
+ return ksz_irq_common_setup(dev, girq, (IRQF_ONESHOT | IRQF_SHARED));
}
static int ksz_pirq_setup(struct ksz_device *dev, u8 p)
@@ -2064,7 +2065,7 @@ static int ksz_pirq_setup(struct ksz_device *dev, u8 p)
if (pirq->irq_num < 0)
return pirq->irq_num;
- return ksz_irq_common_setup(dev, pirq);
+ return ksz_irq_common_setup(dev, pirq, IRQF_ONESHOT);
}
static int ksz_setup(struct dsa_switch *ds)
--
2.34.1
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 <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index adf8391dd29f..2160a3e61a5a 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2567,9 +2567,13 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
dev->chip_id == KSZ9567_CHIP_ID)
proto = DSA_TAG_PROTO_KSZ9477;
- if (is_lan937x(dev))
+ if (is_lan937x(dev)) {
proto = DSA_TAG_PROTO_LAN937X_VALUE;
+ if (ds->dst->last_switch)
+ proto = DSA_TAG_PROTO_LAN937X_CASCADE_VALUE;
+ }
+
return proto;
}
--
2.34.1
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 <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 2160a3e61a5a..0df71156a540 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -3213,6 +3213,7 @@ int ksz_switch_register(struct ksz_device *dev)
{
const struct ksz_chip_data *info;
struct device_node *port, *ports;
+ struct dsa_switch_tree *dst;
phy_interface_t interface;
unsigned int port_num;
int ret;
@@ -3330,6 +3331,15 @@ int ksz_switch_register(struct ksz_device *dev)
return ret;
}
+ /* Do not proceed further if device tree setup is not done.
+ * dsa_register_switch() will not report error in case of
+ * cascaded switch. This will lead to scheduling mib read
+ * work and kernel warning.
+ */
+ dst = dev->ds->dst;
+ if (!dst->setup)
+ return 0;
+
/* Read MIB counters every 30 seconds to avoid overflow. */
dev->mib_read_interval = msecs_to_jiffies(5000);
--
2.34.1
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 <[email protected]>
---
drivers/net/dsa/microchip/ksz_common.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 0df71156a540..913296c5dd50 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2230,7 +2230,8 @@ static void ksz_mib_read_work(struct work_struct *work)
int i;
for (i = 0; i < dev->info->port_cnt; i++) {
- if (dsa_is_unused_port(dev->ds, i))
+ if (dsa_is_unused_port(dev->ds, i) ||
+ dsa_is_dsa_port(dev->ds, i))
continue;
p = &dev->ports[i];
--
2.34.1
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 <[email protected]>
---
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(-)
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 913296c5dd50..b8b7b5b7b52d 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1748,9 +1748,9 @@ static void ksz_get_strings(struct dsa_switch *ds, int port,
static void ksz_update_port_member(struct ksz_device *dev, int port)
{
+ u8 port_member = 0, cpu_port, dsa_port;
struct ksz_port *p = &dev->ports[port];
struct dsa_switch *ds = dev->ds;
- u8 port_member = 0, cpu_port;
const struct dsa_port *dp;
int i, j;
@@ -1759,6 +1759,7 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
dp = dsa_to_port(ds, port);
cpu_port = BIT(dsa_upstream_port(ds, port));
+ dsa_port = BIT(dev->dsa_port);
for (i = 0; i < ds->num_ports; i++) {
const struct dsa_port *other_dp = dsa_to_port(ds, i);
@@ -1798,10 +1799,10 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
val |= BIT(j);
}
- dev->dev_ops->cfg_port_member(dev, i, val | cpu_port);
+ dev->dev_ops->cfg_port_member(dev, i, val | cpu_port | dsa_port);
}
- dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port);
+ dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port | dsa_port);
}
static int ksz_sw_mdio_read(struct mii_bus *bus, int addr, int regnum)
diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
index 5108a3f4bf76..b17bb1ea2a4a 100644
--- a/drivers/net/dsa/microchip/lan937x_main.c
+++ b/drivers/net/dsa/microchip/lan937x_main.c
@@ -198,7 +198,7 @@ void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port)
true);
if (cpu_port)
- member = dsa_user_ports(ds);
+ member = dsa_user_ports(ds) | dsa_dsa_ports(ds);
else
member = BIT(dsa_upstream_port(ds, port));
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 55651ad29193..939aa6ff1a38 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -591,6 +591,10 @@ static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
dsa_switch_for_each_port((_dp), (_ds)) \
if (dsa_port_is_cpu((_dp)))
+#define dsa_switch_for_each_dsa_port(_dp, _ds) \
+ dsa_switch_for_each_port((_dp), (_ds)) \
+ if (dsa_port_is_dsa((_dp)))
+
#define dsa_switch_for_each_cpu_port_continue_reverse(_dp, _ds) \
dsa_switch_for_each_port_continue_reverse((_dp), (_ds)) \
if (dsa_port_is_cpu((_dp)))
@@ -617,6 +621,17 @@ static inline u32 dsa_cpu_ports(struct dsa_switch *ds)
return mask;
}
+static inline u32 dsa_dsa_ports(struct dsa_switch *ds)
+{
+ struct dsa_port *dsa_dp;
+ u32 mask = 0;
+
+ dsa_switch_for_each_dsa_port(dsa_dp, ds)
+ mask |= BIT(dsa_dp->index);
+
+ return mask;
+}
+
/* Return the local port used to reach an arbitrary switch device */
static inline unsigned int dsa_routing_port(struct dsa_switch *ds, int device)
{
--
2.34.1
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 <[email protected]>
---
drivers/net/dsa/microchip/ksz9477.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index bf13d47c26cf..4c12131098b1 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -399,7 +399,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port,
vlan_table[1] |= BIT(port);
else
vlan_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);
--
2.34.1
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 <[email protected]>
---
drivers/net/dsa/microchip/ksz9477.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 4c12131098b1..521d8c2e1540 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1116,18 +1116,22 @@ void ksz9477_config_cpu_port(struct dsa_switch *ds)
int ksz9477_enable_stp_addr(struct ksz_device *dev)
{
+ u32 fwd_port = BIT(dev->cpu_port);
const u32 *masks;
u32 data;
int ret;
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)
return ret;
--
2.34.1
On Thu, Feb 02, 2023 at 06:29:23PM +0530, Rakesh Sankaranarayanan wrote:
> 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
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
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
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,
On Thu, 2023-02-02 at 16:19 +0100, Andrew Lunn wrote:
> > 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.
Rakesh S.
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:
> > 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
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 <[email protected]>
> ---
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.
On Fri, Feb 03, 2023 at 10:43:40AM +0000, [email protected] wrote:
> Hi Andrew,
>
> On Thu, 2023-02-02 at 16:19 +0100, Andrew Lunn wrote:
> > > 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?
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.
>
> Signed-off-by: Rakesh Sankaranarayanan <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz_common.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index adf8391dd29f..2160a3e61a5a 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -2567,9 +2567,13 @@ static enum dsa_tag_protocol ksz_get_tag_protocol(struct dsa_switch *ds,
> dev->chip_id == KSZ9567_CHIP_ID)
> proto = DSA_TAG_PROTO_KSZ9477;
>
> - if (is_lan937x(dev))
> + if (is_lan937x(dev)) {
> proto = DSA_TAG_PROTO_LAN937X_VALUE;
>
> + if (ds->dst->last_switch)
> + proto = DSA_TAG_PROTO_LAN937X_CASCADE_VALUE;
> + }
Also nope, see the comment on patch 1.
> +
> return proto;
> }
>
> --
> 2.34.1
>
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:
> > 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.
On Thu, Feb 02, 2023 at 06:29:28PM +0530, Rakesh Sankaranarayanan wrote:
> 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 <[email protected]>
> ---
> 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(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 913296c5dd50..b8b7b5b7b52d 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1748,9 +1748,9 @@ static void ksz_get_strings(struct dsa_switch *ds, int port,
>
> static void ksz_update_port_member(struct ksz_device *dev, int port)
> {
> + u8 port_member = 0, cpu_port, dsa_port;
Don't need these unimaginative names ("cpu_port", "dsa_port" for what is
actually a port *mask*). You can bitwise-OR the upstream port and the
downstream cascade port directly into "port_member", and the code in
ksz_update_port_member() would tolerate it just fine.
> struct ksz_port *p = &dev->ports[port];
> struct dsa_switch *ds = dev->ds;
> - u8 port_member = 0, cpu_port;
> const struct dsa_port *dp;
> int i, j;
>
> @@ -1759,6 +1759,7 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
>
> dp = dsa_to_port(ds, port);
> cpu_port = BIT(dsa_upstream_port(ds, port));
> + dsa_port = BIT(dev->dsa_port);
If dev->dsa_port is 0xff, what is BIT(0xff)?
>
> for (i = 0; i < ds->num_ports; i++) {
> const struct dsa_port *other_dp = dsa_to_port(ds, i);
> @@ -1798,10 +1799,10 @@ static void ksz_update_port_member(struct ksz_device *dev, int port)
> val |= BIT(j);
> }
>
> - dev->dev_ops->cfg_port_member(dev, i, val | cpu_port);
> + dev->dev_ops->cfg_port_member(dev, i, val | cpu_port | dsa_port);
> }
>
> - dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port);
> + dev->dev_ops->cfg_port_member(dev, port, port_member | cpu_port | dsa_port);
> }
>
> static int ksz_sw_mdio_read(struct mii_bus *bus, int addr, int regnum)
> diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> index 5108a3f4bf76..b17bb1ea2a4a 100644
> --- a/drivers/net/dsa/microchip/lan937x_main.c
> +++ b/drivers/net/dsa/microchip/lan937x_main.c
> @@ -198,7 +198,7 @@ void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port)
> true);
>
> if (cpu_port)
> - member = dsa_user_ports(ds);
> + member = dsa_user_ports(ds) | dsa_dsa_ports(ds);
I'm wondering if a single dsa_switch_for_each_port() list traversal plus
an "if ()" wouldn't be more efficient here.
> else
> member = BIT(dsa_upstream_port(ds, port));
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 55651ad29193..939aa6ff1a38 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -591,6 +591,10 @@ static inline bool dsa_is_user_port(struct dsa_switch *ds, int p)
> dsa_switch_for_each_port((_dp), (_ds)) \
> if (dsa_port_is_cpu((_dp)))
>
> +#define dsa_switch_for_each_dsa_port(_dp, _ds) \
> + dsa_switch_for_each_port((_dp), (_ds)) \
> + if (dsa_port_is_dsa((_dp)))
> +
> #define dsa_switch_for_each_cpu_port_continue_reverse(_dp, _ds) \
> dsa_switch_for_each_port_continue_reverse((_dp), (_ds)) \
> if (dsa_port_is_cpu((_dp)))
> @@ -617,6 +621,17 @@ static inline u32 dsa_cpu_ports(struct dsa_switch *ds)
> return mask;
> }
>
> +static inline u32 dsa_dsa_ports(struct dsa_switch *ds)
> +{
> + struct dsa_port *dsa_dp;
can name this just "dp"
> + u32 mask = 0;
> +
> + dsa_switch_for_each_dsa_port(dsa_dp, ds)
> + mask |= BIT(dsa_dp->index);
> +
> + return mask;
> +}
> +
> /* Return the local port used to reach an arbitrary switch device */
> static inline unsigned int dsa_routing_port(struct dsa_switch *ds, int device)
> {
> --
> 2.34.1
>
On Thu, Feb 02, 2023 at 06:29:29PM +0530, Rakesh Sankaranarayanan wrote:
> 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 <[email protected]>
> ---
> drivers/net/dsa/microchip/ksz9477.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index bf13d47c26cf..4c12131098b1 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -399,7 +399,7 @@ int ksz9477_port_vlan_add(struct ksz_device *dev, int port,
> vlan_table[1] |= BIT(port);
> else
> vlan_table[1] &= ~BIT(port);
> - vlan_table[1] &= ~(BIT(dev->cpu_port));
> + vlan_table[1] &= ~(BIT(dev->cpu_port) | BIT(dev->dsa_port));
same comment, what if dev->dsa_port is 0xff?
>
> vlan_table[2] |= BIT(port) | BIT(dev->cpu_port);
>
> --
> 2.34.1
>
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,
> [email protected] wrote:
> > Hi Andrew,
> >
> > On Thu, 2023-02-02 at 16:19 +0100, Andrew Lunn wrote:
> > > > 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.