[U-Boot] [RFC PATCH v2 0/15] Provide a mechanism to avoid using #ifdef everywhere
Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes different boards compile different versions of the source code, meaning that we must build all boards to check for failures. It is easy to misspell an #ifdef and there is not as much checking of this by the compiler. Multiple dependent #ifdefs are harder to do than with if..then..else. Variable declarations must be #idefed as well as the code that uses them, often much later in the file/function. #ifdef indents don't match code indents and have their own separate indent feature. Overall, excessive use of #idef hurts readability and makes the code harder to modify and refactor. For people coming newly into the code base, #ifdefs can be a big barrier.
The use of #ifdef in U-Boot has possibly got a little out of hand. In an attempt to turn the tide, this series includes a patch which provides a way to make CONFIG macros available to C code without using the preprocessor. This makes it possible to use standard C conditional features such as if/then instead of #ifdef. A README update exhorts compliance.
As an example of how to use this, this series replaces all but two #ifdefs from the main code body of common/main.c, which 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.2 powerpc: (622 boards) text +9.1 data +0.0 bss +1.9 sh: (21 boards) bss +2.5 nios2: (3 boards) text +24.0 data -1.3 bss +1.3 arm: (285 boards) spl/u-boot-spl:text +0.4 text -2.3 bss +5.5 nds32: (3 boards) text -29.3 bss +10.7
Note that a config_drop.h file is added - this defines all the CONFIGs which are not used in any board config file. Without this, autoconf cannot define the macros for this CONFIGs.
Compile time for main.c does not seem to be any different in my tests. The time to perform the 'dep' step (which now creates autoconf.h) increases, from about 2.8s to about 4.6s. This additional time is used to grep, sed and sort the contents of all the header file in U-Boot. The time for an incremental build is not affected.
It would be much more efficient to maintain a list of all available CONFIG defines, but no such list exists at present.
Changes in v2: - Split out changes to main.c into separate patches - Fix up a few errors and comments in the original RFC - Use autoconf_...() instead of config_...() - Use autoconf_has_...() instead of config_..._enabled() - Add a grep to the sed/sort pipe to speed up processing
Simon Glass (15): Implement autoconf header file at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263 net: Add prototype for update_tftp, and use autoconf main: Separate out the two abortboot() functions main: Move boot_delay code into its own function main: Use autoconf for boot retry feature main: Remove CONFIG #ifdefs from the abortboot() code main: Use get/setenv_ulong() main: Use autoconf for boot_delay code main: Use autoconf for parser selection main: Use autoconf in command line reading main: Use autoconf in main_loop() main: Correct header order main: Add debug_parser() to avoid #ifdefs main: Add debug_bootkeys to avoid #ifdefs
Makefile | 42 ++- README | 87 ++++- common/cmd_fitupd.c | 3 +- common/main.c | 809 ++++++++++++++++++------------------------ common/update.c | 24 +- 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 | 3 + tools/scripts/define2conf.sed | 37 ++ tools/scripts/define2list.sed | 31 ++ 15 files changed, 579 insertions(+), 495 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed
Add support for generating an autoconf.h header file that can be used in the source instead of #ifdef.
For example, instead of:
#ifdef CONFIG_VERSION_VARIABLE setenv("ver", version_string); /* set version variable */ #endif
you can do:
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */
The compiler will ensure that the dead code is eliminated, so the result is the same.
Where the value of the CONFIG define is 0, you can use the autoconf_has...() form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is defined at all, it affects behaviour:
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); #endif
So we use:
if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) s = getenv ("bootdelay");
This later form should only be used for such 'difficult' defines where a zero value still means that the CONFIG should be considered to be defined.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: - Split out changes to main.c into separate patches - Fix up a few errors and comments in the original RFC - Use autoconf_...() instead of config_...() - Use autoconf_has_...() instead of config_..._enabled() - Add a grep to the sed/sort pipe to speed up processing
Makefile | 42 ++++++++++++++++++++- README | 87 +++++++++++++++++++++++++++++++++++++++++-- include/common.h | 3 ++ include/config_drop.h | 17 +++++++++ tools/scripts/define2conf.sed | 37 ++++++++++++++++++ tools/scripts/define2list.sed | 31 +++++++++++++++ 6 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed
diff --git a/Makefile b/Makefile index fc18dd4..9f4f55d 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,44 @@ $(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 ; \ + : We can grep for CONFIG since the value will be dropped ; \ + ( \ + find ${src} -name "*.h" -type f | xargs \ + cat | grep CONFIG | \ + sed -n -f tools/scripts/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 +809,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/include/common.h b/include/common.h index 4ad17ea..491783b 100644 --- a/include/common.h +++ b/include/common.h @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short; typedef volatile unsigned char vu_char;
#include <config.h> +#ifndef DO_DEPS_ONLY +#include <generated/autoconf.h> +#endif #include <asm-offsets.h> #include <linux/bitops.h> #include <linux/types.h> diff --git a/include/config_drop.h b/include/config_drop.h new file mode 100644 index 0000000..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/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed new file mode 100644 index 0000000..2c4a2ef --- /dev/null +++ b/tools/scripts/define2conf.sed @@ -0,0 +1,37 @@ +# +# Sed script to parse CPP macros and generate a list of CONFIG macros +# +# This converts: +# #define CONFIG_XXX value +#into: +# #define 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]*CONFIG_/autoconf_/; + # Change to form CONFIG_*=VALUE + s/[\t ][\t ]*/=/; + # Handle lines with no value + s/^([^=]*)$/\1=/; + # Drop trailing spaces + s/ *$//; + # Change empty values to '1' + s/=$/=1/; + # Add #define at the start + s/^([^=]*)=/#define \L\1() / + # print the line + p + # Create autoconf_has_...(), value 1 + s/().*/() 1/ + s/(autoconf_)/\1has_/ + # print the line + p +} diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed new file mode 100644 index 0000000..152280d --- /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/CONFIG_(.*)/#define autoconf_\L\1/ + # Append 0 + s/$/() 0/ + # print the line + p + # Create autoconf_has_...(), value 0 + s/(autoconf_)/\1has_/ + # print the line + p +}
Hi Simon,
On Sun, Feb 24, 2013 at 11:25 AM, Simon Glass sjg@chromium.org wrote:
Add support for generating an autoconf.h header file that can be used in the source instead of #ifdef.
For example, instead of:
#ifdef CONFIG_VERSION_VARIABLE setenv("ver", version_string); /* set version variable */ #endif
you can do:
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */
You are changing the meaning between these two examples. The old code was #ifDEF, which means the new example needs to be autoconf_HAS_*. Is there a reason to muddy the waters by recommending people use this automatic value of 0 instead of using the "has" function? Any more than without this patch we should go change most all the #ifdef to #if?
The compiler will ensure that the dead code is eliminated, so the result is the same.
Where the value of the CONFIG define is 0, you can use the autoconf_has...() form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is defined at all, it affects behaviour:
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); #endif
So we use:
if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) s = getenv ("bootdelay");This later form should only be used for such 'difficult' defines where a zero value still means that the CONFIG should be considered to be defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Split out changes to main.c into separate patches
- Fix up a few errors and comments in the original RFC
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()
- Add a grep to the sed/sort pipe to speed up processing
Makefile | 42 ++++++++++++++++++++- README | 87 +++++++++++++++++++++++++++++++++++++++++-- include/common.h | 3 ++ include/config_drop.h | 17 +++++++++ tools/scripts/define2conf.sed | 37 ++++++++++++++++++ tools/scripts/define2list.sed | 31 +++++++++++++++ 6 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed
diff --git a/Makefile b/Makefile index fc18dd4..9f4f55d 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,44 @@ $(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
You forgot to update this comment when changing to autoconf_has. Grep perhaps?
+# 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 ; \: We can grep for CONFIG since the value will be dropped ; \( \find ${src} -name "*.h" -type f | xargs \cat | grep CONFIG | \sed -n -f tools/scripts/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; \
From looking at define2list.sed (its name is confusing so sorry if I'm
interpreting wrong) it lists all things NOT enabled. But I guess you are wanting exact text with which to prune the -all.tmp. Could you craft it such that you can use .tmp directly to prune -all.tmp instead of having to generate this -enable.tmp file (which is misleading and potentially a waste of sed time)? Please rename define2list.sed and define2conf.sed to something which has meaning. This is quite hard to read as it.
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 +809,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
Isn't it true that dead code stripping doesn't strip variables only used by that dead code? So stack usage is wasted everywhere in this new model?
+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
Use the new names here...
+for enabled options, or:
+# #define config_xxx() 0
+for disabled options. You can therefore generally change code like this:
#ifdef CONFIG_XXXdo_something#elsedo_something_else#endif+to this:
if (config_xxx())do_something;elsedo_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#endifif (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:
- Use #ifdef to conditionally compile an exported function or variable
- Use ordinary C code with config_xxx() everywhere else
- Mark your functions and data structures static where possible
- Use the config_xxx_enabled() variants only if essential
Fix names here.
- When changing existing code, first create a new patch to replace
#ifdefs in the surrounding areadiff --git a/include/common.h b/include/common.h index 4ad17ea..491783b 100644 --- a/include/common.h +++ b/include/common.h @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short; typedef volatile unsigned char vu_char;
#include <config.h> +#ifndef DO_DEPS_ONLY +#include <generated/autoconf.h> +#endif #include <asm-offsets.h> #include <linux/bitops.h> #include <linux/types.h> diff --git a/include/config_drop.h b/include/config_drop.h new file mode 100644 index 0000000..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 */
You mean not referred to by any config header?
+#define CONFIG_MENUPROMPT "Auto-boot prompt" +#define CONFIG_MENUKEY +#define CONFIG_UPDATE_TFTP
+#endif diff --git a/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed new file mode 100644 index 0000000..2c4a2ef --- /dev/null +++ b/tools/scripts/define2conf.sed @@ -0,0 +1,37 @@ +# +# Sed script to parse CPP macros and generate a list of CONFIG macros +# +# This converts: +# #define CONFIG_XXX value +#into: +# #define config_xxx() value +# #define config_xxx_enabled() 1
Fix the names here.
+#
+# Macros with parameters are ignored. +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
d
Any reason not to use "[A-Za-z0-9_]+" instead of "[A-Za-z0-9_][A-Za-z0-9_]*"?
+}
+# Only process values prefixed with #define CONFIG_ +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
# Strip the #define prefixs/#define[ \t]*CONFIG_/autoconf_/;# Change to form CONFIG_*=VALUEs/[\t ][\t ]*/=/;# Handle lines with no values/^\([^=]*\)$/\1=/;# Drop trailing spacess/ *$//;# Change empty values to '1's/=$/=1/;# Add #define at the starts/^\([^=]*\)=/#define \L\1() /# print the linep# Create autoconf_has_...(), value 1s/().*/() 1/s/\(autoconf_\)/\1has_/# print the linep+} diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed
Please change the name to be more clear as to its purpose. "list" doesn't convey to me that it is all configs that should be set as undefined.
new file mode 100644 index 0000000..152280d --- /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
Fix names here.
+# 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 prefixs/#define *//;# Remove the values/[ \t].*//;# Convert to lower case, prepend #defines/CONFIG_\(.*\)/#define autoconf_\L\1/# Append 0s/$/() 0/# print the linep# Create autoconf_has_...(), value 0s/\(autoconf_\)/\1has_/# print the linep+}
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Joe,
On Sun, Feb 24, 2013 at 11:50 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:25 AM, Simon Glass sjg@chromium.org wrote:
Add support for generating an autoconf.h header file that can be used in the source instead of #ifdef.
For example, instead of:
#ifdef CONFIG_VERSION_VARIABLE setenv("ver", version_string); /* set version variable */ #endif
you can do:
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */You are changing the meaning between these two examples. The old code was #ifDEF, which means the new example needs to be autoconf_HAS_*. Is there a reason to muddy the waters by recommending people use this automatic value of 0 instead of using the "has" function? Any more than without this patch we should go change most all the #ifdef to #if?
Thanks much for going through this so carefully.
Here is my thinking on this point - I will respond to the rest of your comments when I get back to this.
I believe we should get rid of the 'has' function eventually.
Most features are anyway just a 0 or a 1 - either the feature is enabled or it is disabled. There are some where a value is provided, and that means that the feature is automatically enabled. I'm not a huge fan of that. To me we should have two completely different concepts in U-Boot:
- enabling an option, which generally just affects which files are compiled - i.e. an option that should generally only appear in Makefiles - configuring something, by which I mean giving the code a value to work with. This should come either from CONFIG for compile-time config, or device tree for run-time config
At present these two are mixed up, and I don't think it aids clarity.
There are very very few CONFIGs that need this 'has' option I think. I know about BOOTDELAY, and I'm sure there are others, but there was recent discussion on the list about whether we should separate out the bootdelay/autoboot value from the enablement, wasn't there?
The compiler will ensure that the dead code is eliminated, so the result is the same.
Where the value of the CONFIG define is 0, you can use the autoconf_has...() form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is defined at all, it affects behaviour:
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); #endif
So we use:
if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) s = getenv ("bootdelay");This later form should only be used for such 'difficult' defines where a zero value still means that the CONFIG should be considered to be defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Split out changes to main.c into separate patches
- Fix up a few errors and comments in the original RFC
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()
- Add a grep to the sed/sort pipe to speed up processing
Makefile | 42 ++++++++++++++++++++- README | 87 +++++++++++++++++++++++++++++++++++++++++-- include/common.h | 3 ++ include/config_drop.h | 17 +++++++++ tools/scripts/define2conf.sed | 37 ++++++++++++++++++ tools/scripts/define2list.sed | 31 +++++++++++++++ 6 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed
diff --git a/Makefile b/Makefile index fc18dd4..9f4f55d 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,44 @@ $(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
You forgot to update this comment when changing to autoconf_has. Grep perhaps?
Yes this whole commit message needs updating, sorry. I didn't get any response on the desirability of autoconf, so just sent out another RFC.
+# 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 ; \: We can grep for CONFIG since the value will be dropped ; \( \find ${src} -name "*.h" -type f | xargs \cat | grep CONFIG | \sed -n -f tools/scripts/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; \From looking at define2list.sed (its name is confusing so sorry if I'm interpreting wrong) it lists all things NOT enabled. But I guess you are wanting exact text with which to prune the -all.tmp. Could you craft it such that you can use .tmp directly to prune -all.tmp instead of having to generate this -enable.tmp file (which is misleading and potentially a waste of sed time)? Please rename define2list.sed and define2conf.sed to something which has meaning. This is quite hard to read as it.
Actually the sed script turns any files containing CONFIG defines into a file that defines autoconf versions of these to zero. It can be used either on things that are enabled, or on things that are not. The point of the 'comm' is to figure which (by a process of elimination) which CONFIGs are present but not enabled.
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 +809,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
Isn't it true that dead code stripping doesn't strip variables only used by that dead code? So stack usage is wasted everywhere in this new model?
Yes, that does seem to happen with my gcc - thanks for pointing it out. It's a little odd, I wonder if there is a justification for it, or if it is just overlooked. It seems particularly strange considering the effort gcc goes to to eliminate unused variables.
+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
Use the new names here...
+for enabled options, or:
+# #define config_xxx() 0
+for disabled options. You can therefore generally change code like this:
#ifdef CONFIG_XXXdo_something#elsedo_something_else#endif+to this:
if (config_xxx())do_something;elsedo_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#endifif (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:
- Use #ifdef to conditionally compile an exported function or variable
- Use ordinary C code with config_xxx() everywhere else
- Mark your functions and data structures static where possible
- Use the config_xxx_enabled() variants only if essential
Fix names here.
- When changing existing code, first create a new patch to replace
#ifdefs in the surrounding areadiff --git a/include/common.h b/include/common.h index 4ad17ea..491783b 100644 --- a/include/common.h +++ b/include/common.h @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short; typedef volatile unsigned char vu_char;
#include <config.h> +#ifndef DO_DEPS_ONLY +#include <generated/autoconf.h> +#endif #include <asm-offsets.h> #include <linux/bitops.h> #include <linux/types.h> diff --git a/include/config_drop.h b/include/config_drop.h new file mode 100644 index 0000000..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 */
You mean not referred to by any config header?
Or in fact any header.
+#define CONFIG_MENUPROMPT "Auto-boot prompt" +#define CONFIG_MENUKEY +#define CONFIG_UPDATE_TFTP
+#endif diff --git a/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed new file mode 100644 index 0000000..2c4a2ef --- /dev/null +++ b/tools/scripts/define2conf.sed @@ -0,0 +1,37 @@ +# +# Sed script to parse CPP macros and generate a list of CONFIG macros +# +# This converts: +# #define CONFIG_XXX value +#into: +# #define config_xxx() value +# #define config_xxx_enabled() 1
Fix the names here.
+#
+# Macros with parameters are ignored. +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
dAny reason not to use "[A-Za-z0-9_]+" instead of "[A-Za-z0-9_][A-Za-z0-9_]*"?
Should be fine, probably I just overlooked it.
+}
+# Only process values prefixed with #define CONFIG_ +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
# Strip the #define prefixs/#define[ \t]*CONFIG_/autoconf_/;# Change to form CONFIG_*=VALUEs/[\t ][\t ]*/=/;# Handle lines with no values/^\([^=]*\)$/\1=/;# Drop trailing spacess/ *$//;# Change empty values to '1's/=$/=1/;# Add #define at the starts/^\([^=]*\)=/#define \L\1() /# print the linep# Create autoconf_has_...(), value 1s/().*/() 1/s/\(autoconf_\)/\1has_/# print the linep+} diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed
Please change the name to be more clear as to its purpose. "list" doesn't convey to me that it is all configs that should be set as undefined.
Will do.
new file mode 100644 index 0000000..152280d --- /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
Fix names here.
+# 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 prefixs/#define *//;# Remove the values/[ \t].*//;# Convert to lower case, prepend #defines/CONFIG_\(.*\)/#define autoconf_\L\1/# Append 0s/$/() 0/# print the linep# Create autoconf_has_...(), value 0s/\(autoconf_\)/\1has_/# print the linep+}
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Regards, Simon
Hi Simon,
On Mon, Feb 25, 2013 at 12:10 AM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On Sun, Feb 24, 2013 at 11:50 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:25 AM, Simon Glass sjg@chromium.org wrote:
Add support for generating an autoconf.h header file that can be used in the source instead of #ifdef.
For example, instead of:
#ifdef CONFIG_VERSION_VARIABLE setenv("ver", version_string); /* set version variable */ #endif
you can do:
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */You are changing the meaning between these two examples. The old code was #ifDEF, which means the new example needs to be autoconf_HAS_*. Is there a reason to muddy the waters by recommending people use this automatic value of 0 instead of using the "has" function? Any more than without this patch we should go change most all the #ifdef to #if?
Thanks much for going through this so carefully.
No problem.
Here is my thinking on this point - I will respond to the rest of your comments when I get back to this.
I believe we should get rid of the 'has' function eventually.
Fair enough. I think it would be nice if your mechanism provided a way to segregate these two. Some sort of naming convention that differed between the two cases. The cases should be easy to identify. If it's an enable switch, then it will be #define'd, but with no value. If it is defined to a value, then it is configuration. This will also help to end the bad practice of board configs using "#define CONFIG_ENABLE_MY_FEATURE 1", which of course it only checked for defined or not... then the unsuspecting user juct changes the 1 to a 0 and gets no change in behavior. By using a different naming convention, adding that "1" would prevent the feature from working up front.
The more I see "autoconf" the more I think that people will confuse this with GNU autoconf. maybe it would be good to avoid that.
Most features are anyway just a 0 or a 1 - either the feature is enabled or it is disabled. There are some where a value is provided, and that means that the feature is automatically enabled. I'm not a huge fan of that. To me we should have two completely different concepts in U-Boot:
- enabling an option, which generally just affects which files are
compiled - i.e. an option that should generally only appear in Makefiles
- configuring something, by which I mean giving the code a value to
work with. This should come either from CONFIG for compile-time config, or device tree for run-time config
At present these two are mixed up, and I don't think it aids clarity.
Maybe something like enable_[lower-case-name]() and config_[lower-case name](). But in matching your current feature-set, you would not even need to generate the config_ functions... only the enable_ functions.
There are very very few CONFIGs that need this 'has' option I think. I know about BOOTDELAY, and I'm sure there are others, but there was recent discussion on the list about whether we should separate out the bootdelay/autoboot value from the enablement, wasn't there?
Indeed. It was a patch I submitted for this release. http://patchwork.ozlabs.org/patch/219303/ It at least removed he dependence on the default value. Even better would be to separate the default value from the selection of building in support as a separate variable.
The compiler will ensure that the dead code is eliminated, so the result is the same.
Where the value of the CONFIG define is 0, you can use the autoconf_has...() form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is defined at all, it affects behaviour:
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); #endif
So we use:
if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) s = getenv ("bootdelay");This later form should only be used for such 'difficult' defines where a zero value still means that the CONFIG should be considered to be defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Split out changes to main.c into separate patches
- Fix up a few errors and comments in the original RFC
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()
- Add a grep to the sed/sort pipe to speed up processing
Makefile | 42 ++++++++++++++++++++- README | 87 +++++++++++++++++++++++++++++++++++++++++-- include/common.h | 3 ++ include/config_drop.h | 17 +++++++++ tools/scripts/define2conf.sed | 37 ++++++++++++++++++ tools/scripts/define2list.sed | 31 +++++++++++++++ 6 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed
diff --git a/Makefile b/Makefile index fc18dd4..9f4f55d 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,44 @@ $(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
You forgot to update this comment when changing to autoconf_has. Grep perhaps?
Yes this whole commit message needs updating, sorry. I didn't get any response on the desirability of autoconf, so just sent out another RFC.
Again, after considering this more, I don't like autoconf. Sorry.
+# 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 ; \: We can grep for CONFIG since the value will be dropped ; \( \find ${src} -name "*.h" -type f | xargs \cat | grep CONFIG | \sed -n -f tools/scripts/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; \From looking at define2list.sed (its name is confusing so sorry if I'm interpreting wrong) it lists all things NOT enabled. But I guess you are wanting exact text with which to prune the -all.tmp. Could you craft it such that you can use .tmp directly to prune -all.tmp instead of having to generate this -enable.tmp file (which is misleading and potentially a waste of sed time)? Please rename define2list.sed and define2conf.sed to something which has meaning. This is quite hard to read as it.
Actually the sed script turns any files containing CONFIG defines into a file that defines autoconf versions of these to zero. It can be used either on things that are enabled, or on things that are not. The point of the 'comm' is to figure which (by a process of elimination) which CONFIGs are present but not enabled.
I'm pretty sure I understand how this was working. What I was asking is if there was a straightforward way to use the output of define2value.sed run on config.h to prune the -all output of define2zero.sed, instead of running define2zero.sed on the config.h.
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 +809,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
Isn't it true that dead code stripping doesn't strip variables only used by that dead code? So stack usage is wasted everywhere in this new model?
Yes, that does seem to happen with my gcc - thanks for pointing it out. It's a little odd, I wonder if there is a justification for it, or if it is just overlooked. It seems particularly strange considering the effort gcc goes to to eliminate unused variables.
Indeed. It seems this would be a big concern for SPL, right?
+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
Use the new names here...
+for enabled options, or:
+# #define config_xxx() 0
+for disabled options. You can therefore generally change code like this:
#ifdef CONFIG_XXXdo_something#elsedo_something_else#endif+to this:
if (config_xxx())do_something;elsedo_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#endifif (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:
- Use #ifdef to conditionally compile an exported function or variable
- Use ordinary C code with config_xxx() everywhere else
- Mark your functions and data structures static where possible
- Use the config_xxx_enabled() variants only if essential
Fix names here.
- When changing existing code, first create a new patch to replace
#ifdefs in the surrounding areadiff --git a/include/common.h b/include/common.h index 4ad17ea..491783b 100644 --- a/include/common.h +++ b/include/common.h @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short; typedef volatile unsigned char vu_char;
#include <config.h> +#ifndef DO_DEPS_ONLY +#include <generated/autoconf.h> +#endif #include <asm-offsets.h> #include <linux/bitops.h> #include <linux/types.h> diff --git a/include/config_drop.h b/include/config_drop.h new file mode 100644 index 0000000..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 */
You mean not referred to by any config header?
Or in fact any header.
If they are not referred to in any header, they why are they needed here? What is the purpose? I guess I just don't yet understand what problem this solves.
+#define CONFIG_MENUPROMPT "Auto-boot prompt" +#define CONFIG_MENUKEY +#define CONFIG_UPDATE_TFTP
+#endif diff --git a/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed new file mode 100644 index 0000000..2c4a2ef --- /dev/null +++ b/tools/scripts/define2conf.sed @@ -0,0 +1,37 @@ +# +# Sed script to parse CPP macros and generate a list of CONFIG macros +# +# This converts: +# #define CONFIG_XXX value +#into: +# #define config_xxx() value +# #define config_xxx_enabled() 1
Fix the names here.
+#
+# Macros with parameters are ignored. +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
dAny reason not to use "[A-Za-z0-9_]+" instead of "[A-Za-z0-9_][A-Za-z0-9_]*"?
Should be fine, probably I just overlooked it.
It's bizarre that + doesn't work.
+}
+# Only process values prefixed with #define CONFIG_ +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
# Strip the #define prefixs/#define[ \t]*CONFIG_/autoconf_/;# Change to form CONFIG_*=VALUEs/[\t ][\t ]*/=/;# Handle lines with no values/^\([^=]*\)$/\1=/;# Drop trailing spacess/ *$//;# Change empty values to '1's/=$/=1/;# Add #define at the starts/^\([^=]*\)=/#define \L\1() /# print the linep# Create autoconf_has_...(), value 1s/().*/() 1/s/\(autoconf_\)/\1has_/# print the linep+} diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed
Please change the name to be more clear as to its purpose. "list" doesn't convey to me that it is all configs that should be set as undefined.
Will do.
The new names are much clearer.
<snip>
Cheers, -Joe
Hi Joe,
On Wed, Feb 27, 2013 at 1:08 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Mon, Feb 25, 2013 at 12:10 AM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On Sun, Feb 24, 2013 at 11:50 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:25 AM, Simon Glass sjg@chromium.org wrote:
Add support for generating an autoconf.h header file that can be used in the source instead of #ifdef.
For example, instead of:
#ifdef CONFIG_VERSION_VARIABLE setenv("ver", version_string); /* set version variable */ #endif
you can do:
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */You are changing the meaning between these two examples. The old code was #ifDEF, which means the new example needs to be autoconf_HAS_*. Is there a reason to muddy the waters by recommending people use this automatic value of 0 instead of using the "has" function? Any more than without this patch we should go change most all the #ifdef to #if?
Thanks much for going through this so carefully.
No problem.
Here is my thinking on this point - I will respond to the rest of your comments when I get back to this.
I believe we should get rid of the 'has' function eventually.
Fair enough. I think it would be nice if your mechanism provided a way to segregate these two. Some sort of naming convention that differed between the two cases. The cases should be easy to identify. If it's an enable switch, then it will be #define'd, but with no value. If it is defined to a value, then it is configuration. This will also help to end the bad practice of board configs using "#define CONFIG_ENABLE_MY_FEATURE 1", which of course it only checked for defined or not... then the unsuspecting user juct changes the 1 to a 0 and gets no change in behavior. By using a different naming convention, adding that "1" would prevent the feature from working up front.
That sounds like you want to change the CONFIG_ defines to separate out the two meanings. I think it would be nice to have something like:
#define CONFIG_ENABLE_MY_FEATURE #define PARAMS_MY_FEATURE_PORT 4
in the board config files, so that it is clear that one is a Makefile/code selection CONFIG and the other is changing parameters. But that is a big change, out of scope of this patch I think.
I could potentially add a check for the "#define CONFIG_ENABLE_MY_FEATURE 1" error you mention, but how would I know whether it is valid? There is currently no way to distinguish the two types of config.
The more I see "autoconf" the more I think that people will confuse this with GNU autoconf. maybe it would be good to avoid that.
Well we can't use config or cfg. I think autoconf is a reasonable choice as we already have an autoconf.mk generated. This extends that to provide access to C code using autoconf.h.
But if you don't like autoconf, please put your thinking hat on and suggest a new name.
Most features are anyway just a 0 or a 1 - either the feature is enabled or it is disabled. There are some where a value is provided, and that means that the feature is automatically enabled. I'm not a huge fan of that. To me we should have two completely different concepts in U-Boot:
- enabling an option, which generally just affects which files are
compiled - i.e. an option that should generally only appear in Makefiles
- configuring something, by which I mean giving the code a value to
work with. This should come either from CONFIG for compile-time config, or device tree for run-time config
At present these two are mixed up, and I don't think it aids clarity.
Maybe something like enable_[lower-case-name]() and config_[lower-case name](). But in matching your current feature-set, you would not even need to generate the config_ functions... only the enable_ functions.
Well config was ruled out unfortunately because it is already used in some places. enable has the same problem. I went with:
autoconf_...()
and for the uncommon case that I would like to deprecate:
autoconf_has_...()
There are very very few CONFIGs that need this 'has' option I think. I know about BOOTDELAY, and I'm sure there are others, but there was recent discussion on the list about whether we should separate out the bootdelay/autoboot value from the enablement, wasn't there?
Indeed. It was a patch I submitted for this release. http://patchwork.ozlabs.org/patch/219303/ It at least removed he dependence on the default value. Even better would be to separate the default value from the selection of building in support as a separate variable.
OK great. Yes that would be best. I suspect that would apply to other similar corner cases.
The compiler will ensure that the dead code is eliminated, so the result is the same.
Where the value of the CONFIG define is 0, you can use the autoconf_has...() form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is defined at all, it affects behaviour:
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); #endif
So we use:
if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) s = getenv ("bootdelay");This later form should only be used for such 'difficult' defines where a zero value still means that the CONFIG should be considered to be defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Split out changes to main.c into separate patches
- Fix up a few errors and comments in the original RFC
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()
- Add a grep to the sed/sort pipe to speed up processing
Makefile | 42 ++++++++++++++++++++- README | 87 +++++++++++++++++++++++++++++++++++++++++-- include/common.h | 3 ++ include/config_drop.h | 17 +++++++++ tools/scripts/define2conf.sed | 37 ++++++++++++++++++ tools/scripts/define2list.sed | 31 +++++++++++++++ 6 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed
diff --git a/Makefile b/Makefile index fc18dd4..9f4f55d 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,44 @@ $(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
You forgot to update this comment when changing to autoconf_has. Grep perhaps?
Yes this whole commit message needs updating, sorry. I didn't get any response on the desirability of autoconf, so just sent out another RFC.
Again, after considering this more, I don't like autoconf. Sorry.
+# 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 ; \: We can grep for CONFIG since the value will be dropped ; \( \find ${src} -name "*.h" -type f | xargs \cat | grep CONFIG | \sed -n -f tools/scripts/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; \From looking at define2list.sed (its name is confusing so sorry if I'm interpreting wrong) it lists all things NOT enabled. But I guess you are wanting exact text with which to prune the -all.tmp. Could you craft it such that you can use .tmp directly to prune -all.tmp instead of having to generate this -enable.tmp file (which is misleading and potentially a waste of sed time)? Please rename define2list.sed and define2conf.sed to something which has meaning. This is quite hard to read as it.
Actually the sed script turns any files containing CONFIG defines into a file that defines autoconf versions of these to zero. It can be used either on things that are enabled, or on things that are not. The point of the 'comm' is to figure which (by a process of elimination) which CONFIGs are present but not enabled.
I'm pretty sure I understand how this was working. What I was asking is if there was a straightforward way to use the output of define2value.sed run on config.h to prune the -all output of define2zero.sed, instead of running define2zero.sed on the config.h.
OK I see. It's not that easy - comm requires that the lines be the same, so it is easier just to generate them with the same script. The time impact of this is tiny - the enabled config is a very small file and does not require scanning all the headers files in U-Boot :-)
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 +809,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
Isn't it true that dead code stripping doesn't strip variables only used by that dead code? So stack usage is wasted everywhere in this new model?
Yes, that does seem to happen with my gcc - thanks for pointing it out. It's a little odd, I wonder if there is a justification for it, or if it is just overlooked. It seems particularly strange considering the effort gcc goes to to eliminate unused variables.
Indeed. It seems this would be a big concern for SPL, right?
In the case where people write large functions with lots of conditional code, yes. But that would be bad practice anyway - does it happen in other situations? I prefer that we try to keep our functions a bit smaller if possible.
+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
Use the new names here...
+for enabled options, or:
+# #define config_xxx() 0
+for disabled options. You can therefore generally change code like this:
#ifdef CONFIG_XXXdo_something#elsedo_something_else#endif+to this:
if (config_xxx())do_something;elsedo_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#endifif (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:
- Use #ifdef to conditionally compile an exported function or variable
- Use ordinary C code with config_xxx() everywhere else
- Mark your functions and data structures static where possible
- Use the config_xxx_enabled() variants only if essential
Fix names here.
- When changing existing code, first create a new patch to replace
#ifdefs in the surrounding areadiff --git a/include/common.h b/include/common.h index 4ad17ea..491783b 100644 --- a/include/common.h +++ b/include/common.h @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short; typedef volatile unsigned char vu_char;
#include <config.h> +#ifndef DO_DEPS_ONLY +#include <generated/autoconf.h> +#endif #include <asm-offsets.h> #include <linux/bitops.h> #include <linux/types.h> diff --git a/include/config_drop.h b/include/config_drop.h new file mode 100644 index 0000000..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 */
You mean not referred to by any config header?
Or in fact any header.
If they are not referred to in any header, they why are they needed here? What is the purpose? I guess I just don't yet understand what problem this solves.
They are needed only because there is code in U-Boot (C files) that checks these configs. If we don't create an autoconf define for them, then that code will not build. It's actually a good check for the dead code problem.
+#define CONFIG_MENUPROMPT "Auto-boot prompt" +#define CONFIG_MENUKEY +#define CONFIG_UPDATE_TFTP
+#endif diff --git a/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed new file mode 100644 index 0000000..2c4a2ef --- /dev/null +++ b/tools/scripts/define2conf.sed @@ -0,0 +1,37 @@ +# +# Sed script to parse CPP macros and generate a list of CONFIG macros +# +# This converts: +# #define CONFIG_XXX value +#into: +# #define config_xxx() value +# #define config_xxx_enabled() 1
Fix the names here.
+#
+# Macros with parameters are ignored. +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
dAny reason not to use "[A-Za-z0-9_]+" instead of "[A-Za-z0-9_][A-Za-z0-9_]*"?
Should be fine, probably I just overlooked it.
It's bizarre that + doesn't work.
Yes it did throw me, but on the bizarre scale I rate this only a 3. There is a note in my sed man page which might explain it, but is very vague.
POSIX.2 BREs should be supported, but they aren't completely because of performance problems. The \n sequence in a regular expression matches the newline character, and similarly for \a, \t, and other sequences.
+}
+# Only process values prefixed with #define CONFIG_ +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
# Strip the #define prefixs/#define[ \t]*CONFIG_/autoconf_/;# Change to form CONFIG_*=VALUEs/[\t ][\t ]*/=/;# Handle lines with no values/^\([^=]*\)$/\1=/;# Drop trailing spacess/ *$//;# Change empty values to '1's/=$/=1/;# Add #define at the starts/^\([^=]*\)=/#define \L\1() /# print the linep# Create autoconf_has_...(), value 1s/().*/() 1/s/\(autoconf_\)/\1has_/# print the linep+} diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed
Please change the name to be more clear as to its purpose. "list" doesn't convey to me that it is all configs that should be set as undefined.
Will do.
The new names are much clearer.
OK good. Please have another think about autoconf - if you really don't like it then please suggest something else that doesn't currently exist in U-Boot.
<snip>
Cheers, -Joe
Regards, Simon
Hi Joe,
On Sun, Feb 24, 2013 at 11:50 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:25 AM, Simon Glass sjg@chromium.org wrote:
Add support for generating an autoconf.h header file that can be used in the source instead of #ifdef.
For example, instead of:
#ifdef CONFIG_VERSION_VARIABLE setenv("ver", version_string); /* set version variable */ #endif
you can do:
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */You are changing the meaning between these two examples. The old code was #ifDEF, which means the new example needs to be autoconf_HAS_*. Is there a reason to muddy the waters by recommending people use this automatic value of 0 instead of using the "has" function? Any more than without this patch we should go change most all the #ifdef to #if?
The compiler will ensure that the dead code is eliminated, so the result is the same.
Where the value of the CONFIG define is 0, you can use the autoconf_has...() form. For example CONFIG_BOOTDELAY can be -ve, 0 or +ve, but if it is defined at all, it affects behaviour:
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); #endif
So we use:
if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) s = getenv ("bootdelay");This later form should only be used for such 'difficult' defines where a zero value still means that the CONFIG should be considered to be defined.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Split out changes to main.c into separate patches
- Fix up a few errors and comments in the original RFC
- Use autoconf_...() instead of config_...()
- Use autoconf_has_...() instead of config_..._enabled()
- Add a grep to the sed/sort pipe to speed up processing
Makefile | 42 ++++++++++++++++++++- README | 87 +++++++++++++++++++++++++++++++++++++++++-- include/common.h | 3 ++ include/config_drop.h | 17 +++++++++ tools/scripts/define2conf.sed | 37 ++++++++++++++++++ tools/scripts/define2list.sed | 31 +++++++++++++++ 6 files changed, 213 insertions(+), 4 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed
diff --git a/Makefile b/Makefile index fc18dd4..9f4f55d 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,44 @@ $(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
You forgot to update this comment when changing to autoconf_has. Grep perhaps?
+# 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 ; \: We can grep for CONFIG since the value will be dropped ; \( \find ${src} -name "*.h" -type f | xargs \cat | grep CONFIG | \sed -n -f tools/scripts/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; \From looking at define2list.sed (its name is confusing so sorry if I'm interpreting wrong) it lists all things NOT enabled. But I guess you are wanting exact text with which to prune the -all.tmp. Could you craft it such that you can use .tmp directly to prune -all.tmp instead of having to generate this -enable.tmp file (which is misleading and potentially a waste of sed time)? Please rename define2list.sed and define2conf.sed to something which has meaning. This is quite hard to read as it.
As I mentioned in the previous email, define2list.sed doesn't filter out enabled configs - it just converts every CONFIG it sees to an autoconf() defined to 0.
I'm not entirely sure what you are looking for, and in so far as I understand it I'm not entirely sure how to do it. Let me explain a bit and then you can tell me where you are coming from.
I create two lists using this define2list.sed script:
- all CONFIGs found in all headers, let's call it A for all - only the enabled CONFIGs, let's call it E for enabled
Then by using comm, I work out which ones must be disabled D = A - E. In other words the output of comm is a list of disabled CONFIGs. Conveniently the sed script defines these to 0.
The other side (defining the things that are enabled, to their actual values), is handled by define2conf.sed.
Regarding wasting sed's time, how can I avoid running sed on all CONFIGs and enabled CONFIGs in order to get the disabled CONFIGs? Yes it is a waste of time calculating E, since it is thrown away, but it seems to be the only way to calculate D.
Re the rename, I will try define2zero.sed and define2autoconf.sed - how does that sound?
[snip]
diff --git a/include/common.h b/include/common.h index 4ad17ea..491783b 100644 --- a/include/common.h +++ b/include/common.h @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short; typedef volatile unsigned char vu_char;
#include <config.h> +#ifndef DO_DEPS_ONLY +#include <generated/autoconf.h> +#endif #include <asm-offsets.h> #include <linux/bitops.h> #include <linux/types.h> diff --git a/include/config_drop.h b/include/config_drop.h new file mode 100644 index 0000000..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 */
You mean not referred to by any config header?
Yes, will fix.
Regards, Simon
This is not currently used, since autoboot is not enabled for this board, but the string is missing a parameter. Add it.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
include/configs/pm9263.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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 "\
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
This is not currently used, since autoboot is not enabled for this board, but the string is missing a parameter. Add it.
Why not enable autoboot for this board so that this setting gets testing?
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
include/configs/pm9263.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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 "\
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Joe,
On Sun, Feb 24, 2013 at 11:53 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
This is not currently used, since autoboot is not enabled for this board, but the string is missing a parameter. Add it.
Why not enable autoboot for this board so that this setting gets testing?
Actually with autoconf this setting is tested, which is how I found the problem. The old code used #ifdef and so the problem was masked.
Or do you think I should enable because that was probably the board vendor's indent?
Regards, Simon
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
include/configs/pm9263.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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 "\
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Hi Simon,
On Mon, Feb 25, 2013 at 11:28 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On Sun, Feb 24, 2013 at 11:53 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
This is not currently used, since autoboot is not enabled for this board, but the string is missing a parameter. Add it.
Why not enable autoboot for this board so that this setting gets testing?
Actually with autoconf this setting is tested, which is how I found the problem. The old code used #ifdef and so the problem was masked.
Or do you think I should enable because that was probably the board vendor's indent?
What I was thinking is that if the board sets the configuration for this, then they surely want this feature to work. It should be enabled by default for this board (unless the maintainer explicitly declares no, in which case the configuration should be removed completely).
+Ilko Iliev iliev@ronetix.at again
Hi Joe,
On Wed, Feb 27, 2013 at 12:40 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Mon, Feb 25, 2013 at 11:28 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On Sun, Feb 24, 2013 at 11:53 AM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
This is not currently used, since autoboot is not enabled for this board, but the string is missing a parameter. Add it.
Why not enable autoboot for this board so that this setting gets testing?
Actually with autoconf this setting is tested, which is how I found the problem. The old code used #ifdef and so the problem was masked.
Or do you think I should enable because that was probably the board vendor's indent?
What I was thinking is that if the board sets the configuration for this, then they surely want this feature to work. It should be enabled by default for this board (unless the maintainer explicitly declares no, in which case the configuration should be removed completely).
I don't disagree with you, but I think that is a matter for the board maintainer. I found a minor problem in the config file which I fixed. But I don't really think I want to enable CONFIG_AUTOBOOT_KEYED, select some values for CONFIG_AUTOBOOT_DELAY_STR2, etc. as well. For my purposes I just need the board to continue to boot.
Somehow we dropped Ilko Iliev iliev@ronetix.at off the Cc here, so have re-added.
Regards, Simon
This function should be declared in net.h. At the same time, let's use autoconf insteaf of #ifdef for its inclusion.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/cmd_fitupd.c | 3 +-- common/main.c | 9 ++------- common/update.c | 24 ++++++++---------------- include/net.h | 3 +++ 4 files changed, 14 insertions(+), 25 deletions(-)
diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c index 7a3789e..618ff7c 100644 --- a/common/cmd_fitupd.c +++ b/common/cmd_fitupd.c @@ -8,13 +8,12 @@
#include <common.h> #include <command.h> +#include <net.h>
#if !defined(CONFIG_UPDATE_TFTP) #error "CONFIG_UPDATE_TFTP required" #endif
-extern int update_tftp(ulong addr); - static int do_fitupd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { ulong addr = 0UL; diff --git a/common/main.c b/common/main.c index e2d2e09..2b8af2c 100644 --- a/common/main.c +++ b/common/main.c @@ -61,10 +61,6 @@ 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 @@ -427,9 +423,8 @@ void main_loop (void) } #endif /* CONFIG_PREBOOT */
-#if defined(CONFIG_UPDATE_TFTP) - update_tftp (0UL); -#endif /* CONFIG_UPDATE_TFTP */ + if (autoconf_update_tftp()) + update_tftp(0UL);
#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); diff --git a/common/update.c b/common/update.c index 94d6a82..9cd9ca2 100644 --- a/common/update.c +++ b/common/update.c @@ -43,19 +43,6 @@ /* env variable holding the location of the update file */ #define UPDATE_FILE_ENV "updatefile"
-/* set configuration defaults if needed */ -#ifndef CONFIG_UPDATE_LOAD_ADDR -#define CONFIG_UPDATE_LOAD_ADDR 0x100000 -#endif - -#ifndef CONFIG_UPDATE_TFTP_MSEC_MAX -#define CONFIG_UPDATE_TFTP_MSEC_MAX 100 -#endif - -#ifndef CONFIG_UPDATE_TFTP_CNT_MAX -#define CONFIG_UPDATE_TFTP_CNT_MAX 0 -#endif - extern ulong TftpRRQTimeoutMSecs; extern int TftpRRQTimeoutCountMax; extern flash_info_t flash_info[]; @@ -244,6 +231,7 @@ int update_tftp(ulong addr) char *filename, *env_addr; int images_noffset, ndepth, noffset; ulong update_addr, update_fladdr, update_size; + int msec_max; void *fit; int ret = 0;
@@ -266,12 +254,16 @@ int update_tftp(ulong addr) /* get load address of downloaded update file */ if ((env_addr = getenv("loadaddr")) != NULL) addr = simple_strtoul(env_addr, NULL, 16); + else if (autoconf_has_update_load_addr()) + addr = autoconf_update_load_addr(); else - addr = CONFIG_UPDATE_LOAD_ADDR; + addr = 0x100000;
+ msec_max = autoconf_has_update_tftp_msec_max() ? + autoconf_update_tftp_msec_max() : 100;
- if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX, - CONFIG_UPDATE_TFTP_CNT_MAX, addr)) { + if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(), + addr)) { printf("Can't load update file, aborting auto-update\n"); return 1; } diff --git a/include/net.h b/include/net.h index 970d4d1..23fb947 100644 --- a/include/net.h +++ b/include/net.h @@ -695,6 +695,9 @@ extern void copy_filename(char *dst, const char *src, int size); /* get a random source port */ extern unsigned int random_port(void);
+/* Update U-Boot over TFTP */ +extern int update_tftp(ulong addr); + /**********************************************************************/
#endif /* __NET_H__ */
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
This function should be declared in net.h. At the same time, let's use autoconf insteaf of #ifdef for its inclusion.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/cmd_fitupd.c | 3 +-- common/main.c | 9 ++------- common/update.c | 24 ++++++++---------------- include/net.h | 3 +++ 4 files changed, 14 insertions(+), 25 deletions(-)
diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c index 7a3789e..618ff7c 100644 --- a/common/cmd_fitupd.c +++ b/common/cmd_fitupd.c @@ -8,13 +8,12 @@
#include <common.h> #include <command.h> +#include <net.h>
#if !defined(CONFIG_UPDATE_TFTP) #error "CONFIG_UPDATE_TFTP required" #endif
-extern int update_tftp(ulong addr);
static int do_fitupd(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { ulong addr = 0UL; diff --git a/common/main.c b/common/main.c index e2d2e09..2b8af2c 100644 --- a/common/main.c +++ b/common/main.c @@ -61,10 +61,6 @@ 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 @@ -427,9 +423,8 @@ void main_loop (void) } #endif /* CONFIG_PREBOOT */
-#if defined(CONFIG_UPDATE_TFTP)
update_tftp (0UL);-#endif /* CONFIG_UPDATE_TFTP */
if (autoconf_update_tftp())
Shouldn't you be using the _has version here?
update_tftp(0UL);#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); diff --git a/common/update.c b/common/update.c index 94d6a82..9cd9ca2 100644 --- a/common/update.c +++ b/common/update.c @@ -43,19 +43,6 @@ /* env variable holding the location of the update file */ #define UPDATE_FILE_ENV "updatefile"
-/* set configuration defaults if needed */ -#ifndef CONFIG_UPDATE_LOAD_ADDR -#define CONFIG_UPDATE_LOAD_ADDR 0x100000 -#endif
-#ifndef CONFIG_UPDATE_TFTP_MSEC_MAX -#define CONFIG_UPDATE_TFTP_MSEC_MAX 100 -#endif
-#ifndef CONFIG_UPDATE_TFTP_CNT_MAX -#define CONFIG_UPDATE_TFTP_CNT_MAX 0 -#endif
extern ulong TftpRRQTimeoutMSecs; extern int TftpRRQTimeoutCountMax; extern flash_info_t flash_info[]; @@ -244,6 +231,7 @@ int update_tftp(ulong addr) char *filename, *env_addr; int images_noffset, ndepth, noffset; ulong update_addr, update_fladdr, update_size;
int msec_max; void *fit; int ret = 0;@@ -266,12 +254,16 @@ int update_tftp(ulong addr) /* get load address of downloaded update file */ if ((env_addr = getenv("loadaddr")) != NULL) addr = simple_strtoul(env_addr, NULL, 16);
else if (autoconf_has_update_load_addr())addr = autoconf_update_load_addr(); else
addr = CONFIG_UPDATE_LOAD_ADDR;
addr = 0x100000;msec_max = autoconf_has_update_tftp_msec_max() ?autoconf_update_tftp_msec_max() : 100;
if (update_load(filename, CONFIG_UPDATE_TFTP_MSEC_MAX,CONFIG_UPDATE_TFTP_CNT_MAX, addr)) {
if (update_load(filename, msec_max, autoconf_update_tftp_cnt_max(),addr)) {
Sneaky using autoconf_update_tftp_cnt_max() to default to 0.
printf("Can't load update file, aborting auto-update\n"); return 1; }diff --git a/include/net.h b/include/net.h index 970d4d1..23fb947 100644 --- a/include/net.h +++ b/include/net.h @@ -695,6 +695,9 @@ extern void copy_filename(char *dst, const char *src, int size); /* get a random source port */ extern unsigned int random_port(void);
+/* Update U-Boot over TFTP */ +extern int update_tftp(ulong addr);
/**********************************************************************/
#endif /* __NET_H__ */
1.8.1.3
Acked-by: Joe Hershberger joe.hershberger@ni.com
There are two implementations of autoboot(). Turn these into two separate functions, and create a single autoboot() which calls either one or the other.
Also it seems that nothing uses autoboot() outside main, so make it static.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 22 ++++++++++------------ include/common.h | 3 --- 2 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/common/main.c b/common/main.c index 2b8af2c..1e12e55 100644 --- a/common/main.c +++ b/common/main.c @@ -92,11 +92,7 @@ extern void mdm_init(void); /* defined in board.c */ * 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); @@ -209,16 +205,11 @@ int abortboot(int bootdelay) 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; @@ -274,7 +265,14 @@ int abortboot(int bootdelay)
return abort; } -# endif /* CONFIG_AUTOBOOT_KEYED */ + +static int abortboot(int bootdelay) +{ + if (autoconf_autoboot_keyed()) + return abortboot_keyed(bootdelay); + else + return abortboot_normal(bootdelay); +} #endif /* CONFIG_BOOTDELAY >= 0 */
/* diff --git a/include/common.h b/include/common.h index 491783b..fb219fd 100644 --- a/include/common.h +++ b/include/common.h @@ -297,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 */
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
There are two implementations of autoboot(). Turn these into two separate functions, and create a single autoboot() which calls either one or the other.
Also it seems that nothing uses autoboot() outside main, so make it static.
You say "autoboot" in this change log, but I think you mean to say "abortboot".
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 22 ++++++++++------------ include/common.h | 3 --- 2 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/common/main.c b/common/main.c index 2b8af2c..1e12e55 100644 --- a/common/main.c +++ b/common/main.c @@ -92,11 +92,7 @@ extern void mdm_init(void); /* defined in board.c */
- 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); @@ -209,16 +205,11 @@ int abortboot(int bootdelay) 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; @@ -274,7 +265,14 @@ int abortboot(int bootdelay)
return abort;} -# endif /* CONFIG_AUTOBOOT_KEYED */
+static int abortboot(int bootdelay) +{
if (autoconf_autoboot_keyed())return abortboot_keyed(bootdelay);elsereturn abortboot_normal(bootdelay);+} #endif /* CONFIG_BOOTDELAY >= 0 */
/* diff --git a/include/common.h b/include/common.h index 491783b..fb219fd 100644 --- a/include/common.h +++ b/include/common.h @@ -297,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
Is CONFIG_MENU gone at this point? Does it no longer reference abortboot()?
extern char console_buffer[];
/* arch/$(ARCH)/lib/board.c */
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Hi Joe,
On Sun, Feb 24, 2013 at 12:13 PM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
There are two implementations of autoboot(). Turn these into two separate functions, and create a single autoboot() which calls either one or the other.
Also it seems that nothing uses autoboot() outside main, so make it static.
You say "autoboot" in this change log, but I think you mean to say "abortboot".
Yes, will fix, thanks.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 22 ++++++++++------------ include/common.h | 3 --- 2 files changed, 10 insertions(+), 15 deletions(-)
diff --git a/common/main.c b/common/main.c index 2b8af2c..1e12e55 100644 --- a/common/main.c +++ b/common/main.c @@ -92,11 +92,7 @@ extern void mdm_init(void); /* defined in board.c */
- 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); @@ -209,16 +205,11 @@ int abortboot(int bootdelay) 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; @@ -274,7 +265,14 @@ int abortboot(int bootdelay)
return abort;} -# endif /* CONFIG_AUTOBOOT_KEYED */
+static int abortboot(int bootdelay) +{
if (autoconf_autoboot_keyed())return abortboot_keyed(bootdelay);elsereturn abortboot_normal(bootdelay);+} #endif /* CONFIG_BOOTDELAY >= 0 */
/* diff --git a/include/common.h b/include/common.h index 491783b..fb219fd 100644 --- a/include/common.h +++ b/include/common.h @@ -297,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
Is CONFIG_MENU gone at this point? Does it no longer reference abortboot()?
Well it isn't needed in this file. In fact it seems since commit 8594753b this code is not needed any more.
extern char console_buffer[];
/* arch/$(ARCH)/lib/board.c */
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Regards, Simon
Move this code into its own function, since it clutters up main_loop().
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 155 ++++++++++++++++++++++++++++++---------------------------- 1 file changed, 79 insertions(+), 76 deletions(-)
diff --git a/common/main.c b/common/main.c index 1e12e55..0df7992 100644 --- a/common/main.c +++ b/common/main.c @@ -91,7 +91,6 @@ extern void mdm_init(void); /* defined in board.c */ * Watch for 'delay' seconds for autoboot stop or autoboot delay string. * returns: 0 - no key string, allow autoboot 1 - got key string, abort */ -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) static int abortboot_keyed(int bootdelay) { int abort = 0; @@ -273,7 +272,6 @@ static int abortboot(int bootdelay) else return abortboot_normal(bootdelay); } -#endif /* CONFIG_BOOTDELAY >= 0 */
/* * Runs the given boot command securely. Specifically: @@ -289,8 +287,7 @@ static int abortboot(int bootdelay) * printing the error message to console. */
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \ - defined(CONFIG_OF_CONTROL) +#ifdef CONFIG_OF_CONTROL static void secure_boot_cmd(char *cmd) { cmd_tbl_t *cmdtp; @@ -331,46 +328,33 @@ static void process_fdt_options(const void *blob)
/* Add an env variable to point to a kernel payload, if available */ addr = fdtdec_get_config_int(gd->fdt_blob, "kernel-offset", 0); - if (addr) - setenv_addr("kernaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr)); + if (addr) { + setenv_addr("kernaddr", + (void *)(autoconf_sys_text_base() + addr)); + }
/* Add an env variable to point to a root disk, if available */ addr = fdtdec_get_config_int(gd->fdt_blob, "rootdisk-offset", 0); - if (addr) - setenv_addr("rootaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr)); + if (addr) { + setenv_addr("rootaddr", + (void *)(autoconf_sys_text_base() + addr)); + } } #endif /* CONFIG_OF_CONTROL */
- -/****************************************************************************/ - -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_OF_CONTROL + char *env; +#endif + char *s; + int bootdelay;
#ifdef CONFIG_BOOTCOUNT_LIMIT bootcount = bootcount_load(); @@ -382,51 +366,8 @@ void main_loop (void) 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); - -# ifdef CONFIG_AUTOBOOT_KEYED - disable_ctrlc(prev); /* restore Control C checking */ -# endif - } -#endif /* CONFIG_PREBOOT */ - - if (autoconf_update_tftp()) - update_tftp(0UL); - -#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) : autoconf_bootdelay();
debug ("### main_loop entered: bootdelay=%d\n\n", bootdelay);
@@ -491,7 +432,69 @@ void main_loop (void) run_command_list(s, -1, 0); } #endif /* CONFIG_MENUKEY */ -#endif /* CONFIG_BOOTDELAY */ +} + +/****************************************************************************/ + +void main_loop(void) +{ +#ifndef CONFIG_SYS_HUSH_PARSER + static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, }; + int len; + int rc = 1; + int flag; +#endif +#ifdef CONFIG_PREBOOT + char *p; +#endif + + bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop"); + +#ifdef CONFIG_MODEM_SUPPORT + debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init); + if (do_mdm_init) { + char *str = strdup(getenv("mdm_cmd")); + setenv("preboot", str); /* set or delete definition */ + if (str != NULL) + free(str); + mdm_init(); /* wait for modem connection */ + } +#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 + p = getenv("preboot"); + if (p) { +# ifdef CONFIG_AUTOBOOT_KEYED + int prev = disable_ctrlc(1); /* disable Control C checking */ +# endif + + run_command_list(p, -1, 0); + +# ifdef CONFIG_AUTOBOOT_KEYED + disable_ctrlc(prev); /* restore Control C checking */ +# endif + } +#endif /* CONFIG_PREBOOT */ + + if (autoconf_update_tftp()) + update_tftp(0UL); + + if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) + process_boot_delay();
#if defined CONFIG_OF_CONTROL set_working_fdt_addr((void *)gd->fdt_blob);
HI Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Move this code into its own function, since it clutters up main_loop().
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 155 ++++++++++++++++++++++++++++++---------------------------- 1 file changed, 79 insertions(+), 76 deletions(-)
diff --git a/common/main.c b/common/main.c index 1e12e55..0df7992 100644 --- a/common/main.c +++ b/common/main.c @@ -91,7 +91,6 @@ extern void mdm_init(void); /* defined in board.c */
- Watch for 'delay' seconds for autoboot stop or autoboot delay string.
- returns: 0 - no key string, allow autoboot 1 - got key string, abort
*/ -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) static int abortboot_keyed(int bootdelay) { int abort = 0; @@ -273,7 +272,6 @@ static int abortboot(int bootdelay) else return abortboot_normal(bootdelay); } -#endif /* CONFIG_BOOTDELAY >= 0 */
/*
- Runs the given boot command securely. Specifically:
@@ -289,8 +287,7 @@ static int abortboot(int bootdelay)
- printing the error message to console.
*/
-#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
defined(CONFIG_OF_CONTROL)+#ifdef CONFIG_OF_CONTROL static void secure_boot_cmd(char *cmd) { cmd_tbl_t *cmdtp; @@ -331,46 +328,33 @@ static void process_fdt_options(const void *blob)
/* Add an env variable to point to a kernel payload, if available */ addr = fdtdec_get_config_int(gd->fdt_blob, "kernel-offset", 0);
if (addr)setenv_addr("kernaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr));
if (addr) {setenv_addr("kernaddr",(void *)(autoconf_sys_text_base() + addr));} /* Add an env variable to point to a root disk, if available */ addr = fdtdec_get_config_int(gd->fdt_blob, "rootdisk-offset", 0);
if (addr)setenv_addr("rootaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr));
if (addr) {setenv_addr("rootaddr",(void *)(autoconf_sys_text_base() + addr));}} #endif /* CONFIG_OF_CONTROL */
-/****************************************************************************/
-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_OF_CONTROL
char *env;+#endif
char *s;int bootdelay;#ifdef CONFIG_BOOTCOUNT_LIMIT bootcount = bootcount_load(); @@ -382,51 +366,8 @@ void main_loop (void) 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);-# ifdef CONFIG_AUTOBOOT_KEYED
disable_ctrlc(prev); /* restore Control C checking */-# endif
}-#endif /* CONFIG_PREBOOT */
if (autoconf_update_tftp())update_tftp(0UL);-#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) : autoconf_bootdelay(); debug ("### main_loop entered: bootdelay=%d\n\n", bootdelay);@@ -491,7 +432,69 @@ void main_loop (void) run_command_list(s, -1, 0); } #endif /* CONFIG_MENUKEY */ -#endif /* CONFIG_BOOTDELAY */ +}
+/****************************************************************************/
+void main_loop(void) +{ +#ifndef CONFIG_SYS_HUSH_PARSER
static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };int len;int rc = 1;int flag;+#endif +#ifdef CONFIG_PREBOOT
char *p;+#endif
bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");+#ifdef CONFIG_MODEM_SUPPORT
debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);if (do_mdm_init) {char *str = strdup(getenv("mdm_cmd"));setenv("preboot", str); /* set or delete definition */if (str != NULL)free(str);mdm_init(); /* wait for modem connection */}+#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
p = getenv("preboot");if (p) {+# ifdef CONFIG_AUTOBOOT_KEYED
int prev = disable_ctrlc(1); /* disable Control C checking */+# endif
run_command_list(p, -1, 0);+# ifdef CONFIG_AUTOBOOT_KEYED
disable_ctrlc(prev); /* restore Control C checking */+# endif
}+#endif /* CONFIG_PREBOOT */
if (autoconf_update_tftp())update_tftp(0UL);if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0)process_boot_delay();#if defined CONFIG_OF_CONTROL set_working_fdt_addr((void *)gd->fdt_blob); -- 1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Change this feature to use autoconf instead of #ifdef.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 74 ++++++++++++++++++++++++++--------------------------------- 1 file changed, 33 insertions(+), 41 deletions(-)
diff --git a/common/main.c b/common/main.c index 0df7992..c00c5bd 100644 --- a/common/main.c +++ b/common/main.c @@ -71,17 +71,11 @@ static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen); static const char erase_seq[] = "\b \b"; /* erase sequence */ static const char tab_seq[] = " "; /* used to expand TABs */
-#ifdef CONFIG_BOOT_RETRY_TIME static uint64_t endtime = 0; /* must be set, default is instant timeout */ static int retry_time = -1; /* -1 so can call readline before main_loop */ -#endif
#define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk())
-#ifndef CONFIG_BOOT_RETRY_MIN -#define CONFIG_BOOT_RETRY_MIN CONFIG_BOOT_RETRY_TIME -#endif - #ifdef CONFIG_MODEM_SUPPORT int do_mdm_init = 0; extern void mdm_init(void); /* defined in board.c */ @@ -181,11 +175,10 @@ static int abortboot_keyed(int bootdelay) delaykey[i].retry ? "delay" : "stop"); # endif
-# ifdef CONFIG_BOOT_RETRY_TIME - /* don't retry auto boot */ - if (! delaykey[i].retry) + /* don't retry auto boot? */ + if (autoconf_boot_retry_time() && + !delaykey[i].retry) retry_time = -1; -# endif abort = 1; } } @@ -374,9 +367,8 @@ static void process_boot_delay(void) #if defined(CONFIG_MENU_SHOW) bootdelay = menu_show(bootdelay); #endif -# ifdef CONFIG_BOOT_RETRY_TIME - init_cmd_timeout (); -# endif /* CONFIG_BOOT_RETRY_TIME */ + if (autoconf_boot_retry_time()) + init_cmd_timeout();
#ifdef CONFIG_POST if (gd->flags & GD_FLG_POSTFAIL) { @@ -509,14 +501,12 @@ void main_loop(void) for (;;); #else for (;;) { -#ifdef CONFIG_BOOT_RETRY_TIME - if (rc >= 0) { + if (autoconf_boot_retry_time() && rc >= 0) { /* Saw enough of a valid command to * restart the timeout. */ reset_cmd_timeout(); } -#endif len = readline (CONFIG_SYS_PROMPT);
flag = 0; /* assume no special flags for now */ @@ -524,19 +514,16 @@ void main_loop(void) strcpy (lastcommand, console_buffer); else if (len == 0) flag |= CMD_FLAG_REPEAT; -#ifdef CONFIG_BOOT_RETRY_TIME - else if (len == -2) { + else if (autoconf_boot_retry_time() && len == -2) { /* -2 means timed out, retry autoboot */ - puts ("\nTimed out waiting for command\n"); -# ifdef CONFIG_RESET_TO_RETRY + puts("\nTimed out waiting for command\n"); /* Reinit board to run initialization code again */ - do_reset (NULL, 0, 0, NULL); -# else - return; /* retry autoboot */ -# endif + if (autoconf_reset_to_retry()) + do_reset(NULL, 0, 0, NULL); + else + return; /* retry autoboot */ } -#endif
if (len == -1) puts ("<INTERRUPT>\n"); @@ -551,6 +538,10 @@ void main_loop(void) #endif /*CONFIG_SYS_HUSH_PARSER*/ }
+/* + * Use ifdef here for the benefit of those archs not using + * -ffunction-sections, since these functions are exported. + */ #ifdef CONFIG_BOOT_RETRY_TIME /*************************************************************************** * initialize command line timeout @@ -558,14 +549,15 @@ 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; + retry_time = autoconf_boot_retry_time();
- if (retry_time >= 0 && retry_time < CONFIG_BOOT_RETRY_MIN) - retry_time = CONFIG_BOOT_RETRY_MIN; + if (retry_time >= 0 retry_time < autoconf_boot_retry_min()) + retry_time = autoconf_boot_retry_min(); }
/*************************************************************************** @@ -787,13 +779,13 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, cread_add_str(buf, init_len, 1, &num, &eol_num, buf, *len);
while (1) { -#ifdef CONFIG_BOOT_RETRY_TIME - while (!tstc()) { /* while no incoming data */ - if (retry_time >= 0 && get_ticks() > endtime) - return (-2); /* timed out */ - WATCHDOG_RESET(); + if (autoconf_boot_retry_time()) { + while (!tstc()) { /* while no incoming data */ + if (retry_time >= 0 && get_ticks() > endtime) + return -2; /* timed out */ + WATCHDOG_RESET(); + } } -#endif if (first && timeout) { uint64_t etime = endtick(timeout);
@@ -1065,14 +1057,14 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) col = plen;
for (;;) { -#ifdef CONFIG_BOOT_RETRY_TIME - while (!tstc()) { /* while no incoming data */ - if (retry_time >= 0 && get_ticks() > endtime) - return (-2); /* timed out */ - WATCHDOG_RESET(); + if (autoconf_boot_retry_time()) { + while (!tstc()) { /* while no incoming data */ + if (retry_time >= 0 && get_ticks() > endtime) + return -2; /* timed out */ + WATCHDOG_RESET(); + } } -#endif - WATCHDOG_RESET(); /* Trigger watchdog, if needed */ + WATCHDOG_RESET(); /* Trigger watchdog, if needed */
#ifdef CONFIG_SHOW_ACTIVITY while (!tstc()) {
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Change this feature to use autoconf instead of #ifdef.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 74 ++++++++++++++++++++++++++--------------------------------- 1 file changed, 33 insertions(+), 41 deletions(-)
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Move this code over to using autoconf. We can add the autoconf values to the delaykey[] array, and move the code that checks for autoconf values into the loop.
Also change to using ARRAY_SIZE on delaykey[].
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 86 ++++++++++++++++++++++------------------------------------- 1 file changed, 32 insertions(+), 54 deletions(-)
diff --git a/common/main.c b/common/main.c index c00c5bd..37e42ca 100644 --- a/common/main.c +++ b/common/main.c @@ -90,15 +90,20 @@ static int abortboot_keyed(int bootdelay) int abort = 0; uint64_t etime = endtick(bootdelay); struct { - char* str; + const char *str; u_int len; int retry; + const char *conf; /* Configuration value */ } delaykey [] = { - { str: getenv ("bootdelaykey"), retry: 1 }, - { str: getenv ("bootdelaykey2"), retry: 1 }, - { str: getenv ("bootstopkey"), retry: 0 }, - { str: getenv ("bootstopkey2"), retry: 0 }, + { str: getenv("bootdelaykey"), retry: 1, + conf: autoconf_autoboot_delay_str() }, + { str: getenv("bootdelaykey2"), retry: 1, + conf: autoconf_autoboot_delay_str2() }, + { str: getenv("bootstopkey"), retry: 0, + conf: autoconf_autoboot_stop_str() }, + { str: getenv("bootstopkey2"), retry: 0, + conf: autoconf_autoboot_stop_str2() }, };
char presskey [MAX_DELAY_STOP_STR]; @@ -106,33 +111,15 @@ static int abortboot_keyed(int bootdelay) u_int presskey_max = 0; u_int i;
-#ifndef CONFIG_ZERO_BOOTDELAY_CHECK - if (bootdelay == 0) + if (!autoconf_zero_bootdelay_check() && bootdelay == 0) return 0; -#endif - -# ifdef CONFIG_AUTOBOOT_PROMPT - printf(CONFIG_AUTOBOOT_PROMPT); -# endif
-# ifdef CONFIG_AUTOBOOT_DELAY_STR - if (delaykey[0].str == NULL) - delaykey[0].str = CONFIG_AUTOBOOT_DELAY_STR; -# endif -# ifdef CONFIG_AUTOBOOT_DELAY_STR2 - if (delaykey[1].str == NULL) - delaykey[1].str = CONFIG_AUTOBOOT_DELAY_STR2; -# endif -# ifdef CONFIG_AUTOBOOT_STOP_STR - if (delaykey[2].str == NULL) - delaykey[2].str = CONFIG_AUTOBOOT_STOP_STR; -# endif -# ifdef CONFIG_AUTOBOOT_STOP_STR2 - if (delaykey[3].str == NULL) - delaykey[3].str = CONFIG_AUTOBOOT_STOP_STR2; -# endif + if (autoconf_has_autoboot_prompt()) + printf(autoconf_autoboot_prompt());
- for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) { + for (i = 0; i < ARRAY_SIZE(delaykey); i++) { + if (delaykey[i].conf && !delaykey[i].str) + delaykey[i].str = delaykey[i].conf; delaykey[i].len = delaykey[i].str == NULL ? 0 : strlen (delaykey[i].str); delaykey[i].len = delaykey[i].len > MAX_DELAY_STOP_STR ? @@ -164,7 +151,7 @@ static int abortboot_keyed(int bootdelay) } }
- for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) { + for (i = 0; i < ARRAY_SIZE(delaykey); i++) { if (delaykey[i].len > 0 && presskey_len >= delaykey[i].len && memcmp (presskey + presskey_len - delaykey[i].len, @@ -189,43 +176,37 @@ static int abortboot_keyed(int bootdelay) puts("key timeout\n"); # endif
-#ifdef CONFIG_SILENT_CONSOLE - if (abort) + if (autoconf_silent_console() && abort) gd->flags &= ~GD_FLG_SILENT; -#endif
return abort; }
-#ifdef CONFIG_MENUKEY static int menukey = 0; -#endif
static int abortboot_normal(int bootdelay) { int abort = 0; unsigned long ts;
-#ifdef CONFIG_MENUPROMPT - printf(CONFIG_MENUPROMPT); -#else - if (bootdelay >= 0) + if (autoconf_menuprompt()) + printf(autoconf_menuprompt()); + else if (bootdelay >= 0) printf("Hit any key to stop autoboot: %2d ", bootdelay); -#endif
-#if defined CONFIG_ZERO_BOOTDELAY_CHECK /* - * Check if key already pressed - * Don't check if bootdelay < 0 + * If we need to do a bootdelay check even if bootdelay is 0, do + * it here, since the loop below will be skipped in this case. + * We don't do this check if bootdelay < 0. */ - if (bootdelay >= 0) { - if (tstc()) { /* we got a key press */ + if (autoconf_zero_bootdelay_check() && bootdelay >= 0) { + /* Check if key already pressed */ + if (tstc()) { /* we got a key press */ (void) getc(); /* consume input */ puts ("\b\b\b 0"); abort = 1; /* don't auto boot */ } } -#endif
while ((bootdelay > 0) && (!abort)) { --bootdelay; @@ -235,11 +216,10 @@ static int abortboot_normal(int bootdelay) if (tstc()) { /* we got a key press */ abort = 1; /* don't auto boot */ bootdelay = 0; /* no more delay */ -# ifdef CONFIG_MENUKEY - menukey = getc(); -# else - (void) getc(); /* consume input */ -# endif + if (autoconf_menukey()) + menukey = getc(); + else + (void) getc(); /* consume input */ break; } udelay(10000); @@ -250,10 +230,8 @@ static int abortboot_normal(int bootdelay)
putc('\n');
-#ifdef CONFIG_SILENT_CONSOLE - if (abort) + if (autoconf_silent_console() && abort) gd->flags &= ~GD_FLG_SILENT; -#endif
return abort; }
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Move this code over to using autoconf. We can add the autoconf values to the delaykey[] array, and move the code that checks for autoconf values into the loop.
Also change to using ARRAY_SIZE on delaykey[].
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 86 ++++++++++++++++++++++------------------------------------- 1 file changed, 32 insertions(+), 54 deletions(-)
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
These functions are now available, so use them to avoid extra code here.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/common/main.c b/common/main.c index 37e42ca..4f11e58 100644 --- a/common/main.c +++ b/common/main.c @@ -318,8 +318,6 @@ static void process_boot_delay(void) #ifdef CONFIG_BOOTCOUNT_LIMIT unsigned long bootcount = 0; unsigned long bootlimit = 0; - char *bcs; - char bcs_set[16]; #endif /* CONFIG_BOOTCOUNT_LIMIT */ #ifdef CONFIG_OF_CONTROL char *env; @@ -331,10 +329,8 @@ static void process_boot_delay(void) 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; + setenv_ulong("bootcount", bootcount); + bootlimit = getenv_ulong("bootlimit", 10, 0); #endif /* CONFIG_BOOTCOUNT_LIMIT */
s = getenv ("bootdelay");
Hi Simon.
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
These functions are now available, so use them to avoid extra code here.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Convert this function and its children to use autoconf instead of #ifdef.
Some header files must now be included unconditionally, so remove some of the #ifdefs from the header section, and put these header files into the right order.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 104 +++++++++++++++++++++++---------------------------------- include/menu.h | 2 -- 2 files changed, 41 insertions(+), 65 deletions(-)
diff --git a/common/main.c b/common/main.c index 4f11e58..a5d3f82 100644 --- a/common/main.c +++ b/common/main.c @@ -31,27 +31,17 @@ #include <watchdog.h> #include <command.h> #include <fdtdec.h> +#include <fdt_support.h> #include <malloc.h> +#include <menu.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 <post.h> #include <linux/ctype.h> -#include <menu.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -258,7 +248,6 @@ static int abortboot(int bootdelay) * printing the error message to console. */
-#ifdef CONFIG_OF_CONTROL static void secure_boot_cmd(char *cmd) { cmd_tbl_t *cmdtp; @@ -311,93 +300,82 @@ static void process_fdt_options(const void *blob) (void *)(autoconf_sys_text_base() + addr)); } } -#endif /* CONFIG_OF_CONTROL */
static void process_boot_delay(void) { -#ifdef CONFIG_BOOTCOUNT_LIMIT unsigned long bootcount = 0; unsigned long bootlimit = 0; -#endif /* CONFIG_BOOTCOUNT_LIMIT */ -#ifdef CONFIG_OF_CONTROL - char *env; -#endif - char *s; + const char *s; int bootdelay;
-#ifdef CONFIG_BOOTCOUNT_LIMIT - bootcount = bootcount_load(); - bootcount++; - bootcount_store (bootcount); - setenv_ulong("bootcount", bootcount); - bootlimit = getenv_ulong("bootlimit", 10, 0); -#endif /* CONFIG_BOOTCOUNT_LIMIT */ + if (autoconf_bootcount_limit()) { + bootcount = bootcount_load(); + bootcount++; + bootcount_store(bootcount); + setenv_ulong("bootcount", bootcount); + bootlimit = getenv_ulong("bootlimit", 10, 0); + }
s = getenv ("bootdelay"); bootdelay = s ? (int)simple_strtol(s, NULL, 10) : autoconf_bootdelay();
debug ("### main_loop entered: bootdelay=%d\n\n", bootdelay);
-#if defined(CONFIG_MENU_SHOW) - bootdelay = menu_show(bootdelay); -#endif + if (autoconf_menu_show()) + bootdelay = menu_show(bootdelay); if (autoconf_boot_retry_time()) init_cmd_timeout();
-#ifdef CONFIG_POST - if (gd->flags & GD_FLG_POSTFAIL) { + if (autoconf_post() && (gd->flags & GD_FLG_POSTFAIL)) { s = getenv("failbootcmd"); - } - else -#endif /* CONFIG_POST */ -#ifdef CONFIG_BOOTCOUNT_LIMIT - if (bootlimit && (bootcount > bootlimit)) { + } else if (autoconf_bootcount_limit() && bootlimit && + (bootcount > bootlimit)) { printf ("Warning: Bootlimit (%u) exceeded. Using altbootcmd.\n", (unsigned)bootlimit); s = getenv ("altbootcmd"); - } - else -#endif /* CONFIG_BOOTCOUNT_LIMIT */ + } else { s = getenv ("bootcmd"); -#ifdef CONFIG_OF_CONTROL - /* Allow the fdt to override the boot command */ - env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd"); - if (env) - s = env; + } + if (autoconf_of_control()) { + char *env;
- process_fdt_options(gd->fdt_blob); + /* Allow the fdt to override the boot command */ + env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd"); + if (env) + s = env;
- /* - * If the bootsecure option was chosen, use secure_boot_cmd(). - * Always use 'env' in this case, since bootsecure requres that the - * bootcmd was specified in the FDT too. - */ - if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0)) - secure_boot_cmd(env); + process_fdt_options(gd->fdt_blob);
-#endif /* CONFIG_OF_CONTROL */ + /* + * If the bootsecure option was chosen, use secure_boot_cmd(). + * Always use 'env' in this case, since bootsecure requres that + * the bootcmd was specified in the FDT too. + */ + if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0)) + secure_boot_cmd(env); + }
debug ("### main_loop: bootcmd="%s"\n", s ? s : "<UNDEFINED>");
if (bootdelay != -1 && s && !abortboot(bootdelay)) { -# ifdef CONFIG_AUTOBOOT_KEYED - int prev = disable_ctrlc(1); /* disable Control C checking */ -# endif + int prev; + + /* disable Control C checking */ + if (autoconf_autoboot_keyed()) + prev = disable_ctrlc(1);
run_command_list(s, -1, 0);
-# ifdef CONFIG_AUTOBOOT_KEYED - disable_ctrlc(prev); /* restore Control C checking */ -# endif + /* restore Control C checking */ + if (autoconf_autoboot_keyed()) + disable_ctrlc(prev); }
-# ifdef CONFIG_MENUKEY - if (menukey == CONFIG_MENUKEY) { + if (autoconf_menukey() && menukey == autoconf_menukey()) { s = getenv("menucmd"); if (s) run_command_list(s, -1, 0); } -#endif /* CONFIG_MENUKEY */ }
/****************************************************************************/ 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__ */
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Convert this function and its children to use autoconf instead of #ifdef.
Some header files must now be included unconditionally, so remove some of the #ifdefs from the header section, and put these header files into the right order.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 104 +++++++++++++++++++++++---------------------------------- include/menu.h | 2 -- 2 files changed, 41 insertions(+), 65 deletions(-)
diff --git a/common/main.c b/common/main.c index 4f11e58..a5d3f82 100644 --- a/common/main.c +++ b/common/main.c @@ -31,27 +31,17 @@ #include <watchdog.h> #include <command.h> #include <fdtdec.h> +#include <fdt_support.h> #include <malloc.h> +#include <menu.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 <post.h> #include <linux/ctype.h> -#include <menu.h>
DECLARE_GLOBAL_DATA_PTR;
@@ -258,7 +248,6 @@ static int abortboot(int bootdelay)
- printing the error message to console.
*/
-#ifdef CONFIG_OF_CONTROL static void secure_boot_cmd(char *cmd) { cmd_tbl_t *cmdtp; @@ -311,93 +300,82 @@ static void process_fdt_options(const void *blob) (void *)(autoconf_sys_text_base() + addr)); } } -#endif /* CONFIG_OF_CONTROL */
static void process_boot_delay(void) { -#ifdef CONFIG_BOOTCOUNT_LIMIT unsigned long bootcount = 0; unsigned long bootlimit = 0; -#endif /* CONFIG_BOOTCOUNT_LIMIT */ -#ifdef CONFIG_OF_CONTROL
char *env;-#endif
char *s;
const char *s; int bootdelay;-#ifdef CONFIG_BOOTCOUNT_LIMIT
bootcount = bootcount_load();bootcount++;bootcount_store (bootcount);setenv_ulong("bootcount", bootcount);bootlimit = getenv_ulong("bootlimit", 10, 0);-#endif /* CONFIG_BOOTCOUNT_LIMIT */
if (autoconf_bootcount_limit()) {bootcount = bootcount_load();bootcount++;bootcount_store(bootcount);setenv_ulong("bootcount", bootcount);bootlimit = getenv_ulong("bootlimit", 10, 0);} s = getenv ("bootdelay"); bootdelay = s ? (int)simple_strtol(s, NULL, 10) : autoconf_bootdelay();
This would look a lot nicer as: bootdelay = getenv_ulong_def("bootdelay", autoconf_bootdelay());
debug ("### main_loop entered: bootdelay=%d\n\n", bootdelay);-#if defined(CONFIG_MENU_SHOW)
bootdelay = menu_show(bootdelay);-#endif
if (autoconf_menu_show())bootdelay = menu_show(bootdelay); if (autoconf_boot_retry_time()) init_cmd_timeout();-#ifdef CONFIG_POST
if (gd->flags & GD_FLG_POSTFAIL) {
if (autoconf_post() && (gd->flags & GD_FLG_POSTFAIL)) { s = getenv("failbootcmd");
}else-#endif /* CONFIG_POST */ -#ifdef CONFIG_BOOTCOUNT_LIMIT
if (bootlimit && (bootcount > bootlimit)) {
} else if (autoconf_bootcount_limit() && bootlimit &&(bootcount > bootlimit)) { printf ("Warning: Bootlimit (%u) exceeded. Using altbootcmd.\n", (unsigned)bootlimit); s = getenv ("altbootcmd");
}else-#endif /* CONFIG_BOOTCOUNT_LIMIT */
} else { s = getenv ("bootcmd");-#ifdef CONFIG_OF_CONTROL
/* Allow the fdt to override the boot command */env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");if (env)s = env;
}if (autoconf_of_control()) {char *env;
process_fdt_options(gd->fdt_blob);
/* Allow the fdt to override the boot command */env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd");if (env)s = env;
/** If the bootsecure option was chosen, use secure_boot_cmd().* Always use 'env' in this case, since bootsecure requres that the* bootcmd was specified in the FDT too.*/if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0))secure_boot_cmd(env);
process_fdt_options(gd->fdt_blob);-#endif /* CONFIG_OF_CONTROL */
/** If the bootsecure option was chosen, use secure_boot_cmd().* Always use 'env' in this case, since bootsecure requres that* the bootcmd was specified in the FDT too.*/if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0))secure_boot_cmd(env);} debug ("### main_loop: bootcmd=\"%s\"\n", s ? s : "<UNDEFINED>"); if (bootdelay != -1 && s && !abortboot(bootdelay)) {-# ifdef CONFIG_AUTOBOOT_KEYED
int prev = disable_ctrlc(1); /* disable Control C checking */-# endif
int prev;/* disable Control C checking */if (autoconf_autoboot_keyed())prev = disable_ctrlc(1); run_command_list(s, -1, 0);-# ifdef CONFIG_AUTOBOOT_KEYED
disable_ctrlc(prev); /* restore Control C checking */-# endif
/* restore Control C checking */if (autoconf_autoboot_keyed())disable_ctrlc(prev); }-# ifdef CONFIG_MENUKEY
if (menukey == CONFIG_MENUKEY) {
if (autoconf_menukey() && menukey == autoconf_menukey()) { s = getenv("menucmd"); if (s) run_command_list(s, -1, 0); }-#endif /* CONFIG_MENUKEY */ }
/****************************************************************************/ 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__ */
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Hi Joe,
On Sun, Feb 24, 2013 at 12:40 PM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Convert this function and its children to use autoconf instead of #ifdef.
Some header files must now be included unconditionally, so remove some of the #ifdefs from the header section, and put these header files into the right order.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 104 +++++++++++++++++++++++---------------------------------- include/menu.h | 2 -- 2 files changed, 41 insertions(+), 65 deletions(-)
diff --git a/common/main.c b/common/main.c index 4f11e58..a5d3f82 100644 --- a/common/main.c +++ b/common/main.c
[snip]
@@ -258,7 +248,6 @@ static int abortboot(int bootdelay)
- printing the error message to console.
*/
-#ifdef CONFIG_OF_CONTROL static void secure_boot_cmd(char *cmd) { cmd_tbl_t *cmdtp; @@ -311,93 +300,82 @@ static void process_fdt_options(const void *blob) (void *)(autoconf_sys_text_base() + addr)); } } -#endif /* CONFIG_OF_CONTROL */
static void process_boot_delay(void) { -#ifdef CONFIG_BOOTCOUNT_LIMIT unsigned long bootcount = 0; unsigned long bootlimit = 0; -#endif /* CONFIG_BOOTCOUNT_LIMIT */ -#ifdef CONFIG_OF_CONTROL
char *env;-#endif
char *s;
const char *s; int bootdelay;-#ifdef CONFIG_BOOTCOUNT_LIMIT
bootcount = bootcount_load();bootcount++;bootcount_store (bootcount);setenv_ulong("bootcount", bootcount);bootlimit = getenv_ulong("bootlimit", 10, 0);-#endif /* CONFIG_BOOTCOUNT_LIMIT */
if (autoconf_bootcount_limit()) {bootcount = bootcount_load();bootcount++;bootcount_store(bootcount);setenv_ulong("bootcount", bootcount);bootlimit = getenv_ulong("bootlimit", 10, 0);} s = getenv ("bootdelay"); bootdelay = s ? (int)simple_strtol(s, NULL, 10) : autoconf_bootdelay();This would look a lot nicer as: bootdelay = getenv_ulong_def("bootdelay", autoconf_bootdelay());
Yes of course, thank you.
[snip]
Regards, Simon
Allow parser selection to make use of autoconf instead of #ifdefs. This requires us to make header includes unconditional, but this is simpler anyway.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 92 +++++++++++++++++++++++++++------------------------------- include/hush.h | 2 -- 2 files changed, 42 insertions(+), 52 deletions(-)
diff --git a/common/main.c b/common/main.c index a5d3f82..e1483db 100644 --- a/common/main.c +++ b/common/main.c @@ -32,14 +32,11 @@ #include <command.h> #include <fdtdec.h> #include <fdt_support.h> +#include <hush.h> #include <malloc.h> #include <menu.h> #include <version.h>
-#ifdef CONFIG_SYS_HUSH_PARSER -#include <hush.h> -#endif - #include <post.h> #include <linux/ctype.h>
@@ -382,12 +379,10 @@ static void process_boot_delay(void)
void main_loop(void) { -#ifndef CONFIG_SYS_HUSH_PARSER static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, }; int len; int rc = 1; int flag; -#endif #ifdef CONFIG_PREBOOT char *p; #endif @@ -447,12 +442,11 @@ void main_loop(void) /* * Main Loop for Monitor Command Processing */ -#ifdef CONFIG_SYS_HUSH_PARSER - parse_file_outer(); - /* This point is never reached */ - for (;;); -#else - for (;;) { + if (autoconf_sys_hush_parser()) { + parse_file_outer(); + /* This point is never reached */ + for (;;); + } else { if (autoconf_boot_retry_time() && rc >= 0) { /* Saw enough of a valid command to * restart the timeout. @@ -487,7 +481,6 @@ void main_loop(void) lastcommand[0] = 0; } } -#endif /*CONFIG_SYS_HUSH_PARSER*/ }
/* @@ -1186,7 +1179,6 @@ int parse_line (char *line, char *argv[])
/****************************************************************************/
-#ifndef CONFIG_SYS_HUSH_PARSER static void process_macros (const char *input, char *output) { char c, prev; @@ -1402,7 +1394,6 @@ static int builtin_run_command(const char *cmd, int flag)
return rc ? rc : repeatable; } -#endif
/* * Run a command using the selected parser. @@ -1413,22 +1404,21 @@ static int builtin_run_command(const char *cmd, int flag) */ int run_command(const char *cmd, int flag) { -#ifndef CONFIG_SYS_HUSH_PARSER - /* - * builtin_run_command can return 0 or 1 for success, so clean up - * its result. - */ - if (builtin_run_command(cmd, flag) == -1) - return 1; - - return 0; -#else - return parse_string_outer(cmd, + if (autoconf_sys_hush_parser()) { + return parse_string_outer(cmd, FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP); -#endif + } else { + /* + * builtin_run_command can return 0 or 1 for success, so + * clean up its result. + */ + if (builtin_run_command(cmd, flag) == -1) + return 1; + + return 0; + } }
-#ifndef CONFIG_SYS_HUSH_PARSER /** * Execute a list of command separated by ; or \n using the built-in parser. * @@ -1469,7 +1459,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) { @@ -1479,13 +1468,16 @@ int run_command_list(const char *cmd, int len, int flag)
if (len == -1) { len = strlen(cmd); -#ifdef CONFIG_SYS_HUSH_PARSER - /* hush will never change our string */ - need_buff = 0; -#else - /* the built-in parser will change our string if it sees \n */ - need_buff = strchr(cmd, '\n') != NULL; -#endif + if (autoconf_sys_hush_parser()) { + /* hush will never change our string */ + need_buff = 0; + } else { + /* + * the built-in parser will change our string if it + * sees \n + */ + need_buff = strchr(cmd, '\n') != NULL; + } } if (need_buff) { buff = malloc(len + 1); @@ -1494,20 +1486,20 @@ int run_command_list(const char *cmd, int len, int flag) memcpy(buff, cmd, len); buff[len] = '\0'; } -#ifdef CONFIG_SYS_HUSH_PARSER - rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON); -#else - /* - * This function will overwrite any \n it sees with a \0, which - * is why it can't work with a const char *. Here we are making - * using of internal knowledge of this function, to avoid always - * doing a malloc() which is actually required only in a case that - * is pretty rare. - */ - rcode = builtin_run_command_list(buff, flag); - if (need_buff) - free(buff); -#endif + if (autoconf_sys_hush_parser()) { + rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON); + } else { + /* + * This function will overwrite any \n it sees with a \0, which + * is why it can't work with a const char *. Here we are making + * using of internal knowledge of this function, to avoid always + * doing a malloc() which is actually required only in a case + * that is pretty rare. + */ + rcode = builtin_run_command_list(buff, flag); + if (need_buff) + free(buff); + }
return rcode; } diff --git a/include/hush.h b/include/hush.h index ecf9222..12c55f4 100644 --- a/include/hush.h +++ b/include/hush.h @@ -36,7 +36,5 @@ int set_local_var(const char *s, int flg_export); void unset_local_var(const char *name); char *get_local_var(const char *s);
-#if defined(CONFIG_HUSH_INIT_VAR) extern int hush_init_var (void); #endif -#endif
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Allow parser selection to make use of autoconf instead of #ifdefs. This requires us to make header includes unconditional, but this is simpler anyway.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 92 +++++++++++++++++++++++++++------------------------------- include/hush.h | 2 -- 2 files changed, 42 insertions(+), 52 deletions(-)
diff --git a/common/main.c b/common/main.c index a5d3f82..e1483db 100644 --- a/common/main.c +++ b/common/main.c @@ -32,14 +32,11 @@ #include <command.h> #include <fdtdec.h> #include <fdt_support.h> +#include <hush.h> #include <malloc.h> #include <menu.h> #include <version.h>
-#ifdef CONFIG_SYS_HUSH_PARSER -#include <hush.h> -#endif
#include <post.h> #include <linux/ctype.h>
@@ -382,12 +379,10 @@ static void process_boot_delay(void)
void main_loop(void) { -#ifndef CONFIG_SYS_HUSH_PARSER static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, }; int len; int rc = 1; int flag; -#endif #ifdef CONFIG_PREBOOT char *p; #endif @@ -447,12 +442,11 @@ void main_loop(void) /* * Main Loop for Monitor Command Processing */ -#ifdef CONFIG_SYS_HUSH_PARSER
parse_file_outer();/* This point is never reached */for (;;);-#else
for (;;) {
if (autoconf_sys_hush_parser()) {parse_file_outer();/* This point is never reached */for (;;);} else { if (autoconf_boot_retry_time() && rc >= 0) { /* Saw enough of a valid command to * restart the timeout.@@ -487,7 +481,6 @@ void main_loop(void) lastcommand[0] = 0; } } -#endif /*CONFIG_SYS_HUSH_PARSER*/ }
/* @@ -1186,7 +1179,6 @@ int parse_line (char *line, char *argv[])
/****************************************************************************/
-#ifndef CONFIG_SYS_HUSH_PARSER static void process_macros (const char *input, char *output) { char c, prev; @@ -1402,7 +1394,6 @@ static int builtin_run_command(const char *cmd, int flag)
return rc ? rc : repeatable;} -#endif
/*
- Run a command using the selected parser.
@@ -1413,22 +1404,21 @@ static int builtin_run_command(const char *cmd, int flag) */ int run_command(const char *cmd, int flag) { -#ifndef CONFIG_SYS_HUSH_PARSER
/** builtin_run_command can return 0 or 1 for success, so clean up* its result.*/if (builtin_run_command(cmd, flag) == -1)return 1;return 0;-#else
return parse_string_outer(cmd,
if (autoconf_sys_hush_parser()) {return parse_string_outer(cmd, FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP);-#endif
} else {/** builtin_run_command can return 0 or 1 for success, so* clean up its result.*/if (builtin_run_command(cmd, flag) == -1)return 1;return 0;}}
-#ifndef CONFIG_SYS_HUSH_PARSER /**
- Execute a list of command separated by ; or \n using the built-in parser.
@@ -1469,7 +1459,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) { @@ -1479,13 +1468,16 @@ int run_command_list(const char *cmd, int len, int flag)
if (len == -1) { len = strlen(cmd);-#ifdef CONFIG_SYS_HUSH_PARSER
/* hush will never change our string */need_buff = 0;-#else
/* the built-in parser will change our string if it sees \n */need_buff = strchr(cmd, '\n') != NULL;-#endif
if (autoconf_sys_hush_parser()) {/* hush will never change our string */need_buff = 0;} else {/** the built-in parser will change our string if it* sees \n*/need_buff = strchr(cmd, '\n') != NULL;} } if (need_buff) { buff = malloc(len + 1);@@ -1494,20 +1486,20 @@ int run_command_list(const char *cmd, int len, int flag) memcpy(buff, cmd, len); buff[len] = '\0'; } -#ifdef CONFIG_SYS_HUSH_PARSER
rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);-#else
/** This function will overwrite any \n it sees with a \0, which* is why it can't work with a const char *. Here we are making* using of internal knowledge of this function, to avoid always* doing a malloc() which is actually required only in a case that* is pretty rare.*/rcode = builtin_run_command_list(buff, flag);if (need_buff)free(buff);-#endif
if (autoconf_sys_hush_parser()) {rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON);} else {/** This function will overwrite any \n it sees with a \0, which* is why it can't work with a const char *. Here we are making* using of internal knowledge of this function, to avoid always* doing a malloc() which is actually required only in a case* that is pretty rare.*/rcode = builtin_run_command_list(buff, flag);if (need_buff)free(buff);} return rcode;} diff --git a/include/hush.h b/include/hush.h index ecf9222..12c55f4 100644 --- a/include/hush.h +++ b/include/hush.h @@ -36,7 +36,5 @@ int set_local_var(const char *s, int flg_export); void unset_local_var(const char *name); char *get_local_var(const char *s);
-#if defined(CONFIG_HUSH_INIT_VAR) extern int hush_init_var (void); #endif
-#endif
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Remove #ifdefs in favour of autoconf for this code. This involves removing a few unnecessary #ifdefs in headers also.
We have two versions of the code - one that handles command line editing and one that is just a simple implementation. Create a new function called readline_into_buffer() which calls either cread_line() or the new simple_readline(), created to hold the 'simple' code.
The cread_print_hist_list() function is not actually used anywhere, so punt it.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 186 ++++++++++++++++++++++++------------------------------ include/command.h | 2 - include/common.h | 2 - 3 files changed, 84 insertions(+), 106 deletions(-)
diff --git a/common/main.c b/common/main.c index e1483db..3966321 100644 --- a/common/main.c +++ b/common/main.c @@ -514,8 +514,6 @@ void reset_cmd_timeout(void) } #endif
-#ifdef CONFIG_CMDLINE_EDITING - /* * cmdline-editing related codes from vivi. * Author: Janghoon Lyu nandy@mizi.com @@ -618,27 +616,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); \ @@ -900,27 +877,27 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, REFRESH_TO_EOL(); continue; } -#ifdef CONFIG_AUTO_COMPLETE - case '\t': { - int num2, col; + case '\t': + if (autoconf_auto_complete()) { + int num2, col;
- /* do not autocomplete when in the middle */ - if (num < eol_num) { - getcmd_cbeep(); - break; - } + /* do not autocomplete when in the middle */ + if (num < eol_num) { + getcmd_cbeep(); + break; + }
- buf[num] = '\0'; - col = strlen(prompt) + eol_num; - num2 = num; - if (cmd_auto_complete(prompt, buf, &num2, &col)) { - col = num2 - num; - num += col; - eol_num += col; + buf[num] = '\0'; + col = strlen(prompt) + eol_num; + num2 = num; + if (cmd_auto_complete(prompt, buf, &num2, + &col)) { + col = num2 - num; + num += col; + eol_num += col; + } + break; } - break; - } -#endif default: cread_add_char(ichar, insert, &num, &eol_num, buf, *len); break; @@ -936,8 +913,6 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, return 0; }
-#endif /* CONFIG_CMDLINE_EDITING */ - /****************************************************************************/
/* @@ -959,46 +934,14 @@ int readline (const char *const prompt) return readline_into_buffer(prompt, console_buffer, 0); }
- -int readline_into_buffer(const char *const prompt, char *buffer, int timeout) +static int simple_readline(const char *const prompt, int plen, char *p, + int timeout) { - char *p = buffer; -#ifdef CONFIG_CMDLINE_EDITING - unsigned int len = CONFIG_SYS_CBSIZE; - int rc; - static int initted = 0; - - /* - * History uses a global array which is not - * writable until after relocation to RAM. - * Revert to non-history version if still - * running from flash. - */ - if (gd->flags & GD_FLG_RELOC) { - if (!initted) { - hist_init(); - initted = 1; - } - - if (prompt) - puts (prompt); - - rc = cread_line(prompt, p, &len, timeout); - return rc < 0 ? rc : len; - - } else { -#endif /* CONFIG_CMDLINE_EDITING */ char * p_buf = p; int n = 0; /* buffer index */ - int plen = 0; /* prompt length */ int col; /* output column cnt */ char c;
- /* print prompt */ - if (prompt) { - plen = strlen (prompt); - puts (prompt); - } col = plen;
for (;;) { @@ -1011,32 +954,32 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) } WATCHDOG_RESET(); /* Trigger watchdog, if needed */
-#ifdef CONFIG_SHOW_ACTIVITY - while (!tstc()) { - show_activity(0); - WATCHDOG_RESET(); + if (autoconf_show_activity()) { + while (!tstc()) { + show_activity(0); + WATCHDOG_RESET(); + } } -#endif c = getc();
/* * 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; @@ -1045,15 +988,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;
@@ -1062,22 +1005,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 (autoconf_auto_complete()) { + /* + * if auto completion triggered + * just continue + */ + *p = '\0'; + if (cmd_auto_complete(prompt, + console_buffer, + &n, &col)) { + /* reset */ + p = p_buf + n; + continue; + } } -#endif puts (tab_seq+(col&07)); col += 8 - (col&07); } else { 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; @@ -1092,9 +1041,42 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) } } } -#ifdef CONFIG_CMDLINE_EDITING +} + +int readline_into_buffer(const char *const prompt, char *buffer, int timeout) +{ + unsigned int len = CONFIG_SYS_CBSIZE; + int rc; + static int initted; + + /* + * History uses a global array which is not + * writable until after relocation to RAM. + * Revert to non-history version if still + * running from flash. + */ + if (autoconf_cmdline_editing() && (gd->flags & GD_FLG_RELOC)) { + if (!initted) { + hist_init(); + initted = 1; + } + + if (prompt) + puts(prompt); + + rc = cread_line(prompt, buffer, &len, timeout); + return rc < 0 ? rc : len; + + } else { + int plen = 0; /* prompt length */ + + /* print prompt */ + if (prompt) { + plen = strlen(prompt); + puts(prompt); + } + return simple_readline(prompt, plen, buffer, timeout); } -#endif }
/****************************************************************************/ diff --git a/include/command.h b/include/command.h index 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 fb219fd..1457349 100644 --- a/include/common.h +++ b/include/common.h @@ -857,9 +857,7 @@ int pcmcia_init (void);
#include <bootstage.h>
-#ifdef CONFIG_SHOW_ACTIVITY void show_activity(int arg); -#endif
/* Multicore arch functions */ #ifdef CONFIG_MP
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Remove #ifdefs in favour of autoconf for this code. This involves removing a few unnecessary #ifdefs in headers also.
We have two versions of the code - one that handles command line editing and one that is just a simple implementation. Create a new function called readline_into_buffer() which calls either cread_line() or the new simple_readline(), created to hold the 'simple' code.
The cread_print_hist_list() function is not actually used anywhere, so punt it.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 186 ++++++++++++++++++++++++------------------------------ include/command.h | 2 - include/common.h | 2 - 3 files changed, 84 insertions(+), 106 deletions(-)
diff --git a/common/main.c b/common/main.c index e1483db..3966321 100644 --- a/common/main.c +++ b/common/main.c @@ -514,8 +514,6 @@ void reset_cmd_timeout(void) } #endif
-#ifdef CONFIG_CMDLINE_EDITING
/*
- cmdline-editing related codes from vivi.
- Author: Janghoon Lyu nandy@mizi.com
@@ -618,27 +616,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); \ @@ -900,27 +877,27 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, REFRESH_TO_EOL(); continue; } -#ifdef CONFIG_AUTO_COMPLETE
case '\t': {int num2, col;
case '\t':if (autoconf_auto_complete()) {int num2, col;
/* do not autocomplete when in the middle */if (num < eol_num) {getcmd_cbeep();break;}
/* do not autocomplete when in the middle */if (num < eol_num) {getcmd_cbeep();break;}
buf[num] = '\0';col = strlen(prompt) + eol_num;num2 = num;if (cmd_auto_complete(prompt, buf, &num2, &col)) {col = num2 - num;num += col;eol_num += col;
buf[num] = '\0';col = strlen(prompt) + eol_num;num2 = num;if (cmd_auto_complete(prompt, buf, &num2,&col)) {col = num2 - num;num += col;eol_num += col;}break; }
break;}-#endif default: cread_add_char(ichar, insert, &num, &eol_num, buf, *len); break; @@ -936,8 +913,6 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, return 0; }
-#endif /* CONFIG_CMDLINE_EDITING */
/****************************************************************************/
/* @@ -959,46 +934,14 @@ int readline (const char *const prompt) return readline_into_buffer(prompt, console_buffer, 0); }
-int readline_into_buffer(const char *const prompt, char *buffer, int timeout) +static int simple_readline(const char *const prompt, int plen, char *p,
int timeout){
char *p = buffer;-#ifdef CONFIG_CMDLINE_EDITING
unsigned int len = CONFIG_SYS_CBSIZE;int rc;static int initted = 0;/** History uses a global array which is not* writable until after relocation to RAM.* Revert to non-history version if still* running from flash.*/if (gd->flags & GD_FLG_RELOC) {if (!initted) {hist_init();initted = 1;}if (prompt)puts (prompt);rc = cread_line(prompt, p, &len, timeout);return rc < 0 ? rc : len;} else {-#endif /* CONFIG_CMDLINE_EDITING */ char * p_buf = p; int n = 0; /* buffer index */
int plen = 0; /* prompt length */ int col; /* output column cnt */ char c;/* print prompt */if (prompt) {plen = strlen (prompt);puts (prompt);} col = plen; for (;;) {@@ -1011,32 +954,32 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) } WATCHDOG_RESET(); /* Trigger watchdog, if needed */
-#ifdef CONFIG_SHOW_ACTIVITY
while (!tstc()) {show_activity(0);WATCHDOG_RESET();
if (autoconf_show_activity()) {while (!tstc()) {show_activity(0);WATCHDOG_RESET();} }-#endif c = getc();
It would be great if you separated the following unrelated formatting changes into a separate patch.
/* * 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;@@ -1045,15 +988,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;@@ -1062,22 +1005,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 (autoconf_auto_complete()) {/** if auto completion triggered* just continue*/*p = '\0';if (cmd_auto_complete(prompt,console_buffer,&n, &col)) {/* reset */p = p_buf + n;continue;} }-#endif puts (tab_seq+(col&07)); col += 8 - (col&07); } else { 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;@@ -1092,9 +1041,42 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) } } } -#ifdef CONFIG_CMDLINE_EDITING +}
+int readline_into_buffer(const char *const prompt, char *buffer, int timeout) +{
unsigned int len = CONFIG_SYS_CBSIZE;int rc;static int initted;/** History uses a global array which is not* writable until after relocation to RAM.* Revert to non-history version if still* running from flash.*/if (autoconf_cmdline_editing() && (gd->flags & GD_FLG_RELOC)) {if (!initted) {hist_init();initted = 1;}if (prompt)puts(prompt);rc = cread_line(prompt, buffer, &len, timeout);return rc < 0 ? rc : len;} else {int plen = 0; /* prompt length *//* print prompt */if (prompt) {plen = strlen(prompt);puts(prompt);}return simple_readline(prompt, plen, buffer, timeout); }-#endif }
/****************************************************************************/ diff --git a/include/command.h b/include/command.h index 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 fb219fd..1457349 100644 --- a/include/common.h +++ b/include/common.h @@ -857,9 +857,7 @@ int pcmcia_init (void);
#include <bootstage.h>
-#ifdef CONFIG_SHOW_ACTIVITY void show_activity(int arg); -#endif
/* Multicore arch functions */
#ifdef CONFIG_MP
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Hi Joe,
On Sun, Feb 24, 2013 at 12:56 PM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Remove #ifdefs in favour of autoconf for this code. This involves removing a few unnecessary #ifdefs in headers also.
We have two versions of the code - one that handles command line editing and one that is just a simple implementation. Create a new function called readline_into_buffer() which calls either cread_line() or the new simple_readline(), created to hold the 'simple' code.
The cread_print_hist_list() function is not actually used anywhere, so punt it.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 186 ++++++++++++++++++++++++------------------------------ include/command.h | 2 - include/common.h | 2 - 3 files changed, 84 insertions(+), 106 deletions(-)
diff --git a/common/main.c b/common/main.c index e1483db..3966321 100644 --- a/common/main.c +++ b/common/main.c
[snip]
It would be great if you separated the following unrelated formatting changes into a separate patch.
OK, will do.
/* * 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;@@ -1045,15 +988,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;@@ -1062,22 +1005,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 (autoconf_auto_complete()) {/** if auto completion triggered* just continue*/*p = '\0';if (cmd_auto_complete(prompt,console_buffer,&n, &col)) {/* reset */p = p_buf + n;continue;} }-#endif puts (tab_seq+(col&07)); col += 8 - (col&07); } else { 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;
[snip]
Regards, Simon
Convert main_loop() over to use autoconf, and add a required prototype to common.h.
The do_mdm_init variable is now always defined, but this seems like an acceptable compromise.
In fdt_support.h the #ifdef used is CONFIG_OF_LIBFDT. However, even if this is not defined we want to make the functions available for our conditional-compilation scheme. The only place where we really don't have access to these support functions is when USE_HOSTCC is defined. So change the #ifdef to that.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 77 +++++++++++++++++++++++---------------------------- include/common.h | 1 + include/fdt_support.h | 4 +-- 3 files changed, 37 insertions(+), 45 deletions(-)
diff --git a/common/main.c b/common/main.c index 3966321..40a79b7 100644 --- a/common/main.c +++ b/common/main.c @@ -63,10 +63,7 @@ static int retry_time = -1; /* -1 so can call readline before main_loop */
#define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk())
-#ifdef CONFIG_MODEM_SUPPORT int do_mdm_init = 0; -extern void mdm_init(void); /* defined in board.c */ -#endif
/*************************************************************************** * Watch for 'delay' seconds for autoboot stop or autoboot delay string. @@ -383,51 +380,47 @@ void main_loop(void) int len; int rc = 1; int flag; -#ifdef CONFIG_PREBOOT - char *p; -#endif
bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
-#ifdef CONFIG_MODEM_SUPPORT - debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init); - if (do_mdm_init) { - char *str = strdup(getenv("mdm_cmd")); - setenv("preboot", str); /* set or delete definition */ - if (str != NULL) - free(str); - mdm_init(); /* wait for modem connection */ + if (autoconf_modem_support()) { + debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init); + if (do_mdm_init) { + char *str = strdup(getenv("mdm_cmd")); + + setenv("preboot", str); /* set or delete definition */ + if (str != NULL) + free(str); + mdm_init(); /* wait for modem connection */ + } } -#endif /* CONFIG_MODEM_SUPPORT */
-#ifdef CONFIG_VERSION_VARIABLE - { + if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */ - } -#endif /* CONFIG_VERSION_VARIABLE */
-#ifdef CONFIG_SYS_HUSH_PARSER - u_boot_hush_start(); -#endif + if (autoconf_sys_hush_parser()) + u_boot_hush_start();
-#if defined(CONFIG_HUSH_INIT_VAR) - hush_init_var(); -#endif + if (autoconf_hush_init_var()) + hush_init_var(); + + if (autoconf_preboot()) { + char *p = getenv("preboot"); + + if (p) { + int prev;
-#ifdef CONFIG_PREBOOT - p = getenv("preboot"); - if (p) { -# ifdef CONFIG_AUTOBOOT_KEYED - int prev = disable_ctrlc(1); /* disable Control C checking */ -# endif + /* disable Control C checking */ + if (autoconf_autoboot_keyed()) + prev = disable_ctrlc(1);
- run_command_list(p, -1, 0); + run_command_list(p, -1, 0);
-# ifdef CONFIG_AUTOBOOT_KEYED - disable_ctrlc(prev); /* restore Control C checking */ -# endif + /* restore Control C checking */ + if (autoconf_autoboot_keyed()) + disable_ctrlc(prev); + } } -#endif /* CONFIG_PREBOOT */
if (autoconf_update_tftp()) update_tftp(0UL); @@ -435,9 +428,8 @@ void main_loop(void) if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) process_boot_delay();
-#if defined CONFIG_OF_CONTROL - set_working_fdt_addr((void *)gd->fdt_blob); -#endif /* CONFIG_OF_CONTROL */ + if (autoconf_of_control() && autoconf_of_libfdt()) + set_working_fdt_addr((void *)gd->fdt_blob);
/* * Main Loop for Monitor Command Processing @@ -472,14 +464,13 @@ void main_loop(void) }
if (len == -1) - puts ("<INTERRUPT>\n"); + puts("<INTERRUPT>\n"); else rc = run_command(lastcommand, flag);
- if (rc <= 0) { - /* invalid command or not repeatable, forget it */ + /* If an invalid command or not repeatable, forget it */ + if (rc <= 0) lastcommand[0] = 0; - } } }
diff --git a/include/common.h b/include/common.h index 1457349..e5eb882 100644 --- a/include/common.h +++ b/include/common.h @@ -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 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 */
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Convert main_loop() over to use autoconf, and add a required prototype to common.h.
The do_mdm_init variable is now always defined, but this seems like an acceptable compromise.
In fdt_support.h the #ifdef used is CONFIG_OF_LIBFDT. However, even if this is not defined we want to make the functions available for our conditional-compilation scheme. The only place where we really don't have access to these support functions is when USE_HOSTCC is defined. So change the #ifdef to that.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 77 +++++++++++++++++++++++---------------------------- include/common.h | 1 + include/fdt_support.h | 4 +-- 3 files changed, 37 insertions(+), 45 deletions(-)
diff --git a/common/main.c b/common/main.c index 3966321..40a79b7 100644 --- a/common/main.c +++ b/common/main.c @@ -63,10 +63,7 @@ static int retry_time = -1; /* -1 so can call readline before main_loop */
#define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk())
-#ifdef CONFIG_MODEM_SUPPORT int do_mdm_init = 0; -extern void mdm_init(void); /* defined in board.c */ -#endif
/***************************************************************************
- Watch for 'delay' seconds for autoboot stop or autoboot delay string.
@@ -383,51 +380,47 @@ void main_loop(void) int len; int rc = 1; int flag; -#ifdef CONFIG_PREBOOT
char *p;-#endif
bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");-#ifdef CONFIG_MODEM_SUPPORT
debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);if (do_mdm_init) {char *str = strdup(getenv("mdm_cmd"));setenv("preboot", str); /* set or delete definition */if (str != NULL)free(str);mdm_init(); /* wait for modem connection */
if (autoconf_modem_support()) {
Why not just remove do_mdm_init and use gd->do_mdm_init here?
debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);if (do_mdm_init) {char *str = strdup(getenv("mdm_cmd"));setenv("preboot", str); /* set or delete definition */if (str != NULL)free(str);mdm_init(); /* wait for modem connection */} }-#endif /* CONFIG_MODEM_SUPPORT */
-#ifdef CONFIG_VERSION_VARIABLE
{
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */
}-#endif /* CONFIG_VERSION_VARIABLE */
-#ifdef CONFIG_SYS_HUSH_PARSER
u_boot_hush_start();-#endif
if (autoconf_sys_hush_parser())u_boot_hush_start();-#if defined(CONFIG_HUSH_INIT_VAR)
hush_init_var();-#endif
if (autoconf_hush_init_var())hush_init_var();if (autoconf_preboot()) {char *p = getenv("preboot");if (p) {int prev;-#ifdef CONFIG_PREBOOT
p = getenv("preboot");if (p) {-# ifdef CONFIG_AUTOBOOT_KEYED
int prev = disable_ctrlc(1); /* disable Control C checking */-# endif
/* disable Control C checking */if (autoconf_autoboot_keyed())prev = disable_ctrlc(1);
run_command_list(p, -1, 0);
run_command_list(p, -1, 0);-# ifdef CONFIG_AUTOBOOT_KEYED
disable_ctrlc(prev); /* restore Control C checking */-# endif
/* restore Control C checking */if (autoconf_autoboot_keyed())disable_ctrlc(prev);} }-#endif /* CONFIG_PREBOOT */
if (autoconf_update_tftp()) update_tftp(0UL);@@ -435,9 +428,8 @@ void main_loop(void) if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) process_boot_delay();
-#if defined CONFIG_OF_CONTROL
set_working_fdt_addr((void *)gd->fdt_blob);-#endif /* CONFIG_OF_CONTROL */
if (autoconf_of_control() && autoconf_of_libfdt())
Why are you adding an additional condition on autoconf_of_libfdt()?
set_working_fdt_addr((void *)gd->fdt_blob); /* * Main Loop for Monitor Command Processing@@ -472,14 +464,13 @@ void main_loop(void) }
if (len == -1)
puts ("<INTERRUPT>\n");
puts("<INTERRUPT>\n"); else rc = run_command(lastcommand, flag);
if (rc <= 0) {/* invalid command or not repeatable, forget it */
/* If an invalid command or not repeatable, forget it */if (rc <= 0) lastcommand[0] = 0;
} }}
diff --git a/include/common.h b/include/common.h index 1457349..e5eb882 100644 --- a/include/common.h +++ b/include/common.h @@ -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
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 */
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Hi Joe,
On Sun, Feb 24, 2013 at 1:33 PM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Convert main_loop() over to use autoconf, and add a required prototype to common.h.
The do_mdm_init variable is now always defined, but this seems like an acceptable compromise.
In fdt_support.h the #ifdef used is CONFIG_OF_LIBFDT. However, even if this is not defined we want to make the functions available for our conditional-compilation scheme. The only place where we really don't have access to these support functions is when USE_HOSTCC is defined. So change the #ifdef to that.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 77 +++++++++++++++++++++++---------------------------- include/common.h | 1 + include/fdt_support.h | 4 +-- 3 files changed, 37 insertions(+), 45 deletions(-)
diff --git a/common/main.c b/common/main.c index 3966321..40a79b7 100644 --- a/common/main.c +++ b/common/main.c @@ -63,10 +63,7 @@ static int retry_time = -1; /* -1 so can call readline before main_loop */
#define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk())
-#ifdef CONFIG_MODEM_SUPPORT int do_mdm_init = 0; -extern void mdm_init(void); /* defined in board.c */ -#endif
/***************************************************************************
- Watch for 'delay' seconds for autoboot stop or autoboot delay string.
@@ -383,51 +380,47 @@ void main_loop(void) int len; int rc = 1; int flag; -#ifdef CONFIG_PREBOOT
char *p;-#endif
bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");-#ifdef CONFIG_MODEM_SUPPORT
debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);if (do_mdm_init) {char *str = strdup(getenv("mdm_cmd"));setenv("preboot", str); /* set or delete definition */if (str != NULL)free(str);mdm_init(); /* wait for modem connection */
if (autoconf_modem_support()) {Why not just remove do_mdm_init and use gd->do_mdm_init here?
Would that be valid? There is board code to set that - I am not sure what the intent is but it seems beyond the scope of this patch to change it.
debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);if (do_mdm_init) {char *str = strdup(getenv("mdm_cmd"));setenv("preboot", str); /* set or delete definition */if (str != NULL)free(str);mdm_init(); /* wait for modem connection */} }-#endif /* CONFIG_MODEM_SUPPORT */
-#ifdef CONFIG_VERSION_VARIABLE
{
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */
}-#endif /* CONFIG_VERSION_VARIABLE */
-#ifdef CONFIG_SYS_HUSH_PARSER
u_boot_hush_start();-#endif
if (autoconf_sys_hush_parser())u_boot_hush_start();-#if defined(CONFIG_HUSH_INIT_VAR)
hush_init_var();-#endif
if (autoconf_hush_init_var())hush_init_var();if (autoconf_preboot()) {char *p = getenv("preboot");if (p) {int prev;-#ifdef CONFIG_PREBOOT
p = getenv("preboot");if (p) {-# ifdef CONFIG_AUTOBOOT_KEYED
int prev = disable_ctrlc(1); /* disable Control C checking */-# endif
/* disable Control C checking */if (autoconf_autoboot_keyed())prev = disable_ctrlc(1);
run_command_list(p, -1, 0);
run_command_list(p, -1, 0);-# ifdef CONFIG_AUTOBOOT_KEYED
disable_ctrlc(prev); /* restore Control C checking */-# endif
/* restore Control C checking */if (autoconf_autoboot_keyed())disable_ctrlc(prev);} }-#endif /* CONFIG_PREBOOT */
if (autoconf_update_tftp()) update_tftp(0UL);@@ -435,9 +428,8 @@ void main_loop(void) if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) process_boot_delay();
-#if defined CONFIG_OF_CONTROL
set_working_fdt_addr((void *)gd->fdt_blob);-#endif /* CONFIG_OF_CONTROL */
if (autoconf_of_control() && autoconf_of_libfdt())Why are you adding an additional condition on autoconf_of_libfdt()?
I think I had a build warning somewhere - I will take another look, and then add a comment if it is still needed.
I actually have a patch on patchwork to remove the next line since I think it is a bad idea. But for now it stays.
set_working_fdt_addr((void *)gd->fdt_blob); /* * Main Loop for Monitor Command Processing@@ -472,14 +464,13 @@ void main_loop(void) }
if (len == -1)
puts ("<INTERRUPT>\n");
puts("<INTERRUPT>\n"); else rc = run_command(lastcommand, flag);
if (rc <= 0) {/* invalid command or not repeatable, forget it */
/* If an invalid command or not repeatable, forget it */if (rc <= 0) lastcommand[0] = 0;
} }}
diff --git a/include/common.h b/include/common.h index 1457349..e5eb882 100644 --- a/include/common.h +++ b/include/common.h @@ -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
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 */
1.8.1.3
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Regards, Simon
Hi Simon,
On Mon, Feb 25, 2013 at 11:50 PM, Simon Glass sjg@chromium.org wrote:
Hi Joe,
On Sun, Feb 24, 2013 at 1:33 PM, Joe Hershberger joe.hershberger@gmail.com wrote:
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Convert main_loop() over to use autoconf, and add a required prototype to common.h.
The do_mdm_init variable is now always defined, but this seems like an acceptable compromise.
In fdt_support.h the #ifdef used is CONFIG_OF_LIBFDT. However, even if this is not defined we want to make the functions available for our conditional-compilation scheme. The only place where we really don't have access to these support functions is when USE_HOSTCC is defined. So change the #ifdef to that.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 77 +++++++++++++++++++++++---------------------------- include/common.h | 1 + include/fdt_support.h | 4 +-- 3 files changed, 37 insertions(+), 45 deletions(-)
diff --git a/common/main.c b/common/main.c index 3966321..40a79b7 100644 --- a/common/main.c +++ b/common/main.c @@ -63,10 +63,7 @@ static int retry_time = -1; /* -1 so can call readline before main_loop */
#define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk())
-#ifdef CONFIG_MODEM_SUPPORT int do_mdm_init = 0; -extern void mdm_init(void); /* defined in board.c */ -#endif
/***************************************************************************
- Watch for 'delay' seconds for autoboot stop or autoboot delay string.
@@ -383,51 +380,47 @@ void main_loop(void) int len; int rc = 1; int flag; -#ifdef CONFIG_PREBOOT
char *p;-#endif
bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");-#ifdef CONFIG_MODEM_SUPPORT
debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);if (do_mdm_init) {char *str = strdup(getenv("mdm_cmd"));setenv("preboot", str); /* set or delete definition */if (str != NULL)free(str);mdm_init(); /* wait for modem connection */
if (autoconf_modem_support()) {Why not just remove do_mdm_init and use gd->do_mdm_init here?
Would that be valid? There is board code to set that - I am not sure what the intent is but it seems beyond the scope of this patch to change it.
From the looks of the source, it's perfectly valid. I can certainly
see your position about it being beyond the scope of this patch. It would be great if someone who cares about this modem code would clean up the mess.
debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);if (do_mdm_init) {char *str = strdup(getenv("mdm_cmd"));setenv("preboot", str); /* set or delete definition */if (str != NULL)free(str);mdm_init(); /* wait for modem connection */} }-#endif /* CONFIG_MODEM_SUPPORT */
-#ifdef CONFIG_VERSION_VARIABLE
{
if (autoconf_version_variable()) setenv("ver", version_string); /* set version variable */
}-#endif /* CONFIG_VERSION_VARIABLE */
-#ifdef CONFIG_SYS_HUSH_PARSER
u_boot_hush_start();-#endif
if (autoconf_sys_hush_parser())u_boot_hush_start();-#if defined(CONFIG_HUSH_INIT_VAR)
hush_init_var();-#endif
if (autoconf_hush_init_var())hush_init_var();if (autoconf_preboot()) {char *p = getenv("preboot");if (p) {int prev;-#ifdef CONFIG_PREBOOT
p = getenv("preboot");if (p) {-# ifdef CONFIG_AUTOBOOT_KEYED
int prev = disable_ctrlc(1); /* disable Control C checking */-# endif
/* disable Control C checking */if (autoconf_autoboot_keyed())prev = disable_ctrlc(1);
run_command_list(p, -1, 0);
run_command_list(p, -1, 0);-# ifdef CONFIG_AUTOBOOT_KEYED
disable_ctrlc(prev); /* restore Control C checking */-# endif
/* restore Control C checking */if (autoconf_autoboot_keyed())disable_ctrlc(prev);} }-#endif /* CONFIG_PREBOOT */
if (autoconf_update_tftp()) update_tftp(0UL);@@ -435,9 +428,8 @@ void main_loop(void) if (autoconf_has_bootdelay() && autoconf_bootdelay() >= 0) process_boot_delay();
-#if defined CONFIG_OF_CONTROL
set_working_fdt_addr((void *)gd->fdt_blob);-#endif /* CONFIG_OF_CONTROL */
if (autoconf_of_control() && autoconf_of_libfdt())Why are you adding an additional condition on autoconf_of_libfdt()?
I think I had a build warning somewhere - I will take another look, and then add a comment if it is still needed.
I guess you determined this to be superfluous.
<snip>
Cheers, -Joe
The headers are a bit out of order, so fix them.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/common/main.c b/common/main.c index 40a79b7..905d0c2 100644 --- a/common/main.c +++ b/common/main.c @@ -28,16 +28,15 @@ /* #define DEBUG */
#include <common.h> -#include <watchdog.h> #include <command.h> #include <fdtdec.h> #include <fdt_support.h> #include <hush.h> #include <malloc.h> #include <menu.h> -#include <version.h> - #include <post.h> +#include <version.h> +#include <watchdog.h> #include <linux/ctype.h>
DECLARE_GLOBAL_DATA_PTR;
Define a simple debug condition at the top of the file, to avoid using lots of #ifdefs later on.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 58 +++++++++++++++++++++++----------------------------------- 1 file changed, 23 insertions(+), 35 deletions(-)
diff --git a/common/main.c b/common/main.c index 905d0c2..75a184c 100644 --- a/common/main.c +++ b/common/main.c @@ -49,7 +49,11 @@ void show_boot_progress (int val) __attribute__((weak, alias("__show_boot_progre
#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) +
char console_buffer[CONFIG_SYS_CBSIZE + 1]; /* console I/O buffer */
@@ -1107,9 +1111,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 */ @@ -1118,10 +1120,8 @@ 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 - return (nargs); + debug_parser("parse_line: nargs=%d\n", nargs); + return nargs; }
argv[nargs++] = line; /* begin of argument string */ @@ -1132,10 +1132,8 @@ 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 - return (nargs); + debug_parser("parse_line: nargs=%d\n", nargs); + return nargs; }
*line++ = '\0'; /* terminate current arg */ @@ -1143,9 +1141,7 @@ 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); }
@@ -1162,12 +1158,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 */
@@ -1255,10 +1249,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); }
/**************************************************************************** @@ -1289,12 +1281,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) { @@ -1312,9 +1304,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) {
/* @@ -1343,9 +1333,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);
Hi Simon,
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Define a simple debug condition at the top of the file, to avoid using lots of #ifdefs later on.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 58 +++++++++++++++++++++++----------------------------------- 1 file changed, 23 insertions(+), 35 deletions(-)
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
Define a simple debug condition at the top of the file, to avoid using lots of #ifdefs later on.
Signed-off-by: Simon Glass sjg@chromium.org --- Changes in v2: None
common/main.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/common/main.c b/common/main.c index 75a184c..22ed6ef 100644 --- a/common/main.c +++ b/common/main.c @@ -54,6 +54,11 @@ void show_boot_progress (int val) __attribute__((weak, alias("__show_boot_progre #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 */
@@ -115,11 +120,9 @@ static int abortboot_keyed(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 @@ -144,10 +147,8 @@ static int abortboot_keyed(int bootdelay) 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");
/* don't retry auto boot? */ if (autoconf_boot_retry_time() && @@ -158,10 +159,8 @@ static int abortboot_keyed(int bootdelay) } } while (!abort && get_ticks() <= etime);
-# if DEBUG_BOOTKEYS if (!abort) - puts("key timeout\n"); -# endif + debug_bootkeys("key timeout\n");
if (autoconf_silent_console() && abort) gd->flags &= ~GD_FLG_SILENT;
Hi Simon
On Sun, Feb 24, 2013 at 11:26 AM, Simon Glass sjg@chromium.org wrote:
Define a simple debug condition at the top of the file, to avoid using lots of #ifdefs later on.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2: None
common/main.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
Reviewed-by: Joe Hershberger joe.hershberger@ni.com
participants (2)
-
Joe Hershberger -
Simon Glass