[U-Boot] [PATCH 1/2] console: Use pre-console buffer to get complete log on all consoles

[U-Boot] [PATCH 1/2] console: Use pre-console buffer to get complete log on all consoles

[Siarhei Siamashka]

Currently the pre-console buffer can accumulate early log messages
and flush them to the serial console as soon as it becomes available.

This patch just adds one more pre-console buffer flushing point and
does all the same for the other consoles too. This is particularly
useful for the vga/hdmi/lcd console, where we can see all the older
messages now (except for the log messages from SPL).

Naturally, we don't want to get an extra copy of the log messages
on the serial console again at the second flushing point, so the
serial console has to be explicitly filtered out.

Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
---
 common/console.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/common/console.c b/common/console.c
index 29560c3..fc1963b 100644
--- a/common/console.c
+++ b/common/console.c
@@ -199,6 +199,20 @@ static void console_putc(int file, const char c)
 	}
 }
 
+#ifdef CONFIG_PRE_CONSOLE_BUFFER
+static void console_putc_noserial(int file, const char c)
+{
+	int i;
+	struct stdio_dev *dev;
+
+	for (i = 0; i < cd_count[file]; i++) {
+		dev = console_devices[file][i];
+		if (dev->putc != NULL && strcmp(dev->name, "serial") != 0)
+			dev->putc(dev, c);
+	}
+}
+#endif
+
 static void console_puts(int file, const char *s)
 {
 	int i;
@@ -236,6 +250,14 @@ static inline void console_putc(int file, const char c)
 	stdio_devices[file]->putc(stdio_devices[file], c);
 }
 
+#ifdef CONFIG_PRE_CONSOLE_BUFFER
+static inline void console_putc_noserial(int file, const char c)
+{
+	if (strcmp(stdio_devices[file]->name, "serial") != 0)
+		stdio_devices[file]->putc(stdio_devices[file], c);
+}
+#endif
+
 static inline void console_puts(int file, const char *s)
 {
 	stdio_devices[file]->puts(stdio_devices[file], s);
@@ -382,6 +404,9 @@ int tstc(void)
 	return serial_tstc();
 }
 
+#define PRE_CONSOLE_FLUSHPOINT1_SERIAL			0
+#define PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL	1
+
 #ifdef CONFIG_PRE_CONSOLE_BUFFER
 #define CIRC_BUF_IDX(idx) ((idx) % (unsigned long)CONFIG_PRE_CON_BUF_SZ)
 
@@ -398,7 +423,7 @@ static void pre_console_puts(const char *s)
 		pre_console_putc(*s++);
 }
 
-static void print_pre_console_buffer(void)
+static void print_pre_console_buffer(int flushpoint)
 {
 	unsigned long i = 0;
 	char *buffer = (char *)CONFIG_PRE_CON_BUF_ADDR;
@@ -407,12 +432,20 @@ static void print_pre_console_buffer(void)
 		i = gd->precon_buf_idx - CONFIG_PRE_CON_BUF_SZ;
 
 	while (i < gd->precon_buf_idx)
-		putc(buffer[CIRC_BUF_IDX(i++)]);
+		switch (flushpoint) {
+		case PRE_CONSOLE_FLUSHPOINT1_SERIAL:
+			putc(buffer[CIRC_BUF_IDX(i++)]);
+			break;
+		case PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL:
+			console_putc_noserial(stdout,
+					      buffer[CIRC_BUF_IDX(i++)]);
+			break;
+		}
 }
 #else
 static inline void pre_console_putc(const char c) {}
 static inline void pre_console_puts(const char *s) {}
-static inline void print_pre_console_buffer(void) {}
+static inline void print_pre_console_buffer(int flushpoint) {}
 #endif
 
 void putc(const char c)
@@ -441,6 +474,7 @@ void putc(const char c)
 		fputc(stdout, c);
 	} else {
 		/* Send directly to the handler */
+		pre_console_putc(c);
 		serial_putc(c);
 	}
 }
@@ -472,6 +506,7 @@ void puts(const char *s)
 		fputs(stdout, s);
 	} else {
 		/* Send directly to the handler */
+		pre_console_puts(s);
 		serial_puts(s);
 	}
 }
@@ -679,7 +714,7 @@ int console_init_f(void)
 		gd->flags |= GD_FLG_SILENT;
 #endif
 
-	print_pre_console_buffer();
+	print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT1_SERIAL);
 
 	return 0;
 }
@@ -794,6 +829,7 @@ done:
 	if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL))
 		return 0;
 #endif
+	print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL);
 	return 0;
 }
 
@@ -869,7 +905,7 @@ int console_init_r(void)
 	if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL))
 		return 0;
 #endif
-
+	print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL);
 	return 0;
 }
 
-- 
2.0.5

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Siarhei Siamashka]

This allows to always have a complete log on the VGA/HDMI/LCD console.

Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
---
 include/configs/sunxi-common.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
index b4324ee..5ddcc42 100644
--- a/include/configs/sunxi-common.h
+++ b/include/configs/sunxi-common.h
@@ -272,10 +272,15 @@
 #ifndef CONFIG_SPL_BUILD
 #include <config_distro_defaults.h>
 
+/* Enable pre-console buffer to get complete log on the VGA console */
+#define CONFIG_PRE_CONSOLE_BUFFER
+#define CONFIG_PRE_CON_BUF_SZ		(1024 * 1024)
+#define CONFIG_PRE_CON_BUF_ADDR		(0x43000000 - CONFIG_PRE_CON_BUF_SZ)
+
 /*
  * 240M RAM (256M minimum minus space for the framebuffer),
- * 32M uncompressed kernel, 16M compressed kernel, 1M fdt,
- * 1M script, 1M pxe and the ramdisk at the end.
+ * 32M uncompressed kernel, 15M compressed kernel, 1M pre-console
+ * buffer, 1M fdt, 1M script, 1M pxe and the ramdisk at the end.
  */
 #define MEM_LAYOUT_ENV_SETTINGS \
 	"bootm_size=0xf000000\0" \
-- 
2.0.5

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Ian Campbell]

On Thu, 2015-01-08 at 09:02 +0200, Siarhei Siamashka wrote:
> This allows to always have a complete log on the VGA/HDMI/LCD console.
> 
> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> ---
>  include/configs/sunxi-common.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index b4324ee..5ddcc42 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -272,10 +272,15 @@
>  #ifndef CONFIG_SPL_BUILD
>  #include <config_distro_defaults.h>
>  
> +/* Enable pre-console buffer to get complete log on the VGA console */
> +#define CONFIG_PRE_CONSOLE_BUFFER
> +#define CONFIG_PRE_CON_BUF_SZ		(1024 * 1024)
> +#define CONFIG_PRE_CON_BUF_ADDR		(0x43000000 - CONFIG_PRE_CON_BUF_SZ)
> +
>  /*
>   * 240M RAM (256M minimum minus space for the framebuffer),
> - * 32M uncompressed kernel, 16M compressed kernel, 1M fdt,
> - * 1M script, 1M pxe and the ramdisk at the end.
> + * 32M uncompressed kernel, 15M compressed kernel, 1M pre-console
> + * buffer, 1M fdt, 1M script, 1M pxe and the ramdisk at the end.

Am I correct in thinking that the pre-console buffer is long gone
(replaced by the actual console) by the time any of these mem layout
choices come into play (i.e. long before anything might load a kernel,
initrd or fdt)?

If yes then I think it is confusing to modify this comment, and the
comment before the pre-console #defines should mention that the buffer
is boottime only/short lived etc.

If no then I'm not sure putting a "u-boot managed" buffer in the middle
of the "user managed" load space is a good idea, since it could lead to
odd/hard to debug corruptions etc if the kernel was >15M.

Either way I think it would be better to put this buffer somewhere else
entirely since it isn't really the same as these things.

>   */
>  #define MEM_LAYOUT_ENV_SETTINGS \
>  	"bootm_size=0xf000000\0" \

Both u-boot-sunxi#master and #next still have 0x10000000 here, which
tree is this patch on? Some branch of Hans' tree (with "sunxi:
sunxi-common.h: Reduce bootm_size to take the framebuffer into account"
in it) I suppose?

Perhaps a better place for the pre-console buffer would be right before
the framebuffer (or at the top of RAM if no video on the board), with
modifications to bootm_size or not depending on the answer to the
original question above.

Ian.

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Siarhei Siamashka]

On Thu, 08 Jan 2015 08:49:54 +0000
Ian Campbell <ijc@hellion.org.uk> wrote:

> On Thu, 2015-01-08 at 09:02 +0200, Siarhei Siamashka wrote:
> > This allows to always have a complete log on the VGA/HDMI/LCD console.
> > 
> > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> > ---
> >  include/configs/sunxi-common.h | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> > index b4324ee..5ddcc42 100644
> > --- a/include/configs/sunxi-common.h
> > +++ b/include/configs/sunxi-common.h
> > @@ -272,10 +272,15 @@
> >  #ifndef CONFIG_SPL_BUILD
> >  #include <config_distro_defaults.h>
> >  
> > +/* Enable pre-console buffer to get complete log on the VGA console */
> > +#define CONFIG_PRE_CONSOLE_BUFFER
> > +#define CONFIG_PRE_CON_BUF_SZ		(1024 * 1024)
> > +#define CONFIG_PRE_CON_BUF_ADDR		(0x43000000 - CONFIG_PRE_CON_BUF_SZ)
> > +
> >  /*
> >   * 240M RAM (256M minimum minus space for the framebuffer),
> > - * 32M uncompressed kernel, 16M compressed kernel, 1M fdt,
> > - * 1M script, 1M pxe and the ramdisk at the end.
> > + * 32M uncompressed kernel, 15M compressed kernel, 1M pre-console
> > + * buffer, 1M fdt, 1M script, 1M pxe and the ramdisk at the end.
> 
> Am I correct in thinking that the pre-console buffer is long gone
> (replaced by the actual console) by the time any of these mem layout
> choices come into play (i.e. long before anything might load a kernel,
> initrd or fdt)?

Yes, my understanding is that it should be already gone, but not too
long ago. Logging in the patched pre-console buffer ends at the time
when the following messages get displayed:
   In:    serial
   Out:   vga
   Err:   vga

Now that you mentioned it, the patch is indeed extending the life time
of the pre-console buffer, and this might be an unwanted side effect.
Maybe Simon has some opinion.

> If yes then I think it is confusing to modify this comment, and the
> comment before the pre-console #defines should mention that the buffer
> is boottime only/short lived etc.

Just in case if something goes really wrong (in theory it shouldn't,
but in practice you know...) it is still somewhat safer to keep the
buffer in its own dedicated area and keep everything else out.
 
> If no then I'm not sure putting a "u-boot managed" buffer in the middle
> of the "user managed" load space is a good idea, since it could lead to
> odd/hard to debug corruptions etc if the kernel was >15M.
>
> Either way I think it would be better to put this buffer somewhere else
> entirely since it isn't really the same as these things.

In a hypothetical scenario of having neither serial console nor
hdmi/lcd/vga console available, the user managed code could probably
dump the pre-console buffer to the SD card and provide some
rudimentary debugging aid. Yes, hardly anyone would ever do this.

If the u-boot log could be somehow passed to the kernel and show in
dmesg, that would be in fact quite useful in my opinion. But this is
well out of scope of discussing the current patch.

> 
> >   */
> >  #define MEM_LAYOUT_ENV_SETTINGS \
> >  	"bootm_size=0xf000000\0" \
> 
> Both u-boot-sunxi#master and #next still have 0x10000000 here, which
> tree is this patch on? Some branch of Hans' tree (with "sunxi:
> sunxi-common.h: Reduce bootm_size to take the framebuffer into account"
> in it) I suppose?

Now the u-boot-sunxi#next branch has "bootm_size=0xf000000" too.

Yes, I was using the Hans' branch with the pending lcd patches,
because I was working with this stuff too.

> Perhaps a better place for the pre-console buffer would be right before
> the framebuffer (or at the top of RAM if no video on the board), with
> modifications to bootm_size or not depending on the answer to the
> original question above.

If this needs any kind of runtime address calculations instead of
compile time constants, then IMHO it becomes unnecessarily complicated.

Anyway. The sunxi part of these changes just needs to assign some
memory area to the pre-console buffer. In the end it does not really
matter which one. The size also does not need to be too large.
For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
of the log buffer can fully cover the FullHD display using the 8x16
font. And this is even assuming no line breaks. I picked 1M only
because it was the smallest unit of the address space allocation in
sunxi-common.h :-)

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Ian Campbell]

On Fri, 2015-01-09 at 13:13 +0200, Siarhei Siamashka wrote:
> > If yes then I think it is confusing to modify this comment, and the
> > comment before the pre-console #defines should mention that the buffer
> > is boottime only/short lived etc.
> 
> Just in case if something goes really wrong (in theory it shouldn't,
> but in practice you know...) it is still somewhat safer to keep the
> buffer in its own dedicated area and keep everything else out.

Nothing in those defines is protecting anything though, if the kernel is
more than 15M it will still overwrite that area.

> > Perhaps a better place for the pre-console buffer would be right before
> > the framebuffer (or at the top of RAM if no video on the board), with
> > modifications to bootm_size or not depending on the answer to the
> > original question above.
> 
> If this needs any kind of runtime address calculations instead of
> compile time constants, then IMHO it becomes unnecessarily complicated.

One for Hans I think, my understanding was that the framebuffer was at
the top of RAM, but having bootm_size set to 0xf0000000 unconditionally
doesn't match that. I suppose the idea is that it corresponds with the
smallest board because it's not worth making it dynamic (I think I
recall Hans saying something like that at the time).

I think you could safely put the early buffer at 0xf000000-DELTA (and
adjust bootm_size to match), rather than worrying about packing it up
below the real framebuffer.

> Anyway. The sunxi part of these changes just needs to assign some
> memory area to the pre-console buffer. In the end it does not really
> matter which one. The size also does not need to be too large.
> For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
> of the log buffer can fully cover the FullHD display using the 8x16
> font. And this is even assuming no line breaks. I picked 1M only
> because it was the smallest unit of the address space allocation in
> sunxi-common.h :-)

I don't think it needs to be allocated in 1M chunks, it just happens to
have been arbitrarily chosen that way so far.

If you want to keep the early buffer down in that region then I think
it'd be better to steal a few KB from the end of the fdt, script or pxe
(all of which will never be anywhere near 1MB) than stealing 1M off the
end of the kernel (it's not totally inconceivable that a kernel might be
approaching ~15M in size)

Ian.

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Simon Glass]

Hi,

On 10 January 2015 at 03:50, Ian Campbell <ijc@hellion.org.uk> wrote:
> On Fri, 2015-01-09 at 13:13 +0200, Siarhei Siamashka wrote:
>> > If yes then I think it is confusing to modify this comment, and the
>> > comment before the pre-console #defines should mention that the buffer
>> > is boottime only/short lived etc.
>>
>> Just in case if something goes really wrong (in theory it shouldn't,
>> but in practice you know...) it is still somewhat safer to keep the
>> buffer in its own dedicated area and keep everything else out.
>
> Nothing in those defines is protecting anything though, if the kernel is
> more than 15M it will still overwrite that area.
>
>> > Perhaps a better place for the pre-console buffer would be right before
>> > the framebuffer (or at the top of RAM if no video on the board), with
>> > modifications to bootm_size or not depending on the answer to the
>> > original question above.
>>
>> If this needs any kind of runtime address calculations instead of
>> compile time constants, then IMHO it becomes unnecessarily complicated.
>
> One for Hans I think, my understanding was that the framebuffer was at
> the top of RAM, but having bootm_size set to 0xf0000000 unconditionally
> doesn't match that. I suppose the idea is that it corresponds with the
> smallest board because it's not worth making it dynamic (I think I
> recall Hans saying something like that at the time).
>
> I think you could safely put the early buffer at 0xf000000-DELTA (and
> adjust bootm_size to match), rather than worrying about packing it up
> below the real framebuffer.
>
>> Anyway. The sunxi part of these changes just needs to assign some
>> memory area to the pre-console buffer. In the end it does not really
>> matter which one. The size also does not need to be too large.
>> For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
>> of the log buffer can fully cover the FullHD display using the 8x16
>> font. And this is even assuming no line breaks. I picked 1M only
>> because it was the smallest unit of the address space allocation in
>> sunxi-common.h :-)
>
> I don't think it needs to be allocated in 1M chunks, it just happens to
> have been arbitrarily chosen that way so far.
>
> If you want to keep the early buffer down in that region then I think
> it'd be better to steal a few KB from the end of the fdt, script or pxe
> (all of which will never be anywhere near 1MB) than stealing 1M off the
> end of the kernel (it's not totally inconceivable that a kernel might be
> approaching ~15M in size)

I don't think it is a good idea to use the 'pre-console buffer' after
the console exists. It is a misnomer.

Also, the reason that the pre-console buffer has pre-allocated memory
is to work around the lack of memory allocation before relocation. Now
that we have initf_mallloc() called very early in boot we could
consider allocating the space instead.

I think this patch is a good feature to implement, but I agree with
others that hard-coded memory locations for U-Boot features should not
exist except in exceptional circumstances (e.g. very early boot).

Re passing the U-Boot console to the kernel, see as an example the
cbmem_console.c driver. It only works on x86 at present and only with
coreboot. It works as a stdio driver, so skips the first part of
U-Boot's output.

So I suggest:

1. Remove the pre-console address and just have a size. Allocate the
space after initf_malloc(). Store the details (buffer start, size and
current pointer) in global_data
2. Add a general mechanism to record the console into a buffer by
renaming and adjusting the existing code. It can then be set up
pre-console, post-console but pre-stdio, and then post-stdio for
recording what is passed to Linux.

Regards,
Simon

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Siarhei Siamashka]

On Sat, 10 Jan 2015 10:50:05 +0000
Ian Campbell <ijc@hellion.org.uk> wrote:
> On Fri, 2015-01-09 at 13:13 +0200, Siarhei Siamashka wrote:
> > > If yes then I think it is confusing to modify this comment, and the
> > > comment before the pre-console #defines should mention that the buffer
> > > is boottime only/short lived etc.

First of all, I apologise for my earlier misleading comment about the
buffer life time.

In the FEL boot mode, the kernel and other things are uploaded to RAM
even before u-boot starts. So being short lived does not make any
difference. If the pre-console buffer is used, then it unconditionally
needs its own dedicated memory area, which does not overlap with
anything else.

> > Just in case if something goes really wrong (in theory it shouldn't,
> > but in practice you know...) it is still somewhat safer to keep the
> > buffer in its own dedicated area and keep everything else out.
> 
> Nothing in those defines is protecting anything though, if the kernel is
> more than 15M it will still overwrite that area.

If the kernel is more than 16M, then the defines do not protect anyone
either. And the situation is not very much different from 15M kernel
size limit. Both are just arbitrarily chosen numbers, which are
likely expected to be large enough (preferably several times larger
than the largest typical kernel).

This part of sunxi-common.h header only serves as the address space
layout documentation (given that no better documentation exists).
If a user has violated the rules and placed some important data
where he shouldn't, then it is reasonable to expect troubles.

> > > Perhaps a better place for the pre-console buffer would be right before
> > > the framebuffer (or at the top of RAM if no video on the board), with
> > > modifications to bootm_size or not depending on the answer to the
> > > original question above.
> > 
> > If this needs any kind of runtime address calculations instead of
> > compile time constants, then IMHO it becomes unnecessarily complicated.
> 
> One for Hans I think, my understanding was that the framebuffer was at
> the top of RAM, but having bootm_size set to 0xf0000000 unconditionally
> doesn't match that. I suppose the idea is that it corresponds with the
> smallest board because it's not worth making it dynamic (I think I
> recall Hans saying something like that at the time).
> 
> I think you could safely put the early buffer at 0xf000000-DELTA (and
> adjust bootm_size to match), rather than worrying about packing it up
> below the real framebuffer.

Right. It's one of the possible ways to do it.

> > Anyway. The sunxi part of these changes just needs to assign some
> > memory area to the pre-console buffer. In the end it does not really
> > matter which one. The size also does not need to be too large.
> > For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
> > of the log buffer can fully cover the FullHD display using the 8x16
> > font. And this is even assuming no line breaks. I picked 1M only
> > because it was the smallest unit of the address space allocation in
> > sunxi-common.h :-)
> 
> I don't think it needs to be allocated in 1M chunks, it just happens to
> have been arbitrarily chosen that way so far.

Yes, I just followed this arbitrarily chosen convention.

> If you want to keep the early buffer down in that region then I think
> it'd be better to steal a few KB from the end of the fdt, script or pxe
> (all of which will never be anywhere near 1MB) than stealing 1M off the
> end of the kernel

Right. It's one of the possible ways to do it.

> (it's not totally inconceivable that a kernel might be approaching ~15M
> in size)

To me this sounds like the existing 16M kernel reservation area is
insufficient. And may need to be increased in order to provide enough
headroom.

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Siarhei Siamashka]

On Sat, 10 Jan 2015 08:24:52 -0700
Simon Glass <sjg@chromium.org> wrote:

> Hi,
> 
> On 10 January 2015 at 03:50, Ian Campbell <ijc@hellion.org.uk> wrote:
> > On Fri, 2015-01-09 at 13:13 +0200, Siarhei Siamashka wrote:
> >> > If yes then I think it is confusing to modify this comment, and the
> >> > comment before the pre-console #defines should mention that the buffer
> >> > is boottime only/short lived etc.
> >>
> >> Just in case if something goes really wrong (in theory it shouldn't,
> >> but in practice you know...) it is still somewhat safer to keep the
> >> buffer in its own dedicated area and keep everything else out.
> >
> > Nothing in those defines is protecting anything though, if the kernel is
> > more than 15M it will still overwrite that area.
> >
> >> > Perhaps a better place for the pre-console buffer would be right before
> >> > the framebuffer (or at the top of RAM if no video on the board), with
> >> > modifications to bootm_size or not depending on the answer to the
> >> > original question above.
> >>
> >> If this needs any kind of runtime address calculations instead of
> >> compile time constants, then IMHO it becomes unnecessarily complicated.
> >
> > One for Hans I think, my understanding was that the framebuffer was at
> > the top of RAM, but having bootm_size set to 0xf0000000 unconditionally
> > doesn't match that. I suppose the idea is that it corresponds with the
> > smallest board because it's not worth making it dynamic (I think I
> > recall Hans saying something like that at the time).
> >
> > I think you could safely put the early buffer at 0xf000000-DELTA (and
> > adjust bootm_size to match), rather than worrying about packing it up
> > below the real framebuffer.
> >
> >> Anyway. The sunxi part of these changes just needs to assign some
> >> memory area to the pre-console buffer. In the end it does not really
> >> matter which one. The size also does not need to be too large.
> >> For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
> >> of the log buffer can fully cover the FullHD display using the 8x16
> >> font. And this is even assuming no line breaks. I picked 1M only
> >> because it was the smallest unit of the address space allocation in
> >> sunxi-common.h :-)
> >
> > I don't think it needs to be allocated in 1M chunks, it just happens to
> > have been arbitrarily chosen that way so far.
> >
> > If you want to keep the early buffer down in that region then I think
> > it'd be better to steal a few KB from the end of the fdt, script or pxe
> > (all of which will never be anywhere near 1MB) than stealing 1M off the
> > end of the kernel (it's not totally inconceivable that a kernel might be
> > approaching ~15M in size)
> 
> I don't think it is a good idea to use the 'pre-console buffer' after
> the console exists. It is a misnomer.

If one is fixated on the idea that "console" == "UART serial console",
then yes, keeping the pre-console buffer after the UART console
is initialized looks like a misnomer. However there are other
consoles too. And in the case of tablets, the "LCD console" may
be the only console that is (reasonably easily) accessible. IMHO,
the "pre-vga console buffer" interpretation is just as valid as the
"pre-serial console buffer".

Again, if we look at the pre-console buffer original idea. It just
does initial logging in the memory buffer because the serial console
becomes available relatively late and needs somewhat complicated
initialization (complicated compared to trivial logging in the memory
buffer). The vga console is in principle exactly the same, except that
it brings "late" and "complicated" to the entirely new level.

I understand that the pre-console buffer had been introduced to solve
one particular problem. But giving it a fresh look, we can see that it
already works perfectly fine for solving other problems too.
 
> Also, the reason that the pre-console buffer has pre-allocated memory
> is to work around the lack of memory allocation before relocation. Now
> that we have initf_mallloc() called very early in boot we could
> consider allocating the space instead.

My understanding is that one of the important reasons to have this
buffer at a known predictable memory location is to simplify JTAG
debugging.

> I think this patch is a good feature to implement, but I agree with
> others that hard-coded memory locations for U-Boot features should not
> exist except in exceptional circumstances (e.g. very early boot).

The platform code is in a perfect control of its address space. It can
make decisions about how much RAM to use (for example, assume that
256MB is the minimum possible amount of RAM on sunxi hardware), chop
off some amount of RAM for simplefb, etc. We are just having a hair
splitting argument with Ian about the suitable location and size of
the pre-console buffer. But no problem exists there in principle.

Doing the fixed memory location reservation provides absolutely the
best console log buffer. No malloc, UART hardware or anything else
is required. You can start logging right away. This could possibly
be enough to justify the exception.

Also because of assigning a fixed location to the pre-console buffer,
it is trivially easy to additionally extract and prepend the log from
SPL with really minimal changes. I can submit an updated patch with SPL
log extraction support just to demonstrate how it works.

> Re passing the U-Boot console to the kernel, see as an example the
> cbmem_console.c driver. It only works on x86 at present and only with
> coreboot.

Cool. It is good to know that this is already at least partially
solved. What makes it x86 and coreboot specific? Is it difficult
to make this usable on all platforms?

> It works as a stdio driver, so skips the first part of U-Boot's output.

By applying my pre-console buffer tweak and enabling pre-console in
the platform header file, the first part of U-Boot's output should be
available for cbmem_console too.

> So I suggest:
> 
> 1. Remove the pre-console address and just have a size. Allocate the
> space after initf_malloc(). Store the details (buffer start, size and
> current pointer) in global_data
> 2. Add a general mechanism to record the console into a buffer by
> renaming and adjusting the existing code. It can then be set up
> pre-console, post-console but pre-stdio, and then post-stdio for
> recording what is passed to Linux.

If this means getting rid of the fixed address for the pre-console
buffer reservation entirely, then we lose the ability to log before
malloc, JTAG debugging becomes more complicated and extracting the SPL
log is also not easy anymore. Or am I missing something?

If we keep the fixed pre-console buffer reservation, but migrate the
pre-console buffer into a new malloc allocated entity as soon as
malloc becomes available, then what do we really gain by doing this?

-- 
Best regards,
Siarhei Siamashka

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Simon Glass]

Hi Siarhei,

On 11 January 2015 at 16:28, Siarhei Siamashka
<siarhei.siamashka@gmail.com> wrote:
> On Sat, 10 Jan 2015 08:24:52 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
>> Hi,
>>
>> On 10 January 2015 at 03:50, Ian Campbell <ijc@hellion.org.uk> wrote:
>> > On Fri, 2015-01-09 at 13:13 +0200, Siarhei Siamashka wrote:
>> >> > If yes then I think it is confusing to modify this comment, and the
>> >> > comment before the pre-console #defines should mention that the buffer
>> >> > is boottime only/short lived etc.
>> >>
>> >> Just in case if something goes really wrong (in theory it shouldn't,
>> >> but in practice you know...) it is still somewhat safer to keep the
>> >> buffer in its own dedicated area and keep everything else out.
>> >
>> > Nothing in those defines is protecting anything though, if the kernel is
>> > more than 15M it will still overwrite that area.
>> >
>> >> > Perhaps a better place for the pre-console buffer would be right before
>> >> > the framebuffer (or at the top of RAM if no video on the board), with
>> >> > modifications to bootm_size or not depending on the answer to the
>> >> > original question above.
>> >>
>> >> If this needs any kind of runtime address calculations instead of
>> >> compile time constants, then IMHO it becomes unnecessarily complicated.
>> >
>> > One for Hans I think, my understanding was that the framebuffer was at
>> > the top of RAM, but having bootm_size set to 0xf0000000 unconditionally
>> > doesn't match that. I suppose the idea is that it corresponds with the
>> > smallest board because it's not worth making it dynamic (I think I
>> > recall Hans saying something like that at the time).
>> >
>> > I think you could safely put the early buffer at 0xf000000-DELTA (and
>> > adjust bootm_size to match), rather than worrying about packing it up
>> > below the real framebuffer.
>> >
>> >> Anyway. The sunxi part of these changes just needs to assign some
>> >> memory area to the pre-console buffer. In the end it does not really
>> >> matter which one. The size also does not need to be too large.
>> >> For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
>> >> of the log buffer can fully cover the FullHD display using the 8x16
>> >> font. And this is even assuming no line breaks. I picked 1M only
>> >> because it was the smallest unit of the address space allocation in
>> >> sunxi-common.h :-)
>> >
>> > I don't think it needs to be allocated in 1M chunks, it just happens to
>> > have been arbitrarily chosen that way so far.
>> >
>> > If you want to keep the early buffer down in that region then I think
>> > it'd be better to steal a few KB from the end of the fdt, script or pxe
>> > (all of which will never be anywhere near 1MB) than stealing 1M off the
>> > end of the kernel (it's not totally inconceivable that a kernel might be
>> > approaching ~15M in size)
>>
>> I don't think it is a good idea to use the 'pre-console buffer' after
>> the console exists. It is a misnomer.
>
> If one is fixated on the idea that "console" == "UART serial console",
> then yes, keeping the pre-console buffer after the UART console
> is initialized looks like a misnomer. However there are other
> consoles too. And in the case of tablets, the "LCD console" may
> be the only console that is (reasonably easily) accessible. IMHO,
> the "pre-vga console buffer" interpretation is just as valid as the
> "pre-serial console buffer".
>
> Again, if we look at the pre-console buffer original idea. It just
> does initial logging in the memory buffer because the serial console
> becomes available relatively late and needs somewhat complicated
> initialization (complicated compared to trivial logging in the memory
> buffer). The vga console is in principle exactly the same, except that
> it brings "late" and "complicated" to the entirely new level.
>
> I understand that the pre-console buffer had been introduced to solve
> one particular problem. But giving it a fresh look, we can see that it
> already works perfectly fine for solving other problems too.

I'm sorry, but pre-console means pre-console in my book. Using it
after the console is inited by console_init_f() is going to cause
confusion. If you are planning to add this function beyond
console_init_f() then it should be renamed to something like console
recording.

>
>> Also, the reason that the pre-console buffer has pre-allocated memory
>> is to work around the lack of memory allocation before relocation. Now
>> that we have initf_mallloc() called very early in boot we could
>> consider allocating the space instead.
>
> My understanding is that one of the important reasons to have this
> buffer at a known predictable memory location is to simplify JTAG
> debugging.

OK, I didn't know that. Still, the base of the malloc() area is fixed
for each board so in fact I doubt there would be any benefit to not
just using malloc(). Worth trying.

This is the first discussion I'm aware of on the topic.

http://patchwork.ozlabs.org/patch/112119/

Are you aware of these patches?

http://patchwork.ozlabs.org/patch/421220/
http://patchwork.ozlabs.org/patch/421224/

>
>> I think this patch is a good feature to implement, but I agree with
>> others that hard-coded memory locations for U-Boot features should not
>> exist except in exceptional circumstances (e.g. very early boot).
>
> The platform code is in a perfect control of its address space. It can
> make decisions about how much RAM to use (for example, assume that
> 256MB is the minimum possible amount of RAM on sunxi hardware), chop
> off some amount of RAM for simplefb, etc. We are just having a hair
> splitting argument with Ian about the suitable location and size of
> the pre-console buffer. But no problem exists there in principle.
>
> Doing the fixed memory location reservation provides absolutely the
> best console log buffer. No malloc, UART hardware or anything else
> is required. You can start logging right away. This could possibly
> be enough to justify the exception.

So is the real problem you are trying to solve that you don't have a
UART? But you do have JTAG? What kind of board is this?

malloc() is a convenient way to allocate memory - if you think of a
new board use it is much easier to just 'enable console recording' and
perhaps select a size than to try to figure out where to put it, and
perhaps overwrite something else.

Perhaps a solution would be not to require the address to be set. If
it is not set, then it can be malloc()'d.

Also I encourage you to look at the board_init_f() init sequence:

static init_fnc_t init_sequence_f[] = {
#ifdef CONFIG_SANDBOX
setup_ram_buf,
#endif
setup_mon_len,
setup_fdt,
#ifdef CONFIG_TRACE
trace_early_init,
#endif
initf_malloc,

I don't believe any board code can run before initf_malloc(). Before
this sequence there is no C stack so the console can't work.

So there really does not seem to be any point to a console that avoids
using malloc() so you can write stuff in these 4 functions.

>
> Also because of assigning a fixed location to the pre-console buffer,
> it is trivially easy to additionally extract and prepend the log from
> SPL with really minimal changes. I can submit an updated patch with SPL
> log extraction support just to demonstrate how it works.

>
>> Re passing the U-Boot console to the kernel, see as an example the
>> cbmem_console.c driver. It only works on x86 at present and only with
>> coreboot.
>
> Cool. It is good to know that this is already at least partially
> solved. What makes it x86 and coreboot specific? Is it difficult
> to make this usable on all platforms?

It is coreboot-specific because it puts the console data in a coreboot
table that is left around in RAM by coreboot. To make it useable on
ARM, for example, it should be changed to stick the data in the device
tree. See for example bootstage_fdt_add_report().

>
>> It works as a stdio driver, so skips the first part of U-Boot's output.
>
> By applying my pre-console buffer tweak and enabling pre-console in
> the platform header file, the first part of U-Boot's output should be
> available for cbmem_console too.

Yes I think so.

>
>> So I suggest:
>>
>> 1. Remove the pre-console address and just have a size. Allocate the
>> space after initf_malloc(). Store the details (buffer start, size and
>> current pointer) in global_data
>> 2. Add a general mechanism to record the console into a buffer by
>> renaming and adjusting the existing code. It can then be set up
>> pre-console, post-console but pre-stdio, and then post-stdio for
>> recording what is passed to Linux.
>
> If this means getting rid of the fixed address for the pre-console
> buffer reservation entirely, then we lose the ability to log before
> malloc, JTAG debugging becomes more complicated and extracting the SPL
> log is also not easy anymore. Or am I missing something?

Do you have a patch for SPL recording? Perhaps we should worry about that later?

But again with SPL, there should be essentially nothing before
malloc() is available so the address can be optional. Again, there
would be no stack before board_init_f(). We cannot call putc() until
global_data exists. I recently sent a series that sets things up for
driver model in SPL, so early malloc() is available.

>
> If we keep the fixed pre-console buffer reservation, but migrate the
> pre-console buffer into a new malloc allocated entity as soon as
> malloc becomes available, then what do we really gain by doing this?

My point is that there really isn't any benefit now to recording a
console before malloc() is available. See above.

It would also be nice to use this feature on platforms that 'lose'
their memory on relocation. In that case, any console recorded will no
longer be available.

Regards,
Simon

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Hans de Goede]

Hi,

On 10-01-15 11:50, Ian Campbell wrote:
> On Fri, 2015-01-09 at 13:13 +0200, Siarhei Siamashka wrote:
>>> If yes then I think it is confusing to modify this comment, and the
>>> comment before the pre-console #defines should mention that the buffer
>>> is boottime only/short lived etc.
>>
>> Just in case if something goes really wrong (in theory it shouldn't,
>> but in practice you know...) it is still somewhat safer to keep the
>> buffer in its own dedicated area and keep everything else out.
>
> Nothing in those defines is protecting anything though, if the kernel is
> more than 15M it will still overwrite that area.
>
>>> Perhaps a better place for the pre-console buffer would be right before
>>> the framebuffer (or at the top of RAM if no video on the board), with
>>> modifications to bootm_size or not depending on the answer to the
>>> original question above.
>>
>> If this needs any kind of runtime address calculations instead of
>> compile time constants, then IMHO it becomes unnecessarily complicated.
>
> One for Hans I think, my understanding was that the framebuffer was at
> the top of RAM, but having bootm_size set to 0xf0000000 unconditionally
> doesn't match that. I suppose the idea is that it corresponds with the
> smallest board because it's not worth making it dynamic (I think I
> recall Hans saying something like that at the time).

Correct, bootm_size used to be 0x10000000 to match the 256M RAM minimum
we've to deal with. Recently it changed to 0x0f000000 to deal with the
framebuffer being shaven of the top of RAM, that reserves 16M, but
the framebuffer is only 9M, so we could easily but the pre-console
buffer at 0x0f000000, that will work for all boards, and won't conflict
with the framebuffer.

So I suggest just putting it at 0x0f000000 until the time comes when
someone adds support for making it dynamic as Simon suggested.

>
> I think you could safely put the early buffer at 0xf000000-DELTA (and
> adjust bootm_size to match), rather than worrying about packing it up
> below the real framebuffer.

See above, there is room after the 0x0f000000 for it :)

>
>> Anyway. The sunxi part of these changes just needs to assign some
>> memory area to the pre-console buffer. In the end it does not really
>> matter which one. The size also does not need to be too large.
>> For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
>> of the log buffer can fully cover the FullHD display using the 8x16
>> font. And this is even assuming no line breaks. I picked 1M only
>> because it was the smallest unit of the address space allocation in
>> sunxi-common.h :-)
>
> I don't think it needs to be allocated in 1M chunks, it just happens to
> have been arbitrarily chosen that way so far.

Correct.

> If you want to keep the early buffer down in that region then I think
> it'd be better to steal a few KB from the end of the fdt, script or pxe
> (all of which will never be anywhere near 1MB) than stealing 1M off the
> end of the kernel (it's not totally inconceivable that a kernel might be
> approaching ~15M in size)

Agreed, as said I think the 7M hole between bootm_size and the framebuffer
is ideal, note the whole is much much bigger on boards with more then 256M
RAM, but less assume the worst case here.

Regards,

Hans

[U-Boot] [U-Boot, 1/2] console: Use pre-console buffer to get complete log on all consoles

[Hans de Goede]

Hi,

On 08-01-15 08:02, Siarhei Siamashka wrote:
> Currently the pre-console buffer can accumulate early log messages
> and flush them to the serial console as soon as it becomes available.
>
> This patch just adds one more pre-console buffer flushing point and
> does all the same for the other consoles too. This is particularly
> useful for the vga/hdmi/lcd console, where we can see all the older
> messages now (except for the log messages from SPL).
>
> Naturally, we don't want to get an extra copy of the log messages
> on the serial console again at the second flushing point, so the
> serial console has to be explicitly filtered out.
>
> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>

Thanks for writing this, this is something which I wanted to have
for a while now :)

Code wise this looks good to me:

Acked-by: Hans de Goede <hdegoede@redhat.com>

I wonder who should pick this patch up though, Tom can you pick
this up ?

Siarhei can you respin the sunxi patch for this using the address
I suggested in my other mail ?

Regards,

Hans


> ---
>   common/console.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 41 insertions(+), 5 deletions(-)
>
> diff --git a/common/console.c b/common/console.c
> index 29560c3..fc1963b 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -199,6 +199,20 @@ static void console_putc(int file, const char c)
>   	}
>   }
>
> +#ifdef CONFIG_PRE_CONSOLE_BUFFER
> +static void console_putc_noserial(int file, const char c)
> +{
> +	int i;
> +	struct stdio_dev *dev;
> +
> +	for (i = 0; i < cd_count[file]; i++) {
> +		dev = console_devices[file][i];
> +		if (dev->putc != NULL && strcmp(dev->name, "serial") != 0)
> +			dev->putc(dev, c);
> +	}
> +}
> +#endif
> +
>   static void console_puts(int file, const char *s)
>   {
>   	int i;
> @@ -236,6 +250,14 @@ static inline void console_putc(int file, const char c)
>   	stdio_devices[file]->putc(stdio_devices[file], c);
>   }
>
> +#ifdef CONFIG_PRE_CONSOLE_BUFFER
> +static inline void console_putc_noserial(int file, const char c)
> +{
> +	if (strcmp(stdio_devices[file]->name, "serial") != 0)
> +		stdio_devices[file]->putc(stdio_devices[file], c);
> +}
> +#endif
> +
>   static inline void console_puts(int file, const char *s)
>   {
>   	stdio_devices[file]->puts(stdio_devices[file], s);
> @@ -382,6 +404,9 @@ int tstc(void)
>   	return serial_tstc();
>   }
>
> +#define PRE_CONSOLE_FLUSHPOINT1_SERIAL			0
> +#define PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL	1
> +
>   #ifdef CONFIG_PRE_CONSOLE_BUFFER
>   #define CIRC_BUF_IDX(idx) ((idx) % (unsigned long)CONFIG_PRE_CON_BUF_SZ)
>
> @@ -398,7 +423,7 @@ static void pre_console_puts(const char *s)
>   		pre_console_putc(*s++);
>   }
>
> -static void print_pre_console_buffer(void)
> +static void print_pre_console_buffer(int flushpoint)
>   {
>   	unsigned long i = 0;
>   	char *buffer = (char *)CONFIG_PRE_CON_BUF_ADDR;
> @@ -407,12 +432,20 @@ static void print_pre_console_buffer(void)
>   		i = gd->precon_buf_idx - CONFIG_PRE_CON_BUF_SZ;
>
>   	while (i < gd->precon_buf_idx)
> -		putc(buffer[CIRC_BUF_IDX(i++)]);
> +		switch (flushpoint) {
> +		case PRE_CONSOLE_FLUSHPOINT1_SERIAL:
> +			putc(buffer[CIRC_BUF_IDX(i++)]);
> +			break;
> +		case PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL:
> +			console_putc_noserial(stdout,
> +					      buffer[CIRC_BUF_IDX(i++)]);
> +			break;
> +		}
>   }
>   #else
>   static inline void pre_console_putc(const char c) {}
>   static inline void pre_console_puts(const char *s) {}
> -static inline void print_pre_console_buffer(void) {}
> +static inline void print_pre_console_buffer(int flushpoint) {}
>   #endif
>
>   void putc(const char c)
> @@ -441,6 +474,7 @@ void putc(const char c)
>   		fputc(stdout, c);
>   	} else {
>   		/* Send directly to the handler */
> +		pre_console_putc(c);
>   		serial_putc(c);
>   	}
>   }
> @@ -472,6 +506,7 @@ void puts(const char *s)
>   		fputs(stdout, s);
>   	} else {
>   		/* Send directly to the handler */
> +		pre_console_puts(s);
>   		serial_puts(s);
>   	}
>   }
> @@ -679,7 +714,7 @@ int console_init_f(void)
>   		gd->flags |= GD_FLG_SILENT;
>   #endif
>
> -	print_pre_console_buffer();
> +	print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT1_SERIAL);
>
>   	return 0;
>   }
> @@ -794,6 +829,7 @@ done:
>   	if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL))
>   		return 0;
>   #endif
> +	print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL);
>   	return 0;
>   }
>
> @@ -869,7 +905,7 @@ int console_init_r(void)
>   	if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL))
>   		return 0;
>   #endif
> -
> +	print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL);
>   	return 0;
>   }
>
>

[U-Boot] [U-Boot, 1/2] console: Use pre-console buffer to get complete log on all consoles

[Tom Rini]

On Mon, Jan 12, 2015 at 12:05:57PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-01-15 08:02, Siarhei Siamashka wrote:
> >Currently the pre-console buffer can accumulate early log messages
> >and flush them to the serial console as soon as it becomes available.
> >
> >This patch just adds one more pre-console buffer flushing point and
> >does all the same for the other consoles too. This is particularly
> >useful for the vga/hdmi/lcd console, where we can see all the older
> >messages now (except for the log messages from SPL).
> >
> >Naturally, we don't want to get an extra copy of the log messages
> >on the serial console again at the second flushing point, so the
> >serial console has to be explicitly filtered out.
> >
> >Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> 
> Thanks for writing this, this is something which I wanted to have
> for a while now :)
> 
> Code wise this looks good to me:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Tom Rini <trini@ti.com>

> I wonder who should pick this patch up though, Tom can you pick
> this up ?

Throw it through sunxi when you've got the rest of the patches you want
that really make use of this ready too?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150112/dbc9a7f5/attachment.pgp>

[U-Boot] [U-Boot, 1/2] console: Use pre-console buffer to get complete log on all consoles

[Hans de Goede]

Hi,

On 12-01-15 14:30, Tom Rini wrote:
> On Mon, Jan 12, 2015 at 12:05:57PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 08-01-15 08:02, Siarhei Siamashka wrote:
>>> Currently the pre-console buffer can accumulate early log messages
>>> and flush them to the serial console as soon as it becomes available.
>>>
>>> This patch just adds one more pre-console buffer flushing point and
>>> does all the same for the other consoles too. This is particularly
>>> useful for the vga/hdmi/lcd console, where we can see all the older
>>> messages now (except for the log messages from SPL).
>>>
>>> Naturally, we don't want to get an extra copy of the log messages
>>> on the serial console again at the second flushing point, so the
>>> serial console has to be explicitly filtered out.
>>>
>>> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
>>
>> Thanks for writing this, this is something which I wanted to have
>> for a while now :)
>>
>> Code wise this looks good to me:
>>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
>
> Acked-by: Tom Rini <trini@ti.com>
>
>> I wonder who should pick this patch up though, Tom can you pick
>> this up ?
>
> Throw it through sunxi when you've got the rest of the patches you want
> that really make use of this ready too?

Sure that works for me.

Regards,

Hans

[U-Boot] [PATCH 2/2] sunxi: Enable pre-console buffer

[Simon Glass]

Hi Siarhei,

On 11 January 2015 at 18:04, Simon Glass <sjg@chromium.org> wrote:
> Hi Siarhei,
>
> On 11 January 2015 at 16:28, Siarhei Siamashka
> <siarhei.siamashka@gmail.com> wrote:
>> On Sat, 10 Jan 2015 08:24:52 -0700
>> Simon Glass <sjg@chromium.org> wrote:
>>
>>> Hi,
>>>
>>> On 10 January 2015 at 03:50, Ian Campbell <ijc@hellion.org.uk> wrote:
>>> > On Fri, 2015-01-09 at 13:13 +0200, Siarhei Siamashka wrote:
>>> >> > If yes then I think it is confusing to modify this comment, and the
>>> >> > comment before the pre-console #defines should mention that the buffer
>>> >> > is boottime only/short lived etc.
>>> >>
>>> >> Just in case if something goes really wrong (in theory it shouldn't,
>>> >> but in practice you know...) it is still somewhat safer to keep the
>>> >> buffer in its own dedicated area and keep everything else out.
>>> >
>>> > Nothing in those defines is protecting anything though, if the kernel is
>>> > more than 15M it will still overwrite that area.
>>> >
>>> >> > Perhaps a better place for the pre-console buffer would be right before
>>> >> > the framebuffer (or at the top of RAM if no video on the board), with
>>> >> > modifications to bootm_size or not depending on the answer to the
>>> >> > original question above.
>>> >>
>>> >> If this needs any kind of runtime address calculations instead of
>>> >> compile time constants, then IMHO it becomes unnecessarily complicated.
>>> >
>>> > One for Hans I think, my understanding was that the framebuffer was at
>>> > the top of RAM, but having bootm_size set to 0xf0000000 unconditionally
>>> > doesn't match that. I suppose the idea is that it corresponds with the
>>> > smallest board because it's not worth making it dynamic (I think I
>>> > recall Hans saying something like that at the time).
>>> >
>>> > I think you could safely put the early buffer at 0xf000000-DELTA (and
>>> > adjust bootm_size to match), rather than worrying about packing it up
>>> > below the real framebuffer.
>>> >
>>> >> Anyway. The sunxi part of these changes just needs to assign some
>>> >> memory area to the pre-console buffer. In the end it does not really
>>> >> matter which one. The size also does not need to be too large.
>>> >> For example 1920 * 1080 / (8 * 16) = 16200. It means that only ~16K
>>> >> of the log buffer can fully cover the FullHD display using the 8x16
>>> >> font. And this is even assuming no line breaks. I picked 1M only
>>> >> because it was the smallest unit of the address space allocation in
>>> >> sunxi-common.h :-)
>>> >
>>> > I don't think it needs to be allocated in 1M chunks, it just happens to
>>> > have been arbitrarily chosen that way so far.
>>> >
>>> > If you want to keep the early buffer down in that region then I think
>>> > it'd be better to steal a few KB from the end of the fdt, script or pxe
>>> > (all of which will never be anywhere near 1MB) than stealing 1M off the
>>> > end of the kernel (it's not totally inconceivable that a kernel might be
>>> > approaching ~15M in size)
>>>
>>> I don't think it is a good idea to use the 'pre-console buffer' after
>>> the console exists. It is a misnomer.
>>
>> If one is fixated on the idea that "console" == "UART serial console",
>> then yes, keeping the pre-console buffer after the UART console
>> is initialized looks like a misnomer. However there are other
>> consoles too. And in the case of tablets, the "LCD console" may
>> be the only console that is (reasonably easily) accessible. IMHO,
>> the "pre-vga console buffer" interpretation is just as valid as the
>> "pre-serial console buffer".

[snip]

Just to repeat I think this patch is a good feature. It's great to see
it. Also, so far as the implementation goes I have no objections - my
comments relate to plans to extend it further.

Regards,
Simon

[U-Boot] [U-Boot, 1/2] console: Use pre-console buffer to get complete log on all consoles

[Hans de Goede]

Hi,

On 12-01-15 12:05, Hans de Goede wrote:
> Hi,
>
> On 08-01-15 08:02, Siarhei Siamashka wrote:
>> Currently the pre-console buffer can accumulate early log messages
>> and flush them to the serial console as soon as it becomes available.
>>
>> This patch just adds one more pre-console buffer flushing point and
>> does all the same for the other consoles too. This is particularly
>> useful for the vga/hdmi/lcd console, where we can see all the older
>> messages now (except for the log messages from SPL).
>>
>> Naturally, we don't want to get an extra copy of the log messages
>> on the serial console again at the second flushing point, so the
>> serial console has to be explicitly filtered out.
>>
>> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
>
> Thanks for writing this, this is something which I wanted to have
> for a while now :)
>
> Code wise this looks good to me:
>
> Acked-by: Hans de Goede <hdegoede@redhat.com>
>
> I wonder who should pick this patch up though, Tom can you pick
> this up ?
>
> Siarhei can you respin the sunxi patch for this using the address
> I suggested in my other mail ?

Never mind, given Tom's ack to take 1/2 upstream through the sunxi
tree I've fixed up the address myself, and queued both for merging
in u-boot-sunxi/next

Regards,

Hans

>
> Regards,
>
> Hans
>
>
>> ---
>>   common/console.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 41 insertions(+), 5 deletions(-)
>>
>> diff --git a/common/console.c b/common/console.c
>> index 29560c3..fc1963b 100644
>> --- a/common/console.c
>> +++ b/common/console.c
>> @@ -199,6 +199,20 @@ static void console_putc(int file, const char c)
>>       }
>>   }
>>
>> +#ifdef CONFIG_PRE_CONSOLE_BUFFER
>> +static void console_putc_noserial(int file, const char c)
>> +{
>> +    int i;
>> +    struct stdio_dev *dev;
>> +
>> +    for (i = 0; i < cd_count[file]; i++) {
>> +        dev = console_devices[file][i];
>> +        if (dev->putc != NULL && strcmp(dev->name, "serial") != 0)
>> +            dev->putc(dev, c);
>> +    }
>> +}
>> +#endif
>> +
>>   static void console_puts(int file, const char *s)
>>   {
>>       int i;
>> @@ -236,6 +250,14 @@ static inline void console_putc(int file, const char c)
>>       stdio_devices[file]->putc(stdio_devices[file], c);
>>   }
>>
>> +#ifdef CONFIG_PRE_CONSOLE_BUFFER
>> +static inline void console_putc_noserial(int file, const char c)
>> +{
>> +    if (strcmp(stdio_devices[file]->name, "serial") != 0)
>> +        stdio_devices[file]->putc(stdio_devices[file], c);
>> +}
>> +#endif
>> +
>>   static inline void console_puts(int file, const char *s)
>>   {
>>       stdio_devices[file]->puts(stdio_devices[file], s);
>> @@ -382,6 +404,9 @@ int tstc(void)
>>       return serial_tstc();
>>   }
>>
>> +#define PRE_CONSOLE_FLUSHPOINT1_SERIAL            0
>> +#define PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL    1
>> +
>>   #ifdef CONFIG_PRE_CONSOLE_BUFFER
>>   #define CIRC_BUF_IDX(idx) ((idx) % (unsigned long)CONFIG_PRE_CON_BUF_SZ)
>>
>> @@ -398,7 +423,7 @@ static void pre_console_puts(const char *s)
>>           pre_console_putc(*s++);
>>   }
>>
>> -static void print_pre_console_buffer(void)
>> +static void print_pre_console_buffer(int flushpoint)
>>   {
>>       unsigned long i = 0;
>>       char *buffer = (char *)CONFIG_PRE_CON_BUF_ADDR;
>> @@ -407,12 +432,20 @@ static void print_pre_console_buffer(void)
>>           i = gd->precon_buf_idx - CONFIG_PRE_CON_BUF_SZ;
>>
>>       while (i < gd->precon_buf_idx)
>> -        putc(buffer[CIRC_BUF_IDX(i++)]);
>> +        switch (flushpoint) {
>> +        case PRE_CONSOLE_FLUSHPOINT1_SERIAL:
>> +            putc(buffer[CIRC_BUF_IDX(i++)]);
>> +            break;
>> +        case PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL:
>> +            console_putc_noserial(stdout,
>> +                          buffer[CIRC_BUF_IDX(i++)]);
>> +            break;
>> +        }
>>   }
>>   #else
>>   static inline void pre_console_putc(const char c) {}
>>   static inline void pre_console_puts(const char *s) {}
>> -static inline void print_pre_console_buffer(void) {}
>> +static inline void print_pre_console_buffer(int flushpoint) {}
>>   #endif
>>
>>   void putc(const char c)
>> @@ -441,6 +474,7 @@ void putc(const char c)
>>           fputc(stdout, c);
>>       } else {
>>           /* Send directly to the handler */
>> +        pre_console_putc(c);
>>           serial_putc(c);
>>       }
>>   }
>> @@ -472,6 +506,7 @@ void puts(const char *s)
>>           fputs(stdout, s);
>>       } else {
>>           /* Send directly to the handler */
>> +        pre_console_puts(s);
>>           serial_puts(s);
>>       }
>>   }
>> @@ -679,7 +714,7 @@ int console_init_f(void)
>>           gd->flags |= GD_FLG_SILENT;
>>   #endif
>>
>> -    print_pre_console_buffer();
>> +    print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT1_SERIAL);
>>
>>       return 0;
>>   }
>> @@ -794,6 +829,7 @@ done:
>>       if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL))
>>           return 0;
>>   #endif
>> +    print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL);
>>       return 0;
>>   }
>>
>> @@ -869,7 +905,7 @@ int console_init_r(void)
>>       if ((stdio_devices[stdin] == NULL) && (stdio_devices[stdout] == NULL))
>>           return 0;
>>   #endif
>> -
>> +    print_pre_console_buffer(PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL);
>>       return 0;
>>   }
>>
>>

[U-Boot] [U-Boot, 1/2] console: Use pre-console buffer to get complete log on all consoles

[Siarhei Siamashka]

On Tue, 13 Jan 2015 11:59:02 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 12-01-15 12:05, Hans de Goede wrote:
> > Hi,
> >
> > On 08-01-15 08:02, Siarhei Siamashka wrote:
> >> Currently the pre-console buffer can accumulate early log messages
> >> and flush them to the serial console as soon as it becomes available.
> >>
> >> This patch just adds one more pre-console buffer flushing point and
> >> does all the same for the other consoles too. This is particularly
> >> useful for the vga/hdmi/lcd console, where we can see all the older
> >> messages now (except for the log messages from SPL).
> >>
> >> Naturally, we don't want to get an extra copy of the log messages
> >> on the serial console again at the second flushing point, so the
> >> serial console has to be explicitly filtered out.
> >>
> >> Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com>
> >
> > Thanks for writing this, this is something which I wanted to have
> > for a while now :)
> >
> > Code wise this looks good to me:
> >
> > Acked-by: Hans de Goede <hdegoede@redhat.com>
> >
> > I wonder who should pick this patch up though, Tom can you pick
> > this up ?
> >
> > Siarhei can you respin the sunxi patch for this using the address
> > I suggested in my other mail ?
> 
> Never mind, given Tom's ack to take 1/2 upstream through the sunxi
> tree I've fixed up the address myself, and queued both for merging
> in u-boot-sunxi/next

Thanks! I was about to send an updated patch, but this works too.

I also submitted a natural evolution of this code, which allows to
see SPL messages on all consoles.

-- 
Best regards,
Siarhei Siamashka