Patchwork [U-Boot,v5,12/23] Add spl load feature

login
register
mail settings
Submitter Simon Glass
Date Feb. 8, 2013, 3:12 p.m.
Message ID <1360336339-10703-13-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/219179/
State Deferred, archived
Delegated to: Tom Rini
Headers show

Comments

Simon Glass - Feb. 8, 2013, 3:12 p.m.
This adds secondary program loader support to the generic board.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 common/board_f.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
Scott Wood - Feb. 12, 2013, 8:29 p.m.
On 02/08/2013 09:12:08 AM, Simon Glass wrote:
> This adds secondary program loader support to the generic board.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  common/board_f.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/common/board_f.c b/common/board_f.c
> index aa10f4b..3a8036f 100644
> --- a/common/board_f.c
> +++ b/common/board_f.c
> @@ -309,6 +309,7 @@ static int reserve_uboot(void)
>  	return 0;
>  }
> 
> +#ifndef CONFIG_SPL_BUILD
>  /* reserve memory for malloc() area */
>  static int reserve_malloc(void)
>  {
> @@ -328,6 +329,7 @@ static int reserve_board(void)
>  			sizeof(bd_t), gd->dest_addr_sp);
>  	return 0;
>  }
> +#endif
> 
>  static int setup_machine(void)
>  {
> @@ -365,6 +367,7 @@ static int reserve_fdt(void)
>  	return 0;
>  }
> 
> +#ifndef CONFIG_SPL_BUILD
>  static int reserve_stacks(void)
>  {
>  	/* setup stack pointer for exceptions */
> @@ -384,6 +387,17 @@ static int reserve_stacks(void)
> 
>  	return 0;
>  }
> +#endif
> +
> +#ifdef CONFIG_SPL_BUILD
> +static int reserve_stacks_spl(void)
> +{
> +	/* Why not -= ? */
> +	gd->dest_addr_sp += 128;	/* leave 32 words for  
> abort-stack */
> +	gd->irq_sp = gd->dest_addr_sp;
> +	return 0;
> +}
> +#endif

"abort-stack" doesn't sound very generic, and that "why not" question  
should probably be answered.

>  static int display_new_sp(void)
>  {
> @@ -524,12 +538,18 @@ static init_fnc_t init_sequence_f[] = {
>  	reserve_lcd,
>  #endif
>  	reserve_uboot,
> +#ifndef CONFIG_SPL_BUILD
>  	reserve_malloc,
>  	reserve_board,
> +#endif
>  	setup_machine,
>  	reserve_global_data,
>  	reserve_fdt,
> +#ifdef CONFIG_SPL_BUILD
> +	reserve_stacks_spl,
> +#else
>  	reserve_stacks,
> +#endif

Why not just put the ifdef inside reserve_stacks()?  Or at least give  
them the same name?

-Scott
Simon Glass - Feb. 12, 2013, 10:23 p.m.
+Albert

Hi Scott,

On Tue, Feb 12, 2013 at 12:29 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 02/08/2013 09:12:08 AM, Simon Glass wrote:
>>
>> This adds secondary program loader support to the generic board.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> Changes in v5: None
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>>  common/board_f.c | 20 ++++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/common/board_f.c b/common/board_f.c
>> index aa10f4b..3a8036f 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -309,6 +309,7 @@ static int reserve_uboot(void)
>>         return 0;
>>  }
>>
>> +#ifndef CONFIG_SPL_BUILD
>>  /* reserve memory for malloc() area */
>>  static int reserve_malloc(void)
>>  {
>> @@ -328,6 +329,7 @@ static int reserve_board(void)
>>                         sizeof(bd_t), gd->dest_addr_sp);
>>         return 0;
>>  }
>> +#endif
>>
>>  static int setup_machine(void)
>>  {
>> @@ -365,6 +367,7 @@ static int reserve_fdt(void)
>>         return 0;
>>  }
>>
>> +#ifndef CONFIG_SPL_BUILD
>>  static int reserve_stacks(void)
>>  {
>>         /* setup stack pointer for exceptions */
>> @@ -384,6 +387,17 @@ static int reserve_stacks(void)
>>
>>         return 0;
>>  }
>> +#endif
>> +
>> +#ifdef CONFIG_SPL_BUILD
>> +static int reserve_stacks_spl(void)
>> +{
>> +       /* Why not -= ? */
>> +       gd->dest_addr_sp += 128;        /* leave 32 words for abort-stack
>> */
>> +       gd->irq_sp = gd->dest_addr_sp;
>> +       return 0;
>> +}
>> +#endif
>
>
> "abort-stack" doesn't sound very generic, and that "why not" question should
> probably be answered.

I'm not sure what you mean by the first comment. For the second, I
agree. It looks wrong to me. I copied Albert who might know.

>
>
>>  static int display_new_sp(void)
>>  {
>> @@ -524,12 +538,18 @@ static init_fnc_t init_sequence_f[] = {
>>         reserve_lcd,
>>  #endif
>>         reserve_uboot,
>> +#ifndef CONFIG_SPL_BUILD
>>         reserve_malloc,
>>         reserve_board,
>> +#endif
>>         setup_machine,
>>         reserve_global_data,
>>         reserve_fdt,
>> +#ifdef CONFIG_SPL_BUILD
>> +       reserve_stacks_spl,
>> +#else
>>         reserve_stacks,
>> +#endif
>
>
> Why not just put the ifdef inside reserve_stacks()?  Or at least give them
> the same name?

OK I will do that.

>
> -Scott

Regards,
Simon
Simon Glass - March 2, 2013, 11:39 p.m.
Hi Scott,

On Tue, Feb 12, 2013 at 2:34 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 02/12/2013 04:23:21 PM, Simon Glass wrote:
>>
>> +Albert
>>
>> Hi Scott,
>>
>> On Tue, Feb 12, 2013 at 12:29 PM, Scott Wood <scottwood@freescale.com>
>> wrote:
>> > On 02/08/2013 09:12:08 AM, Simon Glass wrote:
>> >>
>> >> This adds secondary program loader support to the generic board.
>> >>
>> >> Signed-off-by: Simon Glass <sjg@chromium.org>
>> >> ---
>> >> Changes in v5: None
>> >> Changes in v4: None
>> >> Changes in v3: None
>> >> Changes in v2: None
>> >>
>> >>  common/board_f.c | 20 ++++++++++++++++++++
>> >>  1 file changed, 20 insertions(+)
>> >>
>> >> diff --git a/common/board_f.c b/common/board_f.c
>> >> index aa10f4b..3a8036f 100644
>> >> --- a/common/board_f.c
>> >> +++ b/common/board_f.c
>> >> @@ -309,6 +309,7 @@ static int reserve_uboot(void)
>> >>         return 0;
>> >>  }
>> >>
>> >> +#ifndef CONFIG_SPL_BUILD
>> >>  /* reserve memory for malloc() area */
>> >>  static int reserve_malloc(void)
>> >>  {
>> >> @@ -328,6 +329,7 @@ static int reserve_board(void)
>> >>                         sizeof(bd_t), gd->dest_addr_sp);
>> >>         return 0;
>> >>  }
>> >> +#endif
>> >>
>> >>  static int setup_machine(void)
>> >>  {
>> >> @@ -365,6 +367,7 @@ static int reserve_fdt(void)
>> >>         return 0;
>> >>  }
>> >>
>> >> +#ifndef CONFIG_SPL_BUILD
>> >>  static int reserve_stacks(void)
>> >>  {
>> >>         /* setup stack pointer for exceptions */
>> >> @@ -384,6 +387,17 @@ static int reserve_stacks(void)
>> >>
>> >>         return 0;
>> >>  }
>> >> +#endif
>> >> +
>> >> +#ifdef CONFIG_SPL_BUILD
>> >> +static int reserve_stacks_spl(void)
>> >> +{
>> >> +       /* Why not -= ? */
>> >> +       gd->dest_addr_sp += 128;        /* leave 32 words for
>> >> abort-stack
>> >> */
>> >> +       gd->irq_sp = gd->dest_addr_sp;
>> >> +       return 0;
>> >> +}
>> >> +#endif
>> >
>> >
>> > "abort-stack" doesn't sound very generic, and that "why not" question
>> > should
>> > probably be answered.
>>
>> I'm not sure what you mean by the first comment.
>
>
> It's ARM-specific terminology ("exception" is more typical than "abort"
> elsewhere).  Why 32 words, BTW?  Why is it larger in SPL versus non-SPL?

These are good questions but I don't know the answers. I guess the
abort stack may need enough space to panic()?

I am copying code from the various arch/xxx/board.c files in order to
merge them.

Regards,
Simon


>
> -Scott

Patch

diff --git a/common/board_f.c b/common/board_f.c
index aa10f4b..3a8036f 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -309,6 +309,7 @@  static int reserve_uboot(void)
 	return 0;
 }
 
+#ifndef CONFIG_SPL_BUILD
 /* reserve memory for malloc() area */
 static int reserve_malloc(void)
 {
@@ -328,6 +329,7 @@  static int reserve_board(void)
 			sizeof(bd_t), gd->dest_addr_sp);
 	return 0;
 }
+#endif
 
 static int setup_machine(void)
 {
@@ -365,6 +367,7 @@  static int reserve_fdt(void)
 	return 0;
 }
 
+#ifndef CONFIG_SPL_BUILD
 static int reserve_stacks(void)
 {
 	/* setup stack pointer for exceptions */
@@ -384,6 +387,17 @@  static int reserve_stacks(void)
 
 	return 0;
 }
+#endif
+
+#ifdef CONFIG_SPL_BUILD
+static int reserve_stacks_spl(void)
+{
+	/* Why not -= ? */
+	gd->dest_addr_sp += 128;	/* leave 32 words for abort-stack */
+	gd->irq_sp = gd->dest_addr_sp;
+	return 0;
+}
+#endif
 
 static int display_new_sp(void)
 {
@@ -524,12 +538,18 @@  static init_fnc_t init_sequence_f[] = {
 	reserve_lcd,
 #endif
 	reserve_uboot,
+#ifndef CONFIG_SPL_BUILD
 	reserve_malloc,
 	reserve_board,
+#endif
 	setup_machine,
 	reserve_global_data,
 	reserve_fdt,
+#ifdef CONFIG_SPL_BUILD
+	reserve_stacks_spl,
+#else
 	reserve_stacks,
+#endif
 	setup_baud_rate,
 	display_new_sp,
 	reloc_fdt,