[net-next PATCH v4 0/4] Add support for XPCS based RSFEC configuration

[net-next PATCH v4 0/4] Add support for XPCS based RSFEC configuration

[Alexander Duyck]

This series adds to xpcs to add the necessary configuration steps
for the DW PCS/rsfec for high speed interfaces.

I picked up the set from Mike Marciniszyn who is no longer with Meta. His
set ending with v3 can be found here:
https://lore.kernel.org/netdev/20260527000958.81108-1-mike.marciniszyn@gmail.com/

v4 - Address AI review comments
   - Simplify the set by just using separate PMA1 instead of scanning for it
v3 - adds an alternate xpcs creation that allows for specifying multiple addr
     values for creating xpcs
   - remove probing in favor of new xpcs create call
v2 - probe mdio bus and use phy_find* to isolate mdio address
v1 - initial version

---

Mike Marciniszyn (Meta) (4):
      net: pcs: xpcs: Expand mdiodev member as an array
      net: pcs: xpcs: Allow for multiple addr in creation
      net: pcs: xpcs: Add hooks for xpcs configuration of rsfec
      net: pcs: xpcs: Add handling for 4 channel rsfec device


 .../net/ethernet/meta/fbnic/fbnic_phylink.c   |   4 +-
 drivers/net/pcs/pcs-xpcs-wx.c                 |   4 +-
 drivers/net/pcs/pcs-xpcs.c                    | 304 +++++++++++++++---
 drivers/net/pcs/pcs-xpcs.h                    |  37 ++-
 include/linux/pcs/pcs-xpcs.h                  |   2 +
 include/uapi/linux/mdio.h                     |   3 +
 6 files changed, 308 insertions(+), 46 deletions(-)

--

[net-next PATCH v4 1/4] net: pcs: xpcs: Expand mdiodev member as an array

[Alexander Duyck]

From: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>

This allows for support of 50GR2 and 100GR2 by extending the
current singleton mdiodev to an array.

DW XPCS can support 25GR1 and 50GR1 using just a single group channel. To
operate at 50R2 or 100R2 we need both halves of the channel combined into
one group channel device.

Adjust current use of singleton mdiodev to use the 0th element.

Signed-off-by: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/pcs/pcs-xpcs-wx.c |    4 ++--
 drivers/net/pcs/pcs-xpcs.c    |   43 +++++++++++++++++++++--------------------
 drivers/net/pcs/pcs-xpcs.h    |    5 ++++-
 3 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs-wx.c b/drivers/net/pcs/pcs-xpcs-wx.c
index fc52f7aa5f59..38c980f974b4 100644
--- a/drivers/net/pcs/pcs-xpcs-wx.c
+++ b/drivers/net/pcs/pcs-xpcs-wx.c
@@ -123,7 +123,7 @@ static int txgbe_pcs_poll_power_up(struct dw_xpcs *xpcs)
 				10000, 1000000, false,
 				xpcs, DW_VR_XS_PCS_DIG_STS);
 	if (ret < 0)
-		dev_err(&xpcs->mdiodev->dev, "xpcs power-up timeout\n");
+		dev_err(&xpcs->mdiodev[0]->dev, "xpcs power-up timeout\n");
 
 	return ret;
 }
@@ -139,7 +139,7 @@ static int txgbe_pma_init_done(struct dw_xpcs *xpcs)
 				100000, 10000000, false,
 				xpcs, DW_VR_XS_PCS_DIG_CTRL1);
 	if (ret < 0)
-		dev_err(&xpcs->mdiodev->dev, "xpcs pma initialization timeout\n");
+		dev_err(&xpcs->mdiodev[0]->dev, "xpcs pma initialization timeout\n");
 
 	return ret;
 }
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index e69fa2f0a0e8..22246abec948 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -211,23 +211,24 @@ static bool __xpcs_linkmode_supported(const struct dw_xpcs_compat *compat,
 
 int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg)
 {
-	return mdiodev_c45_read(xpcs->mdiodev, dev, reg);
+	return mdiodev_c45_read(xpcs->mdiodev[0], dev, reg);
 }
 
 int xpcs_write(struct dw_xpcs *xpcs, int dev, u32 reg, u16 val)
 {
-	return mdiodev_c45_write(xpcs->mdiodev, dev, reg, val);
+	return mdiodev_c45_write(xpcs->mdiodev[0], dev, reg, val);
 }
 
 int xpcs_modify(struct dw_xpcs *xpcs, int dev, u32 reg, u16 mask, u16 set)
 {
-	return mdiodev_c45_modify(xpcs->mdiodev, dev, reg, mask, set);
+	return mdiodev_c45_modify(xpcs->mdiodev[0], dev, reg, mask, set);
 }
 
 static int xpcs_modify_changed(struct dw_xpcs *xpcs, int dev, u32 reg,
 			       u16 mask, u16 set)
 {
-	return mdiodev_c45_modify_changed(xpcs->mdiodev, dev, reg, mask, set);
+	return mdiodev_c45_modify_changed(xpcs->mdiodev[0], dev, reg,
+					  mask, set);
 }
 
 static int xpcs_read_vendor(struct dw_xpcs *xpcs, int dev, u32 reg)
@@ -304,7 +305,7 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
 #define xpcs_warn(__xpcs, __state, __args...) \
 ({ \
 	if ((__state)->link) \
-		dev_warn(&(__xpcs)->mdiodev->dev, ##__args); \
+		dev_warn(&(__xpcs)->mdiodev[0]->dev, ##__args); \
 })
 
 static int xpcs_read_fault_c73(struct dw_xpcs *xpcs,
@@ -400,7 +401,7 @@ static void xpcs_link_up_usxgmii(struct dw_xpcs *xpcs, int speed)
 	return;
 
 out:
-	dev_err(&xpcs->mdiodev->dev, "%s: XPCS access returned %pe\n",
+	dev_err(&xpcs->mdiodev[0]->dev, "%s: XPCS access returned %pe\n",
 		__func__, ERR_PTR(ret));
 }
 
@@ -726,7 +727,7 @@ static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
 
 	ret = xpcs_switch_interface_mode(xpcs, interface);
 	if (ret)
-		dev_err(&xpcs->mdiodev->dev, "switch interface failed: %pe\n",
+		dev_err(&xpcs->mdiodev[0]->dev, "switch interface failed: %pe\n",
 			ERR_PTR(ret));
 
 	if (!xpcs->need_reset)
@@ -734,14 +735,14 @@ static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
 
 	compat = xpcs_find_compat(xpcs, interface);
 	if (!compat) {
-		dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n",
+		dev_err(&xpcs->mdiodev[0]->dev, "unsupported interface %s\n",
 			phy_modes(interface));
 		return;
 	}
 
 	ret = xpcs_soft_reset(xpcs, compat);
 	if (ret)
-		dev_err(&xpcs->mdiodev->dev, "soft reset failed: %pe\n",
+		dev_err(&xpcs->mdiodev[0]->dev, "soft reset failed: %pe\n",
 			ERR_PTR(ret));
 
 	xpcs->need_reset = false;
@@ -1189,30 +1190,30 @@ static void xpcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
 
 	switch (compat->an_mode) {
 	case DW_10GBASER:
-		phylink_mii_c45_pcs_get_state(xpcs->mdiodev, state);
+		phylink_mii_c45_pcs_get_state(xpcs->mdiodev[0], state);
 		break;
 	case DW_AN_C73:
 		ret = xpcs_get_state_c73(xpcs, state, compat);
 		if (ret)
-			dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
+			dev_err(&xpcs->mdiodev[0]->dev, "%s returned %pe\n",
 				"xpcs_get_state_c73", ERR_PTR(ret));
 		break;
 	case DW_AN_C37_SGMII:
 		ret = xpcs_get_state_c37_sgmii(xpcs, state);
 		if (ret)
-			dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
+			dev_err(&xpcs->mdiodev[0]->dev, "%s returned %pe\n",
 				"xpcs_get_state_c37_sgmii", ERR_PTR(ret));
 		break;
 	case DW_AN_C37_1000BASEX:
 		ret = xpcs_get_state_c37_1000basex(xpcs, neg_mode, state);
 		if (ret)
-			dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
+			dev_err(&xpcs->mdiodev[0]->dev, "%s returned %pe\n",
 				"xpcs_get_state_c37_1000basex", ERR_PTR(ret));
 		break;
 	case DW_2500BASEX:
 		ret = xpcs_get_state_2500basex(xpcs, state);
 		if (ret)
-			dev_err(&xpcs->mdiodev->dev, "%s returned %pe\n",
+			dev_err(&xpcs->mdiodev[0]->dev, "%s returned %pe\n",
 				"xpcs_get_state_2500basex", ERR_PTR(ret));
 		break;
 	default:
@@ -1232,14 +1233,14 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
 
 	if (interface == PHY_INTERFACE_MODE_1000BASEX) {
 		if (speed != SPEED_1000) {
-			dev_err(&xpcs->mdiodev->dev,
+			dev_err(&xpcs->mdiodev[0]->dev,
 				"%s: speed %dMbps not supported\n",
 				__func__, speed);
 			return;
 		}
 
 		if (duplex != DUPLEX_FULL)
-			dev_err(&xpcs->mdiodev->dev,
+			dev_err(&xpcs->mdiodev[0]->dev,
 				"%s: half duplex not supported\n",
 				__func__);
 	}
@@ -1247,7 +1248,7 @@ static void xpcs_link_up_sgmii_1000basex(struct dw_xpcs *xpcs,
 	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MII_BMCR,
 			 mii_bmcr_encode_fixed(speed, duplex));
 	if (ret)
-		dev_err(&xpcs->mdiodev->dev, "%s: xpcs_write returned %pe\n",
+		dev_err(&xpcs->mdiodev[0]->dev, "%s: xpcs_write returned %pe\n",
 			__func__, ERR_PTR(ret));
 }
 
@@ -1533,7 +1534,7 @@ static struct dw_xpcs *xpcs_create_data(struct mdio_device *mdiodev)
 		return ERR_PTR(-ENOMEM);
 
 	mdio_device_get(mdiodev);
-	xpcs->mdiodev = mdiodev;
+	xpcs->mdiodev[0] = mdiodev;
 	xpcs->pcs.ops = &xpcs_phylink_ops;
 	xpcs->pcs.poll = true;
 
@@ -1542,7 +1543,7 @@ static struct dw_xpcs *xpcs_create_data(struct mdio_device *mdiodev)
 
 static void xpcs_free_data(struct dw_xpcs *xpcs)
 {
-	mdio_device_put(xpcs->mdiodev);
+	mdio_device_put(xpcs->mdiodev[0]);
 	kfree(xpcs);
 }
 
@@ -1552,7 +1553,7 @@ static int xpcs_init_clks(struct dw_xpcs *xpcs)
 		[DW_XPCS_CORE_CLK] = "core",
 		[DW_XPCS_PAD_CLK] = "pad",
 	};
-	struct device *dev = &xpcs->mdiodev->dev;
+	struct device *dev = &xpcs->mdiodev[0]->dev;
 	int ret, i;
 
 	for (i = 0; i < DW_XPCS_NUM_CLKS; ++i)
@@ -1580,7 +1581,7 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
 {
 	const struct dw_xpcs_info *info;
 
-	info = dev_get_platdata(&xpcs->mdiodev->dev);
+	info = dev_get_platdata(&xpcs->mdiodev[0]->dev);
 	if (!info) {
 		xpcs->info.pcs = DW_XPCS_ID_NATIVE;
 		xpcs->info.pma = DW_XPCS_PMA_ID_NATIVE;
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 929fa238445e..5c804cfd47d3 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -105,10 +105,13 @@ enum dw_xpcs_clock {
 	DW_XPCS_NUM_CLKS,
 };
 
+/* 2 channels per group DW PCS devices */
+#define DW_XPCS_MAX_CHANNELS 2
+
 struct dw_xpcs {
 	struct dw_xpcs_info info;
 	const struct dw_xpcs_desc *desc;
-	struct mdio_device *mdiodev;
+	struct mdio_device *mdiodev[DW_XPCS_MAX_CHANNELS];
 	struct clk_bulk_data clks[DW_XPCS_NUM_CLKS];
 	struct phylink_pcs pcs;
 	phy_interface_t interface;

[net-next PATCH v4 2/4] net: pcs: xpcs: Allow for multiple addr in creation

[Alexander Duyck]

From: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>

The DW channel 10g-100g IP builds the PCS out of multiple addr
mdio addresses.

Add xpcs_create_mdiodevs() and xpcs_create_pcs_mdiodevs calls so that a
driver using the above part can tell xpcs the addr values in one call.

xpcs_create_data(), xpcs_create(), and xpcs_free_data() are extended
to accept a variable number of mdio devices.  Existing singleton
interfaces are re-cast as a having an array pointing to a single
mdio device.

Signed-off-by: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/ethernet/meta/fbnic/fbnic_phylink.c |    4 +
 drivers/net/pcs/pcs-xpcs.c                      |  106 ++++++++++++++++++-----
 drivers/net/pcs/pcs-xpcs.h                      |    1 
 include/linux/pcs/pcs-xpcs.h                    |    2 
 4 files changed, 88 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
index 09c5225111be..88fc78d55c7a 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
@@ -200,9 +200,11 @@ int fbnic_phylink_create(struct net_device *netdev)
 	struct fbnic_dev *fbd = fbn->fbd;
 	struct phylink_pcs *pcs;
 	struct phylink *phylink;
+	/* See fbnic_mdio.c addr usage */
+	int addrs[] = { 0, 1 };
 	int err;
 
-	pcs = xpcs_create_pcs_mdiodev(fbd->mdio_bus, 0);
+	pcs = xpcs_create_pcs_mdiodevs(fbd->mdio_bus, addrs, ARRAY_SIZE(addrs));
 	if (IS_ERR(pcs)) {
 		err = PTR_ERR(pcs);
 		dev_err(fbd->dev, "Failed to create PCS device: %d\n", err);
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 22246abec948..2e236ff1ad85 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -214,9 +214,18 @@ int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg)
 	return mdiodev_c45_read(xpcs->mdiodev[0], dev, reg);
 }
 
+static int
+xpcs_mdev_write_ch(struct dw_xpcs *xpcs, int ch, int dev, u32 reg, u16 val)
+{
+	if (ch >= xpcs->channels)
+		return -ENXIO;
+
+	return mdiodev_c45_write(xpcs->mdiodev[ch], dev, reg, val);
+}
+
 int xpcs_write(struct dw_xpcs *xpcs, int dev, u32 reg, u16 val)
 {
-	return mdiodev_c45_write(xpcs->mdiodev[0], dev, reg, val);
+	return xpcs_mdev_write_ch(xpcs, 0, dev, reg, val);
 }
 
 int xpcs_modify(struct dw_xpcs *xpcs, int dev, u32 reg, u16 mask, u16 set)
@@ -1525,16 +1534,21 @@ static int xpcs_identify(struct dw_xpcs *xpcs)
 	return -ENODEV;
 }
 
-static struct dw_xpcs *xpcs_create_data(struct mdio_device *mdiodev)
+static struct dw_xpcs *
+xpcs_create_data(struct mdio_device **mdiodev, int channels)
 {
 	struct dw_xpcs *xpcs;
+	int i;
 
 	xpcs = kzalloc_obj(*xpcs);
 	if (!xpcs)
 		return ERR_PTR(-ENOMEM);
 
-	mdio_device_get(mdiodev);
-	xpcs->mdiodev[0] = mdiodev;
+	xpcs->channels = channels;
+	for (i = 0; i < channels; i++) {
+		mdio_device_get(mdiodev[i]);
+		xpcs->mdiodev[i] = mdiodev[i];
+	}
 	xpcs->pcs.ops = &xpcs_phylink_ops;
 	xpcs->pcs.poll = true;
 
@@ -1543,7 +1557,10 @@ static struct dw_xpcs *xpcs_create_data(struct mdio_device *mdiodev)
 
 static void xpcs_free_data(struct dw_xpcs *xpcs)
 {
-	mdio_device_put(xpcs->mdiodev[0]);
+	int i;
+
+	for (i = 0; i < xpcs->channels; i++)
+		mdio_device_put(xpcs->mdiodev[i]);
 	kfree(xpcs);
 }
 
@@ -1592,12 +1609,12 @@ static int xpcs_init_id(struct dw_xpcs *xpcs)
 	return xpcs_identify(xpcs);
 }
 
-static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
+static struct dw_xpcs *xpcs_create(struct mdio_device **mdiodev, int channels)
 {
 	struct dw_xpcs *xpcs;
 	int ret;
 
-	xpcs = xpcs_create_data(mdiodev);
+	xpcs = xpcs_create_data(mdiodev, channels);
 	if (IS_ERR(xpcs))
 		return xpcs;
 
@@ -1628,25 +1645,24 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
 	return ERR_PTR(ret);
 }
 
-/**
- * xpcs_create_mdiodev() - create a DW xPCS instance with the MDIO @addr
- * @bus: pointer to the MDIO-bus descriptor for the device to be looked at
- * @addr: device MDIO-bus ID
- *
- * Return: a pointer to the DW XPCS handle if successful, otherwise -ENODEV if
- * the PCS device couldn't be found on the bus and other negative errno related
- * to the data allocation and MDIO-bus communications.
- */
-struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr)
+static struct dw_xpcs *
+xpcs_create_mdiodevs(struct mii_bus *bus, const int *addrs, int channels)
 {
-	struct mdio_device *mdiodev;
+	struct mdio_device *mdiodev[DW_XPCS_MAX_CHANNELS], *rval;
 	struct dw_xpcs *xpcs;
+	int i;
+
+	if (!addrs || channels <= 0 || channels > DW_XPCS_MAX_CHANNELS)
+		return ERR_PTR(-EINVAL);
 
-	mdiodev = mdio_device_create(bus, addr);
-	if (IS_ERR(mdiodev))
-		return ERR_CAST(mdiodev);
+	for (i = 0; i < channels; i++) {
+		rval = mdio_device_create(bus, addrs[i]);
+		if (IS_ERR(rval))
+			goto out_nomem;
+		mdiodev[i] = rval;
+	}
 
-	xpcs = xpcs_create(mdiodev);
+	xpcs = xpcs_create(mdiodev, channels);
 
 	/* xpcs_create() has taken a refcount on the mdiodev if it was
 	 * successful. If xpcs_create() fails, this will free the mdio
@@ -1654,9 +1670,51 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr)
 	 * anymore, and putting it here will allow mdio_device_put() in
 	 * xpcs_destroy() to automatically free the mdio device.
 	 */
-	mdio_device_put(mdiodev);
+	for (i = 0; i < channels; i++)
+		mdio_device_put(mdiodev[i]);
 
 	return xpcs;
+out_nomem:
+	while (i--)
+		mdio_device_free(mdiodev[i]);
+	return ERR_CAST(rval);
+}
+
+/**
+ * xpcs_create_pcs_mdiodevs() - create a DW xPCS instance with multiple @addrs
+ * @bus: pointer to the embedded phylink PCS instance
+ * @addrs: an array of int
+ * @channels: the number of addrs items or channels
+ *
+ * Return: a pointer to the phylink PCS if successful, otherwise -ENODEV if
+ * the PCS device couldn't be found on the bus and other negative errno related
+ * to the data allocation and MDIO-bus communications.
+ */
+struct phylink_pcs *
+xpcs_create_pcs_mdiodevs(struct mii_bus *bus, const int *addrs, int channels)
+{
+	struct dw_xpcs *xpcs;
+
+	xpcs = xpcs_create_mdiodevs(bus, addrs, channels);
+	if (IS_ERR(xpcs))
+		return ERR_CAST(xpcs);
+
+	return &xpcs->pcs;
+}
+EXPORT_SYMBOL_GPL(xpcs_create_pcs_mdiodevs);
+
+/**
+ * xpcs_create_mdiodev() - create a DW xPCS instance with the MDIO @addr
+ * @bus: pointer to the MDIO-bus descriptor for the device to be looked at
+ * @addr: device MDIO-bus ID
+ *
+ * Return: a pointer to the DW XPCS handle if successful, otherwise -ENODEV if
+ * the PCS device couldn't be found on the bus and other negative errno related
+ * to the data allocation and MDIO-bus communications.
+ */
+struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr)
+{
+	return xpcs_create_mdiodevs(bus, &addr, 1);
 }
 EXPORT_SYMBOL_GPL(xpcs_create_mdiodev);
 
@@ -1694,7 +1752,7 @@ struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode)
 	if (!mdiodev)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	xpcs = xpcs_create(mdiodev);
+	xpcs = xpcs_create(&mdiodev, 1);
 
 	/* xpcs_create() has taken a refcount on the mdiodev if it was
 	 * successful. If xpcs_create() fails, this will free the mdio
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 5c804cfd47d3..36f4906e689a 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -114,6 +114,7 @@ struct dw_xpcs {
 	struct mdio_device *mdiodev[DW_XPCS_MAX_CHANNELS];
 	struct clk_bulk_data clks[DW_XPCS_NUM_CLKS];
 	struct phylink_pcs pcs;
+	int channels;
 	phy_interface_t interface;
 	bool need_reset;
 	u8 eee_mult_fact;
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 36073f7b6bb4..d36904ae7cc9 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -57,6 +57,8 @@ struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr);
 struct dw_xpcs *xpcs_create_fwnode(struct fwnode_handle *fwnode);
 void xpcs_destroy(struct dw_xpcs *xpcs);
 
+struct phylink_pcs *
+xpcs_create_pcs_mdiodevs(struct mii_bus *bus, const int *addrs, int channels);
 struct phylink_pcs *xpcs_create_pcs_mdiodev(struct mii_bus *bus, int addr);
 void xpcs_destroy_pcs(struct phylink_pcs *pcs);
 

Re: [net-next PATCH v4 2/4] net: pcs: xpcs: Allow for multiple addr in creation

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [2]: [net-next PATCH v4 2/4] net: pcs: xpcs: Allow for multiple addr in creation ---
commit 659d2ab0068099d84cd67aa41fc9d8519508054c
Author: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>

net: pcs: xpcs: Allow for multiple addr in creation

This extends xpcs_create_data(), xpcs_create() and xpcs_free_data() to take
an array of mdio devices, adds a new exported xpcs_create_pcs_mdiodevs()
entry point, and switches fbnic over to registering two MDIO addresses.  A
new xpcs->channels field records how many mdio devices were provisioned.

A few questions below on the new multi-channel plumbing and the
kernel-doc/changelog wording.

The changelog says:

    Add xpcs_create_mdiodevs() and xpcs_create_pcs_mdiodevs calls so that a
    driver using the above part can tell xpcs the addr values in one call.

xpcs_create_mdiodevs() is added as static, with no EXPORT_SYMBOL_GPL and no
declaration in include/linux/pcs/pcs-xpcs.h, so a driver that needs the
struct dw_xpcs handle (the xpcs_create_mdiodev() users) still cannot pass
more than one address.  Should the changelog only mention
xpcs_create_pcs_mdiodevs() as the driver-facing addition?

> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
> index 09c5225111be3..88fc78d55c7a5 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_phylink.c
> @@ -200,9 +200,11 @@ int fbnic_phylink_create(struct net_device *netdev)
>  	struct fbnic_dev *fbd = fbn->fbd;
>  	struct phylink_pcs *pcs;
>  	struct phylink *phylink;
> +	/* See fbnic_mdio.c addr usage */
> +	int addrs[] = { 0, 1 };
>  	int err;
>  
> -	pcs = xpcs_create_pcs_mdiodev(fbd->mdio_bus, 0);
> +	pcs = xpcs_create_pcs_mdiodevs(fbd->mdio_bus, addrs, ARRAY_SIZE(addrs));
>  	if (IS_ERR(pcs)) {
>  		err = PTR_ERR(pcs);
>  		dev_err(fbd->dev, "Failed to create PCS device: %d\n", err);

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 6a645df58f9b8..a6b993745f3be 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -214,9 +214,18 @@ int xpcs_read(struct dw_xpcs *xpcs, int dev, u32 reg)
>  	return mdiodev_c45_read(xpcs->mdiodev[0], dev, reg);
>  }
>  
> +static int
> +xpcs_mdev_write_ch(struct dw_xpcs *xpcs, int ch, int dev, u32 reg, u16 val)
> +{
> +	if (ch >= xpcs->channels)
> +		return -ENXIO;
> +
> +	return mdiodev_c45_write(xpcs->mdiodev[ch], dev, reg, val);
> +}
> +
>  int xpcs_write(struct dw_xpcs *xpcs, int dev, u32 reg, u16 val)
>  {
> -	return mdiodev_c45_write(xpcs->mdiodev[0], dev, reg, val);
> +	return xpcs_mdev_write_ch(xpcs, 0, dev, reg, val);
>  }
>  
>  int xpcs_modify(struct dw_xpcs *xpcs, int dev, u32 reg, u16 mask, u16 set)

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
Only the write path becomes channel aware here.  xpcs_read(),
xpcs_modify(), xpcs_modify_changed() and the
phylink_mii_c45_pcs_get_state(xpcs->mdiodev[0], ...) call in
xpcs_get_state_c73() stay pinned to mdiodev[0], and no per-channel read or
read-modify-write helper appears later in the series either (checked at
7b16dc0f9f78, where xpcs_write_pcs_ch() and xpcs_mdev_write_ch() are the
only channel-aware accessors).

For fbnic, addr 1 is a distinct register page:

drivers/net/ethernet/meta/fbnic/fbnic_mdio.c maps addr 0/1 onto
FBNIC_PCS_PAGE(addr), described as allowing access to both halves of the
PCS for the 50R2 config.

So in the two-instance modes (PHY_INTERFACE_MODE_LAUI and
PHY_INTERFACE_MODE_100GBASEP, both with .channels = 2), instance 1 gets the
vendor writes from xpcs_config_rsfec_pma() later in the series, but:

xpcs_pre_config() -> xpcs_soft_reset() -> xpcs_modify() -> mdiodev[0]
xpcs_get_state_c73() -> xpcs_read(MDIO_STAT1/STAT2) -> mdiodev[0]

Does instance 1 need its own soft reset for those vendor writes to latch,
and should its fault/alignment status be folded into the state reported to
phylink?  As written, a faulted or unreset second half would not be visible.

> @@ -1507,16 +1516,21 @@ static int xpcs_identify(struct dw_xpcs *xpcs)

[ ... ]

> @@ -1610,25 +1627,24 @@ static struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
>  	return ERR_PTR(ret);
>  }
>  
> -/**
> - * xpcs_create_mdiodev() - create a DW xPCS instance with the MDIO @addr
> - * @bus: pointer to the MDIO-bus descriptor for the device to be looked at
> - * @addr: device MDIO-bus ID
> - *
> - * Return: a pointer to the DW XPCS handle if successful, otherwise -ENODEV if
> - * the PCS device couldn't be found on the bus and other negative errno related
> - * to the data allocation and MDIO-bus communications.
> - */
> -struct dw_xpcs *xpcs_create_mdiodev(struct mii_bus *bus, int addr)
> +static struct dw_xpcs *
> +xpcs_create_mdiodevs(struct mii_bus *bus, const int *addrs, int channels)
>  {
> -	struct mdio_device *mdiodev;
> +	struct mdio_device *mdiodev[DW_XPCS_MAX_CHANNELS], *rval;
>  	struct dw_xpcs *xpcs;
> +	int i;
> +
> +	if (!addrs || channels <= 0 || channels > DW_XPCS_MAX_CHANNELS)
> +		return ERR_PTR(-EINVAL);

[ ... ]

> +/**
> + * xpcs_create_pcs_mdiodevs() - create a DW xPCS instance with multiple @addrs
> + * @bus: pointer to the embedded phylink PCS instance

[Severity: Low]
[Finding: main-1-4]
[Sources: opus-5]
Should @bus be described as the MDIO-bus descriptor, as the existing
xpcs_create_mdiodev() kernel-doc does?  It is a struct mii_bus * that gets
handed to mdio_device_create(bus, addrs[i]); the embedded phylink PCS is
what the function returns.

> + * @addrs: an array of int
> + * @channels: the number of addrs items or channels
> + *
> + * Return: a pointer to the phylink PCS if successful, otherwise -ENODEV if
> + * the PCS device couldn't be found on the bus and other negative errno related
> + * to the data allocation and MDIO-bus communications.
> + */

[Severity: Low]
[Finding: main-1-4]
[Sources: opus-5]
Could the Return: section also mention -EINVAL?  The new check in
xpcs_create_mdiodevs():

	if (!addrs || channels <= 0 || channels > DW_XPCS_MAX_CHANNELS)
		return ERR_PTR(-EINVAL);

is reachable through this exported wrapper, but only -ENODEV and
allocation/MDIO errors are listed.  Both wording issues are still present at
the end of the series (7b16dc0f9f78).

> +struct phylink_pcs *
> +xpcs_create_pcs_mdiodevs(struct mii_bus *bus, const int *addrs, int channels)
> +{
> +	struct dw_xpcs *xpcs;
> +
> +	xpcs = xpcs_create_mdiodevs(bus, addrs, channels);
> +	if (IS_ERR(xpcs))
> +		return ERR_CAST(xpcs);
> +
> +	return &xpcs->pcs;
> +}
> +EXPORT_SYMBOL_GPL(xpcs_create_pcs_mdiodevs);

[ ... ]

> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index 5c804cfd47d3c..36f4906e689a8 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -114,6 +114,7 @@ struct dw_xpcs {
>  	struct mdio_device *mdiodev[DW_XPCS_MAX_CHANNELS];
>  	struct clk_bulk_data clks[DW_XPCS_NUM_CLKS];
>  	struct phylink_pcs pcs;
> +	int channels;

[Severity: Low]
[Finding: main-1-2]
[Sources: opus-5]
This isn't a bug, but the name channels now carries two different meanings
in this driver.  Here it counts the MDIO devices a driver provisioned, while
struct pma_pcs_values::channels in pcs-xpcs.c counts the PCS instances the
active link mode uses (.channels = 1 for 25GBASER/50GBASER, .channels = 2
for LAUI/100GBASEP).

fbnic_phylink_create() passes addrs[] = { 0, 1 } before any interface is
selected, so xpcs->channels is 2 even in the single-instance modes fbnic
also advertises.  The bound in xpcs_mdev_write_ch():

	if (ch >= xpcs->channels)
		return -ENXIO;

then checks against the provisioned count rather than the active mode's
count.  Would a name like n_mdiodevs or num_addrs make that invariant
clearer?

The fbnic side also repeats the fbnic_mdio.c address layout as bare numbers
tied to it only by the "/* See fbnic_mdio.c addr usage */" comment; could
those addresses come from a shared define?

>  	phy_interface_t interface;
>  	bool need_reset;
>  	u8 eee_mult_fact;

[net-next PATCH v4 3/4] net: pcs: xpcs: Add hooks for xpcs configuration of rsfec

[Alexander Duyck]

From: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>

The DW PCS IP data sheet calls out the need to populate these vendor
registers when operating at speeds above 10Gbps.  This change enables the
correct FEC settings to enable RS-FEC encoding on the link which is the
standard used for most links at these higher speeds.

Signed-off-by: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/pcs/pcs-xpcs.c |   67 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/pcs/pcs-xpcs.h |    6 ++++
 include/uapi/linux/mdio.h  |    3 ++
 3 files changed, 76 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 2e236ff1ad85..c252f8ba6517 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1412,6 +1412,69 @@ static int xpcs_read_ids(struct dw_xpcs *xpcs)
 	return 0;
 }
 
+struct pma_pcs_values {
+	int channels;
+	u16 rsfec_ctrl;
+};
+
+static int
+xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
+{
+	/* fbnic's DW IP exposes the RS-FEC PMA at SEP_PMA1, so use it
+	 * directly. Scanning DEVS1 for the correct PMA MMD is left as future
+	 * work for enabling a non-fbnic device on this PCS.
+	 */
+	int pma_mmd = MDIO_MMD_SEP_PMA1;
+	int ret = 0, i;
+
+	for (i = 0; ret >= 0 && i < v->channels; i++) {
+		ret = xpcs_mdev_write_ch(xpcs, i, pma_mmd,
+					 MDIO_PMA_RSFEC_CTRL, v->rsfec_ctrl);
+	}
+
+	return ret;
+}
+
+static int xpcs_25gbaser_pma_config(struct dw_xpcs *xpcs)
+{
+	const struct pma_pcs_values v = {
+		.rsfec_ctrl = 0,
+		.channels = 1,
+	};
+
+	return xpcs_config_rsfec_pma(xpcs, &v);
+}
+
+static int xpcs_50gbaser_pma_config(struct dw_xpcs *xpcs)
+{
+	const struct pma_pcs_values v = {
+		.rsfec_ctrl = DW_VR_RSFEC_CTRL_TC_PAD_ALTER,
+		.channels = 1,
+	};
+
+	return xpcs_config_rsfec_pma(xpcs, &v);
+}
+
+static int xpcs_50gbaser2_pma_config(struct dw_xpcs *xpcs)
+{
+	const struct pma_pcs_values v = {
+		.rsfec_ctrl = DW_VR_RSFEC_CTRL_TC_PAD_ALTER,
+		.channels = 2,
+	};
+
+	return xpcs_config_rsfec_pma(xpcs, &v);
+}
+
+static int xpcs_100gbasep_pma_config(struct dw_xpcs *xpcs)
+{
+	const struct pma_pcs_values v = {
+		.rsfec_ctrl = MDIO_PMA_RSFEC_CTRL_4LANE_PMD,
+		.channels = 2,
+	};
+
+	return xpcs_config_rsfec_pma(xpcs, &v);
+}
+
 static const struct dw_xpcs_compat synopsys_xpcs_compat[] = {
 	{
 		.interface = PHY_INTERFACE_MODE_USXGMII,
@@ -1425,6 +1488,7 @@ static const struct dw_xpcs_compat synopsys_xpcs_compat[] = {
 		.interface = PHY_INTERFACE_MODE_25GBASER,
 		.supported = xpcs_25gbaser_features,
 		.an_mode = DW_AN_C73,
+		.pma_config = xpcs_25gbaser_pma_config,
 	}, {
 		.interface = PHY_INTERFACE_MODE_XLGMII,
 		.supported = xpcs_xlgmii_features,
@@ -1433,14 +1497,17 @@ static const struct dw_xpcs_compat synopsys_xpcs_compat[] = {
 		.interface = PHY_INTERFACE_MODE_50GBASER,
 		.supported = xpcs_50gbaser_features,
 		.an_mode = DW_AN_C73,
+		.pma_config = xpcs_50gbaser_pma_config,
 	}, {
 		.interface = PHY_INTERFACE_MODE_LAUI,
 		.supported = xpcs_50gbaser2_features,
 		.an_mode = DW_AN_C73,
+		.pma_config = xpcs_50gbaser2_pma_config,
 	}, {
 		.interface = PHY_INTERFACE_MODE_100GBASEP,
 		.supported = xpcs_100gbasep_features,
 		.an_mode = DW_AN_C73,
+		.pma_config = xpcs_100gbasep_pma_config,
 	}, {
 		.interface = PHY_INTERFACE_MODE_10GBASER,
 		.supported = xpcs_10gbaser_features,
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index 36f4906e689a..ab47d6dd050c 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -94,6 +94,12 @@
 #define DW_VR_MII_DIG_CTRL2_TX_POL_INV		BIT(4)
 #define DW_VR_MII_DIG_CTRL2_RX_POL_INV		BIT(0)
 
+/* Clause 133 defines */
+/* RSFEC transcode pad alter
+ * DW vendor extension in RS-FEC control
+ */
+#define DW_VR_RSFEC_CTRL_TC_PAD_ALTER	BIT(10)
+
 #define DW_XPCS_INFO_DECLARE(_name, _pcs, _pma)				\
 	static const struct dw_xpcs_info _name = { .pcs = _pcs, .pma = _pma }
 
diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
index b2541c948fc1..5219c877b2cf 100644
--- a/include/uapi/linux/mdio.h
+++ b/include/uapi/linux/mdio.h
@@ -317,6 +317,9 @@
 #define MDIO_PMA_10GBR_FECABLE_ABLE	0x0001	/* FEC ability */
 #define MDIO_PMA_10GBR_FECABLE_ERRABLE	0x0002	/* FEC error indic. ability */
 
+/* RSFEC PMA Control register */
+#define MDIO_PMA_RSFEC_CTRL_4LANE_PMD	BIT(3)
+
 /* PMA 10GBASE-R Fast Retrain status and control register. */
 #define MDIO_PMA_10GBR_FSRT_ENABLE	0x0001	/* Fast retrain enable */
 

Re: [net-next PATCH v4 3/4] net: pcs: xpcs: Add hooks for xpcs configuration of rsfec

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [3]: [net-next PATCH v4 3/4] net: pcs: xpcs: Add hooks for xpcs configuration of rsfec ---
commit 6935ace349fd9a75ff7b20b38cca3d38ecc8571b
Author: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>

net: pcs: xpcs: Add hooks for xpcs configuration of rsfec

This adds a per-interface-mode .pma_config hook to the Synopsys xPCS
compat table and uses it to write MDIO_PMA_RSFEC_CTRL for 25GBASE-R,
50GBASE-R, LAUI and 100GBASE-P. A new vendor bit is added to pcs-xpcs.h
and a new bit define is added to include/uapi/linux/mdio.h.

A few questions below.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index a6b993745f3be..3800c129dae2e 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -1394,6 +1394,69 @@ static int xpcs_read_ids(struct dw_xpcs *xpcs)
>  	return 0;
>  }
>  
> +struct pma_pcs_values {
> +	int channels;
> +	u16 rsfec_ctrl;
> +};
> +
> +static int
> +xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
> +{
> +	/* fbnic's DW IP exposes the RS-FEC PMA at SEP_PMA1, so use it
> +	 * directly. Scanning DEVS1 for the correct PMA MMD is left as future
> +	 * work for enabling a non-fbnic device on this PCS.
> +	 */
> +	int pma_mmd = MDIO_MMD_SEP_PMA1;
> +	int ret = 0, i;
> +
> +	for (i = 0; ret >= 0 && i < v->channels; i++) {
> +		ret = xpcs_mdev_write_ch(xpcs, i, pma_mmd,
> +					 MDIO_PMA_RSFEC_CTRL, v->rsfec_ctrl);
> +	}
> +
> +	return ret;
> +}

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5, sonnet-5]
The comment states the SEP_PMA1 placement is an fbnic property, but the
hooks below are attached to synopsys_xpcs_compat[], which xpcs_identify()
selects for every device matching DW_XPCS_ID/DW_XPCS_ID_MASK (stmmac via
xpcs_create_fwnode()/xpcs_create_mdiodev(), ksz9477, txgbe SP). Should
these hooks be gated on xpcs->info.pma, or live in a device specific
compat table?

The file already has both mechanisms: xpcs_do_config() checks
WX_TXGBE_XPCS_PMA_10G_ID, xpcs_create() checks
MP_FBNIC_XPCS_PMA_100G_ID, and nxp_sja1105_xpcs_compat /
nxp_sja1110_xpcs_compat show the separate table approach.

There is also no MDIO_DEVS1 / MDIO_DEVS_SEP_PMA1 presence check before
writing, so on a DW xPCS that does not implement SEP_PMA1 this writes
into an MMD that may not exist. Is that intended?

Second question on the same loop: the iteration count comes from
v->channels rather than xpcs->channels. Given:

drivers/net/pcs/pcs-xpcs.c:xpcs_mdev_write_ch() {
	if (ch >= xpcs->channels)
		return -ENXIO;
	...
}

and xpcs_do_config() propagating the pma_config() error, wouldn't a
single channel xPCS instance selecting PHY_INTERFACE_MODE_LAUI or
PHY_INTERFACE_MODE_100GBASEP (both .channels = 2 below) fail interface
configuration outright with -ENXIO, after channel 0 has already been
written and with no unwind of that write?

In the current tree fbnic creates its PCS with two addresses so
xpcs->channels is 2 and this is not reachable today, and the 25G
Wangxun AML part never instantiates an xPCS at all, so this looks
latent rather than an active regression. Would deriving the loop bound
from xpcs->channels avoid it entirely?

> +
> +static int xpcs_25gbaser_pma_config(struct dw_xpcs *xpcs)
> +{
> +	const struct pma_pcs_values v = {
> +		.rsfec_ctrl = 0,
> +		.channels = 1,
> +	};

[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
The commit message says this "enables the correct FEC settings to enable
RS-FEC encoding on the link". Which write sets an RS-FEC enable bit?

For 25GBASE-R the value written to MDIO_PMA_RSFEC_CTRL is a literal 0.
The other three hooks write only DW_VR_RSFEC_CTRL_TC_PAD_ALTER (BIT(10))
or MDIO_PMA_RSFEC_CTRL_4LANE_PMD (BIT(3)).

Also, xpcs_config_rsfec_pma() does a full 16-bit write rather than a
masked update via the existing xpcs_modify() helper, so the RS-FEC
enable / bypass-correction / bypass-indication bits of this
standards-defined register are unconditionally cleared. Is it safe to
discard whatever firmware or an earlier bring-up stage programmed there?

A related question on the keying: the value depends only on
phy_interface_t and not on the selected FEC mode. fbnic handles
FBNIC_AUI_LAUI2 with FBNIC_FEC_OFF in fbnic_mac_get_link_status(), yet
PHY_INTERFACE_MODE_LAUI here always programs the transcode pad alter
bit, and FBNIC_AUI_25GAUI with FBNIC_FEC_RS still ends up writing 0.
Should the register contents track the negotiated FEC mode?

Nothing reads the register back or checks an RS-FEC status bit
afterwards, so a wrong value would be silent.

> +
> +	return xpcs_config_rsfec_pma(xpcs, &v);
> +}
> +
> +static int xpcs_50gbaser_pma_config(struct dw_xpcs *xpcs)
> +{
> +	const struct pma_pcs_values v = {
> +		.rsfec_ctrl = DW_VR_RSFEC_CTRL_TC_PAD_ALTER,
> +		.channels = 1,
> +	};
> +
> +	return xpcs_config_rsfec_pma(xpcs, &v);
> +}
> +
> +static int xpcs_50gbaser2_pma_config(struct dw_xpcs *xpcs)
> +{
> +	const struct pma_pcs_values v = {
> +		.rsfec_ctrl = DW_VR_RSFEC_CTRL_TC_PAD_ALTER,
> +		.channels = 2,
> +	};
> +
> +	return xpcs_config_rsfec_pma(xpcs, &v);
> +}
> +
> +static int xpcs_100gbasep_pma_config(struct dw_xpcs *xpcs)
> +{
> +	const struct pma_pcs_values v = {
> +		.rsfec_ctrl = MDIO_PMA_RSFEC_CTRL_4LANE_PMD,
> +		.channels = 2,
> +	};
> +
> +	return xpcs_config_rsfec_pma(xpcs, &v);
> +}
> +
>  static const struct dw_xpcs_compat synopsys_xpcs_compat[] = {

[ ... ]

> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index 36f4906e689a8..ab47d6dd050c7 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -94,6 +94,12 @@
>  #define DW_VR_MII_DIG_CTRL2_TX_POL_INV		BIT(4)
>  #define DW_VR_MII_DIG_CTRL2_RX_POL_INV		BIT(0)
>  
> +/* Clause 133 defines */
> +/* RSFEC transcode pad alter
> + * DW vendor extension in RS-FEC control
> + */
> +#define DW_VR_RSFEC_CTRL_TC_PAD_ALTER	BIT(10)

[Severity: Low]
[Finding: main-3-8]
[Sources: opus-5]
This isn't a bug, but is "Clause 133" the right reference here? In IEEE
802.3-2022 the RS-FEC sublayer and its MDIO registers are Clause 134
(Clause 108 for 25G RS-FEC), and Clause 133 is a PCS clause. The next
patch in this series cites "IEEE 133.2.2" for PCS alignment marker
content, which suggests the same.

Also, the macro name implies a register macro DW_VR_RSFEC_CTRL, which
does not exist anywhere in the tree; the register actually written is
MDIO_PMA_RSFEC_CTRL in MDIO_MMD_SEP_PMA1. The rest of the file prefixes
a bit macro with the register macro it belongs to, for example
DW_VR_MII_DIG_CTRL2 -> DW_VR_MII_DIG_CTRL2_TX_POL_INV.

The define is also placed right after the DW_VR_MII_* block, which is
MMD 31 vendor MII, while this bit belongs to a PMA MMD register. Would
moving it and renaming it to match the register make this clearer?

> +
>  #define DW_XPCS_INFO_DECLARE(_name, _pcs, _pma)				\
>  	static const struct dw_xpcs_info _name = { .pcs = _pcs, .pma = _pma }
>  
> diff --git a/include/uapi/linux/mdio.h b/include/uapi/linux/mdio.h
> index b2541c948fc1e..5219c877b2cfd 100644
> --- a/include/uapi/linux/mdio.h
> +++ b/include/uapi/linux/mdio.h
> @@ -317,6 +317,9 @@
>  #define MDIO_PMA_10GBR_FECABLE_ABLE	0x0001	/* FEC ability */
>  #define MDIO_PMA_10GBR_FECABLE_ERRABLE	0x0002	/* FEC error indic. ability */
>  
> +/* RSFEC PMA Control register */
> +#define MDIO_PMA_RSFEC_CTRL_4LANE_PMD	BIT(3)
> +

[Severity: Low]
[Finding: main-1-2]
[Sources: opus-5]
This isn't a bug, but could the IEEE clause and table that define bit 3
of register 1.200 be cited here? Since the sibling bit of the same
register added by this patch (DW_VR_RSFEC_CTRL_TC_PAD_ALTER) is
described as a DW vendor extension and kept in the driver header, it is
hard to tell from the patch whether bit 3 is genuinely standard or also
Synopsys specific. If it is vendor specific, pcs-xpcs.h with a DW_VR_
prefix would be the better home.

Placement is also a little odd: the new define sits inside the
10GBASE-R FEC ability block, between MDIO_PMA_10GBR_FECABLE_ERRABLE and
MDIO_PMA_10GBR_FSRT_ENABLE, rather than next to the
MDIO_PMA_RSFEC_CTRL register definition earlier in the file. Would
moving it beside MDIO_PMA_RSFEC_CTRL be preferable?

>  /* PMA 10GBASE-R Fast Retrain status and control register. */
>  #define MDIO_PMA_10GBR_FSRT_ENABLE	0x0001	/* Fast retrain enable */
>

[net-next PATCH v4 4/4] net: pcs: xpcs: Add handling for 4 channel rsfec device

[Alexander Duyck]

From: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>

This patch introduces the configuration of vendor specific registers for
alignment encoding, PCS Mode, and VL_INTVL over the one or two instances as
required.

The DW PCS IP specification calls out the need to configure both lanes
identically when using 2 lane modes such as 50-R2 and 100-R2, so the
programming is repeated for each channel.

The encoding tables are derived from the IEEE 8023-2022 spec sections
82.2.7 and tables 82-2 and 82-3 for the alignment markers and their
insertion.

Note that there is a conflict between VRs DW_VR_XS_PCS_DIG_STS and the
DW_PCS_IP DW_VR_MII_PCS_PCS_MODE.  The bit mask for
DW_VR_XS_PCS_DIG_STS/RX_FIFO_ERR fits within the reserved bits for the DW
PCS IP DW_VR_MII_PCS_PCS_MODE register so there is no issue.

There is also a conflict between DW_VR_MII_PCS_VL_INTVL and
DW_VR_MII_AN_INTR_STS but an_mode differs, so again there is no issue.

Signed-off-by: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 drivers/net/pcs/pcs-xpcs.c |   96 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/pcs/pcs-xpcs.h |   25 +++++++++++
 2 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index c252f8ba6517..d9c444528f55 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1412,11 +1412,67 @@ static int xpcs_read_ids(struct dw_xpcs *xpcs)
 	return 0;
 }
 
+/* m0 - m2 from Table 82-2/82-3
+ * m4 - m6 are skipped since they are inversions of m0 - m2.
+ * Inverted parity fields (IEEE 82.2.8) bip3 and bip7 are omitted.
+ */
+struct lane_markers {
+	u8 m0, m1, m2;
+};
+
+/* Alignment marker encodings, see table 82-2 in IEEE 802.3-2022 */
+static const struct lane_markers xpcs_100gbaser_markers[] = {
+	{0xc1, 0x68, 0x21},
+	{0x9d, 0x71, 0x8e},
+	{0x59, 0x4b, 0xe8},
+	{0x4d, 0x95, 0x7b},
+};
+
+/* Alignment marker encodings, see table 82-3 in IEEE 802.3-2022
+ * The content of the 50G markers is identical to 40G values (IEEE 133.2.2).
+ */
+static const struct lane_markers xpcs_50gbaser_markers[] = {
+	{0x90, 0x76, 0x47},
+	{0xf0, 0xc4, 0xe6},
+	{0xc5, 0x65, 0x9b},
+	{0xa2, 0x79, 0x3d},
+};
+
 struct pma_pcs_values {
+	const struct lane_markers *vl0_markers;
+	const struct lane_markers *vl123_markers;
 	int channels;
 	u16 rsfec_ctrl;
+	u16 pcs_mode;
+	u16 vl_intvl;
 };
 
+#define XPCS_VL_TO_REG(vl, lh) \
+	(((vl) * 2) + DW_VR_MII_PCS_VL0_##lh)
+
+static int
+xpcs_write_pcs_ch(struct dw_xpcs *xpcs, int ch, int reg, u16 val)
+{
+	return xpcs_mdev_write_ch(xpcs, ch, MDIO_MMD_PCS, reg, val);
+}
+
+static int xpcs_config_ch_vl_markers(struct dw_xpcs *xpcs, int ch, int vl,
+				     const struct lane_markers *m)
+{
+	int ret;
+
+	/* m0, m1, m2 written to _L and _H registers
+	 *
+	 * _L = (m1 << 8) | m0
+	 * _H = m2
+	 */
+	ret = xpcs_write_pcs_ch(xpcs, ch, XPCS_VL_TO_REG(vl, L),
+				((u16)m->m1 << 8) | m->m0);
+	if (ret < 0)
+		return ret;
+	return xpcs_write_pcs_ch(xpcs, ch, XPCS_VL_TO_REG(vl, H), m->m2);
+}
+
 static int
 xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
 {
@@ -1425,9 +1481,28 @@ xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
 	 * work for enabling a non-fbnic device on this PCS.
 	 */
 	int pma_mmd = MDIO_MMD_SEP_PMA1;
-	int ret = 0, i;
+	int ret = 0, i, vl;
 
 	for (i = 0; ret >= 0 && i < v->channels; i++) {
+		/* code word markings */
+		for (vl = 0; ret >= 0 && vl < 4; vl++) {
+			const struct lane_markers *m;
+
+			m = vl ? v->vl123_markers + vl - 1 : v->vl0_markers;
+			ret = xpcs_config_ch_vl_markers(xpcs, i, vl, m);
+		}
+		if (ret < 0)
+			break;
+		/* vendor registers */
+		ret = xpcs_write_pcs_ch(xpcs, i,
+					DW_VR_MII_PCS_VL_INTVL, v->vl_intvl);
+		if (ret < 0)
+			break;
+		ret = xpcs_write_pcs_ch(xpcs, i,
+					DW_VR_MII_PCS_PCS_MODE, v->pcs_mode);
+		if (ret < 0)
+			break;
+		/* rsfec register */
 		ret = xpcs_mdev_write_ch(xpcs, i, pma_mmd,
 					 MDIO_PMA_RSFEC_CTRL, v->rsfec_ctrl);
 	}
@@ -1440,6 +1515,13 @@ static int xpcs_25gbaser_pma_config(struct dw_xpcs *xpcs)
 	const struct pma_pcs_values v = {
 		.rsfec_ctrl = 0,
 		.channels = 1,
+		/* 25g markers from 100g and 50g tables per 802.3-2022
+		 * 108.5.2.4
+		 */
+		.vl0_markers = &xpcs_100gbaser_markers[0],
+		.vl123_markers = &xpcs_50gbaser_markers[1],
+		.vl_intvl = 20479,
+		.pcs_mode = DW_VR_MII_PCS_MODE_CLAUSE107,
 	};
 
 	return xpcs_config_rsfec_pma(xpcs, &v);
@@ -1450,6 +1532,10 @@ static int xpcs_50gbaser_pma_config(struct dw_xpcs *xpcs)
 	const struct pma_pcs_values v = {
 		.rsfec_ctrl = DW_VR_RSFEC_CTRL_TC_PAD_ALTER,
 		.channels = 1,
+		.vl0_markers = &xpcs_50gbaser_markers[0],
+		.vl123_markers = &xpcs_50gbaser_markers[1],
+		.pcs_mode = 0,
+		.vl_intvl = 20479,
 	};
 
 	return xpcs_config_rsfec_pma(xpcs, &v);
@@ -1460,6 +1546,10 @@ static int xpcs_50gbaser2_pma_config(struct dw_xpcs *xpcs)
 	const struct pma_pcs_values v = {
 		.rsfec_ctrl = DW_VR_RSFEC_CTRL_TC_PAD_ALTER,
 		.channels = 2,
+		.vl0_markers = &xpcs_50gbaser_markers[0],
+		.vl123_markers = &xpcs_50gbaser_markers[1],
+		.pcs_mode = 0,
+		.vl_intvl = 20479,
 	};
 
 	return xpcs_config_rsfec_pma(xpcs, &v);
@@ -1470,6 +1560,10 @@ static int xpcs_100gbasep_pma_config(struct dw_xpcs *xpcs)
 	const struct pma_pcs_values v = {
 		.rsfec_ctrl = MDIO_PMA_RSFEC_CTRL_4LANE_PMD,
 		.channels = 2,
+		.vl0_markers = &xpcs_100gbaser_markers[0],
+		.vl123_markers = &xpcs_100gbaser_markers[1],
+		.pcs_mode = DW_VR_MII_PCS_MODE_DISABLE_MLD,
+		.vl_intvl = 16383,
 	};
 
 	return xpcs_config_rsfec_pma(xpcs, &v);
diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
index ab47d6dd050c..adb7b1c13c2d 100644
--- a/drivers/net/pcs/pcs-xpcs.h
+++ b/drivers/net/pcs/pcs-xpcs.h
@@ -100,6 +100,31 @@
  */
 #define DW_VR_RSFEC_CTRL_TC_PAD_ALTER	BIT(10)
 
+/* Vendor specific 4 channel PCS registers */
+
+/* DW_VR_MII_PCS_VL_INTVL and DW_VR_MII_AN_INTR_STS conflict
+ * but code paths are different
+ */
+#define DW_VR_MII_PCS_VL_INTVL		0x8002
+/* 0x8008 - 0x800f */
+#define DW_VR_MII_PCS_VL0_L		0x8008
+#define DW_VR_MII_PCS_VL0_H		0x8009
+#define DW_VR_MII_PCS_PCS_MODE		0x8010
+
+/* DW_VR_MII_PCS_PCS_MODE bits */
+#define DW_VR_MII_PCS_MODE_HI_BER25		BIT(2)
+#define DW_VR_MII_PCS_MODE_DISABLE_MLD		BIT(1)
+#define DW_VR_MII_PCS_MODE_CLAUSE49		BIT(0)
+
+/* 25G requires these two bits are set.
+ *
+ * The CLAUSE49 bit changes the interface with the MAC
+ * to 64 bit and the BER25 bit changes the measurement
+ * interval to 2ms.
+ */
+#define DW_VR_MII_PCS_MODE_CLAUSE107 \
+	(DW_VR_MII_PCS_MODE_HI_BER25 | DW_VR_MII_PCS_MODE_CLAUSE49)
+
 #define DW_XPCS_INFO_DECLARE(_name, _pcs, _pma)				\
 	static const struct dw_xpcs_info _name = { .pcs = _pcs, .pma = _pma }
 

Re: [net-next PATCH v4 4/4] net: pcs: xpcs: Add handling for 4 channel rsfec device

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [4]: [net-next PATCH v4 4/4] net: pcs: xpcs: Add handling for 4 channel rsfec device ---
commit 7b16dc0f9f781491095c890579bd7608f7d1a49f
Author: Mike Marciniszyn (Meta) <mike.marciniszyn@gmail.com>

net: pcs: xpcs: Add handling for 4 channel rsfec device

This adds programming of the DW PCS vendor registers for the alignment
marker encodings, the PCS mode and the alignment marker insertion interval,
repeated over each configured channel.  Marker tables for 100G and 50G are
added and selected per interface mode.  No Link: tags are present in the
commit header.

A few questions on the new registers, constants and comments below.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 3800c129dae2e..8f990291d5190 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -1394,11 +1394,67 @@ static int xpcs_read_ids(struct dw_xpcs *xpcs)
>  	return 0;
>  }
>  
> +/* m0 - m2 from Table 82-2/82-3
> + * m4 - m6 are skipped since they are inversions of m0 - m2.
> + * Inverted parity fields (IEEE 82.2.8) bip3 and bip7 are omitted.
> + */
> +struct lane_markers {
> +	u8 m0, m1, m2;
> +};
> +
> +/* Alignment marker encodings, see table 82-2 in IEEE 802.3-2022 */
> +static const struct lane_markers xpcs_100gbaser_markers[] = {
> +	{0xc1, 0x68, 0x21},
> +	{0x9d, 0x71, 0x8e},
> +	{0x59, 0x4b, 0xe8},
> +	{0x4d, 0x95, 0x7b},
> +};
> +
> +/* Alignment marker encodings, see table 82-3 in IEEE 802.3-2022
> + * The content of the 50G markers is identical to 40G values (IEEE 133.2.2).
> + */
> +static const struct lane_markers xpcs_50gbaser_markers[] = {
> +	{0x90, 0x76, 0x47},
> +	{0xf0, 0xc4, 0xe6},
> +	{0xc5, 0x65, 0x9b},
> +	{0xa2, 0x79, 0x3d},
> +};
> +
>  struct pma_pcs_values {
> +	const struct lane_markers *vl0_markers;
> +	const struct lane_markers *vl123_markers;
>  	int channels;
>  	u16 rsfec_ctrl;
> +	u16 pcs_mode;
> +	u16 vl_intvl;
>  };
>  
> +#define XPCS_VL_TO_REG(vl, lh) \
> +	(((vl) * 2) + DW_VR_MII_PCS_VL0_##lh)
> +
> +static int
> +xpcs_write_pcs_ch(struct dw_xpcs *xpcs, int ch, int reg, u16 val)
> +{
> +	return xpcs_mdev_write_ch(xpcs, ch, MDIO_MMD_PCS, reg, val);
> +}
> +
> +static int xpcs_config_ch_vl_markers(struct dw_xpcs *xpcs, int ch, int vl,
> +				     const struct lane_markers *m)
> +{
> +	int ret;
> +
> +	/* m0, m1, m2 written to _L and _H registers
> +	 *
> +	 * _L = (m1 << 8) | m0
> +	 * _H = m2
> +	 */
> +	ret = xpcs_write_pcs_ch(xpcs, ch, XPCS_VL_TO_REG(vl, L),
> +				((u16)m->m1 << 8) | m->m0);
> +	if (ret < 0)
> +		return ret;
> +	return xpcs_write_pcs_ch(xpcs, ch, XPCS_VL_TO_REG(vl, H), m->m2);
> +}
> +
>  static int
>  xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
>  {
> @@ -1407,9 +1463,28 @@ xpcs_config_rsfec_pma(struct dw_xpcs *xpcs, const struct pma_pcs_values *v)
>  	 * work for enabling a non-fbnic device on this PCS.
>  	 */
>  	int pma_mmd = MDIO_MMD_SEP_PMA1;
> -	int ret = 0, i;
> +	int ret = 0, i, vl;
>  
>  	for (i = 0; ret >= 0 && i < v->channels; i++) {
> +		/* code word markings */
> +		for (vl = 0; ret >= 0 && vl < 4; vl++) {
> +			const struct lane_markers *m;
> +
> +			m = vl ? v->vl123_markers + vl - 1 : v->vl0_markers;
> +			ret = xpcs_config_ch_vl_markers(xpcs, i, vl, m);
> +		}
> +		if (ret < 0)
> +			break;
> +		/* vendor registers */
> +		ret = xpcs_write_pcs_ch(xpcs, i,
> +					DW_VR_MII_PCS_VL_INTVL, v->vl_intvl);
> +		if (ret < 0)
> +			break;
> +		ret = xpcs_write_pcs_ch(xpcs, i,
> +					DW_VR_MII_PCS_PCS_MODE, v->pcs_mode);
> +		if (ret < 0)
> +			break;
> +		/* rsfec register */
>  		ret = xpcs_mdev_write_ch(xpcs, i, pma_mmd,
>  					 MDIO_PMA_RSFEC_CTRL, v->rsfec_ctrl);
>  	}

[Severity: Low]
[Finding: main-1-3]
[Sources: opus-5]
This isn't a bug, but should xpcs_config_rsfec_pma() keep its name now that
it also programs PCS layer registers?

It is registered as the .pma_config callback of struct dw_xpcs_compat and
invoked from xpcs_config():

	if (compat->pma_config) {
		ret = compat->pma_config(xpcs);

After this patch the body writes MDIO_MMD_PCS vendor registers (the
alignment markers 0x8008-0x800f via xpcs_config_ch_vl_markers(), the VL
interval 0x8002 and the PCS mode 0x8010) in addition to the single
MDIO_MMD_SEP_PMA1 MDIO_PMA_RSFEC_CTRL write.

The other .pma_config implementations stay inside PMA/VEND2 register space:
nxp_sja1105_sgmii_pma_config() writes DW_VR_MII_DIG_CTRL2 in VEND2,
nxp_sja1110_pma_config() writes the MDIO_MMD_VEND2 PMA PLL registers, and
txgbe_pma_config_*() write PMA registers.

Would renaming it (xpcs_config_pcs_pma or xpcs_config_mode) or splitting
the PCS layer programming into its own step make the layering clearer?

> @@ -1422,6 +1497,13 @@ static int xpcs_25gbaser_pma_config(struct dw_xpcs *xpcs)
>  	const struct pma_pcs_values v = {
>  		.rsfec_ctrl = 0,
>  		.channels = 1,
> +		/* 25g markers from 100g and 50g tables per 802.3-2022
> +		 * 108.5.2.4
> +		 */
> +		.vl0_markers = &xpcs_100gbaser_markers[0],
> +		.vl123_markers = &xpcs_50gbaser_markers[1],
> +		.vl_intvl = 20479,
> +		.pcs_mode = DW_VR_MII_PCS_MODE_CLAUSE107,
>  	};
>  
>  	return xpcs_config_rsfec_pma(xpcs, &v);

[Severity: Low]
[Finding: main-1-2]
[Sources: opus-5]
Where do the .vl_intvl values come from?  The commit message says:

    The encoding tables are derived from the IEEE 8023-2022 spec sections
    82.2.7 and tables 82-2 and 82-3 for the alignment markers and their
    insertion.

82.2.7 specifies a 16384 block insertion period, which matches the 16383
used by xpcs_100gbasep_pma_config(), but the 20479 used here and by
xpcs_50gbaser_pma_config()/xpcs_50gbaser2_pma_config() corresponds to the
Clause 133/108 50G and 25G period, which the message does not cite.  Could
the reference for 20479 and the reason for the -1 bias be spelled out?

The two values also need different widths (16383 is 0x3fff, 14 bits; 20479
is 0x4fff, 15 bits) and are written raw with no #define and no
GENMASK()/FIELD_PREP(), unlike the other multi-bit fields in this header
such as DW_VR_MII_EEE_MULT_FACT_100NS.  Is the interval field width
documented anywhere?  If it were [13:0], 20479 would truncate to 4095 and
bit 14 would land on a neighbouring control bit, and an MDIO write cannot
report that.

Also, the lane count is hardcoded in xpcs_config_rsfec_pma():

		for (vl = 0; ret >= 0 && vl < 4; vl++) {

Would it be better as a field of pma_pcs_values, or tied to ARRAY_SIZE() of
the marker tables and the documented 0x8008-0x800f window?  As written, the
single channel 25G and 50G-R1 configurations still program the VL1..VL3
marker sets, so the table does not express which fields are meaningful per
mode.

Last, the subject says "4 channel rsfec device" while DW_XPCS_MAX_CHANNELS
is 2 ("2 channels per group DW PCS devices") and the largest .channels used
is 2, which also reads differently from the message body ("over the one or
two instances as required").

[ ... ]

> @@ -1442,6 +1528,10 @@ static int xpcs_50gbaser2_pma_config(struct dw_xpcs *xpcs)
>  	const struct pma_pcs_values v = {
>  		.rsfec_ctrl = DW_VR_RSFEC_CTRL_TC_PAD_ALTER,
>  		.channels = 2,
> +		.vl0_markers = &xpcs_50gbaser_markers[0],
> +		.vl123_markers = &xpcs_50gbaser_markers[1],
> +		.pcs_mode = 0,
> +		.vl_intvl = 20479,
>  	};
>  
>  	return xpcs_config_rsfec_pma(xpcs, &v);
> @@ -1452,6 +1542,10 @@ static int xpcs_100gbasep_pma_config(struct dw_xpcs *xpcs)
>  	const struct pma_pcs_values v = {
>  		.rsfec_ctrl = MDIO_PMA_RSFEC_CTRL_4LANE_PMD,
>  		.channels = 2,
> +		.vl0_markers = &xpcs_100gbaser_markers[0],
> +		.vl123_markers = &xpcs_100gbaser_markers[1],
> +		.pcs_mode = DW_VR_MII_PCS_MODE_DISABLE_MLD,
> +		.vl_intvl = 16383,
>  	};
>  
>  	return xpcs_config_rsfec_pma(xpcs, &v);

[Severity: Medium]
[Finding: main-2-5]
[Sources: opus-5]
Why do the two .channels = 2 configurations differ in MLD handling and in
the marker interval?  The commit message explains the repetition with:

    The DW PCS IP specification calls out the need to configure both lanes
    identically when using 2 lane modes such as 50-R2 and 100-R2, so the
    programming is repeated for each channel.

but xpcs_100gbasep_pma_config() sets DW_VR_MII_PCS_MODE_DISABLE_MLD and
.vl_intvl = 16383, while xpcs_50gbaser2_pma_config() leaves .pcs_mode = 0
and uses 20479.  On top of that, the RS-FEC control value here is
MDIO_PMA_RSFEC_CTRL_4LANE_PMD, a 4 lane PMD bit.

PHY_INTERFACE_MODE_100GBASEP is described in include/linux/phy.h as
"100GBase-P - with Clause 134 FEC", and fbnic maps a 2 lane AUI to it:

drivers/net/ethernet/meta/fbnic/fbnic_phylink.c:
	case FBNIC_AUI_100GAUI2:
		return PHY_INTERFACE_MODE_100GBASEP;

16383 is the Clause 82 (40G/100G-R4) insertion period and 20479 is the
Clause 133 50G period used by the sibling 2 channel configuration.

Is 100GBASEP here intended as a Clause 133 100GBASE-R2 (which would suggest
20479 and MLD semantics like 50G-R2), or as a Clause 82/91 4 lane 100G?  And
is programming only four virtual lane marker sets correct for it?  Every
fbnic 100G link brings this path up through xpcs_config() ->
compat->pma_config(), so if the values do not match the hardware the
alignment marker/FEC lock would fail and the link would not come up.

> diff --git a/drivers/net/pcs/pcs-xpcs.h b/drivers/net/pcs/pcs-xpcs.h
> index ab47d6dd050c7..adb7b1c13c2df 100644
> --- a/drivers/net/pcs/pcs-xpcs.h
> +++ b/drivers/net/pcs/pcs-xpcs.h
> @@ -100,6 +100,31 @@
>   */
>  #define DW_VR_RSFEC_CTRL_TC_PAD_ALTER	BIT(10)
>  
> +/* Vendor specific 4 channel PCS registers */
> +
> +/* DW_VR_MII_PCS_VL_INTVL and DW_VR_MII_AN_INTR_STS conflict
> + * but code paths are different
> + */
> +#define DW_VR_MII_PCS_VL_INTVL		0x8002

[Severity: Low]
[Finding: main-3-7]
[Sources: opus-5]
Is there really a conflict to document here?  The new register is only ever
reached through xpcs_write_pcs_ch():

	return xpcs_mdev_write_ch(xpcs, ch, MDIO_MMD_PCS, reg, val);

that is MMD 3, while every access to DW_VR_MII_AN_INTR_STS uses
MDIO_MMD_VEND2, that is MMD 31:

	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, 0);

C45 register space is per MMD, so MMD3:0x8002 and MMD31:0x8002 are simply
unrelated registers.  The same applies to the commit message:

    There is also a conflict between DW_VR_MII_PCS_VL_INTVL and
    DW_VR_MII_AN_INTR_STS but an_mode differs, so again there is no issue.

Would "different MMD" be the accurate reason instead of "code paths are
different" and "an_mode differs"?  As written, a later reader could
conclude the MMD selection does not matter, or add an an_mode based guard
that has no purpose.

Two smaller wording points in the same patch.  In the lane_markers comment:

    Inverted parity fields (IEEE 82.2.8) bip3 and bip7 are omitted.

bip3 is the parity field and bip7 is its inversion, so calling both
"inverted parity fields" reads incorrectly.  Also the spec references such
as "IEEE 82.2.8" and "IEEE 133.2.2" omit 802.3, could they be written as
IEEE 802.3 clause 82.2.8 and clause 133.2.2?

> +/* 0x8008 - 0x800f */
> +#define DW_VR_MII_PCS_VL0_L		0x8008
> +#define DW_VR_MII_PCS_VL0_H		0x8009
> +#define DW_VR_MII_PCS_PCS_MODE		0x8010
> +
> +/* DW_VR_MII_PCS_PCS_MODE bits */
> +#define DW_VR_MII_PCS_MODE_HI_BER25		BIT(2)
> +#define DW_VR_MII_PCS_MODE_DISABLE_MLD		BIT(1)
> +#define DW_VR_MII_PCS_MODE_CLAUSE49		BIT(0)

[Severity: Low]
[Finding: main-1-1]
[Sources: opus-5]
Is the DW_VR_MII_ prefix the right one for registers that live in the PCS
MMD?  Every pre-existing DW_VR_MII_* define in this header is a VR MII MMD
register: DW_VR_MII_AN_CTRL and DW_VR_MII_AN_INTR_STS are accessed with
MDIO_MMD_VEND2, and DW_VR_MII_DIG_CTRL2 likewise in pcs-xpcs-nxp.c.  The
PCS MMD vendor registers use the DW_VR_XS_PCS_ prefix with the DW_VENDOR
bit added at access time by xpcs_read_vendor()/xpcs_write_vendor(), whereas
these new defines hard-code the DW_VENDOR bit (0x8002/0x8008/0x8010), which
is exactly how DW_VR_MII_PCS_PCS_MODE ends up equal to
DW_VENDOR | DW_VR_XS_PCS_DIG_STS.

The new bit names also share a prefix with existing defines for an
unrelated register.  DW_VR_MII_AN_CTRL (0x8001, MMD 31) already has:

	#define DW_VR_MII_PCS_MODE_MASK			GENMASK(2, 1)
	#define DW_VR_MII_PCS_MODE_C37_1000BASEX	0x0
	#define DW_VR_MII_PCS_MODE_C37_SGMII		0x2

which xpcs_config_aneg_c37_sgmii() uses as field values:

	mask = DW_VR_MII_PCS_MODE_MASK | DW_VR_MII_TX_CONFIG_MASK;
	val = FIELD_PREP(DW_VR_MII_PCS_MODE_MASK,
			 DW_VR_MII_PCS_MODE_C37_SGMII);

A later xpcs_modify() on the new 0x8010 register that picks up the
plausible looking DW_VR_MII_PCS_MODE_MASK would silently drop
DW_VR_MII_PCS_MODE_CLAUSE49 (BIT(0)).  Could the new register and bit names
be given a distinct prefix, and the DW_VENDOR bit be applied through the
existing vendor accessors?

Re: [net-next PATCH v4 0/4] Add support for XPCS based RSFEC configuration

[Jakub Kicinski]

On Wed, 29 Jul 2026 15:16:25 -0700 Alexander Duyck wrote:
> This series adds to xpcs to add the necessary configuration steps
> for the DW PCS/rsfec for high speed interfaces.
> 
> I picked up the set from Mike Marciniszyn who is no longer with Meta. His
> set ending with v3 can be found here:
> https://lore.kernel.org/netdev/20260527000958.81108-1-mike.marciniszyn@gmail.com/

Since we got no human reviews I sent out the AI comments.
LMK if you plan to respin, please.

Re: [net-next PATCH v4 0/4] Add support for XPCS based RSFEC configuration

[Alexander Duyck]

On Mon, Aug 3, 2026 at 6:49 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 29 Jul 2026 15:16:25 -0700 Alexander Duyck wrote:
> > This series adds to xpcs to add the necessary configuration steps
> > for the DW PCS/rsfec for high speed interfaces.
> >
> > I picked up the set from Mike Marciniszyn who is no longer with Meta. His
> > set ending with v3 can be found here:
> > https://lore.kernel.org/netdev/20260527000958.81108-1-mike.marciniszyn@gmail.com/
>
> Since we got no human reviews I sent out the AI comments.
> LMK if you plan to respin, please.

I think I will go through and respin. Not all the comments are really
applicable, but I think it may have pointed out a few items that can
be addressed in terms of the documentation and some of the naming.