[U-Boot] CFG_64BIT_xxx and friends
Hi all,
after testing the recent U-Boot code on a couple of 405EP boards I noticed, that the memsize in the output of the "bdinfo" command is always 0x00000000.
This is caused by using 64 types and format directives in printf that only work when CFG_64BIT_VSPRINTF is defined.
So what's the best way to fix this? Here are four solutions. My favorite is no. 2.
1) Define CFG_64BIT_STRTOUL for all effected board. Currently all 405 boards have memsize output as 0 in bdinfo.
2) Define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all 4xx boards in include/ppc4xx.h:
diff --git a/include/ppc4xx.h b/include/ppc4xx.h index 59a3b06..f0dfa38 100644 --- a/include/ppc4xx.h +++ b/include/ppc4xx.h @@ -102,13 +102,14 @@
#endif /* 440EP/EPX 440GR/GRX 440SP/SPE 460EX/GT/SX 405EX*/
-#if defined(CONFIG_440) /* * Enable long long (%ll ...) printf format on 440 PPC's since most of * them support 36bit physical addressing */ #define CFG_64BIT_VSPRINTF #define CFG_64BIT_STRTOUL + +#if defined(CONFIG_440) #include <ppc440.h> #else #include <ppc405.h>
3) Generally define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all boards.
4) Use an (ugly) workaround in common/cmd_bdinfo.c:
static void print_lnum(const char *name, u64 value) { #ifdef CFG_64BIT_VSPRINTF printf ("%-12s= 0x%.8llX\n", name, value); #else u32 *value32 = (u32*)&value; if (value32[0]) printf ("%-12s= 0x%lX%.8lX\n", name, value32[0], value32[1]); else printf ("%-12s= 0x%.8lX\n", name, value32[1]); #endif }
So what do you think?
Matthias
Dear Matthias,
in message 200809041609.26474.matthias.fuchs@esd-electronics.com you wrote:
after testing the recent U-Boot code on a couple of 405EP boards I noticed, that the memsize in the output of the "bdinfo" command is always 0x00000000.
This is caused by using 64 types and format directives in printf that only work when CFG_64BIT_VSPRINTF is defined.
Yeah, that's one more of these ugly bugs.
So what's the best way to fix this? Here are four solutions. My favorite is no. 2.
- Define CFG_64BIT_STRTOUL for all effected board.
Currently all 405 boards have memsize output as 0 in bdinfo.
- Define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all 4xx boards in
include/ppc4xx.h:
...
Generally define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all boards.
Use an (ugly) workaround in common/cmd_bdinfo.c:
I vote for # 5:
5) Delete al references to CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL and unconditionally enable it for all boards.
Any takers to submit a patch?
Best regards,
Wolfgang Denk
On 01:12 Sun 07 Sep , Wolfgang Denk wrote:
Dear Matthias,
in message 200809041609.26474.matthias.fuchs@esd-electronics.com you wrote:
after testing the recent U-Boot code on a couple of 405EP boards I noticed, that the memsize in the output of the "bdinfo" command is always 0x00000000.
This is caused by using 64 types and format directives in printf that only work when CFG_64BIT_VSPRINTF is defined.
Yeah, that's one more of these ugly bugs.
So what's the best way to fix this? Here are four solutions. My favorite is no. 2.
- Define CFG_64BIT_STRTOUL for all effected board.
Currently all 405 boards have memsize output as 0 in bdinfo.
- Define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all 4xx boards in
include/ppc4xx.h:
...
Generally define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all boards.
Use an (ugly) workaround in common/cmd_bdinfo.c:
I vote for # 5:
- Delete al references to CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL
and unconditionally enable it for all boards.
Any takers to submit a patch?
If possible not because it will increase the size of u-boot for board which not need it. When we will have kconfig it will be simple to activated it for ppc and other board automaticly.
Best Regards, J.
Dear Jean-Christophe PLAGNIOL-VILLARD,
In message 20080906230546.GI1249@game.jcrosoft.org you wrote:
- Delete al references to CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL
and unconditionally enable it for all boards.
Any takers to submit a patch?
If possible not because it will increase the size of u-boot for board which not need it. When we will have kconfig it will be simple to activated it for ppc and other board automaticly.
You try to beat me on being the guard dog of memory footprint? That'll be a hard job ;-)
Seriously: How much of code size are we talking about? And activating / deactivation the feature is not so trivial as it affects the printf() format specifiers we have to use.
Best regards,
Wolfgang Denk
On Sunday 07 September 2008, Wolfgang Denk wrote:
- Delete al references to CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL
and unconditionally enable it for all boards.
Any takers to submit a patch?
If possible not because it will increase the size of u-boot for board which not need it. When we will have kconfig it will be simple to activated it for ppc and other board automaticly.
You try to beat me on being the guard dog of memory footprint? That'll be a hard job ;-)
Seriously: How much of code size are we talking about? And activating / deactivation the feature is not so trivial as it affects the printf() format specifiers we have to use.
I'm with Wolfgang here and think it would be best to unconditionally support the 64bit printf format too.
Best regards, Stefan
===================================================================== DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de =====================================================================
Stefan Roese sr@denx.de wrote:
Seriously: How much of code size are we talking about? And activating / deactivation the feature is not so trivial as it affects the printf() format specifiers we have to use.
I'm with Wolfgang here and think it would be best to unconditionally support the 64bit printf format too.
Would be nice to see some numbers first though. I suspect there won't be much difference, but it could be the printf() implementation does something stupid, and it's much easier to tell when the config symbol is still in place.
Haavard
Here is the U-Boot size for the PLU405 board (405EP-based) with and without
#define CFG_64BIT_VSPRINTF #define CFG_64BIT_STRTOUL .
without: # ppc_4xx-size u-boot text data bss dec hex filename 289568 17532 301312 608412 9489c u-boot
with 64bit format handling: # ppc_4xx-size u-boot text data bss dec hex filename 291368 17532 301312 610212 94fa4 u-boot
So the difference is 1800 bytes on this architecture.
Matthias
On Monday 08 September 2008 13:00, Haavard Skinnemoen wrote:
Stefan Roese sr@denx.de wrote:
Seriously: How much of code size are we talking about? And activating / deactivation the feature is not so trivial as it affects the printf() format specifiers we have to use.
I'm with Wolfgang here and think it would be best to unconditionally support the 64bit printf format too.
Would be nice to see some numbers first though. I suspect there won't be much difference, but it could be the printf() implementation does something stupid, and it's much easier to tell when the config symbol is still in place.
Haavard
Matthias Fuchs matthias.fuchs@esd-electronics.com wrote:
Here is the U-Boot size for the PLU405 board (405EP-based) with and without
#define CFG_64BIT_VSPRINTF #define CFG_64BIT_STRTOUL .
without: # ppc_4xx-size u-boot text data bss dec hex filename 289568 17532 301312 608412 9489c u-boot
with 64bit format handling: # ppc_4xx-size u-boot text data bss dec hex filename 291368 17532 301312 610212 94fa4 u-boot
So the difference is 1800 bytes on this architecture.
Thanks.
That's a bit more than expected. Is this with or without --gc-sections? Linking with --gc-sections should make simple_strtoull() go away unless it's actually used.
Another thing that might hurt is that lib_generic/vsprintf.c reinvents do_div() without the out-of-line __div_64_32() bit. Converting it to use do_div() from include/div64.h should help.
In fact, enabling CFG_64BIT_VSPRINTF unconditionally without fixing this first will probably break all architectures that don't link with libgcc.
Haavard
Haavard Skinnemoen haavard.skinnemoen@atmel.com wrote:
That's a bit more than expected. Is this with or without --gc-sections? Linking with --gc-sections should make simple_strtoull() go away unless it's actually used.
That's assuming the fdt and image code doesn't interpret CFG_64BIT_VSPRINTF as CFG_BLOAT_ME_HARDER, which it does. So enabling CFG_64BIT_VSPRINTF does increase the code size even with --gc-sections.
I think fdt and common/image.c should stop abusing CFG_64BIT_VSPRINTF and get its own symbol instead, e.g. CFG_64BIT_PHYS_ADDR, and perhaps a nice str_to_addr() wrapper which selects between strtoul and strtoull based on this symbol.
Another thing that might hurt is that lib_generic/vsprintf.c reinvents do_div() without the out-of-line __div_64_32() bit. Converting it to use do_div() from include/div64.h should help.
It does. A lot. Here are some numbers from avr32:
vanilla: text data bss dec hex filename 96232 7528 216208 319968 4e1e0 ./u-boot
with CFG_64BIT_VSPRINTF: text data bss dec hex filename 98100 7528 216208 321836 4e92c ./u-boot
with CFG_64BIT_VSPRINTF and generic do_div(): text data bss dec hex filename 96396 7528 216208 320132 4e284 ./u-boot
So I highly recommend applying the patch below before killing CFG_64BIT_VSPRINTF.
In fact, enabling CFG_64BIT_VSPRINTF unconditionally without fixing this first will probably break all architectures that don't link with libgcc.
I was sort of expecting avr32 to break because of this, but it didn't. Apparently, the top-level Makefile adds -lgcc unconditionally...which makes it quite hard to catch undesired bloat like this.
Haavard
From: Haavard Skinnemoen haavard.skinnemoen@atmel.com Subject: vsprintf: Use generic do_div()
lib_generic/vsprintf.c uses its own reimplementation of do_div(). Switch it over to use the generic implementation from include/div64.h. This saves 2K of .text on avr32 with CFG_64BIT_VSPRINTF enabled.
Signed-off-by: Haavard Skinnemoen haavard.skinnemoen@atmel.com
diff --git a/lib_generic/vsprintf.c b/lib_generic/vsprintf.c index 7c9cfe1..a4f8a83 100644 --- a/lib_generic/vsprintf.c +++ b/lib_generic/vsprintf.c @@ -15,6 +15,7 @@ #include <linux/ctype.h>
#include <common.h> +#include <div64.h> #if !defined (CONFIG_PANIC_HANG) #include <command.h> /*cmd_boot.c*/ @@ -106,22 +107,6 @@ static int skip_atoi(const char **s) #define LARGE 64 /* use 'ABCDEF' instead of 'abcdef' */
#ifdef CFG_64BIT_VSPRINTF -#define do_div(n,base) ({ \ - unsigned int __res; \ - __res = ((unsigned long long) n) % base; \ - n = ((unsigned long long) n) / base; \ - __res; \ -}) -#else -#define do_div(n,base) ({ \ - int __res; \ - __res = ((unsigned long) n) % base; \ - n = ((unsigned long) n) / base; \ - __res; \ -}) -#endif - -#ifdef CFG_64BIT_VSPRINTF static char * number(char * str, long long num, unsigned int base, int size, int precision ,int type) #else static char * number(char * str, long num, unsigned int base, int size, int precision ,int type)
Haavard Skinnemoen wrote:
Haavard Skinnemoen haavard.skinnemoen@atmel.com wrote:
That's a bit more than expected. Is this with or without --gc-sections? Linking with --gc-sections should make simple_strtoull() go away unless it's actually used.
That's assuming the fdt and image code doesn't interpret CFG_64BIT_VSPRINTF as CFG_BLOAT_ME_HARDER, which it does. So enabling CFG_64BIT_VSPRINTF does increase the code size even with --gc-sections.
I think fdt and common/image.c should stop abusing CFG_64BIT_VSPRINTF and get its own symbol instead, e.g. CFG_64BIT_PHYS_ADDR, and perhaps a nice str_to_addr() wrapper which selects between strtoul and strtoull based on this symbol.
Hi Haavard,
fdt and common.image.c don't use CFG_64BIT_VSPRINTF:
$ find . -name "*.c" | xargs grep CFG_64BIT_VSPRINTF ./disk/part.c:#if defined(CFG_64BIT_LBA) && defined(CFG_64BIT_VSPRINTF) ./common/cmd_ide.c:#if defined(CFG_64BIT_LBA) && defined(CFG_64BIT_VSPRINTF) ./common/cmd_ide.c:#if defined(CFG_64BIT_LBA) && defined(CFG_64BIT_VSPRINTF) ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF
...they use CFG_64BIT_STRTOUL. If a config defines CFG_64BIT_STRTOUL, it is reasonable that the code uses it. I don't see any disadvantage of this vs. creating a new CFG_64BIT_PHYS_ADDR (although I would not object to that being created).
Only a select set of PowerPC targets actually define CFG_64BIT_STRTOUL:
$ find . -name "*.[ch]" | xargs grep CFG_64BIT_STRTOUL ./cpu/mpc85xx/mp.c:#ifdef CFG_64BIT_STRTOUL ./include/configs/MPC8540ADS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8572DS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8536DS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8548CDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8568MDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8541CDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8610HPCD.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8641HPCN.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/sbc8641d.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8555CDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8560ADS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8544DS.h:#define CFG_64BIT_STRTOUL 1 ./include/ppc4xx.h:#define CFG_64BIT_STRTOUL ./common/cmd_fdt.c:#ifdef CFG_64BIT_STRTOUL ./common/cmd_fdt.c:#ifdef CFG_64BIT_STRTOUL ./common/image.c:#ifdef CFG_64BIT_STRTOUL ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_STRTOUL ./lib_generic/vsprintf.c:#endif /* CFG_64BIT_STRTOUL */
[snip]
Best regards, gvb
Jerry Van Baren gvb.uboot@gmail.com wrote:
Haavard Skinnemoen wrote:
Haavard Skinnemoen haavard.skinnemoen@atmel.com wrote:
That's a bit more than expected. Is this with or without --gc-sections? Linking with --gc-sections should make simple_strtoull() go away unless it's actually used.
That's assuming the fdt and image code doesn't interpret CFG_64BIT_VSPRINTF as CFG_BLOAT_ME_HARDER, which it does. So enabling CFG_64BIT_VSPRINTF does increase the code size even with --gc-sections.
I think fdt and common/image.c should stop abusing CFG_64BIT_VSPRINTF and get its own symbol instead, e.g. CFG_64BIT_PHYS_ADDR, and perhaps a nice str_to_addr() wrapper which selects between strtoul and strtoull based on this symbol.
Hi Haavard,
fdt and common.image.c don't use CFG_64BIT_VSPRINTF:
$ find . -name "*.c" | xargs grep CFG_64BIT_VSPRINTF ./disk/part.c:#if defined(CFG_64BIT_LBA) && defined(CFG_64BIT_VSPRINTF) ./common/cmd_ide.c:#if defined(CFG_64BIT_LBA) && defined(CFG_64BIT_VSPRINTF) ./common/cmd_ide.c:#if defined(CFG_64BIT_LBA) && defined(CFG_64BIT_VSPRINTF) ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_VSPRINTF
...they use CFG_64BIT_STRTOUL.
Ah, sorry. I meant CFG_64BIT_STRTOUL.
If a config defines CFG_64BIT_STRTOUL, it is reasonable that the code uses it. I don't see any disadvantage of this vs. creating a new CFG_64BIT_PHYS_ADDR (although I would not object to that being created).
Just because a 64-bit strtoul exists doesn't mean you _have_ to use it.
Only a select set of PowerPC targets actually define CFG_64BIT_STRTOUL:
$ find . -name "*.[ch]" | xargs grep CFG_64BIT_STRTOUL ./cpu/mpc85xx/mp.c:#ifdef CFG_64BIT_STRTOUL ./include/configs/MPC8540ADS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8572DS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8536DS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8548CDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8568MDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8541CDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8610HPCD.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8641HPCN.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/sbc8641d.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8555CDS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8560ADS.h:#define CFG_64BIT_STRTOUL 1 ./include/configs/MPC8544DS.h:#define CFG_64BIT_STRTOUL 1 ./include/ppc4xx.h:#define CFG_64BIT_STRTOUL ./common/cmd_fdt.c:#ifdef CFG_64BIT_STRTOUL ./common/cmd_fdt.c:#ifdef CFG_64BIT_STRTOUL ./common/image.c:#ifdef CFG_64BIT_STRTOUL ./lib_generic/vsprintf.c:#ifdef CFG_64BIT_STRTOUL ./lib_generic/vsprintf.c:#endif /* CFG_64BIT_STRTOUL */
Ok, so CFG_64BIT_STRTOUL effectively means "I want 64-bit physical addresses" today. It's a bit unintuitive, but I guess we have bigger problems than that.
I must say it was a bit surprising that my u-boot image instantly grew by 200 bytes when I thought I just made a new function available, and --gc-sections was being used.
Haavard
participants (6)
-
Haavard Skinnemoen -
Jean-Christophe PLAGNIOL-VILLARD -
Jerry Van Baren -
Matthias Fuchs -
Stefan Roese -
Wolfgang Denk