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

[Vikas MANOCHA]

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 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/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.