[U-Boot] [PATCH v5 16/23] Adjust board_f.c for ppc

[Scott Wood]

On 02/12/2013 04:41:15 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Tue, Feb 12, 2013 at 2:32 PM, Scott Wood <scottwood at freescale.com>  
> wrote:
> > On 02/08/2013 09:12:12 AM, Simon Glass wrote:
> >>
> >>  #ifndef CONFIG_SPL_BUILD
> >>  static int reserve_stacks(void)
> >>  {
> >> +#ifdef CONFIG_PPC
> >> +       ulong *s;
> >> +#endif
> >> +
> >>         /* setup stack pointer for exceptions */
> >>         gd->dest_addr_sp -= 16;
> >>         gd->dest_addr_sp &= ~0xf;
> >> @@ -398,6 +532,14 @@ static int reserve_stacks(void)
> >>         /* leave 3 words for abort-stack, plus 1 for alignment */
> >>         gd->dest_addr_sp -= 16;
> >>
> >> +#ifdef CONFIG_PPC
> >> +       /* Clear initial stack frame */
> >> +       s = (ulong *) gd->dest_addr_sp;
> >> +       *s = 0; /* Terminate back chain */
> >> +       *++s = 0; /* NULL return address */
> >> +       gd->dest_addr_sp = (ulong) s;
> >> +#endif
> >> +
> >
> >
> > PPC ABI requires 16-byte stack alignment, which would be broken by  
> the
> > CONFIG_USE_IRQ section (which even still has an "ARM ABI" comment).
> >
> > I think this entire function should be kept in arch code.  Stack  
> layout is
> > inherently architecture/ABI specific.  Some architectures even have  
> a stack
> > that grows upward (not sure if any such are supported by U-Boot).
> 
> Thanks for reviewing all this.
> 
> While I am working to avoid it, one option is to create a weak
> function which archs can override. The reason I am keen to avoid it,
> at least for a first implementation, is that it obscures the
> similarities.

That's fine for most of the file, but I think there's not much that's  
truly generic when it comes to setting up the stack.  It's not as if  
this is a huge function (at least before it grows a bunch of ifdefs).

> In this case we could just just force 16-byte alignment,
> and make the PPC code unconditional. It shouldn't hurt anything.

The CONFIG_USE_IRQ section also has references to FIQs... if it's meant  
to be an ARM-specific CONFIG, perhaps it should be renamed (and  
definitely it should be documented).

-Scott