PING: [PATCH] net: smsc911x: Reset PHY during initialization

PING: [PATCH] net: smsc911x: Reset PHY during initialization

[Pavel Fedin]

 Hello! So, what should we do with this?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] On Behalf Of Pavel
> Fedin
> Sent: Monday, November 02, 2015 10:19 AM
> To: 'David Miller'
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; steve.glendinning@shawell.net
> Subject: RE: [PATCH] net: smsc911x: Reset PHY during initialization
> 
>  Hello!
> 
> > > On certain hardware after software reboot the chip may get stuck and fail
> > > to reinitialize during reset. This can be fixed by ensuring that PHY is
> > > reset too.
> > >
> > > Old PHY resetting method required operational MDIO interface, therefore
> > > the chip should have been already set up. In order to be able to function
> > > during probe, it is changed to use PMT_CTRL register.
> > >
> > > The problem could be observed on SMDK5410 board.
> > >
> > > Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> >
> > I'm pretty sure this is going to break the PHY loopback test.
> 
>  It's not (at least in normal situation), because first we do the test, and only then, if it
> fails, we reset the PHY. So, during
> normal operation of the driver, when loopback test succeeds at first attempt, we never attempt
> to reset the PHY. And, by the way, at
> least on my board this PHY reset did nothing useful, because after it loopback test still
> failed, all 100 times.
>  And, we don't use PHY reset anywhere outside of loopback test.
> 
> > This is such a tricky and fragile area to get right, therefore I
> > want you to specifically guard any change in how PHY reset is
> > done with tests against the specific chips that have the problem.
> 
>  Well, i could do one (or some combination) of the following, if you really want to:
>  a) Leave PHY reset inside loopback test as it is, but add a second routine and call it only
> before smsc911x_soft_reset().
>  b) Reset PHY only conditionally, using the following algorithm:
>     if smsc911x_soft_reset() {
>         /* NIC reset failed, kick the PHY and retry */
>         smsc911x_phy_reset()
>         if (smsc_911x_soft_reset())
>             return -ENODEV;
>     }
>  c) Do extra PHY reset only if some hint in device tree is specified (or the machine is known
> to have the problem)
> 
>  But, i dislike approach (a) for code duplication, because datasheet says that both reset
> methods are equivalent; i dislike approach
> (b) for actually being a hack; and i dislike (c) for adding extra stuff which seems to be not
> necessary. After all, what is wrong
> with just extra PHY reset before attempting to program anything? By the way, i test my patch
> with both software reboot and hardware
> reboot, and even powercycle. Everything is quite stable.
>  Well, it's up to you to decide. But i'd like to get the fix upstreamed somehow because from
> time to time we use these boards for
> tests, and it's quite annoiying to be unable to reboot them properly.
> 
> > Furthermore, I want you to test whether this has any negative
> > effects on the PHY loopback test.
> 
>  I did. I never disabled loopback test, so i actually discovered a problem (even two) with it.
> First, the problem was chip reset
> timeout. By increasing it to 300ms this problem seemed to be fixed, but loopback test started
> to fail. This was how i found and
> fixed crash with missing phy_disconnect(). I examined the code and discovered that upon
> loopback test failure we reset the PHY and
> retry. But in my case this PHY reset never did the right thing, and all loopback attempts
> (10x10 IIRC) were failing.
>  Some comments in the code gave me a clue that the main NIC can misbehave on reset if e.g. PHY
> is in powerdown state. I printed
> value of its control register and discovered that it was not the case. But then i discovered
> that we actually never try to reset the
> PHY before doing anything. Also, while studying the datasheed i discovered that there is a
> shorthand for resetting PHY without MDIO
> bus set up, but our driver doesn't use it, preferring MDIO method instead, which already
> requires operational NIC.
>  So, i suggested that PHY is just not being reset when board is rebooted by software. I wrote
> the patch and it worked. I verified it
> by reverting my previous NIC reset timeout increase, and it continued to work. After this
> loopback test on my board passes at first
> attempt.
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: PING: [PATCH] net: smsc911x: Reset PHY during initialization

[David Miller]

From: Pavel Fedin <p.fedin@samsung.com>
Date: Tue, 10 Nov 2015 09:36:24 +0300

>  Hello! So, what should we do with this?

If you think I should reconsider the patch, you should resubmit it.
The ball is always in your court.

RE: PING: [PATCH] net: smsc911x: Reset PHY during initialization

[Pavel Fedin]

 Hello!

> If you think I should reconsider the patch, you should resubmit it.

 I understand this, of course. But, before doing this i'd like to
clarify your concern, why exactly you think that loopback test will
break. Because the (simplified) algorithm is:

do {
    result = loopback_test()
    if (result == failed)
        reset_phy()
} while (result == ok)

 So, if loopback test works for the first time, PHY reset will never
be done. So, the conclusion is: in real situation it is never used at all.
 Conclusion no 2, coming from my tests: if loopback test fails, phy reset
actually does not fix it. So, perhaps, it's not needed there at all.

 I understand, that some other boards might behave differently, and loopback test was written this very way for some reason. Also, i
understand that you, as a maintainer, have rights for the final decision. And in order to rework the patch, i'd like to discuss with
you, which rework you would prefer. I came up with three possibilities:
--- cut ---
 a) Leave PHY reset inside loopback test as it is, but add a second routine and call it only before smsc911x_soft_reset().
 b) Reset PHY only conditionally, using the following algorithm:
    if smsc911x_soft_reset() {
        /* NIC reset failed, kick the PHY and retry */
        smsc911x_phy_reset()
        if (smsc_911x_soft_reset())
            return -ENODEV;
    }
 c) Do extra PHY reset only if some hint in device tree is specified (or the machine is known to have the problem)
--- cut ---
 I can even try to guess your thoughts (because you never elaborated them for me):

 1. loopback test will break because PHY has been reset before. In this case, (b) or (c) is a way to go.
 2. loopback test will break because of reset method change. In this case (a) is the way to go.

 So, what do you really think?

 BTW, where is Steve, whose address is specified in MAINTAINERS for this code? Is it abandonware?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: PING: [PATCH] net: smsc911x: Reset PHY during initialization

[David Miller]

From: Pavel Fedin <p.fedin@samsung.com>
Date: Wed, 11 Nov 2015 10:05:24 +0300

>  Hello!
> 
>> If you think I should reconsider the patch, you should resubmit it.
> 
>  I understand this, of course. But, before doing this i'd like to
> clarify your concern, why exactly you think that loopback test will
> break.

If I didn't reply it means I don't have anything constructive to say
to you, and probably I'll end up agreeing with your analysis of the
loopback test issue.

I'm not going to ask more than one more time for you to repost your
patch.

RE: PING: [PATCH] net: smsc911x: Reset PHY during initialization

[Pavel Fedin]

 Hello!

> >> If you think I should reconsider the patch, you should resubmit it.
> >
> >  I understand this, of course. But, before doing this i'd like to
> > clarify your concern, why exactly you think that loopback test will
> > break.
> 
> If I didn't reply it means I don't have anything constructive to say
> to you, and probably I'll end up agreeing with your analysis of the
> loopback test issue.
> 
> I'm not going to ask more than one more time for you to repost your
> patch.

 But, in this case, i don't have anything to change, do i? Or is it
just a formal requirement to RESEND? I can do this, if you want to.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: PING: [PATCH] net: smsc911x: Reset PHY during initialization

[David Miller]

From: Pavel Fedin <p.fedin@samsung.com>
Date: Thu, 12 Nov 2015 10:04:39 +0300

> Or is it just a formal requirement to RESEND?

Yes, this always what I ask people to do.