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

[Simon Glass]

Hi Albert,

On Fri, Feb 3, 2012 at 2:06 PM, Albert ARIBAUD
<albert.u.boot at aribaud.net> wrote:
> 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?
>

It would be better if you NAKed the specific patch, but anyway, I
understood what you meant.

>
>> 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.

I have had a look at this and I don't believe that I can. I need to
call it from C and so it needs to conform to the C calling standard. I
will send a new series showing what I mean.

>
>
>> 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.

Yes I don't propose to do this now. I believe that the first 100
instructions may well need to have differences between the CPUs. I
believe we need a common file but that each CPU will need the ability
to override this, using the common file as needed.

Are you saying that you want a start.S in arch/arm/cpu and each
arch/arch/cpu/*/ ? I would prefer there was only one file named
start.S.

>
>
>> 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.
>

I am happy to take these sorts of things on over time, but really I
want to get the existing efforts in first.

>
>>> 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.

Yes, and I would love to change the function signature of
board_init_r() to remove the parameters, and add a generic 'change sp
and jump' function to each architecture. Not planning to do so though.
For now, this code needs to be specific to this task - only able to
call board_init_r() with the parameters it needs and in the right
order.

>
>
>> 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

We can't agree on everything...anyway that discussion is not relevant
to this series at present.

>
>
>>> 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.

OK and I hope that file is not arch/arm/cpu/start.S which would
introduce confusion I think.

>
>
>>>> 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?

About 800 bytes for me:

   text	   data	    bss	    dec	    hex	filename
 177653	   3932	 218240	 399825	  619d1	u-boot
 178429	   3932	 218240	 400601	  61cd9	u-boot

>
>
>>>> 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?

Since I didn't hear anything I have elected to patch this up a
different way - basically splitting out memcpy() and memset() from
string.c. See what you think of the new series. It introduces no
breakages now, at least.

>
> Amicalement,
> --
> Albert.

Regards,
Simon