[PATCH] Fix data abort in startup for at91 machines based on ARM926EJS
Eugen.Hristev at microchip.com
Eugen.Hristev at microchip.com
Mon Mar 22 09:56:41 CET 2021
Hello Martin,
Okay, I get it. Thanks for testing this.
I applied your patch as it was original, and I updated a bit the commit
short message to match requirements.
also,
Reviewed-by: Eugen Hristev <eugen.hristev at microchip.com>
Applied to u-boot-atmel/next
On 3/20/21 1:07 PM, Martin Townsend wrote:
> Hi Eugen,
>
> It didn't work I'm afraid. I took a look at the code and I don't think
> the stack is available, in start.S we have the following for the reset
> vector:
>
> reset:
> /*
> * set the cpu to SVC32 mode
> */
> mrs r0,cpsr
> bic r0,r0,#0x1f
> orr r0,r0,#0xd3
> msr cpsr,r0
>
> /*
> * we do sys-critical inits only at reboot,
> * not when booting from ram!
> */
> #ifndef CONFIG_SKIP_LOWLEVEL_INIT
> bl cpu_init_crit
> #endif
>
> bl _main
>
> cpu_init_crit calls lowlevel_init but the stack is not setup until _main
> is executed
>
> ENTRY(_main)
>
> /*
> * Set up initial C runtime environment and call board_init_f(0).
> */
>
> #if defined(CONFIG_TPL_BUILD) && defined(CONFIG_TPL_NEEDS_SEPARATE_STACK)
> ldr r0, =(CONFIG_TPL_STACK)
> #elif defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
> ldr r0, =(CONFIG_SPL_STACK)
> #else
> ldr r0, =(CONFIG_SYS_INIT_SP_ADDR)
> #endif
> bic r0, r0, #7 /* 8-byte alignment for ABI compliance */
> mov sp, r0
>
> which kind of makes sense as the lowlevel_init will initialise the SDRAM
> that the stack requires.
>
> <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:* 19 March 2021 06:49
> *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
> Yes, please, try like this, continue to use r4 as it was done initially,
> but save r4 to stack and restore before returning. Let's see if this
> solution works for you.
> I will wait for your v2 of the patch.
>
> Thanks !
> Eugen
>
> On 3/18/21 10:31 PM, Martin Townsend wrote:
>> Hi Eugen,
>>
>> Pop and push gets my vote as it future proofs the code from usage of r6
>> in lowlevel_init. Let me know if you want me to respin the patch with
>> this and test it out on my board here.
>>
>> 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:* 18 March 2021 16:53
>> *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
>> 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