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

[Simon Glass]

Hi Daniel,

On 21 November 2014 21:46, Daniel Schwierzeck
<daniel.schwierzeck at 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 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/

Excellent, thanks for digging into this.

Regards,
Simon