Re: [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
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.b...@aribaud.net] > Sent: Sunday, January 31, 2016 7:01 AM > To: Vikas MANOCHA > Cc: u-boot@lists.denx.de; Simon Glass; re...@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 > wrote: > > Hi Albert, > > > > > -Original Message- > > > From: Albert ARIBAUD [mailto:albert.u.b...@aribaud.net] > > > Sent: Friday, January 29, 2016 9:16 AM > > > To: Vikas MANOCHA > > > Cc: u-boot@lists.denx.de; Simon Glass; re...@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 > > > 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 shoul
Re: [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
Hello Vikas, On Sat, 30 Jan 2016 00:36:55 +0100, Vikas MANOCHA wrote: > Hi Albert, > > > -Original Message- > > From: Albert ARIBAUD [mailto:albert.u.b...@aribaud.net] > > Sent: Friday, January 29, 2016 9:16 AM > > To: Vikas MANOCHA > > Cc: u-boot@lists.denx.de; Simon Glass; re...@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 > > 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
Hi Albert, > -Original Message- > From: Albert ARIBAUD [mailto:albert.u.b...@aribaud.net] > Sent: Friday, January 29, 2016 9:16 AM > To: Vikas MANOCHA > Cc: u-boot@lists.denx.de; Simon Glass; re...@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 > 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
Hello Vikas, On Mon, 18 Jan 2016 18:52:57 -0800, Vikas Manocha 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. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
On 18 January 2016 at 19:52, Vikas Manocha 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 > --- > arch/arm/lib/crt0.S | 25 +++-- > 1 file changed, 7 insertions(+), 18 deletions(-) Reviewed-by: Simon Glass (not tested :-) ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs
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 --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
