[U-Boot] [PATCH 1/7] common/board_f: add setup of initial stack frame for MIPS

[Tom Rini]

On Fri, Nov 21, 2014 at 09:46:09PM +0100, Daniel Schwierzeck wrote:
> Hi Simon,
> 
> On 20.11.2014 18:22, Simon Glass wrote:
> > Hi Daniel,
> > 
> > On 20 November 2014 16:54, Daniel Schwierzeck
> > <daniel.schwierzeck at gmail.com> wrote:
> >>
> >>
> >> On 19.11.2014 23:22, Simon Glass wrote:
> >>> Hi Daniel,
> >>>
> >>> On 19 November 2014 16:59, Daniel Schwierzeck
> >>> <daniel.schwierzeck at gmail.com> wrote:
> >>>> Hi Simon,
> >>>>
> >>>> On 17.11.2014 07:24, Simon Glass wrote:
> >>>>> Hi Daniel,
> >>>>>
> >>>>> On 15 November 2014 22:46, Daniel Schwierzeck
> >>>>> <daniel.schwierzeck at gmail.com> wrote:
> >>>>>> The MIPS specific setup of the initial stack frame was not
> >>>>>> ported to generic board_f.
> >>>>>>
> >>>>>> Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>>  common/board_f.c | 8 +++++++-
> >>>>>>  1 file changed, 7 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/common/board_f.c b/common/board_f.c
> >>>>>> index b5bebc9..57e8a67 100644
> >>>>>> --- a/common/board_f.c
> >>>>>> +++ b/common/board_f.c
> >>>>>> @@ -579,7 +579,7 @@ static int reserve_stacks(void)
> >>>>>>         gd->irq_sp = gd->start_addr_sp;
> >>>>>>  # endif
> >>>>>>  #else
> >>>>>> -# ifdef CONFIG_PPC
> >>>>>> +# if defined(CONFIG_PPC) || defined(CONFIG_MIPS)
> >>>>>>         ulong *s;
> >>>>>>  # endif
> >>>>>>
> >>>>>> @@ -609,6 +609,12 @@ static int reserve_stacks(void)
> >>>>>>         s = (ulong *) gd->start_addr_sp;
> >>>>>>         *s = 0; /* Terminate back chain */
> >>>>>>         *++s = 0; /* NULL return address */
> >>>>>> +# elif defined(CONFIG_MIPS)
> >>>>>> +       /* Clear initial stack frame */
> >>>>>> +       s = (ulong *) gd->start_addr_sp;
> >>>>>> +       *s-- = 0;
> >>>>>> +       *s-- = 0;
> >>>>>> +       gd->start_addr_sp = (ulong) s;
> >>>>>>  # endif /* Architecture specific code */
> >>>>>
> >>>>> Great to see this happening.
> >>>>>
> >>>>> There is a comment in the code here:
> >>>>>
> >>>>> /*
> >>>>> * Handle architecture-specific things here
> >>>>> * TODO(sjg at chromium.org): Perhaps create arch_reserve_stack()
> >>>>> * to handle this and put in arch/xxx/lib/stack.c
> >>>>> */
> >>>>>
> >>>>> Perhaps we should do this. You could create a weak function which is
> >>>>> called for all archs, and implement it just for MIPS at present. I'm
> >>>>> not sure about a good prototype. Perhaps pass it gd and comment that
> >>>>> it is allowed to change memory to set up the stack, and adjust
> >>>>> gd->start_addr_sp and other stack-related values.
> >>>>>
> >>>>> Also while I see that PPC writes above the stack pointer, I'm not sure
> >>>>> why it is valid. Should you in fact use:
> >>>>>
> >>>>> *--s = 0;
> >>>>> *--s = 0;
> >>>>
> >>>> I'd like to have those patches merged for 2015.01. So I want to keep the
> >>>> current code to not break anything. Maybe this is not necessary at all.
> >>>> The MIPS Malta board already uses generic board and does not seem to
> >>>> have any problems.
> >>>
> >>> I don't see a problem with merging this for 2015.01. Are you saying
> >>> you don't think it is needed but can't be sure? So you want to merge
> >>> it and see what people report?
> >>
> >> that code is taken unmodified from arch/mips/lib/board.c to not change
> >> or break anything. But that code is old and maybe copied from PowerPC in
> >> the early phases of U-Boot. I'm only saying that I need to investigate
> >> if that code could be dropped or not. But that is a task for the next
> >> merge window.
> >>
> >>>
> >>> In that case I think you should add a comment to that effect, but also
> >>> do the function as I mentioned above. We are trying to remove the
> >>> arch-specific code in this file and certainly don't want to add more.
> >>>
> >>
> >> ok I'll send an updated patch.
> > 
> > Thanks - and as I mentioned it seems wrong to write to a word above
> > the top of the stack.
> > 
> 
> I discard this patch. The only requirement for the stack pointer on MIPS
> is alignment on a 8 Byte boundary. reserve_stacks already aligns it to
> 16 Byte (gd->start_addr_sp &= ~0xf;).
> 
> To make stack walking and backtraces working flawlessy in gdb, I found
> another solution [1].
> 
> [1] http://patchwork.ozlabs.org/patch/413182/

And I of course missed this email while sorting out other problems with
stuff I was applying.  I'll just back this out and take the better fix.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141124/ee1fa20c/attachment.pgp>