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 Tested-by: Jonathan Cormier <[email protected]> Reviewed-by: Jonathan Cormier <[email protected]> _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
