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

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

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, and Théo Lebrun for helpful
suggestions and discussion on this patch series.

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

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

[Jakub Kicinski]

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

This sets phylink_pcs.poll on the macb SGMII PCS so that phylink polls the
PCS for link state instead of relying on an onboard PHY, which is needed
for offboard transceivers such as SFPs.

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

[Severity: Medium]
Can this silently disarm Wake-on-LAN across a suspend?

Setting poll on the SGMII PCS makes phylink arm its periodic link_poll
timer, since macb also sets phylink_config.default_an_inband = true in
macb_mii_probe(), so cfg_link_an_mode ends up as MLO_AN_INBAND:

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

That is reached from phylink_start() -> phylink_mac_initial_config() ->
phylink_major_config() -> phylink_pcs_poll_start().

The timer is only cancelled by phylink_stop() (timer_delete_sync of
pl->link_poll) or phylink_pcs_poll_stop(), but macb_suspend() skips
phylink_stop() entirely on the Wake-on-LAN path:

drivers/net/ethernet/cadence/macb_main.c:macb_suspend() {
	...
	/* Enable Receive engine */
	macb_writel(bp, NCR, tmp | MACB_BIT(RE));
	...
	enable_irq_wake(bp->queues[0].irq);
	}
	...
	if (!(bp->wol & MACB_WOL_ENABLED)) {
		rtnl_lock();
		phylink_stop(bp->phylink);
		rtnl_unlock();
	...
}

macb also does not use the Wake-on-LAN aware phylink_suspend(pl, true) /
phylink_resume() pair, so pl->phylink_disable_state stays 0 and
phylink_fixed_poll() keeps re-arming itself and queuing pl->resolve on
system_power_efficient_wq, which is not freezable.

If the poller then observes a PCS link change during the suspend window,
macb_mac_link_down() clears exactly the bit macb_suspend() just set:

drivers/net/ethernet/cadence/macb_main.c:macb_mac_link_down() {
	/* Disable Rx and Tx */
	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
	macb_writel(bp, NCR, ctrl);
}

The WOL register and the queue-0 wake interrupt remain enabled, so the
driver still believes Wake-on-LAN is armed while the receive engine is off
and a magic packet or ARP frame can no longer be received.  Clocks stay on
here because macb_runtime_suspend() skips macb_clks_disable() when
device_may_wakeup(), so the register write does reach the MAC.

The opposite direction looks odd too: a poll-triggered macb_mac_link_up()
re-enables RE|TE and re-writes the per-queue IER with
bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP) after the RX queues
have been tied off or set to MACB_BIT(QUEUE_DISABLE) and NAPI has been
disabled, on an IRQ that is an armed system wake source.

phylink documents the intended behaviour for this case in
phylink_suspend():

	/* We do not call mac_link_down() here as we want the
	 * link to remain up to receive the WoL packets.
	 */

Would switching macb_suspend()/macb_resume() to phylink_suspend(bp->phylink,
true) / phylink_resume(), or otherwise stopping the poller on the
Wake-on-LAN path, be preferable now that a periodic PCS poller exists?
-- 
pw-bot: cr

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

[Nathan Whitehorn]

On 8/11/26 21:54, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: macb: Poll for link state changes when using the internal PCS.
>
> This sets phylink_pcs.poll on the macb SGMII PCS so that phylink polls the
> PCS for link state instead of relying on an onboard PHY, which is needed
> for offboard transceivers such as SFPs.
>
>> 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;
> [Severity: Medium]
> Can this silently disarm Wake-on-LAN across a suspend?
>
> Setting poll on the SGMII PCS makes phylink arm its periodic link_poll
> timer, since macb also sets phylink_config.default_an_inband = true in
> macb_mii_probe(), so cfg_link_an_mode ends up as MLO_AN_INBAND:
>
> 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);
> }
>
> That is reached from phylink_start() -> phylink_mac_initial_config() ->
> phylink_major_config() -> phylink_pcs_poll_start().
>
> The timer is only cancelled by phylink_stop() (timer_delete_sync of
> pl->link_poll) or phylink_pcs_poll_stop(), but macb_suspend() skips
> phylink_stop() entirely on the Wake-on-LAN path:
>
> drivers/net/ethernet/cadence/macb_main.c:macb_suspend() {
> 	...
> 	/* Enable Receive engine */
> 	macb_writel(bp, NCR, tmp | MACB_BIT(RE));
> 	...
> 	enable_irq_wake(bp->queues[0].irq);
> 	}
> 	...
> 	if (!(bp->wol & MACB_WOL_ENABLED)) {
> 		rtnl_lock();
> 		phylink_stop(bp->phylink);
> 		rtnl_unlock();
> 	...
> }
>
> macb also does not use the Wake-on-LAN aware phylink_suspend(pl, true) /
> phylink_resume() pair, so pl->phylink_disable_state stays 0 and
> phylink_fixed_poll() keeps re-arming itself and queuing pl->resolve on
> system_power_efficient_wq, which is not freezable.
>
> If the poller then observes a PCS link change during the suspend window,
> macb_mac_link_down() clears exactly the bit macb_suspend() just set:
>
> drivers/net/ethernet/cadence/macb_main.c:macb_mac_link_down() {
> 	/* Disable Rx and Tx */
> 	ctrl = macb_readl(bp, NCR) & ~(MACB_BIT(RE) | MACB_BIT(TE));
> 	macb_writel(bp, NCR, ctrl);
> }
>
> The WOL register and the queue-0 wake interrupt remain enabled, so the
> driver still believes Wake-on-LAN is armed while the receive engine is off
> and a magic packet or ARP frame can no longer be received.  Clocks stay on
> here because macb_runtime_suspend() skips macb_clks_disable() when
> device_may_wakeup(), so the register write does reach the MAC.
>
> The opposite direction looks odd too: a poll-triggered macb_mac_link_up()
> re-enables RE|TE and re-writes the per-queue IER with
> bp->rx_intr_mask | MACB_TX_INT_FLAGS | MACB_BIT(HRESP) after the RX queues
> have been tied off or set to MACB_BIT(QUEUE_DISABLE) and NAPI has been
> disabled, on an IRQ that is an armed system wake source.
>
> phylink documents the intended behaviour for this case in
> phylink_suspend():
>
> 	/* We do not call mac_link_down() here as we want the
> 	 * link to remain up to receive the WoL packets.
> 	 */
>
> Would switching macb_suspend()/macb_resume() to phylink_suspend(bp->phylink,
> true) / phylink_resume(), or otherwise stopping the poller on the
> Wake-on-LAN path, be preferable now that a periodic PCS poller exists?

I am really not sure, to be honest. This patch just copies the existing 
logic for the fixed-link case, which also polls. I'm not sure things are 
any different; if this does happen -- and I agree with your logic that 
it could -- it would not be a new behavior.

I am a little reticent to include fixes to this pre-existing situation 
in this patch unless it is unmergeable without doing so, especially 
given that I think a full solution would be a substantially larger 
change, but if it is needed, I would be happy to work on adding it.
-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

[PATCH net-next v4 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 | 30 ++++++++++++++++++------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index c15a9c7e69d3..2252e6a55072 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,
@@ -609,8 +614,10 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
 		new = old | BMCR_ANENABLE;
 	else
 		new = old & ~BMCR_ANENABLE;
-	if (old != new)
+	if (old != new) {
 		gem_writel(bp, PCSCNTRL, new);
+		return 1;
+	}
 
 	return 0;
 }
@@ -750,7 +757,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 +966,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 +1042,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 +1072,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 +4946,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 v4 2/2] net: macb: add support for 1000BASE-X autonegotiation to PCS

[Jakub Kicinski]

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

This decouples PCS enablement from SGMII mode selection so the macb PCS
can also be used in 1000BASE-X mode, advertises 1000BASE-X to phylink,
and adds an autonegotiation restart implementation for the PCS.

A few questions on the changes below.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index c15a9c7e69d3..2252e6a55072 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,
> @@ -609,8 +614,10 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>  		new = old | BMCR_ANENABLE;
>  	else
>  		new = old & ~BMCR_ANENABLE;
> -	if (old != new)
> +	if (old != new) {
>  		gem_writel(bp, PCSCNTRL, new);
> +		return 1;
> +	}
>  
>  	return 0;
>  }

[Severity: Medium]
Is the positive return here attached to the right register write?  The
kernel-doc for .pcs_config() in include/linux/phylink.h says:

  * When an autonegotiation restart is required for 802.3z modes, .pcs_config()
  * should return a positive non-zero integer (e.g. 1) to indicate to phylink
  * to call the pcs_an_restart() method.

and phylink_mii_c22_pcs_config() derives that value from the advertisement
write, not from the BMCR autonegotiation-enable write:

	ret = mdiobus_modify_changed(pcs->bus, pcs->addr,
				     MII_ADVERTISE, 0xffff, adv);
	if (ret < 0)
		return ret;
	changed = ret;

In macb_pcs_config() the earlier PCSANADV (clause 37 base page) write result
is discarded:

	old = gem_readl(bp, PCSANADV);
	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
	if (new != -EINVAL && old != new)
		gem_writel(bp, PCSANADV, new);

and 1 is returned only when the BMCR_ANENABLE bit in PCSCNTRL toggles.

Does that mean an advertisement-only change never restarts autonegotiation?
On an established in-band 1000BASE-X link, ethtool -A or ethtool -s ...
advertise ... reaches phylink_change_inband_advert(), whose only trigger for
the restart is the positive return:

	ret = phylink_pcs_config(pl->pcs, pl->pcs_neg_mode, &pl->link_config,
				 !!(pl->link_config.pause & MLO_PAUSE_AN));
	if (ret < 0)
		return ret;

	if (ret > 0)
		phylink_pcs_an_restart(pl);

Since PCSAUTONEG is on out of reset and phylink keeps it on for in-band mode
(default_an_inband is set in macb_mii_probe()), PCSCNTRL is unchanged, 0 is
returned, and the freshly written base page is never put on the wire until a
link bounce or a larger reconfiguration.  The newly added
macb_pcs_an_restart() would then not be reached for the advertisement case
the ops contract describes, while the case that does return 1 (the
autonegotiation enable/disable toggle) is not the case a restart is asked
for.

Would recording a changed flag in the PCSANADV branch and returning that,
as phylink_mii_c22_pcs_config() does, be more in line with the contract?

Also, could the commit message mention the change of pcs_config() return
semantics?

[ ... ]

> @@ -957,7 +966,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)
                 ^^^^^^^^^

[Severity: Low]
This isn't a bug, but the continuation line in macb_mac_select_pcs() is
indented with a tab followed by spaces, which checkpatch.pl flags as "code
indent should use tabs where possible".

>  		return &bp->phylink_sgmii_pcs;
>  	else
>  		return NULL;

[ ... ]

> @@ -1061,9 +1072,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 +4946,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]
Does the serdes get brought up when phy-mode is "1000base-x"?
init_reset_optional() still gates the serdes and ZynqMP GEM power-management
setup on SGMII only:

	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 = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);

With phy-mode = "1000base-x" on a part with MACB_CAPS_PCS that uses
init_reset_optional(), bp->phy stays NULL, so the later
phy_set_mode_ext(bp->phy, ...) and phy_power_on(bp->phy) calls in
macb_open() are no-ops, while macb_init_dflt() and macb_mac_config() still
set GEM_BIT(PCSSEL) and macb_mii_probe() advertises 1000BASE-X.  Would that
leave the interface up with an unconfigured, unpowered serdes and no
diagnostic?

I could not determine from the tree whether the GT serdes and the
GEM_CONFIG_SGMII_MODE firmware call are strictly required for 1000BASE-X on
those parts, so this may be fine on the affected hardware.  Should
init_reset_optional() cover PHY_INTERFACE_MODE_1000BASEX as well, for
consistency with the four sites updated here?

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

[Nathan Whitehorn]

On 8/11/26 21:54, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net: macb: add support for 1000BASE-X autonegotiation to PCS
>
> This decouples PCS enablement from SGMII mode selection so the macb PCS
> can also be used in 1000BASE-X mode, advertises 1000BASE-X to phylink,
> and adds an autonegotiation restart implementation for the PCS.
>
> A few questions on the changes below.
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index c15a9c7e69d3..2252e6a55072 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,
>> @@ -609,8 +614,10 @@ static int macb_pcs_config(struct phylink_pcs *pcs,
>>   		new = old | BMCR_ANENABLE;
>>   	else
>>   		new = old & ~BMCR_ANENABLE;
>> -	if (old != new)
>> +	if (old != new) {
>>   		gem_writel(bp, PCSCNTRL, new);
>> +		return 1;
>> +	}
>>   
>>   	return 0;
>>   }
> [Severity: Medium]
> Is the positive return here attached to the right register write?  The
> kernel-doc for .pcs_config() in include/linux/phylink.h says:
>
>    * When an autonegotiation restart is required for 802.3z modes, .pcs_config()
>    * should return a positive non-zero integer (e.g. 1) to indicate to phylink
>    * to call the pcs_an_restart() method.
>
> and phylink_mii_c22_pcs_config() derives that value from the advertisement
> write, not from the BMCR autonegotiation-enable write:
>
> 	ret = mdiobus_modify_changed(pcs->bus, pcs->addr,
> 				     MII_ADVERTISE, 0xffff, adv);
> 	if (ret < 0)
> 		return ret;
> 	changed = ret;
>
> In macb_pcs_config() the earlier PCSANADV (clause 37 base page) write result
> is discarded:
>
> 	old = gem_readl(bp, PCSANADV);
> 	new = phylink_mii_c22_pcs_encode_advertisement(interface, advertising);
> 	if (new != -EINVAL && old != new)
> 		gem_writel(bp, PCSANADV, new);
>
> and 1 is returned only when the BMCR_ANENABLE bit in PCSCNTRL toggles.
>
> Does that mean an advertisement-only change never restarts autonegotiation?
> On an established in-band 1000BASE-X link, ethtool -A or ethtool -s ...
> advertise ... reaches phylink_change_inband_advert(), whose only trigger for
> the restart is the positive return:
>
> 	ret = phylink_pcs_config(pl->pcs, pl->pcs_neg_mode, &pl->link_config,
> 				 !!(pl->link_config.pause & MLO_PAUSE_AN));
> 	if (ret < 0)
> 		return ret;
>
> 	if (ret > 0)
> 		phylink_pcs_an_restart(pl);
>
> Since PCSAUTONEG is on out of reset and phylink keeps it on for in-band mode
> (default_an_inband is set in macb_mii_probe()), PCSCNTRL is unchanged, 0 is
> returned, and the freshly written base page is never put on the wire until a
> link bounce or a larger reconfiguration.  The newly added
> macb_pcs_an_restart() would then not be reached for the advertisement case
> the ops contract describes, while the case that does return 1 (the
> autonegotiation enable/disable toggle) is not the case a restart is asked
> for.
>
> Would recording a changed flag in the PCSANADV branch and returning that,
> as phylink_mii_c22_pcs_config() does, be more in line with the contract?
>
> Also, could the commit message mention the change of pcs_config() return
> semantics?
>
> [ ... ]

This is a good catch and the patch is indeed wrong; apologies. I will 
resend a corrected one, and also make the suggested change to the commit 
message.

>> @@ -957,7 +966,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)
>                   ^^^^^^^^^
>
> [Severity: Low]
> This isn't a bug, but the continuation line in macb_mac_select_pcs() is
> indented with a tab followed by spaces, which checkpatch.pl flags as "code
> indent should use tabs where possible".

Ah, OK, this can be fixed.

>
>>   		return &bp->phylink_sgmii_pcs;
>>   	else
>>   		return NULL;
> [ ... ]
>
>> @@ -1061,9 +1072,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 +4946,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]
> Does the serdes get brought up when phy-mode is "1000base-x"?
> init_reset_optional() still gates the serdes and ZynqMP GEM power-management
> setup on SGMII only:
>
> 	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 = zynqmp_pm_set_gem_config(pm_info[1], GEM_CONFIG_SGMII_MODE, 1);
>
> With phy-mode = "1000base-x" on a part with MACB_CAPS_PCS that uses
> init_reset_optional(), bp->phy stays NULL, so the later
> phy_set_mode_ext(bp->phy, ...) and phy_power_on(bp->phy) calls in
> macb_open() are no-ops, while macb_init_dflt() and macb_mac_config() still
> set GEM_BIT(PCSSEL) and macb_mii_probe() advertises 1000BASE-X.  Would that
> leave the interface up with an unconfigured, unpowered serdes and no
> diagnostic?

The equipment we are using this for (custom electronics for a particle 
physics experiment) operates with a phy-mode of "1000base-x" in its 
default device tree, so I can confirm this works fully correctly in that 
mode.

>
> I could not determine from the tree whether the GT serdes and the
> GEM_CONFIG_SGMII_MODE firmware call are strictly required for 1000BASE-X on
> those parts, so this may be fine on the affected hardware.  Should
> init_reset_optional() cover PHY_INTERFACE_MODE_1000BASEX as well, for
> consistency with the four sites updated here?

This particular block, per an earlier discussion 
(https://lore.kernel.org/netdev/DKH9Z0IMH95B.3JWB4PXQC9SN5@bootlin.com/), 
is ZynqMP-specific and onboard-SGMII-PHY specific, so does not apply to 
1000BASE-X mode and I had left it alone here. It certainly could be 
updated for consistency, but that doesn't seem to be needed, either. 
Happy to go either way on this.
-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