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

[PATCH net-next v3 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; the second extends the existing SGMII
autonegotiation code to also support 1000BASE-X autonegotiation.

Changelog:
- v1: Original patch
- v2: Split into two pieces and clean-up of a few details in anrestart().
- v3: Fix mistakes in commit message for patch 1 and improve wording.

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 and Andrew Lunn for helpful suggestions on this patch
series.
-Nathan

[PATCH net-next v3 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>
---
 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.34.1

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

[Théo Lebrun]

Hello Nathan,

On Wed Jul 29, 2026 at 9:21 PM CEST, Nathan Whitehorn wrote:
> 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>
> ---
>  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;

Hardware has an IRQ for that, would you want to use it instead of
polling? The user guide contains more info.

   /* Bitfields in ISR/IER/IDR/IMR */
   ...
   #define MACB_ISR_LINK_OFFSET  9 /* Enable link change interrupt */
   #define MACB_ISR_LINK_SIZE 1
   ...

I've checked with an on-board SGMII PHY and I get an IRQ once unmasked.

If you don't care, then:

Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

[Nathan Whitehorn]

On 8/5/26 15:43, Théo Lebrun wrote:
> Hello Nathan,
>
> On Wed Jul 29, 2026 at 9:21 PM CEST, Nathan Whitehorn wrote:
>> 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>
>> ---
>>   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;
> Hardware has an IRQ for that, would you want to use it instead of
> polling? The user guide contains more info.
>
>     /* Bitfields in ISR/IER/IDR/IMR */
>     ...
>     #define MACB_ISR_LINK_OFFSET  9 /* Enable link change interrupt */
>     #define MACB_ISR_LINK_SIZE 1
>     ...
>
> I've checked with an on-board SGMII PHY and I get an IRQ once unmasked.
>
> If you don't care, then:
>
> Reviewed-by: Théo Lebrun <theo.lebrun@bootlin.com>

Thanks, that's really good to know! I will look into it. For right now, 
though, if it is all right, I think I would prefer to keep this as-is 
and come back with a later patch that switches this to use the 
interrupt, just to get the base functionality into the tree.
-Nathan

>
> Regards,
>
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>

-- 
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

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

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

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c15a9c7e69d3..310ad8eac21f 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,
@@ -750,7 +755,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 +964,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 +1040,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 +1070,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 +4944,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.34.1

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

[Paolo Abeni]

From: AI Reviewer <ai@example.com>

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://sashiko.dev/
---
--- Patch [2]: [PATCH net-next v3 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS ---
commit 3a10158740d1fea523a081adf59e6bcdf1cd55d1
Author: Nathan Whitehorn <nwhitehorn@pa.msu.edu>

    net: macb: add support for 1000BASE-X autonegotiation to PCS
    
    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.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c15a9c7e69d37..310ad8eac21fc 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);
>  }

[Severity: Medium]
Since 1000BASE-X utilizes base page advertisements that can dynamically change,
should the adjacent macb_pcs_config() function be updated to return 1 when
changes are made?

Looking at macb_pcs_config():

static int macb_pcs_config(...)
{
...
	if (old != new)
		gem_writel(bp, PCSCNTRL, new);

	return 0;
}

Returning 0 unconditionally appears to prevent phylink from restarting
autonegotiation when the advertisement changes (like flow control or PAUSE
settings), meaning the new configuration will not take effect until the link
is bounced.

[ ... ]

> @@ -750,7 +755,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);

[Severity: High]
This is a pre-existing issue, but could macb_mac_config() race with
macb_set_rx_mode() over the NCFGR register?

macb_mac_config() modifies NCFGR while holding bp->lock:

	spin_lock_irqsave(&bp->lock, flags);
	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
	...
	if (old_ctrl ^ ctrl)
		macb_or_gem_writel(bp, NCFGR, ctrl);

However, macb_set_rx_mode() performs a lockless read-modify-write on NCFGR
protected only by netif_addr_lock_bh:

	cfg = macb_readl(bp, NCFGR);
	if (dev->flags & IFF_PROMISC) {
	...

Could this concurrent modification lead to lost configuration updates, such as
overwriting PCSSEL/SGMIIEN bits or dropping promiscuous/multicast flags?

[ ... ]

> @@ -1061,9 +1070,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: High]
With the addition of 1000BASE-X support here, does the init_reset_optional()
function also need updating to prevent a regression on platforms using a SerDes?

Looking at init_reset_optional() later in the file:

	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);

Since this explicitly checks only for SGMII mode, won't the hardware
initialization for the SerDes lane fail when the MAC is configured for
1000BASE-X, leaving the generic PHY uninitialized and preventing the hardware
from linking up?
-- 
This is an AI-generated review.

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

[Nathan Whitehorn]

On 8/4/26 04:37, Paolo Abeni wrote:
> From: AI Reviewer <ai@example.com>
>
> 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://sashiko.dev/
> ---
> --- Patch [2]: [PATCH net-next v3 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS ---
> commit 3a10158740d1fea523a081adf59e6bcdf1cd55d1
> Author: Nathan Whitehorn <nwhitehorn@pa.msu.edu>
>
>      net: macb: add support for 1000BASE-X autonegotiation to PCS
>      
>      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.
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index c15a9c7e69d37..310ad8eac21fc 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);
>>   }
> [Severity: Medium]
> Since 1000BASE-X utilizes base page advertisements that can dynamically change,
> should the adjacent macb_pcs_config() function be updated to return 1 when
> changes are made?
>
> Looking at macb_pcs_config():
>
> static int macb_pcs_config(...)
> {
> ...
> 	if (old != new)
> 		gem_writel(bp, PCSCNTRL, new);
>
> 	return 0;
> }
>
> Returning 0 unconditionally appears to prevent phylink from restarting
> autonegotiation when the advertisement changes (like flow control or PAUSE
> settings), meaning the new configuration will not take effect until the link
> is bounced.
>
> [ ... ]

Thank you! This is a good catch of a pre-existing bug in 
macb_pcs_config() that matters a lot more with removable transceivers 
and I will fix it in an updated patch.

>
>> @@ -750,7 +755,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);
> [Severity: High]
> This is a pre-existing issue, but could macb_mac_config() race with
> macb_set_rx_mode() over the NCFGR register?
>
> macb_mac_config() modifies NCFGR while holding bp->lock:
>
> 	spin_lock_irqsave(&bp->lock, flags);
> 	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
> 	...
> 	if (old_ctrl ^ ctrl)
> 		macb_or_gem_writel(bp, NCFGR, ctrl);
>
> However, macb_set_rx_mode() performs a lockless read-modify-write on NCFGR
> protected only by netif_addr_lock_bh:
>
> 	cfg = macb_readl(bp, NCFGR);
> 	if (dev->flags & IFF_PROMISC) {
> 	...
>
> Could this concurrent modification lead to lost configuration updates, such as
> overwriting PCSSEL/SGMIIEN bits or dropping promiscuous/multicast flags?
>
> [ ... ]

It probably could. I am a bit hesitant to fix this as part of this patch 
series for scope reasons, though, and would like to defer to the driver 
maintainers on the proper fix for this one. I don't think things are any 
worse with this patch than they were (1000BASE-X vs. SGMII 
autonegotiation modes are essentially static, except on physical SFP 
replacement, unlike promiscuous/multicast).

>
>> @@ -1061,9 +1070,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: High]
> With the addition of 1000BASE-X support here, does the init_reset_optional()
> function also need updating to prevent a regression on platforms using a SerDes?
>
> Looking at init_reset_optional() later in the file:
>
> 	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);
>
> Since this explicitly checks only for SGMII mode, won't the hardware
> initialization for the SerDes lane fail when the MAC is configured for
> 1000BASE-X, leaving the generic PHY uninitialized and preventing the hardware
> from linking up?

This block of code is specific to an onboard PHY -- which does not 
generally (ever?) exist in 1000BASE-X mode -- and what looks like some 
kind of Xilinx Zynq-specific quirks that I don't know enough details 
about to understand whether it is relevant in this case or test. As 
such, I had decided not to touch this code in this patch, but would 
welcome any feedback from people more familiar with the Xilinx case than 
I am and am happy to make changes if appropriate.

Thanks,
Nathan

-- 
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

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

[Théo Lebrun]

Hello Nathan,

On Tue Aug 4, 2026 at 5:08 PM CEST, Nathan Whitehorn wrote:
>
>
> On 8/4/26 04:37, Paolo Abeni wrote:
>> From: AI Reviewer <ai@example.com>
>>
>> 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://sashiko.dev/
>> ---
>> --- Patch [2]: [PATCH net-next v3 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS ---
>> commit 3a10158740d1fea523a081adf59e6bcdf1cd55d1
>> Author: Nathan Whitehorn <nwhitehorn@pa.msu.edu>
>>
>>      net: macb: add support for 1000BASE-X autonegotiation to PCS
>>      
>>      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.
>>
>>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>>> index c15a9c7e69d37..310ad8eac21fc 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);
>>>   }
>> [Severity: Medium]
>> Since 1000BASE-X utilizes base page advertisements that can dynamically change,
>> should the adjacent macb_pcs_config() function be updated to return 1 when
>> changes are made?
>>
>> Looking at macb_pcs_config():
>>
>> static int macb_pcs_config(...)
>> {
>> ...
>> 	if (old != new)
>> 		gem_writel(bp, PCSCNTRL, new);
>>
>> 	return 0;
>> }
>>
>> Returning 0 unconditionally appears to prevent phylink from restarting
>> autonegotiation when the advertisement changes (like flow control or PAUSE
>> settings), meaning the new configuration will not take effect until the link
>> is bounced.
>>
>> [ ... ]
>
> Thank you! This is a good catch of a pre-existing bug in 
> macb_pcs_config() that matters a lot more with removable transceivers 
> and I will fix it in an updated patch.
>
>>
>>> @@ -750,7 +755,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);
>> [Severity: High]
>> This is a pre-existing issue, but could macb_mac_config() race with
>> macb_set_rx_mode() over the NCFGR register?
>>
>> macb_mac_config() modifies NCFGR while holding bp->lock:
>>
>> 	spin_lock_irqsave(&bp->lock, flags);
>> 	old_ctrl = ctrl = macb_or_gem_readl(bp, NCFGR);
>> 	...
>> 	if (old_ctrl ^ ctrl)
>> 		macb_or_gem_writel(bp, NCFGR, ctrl);
>>
>> However, macb_set_rx_mode() performs a lockless read-modify-write on NCFGR
>> protected only by netif_addr_lock_bh:
>>
>> 	cfg = macb_readl(bp, NCFGR);
>> 	if (dev->flags & IFF_PROMISC) {
>> 	...
>>
>> Could this concurrent modification lead to lost configuration updates, such as
>> overwriting PCSSEL/SGMIIEN bits or dropping promiscuous/multicast flags?
>>
>> [ ... ]
>
> It probably could. I am a bit hesitant to fix this as part of this patch 
> series for scope reasons, though, and would like to defer to the driver 
> maintainers on the proper fix for this one. I don't think things are any 
> worse with this patch than they were (1000BASE-X vs. SGMII 
> autonegotiation modes are essentially static, except on physical SFP 
> replacement, unlike promiscuous/multicast).

Sashiko loves raising such pre-existing issues! macb_set_rx_mode() is at
fault and you have no relation to it, so don't feel obliged to address
that finding.

Also, it does no distinction on the frequency of the codepaths (here two
functions called approx once per boot). It would be a proper severity
high if it was phylink_get_state versus IRQ for example...

Fix only if you want.

>>> @@ -1061,9 +1070,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: High]
>> With the addition of 1000BASE-X support here, does the init_reset_optional()
>> function also need updating to prevent a regression on platforms using a SerDes?
>>
>> Looking at init_reset_optional() later in the file:
>>
>> 	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);
>>
>> Since this explicitly checks only for SGMII mode, won't the hardware
>> initialization for the SerDes lane fail when the MAC is configured for
>> 1000BASE-X, leaving the generic PHY uninitialized and preventing the hardware
>> from linking up?
>
> This block of code is specific to an onboard PHY -- which does not 
> generally (ever?) exist in 1000BASE-X mode -- and what looks like some 
> kind of Xilinx Zynq-specific quirks that I don't know enough details 
> about to understand whether it is relevant in this case or test. As 
> such, I had decided not to touch this code in this patch, but would 
> welcome any feedback from people more familiar with the Xilinx case than 
> I am and am happy to make changes if appropriate.

Agreed with you. It's ZyncMQ quirks, and if it was done that way most
likely the HW only supports SGMII and they have two-three wrapper
registers that must be written.

I asked an LLM to make a scan and upstream devicetrees seem to confirm,
with only RGMII or SGMII PHYs.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com