Re: [PATCH regression] net: phy: fix initialization (config_init) for Marvel 88E1116R PHYs

[Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>]

On 04/03/2014 12:12 AM, Alexander Holler wrote:
> Am 02.04.2014 22:25, schrieb Sebastian Hesselbarth:
>> On 04/02/2014 09:01 PM, Florian Fainelli wrote:
>>> 2014-04-02 2:09 GMT-07:00 Alexander Holler <holler@ahsoftware.de>:
>>>> Am 02.04.2014 02:57, schrieb Florian Fainelli:
>>>>> 2014-04-01 16:55 GMT-07:00 Alexander Holler <holler@ahsoftware.de>:
>>>>>> Commit 7cd1463664c2a15721ff4ccfb61d4d970815cb3d (introduced with 3.14)
>>>>>> changed the initialization of the mv643xx_eth driver to use phy_init_hw()
>>>>>> to reset the PHY. Unfortunately the initialization for the 88E1116R PHY
>>>>>> was broken such, that it used mdelay() instead of really waiting for a
>>>>>> reset to finish.
>>>
>>> Can you resubmit with the following information:
>>>
>>> - you specify the commit that introduce the problem in parenthesis,
>>> e.g: deadbeef ("dead: beef: cafe babe")
>>> - put this dmesg snippet in your log message to clearly illustrate
>>> what is happening
>>> - clarify that the PHY needs to be polled in a comment in
>>> m88e1116r_config_init()
>>>
>>> Meanwhile, it would be good if someone with access to this particular
>>> PHY datasheet could look for known erratas, problems, non-standard
>>> compliant behavior ....
>>
>> Alexander,
>>
>> I tried todays linux/master on Seagate Dockstar with Marvell 88E1116R
>> (0x01410e40) and cannot reproduce any regression. I tried booting from
>> tftp and usb, I also rebooted twice to see if there are any side
>> effects - nothing - ethernet always comes up as expected.
>>
>> I am curious, how you determined above commit to be the cause of the
>> regression you are seeing. Can you bisect, if you didn't already?
> 
> There was no bisecting necessary. I've just looked at what changed in
> mv643xx_eth since 3.13 and the first commit I've reverted was already a
> hit. Reading a bit source revealed the differences between the old reset
> and the newly used one and ended up with my patch (first try) and was a
> hit too.

Honestly, your own fix changes a different driver than mv643xx_eth.
There is a lot of changes from v3.13 to v3.14 and bisecting really
helps to pin-point the one offending patch. As you can see from my
tests with Dockstar, poking in the PHY driver may not be the right
place to fix it.

> Actually I assumed the reset needs longer than the 500ms, but as the
> printks revealed, the reset is much faster.
> So the problem seems to be the much increased time (1s) the newly used
> reset function idles in mdelay.

You assume that the PHY issue comes from waiting for too long _after_
the reset? And again, the very same PHY on Dockstar is not affected.

> But I think I have found the real reason. and the change of the reset
> just increased the chance the problem is hit (here with 100% success or
> fail rate however you want to name it).
> 
> Just give me a day or two to find the time to verify my assumption (I
> don't want to speculate) and maybe find a real fix for the problem. Of
> course, I still like my patch because it greatly decreases the time
> necessary for a reset (and the chance to hit the problem).

Well, you can share your idea anytime. You already speculated that PHY
reset on 88e1116r is broken but it seems that is not true. The more
you share of your issue and the tries to fix it, the more likely is it
we can follow your patch immediately.

Again, if you really want to find the real patch breaking Sheevaplug,
use git bisect.

Sebastian