[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Stefan Roese]

Until now these defines were only enabled for the PPC440 variants. This
patch now defines CONFIG_SYS_64BIT_VSPRINTF and CONFIG_SYS_64BIT_STRTOUL
on PPC40x now as well. This removes the compilation warning with the new
updated NAND code.

Signed-off-by: Stefan Roese <sr@denx.de>
---
 include/ppc4xx.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/ppc4xx.h b/include/ppc4xx.h
index 55ff323..d9f04f7 100644
--- a/include/ppc4xx.h
+++ b/include/ppc4xx.h
@@ -109,13 +109,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 CONFIG_SYS_64BIT_VSPRINTF
 #define CONFIG_SYS_64BIT_STRTOUL
+
+#if defined(CONFIG_440)
 #include <ppc440.h>
 #else
 #include <ppc405.h>
-- 
1.6.3.3

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Wolfgang Denk]

Dear Stefan Roese,

In message <1246873688-25113-1-git-send-email-sr@denx.de> you wrote:
> Until now these defines were only enabled for the PPC440 variants. This
> patch now defines CONFIG_SYS_64BIT_VSPRINTF and CONFIG_SYS_64BIT_STRTOUL
> on PPC40x now as well. This removes the compilation warning with the new
> updated NAND code.
> 
> Signed-off-by: Stefan Roese <sr@denx.de>
> ---
>  include/ppc4xx.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/include/ppc4xx.h b/include/ppc4xx.h
> index 55ff323..d9f04f7 100644
> --- a/include/ppc4xx.h
> +++ b/include/ppc4xx.h
> @@ -109,13 +109,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 CONFIG_SYS_64BIT_VSPRINTF
>  #define CONFIG_SYS_64BIT_STRTOUL
> +
> +#if defined(CONFIG_440)
>  #include <ppc440.h>
>  #else
>  #include <ppc405.h>

NAK.

This needs to be cleaned up, but differently. Sorry that I did not
catch this earlier - it seems the related code was introduced by
commit f2302d44 (which sailed under the innocent title "Fix merge
problems").

Please move these CONFIG_SYS_* settings out of this file.

For me it is not acceptable to set configuration options in global
header files like include/ppc4xx.h; CONFIG_* settings are supposed to
be selected by the board configuration files, and only there.

I hope we don't have any more such #defines hidden in other header
files?

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
Research is what I'm doing when I don't know what I'm doing.
                                                 -- Wernher von Braun

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Stefan Roese]

Hi Wolfgang,

On Monday 06 July 2009 13:17:59 Wolfgang Denk wrote:
> > Until now these defines were only enabled for the PPC440 variants. This
> > patch now defines CONFIG_SYS_64BIT_VSPRINTF and CONFIG_SYS_64BIT_STRTOUL
> > on PPC40x now as well. This removes the compilation warning with the new
> > updated NAND code.
> >
> > Signed-off-by: Stefan Roese <sr@denx.de>
> > ---
> >  include/ppc4xx.h |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/ppc4xx.h b/include/ppc4xx.h
> > index 55ff323..d9f04f7 100644
> > --- a/include/ppc4xx.h
> > +++ b/include/ppc4xx.h
> > @@ -109,13 +109,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 CONFIG_SYS_64BIT_VSPRINTF
> >  #define CONFIG_SYS_64BIT_STRTOUL
> > +
> > +#if defined(CONFIG_440)
> >  #include <ppc440.h>
> >  #else
> >  #include <ppc405.h>
>
> NAK.
>
> This needs to be cleaned up, but differently. Sorry that I did not
> catch this earlier - it seems the related code was introduced by
> commit f2302d44 (which sailed under the innocent title "Fix merge
> problems").
>
> Please move these CONFIG_SYS_* settings out of this file.
>
> For me it is not acceptable to set configuration options in global
> header files like include/ppc4xx.h; CONFIG_* settings are supposed to
> be selected by the board configuration files, and only there.

I don't share this opinion. I think it's perfectly valid to enable CONFIG_* 
settings in global header files (or some other global files). Sometimes there 
are dependencies that can (or even should) be solved this way. One example is 
the 4xx NAND driver which needs to configure/set CONFIG_MTD_NAND_ECC_SMC for 
correct operation. This is currently done directly in 
drivers/mtd/nand/nand_ecc.c. The plan also proposed by Scott Wood is to remove 
this define from this file once we have the Kconfig framework intact. Then 
Kconfig will enable this define as a dependency for PPC4xx. As you know this 
is common praxis in Linux as well.

> I hope we don't have any more such #defines hidden in other header
> files?

I vote for completely removing these defines then (or at least 
CONFIG_SYS_64BIT_VSPRINTF) and by this enabling the 64bit printf format for 
all boards. I myself have hunted problems disguised by incorrect 64bit 
variables/values printed in some %ll formats a few times, before I noticed 
that this 64bit format it not supported at all.

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] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Wolfgang Denk]

Dear Stefan Roese,

In message <200907061739.33498.sr@denx.de> you wrote:
>
> > Please move these CONFIG_SYS_* settings out of this file.
> >
> > For me it is not acceptable to set configuration options in global
> > header files like include/ppc4xx.h; CONFIG_* settings are supposed to
> > be selected by the board configuration files, and only there.
> 
> I don't share this opinion. I think it's perfectly valid to enable CONFIG_* 
> settings in global header files (or some other global files). Sometimes there 
> are dependencies that can (or even should) be solved this way. One example is 
> the 4xx NAND driver which needs to configure/set CONFIG_MTD_NAND_ECC_SMC for 
> correct operation. This is currently done directly in 

In such a case please do not name the varoable CONFIG_.

Rule is: CONFIG_* and CONFIG_SYS_* are to be set / unset in the board
configuration files only (eventually with help of trickery  from  the
top level Makefile).

> drivers/mtd/nand/nand_ecc.c. The plan also proposed by Scott Wood is to remove 
> this define from this file once we have the Kconfig framework intact. Then 
> Kconfig will enable this define as a dependency for PPC4xx. As you know this 
> is common praxis in Linux as well.

Using Kconfig is one thing, and OK.

To me this is even more reason to forbid using CONFIG_* and
CONFIG_SYS_* outside board config files.

> > I hope we don't have any more such #defines hidden in other header
> > files?
> 
> I vote for completely removing these defines then (or at least 
> CONFIG_SYS_64BIT_VSPRINTF) and by this enabling the 64bit printf format for 
> all boards. I myself have hunted problems disguised by incorrect 64bit 

I don't want this because of the memory footprint.

Actually I'm pretty much annoyed we need this at all.

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
Where there's no emotion, there's no motive for violence.
	-- Spock, "Dagger of the Mind", stardate 2715.1

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Scott Wood]

Wolfgang Denk wrote:
>>> I hope we don't have any more such #defines hidden in other header
>>> files?
>> I vote for completely removing these defines then (or at least 
>> CONFIG_SYS_64BIT_VSPRINTF) and by this enabling the 64bit printf format for 
>> all boards. I myself have hunted problems disguised by incorrect 64bit 
> 
> I don't want this because of the memory footprint.
> 
> Actually I'm pretty much annoyed we need this at all.

Note that the overhead of 64-bit printf (assuming the 64-bit 
divide/remainder functions in libgcc aren't pulled in for anything else) 
  is 2524 bytes on powerpc (using GCC 4.3.2).  That's less than NFS, 
which gets turned on by default (as was brought up recently). :-)

What if we were to invert the option (CONFIG_SYS_NO_64BIT_VSPRINTF) so 
that it would only be disabled on boards at the intersection of tight 
space constraints and a board maintainer who's pretty sure this board 
doesn't need it?  There could also be some warning from printf() if %ll 
is used when not supported, and/or it could still check for %ll and pop 
a long long from the varargs but discard the high half.

-Scott

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Wolfgang Denk]

Dear Scott Wood,

In message <4A550B66.1090506@freescale.com> you wrote:
>
> Note that the overhead of 64-bit printf (assuming the 64-bit 
> divide/remainder functions in libgcc aren't pulled in for anything else) 
>   is 2524 bytes on powerpc (using GCC 4.3.2).  That's less than NFS, 
> which gets turned on by default (as was brought up recently). :-)

But compare the functionality...

> What if we were to invert the option (CONFIG_SYS_NO_64BIT_VSPRINTF) so 
> that it would only be disabled on boards at the intersection of tight 
> space constraints and a board maintainer who's pretty sure this board 
> doesn't need it?  There could also be some warning from printf() if %ll 
> is used when not supported, and/or it could still check for %ll and pop 
> a long long from the varargs but discard the high half.

And by default we would add CONFIG_SYS_NO_64BIT_VSPRINTF to all board
config files?

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
There is is no reason for any individual to have a computer in  their
home.      -- Ken Olsen (President of Digital Equipment Corporation),
              Convention of the World Future Society, in Boston, 1977

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Scott Wood]

Wolfgang Denk wrote:
> Dear Scott Wood,
> 
> In message <4A550B66.1090506@freescale.com> you wrote:
>> Note that the overhead of 64-bit printf (assuming the 64-bit 
>> divide/remainder functions in libgcc aren't pulled in for anything else) 
>>   is 2524 bytes on powerpc (using GCC 4.3.2).  That's less than NFS, 
>> which gets turned on by default (as was brought up recently). :-)
> 
> But compare the functionality...

I use 64-bit prints from time to time.  I've never used NFS in u-boot.

More relevantly, it's obvious if NFS is missing -- it says "Unknown 
command".  It's not always obvious whether (or why) the printf output 
you're looking at has been corrupted by an incomplete implementation.

>> What if we were to invert the option (CONFIG_SYS_NO_64BIT_VSPRINTF) so 
>> that it would only be disabled on boards at the intersection of tight 
>> space constraints and a board maintainer who's pretty sure this board 
>> doesn't need it?  There could also be some warning from printf() if %ll 
>> is used when not supported, and/or it could still check for %ll and pop 
>> a long long from the varargs but discard the high half.
> 
> And by default we would add CONFIG_SYS_NO_64BIT_VSPRINTF to all board
> config files?

No, that's the point -- it would require the board maintainer to 
explicitly say "this board doesn't need this".  By default we would 
provide a correct printf.

-Scott

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Wolfgang Denk]

Dear Scott Wood,

In message <4A550FAE.30500@freescale.com> you wrote:
>
> > And by default we would add CONFIG_SYS_NO_64BIT_VSPRINTF to all board
> > config files?
> 
> No, that's the point -- it would require the board maintainer to 
> explicitly say "this board doesn't need this".  By default we would 
> provide a correct printf.

But then applying this patch would break some boards that are working
now. Shirking off responsibility and have the board  maintainers  fix
it again is IMHO not the right thing to do.

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
If a train station is a place where a train stops,
                                           then what's a workstation?

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Scott Wood]

Wolfgang Denk wrote:
> Dear Scott Wood,
> 
> In message <4A550FAE.30500@freescale.com> you wrote:
>>> And by default we would add CONFIG_SYS_NO_64BIT_VSPRINTF to all board
>>> config files?
>> No, that's the point -- it would require the board maintainer to 
>> explicitly say "this board doesn't need this".  By default we would 
>> provide a correct printf.
> 
> But then applying this patch would break some boards that are working
> now. Shirking off responsibility and have the board  maintainers  fix
> it again is IMHO not the right thing to do.

What would break?  If things would no longer fit where they currently 
fit, that could happen on any change that increases code size -- 
possibly even just by changing compilers (this exact thing happened to 
the NAND bootstrap on some boards with very recent GCC).  That's life, 
and IMHO it is not reasonable to arbitrarily block changes that fix bugs 
because of the theoretical possibility that it might push someone's size 
over the limit.

A quick grep shows several instances of %ll/%L in other places that may 
not be obvious to the board maintainer (cmd_mmc, ubi, disk/part_efi, 
cpu/mpc8xxx/ddr, lmb, disk/part, cmd_ide, reiserfs).  Boards that use 
those without 64-bit printf are broken *right now*.

-Scott

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Stefan Roese]

On Thursday 09 July 2009 00:27:38 Scott Wood wrote:
> What would break?  If things would no longer fit where they currently
> fit, that could happen on any change that increases code size --
> possibly even just by changing compilers (this exact thing happened to
> the NAND bootstrap on some boards with very recent GCC).  That's life,
> and IMHO it is not reasonable to arbitrarily block changes that fix bugs
> because of the theoretical possibility that it might push someone's size
> over the limit.
>
> A quick grep shows several instances of %ll/%L in other places that may
> not be obvious to the board maintainer (cmd_mmc, ubi, disk/part_efi,
> cpu/mpc8xxx/ddr, lmb, disk/part, cmd_ide, reiserfs).  Boards that use
> those without 64-bit printf are broken *right now*.

Correct. And these problems are not easily spotted since you usually don't 
expect that "simple things" like a printf format are not working.

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] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Wolfgang Denk]

Dear Scott Wood,

In message <4A551D5A.5060300@freescale.com> you wrote:
>
> > But then applying this patch would break some boards that are working
> > now. Shirking off responsibility and have the board  maintainers  fix
> > it again is IMHO not the right thing to do.
> 
> What would break?  If things would no longer fit where they currently 
> fit, that could happen on any change that increases code size -- 

Right, which is why I continue to resist to certain changes that
increase code size.

> possibly even just by changing compilers (this exact thing happened to 
> the NAND bootstrap on some boards with very recent GCC).  That's life, 

But then changing the tool chain is something that is under control of
the board maintainer, so he is automatically also called on fixing any
problems resulting from this.

This is different from changes that other people are doing.

> A quick grep shows several instances of %ll/%L in other places that may 
> not be obvious to the board maintainer (cmd_mmc, ubi, disk/part_efi, 
> cpu/mpc8xxx/ddr, lmb, disk/part, cmd_ide, reiserfs).  Boards that use 
> those without 64-bit printf are broken *right now*.

Agreed. It would be good to get at least error messages for such
cases. It is not good that they go through unnoticed.

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
There's no sense in being precise  when  you  don't  even  know  what
you're talking about.                             -- John von Neumann

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Jerry Van Baren]

Scott Wood wrote:
> Wolfgang Denk wrote:
>>>> I hope we don't have any more such #defines hidden in other header
>>>> files?
>>> I vote for completely removing these defines then (or at least 
>>> CONFIG_SYS_64BIT_VSPRINTF) and by this enabling the 64bit printf format for 
>>> all boards. I myself have hunted problems disguised by incorrect 64bit 
>> I don't want this because of the memory footprint.

[snip]

> There could also be some warning from printf() if %ll 
> is used when not supported, and/or it could still check for %ll and pop 
> a long long from the varargs but discard the high half.
> 
> -Scott

Regardless of the in/out debate, we should print a warning if %ll is 
used but not supported.  I would suggest simply printing the "%lld" (or 
whatever the format is) and pop two longs from the varargs.  That would 
make it clear something is missing and probably wrong.

I don't like printing half and discarding half: it will be erroneous 
with no warning if the upper half != 0.  It would also have endian 
complications since the half you want to discard depends on the 
machine's endianness (not insurmountable).

One possible enhancement is to special-case %ll[0-9]*[Xx] and treat it 
as as two %08lx formats.  Hmmm, this would need correct endian handling 
too. :-/

Best regards,
gvb

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Scott Wood]

Jerry Van Baren wrote:
> Regardless of the in/out debate, we should print a warning if %ll is 
> used but not supported.  I would suggest simply printing the "%lld" (or 
> whatever the format is) and pop two longs from the varargs.  That would 
> make it clear something is missing and probably wrong.

Better to pop a long long -- two longs will pop too much if we ever have 
a 64-bit u-boot.  It would be perverse to not have 64-bit printf in such 
a case, but if it has to be manually selected, and only affects long 
long so as to not be immediately noticed, it could easily happen.

> I don't like printing half and discarding half: it will be erroneous 
> with no warning if the upper half != 0. 

Yes, but it'd be less erroneous than what we have now.

> It would also have endian 
> complications since the half you want to discard depends on the 
> machine's endianness (not insurmountable).

Popping a long long and then casting should take care of that.

-Scott

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Stefan Roese]

On Thursday 09 July 2009 00:18:54 Jerry Van Baren wrote:
> Regardless of the in/out debate, we should print a warning if %ll is
> used but not supported.  I would suggest simply printing the "%lld" (or
> whatever the format is) and pop two longs from the varargs.  That would
> make it clear something is missing and probably wrong.
>
> I don't like printing half and discarding half: it will be erroneous
> with no warning if the upper half != 0.  It would also have endian
> complications since the half you want to discard depends on the
> machine's endianness (not insurmountable).
>
> One possible enhancement is to special-case %ll[0-9]*[Xx] and treat it
> as as two %08lx formats.  Hmmm, this would need correct endian handling
> too. :-/

All this would increase the code size for those boards not supporting the 
64bit printf format. Not sure by how much, but I suggest to just fix the 
problem by supporting this format correctly instead of adding new code to 
print some warnings here.

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] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Jerry Van Baren]

Stefan Roese wrote:
> On Thursday 09 July 2009 00:18:54 Jerry Van Baren wrote:
>> Regardless of the in/out debate, we should print a warning if %ll is
>> used but not supported.  I would suggest simply printing the "%lld" (or
>> whatever the format is) and pop two longs from the varargs.  That would
>> make it clear something is missing and probably wrong.
>>
>> I don't like printing half and discarding half: it will be erroneous
>> with no warning if the upper half != 0.  It would also have endian
>> complications since the half you want to discard depends on the
>> machine's endianness (not insurmountable).
>>
>> One possible enhancement is to special-case %ll[0-9]*[Xx] and treat it
>> as as two %08lx formats.  Hmmm, this would need correct endian handling
>> too. :-/
> 
> All this would increase the code size for those boards not supporting the 
> 64bit printf format. Not sure by how much, but I suggest to just fix the 
> problem by supporting this format correctly instead of adding new code to 
> print some warnings here.
> 
> Best regards,
> Stefan

That is what Scott is trying to do, but fixing 64bit printf causes a 
2K++ increase in size to the boards that don't currently support 64bit 
printf (some of which are broken due to the error).  Wolfgang is 
resisting that.

Adding code to print a warning and handle the varargs properly will 
probably be less than 100 bytes.  It looks like this is the compromise 
Wolfgang favors.

On a related note, I am *strongly* opposed to popping a long long and 
only printing half of it.  While odds are good that it will be correct, 
when the upper half is non-zero, it will be silently printing a totally 
erroneous value.

FWIIW, in the avionics business, silently displaying an erroneous value 
is the unpardonable sin.
   <http://en.wikipedia.org/wiki/Unpardonable_sin>
It is better to sometimes not display correct data and never display 
incorrect data than it is to rarely (as in 1e-9 .. 1e-12) display 
incorrect data.  Hundreds of people have died because of this.[1]

Best regards,
gvb

[1] Just a couple examples:
<http://online.wsj.com/article/SB124411224440184797.html> (unknown 
cause, but incorrect airspeed indication is a known contributing factor)
<http://www.computerweekly.com/blogs/tony_collins/2009/06/youre-flying-too-fast-and-too.html>
<http://www.computerweekly.com/Articles/2009/06/12/236425/crash-one-birgenair-flight-301.htm>
<http://www.computerweekly.com/Articles/2009/06/12/236431/crash-two-aeroper-flight-603.htm>

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Stefan Roese]

On Thursday 09 July 2009 14:24:49 Jerry Van Baren wrote:
> > All this would increase the code size for those boards not supporting the
> > 64bit printf format. Not sure by how much, but I suggest to just fix the
> > problem by supporting this format correctly instead of adding new code to
> > print some warnings here.
> >
> > Best regards,
> > Stefan
>
> That is what Scott is trying to do, but fixing 64bit printf causes a
> 2K++ increase in size to the boards that don't currently support 64bit
> printf (some of which are broken due to the error).  Wolfgang is
> resisting that.
>
> Adding code to print a warning and handle the varargs properly will
> probably be less than 100 bytes.  It looks like this is the compromise
> Wolfgang favors.

I doubt that this could be done in less than 100 bytes. A descriptive error 
message string alone will probably be around >= 60 chars. But I know this is 
nitpicking.

I'm still voting for adding this 2k and doing it correctly on all boards. With 
the option to disable this 64bit support (as Scott suggested) on boards with 
very tight resources.

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] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Jerry Van Baren]

Stefan Roese wrote:
> On Thursday 09 July 2009 14:24:49 Jerry Van Baren wrote:
>>> All this would increase the code size for those boards not supporting the
>>> 64bit printf format. Not sure by how much, but I suggest to just fix the
>>> problem by supporting this format correctly instead of adding new code to
>>> print some warnings here.
>>>
>>> Best regards,
>>> Stefan
>> That is what Scott is trying to do, but fixing 64bit printf causes a
>> 2K++ increase in size to the boards that don't currently support 64bit
>> printf (some of which are broken due to the error).  Wolfgang is
>> resisting that.
>>
>> Adding code to print a warning and handle the varargs properly will
>> probably be less than 100 bytes.  It looks like this is the compromise
>> Wolfgang favors.
> 
> I doubt that this could be done in less than 100 bytes. A descriptive error 
> message string alone will probably be around >= 60 chars. But I know this is 
> nitpicking.

Agreed.  FWIIW, I wasn't assuming a /descriptive/ error message.  I was 
assuming printf would simply print the format string, e.g. "%lld", 
rather than a possibly erroneous value.  Another alternative would be to 
do the spreadsheet idiom and print hashes "########".

> I'm still voting for adding this 2k and doing it correctly on all boards. With 
> the option to disable this 64bit support (as Scott suggested) on boards with 
> very tight resources.

Me too.  <shrug>

> Best regards,
> Stefan

Thanks,
gvb

[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Stefan Roese]

Hi Wolfgang,

On Wednesday 08 July 2009 22:01:23 Wolfgang Denk wrote:
> > > Please move these CONFIG_SYS_* settings out of this file.
> > >
> > > For me it is not acceptable to set configuration options in global
> > > header files like include/ppc4xx.h; CONFIG_* settings are supposed to
> > > be selected by the board configuration files, and only there.
> >
> > I don't share this opinion. I think it's perfectly valid to enable
> > CONFIG_* settings in global header files (or some other global files).
> > Sometimes there are dependencies that can (or even should) be solved this
> > way. One example is the 4xx NAND driver which needs to configure/set
> > CONFIG_MTD_NAND_ECC_SMC for correct operation. This is currently done
> > directly in
>
> In such a case please do not name the varoable CONFIG_.

I didn't "name" it this way. It's a plain copy from Linux. And this is done 
intentionally so that we can easily "borrow" code from there. Changing it's 
name (and this is just one example, there are most likely many of those ones) 
into something else would be plain stupid from my point of view.

> Rule is: CONFIG_* and CONFIG_SYS_* are to be set / unset in the board
> configuration files only (eventually with help of trickery  from  the
> top level Makefile).

Then please grep for "#define CONFIG_" in the include directory. You will be 
surprised how many of these defines there are outside of the include/configs 
directory.

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] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants

[Wolfgang Denk]

Dear Stefan Roese,

In message <200907090654.02336.sr@denx.de> you wrote:
> 
> Then please grep for "#define CONFIG_" in the include directory. You will be 
> surprised how many of these defines there are outside of the include/configs 
> directory.

This is no excuse for adding even more such code.

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
Never put off until tomorrow what you can put off indefinitely.