SPL: sunxi: don't force .BSS into DRAM
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 SPL: sunxi: don't force .BSS into DRAM
Hi Hans,
(CC:ing the list, which I accidentally forgot on this first post)
Quoted 15 lines · Hans de Goede
Hi Andre,
Quoted 11 lines · Andre Przywara
Probably due to some (ill-founded) fear of a large BSS all sunxi boards forced their SPL BSS section into DRAM.
This only works if there is no usage of a .BSS variable before the DRAM is initialised.
The recent inclusion of tiny-printf breaks this assumption (it has two variables in .BSS), so any early printf (printing a number) hangs a board.
This in particular breaks the (WIP) Pine64 SPL, which at the moment links Allwinner's libdram library, trying to print debug information:
DRAM:DRAM driver version: V1.0
DRAM Type = <hangs>
Quoted 8 lines
Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in DRAM, esp. since the bss use may grow over time, and the SPL space is quite small.
Moreover, given that tiny-printf is specifically meant for use in SPL / restricted environments and having BSS in DRAM is not unheard of for other boards, it seems to me like this is something which should really be fixed in tinyprintf instead.
I really don't know. This simple bug has cost me at least two hours yesterday, since it was the rather innocent access to a variable that caused the issue. And if it wouldn't have been for Siarhei to point me in the right direction I'd have spend even more time to find a fix.
This whole BSS in DRAM construct is very fragile and not obvious to blame for any bug that you happen to see. For instance we were assuming it to be due to unreliable SRAM C before.
Also we *do* a printf very early, which works because it doesn't involve any numbers.
And really I think it's quite a stretch to ask the casual programmer to keep in mind that some variables may end up in BSS and this must not happen before DRAM is initialised.
So either we find a way to make this break if BSS variables are involved in functions called before DRAM init (by somehow annotating files that are used before or during DRAM init).
Or we find other spaces for the BSS in other SRAMs.
Like I described here:
http://lists.denx.de/pipermail/u-boot/2016-June/259392.html
Cheers,
Andre.
Quoted 70 lines
Have you tried looking into fixing this at the tinyprintf level ?
Regards,
Hans
Quoted 63 lines
As it turns out the normal BSS size for sunxi is about 256 Bytes, so we can happily remove the symbols and the linker script part that was forcing the section into DRAM and let the linker naturally put it into SRAM.
Tested on BananaPi M1 and Pine64(-SPL), also buildman sunxi was happy.Thanks to Siarhei for providing helpful hints!
Signed-off-by Andre Przywara <[email protected]>--- arch/arm/cpu/armv7/sunxi/u-boot-spl.lds | 4 +--- include/configs/sunxi-common.h | 4 ---- 2 files changed, 1 insertion(+), 7 deletions(-)arch/arm/cpu/armv7/sunxi/u-boot-spl.lds +1
-3@@ -16,8 +16,6 @@ */ MEMORY { .sram : ORIGIN = CONFIG_SPL_TEXT_BASE,\ LENGTH = CONFIG_SPL_MAX_SIZE } -MEMORY { .sdram : ORIGIN = CONFIG_SPL_BSS_START_ADDR, \ - LENGTH = CONFIG_SPL_BSS_MAX_SIZE } OUTPUT_FORMAT("elf32-littlearm", "elf32-littlearm", "elf32-littlearm") OUTPUT_ARCH(arm)@@ -54,5 +52,5 @@ SECTIONS *(.bss*) . = ALIGN(4); __bss_end = .; - } > .sdram + } > .sram }include/configs/sunxi-common.h +0
-4@@ -75,7 +75,6 @@* since it needs to fit in with the other values. By also #defining it * we get warnings if the Kconfig value mismatches. */ #define CONFIG_SPL_STACK_R_ADDR 0x2fe00000 -#define CONFIG_SPL_BSS_START_ADDR 0x2ff80000#else #define SDRAM_OFFSET (x) 0x4##x#define CONFIG_SYS_SDRAM_BASE 0x40000000@@ -86,11 +85,8 @@* since it needs to fit in with the other values. By also #defining it * we get warnings if the Kconfig value mismatches. */ #define CONFIG_SPL_STACK_R_ADDR 0x4fe00000 -#define CONFIG_SPL_BSS_START_ADDR 0x4ff80000#endif -#define CONFIG_SPL_BSS_MAX_SIZE 0x00080000/* 512 KiB */ -#if defined(CONFIG_MACH_SUN9I ) || defined(CONFIG_MACH_SUN50I )/* * The A80's A1 sram starts at 0x00010000 rather then at 0x00000000 and is
PATCH 1/1 SPL: sunxi: don't force .BSS into DRAM
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Andre" == Andre Przywara <[email protected]> writes:
Hi,
Quoted 4 lines
I really don't know. This simple bug has cost me at least two hours yesterday, since it was the rather innocent access to a variable that caused the issue. And if it wouldn't have been for Siarhei to point me in the right direction I'd have spend even more time to find a fix.
Agreed. The toolchain will complain loudly about an overflow of SRAM space, but not about BSS access before DRAM is available.
Signature · 1 line
Bye, Peter Korsgaard
PATCH 1/1 SPL: sunxi: don't force .BSS into DRAM
Quoted 4 lines · Peter Korsgaard
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Andre" == Andre Przywara <[email protected]> writes:
Hi,
Quoted 8 lines
Quoted 4 lines
I really don't know. This simple bug has cost me at least two hours yesterday, since it was the rather innocent access to a variable that caused the issue. And if it wouldn't have been for Siarhei to point me in the right direction I'd have spend even more time to find a fix.
Agreed. The toolchain will complain loudly about an overflow of SRAM space, but not about BSS access before DRAM is available.
I see two problems:
- this fixes sunxi and possibly leaves other platforms unfixed
- when the spl on sunxi grows some more, the bss won't fit and you'll have a problem again, except much bigger this time
I'd rather have this fixed on tiny-printf level with a bit of documentation on why that's done the way it's done.
Signature · 2 lines
Best regards, Marek Vasut
A message that is not in the archive (sent off-list or not yet received): CAPnjgZ2ajoU90kSNMoHVhbXN4CdeKzs5-SQ-Hq-xePfC0brJ3A@mail.gmail.com
PATCH 1/1 SPL: sunxi: don't force .BSS into DRAM
Hi,
Quoted 28 lines · Simon Glass
Hi,
Quoted 25 lines · Hans de Goede
Hi Andre,
Quoted 11 lines · Andre Przywara
Probably due to some (ill-founded) fear of a large BSS all sunxi boards forced their SPL BSS section into DRAM.
This only works if there is no usage of a .BSS variable before the DRAM is initialised.
The recent inclusion of tiny-printf breaks this assumption (it has two variables in .BSS), so any early printf (printing a number) hangs a board. This in particular breaks the (WIP) Pine64 SPL, which at the moment links Allwinner's libdram library, trying to print debug information:
DRAM:DRAM driver version: V1.0
DRAM Type = <hangs>Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in DRAM, esp. since the bss use may grow over time, and the SPL space is quite small.
Moreover, given that tiny-printf is specifically meant for use in SPL / restricted environments and having BSS in DRAM is not unheard of for other boards, it seems to me like this is something which should really be fixed in tinyprintf instead.
Quoted 4 lines
Quoted 1 line
Have you tried looking into fixing this at the tinyprintf level ?
Marek's fix is one option. Another is to make use of
global_data, which will be available from very early and it set to zero.
I think Marek's fix is fine, we should go and merge that.
As for the worries about not using BSS before DRAM is initialized coming back to bite us. Yes that may happen, but we are not the only board / mach / soc code to do this. I agree that documenting this somewhere would be good (patches welcome).
As for just putting BSS in the sram, nack. Thanks to various efforts we currently have some free space for the SPL, but in the future we will likely add NAND support, and try to move the SPL to dm (with static device instantiation, no space for dt) so that we can get rid of the duplicate non-dm + dm gpio and uart code we currently have. These things combined will push things to their limit and may very well grow the BSS section too.
Regards,
Hans
PATCH 1/1 SPL: sunxi: don't force .BSS into DRAM
Hi Hans,
Quoted 53 lines · Hans de Goede
Hi,
Quoted 35 lines · Simon Glass
Hi,
Quoted 31 lines · Hans de Goede
Hi Andre,
Quoted 14 lines · Andre Przywara
Probably due to some (ill-founded) fear of a large BSS all sunxi boards forced their SPL BSS section into DRAM.
This only works if there is no usage of a .BSS variable before the DRAM is initialised.
The recent inclusion of tiny-printf breaks this assumption (it has two variables in .BSS), so any early printf (printing a number) hangs a board.
This in particular breaks the (WIP) Pine64 SPL, which at the moment links
Allwinner's libdram library, trying to print debug information:
DRAM:DRAM driver version: V1.0
DRAM Type = <hangs>Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in DRAM, esp. since the bss use may grow over time, and the SPL space is quite
small.Moreover, given that tiny-printf is specifically meant for use in SPL / restricted environments and having BSS in DRAM is not unheard of for other boards, it seems to me like this is something which should really be fixed in tinyprintf instead.
Quoted 5 lines
Quoted 1 line
Have you tried looking into fixing this at the tinyprintf level ?
Marek's fix is one option. Another is to make use of
global_data, which will be available from very early and it set to zero.I think Marek's fix is fine, we should go and merge that.
As for the worries about not using BSS before DRAM is initialized coming back to bite us. Yes that may happen, but we are not the only board / mach / soc code to do this. I agree that documenting this somewhere would be good (patches welcome).
Quoted 7 lines
As for just putting BSS in the sram, nack. Thanks to various efforts we currently have some free space for the SPL, but in the future we will likely add NAND support, and try to move the SPL to dm (with static device instantiation, no space for dt) so that we can get rid of the duplicate non-dm + dm gpio and uart code we currently have. These things combined will push things to their limit and may very well grow the BSS section too.
FWIW dm is designed to avoid using BSS. Also if it helps, see my series which converts the DT to C data structures.
Regards,
Simon
PATCH 1/1 SPL: sunxi: don't force .BSS into DRAM
In reply to Hans de GoedeOn 02/07/16 12:49, Hans de Goede wrote:
Hi Hans,
Quoted 32 lines · Simon Glass
Quoted 31 lines
Hi,
Quoted 28 lines · Hans de Goede
Hi Andre,
Quoted 13 lines · Andre Przywara
Probably due to some (ill-founded) fear of a large BSS all sunxi boards forced their SPL BSS section into DRAM.
This only works if there is no usage of a .BSS variable before the DRAM is initialised.
The recent inclusion of tiny-printf breaks this assumption (it has two variables in .BSS), so any early printf (printing a number) hangs a board.
This in particular breaks the (WIP) Pine64 SPL, which at the moment links
Allwinner's libdram library, trying to print debug information:
DRAM:DRAM driver version: V1.0
DRAM Type = <hangs>Hmm, although 256 bytes is not a lot I would prefer for BSS to stay in DRAM, esp. since the bss use may grow over time, and the SPL space is quite
small.Moreover, given that tiny-printf is specifically meant for use in SPL / restricted environments and having BSS in DRAM is not unheard of for other boards, it seems to me like this is something which should really be fixed in tinyprintf instead.
Quoted 6 lines
Quoted 4 lines
Quoted 1 line
Have you tried looking into fixing this at the tinyprintf level ?
Marek's fix is one option. Another is to make use of
global_data, which will be available from very early and it set to zero.I think Marek's fix is fine, we should go and merge that.
Only that it's not sufficient as it (all three variables should be covered). I have a better version which I will send.
I found the promising -fno-zero-initialized-in-bss gcc switch, but this apparently only works for *explicitly* initialized variables. So:
static char zs = 0;
is fine (moved to .data), but without the "= 0" it's still in BSS. Initialising those variables is pointless and confusing, so at least requires a comment. At this point I think we can just use the explicit section attribute.
A more involved way would be to avoid those global variables in the first place, but I don't think it's worth the effort.
Quoted 3 lines
As for the worries about not using BSS before DRAM is initialized coming back to bite us. Yes that may happen, but we are not the only board / mach / soc code to do this.
I don't think that's a good excuse ;-)
Quoted 2 lines
I agree that documenting
this somewhere would be good (patches welcome).
I wonder if there is a way to _enforce_ this.
Can't we somehow mark code that runs before DRAM init and check that this marked code doesn't have anything in BSS?
Or maybe tagging this code (or the files) and putting their BSS variables in SRAM, while letting the rest of the SPL use a DRAM based BSS? Having compile warnings or errors is much better than hard to debug breaks.
Quoted 7 lines
As for just putting BSS in the sram, nack. Thanks to various efforts we currently have some free space for the SPL, but in the future we will likely add NAND support, and try to move the SPL to dm (with static device instantiation, no space for dt) so that we can get rid of the duplicate non-dm + dm gpio and uart code we currently have. These things combined will push things to their limit and may very well grow the BSS section too.
I am bit worried that U-Boot is accepting hacks that paper over other hacks. Frankly: we shouldn't save memory beyond the point where it breaks things and makes them close to unmaintainable. Relying on variables to be in certain sections or not being accessed before a certain point in time is fragile (in fact it just broke!), so please note my protest against this approach.
We have more SRAM space available in many SoCs, so I will send another patch that makes use of them if possible (starting with the A64). That should be the route that we take for the future, limiting this "BSS in DRAM" hack to just the SoCs that are in desperate need for it.
Cheers,
Andre