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

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

[Stefan Roese]

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 at 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@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
-- 
1.6.2.4

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

[Detlev Zundel]

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


-- 
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: dzu at denx.de

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

[Stefan Roese]

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 at denx.de
=====================================================================

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

[Detlev Zundel]

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: dzu at denx.de

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

[Stefan Roese]

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 at denx.de
=====================================================================

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
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] [PATCH] ppc4xx: Add Sequoia RAM-booting target

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Time is a drug. Too much of it kills you.
                                      - Terry Pratchett, _Small Gods_

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

[Scott Wood]

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

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

[Stefan Roese]

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 at denx.de
=====================================================================

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

[Detlev Zundel]

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: dzu at denx.de