Patchwork [U-Boot,01/10] ARM: move interrupt_init to before relocation

login
register
mail settings
Submitter Rob Herring
Date May 15, 2013, 7:56 p.m.
Message ID <1368647776-12940-1-git-send-email-robherring2@gmail.com>
Download mbox | patch
Permalink /patch/244169/
State Superseded
Delegated to: Albert ARIBAUD
Headers show

Comments

Rob Herring - May 15, 2013, 7:56 p.m.
From: Rob Herring <rob.herring@calxeda.com>

interrupt_init also sets up the abort stack, but is not setup before
relocation. So any aborts during relocation will hang and not print out
any useful information. Fix this by moving the interrupt_init to after
the stack setup in board_init_f.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
 arch/arm/lib/board.c | 1 +
 1 file changed, 1 insertion(+)
Albert ARIBAUD - May 15, 2013, 8:26 p.m.
Hi Rob,

On Wed, 15 May 2013 14:56:07 -0500, Rob Herring <robherring2@gmail.com>
wrote:

> From: Rob Herring <rob.herring@calxeda.com>
> 
> interrupt_init also sets up the abort stack, but is not setup before
> relocation. So any aborts during relocation will hang and not print out
> any useful information. Fix this by moving the interrupt_init to after
> the stack setup in board_init_f.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
>  arch/arm/lib/board.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 09ab4ad..6dbe7e2 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -447,6 +447,7 @@ void board_init_f(ulong bootflag)
>  	addr_sp += 128;	/* leave 32 words for abort-stack   */
>  	gd->irq_sp = addr_sp;
>  #endif
> +	interrupt_init();
>  
>  	debug("New Stack Pointer is: %08lx\n", addr_sp);
>  

I fail to understand how this is even supposed to work through
relocation: exception vectors are not relocated, so if they work before
relocation, then they won't work any more afterward unless some code is
added to relocate them.

Also: if this patch is moving interrupt_init(), then where is the line
where a call to interrupt_init() is removed?

Amicalement,
Rob Herring - May 15, 2013, 9:29 p.m.
On Wed, May 15, 2013 at 3:26 PM, Albert ARIBAUD
<albert.u.boot@aribaud.net> wrote:
> Hi Rob,
>
> On Wed, 15 May 2013 14:56:07 -0500, Rob Herring <robherring2@gmail.com>
> wrote:
>
>> From: Rob Herring <rob.herring@calxeda.com>
>>
>> interrupt_init also sets up the abort stack, but is not setup before
>> relocation. So any aborts during relocation will hang and not print out
>> any useful information. Fix this by moving the interrupt_init to after
>> the stack setup in board_init_f.
>>
>> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
>> ---
>>  arch/arm/lib/board.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
>> index 09ab4ad..6dbe7e2 100644
>> --- a/arch/arm/lib/board.c
>> +++ b/arch/arm/lib/board.c
>> @@ -447,6 +447,7 @@ void board_init_f(ulong bootflag)
>>       addr_sp += 128; /* leave 32 words for abort-stack   */
>>       gd->irq_sp = addr_sp;
>>  #endif
>> +     interrupt_init();
>>
>>       debug("New Stack Pointer is: %08lx\n", addr_sp);
>>
>
> I fail to understand how this is even supposed to work through
> relocation: exception vectors are not relocated, so if they work before
> relocation, then they won't work any more afterward unless some code is
> added to relocate them.

They work before and after in my testing. The vectors are relocated
along with the rest of u-boot and the vector base is updated by
c_runtime_cpu_setup. I'm simply setting up the abort stack earlier. An
alternative would be to setup a different abort handler and stack
before relocation. I'm open to suggestions, but as it stands now
aborts before or during relocation will simply hang without this.

> Also: if this patch is moving interrupt_init(), then where is the line
> where a call to interrupt_init() is removed?

Yes, the later call to interrupt_init should be removed assuming we
keep this approach.

Rob
Albert ARIBAUD - May 16, 2013, 9:44 a.m.
Hi Rob,

On Wed, 15 May 2013 16:29:41 -0500, Rob Herring <robherring2@gmail.com>
wrote:

> On Wed, May 15, 2013 at 3:26 PM, Albert ARIBAUD
> <albert.u.boot@aribaud.net> wrote:
> > Hi Rob,
> >
> > On Wed, 15 May 2013 14:56:07 -0500, Rob Herring <robherring2@gmail.com>
> > wrote:
> >
> >> From: Rob Herring <rob.herring@calxeda.com>
> >>
> >> interrupt_init also sets up the abort stack, but is not setup before
> >> relocation. So any aborts during relocation will hang and not print out
> >> any useful information. Fix this by moving the interrupt_init to after
> >> the stack setup in board_init_f.
> >>
> >> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> >> ---
> >>  arch/arm/lib/board.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> >> index 09ab4ad..6dbe7e2 100644
> >> --- a/arch/arm/lib/board.c
> >> +++ b/arch/arm/lib/board.c
> >> @@ -447,6 +447,7 @@ void board_init_f(ulong bootflag)
> >>       addr_sp += 128; /* leave 32 words for abort-stack   */
> >>       gd->irq_sp = addr_sp;
> >>  #endif
> >> +     interrupt_init();
> >>
> >>       debug("New Stack Pointer is: %08lx\n", addr_sp);
> >>
> >
> > I fail to understand how this is even supposed to work through
> > relocation: exception vectors are not relocated, so if they work before
> > relocation, then they won't work any more afterward unless some code is
> > added to relocate them.
> 
> They work before and after in my testing. The vectors are relocated
> along with the rest of u-boot and the vector base is updated by
> c_runtime_cpu_setup. I'm simply setting up the abort stack earlier. An
> alternative would be to setup a different abort handler and stack
> before relocation. I'm open to suggestions, but as it stands now
> aborts before or during relocation will simply hang without this.

Thanks for the clarification.

This opens at least two quite interesting issues:

1. interrupts_init() does not initialize interrupts but stacks, and
   does not initialize only interrupt stacks but also exception stacks;
   it badly needs a rename (and so does the abort stack, BTW).

2. Much more worrying, interrupt vectors are only fixed for armv7, and
   only by setting VBAR. This seems to imply that for all other ARM
   targets, VBAR is not modified, and neither are the exception vectors
   at either 0x00000000 or 0xFFFF0000; and this means that exceptions
   are never pointed to the RAM-running U-Boot; they would work for NOR
   FLASH based U-Boots, but any other case I simply fail to see how it
   would work as intended.

But issue 2 is so big that I probably simply missed something obvious.
Anyone able to show me how dumb I am here, feel free. :)

Regarding your change, Rob, it's good news anyway as I'm not going to
ask you to overhaul the whole of ARM exception handling code in any
case :) and I am fine with the change as it is, except:

> > Also: if this patch is moving interrupt_init(), then where is the line
> > where a call to interrupt_init() is removed?
> 
> Yes, the later call to interrupt_init should be removed assuming we
> keep this approach.

Then I would like the removal to be paired with this addition in a
single commit.

> Rob

Amicalement,

Patch

diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
index 09ab4ad..6dbe7e2 100644
--- a/arch/arm/lib/board.c
+++ b/arch/arm/lib/board.c
@@ -447,6 +447,7 @@  void board_init_f(ulong bootflag)
 	addr_sp += 128;	/* leave 32 words for abort-stack   */
 	gd->irq_sp = addr_sp;
 #endif
+	interrupt_init();
 
 	debug("New Stack Pointer is: %08lx\n", addr_sp);