* RE: [PATCH] net: smsc911x: Reset PHY during initialization
2015-11-01 21:51 ` David Miller
@ 2015-11-02 7:19 ` Pavel Fedin
0 siblings, 0 replies; 3+ messages in thread
From: Pavel Fedin @ 2015-11-02 7:19 UTC (permalink / raw)
To: 'David Miller'; +Cc: netdev, linux-kernel, steve.glendinning
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
^ permalink raw reply [flat|nested] 3+ messages in thread