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

2009-07-17 Thread Wolfgang Denk
Dear Stefan Roese,

In message 200907090654.02336...@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: w...@denx.de
Never put off until tomorrow what you can put off indefinitely.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-09 Thread Wolfgang Denk
Dear Scott Wood,

In message 4a551d5a.5060...@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: w...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-09 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-09 Thread 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: off...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-09 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread Wolfgang Denk
Dear Scott Wood,

In message 4a550b66.1090...@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: w...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread Scott Wood
Wolfgang Denk wrote:
 Dear Scott Wood,
 
 In message 4a550b66.1090...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread Wolfgang Denk
Dear Scott Wood,

In message 4a550fae.30...@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: w...@denx.de
If a train station is a place where a train stops,
   then what's a workstation?
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread 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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread Scott Wood
Wolfgang Denk wrote:
 Dear Scott Wood,
 
 In message 4a550fae.30...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread 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: off...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread 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: off...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-08 Thread 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: off...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-06 Thread 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 s...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-06 Thread Wolfgang Denk
Dear Stefan Roese,

In message 1246873688-25113-1-git-send-email...@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 s...@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: w...@denx.de
Research is what I'm doing when I don't know what I'm doing.
 -- Wernher von Braun
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


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

2009-07-06 Thread 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 s...@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: off...@denx.de
=
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot