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

[PATCH 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

Claudiu Beznea (2):
  net: phylink: add helper to initialize phylink's phydev
  net: macb: fix connectivity after resume

 drivers/net/ethernet/cadence/macb_main.c |  1 +
 drivers/net/phy/phylink.c                | 10 ++++++++++
 include/linux/phylink.h                  |  1 +
 3 files changed, 12 insertions(+)

-- 
2.34.1

[PATCH 1/2] net: phylink: add helper to initialize phylink's phydev

[Claudiu Beznea]

Add helper to initialize phydev embedded in a phylink object.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/net/phy/phylink.c | 10 ++++++++++
 include/linux/phylink.h   |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 09cc65c0da93..1e2478b8cd5f 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2541,6 +2541,16 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_eee *eee)
 }
 EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee);
 
+/**
+ * phylink_init_phydev() - initialize phydev associated to phylink
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ */
+int phylink_init_phydev(struct phylink *pl)
+{
+	return phy_init_hw(pl->phydev);
+}
+EXPORT_SYMBOL_GPL(phylink_init_phydev);
+
 /* This emulates MII registers for a fixed-mode phy operating as per the
  * passed in state. "aneg" defines if we report negotiation is possible.
  *
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..6a969aa75c7f 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -609,6 +609,7 @@ int phylink_ethtool_set_eee(struct phylink *, struct ethtool_eee *);
 int phylink_mii_ioctl(struct phylink *, struct ifreq *, int);
 int phylink_speed_down(struct phylink *pl, bool sync);
 int phylink_speed_up(struct phylink *pl);
+int phylink_init_phydev(struct phylink *pl);
 
 #define phylink_zero(bm) \
 	bitmap_zero(bm, __ETHTOOL_LINK_MODE_MASK_NBITS)
-- 
2.34.1

Re: [PATCH 1/2] net: phylink: add helper to initialize phylink's phydev

[Russell King (Oracle)]

On Mon, Dec 05, 2022 at 05:33:27PM +0200, Claudiu Beznea wrote:
> Add helper to initialize phydev embedded in a phylink object.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/net/phy/phylink.c | 10 ++++++++++
>  include/linux/phylink.h   |  1 +
>  2 files changed, 11 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 09cc65c0da93..1e2478b8cd5f 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -2541,6 +2541,16 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_eee *eee)
>  }
>  EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee);
>  
> +/**
> + * phylink_init_phydev() - initialize phydev associated to phylink
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + */
> +int phylink_init_phydev(struct phylink *pl)
> +{
> +	return phy_init_hw(pl->phydev);
> +}
> +EXPORT_SYMBOL_GPL(phylink_init_phydev);

I'd guess this is something that many MAC drivers will need to do when
resuming if the PHY has lost power.

Maybe a better solution would be to integrate it into phylink_resume(),
when we know that the PHY has lost power - maybe the MAC driver can
tell phylink that detail, and be updated to use phylink_suspend() and
phylink_resume() ?

macb_set_wol() sets bp->wol's MACB_WOL_ENABLED flag depending on
whether WAKE_MAGIC is set in wolopts. No other wolopts are supported.
Generic code sets netdev->wol_enabled if set_wol() was successful and
wolopts is nonzero, indicating that WoL is enabled, and thus
phylink_stop() won't be called if WoL is enabled (similar to what
macb_suspend() is doing.)

Given that the macb MAC seems to be implementing WoL, it should call
phylink_suspend() with mac_wol=true.

Please can you look into this, thanks.

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

Re: [PATCH 1/2] net: phylink: add helper to initialize phylink's phydev

[Claudiu.Beznea]

On 05.12.2022 17:57, Russell King (Oracle) wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Mon, Dec 05, 2022 at 05:33:27PM +0200, Claudiu Beznea wrote:
>> Add helper to initialize phydev embedded in a phylink object.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/net/phy/phylink.c | 10 ++++++++++
>>  include/linux/phylink.h   |  1 +
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
>> index 09cc65c0da93..1e2478b8cd5f 100644
>> --- a/drivers/net/phy/phylink.c
>> +++ b/drivers/net/phy/phylink.c
>> @@ -2541,6 +2541,16 @@ int phylink_ethtool_set_eee(struct phylink *pl, struct ethtool_eee *eee)
>>  }
>>  EXPORT_SYMBOL_GPL(phylink_ethtool_set_eee);
>>
>> +/**
>> + * phylink_init_phydev() - initialize phydev associated to phylink
>> + * @pl: a pointer to a &struct phylink returned from phylink_create()
>> + */
>> +int phylink_init_phydev(struct phylink *pl)
>> +{
>> +     return phy_init_hw(pl->phydev);
>> +}
>> +EXPORT_SYMBOL_GPL(phylink_init_phydev);
> 
> I'd guess this is something that many MAC drivers will need to do when
> resuming if the PHY has lost power.
> 
> Maybe a better solution would be to integrate it into phylink_resume(),

OK, I'll look into this.

> when we know that the PHY has lost power - maybe the MAC driver can
> tell phylink that detail, and be updated to use phylink_suspend() and
> phylink_resume() ?

Cutting the power is arch specific and it may depends on the PM mode that
system will go (at least for AT91 architecture). At the moment there is no
way for drivers to know about architecture specific power management mode.
There was an attempt to implement this (few years ago, see [1]) but it
wasn't accepted (from what I can see in the source code at the moment).

So, in case we choose to move it to phylink_resume() we will have to
reinitialize the PHY unconditionally (see below). Would this be OK?

[1] https://lore.kernel.org/lkml/20170623010837.11199-1-f.fainelli@gmail.com/

> 
> macb_set_wol() sets bp->wol's MACB_WOL_ENABLED flag depending on
> whether WAKE_MAGIC is set in wolopts. No other wolopts are supported.
> Generic code sets netdev->wol_enabled if set_wol() was successful and
> wolopts is nonzero, indicating that WoL is enabled, and thus
> phylink_stop() won't be called if WoL is enabled (similar to what
> macb_suspend() is doing.)
> 
> Given that the macb MAC seems to be implementing WoL, it should call
> phylink_suspend() with mac_wol=true.

In AT91 BSR (backup and self-refresh) could coexist with other PM modes
(that doesn't cut power). And they are mapped to Linux standard PM specific
modes. So, whenever one would execute:
echo mem > /sys/power/state # or
echo standby > /sys/power/state

BSR or other PM mode could be executed. MACB driver could know only if
system goes to mem Linux PM mode or standby Linux PM mode. But BSR could be
mapped either to mem or standby. We can't decide from driver if we go to BSR.

In BSR there are minimum wakeup sources (some reserved pins and RTC alarm).
There is no way to resume from WoL. Arch specific PM code could decide to
not go to BSR if MACB may wakeup thus on MACB driver we could decide to run
phylink_suspend()/phylink_resume() based on not having the MACB driver
configured as a wakeup source. But it will not mean in all cases that we go
to BSR. And imposing on arch specific code to not go to BSR if MACB may
wakeup may be a pain for users (in case they switch from one PM mode to
another as they will need to reconfigure the wakeup sources every time).

Hope I was clear.

Thank you for your review,
Claudiu Beznea

> 
> Please can you look into this, thanks.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/2] net: phylink: add helper to initialize phylink's phydev

[Russell King (Oracle)]

Hi,

On Wed, Dec 07, 2022 at 10:49:39AM +0000, Claudiu.Beznea@microchip.com wrote:
> On 05.12.2022 17:57, Russell King (Oracle) wrote:
> > when we know that the PHY has lost power - maybe the MAC driver can
> > tell phylink that detail, and be updated to use phylink_suspend() and
> > phylink_resume() ?
> 
> Cutting the power is arch specific and it may depends on the PM mode that
> system will go (at least for AT91 architecture). At the moment there is no
> way for drivers to know about architecture specific power management mode.
> There was an attempt to implement this (few years ago, see [1]) but it
> wasn't accepted (from what I can see in the source code at the moment).
> 
> So, in case we choose to move it to phylink_resume() we will have to
> reinitialize the PHY unconditionally (see below). Would this be OK?

I guess it would - off the top of my head, I can't think why a call to
phy_init_hw() would cause an issue, but maybe my fellow phylib
maintainers have a different opinion.

Thanks.

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

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

[Claudiu Beznea]

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
phylink_init_phydev() has been called on resume path before
phylink_start(). Up to
commit bf0ad1893442 ("net: macb: Specify PHY PM management done by MAC")
this has been handled by mdio_bus_phy_resume().

This has been tested on SAMA7G5-EK (with KSZ9131 and KSZ8081 PHYs), on
SAMA5D2 (with KSZ8081 PHY), on SAM9X60 (with KSZ8081 PHY).

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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 95667b979fab..8baa53706721 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5238,6 +5238,7 @@ static int __maybe_unused macb_resume(struct device *dev)
 	if (!device_may_wakeup(&bp->dev->dev))
 		phy_init(bp->sgmii_phy);
 
+	phylink_init_phydev(bp->phylink);
 	phylink_start(bp->phylink);
 	rtnl_unlock();
 
-- 
2.34.1