Patchwork [U-Boot,RFC] Provide a mechanism to avoid using #ifdef everywhere

login
register
mail settings
Submitter Simon Glass
Date Feb. 18, 2013, 5:18 p.m.
Message ID <1361207920-24983-1-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/221424/
State Superseded, archived
Headers show

Comments

Simon Glass - Feb. 18, 2013, 5:18 p.m.
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 patch 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 patch replaces all but two #ifdefs
from the main code body of common/main.c, which has the dubious distinction
of having the most #ifdefs by at least one measure:

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

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

       x86: (3 boards)   text -1.3   data +1.3
   sandbox: (1 boards)   bss +16.0
      m68k: (50 boards)   text -4.8
   powerpc: (634 boards)   text +7.4   data +0.0   bss +2.1
        sh: (21 boards)   text -9.1   bss +2.5
     nios2: (3 boards)   text +24.0
       arm: (287 boards)   spl/u-boot-spl:text -0.3   text -2.3   bss +7.2
     nds32: (3 boards)   text -16.0

Signed-off-by: Simon Glass <sjg@chromium.org>
---
 Makefile                      |  41 ++-
 README                        |  87 ++++-
 common/cmd_fitupd.c           |   1 +
 common/main.c                 | 794 ++++++++++++++++++------------------------
 include/command.h             |   2 -
 include/common.h              |   9 +-
 include/config_drop.h         |  17 +
 include/configs/pm9263.h      |   2 +-
 include/fdt_support.h         |   4 +-
 include/hush.h                |   2 -
 include/menu.h                |   2 -
 include/net.h                 |   2 +
 tools/scripts/define2conf.sed |  36 ++
 tools/scripts/define2list.sed |  31 ++
 14 files changed, 563 insertions(+), 467 deletions(-)
 create mode 100644 include/config_drop.h
 create mode 100644 tools/scripts/define2conf.sed
 create mode 100644 tools/scripts/define2list.sed
Wolfgang Denk - Feb. 18, 2013, 7:23 p.m.
Dear Simon,

In message <1361207920-24983-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
...
> +# Create a C header file where every '#define CONFIG_XXX value' becomes
> +# '#define config_xxx() value', or '#define config_xxx() 0' where the CONFIG
> +# is not used by this board configuration. This allows C code to do things
> +# like 'if (config_xxx())' and have the compiler remove the dead code,
> +# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases
> +# if the config_...() 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 config_xxx_enabled(), setting the
> +# value to 0 if the option is disabled, 1 if enabled. This last feature will
> +# hopefully be deprecated soon.

I think config_* is not a good name to use here - it has never been a
reserved prefix so far, so it is IMO a bad idea to turn it into one
now .  We already have quite a number such variable names in the code
all over the place (not sure I caught them all):

config_2			    config_io_ctrl
config_8260_ioports		    config_kbc_gpio
config_8560_ioports		    config_list
config_address			    config_list_t
config_backside_crossbar_mux	    config_mpc8xx_ioports
config_base			    config_nfc_clk
config_bit			    config_node
config_buf			    config_on_ebc_cs4_is_small_flash
config_byte			    config_pci_bridge
config_change			    config_periph_clk
config_clock			    config_pin
config_cmd_ctrl			    config_pll_clk
config_core_clk			    config_qe_ioports
config_data			    config_reg
config_data_eye_leveling_samples    config_reg_helper
config_ddr			    config_s
config_ddr_clk			    config_sdram
config_ddr_data			    config_select_P
config_ddr_phy			    config_selection
config_desc			    config_selection_t
config_device			    config_status
config_fifo			    config_table
config_file_size		    config_val
config_frontside_crossbar_vsc3316   config_val_P
config_genmii_advert		    config_validity
config_hub_port			    config_validity_t
config_id			    config_vtp
config_instance


> +The compiler will see that config_xxx() evalutes to a constant and will
> +eliminate the dead code. The resulting code (and code size) is the same.

Did you measure the impact on compile time?

> +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 '_enabled' suffix is provided, where
> +the value is always either 0 or 1:
> +
> +	// Will work even if boaard config has '#define CONFIG_BOOTDELAY 0'
> +	if (config_bootdelay_enabled())
> +		do_something;
> +
> +(Probably such config options should be deprecated and then we can remove
> +this feature)

These are perfectly valid cases, I think - there are quite a number of
these, including defines for console index, PHY address, etc. etc.

> --- a/common/cmd_fitupd.c
> +++ b/common/cmd_fitupd.c
> @@ -8,6 +8,7 @@
>  
>  #include <common.h>
>  #include <command.h>
> +#include <net.h>
>  
>  #if !defined(CONFIG_UPDATE_TFTP)
>  #error "CONFIG_UPDATE_TFTP required"

This seems to be an unrelated change?


> diff --git a/common/main.c b/common/main.c
> index e2d2e09..cd42b67 100644
> --- a/common/main.c
> +++ b/common/main.c
...
> -#include <malloc.h>		/* for free() prototype */
...
> +#include <malloc.h>

Why dropping the comment?

> -#undef DEBUG_PARSER
> +#define DEBUG_PARSER	0	/* set to 1 to debug */
> +
> +
> +#define debug_parser(fmt, args...)		\
> +	debug_cond(DEBUG_PARSER, fmt, ##args)
> +
> +#ifndef DEBUG_BOOTKEYS
> +#define DEBUG_BOOTKEYS 0
> +#endif
> +#define debug_bootkeys(fmt, args...)		\
> +	debug_cond(DEBUG_BOOTKEYS, fmt, ##args)

This is also a different type of changes.  Please keep such separate.

> -#ifndef CONFIG_ZERO_BOOTDELAY_CHECK
> -	if (bootdelay == 0)
> +	if (config_zero_bootdelay_check() && bootdelay == 0)

This appears to be plain wrong.  That was a "#ifndef" before...

> +	if (config_autoboot_delay_str() && delaykey[0].str == NULL)
> +		delaykey[0].str = config_autoboot_delay_str();

Hm.... constructs like these make me think about side effects.  As is,
your implementation does not pretect against any.  This may be
dangerous - you are evaluating the macro multiple times now, while
before it was only a defined() test, folowed by a single evaluation.

And to be honest - I find the old code easier to read.

> -		for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) {
> +		for (i = 0; i < ARRAY_SIZE(delaykey); i++) {

Once more, a totally unrelated code change.  Please split into
independent commits.


> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK
>  	/*
>  	 * Check if key already pressed
>  	 * Don't check if bootdelay < 0
>  	 */
> -	if (bootdelay >= 0) {
> +	if (config_zero_bootdelay_check() && bootdelay >= 0) {
>  		if (tstc()) {	/* we got a key press	*/
>  			(void) getc();  /* consume input	*/
>  			puts ("\b\b\b 0");
>  			abort = 1;	/* don't auto boot	*/
>  		}
>  	}
> -#endif

I think code like this needs more careful editing.

With the #ifdef, it was clear that the comment only applies if
CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly becomes
valid _always_, which is pretty much misleading to someone trying to
understand this code.   I think it would be necessary to rephrase the
commend, and move it partially into the if() block.


> -void main_loop (void)
> +static void process_boot_delay(void)
>  {
> -#ifndef CONFIG_SYS_HUSH_PARSER
> -	static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };
> -	int len;
> -	int rc = 1;
> -	int flag;
> -#endif
> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
> -		defined(CONFIG_OF_CONTROL)
> -	char *env;
> -#endif
> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
> -	char *s;
> -	int bootdelay;
> -#endif
> -#ifdef CONFIG_PREBOOT
> -	char *p;
> -#endif
> -#ifdef CONFIG_BOOTCOUNT_LIMIT
>  	unsigned long bootcount = 0;
>  	unsigned long bootlimit = 0;
> -	char *bcs;
> -	char bcs_set[16];
> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
> -
> -	bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
> -
> -#ifdef CONFIG_BOOTCOUNT_LIMIT
> -	bootcount = bootcount_load();
> -	bootcount++;
> -	bootcount_store (bootcount);
> -	sprintf (bcs_set, "%lu", bootcount);
> -	setenv ("bootcount", bcs_set);
> -	bcs = getenv ("bootlimit");
> -	bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0;
> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
> -
> -#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 */
> -	}
> -#endif  /* CONFIG_MODEM_SUPPORT */
> -
> -#ifdef CONFIG_VERSION_VARIABLE
> -	{
> -		setenv ("ver", version_string);  /* set version variable */
> -	}
> -#endif /* CONFIG_VERSION_VARIABLE */
> -
> -#ifdef CONFIG_SYS_HUSH_PARSER
> -	u_boot_hush_start ();
> -#endif
> -
> -#if defined(CONFIG_HUSH_INIT_VAR)
> -	hush_init_var ();
> -#endif
> -
> -#ifdef CONFIG_PREBOOT
> -	if ((p = getenv ("preboot")) != NULL) {
> -# ifdef CONFIG_AUTOBOOT_KEYED
> -		int prev = disable_ctrlc(1);	/* disable Control C checking */
> -# endif
> -
> -		run_command_list(p, -1, 0);
> +	const char *s;
> +	int bootdelay;
>  
> -# ifdef CONFIG_AUTOBOOT_KEYED
> -		disable_ctrlc(prev);	/* restore Control C checking */
> -# endif

I'm afraid here the patch becomes unreadable, at least to me - I give
up.


I find the idea interesting and definitely worth to carry on, but it
appears we're still a pretty long way off from usable code.

Best regards,

Wolfgang Denk
Tom Rini - Feb. 18, 2013, 9:36 p.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/18/2013 02:23 PM, Wolfgang Denk wrote:
> Dear Simon,
> 
> In message <1361207920-24983-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
> ...
>> +# Create a C header file where every '#define CONFIG_XXX value'
>> becomes +# '#define config_xxx() value', or '#define config_xxx()
>> 0' where the CONFIG +# is not used by this board configuration.
>> This allows C code to do things +# like 'if (config_xxx())' and
>> have the compiler remove the dead code, +# instead of using
>> '#ifdef CONFIG_XXX...#endif'. Note that in most cases +# if the
>> config_...() 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
>> config_xxx_enabled(), setting the +# value to 0 if the option is
>> disabled, 1 if enabled. This last feature will +# hopefully be
>> deprecated soon.
> 
> I think config_* is not a good name to use here - it has never been
> a reserved prefix so far, so it is IMO a bad idea to turn it into
> one now .  We already have quite a number such variable names in
> the code all over the place (not sure I caught them all):

Indeed.  It's not a good choice to suddenly reserve now either.
build_has_xxx() ?  I'm not sure.

[snip]
>> +The compiler will see that config_xxx() evalutes to a constant
>> and will +eliminate the dead code. The resulting code (and code
>> size) is the same.
> 
> Did you measure the impact on compile time?

Probably won't have a good handle on this without converting a whole
build target's needs (just about).

[snip]
>> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK /* * Check if key
>> already pressed * Don't check if bootdelay < 0 */ -	if (bootdelay
>> >= 0) { +	if (config_zero_bootdelay_check() && bootdelay >= 0) { 
>> if (tstc()) {	/* we got a key press	*/ (void) getc();  /* consume
>> input	*/ puts ("\b\b\b 0"); abort = 1;	/* don't auto boot	*/ } } 
>> -#endif
> 
> I think code like this needs more careful editing.
> 
> With the #ifdef, it was clear that the comment only applies if 
> CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly
> becomes valid _always_, which is pretty much misleading to someone
> trying to understand this code.   I think it would be necessary to
> rephrase the commend, and move it partially into the if() block.

Exactly.  This type of change will require a lot of re-commenting to
make it clear what's going on now, and after the change even more-so.

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRIp7WAAoJENk4IS6UOR1W1S0QAKzMt8KkcVdRTFElSjlze35P
PxFkO/W8YchPkzwMdhpU4UxHNYoFziLk5pLfj8hhh9uyQ7Lp0I411PZtJ+mkt3I5
RQf8xPHF9PDN2w0ZsxYKd0JE9LgFB/b9EmOOpzxy7Bge3aEGrfnhqgjNgnPGgVgO
eCcLGZLrGYlbcL9SOJNxBdFjgCxJvRfrNtsaLOJc5SveeqMNGISp6xc5WDWnmf1f
JAHNg7d9ik5d782AC76jbNUVOIp+85N0dMjhCdLR4YZBdNTC5grAW6x7gEGj+vYZ
xUE2/Y20FG1Ie3vQjbbW1gUhYtxCxjFLl+UkUOn5bf6F0eDgyUvaSt17nrO3GSbQ
hgunWaw9fZoVKMhb1WNnRHmLU5gS9rVlYGGsGibiMs1VPuiYpTLM/kuxfVitxJO0
Ysgkyotgfj2RbnKuBCkMVmvxBzIC3S2bAtxY97TwVrpEh2ZB7r2YwEKak8WhVxyQ
nMyMulgpZoMJLM3fJEcF/kQPIzsKtz1Fl/YQXGZlI2lQpohE03kAPVQDyVeTQpGA
FzGOUwVZY4WbcKrpcCV4tJOEnHRVyDR8ntQx0udMjtChaLE40fAmmUlWmWrnE4yV
SVBM+PS2d7NCXt85QpS0eMc/UdFI0v1i6R24KEHEfqQe1WEdQb1wC7XXYblutZ8z
ySlnbeEMfeDg5i5FHX46
=CF0y
-----END PGP SIGNATURE-----
Simon Glass - Feb. 19, 2013, 5:13 a.m.
Hi Wolfgang,

On Mon, Feb 18, 2013 at 11:23 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon,
>
> In message <1361207920-24983-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
> ...
>> +# Create a C header file where every '#define CONFIG_XXX value' becomes
>> +# '#define config_xxx() value', or '#define config_xxx() 0' where the CONFIG
>> +# is not used by this board configuration. This allows C code to do things
>> +# like 'if (config_xxx())' and have the compiler remove the dead code,
>> +# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases
>> +# if the config_...() 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 config_xxx_enabled(), setting the
>> +# value to 0 if the option is disabled, 1 if enabled. This last feature will
>> +# hopefully be deprecated soon.

Thanks for looking at this so closely - it's just an RFC at this
stage, but I think it has promise.

>
> I think config_* is not a good name to use here - it has never been a
> reserved prefix so far, so it is IMO a bad idea to turn it into one
> now .  We already have quite a number such variable names in the code
> all over the place (not sure I caught them all):
>
> config_2                            config_io_ctrl
> config_8260_ioports                 config_kbc_gpio
> config_8560_ioports                 config_list
> config_address                      config_list_t
> config_backside_crossbar_mux        config_mpc8xx_ioports
> config_base                         config_nfc_clk
> config_bit                          config_node
> config_buf                          config_on_ebc_cs4_is_small_flash
> config_byte                         config_pci_bridge
> config_change                       config_periph_clk
> config_clock                        config_pin
> config_cmd_ctrl                     config_pll_clk
> config_core_clk                     config_qe_ioports
> config_data                         config_reg
> config_data_eye_leveling_samples    config_reg_helper
> config_ddr                          config_s
> config_ddr_clk                      config_sdram
> config_ddr_data                     config_select_P
> config_ddr_phy                      config_selection
> config_desc                         config_selection_t
> config_device                       config_status
> config_fifo                         config_table
> config_file_size                    config_val
> config_frontside_crossbar_vsc3316   config_val_P
> config_genmii_advert                config_validity
> config_hub_port                     config_validity_t
> config_id                           config_vtp
> config_instance
>

These are variables, so won't conflict with the function macros used
by this patch. But maybe we should try for something like cfg_...()?

>
>> +The compiler will see that config_xxx() evalutes to a constant and will
>> +eliminate the dead code. The resulting code (and code size) is the same.
>
> Did you measure the impact on compile time?

I did for the 'sed' step - it was a 2-3 seconds to regenerate the header files.

Full reconfig/recompile goes from about about 30s to 34s.
Incremental build doesn't change noticeably.

I also tried recompiling both the mainline U-Boot's main.c 100 times,
and then this one. I could not see any time difference, which is a
little surprising. Maybe the C compiler's DCE is pretty early in the
the process?

>
>> +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 '_enabled' suffix is provided, where
>> +the value is always either 0 or 1:
>> +
>> +     // Will work even if boaard config has '#define CONFIG_BOOTDELAY 0'
>> +     if (config_bootdelay_enabled())
>> +             do_something;
>> +
>> +(Probably such config options should be deprecated and then we can remove
>> +this feature)
>
> These are perfectly valid cases, I think - there are quite a number of
> these, including defines for console index, PHY address, etc. etc.

Well the issue is not so much the value, but whether the value enables
something. So for example:

config_console_index()

is perfectly fine if it just specifies the console index. But if it
also enables are particular feature (e.g. turning the console on),
then causes problems. I actually think such cases are quite rare, and
probably cause other confusion also.

>
>> --- a/common/cmd_fitupd.c
>> +++ b/common/cmd_fitupd.c
>> @@ -8,6 +8,7 @@
>>
>>  #include <common.h>
>>  #include <command.h>
>> +#include <net.h>
>>
>>  #if !defined(CONFIG_UPDATE_TFTP)
>>  #error "CONFIG_UPDATE_TFTP required"
>
> This seems to be an unrelated change?

Yes - just to make it fail here rather than die with an obscure message later.

>
>
>> diff --git a/common/main.c b/common/main.c
>> index e2d2e09..cd42b67 100644
>> --- a/common/main.c
>> +++ b/common/main.c
> ...
>> -#include <malloc.h>          /* for free() prototype */
> ...
>> +#include <malloc.h>
>
> Why dropping the comment?

Actually main.c includes this file twice - I dropped the one with the
comment. There are far to many changes in main.c for the diff to be
very useful unfortunately. It's just an RFC, but my intent with main.c
was to take the concept as far as I could.

>
>> -#undef DEBUG_PARSER
>> +#define DEBUG_PARSER 0       /* set to 1 to debug */
>> +
>> +
>> +#define debug_parser(fmt, args...)           \
>> +     debug_cond(DEBUG_PARSER, fmt, ##args)
>> +
>> +#ifndef DEBUG_BOOTKEYS
>> +#define DEBUG_BOOTKEYS 0
>> +#endif
>> +#define debug_bootkeys(fmt, args...)         \
>> +     debug_cond(DEBUG_BOOTKEYS, fmt, ##args)
>
> This is also a different type of changes.  Please keep such separate.

Yes

>
>> -#ifndef CONFIG_ZERO_BOOTDELAY_CHECK
>> -     if (bootdelay == 0)
>> +     if (config_zero_bootdelay_check() && bootdelay == 0)
>
> This appears to be plain wrong.  That was a "#ifndef" before...
>
>> +     if (config_autoboot_delay_str() && delaykey[0].str == NULL)
>> +             delaykey[0].str = config_autoboot_delay_str();
>
> Hm.... constructs like these make me think about side effects.  As is,
> your implementation does not pretect against any.  This may be
> dangerous - you are evaluating the macro multiple times now, while
> before it was only a defined() test, folowed by a single evaluation.
>
> And to be honest - I find the old code easier to read.
>
>> -             for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) {
>> +             for (i = 0; i < ARRAY_SIZE(delaykey); i++) {
>
> Once more, a totally unrelated code change.  Please split into
> independent commits.
>

I will sort out these comments when I put together a proper series for this.

>
>> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK
>>       /*
>>        * Check if key already pressed
>>        * Don't check if bootdelay < 0
>>        */
>> -     if (bootdelay >= 0) {
>> +     if (config_zero_bootdelay_check() && bootdelay >= 0) {
>>               if (tstc()) {   /* we got a key press   */
>>                       (void) getc();  /* consume input        */
>>                       puts ("\b\b\b 0");
>>                       abort = 1;      /* don't auto boot      */
>>               }
>>       }
>> -#endif
>
> I think code like this needs more careful editing.
>
> With the #ifdef, it was clear that the comment only applies if
> CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly becomes
> valid _always_, which is pretty much misleading to someone trying to
> understand this code.   I think it would be necessary to rephrase the
> commend, and move it partially into the if() block.

Yes I see.

>
>
>> -void main_loop (void)
>> +static void process_boot_delay(void)
>>  {
>> -#ifndef CONFIG_SYS_HUSH_PARSER
>> -     static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };
>> -     int len;
>> -     int rc = 1;
>> -     int flag;
>> -#endif
>> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
>> -             defined(CONFIG_OF_CONTROL)
>> -     char *env;
>> -#endif
>> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
>> -     char *s;
>> -     int bootdelay;
>> -#endif
>> -#ifdef CONFIG_PREBOOT
>> -     char *p;
>> -#endif
>> -#ifdef CONFIG_BOOTCOUNT_LIMIT
>>       unsigned long bootcount = 0;
>>       unsigned long bootlimit = 0;
>> -     char *bcs;
>> -     char bcs_set[16];
>> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
>> -
>> -     bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
>> -
>> -#ifdef CONFIG_BOOTCOUNT_LIMIT
>> -     bootcount = bootcount_load();
>> -     bootcount++;
>> -     bootcount_store (bootcount);
>> -     sprintf (bcs_set, "%lu", bootcount);
>> -     setenv ("bootcount", bcs_set);
>> -     bcs = getenv ("bootlimit");
>> -     bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0;
>> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
>> -
>> -#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 */
>> -     }
>> -#endif  /* CONFIG_MODEM_SUPPORT */
>> -
>> -#ifdef CONFIG_VERSION_VARIABLE
>> -     {
>> -             setenv ("ver", version_string);  /* set version variable */
>> -     }
>> -#endif /* CONFIG_VERSION_VARIABLE */
>> -
>> -#ifdef CONFIG_SYS_HUSH_PARSER
>> -     u_boot_hush_start ();
>> -#endif
>> -
>> -#if defined(CONFIG_HUSH_INIT_VAR)
>> -     hush_init_var ();
>> -#endif
>> -
>> -#ifdef CONFIG_PREBOOT
>> -     if ((p = getenv ("preboot")) != NULL) {
>> -# ifdef CONFIG_AUTOBOOT_KEYED
>> -             int prev = disable_ctrlc(1);    /* disable Control C checking */
>> -# endif
>> -
>> -             run_command_list(p, -1, 0);
>> +     const char *s;
>> +     int bootdelay;
>>
>> -# ifdef CONFIG_AUTOBOOT_KEYED
>> -             disable_ctrlc(prev);    /* restore Control C checking */
>> -# endif
>
> I'm afraid here the patch becomes unreadable, at least to me - I give
> up.
>

Agreed, it's just too hard to follow. in this form. What is happening
here is that I split the boot delay code into two functions, one for
each of the available options. It really needs a number of smaller
patches to be useful.

But if you have time, please take a look at the sed scripts and the Makefile.

>
> I find the idea interesting and definitely worth to carry on, but it
> appears we're still a pretty long way off from usable code.

OK good.

The background here is that I have been wondering about this for a
while, but have never really come up with a good way (in the absence
of a unified Kconfig) of getting a complete list of CONFIG variables.
There was a mailing list post about this a few weeks ago. But then
David Hendrix complained about main.c which spurred me into action,
and I thought maybe we could just live with a brute force
'find/grep/sed' approach for now, rather than trying to be too clever.
That's what I have done here.

If you look at the image series I did, for example
http://patchwork.ozlabs.org/patch/209601/, you will see that I was
trying to avoid adding back #ifdefs into the cleaned-up code. That was
a point solution, not particularly elegant, and I would much prefer
that we get a built-in solution.

I will come back to this when I have time - will be a week or two.

Regards,
Simon

>
> 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@denx.de
> Too bad that all the people who know how to run the country are  busy
> driving taxicabs and cutting hair.                     - George Burns
Simon Glass - Feb. 19, 2013, 5:18 a.m.
Hi Tom,

On Mon, Feb 18, 2013 at 1:36 PM, Tom Rini <trini@ti.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/18/2013 02:23 PM, Wolfgang Denk wrote:
>> Dear Simon,
>>
>> In message <1361207920-24983-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
>> ...
>>> +# Create a C header file where every '#define CONFIG_XXX value'
>>> becomes +# '#define config_xxx() value', or '#define config_xxx()
>>> 0' where the CONFIG +# is not used by this board configuration.
>>> This allows C code to do things +# like 'if (config_xxx())' and
>>> have the compiler remove the dead code, +# instead of using
>>> '#ifdef CONFIG_XXX...#endif'. Note that in most cases +# if the
>>> config_...() 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
>>> config_xxx_enabled(), setting the +# value to 0 if the option is
>>> disabled, 1 if enabled. This last feature will +# hopefully be
>>> deprecated soon.
>>
>> I think config_* is not a good name to use here - it has never been
>> a reserved prefix so far, so it is IMO a bad idea to turn it into
>> one now .  We already have quite a number such variable names in
>> the code all over the place (not sure I caught them all):
>
> Indeed.  It's not a good choice to suddenly reserve now either.
> build_has_xxx() ?  I'm not sure.

So config_bootdelay(), or cfg_bootdelay() for the value, and
build_has_bootdelay() for the enable?

>
> [snip]
>>> +The compiler will see that config_xxx() evalutes to a constant
>>> and will +eliminate the dead code. The resulting code (and code
>>> size) is the same.
>>
>> Did you measure the impact on compile time?
>
> Probably won't have a good handle on this without converting a whole
> build target's needs (just about).

Possibly, but it seems like the biggest impact is the slower
reconfigure - sed runs for several seconds.

>
> [snip]
>>> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK /* * Check if key
>>> already pressed * Don't check if bootdelay < 0 */ -  if (bootdelay
>>> >= 0) { +    if (config_zero_bootdelay_check() && bootdelay >= 0) {
>>> if (tstc()) {        /* we got a key press   */ (void) getc();  /* consume
>>> input        */ puts ("\b\b\b 0"); abort = 1;        /* don't auto boot      */ } }
>>> -#endif
>>
>> I think code like this needs more careful editing.
>>
>> With the #ifdef, it was clear that the comment only applies if
>> CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly
>> becomes valid _always_, which is pretty much misleading to someone
>> trying to understand this code.   I think it would be necessary to
>> rephrase the commend, and move it partially into the if() block.
>
> Exactly.  This type of change will require a lot of re-commenting to
> make it clear what's going on now, and after the change even more-so.

I suspect cleaning up main.c would involve a number of patches in the end.

BTW this patch also is interesting from the point of view of the
generic board series. At the moment I am using a list of function
pointers, which is quite nice, but does defeat the compiler
optimisations, so adds about 1KB of code size. Also it's hard to
imagine removing the #ifdefs from a list of function pointers using
this mechanism.

Regards,
Simon

>
> - --
> Tom
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
>
> iQIcBAEBAgAGBQJRIp7WAAoJENk4IS6UOR1W1S0QAKzMt8KkcVdRTFElSjlze35P
> PxFkO/W8YchPkzwMdhpU4UxHNYoFziLk5pLfj8hhh9uyQ7Lp0I411PZtJ+mkt3I5
> RQf8xPHF9PDN2w0ZsxYKd0JE9LgFB/b9EmOOpzxy7Bge3aEGrfnhqgjNgnPGgVgO
> eCcLGZLrGYlbcL9SOJNxBdFjgCxJvRfrNtsaLOJc5SveeqMNGISp6xc5WDWnmf1f
> JAHNg7d9ik5d782AC76jbNUVOIp+85N0dMjhCdLR4YZBdNTC5grAW6x7gEGj+vYZ
> xUE2/Y20FG1Ie3vQjbbW1gUhYtxCxjFLl+UkUOn5bf6F0eDgyUvaSt17nrO3GSbQ
> hgunWaw9fZoVKMhb1WNnRHmLU5gS9rVlYGGsGibiMs1VPuiYpTLM/kuxfVitxJO0
> Ysgkyotgfj2RbnKuBCkMVmvxBzIC3S2bAtxY97TwVrpEh2ZB7r2YwEKak8WhVxyQ
> nMyMulgpZoMJLM3fJEcF/kQPIzsKtz1Fl/YQXGZlI2lQpohE03kAPVQDyVeTQpGA
> FzGOUwVZY4WbcKrpcCV4tJOEnHRVyDR8ntQx0udMjtChaLE40fAmmUlWmWrnE4yV
> SVBM+PS2d7NCXt85QpS0eMc/UdFI0v1i6R24KEHEfqQe1WEdQb1wC7XXYblutZ8z
> ySlnbeEMfeDg5i5FHX46
> =CF0y
> -----END PGP SIGNATURE-----
Wolfgang Denk - Feb. 19, 2013, 9:19 a.m.
Dear Simon,

In message <CAPnjgZ38HPmOf07CMYssa1Q167sRjzDpbBUbCVMaoGdRoWESog@mail.gmail.com> you wrote:
> 
> Thanks for looking at this so closely - it's just an RFC at this
> stage, but I think it has promise.

Agreed.

> > I think config_* is not a good name to use here - it has never been a
> > reserved prefix so far, so it is IMO a bad idea to turn it into one
> > now .  We already have quite a number such variable names in the code
> > all over the place (not sure I caught them all):
...
> These are variables, so won't conflict with the function macros used
> by this patch. But maybe we should try for something like cfg_...()?

You are wrong.  This includes a number of functions, and macros, too,
for example:

	void config_sdram(const struct emif_regs *regs);
	void config_ddr_phy(const struct emif_regs *regs);
	void config_cmd_ctrl(const struct cmd_control *cmd);
	void config_ddr_data(int data_macrono, ...)
	void config_io_ctrl(unsigned long val);
	void config_ddr(unsigned int pll, unsigned int ioctrl,
	void config_data_eye_leveling_samples(u32 emif_base);
	static int config_pll_clk(enum pll_clocks index, ...)
	static int config_core_clk(u32 ref, u32 freq)
	# define config_fifo(dir, idx, addr)

And cfg_ is not much better:

	#define cfg_read(val, addr, type, op)
	#define cfg_write(val, addr, type, op)
	u16 cfg_type = 0;
	unsigned cfg_val;
	u32 *cfg_reg;
	uint    cfg_addr;
	uint    cfg_data;
	...

> I did for the 'sed' step - it was a 2-3 seconds to regenerate the header files.
> 
> Full reconfig/recompile goes from about about 30s to 34s.
> Incremental build doesn't change noticeably.

I'm mostly concerned about build time for the autobuilder, which does
full config / build cycles for all boards.  Here more than 10%
increase hurt...

> I also tried recompiling both the mainline U-Boot's main.c 100 times,
> and then this one. I could not see any time difference, which is a
> little surprising. Maybe the C compiler's DCE is pretty early in the
> the process?

This is a surprising result, indeed.

> >> +     if (config_autoboot_delay_str() && delaykey[0].str == NULL)
> >> +             delaykey[0].str = config_autoboot_delay_str();
> >
> > Hm.... constructs like these make me think about side effects.  As is,
> > your implementation does not pretect against any.  This may be
> > dangerous - you are evaluating the macro multiple times now, while
> > before it was only a defined() test, folowed by a single evaluation.

You did not comment on this remark.  I think it is something to keep
in mind!

> > And to be honest - I find the old code easier to read.

:-)

> But if you have time, please take a look at the sed scripts and the Makefile.

Sorry, but I can't find the audacity to bend my mind around these
currently ;-)

But you might have a look at "tools/scripts/define2mk.sed" and either
use this as is, or base your code on this.  I would find it good to
use the same code for the same (or so very similar) purposes.

> The background here is that I have been wondering about this for a
> while, but have never really come up with a good way (in the absence
> of a unified Kconfig) of getting a complete list of CONFIG variables.

Does not the already existing "include/autoconf.mk" contain this
information?  In any case, please check "tools/scripts/define2mk.sed"

> There was a mailing list post about this a few weeks ago. But then
> David Hendrix complained about main.c which spurred me into action,
> and I thought maybe we could just live with a brute force
> 'find/grep/sed' approach for now, rather than trying to be too clever.
> That's what I have done here.

Understood, and appreciated.

> I will come back to this when I have time - will be a week or two.

Thanks.

Best regards,

Wolfgang Denk
Harvey Chapman - Feb. 19, 2013, 2:17 p.m.
I'm not sure if this has been discussed, but I've found as a new u-boot user there are often features that require enabling other CONFIG macros that I think should just be auto-enabled as dependencies. Please keep this in mind for any future designs. For example, when enabling CONFIG_CMD_UBI and CONFIG_CMD_UBIFS, my builds failed with linker errors because I didn't know that I needed to also define CONFIG_RBTREE and CONFIG_LZO.

Thanks,
Harvey
Simon Glass - Feb. 19, 2013, 4:48 p.m.
Hi Wolfgang,

On Tue, Feb 19, 2013 at 1:19 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon,
>
> In message <CAPnjgZ38HPmOf07CMYssa1Q167sRjzDpbBUbCVMaoGdRoWESog@mail.gmail.com> you wrote:
>>
>> Thanks for looking at this so closely - it's just an RFC at this
>> stage, but I think it has promise.
>
> Agreed.
>
>> > I think config_* is not a good name to use here - it has never been a
>> > reserved prefix so far, so it is IMO a bad idea to turn it into one
>> > now .  We already have quite a number such variable names in the code
>> > all over the place (not sure I caught them all):
> ...
>> These are variables, so won't conflict with the function macros used
>> by this patch. But maybe we should try for something like cfg_...()?
>
> You are wrong.  This includes a number of functions, and macros, too,
> for example:
>
>         void config_sdram(const struct emif_regs *regs);
>         void config_ddr_phy(const struct emif_regs *regs);
>         void config_cmd_ctrl(const struct cmd_control *cmd);
>         void config_ddr_data(int data_macrono, ...)
>         void config_io_ctrl(unsigned long val);
>         void config_ddr(unsigned int pll, unsigned int ioctrl,
>         void config_data_eye_leveling_samples(u32 emif_base);
>         static int config_pll_clk(enum pll_clocks index, ...)
>         static int config_core_clk(u32 ref, u32 freq)
>         # define config_fifo(dir, idx, addr)

I will see if I can think of some other sane and obvious name that is
little used in the source. Ideas welcome.

But this isn't a very large number. It seems to be that we could
easily adjust these symbols if we wanted to (and bear in mind that
there are currently no conflicts, it is just confusing) - here are the
files:

for str in $(find . -name *.c -or -name *.h |xargs sed -n -f /tmp/s
|sort |uniq); do echo $str; find . -name *.c -or -name *.h |xargs grep
"[^a-zA-Z0-9_<]$str"; done
config_aneg(
./drivers/qe/uec.c:			err = curphy->config_aneg(uec->mii_info);
config_buf(
./drivers/usb/gadget/ether.c:			value = config_buf(gadget, req->buf,
./drivers/usb/gadget/composite.c:static int config_buf(struct
usb_configuration *config,
./drivers/usb/gadget/composite.c:			return config_buf(c, speed,
cdev->req->buf, type);
config_clock(
./arch/arm/cpu/armv7/tegra20/usb.c:static void config_clock(const u32 timing[])
./arch/arm/cpu/armv7/tegra20/usb.c:	config_clock(usb_pll[freq]);
config_ddr(
./board/phytec/pcm051/board.c:	config_ddr(DDR_CLK_MHZ,
MT41J256M8HX15E_IOCTRL_VALUE, &ddr3_data,
./board/ti/am335x/board.c:		config_ddr(303,
MT41J128MJT125_IOCTRL_VALUE, &ddr3_data,
./board/ti/am335x/board.c:		config_ddr(303,
MT41J512M8RH125_IOCTRL_VALUE, &ddr3_evm_data,
./board/ti/am335x/board.c:		config_ddr(266,
MT47H128M16RT25E_IOCTRL_VALUE, &ddr2_data,
./arch/arm/include/asm/arch-am33xx/ddr_defs.h:void config_ddr(unsigned
int pll, unsigned int ioctrl,
./arch/arm/cpu/armv7/am33xx/emif4.c:void config_ddr(unsigned int pll,
unsigned int ioctrl,
config_desc(
./drivers/usb/gadget/composite.c:static int config_desc(struct
usb_composite_dev *cdev, unsigned w_value)
./drivers/usb/gadget/composite.c:			value = config_desc(cdev, w_value);
config_device(
./drivers/pci/pci.c:			cfg->config_device(hose, dev, cfg);
config_fifo(
./drivers/usb/musb/musb_core.c:# define config_fifo(dir, idx, addr)
./drivers/usb/musb/musb_core.c:# define config_fifo(dir, idx, addr) \
./drivers/usb/musb/musb_core.c:			config_fifo(tx, idx, fifoaddr);
./drivers/usb/musb/musb_core.c:			config_fifo(rx, idx, fifoaddr);
config_pin(
./drivers/gpio/db8500_gpio.c:static void config_pin(unsigned long cfg)
./drivers/gpio/db8500_gpio.c: * Configures several pins using
config_pin(). Refer to that function for
./drivers/gpio/db8500_gpio.c:		config_pin(cfgs[i]);
config_sdram(
./arch/arm/include/asm/arch-am33xx/ddr_defs.h:void config_sdram(const
struct emif_regs *regs);
./arch/arm/cpu/armv7/am33xx/emif4.c:	config_sdram(regs);
./arch/arm/cpu/armv7/am33xx/ddr.c:void config_sdram(const struct
emif_regs *regs)
config_vtp(
./arch/arm/cpu/armv7/am33xx/emif4.c:static void config_vtp(void)
./arch/arm/cpu/armv7/am33xx/emif4.c:	config_vtp();

>
> And cfg_ is not much better:
>
>         #define cfg_read(val, addr, type, op)
>         #define cfg_write(val, addr, type, op)
>         u16 cfg_type = 0;
>         unsigned cfg_val;
>         u32 *cfg_reg;
>         uint    cfg_addr;
>         uint    cfg_data;
>         ...

./drivers/video/exynos_fb.c:		vid->cfg_gpio();
cfg_ldo(
./drivers/video/exynos_fb.c:		vid->cfg_ldo();
cfg_oc(
./board/netta/pcmcia.c:static void cfg_oc(void)
./board/netta/pcmcia.c:	cfg_oc();
cfg_read(
./drivers/pci/pci_indirect.c:#define cfg_read(val, addr, type,
op)	*val = op((type)(addr))
./arch/x86/lib/pci_type1.c:#define cfg_read(val, addr, op)		(*val =
op((int)(addr)))
./arch/powerpc/cpu/mpc83xx/pcie.c:#define cfg_read(val, addr, type, op) \
./arch/powerpc/cpu/mpc8220/pci.c:#define cfg_read(val, addr, type,
op)		*val = op((type)(addr));
./arch/m68k/cpu/mcf547x_8x/pci.c:#define cfg_read(val, addr, type,
op)		*val = op((type)(addr));
./arch/m68k/cpu/mcf5445x/pci.c:#define cfg_read(val, addr, type,
op)		*val = op((type)(addr));
cfg_shdn(
./board/netta/pcmcia.c:static void cfg_shdn(void)
./board/netta/pcmcia.c:	cfg_shdn();
cfg_vccd(
./board/netta/pcmcia.c:static void cfg_vccd(int no)
./board/netta/pcmcia.c:	cfg_vccd(0); cfg_vccd(1);	/* 3V and 5V off */
cfg_vppd(
./board/netta/pcmcia.c:static void cfg_vppd(int no)
./board/netta/pcmcia.c:	cfg_vppd(0); cfg_vppd(1);	/* VPPD0,VPPD1 VAVPP
=> Hi-Z */
cfg_write(
./drivers/pci/pci_indirect.c:#define cfg_write(val, addr, type,
op)	op((type *)(addr), (val))
./arch/x86/lib/pci_type1.c:#define cfg_write(val, addr, op)	op((val),
(int)(addr))
./arch/powerpc/cpu/mpc83xx/pcie.c:#define cfg_write(val, addr, type, op) \
./arch/powerpc/cpu/mpc8220/pci.c:#define cfg_write(val, addr, type,
op)		op((type *)(addr), (val));
./arch/m68k/cpu/mcf547x_8x/pci.c:#define cfg_write(val, addr, type,
op)		op((type *)(addr), (val));
./arch/m68k/cpu/mcf5445x/pci.c:#define cfg_write(val, addr, type,
op)		op((type *)(addr), (val));

That's a very manageable and small series of patches I think if we
want to use either. I do like an obvious name, and we already have
CONFIG_...

>
>> I did for the 'sed' step - it was a 2-3 seconds to regenerate the header files.
>>
>> Full reconfig/recompile goes from about about 30s to 34s.
>> Incremental build doesn't change noticeably.
>
> I'm mostly concerned about build time for the autobuilder, which does
> full config / build cycles for all boards.  Here more than 10%
> increase hurt...

Yes this will definitely increase the time. The current brute force
'sed' of all headers isn't very efficient. How impossible would it be
to regenerate this only when someone adds a new CONFIG, and then check
it into the source?

>
>> I also tried recompiling both the mainline U-Boot's main.c 100 times,
>> and then this one. I could not see any time difference, which is a
>> little surprising. Maybe the C compiler's DCE is pretty early in the
>> the process?
>
> This is a surprising result, indeed.
>
>> >> +     if (config_autoboot_delay_str() && delaykey[0].str == NULL)
>> >> +             delaykey[0].str = config_autoboot_delay_str();
>> >
>> > Hm.... constructs like these make me think about side effects.  As is,
>> > your implementation does not pretect against any.  This may be
>> > dangerous - you are evaluating the macro multiple times now, while
>> > before it was only a defined() test, folowed by a single evaluation.
>
> You did not comment on this remark.  I think it is something to keep
> in mind!

Yes it is.

You could use the ..._enabled() form to get around this, but it may be
reasonable to state that CONFIGs are not allowed side effects. I have
previously tried to add function calls into a few CONFIGs and just got
compile errors. So it isn't a new problem.

>
>> > And to be honest - I find the old code easier to read.
>
> :-)

I prefer the one without #ifdefs, but I suppose this is not the worst
example of #ifdef stuff in U-Boot.

#  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

	if (config_autoboot_delay_str() && delaykey[0].str == NULL)
		delaykey[0].str = config_autoboot_delay_str();
	if (config_autoboot_delay_str2() && delaykey[1].str == NULL)
		delaykey[1].str = config_autoboot_delay_str2();
	if (config_autoboot_stop_str() && delaykey[2].str == NULL)
		delaykey[2].str = config_autoboot_stop_str();
	if (config_autoboot_stop_str2() && delaykey[3].str == NULL)
		delaykey[3].str = config_autoboot_stop_str2();


>
>> But if you have time, please take a look at the sed scripts and the Makefile.
>
> Sorry, but I can't find the audacity to bend my mind around these
> currently ;-)
>
> But you might have a look at "tools/scripts/define2mk.sed" and either
> use this as is, or base your code on this.  I would find it good to
> use the same code for the same (or so very similar) purposes.

Yes found it, copied it :-)

>
>> The background here is that I have been wondering about this for a
>> while, but have never really come up with a good way (in the absence
>> of a unified Kconfig) of getting a complete list of CONFIG variables.
>
> Does not the already existing "include/autoconf.mk" contain this
> information?  In any case, please check "tools/scripts/define2mk.sed"

It only has a list of CONFIG variables that are enabled for the board.
The C code will then get compile errors if it uses a config that is
not enabled. So we need to define all the others to be 0 so that the
code still compiles.

>
>> There was a mailing list post about this a few weeks ago. But then
>> David Hendrix complained about main.c which spurred me into action,
>> and I thought maybe we could just live with a brute force
>> 'find/grep/sed' approach for now, rather than trying to be too clever.
>> That's what I have done here.
>
> Understood, and appreciated.
>
>> I will come back to this when I have time - will be a week or two.
>
> Thanks.
>
> Best regards,
>
> Wolfgang Denk

Regards,
Simon

>
> --
> 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@denx.de
> If you think the problem is bad now, just wait until we've solved it.
>                                                         Epstein's Law
Wolfgang Denk - Feb. 19, 2013, 7:14 p.m.
Dear Simon Glass,

In message <CAPnjgZ2NvwAB0t4v=41BaVxqMaCgAU7XBTTbM1ZRT0fTA_40tA@mail.gmail.com> you wrote:
> 
> > You are wrong.  This includes a number of functions, and macros, too,
> > for example:
...
> That's a very manageable and small series of patches I think if we
> want to use either. I do like an obvious name, and we already have
> CONFIG_...

I think we really need to define a new, so far unused name space for
these, and reserve it for such purpose.

> Yes this will definitely increase the time. The current brute force
> 'sed' of all headers isn't very efficient. How impossible would it be
> to regenerate this only when someone adds a new CONFIG, and then check
> it into the source?

Doesn't work - assume you are hacking on your new code (without
checking in) - and if runs haywire because the needed re-scan is not
done...

> > Does not the already existing "include/autoconf.mk" contain this
> > information?  In any case, please check "tools/scripts/define2mk.sed"
> 
> It only has a list of CONFIG variables that are enabled for the board.
> The C code will then get compile errors if it uses a config that is
> not enabled. So we need to define all the others to be 0 so that the
> code still compiles.

I see.

Thanks.

Wolfgang Denk
Simon Glass - Feb. 21, 2013, 8:58 p.m.
Hi Wolfgang,

On Tue, Feb 19, 2013 at 11:14 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <CAPnjgZ2NvwAB0t4v=41BaVxqMaCgAU7XBTTbM1ZRT0fTA_40tA@mail.gmail.com> you wrote:
>>
>> > You are wrong.  This includes a number of functions, and macros, too,
>> > for example:
> ...
>> That's a very manageable and small series of patches I think if we
>> want to use either. I do like an obvious name, and we already have
>> CONFIG_...
>
> I think we really need to define a new, so far unused name space for
> these, and reserve it for such purpose.

What about:

autoconf_...(): value of CONFIG (or 0 if not defined)
autoconf_has_...(): 1 if the CONFIG is defined, 0 if not defined
(rarely needed I think)

This doesn't seem to be used currently.

>
>> Yes this will definitely increase the time. The current brute force
>> 'sed' of all headers isn't very efficient. How impossible would it be
>> to regenerate this only when someone adds a new CONFIG, and then check
>> it into the source?
>
> Doesn't work - assume you are hacking on your new code (without
> checking in) - and if runs haywire because the needed re-scan is not
> done...

You would get compile errors in this case. I'm not sure how we can
optimise this then.

>
>> > Does not the already existing "include/autoconf.mk" contain this
>> > information?  In any case, please check "tools/scripts/define2mk.sed"
>>
>> It only has a list of CONFIG variables that are enabled for the board.
>> The C code will then get compile errors if it uses a config that is
>> not enabled. So we need to define all the others to be 0 so that the
>> code still compiles.
>
> I see.
>
> Thanks.
>
> 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@denx.de
> The human race is faced with a cruel choice: work  or  daytime  tele-
> vision.

Regards,
Simon

Patch

diff --git a/Makefile b/Makefile
index fc18dd4..5ca3a57 100644
--- a/Makefile
+++ b/Makefile
@@ -614,6 +614,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 \
@@ -688,6 +689,43 @@  $(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 config_xxx() value', or '#define config_xxx() 0' where the CONFIG
+# is not used by this board configuration. This allows C code to do things
+# like 'if (config_xxx())' and have the compiler remove the dead code,
+# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases
+# if the config_...() 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 config_xxx_enabled(), 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/define2conf.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 ; \
+		( \
+			find ${src} -name "*.h" -type f | xargs \
+			cat | \
+			sed -n -f tools/scripts/define2list.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/define2list.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 $@
@@ -770,7 +808,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 d8cb394..3e89551 100644
--- a/README
+++ b/README
@@ -5434,11 +5434,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 config_xxx() value
+
+for enabled options, or:
+
+#	#define config_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 (config_xxx())
+		do_something;
+	else
+		do_something_else;
+
+The compiler will see that config_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 (config_xxx() && !config_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 '_enabled' suffix is provided, where
+the value is always either 0 or 1:
+
+	// Will work even if boaard config has '#define CONFIG_BOOTDELAY 0'
+	if (config_bootdelay_enabled())
+		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 config_xxx() everywhere else
+  3. Mark your functions and data structures static where possible
+  4. Use the config_xxx_enabled() variants only if essential
+  5. When changing existing code, first create a new patch to replace
+        #ifdefs in the surrounding area
diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c
index 7a3789e..3f62d83 100644
--- a/common/cmd_fitupd.c
+++ b/common/cmd_fitupd.c
@@ -8,6 +8,7 @@ 
 
 #include <common.h>
 #include <command.h>
+#include <net.h>
 
 #if !defined(CONFIG_UPDATE_TFTP)
 #error "CONFIG_UPDATE_TFTP required"
diff --git a/common/main.c b/common/main.c
index e2d2e09..cd42b67 100644
--- a/common/main.c
+++ b/common/main.c
@@ -28,30 +28,16 @@ 
 /* #define	DEBUG	*/
 
 #include <common.h>
-#include <watchdog.h>
 #include <command.h>
 #include <fdtdec.h>
-#include <malloc.h>
-#include <version.h>
-#ifdef CONFIG_MODEM_SUPPORT
-#include <malloc.h>		/* for free() prototype */
-#endif
-
-#ifdef CONFIG_SYS_HUSH_PARSER
-#include <hush.h>
-#endif
-
-#ifdef CONFIG_OF_CONTROL
-#include <fdtdec.h>
-#endif
-
-#ifdef CONFIG_OF_LIBFDT
 #include <fdt_support.h>
-#endif /* CONFIG_OF_LIBFDT */
-
+#include <hush.h>
+#include <malloc.h>
+#include <menu.h>
 #include <post.h>
+#include <version.h>
+#include <watchdog.h>
 #include <linux/ctype.h>
-#include <menu.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -61,13 +47,19 @@  DECLARE_GLOBAL_DATA_PTR;
 void inline __show_boot_progress (int val) {}
 void show_boot_progress (int val) __attribute__((weak, alias("__show_boot_progress")));
 
-#if defined(CONFIG_UPDATE_TFTP)
-int update_tftp (ulong addr);
-#endif /* CONFIG_UPDATE_TFTP */
-
 #define MAX_DELAY_STOP_STR 32
 
-#undef DEBUG_PARSER
+#define DEBUG_PARSER	0	/* set to 1 to debug */
+
+
+#define debug_parser(fmt, args...)		\
+	debug_cond(DEBUG_PARSER, fmt, ##args)
+
+#ifndef DEBUG_BOOTKEYS
+#define DEBUG_BOOTKEYS 0
+#endif
+#define debug_bootkeys(fmt, args...)		\
+	debug_cond(DEBUG_BOOTKEYS, fmt, ##args)
 
 char        console_buffer[CONFIG_SYS_CBSIZE + 1];	/* console I/O buffer	*/
 
@@ -75,32 +67,18 @@  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 */
-#endif
 
 /***************************************************************************
  * 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) && (CONFIG_BOOTDELAY >= 0)
-# if defined(CONFIG_AUTOBOOT_KEYED)
-#ifndef CONFIG_MENU
-static inline
-#endif
-int abortboot(int bootdelay)
+static int abortboot_keyed(int bootdelay)
 {
 	int abort = 0;
 	uint64_t etime = endtick(bootdelay);
@@ -121,31 +99,20 @@  int abortboot(int bootdelay)
 	u_int presskey_max = 0;
 	u_int i;
 
-#ifndef CONFIG_ZERO_BOOTDELAY_CHECK
-	if (bootdelay == 0)
+	if (config_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
+	if (config_autoboot_prompt_enabled())
+		printf(config_autoboot_prompt());
+
+	if (config_autoboot_delay_str() && delaykey[0].str == NULL)
+		delaykey[0].str = config_autoboot_delay_str();
+	if (config_autoboot_delay_str2() && delaykey[1].str == NULL)
+		delaykey[1].str = config_autoboot_delay_str2();
+	if (config_autoboot_stop_str() && delaykey[2].str == NULL)
+		delaykey[2].str = config_autoboot_stop_str();
+	if (config_autoboot_stop_str2() && delaykey[3].str == NULL)
+		delaykey[3].str = config_autoboot_stop_str2();
 
 	for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) {
 		delaykey[i].len = delaykey[i].str == NULL ?
@@ -156,11 +123,9 @@  int abortboot(int bootdelay)
 		presskey_max = presskey_max > delaykey[i].len ?
 				    presskey_max : delaykey[i].len;
 
-#  if DEBUG_BOOTKEYS
-		printf("%s key:<%s>\n",
+		debug_bootkeys("%s key:<%s>\n",
 		       delaykey[i].retry ? "delay" : "stop",
 		       delaykey[i].str ? delaykey[i].str : "NULL");
-#  endif
 	}
 
 	/* In order to keep up with incoming data, check timeout only
@@ -179,74 +144,56 @@  int abortboot(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,
 				    delaykey[i].str,
 				    delaykey[i].len) == 0) {
-#  if DEBUG_BOOTKEYS
-				printf("got %skey\n",
-				       delaykey[i].retry ? "delay" : "stop");
-#  endif
+				debug_bootkeys("got %skey\n",
+					delaykey[i].retry ? "delay" : "stop");
 
-#  ifdef CONFIG_BOOT_RETRY_TIME
 				/* don't retry auto boot */
-				if (! delaykey[i].retry)
+				if (config_boot_retry_time() &&
+						!delaykey[i].retry)
 					retry_time = -1;
-#  endif
 				abort = 1;
 			}
 		}
 	} while (!abort && get_ticks() <= etime);
 
-#  if DEBUG_BOOTKEYS
 	if (!abort)
-		puts("key timeout\n");
-#  endif
+		debug_bootkeys("key timeout\n");
 
-#ifdef CONFIG_SILENT_CONSOLE
-	if (abort)
+	if (config_silent_console() && abort)
 		gd->flags &= ~GD_FLG_SILENT;
-#endif
 
 	return abort;
 }
 
-# else	/* !defined(CONFIG_AUTOBOOT_KEYED) */
-
-#ifdef CONFIG_MENUKEY
 static int menukey = 0;
-#endif
 
-#ifndef CONFIG_MENU
-static inline
-#endif
-int abortboot(int bootdelay)
+static int abortboot_normal(int bootdelay)
 {
 	int abort = 0;
 	unsigned long ts;
 
-#ifdef CONFIG_MENUPROMPT
-	printf(CONFIG_MENUPROMPT);
-#else
-	if (bootdelay >= 0)
+	if (config_menuprompt())
+		printf(config_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 (bootdelay >= 0) {
+	if (config_zero_bootdelay_check() && bootdelay >= 0) {
 		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;
@@ -256,11 +203,10 @@  int abortboot(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 (config_menukey())
+					menukey = getc();
+				else
+					(void) getc();  /* consume input */
 				break;
 			}
 			udelay(10000);
@@ -271,15 +217,19 @@  int abortboot(int bootdelay)
 
 	putc('\n');
 
-#ifdef CONFIG_SILENT_CONSOLE
-	if (abort)
+	if (config_silent_console() && abort)
 		gd->flags &= ~GD_FLG_SILENT;
-#endif
 
 	return abort;
 }
-# endif	/* CONFIG_AUTOBOOT_KEYED */
-#endif	/* CONFIG_BOOTDELAY >= 0  */
+
+static int abortboot(int bootdelay)
+{
+	if (config_autoboot_keyed())
+		return abortboot_keyed(bootdelay);
+	else
+		return abortboot_normal(bootdelay);
+}
 
 /*
  * Runs the given boot command securely.  Specifically:
@@ -295,8 +245,6 @@  int abortboot(int bootdelay)
  * printing the error message to console.
  */
 
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
-	defined(CONFIG_OF_CONTROL)
 static void secure_boot_cmd(char *cmd)
 {
 	cmd_tbl_t *cmdtp;
@@ -337,190 +285,170 @@  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 *)(config_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 *)(config_sys_text_base() + addr));
+	}
 }
-#endif /* CONFIG_OF_CONTROL */
-
-
-/****************************************************************************/
 
-void main_loop (void)
+static void process_boot_delay(void)
 {
-#ifndef CONFIG_SYS_HUSH_PARSER
-	static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };
-	int len;
-	int rc = 1;
-	int flag;
-#endif
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
-		defined(CONFIG_OF_CONTROL)
-	char *env;
-#endif
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
-	char *s;
-	int bootdelay;
-#endif
-#ifdef CONFIG_PREBOOT
-	char *p;
-#endif
-#ifdef CONFIG_BOOTCOUNT_LIMIT
 	unsigned long bootcount = 0;
 	unsigned long bootlimit = 0;
-	char *bcs;
-	char bcs_set[16];
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
-
-	bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
-
-#ifdef CONFIG_BOOTCOUNT_LIMIT
-	bootcount = bootcount_load();
-	bootcount++;
-	bootcount_store (bootcount);
-	sprintf (bcs_set, "%lu", bootcount);
-	setenv ("bootcount", bcs_set);
-	bcs = getenv ("bootlimit");
-	bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0;
-#endif /* CONFIG_BOOTCOUNT_LIMIT */
-
-#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 */
-	}
-#endif  /* CONFIG_MODEM_SUPPORT */
-
-#ifdef CONFIG_VERSION_VARIABLE
-	{
-		setenv ("ver", version_string);  /* set version variable */
-	}
-#endif /* CONFIG_VERSION_VARIABLE */
-
-#ifdef CONFIG_SYS_HUSH_PARSER
-	u_boot_hush_start ();
-#endif
-
-#if defined(CONFIG_HUSH_INIT_VAR)
-	hush_init_var ();
-#endif
-
-#ifdef CONFIG_PREBOOT
-	if ((p = getenv ("preboot")) != NULL) {
-# ifdef CONFIG_AUTOBOOT_KEYED
-		int prev = disable_ctrlc(1);	/* disable Control C checking */
-# endif
-
-		run_command_list(p, -1, 0);
+	const char *s;
+	int bootdelay;
 
-# ifdef CONFIG_AUTOBOOT_KEYED
-		disable_ctrlc(prev);	/* restore Control C checking */
-# endif
+	if (config_bootcount_limit()) {
+		bootcount = bootcount_load();
+		bootcount++;
+		bootcount_store(bootcount);
+		setenv_ulong("bootcount", bootcount);
+		bootlimit = getenv_ulong("bootlimit", 10, 0);
 	}
-#endif /* CONFIG_PREBOOT */
-
-#if defined(CONFIG_UPDATE_TFTP)
-	update_tftp (0UL);
-#endif /* CONFIG_UPDATE_TFTP */
 
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
 	s = getenv ("bootdelay");
-	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY;
+	bootdelay = s ? (int)simple_strtol(s, NULL, 10) : config_bootdelay();
 
 	debug ("### main_loop entered: bootdelay=%d\n\n", bootdelay);
 
-#if defined(CONFIG_MENU_SHOW)
-	bootdelay = menu_show(bootdelay);
-#endif
-# ifdef CONFIG_BOOT_RETRY_TIME
-	init_cmd_timeout ();
-# endif	/* CONFIG_BOOT_RETRY_TIME */
+	if (config_menu_show())
+		bootdelay = menu_show(bootdelay);
+	if (config_boot_retry_time())
+		init_cmd_timeout();
 
-#ifdef CONFIG_POST
-	if (gd->flags & GD_FLG_POSTFAIL) {
+	if (config_post() && (gd->flags & GD_FLG_POSTFAIL)) {
 		s = getenv("failbootcmd");
-	}
-	else
-#endif /* CONFIG_POST */
-#ifdef CONFIG_BOOTCOUNT_LIMIT
-	if (bootlimit && (bootcount > bootlimit)) {
+	} else if (config_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");
-#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 (config_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>");
 
 	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 (config_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 (config_autoboot_keyed())
+			disable_ctrlc(prev);
 	}
 
-# ifdef CONFIG_MENUKEY
-	if (menukey == CONFIG_MENUKEY) {
+	if (config_menukey() && menukey == config_menukey()) {
 		s = getenv("menucmd");
 		if (s)
 			run_command_list(s, -1, 0);
 	}
-#endif /* CONFIG_MENUKEY */
-#endif /* CONFIG_BOOTDELAY */
+}
 
-#if defined CONFIG_OF_CONTROL
-	set_working_fdt_addr((void *)gd->fdt_blob);
-#endif /* CONFIG_OF_CONTROL */
+/****************************************************************************/
+
+void main_loop(void)
+{
+	static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };
+	int len;
+	int rc = 1;
+	int flag;
+
+	bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
+
+	if (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 (config_version_variable())
+		setenv("ver", version_string);  /* set version variable */
+
+	if (config_sys_hush_parser())
+		u_boot_hush_start();
+
+	if (config_hush_init_var())
+		hush_init_var();
+
+	if (config_preboot()) {
+		char *p = getenv("preboot");
+
+		if (p) {
+			int prev;
+
+			/* disable Control C checking */
+			if (config_autoboot_keyed())
+				prev = disable_ctrlc(1);
+
+			run_command_list(p, -1, 0);
+
+			/* restore Control C checking */
+			if (config_autoboot_keyed())
+				disable_ctrlc(prev);
+		}
+	}
+
+	if (config_update_tftp())
+		update_tftp(0UL);
+
+	if (config_bootdelay_enabled() && config_bootdelay() >= 0)
+		process_boot_delay();
+
+	if (config_of_control() && config_of_libfdt())
+		set_working_fdt_addr((void *)gd->fdt_blob);
 
 	/*
 	 * Main Loop for Monitor Command Processing
 	 */
-#ifdef CONFIG_SYS_HUSH_PARSER
-	parse_file_outer();
-	/* This point is never reached */
-	for (;;);
-#else
-	for (;;) {
-#ifdef CONFIG_BOOT_RETRY_TIME
-		if (rc >= 0) {
-			/* Saw enough of a valid command to
-			 * restart the timeout.
-			 */
-			reset_cmd_timeout();
+	if (config_sys_hush_parser()) {
+		parse_file_outer();
+		/* This point is never reached */
+		for (;;);
+	} else {
+		if (config_boot_retry_time()) {
+			if (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 */
@@ -528,33 +456,32 @@  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 (config_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 (config_reset_to_retry())
+				do_reset(NULL, 0, 0, NULL);
+			else
+				return;		/* retry autoboot */
 		}
-#endif
 
 		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 */
+		/* invalid command or not repeatable, forget it */
+		if (rc <= 0)
 			lastcommand[0] = 0;
-		}
 	}
-#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
@@ -562,14 +489,18 @@  void main_loop (void)
 void init_cmd_timeout(void)
 {
 	char *s = getenv ("bootretry");
+	int retry_min;
 
 	if (s != NULL)
 		retry_time = (int)simple_strtol(s, NULL, 10);
 	else
 		retry_time =  CONFIG_BOOT_RETRY_TIME;
 
-	if (retry_time >= 0 && retry_time < CONFIG_BOOT_RETRY_MIN)
-		retry_time = CONFIG_BOOT_RETRY_MIN;
+	retry_min = config_boot_retry_min() ? config_boot_retry_min()
+			: config_boot_retry_time();
+
+	if (retry_time >= 0 && retry_time < retry_min)
+		retry_time = retry_min;
 }
 
 /***************************************************************************
@@ -581,8 +512,6 @@  void reset_cmd_timeout(void)
 }
 #endif
 
-#ifdef CONFIG_CMDLINE_EDITING
-
 /*
  * cmdline-editing related codes from vivi.
  * Author: Janghoon Lyu <nandy@mizi.com>
@@ -685,27 +614,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);	\
@@ -791,13 +699,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 (config_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);
 
@@ -967,27 +875,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 (config_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;
@@ -1003,8 +911,6 @@  static int cread_line(const char *const prompt, char *buf, unsigned int *len,
 	return 0;
 }
 
-#endif /* CONFIG_CMDLINE_EDITING */
-
 /****************************************************************************/
 
 /*
@@ -1026,84 +932,52 @@  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 (;;) {
-#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 (config_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()) {
-			show_activity(0);
-			WATCHDOG_RESET();
+		if (config_show_activity()) {
+			while (!tstc()) {
+				show_activity(0);
+				WATCHDOG_RESET();
+			}
 		}
-#endif
 		c = getc();
 
 		/*
 		 * Special character handling
 		 */
 		switch (c) {
-		case '\r':				/* Enter		*/
+		case '\r':			/* Enter		*/
 		case '\n':
 			*p = '\0';
 			puts ("\r\n");
-			return (p - p_buf);
+			return p - p_buf;
 
-		case '\0':				/* nul			*/
+		case '\0':			/* nul			*/
 			continue;
 
-		case 0x03:				/* ^C - break		*/
+		case 0x03:			/* ^C - break		*/
 			p_buf[0] = '\0';	/* discard input */
-			return (-1);
+			return -1;
 
-		case 0x15:				/* ^U - erase line	*/
+		case 0x15:			/* ^U - erase line	*/
 			while (col > plen) {
 				puts (erase_seq);
 				--col;
@@ -1112,15 +986,15 @@  int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
 			n = 0;
 			continue;
 
-		case 0x17:				/* ^W - erase word	*/
+		case 0x17:			/* ^W - erase word	*/
 			p=delete_char(p_buf, p, &col, &n, plen);
 			while ((n > 0) && (*p != ' ')) {
 				p=delete_char(p_buf, p, &col, &n, plen);
 			}
 			continue;
 
-		case 0x08:				/* ^H  - backspace	*/
-		case 0x7F:				/* DEL - backspace	*/
+		case 0x08:			/* ^H  - backspace	*/
+		case 0x7F:			/* DEL - backspace	*/
 			p=delete_char(p_buf, p, &col, &n, plen);
 			continue;
 
@@ -1129,22 +1003,28 @@  int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
 			 * Must be a normal character then
 			 */
 			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 (c == '\t') {	/* expand TABs */
+					if (config_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 {
 					char buf[2];
 
 					/*
-					 * Echo input using puts() to force am
+					 * Echo input using puts() to force an
 					 * LCD flush if we are using an LCD
 					 */
 					++col;
@@ -1159,14 +1039,47 @@  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 (config_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
 }
 
 /****************************************************************************/
 
-static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen)
+static char *delete_char (char *buffer, char *p, int *colp, int *np, int plen)
 {
 	char *s;
 
@@ -1202,9 +1115,7 @@  int parse_line (char *line, char *argv[])
 {
 	int nargs = 0;
 
-#ifdef DEBUG_PARSER
-	printf ("parse_line: \"%s\"\n", line);
-#endif
+	debug_parser("parse_line: \"%s\"\n", line);
 	while (nargs < CONFIG_SYS_MAXARGS) {
 
 		/* skip any white space */
@@ -1213,9 +1124,7 @@  int parse_line (char *line, char *argv[])
 
 		if (*line == '\0') {	/* end of line, no more args	*/
 			argv[nargs] = NULL;
-#ifdef DEBUG_PARSER
-		printf ("parse_line: nargs=%d\n", nargs);
-#endif
+			debug_parser("parse_line: nargs=%d\n", nargs);
 			return (nargs);
 		}
 
@@ -1227,9 +1136,7 @@  int parse_line (char *line, char *argv[])
 
 		if (*line == '\0') {	/* end of line, no more args	*/
 			argv[nargs] = NULL;
-#ifdef DEBUG_PARSER
-		printf ("parse_line: nargs=%d\n", nargs);
-#endif
+			debug_parser("parse_line: nargs=%d\n", nargs);
 			return (nargs);
 		}
 
@@ -1238,15 +1145,12 @@  int parse_line (char *line, char *argv[])
 
 	printf ("** Too many args (max. %d) **\n", CONFIG_SYS_MAXARGS);
 
-#ifdef DEBUG_PARSER
-	printf ("parse_line: nargs=%d\n", nargs);
-#endif
+	debug_parser("parse_line: nargs=%d\n", nargs);
 	return (nargs);
 }
 
 /****************************************************************************/
 
-#ifndef CONFIG_SYS_HUSH_PARSER
 static void process_macros (const char *input, char *output)
 {
 	char c, prev;
@@ -1258,12 +1162,10 @@  static void process_macros (const char *input, char *output)
 	/* 1 = waiting for '(' or '{' */
 	/* 2 = waiting for ')' or '}' */
 	/* 3 = waiting for '''  */
-#ifdef DEBUG_PARSER
 	char *output_start = output;
 
-	printf ("[PROCESS_MACROS] INPUT len %d: \"%s\"\n", strlen (input),
-		input);
-#endif
+	debug_parser("[PROCESS_MACROS] INPUT len %zd: \"%s\"\n", strlen(input),
+		     input);
 
 	prev = '\0';		/* previous character   */
 
@@ -1351,10 +1253,8 @@  static void process_macros (const char *input, char *output)
 	else
 		*(output - 1) = 0;
 
-#ifdef DEBUG_PARSER
-	printf ("[PROCESS_MACROS] OUTPUT len %d: \"%s\"\n",
-		strlen (output_start), output_start);
-#endif
+	debug_parser("[PROCESS_MACROS] OUTPUT len %zd: \"%s\"\n",
+		strlen(output_start), output_start);
 }
 
 /****************************************************************************
@@ -1385,12 +1285,12 @@  static int builtin_run_command(const char *cmd, int flag)
 	int repeatable = 1;
 	int rc = 0;
 
-#ifdef DEBUG_PARSER
-	printf ("[RUN_COMMAND] cmd[%p]=\"", cmd);
-	puts (cmd ? cmd : "NULL");	/* use puts - string may be loooong */
-	puts ("\"\n");
-#endif
-
+	debug_parser("[RUN_COMMAND] cmd[%p]=\"", cmd);
+	if (DEBUG_PARSER) {
+		/* use puts - string may be loooong */
+		puts(cmd ? cmd : "NULL");
+		puts("\"\n");
+	}
 	clear_ctrlc();		/* forget any previous Control C */
 
 	if (!cmd || !*cmd) {
@@ -1408,9 +1308,7 @@  static int builtin_run_command(const char *cmd, int flag)
 	 * repeatable commands
 	 */
 
-#ifdef DEBUG_PARSER
-	printf ("[PROCESS_SEPARATORS] %s\n", cmd);
-#endif
+	debug_parser("[PROCESS_SEPARATORS] %s\n", cmd);
 	while (*str) {
 
 		/*
@@ -1439,9 +1337,7 @@  static int builtin_run_command(const char *cmd, int flag)
 		}
 		else
 			str = sep;	/* no more commands for next pass */
-#ifdef DEBUG_PARSER
-		printf ("token: \"%s\"\n", token);
-#endif
+		debug_parser("token: \"%s\"\n", token);
 
 		/* find macros in this token and replace them */
 		process_macros (token, finaltoken);
@@ -1462,7 +1358,6 @@  static int builtin_run_command(const char *cmd, int flag)
 
 	return rc ? rc : repeatable;
 }
-#endif
 
 /*
  * Run a command using the selected parser.
@@ -1473,22 +1368,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 (config_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.
  *
@@ -1529,7 +1423,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)
 {
@@ -1539,13 +1432,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 (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;
+		}
 	}
 	if (need_buff) {
 		buff = malloc(len + 1);
@@ -1554,20 +1450,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 (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);
+	}
 
 	return rcode;
 }
diff --git a/include/command.h b/include/command.h
index 3785eb9..80da938 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 4ad17ea..e5eb882 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>
@@ -294,9 +297,6 @@  int	readline_into_buffer(const char *const prompt, char *buffer,
 int	parse_line (char *, char *[]);
 void	init_cmd_timeout(void);
 void	reset_cmd_timeout(void);
-#ifdef CONFIG_MENU
-int	abortboot(int bootdelay);
-#endif
 extern char console_buffer[];
 
 /* arch/$(ARCH)/lib/board.c */
@@ -310,6 +310,7 @@  extern ulong monitor_flash_len;
 int mac_read_from_eeprom(void);
 extern u8 _binary_dt_dtb_start[];	/* embedded device tree blob */
 int set_cpu_clk_info(void);
+extern int mdm_init(void); /* defined in board.c */
 
 /**
  * Show the DRAM size in a board-specific way
@@ -857,9 +858,7 @@  int	pcmcia_init (void);
 
 #include <bootstage.h>
 
-#ifdef CONFIG_SHOW_ACTIVITY
 void show_activity(int arg);
-#endif
 
 /* Multicore arch functions */
 #ifdef CONFIG_MP
diff --git a/include/config_drop.h b/include/config_drop.h
new file mode 100644
index 0000000..bf2beaa
--- /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 referred to anywhere in U-Boot */
+#define CONFIG_MENUPROMPT	"Auto-boot prompt"
+#define CONFIG_MENUKEY
+#define CONFIG_UPDATE_TFTP
+
+#endif
diff --git a/include/configs/pm9263.h b/include/configs/pm9263.h
index b60a9ad..6f6ddfa 100644
--- a/include/configs/pm9263.h
+++ b/include/configs/pm9263.h
@@ -355,7 +355,7 @@ 
 
 #define CONFIG_BOOTCOMMAND		"run flashboot"
 #define CONFIG_ROOTPATH			"/ronetix/rootfs"
-#define CONFIG_AUTOBOOT_PROMPT		"autoboot in %d seconds\n"
+#define CONFIG_AUTOBOOT_PROMPT		"autoboot in %d seconds\n", bootdelay
 
 #define CONFIG_CON_ROT			"fbcon=rotate:3 "
 #define CONFIG_BOOTARGS			"root=/dev/mtdblock4 rootfstype=jffs2 "\
diff --git a/include/fdt_support.h b/include/fdt_support.h
index 2cccc35..cf8f5e0 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>
 
@@ -132,5 +132,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 */
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
diff --git a/include/menu.h b/include/menu.h
index 7af5fdb..1cdcb88 100644
--- a/include/menu.h
+++ b/include/menu.h
@@ -28,7 +28,5 @@  int menu_item_add(struct menu *m, char *item_key, void *item_data);
 int menu_destroy(struct menu *m);
 void menu_display_statusline(struct menu *m);
 
-#if defined(CONFIG_MENU_SHOW)
 int menu_show(int bootdelay);
-#endif
 #endif /* __MENU_H__ */
diff --git a/include/net.h b/include/net.h
index 970d4d1..1b56b7d 100644
--- a/include/net.h
+++ b/include/net.h
@@ -695,6 +695,8 @@  extern void copy_filename(char *dst, const char *src, int size);
 /* get a random source port */
 extern unsigned int random_port(void);
 
+extern int update_tftp(ulong addr);
+
 /**********************************************************************/
 
 #endif /* __NET_H__ */
diff --git a/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed
new file mode 100644
index 0000000..269eaba
--- /dev/null
+++ b/tools/scripts/define2conf.sed
@@ -0,0 +1,36 @@ 
+#
+# Sed script to parse CPP macros and generate a list of CONFIG macros
+#
+# This converts:
+#	#define CONFIG_XXX value
+#into:
+#	#define config_xxx() value
+#	#define config_xxx_enabled() 1
+#
+
+# Macros with parameters are ignored.
+/^#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]*//;
+	# 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 a version suffixed with _enabled, value 1
+	s/().*/_enabled() 1/
+	# print the line
+	p
+}
diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed
new file mode 100644
index 0000000..8df8aa0
--- /dev/null
+++ b/tools/scripts/define2list.sed
@@ -0,0 +1,31 @@ 
+#
+# Sed script to parse CPP macros and generate a list of CONFIG macros
+#
+# This converts:
+#	#define CONFIG_XXX value
+#into:
+#	#define config_xxx() 0
+#	#define config_xxx_enabled() 0
+
+# Macros with parameters are ignored.
+/^#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/\(.*\)/#define \L\1/
+	# Append 0
+	s/$/() 0/
+	# print the line
+	p
+	# Create a version suffixed with _enabled, value 0
+	s/().*/_enabled() 0/
+	# print the line
+	p
+}