Re: [PATCH 1/3] Add ARM tune file overhaul based largely on work from Mark Hatle

[Phil Blundell <philb@gnu.org>]

On Tue, 2011-07-26 at 13:44 +0100, Richard Purdie wrote:
> +TARGET_FPU = "${@d.getVar('ARMPKGSFX_FPU', True).strip('-') or 'soft'}"

This seems a bit backwards.  Shouldn't TARGET_FPU be the primary
variable and then the package suffix be computed from that, rather than
vice versa?

> +ARMPKGSFX_THUMB .= "${@bb.utils.contains("TUNE_FEATURES", [ "armv4", "thumb" ], "t", "", d)}"
> +ARMPKGSFX_THUMB .= "${@bb.utils.contains("TUNE_FEATURES", [ "armv5", "thumb" ], "t", "", d)}"
> +ARMPKGSFX_THUMB .= "${@bb.utils.contains("TUNE_FEATURES", [ "armv6", "thumb" ], "t2", "", d)}"
> +ARMPKGSFX_THUMB .= "${@bb.utils.contains("TUNE_FEATURES", [ "armv7", "thumb" ], "t2", "", d)}"

This is wrong: ARMv6 doesn't imply Thumb-2.

> +# Whether to compile with code to allow interworking between the two
> +# instruction sets. This allows thumb code to be executed on a primarily
> +# arm system and vice versa. It is strongly recommended that DISTROs not
> +# turn this off - the actual cost is very small.
> +TUNEVALID[no-thumb-interwork] = "Disable mixing of thumb and ARM functions"
> +TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "no-thumb-interwork", "-mno-thumb-interwork", "-mthumb-interwork", d)}"
> +OVERRIDES .= "${@bb.utils.contains("TUNE_FEATURES", "no-thumb-interwork", ":thumb-interwork", "", d)}"

This is only relevant for v4t, I guess.  Interworking is basically
always on for v5 and later and (CeSI aside) it's impossible on v4, so
hardly anybody is going to be flipping this switch.  I'm not sure it
really merits an OVERRIDE.

> --- a/meta/conf/machine/include/tune-xscale.inc
> +++ b/meta/conf/machine/include/tune-xscale.inc
> @@ -1,11 +1,17 @@
> -require conf/machine/include/arm/arch-arm.inc
> +DEFAULTTUNE ?= "xscale"
>  
> -INHERIT += "siteinfo"
> +require conf/machine/include/arm/arch-armv5-dsp.inc
>  
> -TUNE_CCARGS = "-march=armv5te -mtune=xscale"
> -TARGET_CC_KERNEL_ARCH = "-march=armv5te -mtune=xscale"
> -TUNE_PKGARCH = "${@['armv5teb', 'armv5te'][bb.data.getVar('SITEINFO_ENDIANESS', d, 1) == 'le']}"
> -PACKAGE_EXTRA_ARCHS = "${@['armeb armv4b armv4tb armv5teb', 'arm armv4 armv4t armv5te'][bb.data.getVar('SITEINFO_ENDIANESS', d, 1) == 'le']}"
> +TUNEVALID[xscale] = "Enable PXA255/PXA26x Xscale specific processor optimizations"
> +TUNE_CCARGS += "${@bb.utils.contains("TUNE_FEATURES", "xscale", "-mtune=xscale", "", d)}"
> +
> +AVAILTUNES += "xscale"
> +TUNE_FEATURES_tune-xscale = "${TUNE_FEATURES_tune-armv5te} xscale"
> +PACKAGE_EXTRA_ARCHS_tune-xscale = "${PACKAGE_EXTRA_ARCHS_tune-armv5te}"
> +
> +AVAILTUNES += "xscale-be"
> +TUNE_FEATURES_tune-xscale = "${TUNE_FEATURES_tune-armv5teb} xscale"
> +PACKAGE_EXTRA_ARCHS_tune-xscale = "${PACKAGE_EXTRA_ARCHS_tune-armv5teb}"

I guess that should be "_tune-xscale-be".

All in all it seems as though there's an awful lot of expanded cross
products in this set of patches and I can't help wondering whether a lot
of this stuff would be better computed programmatically.  I wouldn't be
at all surprised if there are other copy-and-paste errors like the
xscale one lurking in that mass of overrides, but it's very hard to spot
them by eye.  It seems particularly unfortunate that everything has to
be written out twice, once for big-endian and once for little-endian,
given that endianness is almost entirely orthogonal to all the other
"tuning" parameters.

p.