Patchwork [U-Boot,alternate,version,V2] at91rm9200: fix lowlevel_init() SMRDATA size

login
register
mail settings
Submitter Jens Scharsig
Date Dec. 5, 2010, 8:12 a.m.
Message ID <4CFB4980.50505@scharsoft.de>
Download mbox | patch
Permalink /patch/74288/
State Superseded
Delegated to: Reinhard Meyer
Headers show

Comments

Jens Scharsig - Dec. 5, 2010, 8:12 a.m.
Signed-off-by: Jens Scharsig <js_at_ng@scharsoft.de>
---

Changes since first version:

* fix: remove on line to much (thanks Andreas Biessmann)
* fix: add _MTEXT_BASE instead of sub from table end

 arch/arm/cpu/arm920t/at91/lowlevel_init.S |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

-- 1.7.1
Reinhard Meyer - Dec. 5, 2010, 10:16 a.m.
Dear Andreas, dear Jens,
> Signed-off-by: Jens Scharsig <js_at_ng@scharsoft.de>
> ---
> 
> Changes since first version:
> 
> * fix: remove on line to much (thanks Andreas Biessmann)
> * fix: add _MTEXT_BASE instead of sub from table end

Just tell me when a patch is "final" and ready to be applied :)

Best Regards,
Reinhard
Jens Scharsig - Dec. 6, 2010, 5:36 p.m.
Dear Reinhard,
> 
> Just tell me when a patch is "final" and ready to be applied :)
> 

If Andreas can confirm the patch, I my case its final


regards Jens
Andreas Bießmann - Dec. 6, 2010, 9:03 p.m.
Dear all,

Am 06.12.2010 um 18:36 schrieb Jens Scharsig:

> Dear Reinhard,
>> 
>> Just tell me when a patch is "final" and ready to be applied :)
>> 
> 
> If Andreas can confirm the patch, I my case its final

Ouch ... it is completely wrong :(

See here ...

---8<---
> @@ -65,7 +65,8 @@ LoopOsc:
> 	ldr	r0, =SMRDATA
> 	ldr	r1, _MTEXT_BASE
> 	sub	r0, r0, r1
> -	add	r2, r0, #80
> +	ldr	r2, =SMRDATAE
> +	add	r2, r2, r1
--->8---

We do load the position of SMRDATA to r0 (which is absolute address of SMRDATA symbol/section however we call it), load the content of _MTEXT_BASE to r1 (which is same as CONFIG_TEXT_BASE).
Then we subtract TEXT_BASE from absolute address of SMRDATA. This lead to some value about 0x500 in r0 which is completely wrong which in turn leads to data abort. ... It would make sense to me to do some construct like this for relocated code, but here we are before relocation and therefore it would be sufficient to 

---8<---
ldr r0, =SMRDATA
ldr r2, =SMRDATAE
--->8---

I have step through the code with JTAG and at least the pll stuff should work correct, but didn't had time to test this in-depth, therefore no patch here.
The approach with begin/end marker is more robust than my first try to get lowlevel_init() fixed for arm920t/at91 but it seems it is completely broken.

More on lowlevel_init at end of the week, I have no time to look for it before. Therefore I do not see it for v2010.12. Jens, can you do some more on that before?

regards

Andreas Bießmann
Jens Scharsig - Dec. 12, 2010, 7:12 p.m.
Dear Andreas Bießmann
> ---8<---
>> @@ -65,7 +65,8 @@ LoopOsc:
>> 	ldr	r0, =SMRDATA
>> 	ldr	r1, _MTEXT_BASE
>> 	sub	r0, r0, r1
>> -	add	r2, r0, #80
>> +	ldr	r2, =SMRDATAE
>> +	add	r2, r2, r1
> --->8---

Ok,this is wrong, But it is code from version 1. The V2 use the correct sub instruction.
> 

> Then we subtract TEXT_BASE from absolute address of SMRDATA. This lead to some value about 0x500 in r0 which is completely wrong which in turn leads to data abort. ... It would make sense to me to do some construct like this for relocated code, but here we are before relocation and therefore it would be sufficient to 

At boot time flash memory is mapded to 0x0. So the address 0x500 and 0x10000500 points the same location in flash.
> 

regards

Jens Scharsig
Reinhard Meyer - Dec. 17, 2010, 7:55 a.m.
Dear Jens Scharsig, dear Andreas Bießmann
>> ---8<---
>>> @@ -65,7 +65,8 @@ LoopOsc:
>>> 	ldr	r0, =SMRDATA
>>> 	ldr	r1, _MTEXT_BASE
>>> 	sub	r0, r0, r1
>>> -	add	r2, r0, #80
>>> +	ldr	r2, =SMRDATAE
>>> +	add	r2, r2, r1
>> --->8---
> 
> Ok,this is wrong, But it is code from version 1. The V2 use the correct sub instruction.
> 
>> Then we subtract TEXT_BASE from absolute address of SMRDATA. This lead to some value about 0x500 in r0 which is completely wrong which in turn leads to data abort. ... It would make sense to me to do some construct like this for relocated code, but here we are before relocation and therefore it would be sufficient to 
> 
> At boot time flash memory is mapded to 0x0. So the address 0x500 and 0x10000500 points the same location in flash.

Is there going to be a V3 soon? And please note, when "checkpatching"
V2 there are style problems:

> ERROR: trailing whitespace
> #56: FILE: arch/arm/cpu/arm920t/at91/lowlevel_init.S:119:
> +SMRDATAE:^I$
> 
> ERROR: trailing whitespace
> #64: FILE: arch/arm/cpu/arm920t/at91/lowlevel_init.S:166:
> +SMRDATA1E:^I^I$
> 
> total: 2 errors, 0 warnings, 31 lines checked

I find it a very valuable tool to do a ../linux-2.6/scripts/checkpatch.pl"
before sending the patch ;)

Best Regards,
Reinhard
Andreas Bießmann - Dec. 23, 2010, 11:37 a.m.
Dear Jens Scharsig,

Am 12.12.2010 um 20:12 schrieb Jens Scharsig:

> Dear Andreas Bießmann
>> ---8<---
>>> @@ -65,7 +65,8 @@ LoopOsc:
>>> 	ldr	r0, =SMRDATA
>>> 	ldr	r1, _MTEXT_BASE
>>> 	sub	r0, r0, r1
>>> -	add	r2, r0, #80
>>> +	ldr	r2, =SMRDATAE
>>> +	add	r2, r2, r1
>> --->8---
> 
> Ok,this is wrong, But it is code from version 1. The V2 use the correct sub instruction.

you are right, I copied the wrong part here.

>> Then we subtract TEXT_BASE from absolute address of SMRDATA. This lead to some value about 0x500 in r0 which is completely wrong which in turn leads to data abort. ... It would make sense to me to do some construct like this for relocated code, but here we are before relocation and therefore it would be sufficient to 
> 
> At boot time flash memory is mapded to 0x0. So the address 0x500 and 0x10000500 points the same location in flash.

That is correct. Sorry, I tested your patch with another textbase cause I can not get my openocd/arm-usb-tiny combo working with my at49bv6416 attached NOR flash.

regards

Andreas Bießmann

Patch

diff --git a/arch/arm/cpu/arm920t/at91/lowlevel_init.S b/arch/arm/cpu/arm920t/at91/lowlevel_init.S
index eaea9d2..b6a9164 100644
--- a/arch/arm/cpu/arm920t/at91/lowlevel_init.S
+++ b/arch/arm/cpu/arm920t/at91/lowlevel_init.S
@@ -65,7 +65,8 @@  LoopOsc:
 	ldr	r0, =SMRDATA
 	ldr	r1, _MTEXT_BASE
 	sub	r0, r0, r1
-	add	r2, r0, #80
+	ldr	r2, =SMRDATAE
+	sub	r2, r2, r1
 pllloop:
 	/* the address */
 	ldr	r1, [r0], #4
@@ -83,7 +84,8 @@  lock:
 	ldr	r0, =SMRDATA1
 	ldr	r1, _MTEXT_BASE
 	sub	r0, r0, r1
-	add	r2, r0, #176
+	ldr	r2, =SMRDATA1E
+	sub	r2, r2, r1
 sdinit:
 	/* the address */
 	ldr	r1, [r0], #4
@@ -114,6 +116,7 @@  SMRDATA:
 	.word CONFIG_SYS_PLLBR_VAL
 	.word AT91_ASM_PMC_MCKR
 	.word CONFIG_SYS_MCKR_VAL
+SMRDATAE:	
 	/* here there's a delay */
 SMRDATA1:
 	.word AT91_ASM_PIOC_ASR
@@ -160,5 +163,6 @@  SMRDATA1:
 	.word CONFIG_SYS_SDRC_MR_VAL3
 	.word CONFIG_SYS_SDRAM
 	.word CONFIG_SYS_SDRAM_VAL
+SMRDATA1E:		
 	/* SMRDATA1 is 176 bytes long */
 #endif /* CONFIG_SKIP_LOWLEVEL_INIT */