[U-Boot] CFG_64BIT_xxx and friends

[U-Boot] CFG_64BIT_xxx and friends

[Matthias Fuchs]

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

[U-Boot] CFG_64BIT_xxx and friends

[Wolfgang Denk]

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.
> 
> 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:
...
> 3) Generally define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all boards.
> 
> 4) 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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"One day," said a dull voice from down below, "I'm going to  be  back
in  form again and you're going to be very sorry you said that. For a
very long time. I might even go so far as to make even more Time just
for you to be sorry in."              - Terry Pratchett, _Small Gods_

[U-Boot] CFG_64BIT_xxx and friends

[Jean-Christophe PLAGNIOL-VILLARD]

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.
> > 
> > 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:
> ...
> > 3) Generally define CFG_64BIT_VSPRINTF and CFG_64BIT_STRTOUL for all boards.
> > 
> > 4) 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?
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.

[U-Boot] CFG_64BIT_xxx and friends

[Wolfgang Denk]

Dear Jean-Christophe PLAGNIOL-VILLARD,

In message <20080906230546.GI1249@game.jcrosoft.org> you wrote:
>
> > 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.

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
We're all sorry for the other guy when he loses his job to a machine.
But when it comes to your job -- that's different. And it always will
be different.
	-- McCoy, "The Ultimate Computer", stardate 4729.4

[U-Boot] CFG_64BIT_xxx and friends

[Stefan Roese]

On Sunday 07 September 2008, Wolfgang Denk wrote:
> > > 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.
>
> 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 at denx.de
=====================================================================

[U-Boot] CFG_64BIT_xxx and friends

[Haavard Skinnemoen]

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

[U-Boot] CFG_64BIT_xxx and friends

[Matthias Fuchs]

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
> 
> 

[U-Boot] CFG_64BIT_xxx and friends

[Haavard Skinnemoen]

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

[U-Boot] CFG_64BIT_xxx and friends

[Haavard Skinnemoen]

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)

[U-Boot] CFG_64BIT_xxx and friends

[Jerry Van Baren]

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

[U-Boot] CFG_64BIT_xxx and friends

[Haavard Skinnemoen]

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