| Message ID | 20231205234229.274601-1-justin.chen@broadcom.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: Only resume phy if it is suspended | expand |
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
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
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...
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.
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.
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.
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;
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(-)