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

[Sam Protsenko]

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).

> --
> Tom