[PATCH net-next v6 0/2] net: macb: 1000BASE-X on internal PCS

[PATCH net-next v6 0/2] net: macb: 1000BASE-X on internal PCS

[Nathan Whitehorn]

This series adds support for 1000BASE-X autonegotiation to the Cadence macb
driver when using the MAC-internal PCS. The existing driver code is oriented
toward the PCS being used with an on-board SGMII PHY, so uses Cisco
SGMII-style autonegotiation exclusively and does not anticipate e.g. link
state changes arising from fiber attach/detach events since the SGMII endpoint
is permanently attached in such cases. The first patch changes the driver
to monitor the PCS's link by polling, following the approach used currently
by this driver for fixed links when the PCS is enabled since the driver
does not currently monitor PCS link state otherwise and this had not come up
with permanently-attached SGMII PHYs that report link state out of band.
The second extends the existing SGMII autonegotiation code to also support
1000BASE-X autonegotiation.

Changelog:
- v1: Original patch
  Link: https://lore.kernel.org/netdev/20260714200904.70428-1-nwhitehorn@pa.msu.edu/
- v2: Split into two pieces and clean-up of a few details in anrestart().
  Link: https://lore.kernel.org/netdev/20260714200904.70428-1-nwhitehorn@pa.msu.edu/
- v3: Fix mistakes in commit message for patch 1 and improve wording.
  Link: https://lore.kernel.org/netdev/20260729192748.6928-1-nwhitehorn@pa.msu.edu/
- v4: Fix return value in macb_pcs_config() to indicate to phylink when the
  autonegotiation advertisement has changed and autonegotiation needs to be
  restarted.
  Link: https://lore.kernel.org/r/20260807201741.3275-1-nwhitehorn@pa.msu.edu
- v5: Fix bug in the v4 return value from macb_pcs_config() that caused
  phylink to be informed in the wrong circumstances about updates. Also adjust
  some whitespace issues in the earlier patch series.
  Link: https://lore.kernel.org/netdev/20260813153705.23407-1-nwhitehorn@pa.msu.edu/
- v6: Rebase after 07362f68e61d8. No functional changes.

Tested on Microchip PolarFire SoC with a variety of SFPs (copper, fiber, GPON)
attached to the I/O Bank-5 PCS lines.

Thanks to Charles Perry, Andrew Lunn, Paolo Abeni, Théo Lebrun, and
Jakob Kicinski for helpful suggestions and discussion on this patch series.
As a brief summary of that discussion, there are a few areas near this patch
that are existing non-idealities in the macb driver, in particular that it uses
polling to monitor link-state with the internal PCS and the driver does not
currently have support for using the link-state change interrupt. The
intention is to address this in a later patch that would remove polling from
both this case and the fixed-link one. Other discussion in earlier rounds
of the patches was related to handling some ZynqMP-specific code that is
related to specific SGMII-only hardware that is not relevant in the 1000BASE-X
case.

[PATCH v6 1/2] net: macb: Poll for link state changes when using the internal PCS.

[Nathan Whitehorn]

When used with an offboard transceiver (e.g. an SFP), link state needs
to be measured at the PCS rather than an onboard PHY. Poll the link
state when the PCS is active to allow the kernel to detect link state
changes in this case.

Signed-off-by: Nathan Whitehorn <nwhitehorn@pa.msu.edu>
Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/net/ethernet/cadence/macb_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index b8234ac4b602..99e40b0ce804 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -1025,6 +1025,7 @@ static int macb_mii_probe(struct net_device *netdev)
 	struct macb *bp = netdev_priv(netdev);
 
 	bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
+	bp->phylink_sgmii_pcs.poll = true;
 	bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;
 
 	bp->phylink_config.dev = &netdev->dev;
-- 
2.43.0

[PATCH v6 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS

[Nathan Whitehorn]

The current PCS code unconditionally uses SGMII autonegotiation, though
the hardware supports both SGMII and 1000BASE-X modes. Decouple the
choice of PCS enablement from use of the SGMII mode when running at
gigabit rates and announce to phylink that 1000BASE-X is a supported
operating mode. This enables direct attachment of the PCS to e.g. an
SFP.

The 1000BASE-X code in phylink also sometimes calls the autonegotiation
restart method, so add an implementation of autonegotiation restart and
make sure that pcs_config() signals to phylink when the AN advertisement
has changed.

Signed-off-by: Nathan Whitehorn <nwhitehorn@pa.msu.edu>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/ethernet/cadence/macb_main.c | 35 ++++++++++++++++++------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 99e40b0ce804..a2bf9f3778e2 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -583,7 +583,12 @@ static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
 
 static void macb_pcs_an_restart(struct phylink_pcs *pcs)
 {
-	/* Not supported */
+	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
+	u32 old, new;
+
+	old = gem_readl(bp, PCSCNTRL);
+	new = old | BMCR_ANRESTART;
+	gem_writel(bp, PCSCNTRL, new);
 }
 
 static int macb_pcs_config(struct phylink_pcs *pcs,
@@ -594,11 +599,16 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
 {
 	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
 	u32 old, new;
+	int ret = 0;
 
 	old = gem_readl(bp, PCSANADV);
 	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
-	if (new != -EINVAL && old != new)
+	if (new != -EINVAL && old != new) {
+		/* pcs_config() is supposed to return 1 if AN advertisement
+		 * has changed */
+		ret = 1;
 		gem_writel(bp, PCSANADV, new);
+	}
 
 	/* Disable AN if it's not to be used, enable otherwise.
 	 * Must be written after PCSSEL is set in NCFGR which is done in
@@ -612,7 +622,7 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
 	if (old != new)
 		gem_writel(bp, PCSCNTRL, new);
 
-	return 0;
+	return ret;
 }
 
 static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = {
@@ -750,7 +760,9 @@ static void macb_mac_config(struct phylink_config *config, unsigned int mode,
 		ctrl &= ~(GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL));
 		ncr &= ~GEM_BIT(ENABLE_HS_MAC);
 
-		if (state->interface == PHY_INTERFACE_MODE_SGMII) {
+		if (state->interface == PHY_INTERFACE_MODE_1000BASEX) {
+			ctrl |= GEM_BIT(PCSSEL);
+		} else if (state->interface == PHY_INTERFACE_MODE_SGMII) {
 			ctrl |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
 		} else if (state->interface == PHY_INTERFACE_MODE_10GBASER) {
 			ctrl |= GEM_BIT(PCSSEL);
@@ -957,7 +969,8 @@ static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
 
 	if (interface == PHY_INTERFACE_MODE_10GBASER)
 		return &bp->phylink_usx_pcs;
-	else if (interface == PHY_INTERFACE_MODE_SGMII)
+	else if (interface == PHY_INTERFACE_MODE_1000BASEX ||
+			interface == PHY_INTERFACE_MODE_SGMII)
 		return &bp->phylink_sgmii_pcs;
 	else
 		return NULL;
@@ -1032,7 +1045,8 @@ static int macb_mii_probe(struct net_device *netdev)
 	bp->phylink_config.type = PHYLINK_NETDEV;
 	bp->phylink_config.mac_managed_pm = true;
 
-	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
+	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
+			bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
 		bp->phylink_config.poll_fixed_state = true;
 		bp->phylink_config.get_fixed_state = macb_get_pcs_fixed_state;
 		/* The PCSAUTONEG bit in PCSCNTRL is on out of reset. Setting
@@ -1061,9 +1075,12 @@ static int macb_mii_probe(struct net_device *netdev)
 			  bp->phylink_config.supported_interfaces);
 		phy_interface_set_rgmii(bp->phylink_config.supported_interfaces);
 
-		if (bp->caps & MACB_CAPS_PCS)
+		if (bp->caps & MACB_CAPS_PCS) {
+			__set_bit(PHY_INTERFACE_MODE_1000BASEX,
+				  bp->phylink_config.supported_interfaces);
 			__set_bit(PHY_INTERFACE_MODE_SGMII,
 				  bp->phylink_config.supported_interfaces);
+		}
 
 		if (bp->caps & MACB_CAPS_HIGH_SPEED) {
 			__set_bit(PHY_INTERFACE_MODE_10GBASER,
@@ -4911,7 +4928,9 @@ static int macb_init_dflt(struct platform_device *pdev)
 	/* Set MII management clock divider */
 	val = macb_mdc_clk_div(bp);
 	val |= macb_dbw(bp);
-	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
+	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX)
+		val |= GEM_BIT(PCSSEL);
+	else if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
 		val |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
 	macb_writel(bp, NCFGR, val);
 
-- 
2.43.0

Re: [PATCH v6 1/2] net: macb: Poll for link state changes when using the internal PCS.

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (1) · 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] Incomplete fix: only the SGMII PCS gets link-state polling;
  the sibling internal 10GBASE-R PCS (`bp->phylink_usx_pcs`) is left…
- [Low] The commit carries no `Fixes:` tag although, for the in-band
  SGMII/PCS configuration without an attached PHY, it repairs a pre-…

Pre-existing issues:
- [Medium] Enabling `poll` on the SGMII PCS creates a self-retriggering
  periodic phylink resolve path that is not quiesced by macb's Wake-on-…

--

net: macb: Poll for link state changes when using the internal PCS.

This sets phylink_pcs.poll on the macb SGMII PCS so phylink arms its
periodic resolve timer and re-reads the PCS link state. That covers the
case where the link is terminated at an offboard transceiver such as an
SFP rather than an onboard PHY.

No Link: tags are present in the commit header.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d1d1c273f4bbf..d48ad5b587d09 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -1025,6 +1025,7 @@ static int macb_mii_probe(struct net_device *netdev)
>  	struct macb *bp = netdev_priv(netdev);
>  
>  	bp->phylink_sgmii_pcs.ops = &macb_phylink_pcs_ops;
> +	bp->phylink_sgmii_pcs.poll = true;

[Severity: Medium]
Should bp->phylink_usx_pcs get the same treatment?  It is initialised on
the very next line and keeps the zero-initialised poll == false.

macb_mac_select_pcs() hands out the USX instance for 10GBASE-R:

	if (interface == PHY_INTERFACE_MODE_10GBASER)
		return &bp->phylink_usx_pcs;

and macb_usx_pcs_get_state() reads a level status bit in exactly the same
way that macb_pcs_get_state() reads PCSSTS:

	val = gem_readl(bp, USX_STATUS);
	state->link = !!(val & GEM_BIT(USX_BLOCK_LOCK));

phylink_pcs_poll_start() arms the timer only for the currently selected
PCS:

	if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND)
		mod_timer(&pl->link_poll, jiffies + HZ);

For an SFP, phylink_sfp_set_config() forces MLO_AN_INBAND, and macb has no
other link notification for this path: phylink_pcs_change() is never
called, and MACB_ISR_LINK_* exists only as a register bit definition in
macb.h, never written to IER nor handled in macb_interrupt().  The
phylink_config.poll_fixed_state / get_fixed_state pair set below is only
consulted by phylink_start() for MLO_AN_FIXED:

	if (pl->cfg_link_an_mode == MLO_AN_FIXED)
		poll |= pl->config->poll_fixed_state;

So with a 10GBASE-R module, does USX_BLOCK_LOCK coming up late leave the
carrier down permanently, and does a later loss of block lock leave a
stale carrier up until some unrelated phylink event (module removal, ip
link down/up) forces a re-resolve?

The follow-up patch in the series, "net: macb: add support for
1000BASE-X autonegotiation to PCS", extends only the SGMII PCS, so this
stays the same at the end of the series.

[Severity: Low]
Would a Fixes: tag be appropriate here?  For the in-band SGMII/PCS
configuration with no attached PHY, nothing re-sampled the PCS before
this change: phylink honours poll_fixed_state/get_fixed_state only in
MLO_AN_FIXED, macb installs no PCS link-change notification, and
pl->link_poll was never armed for the PCS path, so macb_pcs_get_state()
ran only at major-config time.

The message carries only Signed-off-by and Reviewed-by, and commit
8876769bf936 ("net: macb: convert to phylink PCS") is not referenced, so
backporters have nothing to key off if this is meant for stable.

If the intent is purely to enable a configuration that was not supported
before (direct PCS-to-SFP attachment, as described in the follow-up
patch), then net-next without a Fixes: tag is fine — could the intent be
made explicit?

[Severity: Medium]
This isn't a bug introduced by this patch, but arming the periodic poll
adds one more trigger to a path that macb_suspend() does not quiesce when
Wake-on-LAN is armed:

drivers/net/ethernet/cadence/macb_main.c:macb_suspend() {
	...
	if (!(bp->wol & MACB_WOL_ENABLED)) {
		rtnl_lock();
		phylink_stop(bp->phylink);
		rtnl_unlock();
	...
}

phylink_stop() is the only caller of timer_delete_sync(&pl->link_poll) and
the resolve flush.  With WoL enabled it is skipped, while
phylink_fixed_poll() keeps re-arming itself and queues the resolve on
system_power_efficient_wq, which is not WQ_FREEZABLE:

	mod_timer(t, jiffies + HZ);
	phylink_run_resolve(pl);
	...
	if (!pl->phylink_disable_state)
		queue_work(system_power_efficient_wq, &pl->resolve);

phylink_resolve() has no netif_device_present() or suspend guard, so a
polled state change calls macb_mac_link_down() (clears NCR RE|TE, masks
queue interrupts) or macb_mac_link_up() (re-enables RE|TE, rewrites
NCFGR, writes per-queue IER, wakes TX queues) on a detached netdev whose
MAC was just armed for WoL.  Can that drop a magic/ARP wake frame while
RE is clear, or re-enable non-WoL queue interrupts on a wake-armed IRQ
line and cause a spurious wake?

The same hazard already exists without this change: macb sets
phylink_config.mac_managed_pm = true, so mdio_bus_phy_suspend() returns
early and the PHY state machine keeps driving resolves during WoL
suspend, and sfp_poll() does the same for SFP setups.  So the underlying
gap looks like macb_suspend()'s WoL path not stopping link management
rather than this one-line flag.

>  	bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;
>  
>  	bp->phylink_config.dev = &netdev->dev;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922151429.23272-1-nwhitehorn%40pa.msu.edu

Re: [PATCH v6 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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] Incomplete feature enablement: PHY_INTERFACE_MODE_1000BASEX is
  now announced to phylink and programmed into the MAC (NCFGR.PCSSEL in…
- [Low] Checkpatch/netdev style violations in the new code, plus a
  redundant local-variable idiom.

--

net: macb: add support for 1000BASE-X autonegotiation to PCS

The patch decouples PCS enablement from the SGMII mode selection so the
gigabit PCS can also be used in 1000BASE-X mode, and announces
PHY_INTERFACE_MODE_1000BASEX to phylink when MACB_CAPS_PCS is present.
It also adds an autonegotiation restart implementation and makes
pcs_config() return 1 when the advertisement was changed.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d48ad5b587d09..3c18e43fca069 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -583,7 +583,12 @@ static void macb_pcs_get_state(struct phylink_pcs *pcs, unsigned int neg_mode,
>  
>  static void macb_pcs_an_restart(struct phylink_pcs *pcs)
>  {
> -	/* Not supported */
> +	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
> +	u32 old, new;
> +
> +	old = gem_readl(bp, PCSCNTRL);
> +	new = old | BMCR_ANRESTART;
> +	gem_writel(bp, PCSCNTRL, new);
>  }
>  
>  static int macb_pcs_config(struct phylink_pcs *pcs,
> @@ -594,11 +599,16 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>  {
>  	struct macb *bp = container_of(pcs, struct macb, phylink_sgmii_pcs);
>  	u32 old, new;
> +	int ret = 0;
>  
>  	old = gem_readl(bp, PCSANADV);
>  	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
> -	if (new != -EINVAL && old != new)
> +	if (new != -EINVAL && old != new) {
> +		/* pcs_config() is supposed to return 1 if AN advertisement
> +		 * has changed */

[Severity: Low]
This isn't a bug, but a few style points that netdev's checkpatch run will
likely flag:

The new block comment in macb_pcs_config() closes on the text line, and
checkpatch wants the trailing */ on a line of its own:

	/* pcs_config() is supposed to return 1 if AN advertisement
	 * has changed
	 */

The continuation lines of the two new conditions, in
macb_mac_select_pcs() and in macb_mii_probe(), are indented with tabs
rather than aligned to the open parenthesis (checkpatch
PARENTHESIS_ALIGNMENT), which makes the continuation look like a
statement body:

	else if (interface == PHY_INTERFACE_MODE_1000BASEX ||
			interface == PHY_INTERFACE_MODE_SGMII)

In macb_pcs_an_restart(), the old/new pair mirrors the conditional-write
idiom used by macb_pcs_config() but there is no old != new comparison,
so the split reads as though a comparison was dropped.  Could this just
be written as a single read-modify-write?

> +		ret = 1;
>  		gem_writel(bp, PCSANADV, new);
> +	}
>  
>  	/* Disable AN if it's not to be used, enable otherwise.
>  	 * Must be written after PCSSEL is set in NCFGR which is done in
> @@ -612,7 +622,7 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>  	if (old != new)
>  		gem_writel(bp, PCSCNTRL, new);
>  
> -	return 0;
> +	return ret;
>  }

[ ... ]

> @@ -957,7 +969,8 @@ static struct phylink_pcs *macb_mac_select_pcs(struct phylink_config *config,
>  
>  	if (interface == PHY_INTERFACE_MODE_10GBASER)
>  		return &bp->phylink_usx_pcs;
> -	else if (interface == PHY_INTERFACE_MODE_SGMII)
> +	else if (interface == PHY_INTERFACE_MODE_1000BASEX ||
> +			interface == PHY_INTERFACE_MODE_SGMII)
>  		return &bp->phylink_sgmii_pcs;
>  	else
>  		return NULL;
> @@ -1032,7 +1045,8 @@ static int macb_mii_probe(struct net_device *netdev)
>  	bp->phylink_config.type = PHYLINK_NETDEV;
>  	bp->phylink_config.mac_managed_pm = true;
>  
> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
> +			bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
>  		bp->phylink_config.poll_fixed_state = true;
>  		bp->phylink_config.get_fixed_state = macb_get_pcs_fixed_state;
>  		/* The PCSAUTONEG bit in PCSCNTRL is on out of reset. Setting
> @@ -1061,9 +1075,12 @@ static int macb_mii_probe(struct net_device *netdev)
>  			  bp->phylink_config.supported_interfaces);
>  		phy_interface_set_rgmii(bp->phylink_config.supported_interfaces);
>  
> -		if (bp->caps & MACB_CAPS_PCS)
> +		if (bp->caps & MACB_CAPS_PCS) {
> +			__set_bit(PHY_INTERFACE_MODE_1000BASEX,
> +				  bp->phylink_config.supported_interfaces);
>  			__set_bit(PHY_INTERFACE_MODE_SGMII,
>  				  bp->phylink_config.supported_interfaces);
> +		}

[Severity: Medium]
1000BASE-X is now announced purely on the runtime-detected MACB_CAPS_PCS
bit, so it becomes selectable on the zynqmp/versal/mpfs/pic64hpsc
configs, all of which use .init = init_reset_optional.

init_reset_optional() still gates the entire serial transceiver bring-up
on the SGMII mode only:

drivers/net/ethernet/cadence/macb_main.c:init_reset_optional() {
	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
		/* Ensure PHY device used in SGMII mode is ready */
		bp->phy = devm_phy_optional_get(&pdev->dev, NULL);
		...
		ret = phy_init(bp->phy);
		...
		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_FIXED, 0);
		...
		ret = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
	}
}

With phy-mode = "1000base-x", that block is skipped, so bp->phy stays
NULL (netdev priv is zero allocated), phy_init() never runs and the
firmware calls that route the GEM onto the serial/PCS path are never
issued.

macb_open() then does:

drivers/net/ethernet/cadence/macb_main.c:macb_open() {
	err = phy_set_mode_ext(bp->phy, PHY_MODE_ETHERNET, bp->phy_interface);
	if (err)
		goto reset_hw;

	err = phy_power_on(bp->phy);
	if (err)
		goto reset_hw;
}

Both are NULL tolerant and return 0, so probe and open succeed while the
PS-GTR lane is left unconfigured and unpowered (xpsgtr_phy_init() is what
enables the lane reference clock and runs xpsgtr_phy_init_sgmii()).

Does this mean the SFP use case named in the commit message ("This
enables direct attachment of the PCS to e.g. an SFP") silently never
links on those platforms, with no diagnostic?  Should
init_reset_optional() take the same path for
PHY_INTERFACE_MODE_1000BASEX?

Separately, macb_open() passes the device tree bp->phy_interface to
phy_set_mode_ext() rather than the interface phylink resolved.  Now that
the same PCS can run as either SGMII or 1000BASE-X, can the transceiver
submode end up differing from the mode programmed into the MAC PCS?

>  
>  		if (bp->caps & MACB_CAPS_HIGH_SPEED) {
>  			__set_bit(PHY_INTERFACE_MODE_10GBASER,
> @@ -4911,7 +4928,9 @@ static int macb_init_dflt(struct platform_device *pdev)
>  	/* Set MII management clock divider */
>  	val = macb_mdc_clk_div(bp);
>  	val |= macb_dbw(bp);
> -	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
> +	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX)
> +		val |= GEM_BIT(PCSSEL);
> +	else if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII)
>  		val |= GEM_BIT(SGMIIEN) | GEM_BIT(PCSSEL);
>  	macb_writel(bp, NCFGR, val);
>  

This is the counterpart site: the MAC side PCS is enabled for 1000BASE-X
on exactly the platforms whose transceiver init above was left SGMII only.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922151429.23272-1-nwhitehorn%40pa.msu.edu

Re: [PATCH net-next v6 0/2] net: macb: 1000BASE-X on internal PCS

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 22 Sep 2026 11:09:23 -0400 you wrote:
> This series adds support for 1000BASE-X autonegotiation to the Cadence macb
> driver when using the MAC-internal PCS. The existing driver code is oriented
> toward the PCS being used with an on-board SGMII PHY, so uses Cisco
> SGMII-style autonegotiation exclusively and does not anticipate e.g. link
> state changes arising from fiber attach/detach events since the SGMII endpoint
> is permanently attached in such cases. The first patch changes the driver
> to monitor the PCS's link by polling, following the approach used currently
> by this driver for fixed links when the PCS is enabled since the driver
> does not currently monitor PCS link state otherwise and this had not come up
> with permanently-attached SGMII PHYs that report link state out of band.
> The second extends the existing SGMII autonegotiation code to also support
> 1000BASE-X autonegotiation.
> 
> [...]

Here is the summary with links:
  - [v6,1/2] net: macb: Poll for link state changes when using the internal PCS.
    https://git.kernel.org/netdev/net-next/c/456a389f594a
  - [v6,2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS
    https://git.kernel.org/netdev/net-next/c/f342b1208f8d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html