[PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR
Michal Simek
michal.simek at xilinx.com
Wed Sep 16 15:41:26 CEST 2020
Hi Simon,
On 01. 09. 20 17:26, Simon Glass wrote:
> 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?
I have sent patches which updates documentation.
Thanks,
Michal
More information about the U-Boot
mailing list