[U-Boot] [RFC PATCH] Provide a mechanism to avoid using #ifdef everywhere

[Simon Glass]

Hi Wolfgang,

On Tue, Feb 19, 2013 at 1:19 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Simon,
>
> In message <CAPnjgZ38HPmOf07CMYssa1Q167sRjzDpbBUbCVMaoGdRoWESog at 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 at denx.de
> If you think the problem is bad now, just wait until we've solved it.
>                                                         Epstein's Law