From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 1 Sep 2020 09:26:42 -0600 Subject: [PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR In-Reply-To: <6aa0107e-df0a-a1dd-a655-6d0e339f2136@xilinx.com> References: <20200719195618.552021-1-sjg@chromium.org> <20200719135448.v4.20.I2428dcb9b077364f9517f2c291db63b6bac1e992@changeid> <438bba0b-472f-44d1-c65c-eeb1e5014088@xilinx.com> <29ab75b6-d3f8-6322-003c-1508a2e9ee35@xilinx.com> <1ec3b857-8835-51d8-ce85-5173e76607d4@xilinx.com> <6aa0107e-df0a-a1dd-a655-6d0e339f2136@xilinx.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Michal, On Tue, 1 Sep 2020 at 09:15, Michal Simek wrote: > > Hi Simon, > > On 30. 08. 20 22:37, Simon Glass wrote: > > Hi Michal, > > > > On Wed, 26 Aug 2020 at 08:12, Michal Simek wrote: > >> > >> Hi, > >> > >> On 25. 08. 20 18:57, Simon Glass wrote: > >>> Hi Michal, > >>> > >>> On Tue, 25 Aug 2020 at 09:13, Michal Simek wrote: > >>>> > >>>> Hi Simon, > >>>> > >>>> On 25. 08. 20 17:04, Simon Glass wrote: > >>>>> Hi Michal, > >>>>> > >>>>> On Mon, 24 Aug 2020 at 08:12, Michal Simek wrote: > >>>>>> > >>>>>> Hi Simon, > >>>>>> > >>>>>> On 22. 08. 20 17:08, Simon Glass wrote: > >>>>>>> Hi Michal, > >>>>>>> > >>>>>>> On Mon, 17 Aug 2020 at 00:49, Michal Simek wrote: > >>>>>>>> > >>>>>>>> Hi Simon, > >>>>>>>> > >>>>>>>> On 16. 08. 20 5:39, Simon Glass wrote: > >>>>>>>>> Hi Michal, > >>>>>>>>> > >>>>>>>>> On Fri, 14 Aug 2020 at 07:28, Michal Simek wrote: > >>>>>>>>>> > >>>>>>>>>> Hi Simon, > >>>>>>>>>> > >>>>>>>>>> ne 19. 7. 2020 v 22:06 odes?latel Simon Glass 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 > >>>>>>>>>>> Reviewed-by: Bin Meng > >>>>>>>>>>> --- > >>>>>>>>>>> > >>>>>>>>>>> (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. > > > > If you don't need CONFIG_BINMAN_FDT then it is fine to disable it. You > > could do that by updating the default condition there, or selecting a > > different value for your arch. > > > >> > >> > >>>> > >>>>> > >>>>>> > >>>>>> 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 > >>>>>> + > >>>>>> +/ { > >>>>>> + binman: binman { > >>>>>> + multiple-images; > >>>>>> + }; > >>>>>> +}; > >>>>>> + > >>>>>> +&binman { > >>>>>> + u-boot-itb { > >>>>>> + filename = "u-boot.itb"; > >>>>>> + fit { > >>>>>> + 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 = ; > >>>>>> + entry = ; > >>>>>> + > >>>>>> + 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? > > > > OK I'll add something in the v2 series. > > Thanks. > > > > >> > >> 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? > > > > At present binman doesn't support including one image in another, > > although since generation of images is ordered, yes it should be > > possible to do it that way. > > ok. > > > > >> 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 > > > > OK you should be able to use the mkimage entry-type for that. > > Normally it should be enough > mkimage { > args = "-T zynqmpimage -R $CONFIG_BOOT_INIT_FILE"; > u-boot-spl-align { > }; > }; > > It means I would have to create new type for u-boot-spl-align file. > And second in argument there is $CONFIG_BOOT_INIT_FILE. One option would be to allow args to be a list of strings, i.e. one string per argument. Then you could do: args = "-T", "zynqmpimage", "-R", CONFIG_BOOT_INIT_FILE; > > Anyway I can simply take spl/boot.bin which is generated already as the > part of build and don't need to call mkimage from here. The goal here is to get rid of arch-specific stuff in Makefile. > > > > > >> > >> And then u-boot.itb placed at CONFIG_SYS_SPI_KERNEL_OFFS offset. > > > > OK, you can access CONFIG options in the .dtsi > > > > Can you please point me to the docs for zynqmp in the U-Boot tree? I > > see stuff about zynq inthe tree but it is quite old. I think there > > needs to be a link from doc/board/xilinx > > We haven't really spent any time on writing proper doc for zynqmp. > But I normally just do this. > > setup arm64 toochain > make xilinx_zynqmp_virt_defconfig > Fill > CONFIG_ZYNQMP_SPL_PM_CFG_OBJ_FILE - which points to pmu configuration > object. > and > CONFIG_PMUFW_INIT_FILE - which points to PMU binary file. > > export DEVICE_TREE=zynqmp-zcu104-revC > make -j8 > > you get u-boot.itb and spl/boot.bin. You copy them to SD card and boot > from it. This will get you to u-boot running from EL3. > > if you add bl31.bin to your source tree or point to it via BL31 variable > itb is created with ATF inside and you will get to to SPL->ATF->U-Boot > in EL2. Would you mind putting the above in a patch? > > > > > Also is there a low-cost zynqmp board about? I'm thinking about adding > > it to my lab. > > The cheapest is likely ultra96/zcu100. But v1 is sold out and v2 is made > by Avnet and they changed a lot of components and never invested time to > really support this board upstream. > But let me ask around if we have any v1 which we can share. OK ta. I can't see any around. > > > > > Finally, these boards seem to use CONFIG_OF_EMBED which is not > > allowed. Can you fix that sometime? > > Only that mini configurations which is used only for memory tests or > flash programming. Conversion to OF_SEPARATE should be easy but we need > to retest all configurations again. OK. That should really be done as a priority as it should not have been done that way. Perhaps after the binman conversion? Regards, Simon