| Message ID | AM7P194MB0756C631FEE7561E8F3D3765D19E9@AM7P194MB0756.EURP194.PROD.OUTLOOK.COM |
|---|---|
| State | Accepted |
| Commit | bbfb81c1876f32b8991cf34cf0fa5525e9f28afd |
| Delegated to: | Eugen Hristev |
| Headers | show |
| Series | Fix data abort in startup for at91 machines based on ARM926EJS | expand |
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 ? Since when this happens ? Do you have a fixes tag for this ? Does it affect any board using the arm926ejs ? Currently we have sam9x60 which is an SoC based on arm 926ejs and we did not see this behavior as of today. 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 Thanks, Eugen > > Signed-off-by: Martin Townsend <martin@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 >
Kind regards Martin Martin Townsend | Embedded Software Engineer martin@rufilla.com<mailto:martin@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>
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@rufilla.com <mailto:martin@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@microchip.com <Eugen.Hristev@microchip.com> > *Sent:* 01 March 2021 12:17 > *To:* Martin Townsend <martin@rufilla.com>; u-boot@lists.denx.de > <u-boot@lists.denx.de> > *Cc:* albert.u.boot@aribaud.net <albert.u.boot@aribaud.net>; > Nicolas.Ferre@microchip.com <Nicolas.Ferre@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@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 >> >
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@rufilla.com<mailto:martin@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>
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@rufilla.com <mailto:martin@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@microchip.com <Eugen.Hristev@microchip.com> > *Sent:* 18 March 2021 16:53 > *To:* Martin Townsend <martin@rufilla.com>; u-boot@lists.denx.de > <u-boot@lists.denx.de> > *Cc:* albert.u.boot@aribaud.net <albert.u.boot@aribaud.net>; > Nicolas.Ferre@microchip.com <Nicolas.Ferre@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@rufilla.com <mailto:martin@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@microchip.com <Eugen.Hristev@microchip.com> >> *Sent:* 01 March 2021 12:17 >> *To:* Martin Townsend <martin@rufilla.com>; u-boot@lists.denx.de >> <u-boot@lists.denx.de> >> *Cc:* albert.u.boot@aribaud.net <albert.u.boot@aribaud.net>; >> Nicolas.Ferre@microchip.com <Nicolas.Ferre@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@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 >>> >> >
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>
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@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@microchip.com <Eugen.Hristev@microchip.com> > *Sent:* 19 March 2021 06:49 > *To:* Martin Townsend <martin@rufilla.com>; u-boot@lists.denx.de > <u-boot@lists.denx.de> > *Cc:* albert.u.boot@aribaud.net <albert.u.boot@aribaud.net>; > Nicolas.Ferre@microchip.com <Nicolas.Ferre@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@rufilla.com <mailto:martin@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@microchip.com <Eugen.Hristev@microchip.com> >> *Sent:* 18 March 2021 16:53 >> *To:* Martin Townsend <martin@rufilla.com>; u-boot@lists.denx.de >> <u-boot@lists.denx.de> >> *Cc:* albert.u.boot@aribaud.net <albert.u.boot@aribaud.net>; >> Nicolas.Ferre@microchip.com <Nicolas.Ferre@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@rufilla.com <mailto:martin@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@microchip.com <Eugen.Hristev@microchip.com> >>> *Sent:* 01 March 2021 12:17 >>> *To:* Martin Townsend <martin@rufilla.com>; u-boot@lists.denx.de >>> <u-boot@lists.denx.de> >>> *Cc:* albert.u.boot@aribaud.net <albert.u.boot@aribaud.net>; >>> Nicolas.Ferre@microchip.com <Nicolas.Ferre@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@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 >>>> >>> >> >
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:
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. Signed-off-by: Martin Townsend <martin@rufilla.com> --- arch/arm/mach-at91/arm926ejs/lowlevel_init.S | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)