[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
different boards compile different versions of the source code, meaning
that we must build all boards to check for failures. It is easy to misspell
an #ifdef and there is not as much checking of this by the compiler. Multiple
dependent #ifdefs are harder to do than with if..then..else. Variable
declarations must be #idefed as well as the code that uses them, often much
later in the file/function. #ifdef indents don't match code indents and
have their own separate indent feature. Overall, excessive use of #idef
hurts readability and makes the code harder to modify and refactor. For
people coming newly into the code base, #ifdefs can be a big barrier.

The use of #ifdef in U-Boot has possibly got a little out of hand. In an
attempt to turn the tide, this series includes a patch which provides a way
to make CONFIG macros available to C code without using the preprocessor.
This makes it possible to use standard C conditional features such as
if/then instead of #ifdef. A README update exhorts compliance.

As an example of how to use this, this series replaces all but two #ifdefs
from the main code body of common/main.c, which is one of the largest users
of #ifdef, even after a recent cleanup:

for f in $(find . -name *.c); do echo $(grep -c "ifdef" $f) $f; do
ne |sort -nr |head
81 ./common/board_r.c
57 ./arch/powerpc/cpu/mpc83xx/cpu_init.c
55 ./common/board_f.c
49 ./common/main.c
48 ./arch/powerpc/lib/board.c
47 ./drivers/video/cfb_console.c
40 ./drivers/mtd/cfi_flash.c
38 ./net/tftp.c
37 ./drivers/usb/host/ohci-hcd.c
36 ./drivers/fpga/ivm_core.c

Code size for this series seems to be roughly neutral (below numbers are
average change in byte size for each region:

  blackfin: (for 24/35 boards)  all -11.0  text -11.0
       x86: (for 1/1 boards)  bss +20.0  data +4.0  text -24.0
     avr32: (for 10/10 boards)  all -8.4  text -8.4
   sandbox: (for 1/1 boards)  all +16.0  bss +16.0
      m68k: (for 41/50 boards)  all -31.9  text -31.9
   powerpc: (for 639/641 boards)  all -20.5  bss +0.0  rodata -0.5  text -20.0
     sparc: (for 5/5 boards)  all -28.8  text -28.8
        sh: (for 16/21 boards)  all -78.2  bss +3.2  rodata -15.5  text -66.0
     nios2: (for 3/3 boards)  all +24.0  bss +1.3  data -1.3  text +24.0
       arm: (for 307/327 boards)  all -41.0  bss +3.5  data +0.1  rodata -3.6
		spl/u-boot-spl:all -0.1  spl/u-boot-spl:bss -0.1  text -41.0

Note that a config_drop.h file is added - this defines all the CONFIGs
which are not used in any board config file. Without this, autoconf cannot
define the macros for this CONFIGs.

Compile time for main.c does not seem to be any different in my tests. The
time to perform the 'dep' step (which now creates autoconf.h) increases,
from about 2.8s to about 4.6s. This additional time is used to grep, sed
and sort the contents of all the header file in U-Boot. The time for an
incremental build is not affected.

It would be much more efficient to maintain a list of all available CONFIG
defines, but no such list exists at present.

Buildman output shows no additional failures from mainline:
01: Merge branch 'master' of git://www.denx.de/git/u-boot-mmc
  blackfin: +   bf561-acvilon cm-bf561 blackstamp br4 bct-brettl2 cm-bf527 dnp5370 bf506f-ezkit ip04 bf527-sdp bf609-ezkit bf537-stamp bf527-ezkit-v2 cm-bf537e tcm-bf518 cm-bf537u bf527-ezkit bf537-pnav cm-bf533 pr1 bf533-ezkit ibf-dsp561 bf537-srv1 cm-bf548 bf537-minotaur bf538f-ezkit bf548-ezkit bf525-ucr2 blackvme tcm-bf537 bf533-stamp bf518f-ezbrd bf527-ad7160-eval bf526-ezbrd bf561-ezkit
      m68k: +   M54455EVB_a66 M5329AFEE M5249EVB idmr M5208EVBE M5475FFE M54451EVB astro_mcf5373l M54418TWR_serial_rmii M54455EVB_intel M5282EVB M54455EVB_i66 M5475GFE M5253DEMO M54455EVB_stm33 M5485BFE M5485DFE M5329BFEE M52277EVB M5475EFE M5475CFE M5485AFE M53017EVB M5475AFE M5485HFE M5235EVB M5253EVBE M54418TWR_nand_mii M54418TWR_nand_rmii_lowfreq TASREG cobra5272 M5475BFE M5475DFE M5275EVB M52277EVB_stmicro eb_cpu5282 eb_cpu5282_internal M54451EVB_stmicro M5271EVB M5485GFE M5485EFE M5485FFE M54418TWR M5235EVB_Flash32 M5373EVB M54418TWR_nand_rmii M54418TWR_serial_mii M5485CFE M54455EVB M5272C3
   powerpc: +   MVBLM7 MVSMR lcd4_lwmon5
        sh: +   rsk7269 rsk7264 sh7757lcr sh7752evb rsk7203
microblaze: +   microblaze-generic
  openrisc: +   openrisc-generic
       arm: +   palmtc zipitz2 VCMA9 lubbock zynq_dcc vpac270_nor_128 colibri_pxa270 kzm9g zynq xaeniax polaris pxa255_idp lp8x4x vpac270_ond_256 vpac270_nor_256 smdk2410 h2200 balloon3 palmld trizepsiv
     nds32: +   adp-ag101p adp-ag102 adp-ag101
02: Implement autoconf header file
03: main: Use autoconf for boot retry feature
04: main: Remove CONFIG #ifdefs from the abortboot() code
05: main: Use autoconf to remove #ifdefs around process_boot_delay()
06: main: Use autoconf for boot_delay code
07: main: Use autoconf for parser selection
08: main: Use autoconf in command line reading
09: main: Use autoconf in main_loop()


Changes in v4:
- Rebase on current master
- Split out new patch to remove #ifdefs around process_boot_delay()
- Tidy up code style nits with new checkpatch

Changes in v3:
- Add comment as to why we use [A-Za-z0-9_][A-Za-z0-9_]*
- Fix missing && in if() statement
- Remove the extra config_of_libfdt() condition in main_loop()
- Remove unneeded retry_min variable
- Rename sed scripts to more useful names
- Simplify code for finding out bootdelay from config or environment
- Update config_xxx() to autoconf_xxx() in comments/README/sed
- Update config_xxx_enabled() to autoconf_has_xxx() in comments/README/sed

Changes in v2:
- Add a grep to the sed/sort pipe to speed up processing
- Fix up a few errors and comments in the original RFC
- Split out changes to main.c into separate patches
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()

Simon Glass (8):
  Implement autoconf header file
  main: Use autoconf for boot retry feature
  main: Remove CONFIG #ifdefs from the abortboot() code
  main: Use autoconf to remove #ifdefs around process_boot_delay()
  main: Use autoconf for boot_delay code
  main: Use autoconf for parser selection
  main: Use autoconf in command line reading
  main: Use autoconf in main_loop()

 Makefile                       |  43 ++-
 README                         |  87 +++++-
 common/main.c                  | 629 ++++++++++++++++++-----------------------
 include/command.h              |   2 -
 include/common.h               |   6 +-
 include/config_drop.h          |  17 ++
 include/fdt_support.h          |   4 +-
 include/hush.h                 |   2 -
 include/menu.h                 |   2 -
 tools/scripts/define2value.sed |  37 +++
 tools/scripts/define2zero.sed  |  32 +++
 11 files changed, 492 insertions(+), 369 deletions(-)
 create mode 100644 include/config_drop.h
 create mode 100644 tools/scripts/define2value.sed
 create mode 100644 tools/scripts/define2zero.sed

-- 
1.8.3

[U-Boot] [PATCH v4 1/8] Implement autoconf header file

[Simon Glass]

Add support for generating an autoconf.h header file that can be used in
the source instead of #ifdef.

For example, instead of:

 #ifdef CONFIG_VERSION_VARIABLE
	setenv("ver", version_string);  /* set version variable */
 #endif

you can do:

	if (autoconf_version_variable())
		setenv("ver", version_string);  /* set version variable */

The compiler will ensure that the dead code is eliminated, so the result
is the same.

Where the value of the CONFIG define is 0, you can use the autoconf_has...()
form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is
defined at all, it affects behaviour:

 #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
	s = getenv ("bootdelay");
 #endif

So we use:

	if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0)
		s = getenv ("bootdelay");

This later form should only be used for such 'difficult' defines where a
zero value still means that the CONFIG should be considered to be defined.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v4: None
Changes in v3:
- Add comment as to why we use [A-Za-z0-9_][A-Za-z0-9_]*
- Rename sed scripts to more useful names
- Update config_xxx() to autoconf_xxx() in comments/README/sed
- Update config_xxx_enabled() to autoconf_has_xxx() in comments/README/sed

Changes in v2:
- Add a grep to the sed/sort pipe to speed up processing
- Fix up a few errors and comments in the original RFC
- Split out changes to main.c into separate patches
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()

 Makefile                       | 43 ++++++++++++++++++++-
 README                         | 87 ++++++++++++++++++++++++++++++++++++++++--
 include/common.h               |  3 ++
 include/config_drop.h          | 17 +++++++++
 tools/scripts/define2value.sed | 37 ++++++++++++++++++
 tools/scripts/define2zero.sed  | 32 ++++++++++++++++
 6 files changed, 215 insertions(+), 4 deletions(-)
 create mode 100644 include/config_drop.h
 create mode 100644 tools/scripts/define2value.sed
 create mode 100644 tools/scripts/define2zero.sed

diff --git a/Makefile b/Makefile
index 693b3f2..3c2ffd4 100644
--- a/Makefile
+++ b/Makefile
@@ -629,6 +629,7 @@ updater:
 # parallel sub-makes creating .depend files simultaneously.
 depend dep:	$(TIMESTAMP_FILE) $(VERSION_FILE) \
 		$(obj)include/autoconf.mk \
+		$(obj)include/generated/autoconf.h \
 		$(obj)include/generated/generic-asm-offsets.h \
 		$(obj)include/generated/asm-offsets.h
 		for dir in $(SUBDIRS) $(CPUDIR) $(LDSCRIPT_MAKEFILE_DIR) ; do \
@@ -703,6 +704,45 @@ $(obj)include/autoconf.mk: $(obj)include/config.h
 		sed -n -f tools/scripts/define2mk.sed > $@.tmp && \
 	mv $@.tmp $@
 
+# Create a C header file where every '#define CONFIG_XXX value' becomes
+# '#define autoconf_xxx() value', or '#define autoconf_xxx() 0' where the
+# CONFIG is not used by this board configuration. This allows C code to do
+# things like 'if (autoconf_xxx())' and have the compiler remove the dead code,
+# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases
+# if the autoconf_...() returns 0 then the option is not enabled. In some rare
+# cases such as CONFIG_BOOTDELAY, the config can be enabled but still have a
+# a value of 0. So in addition we a #define autoconf_has_xxx(), setting the
+# value to 0 if the option is disabled, 1 if enabled. This last feature will
+# hopefully be deprecated soon.
+# The file is regenerated when any U-Boot header file changes.
+$(obj)include/generated/autoconf.h: $(obj)include/config.h
+	@$(XECHO) Generating $@ ; \
+	set -e ; \
+	: Extract the config macros to a C header file ; \
+	$(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \
+		sed -n -f tools/scripts/define2value.sed > $@.tmp; \
+	: Regenerate our list of all config macros if neeed ; \
+	if [ ! -f $@-all.tmp ] || \
+		find $(src) -name '*.h' -type f -newer $@-all.tmp | \
+			egrep -qv 'include/(autoconf.h|generated|config.h)'; \
+			then \
+		: Extract all config macros from all C header files ; \
+		: We can grep for CONFIG since the value will be dropped ; \
+		( \
+			find ${src} -name "*.h" -type f | xargs \
+			cat | grep CONFIG | \
+			sed -n -f tools/scripts/define2zero.sed \
+		) | sort | uniq > $@-all.tmp; \
+	fi; \
+	: Extract the enabled config macros to a C header file ; \
+	$(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \
+		sed -n -f tools/scripts/define2zero.sed | \
+			sort > $@-enabled.tmp; \
+	set -e ; \
+	: Find CONFIGs that are not enabled ; \
+	comm -13 $@-enabled.tmp $@-all.tmp >>$@.tmp && \
+	mv $@.tmp $@
+
 $(obj)include/generated/generic-asm-offsets.h:	$(obj)include/autoconf.mk.dep \
 	$(obj)lib/asm-offsets.s
 	@$(XECHO) Generating $@
@@ -785,7 +825,8 @@ include/license.h: tools/bin2header COPYING
 unconfig:
 	@rm -f $(obj)include/config.h $(obj)include/config.mk \
 		$(obj)board/*/config.tmp $(obj)board/*/*/config.tmp \
-		$(obj)include/autoconf.mk $(obj)include/autoconf.mk.dep
+		$(obj)include/autoconf.mk $(obj)include/autoconf.mk.dep \
+		$(obj)include/generated/autoconf.h
 
 %_config::	unconfig
 	@$(MKCONFIG) -A $(@:_config=)
diff --git a/README b/README
index cd0336c..5ed67e5 100644
--- a/README
+++ b/README
@@ -5711,11 +5711,92 @@ Notes:
 * If you modify existing code, make sure that your new code does not
   add to the memory footprint of the code ;-) Small is beautiful!
   When adding new features, these should compile conditionally only
-  (using #ifdef), and the resulting code with the new feature
-  disabled must not need more memory than the old code without your
-  modification.
+  (avoiding #ifdef where at all possible), and the resulting code with
+  the new feature disabled must not need more memory than the old code
+  without your modification.
 
 * Remember that there is a size limit of 100 kB per message on the
   u-boot mailing list. Bigger patches will be moderated. If they are
   reasonable and not too big, they will be acknowledged. But patches
   bigger than the size limit should be avoided.
+
+
+Use of #ifdef:
+--------------
+Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
+different boards compile different versions of the source code, meaning
+that we must build all boards to check for failures. It is easy to misspell
+an #ifdef and there is not as much checking of this by the compiler. For
+someone coming new into the code base, #ifdefs are a big turn-off. Multiple
+dependent #ifdefs are harder to do than with if..then..else. Variable
+declarations must be #idefed as well as the code that uses them, often much
+later in the file/function. #ifdef indents don't match code indents and
+have their own separate indent feature. Overall, excessive use of #idef
+hurts readability and makes the code harder to modify and refactor.
+
+In an effort to reduce the use of #ifdef in U-Boot, without requiring lots
+of special static inlines all over the header files, a single autoconf.h
+header file with lower-case function-type macros has been made available.
+
+This file has either:
+
+#	#define autoconf_xxx() value
+
+for enabled options, or:
+
+#	#define autoconf_xxx() 0
+
+for disabled options. You can therefore generally change code like this:
+
+	#ifdef CONFIG_XXX
+		do_something
+	#else
+		do_something_else
+	#endif
+
+to this:
+
+	if (autoconf_xxx())
+		do_something;
+	else
+		do_something_else;
+
+The compiler will see that autoconf_xxx() evalutes to a constant and will
+eliminate the dead code. The resulting code (and code size) is the same.
+
+Multiple #ifdefs can be converted also:
+
+	#if defined(CONFIG_XXX) && !defined(CONFIG_YYY)
+		do_something
+	#endif
+
+	if (autoconf_xxx() && !autoconf_yyy())
+		do_something;
+
+Where the macro evaluates to a string, it will be non-NULL, so the above
+will work whether the macro is a string or a number.
+
+This takes care of almost all CONFIG macros. Unfortunately there are a few
+cases where a value of 0 does not mean the option is disabled. For example
+CONFIG_BOOTDELAY can be defined to 0, which means that the bootdelay
+code should be used, but with a value of 0. To get around this and other
+sticky cases, an addition macro with an 'has_' prefix is provided, where
+the value is always either 0 or 1:
+
+	// Will work even if boaard config has '#define CONFIG_BOOTDELAY 0'
+	if (autoconf_has_bootdelay())
+		do_something;
+
+(Probably such config options should be deprecated and then we can remove
+this feature)
+
+U-Boot already has a Makefile scheme to permit files to be easily included
+based on CONFIG. This can be used where the code to be compiled exists in
+its own source file. So the following rules apply:
+
+  1. Use #ifdef to conditionally compile an exported function or variable
+  2. Use ordinary C code with autoconf_xxx() everywhere else
+  3. Mark your functions and data structures static where possible
+  4. Use the autoconf_has_xxx() variants only if essential
+  5. When changing existing code, first create a new patch to replace
+        #ifdefs in the surrounding area
diff --git a/include/common.h b/include/common.h
index 126891d..de18083 100644
--- a/include/common.h
+++ b/include/common.h
@@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short;
 typedef volatile unsigned char	vu_char;
 
 #include <config.h>
+#ifndef DO_DEPS_ONLY
+#include <generated/autoconf.h>
+#endif
 #include <asm-offsets.h>
 #include <linux/bitops.h>
 #include <linux/types.h>
diff --git a/include/config_drop.h b/include/config_drop.h
new file mode 100644
index 0000000..bf68b50
--- /dev/null
+++ b/include/config_drop.h
@@ -0,0 +1,17 @@
+/*
+ * Copyright 2013 Google, Inc
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License Version 2. This file is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _CONFIG_DROP_H
+#define _CONFIG_DROP_H
+
+/* Options which don't seem to be defined by any header in U-Boot */
+#define CONFIG_MENUPROMPT	"Auto-boot prompt"
+#define CONFIG_MENUKEY
+#define CONFIG_UPDATE_TFTP
+
+#endif
diff --git a/tools/scripts/define2value.sed b/tools/scripts/define2value.sed
new file mode 100644
index 0000000..205f9fe
--- /dev/null
+++ b/tools/scripts/define2value.sed
@@ -0,0 +1,37 @@
+#
+# Sed script to parse CPP macros and generate a list of CONFIG macros
+#
+# This converts:
+#	#define CONFIG_XXX value
+#into:
+#	#define autoconf_xxx() value
+#	#define autoconf_has_xxx() 1
+
+# Macros with parameters are ignored.
+# (Note we avoid + since it doesn't appear to work)
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
+	d
+}
+
+# Only process values prefixed with #define CONFIG_
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
+	# Strip the #define prefix
+	s/#define[ \t]*CONFIG_/autoconf_/;
+	# Change to form CONFIG_*=VALUE
+	s/[\t ][\t ]*/=/;
+	# Handle lines with no value
+	s/^\([^=]*\)$/\1=/;
+	# Drop trailing spaces
+	s/ *$//;
+	# Change empty values to '1'
+	s/=$/=1/;
+	# Add #define at the start
+	s/^\([^=]*\)=/#define \L\1() /
+	# print the line
+	p
+	# Create autoconf_has_...(), value 1
+	s/().*/() 1/
+	s/\(autoconf_\)/\1has_/
+	# print the line
+	p
+}
diff --git a/tools/scripts/define2zero.sed b/tools/scripts/define2zero.sed
new file mode 100644
index 0000000..95e6860
--- /dev/null
+++ b/tools/scripts/define2zero.sed
@@ -0,0 +1,32 @@
+#
+# Sed script to parse CPP macros and generate a list of autoconf macros
+#
+# This converts:
+#	#define CONFIG_XXX value
+#into:
+#	#define autoconf_xxx() 0
+#	#define autoconf_has_xxx() 0
+
+# Macros with parameters are ignored.
+# (Note we avoid + since it doesn't appear to work)
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
+	s/.*//
+}
+
+# Only process values prefixed with #define CONFIG_
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
+	# Strip the #define prefix
+	s/#define *//;
+	# Remove the value
+	s/[ \t].*//;
+	# Convert to lower case, prepend #define
+	s/CONFIG_\(.*\)/#define autoconf_\L\1/
+	# Append 0
+	s/$/() 0/
+	# print the line
+	p
+	# Create autoconf_has_...(), value 0
+	s/\(autoconf_\)/\1has_/
+	# print the line
+	p
+}
-- 
1.8.3

[U-Boot] [PATCH v4 2/8] main: Use autoconf for boot retry feature

[Simon Glass]

Change this feature to use autoconf instead of #ifdef.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v4:
- Rebase on current master
- Tidy up code style nits with new checkpatch

Changes in v3:
- Fix missing && in if() statement
- Remove unneeded retry_min variable

Changes in v2: None

 common/main.c | 73 ++++++++++++++++++++++++++---------------------------------
 1 file changed, 32 insertions(+), 41 deletions(-)

diff --git a/common/main.c b/common/main.c
index 56da214..3a143ae 100644
--- a/common/main.c
+++ b/common/main.c
@@ -65,17 +65,11 @@ static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen);
 static const char erase_seq[] = "\b \b";		/* erase sequence	*/
 static const char   tab_seq[] = "        ";		/* used to expand TABs	*/
 
-#ifdef CONFIG_BOOT_RETRY_TIME
 static uint64_t endtime = 0;  /* must be set, default is instant timeout */
 static int      retry_time = -1; /* -1 so can call readline before main_loop */
-#endif
 
 #define	endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk())
 
-#ifndef CONFIG_BOOT_RETRY_MIN
-#define CONFIG_BOOT_RETRY_MIN CONFIG_BOOT_RETRY_TIME
-#endif
-
 #ifdef CONFIG_MODEM_SUPPORT
 int do_mdm_init = 0;
 extern void mdm_init(void); /* defined in board.c */
@@ -174,11 +168,10 @@ static int abortboot_keyed(int bootdelay)
 					       delaykey[i].retry ? "delay" :
 					       "stop");
 
-#  ifdef CONFIG_BOOT_RETRY_TIME
-				/* don't retry auto boot */
-				if (! delaykey[i].retry)
+				/* don't retry auto boot? */
+				if (autoconf_boot_retry_time() &&
+				    !delaykey[i].retry)
 					retry_time = -1;
-#  endif
 				abort = 1;
 			}
 		}
@@ -368,9 +361,8 @@ static void process_boot_delay(void)
 #if defined(CONFIG_MENU_SHOW)
 	bootdelay = menu_show(bootdelay);
 #endif
-# ifdef CONFIG_BOOT_RETRY_TIME
-	init_cmd_timeout ();
-# endif	/* CONFIG_BOOT_RETRY_TIME */
+	if (autoconf_boot_retry_time())
+		init_cmd_timeout();
 
 #ifdef CONFIG_POST
 	if (gd->flags & GD_FLG_POSTFAIL) {
@@ -499,14 +491,12 @@ void main_loop(void)
 	for (;;);
 #else
 	for (;;) {
-#ifdef CONFIG_BOOT_RETRY_TIME
-		if (rc >= 0) {
+		if (autoconf_boot_retry_time() && rc >= 0) {
 			/* Saw enough of a valid command to
 			 * restart the timeout.
 			 */
 			reset_cmd_timeout();
 		}
-#endif
 		len = readline (CONFIG_SYS_PROMPT);
 
 		flag = 0;	/* assume no special flags for now */
@@ -514,19 +504,16 @@ void main_loop(void)
 			strcpy (lastcommand, console_buffer);
 		else if (len == 0)
 			flag |= CMD_FLAG_REPEAT;
-#ifdef CONFIG_BOOT_RETRY_TIME
-		else if (len == -2) {
+		else if (autoconf_boot_retry_time() && len == -2) {
 			/* -2 means timed out, retry autoboot
 			 */
-			puts ("\nTimed out waiting for command\n");
-# ifdef CONFIG_RESET_TO_RETRY
+			puts("\nTimed out waiting for command\n");
 			/* Reinit board to run initialization code again */
-			do_reset (NULL, 0, 0, NULL);
-# else
-			return;		/* retry autoboot */
-# endif
+			if (autoconf_reset_to_retry())
+				do_reset(NULL, 0, 0, NULL);
+			else
+				return;		/* retry autoboot */
 		}
-#endif
 
 		if (len == -1)
 			puts ("<INTERRUPT>\n");
@@ -541,6 +528,10 @@ void main_loop(void)
 #endif /*CONFIG_SYS_HUSH_PARSER*/
 }
 
+/*
+ * Use ifdef here for the benefit of those archs not using
+ * -ffunction-sections, since these functions are exported.
+ */
 #ifdef CONFIG_BOOT_RETRY_TIME
 /***************************************************************************
  * initialize command line timeout
@@ -552,10 +543,10 @@ void init_cmd_timeout(void)
 	if (s != NULL)
 		retry_time = (int)simple_strtol(s, NULL, 10);
 	else
-		retry_time =  CONFIG_BOOT_RETRY_TIME;
+		retry_time = autoconf_boot_retry_time();
 
-	if (retry_time >= 0 && retry_time < CONFIG_BOOT_RETRY_MIN)
-		retry_time = CONFIG_BOOT_RETRY_MIN;
+	if (retry_time >= 0 && retry_time < autoconf_boot_retry_min())
+		retry_time = autoconf_boot_retry_min();
 }
 
 /***************************************************************************
@@ -777,13 +768,13 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len,
 		cread_add_str(buf, init_len, 1, &num, &eol_num, buf, *len);
 
 	while (1) {
-#ifdef CONFIG_BOOT_RETRY_TIME
-		while (!tstc()) {	/* while no incoming data */
-			if (retry_time >= 0 && get_ticks() > endtime)
-				return (-2);	/* timed out */
-			WATCHDOG_RESET();
+		if (autoconf_boot_retry_time()) {
+			while (!tstc()) {	/* while no incoming data */
+				if (retry_time >= 0 && get_ticks() > endtime)
+					return -2;	/* timed out */
+				WATCHDOG_RESET();
+			}
 		}
-#endif
 		if (first && timeout) {
 			uint64_t etime = endtick(timeout);
 
@@ -1055,14 +1046,14 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
 	col = plen;
 
 	for (;;) {
-#ifdef CONFIG_BOOT_RETRY_TIME
-		while (!tstc()) {	/* while no incoming data */
-			if (retry_time >= 0 && get_ticks() > endtime)
-				return (-2);	/* timed out */
-			WATCHDOG_RESET();
+		if (autoconf_boot_retry_time()) {
+			while (!tstc()) {	/* while no incoming data */
+				if (retry_time >= 0 && get_ticks() > endtime)
+					return -2;	/* timed out */
+				WATCHDOG_RESET();
+			}
 		}
-#endif
-		WATCHDOG_RESET();		/* Trigger watchdog, if needed */
+		WATCHDOG_RESET();	/* Trigger watchdog, if needed */
 
 #ifdef CONFIG_SHOW_ACTIVITY
 		while (!tstc()) {
-- 
1.8.3

[U-Boot] [PATCH v4 3/8] main: Remove CONFIG #ifdefs from the abortboot() code

[Simon Glass]

Move this code over to using autoconf. We can add the autoconf values to
the delaykey[] array, and move the code that checks for autoconf values into
the loop.

Also change to using ARRAY_SIZE on delaykey[].

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v4:
- Rebase on current master

Changes in v3: None
Changes in v2: None

 common/main.c | 90 ++++++++++++++++++++++-------------------------------------
 1 file changed, 34 insertions(+), 56 deletions(-)

diff --git a/common/main.c b/common/main.c
index 3a143ae..3a4754d 100644
--- a/common/main.c
+++ b/common/main.c
@@ -86,15 +86,20 @@ static int abortboot_keyed(int bootdelay)
 	int abort = 0;
 	uint64_t etime = endtick(bootdelay);
 	struct {
-		char* str;
+		const char *str;
 		u_int len;
 		int retry;
+		const char *conf;	/* Configuration value */
 	}
 	delaykey [] = {
-		{ str: getenv ("bootdelaykey"),  retry: 1 },
-		{ str: getenv ("bootdelaykey2"), retry: 1 },
-		{ str: getenv ("bootstopkey"),   retry: 0 },
-		{ str: getenv ("bootstopkey2"),  retry: 0 },
+		{ str: getenv("bootdelaykey"),  retry: 1,
+			conf: autoconf_autoboot_delay_str() },
+		{ str: getenv("bootdelaykey2"), retry: 1,
+			conf: autoconf_autoboot_delay_str2() },
+		{ str: getenv("bootstopkey"),   retry: 0,
+			conf: autoconf_autoboot_stop_str() },
+		{ str: getenv("bootstopkey2"),  retry: 0,
+			conf: autoconf_autoboot_stop_str2() },
 	};
 
 	char presskey [MAX_DELAY_STOP_STR];
@@ -102,33 +107,15 @@ static int abortboot_keyed(int bootdelay)
 	u_int presskey_max = 0;
 	u_int i;
 
-#ifndef CONFIG_ZERO_BOOTDELAY_CHECK
-	if (bootdelay == 0)
+	if (!autoconf_zero_bootdelay_check() && bootdelay == 0)
 		return 0;
-#endif
 
-#  ifdef CONFIG_AUTOBOOT_PROMPT
-	printf(CONFIG_AUTOBOOT_PROMPT);
-#  endif
-
-#  ifdef CONFIG_AUTOBOOT_DELAY_STR
-	if (delaykey[0].str == NULL)
-		delaykey[0].str = CONFIG_AUTOBOOT_DELAY_STR;
-#  endif
-#  ifdef CONFIG_AUTOBOOT_DELAY_STR2
-	if (delaykey[1].str == NULL)
-		delaykey[1].str = CONFIG_AUTOBOOT_DELAY_STR2;
-#  endif
-#  ifdef CONFIG_AUTOBOOT_STOP_STR
-	if (delaykey[2].str == NULL)
-		delaykey[2].str = CONFIG_AUTOBOOT_STOP_STR;
-#  endif
-#  ifdef CONFIG_AUTOBOOT_STOP_STR2
-	if (delaykey[3].str == NULL)
-		delaykey[3].str = CONFIG_AUTOBOOT_STOP_STR2;
-#  endif
-
-	for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) {
+	if (autoconf_has_autoboot_prompt())
+		printf(autoconf_autoboot_prompt());
+
+	for (i = 0; i < ARRAY_SIZE(delaykey); i++) {
+		if (delaykey[i].conf && !delaykey[i].str)
+			delaykey[i].str = delaykey[i].conf;
 		delaykey[i].len = delaykey[i].str == NULL ?
 				    0 : strlen (delaykey[i].str);
 		delaykey[i].len = delaykey[i].len > MAX_DELAY_STOP_STR ?
@@ -158,7 +145,7 @@ static int abortboot_keyed(int bootdelay)
 			}
 		}
 
-		for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) {
+		for (i = 0; i < ARRAY_SIZE(delaykey); i++) {
 			if (delaykey[i].len > 0 &&
 			    presskey_len >= delaykey[i].len &&
 			    memcmp (presskey + presskey_len - delaykey[i].len,
@@ -180,45 +167,39 @@ static int abortboot_keyed(int bootdelay)
 	if (!abort)
 		debug_bootkeys("key timeout\n");
 
-#ifdef CONFIG_SILENT_CONSOLE
-	if (abort)
+	if (autoconf_silent_console() && abort)
 		gd->flags &= ~GD_FLG_SILENT;
-#endif
 
 	return abort;
 }
 
 # else	/* !defined(CONFIG_AUTOBOOT_KEYED) */
 
-#ifdef CONFIG_MENUKEY
-static int menukey = 0;
-#endif
+static int menukey;
 
 static int abortboot_normal(int bootdelay)
 {
 	int abort = 0;
 	unsigned long ts;
 
-#ifdef CONFIG_MENUPROMPT
-	printf(CONFIG_MENUPROMPT);
-#else
-	if (bootdelay >= 0)
+	if (autoconf_menuprompt())
+		printf(autoconf_menuprompt());
+	else if (bootdelay >= 0)
 		printf("Hit any key to stop autoboot: %2d ", bootdelay);
-#endif
 
-#if defined CONFIG_ZERO_BOOTDELAY_CHECK
 	/*
-	 * Check if key already pressed
-	 * Don't check if bootdelay < 0
+	 * If we need to do a bootdelay check even if bootdelay is 0, do
+	 * it here, since the loop below will be skipped in this case.
+	 * We don't do this check if bootdelay < 0.
 	 */
-	if (bootdelay >= 0) {
-		if (tstc()) {	/* we got a key press	*/
+	if (autoconf_zero_bootdelay_check() && bootdelay >= 0) {
+		/* Check if key already pressed */
+		if (tstc()) {	/* we got a key press */
 			(void) getc();  /* consume input	*/
 			puts ("\b\b\b 0");
 			abort = 1;	/* don't auto boot	*/
 		}
 	}
-#endif
 
 	while ((bootdelay > 0) && (!abort)) {
 		--bootdelay;
@@ -228,11 +209,10 @@ static int abortboot_normal(int bootdelay)
 			if (tstc()) {	/* we got a key press	*/
 				abort  = 1;	/* don't auto boot	*/
 				bootdelay = 0;	/* no more delay	*/
-# ifdef CONFIG_MENUKEY
-				menukey = getc();
-# else
-				(void) getc();  /* consume input	*/
-# endif
+				if (autoconf_menukey())
+					menukey = getc();
+				else
+					(void) getc();  /* consume input */
 				break;
 			}
 			udelay(10000);
@@ -243,10 +223,8 @@ static int abortboot_normal(int bootdelay)
 
 	putc('\n');
 
-#ifdef CONFIG_SILENT_CONSOLE
-	if (abort)
+	if (autoconf_silent_console() && abort)
 		gd->flags &= ~GD_FLG_SILENT;
-#endif
 
 	return abort;
 }
-- 
1.8.3

[U-Boot] [PATCH v4 4/8] main: Use autoconf to remove #ifdefs around process_boot_delay()

[Simon Glass]

Use autoconf to make process_boot_delay() be compiled always, and adjust
the caller and related functions as needed.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v4:
- Split out new patch to remove #ifdefs around process_boot_delay()

Changes in v3: None
Changes in v2: None

 common/main.c | 71 ++++++++++++++++++++++++++---------------------------------
 1 file changed, 31 insertions(+), 40 deletions(-)

diff --git a/common/main.c b/common/main.c
index 3a4754d..dba6cee 100644
--- a/common/main.c
+++ b/common/main.c
@@ -79,8 +79,6 @@ extern void mdm_init(void); /* defined in board.c */
  * Watch for 'delay' seconds for autoboot stop or autoboot delay string.
  * returns: 0 -  no key string, allow autoboot 1 - got key string, abort
  */
-#if defined(CONFIG_BOOTDELAY)
-# if defined(CONFIG_AUTOBOOT_KEYED)
 static int abortboot_keyed(int bootdelay)
 {
 	int abort = 0;
@@ -173,8 +171,6 @@ static int abortboot_keyed(int bootdelay)
 	return abort;
 }
 
-# else	/* !defined(CONFIG_AUTOBOOT_KEYED) */
-
 static int menukey;
 
 static int abortboot_normal(int bootdelay)
@@ -228,17 +224,14 @@ static int abortboot_normal(int bootdelay)
 
 	return abort;
 }
-# endif	/* CONFIG_AUTOBOOT_KEYED */
 
 static int abortboot(int bootdelay)
 {
-#ifdef CONFIG_AUTOBOOT_KEYED
-	return abortboot_keyed(bootdelay);
-#else
-	return abortboot_normal(bootdelay);
-#endif
+	if (autoconf_autoboot_keyed())
+		return abortboot_keyed(bootdelay);
+	else
+		return abortboot_normal(bootdelay);
 }
-#endif	/* CONFIG_BOOTDELAY */
 
 /*
  * Runs the given boot command securely.  Specifically:
@@ -254,7 +247,6 @@ static int abortboot(int bootdelay)
  * printing the error message to console.
  */
 
-#if defined(CONFIG_BOOTDELAY) && defined(CONFIG_OF_CONTROL)
 static void secure_boot_cmd(char *cmd)
 {
 	cmd_tbl_t *cmdtp;
@@ -295,22 +287,21 @@ static void process_fdt_options(const void *blob)
 
 	/* Add an env variable to point to a kernel payload, if available */
 	addr = fdtdec_get_config_int(gd->fdt_blob, "kernel-offset", 0);
-	if (addr)
-		setenv_addr("kernaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr));
+	if (addr) {
+		setenv_addr("kernaddr",
+			    (void *)(autoconf_sys_text_base() + addr));
+	}
 
 	/* Add an env variable to point to a root disk, if available */
 	addr = fdtdec_get_config_int(gd->fdt_blob, "rootdisk-offset", 0);
-	if (addr)
-		setenv_addr("rootaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr));
+	if (addr) {
+		setenv_addr("rootaddr",
+			    (void *)(autoconf_sys_text_base() + addr));
+	}
 }
-#endif /* CONFIG_OF_CONTROL */
 
-#ifdef CONFIG_BOOTDELAY
 static void process_boot_delay(void)
 {
-#ifdef CONFIG_OF_CONTROL
-	char *env;
-#endif
 	char *s;
 	int bootdelay;
 #ifdef CONFIG_BOOTCOUNT_LIMIT
@@ -327,7 +318,7 @@ static void process_boot_delay(void)
 #endif /* CONFIG_BOOTCOUNT_LIMIT */
 
 	s = getenv ("bootdelay");
-	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY;
+	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : autoconf_bootdelay();
 
 #ifdef CONFIG_OF_CONTROL
 	bootdelay = fdtdec_get_config_int(gd->fdt_blob, "bootdelay",
@@ -357,23 +348,24 @@ static void process_boot_delay(void)
 	else
 #endif /* CONFIG_BOOTCOUNT_LIMIT */
 		s = getenv ("bootcmd");
-#ifdef CONFIG_OF_CONTROL
-	/* Allow the fdt to override the boot command */
-	env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");
-	if (env)
-		s = env;
+	if (autoconf_of_control()) {
+		char *env;
 
-	process_fdt_options(gd->fdt_blob);
+		/* Allow the fdt to override the boot command */
+		env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");
+		if (env)
+			s = env;
 
-	/*
-	 * If the bootsecure option was chosen, use secure_boot_cmd().
-	 * Always use 'env' in this case, since bootsecure requres that the
-	 * bootcmd was specified in the FDT too.
-	 */
-	if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0))
-		secure_boot_cmd(env);
+		process_fdt_options(gd->fdt_blob);
 
-#endif /* CONFIG_OF_CONTROL */
+		/*
+		* If the bootsecure option was chosen, use secure_boot_cmd().
+		* Always use 'env' in this case, since bootsecure requres that
+		* the bootcmd was specified in the FDT too.
+		*/
+		if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0))
+			secure_boot_cmd(env);
+	}
 
 	debug ("### main_loop: bootcmd=\"%s\"\n", s ? s : "<UNDEFINED>");
 
@@ -397,7 +389,6 @@ static void process_boot_delay(void)
 	}
 #endif /* CONFIG_MENUKEY */
 }
-#endif /* CONFIG_BOOTDELAY */
 
 void main_loop(void)
 {
@@ -457,9 +448,9 @@ void main_loop(void)
 	update_tftp(0UL);
 #endif /* CONFIG_UPDATE_TFTP */
 
-#ifdef CONFIG_BOOTDELAY
-	process_boot_delay();
-#endif
+	if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0)
+		process_boot_delay();
+
 	/*
 	 * Main Loop for Monitor Command Processing
 	 */
-- 
1.8.3

[U-Boot] [PATCH v4 5/8] main: Use autoconf for boot_delay code

[Simon Glass]

Convert this function and its children to use autoconf instead of #ifdef.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v4:
- Rebase on current master

Changes in v3:
- Simplify code for finding out bootdelay from config or environment

Changes in v2: None

 common/main.c  | 74 +++++++++++++++++++++++++---------------------------------
 include/menu.h |  2 --
 2 files changed, 32 insertions(+), 44 deletions(-)

diff --git a/common/main.c b/common/main.c
index dba6cee..fc55e06 100644
--- a/common/main.c
+++ b/common/main.c
@@ -302,52 +302,42 @@ static void process_fdt_options(const void *blob)
 
 static void process_boot_delay(void)
 {
-	char *s;
-	int bootdelay;
-#ifdef CONFIG_BOOTCOUNT_LIMIT
 	unsigned long bootcount = 0;
 	unsigned long bootlimit = 0;
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
-
-#ifdef CONFIG_BOOTCOUNT_LIMIT
-	bootcount = bootcount_load();
-	bootcount++;
-	bootcount_store (bootcount);
-	setenv_ulong("bootcount", bootcount);
-	bootlimit = getenv_ulong("bootlimit", 10, 0);
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
-
-	s = getenv ("bootdelay");
-	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : autoconf_bootdelay();
-
-#ifdef CONFIG_OF_CONTROL
-	bootdelay = fdtdec_get_config_int(gd->fdt_blob, "bootdelay",
-			bootdelay);
-#endif
+	const char *s;
+	int bootdelay;
+
+	if (autoconf_bootcount_limit()) {
+		bootcount = bootcount_load();
+		bootcount++;
+		bootcount_store(bootcount);
+		setenv_ulong("bootcount", bootcount);
+		bootlimit = getenv_ulong("bootlimit", 10, 0);
+	}
 
+	bootdelay = getenv_ulong("bootdelay", 10, autoconf_bootdelay());
+
+	if (autoconf_of_control()) {
+		bootdelay = fdtdec_get_config_int(gd->fdt_blob, "bootdelay",
+						  bootdelay);
+	}
 	debug ("### main_loop entered: bootdelay=%d\n\n", bootdelay);
 
-#if defined(CONFIG_MENU_SHOW)
-	bootdelay = menu_show(bootdelay);
-#endif
+	if (autoconf_menu_show())
+		bootdelay = menu_show(bootdelay);
 	if (autoconf_boot_retry_time())
 		init_cmd_timeout();
 
-#ifdef CONFIG_POST
-	if (gd->flags & GD_FLG_POSTFAIL) {
+	if (autoconf_post() && (gd->flags & GD_FLG_POSTFAIL)) {
 		s = getenv("failbootcmd");
-	}
-	else
-#endif /* CONFIG_POST */
-#ifdef CONFIG_BOOTCOUNT_LIMIT
-	if (bootlimit && (bootcount > bootlimit)) {
+	} else if (autoconf_bootcount_limit() && bootlimit &&
+			(bootcount > bootlimit)) {
 		printf ("Warning: Bootlimit (%u) exceeded. Using altbootcmd.\n",
 		        (unsigned)bootlimit);
 		s = getenv ("altbootcmd");
-	}
-	else
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
+	} else {
 		s = getenv ("bootcmd");
+	}
 	if (autoconf_of_control()) {
 		char *env;
 
@@ -370,24 +360,24 @@ static void process_boot_delay(void)
 	debug ("### main_loop: bootcmd=\"%s\"\n", s ? s : "<UNDEFINED>");
 
 	if (bootdelay != -1 && s && !abortboot(bootdelay)) {
-#ifdef CONFIG_AUTOBOOT_KEYED
-		int prev = disable_ctrlc(1);	/* disable Control C checking */
-#endif
+		int prev;
+
+		/* disable Control C checking */
+		if (autoconf_autoboot_keyed())
+			prev = disable_ctrlc(1);
 
 		run_command_list(s, -1, 0);
 
-#ifdef CONFIG_AUTOBOOT_KEYED
-		disable_ctrlc(prev);	/* restore Control C checking */
-#endif
+		/* restore Control C checking */
+		if (autoconf_autoboot_keyed())
+			disable_ctrlc(prev);
 	}
 
-#ifdef CONFIG_MENUKEY
-	if (menukey == CONFIG_MENUKEY) {
+	if (autoconf_menukey() && menukey == autoconf_menukey()) {
 		s = getenv("menucmd");
 		if (s)
 			run_command_list(s, -1, 0);
 	}
-#endif /* CONFIG_MENUKEY */
 }
 
 void main_loop(void)
diff --git a/include/menu.h b/include/menu.h
index d8200ee..bcc3ec4 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -31,7 +31,5 @@ int menu_destroy(struct menu *m);
 void menu_display_statusline(struct menu *m);
 int menu_default_choice(struct menu *m, void **choice);
 
-#if defined(CONFIG_MENU_SHOW)
 int menu_show(int bootdelay);
-#endif
 #endif /* __MENU_H__ */
-- 
1.8.3

[U-Boot] [PATCH v4 6/8] main: Use autoconf for parser selection

[Simon Glass]

Allow parser selection to make use of autoconf instead of #ifdefs.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v4:
- Rebase on current master

Changes in v3: None
Changes in v2: None

 common/main.c  | 87 +++++++++++++++++++++++++++-------------------------------
 include/hush.h |  2 --
 2 files changed, 41 insertions(+), 48 deletions(-)

diff --git a/common/main.c b/common/main.c
index fc55e06..b09bfb4 100644
--- a/common/main.c
+++ b/common/main.c
@@ -382,12 +382,10 @@ static void process_boot_delay(void)
 
 void main_loop(void)
 {
-#ifndef CONFIG_SYS_HUSH_PARSER
 	static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };
 	int len;
 	int rc = 1;
 	int flag;
-#endif
 #ifdef CONFIG_PREBOOT
 	char *p;
 #endif
@@ -444,12 +442,11 @@ void main_loop(void)
 	/*
 	 * Main Loop for Monitor Command Processing
 	 */
-#ifdef CONFIG_SYS_HUSH_PARSER
-	parse_file_outer();
-	/* This point is never reached */
-	for (;;);
-#else
-	for (;;) {
+	if (autoconf_sys_hush_parser()) {
+		parse_file_outer();
+		/* This point is never reached */
+		for (;;);
+	} else {
 		if (autoconf_boot_retry_time() && rc >= 0) {
 			/* Saw enough of a valid command to
 			 * restart the timeout.
@@ -484,7 +481,6 @@ void main_loop(void)
 			lastcommand[0] = 0;
 		}
 	}
-#endif /*CONFIG_SYS_HUSH_PARSER*/
 }
 
 /*
@@ -1174,7 +1170,6 @@ int parse_line (char *line, char *argv[])
 
 /****************************************************************************/
 
-#ifndef CONFIG_SYS_HUSH_PARSER
 static void process_macros (const char *input, char *output)
 {
 	char c, prev;
@@ -1382,7 +1377,6 @@ static int builtin_run_command(const char *cmd, int flag)
 
 	return rc ? rc : repeatable;
 }
-#endif
 
 /*
  * Run a command using the selected parser.
@@ -1393,22 +1387,21 @@ static int builtin_run_command(const char *cmd, int flag)
  */
 int run_command(const char *cmd, int flag)
 {
-#ifndef CONFIG_SYS_HUSH_PARSER
-	/*
-	 * builtin_run_command can return 0 or 1 for success, so clean up
-	 * its result.
-	 */
-	if (builtin_run_command(cmd, flag) == -1)
-		return 1;
-
-	return 0;
-#else
-	return parse_string_outer(cmd,
+	if (autoconf_sys_hush_parser()) {
+		return parse_string_outer(cmd,
 			FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP);
-#endif
+	} else {
+		/*
+		* builtin_run_command can return 0 or 1 for success, so
+		* clean up its result.
+		*/
+		if (builtin_run_command(cmd, flag) == -1)
+			return 1;
+
+		return 0;
+	}
 }
 
-#ifndef CONFIG_SYS_HUSH_PARSER
 /**
  * Execute a list of command separated by ; or \n using the built-in parser.
  *
@@ -1449,7 +1442,6 @@ static int builtin_run_command_list(char *cmd, int flag)
 
 	return rcode;
 }
-#endif
 
 int run_command_list(const char *cmd, int len, int flag)
 {
@@ -1459,13 +1451,16 @@ int run_command_list(const char *cmd, int len, int flag)
 
 	if (len == -1) {
 		len = strlen(cmd);
-#ifdef CONFIG_SYS_HUSH_PARSER
-		/* hush will never change our string */
-		need_buff = 0;
-#else
-		/* the built-in parser will change our string if it sees \n */
-		need_buff = strchr(cmd, '\n') != NULL;
-#endif
+		if (autoconf_sys_hush_parser()) {
+			/* hush will never change our string */
+			need_buff = 0;
+		} else {
+			/*
+			 * the built-in parser will change our string if it
+			 * sees \n
+			 */
+			need_buff = strchr(cmd, '\n') != NULL;
+		}
 	}
 	if (need_buff) {
 		buff = malloc(len + 1);
@@ -1474,20 +1469,20 @@ int run_command_list(const char *cmd, int len, int flag)
 		memcpy(buff, cmd, len);
 		buff[len] = '\0';
 	}
-#ifdef CONFIG_SYS_HUSH_PARSER
-	rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
-#else
-	/*
-	 * This function will overwrite any \n it sees with a \0, which
-	 * is why it can't work with a const char *. Here we are making
-	 * using of internal knowledge of this function, to avoid always
-	 * doing a malloc() which is actually required only in a case that
-	 * is pretty rare.
-	 */
-	rcode = builtin_run_command_list(buff, flag);
-	if (need_buff)
-		free(buff);
-#endif
+	if (autoconf_sys_hush_parser()) {
+		rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);
+	} else {
+		/*
+		* This function will overwrite any \n it sees with a \0, which
+		* is why it can't work with a const char *. Here we are making
+		* using of internal knowledge of this function, to avoid always
+		* doing a malloc() which is actually required only in a case
+		* that is pretty rare.
+		*/
+		rcode = builtin_run_command_list(buff, flag);
+		if (need_buff)
+			free(buff);
+	}
 
 	return rcode;
 }
diff --git a/include/hush.h b/include/hush.h
index ecf9222..12c55f4 100644
--- a/include/hush.h
+++ b/include/hush.h
@@ -36,7 +36,5 @@ int set_local_var(const char *s, int flg_export);
 void unset_local_var(const char *name);
 char *get_local_var(const char *s);
 
-#if defined(CONFIG_HUSH_INIT_VAR)
 extern int hush_init_var (void);
 #endif
-#endif
-- 
1.8.3

[U-Boot] [PATCH v4 7/8] main: Use autoconf in command line reading

[Simon Glass]

Remove #ifdefs in favour of autoconf for this code. This involves removing
a few unnecessary #ifdefs in headers also.

We have two versions of the code - one that handles command line editing and
one that is just a simple implementation. Create a new function called
readline_into_buffer() which calls either cread_line() or the new
simple_readline(), created to hold the 'simple' code.

The cread_print_hist_list() function is not actually used anywhere, so punt
it.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v4: None
Changes in v3: None
Changes in v2: None

 common/main.c     | 164 ++++++++++++++++++++++++------------------------------
 include/command.h |   2 -
 include/common.h  |   2 -
 3 files changed, 73 insertions(+), 95 deletions(-)

diff --git a/common/main.c b/common/main.c
index b09bfb4..a854c3b 100644
--- a/common/main.c
+++ b/common/main.c
@@ -513,8 +513,6 @@ void reset_cmd_timeout(void)
 }
 #endif
 
-#ifdef CONFIG_CMDLINE_EDITING
-
 /*
  * cmdline-editing related codes from vivi.
  * Author: Janghoon Lyu <nandy@mizi.com>
@@ -617,27 +615,6 @@ static char* hist_next(void)
 	return (ret);
 }
 
-#ifndef CONFIG_CMDLINE_EDITING
-static void cread_print_hist_list(void)
-{
-	int i;
-	unsigned long n;
-
-	n = hist_num - hist_max;
-
-	i = hist_add_idx + 1;
-	while (1) {
-		if (i > hist_max)
-			i = 0;
-		if (i == hist_add_idx)
-			break;
-		printf("%s\n", hist_list[i]);
-		n++;
-		i++;
-	}
-}
-#endif /* CONFIG_CMDLINE_EDITING */
-
 #define BEGINNING_OF_LINE() {			\
 	while (num) {				\
 		getcmd_putch(CTL_BACKSPACE);	\
@@ -899,27 +876,27 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len,
 			REFRESH_TO_EOL();
 			continue;
 		}
-#ifdef CONFIG_AUTO_COMPLETE
-		case '\t': {
-			int num2, col;
+		case '\t':
+			if (autoconf_auto_complete()) {
+				int num2, col;
 
-			/* do not autocomplete when in the middle */
-			if (num < eol_num) {
-				getcmd_cbeep();
-				break;
-			}
+				/* do not autocomplete when in the middle */
+				if (num < eol_num) {
+					getcmd_cbeep();
+					break;
+				}
 
-			buf[num] = '\0';
-			col = strlen(prompt) + eol_num;
-			num2 = num;
-			if (cmd_auto_complete(prompt, buf, &num2, &col)) {
-				col = num2 - num;
-				num += col;
-				eol_num += col;
+				buf[num] = '\0';
+				col = strlen(prompt) + eol_num;
+				num2 = num;
+				if (cmd_auto_complete(prompt, buf, &num2,
+						      &col)) {
+					col = num2 - num;
+					num += col;
+					eol_num += col;
+				}
+				break;
 			}
-			break;
-		}
-#endif
 		default:
 			cread_add_char(ichar, insert, &num, &eol_num, buf, *len);
 			break;
@@ -935,8 +912,6 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len,
 	return 0;
 }
 
-#endif /* CONFIG_CMDLINE_EDITING */
-
 /****************************************************************************/
 
 /*
@@ -958,46 +933,14 @@ int readline (const char *const prompt)
 	return readline_into_buffer(prompt, console_buffer, 0);
 }
 
-
-int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
+static int simple_readline(const char *const prompt, int plen, char *p,
+			   int timeout)
 {
-	char *p = buffer;
-#ifdef CONFIG_CMDLINE_EDITING
-	unsigned int len = CONFIG_SYS_CBSIZE;
-	int rc;
-	static int initted = 0;
-
-	/*
-	 * History uses a global array which is not
-	 * writable until after relocation to RAM.
-	 * Revert to non-history version if still
-	 * running from flash.
-	 */
-	if (gd->flags & GD_FLG_RELOC) {
-		if (!initted) {
-			hist_init();
-			initted = 1;
-		}
-
-		if (prompt)
-			puts (prompt);
-
-		rc = cread_line(prompt, p, &len, timeout);
-		return rc < 0 ? rc : len;
-
-	} else {
-#endif	/* CONFIG_CMDLINE_EDITING */
 	char * p_buf = p;
 	int	n = 0;				/* buffer index		*/
-	int	plen = 0;			/* prompt length	*/
 	int	col;				/* output column cnt	*/
 	char	c;
 
-	/* print prompt */
-	if (prompt) {
-		plen = strlen (prompt);
-		puts (prompt);
-	}
 	col = plen;
 
 	for (;;) {
@@ -1010,12 +953,12 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
 		}
 		WATCHDOG_RESET();	/* Trigger watchdog, if needed */
 
-#ifdef CONFIG_SHOW_ACTIVITY
-		while (!tstc()) {
-			show_activity(0);
-			WATCHDOG_RESET();
+		if (autoconf_show_activity()) {
+			while (!tstc()) {
+				show_activity(0);
+				WATCHDOG_RESET();
+			}
 		}
-#endif
 		c = getc();
 
 		/*
@@ -1062,14 +1005,20 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
 			 */
 			if (n < CONFIG_SYS_CBSIZE-2) {
 				if (c == '\t') {	/* expand TABs */
-#ifdef CONFIG_AUTO_COMPLETE
-					/* if auto completion triggered just continue */
-					*p = '\0';
-					if (cmd_auto_complete(prompt, console_buffer, &n, &col)) {
-						p = p_buf + n;	/* reset */
-						continue;
+					if (autoconf_auto_complete()) {
+						/*
+						 * if auto completion triggered
+						 * just continue
+						 */
+						*p = '\0';
+						if (cmd_auto_complete(prompt,
+								console_buffer,
+								&n, &col)) {
+							/* reset */
+							p = p_buf + n;
+							continue;
+						}
 					}
-#endif
 					puts (tab_seq+(col&07));
 					col += 8 - (col&07);
 				} else {
@@ -1091,9 +1040,42 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
 			}
 		}
 	}
-#ifdef CONFIG_CMDLINE_EDITING
+}
+
+int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
+{
+	unsigned int len = CONFIG_SYS_CBSIZE;
+	int rc;
+	static int initted;
+
+	/*
+	 * History uses a global array which is not
+	 * writable until after relocation to RAM.
+	 * Revert to non-history version if still
+	 * running from flash.
+	 */
+	if (autoconf_cmdline_editing() && (gd->flags & GD_FLG_RELOC)) {
+		if (!initted) {
+			hist_init();
+			initted = 1;
+		}
+
+		if (prompt)
+			puts(prompt);
+
+		rc = cread_line(prompt, buffer, &len, timeout);
+		return rc < 0 ? rc : len;
+
+	} else {
+		int plen = 0;			/* prompt length */
+
+		/* print prompt */
+		if (prompt) {
+			plen = strlen(prompt);
+			puts(prompt);
+		}
+		return simple_readline(prompt, plen, buffer, timeout);
 	}
-#endif
 }
 
 /****************************************************************************/
diff --git a/include/command.h b/include/command.h
index 65692fd..813297c 100644
--- a/include/command.h
+++ b/include/command.h
@@ -75,10 +75,8 @@ cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len);
 
 extern int cmd_usage(const cmd_tbl_t *cmdtp);
 
-#ifdef CONFIG_AUTO_COMPLETE
 extern int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[]);
 extern int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp);
-#endif
 
 /*
  * Monitor Command
diff --git a/include/common.h b/include/common.h
index de18083..f38cdad 100644
--- a/include/common.h
+++ b/include/common.h
@@ -906,9 +906,7 @@ int	pcmcia_init (void);
 
 #include <bootstage.h>
 
-#ifdef CONFIG_SHOW_ACTIVITY
 void show_activity(int arg);
-#endif
 
 /* Multicore arch functions */
 #ifdef CONFIG_MP
-- 
1.8.3

[U-Boot] [PATCH v4 8/8] main: Use autoconf in main_loop()

[Simon Glass]

Convert main_loop() over to use autoconf, and add a required prototype
to common.h.

The do_mdm_init variable is now always defined, but this seems like an
acceptable compromise.

In fdt_support.h the #ifdef used is CONFIG_OF_LIBFDT. However, even if
this is not defined we want to make the functions available for our
conditional-compilation scheme. The only place where we really don't
have access to these support functions is when USE_HOSTCC is defined.
So change the #ifdef to that.

Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v4:
- Rebase on current master

Changes in v3:
- Remove the extra config_of_libfdt() condition in main_loop()

Changes in v2: None

 common/main.c         | 72 +++++++++++++++++++++++----------------------------
 include/common.h      |  1 +
 include/fdt_support.h |  4 +--
 3 files changed, 35 insertions(+), 42 deletions(-)

diff --git a/common/main.c b/common/main.c
index a854c3b..a5ef5fb 100644
--- a/common/main.c
+++ b/common/main.c
@@ -70,10 +70,7 @@ static int      retry_time = -1; /* -1 so can call readline before main_loop */
 
 #define	endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk())
 
-#ifdef CONFIG_MODEM_SUPPORT
 int do_mdm_init = 0;
-extern void mdm_init(void); /* defined in board.c */
-#endif
 
 /***************************************************************************
  * Watch for 'delay' seconds for autoboot stop or autoboot delay string.
@@ -386,51 +383,47 @@ void main_loop(void)
 	int len;
 	int rc = 1;
 	int flag;
-#ifdef CONFIG_PREBOOT
-	char *p;
-#endif
 
 	bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
 
-#ifdef CONFIG_MODEM_SUPPORT
-	debug("DEBUG: main_loop:   do_mdm_init=%d\n", do_mdm_init);
-	if (do_mdm_init) {
-		char *str = strdup(getenv("mdm_cmd"));
-		setenv("preboot", str);  /* set or delete definition */
-		if (str != NULL)
-			free(str);
-		mdm_init(); /* wait for modem connection */
+	if (autoconf_modem_support()) {
+		debug("DEBUG: main_loop:   do_mdm_init=%d\n", do_mdm_init);
+		if (do_mdm_init) {
+			char *str = strdup(getenv("mdm_cmd"));
+
+			setenv("preboot", str);  /* set or delete definition */
+			if (str != NULL)
+				free(str);
+			mdm_init(); /* wait for modem connection */
+		}
 	}
-#endif  /* CONFIG_MODEM_SUPPORT */
 
-#ifdef CONFIG_VERSION_VARIABLE
-	{
+	if (autoconf_version_variable())
 		setenv("ver", version_string);  /* set version variable */
-	}
-#endif /* CONFIG_VERSION_VARIABLE */
 
-#ifdef CONFIG_SYS_HUSH_PARSER
-	u_boot_hush_start();
-#endif
+	if (autoconf_sys_hush_parser())
+		u_boot_hush_start();
 
-#if defined(CONFIG_HUSH_INIT_VAR)
-	hush_init_var();
-#endif
+	if (autoconf_hush_init_var())
+		hush_init_var();
+
+	if (autoconf_preboot()) {
+		char *p = getenv("preboot");
+
+		if (p) {
+			int prev;
 
-#ifdef CONFIG_PREBOOT
-	p = getenv("preboot");
-	if (p != NULL) {
-# ifdef CONFIG_AUTOBOOT_KEYED
-		int prev = disable_ctrlc(1);	/* disable Control C checking */
-# endif
+			/* disable Control C checking */
+			if (autoconf_autoboot_keyed())
+				prev = disable_ctrlc(1);
 
-		run_command_list(p, -1, 0);
+			run_command_list(p, -1, 0);
 
-# ifdef CONFIG_AUTOBOOT_KEYED
-		disable_ctrlc(prev);	/* restore Control C checking */
-# endif
+			/* restore Control C checking */
+			if (autoconf_autoboot_keyed())
+				disable_ctrlc(prev);
+		}
 	}
-#endif /* CONFIG_PREBOOT */
 
 #if defined(CONFIG_UPDATE_TFTP)
 	update_tftp(0UL);
@@ -472,14 +465,13 @@ void main_loop(void)
 		}
 
 		if (len == -1)
-			puts ("<INTERRUPT>\n");
+			puts("<INTERRUPT>\n");
 		else
 			rc = run_command(lastcommand, flag);
 
-		if (rc <= 0) {
-			/* invalid command or not repeatable, forget it */
+		/* If an invalid command or not repeatable, forget it */
+		if (rc <= 0)
 			lastcommand[0] = 0;
-		}
 	}
 }
 
diff --git a/include/common.h b/include/common.h
index f38cdad..0118ebf 100644
--- a/include/common.h
+++ b/include/common.h
@@ -328,6 +328,7 @@ extern u8 _binary_dt_dtb_start[];	/* embedded device tree blob */
 int set_cpu_clk_info(void);
 int print_cpuinfo(void);
 int update_flash_size(int flash_size);
+extern int mdm_init(void); /* defined in board.c */
 
 /**
  * Show the DRAM size in a board-specific way
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 8f07a67..ce34c6b 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -24,7 +24,7 @@
 #ifndef __FDT_SUPPORT_H
 #define __FDT_SUPPORT_H
 
-#ifdef CONFIG_OF_LIBFDT
+#ifndef USE_HOSTCC
 
 #include <libfdt.h>
 
@@ -130,5 +130,5 @@ static inline int fdt_status_disabled_by_alias(void *fdt, const char* alias)
 	return fdt_set_status_by_alias(fdt, alias, FDT_STATUS_DISABLED, 0);
 }
 
-#endif /* ifdef CONFIG_OF_LIBFDT */
+#endif /* ifdef USE_HOSTCC */
 #endif /* ifndef __FDT_SUPPORT_H */
-- 
1.8.3

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Albert ARIBAUD]

Hi Simon,

On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass <sjg@chromium.org>
wrote:

> Note that a config_drop.h file is added - this defines all the CONFIGs
> which are not used in any board config file. Without this, autoconf cannot
> define the macros for this CONFIGs.
> 
> Compile time for main.c does not seem to be any different in my tests. The
> time to perform the 'dep' step (which now creates autoconf.h) increases,
> from about 2.8s to about 4.6s. This additional time is used to grep, sed
> and sort the contents of all the header file in U-Boot. The time for an
> incremental build is not affected.
> 
> It would be much more efficient to maintain a list of all available CONFIG
> defines, but no such list exists at present.

Stop me if I am wrong, but do you not have this list already, since at
one point you grep, sed and sort the whole list of config options, then
at another point generate the list of unused ones?

Granted, that's the list of config options defined, not necessarily the
list of options used, but a second variation of the grep/sed/sort might
give you a hint on that.

Plus, I would love having scripts in tools/ that look for either
defined or used config options.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Albert,

On Sun, Jun 23, 2013 at 12:29 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net>wrote:

> Hi Simon,
>
> On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass <sjg@chromium.org>
> wrote:
>
> > Note that a config_drop.h file is added - this defines all the CONFIGs
> > which are not used in any board config file. Without this, autoconf
> cannot
> > define the macros for this CONFIGs.
> >
> > Compile time for main.c does not seem to be any different in my tests.
> The
> > time to perform the 'dep' step (which now creates autoconf.h) increases,
> > from about 2.8s to about 4.6s. This additional time is used to grep, sed
> > and sort the contents of all the header file in U-Boot. The time for an
> > incremental build is not affected.
> >
> > It would be much more efficient to maintain a list of all available
> CONFIG
> > defines, but no such list exists at present.
>
> Stop me if I am wrong, but do you not have this list already, since at
> one point you grep, sed and sort the whole list of config options, then
> at another point generate the list of unused ones?
>

Well yes I create the list. But I don't 'have' it in the sense that it is a
pre-existing file in the tree. My point was that if the file existed I
would not need to create it in the build system. I asked about this at one
point, and the comment was made that putting it in the source tree
'staticly' is risky, since someone might add a new option and it would not
work.

Perhaps when Kconfig is in there things will be different.


>
> Granted, that's the list of config options defined, not necessarily the
> list of options used, but a second variation of the grep/sed/sort might
> give you a hint on that.
>
> Plus, I would love having scripts in tools/ that look for either
> defined or used config options.
>

With this series you kind-of get this feature - you can look at the files
it creates along the way.

Regards,
Simon

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Albert ARIBAUD]

Hi Simon,

On Mon, 24 Jun 2013 17:52:03 -0700, Simon Glass <sjg@chromium.org>
wrote:

> Hi Albert,
> 
> On Sun, Jun 23, 2013 at 12:29 AM, Albert ARIBAUD
> <albert.u.boot@aribaud.net>wrote:
> 
> > Hi Simon,
> >
> > On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass <sjg@chromium.org>
> > wrote:
> >
> > > Note that a config_drop.h file is added - this defines all the CONFIGs
> > > which are not used in any board config file. Without this, autoconf
> > cannot
> > > define the macros for this CONFIGs.
> > >
> > > Compile time for main.c does not seem to be any different in my tests.
> > The
> > > time to perform the 'dep' step (which now creates autoconf.h) increases,
> > > from about 2.8s to about 4.6s. This additional time is used to grep, sed
> > > and sort the contents of all the header file in U-Boot. The time for an
> > > incremental build is not affected.
> > >
> > > It would be much more efficient to maintain a list of all available
> > CONFIG
> > > defines, but no such list exists at present.
> >
> > Stop me if I am wrong, but do you not have this list already, since at
> > one point you grep, sed and sort the whole list of config options, then
> > at another point generate the list of unused ones?
> >
> 
> Well yes I create the list. But I don't 'have' it in the sense that it is a
> pre-existing file in the tree. My point was that if the file existed I
> would not need to create it in the build system. I asked about this at one
> point, and the comment was made that putting it in the source tree
> 'staticly' is risky, since someone might add a new option and it would not
> work.
> 
> Perhaps when Kconfig is in there things will be different.

Understod.

> > Granted, that's the list of config options defined, not necessarily the
> > list of options used, but a second variation of the grep/sed/sort might
> > give you a hint on that.
> >
> > Plus, I would love having scripts in tools/ that look for either
> > defined or used config options.
> >
> 
> With this series you kind-of get this feature - you can look at the files
> it creates along the way.

What I meant is, this patch creates those lists with scripts. I'd like
these scripts to be available to the developer in tools/ so that anyone
can regenerate the lists (and do this only) at any time.

> Regards,
> Simon

Amicalement,
-- 
Albert.

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Albert,

On Thu, Jun 27, 2013 at 12:04 AM, Albert ARIBAUD
<albert.u.boot@aribaud.net>wrote:

> Hi Simon,
>
> On Mon, 24 Jun 2013 17:52:03 -0700, Simon Glass <sjg@chromium.org>
> wrote:
>
> > Hi Albert,
> >
> > On Sun, Jun 23, 2013 at 12:29 AM, Albert ARIBAUD
> > <albert.u.boot@aribaud.net>wrote:
> >
> > > Hi Simon,
> > >
> > > On Mon, 17 Jun 2013 07:44:52 -0700, Simon Glass <sjg@chromium.org>
> > > wrote:
> > >
> > > > Note that a config_drop.h file is added - this defines all the
> CONFIGs
> > > > which are not used in any board config file. Without this, autoconf
> > > cannot
> > > > define the macros for this CONFIGs.
> > > >
> > > > Compile time for main.c does not seem to be any different in my
> tests.
> > > The
> > > > time to perform the 'dep' step (which now creates autoconf.h)
> increases,
> > > > from about 2.8s to about 4.6s. This additional time is used to grep,
> sed
> > > > and sort the contents of all the header file in U-Boot. The time for
> an
> > > > incremental build is not affected.
> > > >
> > > > It would be much more efficient to maintain a list of all available
> > > CONFIG
> > > > defines, but no such list exists at present.
> > >
> > > Stop me if I am wrong, but do you not have this list already, since at
> > > one point you grep, sed and sort the whole list of config options, then
> > > at another point generate the list of unused ones?
> > >
> >
> > Well yes I create the list. But I don't 'have' it in the sense that it
> is a
> > pre-existing file in the tree. My point was that if the file existed I
> > would not need to create it in the build system. I asked about this at
> one
> > point, and the comment was made that putting it in the source tree
> > 'staticly' is risky, since someone might add a new option and it would
> not
> > work.
> >
> > Perhaps when Kconfig is in there things will be different.
>
> Understod.
>
> > > Granted, that's the list of config options defined, not necessarily the
> > > list of options used, but a second variation of the grep/sed/sort might
> > > give you a hint on that.
> > >
> > > Plus, I would love having scripts in tools/ that look for either
> > > defined or used config options.
> > >
> >
> > With this series you kind-of get this feature - you can look at the files
> > it creates along the way.
>
> What I meant is, this patch creates those lists with scripts. I'd like
> these scripts to be available to the developer in tools/ so that anyone
> can regenerate the lists (and do this only) at any time.
>

Yes, I figured that's what you were angling for. I will take a look.

Regards,
Simon

[U-Boot] [PATCH v4 4/8] main: Use autoconf to remove #ifdefs around process_boot_delay()

[Michal Simek]

On 06/17/2013 04:44 PM, Simon Glass wrote:
> Use autoconf to make process_boot_delay() be compiled always, and adjust
> the caller and related functions as needed.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v4:
> - Split out new patch to remove #ifdefs around process_boot_delay()
> 
> Changes in v3: None
> Changes in v2: None
> 
>  common/main.c | 71 ++++++++++++++++++++++++++---------------------------------
>  1 file changed, 31 insertions(+), 40 deletions(-)
> 
> diff --git a/common/main.c b/common/main.c
> index 3a4754d..dba6cee 100644
> --- a/common/main.c
> +++ b/common/main.c
> @@ -79,8 +79,6 @@ extern void mdm_init(void); /* defined in board.c */
>   * Watch for 'delay' seconds for autoboot stop or autoboot delay string.
>   * returns: 0 -  no key string, allow autoboot 1 - got key string, abort
>   */
> -#if defined(CONFIG_BOOTDELAY)
> -# if defined(CONFIG_AUTOBOOT_KEYED)
>  static int abortboot_keyed(int bootdelay)
>  {
>  	int abort = 0;
> @@ -173,8 +171,6 @@ static int abortboot_keyed(int bootdelay)
>  	return abort;
>  }
>  
> -# else	/* !defined(CONFIG_AUTOBOOT_KEYED) */
> -
>  static int menukey;
>  
>  static int abortboot_normal(int bootdelay)
> @@ -228,17 +224,14 @@ static int abortboot_normal(int bootdelay)
>  
>  	return abort;
>  }
> -# endif	/* CONFIG_AUTOBOOT_KEYED */
>  
>  static int abortboot(int bootdelay)
>  {
> -#ifdef CONFIG_AUTOBOOT_KEYED
> -	return abortboot_keyed(bootdelay);
> -#else
> -	return abortboot_normal(bootdelay);
> -#endif
> +	if (autoconf_autoboot_keyed())
> +		return abortboot_keyed(bootdelay);
> +	else
> +		return abortboot_normal(bootdelay);
>  }
> -#endif	/* CONFIG_BOOTDELAY */
>  
>  /*
>   * Runs the given boot command securely.  Specifically:
> @@ -254,7 +247,6 @@ static int abortboot(int bootdelay)
>   * printing the error message to console.
>   */
>  
> -#if defined(CONFIG_BOOTDELAY) && defined(CONFIG_OF_CONTROL)
>  static void secure_boot_cmd(char *cmd)
>  {
>  	cmd_tbl_t *cmdtp;
> @@ -295,22 +287,21 @@ static void process_fdt_options(const void *blob)
>  
>  	/* Add an env variable to point to a kernel payload, if available */
>  	addr = fdtdec_get_config_int(gd->fdt_blob, "kernel-offset", 0);
> -	if (addr)
> -		setenv_addr("kernaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr));
> +	if (addr) {
> +		setenv_addr("kernaddr",
> +			    (void *)(autoconf_sys_text_base() + addr));
> +	}
>  
>  	/* Add an env variable to point to a root disk, if available */
>  	addr = fdtdec_get_config_int(gd->fdt_blob, "rootdisk-offset", 0);
> -	if (addr)
> -		setenv_addr("rootaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr));
> +	if (addr) {
> +		setenv_addr("rootaddr",
> +			    (void *)(autoconf_sys_text_base() + addr));
> +	}
>  }
> -#endif /* CONFIG_OF_CONTROL */
>  
> -#ifdef CONFIG_BOOTDELAY
>  static void process_boot_delay(void)
>  {
> -#ifdef CONFIG_OF_CONTROL
> -	char *env;
> -#endif
>  	char *s;
>  	int bootdelay;
>  #ifdef CONFIG_BOOTCOUNT_LIMIT
> @@ -327,7 +318,7 @@ static void process_boot_delay(void)
>  #endif /* CONFIG_BOOTCOUNT_LIMIT */
>  
>  	s = getenv ("bootdelay");
> -	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY;
> +	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : autoconf_bootdelay();
>  
>  #ifdef CONFIG_OF_CONTROL
>  	bootdelay = fdtdec_get_config_int(gd->fdt_blob, "bootdelay",
> @@ -357,23 +348,24 @@ static void process_boot_delay(void)
>  	else
>  #endif /* CONFIG_BOOTCOUNT_LIMIT */
>  		s = getenv ("bootcmd");
> -#ifdef CONFIG_OF_CONTROL
> -	/* Allow the fdt to override the boot command */
> -	env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");
> -	if (env)
> -		s = env;
> +	if (autoconf_of_control()) {
> +		char *env;
>  
> -	process_fdt_options(gd->fdt_blob);
> +		/* Allow the fdt to override the boot command */
> +		env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");
> +		if (env)
> +			s = env;
>  
> -	/*
> -	 * If the bootsecure option was chosen, use secure_boot_cmd().
> -	 * Always use 'env' in this case, since bootsecure requres that the
> -	 * bootcmd was specified in the FDT too.
> -	 */
> -	if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0))
> -		secure_boot_cmd(env);
> +		process_fdt_options(gd->fdt_blob);
>  
> -#endif /* CONFIG_OF_CONTROL */
> +		/*
> +		* If the bootsecure option was chosen, use secure_boot_cmd().
> +		* Always use 'env' in this case, since bootsecure requres that
> +		* the bootcmd was specified in the FDT too.
> +		*/

This indentation doesn't look good to me.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 263 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131028/84c268eb/attachment.pgp>

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Detlev,

On 17 January 2014 08:13, Detlev Zundel <dzu@denx.de> wrote:
>
> Hi Simon,
>
> [...]
>
> >> I think the Linux code has two big advantages - for one, we increase the
> >> overlap with Linux kernel proper and secondly we keep the 'grep'ability
> >> of the names which I really missed in your proposal.
> >
> > Yes that seems reasonable.
>
> Ok, I'm glad we are in agreement.
>
> >>> Many of U-Boot's options are not just yes/no, so I'm not sure what will
> >>> happen with those. Perhaps they just can't be used with this macro? Part of
> >>> my intent was to allow any option to be used.
> >>
> >> In those cases the defines then are a "shortcut" to using a boolean + a
> >> value and we can make it work by introducing this boolean?  I have no
> >> idea of how much work this would be, but it might be worthwhile getting
> >> some real numbers to that.
> >>
> >>> So for example you can do this:
> >>>
> >>> struct {
> >>>> const char *str;
> >>>> u_int len;
> >>>> int retry;
> >>>> const char *conf; /* Configuration value */
> >>>> }
> >>>> delaykey [] = {
> >>>> { str: getenv("bootdelaykey"),  retry: 1,
> >>>> conf: autoconf_autoboot_delay_str() },
> >>>> { str: getenv("bootdelaykey2"), retry: 1,
> >>>> conf: autoconf_autoboot_delay_str2() },
> >>>> { str: getenv("bootstopkey"),   retry: 0,
> >>>> conf: autoconf_autoboot_stop_str() },
> >>>> { str: getenv("bootstopkey2"),  retry: 0,
> >>>> conf: autoconf_autoboot_stop_str2() },
> >>>> };
> >>>
> >>>
> >>>
> >>> or this:
> >>>
> >>> /* don't retry auto boot? */
> >>>> if (autoconf_boot_retry_time() &&
> >>>>     !delaykey[i].retry)
> >>>> retry_time = -1;
> >>>
> >>>
> >>> Note that autoconf_boot_retry_time() will return 0 if the CONFIG is not
> >>> defined, or if its value is 0.
> >>
> >> I'm having real trouble figuring out what this would do on first sight.
> >> Of course you could call me lazy, but by experience I tend to favour
> >> solutions that do not need geniuses to understand ;)
> >
> > Well it is simply that we currently have options which may or may not
> > be defined. If they are not defined, then it is an error to refer to
> > them, so they must be guarded by an #ifdef. By defining them to 0 when
> > not defined, we can avoid that guard.
>
> Ok, I see.  But in this way we are actually shutting up the compiler on
> code paths that we did not have before.  This in effect is a "rather be
> quiet than error" strategy and I'm not sure that I want to use that
> when doing such changes.  Call me old-fashioned, but I'd prefer to throw
> an error and fix it after thinking it through from todays perspective
>
> (I can say this easily if I'm not the one who has to do the fixes ;)


Well another approach would be to change the meaning of options like
CONFIG_BOOTDELAY:

- Boot Delay: CONFIG_BOOTDELAY - in seconds
Delay before automatically booting the default image;
set to -1 to disable autoboot.
set to -2 to autoboot with no delay and not check for abort
(even when CONFIG_ZERO_BOOTDELAY_CHECK is defined).

Here it is enabled if >= 0, disabled if -1 and -2 for another option.
If it is not defined at all then it is effectively -1, meaning
disabled. IMO these could be separated out a little better. The
problem here is that we can't have code like:

if (CONFIG_BOOTDELAY != -1) {
   // do some bootdelay processing
}

as we will get a compile error if CONFIG_BOOTDELAY is not defined. In
this case, CONFIG_BOOTDELAY being undefined is equivalent to defining
it to -1 (I think).

There are only a small number of options in this category, but even
with one, it prevents the technique above.

>
>
> >>> It seems to me we should provide the Linux feature in U-Boot as part of the
> >>> Kconfig stuff, and see where it takes us. Combined with a bit more
> >>> rationalisation of things like main.c it might be enough.
> >>
> >> Why not reimplement your patch set along those lines?  I still really
> >> would _love_ to see us using the compiler more to check for errors and
> >> reduce the number of "potential source code combination" that we have.
> >
> > Well certainly I could, but I did not want to do this while
> > Kbuild/Kconfig is in progress, and we are not quite clear on what to
> > do. I think Kbuild is done - we will probably get the Linux autoconf
> > stuff for free with Kconfig which I understand is coming very soon.
>
> This makes a lot of sense yes.  Actually I'm pretty excited about this
> excellent and continuous work from Masahiro-san on that topic!

Yes, looking forward to an early merge!

>
>
> > After that I can certainly take another look at main.c and see how it
> > can be improved.
>
> Sure, its only that I wanted to keep the ball rolling as I really
> believe the wins to be had from such a change are substantial for our
> codebase.

Certainly main.c could use something like this as my example showed.
In fact it might be nice to split out the parser into a separate file.

Regards,
Simon

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Detlev Zundel]

Hi Simon,

[...]

>> I think the Linux code has two big advantages - for one, we increase the
>> overlap with Linux kernel proper and secondly we keep the 'grep'ability
>> of the names which I really missed in your proposal.
>
> Yes that seems reasonable.

Ok, I'm glad we are in agreement.

>>> Many of U-Boot's options are not just yes/no, so I'm not sure what will
>>> happen with those. Perhaps they just can't be used with this macro? Part of
>>> my intent was to allow any option to be used.
>>
>> In those cases the defines then are a "shortcut" to using a boolean + a
>> value and we can make it work by introducing this boolean?  I have no
>> idea of how much work this would be, but it might be worthwhile getting
>> some real numbers to that.
>>
>>> So for example you can do this:
>>>
>>> struct {
>>>> const char *str;
>>>> u_int len;
>>>> int retry;
>>>> const char *conf; /* Configuration value */
>>>> }
>>>> delaykey [] = {
>>>> { str: getenv("bootdelaykey"),  retry: 1,
>>>> conf: autoconf_autoboot_delay_str() },
>>>> { str: getenv("bootdelaykey2"), retry: 1,
>>>> conf: autoconf_autoboot_delay_str2() },
>>>> { str: getenv("bootstopkey"),   retry: 0,
>>>> conf: autoconf_autoboot_stop_str() },
>>>> { str: getenv("bootstopkey2"),  retry: 0,
>>>> conf: autoconf_autoboot_stop_str2() },
>>>> };
>>>
>>>
>>>
>>> or this:
>>>
>>> /* don't retry auto boot? */
>>>> if (autoconf_boot_retry_time() &&
>>>>     !delaykey[i].retry)
>>>> retry_time = -1;
>>>
>>>
>>> Note that autoconf_boot_retry_time() will return 0 if the CONFIG is not
>>> defined, or if its value is 0.
>>
>> I'm having real trouble figuring out what this would do on first sight.
>> Of course you could call me lazy, but by experience I tend to favour
>> solutions that do not need geniuses to understand ;)
>
> Well it is simply that we currently have options which may or may not
> be defined. If they are not defined, then it is an error to refer to
> them, so they must be guarded by an #ifdef. By defining them to 0 when
> not defined, we can avoid that guard.

Ok, I see.  But in this way we are actually shutting up the compiler on
code paths that we did not have before.  This in effect is a "rather be
quiet than error" strategy and I'm not sure that I want to use that
when doing such changes.  Call me old-fashioned, but I'd prefer to throw
an error and fix it after thinking it through from todays perspective

(I can say this easily if I'm not the one who has to do the fixes ;)

>>> It seems to me we should provide the Linux feature in U-Boot as part of the
>>> Kconfig stuff, and see where it takes us. Combined with a bit more
>>> rationalisation of things like main.c it might be enough.
>>
>> Why not reimplement your patch set along those lines?  I still really
>> would _love_ to see us using the compiler more to check for errors and
>> reduce the number of "potential source code combination" that we have.
>
> Well certainly I could, but I did not want to do this while
> Kbuild/Kconfig is in progress, and we are not quite clear on what to
> do. I think Kbuild is done - we will probably get the Linux autoconf
> stuff for free with Kconfig which I understand is coming very soon.

This makes a lot of sense yes.  Actually I'm pretty excited about this
excellent and continuous work from Masahiro-san on that topic!

> After that I can certainly take another look at main.c and see how it
> can be improved.

Sure, its only that I wanted to keep the ball rolling as I really
believe the wins to be had from such a change are substantial for our
codebase.

Cheers
  Detlev

-- 
Golden rule #12:   When the comments do not match the code, they probably are
both wrong. -- Steven Rostedt <1300126962.9910.128.camel@gandalf.stny.rr.com>
--
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 v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Detlev,

On 14 January 2014 03:11, Detlev Zundel <dzu@denx.de> wrote:
> Hi Simon,
>
> as I don't see any follow-up, allow me to jump in here even that late :)
>
>> Hi Wolfgang,
>>
>> On Wed, Nov 6, 2013 at 1:24 AM, Wolfgang Denk <wd@denx.de> wrote:
>>> Dear Simon Glass,
>>>
>>> In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
>>>> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>>>> different boards compile different versions of the source code, meaning
>>>> that we must build all boards to check for failures. It is easy to
>> misspell
>>>> an #ifdef and there is not as much checking of this by the compiler.
>> Multiple
>>>> dependent #ifdefs are harder to do than with if..then..else. Variable
>>>> declarations must be #idefed as well as the code that uses them, often
>> much
>>>> later in the file/function. #ifdef indents don't match code indents and
>>>> have their own separate indent feature. Overall, excessive use of #idef
>>>> hurts readability and makes the code harder to modify and refactor. For
>>>> people coming newly into the code base, #ifdefs can be a big barrier.
>>>>
>>>> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>>>> attempt to turn the tide, this series includes a patch which provides a
>> way
>>>> to make CONFIG macros available to C code without using the preprocessor.
>>>> This makes it possible to use standard C conditional features such as
>>>> if/then instead of #ifdef. A README update exhorts compliance.
>>>
>>> As mentioned before, I'm really interested in seeing something like
>>> this going into mainline, but I have some doubts about the actual
>>> implementation.
>>>
>>> To summarize:  Your current proposal was to convert code snippets
>>> like this:
>>>
>>>         #ifdef CONFIG_VERSION_VARIABLE
>>>                 setenv("ver", version_string);  /* set version variable */
>>>         #endif
>>>
>>> into
>>>
>>>         if (autoconf_version_variable())
>>>                 setenv("ver", version_string);  /* set version variable */
>>>
>>> By chance I ran about "include/linux/kconfig.h" in the Linux kernel
>>> tree, which provides (among other things) the IS_ENABLED() macro that
>>> implements essentially the very same feature.  Using this, the same
>>> code would be written as:
>>>
>>>         if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
>>>                 setenv("ver", version_string);  /* set version variable */
>>>
>>> I agree that this does not solve some of the isses that have been
>>> raised about this change (indentation level increses - which may in
>>> turn require reformatting of bigger parts of the code; code becomes
>>> less readable), but on the other hand it avoids the need for a new
>>> autoconf header file, and it should be possible to introduce this
>>> easily step by step.
>>>
>>> And I really like the idea of re-using existing code that is already
>>> known to Linux hackers, especially as we we are currently having our
>>> eyes on the Kconfig stuff anyway.
>>>
>>>
>>> What do you think?
>>
>> Fair enough, sounds reasonable. The relevant kernel code is quite unusual...
>>
>> /*
>>>  * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
>>>  * these only work with boolean and tristate options.
>>>  */
>>> /*
>>>  * Getting something that works in C and CPP for an arg that may or may
>>>  * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
>>>  * we match on the placeholder define, insert the "0," for arg1 and
>>> generate
>>>  * the triplet (0, 1, 0).  Then the last step cherry picks the 2nd arg (a
>>> one).
>>>  * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and
>>> when
>>>  * the last step cherry picks the 2nd arg, we get a zero.
>>>  */
>>> #define __ARG_PLACEHOLDER_1 0,
>>> #define config_enabled(cfg) _config_enabled(cfg)
>>> #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
>>> #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
>>> #define ___config_enabled(__ignored, val, ...) val
>>> /*
>>>  * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or
>>> 'm',
>>>  * 0 otherwise.
>>>  *
>>>  */
>>> #define IS_ENABLED(option) \
>>> (config_enabled(option) || config_enabled(option##_MODULE))
>>
>
> I think the Linux code has two big advantages - for one, we increase the
> overlap with Linux kernel proper and secondly we keep the 'grep'ability
> of the names which I really missed in your proposal.

Yes that seems reasonable.

>
>> Many of U-Boot's options are not just yes/no, so I'm not sure what will
>> happen with those. Perhaps they just can't be used with this macro? Part of
>> my intent was to allow any option to be used.
>
> In those cases the defines then are a "shortcut" to using a boolean + a
> value and we can make it work by introducing this boolean?  I have no
> idea of how much work this would be, but it might be worthwhile getting
> some real numbers to that.
>
>> So for example you can do this:
>>
>> struct {
>>> const char *str;
>>> u_int len;
>>> int retry;
>>> const char *conf; /* Configuration value */
>>> }
>>> delaykey [] = {
>>> { str: getenv("bootdelaykey"),  retry: 1,
>>> conf: autoconf_autoboot_delay_str() },
>>> { str: getenv("bootdelaykey2"), retry: 1,
>>> conf: autoconf_autoboot_delay_str2() },
>>> { str: getenv("bootstopkey"),   retry: 0,
>>> conf: autoconf_autoboot_stop_str() },
>>> { str: getenv("bootstopkey2"),  retry: 0,
>>> conf: autoconf_autoboot_stop_str2() },
>>> };
>>
>>
>>
>> or this:
>>
>> /* don't retry auto boot? */
>>> if (autoconf_boot_retry_time() &&
>>>     !delaykey[i].retry)
>>> retry_time = -1;
>>
>>
>> Note that autoconf_boot_retry_time() will return 0 if the CONFIG is not
>> defined, or if its value is 0.
>
> I'm having real trouble figuring out what this would do on first sight.
> Of course you could call me lazy, but by experience I tend to favour
> solutions that do not need geniuses to understand ;)

Well it is simply that we currently have options which may or may not
be defined. If they are not defined, then it is an error to refer to
them, so they must be guarded by an #ifdef. By defining them to 0 when
not defined, we can avoid that guard.

>
>> It seems to me we should provide the Linux feature in U-Boot as part of the
>> Kconfig stuff, and see where it takes us. Combined with a bit more
>> rationalisation of things like main.c it might be enough.
>
> Why not reimplement your patch set along those lines?  I still really
> would _love_ to see us using the compiler more to check for errors and
> reduce the number of "potential source code combination" that we have.

Well certainly I could, but I did not want to do this while
Kbuild/Kconfig is in progress, and we are not quite clear on what to
do. I think Kbuild is done - we will probably get the Linux autoconf
stuff for free with Kconfig which I understand is coming very soon.

After that I can certainly take another look at main.c and see how it
can be improved.

>
> Thanks for all the work so far!
>   Detlev
>
> --
> The  C++  STL, with its dyslexia-inducing syntax blizzard of colons and angle
> brackets, guarantees that if you try to declare any reasonable data structure,
> your first seven attempts will result in compiler errors of Wagnerian fierce-
> ness.                         -- James Mickens

Hard to disagree with this.

> --
> 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

Regards,
Simon

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Detlev Zundel]

Hi Simon,

as I don't see any follow-up, allow me to jump in here even that late :)

> Hi Wolfgang,
>
> On Wed, Nov 6, 2013 at 1:24 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Simon Glass,
>>
>> In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
>>> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>>> different boards compile different versions of the source code, meaning
>>> that we must build all boards to check for failures. It is easy to
> misspell
>>> an #ifdef and there is not as much checking of this by the compiler.
> Multiple
>>> dependent #ifdefs are harder to do than with if..then..else. Variable
>>> declarations must be #idefed as well as the code that uses them, often
> much
>>> later in the file/function. #ifdef indents don't match code indents and
>>> have their own separate indent feature. Overall, excessive use of #idef
>>> hurts readability and makes the code harder to modify and refactor. For
>>> people coming newly into the code base, #ifdefs can be a big barrier.
>>>
>>> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>>> attempt to turn the tide, this series includes a patch which provides a
> way
>>> to make CONFIG macros available to C code without using the preprocessor.
>>> This makes it possible to use standard C conditional features such as
>>> if/then instead of #ifdef. A README update exhorts compliance.
>>
>> As mentioned before, I'm really interested in seeing something like
>> this going into mainline, but I have some doubts about the actual
>> implementation.
>>
>> To summarize:  Your current proposal was to convert code snippets
>> like this:
>>
>>         #ifdef CONFIG_VERSION_VARIABLE
>>                 setenv("ver", version_string);  /* set version variable */
>>         #endif
>>
>> into
>>
>>         if (autoconf_version_variable())
>>                 setenv("ver", version_string);  /* set version variable */
>>
>> By chance I ran about "include/linux/kconfig.h" in the Linux kernel
>> tree, which provides (among other things) the IS_ENABLED() macro that
>> implements essentially the very same feature.  Using this, the same
>> code would be written as:
>>
>>         if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
>>                 setenv("ver", version_string);  /* set version variable */
>>
>> I agree that this does not solve some of the isses that have been
>> raised about this change (indentation level increses - which may in
>> turn require reformatting of bigger parts of the code; code becomes
>> less readable), but on the other hand it avoids the need for a new
>> autoconf header file, and it should be possible to introduce this
>> easily step by step.
>>
>> And I really like the idea of re-using existing code that is already
>> known to Linux hackers, especially as we we are currently having our
>> eyes on the Kconfig stuff anyway.
>>
>>
>> What do you think?
>
> Fair enough, sounds reasonable. The relevant kernel code is quite unusual...
>
> /*
>>  * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
>>  * these only work with boolean and tristate options.
>>  */
>> /*
>>  * Getting something that works in C and CPP for an arg that may or may
>>  * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
>>  * we match on the placeholder define, insert the "0," for arg1 and
>> generate
>>  * the triplet (0, 1, 0).  Then the last step cherry picks the 2nd arg (a
>> one).
>>  * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and
>> when
>>  * the last step cherry picks the 2nd arg, we get a zero.
>>  */
>> #define __ARG_PLACEHOLDER_1 0,
>> #define config_enabled(cfg) _config_enabled(cfg)
>> #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
>> #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
>> #define ___config_enabled(__ignored, val, ...) val
>> /*
>>  * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or
>> 'm',
>>  * 0 otherwise.
>>  *
>>  */
>> #define IS_ENABLED(option) \
>> (config_enabled(option) || config_enabled(option##_MODULE))
>

I think the Linux code has two big advantages - for one, we increase the
overlap with Linux kernel proper and secondly we keep the 'grep'ability
of the names which I really missed in your proposal.

> Many of U-Boot's options are not just yes/no, so I'm not sure what will
> happen with those. Perhaps they just can't be used with this macro? Part of
> my intent was to allow any option to be used.

In those cases the defines then are a "shortcut" to using a boolean + a
value and we can make it work by introducing this boolean?  I have no
idea of how much work this would be, but it might be worthwhile getting
some real numbers to that.

> So for example you can do this:
>
> struct {
>> const char *str;
>> u_int len;
>> int retry;
>> const char *conf; /* Configuration value */
>> }
>> delaykey [] = {
>> { str: getenv("bootdelaykey"),  retry: 1,
>> conf: autoconf_autoboot_delay_str() },
>> { str: getenv("bootdelaykey2"), retry: 1,
>> conf: autoconf_autoboot_delay_str2() },
>> { str: getenv("bootstopkey"),   retry: 0,
>> conf: autoconf_autoboot_stop_str() },
>> { str: getenv("bootstopkey2"),  retry: 0,
>> conf: autoconf_autoboot_stop_str2() },
>> };
>
>
>
> or this:
>
> /* don't retry auto boot? */
>> if (autoconf_boot_retry_time() &&
>>     !delaykey[i].retry)
>> retry_time = -1;
>
>
> Note that autoconf_boot_retry_time() will return 0 if the CONFIG is not
> defined, or if its value is 0.

I'm having real trouble figuring out what this would do on first sight.
Of course you could call me lazy, but by experience I tend to favour
solutions that do not need geniuses to understand ;)

> It seems to me we should provide the Linux feature in U-Boot as part of the
> Kconfig stuff, and see where it takes us. Combined with a bit more
> rationalisation of things like main.c it might be enough.

Why not reimplement your patch set along those lines?  I still really
would _love_ to see us using the compiler more to check for errors and
reduce the number of "potential source code combination" that we have.

Thanks for all the work so far!
  Detlev

-- 
The  C++  STL, with its dyslexia-inducing syntax blizzard of colons and angle
brackets, guarantees that if you try to declare any reasonable data structure,
your first seven attempts will result in compiler errors of Wagnerian fierce-
ness.                         -- James Mickens
--
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 v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Wolfgang Denk]

Dear Simon,

In message <CAPnjgZ3B4sZi_2h2Sw7oop932CcgOu8HO5E_fKmjcyv4wS+eZA@mail.gmail.com> you wrote:
>
...
> > By chance I ran about "include/linux/kconfig.h" in the Linux kernel
> > tree, which provides (among other things) the IS_ENABLED() macro that
> > implements essentially the very same feature.  Using this, the same
> > code would be written as:
> >
> >         if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
> >                 setenv("ver", version_string);  /* set version variable */
...

> Fair enough, sounds reasonable. The relevant kernel code is quite unusual...

Indeed, this code is really a bit creepy...

> Many of U-Boot's options are not just yes/no, so I'm not sure what will
> happen with those. Perhaps they just can't be used with this macro? Part of
> my intent was to allow any option to be used.

I see.  Um... But then, I think the usage of the macro is somewhat
dangerous anyway.

> So for example you can do this:
> 
> struct {
> > const char *str;
> > u_int len;
> > int retry;
> > const char *conf; /* Configuration value */
> > }
> > delaykey [] = {
> > { str: getenv("bootdelaykey"),  retry: 1,
> > conf: autoconf_autoboot_delay_str() },
> > { str: getenv("bootdelaykey2"), retry: 1,
> > conf: autoconf_autoboot_delay_str2() },
> > { str: getenv("bootstopkey"),   retry: 0,
> > conf: autoconf_autoboot_stop_str() },
> > { str: getenv("bootstopkey2"),  retry: 0,
> > conf: autoconf_autoboot_stop_str2() },
> > };

Well, this is not exactly easy to read either.

> or this:
> 
> /* don't retry auto boot? */
> > if (autoconf_boot_retry_time() &&
> >     !delaykey[i].retry)
> > retry_time = -1;
> 
> Note that autoconf_boot_retry_time() will return 0 if the CONFIG is not
> defined, or if its value is 0.

I see what yo mean, but I'd rather clean up the code to avoid such
ambiguities.

> It seems to me we should provide the Linux feature in U-Boot as part of the
> Kconfig stuff, and see where it takes us. Combined with a bit more
> rationalisation of things like main.c it might be enough.

Sounds like a plan - but we don't have to wait; we can add the header
file any time we ike, and start converting pieces of code that seem to
deserve such treatment.  And, probably even more interesting, we can
request it to be used for any new code being added.

> I like the simplicity and power of the autoconf feature, but I have similar
> reservations to others.

I understand you.  Eventuyally ther eis no easy solution that
satisfies all.

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
Dealing with failure is easy: work hard to improve. Success  is  also
easy  to  handle:  you've  solved  the  wrong  problem.  Work hard to
improve.

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Wolfgang,

On Wed, Nov 6, 2013 at 1:24 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
>> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>> different boards compile different versions of the source code, meaning
>> that we must build all boards to check for failures. It is easy to
misspell
>> an #ifdef and there is not as much checking of this by the compiler.
Multiple
>> dependent #ifdefs are harder to do than with if..then..else. Variable
>> declarations must be #idefed as well as the code that uses them, often
much
>> later in the file/function. #ifdef indents don't match code indents and
>> have their own separate indent feature. Overall, excessive use of #idef
>> hurts readability and makes the code harder to modify and refactor. For
>> people coming newly into the code base, #ifdefs can be a big barrier.
>>
>> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>> attempt to turn the tide, this series includes a patch which provides a
way
>> to make CONFIG macros available to C code without using the preprocessor.
>> This makes it possible to use standard C conditional features such as
>> if/then instead of #ifdef. A README update exhorts compliance.
>
> As mentioned before, I'm really interested in seeing something like
> this going into mainline, but I have some doubts about the actual
> implementation.
>
> To summarize:  Your current proposal was to convert code snippets
> like this:
>
>         #ifdef CONFIG_VERSION_VARIABLE
>                 setenv("ver", version_string);  /* set version variable */
>         #endif
>
> into
>
>         if (autoconf_version_variable())
>                 setenv("ver", version_string);  /* set version variable */
>
> By chance I ran about "include/linux/kconfig.h" in the Linux kernel
> tree, which provides (among other things) the IS_ENABLED() macro that
> implements essentially the very same feature.  Using this, the same
> code would be written as:
>
>         if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
>                 setenv("ver", version_string);  /* set version variable */
>
> I agree that this does not solve some of the isses that have been
> raised about this change (indentation level increses - which may in
> turn require reformatting of bigger parts of the code; code becomes
> less readable), but on the other hand it avoids the need for a new
> autoconf header file, and it should be possible to introduce this
> easily step by step.
>
> And I really like the idea of re-using existing code that is already
> known to Linux hackers, especially as we we are currently having our
> eyes on the Kconfig stuff anyway.
>
>
> What do you think?

Fair enough, sounds reasonable. The relevant kernel code is quite unusual...

/*
>  * Helper macros to use CONFIG_ options in C/CPP expressions. Note that
>  * these only work with boolean and tristate options.
>  */
> /*
>  * Getting something that works in C and CPP for an arg that may or may
>  * not be defined is tricky.  Here, if we have "#define CONFIG_BOOGER 1"
>  * we match on the placeholder define, insert the "0," for arg1 and
> generate
>  * the triplet (0, 1, 0).  Then the last step cherry picks the 2nd arg (a
> one).
>  * When CONFIG_BOOGER is not defined, we generate a (... 1, 0) pair, and
> when
>  * the last step cherry picks the 2nd arg, we get a zero.
>  */
> #define __ARG_PLACEHOLDER_1 0,
> #define config_enabled(cfg) _config_enabled(cfg)
> #define _config_enabled(value) __config_enabled(__ARG_PLACEHOLDER_##value)
> #define __config_enabled(arg1_or_junk) ___config_enabled(arg1_or_junk 1, 0)
> #define ___config_enabled(__ignored, val, ...) val
> /*
>  * IS_ENABLED(CONFIG_FOO) evaluates to 1 if CONFIG_FOO is set to 'y' or
> 'm',
>  * 0 otherwise.
>  *
>  */
> #define IS_ENABLED(option) \
> (config_enabled(option) || config_enabled(option##_MODULE))



Many of U-Boot's options are not just yes/no, so I'm not sure what will
happen with those. Perhaps they just can't be used with this macro? Part of
my intent was to allow any option to be used.


So for example you can do this:

struct {
> const char *str;
> u_int len;
> int retry;
> const char *conf; /* Configuration value */
> }
> delaykey [] = {
> { str: getenv("bootdelaykey"),  retry: 1,
> conf: autoconf_autoboot_delay_str() },
> { str: getenv("bootdelaykey2"), retry: 1,
> conf: autoconf_autoboot_delay_str2() },
> { str: getenv("bootstopkey"),   retry: 0,
> conf: autoconf_autoboot_stop_str() },
> { str: getenv("bootstopkey2"),  retry: 0,
> conf: autoconf_autoboot_stop_str2() },
> };



or this:

/* don't retry auto boot? */
> if (autoconf_boot_retry_time() &&
>     !delaykey[i].retry)
> retry_time = -1;


Note that autoconf_boot_retry_time() will return 0 if the CONFIG is not
defined, or if its value is 0.

It seems to me we should provide the Linux feature in U-Boot as part of the
Kconfig stuff, and see where it takes us. Combined with a bit more
rationalisation of things like main.c it might be enough.

I like the simplicity and power of the autoconf feature, but I have similar
reservations to others.

>
> 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
> PROGRAM - n.  A magic spell cast over a computer  allowing it to turn
> one's input into error messages.
> v. tr. - To engage in a pastime similar to banging one's head against
> a wall, but with fewer opportunities for reward.

Regards,
Simon

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Albert ARIBAUD]

Hi Wolfgang,

On Wed, 06 Nov 2013 08:24:44 +0100, Wolfgang Denk <wd@denx.de> wrote:

> Dear Simon Glass,
> 
> In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
> > Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> > different boards compile different versions of the source code, meaning
> > that we must build all boards to check for failures. It is easy to misspell
> > an #ifdef and there is not as much checking of this by the compiler. Multiple
> > dependent #ifdefs are harder to do than with if..then..else. Variable
> > declarations must be #idefed as well as the code that uses them, often much
> > later in the file/function. #ifdef indents don't match code indents and
> > have their own separate indent feature. Overall, excessive use of #idef
> > hurts readability and makes the code harder to modify and refactor. For
> > people coming newly into the code base, #ifdefs can be a big barrier.
> > 
> > The use of #ifdef in U-Boot has possibly got a little out of hand. In an
> > attempt to turn the tide, this series includes a patch which provides a way
> > to make CONFIG macros available to C code without using the preprocessor.
> > This makes it possible to use standard C conditional features such as
> > if/then instead of #ifdef. A README update exhorts compliance.
> 
> As mentioned before, I'm really interested in seeing something like
> this going into mainline, but I have some doubts about the actual
> implementation.
> 
> To summarize:  Your current proposal was to convert code snippets
> like this:
> 
> 	#ifdef CONFIG_VERSION_VARIABLE
> 		setenv("ver", version_string);  /* set version variable */
> 	#endif
> 
> into
> 
> 	if (autoconf_version_variable())
> 		setenv("ver", version_string);  /* set version variable */
> 
> By chance I ran about "include/linux/kconfig.h" in the Linux kernel
> tree, which provides (among other things) the IS_ENABLED() macro that
> implements essentially the very same feature.  Using this, the same
> code would be written as:
> 
> 	if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
> 		setenv("ver", version_string);  /* set version variable */
> 
> I agree that this does not solve some of the isses that have been
> raised about this change (indentation level increses - which may in
> turn require reformatting of bigger parts of the code; code becomes
> less readable), but on the other hand it avoids the need for a new
> autoconf header file, and it should be possible to introduce this
> easily step by step.
> 
> And I really like the idea of re-using existing code that is already
> known to Linux hackers, especially as we we are currently having our
> eyes on the Kconfig stuff anyway.
> 
> What do you think?

Agreed on the whole -- plus, introducing indentation in configuration
option testing will make it easier to spot and understand nested
configuration conditionals.
 
> Best regards,
> 
> Wolfgang Denk
> 


Amicalement,
-- 
Albert.

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Stefan Roese]

Hi Wolfgang,

On 06.11.2013 08:24, Wolfgang Denk wrote:

<snip>

> 	if (autoconf_version_variable())
> 		setenv("ver", version_string);  /* set version variable */
> 
> By chance I ran about "include/linux/kconfig.h" in the Linux kernel
> tree, which provides (among other things) the IS_ENABLED() macro that
> implements essentially the very same feature.  Using this, the same
> code would be written as:
> 
> 	if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
> 		setenv("ver", version_string);  /* set version variable */
> 
> I agree that this does not solve some of the isses that have been
> raised about this change (indentation level increses - which may in
> turn require reformatting of bigger parts of the code; code becomes
> less readable), but on the other hand it avoids the need for a new
> autoconf header file, and it should be possible to introduce this
> easily step by step.
> 
> And I really like the idea of re-using existing code that is already
> known to Linux hackers, especially as we we are currently having our
> eyes on the Kconfig stuff anyway.

I just recently also noticed this IS_ENABLED() feature (in barebox btw)
and thought directly about Simon's patchset regarding this matter. And I
personally would favor IS_ENABLED() to the newly created autoconf_xxx names.

Thanks,
Stefan

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Wolfgang Denk]

Dear Simon Glass,

In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> different boards compile different versions of the source code, meaning
> that we must build all boards to check for failures. It is easy to misspell
> an #ifdef and there is not as much checking of this by the compiler. Multiple
> dependent #ifdefs are harder to do than with if..then..else. Variable
> declarations must be #idefed as well as the code that uses them, often much
> later in the file/function. #ifdef indents don't match code indents and
> have their own separate indent feature. Overall, excessive use of #idef
> hurts readability and makes the code harder to modify and refactor. For
> people coming newly into the code base, #ifdefs can be a big barrier.
> 
> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
> attempt to turn the tide, this series includes a patch which provides a way
> to make CONFIG macros available to C code without using the preprocessor.
> This makes it possible to use standard C conditional features such as
> if/then instead of #ifdef. A README update exhorts compliance.

As mentioned before, I'm really interested in seeing something like
this going into mainline, but I have some doubts about the actual
implementation.

To summarize:  Your current proposal was to convert code snippets
like this:

	#ifdef CONFIG_VERSION_VARIABLE
		setenv("ver", version_string);  /* set version variable */
	#endif

into

	if (autoconf_version_variable())
		setenv("ver", version_string);  /* set version variable */

By chance I ran about "include/linux/kconfig.h" in the Linux kernel
tree, which provides (among other things) the IS_ENABLED() macro that
implements essentially the very same feature.  Using this, the same
code would be written as:

	if (IS_ENABLED(CONFIG_VERSION_VARIABLE))
		setenv("ver", version_string);  /* set version variable */

I agree that this does not solve some of the isses that have been
raised about this change (indentation level increses - which may in
turn require reformatting of bigger parts of the code; code becomes
less readable), but on the other hand it avoids the need for a new
autoconf header file, and it should be possible to introduce this
easily step by step.

And I really like the idea of re-using existing code that is already
known to Linux hackers, especially as we we are currently having our
eyes on the Kconfig stuff anyway.


What do you think?

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
PROGRAM - n.  A magic spell cast over a computer  allowing it to turn
one's input into error messages.
v. tr. - To engage in a pastime similar to banging one's head against
a wall, but with fewer opportunities for reward.

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Tom,

On Mon, Oct 28, 2013 at 9:32 AM, Tom Rini <trini@ti.com> wrote:
> On Mon, Oct 28, 2013 at 03:44:01PM +0100, Wolfgang Denk wrote:
>> Dear Simon,
>>
>> In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
>> > Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>> > different boards compile different versions of the source code, meaning
>> > that we must build all boards to check for failures. It is easy to misspell
>> > an #ifdef and there is not as much checking of this by the compiler. Multiple
>> > dependent #ifdefs are harder to do than with if..then..else. Variable
>> > declarations must be #idefed as well as the code that uses them, often much
>> > later in the file/function. #ifdef indents don't match code indents and
>> > have their own separate indent feature. Overall, excessive use of #idef
>> > hurts readability and makes the code harder to modify and refactor. For
>> > people coming newly into the code base, #ifdefs can be a big barrier.
>>
>> While I agree in general with the goal and the implementation, there
>> is an implementation detail I dislike - this is that the new code
>> is harder to type and to read - I mean, something like
>> CONFIG_SYS_HUSH_PARSER is already clumsy enough, but making this
>> autoconf_sys_hush_parser() appears to be worse.  Also, the connection
>> to a CONFIG_* option is not easily visible.
>>
>>
>> I also had feedback from Detlev (who is unfortunately on a business
>> trip again so he didn't find time [yet] to comment himself); he
>> commented that he really likes the idea, but does not like that we now
>> have to access the well-known contants using a new name.
>>
>>
>> Maybe we can find a shorter / easier to read way to do this - I think
>> it would be really nice if we could see the well-known names.
>
> I guess this is what I get for not being like Linus sometimes[1].  I think
> what this series highlights is that we have a lot of code in
> common/main.c that needs either re-thinking, re-factoring or splitting
> out into difference functions and files.  And when we're turning
> #ifdef CONFIG_FOO
>         ... whatever ...
> #endif
> into
>         if (autoconf_foo()) {
>                 ... whatever ...
>         }
>
> The code starts looking worse in some cases since we're already 3
> indents in.  The problem is we have lots of ifdef code, in some areas of
> the code.  This hides the problem, not fixes the problem.

While I agree with this, the question is more whether using the
compiler instead of the preprocessor helps with reducing the number of
code paths and the number of boards we must build to test things. In
many cases the CONFIG options hide features that we don't want to
enable.

I did a series that improved main.c in various ways including
splitting the code differently - there are a few more things that
could be done, but it will still be a mountain of #ifdefs.

I would quite like to split the command editing stuff into its own
file - in fact main.c could be split into several files:

- command line
- command editing
- parser

But does this help? I wasn't entirely sure.

>
> Looking over the first parts of the series, weak functions for example
> would help in a number of cases, especially if we split things out of
> main.c and into other files too.

I am not a huge fan of weak functions since it isn't clear what
happens when they are called. But again if you have a specific example
it would help me understand your intent here.

Regards,
Simon

>
> --
> Tom
>
> [1]: By which I mean being very "forceful" when saying I don't like an
> idea.

BTW I am not sure about this idea either - let's figure out exactly
what it can help with and whether it is worth it. Perhaps main.c was
not the best choice - but board files have loads of #ifdefs also.

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Tom Rini]

On Mon, Oct 28, 2013 at 03:44:01PM +0100, Wolfgang Denk wrote:
> Dear Simon,
> 
> In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
> > Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> > different boards compile different versions of the source code, meaning
> > that we must build all boards to check for failures. It is easy to misspell
> > an #ifdef and there is not as much checking of this by the compiler. Multiple
> > dependent #ifdefs are harder to do than with if..then..else. Variable
> > declarations must be #idefed as well as the code that uses them, often much
> > later in the file/function. #ifdef indents don't match code indents and
> > have their own separate indent feature. Overall, excessive use of #idef
> > hurts readability and makes the code harder to modify and refactor. For
> > people coming newly into the code base, #ifdefs can be a big barrier.
> 
> While I agree in general with the goal and the implementation, there
> is an implementation detail I dislike - this is that the new code
> is harder to type and to read - I mean, something like
> CONFIG_SYS_HUSH_PARSER is already clumsy enough, but making this
> autoconf_sys_hush_parser() appears to be worse.  Also, the connection
> to a CONFIG_* option is not easily visible.
> 
> 
> I also had feedback from Detlev (who is unfortunately on a business
> trip again so he didn't find time [yet] to comment himself); he
> commented that he really likes the idea, but does not like that we now
> have to access the well-known contants using a new name.
> 
> 
> Maybe we can find a shorter / easier to read way to do this - I think
> it would be really nice if we could see the well-known names.

I guess this is what I get for not being like Linus sometimes[1].  I think
what this series highlights is that we have a lot of code in
common/main.c that needs either re-thinking, re-factoring or splitting
out into difference functions and files.  And when we're turning
#ifdef CONFIG_FOO
	... whatever ...
#endif
into
	if (autoconf_foo()) {
		... whatever ...
	}

The code starts looking worse in some cases since we're already 3
indents in.  The problem is we have lots of ifdef code, in some areas of
the code.  This hides the problem, not fixes the problem.

Looking over the first parts of the series, weak functions for example
would help in a number of cases, especially if we split things out of
main.c and into other files too.

-- 
Tom

[1]: By which I mean being very "forceful" when saying I don't like an
idea.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131028/991e8933/attachment.pgp>

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Wolfgang Denk]

Dear Simon,

In message <1382800457-26608-1-git-send-email-sjg@chromium.org> you wrote:
> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> different boards compile different versions of the source code, meaning
> that we must build all boards to check for failures. It is easy to misspell
> an #ifdef and there is not as much checking of this by the compiler. Multiple
> dependent #ifdefs are harder to do than with if..then..else. Variable
> declarations must be #idefed as well as the code that uses them, often much
> later in the file/function. #ifdef indents don't match code indents and
> have their own separate indent feature. Overall, excessive use of #idef
> hurts readability and makes the code harder to modify and refactor. For
> people coming newly into the code base, #ifdefs can be a big barrier.

While I agree in general with the goal and the implementation, there
is an implementation detail I dislike - this is that the new code
is harder to type and to read - I mean, something like
CONFIG_SYS_HUSH_PARSER is already clumsy enough, but making this
autoconf_sys_hush_parser() appears to be worse.  Also, the connection
to a CONFIG_* option is not easily visible.


I also had feedback from Detlev (who is unfortunately on a business
trip again so he didn't find time [yet] to comment himself); he
commented that he really likes the idea, but does not like that we now
have to access the well-known contants using a new name.


Maybe we can find a shorter / easier to read way to do this - I think
it would be really nice if we could see the well-known names.

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
Where there's no emotion, there's no motive for violence.
	-- Spock, "Dagger of the Mind", stardate 2715.1

[U-Boot] [PATCH v4 0/8] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
different boards compile different versions of the source code, meaning
that we must build all boards to check for failures. It is easy to misspell
an #ifdef and there is not as much checking of this by the compiler. Multiple
dependent #ifdefs are harder to do than with if..then..else. Variable
declarations must be #idefed as well as the code that uses them, often much
later in the file/function. #ifdef indents don't match code indents and
have their own separate indent feature. Overall, excessive use of #idef
hurts readability and makes the code harder to modify and refactor. For
people coming newly into the code base, #ifdefs can be a big barrier.

The use of #ifdef in U-Boot has possibly got a little out of hand. In an
attempt to turn the tide, this series includes a patch which provides a way
to make CONFIG macros available to C code without using the preprocessor.
This makes it possible to use standard C conditional features such as
if/then instead of #ifdef. A README update exhorts compliance.

As an example of how to use this, this series replaces all but two #ifdefs
from the main code body of common/main.c, which is one of the largest users
of #ifdef, even after a recent cleanup:

$ for f in $(find . -name *.c); do echo $(grep -c "ifdef" $f) $f; \
	done |sort -nr |head
82 ./common/board_r.c
57 ./common/board_f.c
57 ./arch/powerpc/cpu/mpc83xx/cpu_init.c
49 ./common/main.c
49 ./arch/powerpc/lib/board.c
45 ./drivers/video/cfb_console.c
40 ./drivers/mtd/cfi_flash.c
38 ./net/tftp.c
37 ./drivers/usb/host/ohci-hcd.c
36 ./drivers/fpga/ivm_core.c

Code size for this series seems to be roughly neutral (below numbers are
average change in byte size for each region:

  blackfin: (for 21/35 boards)  all -13.7  text -13.7
       x86: (for 1/1 boards)  bss +4.0  data +4.0  text -8.0
     avr32: (for 10/10 boards)  all -7.2  data -2.0  text -5.2
      m68k: (for 35/50 boards)  all -30.9  text -30.9
   powerpc: (for 673/675 boards)  all -20.8  bss +0.4  data +0.0  rodata -0.7
	spl/u-boot-spl:all +0.1  spl/u-boot-spl:data +0.1  text -20.5
   sandbox: (for 1/1 boards)  all +16.0  bss +16.0
        sh: (for 16/21 boards)  all -76.2  bss +3.2  rodata -15.5  text -64.0
     nios2: (for 3/3 boards)  all +18.7  bss +1.3  data -1.3  text +18.7
microblaze: (for 1/1 boards)  all -864.0  bss +8.0  rodata -216.0  text -656.0
       arm: (for 286/344 boards)  all -51.7  bss -5.7  data +0.2  rodata -3.9
	spl/u-boot-spl:all +0.2  spl/u-boot-spl:bss +0.2  text -42.4
     nds32: (for 3/3 boards)  all -52.0  text -52.0

Note that a config_drop.h file is added - this defines all the CONFIGs
which are not used in any board config file. Without this, autoconf cannot
define the macros for this CONFIGs.

Compile time for main.c does not seem to be any different in my tests. The
time to perform the 'dep' step (which now creates autoconf.h) increases,
from about 2.8s to about 4.6s. This additional time is used to grep, sed
and sort the contents of all the header file in U-Boot. The time for an
incremental build is not affected.

It would be much more efficient to maintain a list of all available CONFIG
defines, but no such list exists at present.

Buildman output shows no additional failures from mainline (commit 02 is
a bug in buildman and commit 11 will be sent separately):
$ ./tools/buildman/buildman -b us-config7 -s
Summary of 10 commits for 1178 boards (32 threads, 1 job per thread)
01: usb: rename board_usb_init_type to usb_init_type
  blackfin: +   bf561-acvilon cm-bf561 blackstamp br4 bct-brettl2 cm-bf527 dnp5370 bf506f-ezkit ip04 bf527-sdp bf609-ezkit bf537-stamp bf527-ezkit-v2 cm-bf537e tcm-bf518 cm-bf537u bf537-pnav cm-bf533 pr1 bf533-ezkit ibf-dsp561 bf537-srv1 cm-bf548 bf537-minotaur bf538f-ezkit bf548-ezkit bf525-ucr2 blackvme bf527-ezkit tcm-bf537 bf533-stamp bf518f-ezbrd bf527-ad7160-eval bf526-ezbrd bf561-ezkit
      m68k: +   M54455EVB_a66 M5329AFEE M5249EVB idmr M5208EVBE eb_cpu5282 M5475FFE M54451EVB astro_mcf5373l M54418TWR_serial_rmii M54455EVB_intel M5282EVB M54455EVB_i66 M5475GFE M5253DEMO M54455EVB_stm33 M5485BFE M5485DFE TASREG M5329BFEE M52277EVB M5475EFE M5475CFE cobra5272 M5485AFE M53017EVB M5485HFE M5235EVB M5253EVBE M54418TWR_nand_mii M54418TWR_nand_rmii_lowfreq M5475BFE M5475DFE M5275EVB M52277EVB_stmicro eb_cpu5282_internal M54451EVB_stmicro M5271EVB M5485GFE M5485EFE M5485FFE M54418TWR M5235EVB_Flash32 M5373EVB M54418TWR_nand_rmii M54418TWR_serial_mii M5485CFE M54455EVB M5475AFE M5272C3
   powerpc: +   MVBLM7 MVSMR lcd4_lwmon5
     sparc: +   grsim grsim_leon2 gr_cpci_ax2000 gr_xc3s_1500 gr_ep2s60
        sh: +   rsk7269 rsk7264 sh7752evb rsk7203 sh7757lcr
microblaze: +   microblaze-generic
  openrisc: +   openrisc-generic
       arm: +   pm9g45 qong palmtc zipitz2 omap3_zoom1 omap3_overo goflexhome davinci_sonata VCMA9 iconnect km_kirkwood_pci ib62x0 lubbock ethernut5 zynq_dcc vpac270_nor_128 colibri_pxa270 sheevaplug kzm9g am3517_crane zynq tnetv107x_evm xaeniax magnesium palmtreo680 kmsuv31 polaris omap3_sdp3430 imx27lite mgcoge3un vpac270_nor_256 pxa255_idp kmnusa kmcoge5un am3517_evm nhk8815_onenand openrd_client openrd_base nhk8815 km_kirkwood dns325 mcx lp8x4x vpac270_ond_256 smdk2410 h2200 jornada balloon3 omap3_evm omap3_logic dockstar portl2 palmld openrd_ultimate trizepsiv pogo_e02 pm9263 devkit8000
02: Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
       arm: +   am43xx_evm dra7xx_evm
   powerpc: +   T1040QDS
03: Implement autoconf header file
       arm:    am43xx_evm dra7xx_evm
   powerpc:    T1040QDS
04: main: Use autoconf for boot retry feature
05: main: Remove CONFIG #ifdefs from the abortboot() code
06: main: Use autoconf to remove #ifdefs around process_boot_delay()
07: main: Use autoconf for boot_delay code
08: main: Use autoconf for parser selection
09: main: Use autoconf in command line reading
10: main: Use autoconf in main_loop()
11: fix Cover-letter-cc


Changes in v4:
- Rebase on current master
- Split out new patch to remove #ifdefs around process_boot_delay()
- Tidy up code style nits with new checkpatch

Changes in v3:
- Add comment as to why we use [A-Za-z0-9_][A-Za-z0-9_]*
- Fix missing && in if() statement
- Remove the extra config_of_libfdt() condition in main_loop()
- Remove unneeded retry_min variable
- Rename sed scripts to more useful names
- Simplify code for finding out bootdelay from config or environment
- Update config_xxx() to autoconf_xxx() in comments/README/sed
- Update config_xxx_enabled() to autoconf_has_xxx() in comments/README/sed

Changes in v2:
- Add a grep to the sed/sort pipe to speed up processing
- Fix up a few errors and comments in the original RFC
- Split out changes to main.c into separate patches
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()

Simon Glass (8):
  Implement autoconf header file
  main: Use autoconf for boot retry feature
  main: Remove CONFIG #ifdefs from the abortboot() code
  main: Use autoconf to remove #ifdefs around process_boot_delay()
  main: Use autoconf for boot_delay code
  main: Use autoconf for parser selection
  main: Use autoconf in command line reading
  main: Use autoconf in main_loop()

 Makefile                       |  43 ++-
 README                         |  87 +++++-
 common/main.c                  | 629 ++++++++++++++++++-----------------------
 include/command.h              |   2 -
 include/common.h               |   6 +-
 include/config_drop.h          |  17 ++
 include/fdt_support.h          |   4 +-
 include/hush.h                 |   2 -
 include/menu.h                 |   2 -
 tools/scripts/define2value.sed |  37 +++
 tools/scripts/define2zero.sed  |  32 +++
 11 files changed, 492 insertions(+), 369 deletions(-)
 create mode 100644 include/config_drop.h
 create mode 100644 tools/scripts/define2value.sed
 create mode 100644 tools/scripts/define2zero.sed

-- 
1.8.4.1