[U-Boot] [PATCH 1/4] mpc83xx: Make start.S true PIC
Remove dependencies on link address. Use GOT and add an new function to calculate the actual address.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- arch/powerpc/cpu/mpc83xx/start.S | 35 +++++++++++++++++++++++++++-------- 1 files changed, 27 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S index 0c8e884..1ab8c88 100644 --- a/arch/powerpc/cpu/mpc83xx/start.S +++ b/arch/powerpc/cpu/mpc83xx/start.S @@ -71,12 +71,12 @@ */ START_GOT GOT_ENTRY(_GOT2_TABLE_) + GOT_ENTRY(_start) GOT_ENTRY(__bss_start) GOT_ENTRY(_end)
#ifndef CONFIG_NAND_SPL GOT_ENTRY(_FIXUP_TABLE_) - GOT_ENTRY(_start) GOT_ENTRY(_start_of_vectors) GOT_ENTRY(_end_of_vectors) GOT_ENTRY(transfer_to_handler) @@ -229,10 +229,12 @@ _start: /* time t 0 */ /* there and deflate the flash size back to minimal size */ /*------------------------------------------------------------*/ bl map_flash_by_law1 - lis r4, (CONFIG_SYS_MONITOR_BASE)@h - ori r4, r4, (CONFIG_SYS_MONITOR_BASE)@l - addi r5, r4, in_flash - _start + EXC_OFF_SYS_RESET - mtlr r5 + + bl 1f +1: mflr r3 /* get current address */ + addi r3, r3, in_flash - 1b + bl add_flash_base + mtlr r3 blr in_flash: #if 1 /* Remapping flash with LAW0. */ @@ -831,10 +833,11 @@ relocate_code: bl _GLOBAL_OFFSET_TABLE_@local-4 mflr r30 #endif - mr r3, r5 /* Destination Address */ - lis r4, CONFIG_SYS_MONITOR_BASE@h /* Source Address */ - ori r4, r4, CONFIG_SYS_MONITOR_BASE@l + lwz r4, GOT(_start) /* Source Address */ + addi r4, r4, -EXC_OFF_SYS_RESET lwz r5, GOT(__bss_start) + mr r3, r10 /* Destination Address */ + sub r5, r5, r4 li r6, CONFIG_SYS_CACHELINE_SIZE /* Cache Line Size */
@@ -1128,6 +1131,21 @@ unlock_ram_in_cache: #endif /* CONFIG_SYS_INIT_RAM_LOCK */
#ifdef CONFIG_SYS_FLASHBOOT + +add_flash_base: + /* Check if already inside flash address space. */ + /* if so, do not add CONFIG_SYS_FLASH_BASE */ + lis r4, (CONFIG_SYS_FLASH_BASE)@h + ori r4, r4, (CONFIG_SYS_FLASH_BASE)@l + cmplw cr0, r3, r4 + ble cr0, 2f /* r3 < r4 ? */ + lis r6, (CONFIG_SYS_FLASH_BASE+(CONFIG_SYS_FLASH_SIZE*1024*1024-1))@h + ori r6, r6, (CONFIG_SYS_FLASH_BASE+(CONFIG_SYS_FLASH_SIZE*1024*1024-1))@l + cmplw cr0, r3, r6 + blelr cr0 /* r3 < r6 ? */ +2: add r3,r3,r4 + blr + map_flash_by_law1: /* When booting from ROM (Flash or EPROM), clear the */ /* Address Mask in OR0 so ROM appears everywhere */ @@ -1179,6 +1197,7 @@ map_flash_by_law1: */ remap_flash_by_law0: /* Initialize the BR0 with the boot ROM starting address. */ + lis r3, (CONFIG_SYS_IMMR)@h /* r3 <= CONFIG_SYS_IMMR */ lwz r4, BR0(r3) li r5, 0x7FFF and r4, r4, r5
link_off calculates the difference between link address and actual load address. This is a must for true PIC u-boot.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- arch/powerpc/cpu/mpc83xx/start.S | 26 ++++++++++++++++++++++++++ include/common.h | 7 +++++++ 2 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S index 1ab8c88..16aed0a 100644 --- a/arch/powerpc/cpu/mpc83xx/start.S +++ b/arch/powerpc/cpu/mpc83xx/start.S @@ -419,6 +419,32 @@ ProgramCheck: _end_of_vectors:
. = 0x3000 +#ifdef CONFIG_SYS_TRUE_PIC + .globl link_off /* const void * link_off(const void * ptr) */ + .type link_off, @function + /* + * Calculates the offset between link address and load address + * and subtracs the offset to from its argument(r3) + * This function must be where _GOT2_TABLE_ is defined + */ +link_off: + /* GOT hand coded as we cannot clobber r12 */ + mflr r4 + bl 1f + .text 2 +0: .long .LCTOC1-1f + .text +1: mflr r6 + lwz r0,0b-1b(r6) + add r6,r0,r6 + mtlr r4 + la r4,.L__GOT2_TABLE_(r6) /* addi r4,r6,.L__GOT2_TABLE_ */ + lwz r5,.L__GOT2_TABLE_(r6) + sub r4,r5,r4 /* r4 - r5 */ + sub r3,r3,r4 /* r4 - r3 */ + blr + .size link_off, .-link_off +#endif
/* * This code finishes saving the registers to the exception frame diff --git a/include/common.h b/include/common.h index 189ad81..a04bd27 100644 --- a/include/common.h +++ b/include/common.h @@ -112,6 +112,13 @@ typedef volatile unsigned char vu_char; #include <asm/arch/hardware.h> #endif
+#ifdef CONFIG_SYS_TRUE_PIC +const void * link_off(const void *); +#else +#define link_off(x) ((const void *)(x)) +#endif +#define LINK_OFF(x) ((__typeof__(x))link_off(x)) + #include <part.h> #include <flash.h> #include <image.h>
By copying the GOT to the end of the INIT_RAM(dcache) and relocating it there, it is much esier to support true PIC on u-boot. This cannot handle FIXUP so C code that depends on fixups before relocation to RAM must use LINK_OFF to calculate the difference.
This depends on the upcoming single-pic-base option to gcc.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- arch/powerpc/cpu/mpc83xx/start.S | 36 ++++++++++++++++++++++++++++++++++++ 1 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/start.S b/arch/powerpc/cpu/mpc83xx/start.S index 16aed0a..af44fdd 100644 --- a/arch/powerpc/cpu/mpc83xx/start.S +++ b/arch/powerpc/cpu/mpc83xx/start.S @@ -291,6 +291,34 @@ in_flash: /* Needed for upcoming -msingle-pic-base */ bl _GLOBAL_OFFSET_TABLE_@local-4 mflr r30 +#ifdef CONFIG_SYS_TRUE_PIC + /* + * Copy GOT to cache and relocate it + * This assumes there is enough space at the end + * INIT_RAM to hold a copy of the GOT. + */ + li r3,0 + bl link_off /* r3 holds link offset at return */ + lis r9, (CONFIG_SYS_INIT_RAM_ADDR+CONFIG_SYS_INIT_RAM_SIZE)@h + ori r9,r9,(CONFIG_SYS_INIT_RAM_ADDR+CONFIG_SYS_INIT_RAM_SIZE)@l + lwz r11,GOT(_GOT2_TABLE_) + add r11,r11,r3 + + li r4, __got2_entries@sectoff@l + mtctr r4 + slwi r5,r4,2 /* r4 * 4 */ + subf r9,r5,r9 /* r9 - r5 */ + + subf r10,r9,r11 /* r11 - r9 */ + subf r30,r10,r30 /* r30 - r10, point PIC(r30) to new GOT */ + addi r11,r11,-4 + addi r9,r9,-4 +1: /* copy GOT and add link offset */ + lwzu r0,4(r11) + add r0,r0,r3 + stwu r0,4(r9) + bdnz 1b +#endif #endif /* r3: IMMR */ lis r3, CONFIG_SYS_IMMR@h @@ -859,9 +887,17 @@ relocate_code: bl _GLOBAL_OFFSET_TABLE_@local-4 mflr r30 #endif +#ifdef CONFIG_SYS_TRUE_PIC + li r3, 0 + bl link_off /* const void * link_off(const void *) */ +#endif lwz r4, GOT(_start) /* Source Address */ addi r4, r4, -EXC_OFF_SYS_RESET lwz r5, GOT(__bss_start) +#ifdef CONFIG_SYS_TRUE_PIC + add r4, r4, r3 /* Add link offset */ + add r5, r5, r3 /* Add link offset */ +#endif mr r3, r10 /* Destination Address */
sub r5, r5, r4
Dear Joakim Tjernlund,
In message 1292838435-14958-3-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
By copying the GOT to the end of the INIT_RAM(dcache) and relocating it there, it is much esier to support true PIC on u-boot. This cannot handle FIXUP so C code that depends on fixups before relocation to RAM must use LINK_OFF to calculate the difference.
...
- /*
* Copy GOT to cache and relocate it* This assumes there is enough space at the end* INIT_RAM to hold a copy of the GOT.*/
Is it correct to assume that system will crash horribly if for some reason the GOT should not fit?
And this means that there is no way to adapt this approach to systems where initial RAM is restrictd and not big enough to hold a copy of the GOT?
Wolfgang Denk wd@denx.de wrote on 2011/01/09 21:54:34:
Dear Joakim Tjernlund,
In message 1292838435-14958-3-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
By copying the GOT to the end of the INIT_RAM(dcache) and relocating it there, it is much esier to support true PIC on u-boot. This cannot handle FIXUP so C code that depends on fixups before relocation to RAM must use LINK_OFF to calculate the difference.
...
- /*
- Copy GOT to cache and relocate it
- This assumes there is enough space at the end
- INIT_RAM to hold a copy of the GOT.
- */
Is it correct to assume that system will crash horribly if for some reason the GOT should not fit?
Yes ATM. I considered adding tests but it will cost more space and I am not sure how to report the error this early.
And this means that there is no way to adapt this approach to systems where initial RAM is restrictd and not big enough to hold a copy of the GOT?
Can't think of one, you need somewhere to put the GOT. You can work on reducing the GOT size though.
Jocke
Only these 2 call sites depends on fixups for my mpc8321 based board.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se --- arch/powerpc/cpu/mpc83xx/cpu_init.c | 2 +- arch/powerpc/lib/board.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c index 7a1cae7..88d9dd8 100644 --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c @@ -507,7 +507,7 @@ int prt_83xx_rsr(void) sep = " "; for (i = 0; i < n; i++) if (rsr & bits[i].mask) { - printf("%s%s", sep, bits[i].desc); + printf("%s%s", sep, LINK_OFF(bits[i].desc)); sep = ", "; } puts("\n"); diff --git a/arch/powerpc/lib/board.c b/arch/powerpc/lib/board.c index 9759e23..4ffec36 100644 --- a/arch/powerpc/lib/board.c +++ b/arch/powerpc/lib/board.c @@ -386,7 +386,7 @@ void board_init_f (ulong bootflag) #endif
for (init_fnc_ptr = init_sequence; *init_fnc_ptr; ++init_fnc_ptr) { - if ((*init_fnc_ptr) () != 0) { + if ((LINK_OFF(*init_fnc_ptr)) () != 0) { hang (); } }
Dear Joakim Tjernlund,
In message 1292838435-14958-4-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
Only these 2 call sites depends on fixups for my mpc8321 based board.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
arch/powerpc/cpu/mpc83xx/cpu_init.c | 2 +- arch/powerpc/lib/board.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c index 7a1cae7..88d9dd8 100644 --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c @@ -507,7 +507,7 @@ int prt_83xx_rsr(void) sep = " "; for (i = 0; i < n; i++) if (rsr & bits[i].mask) {
printf("%s%s", sep, bits[i].desc);
printf("%s%s", sep, LINK_OFF(bits[i].desc)); sep = ", ";
Is my understanding correct that these changes are sufficient only for your board, and only for your current configuration? And that your code would break (resp. require more LINK_OFF fixups) if you would - for example - decide to enable CONFIG_DISPLAY_AER_FULL in your board configuration (cf. print_83xx_arb_event() above in the same source file) ?
I object against such a fragile and insular approach.
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2011/01/09 21:29:04:
Dear Joakim Tjernlund,
In message 1292838435-14958-4-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
Only these 2 call sites depends on fixups for my mpc8321 based board.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
arch/powerpc/cpu/mpc83xx/cpu_init.c | 2 +- arch/powerpc/lib/board.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c index 7a1cae7..88d9dd8 100644 --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c @@ -507,7 +507,7 @@ int prt_83xx_rsr(void) sep = " "; for (i = 0; i < n; i++) if (rsr & bits[i].mask) {
printf("%s%s", sep, bits[i].desc);
printf("%s%s", sep, LINK_OFF(bits[i].desc)); sep = ", "; }Is my understanding correct that these changes are sufficient only for your board, and only for your current configuration? And that your code would break (resp. require more LINK_OFF fixups) if you would - for example - decide to enable CONFIG_DISPLAY_AER_FULL in your board configuration (cf. print_83xx_arb_event() above in the same source file) ?
It would break only if link address != load address. That is, if you want to use my new CONFIG_SYS_TRUE_PIC feature and be able to load u-boot at any address regardless of link address you would have to add LINK_OFF calls into print_83xx_arb_event() too if you want to use it.
I object against such a fragile and insular approach.
Considering you were tempted to add my previous approach which had LINK_OFF calls all over I don't see were this objection comes from. Have you changed your mind?
Jocke
On Sun, 9 Jan 2011 21:48:47 +0100 Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
Wolfgang Denk wd@denx.de wrote on 2011/01/09 21:29:04:
Dear Joakim Tjernlund,
In message 1292838435-14958-4-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
Only these 2 call sites depends on fixups for my mpc8321 based board.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
arch/powerpc/cpu/mpc83xx/cpu_init.c | 2 +- arch/powerpc/lib/board.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c index 7a1cae7..88d9dd8 100644 --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c @@ -507,7 +507,7 @@ int prt_83xx_rsr(void) sep = " "; for (i = 0; i < n; i++) if (rsr & bits[i].mask) {
printf("%s%s", sep, bits[i].desc);
printf("%s%s", sep, LINK_OFF(bits[i].desc)); sep = ", "; }Is my understanding correct that these changes are sufficient only for your board, and only for your current configuration? And that your code would break (resp. require more LINK_OFF fixups) if you would - for example - decide to enable CONFIG_DISPLAY_AER_FULL in your board configuration (cf. print_83xx_arb_event() above in the same source file) ?
It would break only if link address != load address. That is, if you want to use my new CONFIG_SYS_TRUE_PIC feature and be able to load u-boot at any address regardless of link address you would have to add LINK_OFF calls into print_83xx_arb_event() too if you want to use it.
Doesn't this add a requirement for future generic pre-relocation code to comply with, to avoid breaking your board?
-Scott
Scott Wood scottwood@freescale.com wrote on 2011/01/10 19:24:02:
On Sun, 9 Jan 2011 21:48:47 +0100 Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
Wolfgang Denk wd@denx.de wrote on 2011/01/09 21:29:04:
Dear Joakim Tjernlund,
In message 1292838435-14958-4-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
Only these 2 call sites depends on fixups for my mpc8321 based board.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
arch/powerpc/cpu/mpc83xx/cpu_init.c | 2 +- arch/powerpc/lib/board.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/cpu/mpc83xx/cpu_init.c b/arch/powerpc/cpu/mpc83xx/cpu_init.c index 7a1cae7..88d9dd8 100644 --- a/arch/powerpc/cpu/mpc83xx/cpu_init.c +++ b/arch/powerpc/cpu/mpc83xx/cpu_init.c @@ -507,7 +507,7 @@ int prt_83xx_rsr(void) sep = " "; for (i = 0; i < n; i++) if (rsr & bits[i].mask) {
printf("%s%s", sep, bits[i].desc);
printf("%s%s", sep, LINK_OFF(bits[i].desc)); sep = ", "; }Is my understanding correct that these changes are sufficient only for your board, and only for your current configuration? And that your code would break (resp. require more LINK_OFF fixups) if you would - for example - decide to enable CONFIG_DISPLAY_AER_FULL in your board configuration (cf. print_83xx_arb_event() above in the same source file) ?
It would break only if link address != load address. That is, if you want to use my new CONFIG_SYS_TRUE_PIC feature and be able to load u-boot at any address regardless of link address you would have to add LINK_OFF calls into print_83xx_arb_event() too if you want to use it.
Doesn't this add a requirement for future generic pre-relocation code to comply with, to avoid breaking your board?
Yes, but I don't mind if my board breaks from time to time. After all it isn't in u-boot so I have had to deal with quite a few breakages already. It is my hope this new feature will spread to other boards as time pass.
Jocke
Dear Joakim Tjernlund,
In message OF6A573B77.9119DEB0-ONC1257814.0069A574-C1257814.007532C7@transmode.se you wrote:
It would break only if link address != load address. That is, if you want to use my new CONFIG_SYS_TRUE_PIC feature and be able to load u-boot at any address regardless of link address you would have to add LINK_OFF calls into print_83xx_arb_event() too if you want to use it.
Doesn't this add a requirement for future generic pre-relocation code to comply with, to avoid breaking your board?
Yes, but I don't mind if my board breaks from time to time. After all it isn't in u-boot so I have had to deal with quite a few breakages already. It is my hope this new feature will spread to other boards as time pass.
You mean you ask to add code that is not only highly fragile but even known to be broken for other boards than yours, and the only board that uses the feature and has been tested with it "isn't in u-boot" ?
Sorry, but it makes no sense to me to add all this. NAK.
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2011/01/17 23:11:59:
Dear Joakim Tjernlund,
In message OF6A573B77.9119DEB0-ONC1257814.0069A574-C1257814.007532C7@transmode.se you wrote:
It would break only if link address != load address. That is, if you want to use my new CONFIG_SYS_TRUE_PIC feature and be able to load u-boot at any address regardless of link address you would have to add LINK_OFF calls into print_83xx_arb_event() too if you want to use it.
Doesn't this add a requirement for future generic pre-relocation code to comply with, to avoid breaking your board?
Yes, but I don't mind if my board breaks from time to time. After all it isn't in u-boot so I have had to deal with quite a few breakages already. It is my hope this new feature will spread to other boards as time pass.
You mean you ask to add code that is not only highly fragile but even known to be broken for other boards than yours, and the only board that uses the feature and has been tested with it "isn't in u-boot" ?
No other board is broken. This new function is neutral to other boards. I am merely saying as my board is the first user of this new feature I expect minor breakage of my board from time to time when someone adds a new function that needs LINK_OFF to work on my board but forgets to actually add the LINK_OFF call. Once more boards uses my new feature this problem goes away.
Wolfgang, once you indicated you were interested in such feature as I have added but my first impl. had LINK_OFF calls all over the place, still you were tempted to add the feature. Now that I have reduced the LINK_OFF calls to a minimum you suddenly want to reject it even though >95% of the LINK_OFF calls are gone. Why this change of heart?
Jocke
Dear Joakim Tjernlund,
In message OF71B40F2E.5C935BA8-ONC125781B.007CB9E6-C125781B.007E5726@transmode.se you wrote:
No other board is broken. This new function is neutral to other boards.
Well, I see this differntly.
Wolfgang, once you indicated you were interested in such feature as I have added but my first impl. had LINK_OFF calls all over the place, still you were tempted to add the feature. Now that I have reduced the LINK_OFF calls to a minimum you suddenly want to reject it even though >95% of the LINK_OFF calls are gone. Why this change of heart?
Yes, I am definitely interested in this feature.
But I still consider the impact to make this really working to heavy (and by working I mean working for all boards, out of the box, without having to go through iterations of breakage to find out where else a LINK_OFF needs to be added).
In the current form, your "simplification" results just from the fact that you just added the bare minimum needed for your board. If - as you hope - more and more boards would use this, more and more of these now omitted LINK_OFFs would have to be added again. The result would be exactly the same mess as your original patch - it doesn't small any better when you try to feed it to me in small bites.
And frankly, adding this stuff for an out-of-tree port is, um..., sorry, but I don't find polite words atm.
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2011/01/18 00:42:10:
Dear Joakim Tjernlund,
In message OF71B40F2E.5C935BA8-ONC125781B.007CB9E6-C125781B.007E5726@transmode.se you wrote:
No other board is broken. This new function is neutral to other boards.
Well, I see this differntly.
Wolfgang, once you indicated you were interested in such feature as I have added but my first impl. had LINK_OFF calls all over the place, still you were tempted to add the feature. Now that I have reduced the LINK_OFF calls to a minimum you suddenly want to reject it even though >95% of the LINK_OFF calls are gone. Why this change of heart?
Yes, I am definitely interested in this feature.
Good.
But I still consider the impact to make this really working to heavy (and by working I mean working for all boards, out of the box, without having to go through iterations of breakage to find out where else a LINK_OFF needs to be added).
Yes, but the target area is fairly small. Only code the runs before relocation which isn't that much. How do you think a solution for all boards would look like? The only other method is can think of is some MMU trickery and I don't even see how you can make that work on all boards and you would probably be locked to specific address ranges if you use BATs as defined on PowerPC
In the current form, your "simplification" results just from the fact that you just added the bare minimum needed for your board. If - as you hope - more and more boards would use this, more and more of these now omitted LINK_OFFs would have to be added again. The result would be exactly the same mess as your original patch - it doesn't small any better when you try to feed it to me in small bites.
My first patch was pretty much only my board too and the result for all boards would be MUCH smaller than the first patch approach. I did only did one board because that was what I could test and I wanted feedback from the list. Did you expect me to blindly port this to every board? The ground work for 83xx is done, a few more LINK_OFF calls is probably needed for all 83xx boards, I can take a stab at the remaining 83xx boards once you embrace this method. The rest will have to be up to the other board maintainers, if they want this feature.
And frankly, adding this stuff for an out-of-tree port is, um..., sorry, but I don't find polite words atm.
Yet you are interested in such a feature. Sorry but I too have a hard time finding polite words.
On Tue, 18 Jan 2011 01:18:34 +0100 Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
How do you think a solution for all boards would look like? The only other method is can think of is some MMU trickery and I don't even see how you can make that work on all boards and
You don't need to make the MMU trick work on all boards, just your board (or cpu family), because it wouldn't be imposing anything on the rest of the system. Do we actually have someone who needs this feature on a board without a suitable MMU, large lockable cache or other SRAM, hardware bank switching, or other mechanism that can be confined to early low-level board/cpu-specific code?
you would probably be locked to specific address ranges if you use BATs as defined on PowerPC
You'd have alignment constraints, but it's good enough for bank switching.
-Scott
Scott Wood scottwood@freescale.com wrote on 2011/01/18 18:27:49:
On Tue, 18 Jan 2011 01:18:34 +0100 Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
How do you think a solution for all boards would look like? The only other method is can think of is some MMU trickery and I don't even see how you can make that work on all boards and
You don't need to make the MMU trick work on all boards, just your board (or cpu family), because it wouldn't be imposing anything on the rest of the system. Do we actually have someone who needs this feature on a board without a suitable MMU, large lockable cache or other SRAM, hardware bank switching, or other mechanism that can be confined to early low-level board/cpu-specific code?
Wolfgang seems to think so. As I read his reply he wants a solution for all boards which I don't think is possible. I do think my approach comes closest though. I did try BATs but I didn't get very far.
you would probably be locked to specific address ranges if you use BATs as defined on PowerPC
You'd have alignment constraints, but it's good enough for bank switching.
Alignment and a suitable bank size so you don't fall into your environment. That may make BATs hard to use in general.
Dear Joakim Tjernlund,
In message OF287114D8.013B6E51-ONC125781C.00667DCD-C125781C.00673260@transmode.se you wrote:
You don't need to make the MMU trick work on all boards, just your board (or cpu family), because it wouldn't be imposing anything on the rest of the system. Do we actually have someone who needs this feature on a board without a suitable MMU, large lockable cache or other SRAM, hardware bank switching, or other mechanism that can be confined to early low-level board/cpu-specific code?
Wolfgang seems to think so. As I read his reply he wants a solution for all
Please be aware that I don't think anything at all. I just comment :-)
I'm not in your position, where I am focussing on "how can I implement this". I'm looking at the code from the outside, and I ask what does it give to me, what of the things I'd like to have does it not give to me, and at which price does it come.
boards which I don't think is possible. I do think my approach comes closest though. I did try BATs but I didn't get very far.
If we are talking about _all_ boards we have to keep a wider view. For example, on MPC8xx there are not BATs. Not to mention other architectures.
Actually I was not asking for support for all boards, not even for all boards of a specific architecture, or a specific CPU. You submitted this patch, and I learned that the code, as is, is only useful for a single board, which appears to be maintained in an out-of-tree port. For all in-tree boards the code, as is, is broken and unusable without further rework. Needless to repeat that it is completely untested for mainline.
So we have a patch that promises a feature, which cannot be used by any of the mainline boards, but it makes the code more complex for zero benefit.
It's a nice and appreciated RFC patch or even example implementation, but I fail to see arguments why we should add this to mainline.
Best regards,
Wolfgang Denk
On Tue, 18 Jan 2011 21:06:34 +0100 Wolfgang Denk wd@denx.de wrote:
Dear Joakim Tjernlund,
In message OF287114D8.013B6E51-ONC125781C.00667DCD-C125781C.00673260@transmode.se you wrote:
boards which I don't think is possible. I do think my approach comes closest though. I did try BATs but I didn't get very far.
What problems did you run into?
If we are talking about _all_ boards we have to keep a wider view. For example, on MPC8xx there are not BATs. Not to mention other architectures.
I don't see why MPC8xx's MMU couldn't be used for this, although it might be a tight fit if you want to get everything into the pinned TLB entries.
Should we be talking about all boards, if the only user of this so far is on hardware that can do it in a less-intrusive way?
Another thing to keep in mind is that Joakim's approach also won't work on all boards -- you need hardware that is capable of selecting from different entry points, where the entry address itself changes rather than flash banks being flipped around.
-Scott
Scott Wood scottwood@freescale.com wrote on 2011/01/18 21:24:16:
On Tue, 18 Jan 2011 21:06:34 +0100 Wolfgang Denk wd@denx.de wrote:
Dear Joakim Tjernlund,
In message OF287114D8.013B6E51-ONC125781C.00667DCD-C125781C.00673260@transmode.se you wrote:
boards which I don't think is possible. I do think my approach comes closest though. I did try BATs but I didn't get very far.
What problems did you run into?
It crashed and burned :) I never got to the details.
If we are talking about _all_ boards we have to keep a wider view. For example, on MPC8xx there are not BATs. Not to mention other architectures.
I don't see why MPC8xx's MMU couldn't be used for this, although it might be a tight fit if you want to get everything into the pinned TLB entries.
I think it is harder that that. You probably need a tlb miss handler.
Should we be talking about all boards, if the only user of this so far is on hardware that can do it in a less-intrusive way?
Another thing to keep in mind is that Joakim's approach also won't work on all boards -- you need hardware that is capable of selecting from different entry points, where the entry address itself changes rather than flash banks being flipped around.
No, you only need gcc support for msingle-pic-base and some extra RAM to hold the GOT(about 8KB would do I think) while still in flash. Ah, you mean how to select which image to select? That is a small piece of SW that checks both images and chooses the valid one and jumps there.
Jocke
On Tue, 18 Jan 2011 21:39:23 +0100 Joakim Tjernlund joakim.tjernlund@transmode.se wrote:
Scott Wood scottwood@freescale.com wrote on 2011/01/18 21:24:16:
I don't see why MPC8xx's MMU couldn't be used for this, although it might be a tight fit if you want to get everything into the pinned TLB entries.
I think it is harder that that. You probably need a tlb miss handler.
Well, as I said, it might be a tight fit. If you can't fit everything within the pinned entries, then you'll need a TLB miss handler, but it could be a simple handler that will insert a non-cacheable identity-mapped entry for whatever address faulted (cacheable regions are covered by pinned entries and won't fault in the first place). Or possibly have an above/below address threshold for determining cacheability.
After relocation, you can turn off the MMU, so it's only things you access before relocation that need to fit in the TLB.
Should we be talking about all boards, if the only user of this so far is on hardware that can do it in a less-intrusive way?
Another thing to keep in mind is that Joakim's approach also won't work on all boards -- you need hardware that is capable of selecting from different entry points, where the entry address itself changes rather than flash banks being flipped around.
No, you only need gcc support for msingle-pic-base and some extra RAM to hold the GOT(about 8KB would do I think) while still in flash. Ah, you mean how to select which image to select? That is a small piece of SW that checks both images and chooses the valid one and jumps there.
OK, I thought you were using something like a DIP switch to toggle low/high exception vectors, and getting different entry points that way.
-Scott
Dear Scott Wood,
In message 20110118142416.5892ca00@udp111988uds.am.freescale.net you wrote:
If we are talking about _all_ boards we have to keep a wider view. For example, on MPC8xx there are not BATs. Not to mention other architectures.
I don't see why MPC8xx's MMU couldn't be used for this, although it might be a tight fit if you want to get everything into the pinned TLB entries.
I just wanted to point out that the "use BATs then" approach is also not a solution that covers all systems.
Should we be talking about all boards, if the only user of this so far is on hardware that can do it in a less-intrusive way?
In the first round of discussion the buzz word was to acchieve a fully position independent U-Boot image. This would obviously have a number of nice use cases, including the often asked for "load test version to free area in RAM and jump to it".
This is somehting that would indeed be useful for all boards, on all architectures, all SoCs. So it makes sense to me to include into the reviewing if a proposed solution is capable of being generalized to other boards / systems or not.
Another thing to keep in mind is that Joakim's approach also won't work on all boards -- you need hardware that is capable of selecting from different entry points, where the entry address itself changes rather than flash banks being flipped around.
There could be other mechanisms in place to determine which U-Boot image should be started, like a tiny piece of software that jumps to the selected image.
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2011/01/18 21:06:34:
Dear Joakim Tjernlund,
In message OF287114D8.013B6E51-ONC125781C.00667DCD-C125781C.00673260@transmode.se you wrote:
You don't need to make the MMU trick work on all boards, just your board (or cpu family), because it wouldn't be imposing anything on the rest of the system. Do we actually have someone who needs this feature on a board without a suitable MMU, large lockable cache or other SRAM, hardware bank switching, or other mechanism that can be confined to early low-level board/cpu-specific code?
Wolfgang seems to think so. As I read his reply he wants a solution for all
Please be aware that I don't think anything at all. I just comment :-)
I'm not in your position, where I am focussing on "how can I implement this". I'm looking at the code from the outside, and I ask what does it give to me, what of the things I'd like to have does it not give to me, and at which price does it come.
boards which I don't think is possible. I do think my approach comes closest though. I did try BATs but I didn't get very far.
If we are talking about _all_ boards we have to keep a wider view. For example, on MPC8xx there are not BATs. Not to mention other architectures.
Actually I was not asking for support for all boards, not even for all boards of a specific architecture, or a specific CPU. You submitted this patch, and I learned that the code, as is, is only useful for a single board, which appears to be maintained in an out-of-tree port. For all in-tree boards the code, as is, is broken and unusable without further rework. Needless to repeat that it is completely untested for mainline.
Ah, finally you make sense to me. I actually tested this with mainline on my board so it is not completely untested in mainline.
So we have a patch that promises a feature, which cannot be used by any of the mainline boards, but it makes the code more complex for zero benefit.
You can test that nothing breaks in other 83xx boards. That would be valuable feedback to me. I can try to complete all NOR based 83xx boards too, but someone needs to test them.
It's a nice and appreciated RFC patch or even example implementation, but I fail to see arguments why we should add this to mainline.
Well, you have to start somewhere and as this involves asm changes in start.S it would be pretty dangerous add these without being able to test. The idea is that once some version of this patch is in, interested parties can apply the same concept on their boards too.
Finally, I would like to remind you about [PATCH] PowerPC: Move -fPIC flag to common place [PATCH] PowerPC: Add support for -msingle-pic-base Those two stands on its own.
Dear Joakim Tjernlund,
In message OFBBE91305.B19350D9-ONC125781C.007192FC-C125781C.007421BB@transmode.se you wrote:
Ah, finally you make sense to me. I actually tested this with mainline on my board so it is not completely untested in mainline.
As your board itself is not in mainline this _is_ completely untested for mainline.
It's a nice and appreciated RFC patch or even example implementation, but I fail to see arguments why we should add this to mainline.
Well, you have to start somewhere and as this involves asm changes in start.S it would be pretty dangerous add these without being able to test. The idea is that once some version of this patch is in, interested parties can apply the same concept on their boards too.
Such testing can be done anywhere, in some test branch. I don't think mainline is the right place for such an intrusive and experiemental feature.
Finally, I would like to remind you about [PATCH] PowerPC: Move -fPIC flag to common place
I have not seen any test reports on this, so I hesitate to apply it (the move to a common place is no problem, of course, but I'm not sure about changing -fPIC into -fpic). I think I remember problems with this in the past; there have even been commits to "use '-fPIC' _instead_ of '-mrelocatable'" in the past. I don't remember the details, but I'd like to see some independent testing before this goes in.
[PATCH] PowerPC: Add support for -msingle-pic-base
Same here. This hits a large number of boards, but I have seen zero test reports.
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2011/01/18 22:25:04:
Dear Joakim Tjernlund,
In message OFBBE91305.B19350D9-ONC125781C.007192FC-C125781C.007421BB@transmode.se you wrote:
Ah, finally you make sense to me. I actually tested this with mainline on my board so it is not completely untested in mainline.
As your board itself is not in mainline this _is_ completely untested for mainline.
It's a nice and appreciated RFC patch or even example implementation, but I fail to see arguments why we should add this to mainline.
Well, you have to start somewhere and as this involves asm changes in start.S it would be pretty dangerous add these without being able to test. The idea is that once some version of this patch is in, interested parties can apply the same concept on their boards too.
Such testing can be done anywhere, in some test branch. I don't think mainline is the right place for such an intrusive and experiemental feature.
Do you have such a branch where you can apply this?
Finally, I would like to remind you about [PATCH] PowerPC: Move -fPIC flag to common place
I have not seen any test reports on this, so I hesitate to apply it (the move to a common place is no problem, of course, but I'm not sure about changing -fPIC into -fpic). I think I remember problems with this in the past; there have even been commits to "use '-fPIC' _instead_ of '-mrelocatable'" in the past. I don't remember the details, but I'd like to see some independent testing before this goes in.
Right, this can be confusing. Without -mrelocatable you don't get fixups so this has to stay as fixups is a must nowadays.
Assuming you don't need fixups, -fpic needs what went into the linker scripts a little while ago so -fpic had little chance before that.
[PATCH] PowerPC: Add support for -msingle-pic-base
Same here. This hits a large number of boards, but I have seen zero test reports.
Scott and Kim, can you give these 2 patches a spin?
Dear Joakim Tjernlund,
In message OFA95D19C0.58C4CBC0-ONC125781C.00785FED-C125781C.0079E902@transmode.se you wrote:
Such testing can be done anywhere, in some test branch. I don't think mainline is the right place for such an intrusive and experiemental feature.
Do you have such a branch where you can apply this?
But I'm have no plans to test it. I don't have time nor resources for that.
Best regards,
Wolfgang Denk
Dear Joakim Tjernlund,
In message 1292838435-14958-1-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
Remove dependencies on link address. Use GOT and add an new function to calculate the actual address.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
arch/powerpc/cpu/mpc83xx/start.S | 35 +++++++++++++++++++++++++++-------- 1 files changed, 27 insertions(+), 8 deletions(-)
It seems this code introduces some subtle changes.
- lis r4, (CONFIG_SYS_MONITOR_BASE)@h
- ori r4, r4, (CONFIG_SYS_MONITOR_BASE)@l
- addi r5, r4, in_flash - _start + EXC_OFF_SYS_RESET
- mtlr r5
The original code references CONFIG_SYS_MONITOR_BASE.
- bl add_flash_base
...
+add_flash_base:
- /* Check if already inside flash address space. */
- /* if so, do not add CONFIG_SYS_FLASH_BASE */
- lis r4, (CONFIG_SYS_FLASH_BASE)@h
- ori r4, r4, (CONFIG_SYS_FLASH_BASE)@l
- cmplw cr0, r3, r4
- ble cr0, 2f /* r3 < r4 ? */
- lis r6, (CONFIG_SYS_FLASH_BASE+(CONFIG_SYS_FLASH_SIZE*1024*1024-1))@h
- ori r6, r6, (CONFIG_SYS_FLASH_BASE+(CONFIG_SYS_FLASH_SIZE*1024*1024-1))@l
- cmplw cr0, r3, r6
- blelr cr0 /* r3 < r6 ? */
+2: add r3,r3,r4
- blr
But your new code does not reference CONFIG_SYS_MONITOR_BASE at all, but uses CONFIG_SYS_FLASH_BASE instead.
On which boards has this been tested?
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2011/01/09 21:44:08:
Dear Joakim Tjernlund,
In message 1292838435-14958-1-git-send-email-Joakim.Tjernlund@transmode.se you wrote:
Remove dependencies on link address. Use GOT and add an new function to calculate the actual address.
Signed-off-by: Joakim Tjernlund Joakim.Tjernlund@transmode.se
arch/powerpc/cpu/mpc83xx/start.S | 35 +++++++++++++++++++++++++++-------- 1 files changed, 27 insertions(+), 8 deletions(-)
It seems this code introduces some subtle changes.
- lis r4, (CONFIG_SYS_MONITOR_BASE)@h
- ori r4, r4, (CONFIG_SYS_MONITOR_BASE)@l
- addi r5, r4, in_flash - _start + EXC_OFF_SYS_RESET
- mtlr r5
The original code references CONFIG_SYS_MONITOR_BASE.
- bl add_flash_base
...
+add_flash_base:
- /* Check if already inside flash address space. */
- /* if so, do not add CONFIG_SYS_FLASH_BASE */
- lis r4, (CONFIG_SYS_FLASH_BASE)@h
- ori r4, r4, (CONFIG_SYS_FLASH_BASE)@l
- cmplw cr0, r3, r4
- ble cr0, 2f /* r3 < r4 ? */
- lis r6, (CONFIG_SYS_FLASH_BASE+(CONFIG_SYS_FLASH_SIZE*1024*1024-1))@h
- ori r6, r6, (CONFIG_SYS_FLASH_BASE+(CONFIG_SYS_FLASH_SIZE*1024*1024-1))@l
- cmplw cr0, r3, r6
- blelr cr0 /* r3 < r6 ? */
+2: add r3,r3,r4
- blr
But your new code does not reference CONFIG_SYS_MONITOR_BASE at all, but uses CONFIG_SYS_FLASH_BASE instead.
You can't assume a fixed address when doing PIC therefore the change.
On which boards has this been tested?
Only on our custom 83xx boards.
Dear Joakim Tjernlund,
In message OF7F8BAE70.CE1486C7-ONC1257813.00727B82-C1257813.0072BE1B@transmode.se you wrote:
But your new code does not reference CONFIG_SYS_MONITOR_BASE at all, but uses CONFIG_SYS_FLASH_BASE instead.
You can't assume a fixed address when doing PIC therefore the change.
I understand this change is intentionally, then?
On which boards has this been tested?
Only on our custom 83xx boards.
Can you please try at least on one (or more) of the standard eval boards as well?
Best regards,
Wolfgang Denk
Wolfgang Denk wd@denx.de wrote on 2011/01/10 00:25:59:
Dear Joakim Tjernlund,
In message OF7F8BAE70.CE1486C7-ONC1257813.00727B82-C1257813.0072BE1B@transmode.se you wrote:
But your new code does not reference CONFIG_SYS_MONITOR_BASE at all, but uses CONFIG_SYS_FLASH_BASE instead.
You can't assume a fixed address when doing PIC therefore the change.
I understand this change is intentionally, then?
Yes it is needed, otherwise is isn't PIC as you assume a fixed start address. The FLASH base is the same in both cases.
On which boards has this been tested?
Only on our custom 83xx boards.
Can you please try at least on one (or more) of the standard eval boards as well?
Don't have one. We borrowed a board during early development but this board has been returned long ago.
Jocke
participants (4)
-
Joakim Tjernlund -
Joakim Tjernlund
-
Scott Wood -
Wolfgang Denk