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