[PATCH] net: phy: Only resume phy if it is suspended

[PATCH] net: phy: Only resume phy if it is suspended

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

Resuming the phy can take quite a bit of time. Lets only resume the
phy if it is suspended.

Signed-off-by: Justin Chen <justin.chen@broadcom.com>
---
 drivers/net/phy/phy.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3376e58e2b88..7fbb21922d64 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1549,7 +1549,8 @@ void phy_start(struct phy_device *phydev)
 		sfp_upstream_start(phydev->sfp_bus);
 
 	/* if phy was suspended, bring the physical link up again */
-	__phy_resume(phydev);
+	if (phydev->suspended)
+		__phy_resume(phydev);
 
 	phydev->state = PHY_UP;
 
-- 
2.34.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH] net: phy: Only resume phy if it is suspended

[Andrew Lunn]

On Tue, Dec 05, 2023 at 03:42:29PM -0800, Justin Chen wrote:
> Resuming the phy can take quite a bit of time. Lets only resume the
> phy if it is suspended.

Humm...

https://lore.kernel.org/netdev/6d45f4da-c45e-4d35-869f-85dd4ec37b31@lunn.ch/T/

If Broadcom PHYs are slow to resume, maybe you should solve this in
the broadcom resume handler, read the status from the hardware and
only do the resume if the hardware is suspended.

     Andrew

Re: [PATCH] net: phy: Only resume phy if it is suspended

[Justin Chen]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]



On 12/5/23 4:03 PM, Andrew Lunn wrote:
> On Tue, Dec 05, 2023 at 03:42:29PM -0800, Justin Chen wrote:
>> Resuming the phy can take quite a bit of time. Lets only resume the
>> phy if it is suspended.
> 
> Humm...
> 
> https://lore.kernel.org/netdev/6d45f4da-c45e-4d35-869f-85dd4ec37b31@lunn.ch/T/
> 
> If Broadcom PHYs are slow to resume, maybe you should solve this in
> the broadcom resume handler, read the status from the hardware and
> only do the resume if the hardware is suspended.
> 
>       Andrew

Right... Guess this won't work. It is odd that during resume we call 
__phy_resume twice. Once from phy_resume() and another at phy_start(). 
Let me rethink this. Thanks for the feedback.

Justin

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4206 bytes --]

Re: [PATCH] net: phy: Only resume phy if it is suspended

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

On 12/5/23 16:10, Justin Chen wrote:
> 
> 
> On 12/5/23 4:03 PM, Andrew Lunn wrote:
>> On Tue, Dec 05, 2023 at 03:42:29PM -0800, Justin Chen wrote:
>>> Resuming the phy can take quite a bit of time. Lets only resume the
>>> phy if it is suspended.
>>
>> Humm...
>>
>> https://lore.kernel.org/netdev/6d45f4da-c45e-4d35-869f-85dd4ec37b31@lunn.ch/T/
>>
>> If Broadcom PHYs are slow to resume, maybe you should solve this in
>> the broadcom resume handler, read the status from the hardware and
>> only do the resume if the hardware is suspended.
>>
>>       Andrew
> 
> Right... Guess this won't work. It is odd that during resume we call 
> __phy_resume twice. Once from phy_resume() and another at phy_start(). 
> Let me rethink this. Thanks for the feedback.

This might be something for us to figure out on the driver side, I think 
historically I have always followed the pattern of doing:

phy_suspend()
phy_stop()

and

phy_resume()
phy_start()

because it used to be necessary to do that way back when...
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] net: phy: Only resume phy if it is suspended

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]

Hi Andrew,

On 12/5/23 16:12, Florian Fainelli wrote:
> On 12/5/23 16:10, Justin Chen wrote:
>>
>>
>> On 12/5/23 4:03 PM, Andrew Lunn wrote:
>>> On Tue, Dec 05, 2023 at 03:42:29PM -0800, Justin Chen wrote:
>>>> Resuming the phy can take quite a bit of time. Lets only resume the
>>>> phy if it is suspended.
>>>
>>> Humm...
>>>
>>> https://lore.kernel.org/netdev/6d45f4da-c45e-4d35-869f-85dd4ec37b31@lunn.ch/T/
>>>
>>> If Broadcom PHYs are slow to resume, maybe you should solve this in
>>> the broadcom resume handler, read the status from the hardware and
>>> only do the resume if the hardware is suspended.
>>>
>>>       Andrew
>>
>> Right... Guess this won't work. It is odd that during resume we call 
>> __phy_resume twice. Once from phy_resume() and another at phy_start(). 
>> Let me rethink this. Thanks for the feedback.
> 
> This might be something for us to figure out on the driver side, I think 
> historically I have always followed the pattern of doing:
> 
> phy_suspend()
> phy_stop()
> 
> and
> 
> phy_resume()
> phy_start()
> 
> because it used to be necessary to do that way back when...

So we discussed with Justin and Doug about this yesterday and the main 
reason for the phy_resume() ... MAC initialization ... phy_start() 
pattern has to do with external RGMII PHYs and the clocking dependency 
between the MAC and the PHY on the RX path. And also a tiny bit of cargo 
culting, but shhh.

When the external RGMII PHYs are suspended they will stop providing a 
RXC back to the Ethernet MAC and our Ethernet MAC like a lot of designs 
out there require the RXC in order to be functional and complete its 
reset procedure correctly (you would think there would be a way to mux 
in a different clock, but that does not appear to be the case). If we 
reset the UniMAC block without a RXC we will typically see duplicate 
packets being received, or absurdly long round trip times as soon as we 
try to use the RX path.

Doug lifted that requirement in GENET with:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=88f6c8bf1aaed5039923fb4c701cab4d42176275

by delaying the MAC reset until we have a link UP confirmation. Since 
this is the same UniMAC design in the bcmasp driver we should be able to 
apply the same strategy and remove the initial phy_resume().

There are also other opportunities for avoiding link disruption upon 
suspend/resume when Wake-on-LAN is enabled and avoid a re-negotiation of 
the link, though that's for another set of changes.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]

Re: [PATCH] net: phy: Only resume phy if it is suspended

[Russell King (Oracle)]

On Thu, Dec 07, 2023 at 09:56:01AM -0800, Florian Fainelli wrote:
> Hi Andrew,
> 
> So we discussed with Justin and Doug about this yesterday and the main
> reason for the phy_resume() ... MAC initialization ... phy_start() pattern
> has to do with external RGMII PHYs and the clocking dependency between the
> MAC and the PHY on the RX path. And also a tiny bit of cargo culting, but
> shhh.
> 
> When the external RGMII PHYs are suspended they will stop providing a RXC
> back to the Ethernet MAC and our Ethernet MAC like a lot of designs out
> there require the RXC in order to be functional and complete its reset
> procedure correctly (you would think there would be a way to mux in a
> different clock, but that does not appear to be the case). If we reset the
> UniMAC block without a RXC we will typically see duplicate packets being
> received, or absurdly long round trip times as soon as we try to use the RX
> path.

This issue keeps appearing, and I think phylib ought to be doing more
to support it, rather than ethernet drivers having to play fancy games.
If one recalls stmmac, that has similar issues - it needs the RXC from
the PHY to reset properly.

I did propose that we have:

+#define PHY_F_RXC_ALWAYS_ON    BIT(30)

that can be passed to phy_attach_direct()'s flags, which phylib drivers
can then act upon to e.g. in the case of at803x, disable their
hibernation mode which stops the RXC when the system isn't suspended.
(AT803x Hibernation mode is enabled by default and the PHY automatically
enters it when the link is down.)

Maybe this flag should be used to determine the resume behaviour,
e.g. to ensure that the RXC is re-enabled early without the MAC driver
needing to be involved?

WoL is a different problem - that depends whether the PHY is itself
doing WoL independently from the MAC, or whether the MAC is involved.
If the MAC is involved, then clearly the MII link between the PHY and
MAC needs to be maintained while the system is in low power mode,
which is an entirely different issue from the RXC being present
while the MAC is being resumed.

Maybe PHY_F_RXC_ALWAYS_ON is a bad name, as I intended it to only refer
to while the system is running, not while in low power mode.

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

Re: [PATCH] net: phy: Only resume phy if it is suspended

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 2622 bytes --]

On 12/7/23 10:50, Russell King (Oracle) wrote:
> On Thu, Dec 07, 2023 at 09:56:01AM -0800, Florian Fainelli wrote:
>> Hi Andrew,
>>
>> So we discussed with Justin and Doug about this yesterday and the main
>> reason for the phy_resume() ... MAC initialization ... phy_start() pattern
>> has to do with external RGMII PHYs and the clocking dependency between the
>> MAC and the PHY on the RX path. And also a tiny bit of cargo culting, but
>> shhh.
>>
>> When the external RGMII PHYs are suspended they will stop providing a RXC
>> back to the Ethernet MAC and our Ethernet MAC like a lot of designs out
>> there require the RXC in order to be functional and complete its reset
>> procedure correctly (you would think there would be a way to mux in a
>> different clock, but that does not appear to be the case). If we reset the
>> UniMAC block without a RXC we will typically see duplicate packets being
>> received, or absurdly long round trip times as soon as we try to use the RX
>> path.
> 
> This issue keeps appearing, and I think phylib ought to be doing more
> to support it, rather than ethernet drivers having to play fancy games.
> If one recalls stmmac, that has similar issues - it needs the RXC from
> the PHY to reset properly.
> 
> I did propose that we have:
> 
> +#define PHY_F_RXC_ALWAYS_ON    BIT(30)
> 
> that can be passed to phy_attach_direct()'s flags, which phylib drivers
> can then act upon to e.g. in the case of at803x, disable their
> hibernation mode which stops the RXC when the system isn't suspended.
> (AT803x Hibernation mode is enabled by default and the PHY automatically
> enters it when the link is down.)
> 
> Maybe this flag should be used to determine the resume behaviour,
> e.g. to ensure that the RXC is re-enabled early without the MAC driver
> needing to be involved?
> 
> WoL is a different problem - that depends whether the PHY is itself
> doing WoL independently from the MAC, or whether the MAC is involved.
> If the MAC is involved, then clearly the MII link between the PHY and
> MAC needs to be maintained while the system is in low power mode,
> which is an entirely different issue from the RXC being present
> while the MAC is being resumed.
> 
> Maybe PHY_F_RXC_ALWAYS_ON is a bad name, as I intended it to only refer
> to while the system is running, not while in low power mode.
> 

Sure, I would like to see something similar and be able to use it, 
especially during boot.

In our particular case however we have a "double" suspend and resume 
which is at best unnecessary and wasting time, and at worst causing some 
unidentified side effects.
-- 
Florian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4221 bytes --]