Hi Samuel,
On Sat, 5 Sep 2020 at 19:49, Samuel Holland <sam...@sholland.org> wrote:
>
> 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.
No that would be painful.
>
> 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.
Yes that might be better I suppose.
>
> >>> +#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.
Well Entry_aft_bl31 uses Entry_named_blob_by_arg which uses Entry_blob
which reads the filename. So we would need to do some refactoring to
avoid overriding the default filename, as I read it.
>
> >>> + };
> >>> + };
> >>> +
> >>> + @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).
OK.
>
> >>> + @config-SEQ {
> >>> + description = "NAME";
> >>> + firmware = "uboot";
> >>> + loadables = "atf";
> >>> + fdt = "fdt-SEQ";
> >>> + };
> >>> + };
> >>> + };
> >>> +#else
> >>> u-boot-img {
> >>> offset = <CONFIG_SPL_PAD_TO>;
> >>> };
> >>> +#endif
> >>> };
> >>> };
Regards,
Simon
