From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonathan Cormer Date: Mon, 05 Mar 2018 11:00:17 -0500 Subject: [U-Boot] [PATCH] watchdog: omap_wdt: improve watchdog reset path In-Reply-To: References: Message-ID: <1520265617.24527.9.camel@criticallink.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de On Sat, 2018-03-03 at 12:00 +0000, u-boot-request at lists.denx.de wrote: >  > On 1 March 2018 at 16:33, Tom Rini wrote: > > > > On Thu, Mar 01, 2018 at 04:10:44PM +0200, Ruslan Bilovol wrote: > > > > > > Hi Lukasz, > > > > > > On Thu, Mar 1, 2018 at 12:53 PM, Lukasz Majewski > > > wrote: > > > > > > > > Hi Ruslan, > > > > > > > > > > > > > > Remove busy looping during watchdog reset. > > > > > Each polling of W_PEND_WTGR bit ("finish posted > > > > > write") after watchdog reset takes 120-140us > > > > > on BeagleBone Black board. Current U-Boot code > > > > > has watchdog resets in random places and often > > > > > there is situation when watchdog is reset > > > > > few times in a row in nested functions. > > > > > This adds extra delays and slows the whole system. > > > > > > > > > > Instead of polling W_PEND_WTGR bit, we skip > > > > > watchdog reset if the bit is set. Anyway, watchdog > > > > > is in the middle of reset *right now*, so we can > > > > > just return. > > > > It seems like similar problem may be in the Linux kernel > > > > driver: > > > > v4.16-rc1/source/drivers/watchdog/omap_wdt.c#L74 > > > > > > > > Linux driver also waits for the write. > > > Right, Linux driver has similar code but it doesn't affect > > > performance. > > > This is because of watchdog usage in Linux, it is enabled and > > > reset by userspace. This is quite rare event. > > > Moreover, since Linux has interrupts enabled and has scheduling, > > > such busy loops in the omap driver are not very different to > > > just mdelay() usage. The system can handle interrupts, and can > > > even do preemption if PREEMPT is enabled. > > > So use of such busy loops in that particular case shouldn't cause > > > any noticeable performance issues. > > True.  But, can you also submit a patch to the kernel side (and CC > > me) ? > > That's far more likely to get the attention of the engineers that > > might > > know of corner cases with the various SoC families where we might > > need > > to keep doing what we're doing or other possible problems.  Thanks! > > > Some additional input from my side. > > My previous measurements were wrong, due to usage of slow USB hub > port > on my laptop. Using another USB port I have next transfer speed for > "fastboot flash" operation: >  - without patch: 2.84 MiB/sec >  - with patch: 7.81  MiB/sec > > So it gives us about 2.75 times improvement, as stated by Ruslan in > his commit message. Also, I tested that WDT continues to work > correctly, and it does (tried several use-cases, and also debug patch > with infinite loop). So again: > >     Tested-by: Sam Protsenko > > Also: > >     Reviewed-by: Sam Protsenko > > I checked the code and I can say that there shouldn't be any concerns > about WDT functionality with this patch. By the way, in my opinion, > for kernel this patch doesn't make much sense, and there might be > even > confusion in case we send it... If there any concerns about it, we > can > invite related engineers in this discussion, asking them to review > this. > > Overall, I think this patch is "must have" in U-Boot, as it gives us > dramatic improvement without any drawbacks, especially for AM335x > boards. It's probably the best approach for WDT reset procedure until > interrupts are enabled for ARM architecture (if we even consider it). Tested patch with AM335x on custom u-boot-2013.10 based branch. Patch cleanly applied, obviously not much in this wdt driver has changed since then. Fastboot flash Before patch: 2.89MiB/s After patch: 8.68MiB/s Tested-by: Jonathan Cormier Reviewed-by: Jonathan Cormier