[U-Boot,1/4] mpc83xx: Make start.S true PIC

Submitted by Joakim Tjernlund on Dec. 20, 2010, 9:47 a.m.

Details

Message ID 1292838435-14958-1-git-send-email-Joakim.Tjernlund@transmode.se
State Not Applicable
Delegated to: Marek Vasut
Headers show

Commit Message

Joakim Tjernlund Dec. 20, 2010, 9:47 a.m.
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(-)

Comments

Wolfgang Denk Jan. 9, 2011, 8:44 p.m.
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
Joakim Tjernlund Jan. 9, 2011, 8:53 p.m.
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.
Wolfgang Denk Jan. 9, 2011, 11:25 p.m.
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
Joakim Tjernlund Jan. 10, 2011, 1:05 a.m.
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

Patch hide | download patch | download mbox

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