From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Simek Date: Tue, 25 Aug 2020 17:12:58 +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> Message-ID: <1ec3b857-8835-51d8-ce85-5173e76607d4@xilinx.com> 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 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. > >> >> 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? Thanks, Michal