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

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 <[email protected]>
> ---
> 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