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

[Guillermo Rodriguez Garcia <guille.rodriguez@gmail.com>]

El mié., 12 feb. 2020 a las 10:28, Ahmad Fatoum
(<a.fatoum@pengutronix.de>) escribió:
>
> 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.

Ah, yes. That indeed looks like a bug.
>
> >
> >> 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

Yes, or:

3. Add a patch to fix the Makefile (as it is done in many other
ptxdist packages)

But anyway, as I said above, I don't have a strong opinion on this.
Setting CROSS_ENV works around the bug, which is both good and bad
(good for obvious reasons, bad because it sidesteps this kind of
issues rather than fixing them).

So, looking forward to v2 :-)

Best,

Guillermo

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



-- 
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de