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

[Jon Cormier]

Because someone was wondering what the usb speed was in linux.  My
kernel is based on 3.2. Note I have USB DMA disabled so that doesn't
help the speed.  Not sure if u-boot is using DMA but I'd guess not.

ADB over usb
$ time adb -s 16019491 push userdata.img /mnt/obb/
userdata.img: 1 file pushed. 3.5 MB/s (45007716 bytes in 12.391s)

ADB over rndis
 $ time adb -s 192.168.2.1 push userdata.img /mnt/obb/
userdata.img: 1 file pushed. 2.9 MB/s (45007716 bytes in 14.900s)

Netcat over rndis
1|root at android:/ # time busybox nc 192.168.2.2 12345 > /mnt/obb/userdata.img
    0m11.12s real     0m0.11s user     0m8.30s system
~3.9MB/s

On Mon, Mar 5, 2018 at 3:45 PM, Ruslan Bilovol <ruslan.bilovol at gmail.com> wrote:
> 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



-- 
Jonathan Cormier
Software Engineer

Voice:  315.425.4045 x222


http://www.CriticalLink.com
6712 Brooklawn Parkway, Syracuse, NY 13211