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

[PATCH net-next v5 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; 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.

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.

[PATCH net-next v5 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 d394f1f43b68..c15a9c7e69d3 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 *dev)
 	struct macb *bp = netdev_priv(dev);
 
 	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 = &dev->dev;
-- 
2.43.0

[PATCH net-next v5 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 c15a9c7e69d3..9097ccb14c13 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 *dev)
 	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 *dev)
 			  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,
@@ -4932,7 +4949,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 net-next v5 1/2] net: macb: Poll for link state changes when using the internal PCS.

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: macb: Poll for link state changes when using the internal PCS.

This sets poll = true on the macb internal SGMII phylink PCS so phylink
arms its periodic link poll timer, letting link state changes be observed
at the PCS instead of at an onboard PHY (for example with an SFP).

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index d394f1f43b685..c15a9c7e69d37 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 *dev)
>  	struct macb *bp = netdev_priv(dev);
>  
>  	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;

[Severity: Low]
This isn't a bug introduced by this patch, but should phylink_usx_pcs also
get poll = true?  The commit message speaks generally about "an offboard
transceiver (e.g. an SFP)" and about polling "when the PCS is active", yet
only the SGMII PCS is updated.

macb_mac_select_pcs() returns &bp->phylink_usx_pcs for
PHY_INTERFACE_MODE_10GBASER, and phylink only arms the timer for the
currently selected PCS:

drivers/net/phy/phylink.c:phylink_pcs_poll_start() {
	if (pl->pcs && pl->pcs->poll && pl->cfg_link_an_mode == MLO_AN_INBAND)
		mod_timer(&pl->link_poll, jiffies + HZ);
}

PHY_INTERFACE_MODE_10GBASER is one of the modes accepted as in-band by
phylink_parse_mode(), so with managed = "in-band-status" the USX PCS ends up
in MLO_AN_INBAND but with poll == false.

macb_usx_pcs_get_state() is a pure register read:

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

and macb has no phylink_pcs_change()/phylink_mac_change() call anywhere, and
poll_fixed_state/get_fixed_state are only installed for SGMII.  Does that
mean USX_BLOCK_LOCK transitions are only sampled when something else (such
as an SFP loss-of-signal transition) forces a resolve?

The USX PCS behaves exactly as it did before this patch, so this is a
pre-existing gap rather than a new failure.  Could the commit message say
whether leaving the USX PCS unpolled is deliberate?

[Severity: Medium]
This is a pre-existing issue, but arming the poll timer adds one more
asynchronous context that touches the hardware, and macb_suspend() does not
stop phylink on the Wake-on-LAN path:

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

phylink_suspend() is never called either, so with WoL enabled
pl->phylink_disable_state stays 0 and phylink_resolve() keeps running (clocks
stay on because macb_suspend() skips pm_runtime_force_suspend() when
device_may_wakeup() is true).

If the PCS link state changes after the WoL arming sequence, can the resolve
undo it?  macb_mac_link_down() clears the receive engine that magic-packet
and ARP wake detection rely on:

	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);

and macb_mac_link_up() goes the other way, re-unmasking every queue while
NAPI is disabled and re-enabling RE|TE:

	queue_writel(queue, IER,
		     bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP));
	...
	macb_writel(bp, NCR, ctrl | MACB_BIT(RE) | MACB_BIT(TE));

phylink_resolve() does not consult netif_device_present(), so nothing appears
to gate this against the suspended state.

The same 1 Hz poll already existed in the baseline for MLO_AN_FIXED SGMII:

	if (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;

and phylink_phy_change()/phylink_sfp_link_down() can already drive a resolve
during a WoL-armed suspend, so this patch only adds another trigger for the
in-band SGMII case.  Would calling phylink_suspend() (or phylink_stop()) on
the WoL path in macb_suspend() be the right fix?
-- 
This is an AI-generated review.

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

[Paolo Abeni]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
net: macb: add support for 1000BASE-X autonegotiation to PCS

This decouples PCS enablement from the SGMII mode selection, advertises
PHY_INTERFACE_MODE_1000BASEX to phylink when the controller has a PCS, and
adds an autonegotiation restart implementation plus a return of 1 from
pcs_config() when the advertisement changed.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c15a9c7e69d37..9097ccb14c13b 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c

[ ... ]

> @@ -1061,9 +1075,12 @@ static int macb_mii_probe(struct net_device *dev)
>  			  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]

Advertising PHY_INTERFACE_MODE_1000BASEX here makes a device tree with
phy-mode = "1000base-x" acceptable on every PCS-capable GEM, including the
zynqmp, versal, mpfs and pic64hpsc configs whose .init hook is
init_reset_optional().  Should that hook also learn about 1000BASE-X?

It still gates the whole SerDes and firmware bring-up on SGMII 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 branch is skipped, so bp->phy stays NULL,
phy_init() is never called on the PS-GTR lane, and GEM_SGMII_MODE is never
set in the ZynqMP GEM_CLK_CTRL register, which is what routes the MAC onto
the serial/PCS clock path.

Is the failure also silent?  macb_open() calls
phy_set_mode_ext(bp->phy, PHY_MODE_ETHERNET, bp->phy_interface) and
phy_power_on(bp->phy), and both return 0 immediately for a NULL phy in
drivers/phy/phy-core.c, so probe and open succeed with no diagnostic while
the link can never come up.

> @@ -4932,7 +4949,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);

[Severity: Medium]

macb_init_dflt() is reached from init_reset_optional() a few statements after
the SGMII-only branch above, and macb_mac_config() sets PCSSEL for
1000BASE-X too.  Does that leave NCFGR.PCSSEL enabled while the SerDes lane
and the firmware GEM clock mux were never programmed on those platforms?

Also, the binding in Documentation/devicetree/bindings/net/cdns,macb.yaml
documents phys with maxItems: 1 for the controller without tying it to
phy-mode = "sgmii", so can a 1000BASE-X board legitimately expect the driver
to initialize that SerDes?

One related asymmetry: for a board declaring phy-mode = "sgmii" with an SFP
attached, phylink may select 1000BASE-X for the PCS while macb_open() still
programs the SerDes submode from the device tree value in bp->phy_interface.
Should the submode follow the interface phylink actually selected?

Note this does not regress a previously working setup, since 1000BASE-X was
not in supported_interfaces before this patch.
-- 
This is an AI-generated review.