From: Wolfgang Denk <wd@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [RFC PATCH] Provide a mechanism to avoid using #ifdef everywhere
Date: Tue, 19 Feb 2013 10:19:15 +0100 [thread overview]
Message-ID: <20130219091915.6690D200530@gemini.denx.de> (raw)
In-Reply-To: <CAPnjgZ38HPmOf07CMYssa1Q167sRjzDpbBUbCVMaoGdRoWESog@mail.gmail.com>
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
--
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 at denx.de
If you think the problem is bad now, just wait until we've solved it.
Epstein's Law
next prev parent reply other threads:[~2013-02-19 9:19 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-18 17:18 [U-Boot] [RFC PATCH] Provide a mechanism to avoid using #ifdef everywhere Simon Glass
2013-02-18 19:23 ` Wolfgang Denk
2013-02-18 21:36 ` Tom Rini
2013-02-19 5:18 ` Simon Glass
2013-02-19 5:13 ` Simon Glass
2013-02-19 9:19 ` Wolfgang Denk [this message]
2013-02-19 14:17 ` Harvey Chapman
2013-02-19 16:48 ` Simon Glass
2013-02-19 19:14 ` Wolfgang Denk
2013-02-21 20:58 ` Simon Glass
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130219091915.6690D200530@gemini.denx.de \
--to=wd@denx.de \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.