From mboxrd@z Thu Jan 1 00:00:00 1970 From: Albert ARIBAUD Date: Sun, 31 Jan 2016 16:00:35 +0100 Subject: [U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs In-Reply-To: <9026814FBF99304F9FA3AC3FB72F3E2F04C2C8D66A@SAFEX1MAIL4.st.com> References: <1453171978-30715-1-git-send-email-vikas.manocha@st.com> <20160129181546.5e427a86@lilith> <9026814FBF99304F9FA3AC3FB72F3E2F04C2C8D66A@SAFEX1MAIL4.st.com> Message-ID: <20160131160035.083b6dae@lilith> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Vikas, On Sat, 30 Jan 2016 00:36:55 +0100, Vikas MANOCHA 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 > > 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.