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

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

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.

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

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.

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.

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.

Note that eventually, having a single start.S will achieve the same 
effect as this 'proc.S' patch.

Note also that this start.S commonalization should be a completely 
different patch set.

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

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

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