diff mbox series

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

Message ID 1691414091-2260697-1-git-send-email-radhey.shyam.pandey@amd.com (mailing list archive)
State Accepted
Commit 6c461e394d11a981c662cc16cebfb05b602e23ba
Delegated to: Netdev Maintainers
Headers show
Series [net] net: macb: In ZynqMP resume always configure PS GTR for non-wakeup source | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1349 this patch: 1349
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 1351 this patch: 1351
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1372 this patch: 1372
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 33 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Pandey, Radhey Shyam Aug. 7, 2023, 1:14 p.m. UTC 
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>
---
 drivers/net/ethernet/cadence/macb_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Jakub Kicinski Aug. 9, 2023, 10:41 p.m. UTC | #1 
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();
Russell King (Oracle) Aug. 9, 2023, 11:04 p.m. UTC | #2 
On Wed, Aug 09, 2023 at 03:41:21PM -0700, Jakub Kicinski wrote:
> 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.

This is not ethernet PHY stuff (drivers/net/phy), it's serdes PHY
(drivers/phy). Their requirements vary a lot.

Given the above description...

In (1), phy_power_off() gets called. If this is actually done by
the serdes PHY driver, then the clocks from the serdes PHY stop.

Whether the PHY is re-initialised or not during the suspend/resume
should not affect it - we haven't asked for it to be powered up
again.

So in (5), I assume that the problem occurs because macb_init_hw()
is called before phy_power_on()... but what do I know... with these
randomly generated abbreviations that are designed to exclude people
from understanding what is going on which seem to be common in the
computing world. FPD? GT? Shrug, utterly meaningless.

However, surely phy_init() while the PHY is supposed to be powered
off should fail? Don't know, this is outside of my area of knowledge.
Adding generic phy maintainers and list...
patchwork-bot+netdevbpf@kernel.org Aug. 15, 2023, 2:10 a.m. UTC | #3 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 7 Aug 2023 18:44:51 +0530 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net: macb: In ZynqMP resume always configure PS GTR for non-wakeup source
    https://git.kernel.org/netdev/net/c/6c461e394d11

You are awesome, thank you!
diff mbox series

Patch

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