Re: [PATCH net-next 2/2] net: pcs: xpcs: select operating mode for 10G-baseR capable PCS

[Daniel Thompson <daniel@riscstar.com>]

On Mon, Oct 19, 2026 at 05:44:53PM +0100, Daniel Thompson wrote:
> On Sun, Oct 18, 2026 at 05:48:39PM +0200, Andrew Lunn wrote:
> > > default value after a reset prevents <=1G modes from working (I think
> > > automatic switch over between 1G and 2.5G also liked this to a reserved
> > > value but I don't fully recall right now).
> > >
> > > On that basis the best bit pattern to use one that isn't implemented by
> > > whatever XPCS we happen to be running on. One simple approach might be
> > > keep using 40GBASE-R but to check PCS Status 2 to ensure we fail in a
> > > non-silent manner on hypothesized future XPCS hardware:
> > >
> > > 	if (mdio_stat2 & MDIO_PCS_STAT2_10GBR) {
> > > 		if (mdio_stat2 & MDIO_PCS_STAT2_40GBR) {
> > > 			dev_err(dev, "40GBASE-R is *not* a reserved "
> > > 			              value on this XPCS");
> > > 			return -ENOTSUP;
> > > 		}
> >
> > Maybe. But we need to ensure it does not cause regressions for fbnic,
> > which might actually support this speed.
>
> I will sketch out an idea based on explicitly checking both my
> assumptions hold by checking PCS Status 2 and PCS Control 2 after
> a soft reset. Thus if you never reset then the code will never
> deploy. Likewise we don't need to rely on my guess that all XPCS
> implementations will reset PCS_TYPE_SEL to 0.
>
> I will also, given everything I've had to share by e-mail,
> improve the commenting a lot!

Patches will follow in due course but in case anyone is curious in the
mean time, that sketched out idea becomes the following. I had to read
the TRM very closely but we can avoid using the obviously weird 40GBR
value (the key information that allowed this version to write a better
value to PCS Control 2 turned out to be a missing bit pattern in a very
narrow column that I must have read tens of times without noticing).

Anyhow, pcs_type_sel_quirk is true if and only if my assumptions hold
(explicit test after a soft reset that 10GBR is supported and that
PCS_TYPE_SEL is 0 after reset). This hugely drops the risk of
regression:

+	/*
+	 * XPCS (at least in some implementations) is a little odd in it's
+	 * handling of PCS CTRL2:PCS_TYPE_SEL. Instead of PCS CTRL1:SPEED
+	 * SELECTION controlling how PCS_TYPE_SEL is interpreted it is the other
+	 * way around: the speed selection is only honored when PCS TYPE SEL has
+	 * specific values.
+	 *
+	 * This is a particular issue on XPCS implementations that implement
+	 * 10GBASE-R because that will be the reset value for PCS_TYPE_SEL. That
+	 * means that at reset, the system does not honor speed selection by
+	 * default. This forces us to recognize an XPCS with this property and
+	 * update PCS_TYPE_SEL accordingly.
+	 */
+	if (xpcs->pcs_type_sel_quirk) {
+		switch (interface) {
+		case PHY_INTERFACE_MODE_SGMII:
+		case PHY_INTERFACE_MODE_1000BASEX:
+		case PHY_INTERFACE_MODE_2500BASEX:
+			/* Really means 10GBX unless speed sel says otherwise */
+			mdio_ctrl2 = MDIO_PCS_CTRL2_10GBX;
+			break;
+		default:
+			/* Restore the reset value for unmatched cases */
+			mdio_ctrl2 = MDIO_PCS_CTRL2_10GBR;
+			break;
+		}
+
+		ret = xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2, mdio_ctrl2);
+		if (ret < 0)
+			return ret;
+	}

The code still doesn't entirely spark joy but I think at this point I
can blame the hardware!


Daniel.