From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0F046C433E6 for ; Thu, 28 Jan 2021 01:16:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D661D64DD0 for ; Thu, 28 Jan 2021 01:16:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231529AbhA1BQa (ORCPT ); Wed, 27 Jan 2021 20:16:30 -0500 Received: from vps0.lunn.ch ([185.16.172.187]:35440 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231132AbhA1BNh (ORCPT ); Wed, 27 Jan 2021 20:13:37 -0500 Received: from andrew by vps0.lunn.ch with local (Exim 4.94) (envelope-from ) id 1l4vrb-002xF8-6S; Thu, 28 Jan 2021 02:12:47 +0100 Date: Thu, 28 Jan 2021 02:12:47 +0100 From: Andrew Lunn To: Russell King - ARM Linux admin Cc: Mike Looijmans , netdev@vger.kernel.org, "David S. Miller" , Heiner Kallweit , Jakub Kicinski , linux-kernel@vger.kernel.org Subject: Re: [PATCH] net: mdiobus: Prevent spike on MDIO bus reset signal Message-ID: References: <20210126073337.20393-1-mike.looijmans@topic.nl> <20210126134937.GI1551@shell.armlinux.org.uk> <20210128002555.GQ1551@shell.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210128002555.GQ1551@shell.armlinux.org.uk> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. 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