[PATCH v3 08/12] sunxi: Convert 64-bit boards to use binman
Samuel Holland
samuel at sholland.org
Sun Sep 6 03:49:01 CEST 2020
Simon,
On 9/5/20 7:18 PM, Simon Glass wrote:
> On Sat, 5 Sep 2020 at 17:10, Samuel Holland <samuel at 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 <sjg at 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
More information about the U-Boot
mailing list