From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Schwierzeck Date: Fri, 21 Nov 2014 21:46:09 +0100 Subject: [U-Boot] [PATCH 1/7] common/board_f: add setup of initial stack frame for MIPS In-Reply-To: References: <1416091618-18700-1-git-send-email-daniel.schwierzeck@gmail.com> <1416091618-18700-2-git-send-email-daniel.schwierzeck@gmail.com> <546CCC74.4030606@gmail.com> <546E1CCB.7030208@gmail.com> Message-ID: <546FA491.4010801@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Simon, On 20.11.2014 18:22, Simon Glass wrote: > Hi Daniel, > > On 20 November 2014 16:54, Daniel Schwierzeck > wrote: >> >> >> On 19.11.2014 23:22, Simon Glass wrote: >>> Hi Daniel, >>> >>> On 19 November 2014 16:59, Daniel Schwierzeck >>> wrote: >>>> Hi Simon, >>>> >>>> On 17.11.2014 07:24, Simon Glass wrote: >>>>> Hi Daniel, >>>>> >>>>> On 15 November 2014 22:46, Daniel Schwierzeck >>>>> wrote: >>>>>> The MIPS specific setup of the initial stack frame was not >>>>>> ported to generic board_f. >>>>>> >>>>>> Signed-off-by: Daniel Schwierzeck >>>>>> >>>>>> --- >>>>>> >>>>>> 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/ -- - Daniel