[PATCH net-next v2 0/2] Keep PHY link during WoL sleep cycle

[PATCH net-next v2 0/2] Keep PHY link during WoL sleep cycle

[Justin Chen]

First we divide the init/deinit path to allow for a partial init/deinit
during a sleep cycle. We also remove some unnecessary small functions at
the same time.

Then we modify the suspend and resume path to allow for a partial bring
down and bring up. This allow us to keep the PHY link up and to resume
network traffic much quicker. Note we only do this when WoL is enabled
since the PHY is already powered. In the non-WoL case we want to follow
the same flow.

Justin Chen (2):
  net: bcmasp: Divide init to allow partial bring up
  net: bcmasp: Keep PHY link during WoL sleep cycle

 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 251 +++++++++---------
 1 file changed, 130 insertions(+), 121 deletions(-)

-- 
2.34.1

[PATCH net-next v2 1/2] net: bcmasp: Divide init to allow partial bring up

[Justin Chen]

To prepare for a partial bring up of the interface during resume,
we break apart the bcmasp_netif_init() function into smaller chunks
that can be called as necessary. Also consolidate some functions that
do not need to be standalone.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
v2
- Moved tx_lpi_timer read to only bcmasp_open(). We don't want to overwrite
  the SW ctx with the HW default in the resume/suspend case.

 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 210 ++++++++----------
 1 file changed, 98 insertions(+), 112 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index ec63f50a849e..e2b51ec903af 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -344,40 +344,35 @@ static netdev_tx_t bcmasp_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
-static void bcmasp_netif_start(struct net_device *dev)
+static void umac_reset_and_init(struct bcmasp_intf *intf,
+				const unsigned char *addr)
 {
-	struct bcmasp_intf *intf = netdev_priv(dev);
-
-	bcmasp_set_rx_mode(dev);
-	napi_enable(&intf->tx_napi);
-	napi_enable(&intf->rx_napi);
-
-	bcmasp_enable_rx_irq(intf, 1);
-	bcmasp_enable_tx_irq(intf, 1);
-	bcmasp_enable_phy_irq(intf, 1);
-
-	phy_start(dev->phydev);
-}
+	struct phy_device *phydev = intf->ndev->phydev;
+	u32 mac0, mac1;
 
-static void umac_reset(struct bcmasp_intf *intf)
-{
 	umac_wl(intf, 0x0, UMC_CMD);
 	umac_wl(intf, UMC_CMD_SW_RESET, UMC_CMD);
 	usleep_range(10, 100);
 	/* We hold the umac in reset and bring it out of
 	 * reset when phy link is up.
 	 */
-}
 
-static void umac_set_hw_addr(struct bcmasp_intf *intf,
-			     const unsigned char *addr)
-{
-	u32 mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
-		    addr[3];
-	u32 mac1 = (addr[4] << 8) | addr[5];
+	umac_wl(intf, 0x800, UMC_FRM_LEN);
+	umac_wl(intf, 0xffff, UMC_PAUSE_CNTRL);
+	umac_wl(intf, 0x800, UMC_RX_MAX_PKT_SZ);
+
+	mac0 = (addr[0] << 24) | (addr[1] << 16) | (addr[2] << 8) |
+		addr[3];
+	mac1 = (addr[4] << 8) | addr[5];
 
 	umac_wl(intf, mac0, UMC_MAC0);
 	umac_wl(intf, mac1, UMC_MAC1);
+
+	/* Reset shadow values since we reset the umac */
+	intf->old_duplex = -1;
+	intf->old_link = -1;
+	intf->old_pause = -1;
+	phydev->eee_cfg.tx_lpi_timer = umac_rl(intf, UMC_EEE_LPI_TIMER);
 }
 
 static void umac_enable_set(struct bcmasp_intf *intf, u32 mask,
@@ -401,13 +396,6 @@ static void umac_enable_set(struct bcmasp_intf *intf, u32 mask,
 		usleep_range(1000, 2000);
 }
 
-static void umac_init(struct bcmasp_intf *intf)
-{
-	umac_wl(intf, 0x800, UMC_FRM_LEN);
-	umac_wl(intf, 0xffff, UMC_PAUSE_CNTRL);
-	umac_wl(intf, 0x800, UMC_RX_MAX_PKT_SZ);
-}
-
 static int bcmasp_tx_reclaim(struct bcmasp_intf *intf)
 {
 	struct bcmasp_intf_stats64 *stats = &intf->stats64;
@@ -927,6 +915,14 @@ static void bcmasp_rgmii_mode_en_set(struct bcmasp_intf *intf, bool enable)
 	rgmii_wl(intf, reg, RGMII_OOB_CNTRL);
 }
 
+static void bcmasp_phy_hw_unprepare(struct bcmasp_intf *intf)
+{
+	if (intf->internal_phy)
+		bcmasp_ephy_enable_set(intf, false);
+	else
+		bcmasp_rgmii_mode_en_set(intf, false);
+}
+
 static void bcmasp_netif_deinit(struct net_device *dev)
 {
 	struct bcmasp_intf *intf = netdev_priv(dev);
@@ -984,11 +980,7 @@ static int bcmasp_stop(struct net_device *dev)
 
 	phy_disconnect(dev->phydev);
 
-	/* Disable internal EPHY or external PHY */
-	if (intf->internal_phy)
-		bcmasp_ephy_enable_set(intf, false);
-	else
-		bcmasp_rgmii_mode_en_set(intf, false);
+	bcmasp_phy_hw_unprepare(intf);
 
 	/* Disable the interface clocks */
 	bcmasp_core_clock_set_intf(intf, false);
@@ -998,10 +990,15 @@ static int bcmasp_stop(struct net_device *dev)
 	return 0;
 }
 
-static void bcmasp_configure_port(struct bcmasp_intf *intf)
+static void bcmasp_phy_hw_prepare(struct bcmasp_intf *intf)
 {
 	u32 reg, id_mode_dis = 0;
 
+	if (intf->internal_phy)
+		bcmasp_ephy_enable_set(intf, true);
+	else
+		bcmasp_rgmii_mode_en_set(intf, true);
+
 	reg = rgmii_rl(intf, RGMII_PORT_CNTRL);
 	reg &= ~RGMII_PORT_MODE_MASK;
 
@@ -1036,26 +1033,8 @@ static void bcmasp_configure_port(struct bcmasp_intf *intf)
 	rgmii_wl(intf, reg, RGMII_OOB_CNTRL);
 }
 
-static int bcmasp_netif_init(struct net_device *dev, bool phy_connect)
+static phy_interface_t bcmasp_phy_iface_for_connect(phy_interface_t mode)
 {
-	struct bcmasp_intf *intf = netdev_priv(dev);
-	phy_interface_t phy_iface = intf->phy_interface;
-	u32 phy_flags = PHY_BRCM_AUTO_PWRDWN_ENABLE |
-			PHY_BRCM_DIS_TXCRXC_NOENRGY |
-			PHY_BRCM_IDDQ_SUSPEND;
-	struct phy_device *phydev = NULL;
-	int ret;
-
-	/* Always enable interface clocks */
-	bcmasp_core_clock_set_intf(intf, true);
-
-	/* Enable internal PHY or external PHY before any MAC activity */
-	if (intf->internal_phy)
-		bcmasp_ephy_enable_set(intf, true);
-	else
-		bcmasp_rgmii_mode_en_set(intf, true);
-	bcmasp_configure_port(intf);
-
 	/* This is an ugly quirk but we have not been correctly
 	 * interpreting the phy_interface values and we have done that
 	 * across different drivers, so at least we are consistent in
@@ -1081,46 +1060,43 @@ static int bcmasp_netif_init(struct net_device *dev, bool phy_connect)
 	 * affected because they use different phy_interface_t values
 	 * or the Generic PHY driver.
 	 */
-	switch (phy_iface) {
+	switch (mode) {
 	case PHY_INTERFACE_MODE_RGMII:
-		phy_iface = PHY_INTERFACE_MODE_RGMII_ID;
-		break;
+		return PHY_INTERFACE_MODE_RGMII_ID;
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		phy_iface = PHY_INTERFACE_MODE_RGMII_RXID;
-		break;
+		return PHY_INTERFACE_MODE_RGMII_RXID;
 	default:
-		break;
+		return mode;
 	}
+}
 
-	if (phy_connect) {
-		phydev = of_phy_connect(dev, intf->phy_dn,
-					bcmasp_adj_link, phy_flags,
-					phy_iface);
-		if (!phydev) {
-			ret = -ENODEV;
-			netdev_err(dev, "could not attach to PHY\n");
-			goto err_phy_disable;
-		}
-
-		if (intf->internal_phy)
-			dev->phydev->irq = PHY_MAC_INTERRUPT;
-
-		/* Indicate that the MAC is responsible for PHY PM */
-		phydev->mac_managed_pm = true;
-
-		/* Set phylib's copy of the LPI timer */
-		phydev->eee_cfg.tx_lpi_timer = umac_rl(intf, UMC_EEE_LPI_TIMER);
+static int bcmasp_phy_attach(struct bcmasp_intf *intf)
+{
+	u32 phy_flags = PHY_BRCM_AUTO_PWRDWN_ENABLE |
+			PHY_BRCM_DIS_TXCRXC_NOENRGY |
+			PHY_BRCM_IDDQ_SUSPEND;
+	struct phy_device *phydev;
+	phy_interface_t phy_iface;
+
+	phy_iface = bcmasp_phy_iface_for_connect(intf->phy_interface);
+	phydev = of_phy_connect(intf->ndev, intf->phy_dn,
+				bcmasp_adj_link, phy_flags,
+				phy_iface);
+	if (!phydev) {
+		netdev_err(intf->ndev, "could not attach to PHY\n");
+		return -ENODEV;
 	}
+	if (intf->internal_phy)
+		intf->ndev->phydev->irq = PHY_MAC_INTERRUPT;
 
-	umac_reset(intf);
-
-	umac_init(intf);
+	phydev->mac_managed_pm = true;
 
-	umac_set_hw_addr(intf, dev->dev_addr);
+	return 0;
+}
 
-	intf->old_duplex = -1;
-	intf->old_link = -1;
-	intf->old_pause = -1;
+static void bcmasp_netif_init(struct net_device *dev)
+{
+	struct bcmasp_intf *intf = netdev_priv(dev);
 
 	bcmasp_init_tx(intf);
 	netif_napi_add_tx(intf->ndev, &intf->tx_napi, bcmasp_tx_poll);
@@ -1132,18 +1108,13 @@ static int bcmasp_netif_init(struct net_device *dev, bool phy_connect)
 
 	intf->crc_fwd = !!(umac_rl(intf, UMC_CMD) & UMC_CMD_CRC_FWD);
 
-	bcmasp_netif_start(dev);
-
-	netif_start_queue(dev);
-
-	return 0;
+	bcmasp_set_rx_mode(dev);
+	napi_enable(&intf->tx_napi);
+	napi_enable(&intf->rx_napi);
 
-err_phy_disable:
-	if (intf->internal_phy)
-		bcmasp_ephy_enable_set(intf, false);
-	else
-		bcmasp_rgmii_mode_en_set(intf, false);
-	return ret;
+	bcmasp_enable_rx_irq(intf, 1);
+	bcmasp_enable_tx_irq(intf, 1);
+	bcmasp_enable_phy_irq(intf, 1);
 }
 
 static int bcmasp_open(struct net_device *dev)
@@ -1161,14 +1132,30 @@ static int bcmasp_open(struct net_device *dev)
 	if (ret)
 		goto err_free_mem;
 
-	ret = bcmasp_netif_init(dev, true);
-	if (ret) {
-		clk_disable_unprepare(intf->parent->clk);
-		goto err_free_mem;
-	}
+	bcmasp_core_clock_set_intf(intf, true);
+
+	bcmasp_phy_hw_prepare(intf);
+
+	ret = bcmasp_phy_attach(intf);
+	if (ret)
+		goto err_phy_attach;
+
+	umac_reset_and_init(intf, dev->dev_addr);
+
+	dev->phydev->eee_cfg.tx_lpi_timer = umac_rl(intf, UMC_EEE_LPI_TIMER);
+
+	bcmasp_netif_init(dev);
+
+	phy_start(dev->phydev);
+
+	netif_start_queue(dev);
 
 	return ret;
 
+err_phy_attach:
+	bcmasp_phy_hw_unprepare(intf);
+	bcmasp_core_clock_set_intf(intf, false);
+	clk_disable_unprepare(intf->parent->clk);
 err_free_mem:
 	bcmasp_reclaim_free_buffers(intf);
 
@@ -1407,10 +1394,7 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
 	bcmasp_netif_deinit(dev);
 
 	if (!intf->wolopts) {
-		if (intf->internal_phy)
-			bcmasp_ephy_enable_set(intf, false);
-		else
-			bcmasp_rgmii_mode_en_set(intf, false);
+		bcmasp_phy_hw_unprepare(intf);
 
 		/* If Wake-on-LAN is disabled, we can safely
 		 * disable the network interface clocks.
@@ -1454,17 +1438,19 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
 	if (ret)
 		return ret;
 
-	ret = bcmasp_netif_init(dev, false);
-	if (ret)
-		goto out;
+	bcmasp_core_clock_set_intf(intf, true);
 
 	bcmasp_resume_from_wol(intf);
 
+	bcmasp_phy_hw_prepare(intf);
+
+	umac_reset_and_init(intf, dev->dev_addr);
+
+	bcmasp_netif_init(dev);
+
+	phy_start(dev->phydev);
+
 	netif_device_attach(dev);
 
 	return 0;
-
-out:
-	clk_disable_unprepare(intf->parent->clk);
-	return ret;
 }
-- 
2.34.1

Re: [PATCH net-next v2 1/2] net: bcmasp: Divide init to allow partial bring up

[Florian Fainelli]

On 5/1/2026 11:56 AM, Justin Chen wrote:
> To prepare for a partial bring up of the interface during resume,
> we break apart the bcmasp_netif_init() function into smaller chunks
> that can be called as necessary. Also consolidate some functions that
> do not need to be standalone.
> 
> Signed-off-by: Justin Chen <justin.chen@broadcom.com>

Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
-- 
Florian

[PATCH net-next v2 2/2] net: bcmasp: Keep PHY link during WoL sleep cycle

[Justin Chen]

We currently more or less restart all the HW on resume whatever WoL is
enabled or not. The PHY is powered when WoL is enabled which means we
have no reason to re-negotiate the link on resume. So instead of doing
a full restart in this case, we do a partial restart and keep the
negotiated link. That way we can resume network taffic with a much smaller
delay. In the non-WoL case, we do not want to keep the PHY powered, so we
follow the same flow.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
v2
- In the resume case with a HW reset. We trigger a relink by setting the link
  to PHY_UP instead of calling phy_restart_aneg().

 .../net/ethernet/broadcom/asp2/bcmasp_intf.c  | 51 ++++++++++++++-----
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
index e2b51ec903af..9ffd0dfa5969 100644
--- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
+++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
@@ -923,7 +923,7 @@ static void bcmasp_phy_hw_unprepare(struct bcmasp_intf *intf)
 		bcmasp_rgmii_mode_en_set(intf, false);
 }
 
-static void bcmasp_netif_deinit(struct net_device *dev)
+static void bcmasp_netif_deinit(struct net_device *dev, bool stop_phy)
 {
 	struct bcmasp_intf *intf = netdev_priv(dev);
 	u32 reg, timeout = 1000;
@@ -946,7 +946,8 @@ static void bcmasp_netif_deinit(struct net_device *dev)
 
 	umac_enable_set(intf, UMC_CMD_TX_EN, 0);
 
-	phy_stop(dev->phydev);
+	if (stop_phy)
+		phy_stop(dev->phydev);
 
 	umac_enable_set(intf, UMC_CMD_RX_EN, 0);
 
@@ -974,7 +975,7 @@ static int bcmasp_stop(struct net_device *dev)
 	/* Stop tx from updating HW */
 	netif_tx_disable(dev);
 
-	bcmasp_netif_deinit(dev);
+	bcmasp_netif_deinit(dev, true);
 
 	bcmasp_reclaim_free_buffers(intf);
 
@@ -1385,15 +1386,20 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
 {
 	struct device *kdev = &intf->parent->pdev->dev;
 	struct net_device *dev = intf->ndev;
+	bool wake;
 
 	if (!netif_running(dev))
 		return 0;
 
 	netif_device_detach(dev);
 
-	bcmasp_netif_deinit(dev);
+	wake = device_may_wakeup(kdev) && intf->wolopts;
 
-	if (!intf->wolopts) {
+	bcmasp_netif_deinit(dev, !wake);
+
+	if (wake) {
+		bcmasp_suspend_to_wol(intf);
+	} else {
 		bcmasp_phy_hw_unprepare(intf);
 
 		/* If Wake-on-LAN is disabled, we can safely
@@ -1402,9 +1408,6 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
 		bcmasp_core_clock_set_intf(intf, false);
 	}
 
-	if (device_may_wakeup(kdev) && intf->wolopts)
-		bcmasp_suspend_to_wol(intf);
-
 	clk_disable_unprepare(intf->parent->clk);
 
 	return 0;
@@ -1428,8 +1431,11 @@ static void bcmasp_resume_from_wol(struct bcmasp_intf *intf)
 
 int bcmasp_interface_resume(struct bcmasp_intf *intf)
 {
+	struct device *kdev = &intf->parent->pdev->dev;
 	struct net_device *dev = intf->ndev;
+	bool wake;
 	int ret;
+	u32 reg;
 
 	if (!netif_running(dev))
 		return 0;
@@ -1438,17 +1444,34 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
 	if (ret)
 		return ret;
 
-	bcmasp_core_clock_set_intf(intf, true);
-
-	bcmasp_resume_from_wol(intf);
+	wake = device_may_wakeup(kdev) && intf->wolopts;
 
-	bcmasp_phy_hw_prepare(intf);
+	bcmasp_core_clock_set_intf(intf, true);
 
-	umac_reset_and_init(intf, dev->dev_addr);
+	/* The interface might be HW reset in some suspend modes, so we may
+	 * need to restore the UNIMAC/PHY if that is the case.
+	 */
+	reg = umac_rl(intf, UMC_CMD);
+	if (wake && (reg & UMC_CMD_RX_EN)) {
+		umac_enable_set(intf, UMC_CMD_TX_EN, 1);
+		bcmasp_resume_from_wol(intf);
+	} else {
+		bcmasp_phy_hw_prepare(intf);
+		umac_reset_and_init(intf, dev->dev_addr);
+	}
 
 	bcmasp_netif_init(dev);
 
-	phy_start(dev->phydev);
+	/* If HW was reset, we need to force a relink */
+	if (wake && !(reg & UMC_CMD_RX_EN)) {
+		mutex_lock(&dev->phydev->lock);
+		if (dev->phydev->state == PHY_RUNNING)
+			dev->phydev->state = PHY_UP;
+		mutex_unlock(&dev->phydev->lock);
+		phy_trigger_machine(dev->phydev);
+	} else if (!wake) {
+		phy_start(dev->phydev);
+	}
 
 	netif_device_attach(dev);
 
-- 
2.34.1

Re: [PATCH net-next v2 2/2] net: bcmasp: Keep PHY link during WoL sleep cycle

[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: bcmasp: Keep PHY link during WoL sleep cycle

This patch splits the suspend/resume paths so that when WoL is enabled
the PHY is left running and the MAC is only partially re-initialized on
resume. phy_stop() is skipped during suspend and phy_start() is replaced
with a forced relink via phy_trigger_machine() when the MAC was reset.

> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> index e2b51ec903af..9ffd0dfa5969 100644
> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c

[ ... ]

> @@ -946,7 +946,8 @@ static void bcmasp_netif_deinit(struct net_device *dev)
>  
>  	umac_enable_set(intf, UMC_CMD_TX_EN, 0);
>  
> -	phy_stop(dev->phydev);
> +	if (stop_phy)
> +		phy_stop(dev->phydev);

Skipping phy_stop() in the WoL path means phy_stop_machine() and the
cancel_delayed_work_sync() on phydev->state_queue are no longer executed
before the ASP clock is disabled in bcmasp_interface_suspend().

The PHY state machine runs on system_power_efficient_wq, which is not
WQ_FREEZABLE, so task freeze does not stop it.  If a PHY IRQ fires (or
the polling timer expires) between clk_disable_unprepare(intf->parent->clk)
and the matching clk_prepare_enable() on resume, the state machine can
reach bcmasp_adj_link() via phy_check_link_status() ->
phy_link_up()/phy_link_down() -> phydev->adjust_link.

bcmasp_adj_link() reads and writes UMC_CMD, UMC_EEE_LPI_TIMER,
UMC_EEE_CTRL and RGMII_OOB_CNTRL, which sit behind the disabled ASP
clock.  Can those accesses hang the bus or return 0xffffffff on these
SoCs?

bcmgenet pins phydev->state = PHY_READY during WoL suspend specifically
to freeze the state machine in a no-op state.  Is an equivalent
protection needed here?

> @@ -1385,15 +1386,20 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
>  {
>  	struct device *kdev = &intf->parent->pdev->dev;
>  	struct net_device *dev = intf->ndev;
> +	bool wake;
>  
>  	if (!netif_running(dev))
>  		return 0;
>  
>  	netif_device_detach(dev);
>  
> -	bcmasp_netif_deinit(dev);
> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>  
> -	if (!intf->wolopts) {
> +	bcmasp_netif_deinit(dev, !wake);
> +
> +	if (wake) {
> +		bcmasp_suspend_to_wol(intf);
> +	} else {
>  		bcmasp_phy_hw_unprepare(intf);

[ ... ]

> @@ -1438,17 +1444,34 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
>  	if (ret)
>  		return ret;
>  
> -	bcmasp_core_clock_set_intf(intf, true);
> -
> -	bcmasp_resume_from_wol(intf);
> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>  
> -	bcmasp_phy_hw_prepare(intf);
> +	bcmasp_core_clock_set_intf(intf, true);
>  
> -	umac_reset_and_init(intf, dev->dev_addr);
> +	/* The interface might be HW reset in some suspend modes, so we may
> +	 * need to restore the UNIMAC/PHY if that is the case.
> +	 */
> +	reg = umac_rl(intf, UMC_CMD);
> +	if (wake && (reg & UMC_CMD_RX_EN)) {
> +		umac_enable_set(intf, UMC_CMD_TX_EN, 1);
> +		bcmasp_resume_from_wol(intf);
> +	} else {
> +		bcmasp_phy_hw_prepare(intf);
> +		umac_reset_and_init(intf, dev->dev_addr);
> +	}

In the wake && !(reg & UMC_CMD_RX_EN) branch (MAC was HW reset during
suspend), bcmasp_resume_from_wol() is not called.

bcmasp_suspend_to_wol() unconditionally does two things that live in
the shared parent register block, not the per-interface UMC block:

	wakeup_intr2_core_wl(intf->parent, 0xffffffff,
			     ASP_WAKEUP_INTR2_MASK_CLEAR);

	if (ndev->phydev && ndev->phydev->eee_cfg.eee_enabled &&
	    intf->parent->eee_fixup)
		intf->parent->eee_fixup(intf, true);

The wakeup interrupt mask and the EEE fixup bits (phy_lpi_overwrite in
ASP_EDPKT_SPARE_REG) are not cleared by a reset that only wipes UMC_CMD.

In this branch the wakeup IRQ remains unmasked (bcmasp_isr_wol would
then fire on subsequent MPD/FILT events during normal traffic) and the
EEE fixup stays asserted.  Should this branch also invoke the parts of
bcmasp_resume_from_wol() / eee_fixup(false) that live in the shared
register block?

Separately, bcmasp_suspend_to_wol() explicitly programs
UMC_CMD_PROMISC:

	reg |= UMC_CMD_RX_EN | UMC_CMD_PROMISC;
	umac_wl(intf, reg, UMC_CMD);

In the wake && (reg & UMC_CMD_RX_EN) fast-resume branch, the only
writes to UMC_CMD are:

	umac_enable_set(intf, UMC_CMD_TX_EN, 1);

and whatever bcmasp_resume_from_wol() does, which only touches
UMC_MPD_CTRL and ASP_WAKEUP_INTR2_MASK_SET.  bcmasp_netif_init() calls
bcmasp_set_rx_mode() which manages ASP_RX_FILTER_MDA but not
UMC_CMD_PROMISC, and bcmasp_adj_link() only reprograms PROMISC inside
its if (reg & UMC_CMD_SW_RESET) branch (not taken here).  Since
old_link/old_duplex/old_pause are preserved across the fast path,
adj_link's changed flag stays zero and it returns without rewriting
UMC_CMD.

Does this leave the MAC stuck in UMC_CMD_PROMISC after every WoL
resume, until the interface is toggled down/up?

>  
>  	bcmasp_netif_init(dev);
>  
> -	phy_start(dev->phydev);
> +	/* If HW was reset, we need to force a relink */
> +	if (wake && !(reg & UMC_CMD_RX_EN)) {
> +		mutex_lock(&dev->phydev->lock);
> +		if (dev->phydev->state == PHY_RUNNING)
> +			dev->phydev->state = PHY_UP;
> +		mutex_unlock(&dev->phydev->lock);
> +		phy_trigger_machine(dev->phydev);
> +	} else if (!wake) {
> +		phy_start(dev->phydev);
> +	}

The forced-relink branch only demotes PHY_RUNNING -> PHY_UP before
phy_trigger_machine().  Since phy_stop() was skipped during suspend,
phydev->state is whatever it was at suspend time, which may instead be
PHY_NOLINK (cable was unplugged), PHY_HALTED or PHY_ERROR.

For PHY_NOLINK, _phy_state_machine() runs phy_check_link_status() +
phy_update_stats() only; it does not trigger _phy_start_aneg() (that is
the PHY_UP case).  If the HW reset that cleared UMC_CMD_RX_EN also
touched the PHY chip (shared reset/power domain), autoneg may need to
be restarted.

Does this path handle states other than PHY_RUNNING, and does it
guarantee the PHY is reinitialized and autoneg restarted when the MAC
has been reset?

> +
>  	netif_device_attach(dev);
>  
>  	return 0;

Re: [PATCH net-next v2 2/2] net: bcmasp: Keep PHY link during WoL sleep cycle

[Justin Chen]

On 5/5/26 5:25 PM, 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: bcmasp: Keep PHY link during WoL sleep cycle
> 
> This patch splits the suspend/resume paths so that when WoL is enabled
> the PHY is left running and the MAC is only partially re-initialized on
> resume. phy_stop() is skipped during suspend and phy_start() is replaced
> with a forced relink via phy_trigger_machine() when the MAC was reset.
> 
>> diff --git a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
>> index e2b51ec903af..9ffd0dfa5969 100644
>> --- a/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
>> +++ b/drivers/net/ethernet/broadcom/asp2/bcmasp_intf.c
> 
> [ ... ]
> 
>> @@ -946,7 +946,8 @@ static void bcmasp_netif_deinit(struct net_device *dev)
>>   
>>   	umac_enable_set(intf, UMC_CMD_TX_EN, 0);
>>   
>> -	phy_stop(dev->phydev);
>> +	if (stop_phy)
>> +		phy_stop(dev->phydev);
> 
> Skipping phy_stop() in the WoL path means phy_stop_machine() and the
> cancel_delayed_work_sync() on phydev->state_queue are no longer executed
> before the ASP clock is disabled in bcmasp_interface_suspend().
> 
> The PHY state machine runs on system_power_efficient_wq, which is not
> WQ_FREEZABLE, so task freeze does not stop it.  If a PHY IRQ fires (or
> the polling timer expires) between clk_disable_unprepare(intf->parent->clk)
> and the matching clk_prepare_enable() on resume, the state machine can
> reach bcmasp_adj_link() via phy_check_link_status() ->
> phy_link_up()/phy_link_down() -> phydev->adjust_link.
> 
> bcmasp_adj_link() reads and writes UMC_CMD, UMC_EEE_LPI_TIMER,
> UMC_EEE_CTRL and RGMII_OOB_CNTRL, which sit behind the disabled ASP
> clock.  Can those accesses hang the bus or return 0xffffffff on these
> SoCs?
> 
> bcmgenet pins phydev->state = PHY_READY during WoL suspend specifically
> to freeze the state machine in a no-op state.  Is an equivalent
> protection needed here?
> 

Will mirror something closer to bcmgenet for v3.

>> @@ -1385,15 +1386,20 @@ int bcmasp_interface_suspend(struct bcmasp_intf *intf)
>>   {
>>   	struct device *kdev = &intf->parent->pdev->dev;
>>   	struct net_device *dev = intf->ndev;
>> +	bool wake;
>>   
>>   	if (!netif_running(dev))
>>   		return 0;
>>   
>>   	netif_device_detach(dev);
>>   
>> -	bcmasp_netif_deinit(dev);
>> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>>   
>> -	if (!intf->wolopts) {
>> +	bcmasp_netif_deinit(dev, !wake);
>> +
>> +	if (wake) {
>> +		bcmasp_suspend_to_wol(intf);
>> +	} else {
>>   		bcmasp_phy_hw_unprepare(intf);
> 
> [ ... ]
> 
>> @@ -1438,17 +1444,34 @@ int bcmasp_interface_resume(struct bcmasp_intf *intf)
>>   	if (ret)
>>   		return ret;
>>   
>> -	bcmasp_core_clock_set_intf(intf, true);
>> -
>> -	bcmasp_resume_from_wol(intf);
>> +	wake = device_may_wakeup(kdev) && intf->wolopts;
>>   
>> -	bcmasp_phy_hw_prepare(intf);
>> +	bcmasp_core_clock_set_intf(intf, true);
>>   
>> -	umac_reset_and_init(intf, dev->dev_addr);
>> +	/* The interface might be HW reset in some suspend modes, so we may
>> +	 * need to restore the UNIMAC/PHY if that is the case.
>> +	 */
>> +	reg = umac_rl(intf, UMC_CMD);
>> +	if (wake && (reg & UMC_CMD_RX_EN)) {
>> +		umac_enable_set(intf, UMC_CMD_TX_EN, 1);
>> +		bcmasp_resume_from_wol(intf);
>> +	} else {
>> +		bcmasp_phy_hw_prepare(intf);
>> +		umac_reset_and_init(intf, dev->dev_addr);
>> +	}
> 
> In the wake && !(reg & UMC_CMD_RX_EN) branch (MAC was HW reset during
> suspend), bcmasp_resume_from_wol() is not called.

In this case, the HW is reset. So bcmasp_resume_from_wol() doesn't need 
to restore what bcmasp_suspend_to_wol() did as it was erased.

Same thing for PROMISC. The default state is SW_RESET, so when we force 
the state machine to run again it will reset PROMISC. So everything 
below should be irrelevant. I also verified on resume in this case the 
registers are the same, so we resumed back to the original state.

> 
> bcmasp_suspend_to_wol() unconditionally does two things that live in
> the shared parent register block, not the per-interface UMC block:
> 
> 	wakeup_intr2_core_wl(intf->parent, 0xffffffff,
> 			     ASP_WAKEUP_INTR2_MASK_CLEAR);
> 
> 	if (ndev->phydev && ndev->phydev->eee_cfg.eee_enabled &&
> 	    intf->parent->eee_fixup)
> 		intf->parent->eee_fixup(intf, true);
> 
> The wakeup interrupt mask and the EEE fixup bits (phy_lpi_overwrite in
> ASP_EDPKT_SPARE_REG) are not cleared by a reset that only wipes UMC_CMD.
> 
> In this branch the wakeup IRQ remains unmasked (bcmasp_isr_wol would
> then fire on subsequent MPD/FILT events during normal traffic) and the
> EEE fixup stays asserted.  Should this branch also invoke the parts of
> bcmasp_resume_from_wol() / eee_fixup(false) that live in the shared
> register block?
> 
> Separately, bcmasp_suspend_to_wol() explicitly programs
> UMC_CMD_PROMISC:
> 
> 	reg |= UMC_CMD_RX_EN | UMC_CMD_PROMISC;
> 	umac_wl(intf, reg, UMC_CMD);
> 
> In the wake && (reg & UMC_CMD_RX_EN) fast-resume branch, the only
> writes to UMC_CMD are:
> 
> 	umac_enable_set(intf, UMC_CMD_TX_EN, 1);
> 
> and whatever bcmasp_resume_from_wol() does, which only touches
> UMC_MPD_CTRL and ASP_WAKEUP_INTR2_MASK_SET.  bcmasp_netif_init() calls
> bcmasp_set_rx_mode() which manages ASP_RX_FILTER_MDA but not
> UMC_CMD_PROMISC, and bcmasp_adj_link() only reprograms PROMISC inside
> its if (reg & UMC_CMD_SW_RESET) branch (not taken here).  Since
> old_link/old_duplex/old_pause are preserved across the fast path,
> adj_link's changed flag stays zero and it returns without rewriting
> UMC_CMD.
> 
> Does this leave the MAC stuck in UMC_CMD_PROMISC after every WoL
> resume, until the interface is toggled down/up?
> 
>>   
>>   	bcmasp_netif_init(dev);
>>   
>> -	phy_start(dev->phydev);
>> +	/* If HW was reset, we need to force a relink */
>> +	if (wake && !(reg & UMC_CMD_RX_EN)) {
>> +		mutex_lock(&dev->phydev->lock);
>> +		if (dev->phydev->state == PHY_RUNNING)
>> +			dev->phydev->state = PHY_UP;
>> +		mutex_unlock(&dev->phydev->lock);
>> +		phy_trigger_machine(dev->phydev);
>> +	} else if (!wake) {
>> +		phy_start(dev->phydev);
>> +	}
> 
> The forced-relink branch only demotes PHY_RUNNING -> PHY_UP before
> phy_trigger_machine().  Since phy_stop() was skipped during suspend,
> phydev->state is whatever it was at suspend time, which may instead be
> PHY_NOLINK (cable was unplugged), PHY_HALTED or PHY_ERROR.
> 
> For PHY_NOLINK, _phy_state_machine() runs phy_check_link_status() +
> phy_update_stats() only; it does not trigger _phy_start_aneg() (that is
> the PHY_UP case).  If the HW reset that cleared UMC_CMD_RX_EN also
> touched the PHY chip (shared reset/power domain), autoneg may need to
> be restarted.
> 
> Does this path handle states other than PHY_RUNNING, and does it
> guarantee the PHY is reinitialized and autoneg restarted when the MAC
> has been reset?
> 

Will mirror to be more like bcmgenet in v3. The PHY chip is not reset or 
powered off in the WoL case, so we do not need to restart autoneg. It 
should look similar to the non HW reset case except we need to reprogram 
the UNIMAC.

Thanks,
Justin

>> +
>>   	netif_device_attach(dev);
>>   
>>   	return 0;