Re: [PATCH v2 02/13] scripts: Add Allwinner eGON image support

[Jules Maselbas <jmaselbas@zdiv.net>]

Hi Marco,

On June 17, 2023 12:00:57 AM GMT+02:00, Marco Felsch <m.felsch@pengutronix.de> wrote:
> Hi Jules,
> 
> since I work on the D1 support I also had to port the eGON image support
> to barebox ^^ Please see my below comments.
> 
> On 23-05-25, Jules Maselbas wrote:
> > On power-up Allwinner SoC starts in boot ROM, aka BROM, which will search
> > for an eGON image: first from the SD card, then from eMMC. If no image is
> > found then the BROM will enter into FEL mode that can be used for initial
> > programming and recovery of devices using USB.
> > 
> > The eGON header, followed by the actual image, must be located at a fixed
> > offset of 8192 bytes (4K) from the start of the disk, either SD; or eMMC.
> > 
> > The eGON header structure is adapted from u-boot: /include/sunxi_image.h,
> > the header structure is also documented on https://linux-sunxi.org/EGON
> > 
> > BROM will load, at most, the first 32KB of the image into SRAM, including
> 
> Nit: as noted on https://linux-sunxi.org it's not always limited to 32KB.
> 
> > the header itself! The jump instruction in the header needs to be patched
> > accordingly with the image size.
> > 
> > Signed-off-by: Jules Maselbas <jmaselbas@zdiv.net>
> > ---
> > rfc->v2:
> >  - removed arch/arm/mach-sunxi/egon_header.c (unused)
> >  - reworked egon_mkimage.c to handle images size not multiple of 4bytes
> >    and added comments
> > 
> >  include/mach/sunxi/egon.h |  59 ++++++++++++++++++
> >  scripts/Kconfig           |   7 +++
> >  scripts/Makefile          |   1 +
> >  scripts/egon_mkimage.c    | 122 ++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 189 insertions(+)
> >  create mode 100644 include/mach/sunxi/egon.h
> >  create mode 100644 scripts/egon_mkimage.c
> > 
> > diff --git a/include/mach/sunxi/egon.h b/include/mach/sunxi/egon.h
> > new file mode 100644
> > index 0000000000..e00992eb7d
> > --- /dev/null
> > +++ b/include/mach/sunxi/egon.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +#ifndef MACH_SUNXI_EGON_H
> > +#define MACH_SUNXI_EGON_H
> > +
> > +struct egon_header {
> > +	uint32_t branch; /* branch instruction to jump over the header */
> > +	uint8_t magic[8]; /* "eGON.BT0" or "eGON.BT1" */
> > +	uint32_t check_sum;
> > +	uint32_t length;
> > +	/*
> > +	 * We use a simplified header, only filling in what is needed
> > +	 * by the boot ROM. To be compatible with Allwinner tools we
> > +	 * would need to implement the proper fields here instead of
> > +	 * padding.
> > +	 *
> > +	 * Actually we want the ability to recognize our "sunxi" variant
> > +	 * of the SPL. To do so, let's place a special signature into the
> > +	 * "pub_head_size" field. We can reasonably expect Allwinner's
> > +	 * boot0 to always have the upper 16 bits of this set to 0 (after
> > +	 * all the value shouldn't be larger than the limit imposed by
> > +	 * SRAM size).
> > +	 * If the signature is present (at 0x14), then we know it's safe
> > +	 * to use the remaining 8 bytes (at 0x18) for our own purposes.
> > +	 * (E.g. sunxi-tools "fel" utility can pass information there.)
> > +	 */
> > +	union {
> > +		uint32_t header_size;
> > +		uint8_t spl_signature[4];
> > +	};
> > +	uint32_t fel_script_address;/* since v0.1, set by sunxi-fel */
> > +	/*
> > +	 * If the fel_uEnv_length member below is set to a non-zero value,
> > +	 * it specifies the size (byte count) of data at fel_script_address.
> > +	 * At the same time this indicates that the data is in uEnv.txt
> > +	 * compatible format, ready to be imported via "env import -t".
> > +	 */
> > +	uint32_t fel_uEnv_length;/* since v0.1, set by sunxi-fel */
> > +	/*
> > +	 * Offset of an ASCIIZ string (relative to the SPL header), which
> > +	 * contains the default device tree name (CONFIG_DEFAULT_DEVICE_TREE).
> > +	 * This is optional and may be set to NULL. Is intended to be used
> > +	 * by flash programming tools for providing nice informative messages
> > +	 * to the users.
> > +	 */
> > +	uint32_t dt_name_offset;/* since v0.2, set by mksunxiboot */
> > +	uint32_t dram_size;/* in MiB, since v0.3, set by SPL */
> > +	uint32_t boot_media;/* written here by the boot ROM */
> > +	/* A padding area (may be used for storing text strings) */
> > +	uint32_t string_pool[13];/* since v0.2, filled by mksunxiboot */
> > +	/* The header must be a multiple of 32 bytes (for VBAR alignment) */
> > +	/* And at least 64 byte (https://patchwork.ozlabs.org/patch/622173) */
> > +};
> > +#define EGON_HDR_BRANCH (0xea000000 | (sizeof(struct egon_header) / 4 - 2))
> > +#define sunxi_egon_header(section) {					\
> > +		__section(section) static const struct egon_header hdr= \
> > +			{ .branch = EGON_HDR_BRANCH, .magic = "eGON" };	\
> > +		__keep_symbolref(hdr);					\
> > +	}
> 
> Using an additional sections seems a bit odd here. We can just write the
> header within the image tool.
That's what I wanted to do in the first place but I struggled a lot to get barebox relocation working.
Having the eGON header embedded in the .text (since the header is loaded by bootrom) is the only solution i found to get the relocation working.
I am all for a better way but I really whish to have a first version applied.

> 
> > +#endif
> > diff --git a/scripts/Kconfig b/scripts/Kconfig
> > index dcd5f32d1d..7517f5b79f 100644
> > --- a/scripts/Kconfig
> > +++ b/scripts/Kconfig
> > @@ -56,6 +56,13 @@ config RK_IMAGE
> >  	help
> >  	  This enables building the image creation tool for Rockchip SoCs
> >  
> > +config EGON_IMAGE
> > +       bool "Allwinner eGON image tool" if COMPILE_HOST_TOOLS
> > +       depends on ARCH_SUNXI || COMPILE_HOST_TOOLS
> > +       default y if ARCH_SUNXI
> > +       help
> > +         This enables building the image creation tool for Allwinner sunxi SoCs
> > +
> >  config OMAP_IMAGE
> >  	bool "TI OMAP image tools" if COMPILE_HOST_TOOLS
> >  	depends on ARCH_OMAP || COMPILE_HOST_TOOLS
> > diff --git a/scripts/Makefile b/scripts/Makefile
> > index 72ad9ad7a6..13e80db7af 100644
> > --- a/scripts/Makefile
> > +++ b/scripts/Makefile
> > @@ -28,6 +28,7 @@ hostprogs-always-$(CONFIG_LAYERSCAPE_PBLIMAGE)		+= pblimage
> >  hostprogs-always-$(CONFIG_STM32_IMAGE)			+= stm32image
> >  hostprogs-always-$(CONFIG_RISCV)			+= prelink-riscv
> >  hostprogs-always-$(CONFIG_RK_IMAGE)			+= rkimage
> > +hostprogs-always-$(CONFIG_EGON_IMAGE)			+= egon_mkimage
> >  HOSTCFLAGS_rkimage = `pkg-config --cflags openssl`
> >  HOSTLDLIBS_rkimage = `pkg-config --libs openssl`
> >  KBUILD_HOSTCFLAGS += -I$(srctree)/scripts/include/
> > diff --git a/scripts/egon_mkimage.c b/scripts/egon_mkimage.c
> > new file mode 100644
> > index 0000000000..5983bdb28a
> > --- /dev/null
> > +++ b/scripts/egon_mkimage.c
> > @@ -0,0 +1,122 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <stdio.h>
> > +#include <errno.h>
> > +#include <stdlib.h>
> > +#include <stdint.h>
> > +#include <string.h>
> > +#include <linux/kernel.h>
> > +
> > +#include "../include/mach/sunxi/egon.h"
> > +
> > +#include "compiler.h"
> > +#include "common.h"
> > +#include "common.c"
> > +
> > +#define STAMP_VALUE 0x5f0a6c39
> > +
> > +static void mkimage(char *infile, char *outfile)
> > +{
> > +	struct egon_header *hdr;
> > +	uint32_t *p32;
> > +	uint32_t sum;
> > +	int i;
> > +	size_t hdr_size = sizeof(*hdr);
> > +	size_t bin_size;
> > +	size_t img_size;
> > +	void *bin;
> > +	int fd, ret;
> > +
> > +	bin = read_file(infile, &bin_size);
> > +	if (!bin) {
> > +		perror("read_file");
> > +		exit(1);
> > +	}
> > +
> > +	/* test if the binary has reserved space for the header */
> > +	hdr = bin;
> > +	if (hdr->branch == EGON_HDR_BRANCH && memcmp(hdr->magic, "eGON", 4) == 0) {
> > +		/* strip/skip existing header */
> > +		bin += hdr_size;
> > +		bin_size -= hdr_size;
> > +	}
> 
> Hm.. the 'normal' way is to write the header via the image tool, like it
> is done for the i.MX. The infile don't need to have reserved space in
> front, instead this tool should prepend the header.
Yes, thisis only to accomodate for having the header in the .text (see my reply above)

> I attached you my two patches adding the eGON image support. Since I
> work on the D1 it is RSIC-V related but the eGON image creation should
> not differ that much, maybe the offset must be adapted which can be done
> via the command line. We could skip this special section handling if my
> patches do work for you as well :)
Sounds nice, I don't when I will have time to test this.

Does the eGON header starts will a risc-v jump instruction ? Or is it still an arm32 insn ?


Cheers