Re: [RFC PATCH 01/11] scripts: Add Allwinner eGON image support

[Jules Maselbas <jmaselbas@zdiv.net>]

On Thu, May 11, 2023 at 09:25:12AM +0200, Sascha Hauer wrote:
> On Thu, May 11, 2023 at 01:37:01AM +0200, 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 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
> > the header itself! The jump instruction in the header needs to be patched
> > accordingly with the image size.
> 
> Do I understand it correctly that all code needed to load the full
> barebox image needs to fit into these 32KiB?
Yes, this is done by the "xload" PBL-only image.
But for developpement I load code through USB to a different SRAM of 108KB!

> Where must this image be placed? Directly at the origin of the SD card?
> That would mean the partition table is somewhere inside the binary,
> right?
The eGON image must start exactly at 8KB offset from the origin of the SD
card. This leave room for the actual partition table.

> 
> > +
> > +#define ALIGN(x, a)		__ALIGN_MASK(x, (typeof(x))(a) - 1)
> > +#define __ALIGN_MASK(x, mask)	(((x) + (mask)) & ~(mask))
> 
> You can reuse the definition of these in scripts/include/linux/kernel.h
Ack

> > +#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;
> > +	size_t bin_size;
> > +	size_t img_size;
> > +	char *bin;
> > +	int fd, ret;
> > +
> > +	bin = read_file(infile, &bin_size);
> > +	if (!bin) {
> > +		perror("read_file");
> > +		exit(1);
> > +	}
> > +
> > +	/* the header must be a multiple of 32 bytes */
> > +	hdr_size = sizeof(*hdr);
> > +
> > +	/* test if the binary has reserved space for the header */
> > +	hdr = (void *)bin;
> 
> Declare bin as void *, this makes the explicit cast unnecessary.
> 
> > +	if (hdr->branch == EGON_HDR_BRANCH && memcmp(hdr->magic, "eGON", 4) == 0) {
> > +		/* strip the existing header */
> > +		bin += hdr_size;
> > +		bin_size -= hdr_size;
> > +	}
> > +	hdr = calloc(1, hdr_size);
> > +	if (!hdr) {
> > +		perror("malloc");
> > +		exit(1);
> > +	}
> > +
> > +	/* The total image length must be a multiple of 4K bytes */
> > +	img_size = ALIGN(hdr_size + bin_size, 4096);
> > +
> > +	hdr->check_sum = 0;
> 
> hdr is already zeroed due to the use of calloc.
Yes, i don't recall why I did this... I don't exactly recall how the
checksum is verified by the boot rom.

> > +	hdr->branch = EGON_HDR_BRANCH;
> > +	hdr->length = cpu_to_le32(img_size);
> > +	memcpy(hdr->magic, "eGON.BT0", 8);
> > +	memcpy(hdr->spl_signature, "SPL", 3);
> > +	hdr->spl_signature[3] = 0x03; /* version 0.3 */
> > +
> > +	/* calculate the checksum */
> > +	sum = STAMP_VALUE;
> > +	for (p32 = (void *) hdr, i = 0; i < hdr_size / sizeof(uint32_t); i++)
> > +		sum += le32_to_cpu(p32[i]);
> > +	for (p32 = (void *) bin, i = 0; i < bin_size / sizeof(uint32_t); i++)
> > +		sum += le32_to_cpu(p32[i]);
> 
> Does this work when bin_size is not a multiple of sizeof(uint32_t)?
> It likely is, but who knows...
bin_size should be a multiple of sizeof(uint32_t), if that's not the case
I don't think that the result will be correct.
But I could add something to handle this exact case.

> You are calculating the checksum up to bin_size, not including the final
> alignment. That works because the alignment is filled with 0x0 which
> doesn't add to this checksum. That is ok, but maybe it's worth noting
> that in the code.
Yes you're right, I'll add a comment, and also on ftruncate that is used
to pad the image with zero.

> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>