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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
