* [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards
@ 2010-10-07 7:01 Stefan Roese
2010-10-07 10:17 ` Wolfgang Denk
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2010-10-07 7:01 UTC (permalink / raw)
To: u-boot
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);
}
--
1.7.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards
2010-10-07 7:01 [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards Stefan Roese
@ 2010-10-07 10:17 ` Wolfgang Denk
2010-10-07 11:27 ` Stefan Roese
0 siblings, 1 reply; 6+ messages in thread
From: Wolfgang Denk @ 2010-10-07 10:17 UTC (permalink / raw)
To: u-boot
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
"Never face facts; if you do, you'll never get up in the morning."
- Marlo Thomas
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards
2010-10-07 10:17 ` Wolfgang Denk
@ 2010-10-07 11:27 ` Stefan Roese
2010-10-07 12:23 ` Wolfgang Denk
0 siblings, 1 reply; 6+ messages in thread
From: Stefan Roese @ 2010-10-07 11:27 UTC (permalink / raw)
To: u-boot
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 at denx.de
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards
2010-10-07 11:27 ` Stefan Roese
@ 2010-10-07 12:23 ` Wolfgang Denk
2010-10-07 12:45 ` Reinhard Meyer
2010-10-07 13:18 ` Stefan Roese
0 siblings, 2 replies; 6+ messages in thread
From: Wolfgang Denk @ 2010-10-07 12:23 UTC (permalink / raw)
To: u-boot
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
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
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
How many QA engineers does it take to screw in a lightbulb? 3: 1 to
screw it in and 2 to say "I told you so" when it doesn't work.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards
2010-10-07 12:23 ` Wolfgang Denk
@ 2010-10-07 12:45 ` Reinhard Meyer
2010-10-07 13:18 ` Stefan Roese
1 sibling, 0 replies; 6+ messages in thread
From: Reinhard Meyer @ 2010-10-07 12:45 UTC (permalink / raw)
To: u-boot
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards
2010-10-07 12:23 ` Wolfgang Denk
2010-10-07 12:45 ` Reinhard Meyer
@ 2010-10-07 13:18 ` Stefan Roese
1 sibling, 0 replies; 6+ messages in thread
From: Stefan Roese @ 2010-10-07 13:18 UTC (permalink / raw)
To: u-boot
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 at denx.de
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-07 13:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-07 7:01 [U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards Stefan Roese
2010-10-07 10:17 ` Wolfgang Denk
2010-10-07 11:27 ` Stefan Roese
2010-10-07 12:23 ` Wolfgang Denk
2010-10-07 12:45 ` Reinhard Meyer
2010-10-07 13:18 ` Stefan Roese
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox