Introduce generic relocation feature

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

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.

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.

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

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

Hi Simon,

Le 26/12/2011 19:24, Simon Glass a ?crit :

Quoted 16 lines

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

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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.

Quoted 11 lines

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?

Quoted 10 lines

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.

Quoted 88 lines

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 +

Quoted 7 lines

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

[+TI maintainers, tx25 board maintainer]

Hi Albert,

Quoted 5 lines

Quoted 5 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 10 lines

Quoted 9 lines

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.

Quoted 2 lines

Side question: is there any reason not to define these CONFIG options systematically?

Code size.

Quoted 9 lines

Quoted 8 lines

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

Quoted 3 lines

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.

Regards,
Simon

Hi Simon,

Le 18/01/2012 20:31, Simon Glass a ?crit :

Quoted 15 lines

[+TI maintainers, tx25 board maintainer]

Hi Albert,

Quoted 10 lines

Quoted 8 lines

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.

Quoted 5 lines

Quoted 1 line

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?

Quoted 8 lines

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

Quoted 2 lines

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.

Quoted 5 lines

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.

Quoted 9 lines

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 21 lines

Quoted 5 lines

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

Quoted 2 lines

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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 10 lines

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 2 lines

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

Quoted 6 lines

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.

Quoted 1 line

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

Quoted 7 lines

Quoted 5 lines

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

Quoted 3 lines

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.

Quoted 15 lines

Quoted 15 lines

Quoted 13 lines

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.

Quoted 5 lines

Quoted 2 lines

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?

Quoted 18 lines

Quoted 16 lines

Quoted 10 lines

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.

Quoted 2 lines

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?

Amicalement,
--
Albert.

Quoted 1 line

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.

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.

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

Quoted 1 line

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

Quoted 19 lines

Quoted 18 lines

Quoted 10 lines

Quoted 8 lines

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

Quoted 4 lines

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.

Quoted 1 line

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.

Regards,
Simon

Hi Simon,

Le 20/02/2012 23:38, Simon Glass a ?crit :

Quoted 6 lines

Quoted 1 line

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.

No need to call it from C. The most correct flow should be that the start.S code would call board_init_f, relocate_code() and board_init_r, so that no C code should call assembly language code, and setting the C environment or modifying it would only happen outside of C code.

Quoted 9 lines

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.

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

I don't want multiple homonym files either. I want a start.S file responsible for the generic ARM startup and another asm or include file responsible for the ARM-architecture specifics (i.e. ARM1176, ARM926EJS etc) if required. The name of that second file I'll think over, but it won't be start.S

Quoted 10 lines

Quoted 3 lines

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

Thanks.

Quoted 27 lines

Quoted 25 lines

Quoted 20 lines

Quoted 12 lines

Quoted 9 lines

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

Quoted 5 lines

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.

Quoted 2 lines

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?

Quoted 4 lines

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.

I'll look this up today.

Quoted 3 lines

Regards,
Simon

Amicalement,
--
Albert.