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? You will find most datasheets talk about a clock must be active for some number of clock cycles prior to the reset signal being released, or minimum delay after power up before reset is released, or talking about a minimum pulse width. Note that looking at a few of the Marvell PHY datasheets, they require a minimum reset pulse width of 10ms and between 5ms and 50ms before the first access. AR8035 also talks about 10ms. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
