Message-ID: <20260112213145.4sw3mu2oqstbafmb@skbuf>
Date: Mon, 12 Jan 2026 23:31:45 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Linus Walleij <linusw@...nel.org>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 2/2] net: dsa: ks8995: Add DSA tagging to KS8995

On Wed, Jan 07, 2026 at 01:57:15PM +0100, Linus Walleij wrote:
> This makes the KS8995 DSA switch use the special tags to direct
> traffic to a specific port and identify traffic coming in on a
> specific port.
> 
> These tags are not available on the sibling devices KSZ8895
> or KSZ8795.
> 
> To do this the switch require us to enable "special tags" in a
> register, then enable tag insertion on the CPU port, meaning the
> CPU port will deliver packets with a special tag indicating which
> port the traffic is coming from, and then we need to enable
> tag removal on all outgoing (LAN) ports, this means that the
> special egress tag is stripped off by the switch before exiting
> the PHY-backed ports.
> 
> Add a MAINTAINERS entry while we're at it.
> 
> Signed-off-by: Linus Walleij <linusw@...nel.org>
> ---
>  MAINTAINERS              |  8 +++++
>  drivers/net/dsa/Kconfig  |  1 +
>  drivers/net/dsa/ks8995.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 94 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5b11839cba9d..310accf05153 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16942,6 +16942,14 @@ F:	drivers/bus/mhi/
>  F:	drivers/pci/endpoint/functions/pci-epf-mhi.c
>  F:	include/linux/mhi.h
>  
> +MICREL KS8995 DSA SWITCH
> +M:	Linus Walleij <linusw@...nel.org>
> +S:	Supported
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/net/dsa/micrel,ks8995.yaml
> +F:	drivers/net/dsa/ks8995.c
> +F:	net/dsa/tag_ks8995.c
> +
>  MICROBLAZE ARCHITECTURE
>  M:	Michal Simek <monstr@...str.eu>
>  S:	Supported
> diff --git a/drivers/net/dsa/Kconfig b/drivers/net/dsa/Kconfig
> index 7eb301fd987d..8925308cc7d7 100644
> --- a/drivers/net/dsa/Kconfig
> +++ b/drivers/net/dsa/Kconfig
> @@ -97,6 +97,7 @@ config NET_DSA_KS8995
>  	tristate "Micrel KS8995 family 5-ports 10/100 Ethernet switches"
>  	depends on SPI
>  	select NET_DSA_TAG_NONE
> +	select NET_DSA_TAG_KS8995
>  	help
>  	  This driver supports the Micrel KS8995 family of 10/100 Mbit ethernet
>  	  switches, managed over SPI.
> diff --git a/drivers/net/dsa/ks8995.c b/drivers/net/dsa/ks8995.c
> index 77d8b842693c..00c8c7853c61 100644
> --- a/drivers/net/dsa/ks8995.c
> +++ b/drivers/net/dsa/ks8995.c
> @@ -3,7 +3,7 @@
>   * SPI driver for Micrel/Kendin KS8995M and KSZ8864RMN ethernet switches
>   *
>   * Copyright (C) 2008 Gabor Juhos <juhosg at openwrt.org>
> - * Copyright (C) 2025 Linus Walleij <linus.walleij@...aro.org>
> + * Copyright (C) 2025-2026 Linus Walleij <linusw@...nel.org>
>   *
>   * This file was based on: drivers/spi/at25.c
>   *     Copyright (C) 2006 David Brownell
> @@ -338,6 +338,12 @@ static int ks8995_reset(struct ks8995_switch *ks)
>  	return ks8995_start(ks);
>  }
>  
> +static bool ks8995_is_ks8995(struct ks8995_switch *ks)
> +{
> +	return ((ks->chip->family_id == FAMILY_KS8995) &&
> +		(ks->chip->chip_id == KS8995_CHIP_ID));
> +}
> +
>  /* ks8995_get_revision - get chip revision
>   * @ks: pointer to switch instance
>   *
> @@ -532,12 +538,89 @@ dsa_tag_protocol ks8995_get_tag_protocol(struct dsa_switch *ds,
>  					 int port,
>  					 enum dsa_tag_protocol mp)
>  {
> -	/* This switch actually uses the 6 byte KS8995 protocol */
> +	struct ks8995_switch *ks = ds->priv;
> +
> +	if (ks8995_is_ks8995(ks))
> +		/* This switch uses the KS8995 protocol */
> +		return DSA_TAG_PROTO_KS8995;
> +
>  	return DSA_TAG_PROTO_NONE;
>  }
>  
> +/* Only the KS8995 supports special (DSA) tagging with special bits
> + * set for the ingress and egress ports. The "special tag" register bit
> + * in the other versions is used for clock edge setting so make sure
> + * to only enable this on the KS8995.

Can you enumerate the part numbers for which this special tag doesn't
exist? I see there is some overlap between switches supported by the
ks8995 driver and the ksz_common driver, correct? At least KSZ8864 and
KSZ8795 can be seen in the ksz_common driver as well, and there they use
the "ksz8795" tail tagging protocol. Correct? Are these the same part
numbers?

> + */
> +static int ks8995_special_tags_setup(struct ks8995_switch *ks)
> +{
> +	int ret;
> +	u8 val;
> +	int i;
> +
> +	ret = ks8995_read_reg(ks, KS8995_REG_GC9, &val);
> +	if (ret) {
> +		dev_err(ks->dev, "failed to read KS8995_REG_GC9\n");
> +		return ret;
> +	}
> +
> +	/* Enable the "special tag" (the DSA port tagging) */
> +	val |= KS8995_GC9_SPECIAL;
> +
> +	ret = ks8995_write_reg(ks, KS8995_REG_GC9, val);
> +	if (ret)
> +		dev_err(ks->dev, "failed to set KS8995_REG_GC11\n");

Does it make sense to introduce ks8995_rmw_reg()?
Also, I believe that this register write error should be fatal.

> +
> +	ret = ks8995_read_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC0), &val);
> +	if (ret) {
> +		dev_err(ks->dev, "failed to read KS8995_REG_PC0 on CPU port\n");
> +		return ret;
> +	}
> +
> +	/* Enable tag INSERTION on the CPU port, this will add the special KS8995 DSA tag
> +	 * to packets entering from the chip, indicating the source port.
> +	 */
> +	val &= ~KS8995_PC0_TAG_REM;
> +	val |= KS8995_PC0_TAG_INS;
> +
> +	ret = ks8995_write_reg(ks, KS8995_REG_PC(KS8995_CPU_PORT, KS8995_REG_PC0), val);
> +	if (ret) {
> +		dev_err(ks->dev, "failed to write KS8995_REG_PC0 on CPU port\n");
> +		return ret;
> +	}
> +
> +	/* Enable tag REMOVAL on all the LAN-facing ports: this will strip the special
> +	 * DSA tag that we add during transmission of the egress packets before they exit
> +	 * the router chip.
> +	 */

Ok, but I disagree with the explanation. The special tag is a VLAN tag,
and is treated as such. These settings actually make the user ports
egress-untagged for all VLANs, and the CPU port egress-tagged for all
VLANs. Right?

This is fine for now, because there is no bridge offloading, but the code
positioning will have to be revisited by then (the driver will have to
count its egress-tagged and egress-untagged VLANs on each port, and
reject combinations of tagged+untagged, similar to ocelot_vlan_prepare()).

> +	for (i = 0; i < KS8995_CPU_PORT; i++) {
> +		ret = ks8995_read_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC0), &val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to read KS8995_REG_PC0 on port %d\n", i);
> +			return ret;
> +		}
> +
> +		val |= KS8995_PC0_TAG_REM;
> +		val &= ~KS8995_PC0_TAG_INS;
> +
> +		ret = ks8995_write_reg(ks, KS8995_REG_PC(i, KS8995_REG_PC0), val);
> +		if (ret) {
> +			dev_err(ks->dev, "failed to write KS8995_REG_PC0 on port %d\n", i);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int ks8995_setup(struct dsa_switch *ds)
>  {
> +	struct ks8995_switch *ks = ds->priv;
> +
> +	if (ks8995_is_ks8995(ks))
> +		/* This switch uses the KS8995 protocol */
> +		return ks8995_special_tags_setup(ks);
> +
>  	return 0;
>  }
>  
> 
> -- 
> 2.52.0
> 


Some other things I didn't get the chance to comment on, when this
driver was first moved to DSA in August (sorry):
- The ks8995_driver needs a shutdown method that calls
  dsa_switch_shutdown(). This is non-optional, see how other drivers
  call things.
- I have no idea why the driver implements
  ks8995_port_pre_bridge_flags() and ks8995_port_bridge_flags() when it
  doesn't implement port_bridge_join(). This is dead code.
- I see there is no effort being made to implement user port isolation
  via Port Control 1 (KS8995_REG_PC1) bits 4-0 (Port VLAN membership).
  When the tagging protocol was DSA_TAG_PROTO_NONE, let's say this was
  more or less tolerable as a wacky swconfig-style setup, but with a
  tagging protocol implemented, the user ports absolutely have to be
  isolated between each other, and if you want forwarding, you use the
  Linux bridge for that.
- Unbridged user ports are supposed to have address learning disabled.
  This is a bridge-specific function. Having it in ks8995_port_bridge_flags()
  is fine, but you also have to disable it at probe time.

Powered by blists - more mailing lists