Re: [PATCH net-next v13 3/4] net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-14 20:47:01
Also in:
linux-devicetree
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 2 · Low: 0 (1)
- [Medium] ksz_spi.c claims the three legacy compatibles "micrel,ks8995",
"micrel,ksz8795" and "micrel,ksz8864", but the only in-tree schema…
- [Medium] In the new KSZ8995XA branch of ksz8_reset_switch()
(drivers/net/dsa/microchip/ksz8.c:256), both the PHY-reset write loop…
Pre-existing issues:
- [Low] Reported concern (validated as NOT a functional regression): the
new `{ "ks8995" }` and `{ "ksz8995xa" }` entries in ksz_spi_ids[]…
--
net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA
This adds KSZ8995XA (Micrel KS8995XA) support to the KSZ DSA driver,
including a reduced set of dev/switch ops, a per-port PHY reset in
ksz8_reset_switch(), DCB helpers for the eight global TOS priority
registers, and new SPI/OF match entries. It also makes the legacy
NET_DSA_KS8995 driver mutually exclusive with NET_DSA_MICROCHIP_KSZ_SPI
and takes over the legacy micrel compatibles.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c index 941ae9f66f70b..d7498132064e8 100644 --- a/drivers/net/dsa/microchip/ksz8.c +++ b/drivers/net/dsa/microchip/ksz8.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -253,7 +255,35 @@ static int ksz8463_reset_switch(struct ksz_device *dev) static int ksz8_reset_switch(struct ksz_device *dev) { - if (ksz_is_ksz88x3(dev)) { + if (ksz_is_ksz8995xa(dev)) { + unsigned int port, reg, val; + int ret; + + /* The KSZ8995XA switch itself cannot be reset by software, but + * often boot loaders have fun with ports, so reset them. This + * reset bit is self-clearing. + */ + for (port = 0; port < dev->info->port_cnt; port++) { + ret = ksz_prmw8(dev, port, REG_PORT_STATUS_3, + PORT_PHY_SOFT_RESET, + PORT_PHY_SOFT_RESET); + if (ret) + return ret; + } + + /* IEEE 802.3 allows up to 500 ms for a PHY reset. */ + for (port = 0; port < dev->info->port_cnt; port++) { + reg = dev->dev_ops->get_port_addr(port, REG_PORT_STATUS_3); + ret = regmap_read_poll_timeout(ksz_regmap_8(dev), reg, val, + !(val & PORT_PHY_SOFT_RESET), + 1000, 600000); + if (ret) + return ret; + }
[Severity: Medium]
Should both of these loops be bounded by dev->phy_port_cnt rather than
dev->info->port_cnt?
The chip data added here declares five ports with no PHY on the last one:
[KSZ8995XA] = {
...
.port_cnt = 5, /* total cpu and user ports */
...
.internal_phy = {true, true, true, true, false},
},
and ksz8_switch_init() sets dev->phy_port_cnt = port_cnt - 1 = 4 with
dev->cpu_port = 4. Other PHY-specific per-port loops in this file, for
example in ksz8_config_cpu_port(), use dev->phy_port_cnt.
With port_cnt the loops also touch index 4, and
PORT_CTRL_ADDR(4, REG_PORT_STATUS_3) resolves to absolute register 0x5F,
for which ksz8_reg.h has no definition; it only defines up to port 4:
#define REG_PORT_1_STATUS_3 0x1F
#define REG_PORT_2_STATUS_3 0x2F
#define REG_PORT_3_STATUS_3 0x3F
#define REG_PORT_4_STATUS_3 0x4F
If PORT_PHY_SOFT_RESET does not read back as zero at 0x5F, the poll
returns -ETIMEDOUT after 600 ms and setup is aborted:
ret = ksz8_reset_switch(dev);
if (ret) {
dev_err(ds->dev, "failed to reset switch\n");
return ret;
}
so probe fails. Alternatively, if 0x5F is the control register of the
standalone PHY5 on MII-P5 (the in-tree wrv54g and mi424wr-d device trees
describe PHY5 being used by a separate WAN netdev at external MDIO
address 5), does this soft-reset a PHY that belongs to an unrelated
network interface on every switch setup?
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/dsa/microchip/ksz_spi.c b/drivers/net/dsa/microchip/ksz_spi.c index 373e9054947cb..e3bb9e604186d 100644 --- a/drivers/net/dsa/microchip/ksz_spi.c +++ b/drivers/net/dsa/microchip/ksz_spi.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -224,6 +228,25 @@ static void ksz_spi_shutdown(struct spi_device *spi) } static const struct of_device_id ksz_dt_ids[] = { + /* + * 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] + }, + { + .compatible = "micrel,ks8995", + .data = &ksz_switch_chips[KSZ8995XA] + },
[Severity: Medium] Do the bindings need updating to match the properties this driver reads for these three compatibles? The only schema matching micrel,ks8995, micrel,ksz8795 and micrel,ksz8864 is Documentation/devicetree/bindings/net/dsa/micrel,ks8995.yaml, which allows only compatible, reg, reset-gpios, ethernet-ports/ports and spi-peripheral-props, with: unevaluatedProperties: false The properties this driver parses are documented only in Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml, and that file does not list any micrel compatible (this series added only microchip,ksz8995xa there). Consumers in the driver for chips now reachable through these compatibles: ksz8_parse_drive_strength() reads and applies microchip,hi-drive-strength-microamp, microchip,lo-drive-strength-microamp and microchip,io-drive-strength-microamp for KSZ8795_CHIP_ID, i.e. for micrel,ksz8795 ksz_switch_register() reads microchip,synclko-125, microchip,synclko-disable, wakeup-source, microchip,pme-active-high and the per-port micrel,fiber-mode ksz_spi_probe() takes spi->irq, and ksz8_setup() calls ksz_girq_setup(), so interrupts is meaningful None of those properties, nor interrupts, is permitted by micrel,ks8995.yaml, so a device tree using this functionality on one of these nodes is rejected by dt_binding_check while the driver honours it. The Documentation tree is unchanged through the end of the series. [ ... ]
quoted hunk ↗ jump to hunk
@@ -317,6 +344,7 @@ static const struct of_device_id ksz_dt_ids[] = { MODULE_DEVICE_TABLE(of, ksz_dt_ids); static const struct spi_device_id ksz_spi_ids[] = { + { "ks8995" }, { "ksz8463" }, { "ksz8765" }, { "ksz8794" },
[Severity: Low] This isn't a bug introduced by this patch, but the new "ks8995" and "ksz8995xa" entries carry no driver_data, matching the existing convention in ksz_spi_ids[] where none of "ksz8463" ... "lan9646" set it either. Since ksz_spi_probe() only uses firmware match data: chip = device_get_match_data(ddev); if (!chip) return -EINVAL; a device instantiated from board info (modalias match only) cannot probe, whereas the legacy ks8995.c used spi_get_device_id(spi)->driver_data. Nothing in the tree instantiates these devices that way, and the final patch of the series states the platform data path is unused, so this is only a cleanup suggestion: either populate driver_data and use spi_get_device_match_data(), or drop the IDs that cannot be matched. -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260913-ks8995-to-ksz8-v13-0-327b16d309eb%40kernel.org