Patchwork [U-Boot,1/3] ARM: lib: Remove CONFIG_ARCH_CPU_INIT dependency

login
register
mail settings
Submitter Fabio Estevam
Date March 1, 2012, 2:02 p.m.
Message ID <1330610560-15978-1-git-send-email-fabio.estevam@freescale.com>
Download mbox | patch
Permalink /patch/144016/
State Accepted
Commit a21c65115bd95572cc80092a31b0e9ecb8710e9f
Headers show

Comments

Fabio Estevam - March 1, 2012, 2:02 p.m.
Create a weak-aliased arch_cpu_init, so that we can get rid of CONFIG_ARCH_CPU_INIT
and always call arch_cpu_init.

This way we do not need to define CONFIG_ARCH_CPU_INIT in every board file, since
arch_cpu_init() is supposed to handle common CPU level code.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 arch/arm/lib/board.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)
Marek Vasut - March 1, 2012, 6:31 p.m.
> Create a weak-aliased arch_cpu_init, so that we can get rid of
> CONFIG_ARCH_CPU_INIT and always call arch_cpu_init.
> 
> This way we do not need to define CONFIG_ARCH_CPU_INIT in every board file,
> since arch_cpu_init() is supposed to handle common CPU level code.

Acked-by: Marek Vasut <marex@denx.de>

> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/lib/board.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 500e216..6463db5 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -224,10 +224,16 @@ void __dram_init_banksize(void)
>  void dram_init_banksize(void)
>  	__attribute__((weak, alias("__dram_init_banksize")));
> 
> +int __arch_cpu_init(void)
> +{
> +	return 0;
> +}
> +int arch_cpu_init(void)
> +	__attribute__((weak, alias("__arch_cpu_init")));
> +
>  init_fnc_t *init_sequence[] = {
> -#if defined(CONFIG_ARCH_CPU_INIT)
>  	arch_cpu_init,		/* basic arch cpu dependent setup */
> -#endif
> +
>  #if defined(CONFIG_BOARD_EARLY_INIT_F)
>  	board_early_init_f,
>  #endif
Albert ARIBAUD - March 1, 2012, 8:51 p.m.
Le 01/03/2012 15:02, Fabio Estevam a écrit :
> Create a weak-aliased arch_cpu_init, so that we can get rid of CONFIG_ARCH_CPU_INIT
> and always call arch_cpu_init.
>
> This way we do not need to define CONFIG_ARCH_CPU_INIT in every board file, since
> arch_cpu_init() is supposed to handle common CPU level code.

This adds (some) (functionally dead) code to all boards that did not 
require arch_cpu_init().

Amicalement,
Fabio Estevam - March 1, 2012, 8:57 p.m.
On Thu, Mar 1, 2012 at 5:51 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:

> This adds (some) (functionally dead) code to all boards that did not require
> arch_cpu_init().

Yes, it adds a "return 0" only. Is this terribly bad?

Isn't it better than having to select CONFIG_ARCH_CPU_INIT for every
board to select arch_cpu_init(), which is supposed to be CPU
dependent?
Albert ARIBAUD - March 1, 2012, 9:44 p.m.
Le 01/03/2012 21:57, Fabio Estevam a écrit :
> On Thu, Mar 1, 2012 at 5:51 PM, Albert ARIBAUD
> <albert.u.boot@aribaud.net>  wrote:
>
>> This adds (some) (functionally dead) code to all boards that did not require
>> arch_cpu_init().
>
> Yes, it adds a "return 0" only. Is this terribly bad?

Well...

> Isn't it better than having to select CONFIG_ARCH_CPU_INIT for every
> board to select arch_cpu_init(), which is supposed to be CPU
> dependent?

It adds a "#define CONFIG_ARCH_CPU_INTI" only. Is this terribly bad ?

If what bothers you is that some board all repeat something that should 
be done only once, then it should bother you just as much that many 
boards repeat something that should not be done at all most of the time.

I understand that you see a repetition of CONFIG_ARCH_CPU_INIT that you 
suspect is highly redundant. But then, you should cure the repetition, 
not the existence of CONFIG_ARCH_CPU_INIT.

How about this approach: since the problem is the presence of CPU- (or 
Soc-)level elements in board-level config files, and since there are 
probably many other such items in board config files (CPU type in a SoC 
and HW IP configurations, for instance) then why not introduce some 
cpu-config and soc-config file which would group all those cpu- (resp 
soc-)level items, including any "#define CONFIG_ARCH_CPU_INIT", and 
replace these items in board config files with a single "#include 
"<cpu>-config.h" (or "<soc>-config.h")? This would significantly reduce 
board config file size.

One could even choose either of two courses depending on how many or how 
few boards require CONFIG_ARCH_CPU_INIT: if many boards require it, let 
the cpu- or soc-common config contain the define, and the few boards 
which don't will have to explicitly undefine it; if only a few boards 
need the define, then it won't be done in the cpu- or soc-common config 
but in each board that requires it.

And CPUs, SoCs and boards that don't need ARCH_CPU_INIT at all won't 
grow a single useless byte.

Amicalement,
Stefano Babic - March 3, 2012, 10:01 a.m.
On 01/03/2012 15:02, Fabio Estevam wrote:
> Create a weak-aliased arch_cpu_init, so that we can get rid of CONFIG_ARCH_CPU_INIT
> and always call arch_cpu_init.
> 
> This way we do not need to define CONFIG_ARCH_CPU_INIT in every board file, since
> arch_cpu_init() is supposed to handle common CPU level code.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/lib/board.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 500e216..6463db5 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -224,10 +224,16 @@ void __dram_init_banksize(void)
>  void dram_init_banksize(void)
>  	__attribute__((weak, alias("__dram_init_banksize")));
>  
> +int __arch_cpu_init(void)
> +{
> +	return 0;
> +}
> +int arch_cpu_init(void)
> +	__attribute__((weak, alias("__arch_cpu_init")));
> +
>  init_fnc_t *init_sequence[] = {
> -#if defined(CONFIG_ARCH_CPU_INIT)
>  	arch_cpu_init,		/* basic arch cpu dependent setup */
> -#endif
> +
>  #if defined(CONFIG_BOARD_EARLY_INIT_F)
>  	board_early_init_f,
>  #endif

Acked-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic
Tom Rini - March 5, 2012, 5:52 p.m.
On Thu, Mar 01, 2012 at 11:02:38AM -0300, Fabio Estevam wrote:
> Create a weak-aliased arch_cpu_init, so that we can get rid of CONFIG_ARCH_CPU_INIT
> and always call arch_cpu_init.
> 
> This way we do not need to define CONFIG_ARCH_CPU_INIT in every board file, since
> arch_cpu_init() is supposed to handle common CPU level code.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  arch/arm/lib/board.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 500e216..6463db5 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -224,10 +224,16 @@ void __dram_init_banksize(void)
>  void dram_init_banksize(void)
>  	__attribute__((weak, alias("__dram_init_banksize")));
>  
> +int __arch_cpu_init(void)
> +{
> +	return 0;
> +}
> +int arch_cpu_init(void)
> +	__attribute__((weak, alias("__arch_cpu_init")));
> +

Please add <linux/compiler.h> and use __weak instead.
Tom Rini - March 5, 2012, 5:56 p.m.
On Thu, Mar 01, 2012 at 10:44:09PM +0100, Albert ARIBAUD wrote:

> How about this approach: since the problem is the presence of CPU-
> (or Soc-)level elements in board-level config files, and since there
> are probably many other such items in board config files (CPU type
> in a SoC and HW IP configurations, for instance) then why not
> introduce some cpu-config and soc-config file which would group all
> those cpu- (resp soc-)level items, including any "#define
> CONFIG_ARCH_CPU_INIT", and replace these items in board config files
> with a single "#include "<cpu>-config.h" (or "<soc>-config.h")? This
> would significantly reduce board config file size.

Note that omap4/5 do something like this already and my hope is to do
this for omap3/similar and make some other common files as well.  It's
not the top priority for me atm however...
Fabio Estevam - March 5, 2012, 5:58 p.m.
On Mon, Mar 5, 2012 at 2:52 PM, Tom Rini <trini@ti.com> wrote:

>> +int __arch_cpu_init(void)
>> +{
>> +     return 0;
>> +}
>> +int arch_cpu_init(void)
>> +     __attribute__((weak, alias("__arch_cpu_init")));
>> +
>
> Please add <linux/compiler.h> and use __weak instead.

Ok, before I rework this patch I would like to confirm with Albert if
I should proceed with this patch series, or if he NACKs this idea.

Regards,

Fabio Estevam
Fabio Estevam - March 5, 2012, 6:12 p.m.
On Mon, Mar 5, 2012 at 2:58 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Mar 5, 2012 at 2:52 PM, Tom Rini <trini@ti.com> wrote:
>
>>> +int __arch_cpu_init(void)
>>> +{
>>> +     return 0;
>>> +}
>>> +int arch_cpu_init(void)
>>> +     __attribute__((weak, alias("__arch_cpu_init")));
>>> +
>>
>> Please add <linux/compiler.h> and use __weak instead.
>
> Ok, before I rework this patch I would like to confirm with Albert if
> I should proceed with this patch series, or if he NACKs this idea.

By "this idea" I mean the removal of CONFIG_ARCH_CPU_INIT.
Albert ARIBAUD - March 6, 2012, 7:26 a.m.
Hi Tom,

Le 05/03/2012 18:56, Tom Rini a écrit :
> On Thu, Mar 01, 2012 at 10:44:09PM +0100, Albert ARIBAUD wrote:
>
>> How about this approach: since the problem is the presence of CPU-
>> (or Soc-)level elements in board-level config files, and since there
>> are probably many other such items in board config files (CPU type
>> in a SoC and HW IP configurations, for instance) then why not
>> introduce some cpu-config and soc-config file which would group all
>> those cpu- (resp soc-)level items, including any "#define
>> CONFIG_ARCH_CPU_INIT", and replace these items in board config files
>> with a single "#include "<cpu>-config.h" (or "<soc>-config.h")? This
>> would significantly reduce board config file size.
>
> Note that omap4/5 do something like this already and my hope is to do
> this for omap3/similar and make some other common files as well.  It's
> not the top priority for me atm however...

Ok, so that's one more reason not to go the "everybody call (a possibly 
weak and empty) arch_cpu_init()" route.

Amicalement,

Patch

diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 500e216..6463db5 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -224,10 +224,16 @@  void __dram_init_banksize(void)
 void dram_init_banksize(void)
 	__attribute__((weak, alias("__dram_init_banksize")));
 
+int __arch_cpu_init(void)
+{
+	return 0;
+}
+int arch_cpu_init(void)
+	__attribute__((weak, alias("__arch_cpu_init")));
+
 init_fnc_t *init_sequence[] = {
-#if defined(CONFIG_ARCH_CPU_INIT)
 	arch_cpu_init,		/* basic arch cpu dependent setup */
-#endif
+
 #if defined(CONFIG_BOARD_EARLY_INIT_F)
 	board_early_init_f,
 #endif