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

login
register
mail settings
Submitter Joakim Tjernlund
Date Dec. 20, 2010, 9:47 a.m.
Message ID <1292838435-14958-1-git-send-email-Joakim.Tjernlund@transmode.se>
Download mbox | patch
Permalink /patch/76178/
State Not Applicable
Delegated to: Marek Vasut
Headers show

Comments

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

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