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

[Simon Glass]

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.

Regards,
Simon