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

[Mike Looijmans <mike.looijmans@topic.nl>]

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