| Message ID | 1543320947-32763-1-git-send-email-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe | expand |
On Tue, Nov 27, 2018 at 12:18:20PM +0000, Yoshihiro Shimoda wrote: > Some PHY device needs edge signal of the reset, but the previous code > is impossible to achieve it like following: > > 1) Kernel boots by using initramfs. > --> No open the nic, so the provious code deasserts the reset by > phy_device_register() and phy_probe(). > 2) Kernel enters the suspend. > --> So, keep the reset signal as deassert. > --> On R-Car Salvator-XS board, unfortunately, the board power is > turned off. > 3) Kernel returns from suspend. > 4) ifconfig eth0 up > --> Then, since edge signal of the reset doesn't happen, > it cannot link up. Hi Yoshihiro It sounds like you should be adding code to the suspend/resume handling of phylib, so that it toggle the reset on resume. You cannot just delete code like you proposed, it is going to break devices. But adding code should be O.K. Andrew
On 27.11.2018 17:44, Andrew Lunn wrote: > On Tue, Nov 27, 2018 at 12:18:20PM +0000, Yoshihiro Shimoda wrote: >> Some PHY device needs edge signal of the reset, but the previous code >> is impossible to achieve it like following: >> >> 1) Kernel boots by using initramfs. >> --> No open the nic, so the provious code deasserts the reset by >> phy_device_register() and phy_probe(). >> 2) Kernel enters the suspend. >> --> So, keep the reset signal as deassert. >> --> On R-Car Salvator-XS board, unfortunately, the board power is >> turned off. >> 3) Kernel returns from suspend. >> 4) ifconfig eth0 up >> --> Then, since edge signal of the reset doesn't happen, >> it cannot link up. > > Hi Yoshihiro > > It sounds like you should be adding code to the suspend/resume > handling of phylib, so that it toggle the reset on resume. You cannot > just delete code like you proposed, it is going to break devices. But > adding code should be O.K. > The commit message mentions that the patch is supposed to fix some issue on the Salvator-XS board. I found the following from a year ago https://www.spinics.net/lists/netdev/msg457308.html which is also about PHY reset and this board. Is there still something open? But as Andrew mentioned already: Just deleting code w/o checking what it's good for and whether this could have side effects, isn't a solution. Especially because the patch would silently remove the call to phy_scan_fixups(). Heiner > Andrew > . >
Hi Heiner, On Tue, Nov 27, 2018 at 8:47 PM Heiner Kallweit <hkallweit1@gmail.com> wrote: > On 27.11.2018 17:44, Andrew Lunn wrote: > > On Tue, Nov 27, 2018 at 12:18:20PM +0000, Yoshihiro Shimoda wrote: > >> Some PHY device needs edge signal of the reset, but the previous code > >> is impossible to achieve it like following: > >> > >> 1) Kernel boots by using initramfs. > >> --> No open the nic, so the provious code deasserts the reset by > >> phy_device_register() and phy_probe(). > >> 2) Kernel enters the suspend. > >> --> So, keep the reset signal as deassert. > >> --> On R-Car Salvator-XS board, unfortunately, the board power is > >> turned off. > >> 3) Kernel returns from suspend. > >> 4) ifconfig eth0 up > >> --> Then, since edge signal of the reset doesn't happen, > >> it cannot link up. > > > > Hi Yoshihiro > > > > It sounds like you should be adding code to the suspend/resume > > handling of phylib, so that it toggle the reset on resume. You cannot > > just delete code like you proposed, it is going to break devices. But > > adding code should be O.K. > > > The commit message mentions that the patch is supposed to fix some > issue on the Salvator-XS board. I found the following from a year ago > https://www.spinics.net/lists/netdev/msg457308.html > which is also about PHY reset and this board. Is there still something > open? But as Andrew mentioned already: Just deleting code w/o > checking what it's good for and whether this could have side effects, > isn't a solution. Especially because the patch would silently remove > the call to phy_scan_fixups(). That's correct: my original motivation for picking up Sergei's patches was to make Ethernet work after suspend/resume on the same Salvator-XS board. The main difference seems to be that I was using NFS root, while Shimoda-san is using an initramfs. Hence there probably is an inconsistency in reset handling for an active vs. inactive Ethernet interface. Gr{oetje,eeting}s, Geert
Hi Andrew, > From: Andrew Lunn, Sent: Wednesday, November 28, 2018 1:44 AM > > On Tue, Nov 27, 2018 at 12:18:20PM +0000, Yoshihiro Shimoda wrote: > > Some PHY device needs edge signal of the reset, but the previous code > > is impossible to achieve it like following: > > > > 1) Kernel boots by using initramfs. > > --> No open the nic, so the provious code deasserts the reset by > > phy_device_register() and phy_probe(). > > 2) Kernel enters the suspend. > > --> So, keep the reset signal as deassert. > > --> On R-Car Salvator-XS board, unfortunately, the board power is > > turned off. > > 3) Kernel returns from suspend. > > 4) ifconfig eth0 up > > --> Then, since edge signal of the reset doesn't happen, > > it cannot link up. > > Hi Yoshihiro > > It sounds like you should be adding code to the suspend/resume > handling of phylib, so that it toggle the reset on resume. You cannot > just delete code like you proposed, it is going to break devices. But > adding code should be O.K. Thank you for your comment! I understood we cannot delete the code, but can add the suspend/resume handling of phylib. I'll make such a patch. Best regards, Yoshihiro Shimoda
Hi Heiner, > From: Heiner Kallwei, Sent: Wednesday, November 28, 2018 4:46 AM > > On 27.11.2018 17:44, Andrew Lunn wrote: > > On Tue, Nov 27, 2018 at 12:18:20PM +0000, Yoshihiro Shimoda wrote: > >> Some PHY device needs edge signal of the reset, but the previous code > >> is impossible to achieve it like following: > >> > >> 1) Kernel boots by using initramfs. > >> --> No open the nic, so the provious code deasserts the reset by > >> phy_device_register() and phy_probe(). > >> 2) Kernel enters the suspend. > >> --> So, keep the reset signal as deassert. > >> --> On R-Car Salvator-XS board, unfortunately, the board power is > >> turned off. > >> 3) Kernel returns from suspend. > >> 4) ifconfig eth0 up > >> --> Then, since edge signal of the reset doesn't happen, > >> it cannot link up. > > > > Hi Yoshihiro > > > > It sounds like you should be adding code to the suspend/resume > > handling of phylib, so that it toggle the reset on resume. You cannot > > just delete code like you proposed, it is going to break devices. But > > adding code should be O.K. > > > The commit message mentions that the patch is supposed to fix some > issue on the Salvator-XS board. I found the following from a year ago > https://www.spinics.net/lists/netdev/msg457308.html > which is also about PHY reset and this board. Is there still something > open? Thank you for your comment! As Geert mentioned on this email thread, that patch can handle if user opened the NIC and then the PHY was active. > But as Andrew mentioned already: Just deleting code w/o > checking what it's good for and whether this could have side effects, > isn't a solution. Especially because the patch would silently remove > the call to phy_scan_fixups(). I should have mentioned on the commit log, but phy_scan_fixups() is called by phy_init_hw() and phy_init_hw() is called phy_attach_direct(). So, I think we can remove phy_scan_fixups(). However, as you mentioned, this patch could have side effects... So, I'll make such a suspend/resume patch that Andrew mentioned later. Best regards, Yoshihiro Shimoda
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index e06613f..e01a752 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -802,16 +802,6 @@ int phy_device_register(struct phy_device *phydev) if (err) return err; - /* Deassert the reset signal */ - phy_device_reset(phydev, 0); - - /* Run all of the fixups for this PHY */ - err = phy_scan_fixups(phydev); - if (err) { - pr_err("PHY %d failed to initialize\n", phydev->mdio.addr); - goto out; - } - err = device_add(&phydev->mdio.dev); if (err) { pr_err("PHY %d failed to add\n", phydev->mdio.addr); @@ -821,8 +811,6 @@ int phy_device_register(struct phy_device *phydev) return 0; out: - /* Assert the reset signal */ - phy_device_reset(phydev, 1); mdiobus_unregister_device(&phydev->mdio); return err; @@ -2202,16 +2190,8 @@ static int phy_probe(struct device *dev) /* Set the state to READY by default */ phydev->state = PHY_READY; - if (phydev->drv->probe) { - /* Deassert the reset signal */ - phy_device_reset(phydev, 0); - + if (phydev->drv->probe) err = phydev->drv->probe(phydev); - if (err) { - /* Assert the reset signal */ - phy_device_reset(phydev, 1); - } - } mutex_unlock(&phydev->lock);
Some PHY device needs edge signal of the reset, but the previous code is impossible to achieve it like following: 1) Kernel boots by using initramfs. --> No open the nic, so the provious code deasserts the reset by phy_device_register() and phy_probe(). 2) Kernel enters the suspend. --> So, keep the reset signal as deassert. --> On R-Car Salvator-XS board, unfortunately, the board power is turned off. 3) Kernel returns from suspend. 4) ifconfig eth0 up --> Then, since edge signal of the reset doesn't happen, it cannot link up. 5) ifconfig eth0 down 6) ifconfig eth0 up --> In this case, it can link up. This patch is possible to break if ->probe() in a phy driver will access the PHY. But, I believe ->config_init() only initializes the PHY. However, I think this patch should be RFC at first. Reported-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/net/phy/phy_device.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)