From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: References: <20200210161550.22562-1-a.fatoum@pengutronix.de> From: Ahmad Fatoum Message-ID: <197244c8-81c0-f328-de8b-d126d768a7cc@pengutronix.de> Date: Wed, 12 Feb 2020 10:28:15 +0100 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Subject: Re: [ptxdist] [PATCH] tf-a: new package for ARM trusted firmware A List-Id: PTXdist Development Mailing List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: ptxdist@pengutronix.de Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ptxdist-bounces@pengutronix.de Sender: "ptxdist" To: Guillermo Rodriguez Garcia Cc: Alejandro Vazquez , ptxdist@pengutronix.de Hi, On 2/12/20 10:07 AM, Guillermo Rodriguez Garcia wrote: >>> Uhm, here I didn't need to specify CROSS_ENV and things seemed to work >>> just fine. >> >> CROSS_ENV sets stuff like CC and LD. I took a look inside the v2.2 TF-A Makefile >> and on line 808 there is a reference to $(CC) before it's defined, > > That would be a bug in the Makefile, and if this was the case, > shouldn't we add a > patch to fix the Makefile (and possibly submit it to upstream), > instead of trying to > work around the bug by setting CROSS_ENV ? > > Anyway I am not sure this is actually a bug -- I see that CC is > defined in line 160 > in v2.2 of the Makefile; that should happen before the usage in line 808. Sorry, see line 93, which happens before the definition. > >> so in that case it >> would come from the environment. It's probably unintended, but to account for >> any other scripts that may reference LD, CC and the like without defining them >> first, I guess it's better to have CROSS_ENV in. Do you agree? > > I guess keeping it can not do any harm. However it can also hide bugs, > so I am not > sure what is best. yes, it's a bug and should be fixed. But this might not be the only instance, so we can either: - Not do anything - Give CC and friends a proper value I think #2 is reasonable. Cheers Ahmad > > Guillermo > >> >>> >>>> >>>>>> +$(STATEDIR)/tf-a.prepare: >>>>>> + @$(call targetinfo) >>>>>> + @$(call touch) >>>>> >>>>> I think this does nothing and can be removed. >>>> >>>> Right. See tf-a.compile below >>>> >>>>> >>>>>> + >>>>>> +# ---------------------------------------------------------------------------- >>>>>> +# Compile >>>>>> +# ---------------------------------------------------------------------------- >>>>>> +TF_A_MAKE_OPT += PLAT=$(PTXCONF_TF_A_PLATFORM) >>>>>> + >>>>>> +TF_A_MAKE_OPT += DEBUG=$(call ptx/ifdef,TF_A_DEBUG,1,0) >>>>>> +TF_A_BUILD_OUTPUT_DIR := $(call ptx/ifdef,TF_A_DEBUG,debug,release) >>>>>> + >>>>>> +TF_A_MAKE_OPT += ARCH=$(PTXCONF_TF_A_ARCH_STRING) >>>>>> +TF_A_MAKE_OPT += LOAD_IMAGE_V2=1 >>> >>> I think this should not be hardcoded. >>> >>> (Sorry, didn't spot this one before) >> >> Will drop it. >> >>> >>>>>> +ifdef PTXCONF_TF_A_BL32_TSP >>>>>> +TF_A_MAKE_OPT += ARM_TSP_RAM_LOCATION=$(PTXCONF_TF_A_BL32_TSP_RAM_LOCATION_STRING) >>>>>> +endif >>>>>> +TF_A_MAKE_OPT += ARM_ARCH_MAJOR=$(PTXCONF_TF_A_ARM_ARCH_MAJOR) >>>>>> +ifdef PTXCONF_TF_A_ARM_ARCH_MINOR >>>>>> +TF_A_MAKE_OPT += ARM_ARCH_MINOR=$(PTXCONF_TF_A_ARM_ARCH_MINOR) >>>>>> +endif >>>>>> + >>>>>> +ifneq ($(PTXCONF_TF_A_DTB),) >>>>>> +TF_A_MAKE_OPT += DTB_FILE_NAME=$(PTXCONF_TF_A_DTB) >>>>>> +endif >>>>>> + >>>>>> +ifdef PTXCONF_TF_A_BL32_SP_MIN >>>>>> +TF_A_MAKE_OPT += AARCH32_SP=sp_min >>>>>> +endif >>>>>> + >>>>>> +TF_A_MAKE_OPT += all >>>>>> + >>>>>> +TF_A_ARTIFACTS = $(addprefix $(TF_A_DIR)/build/$(PTXCONF_TF_A_PLATFORM)/$(TF_A_BUILD_OUTPUT_DIR)/, \ >>>>>> + $(call remove_quotes,$(PTXCONF_TF_A_ARTIFACTS))) >>>>>> + >>>>>> +$(STATEDIR)/tf-a.compile: >>>>>> + @$(call targetinfo) >>>>>> + @rm -rf $(TF_A_DIR)/build/ >>>>>> + @$(call world/compile, TF_A) >>>>>> + @$(call touch) >>>>> >>>>> Can't you use the default compile rule here? >>>> >>>> Yes. I will move the deletion of the build directory to the prepare stage. >>>> Seems like old versions of TF-A had problems when reconfiguring without a clean? >>> >>> Not sure; I have not seen such problems myself. >> >> I've sent a question to the colleague who first added the line. >> Will keep it in for now. >> >>> >>>> >>>>>> + >>>>>> +# ---------------------------------------------------------------------------- >>>>>> +# Install >>>>>> +# ---------------------------------------------------------------------------- >>>>>> + >>>>>> +$(STATEDIR)/tf-a.install: >>>>>> + @$(call targetinfo) >>>>>> + >>>>>> +ifeq ($(TF_A_ARTIFACTS),) >>>>>> + $(warning TF_A_ARTIFACTS is empty. nothing to install.) >>>>>> +else >>>>>> + @install -D -m644 $(TF_A_ARTIFACTS) $(IMAGEDIR) >>>>>> +endif >>>>> >>>>> Shouldn't this (copying files to IMAGEDIR) happen in targetinstall >>>>> rather than install? Again based on what is being done for other >>>>> bootloaders. >>>> >>>> On some systems, the BootROM doesn't directly invoke the TF-A, but instead it is >>>> a payload embedded into another bootloader. For this to work, we need to do it >>>> in the install stage, so the bootloader rule can depend on this and have the >>>> artifact available. >>>> >>>> That was the original line of thought, but apparently, how I did it is not >>>> completely sound. I should instead have both an install and targetinstall: >>>> >>>> - install installs into sysroot for use by other non-image rules >>>> - targetinstall installs into IMAGEDIR for use by image rules >>>> >>>> I will incorporate these changes along with your other suggestions in a v2. >>> >>> Thank you, >>> >>> Guillermo >>> >>>> >>>> Thanks! >>>> Ahmad >>>> >>>>> >>>>> Thanks, >>>>> >>>>> Guillermo >>>>> >>>>>> + >>>>>> + @$(call touch) >>>>>> + >>>>>> +# ---------------------------------------------------------------------------- >>>>>> +# Target-Install >>>>>> +# ---------------------------------------------------------------------------- >>>>>> + >>>>>> +$(STATEDIR)/tf-a.targetinstall: >>>>>> + @$(call targetinfo) >>>>>> + @$(call touch) >>>>>> + >>>>>> +# ---------------------------------------------------------------------------- >>>>>> +# Clean >>>>>> +# ---------------------------------------------------------------------------- >>>>>> + >>>>>> +$(STATEDIR)/tf-a.clean: >>>>>> + @$(call targetinfo) >>>>>> + @$(call clean_pkg, TF_A) >>>>>> + @rm -f $(IMAGEDIR)/bl1.bin $(IMAGEDIR)/fip.bin >>>>>> + >>>>>> +# vim: syntax=make >>>>>> -- >>>>>> 2.25.0 >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> ptxdist mailing list >>>>>> ptxdist@pengutronix.de >>>>> >>>>> >>>>> >>>> >>>> -- >>>> Pengutronix e.K. | | >>>> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >>>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >>>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | >>> >>> >>> >> >> -- >> Pengutronix e.K. | | >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | https://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ ptxdist mailing list ptxdist@pengutronix.de