Thanks, Phil!
See my answers in-line.
Khem, Richard, Mark, can you please review and comment on this new
version of the patch?
On 03/29/2016 01:10 AM, Phil Blundell wrote:
On Mon, 2016-03-28 at 18:29 +0300, Daniel Dragomir wrote:
For AArch64 state, the default tune is aarch32 and include which
include just the aarch64 feature.
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?
Sorry.
For AArch64 state, the default tune is aarch32 and include just the aarch64
feature.
meta/conf/machine/include/arm/arch-arm64.inc | 36 -------
meta/conf/machine/include/arm/arch-armv8.inc | 1 -
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.
OK, I'll add a comment.
Before, arch-armv8.inc which apparently add support for generic armv8, only
include arch-arm64.inc, so it's for 64-bit.
But just armv8-A supports 32 and 64 states... armv8-R and armv8-M are just
for 32. So it wasn't generic. Details here:
http://www.arm.com/products/processors/instruction-set-architectures/armv8-r-architecture.php
arch-arm8a.inc add now support for armv8-A processors which support 2
states:
32-bit and 64-bit states. It makes more sense to have support for both
states
in the same inc file because both refer to the same arch.
+TUNEVALID[crc] = "Enable CRC instructions for ARMv8-a"
+ARMPKGSFX_FPU .= "${@bb.utils.contains('TUNE_FEATURES', 'crc', '-crc', '', d)}"
+ARMPKGSFX_FPU_64 = "${@bb.utils.contains('TUNE_FEATURES', 'crc', '-crc', '',
d)}"
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.
You're right. I'll add a new suffix variable for crc and crypto witch will
be use for both 32 and 64 tunes.
ARMPKGSFX_EXT - This is the EXTENSIONS specific suffix. The suffix
indicates specific extentions enabled. 'crc' and 'crypto' are defined.
Curently it is used just for armv8a architecture.
+TUNEVALID[fp-armv8] = "Enable ARMv8 Vector Floating Point unit."
+ARMPKGSFX_FPU .= "${@bb.utils.contains('TUNE_FEATURES', 'fp-armv8', '-fp-armv8',
'', d)}"
I don't entirely understand why this one doesn't have an FPU_64
equivalent. Are you always enabling vfp for A64?
Yes. Floating point is enabled by default. From documentation:
"fp- Enable floating-point instructions. This is on by default for all
possible values for options -marchand -mcpu."
https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html
+MACHINEOVERRIDES =. "${@bb.utils.contains('TUNE_FEATURES', 'aarch32', 'aarch32:',
'' ,d)}"
+MACHINEOVERRIDES .= "${@bb.utils.contains('TUNE_FEATURES', 'aarch64', ':aarch64',
'' ,d)}"
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.
I changed armv8a with aarch32 because Khem Raj asked for a previous version
of the patch and in uther discussions related...
http://lists.openembedded.org/pipermail/openembedded-core/2016-March/118904.html
So, you say that we should not use aarch32 and aarch64 for armv8a, but
to use something like armv8a64 and armv8a32?
I have no problem with this, but we need to take a decision agreed by
everyone.
It's the second time I change the tune names :)
+ARMPKGARCH_tune-aarch64 ?= "aarch64"
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?
Maybe with armv8a64 and armv8a32 this will be solved.
+BASE_LIB_tune-aarch64 = "lib64"
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)?
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 = "${@d.getVar('BASE_LIB_tune-' + (d.getVar('DEFAULTTUNE', True) or
'INVALID'), True) or d.getVar('BASELIB', True)}"
I can use ${BASE_LIB_tune-aarch64} for all the other tunes to be easier
to override.
+# Duplicated from arch-arm.inc
Please add a comment explaining why you can't include arch-arm.inc.
I took the content from arch-arm64.inc and add it here. This duplication
was done in the first commit that add the arch-arm64.inc file.
So, I don't know exactly the reason. Maybe this was discussed on the first
implementation of aarch64 tune made by Mark.
Daniel
p.
--
_______________________________________________
Openembedded-core mailing list
Openembedded-core@lists.openembedded.org
http://lists.openembedded.org/mailman/listinfo/openembedded-core
