[U-Boot] [RFC][PATCH] Code Clean-up (weak functions)

[Shinya Kuribayashi]

Graeme Russ wrote:
> This patch makes all definitions, declarations and usages of weak functions
> consistent.
> 
> Signed-off-by: Graeme Russ <graeme.russ at gmail.com>
> ---
> 
> WARNING: This patch hits a _lot_ of arches - Please
> 
> The following patch applies the following rules:
> 
>  - All functions are defined without __attribute__((weak)) in header files

Agreed.

>  - All weak functions are declared as __function() in the source file with
>    funtion() __attribute__((weak, alias("function"))); on the line immediately
>    after the closing brace of __function() - for example:
>      void __do_something (args)
>      {
>      ...some code...
>      }
>      do_something(args) __atttribute__((weak, alias("__do_something")));

Why do we need to being consistent?  I don't see real/technical benefits
to doing so.  Using alias or not should be each developer's business.  I
completely disagree with forcing alias use here.

> - There is no purely weak functions and therfore no longer code like:
>     if (do_something)
>       do_somthing();
>   All instances have been replaced by empty functions with an alias. e.g.
>      void __do_something (args) {}
>      do_something(args) __atttribute__((weak, alias("__do_something")));

Agreed, please fix them.

> diff --git a/board/incaip/incaip.c b/board/incaip/incaip.c
> index 3b30970..3ee3ac9 100644
> --- a/board/incaip/incaip.c
> +++ b/board/incaip/incaip.c
> @@ -31,7 +31,7 @@
> 
>  extern uint incaip_get_cpuclk(void);
> 
> -void _machine_restart(void)
> +void machine_restart(void)
>  {
>  	*INCA_IP_WDT_RST_REQ = 0x3f;
>  }

Why change the function name?  It's derived from Linux/MIPS, and I'd
like to keep consistent with with him.  Please don't touch it.

> diff --git a/board/purple/purple.c b/board/purple/purple.c
> index 54bef65..c243487 100644
> --- a/board/purple/purple.c
> +++ b/board/purple/purple.c
> @@ -54,7 +54,7 @@ extern int	asc_serial_getc		(void);
>  extern int	asc_serial_tstc		(void);
>  extern void	asc_serial_setbrg	(void);
> 
> -void _machine_restart(void)
> +void machine_restart(void)
>  {
>  	void (*f)(void) = (void *) 0xbfc00000;
> 
> diff --git a/board/tb0229/tb0229.c b/board/tb0229/tb0229.c
> index d3f05b2..f74573b 100644
> --- a/board/tb0229/tb0229.c
> +++ b/board/tb0229/tb0229.c
> @@ -16,7 +16,7 @@
>  #include <asm/reboot.h>
>  #include <pci.h>
> 
> -void _machine_restart(void)
> +void machine_restart(void)
>  {
>  	void (*f)(void) = (void *) 0xbfc00000;
> 

Ditto.

> diff --git a/cpu/mips/cpu.c b/cpu/mips/cpu.c
> index b7180b0..84c4730 100644
> --- a/cpu/mips/cpu.c
> +++ b/cpu/mips/cpu.c
> @@ -38,13 +38,13 @@
>  	:								\
>  	: "i" (op), "R" (*(unsigned char *)(addr)))
> 
> -void __attribute__((weak)) _machine_restart(void)
> -{
> -}
> +void inline __machine_restart(void) {}
> +void inline machine_restart (void)
> +	__attribute__((weak, alias("__machine_restart")));
> 
>  int do_reset(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  {
> -	_machine_restart();
> +	machine_restart();
> 
>  	fprintf(stderr, "*** reset failed ***\n");
>  	return 0;

Why inline?  It seems totally wrong to me.

> diff --git a/include/asm-mips/reboot.h b/include/asm-mips/reboot.h
> index 978d206..26885c0 100644
> --- a/include/asm-mips/reboot.h
> +++ b/include/asm-mips/reboot.h
> @@ -9,6 +9,6 @@
>  #ifndef _ASM_REBOOT_H
>  #define _ASM_REBOOT_H
> 
> -extern void _machine_restart(void);
> +extern void machine_restart(void);
> 
>  #endif /* _ASM_REBOOT_H */

Ditto.


Then I've read whole this thread, and will vote for Remy.

> > 
>> >>  All instances have been replaced by empty functions with an alias. e.g.
>> >>     void __do_something (args) {}
>> >>     do_something(args) __atttribute__((weak, alias("__do_something")));
> > 
> > Notice that in Linux, the 'alias' construction is not being used
> > massively. Can it be removed here also, or is it somehow mandatory
> > here?
> 
> I don't think it is mandatory but it is in the majority in u-boot.

If it's not mandatory, please drop all aliases.  That saves source code
size (not generated u-boot image size) a little bit.  Majority or not
does not make sense here.

-- 
Shinya Kuribayashi
NEC Electronics