[U-Boot] [PATCH] ppc4xx: Add Sequoia RAM-booting target

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 0x1000000) 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:
440EPX>load 0x1000000 /tftpboot/sequoia/u-boot.bin 440EPX>go 0x1000000
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 sr@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/Makefile b/Makefile index 137c88f..1ae93ab 100644 --- a/Makefile +++ b/Makefile @@ -1533,6 +1533,17 @@ rainier_nand_config: unconfig @echo "TEXT_BASE = 0x01000000" > $(obj)board/amcc/sequoia/config.tmp @echo "CONFIG_NAND_U_BOOT = y" >> $(obj)include/config.mk
+sequoia_ramboot_config \ +rainier_ramboot_config: unconfig + @mkdir -p $(obj)include $(obj)board/amcc/sequoia + @echo "#define CONFIG_SYS_RAMBOOT" > $(obj)include/config.h + @echo "#define CONFIG_$$(echo $(subst ,,$(@:_config=)) | \ + tr '[:lower:]' '[:upper:]')" >> $(obj)include/config.h + @$(MKCONFIG) -n $@ -a sequoia ppc ppc4xx sequoia amcc + @echo "TEXT_BASE = 0x01000000" > $(obj)board/amcc/sequoia/config.tmp + @echo "LDSCRIPT = board/amcc/sequoia/u-boot-ram.lds" >> \ + $(obj)board/amcc/sequoia/config.tmp + taihu_config: unconfig @$(MKCONFIG) $(@:_config=) ppc ppc4xx taihu amcc
diff --git a/board/amcc/sequoia/init.S b/board/amcc/sequoia/init.S index 4452d26..3c0e400 100644 --- a/board/amcc/sequoia/init.S +++ b/board/amcc/sequoia/init.S @@ -43,12 +43,19 @@ tlbtab: /* vxWorks needs this as first entry for the Machine Check interrupt */ tlbentry( 0x40000000, SZ_256M, 0, 0, AC_R|AC_W|AC_X|SA_G|SA_I )
+ /* + * The RAM-boot version skips the SDRAM TLB (identified by EPN=0). This + * entry is already configured for SDRAM via the JTAG debugger and mustn't + * be re-initialized by this RAM-booting U-Boot version. + */ +#ifndef CONFIG_SYS_RAMBOOT /* TLB-entry for DDR SDRAM (Up to 2GB) */ #ifdef CONFIG_4xx_DCACHE tlbentry( CONFIG_SYS_SDRAM_BASE, SZ_256M, CONFIG_SYS_SDRAM_BASE, 0, AC_R|AC_W|AC_X|SA_G) #else tlbentry( CONFIG_SYS_SDRAM_BASE, SZ_256M, CONFIG_SYS_SDRAM_BASE, 0, AC_R|AC_W|AC_X|SA_G|SA_I ) #endif +#endif /* CONFIG_SYS_RAMBOOT */
/* TLB-entry for EBC */ tlbentry( CONFIG_SYS_BCSR_BASE, SZ_256M, CONFIG_SYS_BCSR_BASE, 1, AC_R|AC_W|AC_X|SA_G|SA_I ) diff --git a/board/amcc/sequoia/sdram.c b/board/amcc/sequoia/sdram.c index 6df4c6d..bde471c 100644 --- a/board/amcc/sequoia/sdram.c +++ b/board/amcc/sequoia/sdram.c @@ -54,7 +54,8 @@ extern void denali_core_search_data_eye(void); ************************************************************************/ phys_size_t initdram (int board_type) { -#if !defined(CONFIG_NAND_U_BOOT) || defined(CONFIG_NAND_SPL) +#if !(defined(CONFIG_NAND_U_BOOT) || defined(CONFIG_SYS_RAMBOOT)) || \ + defined(CONFIG_NAND_SPL) ulong speed = get_bus_freq(0);
mtsdram(DDR0_02, 0x00000000); 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; #ifdef CONFIG_440EPX unsigned long usb2d0cr = 0; unsigned long usb2phy0cr, usb2h0cr = 0; @@ -132,6 +134,7 @@ int misc_init_r(void) char *act = getenv("usbact"); #endif
+#if !defined(CONFIG_SYS_NO_FLASH) /* Re-do flash sizing to get full correct info */
/* adjust flash start and offset */ @@ -171,6 +174,7 @@ int misc_init_r(void) CONFIG_ENV_ADDR_REDUND + 2*CONFIG_ENV_SECT_SIZE - 1, &flash_info[0]); #endif +#endif /* CONFIG_SYS_NO_FLASH */
/* * USB suff... @@ -515,7 +519,7 @@ int post_hotkeys_pressed(void) } #endif /* CONFIG_POST */
-#if defined(CONFIG_NAND_U_BOOT) +#if defined(CONFIG_NAND_U_BOOT) || defined(CONFIG_SYS_RAMBOOT) /* * On NAND-booting sequoia, we need to patch the chips select numbers * in the dtb (CS0 - NAND, CS3 - NOR) diff --git a/board/amcc/sequoia/u-boot-ram.lds b/board/amcc/sequoia/u-boot-ram.lds new file mode 100644 index 0000000..9393b65 --- /dev/null +++ b/board/amcc/sequoia/u-boot-ram.lds @@ -0,0 +1,126 @@ +/* + * (C) Copyright 2009 + * Stefan Roese, DENX Software Engineering, sr@denx.de. + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +OUTPUT_ARCH(powerpc) +SECTIONS +{ + /* Read-only sections, merged into text segment: */ + . = + SIZEOF_HEADERS; + .interp : { *(.interp) } + .hash : { *(.hash) } + .dynsym : { *(.dynsym) } + .dynstr : { *(.dynstr) } + .rel.text : { *(.rel.text) } + .rela.text : { *(.rela.text) } + .rel.data : { *(.rel.data) } + .rela.data : { *(.rela.data) } + .rel.rodata : { *(.rel.rodata) } + .rela.rodata : { *(.rela.rodata) } + .rel.got : { *(.rel.got) } + .rela.got : { *(.rela.got) } + .rel.ctors : { *(.rel.ctors) } + .rela.ctors : { *(.rela.ctors) } + .rel.dtors : { *(.rel.dtors) } + .rela.dtors : { *(.rela.dtors) } + .rel.bss : { *(.rel.bss) } + .rela.bss : { *(.rela.bss) } + .rel.plt : { *(.rel.plt) } + .rela.plt : { *(.rela.plt) } + .init : { *(.init) } + .plt : { *(.plt) } + .text : + { + cpu/ppc4xx/start.o (.text) + + *(.text) + *(.fixup) + *(.got1) + } + _etext = .; + PROVIDE (etext = .); + .rodata : + { + *(SORT_BY_ALIGNMENT(SORT_BY_NAME(.rodata*))) + } + .fini : { *(.fini) } =0 + .ctors : { *(.ctors) } + .dtors : { *(.dtors) } + + /* Read-write section, merged into data segment: */ + . = (. + 0x00FF) & 0xFFFFFF00; + _erotext = .; + PROVIDE (erotext = .); + .reloc : + { + *(.got) + _GOT2_TABLE_ = .; + *(.got2) + _FIXUP_TABLE_ = .; + *(.fixup) + } + __got2_entries = (_FIXUP_TABLE_ - _GOT2_TABLE_) >>2; + __fixup_entries = (. - _FIXUP_TABLE_)>>2; + + .data : + { + *(.data) + *(.data1) + *(.sdata) + *(.sdata2) + *(.dynamic) + CONSTRUCTORS + } + _edata = .; + PROVIDE (edata = .); + + . = .; + __u_boot_cmd_start = .; + .u_boot_cmd : { *(.u_boot_cmd) } + __u_boot_cmd_end = .; + + + . = .; + __start___ex_table = .; + __ex_table : { *(__ex_table) } + __stop___ex_table = .; + + . = ALIGN(256); + __init_begin = .; + .text.init : { *(.text.init) } + .data.init : { *(.data.init) } + . = ALIGN(256); + __init_end = .; + + __bss_start = .; + .bss (NOLOAD) : + { + *(.sbss) *(.scommon) + *(.dynbss) + *(.bss) + *(COMMON) + . = ALIGN(4); + } + + _end = . ; + PROVIDE (end = .); +} diff --git a/cpu/ppc4xx/start.S b/cpu/ppc4xx/start.S index f2b8908..ac96fc2 100644 --- a/cpu/ppc4xx/start.S +++ b/cpu/ppc4xx/start.S @@ -257,6 +257,14 @@ bl board_init_f #endif
+#if defined(CONFIG_SYS_RAMBOOT) + /* + * 4xx RAM-booting U-Boot image is started from offset 0 + */ + .text + bl _start_440 +#endif + /* * 440 Startup -- on reset only the top 4k of the effective * address space is mapped in by an entry in the instruction @@ -444,10 +452,17 @@ skip_debug_init: addis r0,0,0x0000 li r1,0x003f /* 64 TLB entries */ mtctr r1 -rsttlb: tlbwe r0,r1,0x0000 /* Invalidate all entries (V=0)*/ - tlbwe r0,r1,0x0001 - tlbwe r0,r1,0x0002 - subi r1,r1,0x0001 + li r4,0 /* Start with TLB #0 */ +rsttlb: +#ifdef CONFIG_SYS_RAMBOOT + tlbre r3,r4,0 /* Read contents from TLB word #0 to get EPN */ + rlwinm. r3,r3,0,0xfffffc00 /* Mask EPN */ + beq tlbnxt /* Skip EPN=0 TLB, this is the SDRAM TLB */ +#endif + tlbwe r0,r4,0 /* Invalidate all entries (V=0)*/ + tlbwe r0,r4,1 + tlbwe r0,r4,2 +tlbnxt: addi r4,r4,1 /* Next TLB */ bdnz rsttlb
/*----------------------------------------------------------------*/ @@ -476,7 +491,13 @@ rsttlb: tlbwe r0,r1,0x0000 /* Invalidate all entries (V=0)*/ li r4,0 /* TLB # */
addi r5,r5,-4 -1: lwzu r0,4(r5) +1: +#ifdef CONFIG_SYS_RAMBOOT + tlbre r3,r4,0 /* Read contents from TLB word #0 */ + rlwinm. r3,r3,0,0x00000200 /* Mask V (valid) bit */ + bne tlbnx2 /* Skip V=1 TLB, this is the SDRAM TLB */ +#endif + lwzu r0,4(r5) cmpwi r0,0 beq 2f /* 0 marks end */ lwzu r1,4(r5) @@ -484,7 +505,7 @@ rsttlb: tlbwe r0,r1,0x0000 /* Invalidate all entries (V=0)*/ tlbwe r0,r4,0 /* TLB Word 0 */ tlbwe r1,r4,1 /* TLB Word 1 */ tlbwe r2,r4,2 /* TLB Word 2 */ - addi r4,r4,1 /* Next TLB */ +tlbnx2: addi r4,r4,1 /* Next TLB */ bdnz 1b
/*----------------------------------------------------------------*/ diff --git a/include/configs/amcc-common.h b/include/configs/amcc-common.h index d3dc3e5..3b733c0 100644 --- a/include/configs/amcc-common.h +++ b/include/configs/amcc-common.h @@ -76,6 +76,17 @@ #define CONFIG_CMD_PING #define CONFIG_CMD_REGINFO
+#if defined(CONFIG_SYS_RAMBOOT) +/* + * Disable NOR FLASH commands on RAM-booting version. One main reason for this + * RAM-booting version is boards with NAND and without NOR. This image can + * be used for initial NAND programming. + */ +#define CONFIG_SYS_NO_FLASH +#undef CONFIG_CMD_FLASH +#undef CONFIG_CMD_IMLS +#endif + /* * Miscellaneous configurable options */ diff --git a/include/configs/sequoia.h b/include/configs/sequoia.h index fa226b2..89acacc 100644 --- a/include/configs/sequoia.h +++ b/include/configs/sequoia.h @@ -112,13 +112,26 @@ /* * Environment */ -#if !defined(CONFIG_NAND_U_BOOT) && !defined(CONFIG_NAND_SPL) -#define CONFIG_ENV_IS_IN_FLASH 1 /* use FLASH for environ vars */ +#if defined(CONFIG_NAND_U_BOOT) || defined(CONFIG_NAND_SPL) +#define CONFIG_ENV_IS_IN_NAND /* use NAND for environ vars */ +#define CONFIG_ENV_IS_EMBEDDED /* use embedded environment */ +#elif defined(CONFIG_SYS_RAMBOOT) +#define CONFIG_ENV_IS_NOWHERE /* Store env in memory only */ +#define CONFIG_ENV_SIZE (8 << 10) +/* + * In RAM-booting version, we have no environment storage. So we need to + * provide at least preliminary MAC addresses for the 4xx EMAC driver to + * register the interfaces. Those two addresses are generated via the + * tools/gen_eth_addr tool and should only be used in a closed laboratory + * environment. + */ +#define CONFIG_ETHADDR 4a:56:49:22:3e:43 +#define CONFIG_ETH1ADDR 02:93:53:d5:06:98 #else -#define CONFIG_ENV_IS_IN_NAND 1 /* use NAND for environ vars */ -#define CONFIG_ENV_IS_EMBEDDED 1 /* use embedded environment */ +#define CONFIG_ENV_IS_IN_FLASH /* use FLASH for environ vars */ #endif
+#if defined(CONFIG_CMD_FLASH) /* * FLASH related */ @@ -148,6 +161,7 @@ #define CONFIG_ENV_ADDR_REDUND (CONFIG_ENV_ADDR-CONFIG_ENV_SECT_SIZE) #define CONFIG_ENV_SIZE_REDUND (CONFIG_ENV_SIZE) #endif +#endif /* CONFIG_CMD_FLASH */
/* * IPL (Initial Program Loader, integrated inside CPU) @@ -211,7 +225,8 @@ * DDR SDRAM */ #define CONFIG_SYS_MBYTES_SDRAM (256) /* 256MB */ -#if !defined(CONFIG_NAND_U_BOOT) && !defined(CONFIG_NAND_SPL) +#if !defined(CONFIG_NAND_U_BOOT) && !defined(CONFIG_NAND_SPL) && \ + !defined(CONFIG_SYS_RAMBOOT) #define CONFIG_DDR_DATA_EYE /* use DDR2 optimization */ #endif #define CONFIG_SYS_MEM_TOP_HIDE (4 << 10) /* don't use last 4kbytes */ @@ -306,7 +321,7 @@ * overwrite part of the U-Boot image which is already loaded from NAND * to SDRAM. */ -#if defined(CONFIG_NAND_U_BOOT) +#if defined(CONFIG_NAND_U_BOOT) || defined(CONFIG_SYS_RAMBOOT) #define CONFIG_SYS_POST_MEMORY_ON 0 #else #define CONFIG_SYS_POST_MEMORY_ON CONFIG_SYS_POST_MEMORY @@ -354,7 +369,8 @@ /* * On Sequoia CS0 and CS3 are switched when configuring for NAND booting */ -#if !defined(CONFIG_NAND_U_BOOT) && !defined(CONFIG_NAND_SPL) +#if !defined(CONFIG_NAND_U_BOOT) && !defined(CONFIG_NAND_SPL) && \ + !defined(CONFIG_SYS_RAMBOOT) #define CONFIG_SYS_NAND_CS 3 /* NAND chip connected to CSx */ /* Memory Bank 0 (NOR-FLASH) initialization */ #define CONFIG_SYS_EBC_PB0AP 0x03017200

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 0x1000000) 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:
440EPX>load 0x1000000 /tftpboot/sequoia/u-boot.bin 440EPX>go 0x1000000
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 sr@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

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: office@denx.de =====================================================================

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

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: office@denx.de =====================================================================

Dear Stefan,
in message 200905071739.56301.sr@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

Wolfgang Denk wrote:
Dear Stefan,
in message 200905071739.56301.sr@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

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: office@denx.de =====================================================================

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

Dear Stefan,
In message 200905071530.43940.sr@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
participants (4)
-
Detlev Zundel
-
Scott Wood
-
Stefan Roese
-
Wolfgang Denk