Re: [U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target
Hi Stefan, So what should I do now? Should I revert to another #ifdef in the variable declaration? Or is the current version ok? I'm not too sure myself. What really tickles me, and what speaks against using this attribute, is the fact that the unused attribute is itself not part of an #ifdef, whereas the intention clearly is that this attribute should only be applied when the ifdefs erases code. BTW: The resulting code/data length is the same, comparing a version with #ifdef's, the attribute version or a version with the variable declaration intact and the warnings. Actually my argument was never about resulting code or data size, but about maintainability. My clear view here is to put as much information as possible into the sources. So if at all possible, let the compiler know. If this is not possible then let the user know by using constructs that at least show a connection to the human reader - i.e. using ifdef blocks with the same name. If that is also not possible then the least we should do is to write a comment with the original intent. Now currently this connection maybe clear for the writer of the patch, but it is in no way obvious in the code. So theoretically, when the #ifdef gets removed, nobody will think about the unused attributes, forget them and then we have effectively lost correct warnings. This could be the case. But this could happen to the #ifdef version as well. That the #ifdef'ed variable declaration stays in the code after removing the code referencing the variables. Someone touching one block with ifdefs will surely - or better should and *can* - check other blocks with the same condition. Using an __unused attribute breaks this connection. Cheers Detlev -- By all means let's be open-minded, but not so open-minded that our brains drop out. -- Richard Dawkins -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target
Hi Stefan,
This patch adds another build target for the AMCC Sequoia PPC440EPx
eval board. This RAM-booting version is targeted for boards without
NOR FLASH (NAND booting) which need a possibility to initially
program their NAND FLASH. Using a JTAG debugger (e.g. BDI2000/3000)
configured to setup the SDRAM, this debugger can load this RAM-
booting image to the target address in SDRAM (in this case 0x100)
and start it there. Then U-Boot's standard NAND commands can be
used to program the NAND FLASH (e.g. nand write ...).
Here the commands to load and start this image from the BDI2000:
440EPXload 0x100 /tftpboot/sequoia/u-boot.bin
440EPXgo 0x100
Please note that this image automatically scans for an already
initialized SDRAM TLB (detected by EPN=0). This TLB will not be
cleared. So your debugger should configure the SDRAM TLB correctly
(as done in the standard Sequoia BDI init script).
Signed-off-by: Stefan Roese s...@denx.de
---
Makefile | 11 +++
board/amcc/sequoia/init.S |7 ++
board/amcc/sequoia/sdram.c|3 +-
board/amcc/sequoia/sequoia.c | 12 +++-
board/amcc/sequoia/u-boot-ram.lds | 126
+
cpu/ppc4xx/start.S| 33 --
include/configs/amcc-common.h | 11 +++
include/configs/sequoia.h | 30 +++--
8 files changed, 215 insertions(+), 18 deletions(-)
create mode 100644 board/amcc/sequoia/u-boot-ram.lds
[...]
diff --git a/board/amcc/sequoia/sequoia.c b/board/amcc/sequoia/sequoia.c
index e824b8f..8b23823 100644
--- a/board/amcc/sequoia/sequoia.c
+++ b/board/amcc/sequoia/sequoia.c
@@ -33,7 +33,9 @@
DECLARE_GLOBAL_DATA_PTR;
+#if !defined(CONFIG_SYS_NO_FLASH)
extern flash_info_t flash_info[CONFIG_SYS_MAX_FLASH_BANKS]; /* info for
FLASH chips */
+#endif
extern void __ft_board_setup(void *blob, bd_t *bd);
ulong flash_get_size(ulong base, int banknum);
@@ -122,9 +124,9 @@ int board_early_init_f(void)
int misc_init_r(void)
{
- uint pbcr;
- int size_val = 0;
- u32 reg;
+ __attribute__((unused)) uint pbcr;
+ __attribute__((unused)) int size_val = 0;
+ __attribute__((unused)) u32 reg;
Am I correct to assume that this should shut up warnings for the ifdef
case? If so, it still seems to be a somewhat rude way to do it. How
long will it take the gcc maintainers to produce a warning: unused
variable is used warning? ;)
Cheers
Detlev
--
Wenn ein Kopf und ein Buch zusammenstossen und es klingt hohl; ist
denn das allemal im Buche?
- Lichtenberg
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target
Hi Detlev,
On Thursday 07 May 2009, Detlev Zundel wrote:
int misc_init_r(void)
{
- uint pbcr;
- int size_val = 0;
- u32 reg;
+ __attribute__((unused)) uint pbcr;
+ __attribute__((unused)) int size_val = 0;
+ __attribute__((unused)) u32 reg;
Am I correct to assume that this should shut up warnings for the ifdef
case?
Yes.
If so, it still seems to be a somewhat rude way to do it. How
long will it take the gcc maintainers to produce a warning: unused
variable is used warning? ;)
I prefer to do it this way instead of encasing the variable declaration into
another #ifdef ... #endif section. This is used in many cases in the Linux
kernel btw. Here the macro __maybe_unsed is defined to
__attribute__((unused)).
So what should I do now? Should I revert to another #ifdef in the variable
declaration? Or is the current version ok?
Thanks.
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: Add Sequoia RAM-booting target
Hi Stefan,
Hi Detlev,
On Thursday 07 May 2009, Detlev Zundel wrote:
int misc_init_r(void)
{
- uint pbcr;
- int size_val = 0;
- u32 reg;
+ __attribute__((unused)) uint pbcr;
+ __attribute__((unused)) int size_val = 0;
+ __attribute__((unused)) u32 reg;
Am I correct to assume that this should shut up warnings for the ifdef
case?
Yes.
If so, it still seems to be a somewhat rude way to do it. How
long will it take the gcc maintainers to produce a warning: unused
variable is used warning? ;)
I prefer to do it this way instead of encasing the variable declaration into
another #ifdef ... #endif section. This is used in many cases in the Linux
kernel btw. Here the macro __maybe_unsed is defined to
__attribute__((unused)).
In many cases? a rgrep on a recent kernel counts 84 incantations, which
is not much for the Linux kernel, I believe.
So what should I do now? Should I revert to another #ifdef in the variable
declaration? Or is the current version ok?
I'm not too sure myself. What really tickles me, and what speaks
against using this attribute, is the fact that the unused attribute is
itself not part of an #ifdef, whereas the intention clearly is that this
attribute should only be applied when the ifdefs erases code.
Now currently this connection maybe clear for the writer of the patch,
but it is in no way obvious in the code. So theoretically, when the
#ifdef gets removed, nobody will think about the unused attributes,
forget them and then we have effectively lost correct warnings.
What do other people think?
Cheers
Detlev
--
(let ((s bottles of beer on the wall)) ((lambda (f) (f f 99))
(lambda (f i) (or (= i 0) (format #t ~a ~a - take one down pass it around
~a ~a\n i s (- i 1) s) (f f (- i 1))
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: d...@denx.de
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target
On Thursday 07 May 2009, Detlev Zundel wrote: If so, it still seems to be a somewhat rude way to do it. How long will it take the gcc maintainers to produce a warning: unused variable is used warning? ;) I prefer to do it this way instead of encasing the variable declaration into another #ifdef ... #endif section. This is used in many cases in the Linux kernel btw. Here the macro __maybe_unsed is defined to __attribute__((unused)). In many cases? a rgrep on a recent kernel counts 84 incantations, which is not much for the Linux kernel, I believe. Perhaps it's quite new to the Linux kernel. I just spotted it the first time a few weeks ago and thought: What a nice way to remove some of the ugly #ifdef's in U-Boot!. :) So what should I do now? Should I revert to another #ifdef in the variable declaration? Or is the current version ok? I'm not too sure myself. What really tickles me, and what speaks against using this attribute, is the fact that the unused attribute is itself not part of an #ifdef, whereas the intention clearly is that this attribute should only be applied when the ifdefs erases code. BTW: The resulting code/data length is the same, comparing a version with #ifdef's, the attribute version or a version with the variable declaration intact and the warnings. Now currently this connection maybe clear for the writer of the patch, but it is in no way obvious in the code. So theoretically, when the #ifdef gets removed, nobody will think about the unused attributes, forget them and then we have effectively lost correct warnings. This could be the case. But this could happen to the #ifdef version as well. That the #ifdef'ed variable declaration stays in the code after removing the code referencing the variables. 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: Add Sequoia RAM-booting target
Dear Stefan,
In message 200905071530.43940...@denx.de you wrote:
On Thursday 07 May 2009, Detlev Zundel wrote:
int misc_init_r(void)
{
- uint pbcr;
- int size_val = 0;
- u32 reg;
+ __attribute__((unused)) uint pbcr;
+ __attribute__((unused)) int size_val = 0;
+ __attribute__((unused)) u32 reg;
Am I correct to assume that this should shut up warnings for the ifdef
case?
Well spotted, thanks.
I prefer to do it this way instead of encasing the variable declaration into
another #ifdef ... #endif section. This is used in many cases in the Linux
kernel btw. Here the macro __maybe_unsed is defined to
__attribute__((unused)).
I don;t accept this, though.
So what should I do now? Should I revert to another #ifdef in the variable
declaration? Or is the current version ok?
Please use #ifdef.
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
Conceptual integrity in turn dictates that the design must proceed
from one mind, or from a very small number of agreeing resonant
minds. - Frederick Brooks Jr., The Mythical Man Month
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target
Dear Stefan, in message 200905071739.56301...@denx.de you wrote: Linux kernel btw. Here the macro __maybe_unsed is defined to __attribute__((unused)). In many cases? a rgrep on a recent kernel counts 84 incantations, which is not much for the Linux kernel, I believe. Perhaps it's quite new to the Linux kernel. I just spotted it the first time a few weeks ago and thought: What a nice way to remove some of the ugly #ifdef's in U-Boot!. :) My understanding was that this is (only?) intended for function declarations to silence warnings about unused function arguments (which may be necessary anyway for compatible call interface with other functions that actually need this arg). This could be the case. But this could happen to the #ifdef version as well. That the #ifdef'ed variable declaration stays in the code after removing the code referencing the variables. No. In this case the compiler will issue warnings abouit unused variable. 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 Time is a drug. Too much of it kills you. - Terry Pratchett, _Small Gods_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target
Wolfgang Denk wrote: Dear Stefan, in message 200905071739.56301...@denx.de you wrote: Linux kernel btw. Here the macro __maybe_unsed is defined to __attribute__((unused)). In many cases? a rgrep on a recent kernel counts 84 incantations, which is not much for the Linux kernel, I believe. Perhaps it's quite new to the Linux kernel. I just spotted it the first time a few weeks ago and thought: What a nice way to remove some of the ugly #ifdef's in U-Boot!. :) My understanding was that this is (only?) intended for function declarations to silence warnings about unused function arguments (which may be necessary anyway for compatible call interface with other functions that actually need this arg). Unusued function argument warnings are not normally enabled in the first place (it's a separate warning class from unused variables). -Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target
Hi Wolfgang, On Thursday 07 May 2009, Wolfgang Denk wrote: Perhaps it's quite new to the Linux kernel. I just spotted it the first time a few weeks ago and thought: What a nice way to remove some of the ugly #ifdef's in U-Boot!. :) My understanding was that this is (only?) intended for function declarations to silence warnings about unused function arguments (which may be necessary anyway for compatible call interface with other functions that actually need this arg). No. This is not the case. Just take a look at the usage in drivers/net for example. You will see this construct is used here exactly to prevent those #ifdef's in the variable declaration in many cases: drivers/net/bnx2.c: int hw_vlan __maybe_unused = 0; ... #ifdef BCM_VLAN if (bp-vlgrp) hw_vlan = 1; else #endif But ok, if nobody else other than me prefers this version then I'll change to those #ifdef's again. 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
