[U-Boot] [PATCH] ppc4xx: Enable support for 64bit printf on all PPC4xx variants
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>
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
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@denx.de =====================================================================
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
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
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
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
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
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
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@denx.de =====================================================================
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
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
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
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@denx.de =====================================================================
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
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@denx.de =====================================================================
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
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@denx.de =====================================================================
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
participants (5)
-
Jerry Van Baren -
Jerry Van Baren -
Scott Wood -
Stefan Roese -
Wolfgang Denk