[PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR

[Michal Simek]

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.

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?

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

I will play with it a little bit more to get more experience with it

Thanks,
Michal