| Message ID | 1361207920-24983-1-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
Dear Simon, In message <1361207920-24983-1-git-send-email-sjg@chromium.org> you wrote: > Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes > different boards compile different versions of the source code, meaning > that we must build all boards to check for failures. It is easy to misspell ... > +# Create a C header file where every '#define CONFIG_XXX value' becomes > +# '#define config_xxx() value', or '#define config_xxx() 0' where the CONFIG > +# is not used by this board configuration. This allows C code to do things > +# like 'if (config_xxx())' and have the compiler remove the dead code, > +# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases > +# if the config_...() returns 0 then the option is not enabled. In some rare > +# cases such as CONFIG_BOOTDELAY, the config can be enabled but still have a > +# a value of 0. So in addition we a #define config_xxx_enabled(), setting the > +# value to 0 if the option is disabled, 1 if enabled. This last feature will > +# hopefully be deprecated soon. I think config_* is not a good name to use here - it has never been a reserved prefix so far, so it is IMO a bad idea to turn it into one now . We already have quite a number such variable names in the code all over the place (not sure I caught them all): config_2 config_io_ctrl config_8260_ioports config_kbc_gpio config_8560_ioports config_list config_address config_list_t config_backside_crossbar_mux config_mpc8xx_ioports config_base config_nfc_clk config_bit config_node config_buf config_on_ebc_cs4_is_small_flash config_byte config_pci_bridge config_change config_periph_clk config_clock config_pin config_cmd_ctrl config_pll_clk config_core_clk config_qe_ioports config_data config_reg config_data_eye_leveling_samples config_reg_helper config_ddr config_s config_ddr_clk config_sdram config_ddr_data config_select_P config_ddr_phy config_selection config_desc config_selection_t config_device config_status config_fifo config_table config_file_size config_val config_frontside_crossbar_vsc3316 config_val_P config_genmii_advert config_validity config_hub_port config_validity_t config_id config_vtp config_instance > +The compiler will see that config_xxx() evalutes to a constant and will > +eliminate the dead code. The resulting code (and code size) is the same. Did you measure the impact on compile time? > +This takes care of almost all CONFIG macros. Unfortunately there are a few > +cases where a value of 0 does not mean the option is disabled. For example > +CONFIG_BOOTDELAY can be defined to 0, which means that the bootdelay > +code should be used, but with a value of 0. To get around this and other > +sticky cases, an addition macro with an '_enabled' suffix is provided, where > +the value is always either 0 or 1: > + > + // Will work even if boaard config has '#define CONFIG_BOOTDELAY 0' > + if (config_bootdelay_enabled()) > + do_something; > + > +(Probably such config options should be deprecated and then we can remove > +this feature) These are perfectly valid cases, I think - there are quite a number of these, including defines for console index, PHY address, etc. etc. > --- a/common/cmd_fitupd.c > +++ b/common/cmd_fitupd.c > @@ -8,6 +8,7 @@ > > #include <common.h> > #include <command.h> > +#include <net.h> > > #if !defined(CONFIG_UPDATE_TFTP) > #error "CONFIG_UPDATE_TFTP required" This seems to be an unrelated change? > diff --git a/common/main.c b/common/main.c > index e2d2e09..cd42b67 100644 > --- a/common/main.c > +++ b/common/main.c ... > -#include <malloc.h> /* for free() prototype */ ... > +#include <malloc.h> Why dropping the comment? > -#undef DEBUG_PARSER > +#define DEBUG_PARSER 0 /* set to 1 to debug */ > + > + > +#define debug_parser(fmt, args...) \ > + debug_cond(DEBUG_PARSER, fmt, ##args) > + > +#ifndef DEBUG_BOOTKEYS > +#define DEBUG_BOOTKEYS 0 > +#endif > +#define debug_bootkeys(fmt, args...) \ > + debug_cond(DEBUG_BOOTKEYS, fmt, ##args) This is also a different type of changes. Please keep such separate. > -#ifndef CONFIG_ZERO_BOOTDELAY_CHECK > - if (bootdelay == 0) > + if (config_zero_bootdelay_check() && bootdelay == 0) This appears to be plain wrong. That was a "#ifndef" before... > + if (config_autoboot_delay_str() && delaykey[0].str == NULL) > + delaykey[0].str = config_autoboot_delay_str(); Hm.... constructs like these make me think about side effects. As is, your implementation does not pretect against any. This may be dangerous - you are evaluating the macro multiple times now, while before it was only a defined() test, folowed by a single evaluation. And to be honest - I find the old code easier to read. > - for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) { > + for (i = 0; i < ARRAY_SIZE(delaykey); i++) { Once more, a totally unrelated code change. Please split into independent commits. > -#if defined CONFIG_ZERO_BOOTDELAY_CHECK > /* > * Check if key already pressed > * Don't check if bootdelay < 0 > */ > - if (bootdelay >= 0) { > + if (config_zero_bootdelay_check() && bootdelay >= 0) { > if (tstc()) { /* we got a key press */ > (void) getc(); /* consume input */ > puts ("\b\b\b 0"); > abort = 1; /* don't auto boot */ > } > } > -#endif I think code like this needs more careful editing. With the #ifdef, it was clear that the comment only applies if CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly becomes valid _always_, which is pretty much misleading to someone trying to understand this code. I think it would be necessary to rephrase the commend, and move it partially into the if() block. > -void main_loop (void) > +static void process_boot_delay(void) > { > -#ifndef CONFIG_SYS_HUSH_PARSER > - static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, }; > - int len; > - int rc = 1; > - int flag; > -#endif > -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \ > - defined(CONFIG_OF_CONTROL) > - char *env; > -#endif > -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) > - char *s; > - int bootdelay; > -#endif > -#ifdef CONFIG_PREBOOT > - char *p; > -#endif > -#ifdef CONFIG_BOOTCOUNT_LIMIT > unsigned long bootcount = 0; > unsigned long bootlimit = 0; > - char *bcs; > - char bcs_set[16]; > -#endif /* CONFIG_BOOTCOUNT_LIMIT */ > - > - bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop"); > - > -#ifdef CONFIG_BOOTCOUNT_LIMIT > - bootcount = bootcount_load(); > - bootcount++; > - bootcount_store (bootcount); > - sprintf (bcs_set, "%lu", bootcount); > - setenv ("bootcount", bcs_set); > - bcs = getenv ("bootlimit"); > - bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0; > -#endif /* CONFIG_BOOTCOUNT_LIMIT */ > - > -#ifdef CONFIG_MODEM_SUPPORT > - debug ("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init); > - if (do_mdm_init) { > - char *str = strdup(getenv("mdm_cmd")); > - setenv ("preboot", str); /* set or delete definition */ > - if (str != NULL) > - free (str); > - mdm_init(); /* wait for modem connection */ > - } > -#endif /* CONFIG_MODEM_SUPPORT */ > - > -#ifdef CONFIG_VERSION_VARIABLE > - { > - setenv ("ver", version_string); /* set version variable */ > - } > -#endif /* CONFIG_VERSION_VARIABLE */ > - > -#ifdef CONFIG_SYS_HUSH_PARSER > - u_boot_hush_start (); > -#endif > - > -#if defined(CONFIG_HUSH_INIT_VAR) > - hush_init_var (); > -#endif > - > -#ifdef CONFIG_PREBOOT > - if ((p = getenv ("preboot")) != NULL) { > -# ifdef CONFIG_AUTOBOOT_KEYED > - int prev = disable_ctrlc(1); /* disable Control C checking */ > -# endif > - > - run_command_list(p, -1, 0); > + const char *s; > + int bootdelay; > > -# ifdef CONFIG_AUTOBOOT_KEYED > - disable_ctrlc(prev); /* restore Control C checking */ > -# endif I'm afraid here the patch becomes unreadable, at least to me - I give up. I find the idea interesting and definitely worth to carry on, but it appears we're still a pretty long way off from usable code. Best regards, Wolfgang Denk
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 02/18/2013 02:23 PM, Wolfgang Denk wrote: > Dear Simon, > > In message <1361207920-24983-1-git-send-email-sjg@chromium.org> you > wrote: >> Many parts of the U-Boot code base are sprinkled with #ifdefs. >> This makes different boards compile different versions of the >> source code, meaning that we must build all boards to check for >> failures. It is easy to misspell > ... >> +# Create a C header file where every '#define CONFIG_XXX value' >> becomes +# '#define config_xxx() value', or '#define config_xxx() >> 0' where the CONFIG +# is not used by this board configuration. >> This allows C code to do things +# like 'if (config_xxx())' and >> have the compiler remove the dead code, +# instead of using >> '#ifdef CONFIG_XXX...#endif'. Note that in most cases +# if the >> config_...() returns 0 then the option is not enabled. In some >> rare +# cases such as CONFIG_BOOTDELAY, the config can be enabled >> but still have a +# a value of 0. So in addition we a #define >> config_xxx_enabled(), setting the +# value to 0 if the option is >> disabled, 1 if enabled. This last feature will +# hopefully be >> deprecated soon. > > I think config_* is not a good name to use here - it has never been > a reserved prefix so far, so it is IMO a bad idea to turn it into > one now . We already have quite a number such variable names in > the code all over the place (not sure I caught them all): Indeed. It's not a good choice to suddenly reserve now either. build_has_xxx() ? I'm not sure. [snip] >> +The compiler will see that config_xxx() evalutes to a constant >> and will +eliminate the dead code. The resulting code (and code >> size) is the same. > > Did you measure the impact on compile time? Probably won't have a good handle on this without converting a whole build target's needs (just about). [snip] >> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK /* * Check if key >> already pressed * Don't check if bootdelay < 0 */ - if (bootdelay >> >= 0) { + if (config_zero_bootdelay_check() && bootdelay >= 0) { >> if (tstc()) { /* we got a key press */ (void) getc(); /* consume >> input */ puts ("\b\b\b 0"); abort = 1; /* don't auto boot */ } } >> -#endif > > I think code like this needs more careful editing. > > With the #ifdef, it was clear that the comment only applies if > CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly > becomes valid _always_, which is pretty much misleading to someone > trying to understand this code. I think it would be necessary to > rephrase the commend, and move it partially into the if() block. Exactly. This type of change will require a lot of re-commenting to make it clear what's going on now, and after the change even more-so. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQIcBAEBAgAGBQJRIp7WAAoJENk4IS6UOR1W1S0QAKzMt8KkcVdRTFElSjlze35P PxFkO/W8YchPkzwMdhpU4UxHNYoFziLk5pLfj8hhh9uyQ7Lp0I411PZtJ+mkt3I5 RQf8xPHF9PDN2w0ZsxYKd0JE9LgFB/b9EmOOpzxy7Bge3aEGrfnhqgjNgnPGgVgO eCcLGZLrGYlbcL9SOJNxBdFjgCxJvRfrNtsaLOJc5SveeqMNGISp6xc5WDWnmf1f JAHNg7d9ik5d782AC76jbNUVOIp+85N0dMjhCdLR4YZBdNTC5grAW6x7gEGj+vYZ xUE2/Y20FG1Ie3vQjbbW1gUhYtxCxjFLl+UkUOn5bf6F0eDgyUvaSt17nrO3GSbQ hgunWaw9fZoVKMhb1WNnRHmLU5gS9rVlYGGsGibiMs1VPuiYpTLM/kuxfVitxJO0 Ysgkyotgfj2RbnKuBCkMVmvxBzIC3S2bAtxY97TwVrpEh2ZB7r2YwEKak8WhVxyQ nMyMulgpZoMJLM3fJEcF/kQPIzsKtz1Fl/YQXGZlI2lQpohE03kAPVQDyVeTQpGA FzGOUwVZY4WbcKrpcCV4tJOEnHRVyDR8ntQx0udMjtChaLE40fAmmUlWmWrnE4yV SVBM+PS2d7NCXt85QpS0eMc/UdFI0v1i6R24KEHEfqQe1WEdQb1wC7XXYblutZ8z ySlnbeEMfeDg5i5FHX46 =CF0y -----END PGP SIGNATURE-----
Hi Wolfgang, On Mon, Feb 18, 2013 at 11:23 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon, > > In message <1361207920-24983-1-git-send-email-sjg@chromium.org> you wrote: >> Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes >> different boards compile different versions of the source code, meaning >> that we must build all boards to check for failures. It is easy to misspell > ... >> +# Create a C header file where every '#define CONFIG_XXX value' becomes >> +# '#define config_xxx() value', or '#define config_xxx() 0' where the CONFIG >> +# is not used by this board configuration. This allows C code to do things >> +# like 'if (config_xxx())' and have the compiler remove the dead code, >> +# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases >> +# if the config_...() returns 0 then the option is not enabled. In some rare >> +# cases such as CONFIG_BOOTDELAY, the config can be enabled but still have a >> +# a value of 0. So in addition we a #define config_xxx_enabled(), setting the >> +# value to 0 if the option is disabled, 1 if enabled. This last feature will >> +# hopefully be deprecated soon. Thanks for looking at this so closely - it's just an RFC at this stage, but I think it has promise. > > I think config_* is not a good name to use here - it has never been a > reserved prefix so far, so it is IMO a bad idea to turn it into one > now . We already have quite a number such variable names in the code > all over the place (not sure I caught them all): > > config_2 config_io_ctrl > config_8260_ioports config_kbc_gpio > config_8560_ioports config_list > config_address config_list_t > config_backside_crossbar_mux config_mpc8xx_ioports > config_base config_nfc_clk > config_bit config_node > config_buf config_on_ebc_cs4_is_small_flash > config_byte config_pci_bridge > config_change config_periph_clk > config_clock config_pin > config_cmd_ctrl config_pll_clk > config_core_clk config_qe_ioports > config_data config_reg > config_data_eye_leveling_samples config_reg_helper > config_ddr config_s > config_ddr_clk config_sdram > config_ddr_data config_select_P > config_ddr_phy config_selection > config_desc config_selection_t > config_device config_status > config_fifo config_table > config_file_size config_val > config_frontside_crossbar_vsc3316 config_val_P > config_genmii_advert config_validity > config_hub_port config_validity_t > config_id config_vtp > config_instance > These are variables, so won't conflict with the function macros used by this patch. But maybe we should try for something like cfg_...()? > >> +The compiler will see that config_xxx() evalutes to a constant and will >> +eliminate the dead code. The resulting code (and code size) is the same. > > Did you measure the impact on compile time? I did for the 'sed' step - it was a 2-3 seconds to regenerate the header files. Full reconfig/recompile goes from about about 30s to 34s. Incremental build doesn't change noticeably. I also tried recompiling both the mainline U-Boot's main.c 100 times, and then this one. I could not see any time difference, which is a little surprising. Maybe the C compiler's DCE is pretty early in the the process? > >> +This takes care of almost all CONFIG macros. Unfortunately there are a few >> +cases where a value of 0 does not mean the option is disabled. For example >> +CONFIG_BOOTDELAY can be defined to 0, which means that the bootdelay >> +code should be used, but with a value of 0. To get around this and other >> +sticky cases, an addition macro with an '_enabled' suffix is provided, where >> +the value is always either 0 or 1: >> + >> + // Will work even if boaard config has '#define CONFIG_BOOTDELAY 0' >> + if (config_bootdelay_enabled()) >> + do_something; >> + >> +(Probably such config options should be deprecated and then we can remove >> +this feature) > > These are perfectly valid cases, I think - there are quite a number of > these, including defines for console index, PHY address, etc. etc. Well the issue is not so much the value, but whether the value enables something. So for example: config_console_index() is perfectly fine if it just specifies the console index. But if it also enables are particular feature (e.g. turning the console on), then causes problems. I actually think such cases are quite rare, and probably cause other confusion also. > >> --- a/common/cmd_fitupd.c >> +++ b/common/cmd_fitupd.c >> @@ -8,6 +8,7 @@ >> >> #include <common.h> >> #include <command.h> >> +#include <net.h> >> >> #if !defined(CONFIG_UPDATE_TFTP) >> #error "CONFIG_UPDATE_TFTP required" > > This seems to be an unrelated change? Yes - just to make it fail here rather than die with an obscure message later. > > >> diff --git a/common/main.c b/common/main.c >> index e2d2e09..cd42b67 100644 >> --- a/common/main.c >> +++ b/common/main.c > ... >> -#include <malloc.h> /* for free() prototype */ > ... >> +#include <malloc.h> > > Why dropping the comment? Actually main.c includes this file twice - I dropped the one with the comment. There are far to many changes in main.c for the diff to be very useful unfortunately. It's just an RFC, but my intent with main.c was to take the concept as far as I could. > >> -#undef DEBUG_PARSER >> +#define DEBUG_PARSER 0 /* set to 1 to debug */ >> + >> + >> +#define debug_parser(fmt, args...) \ >> + debug_cond(DEBUG_PARSER, fmt, ##args) >> + >> +#ifndef DEBUG_BOOTKEYS >> +#define DEBUG_BOOTKEYS 0 >> +#endif >> +#define debug_bootkeys(fmt, args...) \ >> + debug_cond(DEBUG_BOOTKEYS, fmt, ##args) > > This is also a different type of changes. Please keep such separate. Yes > >> -#ifndef CONFIG_ZERO_BOOTDELAY_CHECK >> - if (bootdelay == 0) >> + if (config_zero_bootdelay_check() && bootdelay == 0) > > This appears to be plain wrong. That was a "#ifndef" before... > >> + if (config_autoboot_delay_str() && delaykey[0].str == NULL) >> + delaykey[0].str = config_autoboot_delay_str(); > > Hm.... constructs like these make me think about side effects. As is, > your implementation does not pretect against any. This may be > dangerous - you are evaluating the macro multiple times now, while > before it was only a defined() test, folowed by a single evaluation. > > And to be honest - I find the old code easier to read. > >> - for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) { >> + for (i = 0; i < ARRAY_SIZE(delaykey); i++) { > > Once more, a totally unrelated code change. Please split into > independent commits. > I will sort out these comments when I put together a proper series for this. > >> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK >> /* >> * Check if key already pressed >> * Don't check if bootdelay < 0 >> */ >> - if (bootdelay >= 0) { >> + if (config_zero_bootdelay_check() && bootdelay >= 0) { >> if (tstc()) { /* we got a key press */ >> (void) getc(); /* consume input */ >> puts ("\b\b\b 0"); >> abort = 1; /* don't auto boot */ >> } >> } >> -#endif > > I think code like this needs more careful editing. > > With the #ifdef, it was clear that the comment only applies if > CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly becomes > valid _always_, which is pretty much misleading to someone trying to > understand this code. I think it would be necessary to rephrase the > commend, and move it partially into the if() block. Yes I see. > > >> -void main_loop (void) >> +static void process_boot_delay(void) >> { >> -#ifndef CONFIG_SYS_HUSH_PARSER >> - static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, }; >> - int len; >> - int rc = 1; >> - int flag; >> -#endif >> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \ >> - defined(CONFIG_OF_CONTROL) >> - char *env; >> -#endif >> -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) >> - char *s; >> - int bootdelay; >> -#endif >> -#ifdef CONFIG_PREBOOT >> - char *p; >> -#endif >> -#ifdef CONFIG_BOOTCOUNT_LIMIT >> unsigned long bootcount = 0; >> unsigned long bootlimit = 0; >> - char *bcs; >> - char bcs_set[16]; >> -#endif /* CONFIG_BOOTCOUNT_LIMIT */ >> - >> - bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop"); >> - >> -#ifdef CONFIG_BOOTCOUNT_LIMIT >> - bootcount = bootcount_load(); >> - bootcount++; >> - bootcount_store (bootcount); >> - sprintf (bcs_set, "%lu", bootcount); >> - setenv ("bootcount", bcs_set); >> - bcs = getenv ("bootlimit"); >> - bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0; >> -#endif /* CONFIG_BOOTCOUNT_LIMIT */ >> - >> -#ifdef CONFIG_MODEM_SUPPORT >> - debug ("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init); >> - if (do_mdm_init) { >> - char *str = strdup(getenv("mdm_cmd")); >> - setenv ("preboot", str); /* set or delete definition */ >> - if (str != NULL) >> - free (str); >> - mdm_init(); /* wait for modem connection */ >> - } >> -#endif /* CONFIG_MODEM_SUPPORT */ >> - >> -#ifdef CONFIG_VERSION_VARIABLE >> - { >> - setenv ("ver", version_string); /* set version variable */ >> - } >> -#endif /* CONFIG_VERSION_VARIABLE */ >> - >> -#ifdef CONFIG_SYS_HUSH_PARSER >> - u_boot_hush_start (); >> -#endif >> - >> -#if defined(CONFIG_HUSH_INIT_VAR) >> - hush_init_var (); >> -#endif >> - >> -#ifdef CONFIG_PREBOOT >> - if ((p = getenv ("preboot")) != NULL) { >> -# ifdef CONFIG_AUTOBOOT_KEYED >> - int prev = disable_ctrlc(1); /* disable Control C checking */ >> -# endif >> - >> - run_command_list(p, -1, 0); >> + const char *s; >> + int bootdelay; >> >> -# ifdef CONFIG_AUTOBOOT_KEYED >> - disable_ctrlc(prev); /* restore Control C checking */ >> -# endif > > I'm afraid here the patch becomes unreadable, at least to me - I give > up. > Agreed, it's just too hard to follow. in this form. What is happening here is that I split the boot delay code into two functions, one for each of the available options. It really needs a number of smaller patches to be useful. But if you have time, please take a look at the sed scripts and the Makefile. > > I find the idea interesting and definitely worth to carry on, but it > appears we're still a pretty long way off from usable code. OK good. The background here is that I have been wondering about this for a while, but have never really come up with a good way (in the absence of a unified Kconfig) of getting a complete list of CONFIG variables. There was a mailing list post about this a few weeks ago. But then David Hendrix complained about main.c which spurred me into action, and I thought maybe we could just live with a brute force 'find/grep/sed' approach for now, rather than trying to be too clever. That's what I have done here. If you look at the image series I did, for example http://patchwork.ozlabs.org/patch/209601/, you will see that I was trying to avoid adding back #ifdefs into the cleaned-up code. That was a point solution, not particularly elegant, and I would much prefer that we get a built-in solution. I will come back to this when I have time - will be a week or two. Regards, Simon > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > Too bad that all the people who know how to run the country are busy > driving taxicabs and cutting hair. - George Burns
Hi Tom, On Mon, Feb 18, 2013 at 1:36 PM, Tom Rini <trini@ti.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 02/18/2013 02:23 PM, Wolfgang Denk wrote: >> Dear Simon, >> >> In message <1361207920-24983-1-git-send-email-sjg@chromium.org> you >> wrote: >>> Many parts of the U-Boot code base are sprinkled with #ifdefs. >>> This makes different boards compile different versions of the >>> source code, meaning that we must build all boards to check for >>> failures. It is easy to misspell >> ... >>> +# Create a C header file where every '#define CONFIG_XXX value' >>> becomes +# '#define config_xxx() value', or '#define config_xxx() >>> 0' where the CONFIG +# is not used by this board configuration. >>> This allows C code to do things +# like 'if (config_xxx())' and >>> have the compiler remove the dead code, +# instead of using >>> '#ifdef CONFIG_XXX...#endif'. Note that in most cases +# if the >>> config_...() returns 0 then the option is not enabled. In some >>> rare +# cases such as CONFIG_BOOTDELAY, the config can be enabled >>> but still have a +# a value of 0. So in addition we a #define >>> config_xxx_enabled(), setting the +# value to 0 if the option is >>> disabled, 1 if enabled. This last feature will +# hopefully be >>> deprecated soon. >> >> I think config_* is not a good name to use here - it has never been >> a reserved prefix so far, so it is IMO a bad idea to turn it into >> one now . We already have quite a number such variable names in >> the code all over the place (not sure I caught them all): > > Indeed. It's not a good choice to suddenly reserve now either. > build_has_xxx() ? I'm not sure. So config_bootdelay(), or cfg_bootdelay() for the value, and build_has_bootdelay() for the enable? > > [snip] >>> +The compiler will see that config_xxx() evalutes to a constant >>> and will +eliminate the dead code. The resulting code (and code >>> size) is the same. >> >> Did you measure the impact on compile time? > > Probably won't have a good handle on this without converting a whole > build target's needs (just about). Possibly, but it seems like the biggest impact is the slower reconfigure - sed runs for several seconds. > > [snip] >>> -#if defined CONFIG_ZERO_BOOTDELAY_CHECK /* * Check if key >>> already pressed * Don't check if bootdelay < 0 */ - if (bootdelay >>> >= 0) { + if (config_zero_bootdelay_check() && bootdelay >= 0) { >>> if (tstc()) { /* we got a key press */ (void) getc(); /* consume >>> input */ puts ("\b\b\b 0"); abort = 1; /* don't auto boot */ } } >>> -#endif >> >> I think code like this needs more careful editing. >> >> With the #ifdef, it was clear that the comment only applies if >> CONFIG_ZERO_BOOTDELAY_CHECK is defined, but now it suddenly >> becomes valid _always_, which is pretty much misleading to someone >> trying to understand this code. I think it would be necessary to >> rephrase the commend, and move it partially into the if() block. > > Exactly. This type of change will require a lot of re-commenting to > make it clear what's going on now, and after the change even more-so. I suspect cleaning up main.c would involve a number of patches in the end. BTW this patch also is interesting from the point of view of the generic board series. At the moment I am using a list of function pointers, which is quite nice, but does defeat the compiler optimisations, so adds about 1KB of code size. Also it's hard to imagine removing the #ifdefs from a list of function pointers using this mechanism. Regards, Simon > > - -- > Tom > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ > > iQIcBAEBAgAGBQJRIp7WAAoJENk4IS6UOR1W1S0QAKzMt8KkcVdRTFElSjlze35P > PxFkO/W8YchPkzwMdhpU4UxHNYoFziLk5pLfj8hhh9uyQ7Lp0I411PZtJ+mkt3I5 > RQf8xPHF9PDN2w0ZsxYKd0JE9LgFB/b9EmOOpzxy7Bge3aEGrfnhqgjNgnPGgVgO > eCcLGZLrGYlbcL9SOJNxBdFjgCxJvRfrNtsaLOJc5SveeqMNGISp6xc5WDWnmf1f > JAHNg7d9ik5d782AC76jbNUVOIp+85N0dMjhCdLR4YZBdNTC5grAW6x7gEGj+vYZ > xUE2/Y20FG1Ie3vQjbbW1gUhYtxCxjFLl+UkUOn5bf6F0eDgyUvaSt17nrO3GSbQ > hgunWaw9fZoVKMhb1WNnRHmLU5gS9rVlYGGsGibiMs1VPuiYpTLM/kuxfVitxJO0 > Ysgkyotgfj2RbnKuBCkMVmvxBzIC3S2bAtxY97TwVrpEh2ZB7r2YwEKak8WhVxyQ > nMyMulgpZoMJLM3fJEcF/kQPIzsKtz1Fl/YQXGZlI2lQpohE03kAPVQDyVeTQpGA > FzGOUwVZY4WbcKrpcCV4tJOEnHRVyDR8ntQx0udMjtChaLE40fAmmUlWmWrnE4yV > SVBM+PS2d7NCXt85QpS0eMc/UdFI0v1i6R24KEHEfqQe1WEdQb1wC7XXYblutZ8z > ySlnbeEMfeDg5i5FHX46 > =CF0y > -----END PGP SIGNATURE-----
Dear Simon, In message <CAPnjgZ38HPmOf07CMYssa1Q167sRjzDpbBUbCVMaoGdRoWESog@mail.gmail.com> you wrote: > > Thanks for looking at this so closely - it's just an RFC at this > stage, but I think it has promise. Agreed. > > I think config_* is not a good name to use here - it has never been a > > reserved prefix so far, so it is IMO a bad idea to turn it into one > > now . We already have quite a number such variable names in the code > > all over the place (not sure I caught them all): ... > These are variables, so won't conflict with the function macros used > by this patch. But maybe we should try for something like cfg_...()? You are wrong. This includes a number of functions, and macros, too, for example: void config_sdram(const struct emif_regs *regs); void config_ddr_phy(const struct emif_regs *regs); void config_cmd_ctrl(const struct cmd_control *cmd); void config_ddr_data(int data_macrono, ...) void config_io_ctrl(unsigned long val); void config_ddr(unsigned int pll, unsigned int ioctrl, void config_data_eye_leveling_samples(u32 emif_base); static int config_pll_clk(enum pll_clocks index, ...) static int config_core_clk(u32 ref, u32 freq) # define config_fifo(dir, idx, addr) And cfg_ is not much better: #define cfg_read(val, addr, type, op) #define cfg_write(val, addr, type, op) u16 cfg_type = 0; unsigned cfg_val; u32 *cfg_reg; uint cfg_addr; uint cfg_data; ... > I did for the 'sed' step - it was a 2-3 seconds to regenerate the header files. > > Full reconfig/recompile goes from about about 30s to 34s. > Incremental build doesn't change noticeably. I'm mostly concerned about build time for the autobuilder, which does full config / build cycles for all boards. Here more than 10% increase hurt... > I also tried recompiling both the mainline U-Boot's main.c 100 times, > and then this one. I could not see any time difference, which is a > little surprising. Maybe the C compiler's DCE is pretty early in the > the process? This is a surprising result, indeed. > >> + if (config_autoboot_delay_str() && delaykey[0].str == NULL) > >> + delaykey[0].str = config_autoboot_delay_str(); > > > > Hm.... constructs like these make me think about side effects. As is, > > your implementation does not pretect against any. This may be > > dangerous - you are evaluating the macro multiple times now, while > > before it was only a defined() test, folowed by a single evaluation. You did not comment on this remark. I think it is something to keep in mind! > > And to be honest - I find the old code easier to read. :-) > But if you have time, please take a look at the sed scripts and the Makefile. Sorry, but I can't find the audacity to bend my mind around these currently ;-) But you might have a look at "tools/scripts/define2mk.sed" and either use this as is, or base your code on this. I would find it good to use the same code for the same (or so very similar) purposes. > The background here is that I have been wondering about this for a > while, but have never really come up with a good way (in the absence > of a unified Kconfig) of getting a complete list of CONFIG variables. Does not the already existing "include/autoconf.mk" contain this information? In any case, please check "tools/scripts/define2mk.sed" > There was a mailing list post about this a few weeks ago. But then > David Hendrix complained about main.c which spurred me into action, > and I thought maybe we could just live with a brute force > 'find/grep/sed' approach for now, rather than trying to be too clever. > That's what I have done here. Understood, and appreciated. > I will come back to this when I have time - will be a week or two. Thanks. Best regards, Wolfgang Denk
I'm not sure if this has been discussed, but I've found as a new u-boot user there are often features that require enabling other CONFIG macros that I think should just be auto-enabled as dependencies. Please keep this in mind for any future designs. For example, when enabling CONFIG_CMD_UBI and CONFIG_CMD_UBIFS, my builds failed with linker errors because I didn't know that I needed to also define CONFIG_RBTREE and CONFIG_LZO. Thanks, Harvey
Hi Wolfgang, On Tue, Feb 19, 2013 at 1:19 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon, > > In message <CAPnjgZ38HPmOf07CMYssa1Q167sRjzDpbBUbCVMaoGdRoWESog@mail.gmail.com> you wrote: >> >> Thanks for looking at this so closely - it's just an RFC at this >> stage, but I think it has promise. > > Agreed. > >> > I think config_* is not a good name to use here - it has never been a >> > reserved prefix so far, so it is IMO a bad idea to turn it into one >> > now . We already have quite a number such variable names in the code >> > all over the place (not sure I caught them all): > ... >> These are variables, so won't conflict with the function macros used >> by this patch. But maybe we should try for something like cfg_...()? > > You are wrong. This includes a number of functions, and macros, too, > for example: > > void config_sdram(const struct emif_regs *regs); > void config_ddr_phy(const struct emif_regs *regs); > void config_cmd_ctrl(const struct cmd_control *cmd); > void config_ddr_data(int data_macrono, ...) > void config_io_ctrl(unsigned long val); > void config_ddr(unsigned int pll, unsigned int ioctrl, > void config_data_eye_leveling_samples(u32 emif_base); > static int config_pll_clk(enum pll_clocks index, ...) > static int config_core_clk(u32 ref, u32 freq) > # define config_fifo(dir, idx, addr) I will see if I can think of some other sane and obvious name that is little used in the source. Ideas welcome. But this isn't a very large number. It seems to be that we could easily adjust these symbols if we wanted to (and bear in mind that there are currently no conflicts, it is just confusing) - here are the files: for str in $(find . -name *.c -or -name *.h |xargs sed -n -f /tmp/s |sort |uniq); do echo $str; find . -name *.c -or -name *.h |xargs grep "[^a-zA-Z0-9_<]$str"; done config_aneg( ./drivers/qe/uec.c: err = curphy->config_aneg(uec->mii_info); config_buf( ./drivers/usb/gadget/ether.c: value = config_buf(gadget, req->buf, ./drivers/usb/gadget/composite.c:static int config_buf(struct usb_configuration *config, ./drivers/usb/gadget/composite.c: return config_buf(c, speed, cdev->req->buf, type); config_clock( ./arch/arm/cpu/armv7/tegra20/usb.c:static void config_clock(const u32 timing[]) ./arch/arm/cpu/armv7/tegra20/usb.c: config_clock(usb_pll[freq]); config_ddr( ./board/phytec/pcm051/board.c: config_ddr(DDR_CLK_MHZ, MT41J256M8HX15E_IOCTRL_VALUE, &ddr3_data, ./board/ti/am335x/board.c: config_ddr(303, MT41J128MJT125_IOCTRL_VALUE, &ddr3_data, ./board/ti/am335x/board.c: config_ddr(303, MT41J512M8RH125_IOCTRL_VALUE, &ddr3_evm_data, ./board/ti/am335x/board.c: config_ddr(266, MT47H128M16RT25E_IOCTRL_VALUE, &ddr2_data, ./arch/arm/include/asm/arch-am33xx/ddr_defs.h:void config_ddr(unsigned int pll, unsigned int ioctrl, ./arch/arm/cpu/armv7/am33xx/emif4.c:void config_ddr(unsigned int pll, unsigned int ioctrl, config_desc( ./drivers/usb/gadget/composite.c:static int config_desc(struct usb_composite_dev *cdev, unsigned w_value) ./drivers/usb/gadget/composite.c: value = config_desc(cdev, w_value); config_device( ./drivers/pci/pci.c: cfg->config_device(hose, dev, cfg); config_fifo( ./drivers/usb/musb/musb_core.c:# define config_fifo(dir, idx, addr) ./drivers/usb/musb/musb_core.c:# define config_fifo(dir, idx, addr) \ ./drivers/usb/musb/musb_core.c: config_fifo(tx, idx, fifoaddr); ./drivers/usb/musb/musb_core.c: config_fifo(rx, idx, fifoaddr); config_pin( ./drivers/gpio/db8500_gpio.c:static void config_pin(unsigned long cfg) ./drivers/gpio/db8500_gpio.c: * Configures several pins using config_pin(). Refer to that function for ./drivers/gpio/db8500_gpio.c: config_pin(cfgs[i]); config_sdram( ./arch/arm/include/asm/arch-am33xx/ddr_defs.h:void config_sdram(const struct emif_regs *regs); ./arch/arm/cpu/armv7/am33xx/emif4.c: config_sdram(regs); ./arch/arm/cpu/armv7/am33xx/ddr.c:void config_sdram(const struct emif_regs *regs) config_vtp( ./arch/arm/cpu/armv7/am33xx/emif4.c:static void config_vtp(void) ./arch/arm/cpu/armv7/am33xx/emif4.c: config_vtp(); > > And cfg_ is not much better: > > #define cfg_read(val, addr, type, op) > #define cfg_write(val, addr, type, op) > u16 cfg_type = 0; > unsigned cfg_val; > u32 *cfg_reg; > uint cfg_addr; > uint cfg_data; > ... ./drivers/video/exynos_fb.c: vid->cfg_gpio(); cfg_ldo( ./drivers/video/exynos_fb.c: vid->cfg_ldo(); cfg_oc( ./board/netta/pcmcia.c:static void cfg_oc(void) ./board/netta/pcmcia.c: cfg_oc(); cfg_read( ./drivers/pci/pci_indirect.c:#define cfg_read(val, addr, type, op) *val = op((type)(addr)) ./arch/x86/lib/pci_type1.c:#define cfg_read(val, addr, op) (*val = op((int)(addr))) ./arch/powerpc/cpu/mpc83xx/pcie.c:#define cfg_read(val, addr, type, op) \ ./arch/powerpc/cpu/mpc8220/pci.c:#define cfg_read(val, addr, type, op) *val = op((type)(addr)); ./arch/m68k/cpu/mcf547x_8x/pci.c:#define cfg_read(val, addr, type, op) *val = op((type)(addr)); ./arch/m68k/cpu/mcf5445x/pci.c:#define cfg_read(val, addr, type, op) *val = op((type)(addr)); cfg_shdn( ./board/netta/pcmcia.c:static void cfg_shdn(void) ./board/netta/pcmcia.c: cfg_shdn(); cfg_vccd( ./board/netta/pcmcia.c:static void cfg_vccd(int no) ./board/netta/pcmcia.c: cfg_vccd(0); cfg_vccd(1); /* 3V and 5V off */ cfg_vppd( ./board/netta/pcmcia.c:static void cfg_vppd(int no) ./board/netta/pcmcia.c: cfg_vppd(0); cfg_vppd(1); /* VPPD0,VPPD1 VAVPP => Hi-Z */ cfg_write( ./drivers/pci/pci_indirect.c:#define cfg_write(val, addr, type, op) op((type *)(addr), (val)) ./arch/x86/lib/pci_type1.c:#define cfg_write(val, addr, op) op((val), (int)(addr)) ./arch/powerpc/cpu/mpc83xx/pcie.c:#define cfg_write(val, addr, type, op) \ ./arch/powerpc/cpu/mpc8220/pci.c:#define cfg_write(val, addr, type, op) op((type *)(addr), (val)); ./arch/m68k/cpu/mcf547x_8x/pci.c:#define cfg_write(val, addr, type, op) op((type *)(addr), (val)); ./arch/m68k/cpu/mcf5445x/pci.c:#define cfg_write(val, addr, type, op) op((type *)(addr), (val)); That's a very manageable and small series of patches I think if we want to use either. I do like an obvious name, and we already have CONFIG_... > >> I did for the 'sed' step - it was a 2-3 seconds to regenerate the header files. >> >> Full reconfig/recompile goes from about about 30s to 34s. >> Incremental build doesn't change noticeably. > > I'm mostly concerned about build time for the autobuilder, which does > full config / build cycles for all boards. Here more than 10% > increase hurt... Yes this will definitely increase the time. The current brute force 'sed' of all headers isn't very efficient. How impossible would it be to regenerate this only when someone adds a new CONFIG, and then check it into the source? > >> I also tried recompiling both the mainline U-Boot's main.c 100 times, >> and then this one. I could not see any time difference, which is a >> little surprising. Maybe the C compiler's DCE is pretty early in the >> the process? > > This is a surprising result, indeed. > >> >> + if (config_autoboot_delay_str() && delaykey[0].str == NULL) >> >> + delaykey[0].str = config_autoboot_delay_str(); >> > >> > Hm.... constructs like these make me think about side effects. As is, >> > your implementation does not pretect against any. This may be >> > dangerous - you are evaluating the macro multiple times now, while >> > before it was only a defined() test, folowed by a single evaluation. > > You did not comment on this remark. I think it is something to keep > in mind! Yes it is. You could use the ..._enabled() form to get around this, but it may be reasonable to state that CONFIGs are not allowed side effects. I have previously tried to add function calls into a few CONFIGs and just got compile errors. So it isn't a new problem. > >> > And to be honest - I find the old code easier to read. > > :-) I prefer the one without #ifdefs, but I suppose this is not the worst example of #ifdef stuff in U-Boot. # ifdef CONFIG_AUTOBOOT_DELAY_STR if (delaykey[0].str == NULL) delaykey[0].str = CONFIG_AUTOBOOT_DELAY_STR; # endif # ifdef CONFIG_AUTOBOOT_DELAY_STR2 if (delaykey[1].str == NULL) delaykey[1].str = CONFIG_AUTOBOOT_DELAY_STR2; # endif # ifdef CONFIG_AUTOBOOT_STOP_STR if (delaykey[2].str == NULL) delaykey[2].str = CONFIG_AUTOBOOT_STOP_STR; # endif # ifdef CONFIG_AUTOBOOT_STOP_STR2 if (delaykey[3].str == NULL) delaykey[3].str = CONFIG_AUTOBOOT_STOP_STR2; # endif if (config_autoboot_delay_str() && delaykey[0].str == NULL) delaykey[0].str = config_autoboot_delay_str(); if (config_autoboot_delay_str2() && delaykey[1].str == NULL) delaykey[1].str = config_autoboot_delay_str2(); if (config_autoboot_stop_str() && delaykey[2].str == NULL) delaykey[2].str = config_autoboot_stop_str(); if (config_autoboot_stop_str2() && delaykey[3].str == NULL) delaykey[3].str = config_autoboot_stop_str2(); > >> But if you have time, please take a look at the sed scripts and the Makefile. > > Sorry, but I can't find the audacity to bend my mind around these > currently ;-) > > But you might have a look at "tools/scripts/define2mk.sed" and either > use this as is, or base your code on this. I would find it good to > use the same code for the same (or so very similar) purposes. Yes found it, copied it :-) > >> The background here is that I have been wondering about this for a >> while, but have never really come up with a good way (in the absence >> of a unified Kconfig) of getting a complete list of CONFIG variables. > > Does not the already existing "include/autoconf.mk" contain this > information? In any case, please check "tools/scripts/define2mk.sed" It only has a list of CONFIG variables that are enabled for the board. The C code will then get compile errors if it uses a config that is not enabled. So we need to define all the others to be 0 so that the code still compiles. > >> There was a mailing list post about this a few weeks ago. But then >> David Hendrix complained about main.c which spurred me into action, >> and I thought maybe we could just live with a brute force >> 'find/grep/sed' approach for now, rather than trying to be too clever. >> That's what I have done here. > > Understood, and appreciated. > >> I will come back to this when I have time - will be a week or two. > > Thanks. > > Best regards, > > Wolfgang Denk Regards, Simon > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > If you think the problem is bad now, just wait until we've solved it. > Epstein's Law
Dear Simon Glass, In message <CAPnjgZ2NvwAB0t4v=41BaVxqMaCgAU7XBTTbM1ZRT0fTA_40tA@mail.gmail.com> you wrote: > > > You are wrong. This includes a number of functions, and macros, too, > > for example: ... > That's a very manageable and small series of patches I think if we > want to use either. I do like an obvious name, and we already have > CONFIG_... I think we really need to define a new, so far unused name space for these, and reserve it for such purpose. > Yes this will definitely increase the time. The current brute force > 'sed' of all headers isn't very efficient. How impossible would it be > to regenerate this only when someone adds a new CONFIG, and then check > it into the source? Doesn't work - assume you are hacking on your new code (without checking in) - and if runs haywire because the needed re-scan is not done... > > Does not the already existing "include/autoconf.mk" contain this > > information? In any case, please check "tools/scripts/define2mk.sed" > > It only has a list of CONFIG variables that are enabled for the board. > The C code will then get compile errors if it uses a config that is > not enabled. So we need to define all the others to be 0 so that the > code still compiles. I see. Thanks. Wolfgang Denk
Hi Wolfgang, On Tue, Feb 19, 2013 at 11:14 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <CAPnjgZ2NvwAB0t4v=41BaVxqMaCgAU7XBTTbM1ZRT0fTA_40tA@mail.gmail.com> you wrote: >> >> > You are wrong. This includes a number of functions, and macros, too, >> > for example: > ... >> That's a very manageable and small series of patches I think if we >> want to use either. I do like an obvious name, and we already have >> CONFIG_... > > I think we really need to define a new, so far unused name space for > these, and reserve it for such purpose. What about: autoconf_...(): value of CONFIG (or 0 if not defined) autoconf_has_...(): 1 if the CONFIG is defined, 0 if not defined (rarely needed I think) This doesn't seem to be used currently. > >> Yes this will definitely increase the time. The current brute force >> 'sed' of all headers isn't very efficient. How impossible would it be >> to regenerate this only when someone adds a new CONFIG, and then check >> it into the source? > > Doesn't work - assume you are hacking on your new code (without > checking in) - and if runs haywire because the needed re-scan is not > done... You would get compile errors in this case. I'm not sure how we can optimise this then. > >> > Does not the already existing "include/autoconf.mk" contain this >> > information? In any case, please check "tools/scripts/define2mk.sed" >> >> It only has a list of CONFIG variables that are enabled for the board. >> The C code will then get compile errors if it uses a config that is >> not enabled. So we need to define all the others to be 0 so that the >> code still compiles. > > I see. > > Thanks. > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > The human race is faced with a cruel choice: work or daytime tele- > vision. Regards, Simon
diff --git a/Makefile b/Makefile index fc18dd4..5ca3a57 100644 --- a/Makefile +++ b/Makefile @@ -614,6 +614,7 @@ updater: # parallel sub-makes creating .depend files simultaneously. depend dep: $(TIMESTAMP_FILE) $(VERSION_FILE) \ $(obj)include/autoconf.mk \ + $(obj)include/generated/autoconf.h \ $(obj)include/generated/generic-asm-offsets.h \ $(obj)include/generated/asm-offsets.h for dir in $(SUBDIRS) $(CPUDIR) $(LDSCRIPT_MAKEFILE_DIR) ; do \ @@ -688,6 +689,43 @@ $(obj)include/autoconf.mk: $(obj)include/config.h sed -n -f tools/scripts/define2mk.sed > $@.tmp && \ mv $@.tmp $@ +# Create a C header file where every '#define CONFIG_XXX value' becomes +# '#define config_xxx() value', or '#define config_xxx() 0' where the CONFIG +# is not used by this board configuration. This allows C code to do things +# like 'if (config_xxx())' and have the compiler remove the dead code, +# instead of using '#ifdef CONFIG_XXX...#endif'. Note that in most cases +# if the config_...() returns 0 then the option is not enabled. In some rare +# cases such as CONFIG_BOOTDELAY, the config can be enabled but still have a +# a value of 0. So in addition we a #define config_xxx_enabled(), setting the +# value to 0 if the option is disabled, 1 if enabled. This last feature will +# hopefully be deprecated soon. +# The file is regenerated when any U-Boot header file changes. +$(obj)include/generated/autoconf.h: $(obj)include/config.h + @$(XECHO) Generating $@ ; \ + set -e ; \ + : Extract the config macros to a C header file ; \ + $(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \ + sed -n -f tools/scripts/define2conf.sed > $@.tmp; \ + : Regenerate our list of all config macros if neeed ; \ + if [ ! -f $@-all.tmp ] || \ + find $(src) -name '*.h' -type f -newer $@-all.tmp | \ + egrep -qv 'include/(autoconf.h|generated|config.h)'; \ + then \ + : Extract all config macros from all C header files ; \ + ( \ + find ${src} -name "*.h" -type f | xargs \ + cat | \ + sed -n -f tools/scripts/define2list.sed \ + ) | sort | uniq > $@-all.tmp; \ + fi; \ + : Extract the enabled config macros to a C header file ; \ + $(CPP) $(CFLAGS) -DDO_DEPS_ONLY -dM include/common.h | \ + sed -n -f tools/scripts/define2list.sed | sort > $@-enabled.tmp; \ + set -e ; \ + : Find CONFIGs that are not enabled ; \ + comm -13 $@-enabled.tmp $@-all.tmp >>$@.tmp && \ + mv $@.tmp $@ + $(obj)include/generated/generic-asm-offsets.h: $(obj)include/autoconf.mk.dep \ $(obj)lib/asm-offsets.s @$(XECHO) Generating $@ @@ -770,7 +808,8 @@ include/license.h: tools/bin2header COPYING unconfig: @rm -f $(obj)include/config.h $(obj)include/config.mk \ $(obj)board/*/config.tmp $(obj)board/*/*/config.tmp \ - $(obj)include/autoconf.mk $(obj)include/autoconf.mk.dep + $(obj)include/autoconf.mk $(obj)include/autoconf.mk.dep \ + $(obj)include/generated/autoconf.h %_config:: unconfig @$(MKCONFIG) -A $(@:_config=) diff --git a/README b/README index d8cb394..3e89551 100644 --- a/README +++ b/README @@ -5434,11 +5434,92 @@ Notes: * If you modify existing code, make sure that your new code does not add to the memory footprint of the code ;-) Small is beautiful! When adding new features, these should compile conditionally only - (using #ifdef), and the resulting code with the new feature - disabled must not need more memory than the old code without your - modification. + (avoiding #ifdef where at all possible), and the resulting code with + the new feature disabled must not need more memory than the old code + without your modification. * Remember that there is a size limit of 100 kB per message on the u-boot mailing list. Bigger patches will be moderated. If they are reasonable and not too big, they will be acknowledged. But patches bigger than the size limit should be avoided. + + +Use of #ifdef: +-------------- +Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes +different boards compile different versions of the source code, meaning +that we must build all boards to check for failures. It is easy to misspell +an #ifdef and there is not as much checking of this by the compiler. For +someone coming new into the code base, #ifdefs are a big turn-off. Multiple +dependent #ifdefs are harder to do than with if..then..else. Variable +declarations must be #idefed as well as the code that uses them, often much +later in the file/function. #ifdef indents don't match code indents and +have their own separate indent feature. Overall, excessive use of #idef +hurts readability and makes the code harder to modify and refactor. + +In an effort to reduce the use of #ifdef in U-Boot, without requiring lots +of special static inlines all over the header files, a single autoconf.h +header file with lower-case function-type macros has been made available. + +This file has either: + +# #define config_xxx() value + +for enabled options, or: + +# #define config_xxx() 0 + +for disabled options. You can therefore generally change code like this: + + #ifdef CONFIG_XXX + do_something + #else + do_something_else + #endif + +to this: + + if (config_xxx()) + do_something; + else + do_something_else; + +The compiler will see that config_xxx() evalutes to a constant and will +eliminate the dead code. The resulting code (and code size) is the same. + +Multiple #ifdefs can be converted also: + + #if defined(CONFIG_XXX) && !defined(CONFIG_YYY) + do_something + #endif + + if (config_xxx() && !config_yyy()) + do_something; + +Where the macro evaluates to a string, it will be non-NULL, so the above +will work whether the macro is a string or a number. + +This takes care of almost all CONFIG macros. Unfortunately there are a few +cases where a value of 0 does not mean the option is disabled. For example +CONFIG_BOOTDELAY can be defined to 0, which means that the bootdelay +code should be used, but with a value of 0. To get around this and other +sticky cases, an addition macro with an '_enabled' suffix is provided, where +the value is always either 0 or 1: + + // Will work even if boaard config has '#define CONFIG_BOOTDELAY 0' + if (config_bootdelay_enabled()) + do_something; + +(Probably such config options should be deprecated and then we can remove +this feature) + +U-Boot already has a Makefile scheme to permit files to be easily included +based on CONFIG. This can be used where the code to be compiled exists in +its own source file. So the following rules apply: + + 1. Use #ifdef to conditionally compile an exported function or variable + 2. Use ordinary C code with config_xxx() everywhere else + 3. Mark your functions and data structures static where possible + 4. Use the config_xxx_enabled() variants only if essential + 5. When changing existing code, first create a new patch to replace + #ifdefs in the surrounding area diff --git a/common/cmd_fitupd.c b/common/cmd_fitupd.c index 7a3789e..3f62d83 100644 --- a/common/cmd_fitupd.c +++ b/common/cmd_fitupd.c @@ -8,6 +8,7 @@ #include <common.h> #include <command.h> +#include <net.h> #if !defined(CONFIG_UPDATE_TFTP) #error "CONFIG_UPDATE_TFTP required" diff --git a/common/main.c b/common/main.c index e2d2e09..cd42b67 100644 --- a/common/main.c +++ b/common/main.c @@ -28,30 +28,16 @@ /* #define DEBUG */ #include <common.h> -#include <watchdog.h> #include <command.h> #include <fdtdec.h> -#include <malloc.h> -#include <version.h> -#ifdef CONFIG_MODEM_SUPPORT -#include <malloc.h> /* for free() prototype */ -#endif - -#ifdef CONFIG_SYS_HUSH_PARSER -#include <hush.h> -#endif - -#ifdef CONFIG_OF_CONTROL -#include <fdtdec.h> -#endif - -#ifdef CONFIG_OF_LIBFDT #include <fdt_support.h> -#endif /* CONFIG_OF_LIBFDT */ - +#include <hush.h> +#include <malloc.h> +#include <menu.h> #include <post.h> +#include <version.h> +#include <watchdog.h> #include <linux/ctype.h> -#include <menu.h> DECLARE_GLOBAL_DATA_PTR; @@ -61,13 +47,19 @@ DECLARE_GLOBAL_DATA_PTR; void inline __show_boot_progress (int val) {} void show_boot_progress (int val) __attribute__((weak, alias("__show_boot_progress"))); -#if defined(CONFIG_UPDATE_TFTP) -int update_tftp (ulong addr); -#endif /* CONFIG_UPDATE_TFTP */ - #define MAX_DELAY_STOP_STR 32 -#undef DEBUG_PARSER +#define DEBUG_PARSER 0 /* set to 1 to debug */ + + +#define debug_parser(fmt, args...) \ + debug_cond(DEBUG_PARSER, fmt, ##args) + +#ifndef DEBUG_BOOTKEYS +#define DEBUG_BOOTKEYS 0 +#endif +#define debug_bootkeys(fmt, args...) \ + debug_cond(DEBUG_BOOTKEYS, fmt, ##args) char console_buffer[CONFIG_SYS_CBSIZE + 1]; /* console I/O buffer */ @@ -75,32 +67,18 @@ static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen); static const char erase_seq[] = "\b \b"; /* erase sequence */ static const char tab_seq[] = " "; /* used to expand TABs */ -#ifdef CONFIG_BOOT_RETRY_TIME static uint64_t endtime = 0; /* must be set, default is instant timeout */ static int retry_time = -1; /* -1 so can call readline before main_loop */ -#endif #define endtick(seconds) (get_ticks() + (uint64_t)(seconds) * get_tbclk()) -#ifndef CONFIG_BOOT_RETRY_MIN -#define CONFIG_BOOT_RETRY_MIN CONFIG_BOOT_RETRY_TIME -#endif - -#ifdef CONFIG_MODEM_SUPPORT int do_mdm_init = 0; -extern void mdm_init(void); /* defined in board.c */ -#endif /*************************************************************************** * Watch for 'delay' seconds for autoboot stop or autoboot delay string. * returns: 0 - no key string, allow autoboot 1 - got key string, abort */ -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) -# if defined(CONFIG_AUTOBOOT_KEYED) -#ifndef CONFIG_MENU -static inline -#endif -int abortboot(int bootdelay) +static int abortboot_keyed(int bootdelay) { int abort = 0; uint64_t etime = endtick(bootdelay); @@ -121,31 +99,20 @@ int abortboot(int bootdelay) u_int presskey_max = 0; u_int i; -#ifndef CONFIG_ZERO_BOOTDELAY_CHECK - if (bootdelay == 0) + if (config_zero_bootdelay_check() && bootdelay == 0) return 0; -#endif -# ifdef CONFIG_AUTOBOOT_PROMPT - printf(CONFIG_AUTOBOOT_PROMPT); -# endif - -# ifdef CONFIG_AUTOBOOT_DELAY_STR - if (delaykey[0].str == NULL) - delaykey[0].str = CONFIG_AUTOBOOT_DELAY_STR; -# endif -# ifdef CONFIG_AUTOBOOT_DELAY_STR2 - if (delaykey[1].str == NULL) - delaykey[1].str = CONFIG_AUTOBOOT_DELAY_STR2; -# endif -# ifdef CONFIG_AUTOBOOT_STOP_STR - if (delaykey[2].str == NULL) - delaykey[2].str = CONFIG_AUTOBOOT_STOP_STR; -# endif -# ifdef CONFIG_AUTOBOOT_STOP_STR2 - if (delaykey[3].str == NULL) - delaykey[3].str = CONFIG_AUTOBOOT_STOP_STR2; -# endif + if (config_autoboot_prompt_enabled()) + printf(config_autoboot_prompt()); + + if (config_autoboot_delay_str() && delaykey[0].str == NULL) + delaykey[0].str = config_autoboot_delay_str(); + if (config_autoboot_delay_str2() && delaykey[1].str == NULL) + delaykey[1].str = config_autoboot_delay_str2(); + if (config_autoboot_stop_str() && delaykey[2].str == NULL) + delaykey[2].str = config_autoboot_stop_str(); + if (config_autoboot_stop_str2() && delaykey[3].str == NULL) + delaykey[3].str = config_autoboot_stop_str2(); for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) { delaykey[i].len = delaykey[i].str == NULL ? @@ -156,11 +123,9 @@ int abortboot(int bootdelay) presskey_max = presskey_max > delaykey[i].len ? presskey_max : delaykey[i].len; -# if DEBUG_BOOTKEYS - printf("%s key:<%s>\n", + debug_bootkeys("%s key:<%s>\n", delaykey[i].retry ? "delay" : "stop", delaykey[i].str ? delaykey[i].str : "NULL"); -# endif } /* In order to keep up with incoming data, check timeout only @@ -179,74 +144,56 @@ int abortboot(int bootdelay) } } - for (i = 0; i < sizeof(delaykey) / sizeof(delaykey[0]); i ++) { + for (i = 0; i < ARRAY_SIZE(delaykey); i++) { if (delaykey[i].len > 0 && presskey_len >= delaykey[i].len && memcmp (presskey + presskey_len - delaykey[i].len, delaykey[i].str, delaykey[i].len) == 0) { -# if DEBUG_BOOTKEYS - printf("got %skey\n", - delaykey[i].retry ? "delay" : "stop"); -# endif + debug_bootkeys("got %skey\n", + delaykey[i].retry ? "delay" : "stop"); -# ifdef CONFIG_BOOT_RETRY_TIME /* don't retry auto boot */ - if (! delaykey[i].retry) + if (config_boot_retry_time() && + !delaykey[i].retry) retry_time = -1; -# endif abort = 1; } } } while (!abort && get_ticks() <= etime); -# if DEBUG_BOOTKEYS if (!abort) - puts("key timeout\n"); -# endif + debug_bootkeys("key timeout\n"); -#ifdef CONFIG_SILENT_CONSOLE - if (abort) + if (config_silent_console() && abort) gd->flags &= ~GD_FLG_SILENT; -#endif return abort; } -# else /* !defined(CONFIG_AUTOBOOT_KEYED) */ - -#ifdef CONFIG_MENUKEY static int menukey = 0; -#endif -#ifndef CONFIG_MENU -static inline -#endif -int abortboot(int bootdelay) +static int abortboot_normal(int bootdelay) { int abort = 0; unsigned long ts; -#ifdef CONFIG_MENUPROMPT - printf(CONFIG_MENUPROMPT); -#else - if (bootdelay >= 0) + if (config_menuprompt()) + printf(config_menuprompt()); + else if (bootdelay >= 0) printf("Hit any key to stop autoboot: %2d ", bootdelay); -#endif -#if defined CONFIG_ZERO_BOOTDELAY_CHECK /* * Check if key already pressed * Don't check if bootdelay < 0 */ - if (bootdelay >= 0) { + if (config_zero_bootdelay_check() && bootdelay >= 0) { if (tstc()) { /* we got a key press */ (void) getc(); /* consume input */ puts ("\b\b\b 0"); abort = 1; /* don't auto boot */ } } -#endif while ((bootdelay > 0) && (!abort)) { --bootdelay; @@ -256,11 +203,10 @@ int abortboot(int bootdelay) if (tstc()) { /* we got a key press */ abort = 1; /* don't auto boot */ bootdelay = 0; /* no more delay */ -# ifdef CONFIG_MENUKEY - menukey = getc(); -# else - (void) getc(); /* consume input */ -# endif + if (config_menukey()) + menukey = getc(); + else + (void) getc(); /* consume input */ break; } udelay(10000); @@ -271,15 +217,19 @@ int abortboot(int bootdelay) putc('\n'); -#ifdef CONFIG_SILENT_CONSOLE - if (abort) + if (config_silent_console() && abort) gd->flags &= ~GD_FLG_SILENT; -#endif return abort; } -# endif /* CONFIG_AUTOBOOT_KEYED */ -#endif /* CONFIG_BOOTDELAY >= 0 */ + +static int abortboot(int bootdelay) +{ + if (config_autoboot_keyed()) + return abortboot_keyed(bootdelay); + else + return abortboot_normal(bootdelay); +} /* * Runs the given boot command securely. Specifically: @@ -295,8 +245,6 @@ int abortboot(int bootdelay) * printing the error message to console. */ -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \ - defined(CONFIG_OF_CONTROL) static void secure_boot_cmd(char *cmd) { cmd_tbl_t *cmdtp; @@ -337,190 +285,170 @@ static void process_fdt_options(const void *blob) /* Add an env variable to point to a kernel payload, if available */ addr = fdtdec_get_config_int(gd->fdt_blob, "kernel-offset", 0); - if (addr) - setenv_addr("kernaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr)); + if (addr) { + setenv_addr("kernaddr", + (void *)(config_sys_text_base() + addr)); + } /* Add an env variable to point to a root disk, if available */ addr = fdtdec_get_config_int(gd->fdt_blob, "rootdisk-offset", 0); - if (addr) - setenv_addr("rootaddr", (void *)(CONFIG_SYS_TEXT_BASE + addr)); + if (addr) { + setenv_addr("rootaddr", + (void *)(config_sys_text_base() + addr)); + } } -#endif /* CONFIG_OF_CONTROL */ - - -/****************************************************************************/ -void main_loop (void) +static void process_boot_delay(void) { -#ifndef CONFIG_SYS_HUSH_PARSER - static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, }; - int len; - int rc = 1; - int flag; -#endif -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) && \ - defined(CONFIG_OF_CONTROL) - char *env; -#endif -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) - char *s; - int bootdelay; -#endif -#ifdef CONFIG_PREBOOT - char *p; -#endif -#ifdef CONFIG_BOOTCOUNT_LIMIT unsigned long bootcount = 0; unsigned long bootlimit = 0; - char *bcs; - char bcs_set[16]; -#endif /* CONFIG_BOOTCOUNT_LIMIT */ - - bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop"); - -#ifdef CONFIG_BOOTCOUNT_LIMIT - bootcount = bootcount_load(); - bootcount++; - bootcount_store (bootcount); - sprintf (bcs_set, "%lu", bootcount); - setenv ("bootcount", bcs_set); - bcs = getenv ("bootlimit"); - bootlimit = bcs ? simple_strtoul (bcs, NULL, 10) : 0; -#endif /* CONFIG_BOOTCOUNT_LIMIT */ - -#ifdef CONFIG_MODEM_SUPPORT - debug ("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init); - if (do_mdm_init) { - char *str = strdup(getenv("mdm_cmd")); - setenv ("preboot", str); /* set or delete definition */ - if (str != NULL) - free (str); - mdm_init(); /* wait for modem connection */ - } -#endif /* CONFIG_MODEM_SUPPORT */ - -#ifdef CONFIG_VERSION_VARIABLE - { - setenv ("ver", version_string); /* set version variable */ - } -#endif /* CONFIG_VERSION_VARIABLE */ - -#ifdef CONFIG_SYS_HUSH_PARSER - u_boot_hush_start (); -#endif - -#if defined(CONFIG_HUSH_INIT_VAR) - hush_init_var (); -#endif - -#ifdef CONFIG_PREBOOT - if ((p = getenv ("preboot")) != NULL) { -# ifdef CONFIG_AUTOBOOT_KEYED - int prev = disable_ctrlc(1); /* disable Control C checking */ -# endif - - run_command_list(p, -1, 0); + const char *s; + int bootdelay; -# ifdef CONFIG_AUTOBOOT_KEYED - disable_ctrlc(prev); /* restore Control C checking */ -# endif + if (config_bootcount_limit()) { + bootcount = bootcount_load(); + bootcount++; + bootcount_store(bootcount); + setenv_ulong("bootcount", bootcount); + bootlimit = getenv_ulong("bootlimit", 10, 0); } -#endif /* CONFIG_PREBOOT */ - -#if defined(CONFIG_UPDATE_TFTP) - update_tftp (0UL); -#endif /* CONFIG_UPDATE_TFTP */ -#if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0) s = getenv ("bootdelay"); - bootdelay = s ? (int)simple_strtol(s, NULL, 10) : CONFIG_BOOTDELAY; + bootdelay = s ? (int)simple_strtol(s, NULL, 10) : config_bootdelay(); debug ("### main_loop entered: bootdelay=%d\n\n", bootdelay); -#if defined(CONFIG_MENU_SHOW) - bootdelay = menu_show(bootdelay); -#endif -# ifdef CONFIG_BOOT_RETRY_TIME - init_cmd_timeout (); -# endif /* CONFIG_BOOT_RETRY_TIME */ + if (config_menu_show()) + bootdelay = menu_show(bootdelay); + if (config_boot_retry_time()) + init_cmd_timeout(); -#ifdef CONFIG_POST - if (gd->flags & GD_FLG_POSTFAIL) { + if (config_post() && (gd->flags & GD_FLG_POSTFAIL)) { s = getenv("failbootcmd"); - } - else -#endif /* CONFIG_POST */ -#ifdef CONFIG_BOOTCOUNT_LIMIT - if (bootlimit && (bootcount > bootlimit)) { + } else if (config_bootcount_limit() && bootlimit && + (bootcount > bootlimit)) { printf ("Warning: Bootlimit (%u) exceeded. Using altbootcmd.\n", (unsigned)bootlimit); s = getenv ("altbootcmd"); - } - else -#endif /* CONFIG_BOOTCOUNT_LIMIT */ + } else { s = getenv ("bootcmd"); -#ifdef CONFIG_OF_CONTROL - /* Allow the fdt to override the boot command */ - env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd"); - if (env) - s = env; + } + if (config_of_control()) { + char *env; - process_fdt_options(gd->fdt_blob); + /* Allow the fdt to override the boot command */ + env = fdtdec_get_config_string(gd->fdt_blob, "bootcmd"); + if (env) + s = env; - /* - * If the bootsecure option was chosen, use secure_boot_cmd(). - * Always use 'env' in this case, since bootsecure requres that the - * bootcmd was specified in the FDT too. - */ - if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0)) - secure_boot_cmd(env); + process_fdt_options(gd->fdt_blob); -#endif /* CONFIG_OF_CONTROL */ + /* + * If the bootsecure option was chosen, use secure_boot_cmd(). + * Always use 'env' in this case, since bootsecure requres that + * the bootcmd was specified in the FDT too. + */ + if (fdtdec_get_config_int(gd->fdt_blob, "bootsecure", 0)) + secure_boot_cmd(env); + } debug ("### main_loop: bootcmd=\"%s\"\n", s ? s : "<UNDEFINED>"); if (bootdelay != -1 && s && !abortboot(bootdelay)) { -# ifdef CONFIG_AUTOBOOT_KEYED - int prev = disable_ctrlc(1); /* disable Control C checking */ -# endif + int prev; + + /* disable Control C checking */ + if (config_autoboot_keyed()) + prev = disable_ctrlc(1); run_command_list(s, -1, 0); -# ifdef CONFIG_AUTOBOOT_KEYED - disable_ctrlc(prev); /* restore Control C checking */ -# endif + /* restore Control C checking */ + if (config_autoboot_keyed()) + disable_ctrlc(prev); } -# ifdef CONFIG_MENUKEY - if (menukey == CONFIG_MENUKEY) { + if (config_menukey() && menukey == config_menukey()) { s = getenv("menucmd"); if (s) run_command_list(s, -1, 0); } -#endif /* CONFIG_MENUKEY */ -#endif /* CONFIG_BOOTDELAY */ +} -#if defined CONFIG_OF_CONTROL - set_working_fdt_addr((void *)gd->fdt_blob); -#endif /* CONFIG_OF_CONTROL */ +/****************************************************************************/ + +void main_loop(void) +{ + static char lastcommand[CONFIG_SYS_CBSIZE] = { 0, }; + int len; + int rc = 1; + int flag; + + bootstage_mark_name(BOOTSTAGE_ID_MAIN_LOOP, "main_loop"); + + if (config_modem_support()) { + debug("DEBUG: main_loop: do_mdm_init=%d\n", do_mdm_init); + if (do_mdm_init) { + char *str = strdup(getenv("mdm_cmd")); + setenv("preboot", str); /* set or delete definition */ + if (str != NULL) + free(str); + mdm_init(); /* wait for modem connection */ + } + } + + if (config_version_variable()) + setenv("ver", version_string); /* set version variable */ + + if (config_sys_hush_parser()) + u_boot_hush_start(); + + if (config_hush_init_var()) + hush_init_var(); + + if (config_preboot()) { + char *p = getenv("preboot"); + + if (p) { + int prev; + + /* disable Control C checking */ + if (config_autoboot_keyed()) + prev = disable_ctrlc(1); + + run_command_list(p, -1, 0); + + /* restore Control C checking */ + if (config_autoboot_keyed()) + disable_ctrlc(prev); + } + } + + if (config_update_tftp()) + update_tftp(0UL); + + if (config_bootdelay_enabled() && config_bootdelay() >= 0) + process_boot_delay(); + + if (config_of_control() && config_of_libfdt()) + set_working_fdt_addr((void *)gd->fdt_blob); /* * Main Loop for Monitor Command Processing */ -#ifdef CONFIG_SYS_HUSH_PARSER - parse_file_outer(); - /* This point is never reached */ - for (;;); -#else - for (;;) { -#ifdef CONFIG_BOOT_RETRY_TIME - if (rc >= 0) { - /* Saw enough of a valid command to - * restart the timeout. - */ - reset_cmd_timeout(); + if (config_sys_hush_parser()) { + parse_file_outer(); + /* This point is never reached */ + for (;;); + } else { + if (config_boot_retry_time()) { + if (rc >= 0) { + /* Saw enough of a valid command to + * restart the timeout. + */ + reset_cmd_timeout(); + } } -#endif len = readline (CONFIG_SYS_PROMPT); flag = 0; /* assume no special flags for now */ @@ -528,33 +456,32 @@ void main_loop (void) strcpy (lastcommand, console_buffer); else if (len == 0) flag |= CMD_FLAG_REPEAT; -#ifdef CONFIG_BOOT_RETRY_TIME - else if (len == -2) { + else if (config_boot_retry_time() && len == -2) { /* -2 means timed out, retry autoboot */ - puts ("\nTimed out waiting for command\n"); -# ifdef CONFIG_RESET_TO_RETRY + puts("\nTimed out waiting for command\n"); /* Reinit board to run initialization code again */ - do_reset (NULL, 0, 0, NULL); -# else - return; /* retry autoboot */ -# endif + if (config_reset_to_retry()) + do_reset(NULL, 0, 0, NULL); + else + return; /* retry autoboot */ } -#endif if (len == -1) - puts ("<INTERRUPT>\n"); + puts("<INTERRUPT>\n"); else rc = run_command(lastcommand, flag); - if (rc <= 0) { - /* invalid command or not repeatable, forget it */ + /* invalid command or not repeatable, forget it */ + if (rc <= 0) lastcommand[0] = 0; - } } -#endif /*CONFIG_SYS_HUSH_PARSER*/ } +/* + * Use ifdef here for the benefit of those archs not using + * -ffunction-sections, since these functions are exported. + */ #ifdef CONFIG_BOOT_RETRY_TIME /*************************************************************************** * initialize command line timeout @@ -562,14 +489,18 @@ void main_loop (void) void init_cmd_timeout(void) { char *s = getenv ("bootretry"); + int retry_min; if (s != NULL) retry_time = (int)simple_strtol(s, NULL, 10); else retry_time = CONFIG_BOOT_RETRY_TIME; - if (retry_time >= 0 && retry_time < CONFIG_BOOT_RETRY_MIN) - retry_time = CONFIG_BOOT_RETRY_MIN; + retry_min = config_boot_retry_min() ? config_boot_retry_min() + : config_boot_retry_time(); + + if (retry_time >= 0 && retry_time < retry_min) + retry_time = retry_min; } /*************************************************************************** @@ -581,8 +512,6 @@ void reset_cmd_timeout(void) } #endif -#ifdef CONFIG_CMDLINE_EDITING - /* * cmdline-editing related codes from vivi. * Author: Janghoon Lyu <nandy@mizi.com> @@ -685,27 +614,6 @@ static char* hist_next(void) return (ret); } -#ifndef CONFIG_CMDLINE_EDITING -static void cread_print_hist_list(void) -{ - int i; - unsigned long n; - - n = hist_num - hist_max; - - i = hist_add_idx + 1; - while (1) { - if (i > hist_max) - i = 0; - if (i == hist_add_idx) - break; - printf("%s\n", hist_list[i]); - n++; - i++; - } -} -#endif /* CONFIG_CMDLINE_EDITING */ - #define BEGINNING_OF_LINE() { \ while (num) { \ getcmd_putch(CTL_BACKSPACE); \ @@ -791,13 +699,13 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, cread_add_str(buf, init_len, 1, &num, &eol_num, buf, *len); while (1) { -#ifdef CONFIG_BOOT_RETRY_TIME - while (!tstc()) { /* while no incoming data */ - if (retry_time >= 0 && get_ticks() > endtime) - return (-2); /* timed out */ - WATCHDOG_RESET(); + if (config_boot_retry_time()) { + while (!tstc()) { /* while no incoming data */ + if (retry_time >= 0 && get_ticks() > endtime) + return -2; /* timed out */ + WATCHDOG_RESET(); + } } -#endif if (first && timeout) { uint64_t etime = endtick(timeout); @@ -967,27 +875,27 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, REFRESH_TO_EOL(); continue; } -#ifdef CONFIG_AUTO_COMPLETE - case '\t': { - int num2, col; + case '\t': + if (config_auto_complete()) { + int num2, col; - /* do not autocomplete when in the middle */ - if (num < eol_num) { - getcmd_cbeep(); - break; - } + /* do not autocomplete when in the middle */ + if (num < eol_num) { + getcmd_cbeep(); + break; + } - buf[num] = '\0'; - col = strlen(prompt) + eol_num; - num2 = num; - if (cmd_auto_complete(prompt, buf, &num2, &col)) { - col = num2 - num; - num += col; - eol_num += col; + buf[num] = '\0'; + col = strlen(prompt) + eol_num; + num2 = num; + if (cmd_auto_complete(prompt, buf, &num2, + &col)) { + col = num2 - num; + num += col; + eol_num += col; + } + break; } - break; - } -#endif default: cread_add_char(ichar, insert, &num, &eol_num, buf, *len); break; @@ -1003,8 +911,6 @@ static int cread_line(const char *const prompt, char *buf, unsigned int *len, return 0; } -#endif /* CONFIG_CMDLINE_EDITING */ - /****************************************************************************/ /* @@ -1026,84 +932,52 @@ int readline (const char *const prompt) return readline_into_buffer(prompt, console_buffer, 0); } - -int readline_into_buffer(const char *const prompt, char *buffer, int timeout) +static int simple_readline(const char *const prompt, int plen, char *p, + int timeout) { - char *p = buffer; -#ifdef CONFIG_CMDLINE_EDITING - unsigned int len = CONFIG_SYS_CBSIZE; - int rc; - static int initted = 0; - - /* - * History uses a global array which is not - * writable until after relocation to RAM. - * Revert to non-history version if still - * running from flash. - */ - if (gd->flags & GD_FLG_RELOC) { - if (!initted) { - hist_init(); - initted = 1; - } - - if (prompt) - puts (prompt); - - rc = cread_line(prompt, p, &len, timeout); - return rc < 0 ? rc : len; - - } else { -#endif /* CONFIG_CMDLINE_EDITING */ char * p_buf = p; int n = 0; /* buffer index */ - int plen = 0; /* prompt length */ int col; /* output column cnt */ char c; - /* print prompt */ - if (prompt) { - plen = strlen (prompt); - puts (prompt); - } col = plen; for (;;) { -#ifdef CONFIG_BOOT_RETRY_TIME - while (!tstc()) { /* while no incoming data */ - if (retry_time >= 0 && get_ticks() > endtime) - return (-2); /* timed out */ - WATCHDOG_RESET(); + if (config_boot_retry_time()) { + while (!tstc()) { /* while no incoming data */ + if (retry_time >= 0 && get_ticks() > endtime) + return -2; /* timed out */ + WATCHDOG_RESET(); + } } -#endif - WATCHDOG_RESET(); /* Trigger watchdog, if needed */ + WATCHDOG_RESET(); /* Trigger watchdog, if needed */ -#ifdef CONFIG_SHOW_ACTIVITY - while (!tstc()) { - show_activity(0); - WATCHDOG_RESET(); + if (config_show_activity()) { + while (!tstc()) { + show_activity(0); + WATCHDOG_RESET(); + } } -#endif c = getc(); /* * Special character handling */ switch (c) { - case '\r': /* Enter */ + case '\r': /* Enter */ case '\n': *p = '\0'; puts ("\r\n"); - return (p - p_buf); + return p - p_buf; - case '\0': /* nul */ + case '\0': /* nul */ continue; - case 0x03: /* ^C - break */ + case 0x03: /* ^C - break */ p_buf[0] = '\0'; /* discard input */ - return (-1); + return -1; - case 0x15: /* ^U - erase line */ + case 0x15: /* ^U - erase line */ while (col > plen) { puts (erase_seq); --col; @@ -1112,15 +986,15 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) n = 0; continue; - case 0x17: /* ^W - erase word */ + case 0x17: /* ^W - erase word */ p=delete_char(p_buf, p, &col, &n, plen); while ((n > 0) && (*p != ' ')) { p=delete_char(p_buf, p, &col, &n, plen); } continue; - case 0x08: /* ^H - backspace */ - case 0x7F: /* DEL - backspace */ + case 0x08: /* ^H - backspace */ + case 0x7F: /* DEL - backspace */ p=delete_char(p_buf, p, &col, &n, plen); continue; @@ -1129,22 +1003,28 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) * Must be a normal character then */ if (n < CONFIG_SYS_CBSIZE-2) { - if (c == '\t') { /* expand TABs */ -#ifdef CONFIG_AUTO_COMPLETE - /* if auto completion triggered just continue */ - *p = '\0'; - if (cmd_auto_complete(prompt, console_buffer, &n, &col)) { - p = p_buf + n; /* reset */ - continue; + if (c == '\t') { /* expand TABs */ + if (config_auto_complete()) { + /* + * if auto completion triggered + * just continue + */ + *p = '\0'; + if (cmd_auto_complete(prompt, + console_buffer, + &n, &col)) { + /* reset */ + p = p_buf + n; + continue; + } } -#endif puts (tab_seq+(col&07)); col += 8 - (col&07); } else { char buf[2]; /* - * Echo input using puts() to force am + * Echo input using puts() to force an * LCD flush if we are using an LCD */ ++col; @@ -1159,14 +1039,47 @@ int readline_into_buffer(const char *const prompt, char *buffer, int timeout) } } } -#ifdef CONFIG_CMDLINE_EDITING +} + +int readline_into_buffer(const char *const prompt, char *buffer, int timeout) +{ + unsigned int len = CONFIG_SYS_CBSIZE; + int rc; + static int initted; + + /* + * History uses a global array which is not + * writable until after relocation to RAM. + * Revert to non-history version if still + * running from flash. + */ + if (config_cmdline_editing() && (gd->flags & GD_FLG_RELOC)) { + if (!initted) { + hist_init(); + initted = 1; + } + + if (prompt) + puts(prompt); + + rc = cread_line(prompt, buffer, &len, timeout); + return rc < 0 ? rc : len; + + } else { + int plen = 0; /* prompt length */ + + /* print prompt */ + if (prompt) { + plen = strlen(prompt); + puts(prompt); + } + return simple_readline(prompt, plen, buffer, timeout); } -#endif } /****************************************************************************/ -static char * delete_char (char *buffer, char *p, int *colp, int *np, int plen) +static char *delete_char (char *buffer, char *p, int *colp, int *np, int plen) { char *s; @@ -1202,9 +1115,7 @@ int parse_line (char *line, char *argv[]) { int nargs = 0; -#ifdef DEBUG_PARSER - printf ("parse_line: \"%s\"\n", line); -#endif + debug_parser("parse_line: \"%s\"\n", line); while (nargs < CONFIG_SYS_MAXARGS) { /* skip any white space */ @@ -1213,9 +1124,7 @@ int parse_line (char *line, char *argv[]) if (*line == '\0') { /* end of line, no more args */ argv[nargs] = NULL; -#ifdef DEBUG_PARSER - printf ("parse_line: nargs=%d\n", nargs); -#endif + debug_parser("parse_line: nargs=%d\n", nargs); return (nargs); } @@ -1227,9 +1136,7 @@ int parse_line (char *line, char *argv[]) if (*line == '\0') { /* end of line, no more args */ argv[nargs] = NULL; -#ifdef DEBUG_PARSER - printf ("parse_line: nargs=%d\n", nargs); -#endif + debug_parser("parse_line: nargs=%d\n", nargs); return (nargs); } @@ -1238,15 +1145,12 @@ int parse_line (char *line, char *argv[]) printf ("** Too many args (max. %d) **\n", CONFIG_SYS_MAXARGS); -#ifdef DEBUG_PARSER - printf ("parse_line: nargs=%d\n", nargs); -#endif + debug_parser("parse_line: nargs=%d\n", nargs); return (nargs); } /****************************************************************************/ -#ifndef CONFIG_SYS_HUSH_PARSER static void process_macros (const char *input, char *output) { char c, prev; @@ -1258,12 +1162,10 @@ static void process_macros (const char *input, char *output) /* 1 = waiting for '(' or '{' */ /* 2 = waiting for ')' or '}' */ /* 3 = waiting for ''' */ -#ifdef DEBUG_PARSER char *output_start = output; - printf ("[PROCESS_MACROS] INPUT len %d: \"%s\"\n", strlen (input), - input); -#endif + debug_parser("[PROCESS_MACROS] INPUT len %zd: \"%s\"\n", strlen(input), + input); prev = '\0'; /* previous character */ @@ -1351,10 +1253,8 @@ static void process_macros (const char *input, char *output) else *(output - 1) = 0; -#ifdef DEBUG_PARSER - printf ("[PROCESS_MACROS] OUTPUT len %d: \"%s\"\n", - strlen (output_start), output_start); -#endif + debug_parser("[PROCESS_MACROS] OUTPUT len %zd: \"%s\"\n", + strlen(output_start), output_start); } /**************************************************************************** @@ -1385,12 +1285,12 @@ static int builtin_run_command(const char *cmd, int flag) int repeatable = 1; int rc = 0; -#ifdef DEBUG_PARSER - printf ("[RUN_COMMAND] cmd[%p]=\"", cmd); - puts (cmd ? cmd : "NULL"); /* use puts - string may be loooong */ - puts ("\"\n"); -#endif - + debug_parser("[RUN_COMMAND] cmd[%p]=\"", cmd); + if (DEBUG_PARSER) { + /* use puts - string may be loooong */ + puts(cmd ? cmd : "NULL"); + puts("\"\n"); + } clear_ctrlc(); /* forget any previous Control C */ if (!cmd || !*cmd) { @@ -1408,9 +1308,7 @@ static int builtin_run_command(const char *cmd, int flag) * repeatable commands */ -#ifdef DEBUG_PARSER - printf ("[PROCESS_SEPARATORS] %s\n", cmd); -#endif + debug_parser("[PROCESS_SEPARATORS] %s\n", cmd); while (*str) { /* @@ -1439,9 +1337,7 @@ static int builtin_run_command(const char *cmd, int flag) } else str = sep; /* no more commands for next pass */ -#ifdef DEBUG_PARSER - printf ("token: \"%s\"\n", token); -#endif + debug_parser("token: \"%s\"\n", token); /* find macros in this token and replace them */ process_macros (token, finaltoken); @@ -1462,7 +1358,6 @@ static int builtin_run_command(const char *cmd, int flag) return rc ? rc : repeatable; } -#endif /* * Run a command using the selected parser. @@ -1473,22 +1368,21 @@ static int builtin_run_command(const char *cmd, int flag) */ int run_command(const char *cmd, int flag) { -#ifndef CONFIG_SYS_HUSH_PARSER - /* - * builtin_run_command can return 0 or 1 for success, so clean up - * its result. - */ - if (builtin_run_command(cmd, flag) == -1) - return 1; - - return 0; -#else - return parse_string_outer(cmd, + if (config_sys_hush_parser()) { + return parse_string_outer(cmd, FLAG_PARSE_SEMICOLON | FLAG_EXIT_FROM_LOOP); -#endif + } else { + /* + * builtin_run_command can return 0 or 1 for success, so + * clean up its result. + */ + if (builtin_run_command(cmd, flag) == -1) + return 1; + + return 0; + } } -#ifndef CONFIG_SYS_HUSH_PARSER /** * Execute a list of command separated by ; or \n using the built-in parser. * @@ -1529,7 +1423,6 @@ static int builtin_run_command_list(char *cmd, int flag) return rcode; } -#endif int run_command_list(const char *cmd, int len, int flag) { @@ -1539,13 +1432,16 @@ int run_command_list(const char *cmd, int len, int flag) if (len == -1) { len = strlen(cmd); -#ifdef CONFIG_SYS_HUSH_PARSER - /* hush will never change our string */ - need_buff = 0; -#else - /* the built-in parser will change our string if it sees \n */ - need_buff = strchr(cmd, '\n') != NULL; -#endif + if (config_sys_hush_parser()) { + /* hush will never change our string */ + need_buff = 0; + } else { + /* + * the built-in parser will change our string if it + * sees \n + */ + need_buff = strchr(cmd, '\n') != NULL; + } } if (need_buff) { buff = malloc(len + 1); @@ -1554,20 +1450,20 @@ int run_command_list(const char *cmd, int len, int flag) memcpy(buff, cmd, len); buff[len] = '\0'; } -#ifdef CONFIG_SYS_HUSH_PARSER - rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON); -#else - /* - * This function will overwrite any \n it sees with a \0, which - * is why it can't work with a const char *. Here we are making - * using of internal knowledge of this function, to avoid always - * doing a malloc() which is actually required only in a case that - * is pretty rare. - */ - rcode = builtin_run_command_list(buff, flag); - if (need_buff) - free(buff); -#endif + if (config_sys_hush_parser()) { + rcode = parse_string_outer(buff, FLAG_PARSE_SEMICOLON); + } else { + /* + * This function will overwrite any \n it sees with a \0, which + * is why it can't work with a const char *. Here we are making + * using of internal knowledge of this function, to avoid always + * doing a malloc() which is actually required only in a case + * that is pretty rare. + */ + rcode = builtin_run_command_list(buff, flag); + if (need_buff) + free(buff); + } return rcode; } diff --git a/include/command.h b/include/command.h index 3785eb9..80da938 100644 --- a/include/command.h +++ b/include/command.h @@ -75,10 +75,8 @@ cmd_tbl_t *find_cmd_tbl (const char *cmd, cmd_tbl_t *table, int table_len); extern int cmd_usage(const cmd_tbl_t *cmdtp); -#ifdef CONFIG_AUTO_COMPLETE extern int var_complete(int argc, char * const argv[], char last_char, int maxv, char *cmdv[]); extern int cmd_auto_complete(const char *const prompt, char *buf, int *np, int *colp); -#endif /* * Monitor Command diff --git a/include/common.h b/include/common.h index 4ad17ea..e5eb882 100644 --- a/include/common.h +++ b/include/common.h @@ -35,6 +35,9 @@ typedef volatile unsigned short vu_short; typedef volatile unsigned char vu_char; #include <config.h> +#ifndef DO_DEPS_ONLY +#include <generated/autoconf.h> +#endif #include <asm-offsets.h> #include <linux/bitops.h> #include <linux/types.h> @@ -294,9 +297,6 @@ int readline_into_buffer(const char *const prompt, char *buffer, int parse_line (char *, char *[]); void init_cmd_timeout(void); void reset_cmd_timeout(void); -#ifdef CONFIG_MENU -int abortboot(int bootdelay); -#endif extern char console_buffer[]; /* arch/$(ARCH)/lib/board.c */ @@ -310,6 +310,7 @@ extern ulong monitor_flash_len; int mac_read_from_eeprom(void); extern u8 _binary_dt_dtb_start[]; /* embedded device tree blob */ int set_cpu_clk_info(void); +extern int mdm_init(void); /* defined in board.c */ /** * Show the DRAM size in a board-specific way @@ -857,9 +858,7 @@ int pcmcia_init (void); #include <bootstage.h> -#ifdef CONFIG_SHOW_ACTIVITY void show_activity(int arg); -#endif /* Multicore arch functions */ #ifdef CONFIG_MP diff --git a/include/config_drop.h b/include/config_drop.h new file mode 100644 index 0000000..bf2beaa --- /dev/null +++ b/include/config_drop.h @@ -0,0 +1,17 @@ +/* + * Copyright 2013 Google, Inc + * + * This file is licensed under the terms of the GNU General Public + * License Version 2. This file is licensed "as is" without any + * warranty of any kind, whether express or implied. + */ + +#ifndef _CONFIG_DROP_H +#define _CONFIG_DROP_H + +/* Options which don't seem to be referred to anywhere in U-Boot */ +#define CONFIG_MENUPROMPT "Auto-boot prompt" +#define CONFIG_MENUKEY +#define CONFIG_UPDATE_TFTP + +#endif diff --git a/include/configs/pm9263.h b/include/configs/pm9263.h index b60a9ad..6f6ddfa 100644 --- a/include/configs/pm9263.h +++ b/include/configs/pm9263.h @@ -355,7 +355,7 @@ #define CONFIG_BOOTCOMMAND "run flashboot" #define CONFIG_ROOTPATH "/ronetix/rootfs" -#define CONFIG_AUTOBOOT_PROMPT "autoboot in %d seconds\n" +#define CONFIG_AUTOBOOT_PROMPT "autoboot in %d seconds\n", bootdelay #define CONFIG_CON_ROT "fbcon=rotate:3 " #define CONFIG_BOOTARGS "root=/dev/mtdblock4 rootfstype=jffs2 "\ diff --git a/include/fdt_support.h b/include/fdt_support.h index 2cccc35..cf8f5e0 100644 --- a/include/fdt_support.h +++ b/include/fdt_support.h @@ -24,7 +24,7 @@ #ifndef __FDT_SUPPORT_H #define __FDT_SUPPORT_H -#ifdef CONFIG_OF_LIBFDT +#ifndef USE_HOSTCC #include <libfdt.h> @@ -132,5 +132,5 @@ static inline int fdt_status_disabled_by_alias(void *fdt, const char* alias) return fdt_set_status_by_alias(fdt, alias, FDT_STATUS_DISABLED, 0); } -#endif /* ifdef CONFIG_OF_LIBFDT */ +#endif /* ifdef USE_HOSTCC */ #endif /* ifndef __FDT_SUPPORT_H */ diff --git a/include/hush.h b/include/hush.h index ecf9222..12c55f4 100644 --- a/include/hush.h +++ b/include/hush.h @@ -36,7 +36,5 @@ int set_local_var(const char *s, int flg_export); void unset_local_var(const char *name); char *get_local_var(const char *s); -#if defined(CONFIG_HUSH_INIT_VAR) extern int hush_init_var (void); #endif -#endif diff --git a/include/menu.h b/include/menu.h index 7af5fdb..1cdcb88 100644 --- a/include/menu.h +++ b/include/menu.h @@ -28,7 +28,5 @@ int menu_item_add(struct menu *m, char *item_key, void *item_data); int menu_destroy(struct menu *m); void menu_display_statusline(struct menu *m); -#if defined(CONFIG_MENU_SHOW) int menu_show(int bootdelay); -#endif #endif /* __MENU_H__ */ diff --git a/include/net.h b/include/net.h index 970d4d1..1b56b7d 100644 --- a/include/net.h +++ b/include/net.h @@ -695,6 +695,8 @@ extern void copy_filename(char *dst, const char *src, int size); /* get a random source port */ extern unsigned int random_port(void); +extern int update_tftp(ulong addr); + /**********************************************************************/ #endif /* __NET_H__ */ diff --git a/tools/scripts/define2conf.sed b/tools/scripts/define2conf.sed new file mode 100644 index 0000000..269eaba --- /dev/null +++ b/tools/scripts/define2conf.sed @@ -0,0 +1,36 @@ +# +# Sed script to parse CPP macros and generate a list of CONFIG macros +# +# This converts: +# #define CONFIG_XXX value +#into: +# #define config_xxx() value +# #define config_xxx_enabled() 1 +# + +# Macros with parameters are ignored. +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ { + d +} + +# Only process values prefixed with #define CONFIG_ +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ { + # Strip the #define prefix + s/#define[ \t]*//; + # Change to form CONFIG_*=VALUE + s/[\t ][\t ]*/=/; + # Handle lines with no value + s/^\([^=]*\)$/\1=/; + # Drop trailing spaces + s/ *$//; + # Change empty values to '1' + s/=$/=1/; + # Add #define at the start + s/^\([^=]*\)=/#define \L\1() / + # print the line + p + # Create a version suffixed with _enabled, value 1 + s/().*/_enabled() 1/ + # print the line + p +} diff --git a/tools/scripts/define2list.sed b/tools/scripts/define2list.sed new file mode 100644 index 0000000..8df8aa0 --- /dev/null +++ b/tools/scripts/define2list.sed @@ -0,0 +1,31 @@ +# +# Sed script to parse CPP macros and generate a list of CONFIG macros +# +# This converts: +# #define CONFIG_XXX value +#into: +# #define config_xxx() 0 +# #define config_xxx_enabled() 0 + +# Macros with parameters are ignored. +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*(/ { + s/.*// +} + +# Only process values prefixed with #define CONFIG_ +/^#define CONFIG_[A-Za-z0-9_][A-Za-z0-9_]*/ { + # Strip the #define prefix + s/#define *//; + # Remove the value + s/[ \t].*//; + # Convert to lower case, prepend #define + s/\(.*\)/#define \L\1/ + # Append 0 + s/$/() 0/ + # print the line + p + # Create a version suffixed with _enabled, value 0 + s/().*/_enabled() 0/ + # print the line + p +}
Many parts of the U-Boot code base are sprinkled with #ifdefs. This makes different boards compile different versions of the source code, meaning that we must build all boards to check for failures. It is easy to misspell an #ifdef and there is not as much checking of this by the compiler. Multiple dependent #ifdefs are harder to do than with if..then..else. Variable declarations must be #idefed as well as the code that uses them, often much later in the file/function. #ifdef indents don't match code indents and have their own separate indent feature. Overall, excessive use of #idef hurts readability and makes the code harder to modify and refactor. For people coming newly into the code base, #ifdefs can be a big barrier. The use of #ifdef in U-Boot has possibly got a little out of hand. In an attempt to turn the tide, this patch provides a way to make CONFIG macros available to C code without using the preprocessor. This makes it possible to use standard C conditional features such as if/then instead of #ifdef. A README update exhorts compliance. As an example of how to use this, this patch replaces all but two #ifdefs from the main code body of common/main.c, which has the dubious distinction of having the most #ifdefs by at least one measure: $ for f in $(find . -name *.c); do echo $(grep -c "ifdef" $f) $f; done \ |sort -nr |head 57 ./common/main.c 57 ./arch/powerpc/cpu/mpc83xx/cpu_init.c 48 ./arch/powerpc/lib/board.c 46 ./drivers/video/cfb_console.c 40 ./drivers/mtd/cfi_flash.c 38 ./net/tftp.c 38 ./common/cmd_bootm.c 37 ./drivers/usb/host/ohci-hcd.c 36 ./drivers/fpga/ivm_core.c 35 ./drivers/usb/gadget/ether.c Code size for this patch seems to be roughly neutral (below numbers are average change in byte size for each region: x86: (3 boards) text -1.3 data +1.3 sandbox: (1 boards) bss +16.0 m68k: (50 boards) text -4.8 powerpc: (634 boards) text +7.4 data +0.0 bss +2.1 sh: (21 boards) text -9.1 bss +2.5 nios2: (3 boards) text +24.0 arm: (287 boards) spl/u-boot-spl:text -0.3 text -2.3 bss +7.2 nds32: (3 boards) text -16.0 Signed-off-by: Simon Glass <sjg@chromium.org> --- Makefile | 41 ++- README | 87 ++++- common/cmd_fitupd.c | 1 + common/main.c | 794 ++++++++++++++++++------------------------ include/command.h | 2 - include/common.h | 9 +- include/config_drop.h | 17 + include/configs/pm9263.h | 2 +- include/fdt_support.h | 4 +- include/hush.h | 2 - include/menu.h | 2 - include/net.h | 2 + tools/scripts/define2conf.sed | 36 ++ tools/scripts/define2list.sed | 31 ++ 14 files changed, 563 insertions(+), 467 deletions(-) create mode 100644 include/config_drop.h create mode 100644 tools/scripts/define2conf.sed create mode 100644 tools/scripts/define2list.sed