Hi Andre,

On 11/11/20 6:52 AM, Andre Przywara wrote:
> So far we used the separate mksunxiboot tool for generating a bootable
> image for Allwinner SPLs, probably just for historical reasons.
> 
> Use the mkimage framework to generate a so called eGON image the
> Allwinner BROM expects.
> The new image type is called "sunxi_egon", to differentiate it
> from the (still to be implemented) secure boot TOC0 image.
> 
> Signed-off-by: Andre Przywara <andre.przyw...@arm.com>
> Reviewed-by: Simon Glass <s...@chromium.org>
> Reviewed-by: Jagan Teki <ja...@openedev.com>
> Tested-by: Jagan Teki <ja...@amarulasolutions.com> # BPI-M64
> 
> ---
>  common/image.c     |   1 +
>  include/image.h    |   1 +
>  tools/Makefile     |   1 +
>  tools/sunxi_egon.c | 133 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 136 insertions(+)
>  create mode 100644 tools/sunxi_egon.c
> 
> diff --git a/common/image.c b/common/image.c
> index 451fc689a89..6923dac7c07 100644
> --- a/common/image.c
> +++ b/common/image.c
> @@ -189,6 +189,7 @@ static const table_entry_t uimage_type[] = {
>       {       IH_TYPE_STM32IMAGE, "stm32image", "STMicroelectronics STM32 
> Image" },
>       {       IH_TYPE_MTKIMAGE,   "mtk_image",   "MediaTek BootROM loadable 
> Image" },
>       {       IH_TYPE_COPRO, "copro", "Coprocessor Image"},
> +     {       IH_TYPE_SUNXI_EGON, "sunxi_egon",  "Allwinner eGON Boot Image" 
> },
>       {       -1,                 "",           "",                   },
>  };
>  
> diff --git a/include/image.h b/include/image.h
> index 00bc03bebec..89bfca2155a 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -308,6 +308,7 @@ enum {
>       IH_TYPE_IMX8MIMAGE,             /* Freescale IMX8MBoot Image    */
>       IH_TYPE_IMX8IMAGE,              /* Freescale IMX8Boot Image     */
>       IH_TYPE_COPRO,                  /* Coprocessor Image for remoteproc*/
> +     IH_TYPE_SUNXI_EGON,             /* Allwinner eGON Boot Image */
>  
>       IH_TYPE_COUNT,                  /* Number of image types */
>  };
> diff --git a/tools/Makefile b/tools/Makefile
> index 51123fd9298..2c9a0c4c4e7 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -104,6 +104,7 @@ dumpimage-mkimage-objs := aisimage.o \
>                       stm32image.o \
>                       $(ROCKCHIP_OBS) \
>                       socfpgaimage.o \
> +                     sunxi_egon.o \
>                       lib/crc16.o \
>                       lib/sha1.o \
>                       lib/sha256.o \
> diff --git a/tools/sunxi_egon.c b/tools/sunxi_egon.c
> new file mode 100644
> index 00000000000..8dcc4c87413
> --- /dev/null
> +++ b/tools/sunxi_egon.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * (C) Copyright 2018 Arm Ltd.
> + */
> +
> +#include "imagetool.h"
> +#include <image.h>
> +
> +#include <asm/arch-sunxi/spl.h>
> +
> +/* checksum initialiser value */
> +#define STAMP_VALUE                     0x5F0A6C39

Using spaces here seems unintentional.

> +
> +/*
> + * NAND requires 8K padding. SD/eMMC gets away with 512 bytes,
> + * but let's use the larger padding to cover both.
> + */
> +#define PAD_SIZE                     8192
> +
> +static int egon_check_params(struct image_tool_params *params)
> +{
> +     /* We just need a binary image file. */
> +     return !params->dflag;
> +}
> +
> +static int egon_verify_header(unsigned char *ptr, int image_size,
> +                           struct image_tool_params *params)
> +{
> +     const struct boot_file_head *header = (void *)ptr;
> +     uint32_t length;
> +
> +     /* First 4 bytes must be an ARM branch instruction. */
> +     if ((le32_to_cpu(header->b_instruction) & 0xff000000) != 0xea000000)
> +             return EXIT_FAILURE;
> +
> +     if (memcmp(header->magic, BOOT0_MAGIC, 8))

sizeof(header->magic)? (same for the memcpy below)

> +             return EXIT_FAILURE;
> +
> +     length = le32_to_cpu(header->length);
> +     /* Must be at least 512 byte aligned. */
> +     if (length & 511)
> +             return EXIT_FAILURE;
> +
> +     /*
> +      * Image could also contain U-Boot proper, so could be bigger.
> +      * But it must not be shorter.
> +      */
> +     if (image_size < length)
> +             return EXIT_FAILURE;
> +
> +     return EXIT_SUCCESS;
> +}
> +
> +static void egon_print_header(const void *buf)
> +{
> +     const struct boot_file_head *header = buf;
> +
> +     printf("Allwinner eGON header, size: %d bytes\n",
> +            le32_to_cpu(header->length));

While the data being printed comes from the header, it describes the image /
image type as a whole, so "image" seems more appropriate.

> +     if (!memcmp(header->spl_signature, SPL_SIGNATURE, 3))
> +             printf("\tSPL header version %d.%d\n",
> +                    header->spl_signature[3] >> SPL_MINOR_BITS,
> +                    header->spl_signature[3] & ((1U << SPL_MINOR_BITS) - 1));
> +     if (header->spl_signature[3] >= SPL_DT_HEADER_VERSION)
> +             printf("\tDT name: %s\n",
> +                    (char *)buf + le32_to_cpu(header->dt_name_offset));

This needs to be inside the SPL_SIGNATURE check, and the DT name is only valid
if dt_name_offset is nonzero.

Maybe use a local variable for the version?

> +}
> +
> +static void egon_set_header(void *buf, struct stat *sbuf, int infd,
> +                         struct image_tool_params *params)
> +{
> +     struct boot_file_head *header = buf;
> +     uint32_t *buf32 = buf;
> +     uint32_t checksum = 0, value;
> +     int i;
> +
> +     /* Generate an ARM branch instruction to jump over the header. */
> +     value = 0xea000000 | (sizeof(struct boot_file_head) / 4 - 2);
> +     header->b_instruction = cpu_to_le32(value);
> +
> +     memcpy(header->magic, BOOT0_MAGIC, 8);
> +     header->check_sum = cpu_to_le32(STAMP_VALUE);
> +     header->length = cpu_to_le32(ALIGN(sbuf->st_size, PAD_SIZE));

When this function runs, padding has already been applied, so you can use simply
sbuf->st_size (or params->file_size).

> +
> +     memcpy(header->spl_signature, SPL_SIGNATURE, 3);
> +
> +     /* If an image name has been provided, use it as the DT name. */
> +     if (params->imagename && params->imagename[0]) {
> +             header->spl_signature[3] = SPL_DT_HEADER_VERSION;
> +
> +             value = offsetof(struct boot_file_head, string_pool);
> +             header->dt_name_offset = cpu_to_le32(value);
> +
> +             strncpy((char *)header->string_pool, params->imagename, 52);
> +             /* Make sure we have a terminating zero byte. */
> +             ((char *)header->string_pool)[51] = 0;
> +     } else
> +             header->spl_signature[3] = SPL_ENV_HEADER_VERSION;
> +
> +     /* Calculate the checksum. Yes, it's that simple. */
> +     for (i = 0; i < sbuf->st_size / 4; i++)
> +             checksum += le32_to_cpu(buf32[i]);
> +     header->check_sum = cpu_to_le32(checksum);
> +}
> +
> +static int egon_check_image_type(uint8_t type)
> +{
> +     return type == IH_TYPE_SUNXI_EGON ? 0 : 1;
> +}
> +
> +static int egon_vrec_header(struct image_tool_params *params,
> +                         struct image_type_params *tparams)
> +{
> +     tparams->hdr = calloc(sizeof(struct boot_file_head), 1);
> +
> +     /* Return padding to 8K blocks. */
> +     return PAD_SIZE - (params->file_size & (PAD_SIZE - 1));

I think "ALIGN(params->file_size, PAD_SIZE) - params->file_size;" is much 
clearer.

And for both patches:

Tested-by: Samuel Holland <sam...@sholland.org>

Cheers,
Samuel

> +}
> +
> +U_BOOT_IMAGE_TYPE(
> +     sunxi_egon,
> +     "Allwinner eGON Boot Image support",
> +     sizeof(struct boot_file_head),
> +     NULL,
> +     egon_check_params,
> +     egon_verify_header,
> +     egon_print_header,
> +     egon_set_header,
> +     NULL,
> +     egon_check_image_type,
> +     NULL,
> +     egon_vrec_header
> +);
> 

Reply via email to