Hi Michal, On Tue, 25 Aug 2020 at 09:13, Michal Simek <michal.si...@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.si...@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.si...@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 <mon...@monstr.eu> wrote: > >>>>>> > >>>>>> Hi Simon, > >>>>>> > >>>>>> ne 19. 7. 2020 v 22:06 odesÃlatel Simon Glass <s...@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 <s...@chromium.org> > >>>>>>> Reviewed-by: Bin Meng <bmeng...@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