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

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@topicproducts.com
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:

quoted

On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:

quoted

On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:

quoted

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?

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@lunn.ch>

     Andrew

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


-- 
Mike Looijmans