On Mon, Mar 5, 2018 at 6:00 PM, Jonathan Cormer <[email protected]> wrote: > On Sat, 2018-03-03 at 12:00 +0000, [email protected] wrote: >> >> On 1 March 2018 at 16:33, Tom Rini <[email protected]> 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 <[email protected]> >> > > 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 <[email protected]> >> >> Also: >> >> Reviewed-by: Sam Protsenko <[email protected]> >> >> 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
Cool, that's even a little bit better than I've got on my setup! Thanks for testing. Regards, Ruslan _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
