| Message ID | 1314449645-16900-1-git-send-email-graeme.russ@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
Dear Graeme Russ, In message <1314449645-16900-1-git-send-email-graeme.russ@gmail.com> you wrote: > This cropped up as an aside to another thread so I thought I would give > it a go. It's pretty rough-and-ready but it does the trick :) Hm....the problem is that we need some buffer to store the data. On some systems this may work, but on many (most?) it doesnt. > diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h > index 2902e61..4ebc5bd 100644 > --- a/arch/x86/include/asm/global_data.h > +++ b/arch/x86/include/asm/global_data.h > @@ -44,6 +44,7 @@ typedef struct global_data { > unsigned long env_addr; /* Address of Environment struct */ > unsigned long cpu_clk; /* CPU clock in Hz! */ > unsigned long bus_clk; > + unsigned long con_buf_idx; /* Console buffer index */ > unsigned long relocaddr; /* Start address of U-Boot in RAM */ > unsigned long start_addr_sp; /* start_addr_stackpointer */ > phys_size_t ram_size; /* RAM size */ > @@ -65,13 +66,14 @@ extern gd_t *gd; > #define GD_ENV_ADDR 5 > #define GD_CPU_CLK 6 > #define GD_BUS_CLK 7 > -#define GD_RELOC_ADDR 8 > -#define GD_START_ADDR_SP 9 > -#define GD_RAM_SIZE 10 > -#define GD_RESET_STATUS 11 > -#define GD_JT 12 > +#define GD_CON_BUF_IDX 8 > +#define GD_RELOC_ADDR 9 > +#define GD_START_ADDR_SP 10 > +#define GD_RAM_SIZE 11 > +#define GD_RESET_STATUS 12 > +#define GD_JT 13 > > -#define GD_SIZE 13 > +#define GD_SIZE 14 Argh... your whole "Word Offsets into Global Data" needs to be removed. This should be auto-generated as asm-offsets. > --- a/common/console.c > +++ b/common/console.c > @@ -323,6 +323,28 @@ int tstc(void) > return serial_tstc(); > } Hm... this adds a lot of code, unconditionally. In thos form this is not acceptable, especially as many boards cannot make use of this, or eventually don't want to make use of it. > + if (gd->flags & GD_FLG_HAVE_CONSOLE) { > + if (gd->flags & GD_FLG_DEVINIT) { > + /* Send to the standard output */ > + fputc(stdout, c); > + } else { > + /* Send directly to the handler */ > + serial_putc(c); > + } > } else { > - /* Send directly to the handler */ > - serial_putc(c); > + pre_console_putc(c); > } > } And this is actually wrong. If we can use the serial console for output, we definitely don;t want to use your buffer any more. > diff --git a/include/configs/eNET.h b/include/configs/eNET.h > index 548d52c..4fb971f 100644 > --- a/include/configs/eNET.h > +++ b/include/configs/eNET.h ...and compilation for all other boards breaks? Best regards, Wolfgang Denk
On 27/08/11 23:48, Wolfgang Denk wrote: > Dear Graeme Russ, > > In message <1314449645-16900-1-git-send-email-graeme.russ@gmail.com> you wrote: >> This cropped up as an aside to another thread so I thought I would give >> it a go. It's pretty rough-and-ready but it does the trick :) > > Hm....the problem is that we need some buffer to store the data. On > some systems this may work, but on many (most?) it doesnt. From what I can tell, a lot of CPUs use on-die cache until SDRAM is initialised. Now the AMD SC520 which I am using (which is a 12 year old part) has 16kB which is way more than enough for initial Global Data, stack and pre-console buffer Now I'm not saying (like you so rightly point out) that every CPU will have the required functionality. It would be interesting to know how much 'temporary memory' each of the various CPUs have. > >> diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h >> index 2902e61..4ebc5bd 100644 >> --- a/arch/x86/include/asm/global_data.h >> +++ b/arch/x86/include/asm/global_data.h >> @@ -44,6 +44,7 @@ typedef struct global_data { >> unsigned long env_addr; /* Address of Environment struct */ >> unsigned long cpu_clk; /* CPU clock in Hz! */ >> unsigned long bus_clk; >> + unsigned long con_buf_idx; /* Console buffer index */ >> unsigned long relocaddr; /* Start address of U-Boot in RAM */ >> unsigned long start_addr_sp; /* start_addr_stackpointer */ >> phys_size_t ram_size; /* RAM size */ >> @@ -65,13 +66,14 @@ extern gd_t *gd; >> #define GD_ENV_ADDR 5 >> #define GD_CPU_CLK 6 >> #define GD_BUS_CLK 7 >> -#define GD_RELOC_ADDR 8 >> -#define GD_START_ADDR_SP 9 >> -#define GD_RAM_SIZE 10 >> -#define GD_RESET_STATUS 11 >> -#define GD_JT 12 >> +#define GD_CON_BUF_IDX 8 >> +#define GD_RELOC_ADDR 9 >> +#define GD_START_ADDR_SP 10 >> +#define GD_RAM_SIZE 11 >> +#define GD_RESET_STATUS 12 >> +#define GD_JT 13 >> >> -#define GD_SIZE 13 >> +#define GD_SIZE 14 > > Argh... your whole "Word Offsets into Global Data" needs to be > removed. This should be auto-generated as asm-offsets. Agreed - I did some work on this but found the rabbit hole gets rather deep as asm-offsets needs to be build in the 'depend' target to avoid file not found errors while building the dependencies. Also, the temporary files need to be cleaned up in the clean target. So I started digging and hit rules.mk - I'm still working on it ;) >> --- a/common/console.c >> +++ b/common/console.c >> @@ -323,6 +323,28 @@ int tstc(void) >> return serial_tstc(); >> } > > Hm... this adds a lot of code, unconditionally. In thos form this is > not acceptable, especially as many boards cannot make use of this, or > eventually don't want to make use of it. As I said, rough-and-ready. Yes, I would expect in the board config file: #define CONFIG_PRE_CONSOLE_BUFFER >> + if (gd->flags & GD_FLG_HAVE_CONSOLE) { >> + if (gd->flags & GD_FLG_DEVINIT) { >> + /* Send to the standard output */ >> + fputc(stdout, c); >> + } else { >> + /* Send directly to the handler */ >> + serial_putc(c); >> + } >> } else { >> - /* Send directly to the handler */ >> - serial_putc(c); >> + pre_console_putc(c); >> } >> } > > And this is actually wrong. If we can use the serial console for > output, we definitely don;t want to use your buffer any more. Look closer - It's a diff weirdness - The actual code is: if (gd->flags & GD_FLG_HAVE_CONSOLE) { if (gd->flags & GD_FLG_DEVINIT) { /* Send to the standard output */ fputc(stdout, c); } else { /* Send directly to the handler */ serial_putc(c); } } else { pre_console_putc(c); } So if we have console, it goes there, otherwise it goes to the buffer >> diff --git a/include/configs/eNET.h b/include/configs/eNET.h >> index 548d52c..4fb971f 100644 >> --- a/include/configs/eNET.h >> +++ b/include/configs/eNET.h > > ...and compilation for all other boards breaks? See above. Also, I have noticed in drivers/i2c/ppc4xx_i2c.c: if (gd->flags & GD_FLG_HAVE_CONSOLE) { printf("I2C %s: failed %d\n", read ? "read" : "write", ret); } and in drivers/i2c/soft_i2c.c: #ifdef DEBUG_I2C #define PRINTD(fmt,args...) do { \ if (gd->flags & GD_FLG_HAVE_CONSOLE) \ printf (fmt ,##args); \ } while (0) #else #define PRINTD(fmt,args...) #endif So there are drivers that anticipate generating output before console is initialised - I think we should not put the onus on the driver and move these conditions to printf() in console.c - Unfortunately this could lead to head-scratching when one _thinks_ a printf() should generate an output, but it is squelched (which is what the pre-console buffer is for) Regards, Graeme
Dear Graeme Russ, In message <4E5985C8.8020404@gmail.com> you wrote: > > > And this is actually wrong. If we can use the serial console for > > output, we definitely don;t want to use your buffer any more. > > Look closer - It's a diff weirdness - The actual code is: I see. Thanks for pointing out. > Also, I have noticed in drivers/i2c/ppc4xx_i2c.c: > > if (gd->flags & GD_FLG_HAVE_CONSOLE) { > printf("I2C %s: failed %d\n", > read ? "read" : "write", ret); > } > > and in drivers/i2c/soft_i2c.c: > > #ifdef DEBUG_I2C > #define PRINTD(fmt,args...) do { \ > if (gd->flags & GD_FLG_HAVE_CONSOLE) \ > printf (fmt ,##args); \ > } while (0) > #else > #define PRINTD(fmt,args...) > #endif > > So there are drivers that anticipate generating output before console is > initialised - I think we should not put the onus on the driver and move > these conditions to printf() in console.c - Unfortunately this could lead > to head-scratching when one _thinks_ a printf() should generate an output, > but it is squelched (which is what the pre-console buffer is for) Indeed - which is why this code is only used for debugging drivers in certain very special configurations. Best regards, Wolfgang Denk
Hi Wolfgang, On 28/08/11 20:08, Wolfgang Denk wrote: > Dear Graeme Russ, [snip] > >> So there are drivers that anticipate generating output before console is >> initialised - I think we should not put the onus on the driver and move >> these conditions to printf() in console.c - Unfortunately this could lead >> to head-scratching when one _thinks_ a printf() should generate an output, >> but it is squelched (which is what the pre-console buffer is for) > > Indeed - which is why this code is only used for debugging drivers in > certain very special configurations. So do you think it is worth handling this in printf() as suggested? I do wonder what unforeseen consequences there can be for an unintended printf() before console has been initialised. I know that a plain-old 16550 will just output garbage because the baud rate is not set, but I wonder if other cases exist where totally unexpected (and unwanted) behaviour will occur because the hardware is not setup yet. I think the safe option is to squelch it in printf(), puts() and putc(). At least then you are only left wondering why something did not print rather than why you hardware is behaving oddly. And with the pre-console buffer (if you can squeeze it in) you automagically get those messages as soon as the console is ready. Regards, Graeme
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index cac12c0..602fb51 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -90,6 +90,8 @@ int x86_cpu_init_f(void) const u32 em_rst = ~X86_CR0_EM; const u32 mp_ne_set = X86_CR0_MP | X86_CR0_NE; + printf("Example pre-console printf()\n"); + /* initialize FPU, reset EM, set MP and NE */ asm ("fninit\n" \ "movl %%cr0, %%eax\n" \ diff --git a/arch/x86/include/asm/global_data.h b/arch/x86/include/asm/global_data.h index 2902e61..4ebc5bd 100644 --- a/arch/x86/include/asm/global_data.h +++ b/arch/x86/include/asm/global_data.h @@ -44,6 +44,7 @@ typedef struct global_data { unsigned long env_addr; /* Address of Environment struct */ unsigned long cpu_clk; /* CPU clock in Hz! */ unsigned long bus_clk; + unsigned long con_buf_idx; /* Console buffer index */ unsigned long relocaddr; /* Start address of U-Boot in RAM */ unsigned long start_addr_sp; /* start_addr_stackpointer */ phys_size_t ram_size; /* RAM size */ @@ -65,13 +66,14 @@ extern gd_t *gd; #define GD_ENV_ADDR 5 #define GD_CPU_CLK 6 #define GD_BUS_CLK 7 -#define GD_RELOC_ADDR 8 -#define GD_START_ADDR_SP 9 -#define GD_RAM_SIZE 10 -#define GD_RESET_STATUS 11 -#define GD_JT 12 +#define GD_CON_BUF_IDX 8 +#define GD_RELOC_ADDR 9 +#define GD_START_ADDR_SP 10 +#define GD_RAM_SIZE 11 +#define GD_RESET_STATUS 12 +#define GD_JT 13 -#define GD_SIZE 13 +#define GD_SIZE 14 /* * Global Data Flags diff --git a/common/console.c b/common/console.c index acc4df3..1fa862d 100644 --- a/common/console.c +++ b/common/console.c @@ -323,6 +323,28 @@ int tstc(void) return serial_tstc(); } +void pre_console_putc(const char c) +{ + char *buffer = (char *)CONFIG_SYS_TMP_CON_BUF_ADDR; + + if (gd->con_buf_idx < CONFIG_SYS_TMP_CON_BUF_SZ) + buffer[gd->con_buf_idx++] = c; +} + +void pre_console_puts(const char *s) +{ + while (*s) + pre_console_putc(*s++); +} + +void print_pre_console_buffer(void) +{ + char *buffer = (char *)CONFIG_SYS_TMP_CON_BUF_ADDR; + + buffer[gd->con_buf_idx] = 0x00; + puts(buffer); +} + void putc(const char c) { #ifdef CONFIG_SILENT_CONSOLE @@ -334,13 +356,16 @@ void putc(const char c) if (gd->flags & GD_FLG_DISABLE_CONSOLE) return; #endif - - if (gd->flags & GD_FLG_DEVINIT) { - /* Send to the standard output */ - fputc(stdout, c); + if (gd->flags & GD_FLG_HAVE_CONSOLE) { + if (gd->flags & GD_FLG_DEVINIT) { + /* Send to the standard output */ + fputc(stdout, c); + } else { + /* Send directly to the handler */ + serial_putc(c); + } } else { - /* Send directly to the handler */ - serial_putc(c); + pre_console_putc(c); } } @@ -355,13 +380,16 @@ void puts(const char *s) if (gd->flags & GD_FLG_DISABLE_CONSOLE) return; #endif - - if (gd->flags & GD_FLG_DEVINIT) { - /* Send to the standard output */ - fputs(stdout, s); + if (gd->flags & GD_FLG_HAVE_CONSOLE) { + if (gd->flags & GD_FLG_DEVINIT) { + /* Send to the standard output */ + fputs(stdout, s); + } else { + /* Send directly to the handler */ + serial_puts(s); + } } else { - /* Send directly to the handler */ - serial_puts(s); + pre_console_puts(s); } } @@ -529,6 +557,10 @@ int console_init_f(void) gd->flags |= GD_FLG_SILENT; #endif + printf("console initialised - dumping buffer\n"); + print_pre_console_buffer(); + printf("buffer dumped\n"); + return 0; } diff --git a/include/configs/eNET.h b/include/configs/eNET.h index 548d52c..4fb971f 100644 --- a/include/configs/eNET.h +++ b/include/configs/eNET.h @@ -164,8 +164,11 @@ #define CONFIG_SYS_STACK_SIZE (32 * 1024) #define CONFIG_SYS_CAR_ADDR 0x19200000 #define CONFIG_SYS_CAR_SIZE (16 * 1024) +#define CONFIG_SYS_TMP_CON_BUF_SZ (1 * 1024) #define CONFIG_SYS_INIT_SP_ADDR (CONFIG_SYS_CAR_ADDR + \ - CONFIG_SYS_CAR_SIZE) + CONFIG_SYS_CAR_SIZE - \ + CONFIG_SYS_TMP_CON_BUF_SZ) +#define CONFIG_SYS_TMP_CON_BUF_ADDR CONFIG_SYS_INIT_SP_ADDR + 1 #define CONFIG_SYS_MONITOR_BASE CONFIG_SYS_TEXT_BASE #define CONFIG_SYS_MONITOR_LEN (256 * 1024) #define CONFIG_SYS_MALLOC_LEN (CONFIG_ENV_SECT_SIZE + \