Simon, On 9/5/20 7:18 PM, Simon Glass wrote: > On Sat, 5 Sep 2020 at 17:10, Samuel Holland <sam...@sholland.org> wrote: >> On 9/1/20 6:14 AM, Simon Glass wrote: >>> At present 64-bit sunxi boards use the Makefile to create a FIT, using >>> USE_SPL_FIT_GENERATOR. This is deprecated. >>> >>> Update sunxi to use binman instead. >>> >>> Signed-off-by: Simon Glass <s...@chromium.org> >>> --- >>> >>> (no changes since v2) >>> >>> Changes in v2: >>> - Add a 'fit-fdt-list' property >>> - Fix 'board' typo in commit message >>> >>> Kconfig | 3 +- >>> Makefile | 18 ++-------- >>> arch/arm/dts/sunxi-u-boot.dtsi | 61 +++++++++++++++++++++++++++++++++- >>> 3 files changed, 63 insertions(+), 19 deletions(-) >>> >>> diff --git a/Kconfig b/Kconfig >>> index 883e3f71d01..837b2f517ae 100644 >>> --- a/Kconfig >>> +++ b/Kconfig >>> @@ -659,12 +659,11 @@ config SPL_FIT_SOURCE >>> >>> config USE_SPL_FIT_GENERATOR >>> bool "Use a script to generate the .its script" >>> - default y if SPL_FIT >>> + default y if SPL_FIT && !ARCH_SUNXI >> >> Now `make u-boot.itb` doesn't work. >> >> u-boot.itb is helpful to have because, with CONFIG_OF_LIST, it can be shared >> across all boards of a platform. Only SPL is board-specific (on arm64 sunxi, >> at >> least). > > It is generated, just with a different filename.
Thanks. From looking at the code and comparing with u-boot-sunxi-with-spl.bin, it seems that u-boot-sunxi-with-spl.fit.fit is the "final" ITB file. My only hesitation is that it seems like an implementation detail, but I guess it's fine for now. >> >> Is there a way to make binman also write the FIT without the SPL? Would that >> require duplicating the whole binman node? > > Yes it would. We could get more complicated and allow an image to > build on another perhaps. I'm not sure what is easiest here. u-boot-sunxi-with-spl.fit.fit is good enough for my purposes, but others may have an opinion. >> >>> config SPL_FIT_GENERATOR >>> string ".its file generator script for U-Boot FIT image" >>> depends on USE_SPL_FIT_GENERATOR >>> - default "board/sunxi/mksunxi_fit_atf.sh" if SPL_LOAD_FIT && ARCH_SUNXI >>> default "arch/arm/mach-rockchip/make_fit_atf.py" if SPL_LOAD_FIT && >>> ARCH_ROCKCHIP >>> default "arch/arm/mach-zynqmp/mkimage_fit_atf.sh" if SPL_LOAD_FIT && >>> ARCH_ZYNQMP >>> default "arch/riscv/lib/mkimage_fit_opensbi.sh" if SPL_LOAD_FIT && >>> RISCV >>> diff --git a/Makefile b/Makefile >>> index 5b4e60496d6..65024c74089 100644 >>> --- a/Makefile >>> +++ b/Makefile >>> @@ -923,11 +923,6 @@ INPUTS-$(CONFIG_REMAKE_ELF) += u-boot.elf >>> INPUTS-$(CONFIG_EFI_APP) += u-boot-app.efi >>> INPUTS-$(CONFIG_EFI_STUB) += u-boot-payload.efi >>> >>> -# Build a combined spl + u-boot image for sunxi >>> -ifeq ($(CONFIG_ARCH_SUNXI)$(CONFIG_ARM64)$(CONFIG_SPL),yyy) >>> -INPUTS-y += u-boot-sunxi-with-spl.bin >>> -endif >>> - >>> # Generate this input file for binman >>> ifeq ($(CONFIG_SPL),) >>> INPUTS-$(CONFIG_ARCH_MEDIATEK) += u-boot-mtk.bin >>> @@ -1024,13 +1019,9 @@ PHONY += inputs >>> inputs: $(INPUTS-y) >>> >>> all: .binman_stamp inputs >>> -# Hack for sunxi which doesn't have a proper binman definition for >>> -# 64-bit boards >>> -ifneq ($(CONFIG_ARCH_SUNXI)$(CONFIG_ARM64),yy) >>> ifeq ($(CONFIG_BINMAN),y) >>> $(call if_changed,binman) >>> endif >>> -endif >>> >>> # Timestamp file to make sure that binman always runs >>> .binman_stamp: FORCE >>> @@ -1336,6 +1327,8 @@ cmd_binman = $(srctree)/tools/binman/binman $(if >>> $(BINMAN_DEBUG),-D) \ >>> $(if $(BINMAN_VERBOSE),-v$(BINMAN_VERBOSE)) \ >>> build -u -d u-boot.dtb -O . -m --allow-missing \ >>> -I . -I $(srctree) -I $(srctree)/board/$(BOARDDIR) \ >>> + -I arch/$(ARCH)/dts -a of-list=$(CONFIG_OF_LIST) \ >>> + -a atf-bl31-path=${BL31} \ >>> $(BINMAN_$(@F)) >>> >>> OBJCOPYFLAGS_u-boot.ldr.hex := -I binary -O ihex >>> @@ -1625,13 +1618,6 @@ u-boot-x86-reset16.bin: u-boot FORCE >>> >>> endif # CONFIG_X86 >>> >>> -ifneq ($(CONFIG_ARCH_SUNXI),) >>> -ifeq ($(CONFIG_ARM64),y) >>> -u-boot-sunxi-with-spl.bin: spl/sunxi-spl.bin u-boot.itb FORCE >>> - $(call if_changed,cat) >>> -endif >>> -endif >>> - >> >> Now `make u-boot-sunxi-with-spl.bin` doesn't work. >> >> This is less of an issue, but still probably breaks some scripts. It breaks >> mine, at least. > > Why do you specify a target? Doesn't it build the file without the target? Yes, the file is built either way. I provide a specific target to avoid building other files I don't need -- for example, a plain `make` used to rebuild the hello world EFI application every time. > One problem with buildman is that there is no definitely of what files > it will produce when run, or at least there is, but it is in the > binman description itself. > > This means that 'make clean' doesn't work fully, for example. I can > think of a few ways to implement this. One would be to put a list of > target files into a text file and have 'make clean' use that. We could > also have an option to tell binman to produce a list of files it would > generate if run. Then we might be able to tell binman to generate a > particular file. Yes, having binman generate a Makefile fragment would be ideal. That would also solve binman being forced to re-run every `make` invocation. For now a plain `make`/`make all` is fine. The forced rebuilds seem to be better under control now. >> >>> OBJCOPYFLAGS_u-boot-app.efi := $(OBJCOPYFLAGS_EFI) >>> u-boot-app.efi: u-boot FORCE >>> $(call if_changed,zobjcopy) >>> diff --git a/arch/arm/dts/sunxi-u-boot.dtsi b/arch/arm/dts/sunxi-u-boot.dtsi >>> index fdd4c80aa46..1d1c3691099 100644 >>> --- a/arch/arm/dts/sunxi-u-boot.dtsi >>> +++ b/arch/arm/dts/sunxi-u-boot.dtsi >>> @@ -5,14 +5,73 @@ >>> mmc1 = &mmc2; >>> }; >>> >>> - binman { >>> + binman: binman { >>> + multiple-images; >>> + }; >>> +}; >>> + >>> +&binman { >>> + u-boot-sunxi-with-spl { >>> filename = "u-boot-sunxi-with-spl.bin"; >>> pad-byte = <0xff>; >> >> style: blank line here (and above "atf" and "@config-SEQ" below). > > I'll send a fix-up patch. > >> >>> blob { >>> filename = "spl/sunxi-spl.bin"; >>> }; >>> +#ifdef CONFIG_ARM64 >>> + fit { >>> + description = "Configuration to load ATF before >>> U-Boot"; >>> + #address-cells = <1>; >>> + fit,fdt-list = "of-list"; >>> + >>> + images { >>> + uboot { >>> + description = "U-Boot (64-bit)"; >>> + type = "standalone"; >>> + arch = "arm64"; >>> + compression = "none"; >>> + load = <0x4a000000>; >>> + >>> + u-boot-nodtb { >>> + }; >>> + }; >>> + atf { >>> + description = "ARM Trusted Firmware"; >>> + type = "firmware"; >>> + arch = "arm64"; >>> + compression = "none"; >>> +/* TODO: Do this with an overwrite in this board's dtb? */ >> >> This address is determined by the physical SRAM layout, so it is per-SoC, not >> per-board. I would suggest omitting load/entry here entirely, and putting >> them >> in the $SOC-u-boot.dtsi. > > That's fine also. I just don't like having #ifdefs here. I tried implementing this, and I ran into the problem that sunxi doesn't define CONFIG_SYS_VENDOR. So this file (from CONFIG_SYS_SOC) and the board-DTS-specific file are the only two magic u-boot.dtsi files available, and I don't think we want a u-boot.dtsi for every board. A possible improvement would be defining BL31_ADDR (and later SCP_ADDR) as macros at the top of the file, like the shell script does. >>> +#ifdef CONFIG_MACH_SUN50I_H6 >>> + load = <0x104000>; >>> + entry = <0x104000>; >>> +#else >>> + load = <0x44000>; >>> + entry = <0x44000>; >>> +#endif >>> + atf-bl31 { >> >> Another regression: you need >> >> filename = "bl31.bin"; >> >> here to match the behavior when the environment variable is not defined. > > That would be better to go in the code. If U-Boot passes an empty > filename to binman then it needs special handling, if we want a > default. (merging the threads here) What special handling are you thinking of? In blob_named_by_arg.py, the default filename is only overwritten from the arg if the arg is not empty. So the code already does the right thing, matching the baseline script: if no environment variable was defined, use a file with the default name in the current directory. It just needs a default name defined here. >>> + }; >>> + }; >>> + >>> + @fdt-SEQ { >>> + description = "NAME"; >>> + type = "flat_dt"; >>> + compression = "none"; >>> + }; >>> + }; >>> + >>> + configurations { >>> + default = "config-1"; >> >> I would expect @DEFAULT-SEQ here. > > For some reason the old script just used config-1. Probably because determining the index of CONFIG_DEFAULT_DEVICE_TREE in CONFIG_OF_LIST in POSIX sh is nontrivial. sunxi SPL always explicitly chooses the config matching CONFIG_DEFAULT_DEVICE_TREE, so "1" vs "DEFAULT-SEQ" would only make a difference if CONFIG_DEFAULT_DEVICE_TREE was not included in CONFIG_OF_LIST. (If using "DEFAULT-SEQ", that would be an error, so it would require that the SPL and FIT were built separately). >>> + @config-SEQ { >>> + description = "NAME"; >>> + firmware = "uboot"; >>> + loadables = "atf"; >>> + fdt = "fdt-SEQ"; >>> + }; >>> + }; >>> + }; >>> +#else >>> u-boot-img { >>> offset = <CONFIG_SPL_PAD_TO>; >>> }; >>> +#endif >>> }; >>> }; >>> >> > > Regards, > Simon > Regards, Samuel