[U-Boot] [PATCH v2] ns16550: Add WATCHDOG_RESET to putc for short watchdog timeout boards

[Stefan Roese]

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