[Buildroot] [PATCHv2 4/4] Add package cbootimage-configs
Julian Scheel
julian at jusst.de
Wed Apr 20 06:18:00 UTC 2016
Hi Thomas,
thanks for the review, see comments below.
On 19.04.2016 23:08, Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 17 Mar 2016 16:03:58 +0100, Julian Scheel wrote:
>> Add package for cbootimage configurations that contains build scripts to
>> generate bct and image files for various tegra based boards using cbootimage
>> file.
>> Additionally the package makefile includes extra hooks to generate
>> self-contained flasher binaries based on u-boot, that are actually able to
>> flash a u-boot instance into mmc memory when loaded to tegra via tegrarcm.
>>
>> Signed-off-by: Julian Scheel <julian at jusst.de>
>
> To be honest, this is too complicated. This package is doing too much
> stuff in its .mk file. At some point, you mention that your .mk code is
> heavily inspired by
> https://github.com/NVIDIA/tegra-uboot-flasher-scripts. Why don't you
> use/package those scripts instead?
The problem with tegra-uboot-flasher-scripts is, that it does a lot more
than what this package does. In fact it includes build logic for all the
required tools (like dtc, tegrarcm, cbootimage, u-boot), which wouldn't
work out well in the context of buildroot. Using externally provided
builds isn't easily possible without major abuse of the scripts, which I
don't think would be merged upstream anyway.
> Or maybe this thing should do less stuff, and simply provide the
> necessary tools for a post-image script to do the job.
If in the end it's decided that this is too complicated for the package
makefile I'd rather create a dedicated package that handles this. So,
that the magic would be in done in some independently maintained script
and we only need a buildroot package that heads the options to that package.
It just felt a bit like overkill for what it does in the end.
> It would be useful if you could submit the defconfig for a Tegra based
> board as part of this series so that we can see how it all fits
> together.
Sure, I'll prepare a defconfig.
> I've added some more comments/questions below.
>
>
>
>> +if BR2_PACKAGE_CBOOTIMAGE_CONFIGS
>> +
>> +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD
>> + string "cbootimage board to build"
>
> I think the "cbootimage" prefix in the string is useless here, and in
> the other options. In menuconfig, we clearly see it's a suboption.
Ack.
>> +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED
>> + bool "sign generated image for secureboot"
>> + help
>> + Sign the image using a provided ssl key. This makes it possible to
>> + run the image in secureboot environments with the matching key being
>> + burned to the tegra fuses.
>> + Be aware that the generated image will have the extension simg
>> + instead of img if signing is enabled.
>> +
>> + Compatible key files can be generated with openssl:
>> + openssl genrsa -out rsa_priv.pem 2048
>> +
>> +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY
>> + string "pkc file name"
>> + depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED
>> + help
>> + Key file to use for signing the loader image.
>
> When there is such a bool to enable/disable + one string giving the
> argument, I believe we generally prefer to just have the string. When
> the string is empty -> feature is disabled.
Makes sense, thanks.
>> +if BR2_PACKAGE_CBOOTIMAGE_CONFIGS_GENERATE_FLASHER
>> +
>> +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT
>> + bool "write partition table to mmc 0"
>> + help
>> + Add a flashing step which is writing a custom gpt partition table
>> + onto the target devices first mmc storage
>> +
>> +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE
>> + string "partition table file"
>> + depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT
>> + default "package/cbootimage-configs/gpt-table"
>> + help
>> + Provide a file that contains the paritition table to write to mmc.
>> + Use format as specified in u-boot's README.gpt documentation file.
>> +
>> + Make sure to escape variable properly, as u-boot shall not evaluate
>> + the variables when setting the environment variable, but only when
>> + running the gpt command.
>> +
>> + Example file content:
>> + uuid_disk=\$\{disk_uuid\};name=rootfs,size=2000MiB,uuid=\$\{rootfs_uuid\};name=data,size=-,uuid=\$\{data_uuid\}
>
> Same here.
>
>> diff --git a/package/cbootimage-configs/cbootimage-configs.hash b/package/cbootimage-configs/cbootimage-configs.hash
>> new file mode 100644
>> index 0000000..a4fd518
>> --- /dev/null
>> +++ b/package/cbootimage-configs/cbootimage-configs.hash
>> @@ -0,0 +1,2 @@
>> +# Locally computed
>> +sha256 f9eaa2e88fa24ea348d6358f18e4bab58ac21c586b5b6cb5300e21a6e1d9de83 cbootimage-configs-9d45319.tar.gz
>> diff --git a/package/cbootimage-configs/cbootimage-configs.mk b/package/cbootimage-configs/cbootimage-configs.mk
>> new file mode 100644
>> index 0000000..69d6d29
>> --- /dev/null
>> +++ b/package/cbootimage-configs/cbootimage-configs.mk
>> @@ -0,0 +1,128 @@
>> +################################################################################
>> +#
>> +# cbootimage-configs
>> +#
>> +################################################################################
>> +
>> +CBOOTIMAGE_CONFIGS_VERSION = 9d45319
>> +CBOOTIMAGE_CONFIGS_SITE = $(call github,avionic-design,cbootimage-configs,$(CBOOTIMAGE_CONFIGS_VERSION))
>
> Why do you use the Avionic Design fork and not the original NVIDIA
> repository, which you reference as the upstream in your Config.in help
> text?
Just because our configurations are not yet merged to the upstream
repository. I really need to push on that, as this package really shall
use upstream. Although it might be wise to have the usual
custom-repository option like it's done for kernel and u-boot. It's not
that unlikely that people would have configs in custom repositories
which are not merged upstream.
>> +CBOOTIMAGE_CONFIGS_DEPENDENCIES = host-cbootimage uboot
>> +CBOOTIMAGE_CONFIGS_INSTALL_TARGET = NO
>> +CBOOTIMAGE_CONFIGS_INSTALL_IMAGES = YES
>> +
>> +CONFIGS_BUILD = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD))
>> +BCT_NAME = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT))
>> +IMG_NAME = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG))
>> +EXTRA_BOOTCMD = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_EXTRA_BOOTCMD))
>> +GPT_FILE = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE))
>> +DFU_MAP = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU_MAP))
>> +KEY_FILE = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY))
>
> The variable namespace in Buildroot is global, so they should always be
> prefixed by the name of the package.
Ok.
>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS),y)
>> +ifeq ($(CONFIGS_BUILD),)
>> + $(warning BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD=$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD))
>
> That's debug stuff.
Yep, already removed in my tree :)
>> + $(warning CONFIGS_BUILD=$(CONFIGS_BUILD))
>
> Ditto.
>
>> + $(error No cbootimage-configuration specififed to build. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD setting)
>> +endif
>> +ifeq ($(BCT_NAME),)
>> + $(error No bct filename specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT setting)
>> +endif
>> +ifeq ($(IMG_NAME),)
>> + $(error No img filename specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG setting)
>> +endif
>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y)
>> +ifeq ($(GPT_FILE),)
>> + $(error No gpt partitions specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE setting)
>> +endif
>> +endif
>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU),y)
>> +ifeq ($(DFU_MAP),)
>> + $(error No dfu map specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU_MAP setting)
>> +endif
>> +endif
>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED),y)
>> +ifeq ($(KEY_FILE),)
>> + $(error No key file specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY setting)
>> +endif
>> +KEY_OPTS = skb=$(KEY_FILE)
>> +else
>> +KEY_OPTS =
>
> Incorrect variable name. Also, I believe this should be separated from
> the big list of checks.
Yeah, you're probably right, that should be separated.
>> +endif
>> +
>> +TARGET_SIZE = $(TARGET_CROSS)size
>
> To be moved to package/Makefile.in I believe, in a separate patch.
Ok.
>> +endif
>> +
>> +define CBOOTIMAGE_CONFIGS_BUILD_CMDS
>> + cp $(UBOOT_BUILDDIR)/u-boot-dtb-tegra.bin $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)/u-boot.bin
>> + (cd $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD) && $(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(KEY_OPTS))
>
> use $(MAKE) -C $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)
Ack.
>> +endef
>> +
>> +# Generate a self-contained u-boot image, flashing u-boot to mmc
>> +define CBOOTIMAGE_CONFIGS_BUILD_FLASHER_CMD
>> + # u-boot bss size: ${BSS_SIZE}
>> + # u-boot-nodtb-tegra.bin size: ${UBOOT_NODTB_TEGRA_SIZE}
>> + # u-boot.dtb size: ${UBOOT_DTB_SIZE}
>> + # ${IMG_NAME} size: ${IMG_SIZE}
>> + # u-boot plus dtb size: ${UBOOT_PLUS_DTB_SIZE}
>> + # padded size: ${PADDED_SIZE}
>> + # bootcmd: $(UBOOT_FLASH_ENV)
>> + # gpt-file: $(PWD)/$(GPT_FILE)
>
> I don't see the usefulness of this comment.
Debugging. I'll remove it.
>> +
>> + cp $(UBOOT_BUILDDIR)/u-boot.dtb ${DTB_FILE}
>> + # Patch environment into u-boot dtb
>> + $(HOST_MAKE_ENV) fdtput -p -t x ${DTB_FILE} '/config' 'bootdelay' '0xfffffffe'
>> + $(HOST_MAKE_ENV) fdtput -p -t s ${DTB_FILE} '/config' 'bootcmd' $(UBOOT_FLASH_ENV)
>
> What guarantees you that fdtput is available? You need to depend on
> host-dtc to be sure that it's available.
Ack, dependency is needed.
>> +
>> + # Assemble u-boot-flasher.bin
>> + cp $(UBOOT_BUILDDIR)/u-boot-nodtb-tegra.bin $(BINARIES_DIR)/u-boot-flasher.bin
>> + cat ${DTB_FILE} >> $(BINARIES_DIR)/u-boot-flasher.bin
>
> Use $() to reference variables. Why don't you do a single cat here ?
Good question. I'll change it to single cat.
>> + truncate -s $(PADDED_SIZE) $(BINARIES_DIR)/u-boot-flasher.bin
>> + cat $(@D)/$(CONFIGS_BUILD)/$(IMG_NAME) >> $(BINARIES_DIR)/u-boot-flasher.bin
>> +endef
>> +
>> +# Computations for assembling u-boot with custom dtb mostly stolen from
>> +# https://github.com/NVIDIA/tegra-uboot-flasher-scripts
>> +# All credits go to Stephen Warren
>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_GENERATE_FLASHER),y)
>> +LOADADDR = 0x80108000 # FIXME: this is Tegra124 only
>> +BSS_SIZE = $(shell $(TARGET_SIZE) -A $(UBOOT_BUILDDIR)/u-boot | grep ".bss\b" | tr -s ' ' | cut -d ' ' -f 2)
>> +UBOOT_NODTB_TEGRA_SIZE = $(shell stat -c %s $(UBOOT_BUILDDIR)/u-boot-nodtb-tegra.bin)
>> +UBOOT_DTB_SIZE = $(shell stat -c %s $(UBOOT_BUILDDIR)/u-boot.dtb)
>> +IMG_SIZE = $(shell stat -c %s $(@D)/$(CONFIGS_BUILD)/$(IMG_NAME))
>> +IMG_SIZE_SECTORS = $(shell echo $$(( $(IMG_SIZE) / 512 )) )
>> +IMG_SIZE_SECTORS_HEX = $(shell printf '0x%x' $(IMG_SIZE_SECTORS) )
>> +UBOOT_PLUS_DTB_SIZE = $(shell echo $$(( $(UBOOT_NODTB_TEGRA_SIZE) + $(UBOOT_DTB_SIZE) )) )
>> +# Keep BSS area clear, acquire an extra 4kb for incereased dtb, align to 4k
>> +# boundary
>> +PADDED_SIZE = $(shell echo $$(( $(UBOOT_PLUS_DTB_SIZE) + $(BSS_SIZE) + (2 * 4 * 1024) - 1 & ~((4 * 1024) - 1) )) )
>> +IMAGEADDR = $(shell echo $$(( $(LOADADDR) + $(PADDED_SIZE) )) )
>> +IMAGEADDR_HEX = $(shell printf '0x%x' $(IMAGEADDR) )
>
> This is really the messy part. That's clearly way too much stuff
> encoded into a Buildroot package.
Well, yes, it's the messy part...
Just for the record, this is what it does:
- Take compiled u-boot
- Add a customized dtb providing environment for the actual flashing of
cbootimage processed u-boot into mmc
- Append the cbootimage processed u-boot to the binary, so it's self
contained
>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y)
>> +PWD = $(shell pwd)
>
> Debugging stuff?
Yes, sorry, leftover.
>> +GPT_SPEC = $(shell cat $(GPT_FILE))
>
> If it's just a string, why is it the path to a file rather than having
> direcly the value in the Config.in option?
Yann asked me to move it to a file in his review of the initial patch,
as quoting gets rather ugly if provided as Config.in option.
This was the previous default:
default
"uuid_disk=\\$$\\{disk_uuid\\};name=rootfs,size=-,uuid=\\$$\\{rootfs_uuid\\}"
And that's just for a single partition, so it becomes really messy if
you add some more.
>> +GPT_BOOTCMD = echo >>> Write partition table to MMC; gpt write mmc 0 '$(GPT_SPEC)'; mmc rescan
>> +else
>> +GPT_BOOTCMD =
>
> Not needed, the default for a variable is to be empty.
>
> Also, remember: all variables should be prefixed by the name of the
> package.
Ack.
>> +endif
>> +
>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU),y)
>> +DFU_BOOTCMD = echo >>> Enter DFU mode; setenv dfu_alt_info '$(DFU_MAP)'; dfu 0 mmc 0;
>> +else
>> +DFU_BOOTCMD =
>> +endif
>> +
>> +UBOOT_FLASH_ENV = "echo >>> Write image to MMC; mmc dev 0 1; mmc write ${IMAGEADDR_HEX} 0 ${IMG_SIZE_SECTORS_HEX}; echo >>> Configure environment; env default -f -a; saveenv; $(GPT_BOOTCMD) $(EXTRA_BOOTCMD); $(DFU_BOOTCMD) echo >>> Resetting system; reset"
>
> All this U-Boot command construction seem really project-specific, and
> I don't see why we should hardcode all that stuff in a Buildroot
> package. Buildroot should give the tools to generate such a U-Boot
> image, but should not hardcode so much logic IMO.
The hardcoded part of this is not really board specific. All it does, is
writing the bootloader to the emmc. While the tegra supports booting
from SPI as well, I've never seen a board doing it. So writing to the
emmc is what's needed on virtually any tegra based board.
In the end this uboot environment is just the tooling to generate an
automated flash writer, as the core tegra flashing tools do only support
loading images for execution, but not writing anything directly to memory.
Board specific are only the options to write a partition table and
automatically expose it via dfu for flashing root filesystems. I added
those as they appear to be rather useful to generate customized,
automated flashing solution for the tegra.
Thanks,
Julian
>> +DTB_FILE = $(@D)/u-boot-flasher.dtb
>> +
>> +# Add the actual assembly hook
>> +CBOOTIMAGE_CONFIGS_POST_INSTALL_IMAGES_HOOKS += CBOOTIMAGE_CONFIGS_BUILD_FLASHER_CMD
>> +endif
>
> Thomas
>
More information about the buildroot
mailing list