| Message ID | 1416091618-18700-2-git-send-email-daniel.schwierzeck@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Daniel Schwierzeck |
| Headers | show |
Hi Daniel, On 15 November 2014 22:46, Daniel Schwierzeck <daniel.schwierzeck@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@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@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; Regards, Simon
Hi Simon, On 17.11.2014 07:24, Simon Glass wrote: > Hi Daniel, > > On 15 November 2014 22:46, Daniel Schwierzeck > <daniel.schwierzeck@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@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@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.
Hi Daniel, On 19 November 2014 16:59, Daniel Schwierzeck <daniel.schwierzeck@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@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@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@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? 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. Regards, Simon
On 19.11.2014 23:22, Simon Glass wrote: > Hi Daniel, > > On 19 November 2014 16:59, Daniel Schwierzeck > <daniel.schwierzeck@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@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@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@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.
Hi Daniel, On 20 November 2014 16:54, Daniel Schwierzeck <daniel.schwierzeck@gmail.com> wrote: > > > On 19.11.2014 23:22, Simon Glass wrote: >> Hi Daniel, >> >> On 19 November 2014 16:59, Daniel Schwierzeck >> <daniel.schwierzeck@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@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@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@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. Regards, Simon
Hi Simon, On 20.11.2014 18:22, Simon Glass wrote: > Hi Daniel, > > On 20 November 2014 16:54, Daniel Schwierzeck > <daniel.schwierzeck@gmail.com> wrote: >> >> >> On 19.11.2014 23:22, Simon Glass wrote: >>> Hi Daniel, >>> >>> On 19 November 2014 16:59, Daniel Schwierzeck >>> <daniel.schwierzeck@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@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@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@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/
Hi Daniel, On 21 November 2014 21:46, Daniel Schwierzeck <daniel.schwierzeck@gmail.com> wrote: > Hi Simon, > > On 20.11.2014 18:22, Simon Glass wrote: >> Hi Daniel, >> >> On 20 November 2014 16:54, Daniel Schwierzeck >> <daniel.schwierzeck@gmail.com> wrote: >>> >>> >>> On 19.11.2014 23:22, Simon Glass wrote: >>>> Hi Daniel, >>>> >>>> On 19 November 2014 16:59, Daniel Schwierzeck >>>> <daniel.schwierzeck@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@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@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@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/ Excellent, thanks for digging into this. Regards, Simon
On Sat, Nov 15, 2014 at 11:46:52PM +0100, Daniel Schwierzeck wrote: > The MIPS specific setup of the initial stack frame was not > ported to generic board_f. > > Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> Applied to u-boot/master, thanks!
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@gmail.com> wrote: > >> > >> > >> On 19.11.2014 23:22, Simon Glass wrote: > >>> Hi Daniel, > >>> > >>> On 19 November 2014 16:59, Daniel Schwierzeck > >>> <daniel.schwierzeck@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@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@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@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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 24.11.2014 23:20, Tom Rini wrote: >>>> >>>> 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. > hm, the patch landed in v2015.01-rc2. Do you want to create a revert patch or shall I include one in my next pull request? - -- - - Daniel -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJUdITqAAoJEPf1RifdDuszTuIIAJQzh2/EU16Hn+l/JC+TvJ3h 802FJMuKWeZRoF6NPrvZQ8x2SUloc2PfEOyQnMgUtChKOjzuba64Gk08ymzLQQ2Z r7Iv4W3so/9jv41PLeuFpVCAzypRuOi01cXGOxJYLgj1U9QRxAmGXxyIM/v4IFP+ 13ykcJLzxsp4qM12dEyBMQ+8uxysO3yjOwqEdk/q8agqttgnNd1ZjQ1Qzifs9udd ITuxfuii4iwzM+XWg06Q01kC60hEHis4P/HHgHile/Wd0yKrX1rGPYozpvZO+mIr r9w8Bnsdm/wgJOLX604MHMMjy6fkivrI7ZlCishEwMmOpnwElfnVwubjPFRQH0I= =r3h1 -----END PGP SIGNATURE-----
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 */ return 0;
The MIPS specific setup of the initial stack frame was not ported to generic board_f. Signed-off-by: Daniel Schwierzeck <daniel.schwierzeck@gmail.com> --- common/board_f.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)