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

On 7/15/26 12:29, Charles Perry wrote:

On Tue, Jul 14, 2026 at 04:09:04PM -0400, Nathan Whitehorn wrote:

quoted

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.

Tested and developed on Microchip Polarfire SoC hardware.

Signed-off-by: Nathan Whitehorn <redacted>
---
  drivers/net/ethernet/cadence/macb_main.c | 31 ++++++++++++++++++------
  1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d394f1f43b68..284a3b03f8c7 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c

@@ -583,7 +583,13 @@ 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;
+	if (old != new)
+		gem_writel(bp, PCSCNTRL, new);

This bit is self-clearing so I don't think the old != new check is
required.

Ah, OK, good to know.

Also, can you comment on why AN restart is needed for this patch? Is it a
requirement for 1000base-x of is it because we now have sgmii and
1000base-x?

Our equipment works well enough without this, and it is just here for 
completeness rather than an actual need. I am happy to drop it from the 
patch if you prefer.

quoted

  }
  
  static int macb_pcs_config(struct phylink_pcs *pcs,

@@ -750,7 +756,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 +965,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;

@@ -1025,13 +1034,15 @@ 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;

Why is this needed for 1000base-x?

Without this, the hardware does not detect link state changes after 
initialization. I am assuming that in the normal SGMII use-case with an 
external onboard PHY etc., those state changes either don't have or are 
signaled by monitoring the PHY, so that support for this wasn't needed. 
With removable modules, fiber, etc., state changes are suddenly both 
possible and otherwise unsignaled. It is possible -- likely, actually -- 
that there is some better way to do this by properly monitoring an 
interrupt somewhere, but I am working without documentation so am not 
sure how to implement that and was trying to keep the patch short. 
Polling worked well enough as an alternative.

Thank you for looking through this!
-Nathan

Thanks,
Charles

-- 
Nathan Whitehorn (he/him)
Associate Professor
Department of Physics and Astronomy
Michigan State University
Biomedical and Physical Sciences 3225
East Lansing, MI 48824
(517) 884-5563