[PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR
Michal Simek
michal.simek at xilinx.com
Wed Aug 26 16:11:50 CEST 2020
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.
>>
>>>
>>>>
>>>> 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?
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?
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
And then u-boot.itb placed at CONFIG_SYS_SPI_KERNEL_OFFS offset.
Thanks,
Michal
More information about the U-Boot
mailing list