[PATCH] Fix data abort in startup for at91 machines based on ARM926EJS
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(-)
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:
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
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
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
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
and r3, r4, r3
and r3, r6, r3 cmp r3, #AT91_PMC_IXR_MCKRDY bne MCKRDY_Loop1-- 2.25.1
Kind regards
Martin
Martin Townsend | Embedded Software Engineer
martin@rufilla.commailto:martin@rufilla.com
W http://www.rufilla.comhttp://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 Policyhttp://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
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
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
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
and r3, r4, r3
and r3, r6, r3 cmp r3, #AT91_PMC_IXR_MCKRDY bne MCKRDY_Loop1-- 2.25.1
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.commailto:martin@rufilla.com
W http://www.rufilla.comhttp://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 Policyhttp://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
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
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
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
and r3, r4, r3
and r3, r6, r3 cmp r3, #AT91_PMC_IXR_MCKRDY bne MCKRDY_Loop1-- 2.25.1
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
________________________________ 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
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
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
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
and r3, r4, r3
and r3, r6, r3 cmp r3, #AT91_PMC_IXR_MCKRDY bne MCKRDY_Loop1-- 2.25.1
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
participants (2)
-
Eugen.Hristev＠microchip.com -
Martin Townsend