Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Jan 28, 2021 at 12:25:55AM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
> > On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
> > > > On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
> > > > > The mdio_bus reset code first de-asserted the reset by allocating with
> > > > > GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
> > > > > the reset signal defaulted to asserted, there'd be a short "spike"
> > > > > before the reset.
> > > > > 
> > > > > Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
> > > > > removes the spike and also removes a line of code since the signal
> > > > > is already high.
> > > > 
> > > > Hi Mike
> > > > 
> > > > This however appears to remove the reset pulse, if the reset line was
> > > > already low to start with. Notice you left
> > > > 
> > > > fsleep(bus->reset_delay_us);
> > > > 
> > > > without any action before it? What are we now waiting for?  Most data
> > > > sheets talk of a reset pulse. Take the reset line high, wait for some
> > > > time, take the reset low, wait for some time, and then start talking
> > > > to the PHY. I think with this patch, we have lost the guarantee of a
> > > > low to high transition.
> > > > 
> > > > Is this spike, followed by a pulse actually causing you problems? If
> > > > so, i would actually suggest adding another delay, to stretch the
> > > > spike. We have no control over the initial state of the reset line, it
> > > > is how the bootloader left it, we have to handle both states.
> > > 
> > > Andrew, I don't get what you're saying.
> > > 
> > > Here is what happens depending on the pre-existing state of the
> > > reset signal:
> > > 
> > > Reset (previously asserted):   ~~~|_|~~~~|_______
> > > Reset (previously deasserted): _____|~~~~|_______
> > >                                   ^ ^    ^
> > >                                   A B    C
> > > 
> > > At point A, the low going transition is because the reset line is
> > > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > > the first thing we do is set it high _without_ any delay. This is
> > > point B. So, a glitch occurs between A and B.
> > > 
> > > We then fsleep() and finally set the GPIO low at point C.
> > > 
> > > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > > transitions. Instead we get:
> > > 
> > > Reset (previously asserted)  : ~~~~~~~~~~|______
> > > Reset (previously deasserted): ____|~~~~~|______
> > >                                    ^     ^
> > >                                    A     C
> > > 
> > > Where A and C are the points described above in the code. Point B
> > > has been eliminated.
> > > 
> > > Therefore, to me the patch looks entirely reasonable and correct.
> > 
> > I wonder if there are any PHYs which actually need a pulse? Would it
> > be better to have:
> > 
> >  Reset (previously asserted):   ~~~|____|~~~~|_______
> >  Reset (previously deasserted): ________|~~~~|_______
> >                                    ^    ^    ^    ^
> >                                    A    B    C    D
> > 
> > Point D is where we actually start talking to the PHY. C-D is
> > reset-post-delay-us, and defaults to 0, but can be set via DT.  B-C is
> > reset-delay-us, and defaults to 10us, but can be set via DT.
> > Currently A-B is '0', so we get the glitch. But should we make A-B the
> > same as B-C, so we get a real pulse?
> 
> I do not see any need for A-B - what is the reason for it?

If level is all that matters, then it is not needed. If a PHY needs an
actual pulse, both a raising and a falling edge, we potentially don't
get the rising edge now.

But the datasheets you have looked at all seem to talk about level,
not pulse. So lets go with this.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew