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

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:

quoted

On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:

quoted

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?

     Andrew