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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The switch reset sequence for KSZ8995XA writes to a read-only status register and an undefined CPU port register instead of performing a PHY reset.
- [Medium] The driver assigns `port_mirror_add` and `port_mirror_del` for KSZ8995XA, which lacks mirroring support, leading to reserved bit corruption.
- [Low] The `supports_mii` array incorrectly advertises MII support for internal PHY ports on the KSZ8995XA.
--

commit 6b2a4a6f73e02d485d9019f443f2944affb0ec9e
Author: Linus Walleij <linusw@kernel.org>

net: dsa: microchip: Support Microchip KSZ8995XA / KS8995XA

This adds support for the Microchip KSZ8995XA also known as the
Micrel KS8995XA switch to the KSZ driver.

> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 0f84b2e7d25d8..0b02588e22fc1 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c

[ ... ]

> @@ -137,7 +138,18 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>  
>  static int ksz8_reset_switch(struct ksz_device *dev)
>  {
> -	if (ksz_is_ksz88x3(dev)) {
> +	if (ksz_is_ksz8995xa(dev)) {
> +		unsigned int port;
> +
> +		/* KSZ8995XA is "reset" by stopping the switch and starting it */
> +		ksz_cfg(dev, REG_SW_ID_1, SW_START_SWITCH, false);
> +		usleep_range(10, 100);
> +		ksz_cfg(dev, REG_SW_ID_1, SW_START_SWITCH, true);
> +
> +		/* Often boot loaders have fun with ports, so reset them */
> +		for (port = 0; port < dev->info->port_cnt; port++)
> +			ksz_port_cfg(dev, port, 0x0f, BIT(4), true);

[Severity: Medium]
Does this sequence correctly perform a PHY reset for the KSZ8995XA?

On this chip, offset 0x0F resolves to 0x1F for Port 0 (Port Status 1),
which is a read-only register, making this write ineffective. Furthermore,
for the CPU port (port 4), this writes to 0x5F, which falls into the
undocumented/reserved register space.

Could this use a different offset for the soft reset?

[ ... ]

> @@ -2416,6 +2507,34 @@ const struct ksz_dev_ops ksz88xx_dev_ops = {
>  	.pme_pwrite8 = ksz8_pme_pwrite8,
>  };
>  
> +/*
> + * Restricted operations for KSZ8995XA, so many things are not supported
> + * by this old switch that we need diet DSA operations.
> + */
> +const struct dsa_switch_ops ksz8995xa_switch_ops = {
> +	.get_tag_protocol	= ksz8995xa_get_tag_protocol,

[ ... ]

> +	.port_fast_age		= ksz8_flush_dyn_mac_table,
> +	.port_mirror_add	= ksz8_port_mirror_add,
> +	.port_mirror_del	= ksz8_port_mirror_del,

[Severity: Medium]
Since the KSZ8995XA hardware does not support port mirroring, could
advertising these mirror operations lead to unintended side effects?

When called, ksz8_port_mirror_add() and ksz8_port_mirror_del() write
to bits 7, 6, and 5 of P_MIRROR_CTRL (0x01). On the KSZ8995XA, these
bits are reserved, as only the lower 5 bits are used for VLAN membership.

> +	.port_change_mtu	= ksz8_change_mtu,
> +	.port_max_mtu		= ksz_max_mtu,
> +	/* TODO: add .port_get/set_apptrust() when we implement priority */
> +};

[ ... ]

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 57c8abfe0147d..ac27beb3054bd 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c

[ ... ]

> @@ -1485,6 +1499,21 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.internal_phy = {true, true, true, true, false},
>  	},
>  
> +	[KSZ8995XA] = {
> +		.chip_id = KSZ8995XA_CHIP_ID, /* Also known as KS8995XA */
> +		.dev_name = "KSZ8995XA",
> +		.cpu_ports = 0x10,	/* can be configured as cpu port */
> +		.port_cnt = 5,		/* total cpu and user ports */
> +		.num_tx_queues = 4,
> +		.num_ipms = 4,
> +		.ops = &ksz8995xa_dev_ops,
> +		.switch_ops = &ksz8995xa_switch_ops,
> +		.phylink_mac_ops = &ksz88x3_phylink_mac_ops,
> +		.regs = ksz8995xa_regs,
> +		.supports_mii = {true, true, true, true, true},

[Severity: Low]
Is it intentional to advertise MII support for all ports here?

Since ports 0-3 are internal PHYs and do not expose an external MII
interface (unlike the CPU port 4), this sets PHY_INTERFACE_MODE_MII in
config->supported_interfaces for these ports via ksz_phylink_get_caps().

Other similar chips like the KSZ8895 use {false, false, false, false, true}
to reflect that only the CPU port uses an external MII.

> +		.internal_phy = {true, true, true, true, false},
> +	},
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-ks8995-to-ksz8-v5-0-98baeb45e665@kernel.org?part=4