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

[Mike Looijmans]

Met vriendelijke groet / kind regards,

Mike Looijmans
System Expert


TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@xxxxxxxxxxxxxxxxx
W: www.topicproducts.com

Please consider the environment before printing this e-mail
On 28-01-2021 02:12, Andrew Lunn wrote:
> 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.

We only caught the "spike" because the reset GPIO was controlled by a 
GPIO expander, so it took about a millisecond to toggle it. With a 
"local" GPIO controller, the pulse duration would be below the 
microsecond range and most PHYs would never see it.

> 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@xxxxxxx>
>
>      Andrew

Just wondering, now, a v2 patch isn't needed? Or should I amend the 
commit text?


--
Mike Looijmans