On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:Thanks, excellent explanation.
On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:Andrew, I don't get what you're saying.
The mdio_bus reset code first de-asserted the reset by allocating withHi Mike
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.
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.
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.