[PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR
Simon Glass
sjg at chromium.org
Tue Sep 1 17:26:42 CEST 2020
Hi Michal,
On Tue, 1 Sep 2020 at 09:15, Michal Simek <michal.simek at xilinx.com> wrote:
>
> Hi Simon,
>
> On 30. 08. 20 22:37, Simon Glass wrote:
> > Hi Michal,
> >
> > On Wed, 26 Aug 2020 at 08:12, Michal Simek <michal.simek at xilinx.com> wrote:
> >>
> >> Hi,
> >>
> >> On 25. 08. 20 18:57, Simon Glass wrote:
> >>> Hi Michal,
> >>>
> >>> On Tue, 25 Aug 2020 at 09:13, Michal Simek <michal.simek at xilinx.com> wrote:
> >>>>
> >>>> Hi Simon,
> >>>>
> >>>> On 25. 08. 20 17:04, Simon Glass wrote:
> >>>>> Hi Michal,
> >>>>>
> >>>>> On Mon, 24 Aug 2020 at 08:12, Michal Simek <michal.simek at xilinx.com> wrote:
> >>>>>>
> >>>>>> Hi Simon,
> >>>>>>
> >>>>>> On 22. 08. 20 17:08, Simon Glass wrote:
> >>>>>>> Hi Michal,
> >>>>>>>
> >>>>>>> On Mon, 17 Aug 2020 at 00:49, Michal Simek <michal.simek at xilinx.com> wrote:
> >>>>>>>>
> >>>>>>>> Hi Simon,
> >>>>>>>>
> >>>>>>>> On 16. 08. 20 5:39, Simon Glass wrote:
> >>>>>>>>> Hi Michal,
> >>>>>>>>>
> >>>>>>>>> On Fri, 14 Aug 2020 at 07:28, Michal Simek <monstr at monstr.eu> wrote:
> >>>>>>>>>>
> >>>>>>>>>> Hi Simon,
> >>>>>>>>>>
> >>>>>>>>>> ne 19. 7. 2020 v 22:06 odesílatel Simon Glass <sjg at chromium.org> napsal:
> >>>>>>>>>>>
> >>>>>>>>>>> This option is used to run arch-specific shell scripts which produce .its
> >>>>>>>>>>> files which are used to produce FIT images. We already have binman which
> >>>>>>>>>>> is designed to produce firmware images. It is more powerful and has tests.
> >>>>>>>>>>>
> >>>>>>>>>>> So this option should be deprecated and not used. Existing uses should be
> >>>>>>>>>>> migrated.
> >>>>>>>>>>>
> >>>>>>>>>>> Mentions of this in code reviews over the last year or so do not seem to
> >>>>>>>>>>> have resulted in action, and things are getting worse.
> >>>>>>>>>>>
> >>>>>>>>>>> So let's add a warning.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
> >>>>>>>>>>> Reviewed-by: Bin Meng <bmeng.cn at gmail.com>
> >>>>>>>>>>> ---
> >>>>>>>>>>>
> >>>>>>>>>>> (no changes since v1)
> >>>>>>>>>>>
> >>>>>>>>>>> Makefile | 9 +++++++++
> >>>>>>>>>>> 1 file changed, 9 insertions(+)
> >>>>>>>>>>>
> >>>>>>>>>>> diff --git a/Makefile b/Makefile
> >>>>>>>>>>> index f1b5be1882..d73c10a973 100644
> >>>>>>>>>>> --- a/Makefile
> >>>>>>>>>>> +++ b/Makefile
> >>>>>>>>>>> @@ -1148,6 +1148,13 @@ ifneq ($(CONFIG_DM_ETH),y)
> >>>>>>>>>>> @echo >&2 "See doc/driver-model/migration.rst for more info."
> >>>>>>>>>>> @echo >&2 "===================================================="
> >>>>>>>>>>> endif
> >>>>>>>>>>> +endif
> >>>>>>>>>>> +ifneq ($(CONFIG_SPL_FIT_GENERATOR),)
> >>>>>>>>>>> + @echo >&2 "===================== WARNING ======================"
> >>>>>>>>>>> + @echo >&2 "This board uses CONFIG_SPL_FIT_GENERATOR. Please migrate"
> >>>>>>>>>>> + @echo >&2 "to binman instead, to avoid the proliferation of"
> >>>>>>>>>>> + @echo >&2 "arch-specific scripts with no tests."
> >>>>>>>>>>> + @echo >&2 "===================================================="
> >>>>>>>>>>> endif
> >>>>>>>>>>> @# Check that this build does not use CONFIG options that we do not
> >>>>>>>>>>> @# know about unless they are in Kconfig. All the existing CONFIG
> >>>>>>>>>>> @@ -1345,6 +1352,8 @@ endif
> >>>>>>>>>>>
> >>>>>>>>>>> # Boards with more complex image requirements can provide an .its source file
> >>>>>>>>>>> # or a generator script
> >>>>>>>>>>> +# NOTE: Please do not use this. We are migrating away from Makefile rules to use
> >>>>>>>>>>> +# binman instead.
> >>>>>>>>>>> ifneq ($(CONFIG_SPL_FIT_SOURCE),"")
> >>>>>>>>>>> U_BOOT_ITS := u-boot.its
> >>>>>>>>>>> $(U_BOOT_ITS): $(subst ",,$(CONFIG_SPL_FIT_SOURCE))
> >>>>>>>>>>> --
> >>>>>>>>>>> 2.28.0.rc0.105.gf9edc3c819-goog
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> I just got to this conversion and I am curious how that transition
> >>>>>>>>>> should look like.
> >>>>>>>>>> I found how FIT image is created which is fine but I didn't find any
> >>>>>>>>>> reference on how to generate images based on CONFIG_OF_LIST.
> >>>>>>>>>> If you look at arch/arm/mach-zynqmp/mkimage_fit_atf.sh you will see
> >>>>>>>>>> that I loop over this entry and create multiple DT nodes and the same
> >>>>>>>>>> amount of configurations to cover it. Is this supported by binman?
> >>>>>>>>>> If yes, what's the syntax for it?
> >>>>>>>>>
> >>>>>>>>> The easiest way is probably to create a new entry type, like zynq-fit.
> >>>>>>>>> Then you can generate the DT using the sequence writer functions. See
> >>>>>>>>> _ReadSubNodes() in fit.py for an example.
> >>>>>>>>>
> >>>>>>>>> You can perhaps have a template subnode and use that in a for loop to
> >>>>>>>>> generate the nodes.
> >>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> I tried several configurations and we can use that for generating qspi
> >>>>>>>>>> images and also images with different configurations to have them
> >>>>>>>>>> ready
> >>>>>>>>>> but first I need to be able to handle the case above.
> >>>>>>>>>
> >>>>>>>>> I was thinking of converting sunxi which has the same need, but it
> >>>>>>>>> sounds like you are on the case. Let me know if you need help.
> >>>>>>>>
> >>>>>>>> Nope. I just saw that message and started to play with it to find out
> >>>>>>>> what needs to be done and how this fits to bigger picture. If this
> >>>>>>>> doesn't work directly then the work needs to be planned which will take
> >>>>>>>> time especially when this utility is new for us and we could have issues
> >>>>>>>> with writing code in python. Would be good if you can do the first shot
> >>>>>>>> because you know this utility and I am more than happy to test it, try
> >>>>>>>> and adopt if needed for our case.
> >>>>>>>>
> >>>>>>>> Sunxi is very similar case as is zynqmp. Difference is they hardcode
> >>>>>>>> default configuration to config_1. ZynqMP is setting up default based on
> >>>>>>>> default DT configured at that time.
> >>>>>>>>
> >>>>>>>> In connection to binman I see that there would be a need to generate
> >>>>>>>> images with ATF and without ATF in configuration node and with different
> >>>>>>>> default configuration. There could be also a need to add additional
> >>>>>>>> loadable entry such as bitstreams.
> >>>>>>>>
> >>>>>>>> Back to zynq-fit new entry type. I don't think it should be zynq/zynqmp
> >>>>>>>> type because as was state in commit message u-boot.itb generation is
> >>>>>>>> very similar for all these boards that's why name for this new entry
> >>>>>>>> should be generic.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I sent an initial series to add this to binman. I've since found a few
> >>>>>>> problems so will send a v2 at some point. You can try it out at
> >>>>>>> u-boot-dm/binman-working
> >>>>>>
> >>>>>> I looked at this branch and add my changes on the top.
> >>>>>>
> >>>>>> The first thing what I see is that I miss fit,fdt-list = "of-list"; in
> >>>>>> sunxi dt file. I had to add it to work for me.
> >>>>>
> >>>>> Ah yes, I decided to add this at the last minute so it is not relying
> >>>>> on a convention.
> >>>>>
> >>>>>>
> >>>>>> With BINMAN_FDT enabled I am getting error that there is no valid
> >>>>>> "binman node" in DT. I didn't study that code yet but that's the point
> >>>>>> of keeping this DT node out there?
> >>>>>
> >>>>> Is this in SPL? Perhaps something is filtering out the node.
> >>>>
> >>>> Nope in full U-Boot but in SPL flow. SPL->ATF->FULL U-Boot.
> >>>
> >>> That needs debugging. I can't understand how the /binman node can be
> >>> missing in U-Boot. The C library is very simple and doesn't handle
> >>> finding nodes in multiple images...perhaps that is the problem?
> >>
> >> I found the reason for this behavior. On our platforms we are checking
> >> specific address where DTB can be placed. And because I have played with
> >> it also with previous image. It was pick up automatically.
> >>
> >> But still missing why BINMAN_FDT should be enabled by default on
> >> platforms which don't call any binman functions. I see that you are
> >> calling that functions from x86 platforms to try to map and find out
> >> some image offsets/positions and it looks like that you are loading them.
> >> I can imagine that this could be use for example for better space
> >> utilization that my boot.bin with SPL can be followed immediately by
> >> u-boot.itb in qspi. But for supporting this I expect spl_spi.c needs to
> >> be aligned. And adding support in a generic way there needs to be an
> >> agreement on node name which should be loaded.
> >
> > If you don't need CONFIG_BINMAN_FDT then it is fine to disable it. You
> > could do that by updating the default condition there, or selecting a
> > different value for your arch.
> >
> >>
> >>
> >>>>
> >>>>>
> >>>>>>
> >>>>>> This is my binman configuration.
> >>>>>>
> >>>>>> diff --git a/arch/arm/dts/zynqmp-u-boot.dtsi
> >>>>>> b/arch/arm/dts/zynqmp-u-boot.dtsi
> >>>>>> new file mode 100644
> >>>>>> index 000000000000..b3364d3e2df8
> >>>>>> --- /dev/null
> >>>>>> +++ b/arch/arm/dts/zynqmp-u-boot.dtsi
> >>>>>> @@ -0,0 +1,72 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>>> +/*
> >>>>>> + * Copyright (C) 2020 Xilinx, Inc.
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include <config.h>
> >>>>>> +
> >>>>>> +/ {
> >>>>>> + binman: binman {
> >>>>>> + multiple-images;
> >>>>>> + };
> >>>>>> +};
> >>>>>> +
> >>>>>> +&binman {
> >>>>>> + u-boot-itb {
> >>>>>> + filename = "u-boot.itb";
> >>>>>> + fit {
> >>>>>> + fit,external-offset = <CONFIG_FIT_EXTERNAL_OFFSET>;
> >>>>>> + description = "FIT image with ATF support";
> >>>>>> + fit,fdt-list = "of-list";
> >>>>>> + #address-cells = <1>;
> >>>>>> +
> >>>>>> + images {
> >>>>>> + uboot {
> >>>>>> + description = "U-Boot (64-bit)";
> >>>>>> + type = "firmware";
> >>>>>> + os = "u-boot";
> >>>>>> + arch = "arm64";
> >>>>>> + compression = "none";
> >>>>>> + load = <CONFIG_SYS_TEXT_BASE>;
> >>>>>> + entry = <CONFIG_SYS_TEXT_BASE>;
> >>>>>> +
> >>>>>> + u-boot-nodtb {
> >>>>>> + };
> >>>>>> + };
> >>>>>> + atf {
> >>>>>> + description = "ARM Trusted Firmware";
> >>>>>> + type = "firmware";
> >>>>>> + os = "arm-trusted-firmware";
> >>>>>> + arch = "arm64";
> >>>>>> + compression = "none";
> >>>>>> + load = <0xfffea000>; /* FIXME */
> >>>>>> + entry = <0xfffea000>;
> >>>>>> +
> >>>>>> + blob-ext {
> >>>>>> + filename = "bl31.bin";
> >>>>>> + };
> >>>>>> + };
> >>>>>> + @fdt-SEQ {
> >>>>>> + description = "NAME";
> >>>>>> + type = "flat_dt";
> >>>>>> + arch = "arm64";
> >>>>>> + compression = "none";
> >>>>>> + };
> >>>>>> + };
> >>>>>> +
> >>>>>> + configurations {
> >>>>>> + default = "config-1";
> >>>>>> + @config-SEQ {
> >>>>>> + description = "NAME";
> >>>>>> + firmware = "atf";
> >>>>>> + loadables = "uboot";
> >>>>>> + fdt = "fdt-SEQ";
> >>>>>> + };
> >>>>>> + };
> >>>>>> + };
> >>>>>> + fdtmap{};
> >>>>>> + };
> >>>>>> +
> >>>>>> +};
> >>>>>>
> >>>>>> Anyway compare to current script default option is hardcoded to
> >>>>>> config-1.
> >>>>>
> >>>>>
> >>>>>> Current arch/arm/mach-zynqmp/mkimage_fit_atf.sh is also
> >>>>>> setting up default option based on selected default DT (I can fix this
> >>>>>> by implementing board_fit_config_name_match() but IIRC it is looping
> >>>>>> over all configurations and slowing down boot).
> >>>>>
> >>>>> Is this using an environment variable to select the default? Would it
> >>>>> be OK to put this in the DT for each individual board?
> >>>>
> >>>> I have added this code to board_fit_config_name_match() to select proper
> >>>> configuration from SPL
> >>>>
> >>>> + if (!strcmp(name, DEVICE_TREE))
> >>>> + return 0;
> >>>>
> >>>> DEVICE_TREE is setup in generated/dt.h.
> >>>>
> >>>> I am not quite sure what you mean by put this to each individual board.
> >>>> Like a property for SPL which DT should be select?
> >>>
> >>> OK I see. In that case I think we need another entry argument to pass
> >>> ${DEVICE_TREE} to the fit entry, and pass it in to binman from the
> >>> Makefile with another -a parameter.
> >>
> >> Can you please include this option?
> >
> > OK I'll add something in the v2 series.
>
> Thanks.
>
> >
> >>
> >> Also what's the easiest way to compose multiple images though binman and
> >> share images among others configurations?
> >>
> >> I will generate u-boot.itb through binman and then I want to use this
> >> file for composing qspi image. Should I just point to u-boot.itb as blob
> >> with filename?
> >
> > At present binman doesn't support including one image in another,
> > although since generation of images is ordered, yes it should be
> > possible to do it that way.
>
> ok.
>
> >
> >> Layout for qspi is quite simply spl/boot.bin which is generated as
> >> ./tools/mkimage -T zynqmpimage -R ./"" -n
> >> "/home/monstr/data/disk/u-boot-bins/zynqmp/pmu.bin" -d
> >> spl/u-boot-spl-align.bin spl/boot.bin >/dev/null && cat /dev/null
> >
> > OK you should be able to use the mkimage entry-type for that.
>
> Normally it should be enough
> mkimage {
> args = "-T zynqmpimage -R $CONFIG_BOOT_INIT_FILE";
> u-boot-spl-align {
> };
> };
>
> It means I would have to create new type for u-boot-spl-align file.
> And second in argument there is $CONFIG_BOOT_INIT_FILE.
One option would be to allow args to be a list of strings, i.e. one
string per argument. Then you could do:
args = "-T", "zynqmpimage", "-R", CONFIG_BOOT_INIT_FILE;
>
> Anyway I can simply take spl/boot.bin which is generated already as the
> part of build and don't need to call mkimage from here.
The goal here is to get rid of arch-specific stuff in Makefile.
>
>
> >
> >>
> >> And then u-boot.itb placed at CONFIG_SYS_SPI_KERNEL_OFFS offset.
> >
> > OK, you can access CONFIG options in the .dtsi
> >
> > Can you please point me to the docs for zynqmp in the U-Boot tree? I
> > see stuff about zynq inthe tree but it is quite old. I think there
> > needs to be a link from doc/board/xilinx
>
> We haven't really spent any time on writing proper doc for zynqmp.
> But I normally just do this.
>
> setup arm64 toochain
> make xilinx_zynqmp_virt_defconfig
> Fill
> CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE - which points to pmu configuration
> object.
> and
> CONFIG_PMUFW_INIT_FILE - which points to PMU binary file.
>
> export DEVICE_TREE=zynqmp-zcu104-revC
> make -j8
>
> you get u-boot.itb and spl/boot.bin. You copy them to SD card and boot
> from it. This will get you to u-boot running from EL3.
>
> if you add bl31.bin to your source tree or point to it via BL31 variable
> itb is created with ATF inside and you will get to to SPL->ATF->U-Boot
> in EL2.
Would you mind putting the above in a patch?
>
> >
> > Also is there a low-cost zynqmp board about? I'm thinking about adding
> > it to my lab.
>
> The cheapest is likely ultra96/zcu100. But v1 is sold out and v2 is made
> by Avnet and they changed a lot of components and never invested time to
> really support this board upstream.
> But let me ask around if we have any v1 which we can share.
OK ta. I can't see any around.
>
> >
> > Finally, these boards seem to use CONFIG_OF_EMBED which is not
> > allowed. Can you fix that sometime?
>
> Only that mini configurations which is used only for memory tests or
> flash programming. Conversion to OF_SEPARATE should be easy but we need
> to retest all configurations again.
OK. That should really be done as a priority as it should not have
been done that way. Perhaps after the binman conversion?
Regards,
Simon
More information about the U-Boot
mailing list