lkml.org 
[lkml]   [2021]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal
    From
    Date
    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

    \
     
     \ /
      Last update: 2021-01-27 08:17    [W:7.040 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site