[PATCH v2 0/2] net: macb: fix connectivity after resume

[PATCH v2 0/2] net: macb: fix connectivity after resume

[Claudiu Beznea]

Hi,

This series fixes connectivity on SAMA7G5 with KSZ9131 ethernet PHY.
Driver fix is in patch 2/2. Patch 1/2 is a prerequisite.

Thank you,
Claudiu Beznea

Changes in v2:
- patch 1 has been updated to call phy_init_hw() on phylink_resume()
- patch 2 is new; the previous one was only calling phy_init_hw()

Claudiu Beznea (2):
  net: phylink: init phydev on phylink_resume()
  net: macb: use phylink_suspend()/phylink_resume()

 drivers/net/ethernet/cadence/macb_main.c | 17 +++++++----------
 drivers/net/phy/phylink.c                |  6 ++++++
 2 files changed, 13 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] net: phylink: init phydev on phylink_resume()

[Claudiu Beznea]

There are scenarios where PHY power is cut off on system suspend.
There are also MAC drivers which handles themselves the PHY on
suspend/resume path. For such drivers the
struct phy_device::mac_managed_phy is set to true and thus the
mdio_bus_phy_suspend()/mdio_bus_phy_resume() wouldn't do the
proper PHY suspend/resume. For such scenarios call phy_init_hw()
from phylink_resume().

Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

Hi, Russel,

I let phy_init_hw() to execute for all devices. I can restrict it only
for PHYs that has struct phy_device::mac_managed_phy = true.

Please let me know what you think.

Thank you,
Claudiu Beznea


 drivers/net/phy/phylink.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 09cc65c0da93..6003c329638e 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2031,6 +2031,12 @@ void phylink_resume(struct phylink *pl)
 {
 	ASSERT_RTNL();
 
+	if (pl->phydev) {
+		int ret = phy_init_hw(pl->phydev);
+		if (ret)
+			return;
+	}
+
 	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
 		/* Wake-on-Lan enabled, MAC handling */
 
-- 
2.34.1

[PATCH v2 2/2] net: macb: use phylink_suspend()/phylink_resume()

[Claudiu Beznea]

Use phylink_suspend() and phylink_resume() for macb driver instead
of phylink_start()/phylink_stop(). This helps on fixing
commit bf0ad1893442 ("net: macb: Specify PHY PM management done by MAC").

Commit bf0ad1893442 ("net: macb: Specify PHY PM management done by MAC")
signals to PHY layer that the PHY PM management is done by the MAC driver
itself. In case this is done the mdio_bus_phy_suspend() and
mdio_bus_phy_resume() will return just at its beginning letting the MAC
driver to handle the PHY power management.

AT91 devices (e.g. SAMA7G5, SAMA5D2) has a special power saving mode
called backup and self-refresh where most of the SoCs parts are shutdown
on suspend and RAM is switched to self-refresh. The rail powering the
on-board ethernet PHY could also be closed.

For scenarios where backup and self-refresh is used the MACB driver needs
to re-initialize the PHY device itself when resuming. Otherwise there is
poor or missing connectivity (e.g. SAMA7G5-EK uses KSZ9131 in RGMII mode
which needs its DLL settings to satisfy RGMII timings). For this call
phylink_suspend()/phylink_resume() on suspend/resume path.

The patch has been tested on SAMA7G5EK (with KSZ9131 and KSZ8081 PHYs)
and SAM9X60EK (with KSZ8081 PHY) boards.

Fixes: bf0ad1893442 ("net: macb: Specify PHY PM management done by MAC")
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---

This patch depends on patch 1/2 from this series. For proper backporting
to older kernel (in case this series is integrated as is) please add the
Depends-on tag on this patch after patch 1/2 is integrated in networking
tree.

Thank you,
Claudiu Beznea

 drivers/net/ethernet/cadence/macb_main.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 95667b979fab..bcd394093d1c 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5142,9 +5142,13 @@ static int __maybe_unused macb_suspend(struct device *dev)
 		napi_disable(&queue->napi_tx);
 	}
 
-	if (!(bp->wol & MACB_WOL_ENABLED)) {
+	if (bp->wol & MACB_WOL_ENABLED) {
 		rtnl_lock();
-		phylink_stop(bp->phylink);
+		phylink_suspend(bp->phylink, true);
+		rtnl_unlock();
+	} else {
+		rtnl_lock();
+		phylink_suspend(bp->phylink, false);
 		phy_exit(bp->sgmii_phy);
 		rtnl_unlock();
 		spin_lock_irqsave(&bp->lock, flags);
@@ -5209,13 +5213,6 @@ static int __maybe_unused macb_resume(struct device *dev)
 		spin_unlock_irqrestore(&bp->lock, flags);
 
 		disable_irq_wake(bp->queues[0].irq);
-
-		/* Now make sure we disable phy before moving
-		 * to common restore path
-		 */
-		rtnl_lock();
-		phylink_stop(bp->phylink);
-		rtnl_unlock();
 	}
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues;
@@ -5238,7 +5235,7 @@ static int __maybe_unused macb_resume(struct device *dev)
 	if (!device_may_wakeup(&bp->dev->dev))
 		phy_init(bp->sgmii_phy);
 
-	phylink_start(bp->phylink);
+	phylink_resume(bp->phylink);
 	rtnl_unlock();
 
 	netif_device_attach(netdev);
-- 
2.34.1

Re: [PATCH v2 1/2] net: phylink: init phydev on phylink_resume()

[Russell King (Oracle)]

On Mon, Dec 12, 2022 at 01:28:44PM +0200, Claudiu Beznea wrote:
> There are scenarios where PHY power is cut off on system suspend.
> There are also MAC drivers which handles themselves the PHY on
> suspend/resume path. For such drivers the
> struct phy_device::mac_managed_phy is set to true and thus the
> mdio_bus_phy_suspend()/mdio_bus_phy_resume() wouldn't do the
> proper PHY suspend/resume. For such scenarios call phy_init_hw()
> from phylink_resume().
> 
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
> 
> Hi, Russel,
> 
> I let phy_init_hw() to execute for all devices. I can restrict it only
> for PHYs that has struct phy_device::mac_managed_phy = true.
> 
> Please let me know what you think.

I think it would be better to only do this in the path where we call
phy_start() - if we do it in the WoL path (where the PHY remains
running), then there is no phy_start() call, so phy_init_hw() could
result in the PHY not working after a suspend/resume event.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] net: phylink: init phydev on phylink_resume()

[Claudiu.Beznea]

On 12.12.2022 14:47, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Dec 12, 2022 at 01:28:44PM +0200, Claudiu Beznea wrote:
>> There are scenarios where PHY power is cut off on system suspend.
>> There are also MAC drivers which handles themselves the PHY on
>> suspend/resume path. For such drivers the
>> struct phy_device::mac_managed_phy is set to true and thus the
>> mdio_bus_phy_suspend()/mdio_bus_phy_resume() wouldn't do the
>> proper PHY suspend/resume. For such scenarios call phy_init_hw()
>> from phylink_resume().
>>
>> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>
>> Hi, Russel,
>>
>> I let phy_init_hw() to execute for all devices. I can restrict it only
>> for PHYs that has struct phy_device::mac_managed_phy = true.
>>
>> Please let me know what you think.
> 
> I think it would be better to only do this in the path where we call
> phy_start() - if we do it in the WoL path (where the PHY remains
> running), then there is no phy_start() call, so phy_init_hw() could
> result in the PHY not working after a suspend/resume event.

This will not work all the time for MACB usage on AT91 devices.

As explained here [1] the scenario where:
- MACB is configured to handle WoL
- the system goes to a suspend mode (named backup and self-refresh (BSR) in
  our case) with power cut off on PHY and limited wake-up source (few pins
  and RTC alarms)

is still valid. In this case MAC IP and MAC PHY are not powered. And in
those cases phylink_resume() will not hit phy_start().


Just my feeling about this: after looking to phylink_resume() it looks to
me that (at least for MACB on AT91) it is better to have the PHY handling
still in the MAC driver (calling phy_init_hw() from driver itself) as this
was the intent (I think) of struct phy_device::mac_managed_phy. But this is
not based on any technical argument at the moment.

Thank you,
Claudiu Beznea

[1]
https://lore.kernel.org/all/4375d733-ed49-869c-635f-0f0ba7304283@microchip.com/

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] net: phylink: init phydev on phylink_resume()

[Vladimir Oltean]

Hi Claudiu,

On Mon, Dec 12, 2022 at 01:28:44PM +0200, Claudiu Beznea wrote:
> There are scenarios where PHY power is cut off on system suspend.
> There are also MAC drivers which handles themselves the PHY on
> suspend/resume path. For such drivers the
> struct phy_device::mac_managed_phy is set to true and thus the
> mdio_bus_phy_suspend()/mdio_bus_phy_resume() wouldn't do the
> proper PHY suspend/resume. For such scenarios call phy_init_hw()
> from phylink_resume().
> 
> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
> 
> Hi, Russel,
> 
> I let phy_init_hw() to execute for all devices. I can restrict it only
> for PHYs that has struct phy_device::mac_managed_phy = true.
> 
> Please let me know what you think.
> 
> Thank you,
> Claudiu Beznea
> 
> 
>  drivers/net/phy/phylink.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 09cc65c0da93..6003c329638e 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2031,6 +2031,12 @@ void phylink_resume(struct phylink *pl)
>  {
>  	ASSERT_RTNL();
>  
> +	if (pl->phydev) {
> +		int ret = phy_init_hw(pl->phydev);
> +		if (ret)
> +			return;
> +	}
> +

If the config_init() method of the driver does things such as this:

	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;

like for example the marvell10g.c driver, won't a user-configured manual
MDI setting get overwritten after a suspend/resume cycle?

>  	if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
>  		/* Wake-on-Lan enabled, MAC handling */
>  
> -- 
> 2.34.1
> 

Re: [PATCH v2 1/2] net: phylink: init phydev on phylink_resume()

[Russell King (Oracle)]

On Mon, Dec 12, 2022 at 01:26:54PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 12.12.2022 14:47, Russell King (Oracle) wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Mon, Dec 12, 2022 at 01:28:44PM +0200, Claudiu Beznea wrote:
> >> There are scenarios where PHY power is cut off on system suspend.
> >> There are also MAC drivers which handles themselves the PHY on
> >> suspend/resume path. For such drivers the
> >> struct phy_device::mac_managed_phy is set to true and thus the
> >> mdio_bus_phy_suspend()/mdio_bus_phy_resume() wouldn't do the
> >> proper PHY suspend/resume. For such scenarios call phy_init_hw()
> >> from phylink_resume().
> >>
> >> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> >> ---
> >>
> >> Hi, Russel,
> >>
> >> I let phy_init_hw() to execute for all devices. I can restrict it only
> >> for PHYs that has struct phy_device::mac_managed_phy = true.
> >>
> >> Please let me know what you think.
> > 
> > I think it would be better to only do this in the path where we call
> > phy_start() - if we do it in the WoL path (where the PHY remains
> > running), then there is no phy_start() call, so phy_init_hw() could
> > result in the PHY not working after a suspend/resume event.
> 
> This will not work all the time for MACB usage on AT91 devices.
> 
> As explained here [1] the scenario where:
> - MACB is configured to handle WoL
> - the system goes to a suspend mode (named backup and self-refresh (BSR) in
>   our case) with power cut off on PHY and limited wake-up source (few pins
>   and RTC alarms)
> 
> is still valid. In this case MAC IP and MAC PHY are not powered. And in
> those cases phylink_resume() will not hit phy_start().

If the MAC is handling WoL, how does the MAC receive the packet to
wake up if the PHY has lost power?

If the PHY loses power, the MAC won't be able to receive the magic
packet, and so WoL will be non-functional, and therefore will be
completely pointless to support in such a configuration.

What am I missing?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] net: phylink: init phydev on phylink_resume()

[Claudiu.Beznea]

On 12.12.2022 16:24, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Dec 12, 2022 at 01:26:54PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 12.12.2022 14:47, Russell King (Oracle) wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Mon, Dec 12, 2022 at 01:28:44PM +0200, Claudiu Beznea wrote:
>>>> There are scenarios where PHY power is cut off on system suspend.
>>>> There are also MAC drivers which handles themselves the PHY on
>>>> suspend/resume path. For such drivers the
>>>> struct phy_device::mac_managed_phy is set to true and thus the
>>>> mdio_bus_phy_suspend()/mdio_bus_phy_resume() wouldn't do the
>>>> proper PHY suspend/resume. For such scenarios call phy_init_hw()
>>>> from phylink_resume().
>>>>
>>>> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>>>> ---
>>>>
>>>> Hi, Russel,
>>>>
>>>> I let phy_init_hw() to execute for all devices. I can restrict it only
>>>> for PHYs that has struct phy_device::mac_managed_phy = true.
>>>>
>>>> Please let me know what you think.
>>>
>>> I think it would be better to only do this in the path where we call
>>> phy_start() - if we do it in the WoL path (where the PHY remains
>>> running), then there is no phy_start() call, so phy_init_hw() could
>>> result in the PHY not working after a suspend/resume event.
>>
>> This will not work all the time for MACB usage on AT91 devices.
>>
>> As explained here [1] the scenario where:
>> - MACB is configured to handle WoL
>> - the system goes to a suspend mode (named backup and self-refresh (BSR) in
>>   our case) with power cut off on PHY and limited wake-up source (few pins
>>   and RTC alarms)
>>
>> is still valid. In this case MAC IP and MAC PHY are not powered. And in
>> those cases phylink_resume() will not hit phy_start().
> 
> If the MAC is handling WoL, how does the MAC receive the packet to
> wake up if the PHY has lost power?

Yes, this can't happen.

> 
> If the PHY loses power, the MAC won't be able to receive the magic
> packet, and so WoL will be non-functional, and therefore will be
> completely pointless to support in such a configuration.
> 
> What am I missing?

As explained in the previous version [1], we currently don't impose any
restriction like this in arch specific PM code (the only place where we can
decide if going to BSR and device_may_wakeup()). I had in mind to do it at
some point but though that user will have to re-do all the wakeup sources
reconfiguration when switching b/w suspend modes that cut or not PHY power.
To be consistent this will have to be done for all devices registered as
wakeup sources (except the few pins and RTC in case). And this may be a
pain for users.

[1]
https://lore.kernel.org/all/4375d733-ed49-869c-635f-0f0ba7304283@microchip.com/

> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 1/2] net: phylink: init phydev on phylink_resume()

[Claudiu.Beznea]

Hi, Vladimir,

On 12.12.2022 15:54, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Claudiu,
> 
> On Mon, Dec 12, 2022 at 01:28:44PM +0200, Claudiu Beznea wrote:
>> There are scenarios where PHY power is cut off on system suspend.
>> There are also MAC drivers which handles themselves the PHY on
>> suspend/resume path. For such drivers the
>> struct phy_device::mac_managed_phy is set to true and thus the
>> mdio_bus_phy_suspend()/mdio_bus_phy_resume() wouldn't do the
>> proper PHY suspend/resume. For such scenarios call phy_init_hw()
>> from phylink_resume().
>>
>> Suggested-by: Russell King (Oracle) <linux@armlinux.org.uk>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>
>> Hi, Russel,
>>
>> I let phy_init_hw() to execute for all devices. I can restrict it only
>> for PHYs that has struct phy_device::mac_managed_phy = true.
>>
>> Please let me know what you think.
>>
>> Thank you,
>> Claudiu Beznea
>>
>>
>>  drivers/net/phy/phylink.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 09cc65c0da93..6003c329638e 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -2031,6 +2031,12 @@ void phylink_resume(struct phylink *pl)
>>  {
>>       ASSERT_RTNL();
>>
>> +     if (pl->phydev) {
>> +             int ret = phy_init_hw(pl->phydev);
>> +             if (ret)
>> +                     return;
>> +     }
>> +
> 
> If the config_init() method of the driver does things such as this:
> 
>         phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
> 
> like for example the marvell10g.c driver, won't a user-configured manual
> MDI setting get overwritten after a suspend/resume cycle?

I'm not fully sure about it. I have to look further though the code.
At the same time phy_init_hw() is called also on mdio_bus_phy_resume().

Thank you,
Claudiu Beznea

> 
>>       if (test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)) {
>>               /* Wake-on-Lan enabled, MAC handling */
>>
>> --
>> 2.34.1
>>

Re: [PATCH v2 0/2] net: macb: fix connectivity after resume

[Jakub Kicinski]

On Mon, 12 Dec 2022 13:28:43 +0200 Claudiu Beznea wrote:
> This series fixes connectivity on SAMA7G5 with KSZ9131 ethernet PHY.
> Driver fix is in patch 2/2. Patch 1/2 is a prerequisite.

I'm not sure if there was a conclusion reached in the discussion 
but I'm worried this will get lost in the merge window shuffle.
So if the conclusion is reached please repost against net.