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

[Ruslan Bilovol]

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.

In U-Boot we have polling instead of interrupts and something like
cooperative multitasking. Also watchdog resets much more often,
and such busy pollings in the driver delay execution of other code.

For example, in DFU we have indefinite loop, something like:
   --->
  |   dfu_write()
  |   watchdog_reset()
  |   ...
  |   handle_usb()
  |      `----- watchdog_reset()
  |       `---- ...
  `----'

And each delay caused by watchdog reset adds significant
overhead comparing to handling USB events or writing to the
storage.

So as bottom line, we don't need to do similar change in
Linux driver because there is almost no impact on performance
in that case. But in case of U-Boot which uses polling instead
of interrupts, this patch causes such a big performance
improvement.

Best regards,
Ruslan

>
>>
>> This noticeably increases performance of the
>> system. Below are some measurements on BBB:
>>  - DFU upload over USB                 15% faster
>>  - fastboot image upload               3x times faster
>>  - USB ep0 transfers with 4k packets   20% faster
>>
>> Signed-off-by: Ruslan Bilovol <ruslan.bilovol at gmail.com>
>> ---
>>  drivers/watchdog/omap_wdt.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
>> index 7b1f429..d724c96 100644
>> --- a/drivers/watchdog/omap_wdt.c
>> +++ b/drivers/watchdog/omap_wdt.c
>> @@ -53,16 +53,25 @@ void hw_watchdog_reset(void)
>>  {
>>       struct wd_timer *wdt = (struct wd_timer *)WDT_BASE;
>>
>> -     /* wait for posted write to complete */
>> -     while ((readl(&wdt->wdtwwps)) & WDT_WWPS_PEND_WTGR)
>> -             ;
>> +     /*
>> +      * Somebody just triggered watchdog reset and write to WTGR
>> register
>> +      * is in progress. It is resetting right now, no need to
>> trigger it
>> +      * again
>> +      */
>> +     if ((readl(&wdt->wdtwwps)) & WDT_WWPS_PEND_WTGR)
>> +             return;
>>
>>       wdt_trgr_pattern = ~wdt_trgr_pattern;
>>       writel(wdt_trgr_pattern, &wdt->wdtwtgr);
>>
>> -     /* wait for posted write to complete */
>> -     while ((readl(&wdt->wdtwwps) & WDT_WWPS_PEND_WTGR))
>> -             ;
>> +     /*
>> +      * Don't wait for posted write to complete, i.e. don't check
>> +      * WDT_WWPS_PEND_WTGR bit in WWPS register. There is no
>> writes to
>> +      * WTGR register outside of this func, and if entering it
>> +      * we see WDT_WWPS_PEND_WTGR bit set, it means watchdog reset
>> +      * was just triggered. This prevents us from wasting time in
>> busy
>> +      * polling of WDT_WWPS_PEND_WTGR bit.
>> +      */
>>  }
>>
>>  static int omap_wdt_set_timeout(unsigned int timeout)
>
>
>
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de