[U-Boot] [PATCH V2] arm/board.c: avoid ifdef using weak default functions

[U-Boot] [PATCH V2] arm/board.c: avoid ifdef using weak default functions

[Alessandro Rubini]

From: Alessandro Rubini <rubini@unipv.it>

While it's a matter of personal taste, I prefer to avoid ifdef when
possible.  For example, I don't like to add BOARD_LATE_INIT in the
config file just to add a board_late_init() function.
Also, I think the file is more readable without the ifdef stuff.
This uses two trivial weak functions to provide defaults for all
functions that were ifdeffed.

This patch was initially rejected in favor of a initcall mechanism
but that approach is not a work in progress any ore.
---

One complaint I got about this is the runtime overhead. Actually,
normal_nop is two instructions (plus the call to it) and void_nop
is one instruction (plus the call), similar to the overhead in
led management for platforms with no leds.


 lib_arm/board.c |   62 ++++++++++++++++++++++++++----------------------------
 1 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/lib_arm/board.c b/lib_arm/board.c
index e148739..122e37a 100644
--- a/lib_arm/board.c
+++ b/lib_arm/board.c
@@ -65,10 +65,26 @@ DECLARE_GLOBAL_DATA_PTR;
 
 ulong monitor_flash_len;
 
-#ifdef CONFIG_HAS_DATAFLASH
-extern int  AT91F_DataflashInit(void);
-extern void dataflash_print_info(void);
-#endif
+/* Some functions may be there or not. Use a weak placeholder to avoid ifdef */
+int __normal_nop(void) { return 0; }
+void __void_nop(void) {}
+
+/* parts of init_sequence that may not be present */
+int arch_cpu_init(void) __attribute__((weak, alias("__normal_nop")));
+int interrupt_init(void) __attribute__((weak, alias("__normal_nop")));
+int print_cpuinfo(void) __attribute__((weak, alias("__normal_nop")));
+int checkboard(void) __attribute__((weak, alias("__normal_nop")));
+
+/* other functions that appear later and depend on config items */
+int AT91F_DataflashInit(void) __attribute__((weak, alias("__normal_nop")));
+void dataflash_print_info(void) __attribute__((weak, alias("__void_nop")));
+void serial_initialize(void) __attribute__((weak, alias("__void_nop")));
+void onenand_init(void) __attribute__((weak, alias("__void_nop")));
+int drv_vfd_init(void) __attribute__((weak, alias("__normal_nop")));
+void api_init(void)  __attribute__((weak, alias("__void_nop")));
+int arch_misc_init(void) __attribute__((weak, alias("__normal_nop")));
+int misc_init_r(void) __attribute__((weak, alias("__normal_nop")));
+int board_late_init(void) __attribute__((weak, alias("__normal_nop")));
 
 #ifndef CONFIG_IDENT_STRING
 #define CONFIG_IDENT_STRING ""
@@ -196,6 +212,11 @@ static int init_func_i2c (void)
 	puts ("ready\n");
 	return (0);
 }
+#else
+static int init_func_i2c (void)
+{
+	return 0;
+}
 #endif
 
 #if defined(CONFIG_CMD_PCI) || defined (CONFIG_PCI)
@@ -205,6 +226,11 @@ static int arm_pci_init(void)
 	pci_init();
 	return 0;
 }
+#else
+static int arm_pci_init(void)
+{
+	return 0;
+}
 #endif /* CONFIG_CMD_PCI || CONFIG_PCI */
 
 /*
@@ -235,32 +261,20 @@ typedef int (init_fnc_t) (void);
 int print_cpuinfo (void);
 
 init_fnc_t *init_sequence[] = {
-#if defined(CONFIG_ARCH_CPU_INIT)
 	arch_cpu_init,		/* basic arch cpu dependent setup */
-#endif
 	board_init,		/* basic board dependent setup */
-#if defined(CONFIG_USE_IRQ)
 	interrupt_init,		/* set up exceptions */
-#endif
 	timer_init,		/* initialize timer */
 	env_init,		/* initialize environment */
 	init_baudrate,		/* initialze baudrate settings */
 	serial_init,		/* serial communications setup */
 	console_init_f,		/* stage 1 init of console */
 	display_banner,		/* say that we are here */
-#if defined(CONFIG_DISPLAY_CPUINFO)
 	print_cpuinfo,		/* display cpu info (and speed) */
-#endif
-#if defined(CONFIG_DISPLAY_BOARDINFO)
 	checkboard,		/* display board info */
-#endif
-#if defined(CONFIG_HARD_I2C) || defined(CONFIG_SOFT_I2C)
 	init_func_i2c,
-#endif
 	dram_init,		/* configure available RAM banks */
-#if defined(CONFIG_CMD_PCI) || defined (CONFIG_PCI)
 	arm_pci_init,
-#endif
 	display_dram_config,
 	NULL,
 };
@@ -335,26 +349,18 @@ void start_armboot (void)
 	nand_init();		/* go init the NAND */
 #endif
 
-#if defined(CONFIG_CMD_ONENAND)
 	onenand_init();
-#endif
 
-#ifdef CONFIG_HAS_DATAFLASH
 	AT91F_DataflashInit();
 	dataflash_print_info();
-#endif
 
 	/* initialize environment */
 	env_relocate ();
 
-#ifdef CONFIG_VFD
 	/* must do this after the framebuffer is allocated */
 	drv_vfd_init();
-#endif /* CONFIG_VFD */
 
-#ifdef CONFIG_SERIAL_MULTI
 	serial_initialize();
-#endif
 
 	/* IP Address */
 	gd->bd->bi_ip_addr = getenv_IPaddr ("ipaddr");
@@ -363,21 +369,15 @@ void start_armboot (void)
 
 	jumptable_init ();
 
-#if defined(CONFIG_API)
 	/* Initialize API */
 	api_init ();
-#endif
 
 	console_init_r ();	/* fully init console as a device */
 
-#if defined(CONFIG_ARCH_MISC_INIT)
 	/* miscellaneous arch dependent initialisations */
 	arch_misc_init ();
-#endif
-#if defined(CONFIG_MISC_INIT_R)
 	/* miscellaneous platform dependent initialisations */
 	misc_init_r ();
-#endif
 
 	/* enable exceptions */
 	enable_interrupts ();
@@ -412,9 +412,7 @@ extern void davinci_eth_set_mac_addr (const u_int8_t *addr);
 	}
 #endif
 
-#ifdef BOARD_LATE_INIT
 	board_late_init ();
-#endif
 
 #ifdef CONFIG_GENERIC_MMC
 	puts ("MMC:   ");
-- 
1.6.0.2

[U-Boot] [PATCH V2] arm/board.c: avoid ifdef using weak default functions

[Joakim Tjernlund]

>
> From: Alessandro Rubini <rubini@unipv.it>
>
> While it's a matter of personal taste, I prefer to avoid ifdef when
> possible.  For example, I don't like to add BOARD_LATE_INIT in the
> config file just to add a board_late_init() function.
> Also, I think the file is more readable without the ifdef stuff.
> This uses two trivial weak functions to provide defaults for all
> functions that were ifdeffed.
>
> This patch was initially rejected in favor of a initcall mechanism
> but that approach is not a work in progress any ore.
> ---
>
> One complaint I got about this is the runtime overhead. Actually,
> normal_nop is two instructions (plus the call to it) and void_nop
> is one instruction (plus the call), similar to the overhead in
> led management for platforms with no leds.

I think (I have proposed this before) that you should get rid of
the init_fnc_t *init_sequence[] array all together. It only
adds relocation overhead(lots of it). Just make the array a function
that calls the init functions directly.

    Jocke

[U-Boot] [PATCH V2] arm/board.c: avoid ifdef using weak default functions

[Tom]

Alessandro Rubini wrote:
> From: Alessandro Rubini <rubini@unipv.it>
> 
> While it's a matter of personal taste, I prefer to avoid ifdef when
> possible.  For example, I don't like to add BOARD_LATE_INIT in the
> config file just to add a board_late_init() function.
> Also, I think the file is more readable without the ifdef stuff.
> This uses two trivial weak functions to provide defaults for all
> functions that were ifdeffed.
> 
> This patch was initially rejected in favor of a initcall mechanism
> but that approach is not a work in progress any ore.
> ---
> 
> One complaint I got about this is the runtime overhead. Actually,
> normal_nop is two instructions (plus the call to it) and void_nop
> is one instruction (plus the call), similar to the overhead in
> led management for platforms with no leds.
> 
> 
>  lib_arm/board.c |   62 ++++++++++++++++++++++++++----------------------------
>  1 files changed, 30 insertions(+), 32 deletions(-)
> 

Thanks.
I cleaned the comment up and pushed this to a testing branch
arm/testing-arm_init.

I like the idea of cleaning up the arm init functions.
The board startup is more readable with the weak functions.

I put this in a testing branch because it is a big change that
needs general runtime testing and comments from everyone. I am also
interested in adding other init changes.

When we reach a concenseus, we can move this into next.

Tom

[U-Boot] [PATCH V2] arm/board.c: avoid ifdef using weak default functions

[Tom]

Joakim Tjernlund wrote:
>> From: Alessandro Rubini <rubini@unipv.it>
>>
>> While it's a matter of personal taste, I prefer to avoid ifdef when
>> possible.  For example, I don't like to add BOARD_LATE_INIT in the
>> config file just to add a board_late_init() function.
>> Also, I think the file is more readable without the ifdef stuff.
>> This uses two trivial weak functions to provide defaults for all
>> functions that were ifdeffed.
>>
>> This patch was initially rejected in favor of a initcall mechanism
>> but that approach is not a work in progress any ore.
>> ---
>>
>> One complaint I got about this is the runtime overhead. Actually,
>> normal_nop is two instructions (plus the call to it) and void_nop
>> is one instruction (plus the call), similar to the overhead in
>> led management for platforms with no leds.
> 
> I think (I have proposed this before) that you should get rid of
> the init_fnc_t *init_sequence[] array all together. It only
> adds relocation overhead(lots of it). Just make the array a function
> that calls the init functions directly.
>

Please resend the RFC patch.
This would also go the arm/testing-arm_init branch.

I am interested in flexibility and maintainability.
Not so much on size.

Tom
>     Jocke
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH V2] arm/board.c: avoid ifdef using weak default functions

[Joakim Tjernlund]

Tom <Tom.Rix@windriver.com> wrote on 26/11/2009 20:18:22:
>
> Joakim Tjernlund wrote:
> >> From: Alessandro Rubini <rubini@unipv.it>
> >>
> >> While it's a matter of personal taste, I prefer to avoid ifdef when
> >> possible.  For example, I don't like to add BOARD_LATE_INIT in the
> >> config file just to add a board_late_init() function.
> >> Also, I think the file is more readable without the ifdef stuff.
> >> This uses two trivial weak functions to provide defaults for all
> >> functions that were ifdeffed.
> >>
> >> This patch was initially rejected in favor of a initcall mechanism
> >> but that approach is not a work in progress any ore.
> >> ---
> >>
> >> One complaint I got about this is the runtime overhead. Actually,
> >> normal_nop is two instructions (plus the call to it) and void_nop
> >> is one instruction (plus the call), similar to the overhead in
> >> led management for platforms with no leds.
> >
> > I think (I have proposed this before) that you should get rid of
> > the init_fnc_t *init_sequence[] array all together. It only
> > adds relocation overhead(lots of it). Just make the array a function
> > that calls the init functions directly.
> >
>
> Please resend the RFC patch.
> This would also go the arm/testing-arm_init branch.

I just sent a complete patch for ppc(as that is my platform).
You choose if you want to do a similar one for ARM.
There probably wont be any space savings for ARM as
you don't use relocation yet.

      Jocke