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@xxxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxx/

> 
> 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!