From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Simek Date: Wed, 16 Sep 2020 15:41:26 +0200 Subject: [PATCH v4 20/27] Makefile: Warn against using CONFIG_SPL_FIT_GENERATOR In-Reply-To: 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 Simon, On 01. 09. 20 17:26, Simon Glass wrote: > 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? I have sent patches which updates documentation. Thanks, Michal