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