From: Mohammad Athari Bin Ismail <[email protected]>
When the PHY wakes up from suspend through WOL event, there is a need to
reconfigure the WOL if the WOL option still enabled. The main operation
is to clear the WOL event status. So that, subsequent WOL event can be
triggered properly.
This fix is needed especially for the PHY that operates in PHY_POLL mode
where there is no handler (such as interrupt handler) available to clear
the WOL event status.
Fixes: 611d779af7ca ("net: phy: fix MDIO bus PM PHY resuming")
Signed-off-by: Mohammad Athari Bin Ismail <[email protected]>
---
drivers/net/phy/phy_device.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 5d5f9a9ee768..d68703ce03b1 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -295,6 +295,7 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
{
+ struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
struct phy_device *phydev = to_phy_device(dev);
int ret;
@@ -314,6 +315,13 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
if (ret < 0)
return ret;
no_resume:
+ /* If the PHY has WOL option still enabled, reconfigure the WOL mainly
+ * to clear the WOL event status.
+ */
+ phy_ethtool_get_wol(phydev, &wol);
+ if (wol.wolopts)
+ phy_ethtool_set_wol(phydev, &wol);
+
if (phydev->attached_dev && phydev->adjust_link)
phy_start_machine(phydev);
--
2.17.1
On Thu, Jul 08, 2021 at 08:42:53AM +0800, [email protected] wrote:
> From: Mohammad Athari Bin Ismail <[email protected]>
>
> When the PHY wakes up from suspend through WOL event, there is a need to
> reconfigure the WOL if the WOL option still enabled. The main operation
> is to clear the WOL event status. So that, subsequent WOL event can be
> triggered properly.
>
> This fix is needed especially for the PHY that operates in PHY_POLL mode
> where there is no handler (such as interrupt handler) available to clear
> the WOL event status.
I still think this architecture is wrong.
The interrupt pin is wired to the PMIC. Can the PMIC be modelled as an
interrupt controller? That would allow the interrupt to be handled as
normal, and would mean you don't need polling, and you don't need this
hack.
Andrew
On 7/7/2021 6:23 PM, Andrew Lunn wrote:
> On Thu, Jul 08, 2021 at 08:42:53AM +0800, [email protected] wrote:
>> From: Mohammad Athari Bin Ismail <[email protected]>
>>
>> When the PHY wakes up from suspend through WOL event, there is a need to
>> reconfigure the WOL if the WOL option still enabled. The main operation
>> is to clear the WOL event status. So that, subsequent WOL event can be
>> triggered properly.
>>
>> This fix is needed especially for the PHY that operates in PHY_POLL mode
>> where there is no handler (such as interrupt handler) available to clear
>> the WOL event status.
>
> I still think this architecture is wrong.
>
> The interrupt pin is wired to the PMIC. Can the PMIC be modelled as an
> interrupt controller? That would allow the interrupt to be handled as
> normal, and would mean you don't need polling, and you don't need this
> hack.
I have to agree with Andrew here, and if the answer is that you cannot
model this PMIC as an interrupt controller, cannot the config_init()
callback of the driver acknowledge then disable the interrupts as it
normally would if you were cold booting the system? This would also
allow you to properly account for the PHY having woken-up the system.
--
Florian
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Thursday, July 8, 2021 9:24 AM
> To: Ismail, Mohammad Athari <[email protected]>
> Cc: Heiner Kallweit <[email protected]>; David S . Miller
> <[email protected]>; Russell King <[email protected]>; Jakub Kicinski
> <[email protected]>; Florian Fainelli <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
> option still enabled
>
> On Thu, Jul 08, 2021 at 08:42:53AM +0800, [email protected]
> wrote:
> > From: Mohammad Athari Bin Ismail <[email protected]>
> >
> > When the PHY wakes up from suspend through WOL event, there is a need
> > to reconfigure the WOL if the WOL option still enabled. The main
> > operation is to clear the WOL event status. So that, subsequent WOL
> > event can be triggered properly.
> >
> > This fix is needed especially for the PHY that operates in PHY_POLL
> > mode where there is no handler (such as interrupt handler) available
> > to clear the WOL event status.
>
> I still think this architecture is wrong.
>
> The interrupt pin is wired to the PMIC. Can the PMIC be modelled as an interrupt
> controller? That would allow the interrupt to be handled as normal, and would
> mean you don't need polling, and you don't need this hack.
Hi Andrew,
In our platform, the PHY interrupt pin is not connected to Host CPU. So, the CPU couldn`t service the PHY interrupt.? The PHY interrupt pin is connected to a power management controller (PMC) as a HW wake up signal. The PMC itself couldn't act as interrupt controller to service the PHY interrupt.
During WOL event, the WOL signal is sent to PMC through the PHY interrupt pin to wake up the PMC. Then, the PMC will wake up the Host CPU and the whole system.
Therefore, for our platform, PHY_POLL mode is chosen in PHY driver.
-Athari-
>
> Andrew
> -----Original Message-----
> From: Florian Fainelli <[email protected]>
> Sent: Thursday, July 8, 2021 10:49 AM
> To: Andrew Lunn <[email protected]>; Ismail, Mohammad Athari
> <[email protected]>
> Cc: Heiner Kallweit <[email protected]>; David S . Miller
> <[email protected]>; Russell King <[email protected]>; Jakub Kicinski
> <[email protected]>; [email protected]; [email protected]
> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
> option still enabled
>
>
>
> On 7/7/2021 6:23 PM, Andrew Lunn wrote:
> > On Thu, Jul 08, 2021 at 08:42:53AM +0800,
> [email protected] wrote:
> >> From: Mohammad Athari Bin Ismail <[email protected]>
> >>
> >> When the PHY wakes up from suspend through WOL event, there is a need
> >> to reconfigure the WOL if the WOL option still enabled. The main
> >> operation is to clear the WOL event status. So that, subsequent WOL
> >> event can be triggered properly.
> >>
> >> This fix is needed especially for the PHY that operates in PHY_POLL
> >> mode where there is no handler (such as interrupt handler) available
> >> to clear the WOL event status.
> >
> > I still think this architecture is wrong.
> >
> > The interrupt pin is wired to the PMIC. Can the PMIC be modelled as an
> > interrupt controller? That would allow the interrupt to be handled as
> > normal, and would mean you don't need polling, and you don't need this
> > hack.
>
> I have to agree with Andrew here, and if the answer is that you cannot model
> this PMIC as an interrupt controller, cannot the config_init() callback of the
> driver acknowledge then disable the interrupts as it normally would if you were
> cold booting the system? This would also allow you to properly account for the
> PHY having woken-up the system.
Hi Florian,
Thank you for the suggestion.
If I understand correctly, you are suggesting to acknowledge and clear the WOL status in config_init() callback function. Am I correct?
If yes, I did try to add a code to clear WOL status in marvell_config_init() function (we are using Marvell Alaska 88E1512). But, I found that, if the platform wake up from S3(mem) or S4(disk), the config_init() callback function is not called. As the result, WOL status not able to be cleared in config_init().
Please advice if you any suggestion.
-Athari-
> --
> Florian
> Hi Andrew,
>
> In our platform, the PHY interrupt pin is not connected to Host CPU. So, the CPU couldn`t service the PHY interrupt.? The PHY interrupt pin is connected to a power management controller (PMC) as a HW wake up signal. The PMC itself couldn't act as interrupt controller to service the PHY interrupt.
>
> During WOL event, the WOL signal is sent to PMC through the PHY interrupt pin to wake up the PMC. Then, the PMC will wake up the Host CPU and the whole system.
How is the PMC connected to the host? LPC? At wake up can you ask it
why it woke you up? What event it was, power restored, power button
press, or WOL? Can the PMC generate interrupts over the LPC? What PMC
is it? Is there a datasheet for it?
Getting your architecture correct will also solve your S3/S4 problems.
Andrew
On 7/8/21 3:10 AM, Ismail, Mohammad Athari wrote:
>
>
>> -----Original Message-----
>> From: Florian Fainelli <[email protected]>
>> Sent: Thursday, July 8, 2021 10:49 AM
>> To: Andrew Lunn <[email protected]>; Ismail, Mohammad Athari
>> <[email protected]>
>> Cc: Heiner Kallweit <[email protected]>; David S . Miller
>> <[email protected]>; Russell King <[email protected]>; Jakub Kicinski
>> <[email protected]>; [email protected]; [email protected]
>> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
>> option still enabled
>>
>>
>>
>> On 7/7/2021 6:23 PM, Andrew Lunn wrote:
>>> On Thu, Jul 08, 2021 at 08:42:53AM +0800,
>> [email protected] wrote:
>>>> From: Mohammad Athari Bin Ismail <[email protected]>
>>>>
>>>> When the PHY wakes up from suspend through WOL event, there is a need
>>>> to reconfigure the WOL if the WOL option still enabled. The main
>>>> operation is to clear the WOL event status. So that, subsequent WOL
>>>> event can be triggered properly.
>>>>
>>>> This fix is needed especially for the PHY that operates in PHY_POLL
>>>> mode where there is no handler (such as interrupt handler) available
>>>> to clear the WOL event status.
>>>
>>> I still think this architecture is wrong.
>>>
>>> The interrupt pin is wired to the PMIC. Can the PMIC be modelled as an
>>> interrupt controller? That would allow the interrupt to be handled as
>>> normal, and would mean you don't need polling, and you don't need this
>>> hack.
>>
>> I have to agree with Andrew here, and if the answer is that you cannot model
>> this PMIC as an interrupt controller, cannot the config_init() callback of the
>> driver acknowledge then disable the interrupts as it normally would if you were
>> cold booting the system? This would also allow you to properly account for the
>> PHY having woken-up the system.
>
> Hi Florian,
>
> Thank you for the suggestion.
> If I understand correctly, you are suggesting to acknowledge and clear the WOL status in config_init() callback function. Am I correct?
> If yes, I did try to add a code to clear WOL status in marvell_config_init() function (we are using Marvell Alaska 88E1512). But, I found that, if the platform wake up from S3(mem) or S4(disk), the config_init() callback function is not called. As the result, WOL status not able to be cleared in config_init().
>
> Please advice if you any suggestion.
This is presumably that you are seeing with stmmac along with phylink?
During S3 resume you should be going back to the kernel provided
re-entry point and resume where we left (warm boot) so
mdio_bus_phy_resume() should call phy_init_hw() which calls
config_init(), have you traced if that is somehow not happening?
During S4 resume (disk), I suppose that you have to involve the boot
loader to restore the DRAM image from the storage disk, and so that does
effectively look like a quasi cold boot from the kernel? If so, that
should still lead to config_init() being called when the PHY is
attached, no?
--
Florian
> -----Original Message-----
> From: Andrew Lunn <[email protected]>
> Sent: Thursday, July 8, 2021 9:11 PM
> To: Ismail, Mohammad Athari <[email protected]>
> Cc: Heiner Kallweit <[email protected]>; David S . Miller
> <[email protected]>; Russell King <[email protected]>; Jakub
> Kicinski <[email protected]>; Florian Fainelli <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
> option still enabled
>
> > Hi Andrew,
> >
> > In our platform, the PHY interrupt pin is not connected to Host CPU. So, the
> CPU couldn`t service the PHY interrupt.? The PHY interrupt pin is connected to
> a power management controller (PMC) as a HW wake up signal. The PMC
> itself couldn't act as interrupt controller to service the PHY interrupt.
> >
> > During WOL event, the WOL signal is sent to PMC through the PHY interrupt
> pin to wake up the PMC. Then, the PMC will wake up the Host CPU and the
> whole system.
>
> How is the PMC connected to the host? LPC? At wake up can you ask it why it
> woke you up? What event it was, power restored, power button press, or
> WOL? Can the PMC generate interrupts over the LPC? What PMC is it? Is
> there a datasheet for it?
>
> Getting your architecture correct will also solve your S3/S4 problems.
Hi Andrew,
I'll try to get more info from our architecture design team.
-Athari-
>
> Andrew
> -----Original Message-----
> From: Florian Fainelli <[email protected]>
> Sent: Friday, July 9, 2021 12:42 AM
> To: Ismail, Mohammad Athari <[email protected]>;
> Andrew Lunn <[email protected]>
> Cc: Heiner Kallweit <[email protected]>; David S . Miller
> <[email protected]>; Russell King <[email protected]>; Jakub
> Kicinski <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
> option still enabled
>
> On 7/8/21 3:10 AM, Ismail, Mohammad Athari wrote:
> >
> >
> >> -----Original Message-----
> >> From: Florian Fainelli <[email protected]>
> >> Sent: Thursday, July 8, 2021 10:49 AM
> >> To: Andrew Lunn <[email protected]>; Ismail, Mohammad Athari
> >> <[email protected]>
> >> Cc: Heiner Kallweit <[email protected]>; David S . Miller
> >> <[email protected]>; Russell King <[email protected]>; Jakub
> >> Kicinski <[email protected]>; [email protected];
> >> [email protected]
> >> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if
> >> WOL option still enabled
> >>
> >>
> >>
> >> On 7/7/2021 6:23 PM, Andrew Lunn wrote:
> >>> On Thu, Jul 08, 2021 at 08:42:53AM +0800,
> >> [email protected] wrote:
> >>>> From: Mohammad Athari Bin Ismail
> <[email protected]>
> >>>>
> >>>> When the PHY wakes up from suspend through WOL event, there is a
> >>>> need to reconfigure the WOL if the WOL option still enabled. The
> >>>> main operation is to clear the WOL event status. So that,
> >>>> subsequent WOL event can be triggered properly.
> >>>>
> >>>> This fix is needed especially for the PHY that operates in PHY_POLL
> >>>> mode where there is no handler (such as interrupt handler)
> >>>> available to clear the WOL event status.
> >>>
> >>> I still think this architecture is wrong.
> >>>
> >>> The interrupt pin is wired to the PMIC. Can the PMIC be modelled as
> >>> an interrupt controller? That would allow the interrupt to be
> >>> handled as normal, and would mean you don't need polling, and you
> >>> don't need this hack.
> >>
> >> I have to agree with Andrew here, and if the answer is that you
> >> cannot model this PMIC as an interrupt controller, cannot the
> >> config_init() callback of the driver acknowledge then disable the
> >> interrupts as it normally would if you were cold booting the system?
> >> This would also allow you to properly account for the PHY having woken-
> up the system.
> >
> > Hi Florian,
> >
> > Thank you for the suggestion.
> > If I understand correctly, you are suggesting to acknowledge and clear the
> WOL status in config_init() callback function. Am I correct?
> > If yes, I did try to add a code to clear WOL status in marvell_config_init()
> function (we are using Marvell Alaska 88E1512). But, I found that, if the
> platform wake up from S3(mem) or S4(disk), the config_init() callback
> function is not called. As the result, WOL status not able to be cleared in
> config_init().
> >
> > Please advice if you any suggestion.
>
> This is presumably that you are seeing with stmmac along with phylink?
>
> During S3 resume you should be going back to the kernel provided re-entry
> point and resume where we left (warm boot) so
> mdio_bus_phy_resume() should call phy_init_hw() which calls config_init(),
> have you traced if that is somehow not happening?
>
> During S4 resume (disk), I suppose that you have to involve the boot loader
> to restore the DRAM image from the storage disk, and so that does
> effectively look like a quasi cold boot from the kernel? If so, that should still
> lead to config_init() being called when the PHY is attached, no?
Hi Florian,
This what I understand from the code flow.
With WOL enabled through ethtool, when the system is put into S3 or S4,
this flag netdev->wol_enabled is set true and cause mdio_bus_phy_may_suspend()
to return false. So, the phydev->suspended_by_mdio_bus remain as 0 when
exiting from mdio_bus_phy_suspend().
During wake up from S3 or S4, as phydev->suspended_by_mdio_bus remain as 0/false
when mdio_bus_phy_resume() is called, it will jump to no_resume skipping
phy_init_hw() as well as phy_resume().
-Athari-
> --
> Florian
On 7/8/21 4:20 PM, Ismail, Mohammad Athari wrote:
>
>
>> -----Original Message-----
>> From: Florian Fainelli <[email protected]>
>> Sent: Friday, July 9, 2021 12:42 AM
>> To: Ismail, Mohammad Athari <[email protected]>;
>> Andrew Lunn <[email protected]>
>> Cc: Heiner Kallweit <[email protected]>; David S . Miller
>> <[email protected]>; Russell King <[email protected]>; Jakub
>> Kicinski <[email protected]>; [email protected]; linux-
>> [email protected]
>> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if WOL
>> option still enabled
>>
>> On 7/8/21 3:10 AM, Ismail, Mohammad Athari wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Florian Fainelli <[email protected]>
>>>> Sent: Thursday, July 8, 2021 10:49 AM
>>>> To: Andrew Lunn <[email protected]>; Ismail, Mohammad Athari
>>>> <[email protected]>
>>>> Cc: Heiner Kallweit <[email protected]>; David S . Miller
>>>> <[email protected]>; Russell King <[email protected]>; Jakub
>>>> Kicinski <[email protected]>; [email protected];
>>>> [email protected]
>>>> Subject: Re: [PATCH net] net: phy: reconfigure PHY WOL in resume if
>>>> WOL option still enabled
>>>>
>>>>
>>>>
>>>> On 7/7/2021 6:23 PM, Andrew Lunn wrote:
>>>>> On Thu, Jul 08, 2021 at 08:42:53AM +0800,
>>>> [email protected] wrote:
>>>>>> From: Mohammad Athari Bin Ismail
>> <[email protected]>
>>>>>>
>>>>>> When the PHY wakes up from suspend through WOL event, there is a
>>>>>> need to reconfigure the WOL if the WOL option still enabled. The
>>>>>> main operation is to clear the WOL event status. So that,
>>>>>> subsequent WOL event can be triggered properly.
>>>>>>
>>>>>> This fix is needed especially for the PHY that operates in PHY_POLL
>>>>>> mode where there is no handler (such as interrupt handler)
>>>>>> available to clear the WOL event status.
>>>>>
>>>>> I still think this architecture is wrong.
>>>>>
>>>>> The interrupt pin is wired to the PMIC. Can the PMIC be modelled as
>>>>> an interrupt controller? That would allow the interrupt to be
>>>>> handled as normal, and would mean you don't need polling, and you
>>>>> don't need this hack.
>>>>
>>>> I have to agree with Andrew here, and if the answer is that you
>>>> cannot model this PMIC as an interrupt controller, cannot the
>>>> config_init() callback of the driver acknowledge then disable the
>>>> interrupts as it normally would if you were cold booting the system?
>>>> This would also allow you to properly account for the PHY having woken-
>> up the system.
>>>
>>> Hi Florian,
>>>
>>> Thank you for the suggestion.
>>> If I understand correctly, you are suggesting to acknowledge and clear the
>> WOL status in config_init() callback function. Am I correct?
>>> If yes, I did try to add a code to clear WOL status in marvell_config_init()
>> function (we are using Marvell Alaska 88E1512). But, I found that, if the
>> platform wake up from S3(mem) or S4(disk), the config_init() callback
>> function is not called. As the result, WOL status not able to be cleared in
>> config_init().
>>>
>>> Please advice if you any suggestion.
>>
>> This is presumably that you are seeing with stmmac along with phylink?
>>
>> During S3 resume you should be going back to the kernel provided re-entry
>> point and resume where we left (warm boot) so
>> mdio_bus_phy_resume() should call phy_init_hw() which calls config_init(),
>> have you traced if that is somehow not happening?
>>
>> During S4 resume (disk), I suppose that you have to involve the boot loader
>> to restore the DRAM image from the storage disk, and so that does
>> effectively look like a quasi cold boot from the kernel? If so, that should still
>> lead to config_init() being called when the PHY is attached, no?
>
> Hi Florian,
>
> This what I understand from the code flow.
>
> With WOL enabled through ethtool, when the system is put into S3 or S4,
> this flag netdev->wol_enabled is set true and cause mdio_bus_phy_may_suspend()
> to return false. So, the phydev->suspended_by_mdio_bus remain as 0 when
> exiting from mdio_bus_phy_suspend().
>
> During wake up from S3 or S4, as phydev->suspended_by_mdio_bus remain as 0/false
> when mdio_bus_phy_resume() is called, it will jump to no_resume skipping
> phy_init_hw() as well as phy_resume().
Ah yes you are right, we just skip resume in that case. OK let me think
about it some more.
--
Florian
> Ah yes you are right, we just skip resume in that case. OK let me think
> about it some more.
The point here is, it is an interrupt, from the perspective of the PHY
hardware and its driver. But the interrupt handler is never being
called because the interrupt output from the chip is not causing an
actual interrupt. Fix that, and your problem goes away. Or you need to
add a whole new mechanism that you are using the interrupt hardware in
the PHY some something else than an actual interrupt.
Andrew
On Thu, Jul 08, 2021 at 09:41:45AM -0700, Florian Fainelli wrote:
> During S4 resume (disk), I suppose that you have to involve the boot
> loader to restore the DRAM image from the storage disk, and so that does
> effectively look like a quasi cold boot from the kernel? If so, that
> should still lead to config_init() being called when the PHY is
> attached, no?
Don't forget that when resuming from S4, we effectively boot the kernel
normally, then check whether we are resuming, before we then start
loading the resume image, suspend the current kernel, shuffle the pages
around, and resume the original kernel.
What that means is that a PHY will see the effects of a normal kernel
boot before we resume the original kernel. If a PHY driver (e.g.)
disables features such as WOL and we do not restore these settings on
resume, then those settings will be reset.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!