* [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
@ 2016-01-19 2:52 Vikas Manocha
2016-01-19 3:11 ` Simon Glass
2016-01-29 17:15 ` Albert ARIBAUD
0 siblings, 2 replies; 6+ messages in thread
From: Vikas Manocha @ 2016-01-19 2:52 UTC (permalink / raw)
To: u-boot
BIC instruction to clear the SP is not allowed in armv7m & is deprecated
in ARMv6T2 & above. This patch cleans the code by using instructions allowed
for armv7m as well as other Arm archs.
Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
---
arch/arm/lib/crt0.S | 25 +++++++------------------
1 file changed, 7 insertions(+), 18 deletions(-)
diff --git a/arch/arm/lib/crt0.S b/arch/arm/lib/crt0.S
index 2f4c14e..3731d0c 100644
--- a/arch/arm/lib/crt0.S
+++ b/arch/arm/lib/crt0.S
@@ -71,18 +71,12 @@ ENTRY(_main)
*/
#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_STACK)
- ldr sp, =(CONFIG_SPL_STACK)
+ ldr r0, =(CONFIG_SPL_STACK)
#else
- ldr sp, =(CONFIG_SYS_INIT_SP_ADDR)
+ ldr r0, =(CONFIG_SYS_INIT_SP_ADDR)
#endif
-#if defined(CONFIG_CPU_V7M) /* v7M forbids using SP as BIC destination */
- mov r3, sp
- bic r3, r3, #7
- mov sp, r3
-#else
- bic sp, sp, #7 /* 8-byte alignment for ABI compliance */
-#endif
- mov r0, sp
+ bic r0, r0, #7 /* 8-byte alignment for ABI compliance */
+ mov sp, r0
bl board_init_f_alloc_reserve
mov sp, r0
/* set up gd here, outside any C code */
@@ -100,14 +94,9 @@ ENTRY(_main)
* 'here' but relocated.
*/
- ldr sp, [r9, #GD_START_ADDR_SP] /* sp = gd->start_addr_sp */
-#if defined(CONFIG_CPU_V7M) /* v7M forbids using SP as BIC destination */
- mov r3, sp
- bic r3, r3, #7
- mov sp, r3
-#else
- bic sp, sp, #7 /* 8-byte alignment for ABI compliance */
-#endif
+ ldr r0, [r9, #GD_START_ADDR_SP] /* sp = gd->start_addr_sp */
+ bic r0, r0, #7
+ mov sp, r0 /* 8-byte alignment for ABI compliance */
ldr r9, [r9, #GD_BD] /* r9 = gd->bd */
sub r9, r9, #GD_SIZE /* new GD is below bd */
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
2016-01-19 2:52 [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs Vikas Manocha
@ 2016-01-19 3:11 ` Simon Glass
2016-01-29 17:15 ` Albert ARIBAUD
1 sibling, 0 replies; 6+ messages in thread
From: Simon Glass @ 2016-01-19 3:11 UTC (permalink / raw)
To: u-boot
On 18 January 2016 at 19:52, Vikas Manocha <vikas.manocha@st.com> wrote:
> BIC instruction to clear the SP is not allowed in armv7m & is deprecated
> in ARMv6T2 & above. This patch cleans the code by using instructions allowed
> for armv7m as well as other Arm archs.
>
> Signed-off-by: Vikas Manocha <vikas.manocha@st.com>
> ---
> arch/arm/lib/crt0.S | 25 +++++++------------------
> 1 file changed, 7 insertions(+), 18 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
(not tested :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
2016-01-19 2:52 [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs Vikas Manocha
2016-01-19 3:11 ` Simon Glass
@ 2016-01-29 17:15 ` Albert ARIBAUD
2016-01-29 23:36 ` Vikas MANOCHA
1 sibling, 1 reply; 6+ messages in thread
From: Albert ARIBAUD @ 2016-01-29 17:15 UTC (permalink / raw)
To: u-boot
Hello Vikas,
On Mon, 18 Jan 2016 18:52:57 -0800, Vikas Manocha
<vikas.manocha@st.com> wrote:
> BIC instruction to clear the SP is not allowed in armv7m & is deprecated
> in ARMv6T2 & above. This patch cleans the code by using instructions allowed
> for armv7m as well as other Arm archs.
I am not against this patch, which has merits on its own; but the
commit message above raises a couple of questions.
1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is
incorrectly being used on sp. However, it is *not*; it is used on r3,
precisely because it cannot be used directly on sp, as the comment
before the bic instruction clearly states. Why does the commit
message raise this non-issue?
2. I could not find any information on BIC being deprecated in
Architecture Reference Manuals of either ArmV7 (section "Deprecated
Features in ARMv7-M", or ARMv6-M (section "Deprecated and Obsolete
Features"). Where does this information come from?
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
2016-01-29 17:15 ` Albert ARIBAUD
@ 2016-01-29 23:36 ` Vikas MANOCHA
2016-01-31 15:00 ` Albert ARIBAUD
0 siblings, 1 reply; 6+ messages in thread
From: Vikas MANOCHA @ 2016-01-29 23:36 UTC (permalink / raw)
To: u-boot
Hi Albert,
> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> Sent: Friday, January 29, 2016 9:16 AM
> To: Vikas MANOCHA
> Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen; Przemyslaw
> Marczak
> Subject: Re: [PATCH] arm: use common instructions applicable to armv7m &
> other arm archs
>
> Hello Vikas,
>
> On Mon, 18 Jan 2016 18:52:57 -0800, Vikas Manocha
> <vikas.manocha@st.com> wrote:
> > BIC instruction to clear the SP is not allowed in armv7m & is
> > deprecated in ARMv6T2 & above. This patch cleans the code by using
> > instructions allowed for armv7m as well as other Arm archs.
>
> I am not against this patch, which has merits on its own; but the commit
> message above raises a couple of questions.
>
> 1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is
> incorrectly being used on sp. However, it is *not*; it is used on r3,
> precisely because it cannot be used directly on sp, as the comment
> before the bic instruction clearly states. Why does the commit
> message raise this non-issue?
For sure, BIC is used correctly. Currently BIC instruction is being used on r3 for armv7m & on SP for others.
The patch is to use same instruction for all ISAs & to make code cleaner.
The intention of message " BIC instruction to clear the SP is not allowed in armv7m" is to justify the usage of register (r0) instead of SP for BIC.
Sorry if it created some confusion, how about following commit message for v2:
"This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.
Just for information, BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above"
>
> 2. I could not find any information on BIC being deprecated in
> Architecture Reference Manuals of either ArmV7 (section "Deprecated
> Features in ARMv7-M", or ARMv6-M (section "Deprecated and Obsolete
> Features"). Where does this information come from?
I got it from some of the web locations, one of them is:
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473j/dom1361289864906.html
Cheers,
Vikas
>
> Amicalement,
> --
> Albert.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
2016-01-29 23:36 ` Vikas MANOCHA
@ 2016-01-31 15:00 ` Albert ARIBAUD
2016-02-01 23:48 ` Vikas MANOCHA
0 siblings, 1 reply; 6+ messages in thread
From: Albert ARIBAUD @ 2016-01-31 15:00 UTC (permalink / raw)
To: u-boot
Hello Vikas,
On Sat, 30 Jan 2016 00:36:55 +0100, Vikas MANOCHA
<vikas.manocha@st.com> wrote:
> Hi Albert,
>
> > -----Original Message-----
> > From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> > Sent: Friday, January 29, 2016 9:16 AM
> > To: Vikas MANOCHA
> > Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen; Przemyslaw
> > Marczak
> > Subject: Re: [PATCH] arm: use common instructions applicable to armv7m &
> > other arm archs
> >
> > Hello Vikas,
> >
> > On Mon, 18 Jan 2016 18:52:57 -0800, Vikas Manocha
> > <vikas.manocha@st.com> wrote:
> > > BIC instruction to clear the SP is not allowed in armv7m & is
> > > deprecated in ARMv6T2 & above. This patch cleans the code by using
> > > instructions allowed for armv7m as well as other Arm archs.
> >
> > I am not against this patch, which has merits on its own; but the commit
> > message above raises a couple of questions.
> >
> > 1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is
> > incorrectly being used on sp. However, it is *not*; it is used on r3,
> > precisely because it cannot be used directly on sp, as the comment
> > before the bic instruction clearly states. Why does the commit
> > message raise this non-issue?
>
> For sure, BIC is used correctly. Currently BIC instruction is being used on r3 for armv7m & on SP for others.
> The patch is to use same instruction for all ISAs & to make code cleaner.
> The intention of message " BIC instruction to clear the SP is not allowed in armv7m" is to justify the usage of register (r0) instead of SP for BIC.
> Sorry if it created some confusion, how about following commit message for v2:
>
> "This patch cleans the code by using instructions allowed for armv7m as well as other Arm archs.
> Just for information, BIC instruction to clear the SP is not allowed in armv7m & is deprecated in ARMv6T2 & above"
Almost good, but... the current code /already/ uses instructions
allowed for ARMv7-M. It even has a conditional on CONFIG_CPU_V7M under
which all instructions are ARMv7-compatible. Proof is, ARMv7 actually
builds (and runs fine).
Granted, if someone built for ARMv7-M but with CONFIG_CPU_V7M unset,
then the assembler would fail on the "BIC sp" instruction, which is
indeed forbidden in ARMv7-M; but such a build cannot happen unless
that same someone actively *circumvented* the U-Boot build process.
What your patch does is therefore definitely not "Fix bad use of
BIC which breaks ARMv7", and not even "fix use of BIC that might cause
ARMv7 to break". Your patch is, however, "Streamline code and remove
ARMv7-M conditional by using only ARMv7-M-allowed instructions" -- and
this it does pretty well.
I would therefore completely drop the part about BIC from the commit
message as it is not actually a problem in the current code -- or more
to the point, as its limitations were already acknowledged and accounted
for in the current code.
However, if the bit about BIC really had to stay (with will require a
very strong reason), it should be corrected, because of the following:
> >
> > 2. I could not find any information on BIC being deprecated in
> > Architecture Reference Manuals of either ArmV7 (section "Deprecated
> > Features in ARMv7-M", or ARMv6-M (section "Deprecated and Obsolete
> > Features"). Where does this information come from?
>
> I got it from some of the web locations, one of them is:
> http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473j/dom1361289864906.html
Firstly, the link above is *not* to the ARMv6-M Architecture Reference
Manual, but from the documentation of an ARM tool, namely the ARM DS-5
compiler (version 5.04). The only authoritative source to determine if
a feature is deprecated in a given ARM architecture is its Architecture
Reference Manual.
But even of the sentence above was authoritative, what it purports as
deprecated is the use of PC and SP in BIC, not the use of BIC itself,
whereas the proposed commit message might be read by a casual (or
non-casual but not quite attentive enough) reader might as "the BIC
instruction to clear the SP is not allowed in armv7m & the BIC
instruction is deprecated in ARMv6T2 & above".
At the very least, that part of the commit message should be reworded
as "Using BIC on SP (as well as PC) is deprecated in ARMv6T2 and above,
and forbidden in ARMv7-M".
However, my opinion is that the commit is not about fixing how BIC is
used but about getting rid of the ARMv7-M conditional, and therefore,
I think that its message should not mention BIC at all.
I would therefore limit the commit message to just this:
This patch cleans the code by using instructions allowed for
ARMv7-M as well as other Arm architectures.
> Cheers,
> Vikas
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
2016-01-31 15:00 ` Albert ARIBAUD
@ 2016-02-01 23:48 ` Vikas MANOCHA
0 siblings, 0 replies; 6+ messages in thread
From: Vikas MANOCHA @ 2016-02-01 23:48 UTC (permalink / raw)
To: u-boot
Thanks Albert,
> I would therefore limit the commit message to just this:
>
> This patch cleans the code by using instructions allowed for
> ARMv7-M as well as other Arm architectures.
I will update the commit message accordingly & send v2.
Rgds,
Vikas
> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> Sent: Sunday, January 31, 2016 7:01 AM
> To: Vikas MANOCHA
> Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen; Przemyslaw
> Marczak
> Subject: Re: [PATCH] arm: use common instructions applicable to armv7m &
> other arm archs
>
> Hello Vikas,
>
> On Sat, 30 Jan 2016 00:36:55 +0100, Vikas MANOCHA
> <vikas.manocha@st.com> wrote:
> > Hi Albert,
> >
> > > -----Original Message-----
> > > From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> > > Sent: Friday, January 29, 2016 9:16 AM
> > > To: Vikas MANOCHA
> > > Cc: u-boot at lists.denx.de; Simon Glass; rev13 at wp.pl; Bo Shen;
> > > Przemyslaw Marczak
> > > Subject: Re: [PATCH] arm: use common instructions applicable to
> > > armv7m & other arm archs
> > >
> > > Hello Vikas,
> > >
> > > On Mon, 18 Jan 2016 18:52:57 -0800, Vikas Manocha
> > > <vikas.manocha@st.com> wrote:
> > > > BIC instruction to clear the SP is not allowed in armv7m & is
> > > > deprecated in ARMv6T2 & above. This patch cleans the code by using
> > > > instructions allowed for armv7m as well as other Arm archs.
> > >
> > > I am not against this patch, which has merits on its own; but the
> > > commit message above raises a couple of questions.
> > >
> > > 1. It seems to imply that in the current arch/arm/lib/crt0.S, BIC is
> > > incorrectly being used on sp. However, it is *not*; it is used on r3,
> > > precisely because it cannot be used directly on sp, as the comment
> > > before the bic instruction clearly states. Why does the commit
> > > message raise this non-issue?
> >
> > For sure, BIC is used correctly. Currently BIC instruction is being used on r3
> for armv7m & on SP for others.
> > The patch is to use same instruction for all ISAs & to make code cleaner.
> > The intention of message " BIC instruction to clear the SP is not allowed in
> armv7m" is to justify the usage of register (r0) instead of SP for BIC.
> > Sorry if it created some confusion, how about following commit message
> for v2:
> >
> > "This patch cleans the code by using instructions allowed for armv7m as
> well as other Arm archs.
> > Just for information, BIC instruction to clear the SP is not allowed in armv7m
> & is deprecated in ARMv6T2 & above"
>
> Almost good, but... the current code /already/ uses instructions allowed for
> ARMv7-M. It even has a conditional on CONFIG_CPU_V7M under which all
> instructions are ARMv7-compatible. Proof is, ARMv7 actually builds (and runs
> fine).
>
> Granted, if someone built for ARMv7-M but with CONFIG_CPU_V7M unset,
> then the assembler would fail on the "BIC sp" instruction, which is indeed
> forbidden in ARMv7-M; but such a build cannot happen unless that same
> someone actively *circumvented* the U-Boot build process.
>
> What your patch does is therefore definitely not "Fix bad use of BIC which
> breaks ARMv7", and not even "fix use of BIC that might cause
> ARMv7 to break". Your patch is, however, "Streamline code and remove
> ARMv7-M conditional by using only ARMv7-M-allowed instructions" -- and
> this it does pretty well.
>
> I would therefore completely drop the part about BIC from the commit
> message as it is not actually a problem in the current code -- or more to the
> point, as its limitations were already acknowledged and accounted for in the
> current code.
>
> However, if the bit about BIC really had to stay (with will require a very strong
> reason), it should be corrected, because of the following:
>
> > >
> > > 2. I could not find any information on BIC being deprecated in
> > > Architecture Reference Manuals of either ArmV7 (section "Deprecated
> > > Features in ARMv7-M", or ARMv6-M (section "Deprecated and
> Obsolete
> > > Features"). Where does this information come from?
> >
> > I got it from some of the web locations, one of them is:
> > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0473j/d
> > om1361289864906.html
>
> Firstly, the link above is *not* to the ARMv6-M Architecture Reference
> Manual, but from the documentation of an ARM tool, namely the ARM DS-5
> compiler (version 5.04). The only authoritative source to determine if a
> feature is deprecated in a given ARM architecture is its Architecture
> Reference Manual.
>
> But even of the sentence above was authoritative, what it purports as
> deprecated is the use of PC and SP in BIC, not the use of BIC itself, whereas
> the proposed commit message might be read by a casual (or non-casual but
> not quite attentive enough) reader might as "the BIC instruction to clear the
> SP is not allowed in armv7m & the BIC instruction is deprecated in ARMv6T2
> & above".
>
> At the very least, that part of the commit message should be reworded as
> "Using BIC on SP (as well as PC) is deprecated in ARMv6T2 and above, and
> forbidden in ARMv7-M".
>
> However, my opinion is that the commit is not about fixing how BIC is used
> but about getting rid of the ARMv7-M conditional, and therefore, I think that
> its message should not mention BIC at all.
>
> I would therefore limit the commit message to just this:
>
> This patch cleans the code by using instructions allowed for
> ARMv7-M as well as other Arm architectures.
>
> > Cheers,
> > Vikas
>
> Amicalement,
> --
> Albert.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-01 23:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-01-19 2:52 [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs Vikas Manocha
2016-01-19 3:11 ` Simon Glass
2016-01-29 17:15 ` Albert ARIBAUD
2016-01-29 23:36 ` Vikas MANOCHA
2016-01-31 15:00 ` Albert ARIBAUD
2016-02-01 23:48 ` Vikas MANOCHA
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox