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

[Marco Felsch <m.felsch@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5116 bytes --]

On 23-06-17, Jules Maselbas wrote:
> 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:

...

> > > +#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.

Hm.. at least on RISC-V I had no problems with
relocate_to_current_adr(). Also checking the ARM relocation code does
not show why the header should be a problem if
relocate_to_current_adr() is used.

> 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)

Thanks for the explanation.

> > 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.

No worries, I will start picking your patches as well for the D1
lowlevel support.

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

Yep, there is a special handling for each architecture. That been said,
I noticed that I had a fixup patch ontop of my image generation tool,
which I attached.

[-- Attachment #2: 0001-fixup-sunxi-add-image-support.patch --]
[-- Type: text/x-diff, Size: 3159 bytes --]

>From 1e9e0c9515e644abeda066c5387d1338d927cbda Mon Sep 17 00:00:00 2001
From: Marco Felsch <marco.felsch@gmail.com>
Date: Wed, 26 Apr 2023 21:51:26 +0200
Subject: [PATCH] fixup! sunxi: add image support

---
 scripts/sunxi_egon.c | 40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/scripts/sunxi_egon.c b/scripts/sunxi_egon.c
index 6d74a83ad8..b339955295 100644
--- a/scripts/sunxi_egon.c
+++ b/scripts/sunxi_egon.c
@@ -29,11 +29,12 @@ static int write_image(int infd, int outfd, enum sunxi_arch arch,
 		       unsigned long pblsize, unsigned long ofs)
 {
 	struct boot_file_head *header;
+	unsigned int header_sz;
 	uint32_t checksum = 0;
-	void *buf, *origbuf;
 	unsigned long size;
 	uint32_t *buf32;
        	uint32_t value;
+	void *buf;
 	int ret;
 	int i;
 
@@ -43,8 +44,10 @@ static int write_image(int infd, int outfd, enum sunxi_arch arch,
 	if (!buf)
 		return 1;
 
+	memset(buf, 0, size);
+
 	header = buf;
-	buf32 = buf;
+	header_sz = sizeof(*header);
 
 	/*
 	 * Different architectures need different first instruction to
@@ -53,7 +56,7 @@ static int write_image(int infd, int outfd, enum sunxi_arch arch,
 	switch (arch) {
 	case SUNXI_ARCH_ARM:
 		/* Generate an ARM branch instruction to jump over the header. */
-		value = 0xea000000 | (sizeof(*header) / 4 - 2);
+		value = 0xea000000 | (header_sz / 4 - 2);
 		header->b_instruction = cpu_to_le32(value);
 		break;
 	case SUNXI_ARCH_RISCV:
@@ -68,10 +71,10 @@ static int write_image(int infd, int outfd, enum sunxi_arch arch,
 		 * is not allowed).
 		 */
 		value = 0x0000006f |
-			((sizeof(*header) & 0x00100000) << 11) |
-			((sizeof(*header) & 0x000007fe) << 20) |
-			((sizeof(*header) & 0x00000800) << 9) |
-			((sizeof(*header) & 0x000ff000) << 0);
+			((header_sz & 0x00100000) << 11) |
+			((header_sz & 0x000007fe) << 20) |
+			((header_sz & 0x00000800) << 9) |
+			((header_sz & 0x000ff000) << 0);
 		header->b_instruction = cpu_to_le32(value);
 		break;
 	default:
@@ -85,33 +88,32 @@ static int write_image(int infd, int outfd, enum sunxi_arch arch,
 	memcpy(header->spl_signature, SPL_SIGNATURE, 3);
 	header->spl_signature[3] = SPL_ENV_HEADER_VERSION;
 
-	/* Calculate the checksum. Yes, it's that simple. */
-	for (i = 0; i < size / 4; i++)
-		checksum += le32_to_cpu(buf32[i]);
-	header->check_sum = cpu_to_le32(checksum);
-
-	origbuf = buf;
-	buf += sizeof(*header);
-	ret = read(infd, buf, pblsize);
+	ret = read(infd, buf + header_sz, pblsize);
 	if (ret > pblsize) {
 		printf("Error: While read: 0x%d > 0x%ld bytes!\n",
 		       ret, pblsize);
-		free(origbuf);
+		free(buf);
 		return 1;
 	}
 
+	/* Calculate the checksum. Yes, it's that simple. */
+	buf32 = buf;
+	for (i = 0; i < size / 4; i++)
+		checksum += le32_to_cpu(buf32[i]);
+	header->check_sum = cpu_to_le32(checksum);
+
 	if (ofs)
 		lseek(outfd, ofs, SEEK_SET);
 
-	ret = write(outfd, origbuf, size);
+	ret = write(outfd, buf, size);
 	if (ret != size) {
 		printf("Error: While write: 0x%d != 0x%ld bytes!\n",
 		       ret, size);
-		free(origbuf);
+		free(buf);
 		return 1;
 	}
 
-	free(origbuf);
+	free(buf);
 
 	return 0;
 }
-- 
2.39.2