| Message ID | 20211211130146.357794-1-francesco.dolcini@toradex.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: perform a PHY reset on resume | expand |
On Sat, Dec 11, 2021 at 02:01:46PM +0100, Francesco Dolcini wrote: > Perform a PHY reset in phy_init_hw() to ensure that the PHY is working > after resume. This is required if the PHY was powered down in suspend > like it is done by the freescale FEC driver in fec_suspend(). > > Link: https://lore.kernel.org/netdev/20211206101326.1022527-1-philippe.schenker@toradex.com/ > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > Philippe: what about something like that? Only compile tested, but I see no reason for this not solving the issue. > > Any delay required on the reset can be specified using reset-assert-us/reset-deassert-us. > > --- > drivers/net/phy/phy_device.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 74d8e1dc125f..7eab0c054adf 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1158,7 +1158,8 @@ int phy_init_hw(struct phy_device *phydev) > { > int ret = 0; > > - /* Deassert the reset signal */ > + /* phy reset required if the phy was powered down during suspend */ > + phy_device_reset(phydev, 1); > phy_device_reset(phydev, 0); > > if (!phydev->drv) I don't particularly like this - this impacts everyone who is using phylib at this point, whereas no reset was happening if the reset was already deasserted here. In the opening remarks to this series, it is stated: If a hardware-design is able to control power to the Ethernet PHY and relying on software to do a reset, the PHY does no longer work after resuming from suspend, given the PHY does need a hardware-reset. This requirement is conditional on the hardware design, it isn't a universal requirement and won't apply everywhere. I think it needs to be described in firmware that this action is required. That said... Please check the datasheet on the PHY regarding application of power and reset. You may find that the PHY datasheet requires the reset to be held active from power up until the clock input is stable - this could mean you need some other arrangement to assert the PHY reset signal after re-applying power sooner than would happen by the proposed point in the kernel.
Hello, On Sat, Dec 11, 2021 at 02:15:54PM +0000, Russell King (Oracle) wrote: > I don't particularly like this - this impacts everyone who is using > phylib at this point, whereas no reset was happening if the reset was > already deasserted here. I understand your concern, but I do not believe that this can create any issue. The code should be able to handle the situation in which the PHY is getting out of reset at that time. > In the opening remarks to this series, it is stated: > > If a hardware-design is able to control power to the Ethernet PHY and > relying on software to do a reset, the PHY does no longer work after > resuming from suspend, given the PHY does need a hardware-reset. > > This requirement is conditional on the hardware design, it isn't a > universal requirement and won't apply everywhere. I think it needs to > be described in firmware that this action is required. That said... > > Please check the datasheet on the PHY regarding application of power and > reset. You may find that the PHY datasheet requires the reset to be held > active from power up until the clock input is stable - this could mean > you need some other arrangement to assert the PHY reset signal after > re-applying power sooner than would happen by the proposed point in the > kernel. I checked before sending this patch, the phy is a KSZ9131 [1] and according to the power sequence timing the reset should be toggled after the power-up. No requirement on the clock or other signals. The HW design is quite simple, we have a regulator controlling the PHY power, and a GPIO controlling the reset. On suspend we remove the power (FEC driver), on resume after enabling the power the PHY require a reset, but nobody is doing it. The issue here is that the phy regulator is handled by the FEC driver, while the RESET_N GPIO can be controlled by both the FEC driver or the phylib. The initial proposal was to handle this into the FEC driver, but it was not considered a good idea, therefore this new proposal. One more comment about that, I do not believe that asserting the reset in the suspend path is a good idea, in the general situation in which the PHY is powered in suspend the power-consumption is likely to be higher if the device is in reset compared to software power-down using the BMCR register. > universal requirement and won't apply everywhere. I think it needs to > be described in firmware that this action is required. That said... Are you thinking at a DTS property? Not sure to understand how do you envision this. At the moment the regulator is not handled by the phylib, and the property should be something like reset-on-resume, I guess ... Francesco [1] https://ww1.microchip.com/downloads/en/DeviceDoc/00002841C.pdf
Hi Francesco, > -----Original Message----- > From: Francesco Dolcini <francesco.dolcini@toradex.com> > Sent: 2021年12月11日 21:02 > To: philippe.schenker@toradex.com; andrew@lunn.ch; Joakim Zhang > <qiangqing.zhang@nxp.com> > Cc: davem@davemloft.net; festevam@gmail.com; kuba@kernel.org; > linux-kernel@vger.kernel.org; linux@armlinux.org.uk; > netdev@vger.kernel.org; Francesco Dolcini <francesco.dolcini@toradex.com> > Subject: [PATCH net-next] net: phy: perform a PHY reset on resume > > Perform a PHY reset in phy_init_hw() to ensure that the PHY is working after > resume. This is required if the PHY was powered down in suspend like it is > done by the freescale FEC driver in fec_suspend(). > > Link: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k > ernel.org%2Fnetdev%2F20211206101326.1022527-1-philippe.schenker%40tor > adex.com%2F&data=04%7C01%7Cqiangqing.zhang%40nxp.com%7C408 > 258b86fec4c39a1b708d9bca6755f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7 > C0%7C0%7C637748245394278104%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C300 > 0&sdata=m17Q5b3CZVI89xmplVVwVvCHEXZrMkY6dYIAmz2v3CE%3D&a > mp;reserved=0 > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > Philippe: what about something like that? Only compile tested, but I see no > reason for this not solving the issue. > > Any delay required on the reset can be specified using > reset-assert-us/reset-deassert-us. Looks fine for me. We can trigger a hardware reset first, then following a soft reset and phy configurations, the logic seems reasonable. Also need PHY maintainers give more comments. Best Regards, Joakim Zhang > --- > drivers/net/phy/phy_device.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > index 74d8e1dc125f..7eab0c054adf 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1158,7 +1158,8 @@ int phy_init_hw(struct phy_device *phydev) { > int ret = 0; > > - /* Deassert the reset signal */ > + /* phy reset required if the phy was powered down during suspend */ > + phy_device_reset(phydev, 1); > phy_device_reset(phydev, 0); > > if (!phydev->drv) > -- > 2.25.1
On Sat, 2021-12-11 at 14:01 +0100, Francesco Dolcini wrote: > Perform a PHY reset in phy_init_hw() to ensure that the PHY is working > after resume. This is required if the PHY was powered down in suspend > like it is done by the freescale FEC driver in fec_suspend(). > > Link: > https://lore.kernel.org/netdev/20211206101326.1022527-1-philippe.schenker@toradex.com/ > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > Philippe: what about something like that? Only compile tested, but I > see no reason for this not solving the issue. > > Any delay required on the reset can be specified using reset-assert- > us/reset-deassert-us. That would of course be the easiest way. However I understand Russel's concerns here, as every PHY is again different and this is basically a hardware-specific design. I like Joakin's idea to add a phy_reset_after_power_on() function in phylib similar to phy_reset_after_clk_enable(). I will prepare a patchset for that so we can discuss further there. Philippe > > --- > drivers/net/phy/phy_device.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/phy_device.c > b/drivers/net/phy/phy_device.c > index 74d8e1dc125f..7eab0c054adf 100644 > --- a/drivers/net/phy/phy_device.c > +++ b/drivers/net/phy/phy_device.c > @@ -1158,7 +1158,8 @@ int phy_init_hw(struct phy_device *phydev) > { > int ret = 0; > > - /* Deassert the reset signal */ > + /* phy reset required if the phy was powered down during > suspend */ > + phy_device_reset(phydev, 1); > phy_device_reset(phydev, 0); > > if (!phydev->drv)
On Sat, Dec 11, 2021 at 02:15:54PM +0000, Russell King (Oracle) wrote: > I don't particularly like this - this impacts everyone who is using > phylib at this point, whereas no reset was happening if the reset was > already deasserted here. Let's drop this patch, Philippe will send a new patch adding a phy_reset_after_power_on() function similar to phy_reset_after_clk_enable(). Francesco
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 74d8e1dc125f..7eab0c054adf 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1158,7 +1158,8 @@ int phy_init_hw(struct phy_device *phydev) { int ret = 0; - /* Deassert the reset signal */ + /* phy reset required if the phy was powered down during suspend */ + phy_device_reset(phydev, 1); phy_device_reset(phydev, 0); if (!phydev->drv)
Perform a PHY reset in phy_init_hw() to ensure that the PHY is working after resume. This is required if the PHY was powered down in suspend like it is done by the freescale FEC driver in fec_suspend(). Link: https://lore.kernel.org/netdev/20211206101326.1022527-1-philippe.schenker@toradex.com/ Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> --- Philippe: what about something like that? Only compile tested, but I see no reason for this not solving the issue. Any delay required on the reset can be specified using reset-assert-us/reset-deassert-us. --- drivers/net/phy/phy_device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)