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.