[U-Boot] [PATCH v3 0/6] Introduce generic relocation feature

[Albert ARIBAUD]

Hi Simon,

Le 18/01/2012 20:31, Simon Glass a écrit :
> [+TI maintainers, tx25 board maintainer]
>
> Hi Albert,

>>> 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 with a lot of different CPUs
>>> to have a similar file.
>>
>> NAK for several reasons:
>
> I think you are NAKing the 'arm: Add processor function library'
> patch out of the series:

Yes. Wasn't that clear?

> Create reloc.h and include it where needed
> define CONFIG_SYS_SKIP_RELOC for all archs
> Add generic relocation feature
> arm: Add processor function library
> arm: Move over to generic relocation
> arm: Remove unused code in start.S
>
> Q1: Should I remove that patch and just repeat the code from there in
> each start.S file?

You should keep the code that jumps to board_init_r as it is.

> The impact would be to remove most of the code in
> the relocate_code() function in each start.S except for the last few
> instructions (reproduced below).

Yes.

>> 1. The code that goes on proc.S is already in start.S, which already
>> contains "things which cannot be written in C and are common functions used
>> by all ARM CPUs". Granted, right now start.S is duplicated in multiple CPU
>> directories; so the thing to do is to merge all ARM start.S files into a
>> single one, rather than merging only one of its parts.
>
> Q2: What should this merged file be called and what directory should
> it be in? This is the question I asked last time, and the lack of an
> answer is the reason why I have been unable to make progress here.
> Please advise your preference and I will sort it out.

Sorry if I missed this question. Obviously the file will be called 
start.S exactly like all its merge ancestors, and it should reside in 
arch/arm/cpu.

Note that I already said the merging of start.S should *not* be part of 
this patch set, and should be a patch set of its own.

> Note I don't think I can do this in one series - there is far too much
> variation between the files for me to take on that way. I need a new
> file than I can bit I bit move things over into, allowing affected
> parties to comment on each as I go.

Note that I do not require that *you* do such a merge either, though 
help is always welcome of course.

>> 2. There is no interest in moving this segment of code from all start.S
>> files into a new proc.S file: there is no gain is code size obviously, and
>> there is an increase in source file count.
>
> Just checking that you see that the code is removed from start.S, not
> moved. The code in proc.S is:
>
> .globl proc_call_board_init_r
> proc_call_board_init_r:
> #ifndef CONFIG_SYS_ICACHE_OFF
> 	mcr	p15, 0, r0, c7, c5, 0	@ invalidate icache
> 	mcr     p15, 0, r0, c7, c10, 4	@ DSB
> 	mcr     p15, 0, r0, c7, c5, 4	@ ISB
> #endif
> 	mov	sp, r3
> 	/* jump to it ... */
> 	mov	pc, r2
>
> which is taken from the end of each relocate_code() copy. Assuming we
> are on the page, then ok.

We are -- the code removed from start.S is relocation, which I quite 
agree with since you replace it with the C relocation code.

>> 3. I consider that files should contain 'things' based on their common
>> /functionality/, not on their similar /nature/, and "going about starting up
>> U-Boot" is a functionality.
>
> The code here sets the stack pointer and calls a function. However I
> agree this unlikely to be useful outside starting up. See Q2.

Ditto.

>> Note that eventually, having a single start.S will achieve the same effect
>> as this 'proc.S' patch.
>
> At present start.S runs for a bit and then jumps to board_init_f(). At
> the end of board_init_f() we jump back to start.S (relocate_code()
> function) and from there to board_init_r(). Also start.S has exception
> handling code.
>
> I think start.S should be thought of as in three parts:
> - early CPU init (hardest to make common)
> - relocation code (last 3 patches of this series)
> - exception code
>
> The first one clearly belongs in start.S. I don't think the other two
> do. They are not related to CPU start-up

Exception code is indeed related to startup code in that they are 
vectored through the same ARM-defined exception vectors table.

>  The relocation code is only
>  there because of the need to call a function with a new stack pointer.

No -- the relocation code is there *and* we need a switch of stack 
pointer, but relocation and stack switching are functionally unrelated.

> Other than that it can be written in C. Apart from reset the exception
> code isn't related to starting up the CPU - it is only there because
> the it is a convenient assembler file (your reasoning suggests that
> you would in fact want this in a new except.S file).

I disagree.

> So maybe we want start.S, reloc.S and except.S?

No. I just agree that relocation can and should go out of start.S. The 
rest -- exception vectors table, startup until board_init_f(), pivot to 
board_init_r() and exception handlers -- stays in start.S

>> Note also that this start.S commonalization should be a completely different
>> patch set.
>
> Are you saying you want a later series which moves all the start.S
> relocate_code() code from each cpu dir into a single place? If so,
> please see Q2.

I would like such a patch series, but I don't ask anyone for it. If I 
have time I might submit it myself.

>>> Code size on my ARMv7 system increases by 54 bytes with generic
>>> relocation. This overhead is mostly just literal pool access and setting
>>> up to call the relocated U-Boot at the end.
>>>
>>> On my system, execution time increases from 10.8ms to 15.6ms due to the
>>> less efficient C implementations of the copy and zero loops. If execution
>>> time is of concern, you can define CONFIG_USE_ARCH_MEMSET and
>>> CONFIG_USE_ARCH_MEMCPY to reduce it. For met this reduces relocation time
>>> to 5.4ms, i.e. twice as fast as the old system.
>>
>> Side question: is there any reason not to define these CONFIG options
>> systematically?
>
> Code size.

What is the code size increase of using arch-specific mem ops?

>>> One problem remains which causes mx31pdk to fail to build. It doesn't
>>> have string.c in its SPL code and the architecture-specific versions of
>>> memset()/memcpy() are too large. I propose to add a local change to
>>> reloc.c that uses inline code for boards that use the old legacy SPL
>>> framework. We can remove it later. This is not included in v2 but I am
>>> interested in comments on this approach. An alternative would be just
>>> to add simple memset()/memcpy() functions just for this board (and one
>>> other affected MX31 board).
>>
>> I am not too fond of the "later removal" approach. In my experience, this
>> invariably tends to the "old bloat in the code no one knows the reason of"
>> situations.
>
> Me neither. The only board affected is the tx25. We could let the
> maintainer fix it up, I suppose. I have no way of testing it.

John: ping?

> Regards,
> Simon

Amicalement,
-- 
Albert.