[U-Boot] [PATCH 1/5] Revert "Add board_pre_console_putc to deal with early console output"

[U-Boot] [PATCH 1/5] Revert "Add board_pre_console_putc to deal with early console output"

[Simon Glass]

This reverts commit 295d3942b806552503243f5cfb36aec6f1b5a9bf.

It turns that this really doesn't work very nicely. Instead we should
have a pre-console panic function so that we know that further execution
is impossible and we don't need to worry about trampling on UARTs, etc.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 README           |   17 -----------------
 common/console.c |   10 +---------
 include/common.h |    7 -------
 3 files changed, 1 insertions(+), 33 deletions(-)

diff --git a/README b/README
index 7adf7c7..84757a5 100644
--- a/README
+++ b/README
@@ -638,23 +638,6 @@ The following options need to be configured:
 		'Sane' compilers will generate smaller code if
 		CONFIG_PRE_CON_BUF_SZ is a power of 2
 
-- Pre-console putc():
-		Prior to the console being initialised, console output is
-		normally silently discarded. This can be annoying if a
-		panic() happens in this time.
-
-		If the CONFIG_PRE_CONSOLE_PUTC option is defined, then
-		U-Boot will call board_pre_console_putc() for each output
-		character in this case, This function should try to output
-		the character if possible, perhaps on all available UARTs
-		(it will need to do this directly, since the console code
-		is not functional yet). Note that if the panic happens
-		early enough, then it is possible that board_init_f()
-		(or even arch_cpu_init() on ARM) has not been called yet.
-		You should init all clocks, GPIOs, etc. that are needed
-		to get the character out. Baud rates will need to default
-		to something sensible.
-
 - Safe printf() functions
 		Define CONFIG_SYS_VSNPRINTF to compile in safe versions of
 		the printf() functions. These are defined in
diff --git a/common/console.c b/common/console.c
index 1d9fd7f..1177f7d 100644
--- a/common/console.c
+++ b/common/console.c
@@ -329,19 +329,14 @@ int tstc(void)
 	return serial_tstc();
 }
 
-#if defined(CONFIG_PRE_CONSOLE_BUFFER) || defined(CONFIG_PRE_CONSOLE_PUTC)
+#ifdef CONFIG_PRE_CONSOLE_BUFFER
 #define CIRC_BUF_IDX(idx) ((idx) % (unsigned long)CONFIG_PRE_CON_BUF_SZ)
 
 static void pre_console_putc(const char c)
 {
-#ifdef CONFIG_PRE_CONSOLE_BUFFER
 	char *buffer = (char *)CONFIG_PRE_CON_BUF_ADDR;
 
 	buffer[CIRC_BUF_IDX(gd->precon_buf_idx++)] = c;
-#endif
-#ifdef CONFIG_PRE_CONSOLE_PUTC
-	board_pre_console_putc(c);
-#endif
 }
 
 static void pre_console_puts(const char *s)
@@ -352,7 +347,6 @@ static void pre_console_puts(const char *s)
 
 static void print_pre_console_buffer(void)
 {
-#ifdef CONFIG_PRE_CONSOLE_BUFFER
 	unsigned long i = 0;
 	char *buffer = (char *)CONFIG_PRE_CON_BUF_ADDR;
 
@@ -361,9 +355,7 @@ static void print_pre_console_buffer(void)
 
 	while (i < gd->precon_buf_idx)
 		putc(buffer[CIRC_BUF_IDX(i++)]);
-#endif
 }
-
 #else
 static inline void pre_console_putc(const char c) {}
 static inline void pre_console_puts(const char *s) {}
diff --git a/include/common.h b/include/common.h
index 59e0b00..b588410 100644
--- a/include/common.h
+++ b/include/common.h
@@ -285,13 +285,6 @@ extern ulong monitor_flash_len;
 int mac_read_from_eeprom(void);
 extern u8 _binary_dt_dtb_start[];	/* embedded device tree blob */
 
-/*
- * Called when console output is requested before the console is available.
- * The board should do its best to get the character out to the user any way
- * it can.
- */
-void board_pre_console_putc(int ch);
-
 /* common/flash.c */
 void flash_perror (int);
 
-- 
1.7.7.3

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Simon Glass]

This patch adds support for console output in the event of a panic() before
the console is inited. The main purpose of this is to deal with a very early
panic() which would otherwise cause a silent hang.

A new board_pre_console_panic() function is added to the board API. If
provided by the board it will be called in the event of a panic before the
console is ready. This function should turn on all available UARTs and
output the string (plus a newline) if it possibly can.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 README           |   34 ++++++++++++++++++++++++++++++++++
 include/common.h |    8 ++++++++
 lib/vsprintf.c   |   25 +++++++++++++++++++++++--
 3 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/README b/README
index 84757a5..38ce52a 100644
--- a/README
+++ b/README
@@ -638,6 +638,40 @@ The following options need to be configured:
 		'Sane' compilers will generate smaller code if
 		CONFIG_PRE_CON_BUF_SZ is a power of 2
 
+- Pre-console panic():
+		Prior to the console being initialised, console output is
+		normally silently discarded. This can be annoying if a
+		panic() happens in this time. One reason for this might be
+		that you have CONFIG_OF_CONTROL defined but have not
+		provided a valid device tree for U-Boot. In general U-Boot's
+		console code cannot resolve this problem, since the console
+		has not been set up, and it may not be known which UART is
+		the console anyway (for example if this information is in
+		the device tree).
+
+		The solution is to define a function called
+		board_pre_console_panic() for your board, which alerts the
+		user however it can. Hopefuly this will involve displaying
+		a message on available UARTs, or perhaps at least flashing
+		an LED. The function should be very careful to only output
+		to UARTs that are known to be unused for peripheral
+		interfaces, and change GPIOs that will have no ill effect
+		on the board. That said, it is fine to do something
+		destructive that will prevent the board from continuing to
+		boot, since it isn't going to...
+
+		The function will need to output serial data directly,
+		since the console code is not functional yet. Note that if
+		the panic happens early enough, then it is possible that
+		board_init_f() (or even arch_cpu_init() on ARM) has not
+		been called yet. You should init all clocks, GPIOs, etc.
+		that are needed to get the string out. Baud rates will need
+		to default to something sensible.
+
+		If your function returns, then the normal panic processing
+		will occur (it will either hang or reset depending on
+		CONFIG_PANIC_HANG).
+
 - Safe printf() functions
 		Define CONFIG_SYS_VSNPRINTF to compile in safe versions of
 		the printf() functions. These are defined in
diff --git a/include/common.h b/include/common.h
index b588410..9db3a6a 100644
--- a/include/common.h
+++ b/include/common.h
@@ -285,6 +285,14 @@ extern ulong monitor_flash_len;
 int mac_read_from_eeprom(void);
 extern u8 _binary_dt_dtb_start[];	/* embedded device tree blob */
 
+/*
+ * Called during a panic() when no console is available. The board should do
+ * its best to get a message to the user any way it can. This function should
+ * return if it can, in which case the system will either hang or reset.
+ * See panic().
+ */
+void board_pre_console_panic(const char *str);
+
 /* common/flash.c */
 void flash_perror (int);
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index e38a4b7..a478ff5ab 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -33,6 +33,9 @@ const char hex_asc[] = "0123456789abcdef";
 #define hex_asc_lo(x)   hex_asc[((x) & 0x0f)]
 #define hex_asc_hi(x)   hex_asc[((x) & 0xf0) >> 4]
 
+DECLARE_GLOBAL_DATA_PTR;
+
+
 static inline char *pack_hex_byte(char *buf, u8 byte)
 {
 	*buf++ = hex_asc_hi(byte);
@@ -777,13 +780,31 @@ int sprintf(char * buf, const char *fmt, ...)
 	return i;
 }
 
+/* Provide a default function for when no console is available */
+static void __board_pre_console_panic(const char *msg)
+{
+	/* Just return since we can't access the console */
+}
+
+void board_pre_console_panic(const char *msg) __attribute__((weak,
+					alias("__board_pre_console_panic")));
+
 void panic(const char *fmt, ...)
 {
+	char str[CONFIG_SYS_PBSIZE];
 	va_list	args;
+
 	va_start(args, fmt);
-	vprintf(fmt, args);
-	putc('\n');
+	vsnprintf(str, sizeof(str), fmt, args);
 	va_end(args);
+
+	if (gd->have_console) {
+		puts(str);
+		putc('\n');
+	} else {
+		board_pre_console_panic(str);
+	}
+
 #if defined (CONFIG_PANIC_HANG)
 	hang();
 #else
-- 
1.7.7.3

[U-Boot] [PATCH 3/5] tegra: Export the UART setup function for use by boards

[Simon Glass]

Allow boards to call the tegra_setup_uarts() function so that they
can set up UARTs on demand. The UART selection enum is moved into the
board.h header file so that boards can use this for pre-console panic.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/board.c        |   26 +++++++-------------------
 arch/arm/include/asm/arch-tegra2/board.h |   17 +++++++++++++++++
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c
index 77a627d..c9a7520 100644
--- a/arch/arm/cpu/armv7/tegra2/board.c
+++ b/arch/arm/cpu/armv7/tegra2/board.c
@@ -24,6 +24,7 @@
 #include <common.h>
 #include <asm/io.h>
 #include "ap20.h"
+#include <asm/arch/board.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/funcmux.h>
 #include <asm/arch/sys_proto.h>
@@ -32,14 +33,6 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
-enum {
-	/* UARTs which we can enable */
-	UARTA	= 1 << 0,
-	UARTB	= 1 << 1,
-	UARTD	= 1 << 3,
-	UART_COUNT = 4,
-};
-
 /*
  * Boot ROM initializes the odmdata in APBDEV_PMC_SCRATCH20_0,
  * so we are using this value to identify memory size.
@@ -101,12 +94,7 @@ int arch_cpu_init(void)
 }
 #endif
 
-/**
- * Set up the specified uarts
- *
- * @param uarts_ids	Mask containing UARTs to init (UARTx)
- */
-static void setup_uarts(int uart_ids)
+void tegra_setup_uarts(int uart_ids)
 {
 	static enum periph_id id_for_uart[] = {
 		PERIPH_ID_UART1,
@@ -116,7 +104,7 @@ static void setup_uarts(int uart_ids)
 	};
 	size_t i;
 
-	for (i = 0; i < UART_COUNT; i++) {
+	for (i = 0; i < TEGRA_UART_COUNT; i++) {
 		if (uart_ids & (1 << i)) {
 			enum periph_id id = id_for_uart[i];
 
@@ -131,15 +119,15 @@ void board_init_uart_f(void)
 	int uart_ids = 0;	/* bit mask of which UART ids to enable */
 
 #ifdef CONFIG_TEGRA2_ENABLE_UARTA
-	uart_ids |= UARTA;
+	uart_ids |= TEGRA_UARTA;
 #endif
 #ifdef CONFIG_TEGRA2_ENABLE_UARTB
-	uart_ids |= UARTB;
+	uart_ids |= TEGRA_UARTB;
 #endif
 #ifdef CONFIG_TEGRA2_ENABLE_UARTD
-	uart_ids |= UARTD;
+	uart_ids |= TEGRA_UARTD;
 #endif
-	setup_uarts(uart_ids);
+	tegra_setup_uarts(uart_ids);
 }
 
 #ifndef CONFIG_SYS_DCACHE_OFF
diff --git a/arch/arm/include/asm/arch-tegra2/board.h b/arch/arm/include/asm/arch-tegra2/board.h
index a90d36c..fb88517 100644
--- a/arch/arm/include/asm/arch-tegra2/board.h
+++ b/arch/arm/include/asm/arch-tegra2/board.h
@@ -24,6 +24,23 @@
 #ifndef _TEGRA_BOARD_H_
 #define _TEGRA_BOARD_H_
 
+enum {
+	/* UARTs which we can enable */
+	TEGRA_UARTA	= 1 << 0,
+	TEGRA_UARTB	= 1 << 1,
+	TEGRA_UARTD	= 1 << 3,
+	TEGRA_UART_ALL	= 0xf,
+
+	TEGRA_UART_COUNT = 4,
+};
+
+/**
+ * Set up the specified UARTs (pinmux and clocks)
+ *
+ * @param uarts_ids	Mask containing UARTs to init (see TEGRA_UARTx)
+ */
+void tegra_setup_uarts(int uart_ids);
+
 /* Setup UARTs for the board according to the selected config */
 void board_init_uart_f(void);
 
-- 
1.7.7.3

[U-Boot] [PATCH 4/5] tegra: Provide tegra_pre_console_panic() for early panics

[Simon Glass]

This function is made available to board which want to display a message
when a panic() occurs before the console is set up. This would otherwise
result in a silent hang or reboot.

Boards should call tegra_pre_console_panic() and pass the UARTs which
are available and safe for a message, as well as the selected clock and
serial multiplier values. Defaults are available as
CONFIG_DEFAULT_NS16550_CLK and CONFIG_DEFAULT_NS16550_MULT.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 arch/arm/cpu/armv7/tegra2/board.c        |   44 ++++++++++++++++++++++++++++++
 arch/arm/include/asm/arch-tegra2/board.h |   13 +++++++++
 include/configs/tegra2-common.h          |    4 +++
 3 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c
index c9a7520..6e76dbe 100644
--- a/arch/arm/cpu/armv7/tegra2/board.c
+++ b/arch/arm/cpu/armv7/tegra2/board.c
@@ -22,6 +22,7 @@
  */
 
 #include <common.h>
+#include <ns16550.h>
 #include <asm/io.h>
 #include "ap20.h"
 #include <asm/arch/board.h>
@@ -137,3 +138,46 @@ void enable_caches(void)
 	dcache_enable();
 }
 #endif
+
+/*
+ * Possible UART locations: we ignore UARTC at 0x70006200 and UARTE at
+ * 0x70006400, since we don't have code to init them
+ */
+static const u32 uart_reg_addr[] = {
+	NV_PA_APB_UARTA_BASE,
+	NV_PA_APB_UARTB_BASE,
+	NV_PA_APB_UARTD_BASE,
+	0
+};
+
+void tegra_pre_console_panic(int uart_ids, unsigned clock_freq,
+			     unsigned multiplier, const char *str)
+{
+	int baudrate, divisor;
+	const u32 *uart_addr;
+
+	/* Enable all permitted UARTs */
+	tegra_setup_uarts(uart_ids);
+
+	/*
+	 * Now send the string out all the selected  UARTs. We don't try all
+	 * possible configurations, but this could be added if required.
+	 */
+	baudrate = CONFIG_BAUDRATE;
+	divisor = (clock_freq + (baudrate * (multiplier / 2))) /
+			(multiplier * baudrate);
+
+	for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {
+		NS16550_t regs = (NS16550_t)*uart_addr;
+		const char *s;
+
+		NS16550_init(regs, divisor);
+		for (s = str; *s; s++) {
+			NS16550_putc(regs, *s);
+			if (*s == '\n')
+				NS16550_putc(regs, '\r');
+		}
+		NS16550_putc(regs, '\n');
+		NS16550_putc(regs, '\r');
+	}
+}
diff --git a/arch/arm/include/asm/arch-tegra2/board.h b/arch/arm/include/asm/arch-tegra2/board.h
index fb88517..fd2489f 100644
--- a/arch/arm/include/asm/arch-tegra2/board.h
+++ b/arch/arm/include/asm/arch-tegra2/board.h
@@ -41,6 +41,19 @@ enum {
  */
 void tegra_setup_uarts(int uart_ids);
 
+/**
+ * Display a panic message on selected UARTs.
+ *
+ * This is called when we have no console yet but have hit a panic(). It
+ * is normally called from board_pre_console_panic(), which passes in the
+ * UARTs that we are permitted to output to.
+ *
+ * We display a message on each UART in the hope that one will reach the
+ * user.
+ */
+void tegra_pre_console_panic(int uart_ids, unsigned clock_freq,
+			     unsigned multiplier, const char *str);
+
 /* Setup UARTs for the board according to the selected config */
 void board_init_uart_f(void);
 
diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2-common.h
index 837f859..14d7602 100644
--- a/include/configs/tegra2-common.h
+++ b/include/configs/tegra2-common.h
@@ -84,6 +84,10 @@
 #define CONFIG_SYS_BAUDRATE_TABLE	{4800, 9600, 19200, 38400, 57600,\
 					115200}
 
+/* Default serial clock and multiplier */
+#define CONFIG_DEFAULT_NS16550_CLK	V_NS16550_CLK
+#define CONFIG_DEFAULT_NS16550_MULT	16
+
 /*
  * This parameter affects a TXFILLTUNING field that controls how much data is
  * sent to the latency fifo before it is sent to the wire. Without this
-- 
1.7.7.3

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Simon Glass]

We enable this feature on all UARTs for Seaboard. This ensures that a
message is printed if CONFIG_OF_CONTROL is in use and a value device tree
is not available.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 board/nvidia/seaboard/seaboard.c |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/seaboard/seaboard.c b/board/nvidia/seaboard/seaboard.c
index 94efb1e..3f69dfc 100644
--- a/board/nvidia/seaboard/seaboard.c
+++ b/board/nvidia/seaboard/seaboard.c
@@ -24,6 +24,7 @@
 #include <common.h>
 #include <asm/io.h>
 #include <asm/arch/tegra2.h>
+#include <asm/arch/board.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/funcmux.h>
 #include <asm/arch/pinmux.h>
@@ -96,3 +97,20 @@ void pin_mux_usb(void)
 	/* For USB's GPIO PD0. For now, since we have no pinmux in fdt */
 	pinmux_tristate_disable(PINGRP_SLXK);
 }
+
+/*
+ * This is called when we have no console. About the only reason that this
+ * happen is if we don't have a valid fdt. So we don't know what kind of
+ * Tegra board we are. We blindly try to print a message every which way we
+ * know.
+ */
+void board_pre_console_panic(const char *str)
+{
+	/* Seaboard has a UART switch on PI3 */
+#ifdef CONFIG_SPI_UART_SWITCH
+	gpio_direction_output(GPIO_PI3, 0);
+#endif
+
+	tegra_pre_console_panic(TEGRA_UART_ALL, CONFIG_DEFAULT_NS16550_CLK,
+				CONFIG_DEFAULT_NS16550_MULT, str);
+}
-- 
1.7.7.3

[U-Boot] [PATCH 4/5] tegra: Provide tegra_pre_console_panic() for early panics

[Stephen Warren]

On 03/19/2012 02:27 PM, Simon Glass wrote:
> This function is made available to board which want to display a message
> when a panic() occurs before the console is set up. This would otherwise
> result in a silent hang or reboot.
> 
> Boards should call tegra_pre_console_panic() and pass the UARTs which
> are available and safe for a message, as well as the selected clock and
> serial multiplier values. Defaults are available as
> CONFIG_DEFAULT_NS16550_CLK and CONFIG_DEFAULT_NS16550_MULT.
...
> diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c
...
> +void tegra_pre_console_panic(int uart_ids, unsigned clock_freq,
> +			     unsigned multiplier, const char *str)
> +{
> +	int baudrate, divisor;
> +	const u32 *uart_addr;
> +
> +	/* Enable all permitted UARTs */
> +	tegra_setup_uarts(uart_ids);

That call honors uart_ids. ...

> +	/*
> +	 * Now send the string out all the selected  UARTs. We don't try all
> +	 * possible configurations, but this could be added if required.
> +	 */
> +	baudrate = CONFIG_BAUDRATE;
> +	divisor = (clock_freq + (baudrate * (multiplier / 2))) /
> +			(multiplier * baudrate);
> +
> +	for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {

Shouldn't this loop also honor uart_ids, and skip sending data to UARTS
not in uart_ids? Sure, they aren't set up above, but perhaps they were
set up elsewhere in the code and hence are capable of sending out data
anyway?

> +		NS16550_t regs = (NS16550_t)*uart_addr;
> +		const char *s;
> +
> +		NS16550_init(regs, divisor);
> +		for (s = str; *s; s++) {
> +			NS16550_putc(regs, *s);
> +			if (*s == '\n')
> +				NS16550_putc(regs, '\r');
> +		}
> +		NS16550_putc(regs, '\n');
> +		NS16550_putc(regs, '\r');
> +	}
> +}

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Stephen Warren]

On 03/19/2012 02:27 PM, Simon Glass wrote:
> We enable this feature on all UARTs for Seaboard. This ensures that a
> message is printed if CONFIG_OF_CONTROL is in use and a value device tree
> is not available.

Why not just enabled this on UARTD, since that's what Seaboard uses?

I guess some derivatives do use UARTB too, which makes things quite
painful. Perhaps at least limit this to UARTB + UARTD, and not all the
others?

[U-Boot] [PATCH 4/5] tegra: Provide tegra_pre_console_panic() for early panics

[Simon Glass]

Hi Stephen,

On Mon, Mar 19, 2012 at 2:16 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/19/2012 02:27 PM, Simon Glass wrote:
>> This function is made available to board which want to display a message
>> when a panic() occurs before the console is set up. This would otherwise
>> result in a silent hang or reboot.
>>
>> Boards should call tegra_pre_console_panic() and pass the UARTs which
>> are available and safe for a message, as well as the selected clock and
>> serial multiplier values. Defaults are available as
>> CONFIG_DEFAULT_NS16550_CLK and CONFIG_DEFAULT_NS16550_MULT.
> ...
>> diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c
> ...
>> +void tegra_pre_console_panic(int uart_ids, unsigned clock_freq,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ?unsigned multiplier, const char *str)
>> +{
>> + ? ? int baudrate, divisor;
>> + ? ? const u32 *uart_addr;
>> +
>> + ? ? /* Enable all permitted UARTs */
>> + ? ? tegra_setup_uarts(uart_ids);
>
> That call honors uart_ids. ...
>
>> + ? ? /*
>> + ? ? ?* Now send the string out all the selected ?UARTs. We don't try all
>> + ? ? ?* possible configurations, but this could be added if required.
>> + ? ? ?*/
>> + ? ? baudrate = CONFIG_BAUDRATE;
>> + ? ? divisor = (clock_freq + (baudrate * (multiplier / 2))) /
>> + ? ? ? ? ? ? ? ? ? ? (multiplier * baudrate);
>> +
>> + ? ? for (uart_addr = uart_reg_addr; *uart_addr; uart_addr++) {
>
> Shouldn't this loop also honor uart_ids, and skip sending data to UARTS
> not in uart_ids? Sure, they aren't set up above, but perhaps they were
> set up elsewhere in the code and hence are capable of sending out data
> anyway?

No, my intent was to use the same value but I overlooked it. I will fix this.

>
>> + ? ? ? ? ? ? NS16550_t regs = (NS16550_t)*uart_addr;
>> + ? ? ? ? ? ? const char *s;
>> +
>> + ? ? ? ? ? ? NS16550_init(regs, divisor);
>> + ? ? ? ? ? ? for (s = str; *s; s++) {
>> + ? ? ? ? ? ? ? ? ? ? NS16550_putc(regs, *s);
>> + ? ? ? ? ? ? ? ? ? ? if (*s == '\n')
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? NS16550_putc(regs, '\r');
>> + ? ? ? ? ? ? }
>> + ? ? ? ? ? ? NS16550_putc(regs, '\n');
>> + ? ? ? ? ? ? NS16550_putc(regs, '\r');
>> + ? ? }
>> +}

Regards,
Simon

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Simon Glass]

Hi Stephen,

On Mon, Mar 19, 2012 at 2:18 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/19/2012 02:27 PM, Simon Glass wrote:
>> We enable this feature on all UARTs for Seaboard. This ensures that a
>> message is printed if CONFIG_OF_CONTROL is in use and a value device tree
>> is not available.
>
> Why not just enabled this on UARTD, since that's what Seaboard uses?
>
> I guess some derivatives do use UARTB too, which makes things quite
> painful. Perhaps at least limit this to UARTB + UARTD, and not all the
> others?

At the moment we can use Seaboard as a generic Tegra2 board, so we
want the widest possible select of UARTs. I think there is one board
that uses A?

Really I would prefer that we explicitly create a generic Tegra2
board, once the fdt stuff is bedded in.

Regards,
Simon

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Stephen Warren]

On 03/19/2012 04:59 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Mar 19, 2012 at 2:18 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 03/19/2012 02:27 PM, Simon Glass wrote:
>>> We enable this feature on all UARTs for Seaboard. This ensures that a
>>> message is printed if CONFIG_OF_CONTROL is in use and a value device tree
>>> is not available.
>>
>> Why not just enabled this on UARTD, since that's what Seaboard uses?
>>
>> I guess some derivatives do use UARTB too, which makes things quite
>> painful. Perhaps at least limit this to UARTB + UARTD, and not all the
>> others?
> 
> At the moment we can use Seaboard as a generic Tegra2 board, so we
> want the widest possible select of UARTs. I think there is one board
> that uses A?
> 
> Really I would prefer that we explicitly create a generic Tegra2
> board, once the fdt stuff is bedded in.

Well, one of Wolfgang's main objections was blasting the panic message
through all possible UARTs, which might send junk to something other
than a debug UART (e.g. machinery and life support systems were
mentioned). This change doesn't seem to solve that. For low-level debug
like this, shouldn't we just route it to one single UART that the config
file selects?

We can certainly think about refactoring things into a unified board
file, but that seems like something unrelated to do later...

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Simon Glass]

Hi Stephen,

On Mon, Mar 19, 2012 at 6:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/19/2012 04:59 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Mar 19, 2012 at 2:18 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 03/19/2012 02:27 PM, Simon Glass wrote:
>>>> We enable this feature on all UARTs for Seaboard. This ensures that a
>>>> message is printed if CONFIG_OF_CONTROL is in use and a value device tree
>>>> is not available.
>>>
>>> Why not just enabled this on UARTD, since that's what Seaboard uses?
>>>
>>> I guess some derivatives do use UARTB too, which makes things quite
>>> painful. Perhaps at least limit this to UARTB + UARTD, and not all the
>>> others?
>>
>> At the moment we can use Seaboard as a generic Tegra2 board, so we
>> want the widest possible select of UARTs. I think there is one board
>> that uses A?
>>
>> Really I would prefer that we explicitly create a generic Tegra2
>> board, once the fdt stuff is bedded in.
>
> Well, one of Wolfgang's main objections was blasting the panic message
> through all possible UARTs, which might send junk to something other
> than a debug UART (e.g. machinery and life support systems were
> mentioned). This change doesn't seem to solve that. For low-level debug
> like this, shouldn't we just route it to one single UART that the config
> file selects?

The objection was that we did it blindly without knowing what ports
were safe to use. Now it is under board control. In the case of a
board where we want the pre-console panic function but only want it on
UARTB we can do that by creating a board file and a config.

The CONFIG cannot select which UART to use, because we only have one
config for all the Seaboard variants, and some use different UARTs.
Only the device tree can tell you which is the console UART. There is
a bit of a conflict here, but keep in mind we are trying to have a
single U-Boot binary - anything that relies on a CONFIG will break
that.

>
> We can certainly think about refactoring things into a unified board
> file, but that seems like something unrelated to do later...

Yes it is. But we use Seaboard as our base board for all the Tegra2
board variants. Some use UARTA, C and one uses D. UART D is a pain
because it is shared with SPI.

So my preference is to leave it as it is, but if you just want it to
be D, then we can go with that for now. At least now it is only a
single line change. Let me know.

Regards,
Simon

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Stephen Warren]

On 03/19/2012 07:31 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Mon, Mar 19, 2012 at 6:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 03/19/2012 04:59 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Mon, Mar 19, 2012 at 2:18 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 03/19/2012 02:27 PM, Simon Glass wrote:
>>>>> We enable this feature on all UARTs for Seaboard. This ensures that a
>>>>> message is printed if CONFIG_OF_CONTROL is in use and a value device tree
>>>>> is not available.
>>>>
>>>> Why not just enabled this on UARTD, since that's what Seaboard uses?
>>>>
>>>> I guess some derivatives do use UARTB too, which makes things quite
>>>> painful. Perhaps at least limit this to UARTB + UARTD, and not all the
>>>> others?
>>>
>>> At the moment we can use Seaboard as a generic Tegra2 board, so we
>>> want the widest possible select of UARTs. I think there is one board
>>> that uses A?
>>>
>>> Really I would prefer that we explicitly create a generic Tegra2
>>> board, once the fdt stuff is bedded in.
>>
>> Well, one of Wolfgang's main objections was blasting the panic message
>> through all possible UARTs, which might send junk to something other
>> than a debug UART (e.g. machinery and life support systems were
>> mentioned). This change doesn't seem to solve that. For low-level debug
>> like this, shouldn't we just route it to one single UART that the config
>> file selects?
> 
> The objection was that we did it blindly without knowing what ports
> were safe to use. Now it is under board control. In the case of a
> board where we want the pre-console panic function but only want it on
> UARTB we can do that by creating a board file and a config.
> 
> The CONFIG cannot select which UART to use, because we only have one
> config for all the Seaboard variants, and some use different UARTs.
> Only the device tree can tell you which is the console UART. There is
> a bit of a conflict here, but keep in mind we are trying to have a
> single U-Boot binary - anything that relies on a CONFIG will break
> that.

I don't believe there's any specific requirement or decision to have a
single U-Boot binary. Who has decided that and where is the discussion?

Having a single set of sources seems like quite a large step for a
bootloader, and perhaps can be achieved with DT. Building a binary for
each specific debug UART you need (and potentially for many other
things) seems entirely reasonable.

>> We can certainly think about refactoring things into a unified board
>> file, but that seems like something unrelated to do later...
> 
> Yes it is. But we use Seaboard as our base board for all the Tegra2
> board variants. Some use UARTA, C and one uses D. UART D is a pain
> because it is shared with SPI.
> 
> So my preference is to leave it as it is, but if you just want it to
> be D, then we can go with that for now. At least now it is only a
> single line change. Let me know.

That seems safest for now, especially considering that only baseline
Seaboard is actually supported in mainline U-Boot.

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Simon Glass]

Hi Stephen,

On Mon, Mar 19, 2012 at 6:46 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/19/2012 07:31 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Mon, Mar 19, 2012 at 6:22 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 03/19/2012 04:59 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Mon, Mar 19, 2012 at 2:18 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> On 03/19/2012 02:27 PM, Simon Glass wrote:
>>>>>> We enable this feature on all UARTs for Seaboard. This ensures that a
>>>>>> message is printed if CONFIG_OF_CONTROL is in use and a value device tree
>>>>>> is not available.
>>>>>
>>>>> Why not just enabled this on UARTD, since that's what Seaboard uses?
>>>>>
>>>>> I guess some derivatives do use UARTB too, which makes things quite
>>>>> painful. Perhaps at least limit this to UARTB + UARTD, and not all the
>>>>> others?
>>>>
>>>> At the moment we can use Seaboard as a generic Tegra2 board, so we
>>>> want the widest possible select of UARTs. I think there is one board
>>>> that uses A?
>>>>
>>>> Really I would prefer that we explicitly create a generic Tegra2
>>>> board, once the fdt stuff is bedded in.
>>>
>>> Well, one of Wolfgang's main objections was blasting the panic message
>>> through all possible UARTs, which might send junk to something other
>>> than a debug UART (e.g. machinery and life support systems were
>>> mentioned). This change doesn't seem to solve that. For low-level debug
>>> like this, shouldn't we just route it to one single UART that the config
>>> file selects?
>>
>> The objection was that we did it blindly without knowing what ports
>> were safe to use. Now it is under board control. In the case of a
>> board where we want the pre-console panic function but only want it on
>> UARTB we can do that by creating a board file and a config.
>>
>> The CONFIG cannot select which UART to use, because we only have one
>> config for all the Seaboard variants, and some use different UARTs.
>> Only the device tree can tell you which is the console UART. There is
>> a bit of a conflict here, but keep in mind we are trying to have a
>> single U-Boot binary - anything that relies on a CONFIG will break
>> that.
>
> I don't believe there's any specific requirement or decision to have a
> single U-Boot binary. Who has decided that and where is the discussion?

That is the whole point of the device tree effort :-)

>
> Having a single set of sources seems like quite a large step for a
> bootloader, and perhaps can be achieved with DT. Building a binary for
> each specific debug UART you need (and potentially for many other
> things) seems entirely reasonable.

That's up to you of course. Our intent is to have a single binary for
each SOC, with the device tree providing all required config.

>
>>> We can certainly think about refactoring things into a unified board
>>> file, but that seems like something unrelated to do later...
>>
>> Yes it is. But we use Seaboard as our base board for all the Tegra2
>> board variants. Some use UARTA, C and one uses D. UART D is a pain
>> because it is shared with SPI.
>>
>> So my preference is to leave it as it is, but if you just want it to
>> be D, then we can go with that for now. At least now it is only a
>> single line change. Let me know.
>
> That seems safest for now, especially considering that only baseline
> Seaboard is actually supported in mainline U-Boot.

You would be surprised. With a device tree I can boot mainline
'seaboard' U-Boot on a few things...

I will update the patch and will see if Wolfgang is comfortable with
this new panic mechanism also.

Regards,
Simon

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Graeme Russ]

Hi Simon,

On Tue, Mar 20, 2012 at 7:27 AM, Simon Glass <sjg@chromium.org> wrote:
> This patch adds support for console output in the event of a panic() before
> the console is inited. The main purpose of this is to deal with a very early
> panic() which would otherwise cause a silent hang.
>
> A new board_pre_console_panic() function is added to the board API. If
> provided by the board it will be called in the event of a panic before the
> console is ready. This function should turn on all available UARTs and
> output the string (plus a newline) if it possibly can.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---

> ?void panic(const char *fmt, ...)
> ?{
> + ? ? ? char str[CONFIG_SYS_PBSIZE];
> ? ? ? ?va_list args;
> +
> ? ? ? ?va_start(args, fmt);
> - ? ? ? vprintf(fmt, args);
> - ? ? ? putc('\n');
> + ? ? ? vsnprintf(str, sizeof(str), fmt, args);
> ? ? ? ?va_end(args);
> +
> + ? ? ? if (gd->have_console) {
> + ? ? ? ? ? ? ? puts(str);
> + ? ? ? ? ? ? ? putc('\n');
> + ? ? ? } else {
> + ? ? ? ? ? ? ? board_pre_console_panic(str);
> + ? ? ? }
> +

Would there be benefit in including an option to dump the pre-console
buffer (if one is enabled) - I think it's contents could be rather useful
in debugging what went wrong...

And something is nagging at me that the API is just 'not right'. I don't
know exactly why, but I really don't like how this is plumbed

panic() calls putc() which, if we do not have a console (and we won't in
the case of the early panic case we are dealing with), will be put into
the pre console buffer (if enabled)

So having panic() call a board specific function to dump the pre console
buffer after the  vprintf() / putc() would achieve the same result (but
you need pre console buffer enabled)

So, if CONFIG_PRE_CONSOLE_BUFFER is defined, we could simply add a call to
dump_pre_console_buffer() at the end of panic(). But we already have a
function to do that - print_pre_console_buffer(). If we added an argument
to print_pre_console_buffer() which is a pointer to a 'putc()' type
function (NULL meaning the standard putc()) and that function lived in the
board files then life would be good. We could also add a call to a setup
function so we are not setting up the UARTS every putc (not that
performance is a priority at this stage, but some boards might quibble
about hitting certain registers continuously)

But what to do if pre console buffer is not defined? The panic message
needs to be sent directly to the 'panic UARTS'...

OK, so what about in panic():
 - If gd->have_console is not set:
    o call the board specific setup_panic_uarts()
    o call print_pre_console_buffer() passing panic_putc()
    o call panic_putc() for all characters in str[]
 - If gd->have_console is set:
    o call putc() for all characters in str[]

setup_panic_uarts() and panic_putc() are overriden in the board files

Regards,

Graeme

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Simon Glass]

Hi Graeme,

On Tue, Mar 20, 2012 at 3:26 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On Tue, Mar 20, 2012 at 7:27 AM, Simon Glass <sjg@chromium.org> wrote:
>> This patch adds support for console output in the event of a panic() before
>> the console is inited. The main purpose of this is to deal with a very early
>> panic() which would otherwise cause a silent hang.
>>
>> A new board_pre_console_panic() function is added to the board API. If
>> provided by the board it will be called in the event of a panic before the
>> console is ready. This function should turn on all available UARTs and
>> output the string (plus a newline) if it possibly can.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>
>> ?void panic(const char *fmt, ...)
>> ?{
>> + ? ? ? char str[CONFIG_SYS_PBSIZE];
>> ? ? ? ?va_list args;
>> +
>> ? ? ? ?va_start(args, fmt);
>> - ? ? ? vprintf(fmt, args);
>> - ? ? ? putc('\n');
>> + ? ? ? vsnprintf(str, sizeof(str), fmt, args);
>> ? ? ? ?va_end(args);
>> +
>> + ? ? ? if (gd->have_console) {
>> + ? ? ? ? ? ? ? puts(str);
>> + ? ? ? ? ? ? ? putc('\n');
>> + ? ? ? } else {
>> + ? ? ? ? ? ? ? board_pre_console_panic(str);
>> + ? ? ? }
>> +
>
> Would there be benefit in including an option to dump the pre-console
> buffer (if one is enabled) - I think it's contents could be rather useful
> in debugging what went wrong...
>
> And something is nagging at me that the API is just 'not right'. I don't
> know exactly why, but I really don't like how this is plumbed
>
> panic() calls putc() which, if we do not have a console (and we won't in
> the case of the early panic case we are dealing with), will be put into
> the pre console buffer (if enabled)
>
> So having panic() call a board specific function to dump the pre console
> buffer after the ?vprintf() / putc() would achieve the same result (but
> you need pre console buffer enabled)
>
> So, if CONFIG_PRE_CONSOLE_BUFFER is defined, we could simply add a call to
> dump_pre_console_buffer() at the end of panic(). But we already have a
> function to do that - print_pre_console_buffer(). If we added an argument
> to print_pre_console_buffer() which is a pointer to a 'putc()' type
> function (NULL meaning the standard putc()) and that function lived in the
> board files then life would be good. We could also add a call to a setup
> function so we are not setting up the UARTS every putc (not that
> performance is a priority at this stage, but some boards might quibble
> about hitting certain registers continuously)
>
> But what to do if pre console buffer is not defined? The panic message
> needs to be sent directly to the 'panic UARTS'...
>
> OK, so what about in panic():
> ?- If gd->have_console is not set:
> ? ?o call the board specific setup_panic_uarts()
> ? ?o call print_pre_console_buffer() passing panic_putc()
> ? ?o call panic_putc() for all characters in str[]
> ?- If gd->have_console is set:
> ? ?o call putc() for all characters in str[]
>
> setup_panic_uarts() and panic_putc() are overriden in the board files

I think this is where we got to last time.

The act of calling this pre-console panic function is destructive - it
may hang the board and output data to UARTs.

I think I understand the scheme you propose. But setup_panic_uarts()
puts the board into a funny state (e.g. may set up or change clocks
and pinmux). You then need a board_pre_console_putc() to actually
output the characters - you don't mention that. That was the patch
that I reverted :-( Yes I understand that you can separate out the
UART init from the part that outputs the characters, but does that
really help?

To put it another way, I think we need to stick with the idea that
this is a panic, not a normal situation. That means that return from
the board panic output function may be difficult or impossible, and so
a putc() arrangement is not a good idea.

For example, I have another board on which there are two possible
oscillator clock settings - and we don't know which to use, and the
arch clock code has not yet been called! In that case I want the
board_pre_console_panic() function to output the string using both
options, so that one will produce a message for the user (the other
will likely produce just a few characters of garbage because the baud
rate is wrong). If we output only a single character then the garbage
and good characters will be interspersed.

So, can we get away from the idea that this is a reliable and stable
way of outputting characters before the console is ready? If you want
the pre-console output, I'm sure we can provide a way of accessing it
which the board pre-console panic function can use.

Regards,
Simon

>
> Regards,
>
> Graeme

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Graeme Russ]

Hi Simon,

On Wed, Mar 21, 2012 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Graeme,
>
> On Tue, Mar 20, 2012 at 3:26 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Simon,
>>

>>
>> OK, so what about in panic():
>> ?- If gd->have_console is not set:
>> ? ?o call the board specific setup_panic_uarts()
>> ? ?o call print_pre_console_buffer() passing panic_putc()
>> ? ?o call panic_putc() for all characters in str[]
>> ?- If gd->have_console is set:
>> ? ?o call putc() for all characters in str[]
>>
>> setup_panic_uarts() and panic_putc() are overriden in the board files
>
> I think this is where we got to last time.
>
> The act of calling this pre-console panic function is destructive - it
> may hang the board and output data to UARTs.
>
> I think I understand the scheme you propose. But setup_panic_uarts()
> puts the board into a funny state (e.g. may set up or change clocks
> and pinmux). You then need a board_pre_console_putc() to actually
> output the characters - you don't mention that. That was the patch

That's panic_putc()

> that I reverted :-( Yes I understand that you can separate out the
> UART init from the part that outputs the characters, but does that
> really help?
>
> To put it another way, I think we need to stick with the idea that
> this is a panic, not a normal situation. That means that return from

Agreed

> the board panic output function may be difficult or impossible, and so
> a putc() arrangement is not a good idea.
>
> For example, I have another board on which there are two possible
> oscillator clock settings - and we don't know which to use, and the
> arch clock code has not yet been called! In that case I want the
> board_pre_console_panic() function to output the string using both
> options, so that one will produce a message for the user (the other
> will likely produce just a few characters of garbage because the baud
> rate is wrong). If we output only a single character then the garbage
> and good characters will be interspersed.

Ouch!

> So, can we get away from the idea that this is a reliable and stable
> way of outputting characters before the console is ready? If you want
> the pre-console output, I'm sure we can provide a way of accessing it
> which the board pre-console panic function can use.

OK, add a function to 'normalise' the pre console buffer by moving the
characters so the string starts at the beginning of the buffer and then
add an extra format tot he start of the panic string :) - But now the
panic buffer needs to be bigger :(

OK, maybe leave it up to the board code to dump the pre-console buffer...

I dunno - It all seems a bit 'wrong' somehow

Regards,

Graeme

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Simon Glass]

Hi Graeme,

On Tue, Mar 20, 2012 at 4:43 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Mar 21, 2012 at 10:22 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Graeme,
>>
>> On Tue, Mar 20, 2012 at 3:26 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>>> Hi Simon,
>>>
>
>>>
>>> OK, so what about in panic():
>>> ?- If gd->have_console is not set:
>>> ? ?o call the board specific setup_panic_uarts()
>>> ? ?o call print_pre_console_buffer() passing panic_putc()
>>> ? ?o call panic_putc() for all characters in str[]
>>> ?- If gd->have_console is set:
>>> ? ?o call putc() for all characters in str[]
>>>
>>> setup_panic_uarts() and panic_putc() are overriden in the board files
>>
>> I think this is where we got to last time.
>>
>> The act of calling this pre-console panic function is destructive - it
>> may hang the board and output data to UARTs.
>>
>> I think I understand the scheme you propose. But setup_panic_uarts()
>> puts the board into a funny state (e.g. may set up or change clocks
>> and pinmux). You then need a board_pre_console_putc() to actually
>> output the characters - you don't mention that. That was the patch
>
> That's panic_putc()
>
>> that I reverted :-( Yes I understand that you can separate out the
>> UART init from the part that outputs the characters, but does that
>> really help?
>>
>> To put it another way, I think we need to stick with the idea that
>> this is a panic, not a normal situation. That means that return from
>
> Agreed
>
>> the board panic output function may be difficult or impossible, and so
>> a putc() arrangement is not a good idea.
>>
>> For example, I have another board on which there are two possible
>> oscillator clock settings - and we don't know which to use, and the
>> arch clock code has not yet been called! In that case I want the
>> board_pre_console_panic() function to output the string using both
>> options, so that one will produce a message for the user (the other
>> will likely produce just a few characters of garbage because the baud
>> rate is wrong). If we output only a single character then the garbage
>> and good characters will be interspersed.
>
> Ouch!

Indeed.

>
>> So, can we get away from the idea that this is a reliable and stable
>> way of outputting characters before the console is ready? If you want
>> the pre-console output, I'm sure we can provide a way of accessing it
>> which the board pre-console panic function can use.
>
> OK, add a function to 'normalise' the pre console buffer by moving the
> characters so the string starts at the beginning of the buffer and then
> add an extra format tot he start of the panic string :) - But now the
> panic buffer needs to be bigger :(
>
> OK, maybe leave it up to the board code to dump the pre-console buffer...

Yes I think it is simpler for now, until we have a better framework.
Both the kernel and U-Boot need early access to information that
either might not arrive, or will only arrive later. This business and
the current hacks in the zImage wrapper are examples of problems that
need solving properly IMO. I am looking at these problems for the
U-Boot SPL case, so at some point this will get nicer in U-Boot. Not
sure about the kernel yet, but hope to avoid zImage for our next
iteration partly because of this nastiness.

>
> I dunno - It all seems a bit 'wrong' somehow

I think you are looking for a unified panic architecture with a
board-specific putc(). That was my previous patch and I just don't
think it works. This new one does solve the problem, avoids Wolfgangs
concerns about dangerous UART output, and is pretty simple to
implement. I believe there is a better way, but we are in the very
early days with device tree, life goes on, and this does work...

Regards,
Simon

>
> Regards,
>
> Graeme

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Stephen Warren]

On 03/20/2012 05:22 PM, Simon Glass wrote:
> On Tue, Mar 20, 2012 at 3:26 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
...
>> OK, so what about in panic():
>>  - If gd->have_console is not set:
>>    o call the board specific setup_panic_uarts()
>>    o call print_pre_console_buffer() passing panic_putc()
>>    o call panic_putc() for all characters in str[]
>>  - If gd->have_console is set:
>>    o call putc() for all characters in str[]
>>
>> setup_panic_uarts() and panic_putc() are overriden in the board files
> 
> I think this is where we got to last time.
> 
> The act of calling this pre-console panic function is destructive - it
> may hang the board and output data to UARTs.

Why would it hang? Well, I assume you're talking about hanging before
actually emitting the panic text, rather than looping afterwards as a
deliberate choice. I'd consider an accidental hang that prevented the
message being seen as a bug.

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Simon Glass]

Hi Stephen,

On Tue, Mar 20, 2012 at 5:34 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/20/2012 05:22 PM, Simon Glass wrote:
>> On Tue, Mar 20, 2012 at 3:26 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> ...
>>> OK, so what about in panic():
>>> ?- If gd->have_console is not set:
>>> ? ?o call the board specific setup_panic_uarts()
>>> ? ?o call print_pre_console_buffer() passing panic_putc()
>>> ? ?o call panic_putc() for all characters in str[]
>>> ?- If gd->have_console is set:
>>> ? ?o call putc() for all characters in str[]
>>>
>>> setup_panic_uarts() and panic_putc() are overriden in the board files
>>
>> I think this is where we got to last time.
>>
>> The act of calling this pre-console panic function is destructive - it
>> may hang the board and output data to UARTs.
>
> Why would it hang? Well, I assume you're talking about hanging before
> actually emitting the panic text, rather than looping afterwards as a
> deliberate choice. I'd consider an accidental hang that prevented the
> message being seen as a bug.

No I would hope it would output the data first. I was merely pointing
out that the board function may decide to hang rather than return
(although I feel it is safe to return since it is being called from
panic() anyway).

Regards,
Simon

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Wolfgang Denk]

Dear Simon Glass,

In message <1332188824-5447-2-git-send-email-sjg@chromium.org> you wrote:
> This patch adds support for console output in the event of a panic() before
> the console is inited. The main purpose of this is to deal with a very early
> panic() which would otherwise cause a silent hang.
> 
> A new board_pre_console_panic() function is added to the board API. If
> provided by the board it will be called in the event of a panic before the
> console is ready. This function should turn on all available UARTs and
> output the string (plus a newline) if it possibly can.

In addition to the more constructive comments made already by Grame, I
object against this patch because I dislike the whole concept.

> +- Pre-console panic():
> +		Prior to the console being initialised, console output is
> +		normally silently discarded. This can be annoying if a
> +		panic() happens in this time. One reason for this might be

True.  This is the reason why it has always been an important design
rule for U-Boot to initialize the console port as soon as possible.

> +		that you have CONFIG_OF_CONTROL defined but have not
> +		provided a valid device tree for U-Boot. In general U-Boot's
> +		console code cannot resolve this problem, since the console
> +		has not been set up, and it may not be known which UART is
> +		the console anyway (for example if this information is in
> +		the device tree).

Please make up your mind:  either you CAN initialize the console, then
you can use it to output messages in a regular way, or you CANNOT
initialize it, in which case you cannot print anything.  There is no
third option.

> +		The solution is to define a function called
> +		board_pre_console_panic() for your board, which alerts the
> +		user however it can. Hopefuly this will involve displaying
> +		a message on available UARTs, or perhaps at least flashing

If you have "available UARTs", you could use this as console, right?

In the previous discussion you explained the situation differently -
you were talking about _any_ UARTs that might be present on the
hardware, without caring about twhat these might actually be used for.

I explained that such an approach is highly dangerous.  I do not want
you toi set a precedent for such stle and code.

> +		an LED. The function should be very careful to only output
> +		to UARTs that are known to be unused for peripheral
> +		interfaces, and change GPIOs that will have no ill effect
> +		on the board. That said, it is fine to do something
> +		destructive that will prevent the board from continuing to
> +		boot, since it isn't going to...

Sorry, but this is bogus. Either you know the console port, or you
don't.  If there is a free UART, it might be attached to custome
hardware you have no idea about.  Ditto for GPIOs.  Please do not
meddle with device state in arbitrary ways.  If there are LED ports on
that board that are intended to signalize status information then it
makes sense to use these - but do not use any other ports that might
be present on the hardware.

> +		The function will need to output serial data directly,
> +		since the console code is not functional yet. Note that if

This is broken design.  If you can initialize the UART as console,
then doi so and use it in the regular way.

> +		the panic happens early enough, then it is possible that
> +		board_init_f() (or even arch_cpu_init() on ARM) has not
> +		been called yet. You should init all clocks, GPIOs, etc.
> +		that are needed to get the string out. Baud rates will need
> +		to default to something sensible.

Again, this is broken design.  We cannot try to catch errors sooner
and sooner and soner, and if needed initialization steps have not been
executed yet, provide additional code for emergency initialization.
We have regular console code, and now we have pre_console_*() stuff,
and soon we will have pre_pre_pre_pre_pre_pre_pre_pre_console_*()
stuff ? NAK.

Just stick to the design principles, and make sure there is as few
stuff that could possibly go wrong before console initialization as
possible.  Than all this crap (excuse me, but I don;t find a better
word) will not be needed.


I also dislike the modifications to the common code.


I think you are trying to solve an unsolvable problem.  If you cannot
accept that the board gets stuck without printing anything on the
debug console port because there are situations when you don't know
which port this is, then you simply should _define_ and _document_ a
single port as debug console port.  Then initialize and use this in
the regular way.  If later information (like from a loaded device
tree) means the console gets switched to anothe rport, that's OK.
That's what Linux will do as well if you assign another console port
there.


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
Gods don't like people not doing much work. People  who  aren't  busy
all the time might start to _think_.  - Terry Pratchett, _Small Gods_

[U-Boot] [PATCH 3/5] tegra: Export the UART setup function for use by boards

[Wolfgang Denk]

Dear Simon Glass,

In message <1332188824-5447-3-git-send-email-sjg@chromium.org> you wrote:
> Allow boards to call the tegra_setup_uarts() function so that they
> can set up UARTs on demand. The UART selection enum is moved into the
> board.h header file so that boards can use this for pre-console panic.

I dislike this.  This is broken by design.

Even if the could would work today, it would break as soon as we
switch to using a clean device model.

Don't do that, you are opening a can of worms that cannot be closed
easily, and you don't do yourself a favour with it.

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
Committee, n.:  A group of men who individually can do nothing but as
a group decide that nothing can be done.                 - Fred Allen

[U-Boot] [PATCH 5/5] tegra: Implement board_pre_console_panic() for Seaboard

[Wolfgang Denk]

Dear Simon Glass,

In message <1332188824-5447-5-git-send-email-sjg@chromium.org> you wrote:
> We enable this feature on all UARTs for Seaboard. This ensures that a
> message is printed if CONFIG_OF_CONTROL is in use and a value device tree
> is not available.

As explained before, I consider this concept broken.  either you know
which port(s) can be used as console (then use these, and this here is
not needed), or you don't know this (then don't mess with these
ports).

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
A stone was placed at a ford in a river with the inscription:
"When this stone is covered it is dangerous to ford here."

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Simon Glass]

Hi Wolfgang,

On Wed, Mar 21, 2012 at 2:02 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1332188824-5447-2-git-send-email-sjg@chromium.org> you wrote:
>> This patch adds support for console output in the event of a panic() before
>> the console is inited. The main purpose of this is to deal with a very early
>> panic() which would otherwise cause a silent hang.
>>
>> A new board_pre_console_panic() function is added to the board API. If
>> provided by the board it will be called in the event of a panic before the
>> console is ready. This function should turn on all available UARTs and
>> output the string (plus a newline) if it possibly can.
>
> In addition to the more constructive comments made already by Grame, I
> object against this patch because I dislike the whole concept.

Well I don't disagree :-)

>
>> +- Pre-console panic():
>> + ? ? ? ? ? ? Prior to the console being initialised, console output is
>> + ? ? ? ? ? ? normally silently discarded. This can be annoying if a
>> + ? ? ? ? ? ? panic() happens in this time. One reason for this might be
>
> True. ?This is the reason why it has always been an important design
> rule for U-Boot to initialize the console port as soon as possible.
>
>> + ? ? ? ? ? ? that you have CONFIG_OF_CONTROL defined but have not
>> + ? ? ? ? ? ? provided a valid device tree for U-Boot. In general U-Boot's
>> + ? ? ? ? ? ? console code cannot resolve this problem, since the console
>> + ? ? ? ? ? ? has not been set up, and it may not be known which UART is
>> + ? ? ? ? ? ? the console anyway (for example if this information is in
>> + ? ? ? ? ? ? the device tree).
>
> Please make up your mind: ?either you CAN initialize the console, then
> you can use it to output messages in a regular way, or you CANNOT
> initialize it, in which case you cannot print anything. ?There is no
> third option.

Well that is very clear - we cannot. We hit panic() before
console_ready() is called.

>
>> + ? ? ? ? ? ? The solution is to define a function called
>> + ? ? ? ? ? ? board_pre_console_panic() for your board, which alerts the
>> + ? ? ? ? ? ? user however it can. Hopefuly this will involve displaying
>> + ? ? ? ? ? ? a message on available UARTs, or perhaps at least flashing
>
> If you have "available UARTs", you could use this as console, right?

Yes, if we knew which it was.

>
> In the previous discussion you explained the situation differently -
> you were talking about _any_ UARTs that might be present on the
> hardware, without caring about twhat these might actually be used for.

Yes, basically the only difference with this series is that the board
file can control what UARTs are used.

>
> I explained that such an approach is highly dangerous. ?I do not want
> you toi set a precedent for such stle and code.

OK

>
>> + ? ? ? ? ? ? an LED. The function should be very careful to only output
>> + ? ? ? ? ? ? to UARTs that are known to be unused for peripheral
>> + ? ? ? ? ? ? interfaces, and change GPIOs that will have no ill effect
>> + ? ? ? ? ? ? on the board. That said, it is fine to do something
>> + ? ? ? ? ? ? destructive that will prevent the board from continuing to
>> + ? ? ? ? ? ? boot, since it isn't going to...
>
> Sorry, but this is bogus. Either you know the console port, or you
> don't. ?If there is a free UART, it might be attached to custome
> hardware you have no idea about. ?Ditto for GPIOs. ?Please do not
> meddle with device state in arbitrary ways. ?If there are LED ports on
> that board that are intended to signalize status information then it
> makes sense to use these - but do not use any other ports that might
> be present on the hardware.

Yes that is true. The board file should know what is safe, but when we
share board files among different hardware variants, we have exactly
this problem.

>
>> + ? ? ? ? ? ? The function will need to output serial data directly,
>> + ? ? ? ? ? ? since the console code is not functional yet. Note that if
>
> This is broken design. ?If you can initialize the UART as console,
> then doi so and use it in the regular way.
>
>> + ? ? ? ? ? ? the panic happens early enough, then it is possible that
>> + ? ? ? ? ? ? board_init_f() (or even arch_cpu_init() on ARM) has not
>> + ? ? ? ? ? ? been called yet. You should init all clocks, GPIOs, etc.
>> + ? ? ? ? ? ? that are needed to get the string out. Baud rates will need
>> + ? ? ? ? ? ? to default to something sensible.
>
> Again, this is broken design. ?We cannot try to catch errors sooner
> and sooner and soner, and if needed initialization steps have not been
> executed yet, provide additional code for emergency initialization.
> We have regular console code, and now we have pre_console_*() stuff,
> and soon we will have pre_pre_pre_pre_pre_pre_pre_pre_console_*()
> stuff ? NAK.
>
> Just stick to the design principles, and make sure there is as few
> stuff that could possibly go wrong before console initialization as
> possible. ?Than all this crap (excuse me, but I don;t find a better
> word) will not be needed.

Fair enough, and agreed. Thanks for looking at this and providing this info.

We know we won't get to console init in this case - there is a panic()
that happens first. So the existing pre-console putc() becomes our
route to displaying a message. The problem with that is only that the
board init hasn't happened yet, so either the pre-console putc() must
init the UARTs or we need a separate init function.

>
>
> I also dislike the modifications to the common code.
>
>
> I think you are trying to solve an unsolvable problem. ?If you cannot
> accept that the board gets stuck without printing anything on the
> debug console port because there are situations when you don't know
> which port this is, then you simply should _define_ and _document_ a
> single port as debug console port. ?Then initialize and use this in
> the regular way. ?If later information (like from a loaded device
> tree) means the console gets switched to anothe rport, that's OK.
> That's what Linux will do as well if you assign another console port
> there.

Yes we are certainly trying to solve an unsolvable problem. There are
some things that will result in a bricked board and we can't solve all
of them. I would be very happy to just accept that. We have the
pre-console stuff which boards can use to do their best, and that
should be good enough for those that care enough. I actually prefer a
pre-console panic to a pre-console putc(), for reasons I went into at
length, but no matter, it's not that important.

So the existing pre-console putc() can be used, if we can sort out how
to make UART init work. Graeme suggested an approach here - I will see
if I can make that work.

Regards,
Simon

>
>
> 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
> Gods don't like people not doing much work. People ?who ?aren't ?busy
> all the time might start to _think_. ?- Terry Pratchett, _Small Gods_

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ1TBN+Ro8f0nDubAYRi54UAV0LxjrdFz-AW5qBgue1+Fw@mail.gmail.com> you wrote:
> 
> > Please make up your mind:  either you CAN initialize the console, then
> > you can use it to output messages in a regular way, or you CANNOT
> > initialize it, in which case you cannot print anything.  There is no
> > third option.
>
> Well that is very clear - we cannot. We hit panic() before
> console_ready() is called.

OK - but this means that no output to a serial console port can be
done, no matter what you try.

> > If you have "available UARTs", you could use this as console, right?
>
> Yes, if we knew which it was.

This is a design issue.  If there are several similar boards that
shall be handled with the same binary, then you must agree on some
common sub-set of features, like on UART port that is available on all
of these, and use this as (early) console port.

> We know we won't get to console init in this case - there is a panic()
> that happens first. So the existing pre-console putc() becomes our
> route to displaying a message. The problem with that is only that the

No.  When you cannot initialize the console, you cannot output
anything to the console. Period.

If there is a way to do some initialization and output charatcers,
than make this your regular console init code, and use it always,
without any special warts.

> board init hasn't happened yet, so either the pre-console putc() must
> init the UARTs or we need a separate init function.

This makes no sense to me.

> So the existing pre-console putc() can be used, if we can sort out how
> to make UART init work. Graeme suggested an approach here - I will see
> if I can make that work.

I don't think I will accept any "pre-console putc()".  This is such a
dirty hack.  Either you can initialize and use putc, then use the
normal console mechanism for it, or you cannot. And "cannot" means
"cannot" here.

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
I have the simplest tastes.  I am always satisfied with the best.
                                                       -- Oscar Wilde

[U-Boot] [PATCH 2/5] Add board_panic_no_console() to deal with early critical errors

[Wolfgang Denk]

Dear Simon,

In message <20120321224801.2C000202A4D@gemini.denx.de> I wrote:
> 
> > So the existing pre-console putc() can be used, if we can sort out how
> > to make UART init work. Graeme suggested an approach here - I will see
> > if I can make that work.
> 
> I don't think I will accept any "pre-console putc()".  This is such a
> dirty hack.  Either you can initialize and use putc, then use the
> normal console mechanism for it, or you cannot. And "cannot" means
> "cannot" here.

Sorry, this was not what I meant.  My fingers were faster than my
brain.  The existing code around CONFIG_PRE_CONSOLE_BUFFER (i. e.
storage in a temporary buffer until console bcomes available) is of
course OK with me.

What I meant was: I do not want to have any "pre console UART output"
code.

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
    \|/ ____ \|/                                     \|/ ____ \|/
     @~/ ,. \~@                                       @~/ ,. \~@
    /_( \__/ )_\                                     /_( \__/ )_\
       \__U_/                                           \__U_/