Dear Simon,
In message <1361207920-24983-1-git-send-email-...@chromium.org> you wrote:
> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes
> different boards compile different versions of the source code, meaning
> that we must build all boards to check for failures. It is easy to misspell
...
> +# Create a C header file where every '#define CONFIG_XXX value' becomes
> +# '#define config_xxx() value', or '#define config_xxx() 0' where the CONFIG
> +# is not used by this board configuration. This allows C code to do things
> +# like 'if (config_xxx())' and have the compiler remove the dead code,
> +# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases
> +# if the config_...() returns 0 then the option is not enabled. In some rare
> +# cases such as CONFIG_BOOTDELAY, the config can be enabled but still have a
> +# a value of 0. So in addition we a #define config_xxx_enabled(), setting the
> +# value to 0 if the option is disabled, 1 if enabled. This last feature will
> +# hopefully be deprecated soon.
I think config_* is not a good name to use here - it has never been a
reserved prefix so far, so it is IMO a bad idea to turn it into one
now . We already have quite a number such variable names in the code
all over the place (not sure I caught them all):
config_2 config_io_ctrl
config_8260_ioports config_kbc_gpio
config_8560_ioports config_list
config_address config_list_t
config_backside_crossbar_mux config_mpc8xx_ioports
config_base config_nfc_clk
config_bit config_node
config_buf config_on_ebc_cs4_is_small_flash
config_byte config_pci_bridge
config_change config_periph_clk
config_clock config_pin
config_cmd_ctrl config_pll_clk
config_core_clk config_qe_ioports
config_data config_reg
config_data_eye_leveling_samples config_reg_helper
config_ddr config_s
config_ddr_clk config_sdram
config_ddr_data config_select_P
config_ddr_phy config_selection
config_desc config_selection_t
config_device config_status
config_fifo config_table
config_file_size config_val
config_frontside_crossbar_vsc3316 config_val_P
config_genmii_advert config_validity
config_hub_port config_validity_t
config_id config_vtp
config_instance
> +The compiler will see that config_xxx() evalutes to a constant and will
> +eliminate the dead code. The resulting code (and code size) is the same.
Did you measure the impact on compile time?
> +This takes care of almost all CONFIG macros. Unfortunately there are a few
> +cases where a value of 0 does not mean the option is disabled. For example
> +CONFIG_BOOTDELAY can be defined to 0, which means that the bootdelay
> +code should be used, but with a value of 0. To get around this and other
> +sticky cases, an addition macro with an '_enabled' suffix is provided, where
> +the value is always either 0 or 1:
> +
> + // Will work even if boaard config has '#define CONFIG_BOOTDELAY 0'
> + if (config_bootdelay_enabled())
> + do_something;
> +
> +(Probably such config options should be deprecated and then we can remove
> +this feature)
These are perfectly valid cases, I think - there are quite a number of
these, including defines for console index, PHY address, etc. etc.
> --- a/common/cmd_fitupd.c
> +++ b/common/cmd_fitupd.c
> @@ -8,6 +8,7 @@
>
> #include <common.h>
> #include <command.h>
> +#include <net.h>
>
> #if !defined(CONFIG_UPDATE_TFTP)
> #error "CONFIG_UPDATE_TFTP required"
This seems to be an unrelated change?
> diff --git a/common/main.c b/common/main.c
> index e2d2e09..cd42b67 100644
> --- a/common/main.c
> +++ b/common/main.c
...
> -#include <malloc.h> /* for free() prototype */
...
> +#include <malloc.h>
Why dropping the comment?
> -#undef DEBUG_PARSER
> +#define DEBUG_PARSER 0 /* set to 1 to debug */
> +
> +
> +#define debug_parser(fmt, args...) \
> + debug_cond(DEBUG_PARSER, fmt, ##args)
> +
> +#ifndef DEBUG_BOOTKEYS
> +#define DEBUG_BOOTKEYS 0
> +#endif
> +#define debug_bootkeys(fmt, args...) \
> + debug_cond(DEBUG_BOOTKEYS, fmt, ##args)
This is also a different type of changes. Please keep such separate.
> -#ifndef CONFIG_ZERO_BOOTDELAY_CHECK
> - if (bootdelay == 0)
> + if (config_zero_bootdelay_check() && bootdelay == 0)
This appears to be plain wrong. That was a "#ifndef" before...
> + if (config_autoboot_delay_str() && delaykey[0].str == NULL)
> + delaykey[0].str = config_autoboot_delay_str();
Hm.... constructs like these make me think about side effects. As is,
your implementation does not pretect against any. This may be
dangerous - you are evaluating the macro multiple times now, while
before it was only a defined() test, folowed by a single evaluation.
And to be honest - I find the old code easier to read.
> - for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) {
> + for (i = 0; i < ARRAY_SIZE(delaykey); i++) {
Once more, a totally unrelated code change. Please split into
independent commits.
> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK
> /*
> * Check if key already pressed
> * Don't check if bootdelay < 0
> */
> - if (bootdelay >= 0) {
> + if (config_zero_bootdelay_check() && bootdelay >= 0) {
> if (tstc()) { /* we got a key press */
> (void) getc(); /* consume input */
> puts ("\b\b\b 0");
> abort = 1; /* don't auto boot */
> }
> }
> -#endif
I think code like this needs more careful editing.
With the #ifdef, it was clear that the comment only applies if
CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly becomes
valid _always_, which is pretty much misleading to someone trying to
understand this code. I think it would be necessary to rephrase the
commend, and move it partially into the if() block.
> -void main_loop (void)
> +static void process_boot_delay(void)
> {
> -#ifndef CONFIG_SYS_HUSH_PARSER
> - static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, };
> - int len;
> - int rc = 1;
> - int flag;
> -#endif
> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \
> - defined(CONFIG_OF_CONTROL)
> - char *env;
> -#endif
> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
> - char *s;
> - int bootdelay;
> -#endif
> -#ifdef CONFIG_PREBOOT
> - char *p;
> -#endif
> -#ifdef CONFIG_BOOTCOUNT_LIMIT
> unsigned long bootcount = 0;
> unsigned long bootlimit = 0;
> - char *bcs;
> - char bcs_set[16];
> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
> -
> - bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop");
> -
> -#ifdef CONFIG_BOOTCOUNT_LIMIT
> - bootcount = bootcount_load();
> - bootcount++;
> - bootcount_store (bootcount);
> - sprintf (bcs_set, "%lu", bootcount);
> - setenv ("bootcount", bcs_set);
> - bcs = getenv ("bootlimit");
> - bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0;
> -#endif /* CONFIG_BOOTCOUNT_LIMIT */
> -
> -#ifdef CONFIG_MODEM_SUPPORT
> - debug ("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init);
> - if (do_mdm_init) {
> - char *str = strdup(getenv("mdm_cmd"));
> - setenv ("preboot", str); /* set or delete definition */
> - if (str != NULL)
> - free (str);
> - mdm_init(); /* wait for modem connection */
> - }
> -#endif /* CONFIG_MODEM_SUPPORT */
> -
> -#ifdef CONFIG_VERSION_VARIABLE
> - {
> - setenv ("ver", version_string); /* set version variable */
> - }
> -#endif /* CONFIG_VERSION_VARIABLE */
> -
> -#ifdef CONFIG_SYS_HUSH_PARSER
> - u_boot_hush_start ();
> -#endif
> -
> -#if defined(CONFIG_HUSH_INIT_VAR)
> - hush_init_var ();
> -#endif
> -
> -#ifdef CONFIG_PREBOOT
> - if ((p = getenv ("preboot")) != NULL) {
> -# ifdef CONFIG_AUTOBOOT_KEYED
> - int prev = disable_ctrlc(1); /* disable Control C checking */
> -# endif
> -
> - run_command_list(p, -1, 0);
> + const char *s;
> + int bootdelay;
>
> -# ifdef CONFIG_AUTOBOOT_KEYED
> - disable_ctrlc(prev); /* restore Control C checking */
> -# endif
I'm afraid here the patch becomes unreadable, at least to me - I give
up.
I find the idea interesting and definitely worth to carry on, but it
appears we're still a pretty long way off from usable code.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Too bad that all the people who know how to run the country are busy
driving taxicabs and cutting hair. - George Burns
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
