Re: PING: [PATCH] net: smsc911x: Reset PHY during initialization
From: Pavel Fedin 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: PING: [PATCH] net: smsc911x: Reset PHY during initialization
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: PING: [PATCH] net: smsc911x: Reset PHY during initialization
From: Pavel Fedin 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: PING: [PATCH] net: smsc911x: Reset PHY during initialization
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: PING: [PATCH] net: smsc911x: Reset PHY during initialization
From: Pavel Fedin 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
PING: [PATCH] net: smsc911x: Reset PHY during initialization
Hello! So, what should we do with this?
Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia
> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On
> Behalf Of Pavel
> Fedin
> Sent: Monday, November 02, 2015 10:19 AM
> To: 'David Miller'
> Cc: net...@vger.kernel.org; linux-kernel@vger.kernel.org;
> steve.glendinn...@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
> >
> > 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 majord...@vger.kernel.org
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo
