[PATCH v9 14/14] treewide: Disable USE_SPL_FIT_GENERATOR by default
Michal Simek
michal.simek at amd.com
Mon Jan 16 11:25:25 CET 2023
Hi Simon,
On 1/15/23 17:31, Simon Glass wrote:
> Hi Michal,
>
> On Tue, 10 Jan 2023 at 08:16, Michal Simek <michal.simek at amd.com> wrote:
>>
>> Hi Simon,
>>
>> On 1/9/23 16:34, Simon Glass wrote:
>>> Hi Michal,
>>>
>>> On Mon, 9 Jan 2023 at 04:07, Michal Simek <michal.simek at amd.com
>>> <mailto:michal.simek at amd.com>> wrote:
>>> >
>>> > Hi,
>>> >
>>> > On 1/8/23 20:36, Simon Glass wrote:
>>> > > Hi Tom,
>>> > >
>>> > > On Sun, 8 Jan 2023 at 09:24, Tom Rini <trini at konsulko.com
>>> <mailto:trini at konsulko.com>> wrote:
>>> > >>
>>> > >> On Sun, Jan 08, 2023 at 09:20:09AM -0700, Simon Glass wrote:
>>> > >>> Hi Tom,
>>> > >>>
>>> > >>> On Sun, 8 Jan 2023 at 09:06, Tom Rini <trini at konsulko.com
>>> <mailto:trini at konsulko.com>> wrote:
>>> > >>>>
>>> > >>>> On Sun, Jan 08, 2023 at 08:48:37AM -0700, Simon Glass wrote:
>>> > >>>>> Hi Tom,
>>> > >>>>>
>>> > >>>>> On Sun, 8 Jan 2023 at 06:41, Tom Rini <trini at konsulko.com
>>> <mailto:trini at konsulko.com>> wrote:
>>> > >>>>>>
>>> > >>>>>> On Sat, Jan 07, 2023 at 02:07:21PM -0700, Simon Glass wrote:
>>> > >>>>>>
>>> > >>>>>>> This option is deprecated and only used by two boards. Enable it for just
>>> > >>>>>>> those two boards, so others don't accidentally enable it.
>>> > >>>>>>>
>>> > >>>>>>> Signed-off-by: Simon Glass <sjg at chromium.org <mailto:sjg at chromium.org>>
>>> > >>>>>> [snip]
>>> > >>>>>>> diff --git a/boot/Kconfig b/boot/Kconfig
>>> > >>>>>>> index 55f06761ef8..7ab0dd14211 100644
>>> > >>>>>>> --- a/boot/Kconfig
>>> > >>>>>>> +++ b/boot/Kconfig
>>> > >>>>>>> @@ -282,12 +282,13 @@ config SPL_FIT_SOURCE
>>> > >>>>>>> config USE_SPL_FIT_GENERATOR
>>> > >>>>>>> bool "Use a script to generate the .its script"
>>> > >>>>>>> depends on SPL_FIT
>>> > >>>>>>> - default y if SPL_FIT && ARCH_ZYNQMP
>>> > >>>>>>> + help
>>> > >>>>>>> + This is deprecated. Please do not use it. Use binman instead.
>>> > >>>>>>
>>> > >>>>>> Lets remove the text around bool so it can't be enabled, and move to
>>> > >>>>>> select'ing it from the two boards that need it. Michal, Luca, what's
>>> > >>>>>> needed to move your two platforms
>>> > >>>>>> (avnet_ultrazedev_cc_v1_0_ultrazedev_som_v1_0 and xilinx_zynqmp_virt off
>>> > >>>>>> of this very legacy option, given that other xilinx platforms have
>>> > >>>>>> already migrated to binman ?
>>> > >>>>>
>>> > >>>>> That's a good idea, but these two boards do not have TARGET Kconfig
>>> > >>>>> options so it is not possible without adding some Kconfig specific to
>>> > >>>>> those boards, then defining it in the defconfig files.
>>> > >>>>>
>>> > >>>>> We already have the legacy warning.
>>> > >>>>
>>> > >>>> Yes, but I swear these are new legacy users as when we started trying to
>>> > >>>> kill off this option it was just a few i.MX platforms lagging behind.
>>> > >>>>
>>> > >>>> Maybe make ARCH_ZYNQMP select DEPRECATED, USE_SPL_FIT_GENERATOR depend
>>> > >>>> on DEPRECATED and add "DERECATED" to the end of the text line? Or maybe
>>> > >>>> Michal or Luca will speak up soon and migrate these over quickly so we
>>> > >>>> can just nuke this.
>>> > >>>
>>> > >>> Isn't that just more tortuous? I can disable SPL_LOAD_FIT on these two
>>> > >>> boards since they don't appear in CI. Then they can convert them when
>>> > >>> ready.
>>> > >>>
>>> > >>> That way we can drop the option now, if that is your goal.
>>> > >>
>>> > >> I thought xilinx_zynqmp_virt was in CI, but I see I'm mistaken. I still
>>> > >> don't want to break platforms outright, and since it's Sunday right now
>>> > >> afterall, we should let Michal and Luca a chance to catch up and chime
>>> > >> in. I hope it's either going to be a quick conversion or expose
>>> > >> something missing and needed in binman, as to why these still haven't
>>> > >> been converted.
>>> > >>
>>> > >
>>> > > OK let's hold off on this patch for now. It is just a clean-up anyway.
>>> >
>>> > First of all. ZynqMP is not wired in CI simply because we are missing some bits
>>> > and pieces in upstream qemu to run it. If we can wire it with Xilinx version we
>>> > can do it. I have asked to fix it our qemu team but they have never done it.
>>> >
>>> > In U-Boot SPL flow this script is used all the time. I use it all the time when
>>> > I build work on ZynqMP. Also buildroot is using it.
>>> >
>>> > In connection to binman. I have looked at it 2/3 times in past. The biggest
>>> > issue which I have with it is that DT node presence in DT which goes to the
>>> system.
>>> >
>>> > Here is example
>>> >
>>> > [u-boot](binman)$ make kontron_sl28_defconfig >/dev/null
>>> > [u-boot](binman)$ make -j8 >/dev/null
>>> > [u-boot](binman)$ dtc -I dtb -O dts dts/dt.dtb 2>/dev/null | grep binman
>>> > binman {
>>> > binman = "/binman";
>>> > u_boot_rom = "/binman/u-boot-rom";
>>> >
>>> > where binmap is the part of target DT.
>>>
>>> Thanks for the info.
>>>
>>> >
>>> > It has side effects.
>>> > 1. DT is bigger
>>>
>>> From my experience the size increase typically is less than 2% which does not
>>> seem significant.
>>
>> It depends how many images you want to build and how many you want to describe.
>> Also in SOM program we are packing multiple DTS to the fit dtb image. It means
>> at the end it will be few kB for almost nothing.
>
> If we want to solve that problem we should use overlays or some other
> way of avoiding the madness of repeating the same material a dozen
> times :-)
We have internal task to take a look at dt overlay support when dtb reselect is
used to also work with dt overlays.
>>
>>>
>>> > 2. It contains information how firmware was packed which is just additional
>>> > information which don't need to be shared.
>>>
>>> That depends on the application. For firmware update and debugging it is
>>> sometime necessary to see where everything is. While I understand the EFI model
>>> of hiding everything away I believe that a better future for the industry is to
>>> have the OS and firmware work together more.
>>
>> of course. But I expect if we start to add it it is just question of time when
>> we get request to remove it.
>> And when I was playing with it xilinx boards are pretty much boards for testing
>> not any final product (I can ignore some exceptions). It means how I see binman
>> we could generate multiple images to cover all configurations which we are
>> testing and which should work together.
>>
>> Based on environment variable arch/arm/mach-zynqmp/mkimage_fit_atf.sh generates
>> u-boot.itb in different ways.
>>
>> u-boot + DTB
>> u-boot + DTB + TFA
>> u-boot + DTB + TFA + TEE
>>
>> where DTB can be fit image with multiple DTBs in it (for DTB reselection)
>> or multiple DTBs flag via configuration nodes.
>
> This series does allow TEE to be omitted, but not TFA since that makes
> the board fail to boot, doesn't it?
Not our configurations. Default configuration is to use TF-A at EL3 and U-Boot
in EL2-NS. But we also use U-Boot in EL3. All mini u-boot configurations are
working like this by default.
But also full U-Boot can start in EL3 without any issue.
EL1-NS which is also supported by U-Boot can be also configured but I didn't see
so far even request for testing it. But it should just work.
Definitely from testing perspective it is something what should be tested.
>>
>> And also images for flash which corresponds with configured offsets.
>>
>> We are also building u-boot based on TEXT_BASE but placing it to different DDR
>> location to make sure that it relocates properly. Or running in directly from
>> memory above 4GB. When binman is adopted I would like to add description for
>> these combinations there too.
>
> You should be able to do that easily enough. My suggestion is to have
> it build both types of multiple-DTB images and give them different
> names (I don't fully understand the different types).
It is more or less about option to be able to setup loading address for
multiple-DTB option you use which works fine in standard usage.
>>
>>>
>>> > 3. binman node as is placed in not documented in dt binding to be able to pass
>>> > dtb check.
>>>
>>> This is the root cause of all of the device tree woes. There was a small amount
>>> of slow progress at the end of last year but now it has gone quiet. I don't know
>>> what to do about this other than prod and wait, since Linux controls the
>>> bindings. Perhaps this is something that could be worked out f2f, I'm not sure.
>>> For now I will keep prodding and see what can be done. But the initial target is
>>> the DM tags, with binman bindings somewhere down the trackz although they may
>>> actually be easier.
>>
>> yes that's why I want to make sure that things what we do are going in a
>> direction that our DTs can be all checked without any violation. And adding
>> binman node to DT is not helping in this effort. Even adding it to *u-boot.dtsi
>> file which stays only in U-Boot is fine for Linux. SR 2.0 IIRC is going to check
>> DT which is provided by firmware itself which is in this case DT from U-Boot
>> with binman node in it.
>
> Well then I hope that ARM / Linaro will become more supportive of
> getting these bindings upstream. It is not for want of trying, but the
> process is quite slow. Perhaps that will help speed it up?
>
> Note that I don't want U-Boot to have things in the DT that are not in
> the bindings.
Hopefully I will have more time for it this year. Let's see if Linaro can help
with this. I wanted them to do it for EFI A/B capsule update based on raw mode.
But the latest version is not sent to mailing list yet.
>>
>>>
>>> It is easy enough to use fdtgrep to strip out the binman {} node if it is not
>>> wanted. I will take a look at a feature for that, but I have a bit of a
>>> backlog,with so many things up in the air, pending patches, etc.
>>
>> I am aware about using fdtgrep for SPL but not against DT used for u-boot
>> proper. I am fine if there is any way to remove it from DT.
>
> You can try fdtgrep to do that, but please understand that the goal
> here is a unified DT across firmware and kernel, not different
> projects using different bindings.
I get it but it is still long way to go.
>>
>>>
>>> >
>>> > I can call binman externally but that would mean additional build step which
>>> > everybody wants to avoid.
>>> > Can we update Makefile and add Kconfig option to pass configuration dtb file
>>> > instead of u-boot.dtb by default?
>>> >
>>> > Something like:
>>> >
>>> > diff --git a/Makefile b/Makefile
>>> > index 75a599b2c437..4d8d67c7d938 100644
>>> > --- a/Makefile
>>> > +++ b/Makefile
>>> > @@ -1322,7 +1322,7 @@ cmd_binman = $(srctree)/tools/binman/binman $(if
>>> > $(BINMAN_DEBUG),-D) \
>>> > $(foreach f,$(BINMAN_TOOLPATHS),--toolpath $(f)) \
>>> > --toolpath $(objtree)/tools \
>>> > $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \
>>> > - build -u -d u-boot.dtb -O . -m \
>>> > + build -u -d $(CONFIG_BINMAN_CONFIG_DTB) -O . -m \
>>> > $(if $(BINMAN_ALLOW_MISSING),--allow-missing --ignore-missing) \
>>> > -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \
>>> > -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \
>>> >
>>> > Then by default u-boot.dtb can be used or platforms can use different file.
>>> > Then I would add zynqmp-binman.dts with configurations which can replace
>>> > existing arch/arm/mach-zynqmp/mkimage_fit_atf.sh script.
>>> > What do you think?
>>>
>>> I'm sure you could do that locally, but I'd prefer to get the bindings sorted
>>> out so that binman nodes can go in there, along with a Kconfig option to
>>> enable/disable them. The more we bifurcate things, the more complicated
>>> everything is.
>>
>> I expect if you move binman to options {}; maybe options { u-boot{};}; node Rob
>> won't have any issue with it. But of course that's needs to be discussed with him.
>>
>>>
>>> For now, can you live with the binman nodes? What problem does it cause?
>>
>> Current script is used only in SPL flow which is not actually officially
>> supported AMD/XILINX flow. It means there won't be any issue on AMD/XILINX side
>> if we simply remove the script but likely we will break users which are using
>> SPL flow. And they are definitely there.
>> All these users will start to see binman nodes and to be fair none will likely
>> spot it anytime soon because if boot is working why you should look at it.
>
> Note that the binman node is dropped from SPL. It only appears in U-Boot proper.
I know.
>>
>> But as I said above when I start to use it I want to use it for all image
>> generations that by one make I get all combinations for testing.
>> It means DT in SD boot mode will also contain information about QSPI image and
>> also other combinations which were built.
>> It will create just confusion which I want to avoid as much as possible.
>
> Yes that's true. In fact, we need a way to select the correct image.
> At present that is done using the fdtmap, but that sits in its own
> area and does not adjust the device tree in the .dtb
>
> Honestly, this all needs a bit of a look once we make some progress on
> the bindings.
I played with binman multiple times in past but never really send support for it
for our SOCs. I am definitely not aware about all features which you have added
over years there. But I want to make sure that
>>
>> Can you live with passing external binman DTB via Kconfig option till fdtgrep
>> option is working?
>
> If you want to add that as an option it seems OK to me, or at least I
> cannot see any problems it would cause. You could even use an env var
> if that is easier. But the upstream board should really build
> everything, as we are doing on rockchip.
The problem with env variable is that users who uses defconfig would have to set
it up. It means all yocto meta layers needs to be updated. With Kconfig I can
simply update defconfig and that's it. No impact on others.
>>
>>
>>> >
>>> > BTW: Is there a way to generate also capsules from binman?
>>>
>>> Is that an EFI capsule? If so, I haven't seen any work on that yet.
>>>
>>
>> yes EFI capsule. This is pretty much what I generate by external script after
>> u-boot compilation.
>>
>> pushd spl
>> /mnt/disk/u-boot/tools/mkeficapsule -g "de6066e8-0256-4fad-8238-e406e274c4cf"
>> boot.bin --index 1 ../capsule1.bin
>> popd
>> /mnt/disk/u-boot/tools/mkeficapsule -g "cf9ecfd4-938b-41c5-8551-1f883ab7dc18"
>> u-boot.itb --index 2 capsule2.bin
>
> OK. I suppose it is easy enough although I'm not sure where those
> UUIDs come from.
There are macros for it in the code and also one structure is defined with it.
21 /* GUIDs for capsule updatable firmware images */
22 #define XILINX_BOOT_IMAGE_GUID \
23 EFI_GUID(0xde6066e8, 0x0256, 0x4fad, 0x82, 0x38, \
24 0xe4, 0x06, 0xe2, 0x74, 0xc4, 0xcf)
25
26 #define XILINX_UBOOT_IMAGE_GUID \
27 EFI_GUID(0xcf9ecfd4, 0x938b, 0x41c5, 0x85, 0x51, \
28 0x1f, 0x88, 0x3a, 0xb7, 0xdc, 0x18)
35 #if CONFIG_IS_ENABLED(EFI_HAVE_CAPSULE_SUPPORT)
36 struct efi_fw_image fw_images[] = {
37 #if defined(XILINX_BOOT_IMAGE_GUID)
38 {
39 .image_type_id = XILINX_BOOT_IMAGE_GUID,
40 .fw_name = u"XILINX-BOOT",
41 .image_index = 1,
42 },
43 #endif
44 #if defined(XILINX_UBOOT_IMAGE_GUID)
45 {
46 .image_type_id = XILINX_UBOOT_IMAGE_GUID,
47 .fw_name = u"XILINX-UBOOT",
48 .image_index = 2,
49 },
50 #endif
51 };
>>
>>
>> Also is there a way to get address from ELF but place BIN from that image
>> instead (or convert it to BIN)?
>> I added support for this to avoid situations that image is placed to incorrect
>> location. I do use it in case of TF-A which can run out of OCM or DDR that don't
>> need to change configuration based on actual file.
>
> I suppose you mean to run objdump on the elf, or perhaps extract the
> segments and put them in a binary? It is not supported, but could be
> added.
Just objcopy -O binary on the elf and fill load and entry address based on
addresses from elf.
>>
>> Is there also a way to call fdtoverlay to compose DTB + DTBO together to create
>> images? Linux kernel does it as the part of make but we are missing this feature
>> in U-Boot but binman could actually help in this.
>
> Again, I'm sure it could be added. Perhaps you could start a new
> thread on new features that would be useful, as I think this thread
> has got a bit off topic?
I will prepare some patches and start to convert cases one by one to reach the
state where we can remove that external script.
Thanks,
Michal
More information about the U-Boot
mailing list