Messages in this thread | | | Subject | Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal | From | Mike Looijmans <> | Date | Wed, 27 Jan 2021 08:08:29 +0100 |
| |
See below.
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 26-01-2021 14:49, 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. > Thanks, excellent explanation.
As a bit of background, we were using a Marvell PHY where the datasheet states that thou shallt not release the reset within 50 ms of power-up. A pull-down on the active-low reset was thus added. Looking at the reset signal with a scope revealed a short spike, visible only because it was being controlled by an I2C GPIO expander. So it's indeed point "B" that we wanted to eliminate.
-- Mike Looijmans
| |