[PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for KSZ9477

[PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for KSZ9477

[Russell King (Oracle)]

This series cleans up the TXGBE configuration for SGMII modes, and then
extends that for KSZ9477's "manual" mode. Finally, we add the necessary
settings for KSZ9477 to work with 1000BASE-X which should cause no
issues for other integrations.

Work for Microchip to do before this series can be merged:

1. work out how to identify their XPCS integration from other
   integrations, so allowing MAC_MANUAL SGMII mode to be selected.
2. verify where the requirement for setting the two bits for 1000BASE-X
   has come from (from what Jose has said, we don't believe it's from
   Synopsys.)

 drivers/net/pcs/pcs-xpcs.c | 64 ++++++++++++++++++++++++++++++++++------------
 drivers/net/pcs/pcs-xpcs.h | 28 ++++++++++++++++++++
 2 files changed, 75 insertions(+), 17 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH RFC net-next 1/4] net: xpcs: add support for configuring width of 10/100M MII connection

[Russell King (Oracle)]

When in SGMII mode, the hardware can be configured to use either 4-bit
or 8-bit MII connection. Currently, we don't change this bit for most
implementations with the exception of TXGBE requiring 8-bit. Move this
decision to the creation code and act on it when configuring SGMII.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 19 +++++++++++++++----
 drivers/net/pcs/pcs-xpcs.h |  8 ++++++++
 2 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 1faa37f0e7b9..12a3d5a80b45 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -695,9 +695,18 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 	val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
 			 DW_VR_MII_PCS_MODE_C37_SGMII);
 
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
-		mask |= DW_VR_MII_AN_CTRL_8BIT;
+	switch (xpcs->sgmii_10_100_8bit) {
+	case DW_XPCS_SGMII_10_100_8BIT:
 		val |= DW_VR_MII_AN_CTRL_8BIT;
+		fallthrough;
+	case DW_XPCS_SGMII_10_100_4BIT:
+		mask |= DW_VR_MII_AN_CTRL_8BIT;
+		fallthrough;
+	case DW_XPCS_SGMII_10_100_UNCHANGED:
+		break;
+	}
+
+	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
 		/* Hardware requires it to be PHY side SGMII */
 		tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
 	} else {
@@ -1450,10 +1459,12 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
 
 	xpcs_get_interfaces(xpcs, xpcs->pcs.supported_interfaces);
 
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
+	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
 		xpcs->pcs.poll = false;
-	else
+		xpcs->sgmii_10_100_8bit = DW_XPCS_SGMII_10_100_8BIT;
+	} else {
 		xpcs->need_reset = true;
+	}
 
 	return xpcs;
 
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index adc5a0b3c883..4d53ccf917f3 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -114,6 +114,12 @@ enum dw_xpcs_clock {
 	DW_XPCS_NUM_CLKS,
 };
 
+enum dw_xpcs_sgmii_10_100 {
+	DW_XPCS_SGMII_10_100_UNCHANGED,
+	DW_XPCS_SGMII_10_100_4BIT,
+	DW_XPCS_SGMII_10_100_8BIT
+};
+
 struct dw_xpcs {
 	struct dw_xpcs_info info;
 	const struct dw_xpcs_desc *desc;
@@ -122,6 +128,8 @@ struct dw_xpcs {
 	struct phylink_pcs pcs;
 	phy_interface_t interface;
 	bool need_reset;
+	/* Width of the MII MAC/XPCS interface in 100M and 10M modes */
+	enum dw_xpcs_sgmii_10_100 sgmii_10_100_8bit;
 };
 
 int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg);
-- 
2.30.2

[PATCH RFC net-next 2/4] net: xpcs: add SGMII mode setting

[Russell King (Oracle)]

Add SGMII mode setting which configures whether XPCS immitates the MAC
end of the link or the PHY end, and in the latter case, where the data
for generating the link's configuration word comes from. This ties up
all the register bits necessary to configure this mode into one
control.

Set this to PHY_HW mode for TXGBE.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 19 +++++++++++--------
 drivers/net/pcs/pcs-xpcs.h | 14 ++++++++++++++
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 12a3d5a80b45..9d54c04ef6ee 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -706,12 +706,10 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 		break;
 	}
 
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
-		/* Hardware requires it to be PHY side SGMII */
-		tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
-	} else {
+	if (xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC)
 		tx_conf = DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII;
-	}
+	else
+		tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
 
 	val |= FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK, tx_conf);
 
@@ -722,12 +720,16 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 	val = 0;
 	mask = DW_VR_MII_DIG_CTRL1_2G5_EN | DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
 
-	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
-		val = DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
+	switch (xpcs->sgmii_mode) {
+	case DW_XPCS_SGMII_MODE_MAC:
+		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+			val = DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
+		break;
 
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
+	case DW_XPCS_SGMII_MODE_PHY_HW:
 		mask |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
 		val |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
+		break;
 	}
 
 	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, mask, val);
@@ -1462,6 +1464,7 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
 		xpcs->pcs.poll = false;
 		xpcs->sgmii_10_100_8bit = DW_XPCS_SGMII_10_100_8BIT;
+		xpcs->sgmii_mode = DW_XPCS_SGMII_MODE_PHY_HW;
 	} else {
 		xpcs->need_reset = true;
 	}
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 4d53ccf917f3..892b85425787 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -120,6 +120,19 @@ enum dw_xpcs_sgmii_10_100 {
 	DW_XPCS_SGMII_10_100_8BIT
 };
 
+/* The SGMII mode:
+ * DW_XPCS_SGMII_MODE_MAC: the XPCS acts as a MAC, reading and acknowledging
+ * the config word.
+ *
+ * DW_XPCS_SGMII_MODE_PHY_HW: the XPCS acts as a PHY, deriving the tx_config
+ * bits 15 (link), 12 (duplex) and 11:10 (speed) from hardware inputs to the
+ * XPCS.
+ */
+enum dw_xpcs_sgmii_mode {
+	DW_XPCS_SGMII_MODE_MAC,		/* XPCS is MAC on SGMII */
+	DW_XPCS_SGMII_MODE_PHY_HW,	/* XPCS is PHY, tx_config from hw */
+};
+
 struct dw_xpcs {
 	struct dw_xpcs_info info;
 	const struct dw_xpcs_desc *desc;
@@ -130,6 +143,7 @@ struct dw_xpcs {
 	bool need_reset;
 	/* Width of the MII MAC/XPCS interface in 100M and 10M modes */
 	enum dw_xpcs_sgmii_10_100 sgmii_10_100_8bit;
+	enum dw_xpcs_sgmii_mode sgmii_mode;
 };
 
 int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg);
-- 
2.30.2

[PATCH RFC net-next 3/4] net: xpcs: add SGMII MAC manual update mode

[Russell King (Oracle)]

Older revisions of the XPCS IP do not support the MAC_AUTO_SW flag and
need the BMCR register updated with the speed information from the PHY.
Split the DW_XPCS_SGMII_MODE_MAC mode into _AUTO and _MANUAL variants,
where _AUTO mode means the update happens in hardware autonomously,
whereas the _MANUAL mode means that we need to update the BMCR register
when the link comes up.

This will be required for the older XPCS IP found in KSZ9477.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
This needs further input from Tristram Ha / Microchip to work out a way
to detect KSZ9477 and set DW_XPCS_SGMII_MODE_MAC_MANUAL. On its own,
this patch does nothing.
---
 drivers/net/pcs/pcs-xpcs.c | 19 +++++++++++++------
 drivers/net/pcs/pcs-xpcs.h | 11 ++++++++---
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 9d54c04ef6ee..1eba0c583f16 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -706,7 +706,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 		break;
 	}
 
-	if (xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC)
+	if (xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC_AUTO ||
+	    xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC_MANUAL)
 		tx_conf = DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII;
 	else
 		tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
@@ -721,11 +722,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
 	mask = DW_VR_MII_DIG_CTRL1_2G5_EN | DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
 
 	switch (xpcs->sgmii_mode) {
-	case DW_XPCS_SGMII_MODE_MAC:
+	case DW_XPCS_SGMII_MODE_MAC_AUTO:
 		if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
 			val = DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
 		break;
 
+	case DW_XPCS_SGMII_MODE_MAC_MANUAL:
+		break;
+
 	case DW_XPCS_SGMII_MODE_PHY_HW:
 		mask |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
 		val |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
@@ -1151,7 +1155,9 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
 {
 	int ret;
 
-	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED &&
+	    !(interface == PHY_INTERFACE_MODE_SGMII &&
+	      xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC_MANUAL))
 		return;
 
 	if (interface == PHY_INTERFACE_MODE_1000BASEX) {
@@ -1168,10 +1174,11 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
 				__func__);
 	}
 
-	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
-			 mii_bmcr_encode_fixed(speed, duplex));
+	ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR,
+			  BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_SPEED100,
+			  mii_bmcr_encode_fixed(speed, duplex));
 	if (ret)
-		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
+		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_modify returned %pe\n",
 			__func__, ERR_PTR(ret));
 }
 
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 892b85425787..96117bd9e2b6 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -121,15 +121,20 @@ enum dw_xpcs_sgmii_10_100 {
 };
 
 /* The SGMII mode:
- * DW_XPCS_SGMII_MODE_MAC: the XPCS acts as a MAC, reading and acknowledging
- * the config word.
+ * DW_XPCS_SGMII_MODE_MAC_AUTO: the XPCS acts as a MAC, accepting the
+ * parameters from the PHY end of the SGMII link and acknowledging the
+ * config word. The XPCS autonomously switches speed.
+ *
+ * DW_XPCS_SGMII_MODE_MAC_MANUAL: the XPCS acts as a MAC as above, but
+ * does not autonomously switch speed.
  *
  * DW_XPCS_SGMII_MODE_PHY_HW: the XPCS acts as a PHY, deriving the tx_config
  * bits 15 (link), 12 (duplex) and 11:10 (speed) from hardware inputs to the
  * XPCS.
  */
 enum dw_xpcs_sgmii_mode {
-	DW_XPCS_SGMII_MODE_MAC,		/* XPCS is MAC on SGMII */
+	DW_XPCS_SGMII_MODE_MAC_AUTO,	/* XPCS is MAC, auto update */
+	DW_XPCS_SGMII_MODE_MAC_MANUAL,	/* XPCS is MAC, manual update */
 	DW_XPCS_SGMII_MODE_PHY_HW,	/* XPCS is PHY, tx_config from hw */
 };
 
-- 
2.30.2

[PATCH RFC net-next 4/4] net: xpcs: allow 1000BASE-X to work with older XPCS IP

[Russell King (Oracle)]

Older XPCS IP requires SGMII_LINK and PHY_SIDE_SGMII to be set when
operating in 1000BASE-X mode even though the XPCS is not configured for
SGMII. An example of a device with older XPCS IP is KSZ9477.

We already don't clear these bits if we switch from SGMII to 1000BASE-X
on TXGBE - which would result in 1000BASE-X with the PHY_SIDE_SGMII bit
left set.

It is currently believed to be safe to set both bits on newer IP
without side-effects.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/pcs/pcs-xpcs.c | 13 +++++++++++--
 drivers/net/pcs/pcs-xpcs.h |  1 +
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 1eba0c583f16..d522e4a5a138 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -774,9 +774,18 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
 			return ret;
 	}
 
-	mask = DW_VR_MII_PCS_MODE_MASK;
+	/* Older XPCS IP requires PHY_MODE (bit 3) and SGMII_LINK (but 4) to
+	 * be set when operating in 1000BASE-X mode. See page 233
+	 * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9477S-Data-Sheet-DS00002392C.pdf
+	 * "5.5.9 SGMII AUTO-NEGOTIATION CONTROL REGISTER"
+	 */
+	mask = DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_AN_CTRL_SGMII_LINK |
+	       DW_VR_MII_TX_CONFIG_MASK;
 	val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
-			 DW_VR_MII_PCS_MODE_C37_1000BASEX);
+			 DW_VR_MII_PCS_MODE_C37_1000BASEX) |
+	      FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK,
+			 DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII) |
+	      DW_VR_MII_AN_CTRL_SGMII_LINK;
 
 	if (!xpcs->pcs.poll) {
 		mask |= DW_VR_MII_AN_INTR_EN;
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 96117bd9e2b6..f0ddd93c7a22 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -73,6 +73,7 @@
 
 /* VR_MII_AN_CTRL */
 #define DW_VR_MII_AN_CTRL_8BIT			BIT(8)
+#define DW_VR_MII_AN_CTRL_SGMII_LINK		BIT(4)
 #define DW_VR_MII_TX_CONFIG_MASK		BIT(3)
 #define DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII	0x1
 #define DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII	0x0
-- 
2.30.2

RE: [PATCH RFC net-next 1/4] net: xpcs: add support for configuring width of 10/100M MII connection

[Tristram.Ha]

> -----Original Message-----
> From: Russell King <rmk@armlinux.org.uk> On Behalf Of Russell King (Oracle)
> Sent: Wednesday, February 5, 2025 5:28 AM
> To: Tristram Ha - C24268 <Tristram.Ha@microchip.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Woojung Huh - C21699
> <Woojung.Huh@microchip.com>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; netdev@vger.kernel.org
> Subject: [PATCH RFC net-next 1/4] net: xpcs: add support for configuring width of
> 10/100M MII connection
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> When in SGMII mode, the hardware can be configured to use either 4-bit
> or 8-bit MII connection. Currently, we don't change this bit for most
> implementations with the exception of TXGBE requiring 8-bit. Move this
> decision to the creation code and act on it when configuring SGMII.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/pcs/pcs-xpcs.c | 19 +++++++++++++++----
>  drivers/net/pcs/pcs-xpcs.h |  8 ++++++++
>  2 files changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 1faa37f0e7b9..12a3d5a80b45 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -695,9 +695,18 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs
> *xpcs,
>         val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
>                          DW_VR_MII_PCS_MODE_C37_SGMII);
> 
> -       if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
> -               mask |= DW_VR_MII_AN_CTRL_8BIT;
> +       switch (xpcs->sgmii_10_100_8bit) {
> +       case DW_XPCS_SGMII_10_100_8BIT:
>                 val |= DW_VR_MII_AN_CTRL_8BIT;
> +               fallthrough;
> +       case DW_XPCS_SGMII_10_100_4BIT:
> +               mask |= DW_VR_MII_AN_CTRL_8BIT;
> +               fallthrough;
> +       case DW_XPCS_SGMII_10_100_UNCHANGED:
> +               break;
> +       }
> +
> +       if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
>                 /* Hardware requires it to be PHY side SGMII */
>                 tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
>         } else {
> @@ -1450,10 +1459,12 @@ static struct dw_xpcs *xpcs_create(struct mdio_device
> *mdiodev)
> 
>         xpcs_get_interfaces(xpcs, xpcs->pcs.supported_interfaces);
> 
> -       if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
> +       if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
>                 xpcs->pcs.poll = false;
> -       else
> +               xpcs->sgmii_10_100_8bit = DW_XPCS_SGMII_10_100_8BIT;
> +       } else {
>                 xpcs->need_reset = true;
> +       }
> 
>         return xpcs;
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index adc5a0b3c883..4d53ccf917f3 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -114,6 +114,12 @@ enum dw_xpcs_clock {
>         DW_XPCS_NUM_CLKS,
>  };
> 
> +enum dw_xpcs_sgmii_10_100 {
> +       DW_XPCS_SGMII_10_100_UNCHANGED,
> +       DW_XPCS_SGMII_10_100_4BIT,
> +       DW_XPCS_SGMII_10_100_8BIT
> +};
> +
>  struct dw_xpcs {
>         struct dw_xpcs_info info;
>         const struct dw_xpcs_desc *desc;
> @@ -122,6 +128,8 @@ struct dw_xpcs {
>         struct phylink_pcs pcs;
>         phy_interface_t interface;
>         bool need_reset;
> +       /* Width of the MII MAC/XPCS interface in 100M and 10M modes */
> +       enum dw_xpcs_sgmii_10_100 sgmii_10_100_8bit;
>  };
> 
>  int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg);
> --

Tested-by: Tristram Ha <tristram.ha@microchip.com>

RE: [PATCH RFC net-next 2/4] net: xpcs: add SGMII mode setting

[Tristram.Ha]

> -----Original Message-----
> From: Russell King <rmk@armlinux.org.uk> On Behalf Of Russell King (Oracle)
> Sent: Wednesday, February 5, 2025 5:28 AM
> To: Tristram Ha - C24268 <Tristram.Ha@microchip.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Woojung Huh - C21699
> <Woojung.Huh@microchip.com>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; netdev@vger.kernel.org
> Subject: [PATCH RFC net-next 2/4] net: xpcs: add SGMII mode setting
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> Add SGMII mode setting which configures whether XPCS immitates the MAC
> end of the link or the PHY end, and in the latter case, where the data
> for generating the link's configuration word comes from. This ties up
> all the register bits necessary to configure this mode into one
> control.
> 
> Set this to PHY_HW mode for TXGBE.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/pcs/pcs-xpcs.c | 19 +++++++++++--------
>  drivers/net/pcs/pcs-xpcs.h | 14 ++++++++++++++
>  2 files changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 12a3d5a80b45..9d54c04ef6ee 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -706,12 +706,10 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs
> *xpcs,
>                 break;
>         }
> 
> -       if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
> -               /* Hardware requires it to be PHY side SGMII */
> -               tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
> -       } else {
> +       if (xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC)
>                 tx_conf = DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII;
> -       }
> +       else
> +               tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
> 
>         val |= FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK, tx_conf);
> 
> @@ -722,12 +720,16 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs
> *xpcs,
>         val = 0;
>         mask = DW_VR_MII_DIG_CTRL1_2G5_EN |
> DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
> 
> -       if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> -               val = DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
> +       switch (xpcs->sgmii_mode) {
> +       case DW_XPCS_SGMII_MODE_MAC:
> +               if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +                       val = DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
> +               break;
> 
> -       if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
> +       case DW_XPCS_SGMII_MODE_PHY_HW:
>                 mask |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
>                 val |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
> +               break;
>         }
> 
>         ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, mask,
> val);
> @@ -1462,6 +1464,7 @@ static struct dw_xpcs *xpcs_create(struct mdio_device
> *mdiodev)
>         if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
>                 xpcs->pcs.poll = false;
>                 xpcs->sgmii_10_100_8bit = DW_XPCS_SGMII_10_100_8BIT;
> +               xpcs->sgmii_mode = DW_XPCS_SGMII_MODE_PHY_HW;
>         } else {
>                 xpcs->need_reset = true;
>         }
> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index 4d53ccf917f3..892b85425787 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -120,6 +120,19 @@ enum dw_xpcs_sgmii_10_100 {
>         DW_XPCS_SGMII_10_100_8BIT
>  };
> 
> +/* The SGMII mode:
> + * DW_XPCS_SGMII_MODE_MAC: the XPCS acts as a MAC, reading and
> acknowledging
> + * the config word.
> + *
> + * DW_XPCS_SGMII_MODE_PHY_HW: the XPCS acts as a PHY, deriving the tx_config
> + * bits 15 (link), 12 (duplex) and 11:10 (speed) from hardware inputs to the
> + * XPCS.
> + */
> +enum dw_xpcs_sgmii_mode {
> +       DW_XPCS_SGMII_MODE_MAC,         /* XPCS is MAC on SGMII */
> +       DW_XPCS_SGMII_MODE_PHY_HW,      /* XPCS is PHY, tx_config from hw */
> +};
> +
>  struct dw_xpcs {
>         struct dw_xpcs_info info;
>         const struct dw_xpcs_desc *desc;
> @@ -130,6 +143,7 @@ struct dw_xpcs {
>         bool need_reset;
>         /* Width of the MII MAC/XPCS interface in 100M and 10M modes */
>         enum dw_xpcs_sgmii_10_100 sgmii_10_100_8bit;
> +       enum dw_xpcs_sgmii_mode sgmii_mode;
>  };
> 
>  int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg);
> --

Tested-by: Tristram Ha <tristram.ha@microchip.com>

RE: [PATCH RFC net-next 3/4] net: xpcs: add SGMII MAC manual update mode

[Tristram.Ha]

> -----Original Message-----
> From: Russell King <rmk@armlinux.org.uk> On Behalf Of Russell King (Oracle)
> Sent: Wednesday, February 5, 2025 5:28 AM
> To: Tristram Ha - C24268 <Tristram.Ha@microchip.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Woojung Huh - C21699
> <Woojung.Huh@microchip.com>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; netdev@vger.kernel.org
> Subject: [PATCH RFC net-next 3/4] net: xpcs: add SGMII MAC manual update mode
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> Older revisions of the XPCS IP do not support the MAC_AUTO_SW flag and
> need the BMCR register updated with the speed information from the PHY.
> Split the DW_XPCS_SGMII_MODE_MAC mode into _AUTO and _MANUAL variants,
> where _AUTO mode means the update happens in hardware autonomously,
> whereas the _MANUAL mode means that we need to update the BMCR register
> when the link comes up.
> 
> This will be required for the older XPCS IP found in KSZ9477.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
> This needs further input from Tristram Ha / Microchip to work out a way
> to detect KSZ9477 and set DW_XPCS_SGMII_MODE_MAC_MANUAL. On its own,
> this patch does nothing.
> ---
>  drivers/net/pcs/pcs-xpcs.c | 19 +++++++++++++------
>  drivers/net/pcs/pcs-xpcs.h | 11 ++++++++---
>  2 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 9d54c04ef6ee..1eba0c583f16 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -706,7 +706,8 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
>                 break;
>         }
> 
> -       if (xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC)
> +       if (xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC_AUTO ||
> +           xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC_MANUAL)
>                 tx_conf = DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII;
>         else
>                 tx_conf = DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII;
> @@ -721,11 +722,14 @@ static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs
> *xpcs,
>         mask = DW_VR_MII_DIG_CTRL1_2G5_EN |
> DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
> 
>         switch (xpcs->sgmii_mode) {
> -       case DW_XPCS_SGMII_MODE_MAC:
> +       case DW_XPCS_SGMII_MODE_MAC_AUTO:
>                 if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
>                         val = DW_VR_MII_DIG_CTRL1_MAC_AUTO_SW;
>                 break;
> 
> +       case DW_XPCS_SGMII_MODE_MAC_MANUAL:
> +               break;
> +
>         case DW_XPCS_SGMII_MODE_PHY_HW:
>                 mask |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
>                 val |= DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL;
> @@ -1151,7 +1155,9 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs
> *xpcs,
>  {
>         int ret;
> 
> -       if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED)
> +       if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED &&
> +           !(interface == PHY_INTERFACE_MODE_SGMII &&
> +             xpcs->sgmii_mode == DW_XPCS_SGMII_MODE_MAC_MANUAL))
>                 return;
> 
>         if (interface == PHY_INTERFACE_MODE_1000BASEX) {
> @@ -1168,10 +1174,11 @@ static void xpcs_link_up_sgmii_1000basex(struct
> dw_xpcs *xpcs,
>                                 __func__);
>         }
> 
> -       ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> -                        mii_bmcr_encode_fixed(speed, duplex));
> +       ret = xpcs_modify(xpcs, MDIO_MMD_VEND2, MII_BMCR,
> +                         BMCR_SPEED1000 | BMCR_FULLDPLX | BMCR_SPEED100,
> +                         mii_bmcr_encode_fixed(speed, duplex));
>         if (ret)
> -               dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
> +               dev_err(&xpcs->mdiodev->dev, "%s: xpcs_modify returned %pe\n",
>                         __func__, ERR_PTR(ret));
>  }
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index 892b85425787..96117bd9e2b6 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -121,15 +121,20 @@ enum dw_xpcs_sgmii_10_100 {
>  };
> 
>  /* The SGMII mode:
> - * DW_XPCS_SGMII_MODE_MAC: the XPCS acts as a MAC, reading and
> acknowledging
> - * the config word.
> + * DW_XPCS_SGMII_MODE_MAC_AUTO: the XPCS acts as a MAC, accepting the
> + * parameters from the PHY end of the SGMII link and acknowledging the
> + * config word. The XPCS autonomously switches speed.
> + *
> + * DW_XPCS_SGMII_MODE_MAC_MANUAL: the XPCS acts as a MAC as above, but
> + * does not autonomously switch speed.
>   *
>   * DW_XPCS_SGMII_MODE_PHY_HW: the XPCS acts as a PHY, deriving the tx_config
>   * bits 15 (link), 12 (duplex) and 11:10 (speed) from hardware inputs to the
>   * XPCS.
>   */
>  enum dw_xpcs_sgmii_mode {
> -       DW_XPCS_SGMII_MODE_MAC,         /* XPCS is MAC on SGMII */
> +       DW_XPCS_SGMII_MODE_MAC_AUTO,    /* XPCS is MAC, auto update */
> +       DW_XPCS_SGMII_MODE_MAC_MANUAL,  /* XPCS is MAC, manual update */
>         DW_XPCS_SGMII_MODE_PHY_HW,      /* XPCS is PHY, tx_config from hw */
>  };
> 
> --

Tested-by: Tristram Ha <tristram.ha@microchip.com>

RE: [PATCH RFC net-next 4/4] net: xpcs: allow 1000BASE-X to work with older XPCS IP

[Tristram.Ha]

> -----Original Message-----
> From: Russell King <rmk@armlinux.org.uk> On Behalf Of Russell King (Oracle)
> Sent: Wednesday, February 5, 2025 5:28 AM
> To: Tristram Ha - C24268 <Tristram.Ha@microchip.com>
> Cc: Vladimir Oltean <olteanv@gmail.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; Woojung Huh - C21699
> <Woojung.Huh@microchip.com>; Andrew Lunn <andrew@lunn.ch>; Heiner Kallweit
> <hkallweit1@gmail.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo Abeni
> <pabeni@redhat.com>; netdev@vger.kernel.org
> Subject: [PATCH RFC net-next 4/4] net: xpcs: allow 1000BASE-X to work with older
> XPCS IP
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> Older XPCS IP requires SGMII_LINK and PHY_SIDE_SGMII to be set when
> operating in 1000BASE-X mode even though the XPCS is not configured for
> SGMII. An example of a device with older XPCS IP is KSZ9477.
> 
> We already don't clear these bits if we switch from SGMII to 1000BASE-X
> on TXGBE - which would result in 1000BASE-X with the PHY_SIDE_SGMII bit
> left set.
> 
> It is currently believed to be safe to set both bits on newer IP
> without side-effects.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/pcs/pcs-xpcs.c | 13 +++++++++++--
>  drivers/net/pcs/pcs-xpcs.h |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 1eba0c583f16..d522e4a5a138 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -774,9 +774,18 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs
> *xpcs,
>                         return ret;
>         }
> 
> -       mask = DW_VR_MII_PCS_MODE_MASK;
> +       /* Older XPCS IP requires PHY_MODE (bit 3) and SGMII_LINK (but 4) to
> +        * be set when operating in 1000BASE-X mode. See page 233
> +        *
> https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDo
> cuments/DataSheets/KSZ9477S-Data-Sheet-DS00002392C.pdf
> +        * "5.5.9 SGMII AUTO-NEGOTIATION CONTROL REGISTER"
> +        */
> +       mask = DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_AN_CTRL_SGMII_LINK |
> +              DW_VR_MII_TX_CONFIG_MASK;
>         val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
> -                        DW_VR_MII_PCS_MODE_C37_1000BASEX);
> +                        DW_VR_MII_PCS_MODE_C37_1000BASEX) |
> +             FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK,
> +                        DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII) |
> +             DW_VR_MII_AN_CTRL_SGMII_LINK;
> 
>         if (!xpcs->pcs.poll) {
>                 mask |= DW_VR_MII_AN_INTR_EN;
> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index 96117bd9e2b6..f0ddd93c7a22 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -73,6 +73,7 @@
> 
>  /* VR_MII_AN_CTRL */
>  #define DW_VR_MII_AN_CTRL_8BIT                 BIT(8)
> +#define DW_VR_MII_AN_CTRL_SGMII_LINK           BIT(4)
>  #define DW_VR_MII_TX_CONFIG_MASK               BIT(3)
>  #define DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII     0x1
>  #define DW_VR_MII_TX_CONFIG_MAC_SIDE_SGMII     0x0
> --

Tested-by: Tristram Ha <tristram.ha@microchip.com>

Re: [PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for KSZ9477

[Russell King (Oracle)]

On Wed, Feb 05, 2025 at 01:27:26PM +0000, Russell King (Oracle) wrote:
> Work for Microchip to do before this series can be merged:
> 
> 1. work out how to identify their XPCS integration from other
>    integrations, so allowing MAC_MANUAL SGMII mode to be selected.

This is now complete.

> 2. verify where the requirement for setting the two bits for 1000BASE-X
>    has come from (from what Jose has said, we don't believe it's from
>    Synopsys.)

I believe this is still outstanding - and is a question that Vladimir
asked of you quite a while ago. What is the status of this?

Until this is answered, we can't move forward with these patches
unless Vladimir is now happy to accept them given Jose's response.
Vladimir seemed to be quite adamant that this needed to be answered.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 4/4] net: xpcs: allow 1000BASE-X to work with older XPCS IP

[Vladimir Oltean]

On Wed, Feb 05, 2025 at 01:27:47PM +0000, Russell King (Oracle) wrote:
> Older XPCS IP requires SGMII_LINK and PHY_SIDE_SGMII to be set when
> operating in 1000BASE-X mode even though the XPCS is not configured for
> SGMII. An example of a device with older XPCS IP is KSZ9477.
> 
> We already don't clear these bits if we switch from SGMII to 1000BASE-X
> on TXGBE - which would result in 1000BASE-X with the PHY_SIDE_SGMII bit
> left set.

Is there a confirmation written down somewhere that a transition from
SGMII to 1000Base-X was explicitly tested? I have to remain a bit
skeptical and say that although the code is indeed like this, it
doesn't mean by itself there are no unintended side effects.

> It is currently believed to be safe to set both bits on newer IP
> without side-effects.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/pcs/pcs-xpcs.c | 13 +++++++++++--
>  drivers/net/pcs/pcs-xpcs.h |  1 +
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 1eba0c583f16..d522e4a5a138 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -774,9 +774,18 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
>  			return ret;
>  	}
>  
> -	mask = DW_VR_MII_PCS_MODE_MASK;
> +	/* Older XPCS IP requires PHY_MODE (bit 3) and SGMII_LINK (but 4) to
                                                                   ~~~
                                                                   bit

> +	 * be set when operating in 1000BASE-X mode. See page 233
> +	 * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9477S-Data-Sheet-DS00002392C.pdf
> +	 * "5.5.9 SGMII AUTO-NEGOTIATION CONTROL REGISTER"
> +	 */
> +	mask = DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_AN_CTRL_SGMII_LINK |
> +	       DW_VR_MII_TX_CONFIG_MASK;
>  	val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
> -			 DW_VR_MII_PCS_MODE_C37_1000BASEX);
> +			 DW_VR_MII_PCS_MODE_C37_1000BASEX) |
> +	      FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK,
> +			 DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII) |
> +	      DW_VR_MII_AN_CTRL_SGMII_LINK;
>  
>  	if (!xpcs->pcs.poll) {
>  		mask |= DW_VR_MII_AN_INTR_EN;

I do believe that this is the kind of patch one would write when the
hardware is completely a black box. But when we have Microchip engineers
here with a channel open towards their hardware design who can help
clarify where the requirement comes from, that just isn't the case.
So I wouldn't rush with this.

Plus, it isn't even the most conservative way in which a (supposedly)
integration-specific requirement is fulfilled in the common Synopsys
driver. If one integration makes vendor-specific choices about these
bits, I wouldn't assume that no other vendors made contradictory choices.

I don't want to say too much before Tristram comes with a statement from
Microchip hardware design, but _if_ it turns out to be a KSZ9477
specific requirement, it still seems safer to only enable this based
(at least) on Tristram's MICROCHIP_KSZ9477_PMA_ID conditional from his
other patch set, if not based on something stronger (a conditional
describing some functional behavior, rather than a specific hardware IP).

Re: [PATCH RFC net-next 4/4] net: xpcs: allow 1000BASE-X to work with older XPCS IP

[Russell King (Oracle)]

On Mon, Feb 10, 2025 at 01:05:55PM +0200, Vladimir Oltean wrote:
> On Wed, Feb 05, 2025 at 01:27:47PM +0000, Russell King (Oracle) wrote:
> > Older XPCS IP requires SGMII_LINK and PHY_SIDE_SGMII to be set when
> > operating in 1000BASE-X mode even though the XPCS is not configured for
> > SGMII. An example of a device with older XPCS IP is KSZ9477.
> > 
> > We already don't clear these bits if we switch from SGMII to 1000BASE-X
> > on TXGBE - which would result in 1000BASE-X with the PHY_SIDE_SGMII bit
> > left set.
> 
> Is there a confirmation written down somewhere that a transition from
> SGMII to 1000Base-X was explicitly tested? I have to remain a bit
> skeptical and say that although the code is indeed like this, it
> doesn't mean by itself there are no unintended side effects.
> 
> > It is currently believed to be safe to set both bits on newer IP
> > without side-effects.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/net/pcs/pcs-xpcs.c | 13 +++++++++++--
> >  drivers/net/pcs/pcs-xpcs.h |  1 +
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index 1eba0c583f16..d522e4a5a138 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -774,9 +774,18 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
> >  			return ret;
> >  	}
> >  
> > -	mask = DW_VR_MII_PCS_MODE_MASK;
> > +	/* Older XPCS IP requires PHY_MODE (bit 3) and SGMII_LINK (but 4) to
>                                                                    ~~~
>                                                                    bit
> 
> > +	 * be set when operating in 1000BASE-X mode. See page 233
> > +	 * https://ww1.microchip.com/downloads/aemDocuments/documents/OTH/ProductDocuments/DataSheets/KSZ9477S-Data-Sheet-DS00002392C.pdf
> > +	 * "5.5.9 SGMII AUTO-NEGOTIATION CONTROL REGISTER"
> > +	 */
> > +	mask = DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_AN_CTRL_SGMII_LINK |
> > +	       DW_VR_MII_TX_CONFIG_MASK;
> >  	val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
> > -			 DW_VR_MII_PCS_MODE_C37_1000BASEX);
> > +			 DW_VR_MII_PCS_MODE_C37_1000BASEX) |
> > +	      FIELD_PREP(DW_VR_MII_TX_CONFIG_MASK,
> > +			 DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII) |
> > +	      DW_VR_MII_AN_CTRL_SGMII_LINK;
> >  
> >  	if (!xpcs->pcs.poll) {
> >  		mask |= DW_VR_MII_AN_INTR_EN;
> 
> I do believe that this is the kind of patch one would write when the
> hardware is completely a black box. But when we have Microchip engineers
> here with a channel open towards their hardware design who can help
> clarify where the requirement comes from, that just isn't the case.
> So I wouldn't rush with this.
> 
> Plus, it isn't even the most conservative way in which a (supposedly)
> integration-specific requirement is fulfilled in the common Synopsys
> driver. If one integration makes vendor-specific choices about these
> bits, I wouldn't assume that no other vendors made contradictory choices.
> 
> I don't want to say too much before Tristram comes with a statement from
> Microchip hardware design, but _if_ it turns out to be a KSZ9477
> specific requirement, it still seems safer to only enable this based
> (at least) on Tristram's MICROCHIP_KSZ9477_PMA_ID conditional from his
> other patch set, if not based on something stronger (a conditional
> describing some functional behavior, rather than a specific hardware IP).

So Jose's public reassurance means nothing?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH RFC net-next 4/4] net: xpcs: allow 1000BASE-X to work with older XPCS IP

[Vladimir Oltean]

On Mon, Feb 10, 2025 at 11:49:21AM +0000, Russell King (Oracle) wrote:
> On Mon, Feb 10, 2025 at 01:05:55PM +0200, Vladimir Oltean wrote:
> > I do believe that this is the kind of patch one would write when the
> > hardware is completely a black box. But when we have Microchip engineers
> > here with a channel open towards their hardware design who can help
> > clarify where the requirement comes from, that just isn't the case.
> > So I wouldn't rush with this.
> > 
> > Plus, it isn't even the most conservative way in which a (supposedly)
> > integration-specific requirement is fulfilled in the common Synopsys
> > driver. If one integration makes vendor-specific choices about these
> > bits, I wouldn't assume that no other vendors made contradictory choices.
> > 
> > I don't want to say too much before Tristram comes with a statement from
> > Microchip hardware design, but _if_ it turns out to be a KSZ9477
> > specific requirement, it still seems safer to only enable this based
> > (at least) on Tristram's MICROCHIP_KSZ9477_PMA_ID conditional from his
> > other patch set, if not based on something stronger (a conditional
> > describing some functional behavior, rather than a specific hardware IP).
> 
> So Jose's public reassurance means nothing?

[ for context to all readers, _this_ public reassurance:
  https://lore.kernel.org/netdev/DM4PR12MB5088BA650B164D5CEC33CA08D3E82@DM4PR12MB5088.namprd12.prod.outlook.com/ ]

Yup, this is what I'm saying. He basically said that it's outside of
Synopsys control how these bits are used in the final integration.
And if so, it's also naturally outside of vendor X's (Microchip) control
how vendor Y adds integration-specific logic to Synopsys-undefined bits
for 1000Base-X mode. Thus, the only thing I'm saying is that it isn't
the safest thing we can do, in Linux, to assume that no other integration
has added a contradictory vendor-defined behavior for these bits.
It's an assumption we aren't even _forced_ to make, so why risk it?

RE: [PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for KSZ9477

[Tristram.Ha]

> Subject: Re: [PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for
> KSZ9477
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> On Wed, Feb 05, 2025 at 01:27:26PM +0000, Russell King (Oracle) wrote:
> > Work for Microchip to do before this series can be merged:
> >
> > 1. work out how to identify their XPCS integration from other
> >    integrations, so allowing MAC_MANUAL SGMII mode to be selected.
> 
> This is now complete.
> 
> > 2. verify where the requirement for setting the two bits for 1000BASE-X
> >    has come from (from what Jose has said, we don't believe it's from
> >    Synopsys.)
> 
> I believe this is still outstanding - and is a question that Vladimir
> asked of you quite a while ago. What is the status of this?
> 
> Until this is answered, we can't move forward with these patches
> unless Vladimir is now happy to accept them given Jose's response.
> Vladimir seemed to be quite adamant that this needed to be answered.

Sorry for the long delay.  After discussing with the Microchip design
team we come up with this explanation for setting the
DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII (bit 3) of DW_VR_MII_AN_CTRL (0x8001)
register in 1000Base-X mode to make it work with AN on.

KSZ9477 has the older version of 1000Base-X Synopsys IP which works in
1000Base-X mode without AN.  When AN is on the port does not pass traffic
because it does not detect a link.  Setting
DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII allows the link to be turned on by
either setting DW_VR_MII_SGMII_LINK_STS (bit 4) or
DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL (bit 0) in DW_VR_MII_DIG_CTRL1 (0x8000)
register.  After that the port can pass traffic.

This is still a specific KSZ9477 problem so it may be safer to put this
code under "if (xpcs->info.pma == MICROCHIP_KSZ9477_PMD_ID)" check.

Re: [PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for KSZ9477

[Vladimir Oltean]

Hi Tristram,

On Tue, Mar 18, 2025 at 07:59:07PM +0000, Tristram.Ha@microchip.com wrote:
> Sorry for the long delay.  After discussing with the Microchip design
> team we come up with this explanation for setting the
> DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII (bit 3) of DW_VR_MII_AN_CTRL (0x8001)
> register in 1000Base-X mode to make it work with AN on.
> 
> KSZ9477 has the older version of 1000Base-X Synopsys IP which works in
> 1000Base-X mode without AN.

I was unaware of the existence of such Synopsys IP. In which relevant
way is it "older"? Does it specifically claim it supports 1000Base-X,
but only without AN? Or if not, is it an SGMII-only base IP, then? The
two are compatible when AN is disabled and the SGMII IP is configured
for 1Gbps, and can be mistaken for one another.

You're making it sound as if "1000Base-X" support was patched by the
Microchip integration over a base PCS IP which did not support it.

> When AN is on the port does not pass traffic because it does not
> detect a link.

Which port does not detect a link? The KSZ9477 port or its link partner?

> Setting DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII allows the link to be
> turned on by either setting DW_VR_MII_SGMII_LINK_STS (bit 4) or
> DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL (bit 0) in DW_VR_MII_DIG_CTRL1
> (0x8000) register.  After that the port can pass traffic.

Can you comment on whether Microchip has given these bits some
integration-specific meaning, or whether their meaning from the base IP,
summarized by me in this table taken from the XPCS data book, has been
preserved unchanged?
https://lore.kernel.org/netdev/20250128152324.3p2ccnxoz5xta7ct@skbuf/

So far, the only noted fact is that they take effect also for
PCS_MODE=0b00 (1000Base-X), and not just for PCS_MODE=0b10 (SGMII),
as originally intended in the base IP. But we don't exactly know what
they do. Just that the Microchip IP wants them set to one.

If their meaning is otherwise the same, all data available to me points
to the conclusion that the "1000Base-X with autoneg on" mode in KSZ9477
is actually SGMII with a config word customized by software, and with
some conditions commented out from the base IP, to allow the code word
to be customizable even in PCS_MODE=0b00.

If the above conclusion is true, I think we need to pay more attention
at whether the transmitted config word is truly 1000Base-X compatible,
as Russell pointed out here:
https://lore.kernel.org/netdev/Z5iiXWkhm2OvbjOx@shell.armlinux.org.uk/#t
The discussion there got pretty involved and branched out into other
topics, but I couldn't find a response from you on that specific second
paragraph.

> This is still a specific KSZ9477 problem so it may be safer to put this
> code under "if (xpcs->info.pma == MICROCHIP_KSZ9477_PMD_ID)" check.

If the meaning of these fields was not kept 100% intact by Microchip
during their integration, including the context in which they are valid,
then I 100% agree. But I would still like an answer to the questions above.

RE: [PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for KSZ9477

[Tristram.Ha]

> Subject: Re: [PATCH RFC net-next 0/4] net: xpcs: cleanups and partial support for
> KSZ9477
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content
> is safe
> 
> Hi Tristram,
> 
> On Tue, Mar 18, 2025 at 07:59:07PM +0000, Tristram.Ha@microchip.com wrote:
> > Sorry for the long delay.  After discussing with the Microchip design
> > team we come up with this explanation for setting the
> > DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII (bit 3) of DW_VR_MII_AN_CTRL
> (0x8001)
> > register in 1000Base-X mode to make it work with AN on.
> >
> > KSZ9477 has the older version of 1000Base-X Synopsys IP which works in
> > 1000Base-X mode without AN.
> 
> I was unaware of the existence of such Synopsys IP. In which relevant
> way is it "older"? Does it specifically claim it supports 1000Base-X,
> but only without AN? Or if not, is it an SGMII-only base IP, then? The
> two are compatible when AN is disabled and the SGMII IP is configured
> for 1Gbps, and can be mistaken for one another.
> 
> You're making it sound as if "1000Base-X" support was patched by the
> Microchip integration over a base PCS IP which did not support it.

The 1000Base-X config word is sent correctly by KSZ9477 and link partner
can receive it.  This is verified by changing the register MII_ADVERTISE
and observing the link partner's MII_LPA register for the same value with
additional bit 0x4000 set.

The KSZ9477 1000Base-X AN is broken and requires the workaround mentioned
in the app note.  Mainly the MII_ADVERTISE register needs to be written
once after PCS mode is changed before restarting the auto-negotiation for
the config word to be sent correctly.

> > When AN is on the port does not pass traffic because it does not
> > detect a link.
> 
> Which port does not detect a link? The KSZ9477 port or its link partner?

The link detection issue is for the local port device.  The link can be
detected properly, and the interrupt can even signal the link up is
completed.  It is just the port will not forward traffic as the link
status is not passed to the local device.  Setting the 2 bits mentioned
before allows the port to know about the link.  This is just a side
effect and is not an intended design as it is really the hardware's
problem not to pass the link status to the local device.

> > Setting DW_VR_MII_TX_CONFIG_PHY_SIDE_SGMII allows the link to be
> > turned on by either setting DW_VR_MII_SGMII_LINK_STS (bit 4) or
> > DW_VR_MII_DIG_CTRL1_PHY_MODE_CTRL (bit 0) in DW_VR_MII_DIG_CTRL1
> > (0x8000) register.  After that the port can pass traffic.
> 
> Can you comment on whether Microchip has given these bits some
> integration-specific meaning, or whether their meaning from the base IP,
> summarized by me in this table taken from the XPCS data book, has been
> preserved unchanged?
> https://lore.kernel.org/netdev/20250128152324.3p2ccnxoz5xta7ct@skbuf/
> 
> So far, the only noted fact is that they take effect also for
> PCS_MODE=0b00 (1000Base-X), and not just for PCS_MODE=0b10 (SGMII),
> as originally intended in the base IP. But we don't exactly know what
> they do. Just that the Microchip IP wants them set to one.
> 
> If their meaning is otherwise the same, all data available to me points
> to the conclusion that the "1000Base-X with autoneg on" mode in KSZ9477
> is actually SGMII with a config word customized by software, and with
> some conditions commented out from the base IP, to allow the code word
> to be customizable even in PCS_MODE=0b00.
> 
> If the above conclusion is true, I think we need to pay more attention
> at whether the transmitted config word is truly 1000Base-X compatible,
> as Russell pointed out here:
> https://lore.kernel.org/netdev/Z5iiXWkhm2OvbjOx@shell.armlinux.org.uk/#t
> The discussion there got pretty involved and branched out into other
> topics, but I couldn't find a response from you on that specific second
> paragraph.
> 
> > This is still a specific KSZ9477 problem so it may be safer to put this
> > code under "if (xpcs->info.pma == MICROCHIP_KSZ9477_PMD_ID)" check.
> 
> If the meaning of these fields was not kept 100% intact by Microchip
> during their integration, including the context in which they are valid,
> then I 100% agree. But I would still like an answer to the questions above.

Since this is a KSZ9477 specific problem is it okay to just program those
bits inside the KSZ9477 DSA driver when the PCS mode is changed from
SGMII to 1000Base-X?  Then the XPCS driver does not need to concern with
this code change.  If a SFP does not work then it is the fault of KSZ9477
hardware and its driver.