Re: [PATCH net-next v14 3/4] net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 1

- [High] PHY soft reset loop unconditionally includes the MAC-only CPU
  port, risking a 600ms polling timeout and probe failure.
- [Low] ksz_spi.c now claims 'micrel,ks8995', 'micrel,ksz8795' and
  'micrel,ksz8864', but the series never reconciles the bindings: the…

--

net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This adds KSZ8995XA (Micrel KS8995XA) support to the KSZ DSA driver,
including a chip data entry, restricted dev/switch ops, DCB helper changes
for the reversed one-bit-per-DSCP TOS registers, and SPI mode 0 setup. It
also moves the legacy Micrel compatibles for KS8995, KSZ8795 and KSZ8864
over from the old KS8995 driver, made mutually exclusive through Kconfig.

> 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

[ ... ]

> @@ -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: Low]
Should the bindings be reconciled along with this ownership transfer?

These three compatibles are now handled by the KSZ driver, but
Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml (the binding
for this driver) only gained microchip,ksz8995xa in the series, and its
compatible enum does not list micrel,ks8995, micrel,ksz8795 or
micrel,ksz8864:

    compatible:
      enum:
        - microchip,ksz8463
        ...
        - microchip,ksz8895  # 5-port version of KSZ8895 family switch
        - microchip,ksz8995xa

The only schema describing the three legacy strings is
Documentation/devicetree/bindings/net/dsa/micrel,ks8995.yaml, which stays a
closed schema allowing just compatible, reg, reset-gpios plus the dsa
ethernet-ports and spi-peripheral-props references:

    properties:
      compatible:
        enum:
          - micrel,ks8995
          - micrel,ksz8795
          - micrel,ksz8864
    ...
    unevaluatedProperties: false

For those same compatibles the KSZ driver now reads microchip,synclko-125,
microchip,synclko-disable, microchip,pme-active-high, wakeup-source, the
per-port micrel,fiber-mode and RGMII delay properties in
ksz_switch_register(), and microchip,io-drive-strength-microamp in
ksz8_parse_drive_strength(). Would a device tree using any of those on a
node with a micrel,* compatible now fail dt_binding_check/dtbs_check
because micrel,ks8995.yaml rejects them?

Also, that schema's description still says:

    The chip itself is managed over SPI, but all the
    PHYs need to be accessed from an external MDIO channel.

Does this still hold for the KSZ driver, which sets
internal_phy = {true, true, true, true, false} for KSZ8995XA and registers
its own MDIO bus through ksz_mdio_register()? Would folding the legacy
compatibles into microchip,ksz.yaml (or referencing it) and updating that
description avoid having two schemas describe the same driver-supported
silicon with divergent property sets?

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -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;
> +		}
> +
> +		/* Some PHYs need additional settling time after reset. */
> +		usleep_range(1000, 2000);
> +	} else if (ksz_is_ksz88x3(dev)) {
>  		/* reset switch */
>  		ksz_cfg(dev, KSZ8863_REG_SW_RESET,
>  			KSZ8863_GLOBAL_SOFTWARE_RESET | KSZ8863_PCS_RESET, true);

[Severity: High]
Should these two loops be bounded to the ports that actually have
an internal PHY?

The KSZ8995XA chip data declares port_cnt = 5 and
internal_phy = {true, true, true, true, false}, i.e. port 4 is the
MAC-only CPU port. Both loops here run from 0 to
dev->info->port_cnt - 1, so port 4 also gets PORT_PHY_SOFT_RESET
written into REG_PORT_STATUS_3 by ksz_prmw8() and is then polled by
regmap_read_poll_timeout() waiting for that bit to self-clear. On a
port with no PHY behind it, is that bit guaranteed to be implemented
and self-clearing? If it is reserved, reads back as written, or reads
as one, the poll spins for the full 600 ms timeout and returns
-ETIMEDOUT, ksz8_reset_switch() propagates that to ksz8_setup(),
which prints "failed to reset switch" and aborts the probe - so the
switch would never come up at all.

Even in the benign case where the CPU port register happens to read
back zero, the write and the poll on port 4 look like they have no
meaning. Would it be better to iterate over dev->phy_port_cnt (set
by ksz8_switch_init() before setup runs), or to skip ports where
!dev->info->internal_phy[port], so that only real PHYs are reset and
polled? If including the CPU port is deliberate and the datasheet
documents the bit as self-clearing there too, a comment saying so
would help.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-ks8995-to-ksz8-v14-0-3d72692b3c97%40kernel.org