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

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

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