[U-Boot] Where I'm going with x86 board.c

[Simon Glass]

Hi Albert,

On Thu, Dec 22, 2011 at 10:01 AM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> Hi Simon,
>
> Le 22/12/2011 16:49, Simon Glass a écrit :
>
>> Hi Graeme,
>>
>> On Thu, Dec 22, 2011 at 3:27 AM, Graeme Russ<graeme.russ at gmail.com>
>>  wrote:
>>>
>>> Hi Simon,
>>>
>>> On 22/12/11 17:44, Simon Glass wrote:
>>>>
>>>> Hi Graeme,
>>>>
>>>> On Tue, Dec 20, 2011 at 4:06 AM, Graeme Russ<graeme.russ at gmail.com>
>>>>  wrote:
>>>>>
>>>>> With Simon's work on generic relocation support, I thought I would
>>>>> throw in
>>>>> what I am planning for arch/x86/lib/board.c
>>>>>
>>>>> Now this is not a patch, it is a work-in-progress complete version of
>>>>> the
>>>>> file (compiles, will test soon) - If feedback is positive, I'll add
>>>>> this to
>>>>> an upcoming patch set
>>>>
>>>>
>>>> Looks good to me+++
>>>
>>>
>>> Thanks for having a look :)
>>>
>>>>> Notice the amount of wrapping around void functions - If all other
>>>>> arch's
>>>>> follow this lead, most of this wrapping can be removed by changing the
>>>>> function signatures.
>>>>>
>>>>> Lines 428 - 585 are effectively the generic init sequence - The rest is
>>>>> wrappers, init sequence arrays, or fluff that should be moved
>>>>>
>>>>> I noticed something along the way - gd is no longer special... let me
>>>>> explain...
>>>>>
>>>>> Some arch's use a dedicated register for the gd pointer - This allows
>>>>> the
>>>>> pointer to be written to prior to relocation. For x86, the gd pointer
>>>>> is
>>>>> simply passed around as a function parameter early - If the
>>>>> init_sequence_f
>>>>> functions accepted a gd pointer as a parameter, there would be no need
>>>>> for
>>>>> it to be global prior to relocation and therefore no need to allocate a
>>>>> permanent register for it - Of course do_init_loop() would no longer be
>>>>> generic for both pre and post relocation. This does mess with the
>>>>> stand-alone API, but as discussed before, stand alone applications
>>>>> should
>>>>> not be accessing gd anyway, so there should be no API to break ;)
>>>>
>>>>
>>>> Actually as it happens I did a bit of an experiment with this some
>>>> weeks ago and my original board stuff had gd as a parameter for the
>>>> pre-reloc functions (some with stubs to other ones like
>>>> console_init_f()). On ARM it really doesn't make a lot of sense to use
>>>> a global variable instead of a parameter. I decided that it was a bit
>>>> much to bite off in one go :-) Partly that was because IMO gd really
>>>> only makes sense prior to relocation - for the post-relocation init
>>>> calls they don't have a lot of need for gd.
>>>
>>>
>>> I don't think this is 100% correct - I'm sure gd is used all over the
>>> place
>>> post init
>>
>>
>> Not enough that it needs to be in its own register. This compile-time
>> decision affects the whole of U-Boot post-relocation. In some routines
>> there is a small code size / performance advantage to having r8
>> available.
>
>
> From measurements I did when working on ARM relocation, I remember having
> tried if reserving a(nother) global register would change code size, and
> IIRC, it did not have a big impact on code size/efficiency.

It does not have a big impact, although it is noticeable if you are
doing crypto (secure boot).

>
>
>> Also it is a bit of a misnomer when used after relocation. There is
>> lots of global data (the whole BSS) not just one structure. Arguably
>> some things like the network code would be smaller/faster using
>> global_data and without total reliance on global variables, but anyway
>> it seems to me that gd is mostly a convenience for pre-relocation
>> where memory may not be available.
>
>
> Indeed, gd is a convenience for pre-relocation, but the point is not that
> memory may not be available : gd points to memory, so there must be some
> memory available anyway, be it SRAM, locked data cache or even SDRAM for
> some boards.
>
> The convenience is that you cannot tell in advance where that memory will
> be, so you need a pointer, and gd is that pointer; it could indeed be passed
> as an argument from function to function, thus going from register to
> register to stack back and forth, or be allocated its own register, and
> considering most of pre-relocation code will want to access gd, allocating
> it globally to a register makes sense as it reduces stack usage (no passing
> of gd) and does not hurt performance that much.

OK

>
>
>>>
>>>> So it has my vote. Once I get somewhere on the reboard series I will
>>>> post my common/board.c. As I may have mentioned I have elected so far
>>>> to leave the initcall list and local functions in
>>>> arch/xxx/lib/board.c.
>>>>
>>>> Would be good to get all this moving.
>>>
>>>
>>> Well, I have a more thorough patch in the making - I creates
>>> init_wrappers.c and init_helpers.c and board.c collapses very
>>> dramatically
>>> (see below) - with a lot of #ifdef hell...
>>
>>
>> I thing that all pre-reloc functions that are called should have the
>> same signature - i.e. return an int. if we (later) move to passing gd
>> around then they would change in that way also.
>
>
> That's one way of seeing it, particularly when these functions are put in a
> table, but note that there is no *reason* for all functions to have the same
> signature, no more than there would be reason for post-reloc functions to
> either. If a design crops up for pre-reloc code where functions involved
> have different signatures, and it is simple, efficient and maintainable, I
> personally won't oppose it on the basis of signature heterogeneity.

The reason is that we are trying to use a function table and have each
function called in turn by a for() loop. That means that each function
should have the same signature.

>
>
>>>
>>> Notice the new board_init_f_r() - This is where U-Boot is running from
>>> Flash, but SDRAM has been initialised and the stack is now in RAM - So
>>> this
>>> is where we move gd to RAM and perform relocation. As a bonus, relocation
>>> can be done with cache enabled :)
>
>
> Not sure why this intermediary function/state is required.

Only because relocation is expensive, so it might be faster. TBD
though and the benefit is expected to be small.

>
>
>> Yes I see. It is not so easy on ARM since we need the MMU also, but I
>> think it can be done. Once data is set up / copied in its new location
>> we can use it even prior to relocation. I also recently created a
>> patch to delay console init until after relocation (cue screams on the
>> list) which saves time.
>
>
> Saving time may be a good thing, but for a bootloader, this would go second
> to always knowing what happens, so if delaying console init means risking
> the loss of some console output in case of stray code execution, then indeed
> there will be screaming.

That's why (if we have it at all) it should be a CONFIG option turned
on only when things are tickety-boo.

If you have time please check my question about where to put ARM
assembler code in the other thread.

Regards,
Simon

>
>> Regards,
>> Simon
>
>
> Amicalement,
> --
> Albert.