The DSA framework requires that dsa_switch_shutdown() be called when the driver is shut down. Make sure to also implement the mutual exclusion required by checking the platform data pointer to be NULL in .remove() and .shutdown(). Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up") Reported-by: Vladimir Oltean Signed-off-by: Linus Walleij --- drivers/net/dsa/ks8995.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c index 77d8b842693c..2522c9057382 100644 --- a/drivers/net/dsa/ks8995.c +++ b/drivers/net/dsa/ks8995.c @@ -833,11 +833,26 @@ static void ks8995_remove(struct spi_device *spi) { struct ks8995_switch *ks = spi_get_drvdata(spi); + if (!ks) + return; + dsa_unregister_switch(ks->ds); /* assert reset */ gpiod_set_value_cansleep(ks->reset_gpio, 1); } +static void ks8995_shutdown(struct spi_device *spi) +{ + struct ks8995_switch *ks = spi_get_drvdata(spi); + + if (!ks) + return; + + dsa_switch_shutdown(ks->ds); + + spi_set_drvdata(spi, NULL); +} + /* ------------------------------------------------------------------------ */ static struct spi_driver ks8995_driver = { .driver = { @@ -846,6 +861,7 @@ static struct spi_driver ks8995_driver = { }, .probe = ks8995_probe, .remove = ks8995_remove, + .shutdown = ks8995_shutdown, .id_table = ks8995_id, }; -- 2.53.0 Because of forking paths when Micrel was acquire by Microchip, two devices also exist with the micrel,* prefix bindings. Ass these to the KSZ SPI driver so users can use the more capable driver. Signed-off-by: Linus Walleij --- drivers/net/dsa/microchip/ksz_spi.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c index d8001734b057..e3bb1ccf2331 100644 --- a/drivers/net/dsa/microchip/ksz_spi.c +++ b/drivers/net/dsa/microchip/ksz_spi.c @@ -312,6 +312,21 @@ static const struct of_device_id ksz_dt_ids[] = { .compatible = "microchip,lan9646", .data = &ksz_switch_chips[LAN9646] }, + /* + * Legacy Micrel bindings. In 2015 Microchip acquired + * Micrel which is the originator of the KSZ series, and + * devices branded for Micrel already existed, as well as + * some device tree bindings. These two products are identical + * to the same Microchip products. + */ + { + .compatible = "micrel,ksz8864", + .data = &ksz_switch_chips[KSZ8864] + }, + { + .compatible = "micrel,ksz8795", + .data = &ksz_switch_chips[KSZ8795] + }, {}, }; MODULE_DEVICE_TABLE(of, ksz_dt_ids); -- 2.53.0 After studying the datasheets for a bit, I can conclude that the register maps for the two KSZ variants explicitly said to be supported by this driver are fully supported by the newer Micrel KSZ driver, including full VLAN support and a different custom tag than what the KS8995 is using. Delete this support, users should be using the KSZ driver CONFIG_NET_DSA_MICROCHIP_KSZ_SPI and any new device trees should use: micrel,ksz8864 -> microchip,ksz8864 micrel,ksz8795 -> microchip,ksz8795 Apparently Microchip acquired Micrel at some point and this created the confusion. Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up") Reported-by: Vladimir Oltean Signed-off-by: Linus Walleij --- drivers/net/dsa/ks8995.c | 160 +++++++++-------------------------------------- 1 file changed, 28 insertions(+), 132 deletions(-) diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c index 2522c9057382..b7c9de39f68e 100644 --- a/drivers/net/dsa/ks8995.c +++ b/drivers/net/dsa/ks8995.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * SPI driver for Micrel/Kendin KS8995M and KSZ8864RMN ethernet switches + * SPI driver for Micrel/Kendin KS8995M ethernet switch. * * Copyright (C) 2008 Gabor Juhos * Copyright (C) 2025 Linus Walleij @@ -114,11 +114,7 @@ #define KS8995_REG_IAD1 0x76 /* Indirect Access Data 1 */ #define KS8995_REG_IAD0 0x77 /* Indirect Access Data 0 */ -#define KSZ8864_REG_ID1 0xfe /* Chip ID in bit 7 */ - #define KS8995_REGS_SIZE 0x80 -#define KSZ8864_REGS_SIZE 0x100 -#define KSZ8795_REGS_SIZE 0x100 #define ID1_CHIPID_M 0xf #define ID1_CHIPID_S 4 @@ -127,11 +123,8 @@ #define ID1_START_SW 1 /* start the switch */ #define FAMILY_KS8995 0x95 -#define FAMILY_KSZ8795 0x87 #define CHIPID_M 0 #define KS8995_CHIP_ID 0x00 -#define KSZ8864_CHIP_ID 0x01 -#define KSZ8795_CHIP_ID 0x09 #define KS8995_CMD_WRITE 0x02U #define KS8995_CMD_READ 0x03U @@ -140,49 +133,6 @@ #define KS8995_NUM_PORTS 5 /* 5 ports including the CPU port */ #define KS8995_RESET_DELAY 10 /* usec */ -enum ks8995_chip_variant { - ks8995, - ksz8864, - ksz8795, - max_variant -}; - -struct ks8995_chip_params { - char *name; - int family_id; - int chip_id; - int regs_size; - int addr_width; - int addr_shift; -}; - -static const struct ks8995_chip_params ks8995_chip[] = { - [ks8995] = { - .name = "KS8995MA", - .family_id = FAMILY_KS8995, - .chip_id = KS8995_CHIP_ID, - .regs_size = KS8995_REGS_SIZE, - .addr_width = 8, - .addr_shift = 0, - }, - [ksz8864] = { - .name = "KSZ8864RMN", - .family_id = FAMILY_KS8995, - .chip_id = KSZ8864_CHIP_ID, - .regs_size = KSZ8864_REGS_SIZE, - .addr_width = 8, - .addr_shift = 0, - }, - [ksz8795] = { - .name = "KSZ8795CLX", - .family_id = FAMILY_KSZ8795, - .chip_id = KSZ8795_CHIP_ID, - .regs_size = KSZ8795_REGS_SIZE, - .addr_width = 12, - .addr_shift = 1, - }, -}; - struct ks8995_switch { struct spi_device *spi; struct device *dev; @@ -190,23 +140,18 @@ struct ks8995_switch { struct mutex lock; struct gpio_desc *reset_gpio; struct bin_attribute regs_attr; - const struct ks8995_chip_params *chip; int revision_id; unsigned int max_mtu[KS8995_NUM_PORTS]; }; static const struct spi_device_id ks8995_id[] = { - {"ks8995", ks8995}, - {"ksz8864", ksz8864}, - {"ksz8795", ksz8795}, + {"ks8995", 0}, { } }; MODULE_DEVICE_TABLE(spi, ks8995_id); static const struct of_device_id ks8995_spi_of_match[] = { { .compatible = "micrel,ks8995" }, - { .compatible = "micrel,ksz8864" }, - { .compatible = "micrel,ksz8795" }, { }, }; MODULE_DEVICE_TABLE(of, ks8995_spi_of_match); @@ -237,10 +182,10 @@ static inline __be16 create_spi_cmd(struct ks8995_switch *ks, int cmd, { u16 result = cmd; - /* make room for address (incl. address shift) */ - result <<= ks->chip->addr_width + ks->chip->addr_shift; + /* make room for address */ + result <<= 8; /* add address */ - result |= address << ks->chip->addr_shift; + result |= address; /* SPI protocol needs big endian */ return cpu_to_be16(result); } @@ -346,79 +291,37 @@ static int ks8995_reset(struct ks8995_switch *ks) static int ks8995_get_revision(struct ks8995_switch *ks) { int err; - u8 id0, id1, ksz8864_id; + u8 id0, id1; /* read family id */ err = ks8995_read_reg(ks, KS8995_REG_ID0, &id0); - if (err) { - err = -EIO; - goto err_out; - } + if (err) + return -EIO; /* verify family id */ - if (id0 != ks->chip->family_id) { + if (id0 != FAMILY_KS8995) { dev_err(&ks->spi->dev, "chip family id mismatch: expected 0x%02x but 0x%02x read\n", - ks->chip->family_id, id0); - err = -ENODEV; - goto err_out; + FAMILY_KS8995, id0); + return -ENODEV; } - switch (ks->chip->family_id) { - case FAMILY_KS8995: - /* try reading chip id at CHIP ID1 */ - err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1); - if (err) { - err = -EIO; - goto err_out; - } - - /* verify chip id */ - if ((get_chip_id(id1) == CHIPID_M) && - (get_chip_id(id1) == ks->chip->chip_id)) { - /* KS8995MA */ - ks->revision_id = get_chip_rev(id1); - } else if (get_chip_id(id1) != CHIPID_M) { - /* KSZ8864RMN */ - err = ks8995_read_reg(ks, KS8995_REG_ID1, &ksz8864_id); - if (err) { - err = -EIO; - goto err_out; - } - - if ((ksz8864_id & 0x80) && - (ks->chip->chip_id == KSZ8864_CHIP_ID)) { - ks->revision_id = get_chip_rev(id1); - } - - } else { - dev_err(&ks->spi->dev, "unsupported chip id for KS8995 family: 0x%02x\n", - id1); - err = -ENODEV; - } - break; - case FAMILY_KSZ8795: - /* try reading chip id at CHIP ID1 */ - err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1); - if (err) { - err = -EIO; - goto err_out; - } + /* try reading chip id at CHIP ID1 */ + err = ks8995_read_reg(ks, KS8995_REG_ID1, &id1); + if (err) + return -EIO; - if (get_chip_id(id1) == ks->chip->chip_id) { - ks->revision_id = get_chip_rev(id1); - } else { - dev_err(&ks->spi->dev, "unsupported chip id for KSZ8795 family: 0x%02x\n", - id1); - err = -ENODEV; - } - break; - default: - dev_err(&ks->spi->dev, "unsupported family id: 0x%02x\n", id0); - err = -ENODEV; - break; + /* verify chip id */ + if ((get_chip_id(id1) == CHIPID_M) && + (get_chip_id(id1) == KS8995_CHIP_ID)) { + /* KS8995MA */ + ks->revision_id = get_chip_rev(id1); + } else { + dev_err(&ks->spi->dev, "unsupported chip id for KS8995 family: 0x%02x\n", + id1); + return -ENODEV; } -err_out: - return err; + + return 0; } static int ks8995_check_config(struct ks8995_switch *ks) @@ -747,12 +650,6 @@ static int ks8995_probe(struct spi_device *spi) { struct ks8995_switch *ks; int err; - int variant = spi_get_device_id(spi)->driver_data; - - if (variant >= max_variant) { - dev_err(&spi->dev, "bad chip variant %d\n", variant); - return -ENODEV; - } ks = devm_kzalloc(&spi->dev, sizeof(*ks), GFP_KERNEL); if (!ks) @@ -761,7 +658,6 @@ static int ks8995_probe(struct spi_device *spi) mutex_init(&ks->lock); ks->spi = spi; ks->dev = &spi->dev; - ks->chip = &ks8995_chip[variant]; ks->reset_gpio = devm_gpiod_get_optional(&spi->dev, "reset", GPIOD_OUT_HIGH); @@ -804,8 +700,8 @@ static int ks8995_probe(struct spi_device *spi) if (err) return err; - dev_info(&spi->dev, "%s device found, Chip ID:%x, Revision:%x\n", - ks->chip->name, ks->chip->chip_id, ks->revision_id); + dev_info(&spi->dev, "KS8995MA device found, Chip ID:%x, Revision:%x\n", + KS8995_CHIP_ID, ks->revision_id); err = ks8995_check_config(ks); if (err) -- 2.53.0 Implementing ks8995_port_pre_bridge_flags() and ks8995_port_bridge_flags() without port_bridge_join() is a no-op. This adds stubs for bridge join/leave callbacks following the pattern of drivers/net/dsa/microchip/ksz_common.c: as we have STP callbacks and these will be called right after bridge join/leave these will take care of the job of setting up the learning which is all we support. Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up") Reported-by: Vladimir Oltean Signed-off-by: Linus Walleij --- drivers/net/dsa/ks8995.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c index b7c9de39f68e..1ed81a8f7d21 100644 --- a/drivers/net/dsa/ks8995.c +++ b/drivers/net/dsa/ks8995.c @@ -461,6 +461,26 @@ static void ks8995_port_disable(struct dsa_switch *ds, int port) dev_dbg(ks->dev, "disable port %d\n", port); } +static int ks8995_port_bridge_join(struct dsa_switch *ds, int port, + struct dsa_bridge bridge, + bool *tx_fwd_offload, + struct netlink_ext_ack *extack) +{ + /* port_stp_state_set() will be called after to put the port in + * appropriate state so there is no need to do anything. + */ + + return 0; +} + +static void ks8995_port_bridge_leave(struct dsa_switch *ds, int port, + struct dsa_bridge bridge) +{ + /* port_stp_state_set() will be called after to put the port in + * forwarding state so there is no need to do anything. + */ +} + static int ks8995_port_pre_bridge_flags(struct dsa_switch *ds, int port, struct switchdev_brport_flags flags, struct netlink_ext_ack *extack) @@ -635,6 +655,8 @@ static int ks8995_get_max_mtu(struct dsa_switch *ds, int port) static const struct dsa_switch_ops ks8995_ds_ops = { .get_tag_protocol = ks8995_get_tag_protocol, .setup = ks8995_setup, + .port_bridge_join = ks8995_port_bridge_join, + .port_bridge_leave = ks8995_port_bridge_leave, .port_pre_bridge_flags = ks8995_port_pre_bridge_flags, .port_bridge_flags = ks8995_port_bridge_flags, .port_enable = ks8995_port_enable, -- 2.53.0 It is unsound to not have proper port isolation on a switch which supports it. Set each port as isolated by default in the setup callback and de-isolate and isolate the ports in the bridge join/leave callbacks. Fixes: a7fe8b266f65 ("net: dsa: ks8995: Add basic switch set-up") Reported-by: Vladimir Oltean Signed-off-by: Linus Walleij --- drivers/net/dsa/ks8995.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 129 insertions(+), 2 deletions(-) diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c index 1ed81a8f7d21..dbae218fb015 100644 --- a/drivers/net/dsa/ks8995.c +++ b/drivers/net/dsa/ks8995.c @@ -80,6 +80,11 @@ #define KS8995_PC0_TAG_REM BIT(1) /* Enable tag removal on port */ #define KS8995_PC0_PRIO_EN BIT(0) /* Enable priority handling */ +#define KS8995_PC1_SNIFF_PORT BIT(7) /* This port is a sniffer port */ +#define KS8995_PC1_RCV_SNIFF BIT(6) /* Packets received goes to sniffer port(s) */ +#define KS8995_PC1_XMIT_SNIFF BIT(5) /* Packets transmitted goes to sniffer port(s) */ +#define KS8995_PC1_PORT_VLAN GENMASK(4, 0) /* Port isolation mask */ + #define KS8995_PC2_TXEN BIT(2) /* Enable TX on port */ #define KS8995_PC2_RXEN BIT(1) /* Enable RX on port */ #define KS8995_PC2_LEARN_DIS BIT(0) /* Disable learning on port */ @@ -441,6 +446,44 @@ dsa_tag_protocol ks8995_get_tag_protocol(struct dsa_switch *ds, static int ks8995_setup(struct dsa_switch *ds) { + struct ks8995_switch *ks = ds->priv; + int ret; + u8 val; + int i; + + /* Isolate all user ports so they can only send packets to itself and the CPU port */ + for (i = 0; i < KS8995_CPU_PORT; i++) { + ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val); + if (ret) { + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i); + return ret; + } + + val &= ~KS8995_PC1_PORT_VLAN; + val |= (BIT(i) | BIT(KS8995_CPU_PORT)); + + ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val); + if (ret) { + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i); + return ret; + } + } + + /* The CPU port should be able to talk to all ports */ + ret = ks8995_read_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), &val); + if (ret) { + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on CPU port\n"); + return ret; + } + + val |= KS8995_PC1_PORT_VLAN; + + ret = ks8995_write_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC1), val); + if (ret) { + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on CPU port\n"); + return ret; + } + return 0; } @@ -466,8 +509,44 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port, bool *tx_fwd_offload, struct netlink_ext_ack *extack) { + struct ks8995_switch *ks = ds->priv; + u8 port_bitmap = 0; + int ret; + u8 val; + int i; + + /* De-isolate this port from any other port on the bridge */ + port_bitmap |= BIT(port); + for (i = 0; i < KS8995_CPU_PORT; i++) { + if (i == port) + continue; + if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge)) + continue; + port_bitmap |= BIT(i); + } + + /* Update all affected ports with the new bitmask */ + for (i = 0; i < KS8995_CPU_PORT; i++) { + if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge)) + continue; + + ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val); + if (ret) { + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i); + return ret; + } + + val |= port_bitmap; + + ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val); + if (ret) { + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i); + return ret; + } + } + /* port_stp_state_set() will be called after to put the port in - * appropriate state so there is no need to do anything. + * appropriate state. */ return 0; @@ -476,8 +555,56 @@ static int ks8995_port_bridge_join(struct dsa_switch *ds, int port, static void ks8995_port_bridge_leave(struct dsa_switch *ds, int port, struct dsa_bridge bridge) { + struct ks8995_switch *ks = ds->priv; + u8 port_bitmap = 0; + int ret; + u8 val; + int i; + + /* Isolate this port from any other port on the bridge */ + for (i = 0; i < KS8995_CPU_PORT; i++) { + /* Current port handled last */ + if (i == port) + continue; + /* Not on this bridge */ + if (!dsa_port_offloads_bridge(dsa_to_port(ds, i), &bridge)) + continue; + + ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), &val); + if (ret) { + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", i); + return; + } + + val &= ~BIT(port); + + ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC1), val); + if (ret) { + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", i); + return; + } + + /* Accumulate this port for access by current */ + port_bitmap |= BIT(i); + } + + /* Isolate this port from all other ports formerly on the bridge */ + ret = ks8995_read_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), &val); + if (ret) { + dev_err(ks->dev, "failed to read KS8995_REG_PC1 on port %d\n", port); + return; + } + + val &= ~port_bitmap; + + ret = ks8995_write_reg(ks, KS8995_REG_PC(port, KS8995_REG_PC1), val); + if (ret) { + dev_err(ks->dev, "failed to write KS8995_REG_PC1 on port %d\n", port); + return; + } + /* port_stp_state_set() will be called after to put the port in - * forwarding state so there is no need to do anything. + * forwarding state. */ } -- 2.53.0