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 > <vikas.mano...@st.com> 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 > > > <vikas.mano...@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. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
