[U-Boot] [PATCH] arm: use common instructions applicable to armv7m & other arm archs

[Albert ARIBAUD]

Hello Vikas,

On Sat, 30 Jan 2016 00:36:55 +0100, Vikas MANOCHA
<vikas.manocha at 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 at 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.