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

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

Tested and developed on Microchip Polarfire SoC hardware.

Signed-off-by: Nathan Whitehorn <nwhitehorn@pa.msu.edu>
---
 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);
 }
 
 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;
 	bp->phylink_usx_pcs.ops = &macb_phylink_usx_pcs_ops;
 
 	bp->phylink_config.dev = &dev->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
@@ -1060,9 +1071,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,
@@ -4931,7 +4945,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);
 
@@ -5456,7 +5472,8 @@ static int init_reset_optional(struct platform_device *pdev)
 	struct macb *bp = netdev_priv(dev);
 	int ret;
 
-	if (bp->phy_interface == PHY_INTERFACE_MODE_SGMII) {
+	if (bp->phy_interface == PHY_INTERFACE_MODE_1000BASEX ||
+	    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);
 
-- 
2.54.0

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

[Andrew Lunn]

On Tue, Jul 14, 2026 at 04:09:04PM -0400, Nathan Whitehorn wrote:
> 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.

linux$ ./scripts/get_maintainer.pl drivers/net/ethernet/cadence/macb_main.c 
"Théo Lebrun" <theo.lebrun@bootlin.com> (maintainer:ATMEL MACB ETHERNET DRIVER)
Conor Dooley <conor.dooley@microchip.com> (reviewer:ATMEL MACB ETHERNET DRIVER)
Andrew Lunn <andrew+netdev@lunn.ch> (maintainer:NETWORKING DRIVERS)
"David S. Miller" <davem@davemloft.net> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <edumazet@google.com> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@kernel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@redhat.com> (maintainer:NETWORKING DRIVERS)
netdev@vger.kernel.org (open list:NETWORKING DRIVERS)
linux-kernel@vger.kernel.org (open list)

You did not Cc: the two developers who know the most about this
hardware.

https://docs.kernel.org/process/submitting-patches.html
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

    Andrew

---
pw-bot: cr

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

[Charles Perry]

On Tue, Jul 14, 2026 at 04:09:04PM -0400, Nathan Whitehorn wrote:
> 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 <nwhitehorn@pa.msu.edu>
> ---
>  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.

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?

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

Thanks,
Charles

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

[Nathan Whitehorn]

On 7/15/26 12:29, Charles Perry wrote:
> On Tue, Jul 14, 2026 at 04:09:04PM -0400, Nathan Whitehorn wrote:
>> 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 <nwhitehorn@pa.msu.edu>
>> ---
>>   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.

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

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

[Andrew Lunn]

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

This does not sound quiet correct. A PCS has two options.

1) As you say, it implements interrupts, and calls
phylink_pcs_change() on each interrupt to let phylink know something
happened.

2) You set poll true, and phylink will poll the PCS every second.

A PHY generally only reports on its media side changing state. I also
don't think phylink will poll the PCS if the PHY changes state.

I don't see anywhere macb calls phylink_pcs_change(), so polling does
seem correct.

Please break this patch up. You can add polling in a patch of its own,
with the commit message explaining why it is needed.

     Andrew

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

[Charles Perry]

On Wed, Jul 15, 2026 at 12:35:48PM -0400, Nathan Whitehorn wrote:
> 
> 
> On 7/15/26 12:29, Charles Perry wrote:
> > On Tue, Jul 14, 2026 at 04:09:04PM -0400, Nathan Whitehorn wrote:
> > > 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 <nwhitehorn@pa.msu.edu>
> > > ---
> > >   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.
> 

Well it does look like the ->pcs_an_restart() callback was made
specifically for 1000base-x because phylink_pcs_an_restart() in phylink.c
checks for phy_interface_mode_is_8023z():

https://elixir.bootlin.com/linux/v7.1.3/source/drivers/net/phy/phylink.c#L1028

Maybe you actually need this.

Thanks,
Charles