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@xxxxxxxxxxxxxxx [mailto:netdev-owner@xxxxxxxxxxxxxxx] On Behalf Of Pavel
> Fedin
> Sent: Monday, November 02, 2015 10:19 AM
> To: 'David Miller'
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; steve.glendinning@xxxxxxxxxxx
> 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@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/