Patchwork [U-Boot,resend,2/2] arm/boards: Define a new config option CONFIG_BOOT_PARAMS_P

login
register
mail settings
Submitter Vipin Kumar
Date Dec. 6, 2012, 9:29 a.m.
Message ID <ee5f783dffac1a94acdd7bb7f8f20cc6c4af7019.1354786138.git.vipin.kumar@st.com>
Download mbox | patch
Permalink /patch/204193/
State Rejected
Delegated to: Wolfgang Denk
Headers show

Comments

Vipin Kumar - Dec. 6, 2012, 9:29 a.m.
A lot of ARM boards are using board_init routine just to initialize boot_params
variable in the global data structure.

This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option
which is assigned to gd->bd->bi_boot_params automatically

Consequently, many board_init routines would not be required in the respective
board directories and a weak definition becomes necessary before their removal
from the code.

Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
---
 README               |  6 ++++++
 arch/arm/lib/board.c | 12 ++++++++++++
 2 files changed, 18 insertions(+)
Stefan Roese - Dec. 6, 2012, 9:44 a.m.
On 12/06/2012 10:29 AM, Vipin Kumar wrote:
> A lot of ARM boards are using board_init routine just to initialize boot_params
> variable in the global data structure.
> 
> This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option
> which is assigned to gd->bd->bi_boot_params automatically
> 
> Consequently, many board_init routines would not be required in the respective
> board directories and a weak definition becomes necessary before their removal
> from the code.
> 
> Signed-off-by: Vipin Kumar <vipin.kumar@st.com>
> ---
>  README               |  6 ++++++
>  arch/arm/lib/board.c | 12 ++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/README b/README
> index 037513a..2077c3b 100644
> --- a/README
> +++ b/README
> @@ -550,6 +550,12 @@ The following options need to be configured:
>  		in a single configuration file and the machine type is
>  		runtime discoverable, do not have to use this setting.
>  
> +		CONFIG_BOOT_PARAMS_P		[relevant for ARM only]
> +
> +		This config option can provide a way to initialize
> +		bi_boot_params from the u-boot infrastructure itself. The
> +		board still has the option to override it in board_init routine
> +
>  - vxWorks boot parameters:
>  
>  		bootvx constructs a valid bootline using the following
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 92cad9a..fa161b8 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -399,6 +399,11 @@ void board_init_f(ulong bootflag)
>  	gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
>  #endif
>  
> +#ifdef CONFIG_BOOT_PARAMS_P
> +	/* Boot params passed to Linux */
> +	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
> +#endif

Again an ugly #ifdef. Why not something like this instead:

Define a default earlier in the code (is 0x100 the best default?):

#ifndef CONFIG_BOOT_PARAMS_P
#define CONFIG_BOOT_PARAMS_P	0x100
#endif

then here just:

	/* Boot params passed to Linux */
	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;

without the #ifdef.

> +
>  	addr_sp -= sizeof (gd_t);
>  	id = (gd_t *) addr_sp;
>  	debug("Reserving %zu Bytes for Global Data at: %08lx\n",
> @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
>  static char *failed = "*** failed ***\n";
>  #endif
>  
> +static int __def_board_init(bd_t *bis)
> +{
> +	return -1;

Is -1 a good value to return as default board_init()?

> +}
> +
> +int board_init(void) __attribute__((weak, alias("__def_board_init")));
> +

Use __weak from inlcude/linux/compiler.h instead:

int __weak board_init(bd_t *bis)
...


Thanks,
Stefan
Vipin Kumar - Dec. 6, 2012, 9:56 a.m.
On 12/6/2012 3:14 PM, Stefan Roese wrote:
> On 12/06/2012 10:29 AM, Vipin Kumar wrote:
>> A lot of ARM boards are using board_init routine just to initialize boot_params
>> variable in the global data structure.
>>
>> This patch lets the board config files to define a CONFIG_BOOT_PARAMS_P option
>> which is assigned to gd->bd->bi_boot_params automatically
>>
>> Consequently, many board_init routines would not be required in the respective
>> board directories and a weak definition becomes necessary before their removal
>> from the code.
>>
>> Signed-off-by: Vipin Kumar<vipin.kumar@st.com>
>> ---
>>   README               |  6 ++++++
>>   arch/arm/lib/board.c | 12 ++++++++++++
>>   2 files changed, 18 insertions(+)
>>
>> diff --git a/README b/README
>> index 037513a..2077c3b 100644
>> --- a/README
>> +++ b/README
>> @@ -550,6 +550,12 @@ The following options need to be configured:
>>   		in a single configuration file and the machine type is
>>   		runtime discoverable, do not have to use this setting.
>>
>> +		CONFIG_BOOT_PARAMS_P		[relevant for ARM only]
>> +
>> +		This config option can provide a way to initialize
>> +		bi_boot_params from the u-boot infrastructure itself. The
>> +		board still has the option to override it in board_init routine
>> +
>>   - vxWorks boot parameters:
>>
>>   		bootvx constructs a valid bootline using the following
>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
>> index 92cad9a..fa161b8 100644
>> --- a/arch/arm/lib/board.c
>> +++ b/arch/arm/lib/board.c
>> @@ -399,6 +399,11 @@ void board_init_f(ulong bootflag)
>>   	gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
>>   #endif
>>
>> +#ifdef CONFIG_BOOT_PARAMS_P
>> +	/* Boot params passed to Linux */
>> +	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
>> +#endif
>
> Again an ugly #ifdef. Why not something like this instead:
>
> Define a default earlier in the code (is 0x100 the best default?):
>
> #ifndef CONFIG_BOOT_PARAMS_P
> #define CONFIG_BOOT_PARAMS_P	0x100
> #endif
>
> then here just:
>
> 	/* Boot params passed to Linux */
> 	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
>

This would mean that I am forcing the boot params at 0x100 for all
boards. Is that the right thing to do

Off-course, all of them might already be initializing
gd->bd->bi_boot_params on their own

> without the #ifdef.
>
>> +
>>   	addr_sp -= sizeof (gd_t);
>>   	id = (gd_t *) addr_sp;
>>   	debug("Reserving %zu Bytes for Global Data at: %08lx\n",
>> @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
>>   static char *failed = "*** failed ***\n";
>>   #endif
>>
>> +static int __def_board_init(bd_t *bis)
>> +{
>> +	return -1;
>
> Is -1 a good value to return as default board_init()?
>

The return value is not checked as of today

>> +}
>> +
>> +int board_init(void) __attribute__((weak, alias("__def_board_init")));
>> +
>
> Use __weak from inlcude/linux/compiler.h instead:
>
> int __weak board_init(bd_t *bis)
> ...

OK, I would do that in v2

Vipin

>
>
> Thanks,
> Stefan
> .
>
Stefan Roese - Dec. 6, 2012, 11:26 a.m.
On 12/06/2012 10:56 AM, Vipin Kumar wrote:

<snip>

>>> +#ifdef CONFIG_BOOT_PARAMS_P
>>> +	/* Boot params passed to Linux */
>>> +	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
>>> +#endif
>>
>> Again an ugly #ifdef. Why not something like this instead:
>>
>> Define a default earlier in the code (is 0x100 the best default?):
>>
>> #ifndef CONFIG_BOOT_PARAMS_P
>> #define CONFIG_BOOT_PARAMS_P	0x100
>> #endif
>>
>> then here just:
>>
>> 	/* Boot params passed to Linux */
>> 	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
>>
>
> This would mean that I am forcing the boot params at 0x100 for all
> boards. Is that the right thing to do
>
> Off-course, all of them might already be initializing
> gd->bd->bi_boot_params on their own

Yes. Before or after this initialization? Will the board_init code
overwrite this one here?

But I also think this could be confusing. The generic way to implement
this would be to remove *all* local board specific assignments of
gd->bd->bi_boot_params and define CONFIG_BOOT_PARAMS_P in their board
config header. But this will be quite intrusive and could be error-prone.

>> without the #ifdef.
>>
>>> +
>>>   	addr_sp -= sizeof (gd_t);
>>>   	id = (gd_t *) addr_sp;
>>>   	debug("Reserving %zu Bytes for Global Data at: %08lx\n",
>>> @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
>>>   static char *failed = "*** failed ***\n";
>>>   #endif
>>>
>>> +static int __def_board_init(bd_t *bis)
>>> +{
>>> +	return -1;
>>
>> Is -1 a good value to return as default board_init()?
>>
>
> The return value is not checked as of today

Then I suggest to use 0.

Thanks,
Stefan
Vipin Kumar - Dec. 7, 2012, 9:40 a.m.
On 12/6/2012 4:56 PM, Stefan Roese wrote:
> On 12/06/2012 10:56 AM, Vipin Kumar wrote:
>
> <snip>
>
>>>> +#ifdef CONFIG_BOOT_PARAMS_P
>>>> +	/* Boot params passed to Linux */
>>>> +	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
>>>> +#endif
>>>
>>> Again an ugly #ifdef. Why not something like this instead:
>>>
>>> Define a default earlier in the code (is 0x100 the best default?):
>>>
>>> #ifndef CONFIG_BOOT_PARAMS_P
>>> #define CONFIG_BOOT_PARAMS_P	0x100
>>> #endif
>>>
>>> then here just:
>>>
>>> 	/* Boot params passed to Linux */
>>> 	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
>>>
>>
>> This would mean that I am forcing the boot params at 0x100 for all
>> boards. Is that the right thing to do
>>
>> Off-course, all of them might already be initializing
>> gd->bd->bi_boot_params on their own
>
> Yes. Before or after this initialization? Will the board_init code
> overwrite this one here?
>

board_init would overwrite it as it is called after relocation from 
board_init_r while this routine is called even before relocation

> But I also think this could be confusing. The generic way to implement
> this would be to remove *all* local board specific assignments of
> gd->bd->bi_boot_params and define CONFIG_BOOT_PARAMS_P in their board
> config header. But this will be quite intrusive and could be error-prone.
>

Yes, that is what I also thought..
Originally, I based my patch on the CONFIG_MACH_TYPE handling. If it is 
defined the board need not initialize gd->bd->bi_arch_number

Can you please suggest what is the best way here. Wolfgang, Albert ?

>>> without the #ifdef.
>>>
>>>> +
>>>>    	addr_sp -= sizeof (gd_t);
>>>>    	id = (gd_t *) addr_sp;
>>>>    	debug("Reserving %zu Bytes for Global Data at: %08lx\n",
>>>> @@ -468,6 +473,13 @@ void board_init_f(ulong bootflag)
>>>>    static char *failed = "*** failed ***\n";
>>>>    #endif
>>>>
>>>> +static int __def_board_init(bd_t *bis)
>>>> +{
>>>> +	return -1;
>>>
>>> Is -1 a good value to return as default board_init()?
>>>
>>
>> The return value is not checked as of today
>
> Then I suggest to use 0.
>

OK. Accepted

> Thanks,
> Stefan
>
> .
>
Wolfgang Denk - Dec. 7, 2012, 2:47 p.m.
Dear Vipin Kumar,

In message <50C1B9A6.1020106@st.com> you wrote:
>
> Can you please suggest what is the best way here. Wolfgang, Albert ?

I don't like this patch at all.   It introduces yet more
architecture specific stuff to lib/board.c, while we actually should
be working on coming up with a common version for all architectures.

If you want to simplify code, then please not by making it worse in
other places.

Best regards,

Wolfgang Denk
Vipin Kumar - Dec. 17, 2012, 8:15 a.m.
On 12/7/2012 8:17 PM, Wolfgang Denk wrote:
> Dear Vipin Kumar,
>
> In message<50C1B9A6.1020106@st.com>  you wrote:
>>
>> Can you please suggest what is the best way here. Wolfgang, Albert ?
>
> I don't like this patch at all.   It introduces yet more
> architecture specific stuff to lib/board.c, while we actually should
> be working on coming up with a common version for all architectures.
>
> If you want to simplify code, then please not by making it worse in
> other places.
>

I agree, but this code adds an option to pass boot params pointer to 
board descriptor.
Just two lines above the intended change, there is a mach_type 
initialization based on the definition of CONFIG_MACH_TYPE. Moreover, 
the lib/board.c exists in arch/arm as of today which means that it can 
contain all the stuff common for arm boards

If I am right, the patch should also be OK

Vipin

> Best regards,
>
> Wolfgang Denk
>

Patch

diff --git a/README b/README
index 037513a..2077c3b 100644
--- a/README
+++ b/README
@@ -550,6 +550,12 @@  The following options need to be configured:
 		in a single configuration file and the machine type is
 		runtime discoverable, do not have to use this setting.
 
+		CONFIG_BOOT_PARAMS_P		[relevant for ARM only]
+
+		This config option can provide a way to initialize
+		bi_boot_params from the u-boot infrastructure itself. The
+		board still has the option to override it in board_init routine
+
 - vxWorks boot parameters:
 
 		bootvx constructs a valid bootline using the following
diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 92cad9a..fa161b8 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -399,6 +399,11 @@  void board_init_f(ulong bootflag)
 	gd->bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */
 #endif
 
+#ifdef CONFIG_BOOT_PARAMS_P
+	/* Boot params passed to Linux */
+	gd->bd->bi_boot_params = CONFIG_BOOT_PARAMS_P;
+#endif
+
 	addr_sp -= sizeof (gd_t);
 	id = (gd_t *) addr_sp;
 	debug("Reserving %zu Bytes for Global Data at: %08lx\n",
@@ -468,6 +473,13 @@  void board_init_f(ulong bootflag)
 static char *failed = "*** failed ***\n";
 #endif
 
+static int __def_board_init(bd_t *bis)
+{
+	return -1;
+}
+
+int board_init(void) __attribute__((weak, alias("__def_board_init")));
+
 /*
  ************************************************************************
  *