| Message ID | 1420700552-17474-2-git-send-email-siarhei.siamashka@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Hans de Goede |
| Headers | show |
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.
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 :-)
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.
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
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.
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?
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
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
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
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" \
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(-)