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

[Justin Chen <justin.chen@broadcom.com>]

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;