[+TI maintainers, tx25 board maintainer]
Hi Albert,
On Tue, Jan 17, 2012 at 11:28 PM, Albert ARIBAUD
<albert.u.b...@aribaud.net> wrote:
> Hi Simon,
>
> Le 26/12/2011 19:24, Simon Glass a écrit :
>
>> (I am resending this rebased so I can continue with this board-unification
>> work and allow people to review patches. There were some comments on the
>> v2 series but my questions have been sitting on the list for 2 weeks so it
>> is probably time for a new series.)
>>
>> This is the second patch series aiming to unify the various board.c files
>> in each architecture into a single one. This series implements a generic
>> relocation feature, which is the bridge between board_init_f() and
>> board_init_r(). 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.
>>
>> Each architecture which uses this framework needs to provide a function
>> called arch_elf_relocate_entry() which processes a single relocation
>> entry. This is a static inline function to reduce code size overhead.
>
>
> Agreed.
OK, that is the first three patches.
>
>
>> 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:
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? 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).
>
> 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.
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.
>
> 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.
>
> 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.
>
> 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. The relocation code is only
there because of the need to call a function with a new stack pointer.
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).
So maybe we want start.S, reloc.S and except.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.
>
>
>> 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.
>
>
>> 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.
>
>
>> Changes in v2:
>> - Use CONFIG_SYS_SKIP_RELOC instead of CONFIG_SYS_LEGACY_BOARD
>> - Import asm-generic/sections.h from Linux and add U-Boot extras
>> - Squash generic link symbols patch into generic relocation patch
>> - Move reloc.c into common/
>> - Add function comments
>> - Use memset, memcpy instead of inline code
>> - Add README file for relocation
>> - Invalidate I-cache when we jump to relocated code
>> - Use an inline relocation function to reduce code size
>> - Make relocation symbols global so we can use them outside start.S
>>
>> Changes in v3:
>> - Remove the 'reloc' tag from each commit
>> - Rebase to master
>>
>> Simon Glass (6):
>> 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
>>
>> README | 4 +
>> arch/arm/cpu/arm1136/start.S | 133 ++------------
>> arch/arm/cpu/arm1176/start.S | 214
>> ++-------------------
>> arch/arm/cpu/arm720t/start.S | 127 ++-----------
>> arch/arm/cpu/arm920t/start.S | 135 ++------------
>> arch/arm/cpu/arm925t/start.S | 135 ++------------
>> arch/arm/cpu/arm926ejs/davinci/spl.c | 1 +
>> arch/arm/cpu/arm926ejs/start.S | 144 ++-------------
>> arch/arm/cpu/arm946es/start.S | 130 ++-----------
>> arch/arm/cpu/arm_intcm/start.S | 135 ++------------
>> arch/arm/cpu/armv7/omap-common/spl.c | 1 +
>> arch/arm/cpu/armv7/start.S | 140 ++------------
>> arch/arm/cpu/ixp/start.S | 127 ++-----------
>> arch/arm/cpu/lh7a40x/start.S | 124 ++-----------
>> arch/arm/cpu/pxa/start.S | 138 ++------------
>> arch/arm/cpu/s3c44b0/start.S | 127 ++-----------
>> arch/arm/cpu/sa1100/start.S | 124 ++-----------
>> arch/arm/include/asm/reloc.h | 56 ++++++
>> arch/arm/lib/Makefile | 2 +
>> arch/arm/lib/board.c | 1 +
>> arch/arm/lib/proc.S | 40 ++++
>> arch/avr32/config.mk | 3 +
>> arch/avr32/lib/board.c | 1 +
>> arch/blackfin/config.mk | 3 +
>> arch/m68k/config.mk | 3 +
>> arch/m68k/lib/board.c | 1 +
>> arch/microblaze/config.mk | 3 +
>> arch/mips/config.mk | 3 +
>> arch/mips/lib/board.c | 1 +
>> arch/nds32/config.mk | 3 +
>> arch/nds32/lib/board.c | 1 +
>> arch/nios2/config.mk | 3 +
>> arch/powerpc/config.mk | 3 +
>> arch/powerpc/lib/board.c | 1 +
>> arch/sandbox/config.mk | 3 +
>> arch/sh/config.mk | 3 +
>> arch/sparc/config.mk | 3 +
>> arch/x86/config.mk | 3 +
>> arch/x86/lib/board.c | 1 +
>> board/freescale/mpc8313erdb/mpc8313erdb.c | 1 +
>> board/freescale/mpc8315erdb/mpc8315erdb.c | 1 +
>> board/samsung/smdk6400/smdk6400_nand_spl.c | 1 +
>> board/sheldon/simpc8313/simpc8313.c | 1 +
>> common/Makefile | 4 +
>> common/reloc.c | 121 ++++++++++++
>> doc/README.relocation | 87 +++++++++
>> include/asm-generic/sections.h | 92 +++++++++
>> include/common.h | 2 +-
>> include/reloc.h | 54 +++++
>> nand_spl/board/freescale/mpc8536ds/nand_boot.c | 1 +
>> nand_spl/board/freescale/mpc8569mds/nand_boot.c | 1 +
>> nand_spl/board/freescale/mpc8572ds/nand_boot.c | 1 +
>> nand_spl/board/freescale/mx31pdk/Makefile | 8 +-
>> nand_spl/board/freescale/mx31pdk/u-boot.lds | 1 +
>> nand_spl/board/freescale/p1010rdb/nand_boot.c | 1 +
>> nand_spl/board/freescale/p1023rds/nand_boot.c | 1 +
>> nand_spl/board/freescale/p1_p2_rdb/nand_boot.c | 1 +
>> nand_spl/board/freescale/p1_p2_rdb_pc/nand_boot.c | 1 +
>> nand_spl/board/karo/tx25/Makefile | 8 +-
>> nand_spl/board/karo/tx25/u-boot.lds | 1 +
>> nand_spl/nand_boot_fsl_nfc.c | 1 +
>> 61 files changed, 705 insertions(+), 1765 deletions(-)
>> create mode 100644 arch/arm/include/asm/reloc.h
>> create mode 100644 arch/arm/lib/proc.S
>> create mode 100644 common/reloc.c
>> create mode 100644 doc/README.relocation
>> create mode 100644 include/asm-generic/sections.h
>> create mode 100644 include/reloc.h
>
>
> Amicalement,
> --
> Albert.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
