reboard: Introduce generic relocation feature

Before adding new relocation functions, move this prototype out of common.h where things are pretty crowded.

Signed-off-by Simon Glass<[email protected]>

---

  arch/arm/cpu/arm926ejs/davinci/spl.c              |    1 +
  arch/arm/cpu/armv7/omap-common/spl.c              |    1 +
  arch/arm/lib/board.c                              |    1 +
  arch/avr32/lib/board.c                            |    1 +
  arch/m68k/lib/board.c                             |    1 +
  arch/mips/lib/board.c                             |    1 +
  arch/nds32/lib/board.c                            |    1 +
  arch/powerpc/lib/board.c                          |    1 +
  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 +
  include/common.h                                  |    2 +-
  include/reloc.h                                   |   39 +++++++++++++++++++++
  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/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/nand_boot_fsl_nfc.c                      |    1 +
  23 files changed, 61 insertions(+), 1 deletions(-)

create mode 100644 include/reloc.h

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 30 lines

Before adding new relocation functions, move this prototype out of common.h where things are pretty crowded.

Signed-off-by Simon Glass<[email protected]>

---

?arch/arm/cpu/arm926ejs/davinci/spl.c ? ? ? ? ? ? ?| ? ?1 +
?arch/arm/cpu/armv7/omap-common/spl.c ? ? ? ? ? ? ?| ? ?1 +
?arch/arm/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?arch/avr32/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?arch/m68k/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?arch/mips/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?arch/nds32/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?arch/powerpc/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?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 +
?include/common.h ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +-
?include/reloc.h ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? 39
+++++++++++++++++++++
?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/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/nand_boot_fsl_nfc.c ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +

Quoted 2 lines

?23 files changed, 61 insertions(+), 1 deletions(-) ?create mode 100644 include/reloc.h

Since this patch set indicates that ARM, and only ARM, is moved over to relocation, I would prefer it that no other arch file be modified in this patch, and that the move of ARM to the new relocation mechanism be done in an atomic commit, so that other architectures can refer to a single commit in order to do their own move.

Amicalement,
--
Albert.

Hi Albert,

Quoted 36 lines · Albert ARIBAUD

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 31 lines

Before adding new relocation functions, move this prototype out of common.h where things are pretty crowded.

Signed-off-by Simon Glass<[email protected]>

---

  arch/arm/cpu/arm926ejs/davinci/spl.c              |    1 +
  arch/arm/cpu/armv7/omap-common/spl.c              |    1 +
  arch/arm/lib/board.c                              |    1 +
  arch/avr32/lib/board.c                            |    1 +
  arch/m68k/lib/board.c                             |    1 +
  arch/mips/lib/board.c                             |    1 +
  arch/nds32/lib/board.c                            |    1 +
  arch/powerpc/lib/board.c                          |    1 +
  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 +
  include/common.h                                  |    2 +-
  include/reloc.h                                   |   39
+++++++++++++++++++++
  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/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/nand_boot_fsl_nfc.c                      |    1 +
  23 files changed, 61 insertions(+), 1 deletions(-)

Quoted 1 line

create mode 100644 include/reloc.h

Quoted 5 lines

Since this patch set indicates that ARM, and only ARM, is moved over to relocation, I would prefer it that no other arch file be modified in this patch, and that the move of ARM to the new relocation mechanism be done in an atomic commit, so that other architectures can refer to a single commit in order to do their own move.

What specifically are you asking for in this patch? I added this at the request of a reviewer of v1, who felt that we should be removing code from common.h instead of adding it, and that relocation is done in only a few sites so should not be in common.h. Do you think this patch should be pulled out of the series and done on its own?

Regards,
Simon

Hi Albert,

Quoted 36 lines · Albert ARIBAUD

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 30 lines

Before adding new relocation functions, move this prototype out of common.h where things are pretty crowded.

Signed-off-by Simon Glass<[email protected]>

---

?arch/arm/cpu/arm926ejs/davinci/spl.c ? ? ? ? ? ? ?| ? ?1 +
?arch/arm/cpu/armv7/omap-common/spl.c ? ? ? ? ? ? ?| ? ?1 +
?arch/arm/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?arch/avr32/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?arch/m68k/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?arch/mips/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?arch/nds32/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?arch/powerpc/lib/board.c ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +
?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 +
?include/common.h ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| ? ?2 +-
?include/reloc.h ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | ? 39
+++++++++++++++++++++
?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/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/nand_boot_fsl_nfc.c ? ? ? ? ? ? ? ? ? ? ?| ? ?1 +

Quoted 2 lines

?23 files changed, 61 insertions(+), 1 deletions(-) ?create mode 100644 include/reloc.h

Quoted 5 lines

Since this patch set indicates that ARM, and only ARM, is moved over to relocation, I would prefer it that no other arch file be modified in this patch, and that the move of ARM to the new relocation mechanism be done in an atomic commit, so that other architectures can refer to a single commit in order to do their own move.

What specifically are you asking for in this patch? I added this at the request of a reviewer of v1, who felt that we should be removing code from common.h instead of adding it, and that relocation is done in only a few sites so should not be in common.h. Do you think this patch should be pulled out of the series and done on its own?

I think the point is that when we pull stuff out of common.h into, say, foo.h then only include foo.h where the functionality moved into foo.h is _currently_ used.

It looks like you've pulled out the functions definitions into reloc.h then included reloc.h everywhere (including common.h). Better would be to moved the definitions to reloc.h, do not include reloc.h in common.h and include reloc.h only where it is strictly needed right now - Add #include<reloc.h> to each arch file as and when it is needed

Regards,

Graeme

Hi Gream,

Hi Gream,

Le 11/12/2011 22:45, Graeme Russ a ?crit :

Quoted 10 lines

I think the point is that when we pull stuff out of common.h into, say, foo.h then only include foo.h where the functionality moved into foo.h is _currently_ used.

It looks like you've pulled out the functions definitions into reloc.h then included reloc.h everywhere (including common.h). Better would be to moved the definitions to reloc.h, do not include reloc.h in common.h and include reloc.h only where it is strictly needed right now - Add #include<reloc.h> ?to each arch file as and when it is needed

Thanks Greame. This is indeed what I meant.

Quoted 4 lines

Regards,

Graeme

Amicalement,
--
Albert.

We are introducing a new generic relocation features and we want this to be the default. So we need to opt all architectures out first. Some may never have relocation, but those that do will eventually move over to this generic relocation framework.

This is part of the unified board effort, but since we are only dealing with relocation in this series, CONFIG_SYS_SKIP_RELOC is more appropriate than CONFIG_SYS_LEGACY_BOARD.

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 8 lines

We are introducing a new generic relocation features and we want this to be the default. So we need to opt all architectures out first. Some may never have relocation, but those that do will eventually move over to this generic relocation framework.

This is part of the unified board effort, but since we are only dealing with relocation in this series, CONFIG_SYS_SKIP_RELOC is more appropriate than CONFIG_SYS_LEGACY_BOARD.

I'm afraid I haven't made myself clear on CONFIG_SYS_SKIP_RELOC. I did not mean it to be an 'old vs new reloc' choice mechanism; I mean it to be the controlling option for whether relocation happens at all or not.

I want a relocation skip option, because it is useful for boards which, for any reason, know that they are already residing at the Right Address(tm).

As far as an option to switch from old to new relocation... If there is a consensus from all custodians that all arches should move to generic relocation, then I think we should avoid allowing the older mechanism to persist at all.

Amicalement,
--
Albert.

Add a library to hold ARM assembler code which is generic across all ARM CPUs. At first it just holds some basic relocation code. The plan is to move more start.S code here.

Hi Simon,

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 3 lines

Add a library to hold ARM assembler code which is generic across all ARM CPUs. At first it just holds some basic relocation code. The plan is to move more start.S code here.

I still don't see the point in this new file, as this code is common across all ARM boards and start.S already has much of such common code. I'd rather see start.S keeping the common code and moving up the hierarchy, and and the {ISA, core, SoC, board} ?specific code be moved out of start.S and down into their 'natural' specific location.

Amicalement,
--
Albert.

Hi Albert,

Quoted 9 lines · Albert ARIBAUD

Hi Simon,

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 3 lines

Add a library to hold ARM assembler code which is generic across all ARM CPUs. At first it just holds some basic relocation code. The plan is to move more start.S code here.

Quoted 5 lines

I still don't see the point in this new file, as this code is common across all ARM boards and start.S already has much of such common code. I'd rather see start.S keeping the common code and moving up the hierarchy, and and the {ISA, core, SoC, board} ?specific code be moved out of start.S and down into their 'natural' specific location.

If we do this then it would be a separate series focused on refactoring start.S. Are you saying that you want that series before we address this relocation series? Where specifically should we put the new start.S and what should we call it? I assume that all the little start.S files will remain or are you wanting to rename all of those into some other filename in arch/arm/cpu/... In which case what should those be called?

It is good to be really specific so that I know the destination port before embarking on a new voyage.

Regards,
Simon

Quoted 5 lines

Amicalement,
--
Albert.

Now that we are using the generic relocation framework, we don't need this code.

Note: Here we lose the ARM1176's enable_mmu code. This seems to duplicate code already in U-Boot now. Can anyone comment on this?

Hi Simon,

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 3 lines

Now that we are using the generic relocation framework, we don't need this code.

Quoted 2 lines

Note: Here we lose the ARM1176's enable_mmu code. This seems to duplicate code already in U-Boot now. Can anyone comment on this?

First thing would be for you to explain why it is removed when it is not related to relocation, I think.

Amicalement,
--
Albert.

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

Hi Simon,

General comments here, detailed comments in reply to n/6 patches.

Le 10/12/2011 20:16, Simon Glass a ?crit :

Quoted 10 lines

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.

Actually most of start.S is very similar across all its avatars in arch/arm, and is a good candidate for being generalized. I would prefer a generalization of start.S with the vector table, generic startup sequence prepare for calling board_init_f, jump to board_init_r with a possible stack switch, exception handlers) , and anything specific moved into the adequate subdirectory.

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

I assume this is due to some arch other than armnot using the same set of entry types, right? If so, I think it would be better to keep arch-specific entry relocation code under conditional complilation in the generic ELF relication source file, so that all ELF-structure dependent code is in a single file.

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.

Indeed, but I think start.S is a better candidate for this, see comment above.

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

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

Seeing as inline will only avoid pushing a couple argument registers and doing a branch, and OTOH may require additional register shuffling in the calling code, how much code does this inlining save?

Quoted 3 lines

- Make relocation symbols global so we can use them outside start.S

Why should they relocation symbols ever be used outside of actually relocating?

Amicalement,
--
Albert.

Quoted 6 lines

Actually most of start.S is very similar across all its avatars in arch/arm, and is a good candidate for being generalized. I would prefer a generalization of start.S with the vector table, generic startup sequence prepare for calling board_init_f, jump to board_init_r with a possible stack switch, exception handlers) , and anything specific moved into the adequate subdirectory.

Yes I agree. However this is not actually the purpose of this series, which is to head (so far as is possible) towards a generic board.c file for all architectures. While I completely agree that start.S is a mess, that is a separate problem!

Quoted 10 lines

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.

I assume this is due to some arch other than armnot using the same set of entry types, right? If so, I think it would be better to keep arch-specific entry relocation code under conditional complilation in the generic ELF relication source file, so that all ELF-structure dependent code is in a single file.

Yes that takes things one step further. I did look at it but could not find an elf.h with defines for all the different relocations. Do you know where to find that?

Quoted 10 lines

Quoted 6 lines

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.

Indeed, but I think start.S is a better candidate for this, see comment above.

My understanding of start.S is that it is the start-up code for the machine, and that over time we would hope to reduce it to a small amount of code which is specific to that particular cpu (i.e. the shared and common code moves out).

We can do this in many small steps - certainly I don't have the appetite for changing everything around all at once.

Anyway. as I may have mentioned before, I feel that calling back into start.S from C code is wrong. That file sits at the beginning of the image whereas the code you call back to doesn't need to. It also means that there is (currently) no generic ARM assembler file (apart from low_level.S) which C code can call to do things which are specific to the architecture (but not the cpu).

Quoted 3 lines

Seeing as inline will only avoid pushing a couple argument registers and doing a branch, and OTOH may require additional register shuffling in the calling code, how much code does this inlining save?

About 48 bytes all up from memory.

Quoted 4 lines

Quoted 1 line

- Make relocation symbols global so we can use them outside start.S

Why should they relocation symbols ever be used outside of actually relocating?

Well since relocation is moving out of start.S to a generic library which is not architecture-specific, we must make these symbols available to it. Otherwise the generic code doesn't know what it is relocating.

Regards,
Simon

Hi againLe 11/12/2011 22:30, Simon Glass a ?crit :

Quoted 11 lines

Quoted 9 lines

Actually most of start.S is very similar across all its avatars in arch/arm,
and is a good candidate for being generalized. I would prefer a generalization of start.S with the vector table, generic startup sequence prepare for calling board_init_f, jump to board_init_r with a possible stack
switch, exception handlers) , and anything specific moved into the adequate
subdirectory.

Quoted 4 lines

Yes I agree. However this is not actually the purpose of this series, which is to head (so far as is possible) towards a generic board.c file for all architectures. While I completely agree that start.S is a mess, that is a separate problem!

Indeed, Generalizing start.S is a separate problem from generalizing board.c; and that means your reboard patch series is unaffected whether you move some code into proc.S or not. Actually, that move to proc.S is just a code factorization, unrelated to relocation -- your series can work just as well without it, only it will keep on taking up code size that could be reduced regardless to the way relocation is done.

Now, seen from the reverse perspective of generalizing start.S, the move to proc.S in this reboard series is a step in the wrong direction, which is why I do not want it. Plus, it'll make your 'reboard' patch more focused on what it is about, i.e. generalizing relocation.

(BTW, even though I understand your greater goal about board.c, I still think the patch subjects should *not* contain this "reboard" prefix nor mention that greater goal. The patch series is about relocation, and should be clear about it.)

Quoted 8 lines

Quoted 8 lines

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

Quoted 6 lines

Quoted 6 lines

I assume this is due to some arch other than armnot using the same set of entry types, right? If so, I think it would be better to keep arch-specific
entry relocation code under conditional complilation in the generic ELF relication source file, so that all ELF-structure dependent code is in a single file.

(sorry for the typoes, BTW)

Quoted 5 lines

Yes that takes things one step further. I did look at it but could not find an elf.h with defines for all the different relocations. Do you know where to find that?

We don't need to find a header file with all the relocation types we will need, we only need one with the two ARM relocation types we need now. Adding them to include/elf.h.

/>>> For ARM, a new arch/arm/lib/proc.S file is created, which holds generic

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

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

Quoted 2 lines

It may be useful for other architectures with a lot of different CPUs to have a similar file.

Indeed, but I think start.S is a better candidate for this, see comment above.

Quoted 7 lines

My understanding of start.S is that it is the start-up code for the machine, and that over time we would hope to reduce it to a small amount of code which is specific to that particular cpu (i.e. the shared and common code moves out).

We can do this in many small steps - certainly I don't have the appetite for changing everything around all at once.

This view of start.S is kind of an /a priori/ -- just because the name has start.S does not mean that there should only be startup code in there. Conversively, one could argue that handling the returning from board_init_f and branching into board_init_r is *still* startup code.

But yes, maybe one day start.S will be split into a file with only the startup code and a file with everything else. Only this is totally unrelated to relocation, and thus will not happen in this patch series.

Quoted 8 lines

Anyway. as I may have mentioned before, I feel that calling back into start.S from C code is wrong. That file sits at the beginning of the image whereas the code you call back to doesn't need to. It also means that there is (currently) no generic ARM assembler file (apart from low_level.S) which C code can call to do things which are specific to the architecture (but not the cpu).

I fail to see the point in your argument about about start.S being "far away" from the C code that call into it or return to it. Never do we consider the "distance" between C callers and callees, so why should we do it here?

As for there being no generic ARM assembler file etc., before complaining that there isn't one, I would like to see why one should be created. For me, just having a couple pieces of code that are 'generic ARM code that C can call' is not a criterion for putting them in a new file. Files must group pieces of code that have common functional purpose, not simply pieces of code that have a common characteristic.

Quoted 8 lines

Quoted 5 lines

Seeing as inline will only avoid pushing a couple argument registers and doing a branch, and OTOH may require additional register shuffling in the calling code, how much code does this inlining save?

About 48 bytes all up from memory.

About 12 instructions? Funny, I wouldn't have thought inlining a function called only once would save that much. Thanks for the info.

Hi Albert,

Quoted 20 lines · Albert ARIBAUD

Hi againLe 11/12/2011 22:30, Simon Glass a ?crit :

Quoted 11 lines

Quoted 9 lines

Actually most of start.S is very similar across all its avatars in arch/arm,
and is a good candidate for being generalized. I would prefer a generalization of start.S with the vector table, generic startup sequence prepare for calling board_init_f, jump to board_init_r with a possible stack
switch, exception handlers) , and anything specific moved into the adequate
subdirectory.

Quoted 4 lines

Yes I agree. However this is not actually the purpose of this series, which is to head (so far as is possible) towards a generic board.c file for all architectures. While I completely agree that start.S is a mess, that is a separate problem!

Quoted 6 lines

Indeed, Generalizing start.S is a separate problem from generalizing board.c; and that means your reboard patch series is unaffected whether you move some code into proc.S or not. Actually, that move to proc.S is just a code factorization, unrelated to relocation -- your series can work just as well without it, only it will keep on taking up code size that could be reduced regardless to the way relocation is done.

OK but sorry I am still a bit unclear.

Since I need to call from C the assembler function which sets up the stack, invalidates the I-cache and jumps to board_init_r(), I need this function to be somewhere. Perhaps in the future we might devise a way of doing some of this in C code, or we might change the API. But for now we need it.

Given that we need it, it makes little sense to me to put it in start.S. It then gets repeated 10 times throughout the code, with every cpu having its own version.

Hi Simon,

Quoted 39 lines · Simon Glass

Hi Albert,

Quoted 20 lines · Albert ARIBAUD

Hi againLe 11/12/2011 22:30, Simon Glass a ?crit :

Quoted 11 lines

Quoted 9 lines

Actually most of start.S is very similar across all its avatars in arch/arm,
and is a good candidate for being generalized. I would prefer a generalization of start.S with the vector table, generic startup sequence prepare for calling board_init_f, jump to board_init_r with a possible stack
switch, exception handlers) , and anything specific moved into the adequate
subdirectory.

Quoted 4 lines

Yes I agree. However this is not actually the purpose of this series, which is to head (so far as is possible) towards a generic board.c file for all architectures. While I completely agree that start.S is a mess, that is a separate problem!

Quoted 6 lines

Indeed, Generalizing start.S is a separate problem from generalizing board.c; and that means your reboard patch series is unaffected whether you move some code into proc.S or not. Actually, that move to proc.S is just a code factorization, unrelated to relocation -- your series can work just as well without it, only it will keep on taking up code size that could be reduced regardless to the way relocation is done.

OK but sorry I am still a bit unclear.

Since I need to call from C the assembler function which sets up the stack, invalidates the I-cache and jumps to board_init_r(), I need this function to be somewhere. Perhaps in the future we might devise a way of doing some of this in C code, or we might change the API. But for now we need it.

Quoted 4 lines

Given that we need it, it makes little sense to me to put it in start.S. It then gets repeated 10 times throughout the code, with every cpu having its own version.

[snip]

The 'problem' is that in order to implement the centralisation of the relocation code, you need to do some non-relocation related fix-ups along the way.

I think it would be good to seperate what you are doing into a 'prepare' series which simply shuffles code around ready for the actual relocation patches - The compiled code should be identical before and after the 'prepare' patches. If you find some code that can be optimised (consolidating duplicate code across SoCs for example) then do this either immediately before are after the 'prepare' patches (if those optimisations make no difference to the relocation changes, then you can even leave them until after the relocation patches)

After the 'prepare' patches, the relocation changes will be much clearer

Remember, there is nothing wrong with submitting multiple series of patches where one depends on the other, but you need to make the precedence clear in the description. This approach is preferable over interleaving patches which are not, technically, related to the subject of what you are doing in the series. If it gets to the point where you simply must have some interleaving patches, then it is OK to do it within the series, but change the subject of the interleaving patches to make it clear that they are not directly related

Rember, once the patches are applied, the concept of a 'series' will be lost, so the tag at the beginning of the subject must clearly represent what that individual patch is about - Having a 'reloc' tag on a code consolidation patch will not make sense in a years time...

Regards,

Graeme