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

[Ruslan Bilovol]

On Mon, Mar 5, 2018 at 6:00 PM, Jonathan Cormer
<jcormier at criticallink.com> wrote:
> 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

Cool, that's even a little bit better than I've got on my setup!
Thanks for testing.

Regards,
Ruslan