[PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe

[PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe

[Yoshihiro Shimoda]

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(-)

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);
 
-- 
1.9.1

Re: [PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe

[Andrew Lunn]

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

Re: [PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe

[Heiner Kallweit]

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
> .
> 

Re: [PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe

[Geert Uytterhoeven]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe

[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

RE: [PATCH/RFC] net: phy: device: Don't deassert the reset when register and probe

[Yoshihiro Shimoda]

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