[PATCH net-next 0/2] net: pcs: xpcs: fix operating mode selection

[PATCH net-next 0/2] net: pcs: xpcs: fix operating mode selection

[Alex Elder]

The XPCS found on Toshiba TC9564 is unable to operate at 2500base-X
and slower with a PHY connected using SGMII/2500base-X.  This XPCS
supports 10Gbase-R, which is the default operating mode, and this
causes mode selection to fail for slower interfaces (SGMII, 1000baseX,
and 2500baseX).  Writing a reserved value to the MDIO_CTRL2 register
avoids this failure.

Wangxun already addresses a similar scenario, but includes custom
quirk handling.

This series implements handling for slower rates for an XPCS that
supports 10Gbase-R.  The first patch rearranges code that performs
soft reset so that it is done *before* the interface mode is
switched.  The second adds code to xpcs_switch_interface_mode()
to update the MDIO_CTRL2 register for interfaces that support
10Gbase-R so that the desired rate change works as intended.

					-Alex

Daniel Thompson (2):
  net: pcs: xpcs: re-order xpcs_pre_config() to update after the reset
  net: pcs: xpcs: select operating mode for 10G-baseR capable PCS

 drivers/net/pcs/pcs-xpcs.c | 94 +++++++++++++++++++++++++++-----------
 1 file changed, 67 insertions(+), 27 deletions(-)


base-commit: 05b5019b01fb9443b09ae4ed34af1135a80a84c8
-- 
2.53.0

[PATCH net-next 1/2] net: pcs: xpcs: re-order xpcs_pre_config() to update after the reset

[Alex Elder]

From: Daniel Thompson <daniel@riscstar.com>

Currently, on Wangxun platforms, the XPCS is configured during the call
to xpcs_switch_interface_mode() and, if the need_reset flag is set, the
XPCS is reset and the configuration will be lost. This is harmless at
present because need_reset will never actually be set on these platforms.

Nevertheless having xpcs_switch_interface_mode() on the wrong side of
the reset is an obstacle for future changes where wiping out programmed
configuration with a reset could be harmful.

Reorder xpcs_pre_config() so the reset can happen before we switch
interface mode. To make this work we have to hoist the special case
logic for SGMII into the parent function.

Signed-off-by: Daniel Thompson <daniel@riscstar.com>
Co-developed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/net/pcs/pcs-xpcs.c | 59 ++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index b415b93d77c15..49abf5ea6f5cc 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -705,46 +705,49 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces)
 static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs,
 				      phy_interface_t interface)
 {
-	int ret = 0;
+	/* Wangxun provides a full alternative implementation to handle quirks */
+	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
+		return txgbe_xpcs_switch_mode(xpcs, interface);
 
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
-		ret = txgbe_xpcs_switch_mode(xpcs, interface);
-	} else if (xpcs->interface != interface) {
-		if (interface == PHY_INTERFACE_MODE_SGMII)
-			xpcs->need_reset = true;
-		xpcs->interface = interface;
-	}
+	xpcs->interface = interface;
 
-	return ret;
+	return 0;
 }
 
 static void xpcs_pre_config(struct phylink_pcs *pcs, phy_interface_t interface)
 {
 	struct dw_xpcs *xpcs = phylink_pcs_to_xpcs(pcs);
+	struct device *dev = &xpcs->mdiodev->dev;
 	const struct dw_xpcs_compat *compat;
+	bool force_reset;
 	int ret;
 
+	/*
+	 * According to the XPCS datasheet, a soft reset is required to initiate
+	 * Clause 37 auto-negotiation when the XPCS switches interface modes.
+	 */
+	force_reset = interface == PHY_INTERFACE_MODE_SGMII;
+
+	if (force_reset || xpcs->need_reset) {
+		compat = xpcs_find_compat(xpcs, interface);
+		if (!compat) {
+			dev_err(dev, "unsupported interface %s\n",
+				phy_modes(interface));
+			return;
+		}
+
+		ret = xpcs_soft_reset(xpcs, compat);
+		if (ret) {
+			dev_err(dev, "soft reset failed: %pe\n", ERR_PTR(ret));
+			return;
+		}
+
+		xpcs->need_reset = false;
+	}
+
 	ret = xpcs_switch_interface_mode(xpcs, interface);
 	if (ret)
-		dev_err(&xpcs->mdiodev->dev, "switch interface failed: %pe\n",
-			ERR_PTR(ret));
-
-	if (!xpcs->need_reset)
-		return;
-
-	compat = xpcs_find_compat(xpcs, interface);
-	if (!compat) {
-		dev_err(&xpcs->mdiodev->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",
-			ERR_PTR(ret));
-
-	xpcs->need_reset = false;
+		dev_err(dev, "switch interface failed: %pe\n", ERR_PTR(ret));
 }
 
 static int xpcs_config_aneg_c37_sgmii(struct dw_xpcs *xpcs,
-- 
2.53.0

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

[Alex Elder]

From: Daniel Thompson <daniel@riscstar.com>

Currently the XPCS found on Toshiba TC9564 is unable to operate at
2500base-X and slower with a PHY connected using SGMII/2500base-X
(in our case a Qualcomm QCA8081).

The problem arises because this XPCS supports 10Gbase-R. That means
that the reset value of SR_XS_PCS_CTRL2:PCS_TYPE_SEL (0) is valid and
this suppresses the modal switching based on bit 13 of SR_PMA_CTRL1
or SR_XS_PCS_CTRL1.

A fix for this behaviour is implemented by txgbe_xpcs_switch_mode()
as part of the quirks for WangXun devices.

Rather than introduce another quirk for TC9564 let's attempt so solve
this generically by setting SR_XS_PCS_CTRL2:PCS_TYPE_SEL to a reserved
value when we detect the right combination of phy interface and XPCS
feature support.

The generic strategy adopted requires the default value of PCS_TYPE_SEL
to be 0 on devices that support 10Gbase-R. Based on TC9564 documentation
and the logic already implemented for WangXun I believe this is likely
to be the case for currently supported XPCS devices. Sadly I don't have
access to generic XPCS docs to confirm. However I think the benefits
of avoiding a cargo culted quirk outweighs the risk of regression.

Signed-off-by: Daniel Thompson <daniel@riscstar.com>
Co-developed-by: Alex Elder <elder@riscstar.com>
Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/net/pcs/pcs-xpcs.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 49abf5ea6f5cc..5316cf9c31f19 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -705,10 +705,47 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces)
 static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs,
 				      phy_interface_t interface)
 {
+	int mdio_stat2, ret;
+
 	/* Wangxun provides a full alternative implementation to handle quirks */
 	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
 		return txgbe_xpcs_switch_mode(xpcs, interface);
 
+	mdio_stat2 = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
+	if (mdio_stat2 < 0)
+		return mdio_stat2;
+
+	/*
+	 * If this XPCS supports 10Gbase-R then that will be the default
+	 * operating mode. There are several interface modes where this default
+	 * is unhelpful. Change the operating mode for interfaces were we know
+	 * the default is wrong, and restore the default otherwise.
+	 */
+	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;
+		}
+		if (ret < 0)
+			return ret;
+	}
+
 	xpcs->interface = interface;
 
 	return 0;
-- 
2.53.0

Re: [PATCH net-next 0/2] net: pcs: xpcs: fix operating mode selection

[Andrew Lunn]

On Wed, Oct 14, 2026 at 05:30:57PM -0500, Alex Elder wrote:
> The XPCS found on Toshiba TC9564 is unable to operate at 2500base-X
> and slower with a PHY connected using SGMII/2500base-X.  This XPCS
> supports 10Gbase-R, which is the default operating mode, and this
> causes mode selection to fail for slower interfaces (SGMII, 1000baseX,
> and 2500baseX).  Writing a reserved value to the MDIO_CTRL2 register
> avoids this failure.
> 
> Wangxun already addresses a similar scenario, but includes custom
> quirk handling.
> 
> This series implements handling for slower rates for an XPCS that
> supports 10Gbase-R.  The first patch rearranges code that performs
> soft reset so that it is done *before* the interface mode is
> switched.  The second adds code to xpcs_switch_interface_mode()
> to update the MDIO_CTRL2 register for interfaces that support
> 10Gbase-R so that the desired rate change works as intended.

You Cc: those reported by get_maintainers, but i think a few other
people should be involved, who have made changes in this area:

Coia Prant <coiaprant@gmail.com>
Choong Yong Liang <yong.liang.choong@linux.intel.com>
Vladimir Oltean <vladimir.oltean@nxp.com>
Jiawen Wu <jiawenwu@trustnetic.com>

       Andrew

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

[Andrew Lunn]

I don't know this PCS driver, so maybe dumb questions....

> Currently the XPCS found on Toshiba TC9564 is unable to operate at
> 2500base-X and slower with a PHY connected using SGMII/2500base-X
> (in our case a Qualcomm QCA8081).

Is the PHY actually relevant here? What about SGMII/2500base-X for an
SFP?

Is this purely a PCS problem, or does what the PCS connect to make a
difference?

> The problem arises because this XPCS supports 10Gbase-R. That means
> that the reset value of SR_XS_PCS_CTRL2:PCS_TYPE_SEL (0) is valid and
> this suppresses the modal switching based on bit 13 of SR_PMA_CTRL1
> or SR_XS_PCS_CTRL1.

~/linux/drivers/net/pcs$ grep SR_XS_PCS_CTRL1 *
~/linux/drivers/net/pcs$ grep SR_PMA_CTRL1 *
~/linux/drivers/net/pcs$

This is not helping me understand what is going on here.

> +	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).

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?

	Andrew

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

[Daniel Thompson]

On Thu, Oct 15, 2026 at 11:09:14PM +0200, Andrew Lunn wrote:
> I don't know this PCS driver, so maybe dumb questions....
>
> > Currently the XPCS found on Toshiba TC9564 is unable to operate at
> > 2500base-X and slower with a PHY connected using SGMII/2500base-X
> > (in our case a Qualcomm QCA8081).
>
> Is the PHY actually relevant here? What about SGMII/2500base-X for an
> SFP?

No, I don't think the phy is relevant here (the bit in brackets just
records the specific hardware combination that we observed the problem
on).


> Is this purely a PCS problem, or does what the PCS connect to make a
> difference?

AFAIK it is purely a PCS problem (but it's all soldered down so I have
no chance to prove I am right by testing it).


> > The problem arises because this XPCS supports 10Gbase-R. That means
> > that the reset value of SR_XS_PCS_CTRL2:PCS_TYPE_SEL (0) is valid and
> > this suppresses the modal switching based on bit 13 of SR_PMA_CTRL1
> > or SR_XS_PCS_CTRL1.
>
> ~/linux/drivers/net/pcs$ grep SR_XS_PCS_CTRL1 *
> ~/linux/drivers/net/pcs$ grep SR_PMA_CTRL1 *
> ~/linux/drivers/net/pcs$
>
> This is not helping me understand what is going on here.

These names are exactly as they appear in the technical documentation
for the TC9564. I assumed these to be inherited from the Synopsis XPCS
docs and would make sense in the context of the XPCS driver.

However, now you mention it I can see that "Speed selection bit (13) of
PCS control 1" would be considerably clearer for reviewers who take a
wider view! Will fix this and will also be a bit clearer about how XPCS
treats PCS_TYPE_SEL.


> > +	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).
>
> 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.

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.

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;
		}


		switch (interface) {
		case PHY_INTERFACE_MODE_SGMII:
		case PHY_INTERFACE_MODE_1000BASEX:
		case PHY_INTERFACE_MODE_2500BASEX:
			/* better comment goes here */
			ret = xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2,
					 MDIO_PCS_CTRL2_40GBR);
			...
		}
		...
	}

Given this function would almost certainly need other changes to
support different speeds this shoudn't be too much of a hardship
for whoever ends up working on a 40GBASE-R-capable XPCS.

I assume it would be OK to define the two new values
(MDIO_PCS_STAT2_40GBR and MDIO_PCS_CTRL2_40GBR) in the uapi?


Daniel.

Re: [PATCH net-next 1/2] net: pcs: xpcs: re-order xpcs_pre_config() to update after the reset

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] This patch adds a generic XPCS soft reset on Wangxun TXGBE
  parts every time SGMII is configured.
- [Medium] The patch drops the interface-change check that gated the
  SGMII reset, while the new comment still says the reset happens on a…
- [Medium] The commit message describes a preparatory reorder that just
  'hoist[s] the special case logic for SGMII into the parent function',…
- [Low] The error handling in xpcs_pre_config() now behaves differently,
  and the commit message does not mention it.

--

net: pcs: xpcs: re-order xpcs_pre_config() to update after the reset

This commit reorders xpcs_pre_config() so that the XPCS soft reset runs
before xpcs_switch_interface_mode(). The SGMII reset trigger moves out of
xpcs_switch_interface_mode() and into xpcs_pre_config().

> Currently, on Wangxun platforms, the XPCS is configured during the call
> to xpcs_switch_interface_mode() and, if the need_reset flag is set, the
> XPCS is reset and the configuration will be lost. This is harmless at
> present because need_reset will never actually be set on these platforms.

[ ... ]

> Reorder xpcs_pre_config() so the reset can happen before we switch
> interface mode. To make this work we have to hoist the special case
> logic for SGMII into the parent function.

[Severity: Medium]
Is this description complete? The patch seems to widen the reset policy,
not just move it.

The old SGMII branch ran only when xpcs->interface != interface. It also
sat in the else branch of the WX_TXGBE_XPCS_PMA_10G_ID check.

The new force_reset condition in xpcs_pre_config() keeps neither of those
checks. Every SGMII pcs_pre_config() now does a real BMCR_RESET write and
poll on every XPCS, Wangxun included (see the inline comments below).

The commit message says Wangxun is safe because need_reset is never set
there, but that doesn't cover the new force_reset path. The last patch in
the series doesn't touch xpcs_pre_config(), so this is still the case at
the end of the series.

Could the commit message describe this change in runtime behaviour?

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index b415b93d77c15..49abf5ea6f5cc 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -705,46 +705,49 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces)
>  static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs,
>  				      phy_interface_t interface)
>  {
> -	int ret = 0;
> +	/* Wangxun provides a full alternative implementation to handle quirks */
> +	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
> +		return txgbe_xpcs_switch_mode(xpcs, interface);
>  
> -	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
> -		ret = txgbe_xpcs_switch_mode(xpcs, interface);
> -	} else if (xpcs->interface != interface) {
> -		if (interface == PHY_INTERFACE_MODE_SGMII)
> -			xpcs->need_reset = true;
> -		xpcs->interface = interface;
> -	}
> +	xpcs->interface = interface;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  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",
> -			ERR_PTR(ret));
> +	/*
> +	 * According to the XPCS datasheet, a soft reset is required to initiate
> +	 * Clause 37 auto-negotiation when the XPCS switches interface modes.
> +	 */
> +	force_reset = interface == PHY_INTERFACE_MODE_SGMII;

[Severity: Medium]
Does this now send a generic soft reset to Wangxun TXGBE parts every time
SGMII is configured?

Nothing here excludes WX_TXGBE_XPCS_PMA_10G_ID. Wangxun uses
synopsys_xpcs_compat, so xpcs_find_compat() returns the DW_AN_C37_SGMII
entry. xpcs_soft_reset() then writes BMCR_RESET to MDIO_MMD_VEND2 and
polls in xpcs_poll_reset(), with a 50 ms initial sleep and a 600 ms
timeout.

Until now Wangxun was kept out of the generic reset. xpcs_create() never
sets need_reset for it:

	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID ||
	    xpcs->info.pma == MP_FBNIC_XPCS_PMA_100G_ID)
		xpcs->pcs.poll = false;
	else
		xpcs->need_reset = true;

Wangxun also has its own sequence in txgbe_xpcs_switch_mode(). It first
waits for power-up-good in txgbe_pcs_poll_power_up(), then programs the
PCS type, PMA and MPLL. It ends with its own DW_VR_RST in
txgbe_pma_init_done().

The new reset now runs before that power-up-good wait. If it fails or
times out, xpcs_pre_config() returns without calling
txgbe_xpcs_switch_mode(). The SerDes could then keep its old
programming, for example the 10GBASE-R PCS type and 10G MPLL values that
txgbe_xpcs_mode_quirk() describes. phylink still goes on to pcs_config
for SGMII.

This path is reachable because txgbe_phylink_init() adds
PHY_INTERFACE_MODE_SGMII to supported_interfaces for SFP media, for
example with a copper SFP module:

phylink_start()
  phylink_mac_initial_config()
    phylink_major_config()
      phylink_pcs_pre_config()
        xpcs_pre_config()
          xpcs_soft_reset()

Should force_reset exclude WX_TXGBE_XPCS_PMA_10G_ID? The final patch in
the series ("select operating mode for 10G-baseR capable PCS") leaves
xpcs_pre_config() unchanged, so this seems to remain at the end of the
series.

[Severity: Medium]
The comment says the reset is needed when the XPCS switches interface
modes. Does the code still check for an actual switch?

Commit 065d3cef99a1 ("net: pcs: xpcs: re-initiate clause 37
Auto-negotiation") set need_reset only inside this check:

	} else if (xpcs->interface != interface) {

After this patch, the generic path writes xpcs->interface but never
compares it. Every phylink_major_config() that requests SGMII now does a
VEND2 BMCR_RESET plus at least a 50 ms sleep in xpcs_poll_reset(), even
when the interface hasn't changed.

That covers phylink_start() and phylink_resume() through
phylink_mac_initial_config(), and phylink_resolve() with
force_major_config. Non-Wangxun SGMII users are affected, for example
stmmac based Intel mGbE and SJA1105/SJA1110.

Was this intended? If so, should the comment be updated? If not, should
force_reset also check xpcs->interface != interface? The last patch in
the series doesn't change this.

>  
> -	if (!xpcs->need_reset)
> -		return;
> +	if (force_reset || xpcs->need_reset) {
> +		compat = xpcs_find_compat(xpcs, interface);
> +		if (!compat) {
> +			dev_err(dev, "unsupported interface %s\n",
> +				phy_modes(interface));
> +			return;
> +		}
>  
> -	compat = xpcs_find_compat(xpcs, interface);
> -	if (!compat) {
> -		dev_err(&xpcs->mdiodev->dev, "unsupported interface %s\n",
> -			phy_modes(interface));
> -		return;
> +		ret = xpcs_soft_reset(xpcs, compat);
> +		if (ret) {
> +			dev_err(dev, "soft reset failed: %pe\n", ERR_PTR(ret));
> +			return;
> +		}

[Severity: Low]
Is this early return intended? Before, xpcs_switch_interface_mode()
always ran, and need_reset was cleared even when the reset failed.

Now, if xpcs_find_compat() or xpcs_soft_reset() fails,
xpcs_switch_interface_mode() is skipped and need_reset stays set.
pcs_pre_config returns void, so phylink still continues into pcs_config.

On Wangxun this means txgbe_xpcs_switch_mode() is skipped. By the end of
the series ("net: pcs: xpcs: select operating mode for 10G-baseR capable
PCS"), xpcs_switch_interface_mode() also writes the MDIO_CTRL2 PCS type.
A failed reset would then leave a 10GBASE-R capable PCS in its default
type.

need_reset is no longer cleared on failure, so every later pre_config
also retries the reset. Should the commit message mention this change in
error handling?

> +
> +		xpcs->need_reset = false;
>  	}
>  
> -	ret = xpcs_soft_reset(xpcs, compat);
> +	ret = xpcs_switch_interface_mode(xpcs, interface);
>  	if (ret)
> -		dev_err(&xpcs->mdiodev->dev, "soft reset failed: %pe\n",
> -			ERR_PTR(ret));
> -
> -	xpcs->need_reset = false;
> +		dev_err(dev, "switch interface failed: %pe\n", ERR_PTR(ret));
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014223100.304486-1-elder%40riscstar.com

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The new 10GBASE-R check in xpcs_switch_interface_mode()
  (drivers/net/pcs/pcs-xpcs.c:714-724) reads PCS MMD 3 register 3.8…
- [Medium] When MDIO_STAT2 bit 0 is set, the default branch of
  xpcs_switch_interface_mode() (pcs-xpcs.c:740-743) writes…

--

net: pcs: xpcs: select operating mode for 10G-baseR capable PCS

This patch makes xpcs_switch_interface_mode() read the PCS MDIO_STAT2
register. When the 10GBASE-R capability bit is set, it programs
SR_XS_PCS_CTRL2:PCS_TYPE_SEL: SGMII, 1000BASE-X and 2500BASE-X get a
reserved value, and every other interface gets the 10GBASE-R type. The
aim is to make the TC9564 XPCS work with a QCA8081 PHY without adding
another quirk.

> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 49abf5ea6f5cc..5316cf9c31f19 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -705,10 +705,47 @@ static void xpcs_get_interfaces(struct dw_xpcs *xpcs, unsigned long *interfaces)
>  static int xpcs_switch_interface_mode(struct dw_xpcs *xpcs,
>  				      phy_interface_t interface)
>  {
> +	int mdio_stat2, ret;
> +
>  	/* Wangxun provides a full alternative implementation to handle quirks */
>  	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID)
>  		return txgbe_xpcs_switch_mode(xpcs, interface);
>  
> +	mdio_stat2 = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
> +	if (mdio_stat2 < 0)
> +		return mdio_stat2;
> +
> +	/*
> +	 * If this XPCS supports 10Gbase-R then that will be the default
> +	 * operating mode. There are several interface modes where this default
> +	 * is unhelpful. Change the operating mode for interfaces were we know
> +	 * the default is wrong, and restore the default otherwise.
> +	 */
> +	if (mdio_stat2 & MDIO_PCS_STAT2_10GBR) {

[Severity: Medium]
Can a missing PCS MMD be treated as 10GBASE-R capable here? The code tests
bit 0 of MDIO_STAT2 but does not check the device-present field in bits
15:14. An absent MMD 3 that reads back as 0xffff would pass the test.

The driver already expects MMD 3 to be absent on some parts.
xpcs_read_ids() falls back to MMD 31 when the IDs read as all zeros or all
ones:

    if (!id || id == 0xffffffff) {

On SJA1105R/S, the read side returns 0xffff for any MMD other than
VEND1/VEND2:

drivers/net/dsa/sja1105/sja1105_mdio.c:sja1105_pcs_mdio_read_c45() {
    ...
	if (mmd != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
		return 0xffff;
    ...
}

The write side rejects the new CTRL2 write:

drivers/net/dsa/sja1105/sja1105_mdio.c:sja1105_pcs_mdio_write_c45() {
    ...
	if (mdio != MDIO_MMD_VEND1 && mmd != MDIO_MMD_VEND2)
		return -EINVAL;
    ...
}

So for an SGMII port on SJA1105R/S:

xpcs_pre_config()
  xpcs_switch_interface_mode()
    xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2)       -> 0xffff
    xpcs_write(xpcs, MDIO_MMD_PCS, MDIO_CTRL2, 4)   -> -EINVAL

xpcs_switch_interface_mode() then returns without setting
xpcs->interface. xpcs_pre_config() would then log "switch interface
failed: -EINVAL" on every major config.

On SJA1110, sja1110_pcs_mdio_read_c45() and sja1110_pcs_mdio_write_c45()
don't filter by MMD, so MMD 3 accesses go to bank 0x300 of the PCS
window. If that location reads with bit 0 set, would 0x0004 be written
into undocumented switch register space?

Would it make sense to also require that
(mdio_stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL? Another
option is to run this only for descs whose compat tables include 10G
modes.

> +		switch (interface) {
> +		case PHY_INTERFACE_MODE_SGMII:
> +		case PHY_INTERFACE_MODE_1000BASEX:
> +		case PHY_INTERFACE_MODE_2500BASEX:

[ ... ]

> +			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;

[Severity: Medium]
Is 10GBASE-R the right PCS type for every interface that reaches this
default branch? The comment says "restore the default otherwise".
However, synopsys_xpcs_compat[] also lists 25GBASER, XLGMII, 50GBASER,
LAUI and 100GBASEP for DW_XPCS_ID. All of those would end up here with
PCS_TYPE_SEL set to 0 (10GBASE-R).

Before this patch, the generic path never wrote PCS CTRL2. The only CTRL2
writes were in the Wangxun code.

Meta fbnic is an in-tree user that reaches this branch:

- It doesn't use the Wangxun PMA ID.
- fbnic_phylink.c only enables 100GBASEP, 50GBASER, LAUI and 25GBASER.
- fbnic_mdio_read_pcs() doesn't intercept MDIO_STAT2, so the gating bit
  comes from the real PCS register.

fbnic_mdio_write_pcs() passes the CTRL2 write straight to hardware:

drivers/net/ethernet/meta/fbnic/fbnic_mdio.c:fbnic_mdio_write_pcs() {
    ...
	fbnic_wr32(fbd, FBNIC_PCS_PAGE(addr) + regnum, val);
}

If the fbnic PCS reports the 10GBR bit in STAT2, would every major config
force the PCS type to 10GBASE-R while the link runs at 25G, 50G or 100G?
I couldn't confirm from the tree whether that bit reads as 1 on fbnic.

> +		}
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	xpcs->interface = interface;
>  
>  	return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014223100.304486-1-elder%40riscstar.com

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

[Andrew Lunn]

On Fri, Oct 16, 2026 at 03:30:56PM +0100, Daniel Thompson wrote:
> On Thu, Oct 15, 2026 at 11:09:14PM +0200, Andrew Lunn wrote:
> > I don't know this PCS driver, so maybe dumb questions....
> >
> > > Currently the XPCS found on Toshiba TC9564 is unable to operate at
> > > 2500base-X and slower with a PHY connected using SGMII/2500base-X
> > > (in our case a Qualcomm QCA8081).
> >
> > Is the PHY actually relevant here? What about SGMII/2500base-X for an
> > SFP?
> 
> No, I don't think the phy is relevant here (the bit in brackets just
> records the specific hardware combination that we observed the problem
> on).
> 
> 
> > Is this purely a PCS problem, or does what the PCS connect to make a
> > difference?
> 
> AFAIK it is purely a PCS problem (but it's all soldered down so I have
> no chance to prove I am right by testing it).

Thanks. Given it was stated i was thinking it could be relevant. But
how can it be relevant? And then i thought that the PCS only supports
10Gbase-R, and the PHY is using rate adaptation to handle slower media
speeds?

That seemed unlikely.
 
> > > The problem arises because this XPCS supports 10Gbase-R. That means
> > > that the reset value of SR_XS_PCS_CTRL2:PCS_TYPE_SEL (0) is valid and
> > > this suppresses the modal switching based on bit 13 of SR_PMA_CTRL1
> > > or SR_XS_PCS_CTRL1.
> >
> > ~/linux/drivers/net/pcs$ grep SR_XS_PCS_CTRL1 *
> > ~/linux/drivers/net/pcs$ grep SR_PMA_CTRL1 *
> > ~/linux/drivers/net/pcs$
> >
> > This is not helping me understand what is going on here.
> 
> These names are exactly as they appear in the technical documentation
> for the TC9564. I assumed these to be inherited from the Synopsis XPCS
> docs and would make sense in the context of the XPCS driver.

I however don't have this documentation, just the driver, and 802.3.

> However, now you mention it I can see that "Speed selection bit (13) of
> PCS control 1" would be considerably clearer for reviewers who take a
> wider view! Will fix this and will also be a bit clearer about how XPCS
> treats PCS_TYPE_SEL.
> 
> 
> > > +	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).
> >
> > 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?

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

> 
> 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.

	Andrew