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**
> 
> _mar...@rufilla.com <mailto:mar...@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.hris...@microchip.com <eugen.hris...@microchip.com>
> *Sent:* 18 March 2021 16:53
> *To:* Martin Townsend <mar...@rufilla.com>; u-boot@lists.denx.de 
> <u-boot@lists.denx.de>
> *Cc:* albert.u.b...@aribaud.net <albert.u.b...@aribaud.net>; 
> nicolas.fe...@microchip.com <nicolas.fe...@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**
>> 
>> _mar...@rufilla.com <mailto:mar...@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.hris...@microchip.com <eugen.hris...@microchip.com>
>> *Sent:* 01 March 2021 12:17
>> *To:* Martin Townsend <mar...@rufilla.com>; u-boot@lists.denx.de 
>> <u-boot@lists.denx.de>
>> *Cc:* albert.u.b...@aribaud.net <albert.u.b...@aribaud.net>; 
>> nicolas.fe...@microchip.com <nicolas.fe...@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 <mar...@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
>>> 
>> 
> 

Reply via email to