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

[Lukasz Majewski]

Hi Ruslan,

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

Thank you for this work - it really improves speed considerably :-).

Reviewed-by: Lukasz Majewski <lukma at denx.de>

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




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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180302/8ba39b3e/attachment.sig>