[PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR
Michal Simek
michal.simek at xilinx.com
Tue Aug 25 17:12:58 CEST 2020
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.
>
>>
>> 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?
Thanks,
Michal
More information about the U-Boot
mailing list