[U-Boot] [PATCH 3/5] Allow arch-specific setting of global_data in board_init_f_mem()

[Simon Glass]

Hi Bin,

On 6 August 2015 at 01:15, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Simon,
>
> On Mon, Aug 3, 2015 at 8:10 AM, Simon Glass <sjg at chromium.org> wrote:
>> At present we have a simple assignment to gd. With some archs this is
>> implemented as a register or through some other means; a simple assignment
>> does not suit in all cases.
>>
>> Change this to a function and add documentation to describe how this all
>> works.
>>
>> Signed-off-by: Simon Glass <sjg at chromium.org>
>> ---
>>
>>  common/board_f.c | 14 +++++++++++---
>>  include/common.h | 40 ++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 51 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/board_f.c b/common/board_f.c
>> index a947770..3d1f305 100644
>> --- a/common/board_f.c
>> +++ b/common/board_f.c
>> @@ -1024,15 +1024,23 @@ void board_init_f_r(void)
>>  #endif /* CONFIG_X86 */
>>
>>  #ifndef CONFIG_X86
>> -ulong board_init_f_mem(ulong top)
>> +__weak void arch_setup_gdt(struct global_data *gd_ptr)
>>  {
>> +       gd = gd_ptr;
>> +}
>> +
>> +ulong board_init_f_mem(ulong top, ulong reserve_size)
>
> What is reserve_size? I don't see it is used by this function. If we
> change the signature, we need also update arc and ppc4xx start.S
> otherwise they will break.

Actually that should have been reverted. I decided to introduce it
later if needed, but found a way around it for x86.

>
>> +{
>> +       struct global_data *gd_ptr;
>> +
>>         /* Leave space for the stack we are running with now */
>>         top -= 0x40;
>>
>>         top -= sizeof(struct global_data);
>>         top = ALIGN(top, 16);
>> -       gd = (struct global_data *)top;
>> -       memset((void *)gd, '\0', sizeof(*gd));
>> +       gd_ptr = (struct global_data *)top;
>> +       memset(gd_ptr, '\0', sizeof(*gd));
>> +       arch_setup_gdt(gd_ptr);
>
> gdt? Should be just arch_setup_gd()?

Yes

>
>>
>>  #ifdef CONFIG_SYS_MALLOC_F_LEN
>>         top -= CONFIG_SYS_MALLOC_F_LEN;
>> diff --git a/include/common.h b/include/common.h
>> index fcc9ae7..029eb1f 100644
>> --- a/include/common.h
>> +++ b/include/common.h
>> @@ -217,6 +217,46 @@ extern char console_buffer[];
>>  /* arch/$(ARCH)/lib/board.c */
>>  void board_init_f(ulong);
>>  void board_init_r(gd_t *, ulong) __attribute__ ((noreturn));
>> +
>> +/**
>> + * board_init_f_mem() - Allocate global data and set stack position
>> + *
>> + * This function is called by each architecture very early in the start-up
>> + * code to set up the environment for board_init_f(). It allocates space for
>> + * global_data (see include/asm-generic/global_data.h) and places the stack
>> + * below this.
>> + *
>> + * This function requires a stack[1] Normally this is at @top. The function
>> + * starts allocating space from 64 bytes below @top. First it creates space
>> + * for global_data. then it calls arch_setup_gd() which sets gd to point to
>
> Nits: . -> ,
>
>> + * the global_data space and can reserve additional bytes of space if
>> + * required). Finally it allocates early malloc() memory
>> + * (CONFIG_SYS_MALLOC_F_LEN). The new top of the stack is just below this,
>> + * and it returned by this function.
>> + *
>> + * [1] Strictly speaking it would be possible to implement this function
>> + * in C on many archs such that it does not require a stack. However this
>> + * does not seem hugely important as only 64 byte are wasted.
>
> Where is the 64 bytes it uses? I only see a variable gd_ptr on the stack.

It's the subtraction at the top of board_init_f_mem().

>
>> + *
>> + * @top:       Top of available memory, also normally the top of the stack
>> + * @return:    New stack location
>> + */
>> +ulong board_init_f_mem(ulong top, ulong reserve_size);
>
> I don't like the name board_init_f_mem(). It actually does two things:
> creates global data pointer and reserve the early malloc() memory
> size. Also it looks like it's more like an arch thing instead of a
> board's. How about setup_gd_f_mem()?

The name is the same as before. It's intended to imply that it gets
the memory ready for board_init_f(). It may expand in future to do
additional setup (e.g. to allocate some other type of space). If we
change the name we would perhaps have to change it again later. At
least this name describes its purpose.

>
>> +
>> +/**
>> + * arch_setup_gdt() - Set up the global_data pointer
>> + *
>> + * This pointer is special in some architectures and cannot easily be assigned
>> + * to. For example on x86 it is implemented by adding a specific record to its
>> + * Global Descriptor Table! So we we provide a function to carry out this task.
>> + * For most architectures this can simply be:
>> + *
>> + *    gd = gd_ptr;
>> + *
>> + * @gd_ptr:    Pointer to global data
>> + */
>> +void arch_setup_gdt(gd_t *gd_ptr);
>> +
>>  int checkboard(void);
>>  int show_board_info(void);
>>  int checkflash(void);
>> --
>
> Regards,
> Bin

Regards,
Simon