Re: [PATCH v4 09/10] tools: spkgimage: add Renesas SPKG format
On 4/17/23 22:17, Ralph Siemsen wrote:
On Mon, Apr 17, 2023 at 07:23:46PM +0200, Marek Vasut wrote:
On 3/8/23 21:26, Ralph Siemsen wrote:
+ spkgimage.o \
Maybe just call the file renesas_spkgimage.o so its clear which
SoC/vendor this file is associtated with.
Okay, will do.
+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 ?
I will give it a try.
+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 .
Thanks for pointing that out. However I think there is value in the
diagnostic print when the value is clamped. Ideally it should help the
user to fix their invoking script/binman/etc.
Of course, I could call clamp() and check if the value differs, but that
seems just as complex as the check_range().
ACK
+ 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 ?
Old habit. I always try to update the length in sync with modifying the
string. If done as a separate line/statement, it is more likely to be
lost during subsequent modifications.
In this case I do not need "len" at all, so I could just do:
Nice
Re: [PATCH v4 09/10] tools: spkgimage: add Renesas SPKG format
On Mon, Apr 17, 2023 at 07:23:46PM +0200, Marek Vasut wrote:
On 3/8/23 21:26, Ralph Siemsen wrote:
+ spkgimage.o \
Maybe just call the file renesas_spkgimage.o so its clear which
SoC/vendor this file is associtated with.
Okay, will do.
+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 ?
I will give it a try.
+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 .
Thanks for pointing that out. However I think there is value in the
diagnostic print when the value is clamped. Ideally it should help the
user to fix their invoking script/binman/etc.
Of course, I could call clamp() and check if the value differs, but that
seems just as complex as the check_range().
+ 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 ?
Old habit. I always try to update the length in sync with modifying the
string. If done as a separate line/statement, it is more likely to be
lost during subsequent modifications.
In this case I do not need "len" at all, so I could just do:
line[strcspn(line, "\n")] = 0;
Ralph
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 00..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
+#include
+#include
+#include
+#include
+#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, );
+ char *val_str = strtok_r(NULL, delim, );
+ 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;
+}
[...]
