drivers/net/pcs/pcs-lynx.c | 53 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 4 deletions(-)
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 using the value returned by
phylink_get_link_timer_ns()
Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Patryk Biel <pbiel7@gmail.com>
---
Changes in v3:
- Add separate link timer macro for 10G-QXGMII (1/4 tick rate vs USXGMII).
- Select link timer macro based on interface mode.
- Link to v2: https://lore.kernel.org/r/20260824-b4-fix-pcs-lynx-an-v2-1-9bb1dec96f0b@gmail.com
Changes in v2:
- Reorder local variable declarations in lynx_pcs_config_usxgmii().
- Move USXGMII replicator link timer configuration before the autonegotiation restart.
- Use phylink_get_link_timer_ns() instead of hardcoded USXGMII
replicator link timer values, converting to 3.2 ns register step.
- Link to v1: https://lore.kernel.org/r/20260820-b4-fix-pcs-lynx-an-v1-1-62d66391eaff@gmail.com
---
drivers/net/pcs/pcs-lynx.c | 53 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 49 insertions(+), 4 deletions(-)
diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
index da4f99059eef7722a5c3bf32df490940cc1135f8..6d94f92a3332959bc4d39969a8ad781ac06951af 100644
--- a/drivers/net/pcs/pcs-lynx.c
+++ b/drivers/net/pcs/pcs-lynx.c
@@ -20,6 +20,12 @@
#define IF_MODE_SPEED_MSK GENMASK(3, 2)
#define IF_MODE_HALF_DUPLEX BIT(4)
+/* USXGMII replicator link timer step is 3.2 ns (312.5M XGMII columns per sec)
+ * for single port mode. For quad port mode, it is 1/4 of that.
+ */
+#define LINK_TIMER_VAL_USXGMII(ns) ((u32)((ns) * 10 / 32))
+#define LINK_TIMER_VAL_10G_QXGMII(ns) ((u32)((ns) * 10 / 128))
+
struct lynx_pcs {
struct phylink_pcs pcs;
struct mdio_device *mdio;
@@ -158,6 +164,9 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs,
{
struct mii_bus *bus = pcs->bus;
int addr = pcs->addr;
+ int link_timer_ns;
+ u32 link_timer;
+ int ret;
if (neg_mode != PHYLINK_PCS_NEG_INBAND_ENABLED) {
dev_err(&pcs->dev, "%s only supports in-band AN for now\n",
@@ -166,10 +175,46 @@ 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;
+ }
+
+ link_timer_ns = phylink_get_link_timer_ns(interface);
+ if (link_timer_ns > 0) {
+ if (interface == PHY_INTERFACE_MODE_10G_QXGMII)
+ link_timer = LINK_TIMER_VAL_10G_QXGMII(link_timer_ns);
+ else
+ link_timer = LINK_TIMER_VAL_USXGMII(link_timer_ns);
+
+ ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2,
+ LINK_TIMER_LO, link_timer & 0xffff);
+ 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, (link_timer >> 16) & 0x1f);
+ if (ret < 0) {
+ dev_err(&pcs->dev, "could not set USXGMII Link Timer 2\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;
+ }
+
+ return ret;
}
static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
---
base-commit: 10cfa109c880092df32e396647b4afdca9be8350
change-id: 20260820-b4-fix-pcs-lynx-an-3fd1d5e94ce6
Best regards,
--
Patryk Biel <pbiel7@gmail.com>Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 2
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] lynx_pcs_config_usxgmii() now writes BMCR_RESET |
BMCR_ANENABLE | BMCR_ANRESTART (drivers/net/pcs/pcs-lynx.c:210-211) on…
- [Low] The commit message describes a user-visible functional defect
('Systems which do not get the USXGMII replicator preconfigured before…
- [Low] The new comment at drivers/net/pcs/pcs-lynx.c:23-25 states
'USXGMII replicator link timer step is 3.2 ns (312.5M XGMII columns…
--
net: pcs: enable autonegotiation for 10g-usxgmii
The Lynx PCS USXGMII path previously only programmed the replicator
advertisement. This patch adds link timer programming derived from
phylink_get_link_timer_ns() and a BMCR write that asserts reset,
autonegotiation enable and autonegotiation restart.
No Link: tags are present in the commit header.
A few questions below.
> 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.
[Severity: Low]
The message describes a user-visible failure ("may therefore fail to
negotiate the link correctly"), but there is no Fixes: tag and no
statement of the intended tree.
git blame on the baseline shows the advertise-only USXGMII configuration
in lynx_pcs_config_usxgmii() came from 0da4c3d393e40e ("net: phy: add
Lynx PCS module"), and the PHY_INTERFACE_MODE_10G_QXGMII path this patch
also touches came from 76cd8a2ea98a3d.
Should this carry a Fixes: tag pointing at one of those, or should the
message instead say this is net-next completion of the USXGMII setup
rather than a fix for broken negotiation?
> diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c
> index da4f99059eef7..6d94f92a33329 100644
> --- a/drivers/net/pcs/pcs-lynx.c
> +++ b/drivers/net/pcs/pcs-lynx.c
> @@ -20,6 +20,12 @@
> #define IF_MODE_SPEED_MSK GENMASK(3, 2)
> #define IF_MODE_HALF_DUPLEX BIT(4)
>
> +/* USXGMII replicator link timer step is 3.2 ns (312.5M XGMII columns per sec)
> + * for single port mode. For quad port mode, it is 1/4 of that.
> + */
> +#define LINK_TIMER_VAL_USXGMII(ns) ((u32)((ns) * 10 / 32))
> +#define LINK_TIMER_VAL_10G_QXGMII(ns) ((u32)((ns) * 10 / 128))
[Severity: Low]
Do the comment and the macro contradict each other here?
The comment says the quad port step "is 1/4 of that", i.e. 1/4 of 3.2 ns
= 0.8 ns, which would need ns * 10 / 8. The macro uses ns * 10 / 128,
which is a 12.8 ns step, four times larger than 3.2 ns rather than a
quarter of it.
With phylink_get_link_timer_ns() returning 1600000 for
PHY_INTERFACE_MODE_10G_QXGMII, the macro yields 125000 ticks, while the
comment's wording implies 2000000.
If the intent is that the per-port column rate is 1/4, so the period is
4x, would something like "for quad port mode, each port sees 1/4 of the
column rate, so the step is 4x larger (12.8 ns)" match the macro?
> struct lynx_pcs {
> struct phylink_pcs pcs;
> struct mdio_device *mdio;
[ ... ]
> @@ -166,10 +175,46 @@ 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;
> + }
> +
> + link_timer_ns = phylink_get_link_timer_ns(interface);
> + if (link_timer_ns > 0) {
> + if (interface == PHY_INTERFACE_MODE_10G_QXGMII)
> + link_timer = LINK_TIMER_VAL_10G_QXGMII(link_timer_ns);
> + else
> + link_timer = LINK_TIMER_VAL_USXGMII(link_timer_ns);
[ ... ]
> + /* Configure autonegotiation */
> + ret = mdiobus_c45_write(bus, addr, MDIO_MMD_VEND2, MII_BMCR,
> + BMCR_RESET | BMCR_ANENABLE | BMCR_ANRESTART);
[Severity: Medium]
Can this bounce an established link?
The BMCR_RESET | BMCR_ANRESTART write happens on every successful
invocation of lynx_pcs_config_usxgmii(), with no check for whether the
interface mode, advertisement or link timer values actually changed.
The .pcs_config() contract in include/linux/phylink.h says:
* pcs_config() will be called when configuration of the PCS is required
* or when the advertisement is possibly updated. It must not unnecessarily
* disrupt an established link.
For a PCS-only in-band link (managed = "in-band-status" with no
phy-handle, so pl->phydev is NULL), a pause parameter change reaches this
path:
drivers/net/phy/phylink.c:phylink_update_pause_state() {
...
if (!pl->phydev)
phylink_change_inband_advert(pl);
...
}
phylink_change_inband_advert() then calls phylink_pcs_config()
unconditionally, so phylink_pcs_config()->lynx_pcs_config()->
lynx_pcs_config_usxgmii() soft-resets the replicator and restarts AN.
phylink_ethtool_ksettings_set() reaches the same helper for in-band
links.
Would it work to make the reset and restart conditional on an actual
configuration change (or on the PCS not yet being initialized), or to
return a positive value so phylink calls .pcs_an_restart() instead?
> + if (ret < 0) {
> + dev_err(&pcs->dev, "could not set USXGMII replicator control config\n");
> + return ret;
> + }
> +
> + return ret;
> }
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-b4-fix-pcs-lynx-an-v3-1-dda3ac4e499c%40gmail.com
On Tue, Sep 22, 2026 at 05:54:46PM +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 using the value returned by > phylink_get_link_timer_ns() > > Co-developed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Patryk Biel <pbiel7@gmail.com> > --- There are 2 problems with the commit message. First the prefix should be "net: pcs: lynx:", and second, you say you enable autoneg for 10g-usxgmii (which doesn't exist) instead of saying you enable it for 10g-qxgmii and usxgmii (to correctly reflect the code). You should probably fix both. Anyway, with both addressed: Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Hi, On Wed, Sep 23, 2026 at 5:05 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > There are 2 problems with the commit message. First the prefix should be > "net: pcs: lynx:", and second, you say you enable autoneg for 10g-usxgmii > (which doesn't exist) instead of saying you enable it for 10g-qxgmii and > usxgmii (to correctly reflect the code). You should probably fix both. > > Anyway, with both addressed: > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com> Thanks, I will send a v4 with fixed commit message. Best regards Patryk
© 2016 - 2026 Red Hat, Inc.