[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <50f96c9d-5ca1-471b-bc6d-a901a8d5b37c@gmail.com>
Date: Thu, 2 May 2024 09:17:51 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
Andrew Lunn <andrew@...n.ch>
Cc: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>, netdev@...r.kernel.org,
lxu@...linear.com, hkallweit1@...il.com, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
linux-kernel@...r.kernel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next] net: phy: add wol config options in phy device
On 5/2/24 08:59, Russell King (Oracle) wrote:
> On Thu, May 02, 2024 at 04:51:42PM +0200, Andrew Lunn wrote:
>> On Tue, Apr 30, 2024 at 10:36:35AM +0530, Raju Lakkaraju wrote:
>>> Introduce a new member named 'wolopts' to the 'phy_device' structure to
>>> store the user-specified Wake-on-LAN (WOL) settings. Update this member
>>> within the phy driver's 'set_wol()' function whenever the WOL configuration
>>> is modified by the user.
>>>
>>> Currently, when the system resumes from sleep, the 'phy_init_hw()' function
>>> resets the PHY's configuration and interrupts, which leads to problems upon
>>> subsequent WOL attempts. By retaining the desired WOL settings in 'wolopts',
>>> we can ensure that the PHY's WOL configuration is correctly reapplied
>>> through 'phy_ethtool_set_wol()' before a system suspend, thereby resolving
>>> the issue
>>
>> Sorry it took a white to review this.
>>
>>>
>>> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
>>> ---
>>> drivers/net/phy/mxl-gpy.c | 5 +++++
>>> drivers/net/phy/phy_device.c | 5 +++++
>>> include/linux/phy.h | 2 ++
>>> 3 files changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/mxl-gpy.c b/drivers/net/phy/mxl-gpy.c
>>> index b2d36a3a96f1..6edb29a1d77e 100644
>>> --- a/drivers/net/phy/mxl-gpy.c
>>> +++ b/drivers/net/phy/mxl-gpy.c
>>> @@ -680,6 +680,7 @@ static int gpy_set_wol(struct phy_device *phydev,
>>> struct net_device *attach_dev = phydev->attached_dev;
>>> int ret;
>>>
>>> + phydev->wolopts = 0;
>>
>> Is this specific to mlx-gpy?
>>
>> You should be trying to solve the problem for all PHYs which support
>> WoL. So i expect the core to be doing most of the work. In fact, i
>> don't think there is any need for driver specific code.
>
> It would be good to hear exactly why its necessary for phylib to track
> this state, and why the PHY isn't retaining it.
Agreed. I contemplated doing something similar while adding support for
Wake-on-LAN to the Broadcom PHY driver, but eventually convinced myself
this was not necessary as the hardware was capable of retaining the
wake-up event, and that the PHY driver *must* be able to charge the PHY
device for wake-up purposes, even on a cold boot.
>
> I suspect this may have something to do with resets - the PHY being
> hardware reset when coming out of resume (resulting in all state
> being lost.) What's resetting it would also be good to track down
> (as in hardware, firmware, or the kernel.)
>
Since it is possible to override the soft_reset callback called by
phy_init_hw(), I would be inclined to make this a driver specific
solution by doing something like:
mxl_gphy_soft_reset(...)
/* Save WoL status */
priv->wol_enabled = ...
return genphy_soft_reset()
--
Florian
Powered by blists - more mailing lists