Re: [ptxdist] [PATCH] tf-a: new package for ARM trusted firmware A

[Ahmad Fatoum <a.fatoum@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