* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
@ 2013-02-26 16:10 Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 01/16] Implement autoconf header file Simon Glass
` (16 more replies)
0 siblings, 17 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:10 UTC (permalink / raw)
To: u-boot
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 series seems to be roughly neutral (below numbers are
average change in byte size for each region:
x86: (for 1/1 boards) all -12.0 data +4.0 text -16.0
m68k: (for 41/50 boards) all -23.7 text -23.7
powerpc: (for 633/635 boards) all -4.4 bss +2.0 data +0.0 text -6.4
sandbox: (for 1/1 boards) all +16.0 bss +16.0
sh: (for 16/21 boards) all -4.8 bss +3.2 text -8.0
nios2: (for 3/3 boards) all +24.0 bss +1.3 data -1.3 text +24.0
arm: (for 292/292 boards) all -11.1 bss +6.0 spl/u-boot-spl:all +0.4 spl/u-boot-spl:text +0.4 text -17.1
nds32: (for 3/3 boards) all -42.7 bss +10.7 text -53.3
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 v3:
- Update config_xxx() to autoconf_xxx() in comments/README/sed
- Update config_xxx_enabled() to autoconf_has_xxx() in comments/README/sed
- Add comment as to why we use [A-Za-z0-9_][A-Za-z0-9_]*
- Rename sed scripts to more useful names
- Fix commit message which said autoboot() instead of abortboot()
- Add note about CONFIG_MENU not being needed in main.c anymore
- Fix missing && in if() statement
- Remove unneeded retry_min variable
- Simplify code for finding out bootdelay from config or environment
- Separate out checkpatch fixes in command line reading code into new patch
- Remove the extra config_of_libfdt() condition in main_loop()
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 (16):
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: Fix typos and checkpatch warnings in command line reading
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 | 43 ++-
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/define2value.sed | 37 ++
tools/scripts/define2zero.sed | 32 ++
15 files changed, 580 insertions(+), 496 deletions(-)
create mode 100644 include/config_drop.h
create mode 100644 tools/scripts/define2value.sed
create mode 100644 tools/scripts/define2zero.sed
--
1.8.1.3
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 01/16] Implement autoconf header file
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
@ 2013-02-26 16:10 ` Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 02/16] at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263 Simon Glass
` (15 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:10 UTC (permalink / raw)
To: u-boot
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 v3:
- Update config_xxx() to autoconf_xxx() in comments/README/sed
- Update config_xxx_enabled() to autoconf_has_xxx() in comments/README/sed
- Add comment as to why we use [A-Za-z0-9_][A-Za-z0-9_]*
- Rename sed scripts to more useful names
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 | 43 ++++++++++++++++++++-
README | 87 ++++++++++++++++++++++++++++++++++++++++--
include/common.h | 3 ++
include/config_drop.h | 17 +++++++++
tools/scripts/define2value.sed | 37 ++++++++++++++++++
tools/scripts/define2zero.sed | 32 ++++++++++++++++
6 files changed, 215 insertions(+), 4 deletions(-)
create mode 100644 include/config_drop.h
create mode 100644 tools/scripts/define2value.sed
create mode 100644 tools/scripts/define2zero.sed
diff --git a/Makefile b/Makefile
index fc18dd4..8ba068d 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,45 @@ $(obj)include/autoconf.mk: $(obj)include/config.h
sed -n -f tools/scripts/define2mk.sed > $@.tmp && \
mv $@.tmp $@
+# Create a C header file where every '#define CONFIG_XXX value' becomes
+# '#define autoconf_xxx() value', or '#define autoconf_xxx() 0' where the
+# CONFIG is not used by this board configuration. This allows C code to do
+# things like 'if (autoconf_xxx())' and have the compiler remove the dead code,
+# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases
+# if the autoconf_...() returns 0 then the option is not enabled. In some rare
+# cases such as CONFIG_BOOTDELAY, the config can be enabled but still have a
+# a value of 0. So in addition we a #define autoconf_has_xxx(), setting the
+# value to 0 if the option is disabled, 1 if enabled. This last feature will
+# hopefully be deprecated soon.
+# The file is regenerated when any U-Boot header file changes.
+$(obj)include/generated/autoconf.h: $(obj)include/config.h
+ @$(XECHO) Generating $@ ; \
+ set -e ; \
+ : Extract the config macros to a C header file ; \
+ $(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \
+ sed -n -f tools/scripts/define2value.sed > $@.tmp; \
+ : Regenerate our list of all config macros if neeed ; \
+ if [ ! -f $@-all.tmp ] || \
+ find $(src) -name '*.h' -type f -newer $@-all.tmp | \
+ egrep -qv 'include/(autoconf.h|generated|config.h)'; \
+ then \
+ : Extract all config macros from all C header files ; \
+ : We can grep for CONFIG since the value will be dropped ; \
+ ( \
+ find ${src} -name "*.h" -type f | xargs \
+ cat | grep CONFIG | \
+ sed -n -f tools/scripts/define2zero.sed \
+ ) | sort | uniq > $@-all.tmp; \
+ fi; \
+ : Extract the enabled config macros to a C header file ; \
+ $(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \
+ sed -n -f tools/scripts/define2zero.sed | \
+ sort > $@-enabled.tmp; \
+ set -e ; \
+ : Find CONFIGs that are not enabled ; \
+ comm -13 $@-enabled.tmp $@-all.tmp >>$@.tmp && \
+ mv $@.tmp $@
+
$(obj)include/generated/generic-asm-offsets.h: $(obj)include/autoconf.mk.dep \
$(obj)lib/asm-offsets.s
@$(XECHO) Generating $@
@@ -770,7 +810,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..e6f3c04 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 autoconf_xxx() value
+
+for enabled options, or:
+
+# #define autoconf_xxx() 0
+
+for disabled options. You can therefore generally change code like this:
+
+ #ifdef CONFIG_XXX
+ do_something
+ #else
+ do_something_else
+ #endif
+
+to this:
+
+ if (autoconf_xxx())
+ do_something;
+ else
+ do_something_else;
+
+The compiler will see that autoconf_xxx() evalutes to a constant and will
+eliminate the dead code. The resulting code (and code size) is the same.
+
+Multiple #ifdefs can be converted also:
+
+ #if defined(CONFIG_XXX) && !defined(CONFIG_YYY)
+ do_something
+ #endif
+
+ if (autoconf_xxx() && !autoconf_yyy())
+ do_something;
+
+Where the macro evaluates to a string, it will be non-NULL, so the above
+will work whether the macro is a string or a number.
+
+This takes care of almost all CONFIG macros. Unfortunately there are a few
+cases where a value of 0 does not mean the option is disabled. For example
+CONFIG_BOOTDELAY can be defined to 0, which means that the bootdelay
+code should be used, but with a value of 0. To get around this and other
+sticky cases, an addition macro with an 'has_' prefix is provided, where
+the value is always either 0 or 1:
+
+ // Will work even if boaard config has '#define CONFIG_BOOTDELAY 0'
+ if (autoconf_has_bootdelay())
+ do_something;
+
+(Probably such config options should be deprecated and then we can remove
+this feature)
+
+U-Boot already has a Makefile scheme to permit files to be easily included
+based on CONFIG. This can be used where the code to be compiled exists in
+its own source file. So the following rules apply:
+
+ 1. Use #ifdef to conditionally compile an exported function or variable
+ 2. Use ordinary C code with autoconf_xxx() everywhere else
+ 3. Mark your functions and data structures static where possible
+ 4. Use the autoconf_has_xxx() variants only if essential
+ 5. When changing existing code, first create a new patch to replace
+ #ifdefs in the surrounding area
diff --git a/include/common.h b/include/common.h
index 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..bf68b50
--- /dev/null
+++ b/include/config_drop.h
@@ -0,0 +1,17 @@
+/*
+ * Copyright 2013 Google, Inc
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License Version 2. This file is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _CONFIG_DROP_H
+#define _CONFIG_DROP_H
+
+/* Options which don't seem to be defined by any header in U-Boot */
+#define CONFIG_MENUPROMPT "Auto-boot prompt"
+#define CONFIG_MENUKEY
+#define CONFIG_UPDATE_TFTP
+
+#endif
diff --git a/tools/scripts/define2value.sed b/tools/scripts/define2value.sed
new file mode 100644
index 0000000..205f9fe
--- /dev/null
+++ b/tools/scripts/define2value.sed
@@ -0,0 +1,37 @@
+#
+# Sed script to parse CPP macros and generate a list of CONFIG macros
+#
+# This converts:
+# #define CONFIG_XXX value
+#into:
+# #define autoconf_xxx() value
+# #define autoconf_has_xxx() 1
+
+# Macros with parameters are ignored.
+# (Note we avoid + since it doesn't appear to work)
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
+ d
+}
+
+# Only process values prefixed with #define CONFIG_
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
+ # Strip the #define prefix
+ s/#define[ \t]*CONFIG_/autoconf_/;
+ # Change to form CONFIG_*=VALUE
+ s/[\t ][\t ]*/=/;
+ # Handle lines with no value
+ s/^\([^=]*\)$/\1=/;
+ # Drop trailing spaces
+ s/ *$//;
+ # Change empty values to '1'
+ s/=$/=1/;
+ # Add #define at the start
+ s/^\([^=]*\)=/#define \L\1() /
+ # print the line
+ p
+ # Create autoconf_has_...(), value 1
+ s/().*/() 1/
+ s/\(autoconf_\)/\1has_/
+ # print the line
+ p
+}
diff --git a/tools/scripts/define2zero.sed b/tools/scripts/define2zero.sed
new file mode 100644
index 0000000..95e6860
--- /dev/null
+++ b/tools/scripts/define2zero.sed
@@ -0,0 +1,32 @@
+#
+# Sed script to parse CPP macros and generate a list of autoconf macros
+#
+# This converts:
+# #define CONFIG_XXX value
+#into:
+# #define autoconf_xxx() 0
+# #define autoconf_has_xxx() 0
+
+# Macros with parameters are ignored.
+# (Note we avoid + since it doesn't appear to work)
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ {
+ s/.*//
+}
+
+# Only process values prefixed with #define CONFIG_
+/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ {
+ # Strip the #define prefix
+ s/#define *//;
+ # Remove the value
+ s/[ \t].*//;
+ # Convert to lower case, prepend #define
+ s/CONFIG_\(.*\)/#define autoconf_\L\1/
+ # Append 0
+ s/$/() 0/
+ # print the line
+ p
+ # Create autoconf_has_...(), value 0
+ s/\(autoconf_\)/\1has_/
+ # print the line
+ p
+}
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 02/16] at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 01/16] Implement autoconf header file Simon Glass
@ 2013-02-26 16:10 ` Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf Simon Glass
` (14 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:10 UTC (permalink / raw)
To: u-boot
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 v3: None
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 01/16] Implement autoconf header file Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 02/16] at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263 Simon Glass
@ 2013-02-26 16:10 ` Simon Glass
2013-03-20 19:40 ` Tom Rini
2013-02-26 16:10 ` [U-Boot] [PATCH v3 04/16] main: Separate out the two abortboot() functions Simon Glass
` (13 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:10 UTC (permalink / raw)
To: u-boot
This function should be declared in net.h. At the same time, let's use
autoconf instead of #ifdef for its inclusion.
Signed-off-by: Simon Glass <sjg@chromium.org>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
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__ */
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 04/16] main: Separate out the two abortboot() functions
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (2 preceding siblings ...)
2013-02-26 16:10 ` [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf Simon Glass
@ 2013-02-26 16:10 ` Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 05/16] main: Move boot_delay code into its own function Simon Glass
` (12 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:10 UTC (permalink / raw)
To: u-boot
There are two implementations of abortboot(). Turn these into two separate
functions, and create a single abortboot() which calls either one or the
other.
Also it seems that nothing uses abortboot() outside main, so make it static.
At this point there is no further use of CONFIG_MENU in main.c.
Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3:
- Fix commit message which said autoboot() instead of abortboot()
- Add note about CONFIG_MENU not being needed in main.c anymore
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 */
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 05/16] main: Move boot_delay code into its own function
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (3 preceding siblings ...)
2013-02-26 16:10 ` [U-Boot] [PATCH v3 04/16] main: Separate out the two abortboot() functions Simon Glass
@ 2013-02-26 16:10 ` Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 06/16] main: Use autoconf for boot retry feature Simon Glass
` (11 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:10 UTC (permalink / raw)
To: u-boot
Move this code into its own function, since it clutters up main_loop().
Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 06/16] main: Use autoconf for boot retry feature
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (4 preceding siblings ...)
2013-02-26 16:10 ` [U-Boot] [PATCH v3 05/16] main: Move boot_delay code into its own function Simon Glass
@ 2013-02-26 16:10 ` Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 07/16] main: Remove CONFIG #ifdefs from the abortboot() code Simon Glass
` (10 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:10 UTC (permalink / raw)
To: u-boot
Change this feature to use autoconf instead of #ifdef.
Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3:
- Fix missing && in if() statement
- Remove unneeded retry_min variable
Changes in v2: None
common/main.c | 73 ++++++++++++++++++++++++++---------------------------------
1 file changed, 32 insertions(+), 41 deletions(-)
diff --git a/common/main.c b/common/main.c
index 0df7992..36d8048 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
@@ -562,10 +553,10 @@ void init_cmd_timeout(void)
if (s != NULL)
retry_time = (int)simple_strtol(s, NULL, 10);
else
- retry_time = CONFIG_BOOT_RETRY_TIME;
+ retry_time = autoconf_boot_retry_time();
- if (retry_time >= 0 && retry_time < CONFIG_BOOT_RETRY_MIN)
- retry_time = CONFIG_BOOT_RETRY_MIN;
+ if (retry_time >= 0 && retry_time < autoconf_boot_retry_min())
+ retry_time = autoconf_boot_retry_min();
}
/***************************************************************************
@@ -787,13 +778,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 +1056,14 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
col = plen;
for (;;) {
-#ifdef CONFIG_BOOT_RETRY_TIME
- while (!tstc()) { /* while no incoming data */
- if (retry_time >= 0 && get_ticks() > endtime)
- return (-2); /* timed out */
- WATCHDOG_RESET();
+ if (autoconf_boot_retry_time()) {
+ while (!tstc()) { /* while no incoming data */
+ if (retry_time >= 0 && get_ticks() > endtime)
+ return -2; /* timed out */
+ WATCHDOG_RESET();
+ }
}
-#endif
- WATCHDOG_RESET(); /* Trigger watchdog, if needed */
+ WATCHDOG_RESET(); /* Trigger watchdog, if needed */
#ifdef CONFIG_SHOW_ACTIVITY
while (!tstc()) {
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 07/16] main: Remove CONFIG #ifdefs from the abortboot() code
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (5 preceding siblings ...)
2013-02-26 16:10 ` [U-Boot] [PATCH v3 06/16] main: Use autoconf for boot retry feature Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 08/16] main: Use get/setenv_ulong() Simon Glass
` (9 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
Move this code over to using autoconf. We can add the autoconf values to
the delaykey[] array, and move the code that checks for autoconf values into
the loop.
Also change to using ARRAY_SIZE on delaykey[].
Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
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 36d8048..e2eb9ec 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;
}
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 08/16] main: Use get/setenv_ulong()
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (6 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 07/16] main: Remove CONFIG #ifdefs from the abortboot() code Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 09/16] main: Use autoconf for boot_delay code Simon Glass
` (8 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
These functions are now available, so use them to avoid extra code here.
Signed-off-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
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 e2eb9ec..8987444 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");
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 09/16] main: Use autoconf for boot_delay code
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (7 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 08/16] main: Use get/setenv_ulong() Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-27 9:14 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 10/16] main: Use autoconf for parser selection Simon Glass
` (7 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
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 v3:
- Simplify code for finding out bootdelay from config or environment
Changes in v2: None
common/main.c | 107 ++++++++++++++++++++++-----------------------------------
| 2 --
2 files changed, 42 insertions(+), 67 deletions(-)
diff --git a/common/main.c b/common/main.c
index 8987444..974fc82 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,81 @@ 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();
+ bootdelay = getenv_ulong("bootdelay", 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 */
}
/****************************************************************************/
--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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 10/16] main: Use autoconf for parser selection
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (8 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 09/16] main: Use autoconf for boot_delay code Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 11/16] main: Fix typos and checkpatch warnings in command line reading Simon Glass
` (6 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
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>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
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 974fc82..d49692d 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>
@@ -381,12 +378,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
@@ -446,12 +441,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.
@@ -486,7 +480,6 @@ void main_loop(void)
lastcommand[0] = 0;
}
}
-#endif /*CONFIG_SYS_HUSH_PARSER*/
}
/*
@@ -1184,7 +1177,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;
@@ -1400,7 +1392,6 @@ static int builtin_run_command(const char *cmd, int flag)
return rc ? rc : repeatable;
}
-#endif
/*
* Run a command using the selected parser.
@@ -1411,22 +1402,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.
*
@@ -1467,7 +1457,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)
{
@@ -1477,13 +1466,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);
@@ -1492,20 +1484,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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 11/16] main: Fix typos and checkpatch warnings in command line reading
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (9 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 10/16] main: Use autoconf for parser selection Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-27 9:25 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 12/16] main: Use autoconf " Simon Glass
` (5 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
There are a few over-long lines and other checkpatch problems in this area
of the code. Prepare the ground for the next patch by tidying these up.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3:
- Separate out checkpatch fixes in command line reading code into new patch
Changes in v2: None
common/main.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/common/main.c b/common/main.c
index d49692d..074ba60 100644
--- a/common/main.c
+++ b/common/main.c
@@ -1021,20 +1021,20 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
* 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;
@@ -1043,15 +1043,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;
@@ -1060,7 +1060,7 @@ 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 */
+ if (c == '\t') { /* expand TABs */
#ifdef CONFIG_AUTO_COMPLETE
/* if auto completion triggered just continue */
*p = '\0';
@@ -1075,7 +1075,7 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
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;
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 12/16] main: Use autoconf in command line reading
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (10 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 11/16] main: Fix typos and checkpatch warnings in command line reading Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-27 9:17 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 13/16] main: Use autoconf in main_loop() Simon Glass
` (4 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
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 v3: None
Changes in v2: None
common/main.c | 164 ++++++++++++++++++++++++------------------------------
include/command.h | 2 -
include/common.h | 2 -
3 files changed, 73 insertions(+), 95 deletions(-)
diff --git a/common/main.c b/common/main.c
index 074ba60..66b74b6 100644
--- a/common/main.c
+++ b/common/main.c
@@ -512,8 +512,6 @@ void reset_cmd_timeout(void)
}
#endif
-#ifdef CONFIG_CMDLINE_EDITING
-
/*
* cmdline-editing related codes from vivi.
* Author: Janghoon Lyu <nandy@mizi.com>
@@ -616,27 +614,6 @@ static char* hist_next(void)
return (ret);
}
-#ifndef CONFIG_CMDLINE_EDITING
-static void cread_print_hist_list(void)
-{
- int i;
- unsigned long n;
-
- n = hist_num - hist_max;
-
- i = hist_add_idx + 1;
- while (1) {
- if (i > hist_max)
- i = 0;
- if (i == hist_add_idx)
- break;
- printf("%s\n", hist_list[i]);
- n++;
- i++;
- }
-}
-#endif /* CONFIG_CMDLINE_EDITING */
-
#define BEGINNING_OF_LINE() { \
while (num) { \
getcmd_putch(CTL_BACKSPACE); \
@@ -898,27 +875,27 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len,
REFRESH_TO_EOL();
continue;
}
-#ifdef CONFIG_AUTO_COMPLETE
- case '\t': {
- int num2, col;
+ case '\t':
+ if (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;
@@ -934,8 +911,6 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len,
return 0;
}
-#endif /* CONFIG_CMDLINE_EDITING */
-
/****************************************************************************/
/*
@@ -957,46 +932,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 (;;) {
@@ -1009,12 +952,12 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
}
WATCHDOG_RESET(); /* Trigger watchdog, if needed */
-#ifdef CONFIG_SHOW_ACTIVITY
- while (!tstc()) {
- show_activity(0);
- WATCHDOG_RESET();
+ if (autoconf_show_activity()) {
+ while (!tstc()) {
+ show_activity(0);
+ WATCHDOG_RESET();
+ }
}
-#endif
c = getc();
/*
@@ -1061,14 +1004,20 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout)
*/
if (n < CONFIG_SYS_CBSIZE-2) {
if (c == '\t') { /* expand TABs */
-#ifdef CONFIG_AUTO_COMPLETE
- /* if auto completion triggered just continue */
- *p = '\0';
- if (cmd_auto_complete(prompt, console_buffer, &n, &col)) {
- p = p_buf + n; /* reset */
- continue;
+ if (autoconf_auto_complete()) {
+ /*
+ * if auto completion triggered
+ * just continue
+ */
+ *p = '\0';
+ if (cmd_auto_complete(prompt,
+ console_buffer,
+ &n, &col)) {
+ /* reset */
+ p = p_buf + n;
+ continue;
+ }
}
-#endif
puts (tab_seq+(col&07));
col += 8 - (col&07);
} else {
@@ -1090,9 +1039,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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 13/16] main: Use autoconf in main_loop()
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (11 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 12/16] main: Use autoconf " Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-27 9:23 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 14/16] main: Correct header order Simon Glass
` (3 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
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 v3:
- Remove the extra config_of_libfdt() condition in main_loop()
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 66b74b6..cd068e7 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.
@@ -382,51 +379,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);
@@ -434,9 +427,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())
+ set_working_fdt_addr((void *)gd->fdt_blob);
/*
* Main Loop for Monitor Command Processing
@@ -471,14 +463,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
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 14/16] main: Correct header order
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (12 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 13/16] main: Use autoconf in main_loop() Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 15/16] main: Add debug_parser() to avoid #ifdefs Simon Glass
` (2 subsequent siblings)
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
The headers are a bit out of order, so fix them.
Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v3: None
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 cd068e7..a464620 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;
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 15/16] main: Add debug_parser() to avoid #ifdefs
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (13 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 14/16] main: Correct header order Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 16/16] main: Add debug_bootkeys " Simon Glass
2013-03-21 14:38 ` [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Tom Rini
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
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>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
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 a464620..8ea9475 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 */
@@ -1105,9 +1109,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 */
@@ -1116,10 +1118,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 */
@@ -1130,10 +1130,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 */
@@ -1141,9 +1139,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);
}
@@ -1160,12 +1156,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 */
@@ -1253,10 +1247,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);
}
/****************************************************************************
@@ -1287,12 +1279,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) {
@@ -1310,9 +1302,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) {
/*
@@ -1341,9 +1331,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);
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 16/16] main: Add debug_bootkeys to avoid #ifdefs
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (14 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 15/16] main: Add debug_parser() to avoid #ifdefs Simon Glass
@ 2013-02-26 16:11 ` Simon Glass
2013-03-21 14:38 ` [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Tom Rini
16 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-02-26 16:11 UTC (permalink / raw)
To: u-boot
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>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
---
Changes in v3: None
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 8ea9475..830800b 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;
--
1.8.1.3
^ permalink raw reply related [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 09/16] main: Use autoconf for boot_delay code
2013-02-26 16:11 ` [U-Boot] [PATCH v3 09/16] main: Use autoconf for boot_delay code Simon Glass
@ 2013-02-27 9:14 ` Joe Hershberger
0 siblings, 0 replies; 37+ messages in thread
From: Joe Hershberger @ 2013-02-27 9:14 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Tue, Feb 26, 2013 at 10:11 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 v3:
> - Simplify code for finding out bootdelay from config or environment
>
> Changes in v2: None
>
> common/main.c | 107 ++++++++++++++++++++++-----------------------------------
> include/menu.h | 2 --
> 2 files changed, 42 insertions(+), 67 deletions(-)
>
Acked-by: Joe Hershberger <joe.hershberger@ni.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 12/16] main: Use autoconf in command line reading
2013-02-26 16:11 ` [U-Boot] [PATCH v3 12/16] main: Use autoconf " Simon Glass
@ 2013-02-27 9:17 ` Joe Hershberger
0 siblings, 0 replies; 37+ messages in thread
From: Joe Hershberger @ 2013-02-27 9:17 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Tue, Feb 26, 2013 at 10:11 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 v3: None
> Changes in v2: None
>
> common/main.c | 164 ++++++++++++++++++++++++------------------------------
> include/command.h | 2 -
> include/common.h | 2 -
> 3 files changed, 73 insertions(+), 95 deletions(-)
>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 13/16] main: Use autoconf in main_loop()
2013-02-26 16:11 ` [U-Boot] [PATCH v3 13/16] main: Use autoconf in main_loop() Simon Glass
@ 2013-02-27 9:23 ` Joe Hershberger
0 siblings, 0 replies; 37+ messages in thread
From: Joe Hershberger @ 2013-02-27 9:23 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Tue, Feb 26, 2013 at 10:11 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 v3:
> - Remove the extra config_of_libfdt() condition in main_loop()
>
> Changes in v2: None
>
> common/main.c | 77 +++++++++++++++++++++++----------------------------
> include/common.h | 1 +
> include/fdt_support.h | 4 +--
> 3 files changed, 37 insertions(+), 45 deletions(-)
>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 11/16] main: Fix typos and checkpatch warnings in command line reading
2013-02-26 16:11 ` [U-Boot] [PATCH v3 11/16] main: Fix typos and checkpatch warnings in command line reading Simon Glass
@ 2013-02-27 9:25 ` Joe Hershberger
0 siblings, 0 replies; 37+ messages in thread
From: Joe Hershberger @ 2013-02-27 9:25 UTC (permalink / raw)
To: u-boot
Hi Simon,
On Tue, Feb 26, 2013 at 10:11 AM, Simon Glass <sjg@chromium.org> wrote:
> There are a few over-long lines and other checkpatch problems in this area
> of the code. Prepare the ground for the next patch by tidying these up.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v3:
> - Separate out checkpatch fixes in command line reading code into new patch
>
> Changes in v2: None
>
> common/main.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
Reviewed-by: Joe Hershberger <joe.hershberger@ni.com>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf
2013-02-26 16:10 ` [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf Simon Glass
@ 2013-03-20 19:40 ` Tom Rini
2013-03-20 19:57 ` Simon Glass
0 siblings, 1 reply; 37+ messages in thread
From: Tom Rini @ 2013-03-20 19:40 UTC (permalink / raw)
To: u-boot
On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
> This function should be declared in net.h. At the same time, let's use
> autoconf instead of #ifdef for its inclusion.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
[snip]
> @@ -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)) {
This doesn't read nearly as clean to me as the old code. Part of the
problem is that we really need a way to foce an CONFIG option to be set
to something and give a default (so, the Kconfig switch-over). Now, in
cases like this it the compiler smart enough to say "oh, msec_max is a
constant, lets not waste the space on a variable" ? Would const'ing
that help or confuse things would be a follow up question.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130320/eb603e19/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf
2013-03-20 19:40 ` Tom Rini
@ 2013-03-20 19:57 ` Simon Glass
2013-03-20 20:30 ` Tom Rini
0 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-03-20 19:57 UTC (permalink / raw)
To: u-boot
Hi Tom,
On Wed, Mar 20, 2013 at 12:40 PM, Tom Rini <trini@ti.com> wrote:
> On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
>> This function should be declared in net.h. At the same time, let's use
>> autoconf instead of #ifdef for its inclusion.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> [snip]
>> @@ -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)) {
>
> This doesn't read nearly as clean to me as the old code. Part of the
> problem is that we really need a way to foce an CONFIG option to be set
> to something and give a default (so, the Kconfig switch-over). Now, in
> cases like this it the compiler smart enough to say "oh, msec_max is a
> constant, lets not waste the space on a variable" ? Would const'ing
> that help or confuse things would be a follow up question.
Yes I believe the compiler does the right thing here. Even if there is
a 'variable' it would only be in a register.
const can be used, but I don't think it would do anything in this case.
In reading the old code, you need to look at the top of the file,
where some code was removed, and take that into account.
-/* 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
-
Regards,
Simon
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf
2013-03-20 19:57 ` Simon Glass
@ 2013-03-20 20:30 ` Tom Rini
2013-03-21 1:47 ` Simon Glass
0 siblings, 1 reply; 37+ messages in thread
From: Tom Rini @ 2013-03-20 20:30 UTC (permalink / raw)
To: u-boot
On Wed, Mar 20, 2013 at 12:57:37PM -0700, Simon Glass wrote:
> Hi Tom,
>
> On Wed, Mar 20, 2013 at 12:40 PM, Tom Rini <trini@ti.com> wrote:
> > On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
> >> This function should be declared in net.h. At the same time, let's use
> >> autoconf instead of #ifdef for its inclusion.
> >>
> >> Signed-off-by: Simon Glass <sjg@chromium.org>
> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
> > [snip]
> >> @@ -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;
OK, this in particular would looke a little better as:
addr = autoconf_has_update_load_addr() ?
autoconf_update_load_addr() : 0x100000;
if ((env_addr = ...)
Set the addr to the default, then override.
> >>
> >> + 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)) {
> >
> > This doesn't read nearly as clean to me as the old code. Part of the
> > problem is that we really need a way to foce an CONFIG option to be set
> > to something and give a default (so, the Kconfig switch-over). Now, in
> > cases like this it the compiler smart enough to say "oh, msec_max is a
> > constant, lets not waste the space on a variable" ? Would const'ing
> > that help or confuse things would be a follow up question.
>
> Yes I believe the compiler does the right thing here. Even if there is
> a 'variable' it would only be in a register.
>
> const can be used, but I don't think it would do anything in this case.
OK.
> In reading the old code, you need to look at the top of the file,
> where some code was removed, and take that into account.
>
> -/* 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
That's what I mean. We have defaults that can be overriden and we use
the value simply. This was a decent way of saying "we know most people
will use X as the value, but allow for override". Now we have to do
if (autoconf_has_foo())
x = autoconf_foo()
else
x = default;
Or:
x = autoconf_has_foo() ? autoconf_foo() : default;
Which usually spills out into 2 lines and isn't my favorite construct
ever. Lets see where I'm at after going over the whole series.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130320/0ca0f483/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf
2013-03-20 20:30 ` Tom Rini
@ 2013-03-21 1:47 ` Simon Glass
0 siblings, 0 replies; 37+ messages in thread
From: Simon Glass @ 2013-03-21 1:47 UTC (permalink / raw)
To: u-boot
Hi Tom,
On Wed, Mar 20, 2013 at 1:30 PM, Tom Rini <trini@ti.com> wrote:
> On Wed, Mar 20, 2013 at 12:57:37PM -0700, Simon Glass wrote:
>> Hi Tom,
>>
>> On Wed, Mar 20, 2013 at 12:40 PM, Tom Rini <trini@ti.com> wrote:
>> > On Tue, Feb 26, 2013 at 08:10:56AM -0800, Simon Glass wrote:
>> >> This function should be declared in net.h. At the same time, let's use
>> >> autoconf instead of #ifdef for its inclusion.
>> >>
>> >> Signed-off-by: Simon Glass <sjg@chromium.org>
>> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
>> > [snip]
>> >> @@ -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;
>
> OK, this in particular would looke a little better as:
> addr = autoconf_has_update_load_addr() ?
> autoconf_update_load_addr() : 0x100000;
> if ((env_addr = ...)
Yes, agreed.
>
> Set the addr to the default, then override.
>
>> >>
>> >> + 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)) {
>> >
>> > This doesn't read nearly as clean to me as the old code. Part of the
>> > problem is that we really need a way to foce an CONFIG option to be set
>> > to something and give a default (so, the Kconfig switch-over). Now, in
>> > cases like this it the compiler smart enough to say "oh, msec_max is a
>> > constant, lets not waste the space on a variable" ? Would const'ing
>> > that help or confuse things would be a follow up question.
>>
>> Yes I believe the compiler does the right thing here. Even if there is
>> a 'variable' it would only be in a register.
>>
>> const can be used, but I don't think it would do anything in this case.
>
> OK.
>
>> In reading the old code, you need to look at the top of the file,
>> where some code was removed, and take that into account.
>>
>> -/* 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
>
> That's what I mean. We have defaults that can be overriden and we use
> the value simply. This was a decent way of saying "we know most people
> will use X as the value, but allow for override". Now we have to do
> if (autoconf_has_foo())
> x = autoconf_foo()
> else
> x = default;
>
> Or:
> x = autoconf_has_foo() ? autoconf_foo() : default;
> Which usually spills out into 2 lines and isn't my favorite construct
> ever. Lets see where I'm at after going over the whole series.
OK, in fact we need to set a way of doing this, and also whether in
some cases we are better off with #ifdefs. My main dislikes of #ifdef
were mentioned in the cover letter, but they do have their place.
Regards,
Simon
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
` (15 preceding siblings ...)
2013-02-26 16:11 ` [U-Boot] [PATCH v3 16/16] main: Add debug_bootkeys " Simon Glass
@ 2013-03-21 14:38 ` Tom Rini
2013-05-13 22:12 ` Simon Glass
16 siblings, 1 reply; 37+ messages in thread
From: Tom Rini @ 2013-03-21 14:38 UTC (permalink / raw)
To: u-boot
On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> different boards compile different versions of the source code, meaning
> that we must build all boards to check for failures. It is easy to misspell
> an #ifdef and there is not as much checking of this by the compiler. Multiple
> dependent #ifdefs are harder to do than with if..then..else. Variable
> declarations must be #idefed as well as the code that uses them, often much
> later in the file/function. #ifdef indents don't match code indents and
> have their own separate indent feature. Overall, excessive use of #idef
> hurts readability and makes the code harder to modify and refactor. For
> people coming newly into the code base, #ifdefs can be a big barrier.
>
> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
> attempt to turn the tide, this series includes a patch which provides a way
> to make CONFIG macros available to C code without using the preprocessor.
> This makes it possible to use standard C conditional features such as
> if/then instead of #ifdef. A README update exhorts compliance.
OK, this is true. Looking over the series, a number of the patches are
just general fixes / improvements that don't depend on the autoconf_...
work. Lets split this out now and take them in now as they seem like
reviewable by inspection code.
For the approach itself, I'm not sure which is best here. In a lot of
cases we're trading an #ifdef for adding a level of indent to already
pretty indented code and that feels like a wash, in terms of readability
to me. We probably need to re-factor some of the code in question there
too for readability, then see about using autoconf_... type things, or
maybe something else.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130321/fa0a101d/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-03-21 14:38 ` [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Tom Rini
@ 2013-05-13 22:12 ` Simon Glass
2013-05-14 21:21 ` Tom Rini
0 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-05-13 22:12 UTC (permalink / raw)
To: u-boot
Hi Tom,
On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini <trini@ti.com> wrote:
> On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
>
>> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>> different boards compile different versions of the source code, meaning
>> that we must build all boards to check for failures. It is easy to misspell
>> an #ifdef and there is not as much checking of this by the compiler. Multiple
>> dependent #ifdefs are harder to do than with if..then..else. Variable
>> declarations must be #idefed as well as the code that uses them, often much
>> later in the file/function. #ifdef indents don't match code indents and
>> have their own separate indent feature. Overall, excessive use of #idef
>> hurts readability and makes the code harder to modify and refactor. For
>> people coming newly into the code base, #ifdefs can be a big barrier.
>>
>> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>> attempt to turn the tide, this series includes a patch which provides a way
>> to make CONFIG macros available to C code without using the preprocessor.
>> This makes it possible to use standard C conditional features such as
>> if/then instead of #ifdef. A README update exhorts compliance.
>
> OK, this is true. Looking over the series, a number of the patches are
> just general fixes / improvements that don't depend on the autoconf_...
> work. Lets split this out now and take them in now as they seem like
> reviewable by inspection code.
Well sorry I didn't make time to get this done last time. I can do
this now or...
>
> For the approach itself, I'm not sure which is best here. In a lot of
> cases we're trading an #ifdef for adding a level of indent to already
> pretty indented code and that feels like a wash, in terms of readability
> to me. We probably need to re-factor some of the code in question there
> too for readability, then see about using autoconf_... type things, or
> maybe something else.
I think you are saying to do the rearrangement and clean-up first,
then add autoconf afterwards. I can do that but really I am wondering
what you think of the autoconf concept? The Kconfig stuff is related
here too, but first I would like to decide on what to do with the
#ifdefs.
Regards,
Simon
>
> --
> Tom
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-13 22:12 ` Simon Glass
@ 2013-05-14 21:21 ` Tom Rini
2013-05-14 21:27 ` Simon Glass
0 siblings, 1 reply; 37+ messages in thread
From: Tom Rini @ 2013-05-14 21:21 UTC (permalink / raw)
To: u-boot
On Mon, May 13, 2013 at 03:12:07PM -0700, Simon Glass wrote:
> Hi Tom,
>
> On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini <trini@ti.com> wrote:
> > On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
> >
> >> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> >> different boards compile different versions of the source code, meaning
> >> that we must build all boards to check for failures. It is easy to misspell
> >> an #ifdef and there is not as much checking of this by the compiler. Multiple
> >> dependent #ifdefs are harder to do than with if..then..else. Variable
> >> declarations must be #idefed as well as the code that uses them, often much
> >> later in the file/function. #ifdef indents don't match code indents and
> >> have their own separate indent feature. Overall, excessive use of #idef
> >> hurts readability and makes the code harder to modify and refactor. For
> >> people coming newly into the code base, #ifdefs can be a big barrier.
> >>
> >> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
> >> attempt to turn the tide, this series includes a patch which provides a way
> >> to make CONFIG macros available to C code without using the preprocessor.
> >> This makes it possible to use standard C conditional features such as
> >> if/then instead of #ifdef. A README update exhorts compliance.
> >
> > OK, this is true. Looking over the series, a number of the patches are
> > just general fixes / improvements that don't depend on the autoconf_...
> > work. Lets split this out now and take them in now as they seem like
> > reviewable by inspection code.
>
> Well sorry I didn't make time to get this done last time. I can do
> this now or...
>
> >
> > For the approach itself, I'm not sure which is best here. In a lot of
> > cases we're trading an #ifdef for adding a level of indent to already
> > pretty indented code and that feels like a wash, in terms of readability
> > to me. We probably need to re-factor some of the code in question there
> > too for readability, then see about using autoconf_... type things, or
> > maybe something else.
>
> I think you are saying to do the rearrangement and clean-up first,
> then add autoconf afterwards. I can do that but really I am wondering
> what you think of the autoconf concept? The Kconfig stuff is related
> here too, but first I would like to decide on what to do with the
> #ifdefs.
I think a lot of our #ifdefery is a problem of code that's in need of
some love and re-org and cleaning and updating. One of the old style
rules I still try and follow is that after a few levels of indent code
doesn't read well. Also big nested #ifdefs don't read well. So we're
trading one in for the other. But your series showed a lot of places
where we can re-factor things to improve readability. So lets go that
way. Then we can see if there's still things to improve on, and what
dead code we still have around.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130514/9bea75fa/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-14 21:21 ` Tom Rini
@ 2013-05-14 21:27 ` Simon Glass
2013-05-14 22:15 ` Tom Rini
0 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-05-14 21:27 UTC (permalink / raw)
To: u-boot
Hi Tom,
On Tue, May 14, 2013 at 2:21 PM, Tom Rini <trini@ti.com> wrote:
> On Mon, May 13, 2013 at 03:12:07PM -0700, Simon Glass wrote:
>> Hi Tom,
>>
>> On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini <trini@ti.com> wrote:
>> > On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
>> >
>> >> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>> >> different boards compile different versions of the source code, meaning
>> >> that we must build all boards to check for failures. It is easy to misspell
>> >> an #ifdef and there is not as much checking of this by the compiler. Multiple
>> >> dependent #ifdefs are harder to do than with if..then..else. Variable
>> >> declarations must be #idefed as well as the code that uses them, often much
>> >> later in the file/function. #ifdef indents don't match code indents and
>> >> have their own separate indent feature. Overall, excessive use of #idef
>> >> hurts readability and makes the code harder to modify and refactor. For
>> >> people coming newly into the code base, #ifdefs can be a big barrier.
>> >>
>> >> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>> >> attempt to turn the tide, this series includes a patch which provides a way
>> >> to make CONFIG macros available to C code without using the preprocessor.
>> >> This makes it possible to use standard C conditional features such as
>> >> if/then instead of #ifdef. A README update exhorts compliance.
>> >
>> > OK, this is true. Looking over the series, a number of the patches are
>> > just general fixes / improvements that don't depend on the autoconf_...
>> > work. Lets split this out now and take them in now as they seem like
>> > reviewable by inspection code.
>>
>> Well sorry I didn't make time to get this done last time. I can do
>> this now or...
>>
>> >
>> > For the approach itself, I'm not sure which is best here. In a lot of
>> > cases we're trading an #ifdef for adding a level of indent to already
>> > pretty indented code and that feels like a wash, in terms of readability
>> > to me. We probably need to re-factor some of the code in question there
>> > too for readability, then see about using autoconf_... type things, or
>> > maybe something else.
>>
>> I think you are saying to do the rearrangement and clean-up first,
>> then add autoconf afterwards. I can do that but really I am wondering
>> what you think of the autoconf concept? The Kconfig stuff is related
>> here too, but first I would like to decide on what to do with the
>> #ifdefs.
>
> I think a lot of our #ifdefery is a problem of code that's in need of
> some love and re-org and cleaning and updating. One of the old style
> rules I still try and follow is that after a few levels of indent code
> doesn't read well. Also big nested #ifdefs don't read well. So we're
> trading one in for the other. But your series showed a lot of places
> where we can re-factor things to improve readability. So lets go that
> way. Then we can see if there's still things to improve on, and what
> dead code we still have around.
So are you saying that you are keen on the autoconf idea?
>
> --
> Tom
Regards,
Simon
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-14 21:27 ` Simon Glass
@ 2013-05-14 22:15 ` Tom Rini
2013-05-14 22:22 ` Vadim Bendebury
0 siblings, 1 reply; 37+ messages in thread
From: Tom Rini @ 2013-05-14 22:15 UTC (permalink / raw)
To: u-boot
On Tue, May 14, 2013 at 02:27:32PM -0700, Simon Glass wrote:
> Hi Tom,
>
> On Tue, May 14, 2013 at 2:21 PM, Tom Rini <trini@ti.com> wrote:
> > On Mon, May 13, 2013 at 03:12:07PM -0700, Simon Glass wrote:
> >> Hi Tom,
> >>
> >> On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini <trini@ti.com> wrote:
> >> > On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
> >> >
> >> >> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> >> >> different boards compile different versions of the source code, meaning
> >> >> that we must build all boards to check for failures. It is easy to misspell
> >> >> an #ifdef and there is not as much checking of this by the compiler. Multiple
> >> >> dependent #ifdefs are harder to do than with if..then..else. Variable
> >> >> declarations must be #idefed as well as the code that uses them, often much
> >> >> later in the file/function. #ifdef indents don't match code indents and
> >> >> have their own separate indent feature. Overall, excessive use of #idef
> >> >> hurts readability and makes the code harder to modify and refactor. For
> >> >> people coming newly into the code base, #ifdefs can be a big barrier.
> >> >>
> >> >> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
> >> >> attempt to turn the tide, this series includes a patch which provides a way
> >> >> to make CONFIG macros available to C code without using the preprocessor.
> >> >> This makes it possible to use standard C conditional features such as
> >> >> if/then instead of #ifdef. A README update exhorts compliance.
> >> >
> >> > OK, this is true. Looking over the series, a number of the patches are
> >> > just general fixes / improvements that don't depend on the autoconf_...
> >> > work. Lets split this out now and take them in now as they seem like
> >> > reviewable by inspection code.
> >>
> >> Well sorry I didn't make time to get this done last time. I can do
> >> this now or...
> >>
> >> >
> >> > For the approach itself, I'm not sure which is best here. In a lot of
> >> > cases we're trading an #ifdef for adding a level of indent to already
> >> > pretty indented code and that feels like a wash, in terms of readability
> >> > to me. We probably need to re-factor some of the code in question there
> >> > too for readability, then see about using autoconf_... type things, or
> >> > maybe something else.
> >>
> >> I think you are saying to do the rearrangement and clean-up first,
> >> then add autoconf afterwards. I can do that but really I am wondering
> >> what you think of the autoconf concept? The Kconfig stuff is related
> >> here too, but first I would like to decide on what to do with the
> >> #ifdefs.
> >
> > I think a lot of our #ifdefery is a problem of code that's in need of
> > some love and re-org and cleaning and updating. One of the old style
> > rules I still try and follow is that after a few levels of indent code
> > doesn't read well. Also big nested #ifdefs don't read well. So we're
> > trading one in for the other. But your series showed a lot of places
> > where we can re-factor things to improve readability. So lets go that
> > way. Then we can see if there's still things to improve on, and what
> > dead code we still have around.
>
> So are you saying that you are keen on the autoconf idea?
I'm saying lets clean up the code and see if we still need something
like autoconf. It seems to provide the most benefit in terms of
readability in places that could read a lot better with some clean up
and refactoring before we add new tools to the pile.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130514/11e50ace/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-14 22:15 ` Tom Rini
@ 2013-05-14 22:22 ` Vadim Bendebury
2013-05-14 22:28 ` Tom Rini
0 siblings, 1 reply; 37+ messages in thread
From: Vadim Bendebury @ 2013-05-14 22:22 UTC (permalink / raw)
To: u-boot
On Tue, May 14, 2013 at 3:15 PM, Tom Rini <trini@ti.com> wrote:
> On Tue, May 14, 2013 at 02:27:32PM -0700, Simon Glass wrote:
>> Hi Tom,
>>
>> On Tue, May 14, 2013 at 2:21 PM, Tom Rini <trini@ti.com> wrote:
>> > On Mon, May 13, 2013 at 03:12:07PM -0700, Simon Glass wrote:
>> >> Hi Tom,
>> >>
>> >> On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini <trini@ti.com> wrote:
>> >> > On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
>> >> >
>> >> >> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>> >> >> different boards compile different versions of the source code, meaning
>> >> >> that we must build all boards to check for failures. It is easy to misspell
>> >> >> an #ifdef and there is not as much checking of this by the compiler. Multiple
>> >> >> dependent #ifdefs are harder to do than with if..then..else. Variable
>> >> >> declarations must be #idefed as well as the code that uses them, often much
>> >> >> later in the file/function. #ifdef indents don't match code indents and
>> >> >> have their own separate indent feature. Overall, excessive use of #idef
>> >> >> hurts readability and makes the code harder to modify and refactor. For
>> >> >> people coming newly into the code base, #ifdefs can be a big barrier.
>> >> >>
>> >> >> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>> >> >> attempt to turn the tide, this series includes a patch which provides a way
>> >> >> to make CONFIG macros available to C code without using the preprocessor.
>> >> >> This makes it possible to use standard C conditional features such as
>> >> >> if/then instead of #ifdef. A README update exhorts compliance.
>> >> >
>> >> > OK, this is true. Looking over the series, a number of the patches are
>> >> > just general fixes / improvements that don't depend on the autoconf_...
>> >> > work. Lets split this out now and take them in now as they seem like
>> >> > reviewable by inspection code.
>> >>
>> >> Well sorry I didn't make time to get this done last time. I can do
>> >> this now or...
>> >>
>> >> >
>> >> > For the approach itself, I'm not sure which is best here. In a lot of
>> >> > cases we're trading an #ifdef for adding a level of indent to already
>> >> > pretty indented code and that feels like a wash, in terms of readability
>> >> > to me. We probably need to re-factor some of the code in question there
>> >> > too for readability, then see about using autoconf_... type things, or
>> >> > maybe something else.
>> >>
>> >> I think you are saying to do the rearrangement and clean-up first,
>> >> then add autoconf afterwards. I can do that but really I am wondering
>> >> what you think of the autoconf concept? The Kconfig stuff is related
>> >> here too, but first I would like to decide on what to do with the
>> >> #ifdefs.
>> >
>> > I think a lot of our #ifdefery is a problem of code that's in need of
>> > some love and re-org and cleaning and updating. One of the old style
>> > rules I still try and follow is that after a few levels of indent code
>> > doesn't read well. Also big nested #ifdefs don't read well. So we're
>> > trading one in for the other. But your series showed a lot of places
>> > where we can re-factor things to improve readability. So lets go that
>> > way. Then we can see if there's still things to improve on, and what
>> > dead code we still have around.
>>
>> So are you saying that you are keen on the autoconf idea?
>
> I'm saying lets clean up the code and see if we still need something
> like autoconf. It seems to provide the most benefit in terms of
> readability in places that could read a lot better with some clean up
> and refactoring before we add new tools to the pile.
>
Yet another great advantage of autoconf is that it ensures a
consistent combination of the configuration options, with the status
quo it is so easy to make a mistake and create a deficient
configuration.
--vb
> --
> Tom
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-14 22:22 ` Vadim Bendebury
@ 2013-05-14 22:28 ` Tom Rini
2013-05-15 14:13 ` Simon Glass
0 siblings, 1 reply; 37+ messages in thread
From: Tom Rini @ 2013-05-14 22:28 UTC (permalink / raw)
To: u-boot
On Tue, May 14, 2013 at 03:22:04PM -0700, Vadim Bendebury wrote:
> On Tue, May 14, 2013 at 3:15 PM, Tom Rini <trini@ti.com> wrote:
> > On Tue, May 14, 2013 at 02:27:32PM -0700, Simon Glass wrote:
> >> Hi Tom,
> >>
> >> On Tue, May 14, 2013 at 2:21 PM, Tom Rini <trini@ti.com> wrote:
> >> > On Mon, May 13, 2013 at 03:12:07PM -0700, Simon Glass wrote:
> >> >> Hi Tom,
> >> >>
> >> >> On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini <trini@ti.com> wrote:
> >> >> > On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
> >> >> >
> >> >> >> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> >> >> >> different boards compile different versions of the source code, meaning
> >> >> >> that we must build all boards to check for failures. It is easy to misspell
> >> >> >> an #ifdef and there is not as much checking of this by the compiler. Multiple
> >> >> >> dependent #ifdefs are harder to do than with if..then..else. Variable
> >> >> >> declarations must be #idefed as well as the code that uses them, often much
> >> >> >> later in the file/function. #ifdef indents don't match code indents and
> >> >> >> have their own separate indent feature. Overall, excessive use of #idef
> >> >> >> hurts readability and makes the code harder to modify and refactor. For
> >> >> >> people coming newly into the code base, #ifdefs can be a big barrier.
> >> >> >>
> >> >> >> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
> >> >> >> attempt to turn the tide, this series includes a patch which provides a way
> >> >> >> to make CONFIG macros available to C code without using the preprocessor.
> >> >> >> This makes it possible to use standard C conditional features such as
> >> >> >> if/then instead of #ifdef. A README update exhorts compliance.
> >> >> >
> >> >> > OK, this is true. Looking over the series, a number of the patches are
> >> >> > just general fixes / improvements that don't depend on the autoconf_...
> >> >> > work. Lets split this out now and take them in now as they seem like
> >> >> > reviewable by inspection code.
> >> >>
> >> >> Well sorry I didn't make time to get this done last time. I can do
> >> >> this now or...
> >> >>
> >> >> >
> >> >> > For the approach itself, I'm not sure which is best here. In a lot of
> >> >> > cases we're trading an #ifdef for adding a level of indent to already
> >> >> > pretty indented code and that feels like a wash, in terms of readability
> >> >> > to me. We probably need to re-factor some of the code in question there
> >> >> > too for readability, then see about using autoconf_... type things, or
> >> >> > maybe something else.
> >> >>
> >> >> I think you are saying to do the rearrangement and clean-up first,
> >> >> then add autoconf afterwards. I can do that but really I am wondering
> >> >> what you think of the autoconf concept? The Kconfig stuff is related
> >> >> here too, but first I would like to decide on what to do with the
> >> >> #ifdefs.
> >> >
> >> > I think a lot of our #ifdefery is a problem of code that's in need of
> >> > some love and re-org and cleaning and updating. One of the old style
> >> > rules I still try and follow is that after a few levels of indent code
> >> > doesn't read well. Also big nested #ifdefs don't read well. So we're
> >> > trading one in for the other. But your series showed a lot of places
> >> > where we can re-factor things to improve readability. So lets go that
> >> > way. Then we can see if there's still things to improve on, and what
> >> > dead code we still have around.
> >>
> >> So are you saying that you are keen on the autoconf idea?
> >
> > I'm saying lets clean up the code and see if we still need something
> > like autoconf. It seems to provide the most benefit in terms of
> > readability in places that could read a lot better with some clean up
> > and refactoring before we add new tools to the pile.
> >
>
> Yet another great advantage of autoconf is that it ensures a
> consistent combination of the configuration options, with the status
> quo it is so easy to make a mistake and create a deficient
> configuration.
There are things I like about the concept, but I really want to see the
problem areas in question made more readable as it will both help in
general, and if we do make this conversion later, make the conversion
easier as we'll hopefully kill off some of the nested and tricky ifdefs.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130514/ba5b855c/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-14 22:28 ` Tom Rini
@ 2013-05-15 14:13 ` Simon Glass
2013-05-16 16:26 ` Tom Rini
0 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-05-15 14:13 UTC (permalink / raw)
To: u-boot
Hi Tom,
On Tue, May 14, 2013 at 3:28 PM, Tom Rini <trini@ti.com> wrote:
> On Tue, May 14, 2013 at 03:22:04PM -0700, Vadim Bendebury wrote:
>> On Tue, May 14, 2013 at 3:15 PM, Tom Rini <trini@ti.com> wrote:
>> > On Tue, May 14, 2013 at 02:27:32PM -0700, Simon Glass wrote:
>> >> Hi Tom,
>> >>
>> >> On Tue, May 14, 2013 at 2:21 PM, Tom Rini <trini@ti.com> wrote:
>> >> > On Mon, May 13, 2013 at 03:12:07PM -0700, Simon Glass wrote:
>> >> >> Hi Tom,
>> >> >>
>> >> >> On Thu, Mar 21, 2013 at 7:38 AM, Tom Rini <trini@ti.com> wrote:
>> >> >> > On Tue, Feb 26, 2013 at 08:10:53AM -0800, Simon Glass wrote:
>> >> >> >
>> >> >> >> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
>> >> >> >> different boards compile different versions of the source code, meaning
>> >> >> >> that we must build all boards to check for failures. It is easy to misspell
>> >> >> >> an #ifdef and there is not as much checking of this by the compiler. Multiple
>> >> >> >> dependent #ifdefs are harder to do than with if..then..else. Variable
>> >> >> >> declarations must be #idefed as well as the code that uses them, often much
>> >> >> >> later in the file/function. #ifdef indents don't match code indents and
>> >> >> >> have their own separate indent feature. Overall, excessive use of #idef
>> >> >> >> hurts readability and makes the code harder to modify and refactor. For
>> >> >> >> people coming newly into the code base, #ifdefs can be a big barrier.
>> >> >> >>
>> >> >> >> The use of #ifdef in U-Boot has possibly got a little out of hand. In an
>> >> >> >> attempt to turn the tide, this series includes a patch which provides a way
>> >> >> >> to make CONFIG macros available to C code without using the preprocessor.
>> >> >> >> This makes it possible to use standard C conditional features such as
>> >> >> >> if/then instead of #ifdef. A README update exhorts compliance.
>> >> >> >
>> >> >> > OK, this is true. Looking over the series, a number of the patches are
>> >> >> > just general fixes / improvements that don't depend on the autoconf_...
>> >> >> > work. Lets split this out now and take them in now as they seem like
>> >> >> > reviewable by inspection code.
>> >> >>
>> >> >> Well sorry I didn't make time to get this done last time. I can do
>> >> >> this now or...
>> >> >>
>> >> >> >
>> >> >> > For the approach itself, I'm not sure which is best here. In a lot of
>> >> >> > cases we're trading an #ifdef for adding a level of indent to already
>> >> >> > pretty indented code and that feels like a wash, in terms of readability
>> >> >> > to me. We probably need to re-factor some of the code in question there
>> >> >> > too for readability, then see about using autoconf_... type things, or
>> >> >> > maybe something else.
>> >> >>
>> >> >> I think you are saying to do the rearrangement and clean-up first,
>> >> >> then add autoconf afterwards. I can do that but really I am wondering
>> >> >> what you think of the autoconf concept? The Kconfig stuff is related
>> >> >> here too, but first I would like to decide on what to do with the
>> >> >> #ifdefs.
>> >> >
>> >> > I think a lot of our #ifdefery is a problem of code that's in need of
>> >> > some love and re-org and cleaning and updating. One of the old style
>> >> > rules I still try and follow is that after a few levels of indent code
>> >> > doesn't read well. Also big nested #ifdefs don't read well. So we're
>> >> > trading one in for the other. But your series showed a lot of places
>> >> > where we can re-factor things to improve readability. So lets go that
>> >> > way. Then we can see if there's still things to improve on, and what
>> >> > dead code we still have around.
>> >>
>> >> So are you saying that you are keen on the autoconf idea?
>> >
>> > I'm saying lets clean up the code and see if we still need something
>> > like autoconf. It seems to provide the most benefit in terms of
>> > readability in places that could read a lot better with some clean up
>> > and refactoring before we add new tools to the pile.
>> >
>>
>> Yet another great advantage of autoconf is that it ensures a
>> consistent combination of the configuration options, with the status
>> quo it is so easy to make a mistake and create a deficient
>> configuration.
>
> There are things I like about the concept, but I really want to see the
> problem areas in question made more readable as it will both help in
> general, and if we do make this conversion later, make the conversion
> easier as we'll hopefully kill off some of the nested and tricky ifdefs.
I've brought over the patches that I can that don't depend on autoconf:
6c0e6c9 (HEAD, ws/us-config5, us-config5) main: Add debug_bootkeys to
avoid #ifdefs
9777b9f main: Add debug_parser() to avoid #ifdefs
2fc85b6 main: Correct header order
24e80be main: Fix typos and checkpatch warnings in command line reading
9e9e3b9 main: Use get/setenv_ulong()
1290cb7 main: Move boot_delay code into its own function
cd8f13e main: Separate out the two abortboot() functions
ca2451c net: Add prototype for update_tftp
4a2a802 at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263
I will run these through the builder and send an interim series
without autoconf. We still have a lot of inline #ifdefs, and static
functions and local variables must also be #ifdefed out if not used.
Still, there is some improvement.
Regards,
Simon
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-15 14:13 ` Simon Glass
@ 2013-05-16 16:26 ` Tom Rini
2013-05-16 18:49 ` Simon Glass
0 siblings, 1 reply; 37+ messages in thread
From: Tom Rini @ 2013-05-16 16:26 UTC (permalink / raw)
To: u-boot
On Wed, May 15, 2013 at 07:13:18AM -0700, Simon Glass wrote:
[snip]
> I've brought over the patches that I can that don't depend on autoconf:
>
> 6c0e6c9 (HEAD, ws/us-config5, us-config5) main: Add debug_bootkeys to
> avoid #ifdefs
> 9777b9f main: Add debug_parser() to avoid #ifdefs
> 2fc85b6 main: Correct header order
> 24e80be main: Fix typos and checkpatch warnings in command line reading
> 9e9e3b9 main: Use get/setenv_ulong()
> 1290cb7 main: Move boot_delay code into its own function
> cd8f13e main: Separate out the two abortboot() functions
> ca2451c net: Add prototype for update_tftp
> 4a2a802 at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263
>
> I will run these through the builder and send an interim series
> without autoconf. We still have a lot of inline #ifdefs, and static
> functions and local variables must also be #ifdefed out if not used.
> Still, there is some improvement.
I've read them over and they look good. Since you didn't re-send
anything I assume they passed your builder test and I'll apply them next
week (and give them my own builder test like everything else gets).
Thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130516/2b82365b/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-16 16:26 ` Tom Rini
@ 2013-05-16 18:49 ` Simon Glass
2013-05-30 13:57 ` Simon Glass
0 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-05-16 18:49 UTC (permalink / raw)
To: u-boot
Hi Tom,
On Thu, May 16, 2013 at 9:26 AM, Tom Rini <trini@ti.com> wrote:
> On Wed, May 15, 2013 at 07:13:18AM -0700, Simon Glass wrote:
>
> [snip]
>> I've brought over the patches that I can that don't depend on autoconf:
>>
>> 6c0e6c9 (HEAD, ws/us-config5, us-config5) main: Add debug_bootkeys to
>> avoid #ifdefs
>> 9777b9f main: Add debug_parser() to avoid #ifdefs
>> 2fc85b6 main: Correct header order
>> 24e80be main: Fix typos and checkpatch warnings in command line reading
>> 9e9e3b9 main: Use get/setenv_ulong()
>> 1290cb7 main: Move boot_delay code into its own function
>> cd8f13e main: Separate out the two abortboot() functions
>> ca2451c net: Add prototype for update_tftp
>> 4a2a802 at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263
>>
>> I will run these through the builder and send an interim series
>> without autoconf. We still have a lot of inline #ifdefs, and static
>> functions and local variables must also be #ifdefed out if not used.
>> Still, there is some improvement.
>
> I've read them over and they look good. Since you didn't re-send
> anything I assume they passed your builder test and I'll apply them next
> week (and give them my own builder test like everything else gets).
> Thanks!
Yes I did a full build and saw no additional failures from my patch.
But then I did a few whitespace changes and only built a few boards
after that. I think it's OK though.
Regards,
Simon
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-16 18:49 ` Simon Glass
@ 2013-05-30 13:57 ` Simon Glass
2013-05-30 15:17 ` Tom Rini
0 siblings, 1 reply; 37+ messages in thread
From: Simon Glass @ 2013-05-30 13:57 UTC (permalink / raw)
To: u-boot
Hi Tom,
On Thu, May 16, 2013 at 11:49 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Tom,
>
> On Thu, May 16, 2013 at 9:26 AM, Tom Rini <trini@ti.com> wrote:
> > On Wed, May 15, 2013 at 07:13:18AM -0700, Simon Glass wrote:
> >
> > [snip]
> >> I've brought over the patches that I can that don't depend on autoconf:
> >>
> >> 6c0e6c9 (HEAD, ws/us-config5, us-config5) main: Add debug_bootkeys to
> >> avoid #ifdefs
> >> 9777b9f main: Add debug_parser() to avoid #ifdefs
> >> 2fc85b6 main: Correct header order
> >> 24e80be main: Fix typos and checkpatch warnings in command line reading
> >> 9e9e3b9 main: Use get/setenv_ulong()
> >> 1290cb7 main: Move boot_delay code into its own function
> >> cd8f13e main: Separate out the two abortboot() functions
> >> ca2451c net: Add prototype for update_tftp
> >> 4a2a802 at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263
> >>
> >> I will run these through the builder and send an interim series
> >> without autoconf. We still have a lot of inline #ifdefs, and static
> >> functions and local variables must also be #ifdefed out if not used.
> >> Still, there is some improvement.
> >
> > I've read them over and they look good. Since you didn't re-send
> > anything I assume they passed your builder test and I'll apply them next
> > week (and give them my own builder test like everything else gets).
> > Thanks!
>
Just checking up if you got to this? I am thinking of respinning the
autoconf series on top of this.
>
> Yes I did a full build and saw no additional failures from my patch.
> But then I did a few whitespace changes and only built a few boards
> after that. I think it's OK though.
>
> Regards,
Simon
^ permalink raw reply [flat|nested] 37+ messages in thread
* [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere
2013-05-30 13:57 ` Simon Glass
@ 2013-05-30 15:17 ` Tom Rini
0 siblings, 0 replies; 37+ messages in thread
From: Tom Rini @ 2013-05-30 15:17 UTC (permalink / raw)
To: u-boot
On Thu, May 30, 2013 at 06:57:02AM -0700, Simon Glass wrote:
> Hi Tom,
>
> On Thu, May 16, 2013 at 11:49 AM, Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Tom,
> >
> > On Thu, May 16, 2013 at 9:26 AM, Tom Rini <trini@ti.com> wrote:
> > > On Wed, May 15, 2013 at 07:13:18AM -0700, Simon Glass wrote:
> > >
> > > [snip]
> > >> I've brought over the patches that I can that don't depend on autoconf:
> > >>
> > >> 6c0e6c9 (HEAD, ws/us-config5, us-config5) main: Add debug_bootkeys to
> > >> avoid #ifdefs
> > >> 9777b9f main: Add debug_parser() to avoid #ifdefs
> > >> 2fc85b6 main: Correct header order
> > >> 24e80be main: Fix typos and checkpatch warnings in command line reading
> > >> 9e9e3b9 main: Use get/setenv_ulong()
> > >> 1290cb7 main: Move boot_delay code into its own function
> > >> cd8f13e main: Separate out the two abortboot() functions
> > >> ca2451c net: Add prototype for update_tftp
> > >> 4a2a802 at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263
> > >>
> > >> I will run these through the builder and send an interim series
> > >> without autoconf. We still have a lot of inline #ifdefs, and static
> > >> functions and local variables must also be #ifdefed out if not used.
> > >> Still, there is some improvement.
> > >
> > > I've read them over and they look good. Since you didn't re-send
> > > anything I assume they passed your builder test and I'll apply them next
> > > week (and give them my own builder test like everything else gets).
> > > Thanks!
> >
>
> Just checking up if you got to this? I am thinking of respinning the
> autoconf series on top of this.
Looking to clear up my patchwork TODO list tomorrow and this is up in
there for eligible patches. Thanks!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130530/9296c023/attachment.pgp>
^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2013-05-30 15:17 UTC | newest]
Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-26 16:10 [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 01/16] Implement autoconf header file Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 02/16] at91: Correct CONFIG_AUTOBOOT_PROMPT definition for pm9263 Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 03/16] net: Add prototype for update_tftp, and use autoconf Simon Glass
2013-03-20 19:40 ` Tom Rini
2013-03-20 19:57 ` Simon Glass
2013-03-20 20:30 ` Tom Rini
2013-03-21 1:47 ` Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 04/16] main: Separate out the two abortboot() functions Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 05/16] main: Move boot_delay code into its own function Simon Glass
2013-02-26 16:10 ` [U-Boot] [PATCH v3 06/16] main: Use autoconf for boot retry feature Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 07/16] main: Remove CONFIG #ifdefs from the abortboot() code Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 08/16] main: Use get/setenv_ulong() Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 09/16] main: Use autoconf for boot_delay code Simon Glass
2013-02-27 9:14 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 10/16] main: Use autoconf for parser selection Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 11/16] main: Fix typos and checkpatch warnings in command line reading Simon Glass
2013-02-27 9:25 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 12/16] main: Use autoconf " Simon Glass
2013-02-27 9:17 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 13/16] main: Use autoconf in main_loop() Simon Glass
2013-02-27 9:23 ` Joe Hershberger
2013-02-26 16:11 ` [U-Boot] [PATCH v3 14/16] main: Correct header order Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 15/16] main: Add debug_parser() to avoid #ifdefs Simon Glass
2013-02-26 16:11 ` [U-Boot] [PATCH v3 16/16] main: Add debug_bootkeys " Simon Glass
2013-03-21 14:38 ` [U-Boot] [PATCH v3 0/16] Provide a mechanism to avoid using #ifdef everywhere Tom Rini
2013-05-13 22:12 ` Simon Glass
2013-05-14 21:21 ` Tom Rini
2013-05-14 21:27 ` Simon Glass
2013-05-14 22:15 ` Tom Rini
2013-05-14 22:22 ` Vadim Bendebury
2013-05-14 22:28 ` Tom Rini
2013-05-15 14:13 ` Simon Glass
2013-05-16 16:26 ` Tom Rini
2013-05-16 18:49 ` Simon Glass
2013-05-30 13:57 ` Simon Glass
2013-05-30 15:17 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.