[PATCH] net: pcs: enable autonegotiation for 10g-usxgmii

[PATCH] net: pcs: enable autonegotiation for 10g-usxgmii

[Patryk Biel]

The Lynx PCS USXGMII setup programs the replicator advertisement, but
does not explicitly enable and restart in-band autonegotiation or program
the replicator link timers.

This leaves the PCS dependent on firmware or bootloader state. Systems
which do not get the USXGMII replicator preconfigured before Linux may
therefore fail to negotiate the link correctly.

After programming the USXGMII device ability, configure the replicator
BMCR with reset, autonegotiation enable and autonegotiation restart. Also
program the replicator link timer registers with the values used by the
ENETC/Felix setup.

Signed-off-by: Patryk Biel <pbiel7@gmail.com>
---
This is a follow-up to the discussion started here:

Link: https://lore.kernel.org/netdev/CA+DkFDaW_wJ5p9_P7pMpz-8iE6xeKkdF-MQcd2m2GcyVUE3S4Q@mail.gmail.com/

To summarize: on systems that don't rely on the U-Boot Felix switch
driver to pre-configure 10G-QXGMII in-band autonegotiation, the Lynx
PCS USXGMII code programs the replicator device ability but never
actually enables/restarts autonegotiation nor sets up the replicator
link timers. This leaves link establishment dependent on bootloader
state that isn't guaranteed to exist.

This series only addresses the PCS side of the problem: it configures
the USXGMII replicator BMCR (reset/AN enable/AN restart) and the link
timer registers whenever lynx_pcs_config_usxgmii() is called, so that
in-band AN comes up correctly regardless of what the bootloader did.

It intentionally does NOT yet remove the "only supports in-band AN for
now" limitation, nor does it wire up neg_mode-based configuration to
support the managed = "in-band-status" property being absent from the
device tree. That part still needs more work/testing on my side (in
particular the in-band-disable path isn't behaving as expected yet
with the PHY I'm testing against), and I'd like to discuss the right
approach for it separately before sending a follow-up series.

Feedback welcome, especially on whether this is an acceptable
incremental step or whether it should be bundled together with the
neg_mode/in-band-disable work
---
 drivers/net/pcs/pcs-lynx.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index a92081560e641ad2b297b7395cc72bf2f59ea16c..33655079fe51dba9dc96b0f7a79c4303e9492463 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -20,6 +20,9 @@
 #define IF_MODE_SPEED_MSK		GENMASK(3, 2)
 #define IF_MODE_HALF_DUPLEX		BIT(4)
 
+#define ENETC_PCS_REPL_LINK_TIMER_1_DEF	0x0003
+#define ENETC_PCS_REPL_LINK_TIMER_2_DEF	0x06a0
+
 struct lynx_pcs {
 	struct phylink_pcs pcs;
 	struct mdio_device *mdio;
@@ -154,6 +157,7 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
 				   const unsigned long *advertising,
 				   unsigned int neg_mode)
 {
+	int ret;
 	struct mii_bus *bus = pcs->bus;
 	int addr = pcs->addr;
 
@@ -164,10 +168,38 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
 	}
 
 	/* Configure device ability for the USXGMII Replicator */
-	return mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
-				 MDIO_USXGMII_10G | MDIO_USXGMII_LINK |
-				 MDIO_USXGMII_FULL_DUPLEX |
-				 ADVERTISE_SGMII | ADVERTISE_LPACK);
+	ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
+				MDIO_USXGMII_10G | MDIO_USXGMII_LINK |
+				MDIO_USXGMII_FULL_DUPLEX |
+				ADVERTISE_SGMII | ADVERTISE_LPACK);
+	if (ret < 0) {
+		dev_err(&pcs->dev, "could not set USXGMII replicator config\n");
+		return ret;
+	}
+
+	/* Configure autonegotiation */
+	ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_BMCR,
+				BMCR_RESET | BMCR_ANENABLE | BMCR_ANRESTART);
+	if (ret < 0) {
+		dev_err(&pcs->dev, "could not set USXGMII replicator control config\n");
+		return ret;
+	}
+
+	ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, LINK_TIMER_LO,
+				ENETC_PCS_REPL_LINK_TIMER_1_DEF);
+	if (ret < 0) {
+		dev_err(&pcs->dev, "could not set USXGMII Link Timer 1\n");
+		return ret;
+	}
+
+	ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, LINK_TIMER_HI,
+				ENETC_PCS_REPL_LINK_TIMER_2_DEF);
+	if (ret < 0) {
+		dev_err(&pcs->dev, "could not set USXGMII Link Timer 2\n");
+		return ret;
+	}
+
+	return ret;
 }
 
 static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,

---
base-commit: 91ec2035134982b98fab0609a9fd8480e8217dc1
change-id: 20260820-b4-fix-pcs-lynx-an-3fd1d5e94ce6

Best regards,
-- 
Patryk Biel <pbiel7@gmail.com>

Re: [PATCH] net: pcs: enable autonegotiation for 10g-usxgmii

[Andrew Lunn]

> @@ -154,6 +157,7 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
>  				   const unsigned long *advertising,
>  				   unsigned int neg_mode)
>  {
> +	int ret;
>  	struct mii_bus *bus = pcs->bus;
>  	int addr = pcs->addr;

Reverse Christmas tree. Please sort the lines longest to shortest.
	
    Andrew

---
pw-bot: cr

Re: [PATCH] net: pcs: enable autonegotiation for 10g-usxgmii

[Ioana Ciornei]

On Thu, Aug 20, 2026 at 07:41:40PM +0200, Patryk Biel wrote:
> The Lynx PCS USXGMII setup programs the replicator advertisement, but
> does not explicitly enable and restart in-band autonegotiation or program
> the replicator link timers.
> 
> This leaves the PCS dependent on firmware or bootloader state. Systems
> which do not get the USXGMII replicator preconfigured before Linux may
> therefore fail to negotiate the link correctly.
> 
> After programming the USXGMII device ability, configure the replicator
> BMCR with reset, autonegotiation enable and autonegotiation restart. Also
> program the replicator link timer registers with the values used by the
> ENETC/Felix setup.
> 

For v2 please explicitly target the net tree by changing the subject
prefix to "PATCH net ".

> Signed-off-by: Patryk Biel <pbiel7@gmail.com>
> ---
> This is a follow-up to the discussion started here:
> 
> Link: https://lore.kernel.org/netdev/CA+DkFDaW_wJ5p9_P7pMpz-8iE6xeKkdF-MQcd2m2GcyVUE3S4Q@mail.gmail.com/
> 
> To summarize: on systems that don't rely on the U-Boot Felix switch
> driver to pre-configure 10G-QXGMII in-band autonegotiation, the Lynx
> PCS USXGMII code programs the replicator device ability but never
> actually enables/restarts autonegotiation nor sets up the replicator
> link timers. This leaves link establishment dependent on bootloader
> state that isn't guaranteed to exist.
> 
> This series only addresses the PCS side of the problem: it configures
> the USXGMII replicator BMCR (reset/AN enable/AN restart) and the link
> timer registers whenever lynx_pcs_config_usxgmii() is called, so that
> in-band AN comes up correctly regardless of what the bootloader did.
> 
> It intentionally does NOT yet remove the "only supports in-band AN for
> now" limitation, nor does it wire up neg_mode-based configuration to
> support the managed = "in-band-status" property being absent from the
> device tree. That part still needs more work/testing on my side (in
> particular the in-band-disable path isn't behaving as expected yet
> with the PHY I'm testing against), and I'd like to discuss the right
> approach for it separately before sending a follow-up series.
> 
> Feedback welcome, especially on whether this is an acceptable
> incremental step or whether it should be bundled together with the
> neg_mode/in-band-disable work
> ---
>  drivers/net/pcs/pcs-lynx.c | 40 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
> index a92081560e641ad2b297b7395cc72bf2f59ea16c..33655079fe51dba9dc96b0f7a79c4303e9492463 100644
> --- a/drivers/net/pcs/pcs-lynx.c
> +++ b/drivers/net/pcs/pcs-lynx.c
> @@ -20,6 +20,9 @@
>  #define IF_MODE_SPEED_MSK		GENMASK(3, 2)
>  #define IF_MODE_HALF_DUPLEX		BIT(4)
>  
> +#define ENETC_PCS_REPL_LINK_TIMER_1_DEF	0x0003
> +#define ENETC_PCS_REPL_LINK_TIMER_2_DEF	0x06a0
> +
>  struct lynx_pcs {
>  	struct phylink_pcs pcs;
>  	struct mdio_device *mdio;
> @@ -154,6 +157,7 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
>  				   const unsigned long *advertising,
>  				   unsigned int neg_mode)
>  {
> +	int ret;
>  	struct mii_bus *bus = pcs->bus;
>  	int addr = pcs->addr;
>  
> @@ -164,10 +168,38 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
>  	}
>  
>  	/* Configure device ability for the USXGMII Replicator */
> -	return mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
> -				 MDIO_USXGMII_10G | MDIO_USXGMII_LINK |
> -				 MDIO_USXGMII_FULL_DUPLEX |
> -				 ADVERTISE_SGMII | ADVERTISE_LPACK);
> +	ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_ADVERTISE,
> +				MDIO_USXGMII_10G | MDIO_USXGMII_LINK |
> +				MDIO_USXGMII_FULL_DUPLEX |
> +				ADVERTISE_SGMII | ADVERTISE_LPACK);
> +	if (ret < 0) {
> +		dev_err(&pcs->dev, "could not set USXGMII replicator config\n");
> +		return ret;
> +	}
> +
> +	/* Configure autonegotiation */
> +	ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_BMCR,
> +				BMCR_RESET | BMCR_ANENABLE | BMCR_ANRESTART);
> +	if (ret < 0) {
> +		dev_err(&pcs->dev, "could not set USXGMII replicator control config\n");
> +		return ret;
> +	}

Doesn't it make more sense to issue AN restart after you configure the
link timer?

> +
> +	ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, LINK_TIMER_LO,
> +				ENETC_PCS_REPL_LINK_TIMER_1_DEF);
> +	if (ret < 0) {
> +		dev_err(&pcs->dev, "could not set USXGMII Link Timer 1\n");
> +		return ret;
> +	}

Please use the link timer value returned by phylink_get_link_timer_ns()
instead of the hardcoded values brought from the u-boot driver. Keep in
mind that the value to be written in LINK_TIMER_LO and LINK_TIMER_HI is
in steps of 3.2 ns.

Ioana