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

[Daniel Thompson <daniel@riscstar.com>]

On Sun, Oct 18, 2026 at 05:48:39PM +0200, Andrew Lunn wrote:
> > > > +	if (mdio_stat2 & MDIO_PCS_STAT2_10GBR) {
> > > > +		switch (interface) {
> > > > +		case PHY_INTERFACE_MODE_SGMII:
> > > > +		case PHY_INTERFACE_MODE_1000BASEX:
> > > > +		case PHY_INTERFACE_MODE_2500BASEX:
> > > > +			/*
> > > > +			 * Why are we writing MDIO_PCS_CTRL2_TYPE + 1? We want
> > > > +			 * the modal behaviour that comes when we pick a
> > > > +			 * reserved value. XPCS allocates extra bits to this
> > > > +			 * field and allocates values from 15 down so
> > > > +			 * MDIO_PCS_CTRL2_TYPE + 1 is the value likely to be
> > > > +			 * allocated last (and hopefully never).
> > > > +			 */
> > > > +			ret = xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2,
> > > > +					 MDIO_PCS_CTRL2_TYPE + 1);
> > > > +			break;
> > > > +		default:
> > > > +			ret = xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2,
> > > > +					 MDIO_PCS_CTRL2_10GBR);
> > > > +			break;
> > > > +		}
> > >
> > > MDIO_PCS_CTRL2_TYPE is 3. Looking at 802.3, the value 4 is
> > > 40GBASE-R. The 2022 version of the standard has values defined up to
> > > 10011b, for speeds up to 400G,
> > >
> > > Alex added support for 25G, 50G, 100G.
> > >
> > > Is this an issue?
> >
> > It will probably be an issue when the kernel is first called up on to
> > support an XPCS that supports 40GBASE-R.
>
> Which is likely to happen sometime in the future. Maybe Alex's
> hardware even supports it, but if you can do 50G, why bother with 40G?
>
>
> > The XPCS (at least in TC956x) handles MDIO_PCS_CTRL2 in a slightly odd way.
> > It 4 bits wide and accepts 10GBASE-R, 10GBASE-X, 5GBASE-R and 2.5GBASE-X
> > is defined in the standards up to 2018. All other bit patterns are
> > reserved and instruct the XPCS to use Speed selection bit (13) of PCS
> > control 1 to choose between 10GBASE-X or 1000BASE-X.
>
> We probably need Alex in this thread, what happens for 25G, 50G, 100G?

That would be very much appreciated (hi Alex)!

I have spent a fairly big chunk of today trying to "get back inside my
own head" since it's been quite a while since I last looked at this
code; Alex (Elder) and I were focusing on other bits.

I made two assumptions when I was reasoning about regressions:

1. No existing code ever wrote to PCS Control 2 and therefore the value
   after reset must be correct for existing working cases.

2. The reset value is bit pattern 0 (10GBASE-R).

The idea of the code above is designed to match only those cases where
we *know* that the reset bit pattern (0) will cause trouble and change
things. It's currently a very narrow match although I might expect
additional cases to be added as other phy interface modes are tested.

I have spotted a very obvious flaw in my reasoning above: the special
case logic for FBNIC that suppresses the initial XPCS reset.
Unfortunately the version history doesn't mention what bad thing we
can avoid by suppressing the reset (perhaps a reset can clobber values
such as PCS_TYPE_SEL that are injected by the firmware).


> Maybe there are different revisions of the IP, and later revisions
> make this register follow the standard?

I'm not able to answer that. All I can say is most XPCS seem to have the
same ID. Finger crossed that, if they change the behaviour they also
change the ID!


> > That means we *must* write a reserved value to this register because the
> > 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!


Daniel.