Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: ldelf and TAs can rely on CFLAGS32/CFLAGS64 #4829

Merged
merged 1 commit into from
Sep 2, 2021

Conversation

etienne-lms
Copy link
Contributor

Defines arch-bits-$(sm) for ldelf and intree TAs sub components
so that they can build using CFLAGS32 (or CFLAGS64) directives
possibly passed by the build environment.

Defines arch-bits-ta_arm32 (resp. 64) in TA devkit to leverage
CFLAGS32 (reps. CFLAGS64) directive passed by the build process. This
change is needed for external package willing to pass specific
directive to TA build sequence as toolchain's sysroot path.

Signed-off-by: Etienne Carriere etienne.carriere@linaro.org

@@ -2,6 +2,13 @@ include mk/cleanvars.mk
sm := $(lastword $(subst /, ,$(dir $(ta-mk-file))))
sm-$(sm) := y

ifeq ($(ta-target),ta_arm32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work? ta-target is reassigned below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed this works. Maybe not for the good reasons :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about replacing with with

ifeq ($(sm),ta_arm32)
arch-bits-ta_arm32 := 32
endif
ifeq ($(sm),ta_arm64)
arch-bits-ta_arm64 := 64
endif

below the assignment to be more clear and do it the same way as in ta/mk/ta_dev_kit.mk

ta-target := $(strip $(if $(CFG_USER_TA_TARGET_$(sm)), \
$(filter $(CFG_USER_TA_TARGET_$(sm)), $(ta-targets)), \
$(default-user-ta-target)))

ifeq ($(ta-target),ta_arm32)
arch-bits-$(sm) := 32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're checking $(ta-target), but then you're using $(sm). I can't make sense of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sm, from build-user-ta.mk is the in-tree TA name (avb, pkcs11, ...).
As build process uses CFLAGS$(arch-bits-$(sm)) (to refer to CFLAGS32/CFLAGS64), we must set arch-bits-$(sm) for each TA name.

External TAs use ta_dev_kit.mk, where sm is ta_arm32 or ta_arm64, so a different path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, thanks for the clarification. It might be worth adding a comment like:
# $(sm) here is the name of the binary being built, for instance "avb" or "pkcs11"

@jenswi-linaro
Copy link
Contributor

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

ldelf/ldelf.mk Outdated Show resolved Hide resolved
ta/mk/ta_dev_kit.mk Outdated Show resolved Hide resolved
Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!
Reviewed-by: Jerome Forissier <jerome@forissier.org>

Defines arch-bits-$(sm) for ldelf and intree TAs sub components
so that they can build using CFLAGS32 (or CFLAGS64) directives
possibly passed by the build environment.

Defines arch-bits-ta_arm32 (resp. 64) in TA devkit to leverage
CFLAGS32 (reps. CFLAGS64) directive passed by the build process. This
change is needed for external package willing to pass specific
directive to TA build sequence as toolchain's sysroot path.

Adds an inline comment describing $(sm) value in the in tree TAs build
instructions.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Jerome Forissier <jerome@forissier.org>
@etienne-lms
Copy link
Contributor Author

Squashed, commit log updated, review tags applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants