[PATCH net-next v3] net: dsa: microchip: enable the SGMII port of the KSZ9897S

[PATCH net-next v3] net: dsa: microchip: enable the SGMII port of the KSZ9897S

[Tapio Reijonen]

The KSZ9897 has an SGMII/1000BASE-X capable port 7, but its chip_info
entry never set .sgmii_port. ksz_has_sgmii_port() is therefore false,
ksz9477_pcs_create() is never called and ksz9477_phylink_mac_select_pcs()
returns NULL for port 7, so a phy-mode = "sgmii" or "1000base-x" link on
that port cannot come up.

Only the KSZ9897S has that port; the KSZ9897R has a second RGMII port
instead. Both report chip ID 0x00989700, so the port cannot be enabled
from the chip ID alone, and enabling it unconditionally breaks the
KSZ9897R badly: ksz9477_setup() propagates the failure of
xpcs_create_pcs_mdiodev() on a part without an XPCS, so the switch as a
whole stops probing.

The variants do differ in the read-only bit 7 of the port 7 XMII control
0 register, which reads one on the KSZ9897S and zero on the KSZ9897R.
Compare the KSZ9897S data sheet DS00002394C section 5.2.4.1 with the
KSZ9897R data sheet DS00002330D section 5.2.3.1.

Read that bit in ksz_switch_detect(), next to the existing KSZ8765 and
KSZ9563 variant detection, and select a KSZ9897S chip_info entry which
is the KSZ9897 one plus .sgmii_port = 7. KSZ9897S_CHIP_ID only selects
that entry and is never read from a device: the low byte holds the
silicon revision, which detection masks off, so no device can report it.

Device trees describe the KSZ9897 without naming the variant, so accept
a detected KSZ9897S for a device tree naming the KSZ9897. No binding
change is needed and existing device trees keep working, whichever
variant is fitted.

Tested on a KSZ9897S with a 1000BASE-LX fibre SFP on port 7: bit 7 of
the port 7 XMII control 0 register reads one, the switch is identified
as a KSZ9897S and the PCS is created. The port advertises
1000baseX/Full, clause 37 in-band autonegotiation completes against the
link partner, and the link comes up at 1Gbps/full duplex and passes
traffic.

Suggested-by: Nicolai Buchwitz <nb@tipi-net.de>
Suggested-by: Jerry Ray <Jerry.Ray@microchip.com>
Signed-off-by: Tapio Reijonen <tapio.reijonen@vaisala.com>
---
Changes in v3:
- Only enable the SGMII port on the KSZ9897S. v2 enabled it for every
  KSZ9897, which makes a KSZ9897R fail to probe at all, because
  ksz9477_setup() propagates the xpcs_create_pcs_mdiodev() failure on a
  part that has no XPCS. Reported by Nicolai Buchwitz.
- Tell the two variants apart at run time in ksz_switch_detect(), from
  the read-only bit 7 of the port 7 XMII control 0 register, and select
  a KSZ9897S chip_info entry, next to the existing KSZ8765 and KSZ9563
  variant detection. Register suggested by Nicolai Buchwitz and
  confirmed by Jerry Ray against both data sheets.
- Accept a detected KSZ9897S for a device tree naming the KSZ9897, so
  no binding change is needed and existing device trees keep working.
- Tested on a KSZ9897S with a 1000BASE-LX fibre SFP on port 7.
- Link to v2: https://lore.kernel.org/r/20260811-ksz9897-sgmii-port-v2-1-04ae287a0b27@vaisala.com

Changes in v2:
- Resend only, no functional change: v1 was dropped when the netdev
  patch queue overflowed.
- Rebased onto current net-next.
- Link to v1: https://lore.kernel.org/r/20260703-ksz9897-sgmii-port-v1-1-387e7632dd0b@vaisala.com
---
 drivers/net/dsa/microchip/ksz_common.c      | 60 ++++++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h      |  5 +++
 include/linux/platform_data/microchip-ksz.h |  1 +
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 1a9d6f83a0232830ac4996ae5f53b8bde0b3747d..78442cc3764363a97015c71cf22f756b06044160 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1486,6 +1486,41 @@ const struct ksz_chip_data ksz_switch_chips[] = {
 		.gbit_capable	= {true, true, true, true, true, true, true},
 	},
 
+	[KSZ9897S] = {
+		.chip_id = KSZ9897S_CHIP_ID,
+		.dev_name = "KSZ9897S",
+		.num_vlans = 4096,
+		.num_alus = 4096,
+		.num_statics = 16,
+		.cpu_ports = 0x7F,	/* can be configured as cpu port */
+		.port_cnt = 7,		/* total physical port count */
+		.port_nirqs = 2,
+		.num_tx_queues = 4,
+		.num_ipms = 8,
+		.ops = &ksz9477_dev_ops,
+		.switch_ops = &ksz9477_switch_ops,
+		.phylink_mac_ops = &ksz9477_phylink_mac_ops,
+		.phy_errata_9477 = true,
+		.mib_names = ksz9477_mib_names,
+		.mib_cnt = ARRAY_SIZE(ksz9477_mib_names),
+		.reg_mib_cnt = MIB_COUNTER_NUM,
+		.regs = ksz9477_regs,
+		.masks = ksz9477_masks,
+		.shifts = ksz9477_shifts,
+		.xmii_ctrl0 = ksz9477_xmii_ctrl0,
+		.xmii_ctrl1 = ksz9477_xmii_ctrl1,
+		.supports_mii	= {false, false, false, false,
+				   false, true, true},
+		.supports_rmii	= {false, false, false, false,
+				   false, true, true},
+		.supports_rgmii = {false, false, false, false,
+				   false, true, true},
+		.internal_phy	= {true, true, true, true,
+				   true, false, false},
+		.gbit_capable	= {true, true, true, true, true, true, true},
+		.sgmii_port = 7,
+	},
+
 	[KSZ9893] = {
 		.chip_id = KSZ9893_CHIP_ID,
 		.dev_name = "KSZ9893",
@@ -1852,6 +1887,13 @@ static int ksz_check_device_id(struct ksz_device *dev)
 		expected_chip_id = expected_chip_data->chip_id;
 	}
 
+	/* The KSZ9897S is only told apart from the KSZ9897R at run time, so a
+	 * device tree naming the KSZ9897 matches it as well.
+	 */
+	if (expected_chip_id == KSZ9897_CHIP_ID &&
+	    dev->chip_id == KSZ9897S_CHIP_ID)
+		return 0;
+
 	if (expected_chip_id != dev->chip_id) {
 		dev_err(dev->dev,
 			"Device tree specifies chip %s but found %s, please fix it!\n",
@@ -2966,7 +3008,6 @@ static int ksz_switch_detect(struct ksz_device *dev)
 		switch (id32) {
 		case KSZ9477_CHIP_ID:
 		case KSZ9896_CHIP_ID:
-		case KSZ9897_CHIP_ID:
 		case KSZ9567_CHIP_ID:
 		case KSZ8567_CHIP_ID:
 		case LAN9370_CHIP_ID:
@@ -2979,6 +3020,23 @@ static int ksz_switch_detect(struct ksz_device *dev)
 			if (dev->chip_id != LAN9646_CHIP_ID)
 				dev->chip_id = id32;
 			break;
+		case KSZ9897_CHIP_ID:
+			/* Only the KSZ9897S has an SGMII port 7; the KSZ9897R
+			 * has a second RGMII port instead. See the KSZ9897S
+			 * data sheet DS00002394C section 5.2.4.1 and the
+			 * KSZ9897R data sheet DS00002330D section 5.2.3.1.
+			 */
+			ret = ksz_read8(dev, KSZ9897_REG_PORT7_XMII_CTRL_0,
+					&id4);
+			if (ret)
+				return ret;
+
+			if (id4 & KSZ9897_PORT7_SGMII_SEL)
+				dev->chip_id = KSZ9897S_CHIP_ID;
+			else
+				dev->chip_id = KSZ9897_CHIP_ID;
+
+			break;
 		case KSZ9893_CHIP_ID:
 			ret = ksz_read8(dev, REG_CHIP_ID4,
 					&id4);
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cbe98494578c371ee22b80f9508c3b35b8d6291a..0cff33df06a4b8ca585d94f9fe437ad62bcd82dc 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -284,6 +284,7 @@ enum ksz_model {
 	KSZ9477,
 	KSZ9896,
 	KSZ9897,
+	KSZ9897S,
 	KSZ9893,
 	KSZ9563,
 	KSZ9567,
@@ -874,6 +875,10 @@ static inline bool ksz_is_sgmii_port(struct ksz_device *dev, int port)
 #define SKU_ID_KSZ8563			0x3c
 #define SKU_ID_KSZ9563			0x1c
 
+/* KSZ9897 specific register: port 7 XMII control 0 */
+#define KSZ9897_REG_PORT7_XMII_CTRL_0	0x7300
+#define KSZ9897_PORT7_SGMII_SEL		BIT(7)
+
 /* Driver set switch broadcast storm protection at 10% rate. */
 #define BROADCAST_STORM_PROT_RATE	10
 
diff --git a/include/linux/platform_data/microchip-ksz.h b/include/linux/platform_data/microchip-ksz.h
index 028781ad40593248bae1b501e40ce31c5e0c69ca..fe8df1774289c401445ef308c8d3656e1e7c25b6 100644
--- a/include/linux/platform_data/microchip-ksz.h
+++ b/include/linux/platform_data/microchip-ksz.h
@@ -34,6 +34,7 @@ enum ksz_chip_id {
 	KSZ9477_CHIP_ID = 0x00947700,
 	KSZ9896_CHIP_ID = 0x00989600,
 	KSZ9897_CHIP_ID = 0x00989700,
+	KSZ9897S_CHIP_ID = 0x00989701,
 	KSZ9893_CHIP_ID = 0x00989300,
 	KSZ9563_CHIP_ID = 0x00956300,
 	KSZ8567_CHIP_ID = 0x00856700,

---
base-commit: 1bb784eb6e38fd73143f021608e4ef3095d0c0d7
change-id: 20260703-ksz9897-sgmii-port-48ddb100e015

Best regards,
-- 
Tapio Reijonen <tapio.reijonen@vaisala.com>

Re: [PATCH net-next v3] net: dsa: microchip: enable the SGMII port of the KSZ9897S

[Tapio Reijonen]

On 8/31/26 08:10, Tapio Reijonen wrote:
> The variants do differ in the read-only bit 7 of the port 7 XMII control
> 0 register, which reads one on the KSZ9897S and zero on the KSZ9897R.

Microchip support have now confirmed this with their internal team:
register 0x7300 reads 0xF8 on the KSZ9897S and 0x78 on the KSZ9897R.
The two values differ in bit 7 only, which is the bit this patch tests.

That closes the one gap in my testing. I have no KSZ9897R sample, so the
"this does not break the R" half was reasoned from the two data sheets
and the driver code; it is now confirmed by the vendor.

Tapio

Re: [net-next,v3] net: dsa: microchip: enable the SGMII port of the KSZ9897S

[netdev-bot+sashiko]

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

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

- [Medium] The new `[KSZ9897S]` chip_data entry in
  drivers/net/dsa/microchip/ksz_common.c (lines 1489-1522) declares…
- [Low] The patch carries no Fixes: tag even though its commit message
  describes a user-visible functional gap ("its chip_info entry never…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  newly routes KSZ9897S parts through it.

--

net: dsa: microchip: enable the SGMII port of the KSZ9897S

This adds a separate KSZ9897S chip_info entry that is the KSZ9897 one plus
.sgmii_port = 7, detects the variant at run time from the read-only bit 7 of
the port 7 XMII control 0 register in ksz_switch_detect(), and lets
ksz_check_device_id() accept a detected KSZ9897S for a device tree naming the
KSZ9897.

Link: https://lore.kernel.org/r/20260811-ksz9897-sgmii-port-v2-1-04ae287a0b27@vaisala.com
Link: https://lore.kernel.org/r/20260703-ksz9897-sgmii-port-v1-1-387e7632dd0b@vaisala.com

One comment on the commit message before the diff:

> The KSZ9897 has an SGMII/1000BASE-X capable port 7, but its chip_info
> entry never set .sgmii_port. ksz_has_sgmii_port() is therefore false,
> ksz9477_pcs_create() is never called and ksz9477_phylink_mac_select_pcs()
> returns NULL for port 7, so a phy-mode = "sgmii" or "1000base-x" link on
> that port cannot come up.

[Severity: Low]
This isn't a bug, but the description above reads like a user-visible
functional gap while the patch carries no Fixes: tag.  Was that
deliberate?

For what it is worth, it looks correct to leave it out: this is hardware
enablement (a new enum ksz_model value, a new KSZ9897S_CHIP_ID in
include/linux/platform_data/microchip-ksz.h, a new chip_data entry and new
run-time variant detection) aimed at net-next, and the direct precedent,
commit e8c35bfce4c13 ("net: dsa: microchip: Add SGMII port support to
KSZ9477 switch"), which introduced .sgmii_port and enabled the previously
non-working SGMII port on the KSZ9477 and LAN9646, has no Fixes: tag
either.

> diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
> index 1a9d6f83a023..78442cc37643 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1486,6 +1486,41 @@ const struct ksz_chip_data ksz_switch_chips[] = {
>  		.gbit_capable	= {true, true, true, true, true, true, true},
>  	},
>  
> +	[KSZ9897S] = {
> +		.chip_id = KSZ9897S_CHIP_ID,
> +		.dev_name = "KSZ9897S",

[ ... ]

> +		.supports_mii	= {false, false, false, false,
> +				   false, true, true},
> +		.supports_rmii	= {false, false, false, false,
> +				   false, true, true},
> +		.supports_rgmii = {false, false, false, false,
> +				   false, true, true},

[Severity: Medium]
Should index 6 of these three arrays be false in the KSZ9897S entry?

Index 6 is physical port 7, which this same entry declares as the SGMII
port, and the commit message says "Only the KSZ9897S has that port; the
KSZ9897R has a second RGMII port instead".  The [KSZ9477] entry, the other
7-port part with SGMII on port 7, sets index 6 of all three arrays to
false:

	drivers/net/dsa/microchip/ksz_common.c:
	[KSZ9477] = {
		...
		.supports_rgmii = {false, false, false, false,
				   false, true, false},
		...
		.sgmii_port = 7,

What happens if a device tree names microchip,ksz9897 (which the new bypass
in ksz_check_device_id() now accepts for a detected KSZ9897S) and declares
phy-mode = "rgmii-id", "mii" or "rmii" on port 7?

ksz_phylink_get_caps() turns the flags above into advertised interface
modes, so phylink accepts the mode:

	drivers/net/dsa/microchip/ksz_common.c:ksz_phylink_get_caps() {
		...
		if (dev->info->supports_rgmii[port])
			phy_interface_set_rgmii(config->supported_interfaces);
		...
	}

But because .sgmii_port = 7 makes ksz_is_sgmii_port() true for that port,
the xMII control registers are then never programmed:

	drivers/net/dsa/microchip/ksz_common.c:ksz_phylink_need_config() {
		/* No need to configure XMII control register when using SGMII. */
		if (ksz_is_sgmii_port(dev, port))
			return false;
		...
	}

and no PCS is attached either:

	drivers/net/dsa/microchip/ksz9477.c:ksz9477_phylink_mac_select_pcs() {
		if (ksz_is_sgmii_port(dev, dp->index) &&
		    (interface == PHY_INTERFACE_MODE_SGMII ||
		    interface == PHY_INTERFACE_MODE_1000BASEX))
			return p->pcs;

		return NULL;
	}

Does that leave port 7 with neither xMII programming nor a PCS, and without
any error or warning, instead of rejecting the mode?

Note the in-tree [LAN9646] entry carries the same combination since commit
e8c35bfce4c13, so there is precedent for it, but it disagrees with the
vendor-authored [KSZ9477] entry and with this commit message.

> +		.internal_phy	= {true, true, true, true,
> +				   true, false, false},
> +		.gbit_capable	= {true, true, true, true, true, true, true},
> +		.sgmii_port = 7,

[Severity: Medium]
This isn't a bug introduced by this patch, but setting .sgmii_port here
makes ksz_has_sgmii_port() true and newly routes KSZ9897S parts through a
PCS lifecycle that has no release path.

ksz9477_pcs_create() allocates the dw_xpcs and stores it in p->pcs:

	drivers/net/dsa/microchip/ksz9477.c:ksz9477_pcs_create() {
		...
		pcs = xpcs_create_pcs_mdiodev(bus, 0);
		if (IS_ERR(pcs))
			return PTR_ERR(pcs);
		p->pcs = pcs;
		...
	}

The counterpart, which does xpcs_clear_clks() plus xpcs_free_data() and
drops the mdio_device reference taken by xpcs_create(), is never called from
drivers/net/dsa/microchip/:

	drivers/net/pcs/pcs-xpcs.c:
	void xpcs_destroy_pcs(struct phylink_pcs *pcs)
	{
		xpcs_destroy(phylink_pcs_to_xpcs(pcs));
	}

Per-port teardown only frees the ACL, and p->pcs is left set:

	drivers/net/dsa/microchip/ksz9477.c:
	static void ksz9477_port_teardown(struct dsa_switch *ds, int port)
	{
		struct ksz_device *dev = ds->priv;

		if (dsa_is_user_port(ds, port))
			ksz9477_port_acl_free(dev, port);
	}

ksz_teardown() has no xpcs_destroy_pcs() call either.  Does every
unbind/rebind cycle, or a DSA setup failure after ksz9477_setup() created
the PCS, then leak the dw_xpcs and its mdio_device reference while the
devm-managed mdiobus it points at goes away?  The omission dates back to
commit e8c35bfce4c13, which first set .sgmii_port for the KSZ9477 and
LAN9646.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831-ksz9897-sgmii-port-v3-1-2376aa4edf11%40vaisala.com