[U-Boot] serial: ns16550: fix debug uart putc called before init

Message ID 20190109193531.12156-1-simon.k.r.goldschmidt@gmail.com
State Accepted
Delegated to: Tom Rini
Headers show
Series
  • [U-Boot] serial: ns16550: fix debug uart putc called before init
Related show

Commit Message

Simon Goldschmidt Jan. 9, 2019, 7:35 p.m.
If _debug_uart_putc() is called before _debug_uart_init(), the
ns16550 debug uart driver hangs in a tight loop waiting for the
tx FIFO to get empty.

As this can happen via a printf sneaking in before the port calls
debug_uart_init(), introduce a config option to ignore characters
before the debug uart is initialized.

This is done by reading the baudrate divisor and aborting if is zero.

The Kconfig option is required as reading the baudrate divisor does
not seem to work for all ns16500 compatibles (which is why the last
attempt on this has been reverted in 1a67969a99).

Tested on socfpga_cyclone5_socrates.

Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
---

 drivers/serial/Kconfig   | 12 ++++++++++++
 drivers/serial/ns16550.c | 20 ++++++++++++++++++--
 2 files changed, 30 insertions(+), 2 deletions(-)

Comments

Simon Glass Jan. 10, 2019, 12:57 p.m. | #1
On Wed, 9 Jan 2019 at 12:35, Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> If _debug_uart_putc() is called before _debug_uart_init(), the
> ns16550 debug uart driver hangs in a tight loop waiting for the
> tx FIFO to get empty.
>
> As this can happen via a printf sneaking in before the port calls
> debug_uart_init(), introduce a config option to ignore characters
> before the debug uart is initialized.
>
> This is done by reading the baudrate divisor and aborting if is zero.
>
> The Kconfig option is required as reading the baudrate divisor does
> not seem to work for all ns16500 compatibles (which is why the last
> attempt on this has been reverted in 1a67969a99).
>
> Tested on socfpga_cyclone5_socrates.
>
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> ---
>
>  drivers/serial/Kconfig   | 12 ++++++++++++
>  drivers/serial/ns16550.c | 20 ++++++++++++++++++--
>  2 files changed, 30 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Tom Rini Jan. 16, 2019, 2:44 a.m. | #2
On Wed, Jan 09, 2019 at 08:35:31PM +0100, Simon Goldschmidt wrote:

> If _debug_uart_putc() is called before _debug_uart_init(), the
> ns16550 debug uart driver hangs in a tight loop waiting for the
> tx FIFO to get empty.
> 
> As this can happen via a printf sneaking in before the port calls
> debug_uart_init(), introduce a config option to ignore characters
> before the debug uart is initialized.
> 
> This is done by reading the baudrate divisor and aborting if is zero.
> 
> The Kconfig option is required as reading the baudrate divisor does
> not seem to work for all ns16500 compatibles (which is why the last
> attempt on this has been reverted in 1a67969a99).
> 
> Tested on socfpga_cyclone5_socrates.
> 
> Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!
Simon Goldschmidt Jan. 16, 2019, 4:52 p.m. | #3
On Wed, Jan 16, 2019 at 3:44 AM Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jan 09, 2019 at 08:35:31PM +0100, Simon Goldschmidt wrote:
>
> > If _debug_uart_putc() is called before _debug_uart_init(), the
> > ns16550 debug uart driver hangs in a tight loop waiting for the
> > tx FIFO to get empty.
> >
> > As this can happen via a printf sneaking in before the port calls
> > debug_uart_init(), introduce a config option to ignore characters
> > before the debug uart is initialized.
> >
> > This is done by reading the baudrate divisor and aborting if is zero.
> >
> > The Kconfig option is required as reading the baudrate divisor does
> > not seem to work for all ns16500 compatibles (which is why the last
> > attempt on this has been reverted in 1a67969a99).
> >
> > Tested on socfpga_cyclone5_socrates.
> >
> > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
>
> Applied to u-boot/master, thanks!

I don't see this or any of my other patches in u-boot/master (neither github
nor denx.de), is that expected?

Regards,
Simon
Tom Rini Jan. 16, 2019, 5:10 p.m. | #4
On Wed, Jan 16, 2019 at 05:52:24PM +0100, Simon Goldschmidt wrote:
> On Wed, Jan 16, 2019 at 3:44 AM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jan 09, 2019 at 08:35:31PM +0100, Simon Goldschmidt wrote:
> >
> > > If _debug_uart_putc() is called before _debug_uart_init(), the
> > > ns16550 debug uart driver hangs in a tight loop waiting for the
> > > tx FIFO to get empty.
> > >
> > > As this can happen via a printf sneaking in before the port calls
> > > debug_uart_init(), introduce a config option to ignore characters
> > > before the debug uart is initialized.
> > >
> > > This is done by reading the baudrate divisor and aborting if is zero.
> > >
> > > The Kconfig option is required as reading the baudrate divisor does
> > > not seem to work for all ns16500 compatibles (which is why the last
> > > attempt on this has been reverted in 1a67969a99).
> > >
> > > Tested on socfpga_cyclone5_socrates.
> > >
> > > Signed-off-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> >
> > Applied to u-boot/master, thanks!
> 
> I don't see this or any of my other patches in u-boot/master (neither github
> nor denx.de), is that expected?

No, that was strange.  While I swore I had pushed, apparently I had not.
Fixed, thanks!

Patch

diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig
index b7ff2960ab..8709bf69c0 100644
--- a/drivers/serial/Kconfig
+++ b/drivers/serial/Kconfig
@@ -446,6 +446,18 @@  config DEBUG_UART_SKIP_INIT
 	  Select this if the UART you want to use for debug output is already
 	  initialized by the time U-Boot starts its execution.
 
+config DEBUG_UART_NS16550_CHECK_ENABLED
+	bool "Check if UART is enabled on output"
+	depends on DEBUG_UART
+	depends on DEBUG_UART_NS16550
+	help
+	  Select this if puts()/putc() might be called before the debug UART
+	  has been initialized. If this is disabled, putc() might sit in a
+	  tight loop if it is called before debug_uart_init() has been called.
+
+	  Note that this does not work for every ns16550-compatible UART and
+	  so has to be enabled carefully or you might notice lost characters.
+
 config ALTERA_JTAG_UART
 	bool "Altera JTAG UART support"
 	depends on DM_SERIAL
diff --git a/drivers/serial/ns16550.c b/drivers/serial/ns16550.c
index 560ca2ae34..6cf2be8f2b 100644
--- a/drivers/serial/ns16550.c
+++ b/drivers/serial/ns16550.c
@@ -272,12 +272,28 @@  static inline void _debug_uart_init(void)
 	serial_dout(&com_port->lcr, UART_LCRVAL);
 }
 
+static inline int NS16550_read_baud_divisor(struct NS16550 *com_port)
+{
+	int ret;
+
+	serial_dout(&com_port->lcr, UART_LCR_BKSE | UART_LCRVAL);
+	ret = serial_din(&com_port->dll) & 0xff;
+	ret |= (serial_din(&com_port->dlm) & 0xff) << 8;
+	serial_dout(&com_port->lcr, UART_LCRVAL);
+
+	return ret;
+}
+
 static inline void _debug_uart_putc(int ch)
 {
 	struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE;
 
-	while (!(serial_din(&com_port->lsr) & UART_LSR_THRE))
-		;
+	while (!(serial_din(&com_port->lsr) & UART_LSR_THRE)) {
+#ifdef CONFIG_DEBUG_UART_NS16550_CHECK_ENABLED
+		if (!NS16550_read_baud_divisor(com_port))
+			return;
+#endif
+	}
 	serial_dout(&com_port->thr, ch);
 }