Re: [PATCH net] net: macb: In ZynqMP resume always configure PS GTR for non-wakeup source

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 7 Aug 2023 18:44:51 +0530 Radhey Shyam Pandey wrote:
> On Zynq UltraScale+ MPSoC ubuntu platform when systemctl issues suspend,
> network manager bring down the interface and goes into suspend. When it
> wakes up it again enables the interface.
> 
> This leads to xilinx-psgtr "PLL lock timeout" on interface bringup, as
> the power management controller power down the entire FPD (including
> SERDES) if none of the FPD devices are in use and serdes is not
> initialized on resume.
> 
> $ sudo rtcwake -m no -s 120 -v
> $ sudo systemctl suspend  <this does ifconfig eth1 down>
> $ ifconfig eth1 up
> xilinx-psgtr fd400000.phy: lane 0 (type 10, protocol 5): PLL lock timeout
> phy phy-fd400000.phy.0: phy poweron failed --> -110
> 
> macb driver is called in this way:
> 1. macb_close: Stop network interface. In this function, it
>    reset MACB IP and disables PHY and network interface.
> 
> 2. macb_suspend: It is called in kernel suspend flow. But because
>    network interface has been disabled(netif_running(ndev) is
>    false), it does nothing and returns directly;
> 
> 3. System goes into suspend state. Some time later, system is
>    waken up by RTC wakeup device;
> 
> 4. macb_resume: It does nothing because network interface has
>    been disabled;
> 
> 5. macb_open: It is called to enable network interface again. ethernet
>    interface is initialized in this API but serdes which is power-off
>    by PMUFW during FPD-off suspend is not initialized again and so
>    we hit GT PLL lock issue on open.
> 
> To resolve this PLL timeout issue always do PS GTR initialization
> when ethernet device is configured as non-wakeup source.
> 
> Fixes: f22bd29ba19a ("net: macb: Fix ZynqMP SGMII non-wakeup source resume failure")
> Fixes: 8b73fa3ae02b ("net: macb: Added ZynqMP-specific initialization")
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>

Could be more of a PHY than MAC question. Adding remaining PHY
maintainers to CC.

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index f6a0f12a6d52..82929ee76739 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -5194,6 +5194,9 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  	unsigned int q;
>  	int err;
>  
> +	if (!device_may_wakeup(&bp->dev->dev))
> +		phy_exit(bp->sgmii_phy);
> +
>  	if (!netif_running(netdev))
>  		return 0;
>  
> @@ -5254,7 +5257,6 @@ static int __maybe_unused macb_suspend(struct device *dev)
>  	if (!(bp->wol & MACB_WOL_ENABLED)) {
>  		rtnl_lock();
>  		phylink_stop(bp->phylink);
> -		phy_exit(bp->sgmii_phy);
>  		rtnl_unlock();
>  		spin_lock_irqsave(&bp->lock, flags);
>  		macb_reset_hw(bp);
> @@ -5284,6 +5286,9 @@ static int __maybe_unused macb_resume(struct device *dev)
>  	unsigned int q;
>  	int err;
>  
> +	if (!device_may_wakeup(&bp->dev->dev))
> +		phy_init(bp->sgmii_phy);
> +
>  	if (!netif_running(netdev))
>  		return 0;
>  
> @@ -5344,8 +5349,6 @@ static int __maybe_unused macb_resume(struct device *dev)
>  	macb_set_rx_mode(netdev);
>  	macb_restore_features(bp);
>  	rtnl_lock();
> -	if (!device_may_wakeup(&bp->dev->dev))
> -		phy_init(bp->sgmii_phy);
>  
>  	phylink_start(bp->phylink);
>  	rtnl_unlock();