[U-Boot] [PATCH] [RFC] Early serial debug console

[U-Boot] [PATCH] [RFC] Early serial debug console

[Peter Tyser]

The following 3 patches enable a basic serial console while U-Boot is still
executing out of flash.  When enabled, the user is dropped to the debug
console when an error occurs in a function in the init_sequence[].  The
user can also drop to a debug console by pressing ctrl-c during bootup.
Many commands do not work since interrupts aren't enabled (no sleeping),
the bss segment hasn't been zeroed out, and the data section can't be
written to.  The i2c and memory commands were updated to work while
executing from flash to aide in programming SPD's and debugging
SDRAM issues.

The code has been tested on an mpc8548 and mpc8572 SBC.  An example session
is pasted below where the SPD has been corrupted.

These patches assume that the previous 2 patches sent out ("mod_i2c_mem()
bugfix" and "Replace references to extern console_buffer with a function call"
have been applied.

Does anyone have suggestions/comments?  Any chance this feature could make
it into mainline U-Boot?

Thanks for any criticism/comments,
Peter



Sample output with corrupted SPD:

U-Boot 1.3.4-00113-g8b3bc07 (Aug 15 2008 - 14:26:37)

CPU:   8572E, Version: 1.0, (0x80e80010)
Core:  E500, Version: 3.0, (0x80210030)
Clock Configuration:
       CPU0:1500 MHz, CPU1:1500 MHz, CCB: 600 MHz,
       DDR: 300 MHz (600 MT/s data rate) (Synchronous), LBC:  75 MHz
L1:    D-cache 32 kB enabled
       I-cache 32 kB enabled
Board: X-ES XPedite5370 3U VPX SBC
I2C:   ready
DRAM:  DDR: No DIMMs found.
*** failed ***
ERROR: init function at fff894bc failed
(debug) => imm 54 200.2
00000200: 00 ? 80
00000201: 00 ? 8
00000202: 00 ? 8
00000203: 0e ? .
(debug) => reset

U-Boot 1.3.4-00113-g8b3bc07 (Aug 15 2008 - 14:26:37)

SPD fixed, board boots...

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debug console

[Peter Tyser]

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 README           |   11 +++++++++++
 common/console.c |   34 +++++++++++++++++++++++++++++-----
 common/serial.c  |    6 ++++--
 include/common.h |    3 +++
 lib_arm/board.c  |   12 ++++++++++++
 lib_i386/board.c |   13 ++++++++++++-
 lib_m68k/board.c |   12 ++++++++++++
 lib_mips/board.c |   12 ++++++++++++
 lib_ppc/board.c  |   13 ++++++++++++-
 lib_sh/board.c   |   12 ++++++++++++
 10 files changed, 119 insertions(+), 9 deletions(-)

diff --git a/README b/README
index 37449d1..1346fe0 100644
--- a/README
+++ b/README
@@ -453,6 +453,17 @@ The following options need to be configured:
 		default i/o. Serial console can be forced with
 		environment 'console=serial'.
 
+		CONFIG_DEBUG_CONSOLE
+		Enables a basic serial console while still executing from
+		flash. The debug console can be used to initially enter SPD
+		information, debug memory issues, debug i2c issues, etc
+		early in the boot process. The debug console is started
+		when an error occurs during hardware initialization (eg
+		DDR can't be configured), or if the user presses ctrl-c
+		early in the boot process. The address to store the
+		early console's command buffer must be specified with the
+		CFG_DEBUG_CONSOLE_ADDR define.
+
 		When CONFIG_SILENT_CONSOLE is defined, all console
 		messages (by U-Boot and Linux!) can be silenced with
 		the "silent" environment variable. See
diff --git a/common/console.c b/common/console.c
index daf0180..a967737 100644
--- a/common/console.c
+++ b/common/console.c
@@ -249,15 +249,16 @@ void vprintf (const char *fmt, va_list args)
 }
 
 /* test if ctrl-c was pressed */
-static int ctrlc_disabled = 0;	/* see disable_ctrl() */
-static int ctrlc_was_pressed = 0;
+static int ctrlc_disabled __attribute__ ((section(".data"))) = 0;
+static int ctrlc_was_pressed __attribute__ ((section(".data"))) = 0;
 int ctrlc (void)
 {
 	if (!ctrlc_disabled && gd->have_console) {
 		if (tstc ()) {
 			switch (getc ()) {
 			case 0x03:		/* ^C - Control C */
-				ctrlc_was_pressed = 1;
+				if (gd->flags & GD_FLG_RELOC)
+					ctrlc_was_pressed = 1;
 				return 1;
 			default:
 				break;
@@ -274,7 +275,8 @@ int disable_ctrlc (int disable)
 {
 	int prev = ctrlc_disabled;	/* save previous state */
 
-	ctrlc_disabled = disable;
+	if (gd->flags & GD_FLG_RELOC)
+		ctrlc_disabled = disable;
 	return prev;
 }
 
@@ -285,13 +287,35 @@ int had_ctrlc (void)
 
 void clear_ctrlc (void)
 {
-	ctrlc_was_pressed = 0;
+	if (gd->flags & GD_FLG_RELOC)
+		ctrlc_was_pressed = 0;
 }
 
 char * console_buffer_addr(void)
 {
+#ifdef CONFIG_DEBUG_CONSOLE
+	return (gd->flags & GD_FLG_RELOC ?
+		console_buffer : CFG_DEBUG_CONSOLE_ADDR);
+#else
 	return console_buffer;
+#endif
+}
+
+#ifdef CONFIG_DEBUG_CONSOLE
+void debug_console (void)
+{
+	char *console_buffer = console_buffer_addr();
+
+	while (1) {
+		if (readline("(debug) => ") <= 0)
+			continue;
+		if (!strcmp(console_buffer, "exit"))
+			break;
+
+		run_command(console_buffer, 0);
+	}
 }
+#endif
 
 #ifdef CONFIG_MODEM_SUPPORT_DEBUG
 char	screen[1024];
diff --git a/common/serial.c b/common/serial.c
index bfda7ca..eb65c4c 100644
--- a/common/serial.c
+++ b/common/serial.c
@@ -29,8 +29,10 @@ DECLARE_GLOBAL_DATA_PTR;
 
 #if defined(CONFIG_SERIAL_MULTI)
 
-static struct serial_device *serial_devices = NULL;
-static struct serial_device *serial_current = NULL;
+static struct serial_device *serial_devices
+	__attribute__ ((section(".data"))) = NULL;
+static struct serial_device *serial_current
+	__attribute__ ((section(".data"))) = NULL;
 
 #if !defined(CONFIG_LWMON) && !defined(CONFIG_PXA27X)
 struct serial_device *__default_serial_console (void)
diff --git a/include/common.h b/include/common.h
index 69f5af4..a2621a7 100644
--- a/include/common.h
+++ b/include/common.h
@@ -629,6 +629,9 @@ int	had_ctrlc (void);	/* have we had a Control-C since last clear? */
 void	clear_ctrlc (void);	/* clear the Control-C condition */
 int	disable_ctrlc (int);	/* 1 to disable, 0 to enable Control-C detect */
 char	*console_buffer_addr(void);
+#ifdef CONFIG_DEBUG_CONSOLE
+void	debug_console(void);
+#endif
 
 /*
  * STDIO based functions (can always be used)
diff --git a/lib_arm/board.c b/lib_arm/board.c
index 1f5409b..252a884 100644
--- a/lib_arm/board.c
+++ b/lib_arm/board.c
@@ -297,6 +297,9 @@ init_fnc_t *init_sequence[] = {
 #endif
 	dram_init,		/* configure available RAM banks */
 	display_dram_config,
+#if defined(CONFIG_DEBUG_CONSOLE)
+	ctrlc,	/* Let user break out of boot sequence if ctrl-c is pressed */
+#endif
 	NULL,
 };
 
@@ -324,7 +327,16 @@ void start_armboot (void)
 
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
 		if ((*init_fnc_ptr)() != 0) {
+#ifdef CONFIG_DEBUG_CONSOLE
+			if (!gd->have_console)
+				hang();
+
+			printf("ERROR: init function at %p failed\n",
+			       *init_fnc_ptr);
+			debug_console();
+#else
 			hang ();
+#endif
 		}
 	}
 
diff --git a/lib_i386/board.c b/lib_i386/board.c
index 22191e6..9c52ac8 100644
--- a/lib_i386/board.c
+++ b/lib_i386/board.c
@@ -222,7 +222,9 @@ init_fnc_t *init_sequence[] = {
 	serial_init,		/* serial communications setup */
 	display_banner,
 	display_dram_config,
-
+#if defined(CONFIG_DEBUG_CONSOLE)
+	ctrlc,	/* Let user break out of boot sequence if ctrl-c is pressed */
+#endif
 	NULL,
 };
 
@@ -254,7 +256,16 @@ void start_i386boot (void)
 		show_boot_progress(0xa130|i);
 
 		if ((*init_fnc_ptr)() != 0) {
+#ifdef CONFIG_DEBUG_CONSOLE
+			if (!gd->have_console)
+				hang();
+
+			printf("ERROR: init function at %p failed\n",
+			       *init_fnc_ptr);
+			debug_console();
+#else
 			hang ();
+#endif
 		}
 	}
 	show_boot_progress(0x23);
diff --git a/lib_m68k/board.c b/lib_m68k/board.c
index dedc9e4..faf2b93 100644
--- a/lib_m68k/board.c
+++ b/lib_m68k/board.c
@@ -253,6 +253,9 @@ init_fnc_t *init_sequence[] = {
 	testdram,
 #endif /* CFG_DRAM_TEST */
 	INIT_FUNC_WATCHDOG_INIT
+#if defined(CONFIG_DEBUG_CONSOLE)
+	ctrlc,	/* Let user break out of boot sequence if ctrl-c is pressed */
+#endif
 	NULL,			/* Terminate this list */
 };
 
@@ -297,7 +300,16 @@ board_init_f (ulong bootflag)
 
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
 		if ((*init_fnc_ptr)() != 0) {
+#ifdef CONFIG_DEBUG_CONSOLE
+			if (!gd->have_console)
+				hang();
+
+			printf("ERROR: init function at %p failed\n",
+			       *init_fnc_ptr);
+			debug_console();
+#else
 			hang ();
+#endif
 		}
 	}
 
diff --git a/lib_mips/board.c b/lib_mips/board.c
index 09b8b3b..ff8caa4 100644
--- a/lib_mips/board.c
+++ b/lib_mips/board.c
@@ -178,6 +178,9 @@ init_fnc_t *init_sequence[] = {
 	display_banner,		/* say that we are here */
 	checkboard,
 	init_func_ram,
+#if defined(CONFIG_DEBUG_CONSOLE)
+	ctrlc,	/* Let user break out of boot sequence if ctrl-c is pressed */
+#endif
 	NULL,
 };
 
@@ -203,7 +206,16 @@ void board_init_f(ulong bootflag)
 
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
 		if ((*init_fnc_ptr)() != 0) {
+#ifdef CONFIG_DEBUG_CONSOLE
+			if (!gd->have_console)
+				hang();
+
+			printf("ERROR: init function at %p failed\n",
+			       *init_fnc_ptr);
+			debug_console();
+#else
 			hang ();
+#endif
 		}
 	}
 
diff --git a/lib_ppc/board.c b/lib_ppc/board.c
index 262b972..1ff7411 100644
--- a/lib_ppc/board.c
+++ b/lib_ppc/board.c
@@ -360,7 +360,9 @@ init_fnc_t *init_sequence[] = {
 	testdram,
 #endif /* CFG_DRAM_TEST */
 	INIT_FUNC_WATCHDOG_RESET
-
+#if defined(CONFIG_DEBUG_CONSOLE)
+	ctrlc,	/* Let user break out of boot sequence if ctrl-c is pressed */
+#endif
 	NULL,			/* Terminate this list */
 };
 
@@ -427,7 +429,16 @@ void board_init_f (ulong bootflag)
 
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
 		if ((*init_fnc_ptr) () != 0) {
+#ifdef CONFIG_DEBUG_CONSOLE
+			if (!gd->have_console)
+				hang();
+
+			printf("ERROR: init function at %p failed\n",
+			       *init_fnc_ptr);
+			debug_console();
+#else
 			hang ();
+#endif
 		}
 	}
 
diff --git a/lib_sh/board.c b/lib_sh/board.c
index eb81bd9..8a0df48 100644
--- a/lib_sh/board.c
+++ b/lib_sh/board.c
@@ -163,6 +163,9 @@ init_fnc_t *init_sequence[] =
 #if defined(CONFIG_CMD_NET)
 	sh_net_init,		/* SH specific eth init */
 #endif
+#if defined(CONFIG_DEBUG_CONSOLE)
+	ctrlc,	/* Let user break out of boot sequence if ctrl-c is pressed */
+#endif
 	NULL			/* Terminate this list */
 };
 
@@ -196,7 +199,16 @@ void sh_generic_init (void)
 
 	for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr , i++) {
 		if ((*init_fnc_ptr) () != 0) {
+#ifdef CONFIG_DEBUG_CONSOLE
+			if (!gd->have_console)
+				hang();
+
+			printf("ERROR: init function at %p failed\n",
+			       *init_fnc_ptr);
+			debug_console();
+#else
 			hang();
+#endif
 		}
 	}
 
-- 
1.5.4.3

[U-Boot] [PATCH 2/3] [RFC] Make memory commands usable before relocation to SDRAM

[Peter Tyser]

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 common/cmd_mem.c |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/common/cmd_mem.c b/common/cmd_mem.c
index f3299bd..6eb1fd4 100644
--- a/common/cmd_mem.c
+++ b/common/cmd_mem.c
@@ -43,6 +43,8 @@
     || defined(CONFIG_CMD_PCI)		\
     || defined(CONFIG_CMD_PORTIO)
 
+DECLARE_GLOBAL_DATA_PTR;
+
 int cmd_get_data_size(char* arg, int default_size)
 {
 	/* Check for a size specification .b, .w or .l.
@@ -83,7 +85,7 @@ uint	dp_last_addr, dp_last_size;
 uint	dp_last_length = 0x40;
 uint	mm_last_addr, mm_last_size;
 
-static	ulong	base_address = 0;
+static	ulong	base_address __attribute__ ((section(".data"))) = 0;
 
 /* Memory Display
  *
@@ -103,8 +105,10 @@ int do_mem_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	/* We use the last specified parameters, unless new ones are
 	 * entered.
 	 */
-	addr = dp_last_addr;
-	size = dp_last_size;
+	if (gd->flags & GD_FLG_RELOC) {
+		addr = dp_last_addr;
+		size = dp_last_size;
+	}
 	length = dp_last_length;
 
 	if (argc < 2) {
@@ -183,9 +187,12 @@ int do_mem_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	}
 #endif
 
-	dp_last_addr = addr;
-	dp_last_length = length;
-	dp_last_size = size;
+	if (gd->flags & GD_FLG_RELOC) {
+		dp_last_addr = addr;
+		dp_last_length = length;
+		dp_last_size = size;
+	}
+
 	return (rc);
 }
 
@@ -1025,8 +1032,10 @@ mod_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[])
 	/* We use the last specified parameters, unless new ones are
 	 * entered.
 	 */
-	addr = mm_last_addr;
-	size = mm_last_size;
+	if (gd->flags & GD_FLG_RELOC) {
+		addr = mm_last_addr;
+		size = mm_last_size;
+	}
 
 	if ((flag & CMD_FLAG_REPEAT) == 0) {
 		/* New command specified.  Check for a size specification.
@@ -1106,8 +1115,11 @@ mod_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[])
 		}
 	} while (nbytes);
 
-	mm_last_addr = addr;
-	mm_last_size = size;
+	if (gd->flags & GD_FLG_RELOC) {
+		mm_last_addr = addr;
+		mm_last_size = size;
+	}
+
 	return 0;
 }
 
-- 
1.5.4.3

[U-Boot] [PATCH 3/3] [RFC] Make i2c commands usable before relocation to SDRAM

[Peter Tyser]

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
 common/cmd_i2c.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/common/cmd_i2c.c b/common/cmd_i2c.c
index 1f32646..ae7ed90 100644
--- a/common/cmd_i2c.c
+++ b/common/cmd_i2c.c
@@ -129,6 +129,8 @@ static int
 mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[]);
 extern int cmd_get_data_size(char* arg, int default_size);
 
+DECLARE_GLOBAL_DATA_PTR;
+
 /*
  * Syntax:
  *	imd {i2c_chip} {addr}{.0, .1, .2} {len}
@@ -144,9 +146,11 @@ int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 	/* We use the last specified parameters, unless new ones are
 	 * entered.
 	 */
-	chip   = i2c_dp_last_chip;
-	addr   = i2c_dp_last_addr;
-	alen   = i2c_dp_last_alen;
+	if (gd->flags & GD_FLG_RELOC) {
+		chip   = i2c_dp_last_chip;
+		addr   = i2c_dp_last_addr;
+		alen   = i2c_dp_last_alen;
+	}
 	length = i2c_dp_last_length;
 
 	if (argc < 3) {
@@ -227,10 +231,12 @@ int do_i2c_md ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
 		nbytes -= linebytes;
 	} while (nbytes > 0);
 
-	i2c_dp_last_chip   = chip;
-	i2c_dp_last_addr   = addr;
-	i2c_dp_last_alen   = alen;
-	i2c_dp_last_length = length;
+	if (gd->flags & GD_FLG_RELOC) {
+		i2c_dp_last_chip   = chip;
+		i2c_dp_last_addr   = addr;
+		i2c_dp_last_alen   = alen;
+		i2c_dp_last_length = length;
+	}
 
 	return 0;
 }
@@ -432,9 +438,11 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[])
 	 * We use the last specified parameters, unless new ones are
 	 * entered.
 	 */
-	chip = i2c_mm_last_chip;
-	addr = i2c_mm_last_addr;
-	alen = i2c_mm_last_alen;
+	if (gd->flags & GD_FLG_RELOC) {
+		chip = i2c_mm_last_chip;
+		addr = i2c_mm_last_addr;
+		alen = i2c_mm_last_alen;
+	}
 
 	if ((flag & CMD_FLAG_REPEAT) == 0) {
 		/*
@@ -529,9 +537,11 @@ mod_i2c_mem(cmd_tbl_t *cmdtp, int incrflag, int flag, int argc, char *argv[])
 		}
 	} while (nbytes);
 
-	i2c_mm_last_chip = chip;
-	i2c_mm_last_addr = addr;
-	i2c_mm_last_alen = alen;
+	if (gd->flags & GD_FLG_RELOC) {
+		i2c_mm_last_chip = chip;
+		i2c_mm_last_addr = addr;
+		i2c_mm_last_alen = alen;
+	}
 
 	return 0;
 }
-- 
1.5.4.3

[U-Boot] [PATCH] [RFC] Early serial debug console

[Dan Malek]

On Aug 15, 2008, at 2:16 PM, Peter Tyser wrote:

> Does anyone have suggestions/comments?  Any chance this feature  
> could make
> it into mainline U-Boot?

Are going to take on the challenge of making this work with
processors that use CPM/QE devices for serial ports? :-)

Thanks.

	-- Dan

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debug console

[Wolfgang Denk]

Dear Peter Tyser,

In message <1218835004-30729-2-git-send-email-ptyser@xes-inc.com> you wrote:
> Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
> ---
>  README           |   11 +++++++++++
>  common/console.c |   34 +++++++++++++++++++++++++++++-----
>  common/serial.c  |    6 ++++--
>  include/common.h |    3 +++
>  lib_arm/board.c  |   12 ++++++++++++
>  lib_i386/board.c |   13 ++++++++++++-
>  lib_m68k/board.c |   12 ++++++++++++
>  lib_mips/board.c |   12 ++++++++++++
>  lib_ppc/board.c  |   13 ++++++++++++-
>  lib_sh/board.c   |   12 ++++++++++++
>  10 files changed, 119 insertions(+), 9 deletions(-)

I understand what you are trying todo, but I think it doesn't work.

You are invoking a numer of pretty complex functions (like readline()
and run_command() and ...) which in turn ionvoke other functions etc.
- all of them written in C with theassumption that they have a  valid
C runtime environment, which is simply not the case before relocation
to RAM.

And your patch seems to be inclomplete, it does not apply.

Some parts seem to be missing (like the necessary changes to eliminate
accesses to the console_buffer[] in BSS).


But in any case - You make a few commands usable, and the behaviour of
the remainig undefined. 

I don't think this is a good idea.

And we pay for this with a log of uglier code (many, many #ifdef's)
and increased memory size.


I'm interested to hear what others say, but so far I tend to reject
this.

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
"'Tis true, 'tis pity, and pity 'tis 'tis true."
    - Poloniouius, in Willie the Shake's _Hamlet, Prince of Darkness_

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debug console

[Peter Tyser]

> I understand what you are trying todo, but I think it doesn't work.
> 
> You are invoking a numer of pretty complex functions (like readline()
> and run_command() and ...) which in turn ionvoke other functions etc.
> - all of them written in C with theassumption that they have a  valid
> C runtime environment, which is simply not the case before relocation
> to RAM.

I attempted to account for this fact.  I followed the program flow of
run_command() and readline() in particular looking for global data
writes and bss reads.  For the instances where global data was being
written, I made it conditional on (gd->flags & GD_FLG_RELOC).  I also
forced some variables to reside in the data section so that they could
be read while executing from flash (__attribute__ ((section(".data")))).
I also monitored the flash WE pin while executing the memory and i2c
commands to make sure that the data section (which is still in flash)
was not being written to.  There is a chance that uninitialized data is
being read from the bss, but I didn't see any and the board functions
fine:)

> And your patch seems to be inclomplete, it does not apply.

Hmm, I built the patches off 07efc9e321619c3dec213310c32e011aa6f02783
with the 2 patches I just submitted also applied ("mod_i2c_mem()
bugfix" and "Replace references to extern console_buffer with a function
call").  What error are you seeing while applying it?


> Some parts seem to be missing (like the necessary changes to eliminate
> accesses to the console_buffer[] in BSS).

I don't follow.  The console_buffer_addr() conditionally places the
console_buffer in SDRAM if U-Boot has relocated, or at
CFG_DEBUG_CONSOLE_ADDR if executing from flash.  That's why the "Replace
references to extern console_buffer with a function call" patch I just
submitted was necessary.

> But in any case - You make a few commands usable, and the behaviour of
> the remainig undefined. 
> 
> I don't think this is a good idea.

I agree that it is hokey that some commands work, some don't.  However,
the ones that are very useful do work:)  The memory test, modify, and
display functions as well as the i2c functions work.  These few commands
allow further diagnosis (and possible fixing) of most board hardware
problems that prevent a board from booting.

> 
> And we pay for this with a log of uglier code (many, many #ifdef's)
> and increased memory size.

Agreed, the code is a bit dirtier, but I think the #ifdefs could be
minimized some.  For example, the sequence of code in lib_xxx/board.c:
 for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) {
        if ((*init_fnc_ptr)() != 0 {
#ifdef CONFIG_DEBUG_CONSOLE
....

could be put in 1 file for all architectures to share.  In that case,
there would only be an additional 4-5 #ifdefs added.  The patches
themselves aren't all that large.

> I'm interested to hear what others say, but so far I tend to reject
> this.

Thanks for the feedback, I'm curious what others have to say as well.
Let me know if I can provide any additional details.

Peter

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debug console

[Wolfgang Denk]

Dear Peter,

In message <1218839108.1273.144.camel@localhost.localdomain> you wrote:
> 
> I attempted to account for this fact.  I followed the program flow of
> run_command() and readline() in particular looking for global data
> writes and bss reads.  For the instances where global data was being

Indeed you did this, but only for a minor part of the U-Boot code - at
the cost of added compexity, ugly $ifdeffery and inconsistent and
undefined behaviour as soon as you run any "uninspected" command.

> written, I made it conditional on (gd->flags & GD_FLG_RELOC).  I also
> forced some variables to reside in the data section so that they could
> be read while executing from flash (__attribute__ ((section(".data")))).

This just helps to make the initial values readable. The variables
still cannot be written. Even worse, on some flash types (namely Intel
ones) such write operations to flash memory can corrupt your U-Boot
image.

> I also monitored the flash WE pin while executing the memory and i2c
> commands to make sure that the data section (which is still in flash)
> was not being written to.  There is a chance that uninitialized data is

But that's just these two commands. What happens when I run any other
command? And you don't prevent me from doing that.

> > And your patch seems to be inclomplete, it does not apply.
> 
> Hmm, I built the patches off 07efc9e321619c3dec213310c32e011aa6f02783
> with the 2 patches I just submitted also applied ("mod_i2c_mem()
> bugfix" and "Replace references to extern console_buffer with a function
> call").  What error are you seeing while applying it?
> 
> 
> > Some parts seem to be missing (like the necessary changes to eliminate
> > accesses to the console_buffer[] in BSS).
> 
> I don't follow.  The console_buffer_addr() conditionally places the
> console_buffer in SDRAM if U-Boot has relocated, or at
> CFG_DEBUG_CONSOLE_ADDR if executing from flash.  That's why the "Replace
> references to extern console_buffer with a function call" patch I just
> submitted was necessary.

Ah, so that patch is a prerequisite? You shoul have  mentioned  that,
then, or poosted the three of them as a series.

> I agree that it is hokey that some commands work, some don't.  However,
> the ones that are very useful do work:)  The memory test, modify, and
> display functions as well as the i2c functions work.  These few commands
> allow further diagnosis (and possible fixing) of most board hardware
> problems that prevent a board from booting.

Well, someone who is working in these stages of bard bringup  usually
uses a debugger.

> Thanks for the feedback, I'm curious what others have to say as well.
> Let me know if I can provide any additional details.

The longer I think about it the more I tend to reject  it.  It  makes
the  code worse for the benefit of just a very special situation that
probably can be handled as well (or even better)  using  a  debugger.

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
Microsoft Multitasking:
                     several applications can crash at the same time.

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debug console

[Peter Tyser]

Thanks for the comments.  Response inline:

On Sat, 2008-08-16 at 09:49 +0200, Wolfgang Denk wrote:
> Dear Peter,
> 
> In message <1218839108.1273.144.camel@localhost.localdomain> you wrote:
> > 
> > I attempted to account for this fact.  I followed the program flow of
> > run_command() and readline() in particular looking for global data
> > writes and bss reads.  For the instances where global data was being
> 
> Indeed you did this, but only for a minor part of the U-Boot code - at
> the cost of added compexity, ugly $ifdeffery and inconsistent and
> undefined behaviour as soon as you run any "uninspected" command.
> 
> > written, I made it conditional on (gd->flags & GD_FLG_RELOC).  I also
> > forced some variables to reside in the data section so that they could
> > be read while executing from flash (__attribute__ ((section(".data")))).
> 
> This just helps to make the initial values readable. The variables
> still cannot be written. Even worse, on some flash types (namely Intel
> ones) such write operations to flash memory can corrupt your U-Boot
> image.

If the debug console is being used, it would generally mean that the
board is not bootable.  Thus the absolute worst (and somewhat unlikely)
outcome of using the debug console would be to render the already
unbootable board further unbootable.

I'd also put the burden of using the debug console responsibly on the
end user (similar to most of U-Boot's normal commands).  Perhaps some
added documentation would alleviate misuse of the debug console?

> > I also monitored the flash WE pin while executing the memory and i2c
> > commands to make sure that the data section (which is still in flash)
> > was not being written to.  There is a chance that uninitialized data is
> 
> But that's just these two commands. What happens when I run any other
> command? And you don't prevent me from doing that.

The same argument applies - if the board already isn't booting, running
a "bad" command from the debug console can't do any real damage.

It would also be relatively easy to make the "repeatable" argument in
the cmd_tbl_s structure be a general purpose flag argument, in which
case commands which have been Ok-ed for use by the debug console could
be marked as such in the command definition.

> > > And your patch seems to be inclomplete, it does not apply.
> > 
> > Hmm, I built the patches off 07efc9e321619c3dec213310c32e011aa6f02783
> > with the 2 patches I just submitted also applied ("mod_i2c_mem()
> > bugfix" and "Replace references to extern console_buffer with a function
> > call").  What error are you seeing while applying it?
> > 
> > 
> > > Some parts seem to be missing (like the necessary changes to eliminate
> > > accesses to the console_buffer[] in BSS).
> > 
> > I don't follow.  The console_buffer_addr() conditionally places the
> > console_buffer in SDRAM if U-Boot has relocated, or at
> > CFG_DEBUG_CONSOLE_ADDR if executing from flash.  That's why the "Replace
> > references to extern console_buffer with a function call" patch I just
> > submitted was necessary.
> 
> Ah, so that patch is a prerequisite? You shoul have  mentioned  that,
> then, or poosted the three of them as a series.

This assumption was mentioned in the patch summary, see the first email
in the series "[PATCH] [RFC] Early serial debug console".

> > I agree that it is hokey that some commands work, some don't.  However,
> > the ones that are very useful do work:)  The memory test, modify, and
> > display functions as well as the i2c functions work.  These few commands
> > allow further diagnosis (and possible fixing) of most board hardware
> > problems that prevent a board from booting.
> 
> Well, someone who is working in these stages of bard bringup  usually
> uses a debugger.

From my experience, there are a few shortcomings to using a debugger to
bring up a board and diagnose hardware issues:
1. Its time consuming/complicated to accurately diagnose hardware issues
2. Its much harder to bring out "tricky" issues like floating DDR
address lines, improper DDR termination, improper CPU DDR timing
parameters, etc
3. Not everyone has a debugger readily available
4. Some boards might not have JTAG/debugger signals brought out to a
header at all, which makes using a debugger impossible

> > Thanks for the feedback, I'm curious what others have to say as well.
> > Let me know if I can provide any additional details.
> 
> The longer I think about it the more I tend to reject  it.  It  makes
> the  code worse for the benefit of just a very special situation that
> probably can be handled as well (or even better)  using  a  debugger.

I personally feel that a debugger is not always the best tool for
initial board bringup/debugging.  After manufacturing, we program the
majority of our boards over the PCI bus (much faster than JTAG), then
run them through a manufacturing test from U-Boot/Linux.  Assuming some
hardware tests fail, it is no trivial matter to modify JTAG tests to try
and narrow down the problem.  Having access to a basic debug console
greatly speeds up this debug process.

I don't disagree that the implementation isn't the cleanest, but I
really do think an early debug console is very useful for board bringup,
debug, tuning, etc.  In my opinion, the few extra bytes of memory the
feature adds and the 20 lines of convoluted code are outweighed by the
usefulness of the feature.

Is there any way the debug console could be implemented in a cleaner
manner that would be more readily accepted into U-Boot mainline?

Best,
Peter

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debug console

[Wolfgang Denk]

Dear Peter Tyser,

In message <1218877618.24773.49.camel@ptyser-laptop> you wrote:
> 
> If the debug console is being used, it would generally mean that the
> board is not bootable.  Thus the absolute worst (and somewhat unlikely)
> outcome of using the debug console would be to render the already
> unbootable board further unbootable.

Well, that's definitely the point where you want to attach your
debugger.

> >From my experience, there are a few shortcomings to using a debugger to
> bring up a board and diagnose hardware issues:
> 1. Its time consuming/complicated to accurately diagnose hardware issues

Isn't that always the case, no matter which tools you use? ;-)

> 2. Its much harder to bring out "tricky" issues like floating DDR
> address lines, improper DDR termination, improper CPU DDR timing
> parameters, etc

Hm... I'm not sure why you think so. But of course there  are  always
situations where special debug code may be necessary. I guess many of
the  U-Boot  developers  have  their  own  special  bag of tricks and
snippets of code they pull out when needed. All this stuff is usually
pretty useful in the given situations, but it is not adequate  to  go
into the mainline U-Boot source tree.

> 3. Not everyone has a debugger readily available

OK. Then you have to dig deeper in your bag of tricks, of course.  On
the  other  hand  you should consider how much time you spend and how
much a BDI3000 costs, and which on eis cheaper.  If  you  are  not  a
student,  and this is not a one-time job for you, the investment into
a BDI3000 always wins.

> 4. Some boards might not have JTAG/debugger signals brought out to a
> header at all, which makes using a debugger impossible

And how do you program U-Boot to flash then?

Frankly, in such a situation you should find out the hardware guy who
is responsible for such a design and kick him really  hard  where  it
really hurts until he gives you a written guarantee never again to do
such a stupid thing.

> I personally feel that a debugger is not always the best tool for
> initial board bringup/debugging.  After manufacturing, we program the
> majority of our boards over the PCI bus (much faster than JTAG), then

Sure. There are many ways to skin a cat. For  example,  many  of  our
customers  fit  pre-programmed  flashes  on their boards. That's even
faster :-)

> run them through a manufacturing test from U-Boot/Linux.  Assuming some
> hardware tests fail, it is no trivial matter to modify JTAG tests to try
> and narrow down the problem.  Having access to a basic debug console
> greatly speeds up this debug process.

If this works for you, fine.

> I don't disagree that the implementation isn't the cleanest, but I
> really do think an early debug console is very useful for board bringup,
> debug, tuning, etc.  In my opinion, the few extra bytes of memory the
> feature adds and the 20 lines of convoluted code are outweighed by the
> usefulness of the feature.
> 
> Is there any way the debug console could be implemented in a cleaner
> manner that would be more readily accepted into U-Boot mainline?

Given the inherent severe  limitations  of  the  implementation,  the
added  code  complexity and the fact that it's only useful for a very
small percentage of use cases, I tend to reject it.

It's weekend, and not many people are active now. Let's  wait  for  a
few  more  days  if  anyboy  shows  up with arguments to support your
patches.

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
COBOL is for morons.                                 -- E.W. Dijkstra

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debugconsole

[Ken.Fuchs at bench.com]

> It's weekend, and not many people are active now. Let's  wait  for  a
> few  more  days  if  anybody  shows  up with arguments to support your
> patches.

In my opinion, the serial device should be the first device
initialized and utilized before all other devices where that
is possible.  For example, it would be very useful to have
serial output while debugging RAM initialization.  An early
serial port prior to U-Boot relocating itself would be very
useful for porting U-Boot to new boards.

An early debug serial port would be a good candidate for
mainline U-Boot:

CONFIG_DEBUG_EARLY_SERIAL has been implemented in a few forks
of U-Boot.

I also favor an early serial debug console as advocated by this
patch, but perhaps its code should be self contained.  I'm not
sure it is a good idea to modify some commands so they can be
run before RAM relocation.  If such a thing can be done simply,
it should be done for all commands.

Is the patch submitted clean enough?  No, the implementation
could be improved with some effort; a few #if - #else - #endif
clauses could be eliminated for example.

Would an easier to maintain implementation even be feasible?

I agree that a JTAG debugger is extremely useful for board
bring-up, but they are very expensive.

I'm strongly in favor of having some early serial functionality
whether in the form of simply being able to send output to the
serial port or having a semi-functional to fully functional shell
prior to RAM relocation.

Sincerely,

Ken Fuchs

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debugconsole

[Scott Wood]

Ken.Fuchs at bench.com wrote:
> I'm strongly in favor of having some early serial functionality
> whether in the form of simply being able to send output to the
> serial port or having a semi-functional to fully functional shell
> prior to RAM relocation.

We already have the former.

-Scott

[U-Boot] [PATCH 1/3] [RFC] Add support for early serial debugconsole

[Wolfgang Denk]

Dear Ken.Fuchs at bench.com,

In message <AA28F077645B324881335614E4F7C4284A4A2E@win-ex01.bench.com> you wrote:
>
> In my opinion, the serial device should be the first device
> initialized and utilized before all other devices where that
> is possible.  For example, it would be very useful to have

This is exactly how U-Boot was designed. However, we try to do this in
C, and we try to support all kinds of hardware to do so.

> serial output while debugging RAM initialization.  An early
> serial port prior to U-Boot relocating itself would be very
> useful for porting U-Boot to new boards.

We have that, and always had this as long U-Boot and PPCBoot exist.

> Is the patch submitted clean enough?  No, the implementation

The patch itself may be ok, but the concept is broken, and that's why
I reject it.

> I agree that a JTAG debugger is extremely useful for board
> bring-up, but they are very expensive.

Calculate you own time wasted on fruitless attempts to  get  a  board
working  times your hourly rate and you get ... enough money for more
than one BDI3000.

> I'm strongly in favor of having some early serial functionality
> whether in the form of simply being able to send output to the
> serial port or having a semi-functional to fully functional shell
> prior to RAM relocation.

But hey, we *do* have this. We just don't have an interactve shell
prompt.

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
That Microsoft, the Trabant of the operating  system  world,  may  be
glancing  over the Berlin Wall at the Audis and BMWs and Mercedes. In
their own universe Trabants and Ladas were mainstream too...
                                                   -- Evan Leibovitch