[U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards
This is needed for board with a very short watchdog timeout, like the lwmon5 with a 100ms timeout. Without this patch this board resets in the commands with long outputs, like "printenv" or "fdt print".
Note that the image size is not increased with this patch when CONFIG_HW_WATCHDOG or CONFIG_WATCHDOG are not defined since the compiler optimizes this additional code away.
Signed-off-by: Stefan Roese sr@denx.de --- v2: - Move declaration and assignment of "count" out of loop as pointed out by Wolfgang (thanks).
drivers/serial/ns16550.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c index 7e833fd..5dc18f4 100644 --- a/drivers/serial/ns16550.c +++ b/drivers/serial/ns16550.c @@ -5,6 +5,7 @@ */
#include <config.h> +#include <common.h> #include <ns16550.h> #include <watchdog.h> #include <linux/types.h> @@ -68,7 +69,13 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
void NS16550_putc (NS16550_t com_port, char c) { - while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0); + int count = 0; + + while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0) { + /* reset watchdog from time to time */ + if ((count++ % (256 << 10)) == 0) + WATCHDOG_RESET(); + } serial_out(c, &com_port->thr); }
Dear Stefan Roese,
In message 1286434900-11804-1-git-send-email-sr@denx.de you wrote:
This is needed for board with a very short watchdog timeout, like the lwmon5 with a 100ms timeout. Without this patch this board resets in the commands with long outputs, like "printenv" or "fdt print".
Sorry, but I still think there must be something awfully wrong with that code.
#include <config.h> +#include <common.h>
Why do you need this new include?
#include <ns16550.h> #include <watchdog.h> #include <linux/types.h> @@ -68,7 +69,13 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
void NS16550_putc (NS16550_t com_port, char c) {
- while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0);
- int count = 0;
- while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0) {
/* reset watchdog from time to time */if ((count++ % (256 << 10)) == 0)WATCHDOG_RESET();- }
We are in the putc() routine here, i. e. when printing a single character. This is a place which never can take 100 milliseconds, so it cannot be the culprit for any watchdog timeouts.
And you reinitialize the counter for each and every character printed. That means that your choice of the loop limit (256 << 10) must short enough that it gets triggered for essentially _all_ invocations of putc(), which is definitely overkill for triggering the watchdog.
I thing this is the wrong place to address the problem.
Best regards,
Wolfgang Denk
Hi Wolfgang,
On Thursday 07 October 2010 12:17:33 Wolfgang Denk wrote:
This is needed for board with a very short watchdog timeout, like the lwmon5 with a 100ms timeout. Without this patch this board resets in the commands with long outputs, like "printenv" or "fdt print".
Sorry, but I still think there must be something awfully wrong with that code.
#include <config.h>
+#include <common.h>
Why do you need this new include?
I added this while testing with udelay (see below). You're right, it's not needed any more.
#include <ns16550.h> #include <watchdog.h> #include <linux/types.h>
@@ -68,7 +69,13 @@ void NS16550_reinit (NS16550_t com_port, int baud_divisor)
void NS16550_putc (NS16550_t com_port, char c) {
- while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0);
- int count = 0;
- while ((serial_in(&com_port->lsr) & UART_LSR_THRE) == 0) {
/* reset watchdog from time to time */if ((count++ % (256 << 10)) == 0)WATCHDOG_RESET();- }
We are in the putc() routine here, i. e. when printing a single character. This is a place which never can take 100 milliseconds, so it cannot be the culprit for any watchdog timeouts.
And you reinitialize the counter for each and every character printed. That means that your choice of the loop limit (256 << 10) must short enough that it gets triggered for essentially _all_ invocations of putc(), which is definitely overkill for triggering the watchdog.
I thing this is the wrong place to address the problem.
The idea of implementing the watchdog_reset call in this function comes from the original implementation in the PPC4xx UART driver. As the ns166550 driver is now used on PPC4xx, this is missing right now and results in resets upon long outputs (e.g. printenv, fdt print) on lwmon5. The "old" PPC4xx UART driver did call udelay() in the putc() function. This implicitly called WATCHDOG_RESET(). I didn't want to add this udelay to ns16550, since this would lead to code increase for platforms not using this watchdog.
Back to the problem: How about moving the watchdog_reset call to serial_puts() in common/serial.c? Or do you have another (better) idea in mind?
Cheers, Stefan
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
Dear Stefan Roese,
In message 201010071327.40328.sr@denx.de you wrote:
The idea of implementing the watchdog_reset call in this function comes from the original implementation in the PPC4xx UART driver. As the ns166550 driver is now used on PPC4xx, this is missing right now and results in resets upon long outputs (e.g. printenv, fdt print) on lwmon5. The "old" PPC4xx UART driver did call udelay() in the putc() function. This implicitly called WATCHDOG_RESET(). I didn't want to add this udelay to ns16550, since this would lead to code increase for platforms not using this watchdog.
Code size increase, and slow down.
Back to the problem: How about moving the watchdog_reset call to serial_puts() in common/serial.c? Or do you have another (better) idea in mind?
Are you really sure this is the right place, i. e. this is where the problem happens?
I doubt that.
lwmon5 which has this nasty 100 ms watchdog (actually 80 ms including allowable tolerances, IIRC) is running at 115bps.
In theory, it would take a string of more than 900 characters for puts() to exceeding this delay, but all strings ever printed in U-Boot are way shorter.
That's why I ask again: are you sure it's puts() that is causing such long delays? Or should the watchdog triggering better happen somewhere else?
If you want to go with the triggering in serial_puts(), you probably either want to trigger unconditionally, before or after each call to puts(). The counter based approach makes no sense to me. If you are lucky it does not show any effect, and if you have bad luck it interferes with the WD_PERIOD / WD_MAX_RATE settings on this board.
Please keep in mind that on this specific board we have both upper and lower bounds for the WD trigger interval.
Best regards,
Wolfgang Denk
Back to the problem: How about moving the watchdog_reset call to serial_puts() in common/serial.c? Or do you have another (better) idea in mind?
Are you really sure this is the right place, i. e. this is where the problem happens?
I doubt that.
lwmon5 which has this nasty 100 ms watchdog (actually 80 ms including allowable tolerances, IIRC) is running at 115bps.
In theory, it would take a string of more than 900 characters for puts() to exceeding this delay, but all strings ever printed in U-Boot are way shorter.
Just my 2 cents: I don't know if that system has hardware handshake, and if that's enable. But a missing CTS could stop output indefinitely. Or a receiving terminal program might use CTS to halt output when its somewhat busy.
Just a thought for a possible scenario...
Best Regards, Reinhard
Hi Wolfgang,
On Thursday 07 October 2010 14:23:52 Wolfgang Denk wrote:
Back to the problem: How about moving the watchdog_reset call to serial_puts() in common/serial.c? Or do you have another (better) idea in mind?
Are you really sure this is the right place, i. e. this is where the problem happens?
I doubt that.
lwmon5 which has this nasty 100 ms watchdog (actually 80 ms including allowable tolerances, IIRC) is running at 115bps.
In theory, it would take a string of more than 900 characters for puts() to exceeding this delay, but all strings ever printed in U-Boot are way shorter.
That's why I ask again: are you sure it's puts() that is causing such long delays? Or should the watchdog triggering better happen somewhere else?
As mentioned before, lwmon5 reset upon commands with very long output, like printenv (with big environment), "fdt print", or "flinfo". Those commands can easily trigger output with more than 50 lines.
I tested again and have seen that lwmon5 currently resets upon approx. 20 lines of output. With an average of 50 chars per line thats 1000 chars. Pretty close to your 900 chars mentioned above. So yes, I'm sure that these reset result from the long serial output, where the CPU is mostly polling for the TX FIFO to become empty again.
If you want to go with the triggering in serial_puts(), you probably either want to trigger unconditionally, before or after each call to puts(). The counter based approach makes no sense to me.
OK.
Cheers, Stefan
-- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-0 Fax: (+49)-8142-66989-80 Email: office@denx.de
participants (3)
-
Reinhard Meyer -
Stefan Roese -
Wolfgang Denk