[U-Boot] [RFC PATCH 0/7] reboard: Introduce generic relocation feature

[Simon Glass]

Hi Graeme,

On Tue, Dec 6, 2011 at 6:56 PM, Graeme Russ <graeme.russ at gmail.com> wrote:
> Hi Simon
>
> Sorry for the late review on this - I've only just now had a good
> chance to look at it...
>

Thanks for the comments.

> On Wed, Dec 7, 2011 at 12:56 PM, Simon Glass <sjg at chromium.org> wrote:
>> Hi Tom,
>>
>> On Mon, Nov 28, 2011 at 3:45 PM, Tom Rini <tom.rini at gmail.com> wrote:
>>> On Mon, Nov 21, 2011 at 4:57 PM, Simon Glass <sjg at chromium.org> wrote:
>>>> This is the second patch series aiming to unify the various board.c files
>>>> in each architecture into a single one. This series creates a libboard
>>>> library and implements relocation in it. It then moves ARM over to use
>>>> this framework, as an example.
>>>>
>>>> On ARM the relocation code is duplicated for each CPU yet it
>>>> is the same. We can bring this up to the arch level. But since (I believe)
>>>> Elf relocation is basically the same process for all archs, there is no
>>>> reason not to bring it up to the generic level.
>>>>
>>>> This series establishes a new libboard library in the board/ subdir and
>>>> puts some relocation code in it. Each architecture which uses this
>>>> framework needs to provide a function called arch_elf_relocate_entry()
>>>> which processes a single relocation entry. If there is concern about
>>>> calling a function for all 2000-odd relocations then I can change this.
>
> Ugh, that overhead is a killer - The relocation can be done entirely in a
> tight loop with a switch statement in your reloc_elf() function. But I
> think reloc_elf() should be weak - x86 relocation can be done in a few
> lines of assembler which is smaller and faster than a C implementation
> will ever be

I will benchmark it tomorrow. If we move the for loop into the
architecture-specific code then it goes away. Just seemed to me that
the less duplicated code we have the better. We could use an inline
function but I'm not keen on that.

>
>>>> For ARM, a new arch/arm/lib/proc.S file is created, which holds generic
>>>> ARM assembler code (things that cannot be written in C and are common
>>>> functions used by all ARM CPUs). This helps reduce duplication. Interrupt
>>>> handling code and perhaps even some startup code can move there later.
>>>>
>>>> It may be useful for other architectures to have a similar file.
>>>>
>>>> This series moves ARM over to use this framework. Overall this means that
>>>> two new files are required 'early' in boot: board/reloc.c and
>>>> arch/arm/lib/proc.S.  This is tricky mainly due to SPL. I believe that
>>>> we may need to adjust link scripts to put these two files early in the
>>>> link scripts also. But I am not sure about this and can't actually find
>>>> a problem as yet. I would much prefer to solve this with a new section
>>>> name like .text.early if we can.
>>>>
>>>> (I should really cc all arch maintainers but I think in that case I get
>>>> an error from the list server. Not sure what the limit is.)
>>>>
>>>> Comments please...
>
> The whole sequence of the series seems a little bit 'random' to me - I
> think patches 2 and 3 can be squahed and the dead code removal in patch 7
> should be done in the same patch(es) that create the dead code.
>
Yes I can clean this up.

> It looks like reloc_make_copy(), reloc_clear_bss() and reloc_elf() have
> been poached from x86 - The copy and clear loops are quite inefficient. I
> have patches at home which change these to use memcpy and memset

Yes they have - in fact I have a series here which unifies board.c
completely for ARM and x86. But I need to take this in baby steps as I
said.

>
>>>
>>> Seems like a good idea.  Got some size's before and after?  Maybe some
>>> boot times too (as you mention 2000-odd function calls during
>>> relocation) ?
>>>
>>> --
>>> Tom
>>
>> Yes I have this info - will package it up tomorrow along with a new
>> series to deal with review comments.
>
> As far as I understand it, the long-term plan is to unify the board init
> sequence across all architectures. I can see three approaches:
>
>  1) Tweak each arch until they are all the same and then 'throw the switch'
>  2) Pick one or two arches and 'throw the switch' on them and migrate the
>    others later
>  3) Pull out common bits and pieced and deal with the left-overs later
>
> It looks like you've opted for number 3. The problem I see in this approach
> is that you are relying on the other arches to follow your lead which is, at
> best, a 50/50 bet.

Sort of. Actually I have gone for option 2. But rather than send a
series which switches over ARM and then x86 (about 40 patches) I have
decided to do it in steps. Relocation is the first step.

>
> Now I look at board_init_f() and board_init_r() in ARM and x86 and think
> 'why on earth isn't most of than in the init loops?'. The boot sequence
> for ARM and x86 should be reducible to (all arches are generally pretty
> close to this, so it should not introduce too much pain when they decide
> to switch)

Yes my generic board.c consists of a couple of init loops. I can email
the patch if you like but it is not ready for the mailing list.

>
>  - Low level init (reset vector etc)
>  - Setup temporary stack and gd storage (typically in CPU cache)
>  + init_f loop <-- SDRAM initialised
>  - Move gd to RAM
>  - Setup new stack in SDRAM
>  - Turn on caching (cannot be done while gd and stack in cache!!!)
>  + Calculate relocation address
>  + Copy to RAM
>  + Do relocation fixups
>  + Clear BSS
>  - Jump to RAM
>  + init_r loop
>  + main_loop()
>
> + = common code
> - = arch, SoC, or board specific
>
> Note: Typically the boot sequence currently has 'Turn on caching' after jump
> to RAM - I have patches in the works which change this for x86
>
> So my vote would be #2 - Unify the ARM and x86 and bring any code that can
> be inside the init loops in and then switch both over to a generic init
> implementation. Other arches can be switched later

Agreed, but I can't send a series that big, so I am doing it in stages.

Regards,
Simon

>
> Regards,
>
> Graeme