[U-Boot] [PATCH] watchdog: omap_wdt: improve watchdog reset path

[Jonathan Cormer]

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 <trini at konsulko.com> 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 <lukma at denx.de>
> > > 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 <semen.protsenko at linaro.org>
> 
> Also:
> 
>     Reviewed-by: Sam Protsenko <semen.protsenko at linaro.org>
> 
> 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 <jcormier at criticallink.com>
Reviewed-by: Jonathan Cormier <jcormier at criticallink.com>