Messages in this thread | | | Subject | Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal | From | Mike Looijmans <> | Date | Tue, 2 Feb 2021 12:40:13 +0100 |
| |
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: >> 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@lunn.ch> > > Andrew
Just wondering, now, a v2 patch isn't needed? Or should I amend the commit text?
-- Mike Looijmans
| |