[PATCH] Fix data abort in startup for at91 machines based on ARM926EJS
Eugen.Hristev at microchip.com
Eugen.Hristev at microchip.com
Thu Mar 18 17:53:51 CET 2021
Hello Martin,
I reviewed things a bit, and, as it looks to me, and according to AAPCS,
we can still use r4, as this is scratch space, but we need to save it
and restore it before and after the code of lowlevel_init.
Because r6 is also scratch, with your patch , we lose the data in r6, in
case someone was using it before the call.
So , do you agree to change this to : still use r4 for operations, but
push r4 to stack and pop r4 before returning ?
If anyone else has a different opinion, please share.
Thanks,
Eugen
On 3/1/21 2:34 PM, Martin Townsend wrote:
>
> Kind regards
>
> Martin
>
> *Martin Townsend**| *Embedded Software Engineer**
>
> _martin at rufilla.com <mailto:martin at rufilla.com>_
>
>
> **
>
> **
>
> *W***_http://www.rufilla.com <http://www.rufilla.com/>_
>
> *T* +44 (0)1865 601201
>
> *A* Building D5 | Culham Science Centre | Abingdon, UK | OX14 3DB
>
> rufilla_logo_transparent <http://www.rufilla.com/>
>
> Rufilla Ltd is a company registered in England and Wales, No. 7093478.
> Registered address : 6a St Andrew’s Court, Wellington Street, Thame,
> Oxfordshire, OX9 3WT, United Kingdom. Rufilla Ltd cannot guarantee
> e-mail transmission to be secure or error-free as information could be
> intercepted, corrupted, lost, destroyed, arrive late or incomplete, or
> contain viruses. The sender therefore does not accept any liability
> whatsoever for any errors or omissions in the contents of this message
> or that arises from any data corruption, interception or unauthorised
> amendment, whether or not these arise as a result of the e-mail
> transmission. The information on which this communication is based has
> been obtained from sources we believe to be reliable, but we do not
> guarantee its accuracy or completeness. All expressions of opinion are
> subject to change without notice. This email may contain confidential
> information and as such, should be treated as confidential. It may also
> contain legally privileged information. It is intended solely for use by
> the normal or ordinary user of the e-mail address to which it has been
> addressed. If you are not the named addressee of this e-mail you are
> prohibited from disseminating, distributing, amending, copying or
> otherwise acting on the contents, including any attachments, of this
> e-mail. Please notify the sender immediately by e-mail if you have
> received this e-mail in error and immediately delete this e-mail from
> your system. You may be asked to confirm that the e-mail received in
> error has been deleted from your system and/or that you have not made
> any unauthorised use, disclosure, distribution or copy of the e-mail.
> Rufilla Ltd is a VAT registered company No.131363252.
>
> Any and all communications sent to us may be monitored and/or stored by
> us to ensure compliance with relevant legislation, rules, and policies.
> All communications are handled in full compliance with current data
> protection legislation including, but not limited to, EU Regulation
> 2016/679 General Data Protection Regulation (“GDPR”). For further
> information, please refer to our _Privacy Policy
> <http://www.rufilla.com/s/wp-content/uploads/2018/06/GDPR_Privacy_Notice_20180615-FINAL.pdf>_
>
> <http://www.rufilla.com/site/wp-content/uploads/2018/06/GDPR_Privacy_Notice_20180615-FINAL.pdf>
>
>
>
> ------------------------------------------------------------------------
> *From:* Eugen.Hristev at microchip.com <Eugen.Hristev at microchip.com>
> *Sent:* 01 March 2021 12:17
> *To:* Martin Townsend <martin at rufilla.com>; u-boot at lists.denx.de
> <u-boot at lists.denx.de>
> *Cc:* albert.u.boot at aribaud.net <albert.u.boot at aribaud.net>;
> Nicolas.Ferre at microchip.com <Nicolas.Ferre at microchip.com>
> *Subject:* Re: [PATCH] Fix data abort in startup for at91 machines based
> on ARM926EJS
> Hi,
>
> On 26.02.2021 10:44, Martin Townsend wrote:
>> The startup code in arm/cpu/arm926ejs preserves the link register across
>> the call to lowlevel_init by using r4:
>>
>> mov r4, lr /* perserve link reg across call */
>> bl lowlevel_init /* go setup pll,mux,memory */
>> mov lr, r4 /* restore link */
>>
>> The lowlevel_init function for at91 machines based on the same CPU uses r4
>> and hence corrupts it causing a data abort when it returns to the startup
>> code. This patch fixes this by using r6 instead of r4 in the lowlevel_init
>> function.
>>
>> Discovered and the fix was tested on a AT91SAM9261 based board.
>
> Very interesting find. I have a few questions:
> How is this reproducible ?
> I'm getting a custom Ronetix PM9261 board working with a 2020.01 version
> of U-Boot and just noticed that it hung with no console output so I
> attached a JTAG debugger and could see that it was stuck in the data
> abort so I single stepped through the code and could see the problem was
> when it returned from lowlevel_init and that the r4 register had been
> corrupted.
> Since when this happens ?
> I have no idea I'm afraid this is the first board that I'm trying to
> bring up that has this processor architecture.
> Do you have a fixes tag for this ?
> I don't I'm afraid
> Does it affect any board using the arm926ejs ?
> I only have this Ronetix PM9261 with has a AT91SAM9261 SoC. I've had a
> look around and can't see any other boards that I have that use the
> arm926ejs architecture.
> Currently we have sam9x60 which is an SoC based on arm 926ejs and we did
> not see this behavior as of today.
> Does it have the SKIP_LOWLEVEL_INIT defined? (Not sure of the exact name
> but it's something like this) The Ronetix board runs U-Boot from the
> NOR flash using XIP so the startup code runs from NOR and must run the
> lowlevel_init function. Maybe the board you have skips the low level
> intiailisation, just a guess though.
>
> Could you also modify the subject, to adhere with the rules subsystem:
> component: change
>
> In your case it's something like
>
> ARM: at91: arm926esj: fix usage of r4 in lowlevel_init
>
> Sure will do if you think it's a valid bug.
>
> Thanks,
> Eugen
>
>>
>> Signed-off-by: Martin Townsend <martin at rufilla.com>
>> ---
>> arch/arm/mach-at91/arm926ejs/lowlevel_init.S | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/arm926ejs/lowlevel_init.S b/arch/arm/mach-at91/arm926ejs/lowlevel_init.S
>> index 71d7582ce0..994f42eb4a 100644
>> --- a/arch/arm/mach-at91/arm926ejs/lowlevel_init.S
>> +++ b/arch/arm/mach-at91/arm926ejs/lowlevel_init.S
>> @@ -71,10 +71,10 @@ POS1:
>> str r0, [r1]
>>
>> /* Reading the PMC Status to detect when the Main Oscillator is enabled */
>> - mov r4, #AT91_PMC_IXR_MOSCS
>> + mov r6, #AT91_PMC_IXR_MOSCS
>> MOSCS_Loop:
>> ldr r3, [r2]
>> - and r3, r4, r3
>> + and r3, r6, r3
>> cmp r3, #AT91_PMC_IXR_MOSCS
>> bne MOSCS_Loop
>>
>> @@ -89,10 +89,10 @@ MOSCS_Loop:
>> str r0, [r1]
>>
>> /* Reading the PMC Status register to detect when the PLLA is locked */
>> - mov r4, #AT91_PMC_IXR_LOCKA
>> + mov r6, #AT91_PMC_IXR_LOCKA
>> MOSCS_Loop1:
>> ldr r3, [r2]
>> - and r3, r4, r3
>> + and r3, r6, r3
>> cmp r3, #AT91_PMC_IXR_LOCKA
>> bne MOSCS_Loop1
>>
>> @@ -109,10 +109,10 @@ MOSCS_Loop1:
>> str r0, [r1]
>>
>> /* Reading the PMC Status to detect when the Master clock is ready */
>> - mov r4, #AT91_PMC_IXR_MCKRDY
>> + mov r6, #AT91_PMC_IXR_MCKRDY
>> MCKRDY_Loop:
>> ldr r3, [r2]
>> - and r3, r4, r3
>> + and r3, r6, r3
>> cmp r3, #AT91_PMC_IXR_MCKRDY
>> bne MCKRDY_Loop
>>
>> @@ -120,10 +120,10 @@ MCKRDY_Loop:
>> str r0, [r1]
>>
>> /* Reading the PMC Status to detect when the Master clock is ready */
>> - mov r4, #AT91_PMC_IXR_MCKRDY
>> + mov r6, #AT91_PMC_IXR_MCKRDY
>> MCKRDY_Loop1:
>> ldr r3, [r2]
>> - and r3, r4, r3
>> + and r3, r6, r3
>> cmp r3, #AT91_PMC_IXR_MCKRDY
>> bne MCKRDY_Loop1
>> PLL_setup_end:
>> --
>> 2.25.1
>>
>
More information about the U-Boot
mailing list