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

[Daniel Schwierzeck]

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/

-- 
- Daniel