[PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR
Simon Glass
sjg at chromium.org
Tue Aug 25 18:57:54 CEST 2020
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?
>
> >
> >>
> >> 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.
>
> Thanks,
> Michal
Regards,
Simon
More information about the U-Boot
mailing list