watchdog: omap_wdt: improve watchdog reset path
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
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.
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
--- drivers/watchdog/omap_wdt.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
drivers/watchdog/omap_wdt.c +15 -6
@@ -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)
Signature · 1 line
1.9.1
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
Hi Ruslan,
Quoted 9 lines
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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.
Quoted 4 lines
Instead of polling
W_PEND_WTGRbit, 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.
Quoted 47 lines
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% fasterSigned-off-by Ruslan Bilovol <[email protected]>--- drivers/watchdog/omap_wdt.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)drivers/watchdog/omap_wdt.c +2
-3@@ -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
drivers/watchdog/omap_wdt.c +1
-0+* is in progress. It is resetting right now, no need to trigger it
drivers/watchdog/omap_wdt.c +7
-3+* 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
drivers/watchdog/omap_wdt.c +3
-0+* 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
drivers/watchdog/omap_wdt.c void hw_watchdog_reset(void) · line 77
Quoted 5 lines
drivers/watchdog/omap_wdt.c +2
-0+* 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 -------------- 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/20180301/d515b771/attachment.sig>
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
Hi Lukasz,
Quoted 16 lines · Lukasz Majewski
Hi Ruslan,
Quoted 9 lines
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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.Quoted 4 lines
Instead of polling
W_PEND_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.
Quoted 4 lines
It seems like similar problem may be in the Linux kernel driver:
v4.16-rc1/source/drivers/watchdog/omap_wdt.c#L74Linux 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
Quoted 66 lines
Quoted 48 lines
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% fasterSigned-off-by Ruslan Bilovol <[email protected]>--- drivers/watchdog/omap_wdt.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)drivers/watchdog/omap_wdt.c +2
-3@@ -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 itdrivers/watchdog/omap_wdt.c +7
-3+* 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
drivers/watchdog/omap_wdt.c +3
-0+* 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
drivers/watchdog/omap_wdt.c void hw_watchdog_reset(void) · line 77
Quoted 5 lines
drivers/watchdog/omap_wdt.c +2
-0+* 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
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
Quoted 24 lines · Ruslan Bilovol
Hi Lukasz,
Quoted 16 lines · Lukasz Majewski
Hi Ruslan,
Quoted 9 lines
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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.Quoted 4 lines
Instead of polling
W_PEND_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.Quoted 4 lines
It seems like similar problem may be in the Linux kernel driver:
v4.16-rc1/source/drivers/watchdog/omap_wdt.c#L74Linux driver also waits for the write.
Quoted 9 lines
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 justmdelay()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!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180301/1aa8f790/attachment.sig>
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
Quoted 35 lines · Tom Rini
Quoted 24 lines · Ruslan Bilovol
Hi Lukasz,
Quoted 16 lines · Lukasz Majewski
Hi Ruslan,
Quoted 9 lines
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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.Quoted 4 lines
Instead of polling
W_PEND_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.Quoted 4 lines
It seems like similar problem may be in the Linux kernel driver:
v4.16-rc1/source/drivers/watchdog/omap_wdt.c#L74Linux driver also waits for the write.
Quoted 9 lines
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 justmdelay()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.
Quoted 5 lines
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:
Also:
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).
Quoted 2 lines
--
Tom
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
In reply to Tom RiniQuoted 35 lines · Tom Rini
Quoted 24 lines · Ruslan Bilovol
Hi Lukasz,
Quoted 16 lines · Lukasz Majewski
Hi Ruslan,
Quoted 9 lines
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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.Quoted 4 lines
Instead of polling
W_PEND_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.Quoted 4 lines
It seems like similar problem may be in the Linux kernel driver:
v4.16-rc1/source/drivers/watchdog/omap_wdt.c#L74Linux driver also waits for the write.
Quoted 9 lines
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 justmdelay()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.
Quoted 4 lines
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!
Do you mean just resend this patch, but include kernel watchdog mailing list?
Or reimplement it for kernel?
In last case it doesn't have much sense - it brings almost no value to the driver, since watchdog usage approach by kernel is too different comparing to U-Boot.
Yet also kernel driver is more complicated and such a change may trigger it's rework. Probably will need to introduce additional synchronization at least.
By the way, if needed, I can resend this patch including watchdog kernel mailing list and related maintainer, so kernel folks can review and provide some feedback.
Best regards,
Ruslan Bilovol
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
Quoted 36 lines · Ruslan Bilovol
Quoted 35 lines · Tom Rini
Quoted 33 lines · Ruslan Bilovol
Hi Lukasz,
Quoted 16 lines · Lukasz Majewski
Hi Ruslan,
Quoted 9 lines
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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.Quoted 4 lines
Instead of polling
W_PEND_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.Quoted 4 lines
It seems like similar problem may be in the Linux kernel driver:
v4.16-rc1/source/drivers/watchdog/omap_wdt.c#L74Linux 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 justmdelay()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.
Quoted 7 lines
Quoted 4 lines
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!
Do you mean just resend this patch, but include kernel watchdog mailing list?
Well, I'm going to set aside what I said here since we've gotten some TI folks to review the patch here.
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180305/b152e509/attachment.sig>
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
In reply to Ruslan BilovolHi Ruslan,
Quoted 53 lines
Hi Lukasz,
Quoted 16 lines · Lukasz Majewski
Hi Ruslan,
Quoted 9 lines
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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.Quoted 4 lines
Instead of polling
W_PEND_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.Quoted 4 lines
It seems like similar problem may be in the Linux kernel driver:
v4.16-rc1/source/drivers/watchdog/omap_wdt.c#L74Linux 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.
Quoted 5 lines
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 :-).
Quoted 71 lines
Best regards,
RuslanQuoted 67 lines
Quoted 48 lines
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% fasterSigned-off-by Ruslan Bilovol <[email protected]>--- drivers/watchdog/omap_wdt.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)(quoted diff) +0
-0
b/drivers/watchdog/omap_wdt.cindex 7b1f429..d724c96 100644drivers/watchdog/omap_wdt.c +2
-3@@ -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 itdrivers/watchdog/omap_wdt.c +7
-3+* 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
drivers/watchdog/omap_wdt.c +3
-0+* 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
drivers/watchdog/omap_wdt.c void hw_watchdog_reset(void) · line 77
Quoted 5 lines
drivers/watchdog/omap_wdt.c +2
-0+* 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>
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
In reply to Ruslan BilovolQuoted 62 lines · Ruslan Bilovol
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.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% fasterSigned-off-by Ruslan Bilovol <[email protected]>--- drivers/watchdog/omap_wdt.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)drivers/watchdog/omap_wdt.c +15
-7@@ -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) --
Quoted 2 lines
1.9.1
Tested this patch on my BeagleBone Black Rev A5A. Works fine. I can see USB throughput improvement when doing "fastboot flash". Here are my results:
- before patch: 2.72 MiB/sec
- when disabling CONFIG_OMAP_WATCHDOG: 5.13 MiB/sec
- with this patch: 5.08 MiB/sec
So I can see 1.87 times improvement in speed, which is really good news for us, especially for flashing big images. I wonder, what is the max USB speed limit for transfer via EP1. E.g., what will we see if test it via Gadget Zero in Linux? Maybe there is a room for further improvements. But those are only my thoughts.
Thanks, Ruslan!
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
In reply to Ruslan BilovolQuoted 14 lines · Ruslan Bilovol
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.
Quoted 7 lines
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% fasterSigned-off-by Ruslan Bilovol <ruslan.bilovol at gmail.com
Given the limited usage of trigger register in this driver, I do not see any issue with this patch.
Thanks and regards,
Lokesh
Quoted 41 lines
--- drivers/watchdog/omap_wdt.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)drivers/watchdog/omap_wdt.c +15
-6@@ -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)
PATCH 1/1 watchdog: omap_wdt: improve watchdog reset path
In reply to Ruslan BilovolQuoted 14 lines · Ruslan Bilovol
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.
Quoted 7 lines
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% fasterSigned-off-by Ruslan Bilovol <[email protected]>
Signature · 1 line
Alex Kiernan
Quoted 20 lines · Ruslan Bilovol
Remove busy looping during watchdog reset. Each polling of
W_PEND_WTGRbit ("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_WTGRbit, we skip watchdog reset if the bit is set. Anyway, watchdog is in the middle of reset *right now*, so we can just return.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
Quoted 5 lines
Signed-off-by Ruslan Bilovol <[email protected]>Tested-by Sam Protsenko <[email protected]>Reviewed-by Lokesh Vutla <[email protected]>Reviewed-by Lukasz Majewski <[email protected]>Tested-by Alex Kiernan <[email protected]>
Applied to u-boot/master, thanks!
Signature · 8 lines
Tom -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180316/c6012d87/attachment.sig>