Re: [PATCH v4 09/10] tools: spkgimage: add Renesas SPKG format

On 3/8/23 21:26, Ralph Siemsen wrote:

[...]

diff --git a/tools/Makefile b/tools/Makefile index e13effbb66..1d7abcb916 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -125,6 +125,7 @@ dumpimage-mkimage-objs := aisimage.o \ stm32image.o \ $(ROCKCHIP_OBS) \ socfpgaimage.o \

• 	spkgimage.o \
  

Maybe just call the file renesas_spkgimage.o so its clear which SoC/vendor this file is associtated with.

	sunxi_egon.o \
	lib/crc16-ccitt.o \
	lib/hash-checksum.o \

diff --git a/tools/spkgimage.c b/tools/spkgimage.c new file mode 100644 index 0000000000..ea8036e114 --- /dev/null +++ b/tools/spkgimage.c @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: BSD-2-Clause +/*

• • Generate Renesas RZ/N1 BootROM header (SPKG)
• • (C) Copyright 2022 Schneider Electric
• • Based on spkg_utility.c
• • (C) Copyright 2016 Renesas Electronics Europe Ltd
• */

+#include "imagetool.h" +#include <limits.h> +#include <image.h> +#include <stdarg.h> +#include <stdint.h> +#include <u-boot/crc.h> +#include "spkgimage.h"

+static struct spkg_file out_buf;

+static uint32_t padding;

Is this padding here and the padding in struct config_file below different padding ? Can we get rid of these static global variables ?

+/* Note: the ordering of the bitfields does not matter */ +static struct config_file {

• unsigned int version:1;
• unsigned int ecc_block_size:2;
• unsigned int ecc_enable:1;
• unsigned int ecc_scheme:3;
• unsigned int ecc_bytes:8;
• unsigned int blp_len;
• unsigned int padding;

+} conf;

+static int check_range(const char *name, int val, int min, int max) +{

• if (val < min) {

• fprintf(stderr, "Warning: param '%s' adjusted to min %d\n",
  

• 	name, min);
  

• val = min;
  

• }
• if (val > max) {

• fprintf(stderr, "Warning: param '%s' adjusted to max %d\n",
  

• 	name, max);
  

• val = max;
  

• }

There is a macro clamp() which implements range limiting .

• return val;

+}

+static int spkgimage_parse_config_line(char *line, size_t line_num) +{

• char *saveptr;
• char *delim = "\t ";
• char *name = strtok_r(line, delim, &saveptr);
• char *val_str = strtok_r(NULL, delim, &saveptr);
• int value = atoi(val_str);
• if (!strcmp("VERSION", name)) {

• conf.version = check_range(name, value, 1, 15);
  

• } else if (!strcmp("NAND_ECC_ENABLE", name)) {

• conf.ecc_enable = check_range(name, value, 0, 1);
  

• } else if (!strcmp("NAND_ECC_BLOCK_SIZE", name)) {

• conf.ecc_block_size = check_range(name, value, 0, 2);
  

• } else if (!strcmp("NAND_ECC_SCHEME", name)) {

• conf.ecc_scheme = check_range(name, value, 0, 7);
  

• } else if (!strcmp("NAND_BYTES_PER_ECC_BLOCK", name)) {

• conf.ecc_bytes = check_range(name, value, 0, 255);
  

• } else if (!strcmp("ADD_DUMMY_BLP", name)) {

• conf.blp_len = value ? SPKG_BLP_SIZE : 0;
  

• } else if (!strcmp("PADDING", name)) {

• if (strrchr(val_str, 'K'))
  

• 	value = value * 1024;
  

• else if (strrchr(val_str, 'M'))
  

• 	value = value * 1024 * 1024;
  

• conf.padding = check_range(name, value, 1, INT_MAX);
  

• } else {

• fprintf(stderr,
  

• 	"config error: unknown keyword on line %ld\n",
  

• 	line_num);
  

• return -EINVAL;
  

• }
• return 0;

+}

+static int spkgimage_parse_config_file(char *filename) +{

• FILE *fcfg;
• char line[256];
• size_t len;
• size_t line_num = 0;
• fcfg = fopen(filename, "r");
• if (!fcfg)

• return -EINVAL;
  

• while (fgets(line, sizeof(line), fcfg)) {

• line_num += 1;
  

• /* Skip blank lines and comments */
  

• if (line[0] == '\n' || line[0] == '#')
  

• 	continue;
  

• /* Strip the trailing newline */
  

• len = strlen(line);
  

• if (line[len - 1] == '\n')
  

• 	line[--len] = 0;
  

Use len - 1 here too to avoid confusion ?

• /* Parse the line */
  

• if (spkgimage_parse_config_line(line, line_num))
  

• 	return -EINVAL;
  

• }
• fclose(fcfg);
• /* Avoid divide-by-zero later on */
• if (!conf.padding)

• conf.padding = 1;
  

• return 0;

+}

[...]