<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<tt>Thanks, Phil! <br>
See my answers in-line.</tt><tt><br>
<br>
</tt><tt>Khem, Richard, Mark, can you please review and comment on
this new <br>
version of the patch? </tt><br>
<br>
<div class="moz-cite-prefix">On 03/29/2016 01:10 AM, Phil Blundell
wrote:<br>
</div>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">On Mon, 2016-03-28 at 18:29 +0300, Daniel Dragomir wrote:
</pre>
<blockquote type="cite">
<pre wrap="">For AArch64 state, the default tune is aarch32 and include which
include just the aarch64 feature.
</pre>
</blockquote>
<pre wrap="">
I don't really understand what this sentence is trying to say. Can you
re-phrase it so as to be more accessible to non-experts?
</pre>
</blockquote>
<br>
<tt>Sorry. </tt><tt><br>
</tt>
<pre wrap="">For AArch64 state, the default tune is aarch32 and include just the aarch64 feature.</pre>
<br>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap=""> meta/conf/machine/include/arm/arch-arm64.inc | 36 -------
meta/conf/machine/include/arm/arch-armv8.inc | 1 -
</pre>
</blockquote>
<pre wrap="">
If you are deleting existing files, please mention that in the commit
message. And in particular, if you are removing a file that supports
generic ARMv8 (if imperfectly) and replacing it with something that is
specific to ARMv8-A, please explain why this is a good thing.</pre>
</blockquote>
<br>
<tt>OK, I'll add a comment.<br>
Before, arch-armv8.inc which apparently add support for generic
armv8, only <br>
include arch-arm64.inc, so it's for 64-bit.<br>
But just armv8-A supports 32 and 64 states... armv8-R and armv8-M
are just <br>
for 32. So it wasn't generic. Details here:<br>
</tt><tt><a class="moz-txt-link-freetext" href="http://www.arm.com/products/processors/instruction-set-architectures/armv8-r-architecture.php">http://www.arm.com/products/processors/instruction-set-architectures/armv8-r-architecture.php</a></tt><br>
<br>
<tt>arch-arm8a.inc add now support for armv8-A processors which
support 2 states</tt><tt>:<br>
32-bit and 64-bit states. It makes more sense to have support for
both states<br>
in the same inc file because both refer to the same arch. <br>
</tt><br>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+TUNEVALID[crc] = "Enable CRC instructions for ARMv8-a"
+ARMPKGSFX_FPU .= <a class="moz-txt-link-rfc2396E" href="mailto:${@bb.utils.contains('TUNE_FEATURES','crc','-crc','',d)}">"${@bb.utils.contains('TUNE_FEATURES', 'crc', '-crc', '', d)}"</a>
+ARMPKGSFX_FPU_64 = <a class="moz-txt-link-rfc2396E" href="mailto:${@bb.utils.contains('TUNE_FEATURES','crc','-crc','',d)}">"${@bb.utils.contains('TUNE_FEATURES', 'crc', '-crc', '', d)}"</a>
</pre>
</blockquote>
<pre wrap="">
Why is this involved with ARMPKGSFX_FPU? The crc instructions are not
related to the fpu. Also, the fact that you need to duplicate both this
and the crypto one for both FPU and FPU_64 seems like an indication that
something elsewhere is misdesigned.</pre>
</blockquote>
<tt><br>
You're right. I'll add a new suffix variable for crc and crypto
witch will<br>
be use for both 32 and 64 tunes.<br>
<br>
</tt><tt>ARMPKGSFX_EXT - This is the EXTENSIONS specific suffix.
The suffix </tt><tt><br>
</tt><tt>indicates specific extentions enabled. 'crc' and 'crypto'
are defined.</tt><tt><br>
</tt><tt>Curently it is used just for armv8a architecture.</tt><br>
<br>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+TUNEVALID[fp-armv8] = "Enable ARMv8 Vector Floating Point unit."
+ARMPKGSFX_FPU .= <a class="moz-txt-link-rfc2396E" href="mailto:${@bb.utils.contains('TUNE_FEATURES','fp-armv8','-fp-armv8','',d)}">"${@bb.utils.contains('TUNE_FEATURES', 'fp-armv8', '-fp-armv8', '', d)}"</a>
</pre>
</blockquote>
<pre wrap="">
I don't entirely understand why this one doesn't have an FPU_64
equivalent. Are you always enabling vfp for A64?</pre>
</blockquote>
<tt><br>
</tt><tt>Yes. Floating point is enabled by default. From
documentation:</tt><tt><br>
</tt><samp><span class="samp">"fp</span></samp><tt> - Enable
floating-point instructions. This is on by default for all
possible
values for options </tt><samp><span class="option">-march</span></samp><tt>
and </tt><samp><span class="option">-mcpu</span></samp><tt>."<br>
<a class="moz-txt-link-freetext" href="https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html">https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html</a><br>
<br>
</tt>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+MACHINEOVERRIDES =. <a class="moz-txt-link-rfc2396E" href="mailto:${@bb.utils.contains('TUNE_FEATURES','aarch32','aarch32:','',d)}">"${@bb.utils.contains('TUNE_FEATURES', 'aarch32', 'aarch32:', '' ,d)}"</a>
+MACHINEOVERRIDES .= <a class="moz-txt-link-rfc2396E" href="mailto:${@bb.utils.contains('TUNE_FEATURES','aarch64',':aarch64','',d)}">"${@bb.utils.contains('TUNE_FEATURES', 'aarch64', ':aarch64', '' ,d)}"</a>
</pre>
</blockquote>
<pre wrap="">
As previously discussed, I am not wild about "aarch32" as the name of an
override which really means "ARMv8 in AArch32 state". I know there is a
school of thought that says that the execution state of older cpus is
not AArch32 even if in practice it is indistinguishable, but this
doesn't seem to match either the expectations that a rational user of OE
is likely to have, or the information in the published literature.
</pre>
</blockquote>
<br>
<tt>I changed armv8a with aarch32 because Khem Raj asked for a
previous version<br>
of the patch and in uther discussions related... <br>
<a class="moz-txt-link-freetext" href="http://lists.openembedded.org/pipermail/openembedded-core/2016-March/118904.html">http://lists.openembedded.org/pipermail/openembedded-core/2016-March/118904.html</a><br>
<br>
So, you say that we should not use aarch32 and aarch64 for armv8a,
but <br>
to use something like armv8a64 and armv8a32?<br>
I have no problem with this, but we need </tt><tt><span
id="result_box" class="short_text" lang="en"><span
class="alt-edited">to take a decision agreed by everyone.<br>
It's the second time I change the tune names :)<br>
<br>
</span></span></tt>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+ARMPKGARCH_tune-aarch64 ?= "aarch64"
</pre>
</blockquote>
<pre wrap="">
This seems a tiny bit short-sighted since presumably there will be an
ARMv9 (or ARMv8.2) at some point. What will ARMPKGARCH for A64 be then?</pre>
</blockquote>
<tt><br>
</tt><tt>Maybe with </tt><tt>armv8a64 and armv8a32 this will be
solved.</tt><br>
<br>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+BASE_LIB_tune-aarch64 = "lib64"
</pre>
</blockquote>
<pre wrap="">
This seems like more a matter of distro policy and I'm not sure it
belongs in a tune file. If it does then can't you rely on the aarch64
MACHINEOVERRIDE and just write "BASE_LIB_aarch64" rather than having to
duplicate this for all four of the tunes (and the -be equivalents)?
</pre>
</blockquote>
<tt></tt><br>
<pre wrap="">BASE_LIB is defined also in many other tune files without any overwrites.
It was in aarch64.inc and I simply copied here.
It need to be used with suffix... see in Readme:
BASE_LIB_tune-<tune> - The "/lib" location for a specific ABI. This is
used in a multilib configuration to place the libraries in the correct,
non-conflicting locations.
And this is how it's used in multilib.conf
baselib = <a class="moz-txt-link-rfc2396E" href="mailto:${@d.getVar('BASE_LIB_tune-'+(d.getVar('DEFAULTTUNE',True)or'INVALID'),True)ord.getVar('BASELIB',True)}">"${@d.getVar('BASE_LIB_tune-' + (d.getVar('DEFAULTTUNE', True) or 'INVALID'), True) or d.getVar('BASELIB', True)}"</a>
</pre>
<tt>I can use ${BASE_LIB_tune-aarch64} for all the other tunes to be
easier <br>
to override.<br>
</tt><tt><br>
</tt>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
</pre>
<blockquote type="cite">
<pre wrap="">+# Duplicated from arch-arm.inc
</pre>
</blockquote>
<pre wrap="">
Please add a comment explaining why you can't include arch-arm.inc.
</pre>
</blockquote>
<br>
<tt>I took the content from arch-arm64.inc and add it here. This
duplication <br>
was done in the first commit that add the </tt><tt><tt>arch-arm64.inc
file</tt>.<br>
So, I don't know exactly the reason. Maybe this was discussed on
the first<br>
implementation of aarch64 tune made by Mark.<br>
<br>
Daniel<br>
</tt><br>
<blockquote cite="mid:1459203003.2322.29.camel@pbcl.net" type="cite">
<pre wrap="">
p.
</pre>
</blockquote>
<br>
</body>
</html>