[PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Pali Rohár]

This patch series fixes generating images in kwbimage format, main fix
is setting correct load address of U-Boot SPL. Also it adds support for
generating kwbimage config file from existing kwbimage file via
dumpimage tool.

Pali Rohár (16):
  tools: kwbimage: Mark all local functions as static
  tools: kwbimage: Deduplicate v1 regtype header finishing
  tools: kwbimage: Fix generating image with multiple DATA_DELAY
    commands
  tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY
    commands
  arm: mvebu: Generate kwbimage.cfg with $(call cmd,...)
  tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY
    command
  tools: kwbimage: Check the return value of image_headersz_v1()
  arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in
    kwbimage
  arm: mvebu: Enable BootROM output on A38x
  tools: kwbimage: Add missing check for maximal value for DATA_DELAY
  tools: kwbimage: Show binary image address in mkimage -l, in addition
    to size
  tools: kwbimage: Dump kwbimage config file on '-p -1' option
  tools: kwbimage: Do not cast const pointers to non-const pointers
  tools: kwbimage/kwboot: Check ext field for non-zero value
  tools: kwbimage: Extract main data image without -p arg for dumpimage
  tools: kwbimage: Fix mkimage/dumpimage -l argument

 arch/arm/mach-mvebu/Makefile        |  17 +-
 arch/arm/mach-mvebu/kwbimage.cfg.in |   7 +-
 tools/kwbimage.c                    | 494 ++++++++++++++++++++++------
 tools/kwbimage.h                    |  10 +-
 tools/kwboot.c                      |   4 +-
 5 files changed, 421 insertions(+), 111 deletions(-)

-- 
2.20.1

[PATCH u-boot-marvell 01/16] tools: kwbimage: Mark all local functions as static

[Pali Rohár]

Mark all local functions as static.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 875f636c7a4f..9f852923dc61 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -197,7 +197,7 @@ static const char *image_boot_mode_name(unsigned int id)
 	return NULL;
 }
 
-int image_boot_mode_id(const char *boot_mode_name)
+static int image_boot_mode_id(const char *boot_mode_name)
 {
 	int i;
 
@@ -208,7 +208,7 @@ int image_boot_mode_id(const char *boot_mode_name)
 	return -1;
 }
 
-int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
+static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
 {
 	int i;
 
@@ -600,7 +600,8 @@ static int kwb_export_pubkey(RSA *key, struct pubkey_der_v1 *dst, FILE *hashf,
 	return 0;
 }
 
-int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig, char *signame)
+static int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig,
+		    char *signame)
 {
 	EVP_PKEY *evp_key;
 	EVP_MD_CTX *ctx;
@@ -660,8 +661,8 @@ err_key:
 	return ret;
 }
 
-int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
-	       char *signame)
+static int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
+		      char *signame)
 {
 	EVP_PKEY *evp_key;
 	EVP_MD_CTX *ctx;
@@ -720,8 +721,8 @@ err_key:
 	return ret;
 }
 
-int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
-			char *signame)
+static int kwb_sign_and_verify(RSA *key, void *data, int datasz,
+			       struct sig_v1 *sig, char *signame)
 {
 	if (kwb_sign(key, data, datasz, sig, signame) < 0)
 		return -1;
@@ -733,7 +734,7 @@ int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
 }
 
 
-int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr)
+static int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr)
 {
 	struct hash_v1 kak_pub_hash;
 	struct image_cfg_element *e;
@@ -1049,9 +1050,9 @@ static size_t image_headersz_v1(int *hasext)
 	return image_headersz_align(headersz, image_get_bootfrom());
 }
 
-int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
-			 struct image_cfg_element *binarye,
-			 struct main_hdr_v1 *main_hdr)
+static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
+				struct image_cfg_element *binarye,
+				struct main_hdr_v1 *main_hdr)
 {
 	struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;
 	uint32_t add_args;
@@ -1133,7 +1134,7 @@ err_close:
 	return -1;
 }
 
-int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
+static int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
 {
 	FILE *hashf;
 	int res;
@@ -1152,8 +1153,8 @@ int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
 	return res < 0 ? 1 : 0;
 }
 
-int kwb_sign_csk_with_kak(struct image_tool_params *params,
-			  struct secure_hdr_v1 *secure_hdr, RSA *csk)
+static int kwb_sign_csk_with_kak(struct image_tool_params *params,
+				 struct secure_hdr_v1 *secure_hdr, RSA *csk)
 {
 	RSA *kak = NULL;
 	RSA *kak_pub = NULL;
@@ -1194,9 +1195,9 @@ int kwb_sign_csk_with_kak(struct image_tool_params *params,
 	return 0;
 }
 
-int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
-			 int payloadsz, size_t headersz, uint8_t *image,
-			 struct secure_hdr_v1 *secure_hdr)
+static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
+				int payloadsz, size_t headersz, uint8_t *image,
+				struct secure_hdr_v1 *secure_hdr)
 {
 	struct image_cfg_element *e_jtagdelay;
 	struct image_cfg_element *e_boxid;
@@ -1413,7 +1414,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	return image;
 }
 
-int recognize_keyword(char *keyword)
+static int recognize_keyword(char *keyword)
 {
 	int kw_id;
 
-- 
2.20.1

[PATCH u-boot-marvell 02/16] tools: kwbimage: Deduplicate v1 regtype header finishing

[Pali Rohár]

Deduplicate code that finishes OPT_HDR_V1_REGISTER_TYPE header by
extracing it into separate function.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 9f852923dc61..9ac74cfbe514 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1247,6 +1247,22 @@ static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
 	return 0;
 }
 
+static void finish_register_set_header_v1(uint8_t **cur, uint8_t **next_ext,
+					  struct register_set_hdr_v1 *register_set_hdr,
+					  int *datai, uint8_t delay)
+{
+	int size = sizeof(struct register_set_hdr_v1) + 8 * (*datai) + 4;
+
+	register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
+	register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
+	register_set_hdr->headersz_msb = size >> 16;
+	register_set_hdr->data[*datai].last_entry.delay = delay;
+	*cur += size;
+	**next_ext = 1;
+	*next_ext = &register_set_hdr->data[*datai].last_entry.next;
+	*datai = 0;
+}
+
 static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 			     uint8_t *ptr, int payloadsz)
 {
@@ -1259,7 +1275,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	uint8_t *image, *cur;
 	int hasext = 0;
 	uint8_t *next_ext = NULL;
-	int cfgi, datai, size;
+	int cfgi, datai;
 
 	/*
 	 * Calculate the size of the header and the size of the
@@ -1357,15 +1373,8 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 		    e->type != IMAGE_CFG_DATA_DELAY)
 			continue;
 		if (e->type == IMAGE_CFG_DATA_DELAY) {
-			size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
-			register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
-			register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
-			register_set_hdr->headersz_msb = size >> 16;
-			register_set_hdr->data[datai].last_entry.delay = e->regdata_delay;
-			cur += size;
-			*next_ext = 1;
-			next_ext = &register_set_hdr->data[datai].last_entry.next;
-			datai = 0;
+			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
+						      &datai, e->regdata_delay);
 			continue;
 		}
 		register_set_hdr->data[datai].entry.address =
@@ -1375,15 +1384,9 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 		datai++;
 	}
 	if (datai != 0) {
-		size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
-		register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
-		register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
-		register_set_hdr->headersz_msb = size >> 16;
-		/* Set delay to the smallest possible value 1ms. */
-		register_set_hdr->data[datai].last_entry.delay = 1;
-		cur += size;
-		*next_ext = 1;
-		next_ext = &register_set_hdr->data[datai].last_entry.next;
+		/* Set delay to the smallest possible value. */
+		finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
+					      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
 	}
 
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
-- 
2.20.1

[PATCH u-boot-marvell 03/16] tools: kwbimage: Fix generating image with multiple DATA_DELAY commands

[Pali Rohár]

Register set header consists of sequence of DATA commands followed by
exactly one DATA_DELAY command. Thus if we are generating image with
multiple DATA_DELAY commands, we need to create more register set headers.

Fix calculation of image size with multiple DATA_DELAY commands and
correctly set pointer to struct register_set_hdr_v1 when initializing new
register set header.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 9ac74cfbe514..6432551065ab 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -991,7 +991,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 
 static size_t image_headersz_v1(int *hasext)
 {
-	struct image_cfg_element *binarye;
+	struct image_cfg_element *binarye, *e;
 	unsigned int count;
 	size_t headersz;
 	int cfgi;
@@ -1008,7 +1008,18 @@ static size_t image_headersz_v1(int *hasext)
 			*hasext = 1;
 	}
 
-	count = image_count_options(IMAGE_CFG_DATA);
+	count = 0;
+	for (cfgi = 0; cfgi < cfgn; cfgi++) {
+		e = &image_cfg[cfgi];
+
+		if (e->type == IMAGE_CFG_DATA)
+			count++;
+
+		if (e->type == IMAGE_CFG_DATA_DELAY) {
+			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
+			count = 0;
+		}
+	}
 	if (count > 0)
 		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 
@@ -1366,12 +1377,13 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	}
 
 	datai = 0;
-	register_set_hdr = (struct register_set_hdr_v1 *)cur;
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
 		e = &image_cfg[cfgi];
 		if (e->type != IMAGE_CFG_DATA &&
 		    e->type != IMAGE_CFG_DATA_DELAY)
 			continue;
+		if (datai == 0)
+			register_set_hdr = (struct register_set_hdr_v1 *)cur;
 		if (e->type == IMAGE_CFG_DATA_DELAY) {
 			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
 						      &datai, e->regdata_delay);
-- 
2.20.1

[PATCH u-boot-marvell 04/16] tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY commands

[Pali Rohár]

Preserve the order of BINARY, DATA and DATA_DELAY commands as they appear
in the input file. They may depend on each other.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 58 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 6432551065ab..f298883b2cb0 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1015,7 +1015,8 @@ static size_t image_headersz_v1(int *hasext)
 		if (e->type == IMAGE_CFG_DATA)
 			count++;
 
-		if (e->type == IMAGE_CFG_DATA_DELAY) {
+		if (e->type == IMAGE_CFG_DATA_DELAY ||
+		    (e->type == IMAGE_CFG_BINARY && count > 0)) {
 			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 			count = 0;
 		}
@@ -1287,6 +1288,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	int hasext = 0;
 	uint8_t *next_ext = NULL;
 	int cfgi, datai;
+	uint8_t delay;
 
 	/*
 	 * Calculate the size of the header and the size of the
@@ -1380,34 +1382,50 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
 		e = &image_cfg[cfgi];
 		if (e->type != IMAGE_CFG_DATA &&
-		    e->type != IMAGE_CFG_DATA_DELAY)
+		    e->type != IMAGE_CFG_DATA_DELAY &&
+		    e->type != IMAGE_CFG_BINARY)
 			continue;
+
 		if (datai == 0)
 			register_set_hdr = (struct register_set_hdr_v1 *)cur;
-		if (e->type == IMAGE_CFG_DATA_DELAY) {
+
+		/* If delay is not specified, use the smallest possible value. */
+		if (e->type == IMAGE_CFG_DATA_DELAY)
+			delay = e->regdata_delay;
+		else
+			delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
+
+		/*
+		 * DATA_DELAY command is the last entry in the register set
+		 * header and BINARY command inserts new binary header.
+		 * Therefore BINARY command requires to finish register set
+		 * header if some DATA command was specified. And DATA_DELAY
+		 * command automatically finish register set header even when
+		 * there was no DATA command.
+		 */
+		if (e->type == IMAGE_CFG_DATA_DELAY ||
+		    (e->type == IMAGE_CFG_BINARY && datai != 0))
 			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
-						      &datai, e->regdata_delay);
-			continue;
+						      &datai, delay);
+
+		if (e->type == IMAGE_CFG_DATA) {
+			register_set_hdr->data[datai].entry.address =
+				cpu_to_le32(e->regdata.raddr);
+			register_set_hdr->data[datai].entry.value =
+				cpu_to_le32(e->regdata.rdata);
+			datai++;
+		}
+
+		if (e->type == IMAGE_CFG_BINARY) {
+			if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
+				return NULL;
 		}
-		register_set_hdr->data[datai].entry.address =
-			cpu_to_le32(e->regdata.raddr);
-		register_set_hdr->data[datai].entry.value =
-			cpu_to_le32(e->regdata.rdata);
-		datai++;
 	}
 	if (datai != 0) {
 		/* Set delay to the smallest possible value. */
+		delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
 		finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
-					      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
-	}
-
-	for (cfgi = 0; cfgi < cfgn; cfgi++) {
-		e = &image_cfg[cfgi];
-		if (e->type != IMAGE_CFG_BINARY)
-			continue;
-
-		if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
-			return NULL;
+					      &datai, delay);
 	}
 
 	if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz + headersz,
-- 
2.20.1

[PATCH u-boot-marvell 05/16] arm: mvebu: Generate kwbimage.cfg with $(call cmd, ...)

[Pali Rohár]

Usage of $(call cmd,...) is standard way to call other commands which
generate things.

It also has the advantage of printing build information in the form
  KWBCFG  arch/arm/mach-mvebu/kwbimage.cfg
if verbosity is disabled, and printing the build command otherwise.

Note that the '#' character needs to be escaped in Makefile when used as
value for make variable assignment.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 7e9c206ed6b8..acbaa6449d3d 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -58,10 +58,13 @@ KWB_REPLACE += SEC_FUSE_DUMP
 KWB_CFG_SEC_FUSE_DUMP = a38x
 endif
 
+quiet_cmd_kwbcfg = KWBCFG  $@
+cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
+	<$< >$(dir $@)$(@F)
+
 $(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \
 		include/config/auto.conf
-	$(Q)sed -ne '$(foreach V,$(KWB_REPLACE),s/^#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
-	<$< >$(dir $@)$(@F)
+	$(call cmd,kwbcfg)
 
 endif # CONFIG_SPL_BUILD
 obj-y	+= gpio.o
-- 
2.20.1

[PATCH u-boot-marvell 06/16] tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY command

[Pali Rohár]

ARM executable code included in kwbimage binary header, which is not
position independent, needs to be loaded and executed by BootROM at the
correct fixed address.

Armada BootROMs load kwbimage header (in which the executable code is also
stored) at fixed address 0x40000000, which seems to be mapped to CESA SRAM.

Thus the only way to specify load and execute address of this executable
code in binary kwbimage header is by filling dummy arguments into the
binary header, using the same mechanism we already have for achieving
128-bit boundary alignment on A370 and AXP SoCs.

Extend kwbimage config file parser to allow to specify load address as
part of BINARY command with syntax:

    BINARY path_to_binary arg1 arg2 ... argN LOAD_ADDRESS address

If the specified load address is invalid or cannot be used, mkimage will
throw fatal error and exit. This will prevent generating kwbimage with
invalid load address for non-position independent binary code.

If no load address is specified, kwbimage will not fill any the dummy
arguments, thus it will behave the same as before this change.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 92 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 76 insertions(+), 16 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index f298883b2cb0..c21ed7b23fcf 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -153,6 +153,7 @@ struct image_cfg_element {
 		unsigned int bootfrom;
 		struct {
 			const char *file;
+			unsigned int loadaddr;
 			unsigned int args[BINARY_MAX_ARGS];
 			unsigned int nargs;
 		} binary;
@@ -991,10 +992,12 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 
 static size_t image_headersz_v1(int *hasext)
 {
-	struct image_cfg_element *binarye, *e;
+	struct image_cfg_element *e;
 	unsigned int count;
 	size_t headersz;
+	struct stat s;
 	int cfgi;
+	int ret;
 
 	/*
 	 * Calculate the size of the header and the size of the
@@ -1020,19 +1023,11 @@ static size_t image_headersz_v1(int *hasext)
 			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 			count = 0;
 		}
-	}
-	if (count > 0)
-		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 
-	for (cfgi = 0; cfgi < cfgn; cfgi++) {
-		int ret;
-		struct stat s;
-
-		binarye = &image_cfg[cfgi];
-		if (binarye->type != IMAGE_CFG_BINARY)
+		if (e->type != IMAGE_CFG_BINARY)
 			continue;
 
-		ret = stat(binarye->binary.file, &s);
+		ret = stat(e->binary.file, &s);
 		if (ret < 0) {
 			char cwd[PATH_MAX];
 			char *dir = cwd;
@@ -1047,18 +1042,48 @@ static size_t image_headersz_v1(int *hasext)
 				"Didn't find the file '%s' in '%s' which is mandatory to generate the image\n"
 				"This file generally contains the DDR3 training code, and should be extracted from an existing bootable\n"
 				"image for your board. Use 'dumpimage -T kwbimage -p 0' to extract it from an existing image.\n",
-				binarye->binary.file, dir);
+				e->binary.file, dir);
 			return 0;
 		}
 
 		headersz += sizeof(struct opt_hdr_v1) + sizeof(uint32_t) +
-			(binarye->binary.nargs) * sizeof(uint32_t);
-		headersz = ALIGN(headersz, 16);
+			(e->binary.nargs) * sizeof(uint32_t);
+
+		if (e->binary.loadaddr) {
+			/*
+			 * BootROM loads kwbimage header (in which the
+			 * executable code is also stored) to address
+			 * 0x40000000. Thus there is restriction for the
+			 * load address of the N-th BINARY image.
+			 */
+			unsigned int low_addr, high_addr;
+
+			low_addr = 0x40000000 + headersz;
+			high_addr = low_addr +
+				    (BINARY_MAX_ARGS - e->binary.nargs) * sizeof(uint32_t);
+
+			if (e->binary.loadaddr % 4 || e->binary.loadaddr < low_addr ||
+			    e->binary.loadaddr > high_addr) {
+				fprintf(stderr,
+					"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
+					"Address must be 4-byte aligned and in range 0x%08x-0x%08x\n.",
+					e->binary.loadaddr, e->binary.file,
+					e->binary.nargs, low_addr, high_addr);
+				return 0;
+			}
+			headersz = e->binary.loadaddr - 0x40000000;
+		} else {
+			headersz = ALIGN(headersz, 16);
+		}
+
 		headersz += ALIGN(s.st_size, 4) + sizeof(uint32_t);
 		if (hasext)
 			*hasext = 1;
 	}
 
+	if (count > 0)
+		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
+
 	return image_headersz_align(headersz, image_get_bootfrom());
 }
 
@@ -1104,11 +1129,16 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
 	/*
 	 * ARM executable code inside the BIN header on some mvebu platforms
 	 * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
+	 * In the case when this code is not position independent (e.g. ARM
+	 * SPL), it must be placed at fixed load and execute address.
 	 * This requirement can be met by inserting dummy arguments into
 	 * BIN header, if needed.
 	 */
 	offset = *cur - (uint8_t *)main_hdr;
-	add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
+	if (binarye->binary.loadaddr)
+		add_args = (binarye->binary.loadaddr - 0x40000000 - offset) / sizeof(uint32_t);
+	else
+		add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
 	if (add_args) {
 		*(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args);
 		*cur += add_args * sizeof(uint32_t);
@@ -1520,10 +1550,40 @@ static int image_create_config_parse_oneline(char *line,
 		el->binary.file = strdup(value1);
 		while (1) {
 			char *value = strtok_r(NULL, delimiters, &saveptr);
+			char *endptr;
 
 			if (!value)
 				break;
-			el->binary.args[argi] = strtoul(value, NULL, 16);
+
+			if (!strcmp(value, "LOAD_ADDRESS")) {
+				value = strtok_r(NULL, delimiters, &saveptr);
+				if (!value) {
+					fprintf(stderr,
+						"Missing address argument for BINARY LOAD_ADDRESS\n");
+					return -1;
+				}
+				el->binary.loadaddr = strtoul(value, &endptr, 16);
+				if (*endptr) {
+					fprintf(stderr,
+						"Invalid argument '%s' for BINARY LOAD_ADDRESS\n",
+						value);
+					return -1;
+				}
+				value = strtok_r(NULL, delimiters, &saveptr);
+				if (value) {
+					fprintf(stderr,
+						"Unexpected argument '%s' after BINARY LOAD_ADDRESS\n",
+						value);
+					return -1;
+				}
+				break;
+			}
+
+			el->binary.args[argi] = strtoul(value, &endptr, 16);
+			if (*endptr) {
+				fprintf(stderr, "Invalid argument '%s' for BINARY\n", value);
+				return -1;
+			}
 			argi++;
 			if (argi >= BINARY_MAX_ARGS) {
 				fprintf(stderr,
-- 
2.20.1

[PATCH u-boot-marvell 07/16] tools: kwbimage: Check the return value of image_headersz_v1()

[Pali Rohár]

Function image_headersz_v1() may return zero on fatal errors.
In this case the function already printed an error message.

Check the return value of image_headersz_v1() in kwbimage_generate(),
and exit on zero value with EXIT_FAILURE.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index c21ed7b23fcf..7759a5f94098 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1984,6 +1984,10 @@ static int kwbimage_generate(struct image_tool_params *params,
 
 	case 1:
 		alloc_len = image_headersz_v1(NULL);
+		if (!alloc_len) {
+			free(image_cfg);
+			exit(EXIT_FAILURE);
+		}
 		break;
 
 	default:
-- 
2.20.1

[PATCH u-boot-marvell 08/16] arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in kwbimage

[Pali Rohár]

U-Boot SPL for mvebu platform is not compiled as position independent.
Therefore it is required to instruct BootROM to load U-Boot SPL at the
correct address. Loading of kwbimage binary code at specific address can be
now achieved by the new LOAD_ADDRESS token as part of BINARY command in
kwbimage config file.

Update mvebu Makefile to put value of $(CONFIG_SPL_TEXT_BASE) into
LOAD_ADDRESS token when generating kwbimage.cfg from kwbimage.cfg.in.

It is required to update regex for sed to find replacement tokens at any
position on a line in kwbimage config file and not only at the beginning of
the line. This is because LOAD_ADDRESS is specified at the end of line
containing the BINARY command.

It looks like all Armada boards set CONFIG_SPL_TEXT_BASE to value
0x40000030. Why this value? It is because main kwbimage header is at
address 0x40000000 and it is 32 bytes long. After the main header there is
the binary header, which consist of 1 byte for type, 3 bytes for size, 1
byte for number of arguments, 3 reserved bytes and then 4 bytes for each
argument. After these arguments comes the executable code.

So arguments start at address 0x40000028. Before commit e6571f38c943 ("arm:
mvebu: Remove dummy BIN header arguments for SPL binary") there were two
(dummy) arguments, which resulted in load address of 0x40000030, always.
After that commit (which removed dummy arguments), load address stayed same
due to the 128-bit alignment done by mkimage.

This patch now reflects the dependency between $(CONFIG_SPL_TEXT_BASE),
load address and dummy kwbimage arguments, and allows the user to adjust
$(CONFIG_SPL_TEXT_BASE) config option to some other value.

For unsupported values, when mkimage/kwbimage cannot set chosen load address
as specified by $(CONFIG_SPL_TEXT_BASE), the build process now fails,
instead of silently generating non-working kwbimage.

Removal of this alignment between $(CONFIG_SPL_TEXT_BASE) and LOAD_ADDRESS
can only be done by compiling U-Boot SPL as position independent. But this
currently is not possible for 32-bit ARM version of U-Boot SPL.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/Makefile        | 5 ++++-
 arch/arm/mach-mvebu/kwbimage.cfg.in | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index acbaa6449d3d..0fc638086ee5 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -30,6 +30,9 @@ obj-$(CONFIG_MVEBU_EFUSE) += efuse.o
 
 extra-y += kwbimage.cfg
 
+KWB_REPLACE += LOAD_ADDRESS
+KWB_CFG_LOAD_ADDRESS = $(CONFIG_SPL_TEXT_BASE)
+
 KWB_REPLACE += BOOT_FROM
 ifneq ($(CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI),)
 	KWB_CFG_BOOT_FROM=spi
@@ -59,7 +62,7 @@ KWB_CFG_SEC_FUSE_DUMP = a38x
 endif
 
 quiet_cmd_kwbcfg = KWBCFG  $@
-cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
+cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
 	<$< >$(dir $@)$(@F)
 
 $(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \
diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
index 049d23c6ef08..2791c21617b3 100644
--- a/arch/arm/mach-mvebu/kwbimage.cfg.in
+++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
@@ -8,5 +8,5 @@ VERSION		1
 # Boot Media configurations
 #@BOOT_FROM
 
-# Binary Header (bin_hdr) with DDR3 training code
-BINARY spl/u-boot-spl.bin
+# Include U-Boot SPL with DDR3 training code into Binary Header
+BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS
-- 
2.20.1

[PATCH u-boot-marvell 09/16] arm: mvebu: Enable BootROM output on A38x

[Pali Rohár]

BootROMs on pre-A38x SoCs enabled its output on UART by default, but A38x'
BootROM has its output on UART disabled by default.

To enable BootROM output on A38x SoC, it is required to set DEBUG flag
(which only enables BootROM output and nothing more) in kwbimage. For UART
images this DEBUG flag is ignored by BootROM.

Enable kwbimage DEBUG flag for all A38x boards.

With this change BootROM prints the following (success) information on UART
before booting U-Boot kwbimage:

  BootROM - 1.73
  Booting from SPI flash

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/Makefile        | 7 +++++++
 arch/arm/mach-mvebu/kwbimage.cfg.in | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 0fc638086ee5..4e15101a40cf 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -61,6 +61,13 @@ KWB_REPLACE += SEC_FUSE_DUMP
 KWB_CFG_SEC_FUSE_DUMP = a38x
 endif
 
+ifdef CONFIG_ARMADA_38X
+# BootROM output is by default enabled on pre-A38x and disabled on A38x
+# DEBUG flag on A38x for non-UART boot source only enable BootROM output and nothing more
+KWB_REPLACE += DEBUG
+KWB_CFG_DEBUG = 1
+endif
+
 quiet_cmd_kwbcfg = KWBCFG  $@
 cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
 	<$< >$(dir $@)$(@F)
diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
index 2791c21617b3..75f90766dda4 100644
--- a/arch/arm/mach-mvebu/kwbimage.cfg.in
+++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
@@ -8,5 +8,8 @@ VERSION		1
 # Boot Media configurations
 #@BOOT_FROM
 
+# Enable BootROM output via DEBUG flag on SoCs which require it
+#@DEBUG
+
 # Include U-Boot SPL with DDR3 training code into Binary Header
 BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS
-- 
2.20.1

[PATCH u-boot-marvell 10/16] tools: kwbimage: Add missing check for maximal value for DATA_DELAY

[Pali Rohár]

Data delay is stored as 8-bit number in kwbimage structure. Ensure the
given value is at most 255.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 7759a5f94098..4b3b40ce24fb 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1610,6 +1610,10 @@ static int image_create_config_parse_oneline(char *line,
 			el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_SDRAM_SETUP;
 		else
 			el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_MS(strtoul(value1, NULL, 10));
+		if (el->regdata_delay > 255) {
+			fprintf(stderr, "Maximal DATA_DELAY is 255\n");
+			return -1;
+		}
 		break;
 	case IMAGE_CFG_BAUDRATE:
 		el->baudrate = strtoul(value1, NULL, 10);
-- 
2.20.1

[PATCH u-boot-marvell 11/16] tools: kwbimage: Show binary image address in mkimage -l, in addition to size

[Pali Rohár]

For debugging purposes it is good to know where the binary image would be
loaded and also it is needed to know if printed size is image size or the
size of header together with image.

Make it unambiguous by showoing that printed size is not the size of the
whole header, but only the size of executable code, and print also the
load/execute address of this binary image.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 4b3b40ce24fb..094ebb1049c3 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1823,9 +1823,12 @@ static void kwbimage_print_header(const void *ptr)
 
 	for_each_opt_hdr_v1 (ohdr, mhdr) {
 		if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {
-			printf("BIN Hdr Size: ");
+			printf("BIN Img Size: ");
 			genimg_print_size(opt_hdr_v1_size(ohdr) - 12 -
 					  4 * ohdr->data[0]);
+			printf("BIN Img Addr: %08x\n", 0x40000000 +
+				(unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
+				8 + 4 * ohdr->data[0]);
 		}
 	}
 
-- 
2.20.1

[PATCH u-boot-marvell 12/16] tools: kwbimage: Dump kwbimage config file on '-p -1' option

[Pali Rohár]

To regenerate kwbimage from existing image, it is needed to have kwbimage
config file. Add a new option to generate kwbimage config file from
existing kwbimage when '-p 1' option is given.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 175 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 094ebb1049c3..2a0216a537ba 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -209,6 +209,17 @@ static int image_boot_mode_id(const char *boot_mode_name)
 	return -1;
 }
 
+static const char *image_nand_ecc_mode_name(unsigned int id)
+{
+	int i;
+
+	for (i = 0; nand_ecc_modes[i].name; i++)
+		if (nand_ecc_modes[i].id == id)
+			return nand_ecc_modes[i].name;
+
+	return NULL;
+}
+
 static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
 {
 	int i;
@@ -343,6 +354,29 @@ static uint32_t image_checksum32(void *start, uint32_t len)
 	return csum;
 }
 
+static unsigned int options_to_baudrate(uint8_t options)
+{
+	switch (options & 0x7) {
+	case MAIN_HDR_V1_OPT_BAUD_2400:
+		return 2400;
+	case MAIN_HDR_V1_OPT_BAUD_4800:
+		return 4800;
+	case MAIN_HDR_V1_OPT_BAUD_9600:
+		return 9600;
+	case MAIN_HDR_V1_OPT_BAUD_19200:
+		return 19200;
+	case MAIN_HDR_V1_OPT_BAUD_38400:
+		return 38400;
+	case MAIN_HDR_V1_OPT_BAUD_57600:
+		return 57600;
+	case MAIN_HDR_V1_OPT_BAUD_115200:
+		return 115200;
+	case MAIN_HDR_V1_OPT_BAUD_DEFAULT:
+	default:
+		return 0;
+	}
+}
+
 static uint8_t baudrate_to_option(unsigned int baudrate)
 {
 	switch (baudrate) {
@@ -2034,6 +2068,143 @@ static int kwbimage_generate(struct image_tool_params *params,
 		return 4 + (4 - s.st_size % 4) % 4;
 }
 
+static int kwbimage_generate_config(void *ptr, struct image_tool_params *params)
+{
+	struct main_hdr_v0 *mhdr0 = (struct main_hdr_v0 *)ptr;
+	struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
+	size_t header_size = kwbheader_size(ptr);
+	struct register_set_hdr_v1 *regset_hdr;
+	struct ext_hdr_v0_reg *regdata;
+	struct ext_hdr_v0 *ehdr0;
+	struct opt_hdr_v1 *ohdr;
+	int cur_idx;
+	int version;
+	FILE *f;
+	int i;
+
+	f = fopen(params->outfile, "w");
+	if (!f) {
+		fprintf(stderr, "Can't open \"%s\": %s\n", params->outfile, strerror(errno));
+		return -1;
+	}
+
+	version = kwbimage_version(ptr);
+
+	if (version != 0)
+		fprintf(f, "VERSION %d\n", version);
+
+	fprintf(f, "BOOT_FROM %s\n", image_boot_mode_name(mhdr->blockid) ?: "<unknown>");
+
+	if (version == 0 && mhdr->blockid == IBR_HDR_NAND_ID)
+		fprintf(f, "NAND_ECC_MODE %s\n", image_nand_ecc_mode_name(mhdr0->nandeccmode));
+
+	if (mhdr->blockid == IBR_HDR_NAND_ID)
+		fprintf(f, "NAND_PAGE_SIZE 0x%x\n", (unsigned)mhdr->nandpagesize);
+
+	if (version != 0 && mhdr->blockid == IBR_HDR_NAND_ID) {
+		fprintf(f, "NAND_BLKSZ 0x%x\n", (unsigned)mhdr->nandblocksize);
+		fprintf(f, "NAND_BADBLK_LOCATION 0x%x\n", (unsigned)mhdr->nandbadblklocation);
+	}
+
+	if (version == 0 && mhdr->blockid == IBR_HDR_SATA_ID)
+		fprintf(f, "SATA_PIO_MODE %u\n", (unsigned)mhdr0->satapiomode);
+
+	/*
+	 * Addresses and sizes which are specified by mkimage command line
+	 * arguments and not in kwbimage config file
+	 */
+
+	if (version != 0)
+		fprintf(f, "#HEADER_SIZE 0x%x\n",
+			((unsigned)mhdr->headersz_msb << 8) | le16_to_cpu(mhdr->headersz_lsb));
+
+	fprintf(f, "#SRC_ADDRESS 0x%x\n", le32_to_cpu(mhdr->srcaddr));
+	fprintf(f, "#BLOCK_SIZE 0x%x\n", le32_to_cpu(mhdr->blocksize));
+	fprintf(f, "#DEST_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->destaddr));
+	fprintf(f, "#EXEC_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->execaddr));
+
+	if (version != 0) {
+		if (options_to_baudrate(mhdr->options))
+			fprintf(f, "BAUDRATE %u\n", options_to_baudrate(mhdr->options));
+		if (options_to_baudrate(mhdr->options) ||
+		    ((mhdr->options >> 3) & 0x3) || ((mhdr->options >> 5) & 0x7)) {
+			fprintf(f, "UART_PORT %u\n", (unsigned)((mhdr->options >> 3) & 0x3));
+			fprintf(f, "UART_MPP 0x%x\n", (unsigned)((mhdr->options >> 5) & 0x7));
+		}
+		if (mhdr->flags & 0x1)
+			fprintf(f, "DEBUG 1\n");
+	}
+
+	cur_idx = 1;
+	for_each_opt_hdr_v1(ohdr, ptr) {
+		if (ohdr->headertype == OPT_HDR_V1_SECURE_TYPE) {
+			fprintf(f, "#SECURE_HEADER\n");
+		} else if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {
+			fprintf(f, "BINARY binary%d.bin", cur_idx);
+			for (i = 0; i < ohdr->data[0]; i++)
+				fprintf(f, " 0x%x", le32_to_cpu(((uint32_t *)ohdr->data)[i + 1]));
+			fprintf(f, " LOAD_ADDRESS 0x%08x\n",
+				0x40000000 + (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
+				8 + 4 * ohdr->data[0]);
+			cur_idx++;
+		} else if (ohdr->headertype == OPT_HDR_V1_REGISTER_TYPE) {
+			regset_hdr = (struct register_set_hdr_v1 *)ohdr;
+			for (i = 0;
+			     i < opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) -
+				 sizeof(regset_hdr->data[0].last_entry);
+			     i++)
+				fprintf(f, "DATA 0x%08x 0x%08x\n",
+					le32_to_cpu(regset_hdr->data[i].entry.address),
+					le32_to_cpu(regset_hdr->data[i].entry.value));
+			if (opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) >=
+			    sizeof(regset_hdr->data[0].last_entry)) {
+				if (regset_hdr->data[0].last_entry.delay)
+					fprintf(f, "DATA_DELAY %u\n",
+						(unsigned)regset_hdr->data[0].last_entry.delay);
+				else
+					fprintf(f, "DATA_DELAY SDRAM_SETUP\n");
+			}
+		}
+	}
+
+	if (version == 0 && mhdr0->ext) {
+		ehdr0 = (struct ext_hdr_v0 *)(mhdr0 + 1);
+		if (ehdr0->offset) {
+			for (regdata = (struct ext_hdr_v0_reg *)((uint8_t *)ptr + ehdr0->offset);
+			     (uint8_t *)regdata < (uint8_t *)ptr + header_size && regdata->raddr &&
+			     regdata->rdata;
+			     regdata++)
+				fprintf(f, "DATA 0x%08x 0x%08x\n", le32_to_cpu(regdata->raddr),
+					le32_to_cpu(regdata->rdata));
+		}
+	}
+
+	if (version == 0 && le16_to_cpu(mhdr0->ddrinitdelay))
+		fprintf(f, "DDR_INIT_DELAY %u\n", (unsigned)le16_to_cpu(mhdr0->ddrinitdelay));
+
+	/* Undocumented reserved fields */
+
+	if (version == 0 && (mhdr0->rsvd1[0] || mhdr0->rsvd1[1] || mhdr0->rsvd1[2]))
+		fprintf(f, "#RSVD1 0x%x 0x%x 0x%x\n", (unsigned)mhdr0->rsvd1[0],
+			(unsigned)mhdr0->rsvd1[1], (unsigned)mhdr0->rsvd1[2]);
+
+	if (version == 0 && mhdr0->rsvd3)
+		fprintf(f, "#RSVD3 0x%x\n", (unsigned)mhdr0->rsvd3);
+
+	if (version == 0 && le16_to_cpu(mhdr0->rsvd2))
+		fprintf(f, "#RSVD2 0x%x\n", (unsigned)le16_to_cpu(mhdr0->rsvd2));
+
+	if (version != 0 && mhdr->reserved4)
+		fprintf(f, "#RESERVED4 0x%x\n", (unsigned)mhdr->reserved4);
+
+	if (version != 0 && mhdr->reserved5)
+		fprintf(f, "#RESERVED5 0x%x\n", (unsigned)le16_to_cpu(mhdr->reserved5));
+
+	fclose(f);
+
+	return 0;
+}
+
 static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params)
 {
 	struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
@@ -2045,6 +2216,10 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
 	ulong image;
 	ulong size;
 
+	/* Generate kwbimage config file when '-p -1' is specified */
+	if (idx == -1)
+		return kwbimage_generate_config(ptr, params);
+
 	for_each_opt_hdr_v1 (ohdr, ptr) {
 		if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
 			continue;
-- 
2.20.1

[PATCH u-boot-marvell 13/16] tools: kwbimage: Do not cast const pointers to non-const pointers

[Pali Rohár]

Avoid casting const to non-const.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/kwbimage.h b/tools/kwbimage.h
index 8d37357e5abd..c000cba4b8d1 100644
--- a/tools/kwbimage.h
+++ b/tools/kwbimage.h
@@ -235,11 +235,11 @@ static inline int opt_hdr_v1_valid_size(const struct opt_hdr_v1 *ohdr,
 {
 	uint32_t ohdr_size;
 
-	if ((void *)(ohdr + 1) > mhdr_end)
+	if ((const void *)(ohdr + 1) > mhdr_end)
 		return 0;
 
 	ohdr_size = opt_hdr_v1_size(ohdr);
-	if (ohdr_size < 8 || (void *)((uint8_t *)ohdr + ohdr_size) > mhdr_end)
+	if (ohdr_size < 8 || (const void *)((const uint8_t *)ohdr + ohdr_size) > mhdr_end)
 		return 0;
 
 	return 1;
-- 
2.20.1

[PATCH u-boot-marvell 14/16] tools: kwbimage/kwboot: Check ext field for non-zero value

[Pali Rohár]

Despite the official specification, BootROM does not look at the lowest bit
of ext field but rather checks if ext field is non-zero.

Moreover original Marvell doimage tool puts into the mhdr->ext field the
number of extended headers, so basically it sets ext filed to non-zero
value if some extended header is present.

Fix U-Boot dumpimage and kwboot tools to parse correctly also kwbimage
files created by Marvell doimage tool, in the same way as the BootROM is
doing it when booting these images.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 2 +-
 tools/kwbimage.h | 6 +++---
 tools/kwboot.c   | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 2a0216a537ba..08b0332f5979 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1899,7 +1899,7 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
 	if (kwbimage_version(ptr) == 0) {
 		struct main_hdr_v0 *mhdr = (struct main_hdr_v0 *)ptr;
 
-		if (mhdr->ext & 0x1) {
+		if (mhdr->ext) {
 			struct ext_hdr_v0 *ext_hdr = (void *)(mhdr + 1);
 
 			csum = image_checksum8(ext_hdr, sizeof(*ext_hdr) - 1);
diff --git a/tools/kwbimage.h b/tools/kwbimage.h
index c000cba4b8d1..9ebc7d72d363 100644
--- a/tools/kwbimage.h
+++ b/tools/kwbimage.h
@@ -208,7 +208,7 @@ static inline size_t kwbheader_size(const void *header)
 		const struct main_hdr_v0 *hdr = header;
 
 		return sizeof(*hdr) +
-		       (hdr->ext & 0x1) ? sizeof(struct ext_hdr_v0) : 0;
+		       hdr->ext ? sizeof(struct ext_hdr_v0) : 0;
 	} else {
 		const struct main_hdr_v1 *hdr = header;
 
@@ -252,7 +252,7 @@ static inline struct opt_hdr_v1 *opt_hdr_v1_first(void *img) {
 		return NULL;
 
 	mhdr = img;
-	if (mhdr->ext & 0x1)
+	if (mhdr->ext)
 		return (struct opt_hdr_v1 *)(mhdr + 1);
 	else
 		return NULL;
@@ -272,7 +272,7 @@ static inline struct opt_hdr_v1 *_opt_hdr_v1_next(struct opt_hdr_v1 *cur)
 
 static inline struct opt_hdr_v1 *opt_hdr_v1_next(struct opt_hdr_v1 *cur)
 {
-	if (*opt_hdr_v1_ext(cur) & 0x1)
+	if (*opt_hdr_v1_ext(cur))
 		return _opt_hdr_v1_next(cur);
 	else
 		return NULL;
diff --git a/tools/kwboot.c b/tools/kwboot.c
index d22e6ea96a5c..c3d8ab654417 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1398,7 +1398,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
 	uint32_t ohdrsz;
 	uint8_t *prev_ext;
 
-	if (hdr->ext & 0x1) {
+	if (hdr->ext) {
 		for_each_opt_hdr_v1 (ohdr, img)
 			if (opt_hdr_v1_next(ohdr) == NULL)
 				break;
@@ -1422,7 +1422,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
 	ohdrsz = sizeof(*ohdr) + 4 + 4 * num_args + binsz + 4;
 	kwboot_img_grow_hdr(hdr, size, ohdrsz);
 
-	*prev_ext |= 1;
+	*prev_ext = 1;
 
 	ohdr->headertype = OPT_HDR_V1_BINARY_TYPE;
 	ohdr->headersz_msb = ohdrsz >> 16;
-- 
2.20.1

[PATCH u-boot-marvell 15/16] tools: kwbimage: Extract main data image without -p arg for dumpimage

[Pali Rohár]

When there is no -p argument for dumpimage tool specified, extract the main
data image from kwbimage file. This makes dumpimage consistent with other
image formats.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 67 +++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 08b0332f5979..3feab40c3fe1 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -2211,7 +2211,7 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
 	size_t header_size = kwbheader_size(ptr);
 	struct opt_hdr_v1 *ohdr;
 	int idx = params->pflag;
-	int cur_idx = 0;
+	int cur_idx;
 	uint32_t offset;
 	ulong image;
 	ulong size;
@@ -2220,41 +2220,54 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
 	if (idx == -1)
 		return kwbimage_generate_config(ptr, params);
 
-	for_each_opt_hdr_v1 (ohdr, ptr) {
-		if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
-			continue;
+	image = 0;
+	size = 0;
+
+	if (idx == 0) {
+		/* Extract data image when -p is not specified or when '-p 0' is specified */
+		offset = le32_to_cpu(mhdr->srcaddr);
 
-		if (idx == cur_idx) {
-			image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
-			size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
-			goto extract;
+		if (mhdr->blockid == IBR_HDR_SATA_ID) {
+			offset -= 1;
+			offset *= 512;
 		}
 
-		++cur_idx;
-	}
+		if (mhdr->blockid == IBR_HDR_SDIO_ID)
+			offset *= 512;
 
-	if (idx != cur_idx) {
-		printf("Image %d is not present\n", idx);
-		return -1;
-	}
-
-	offset = le32_to_cpu(mhdr->srcaddr);
+		if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
+			offset = header_size;
 
-	if (mhdr->blockid == IBR_HDR_SATA_ID) {
-		offset -= 1;
-		offset *= 512;
-	}
+		image = (ulong)((uint8_t *)ptr + offset);
+		size = le32_to_cpu(mhdr->blocksize) - 4;
+	} else {
+		/* Extract N-th binary header executabe image when other '-p N' is specified */
+		cur_idx = 1;
+		for_each_opt_hdr_v1(ohdr, ptr) {
+			if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
+				continue;
 
-	if (mhdr->blockid == IBR_HDR_SDIO_ID)
-		offset *= 512;
+			if (idx == cur_idx) {
+				image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
+				size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
+				break;
+			}
 
-	if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
-		offset = header_size;
+			++cur_idx;
+		}
 
-	image = (ulong)((uint8_t *)ptr + offset);
-	size = le32_to_cpu(mhdr->blocksize) - 4;
+		if (!image) {
+			fprintf(stderr, "Argument -p %d is invalid\n", idx);
+			fprintf(stderr, "Available subimages:\n");
+			fprintf(stderr, " -p -1  - kwbimage config file\n");
+			fprintf(stderr, " -p 0   - data image\n");
+			if (cur_idx - 1 > 0)
+				fprintf(stderr, " -p N   - Nth binary header image (totally: %d)\n",
+					cur_idx - 1);
+			return -1;
+		}
+	}
 
-extract:
 	return imagetool_save_subimage(params->outfile, image, size);
 }
 
-- 
2.20.1

[PATCH u-boot-marvell 16/16] tools: kwbimage: Fix mkimage/dumpimage -l argument

[Pali Rohár]

Do not check for kwbimage configuration file when just showing information
about existing kwbimage file.

The check for kwbimage configuration file is required only when creating
kwbimage, not when showing information about image or when extracting data
from image.

With this change, it is possible to call mkimage -l and dumpimage -l also
for existing kwbimage file.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 3feab40c3fe1..4a3daf2d7cb0 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -2276,7 +2276,8 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
  */
 static int kwbimage_check_params(struct image_tool_params *params)
 {
-	if (!params->iflag && (!params->imagename || !strlen(params->imagename))) {
+	if (!params->lflag && !params->iflag &&
+	    (!params->imagename || !strlen(params->imagename))) {
 		char *msg = "Configuration file for kwbimage creation omitted";
 
 		fprintf(stderr, "Error:%s - %s\n", params->cmdname, msg);
-- 
2.20.1

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Stefan Roese]

Hi Pali,

while testing with this patchset (amongst others), I get this error
while building for "theadorable_debug":

$ make theadorable_debug_defconfig
$ make -s -j20
Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin with 0 args.
Address must be 4-byte aligned and in range 0x40000028-0x40000424
.make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
make: *** Deleting file 'u-boot-spl.kwb'

Could you please take a look on whats going wrong here? Do I need to
change CONFIG_SPL_TEXT_BASE now? And if yes, why?

Thanks,
Stefan

On 12/21/21 16:54, Pali Rohár wrote:
> This patch series fixes generating images in kwbimage format, main fix
> is setting correct load address of U-Boot SPL. Also it adds support for
> generating kwbimage config file from existing kwbimage file via
> dumpimage tool.
> 
> Pali Rohár (16):
>    tools: kwbimage: Mark all local functions as static
>    tools: kwbimage: Deduplicate v1 regtype header finishing
>    tools: kwbimage: Fix generating image with multiple DATA_DELAY
>      commands
>    tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY
>      commands
>    arm: mvebu: Generate kwbimage.cfg with $(call cmd,...)
>    tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY
>      command
>    tools: kwbimage: Check the return value of image_headersz_v1()
>    arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in
>      kwbimage
>    arm: mvebu: Enable BootROM output on A38x
>    tools: kwbimage: Add missing check for maximal value for DATA_DELAY
>    tools: kwbimage: Show binary image address in mkimage -l, in addition
>      to size
>    tools: kwbimage: Dump kwbimage config file on '-p -1' option
>    tools: kwbimage: Do not cast const pointers to non-const pointers
>    tools: kwbimage/kwboot: Check ext field for non-zero value
>    tools: kwbimage: Extract main data image without -p arg for dumpimage
>    tools: kwbimage: Fix mkimage/dumpimage -l argument
> 
>   arch/arm/mach-mvebu/Makefile        |  17 +-
>   arch/arm/mach-mvebu/kwbimage.cfg.in |   7 +-
>   tools/kwbimage.c                    | 494 ++++++++++++++++++++++------
>   tools/kwbimage.h                    |  10 +-
>   tools/kwboot.c                      |   4 +-
>   5 files changed, 421 insertions(+), 111 deletions(-)
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Pali Rohár]

On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
> Hi Pali,
> 
> while testing with this patchset (amongst others), I get this error
> while building for "theadorable_debug":
> 
> $ make theadorable_debug_defconfig
> $ make -s -j20
> Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin with 0 args.
> Address must be 4-byte aligned and in range 0x40000028-0x40000424
> .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
> make: *** Deleting file 'u-boot-spl.kwb'
> 
> Could you please take a look on whats going wrong here? Do I need to
> change CONFIG_SPL_TEXT_BASE now? And if yes, why?

Hello!

If everything is working fine without this patch series then address
must not be changed.

Now I realized that some boards have text base address 0x40004030 and
some have 0x40000030. When I was looking it during writing this patch
series, I have not spotted that there is different digit "4" in the
middle... And therefore I was in impression that every 32-bit Armada has
base address 0x40000000 (increased by magic constant 0x30 which is
explained in one of the patches).

So now I need to figure out, why some boards have base address
0x40004000 and some have 0x40000000. It it somewhere documented this
magic address? Or do you know source of this address for your board?

In my opinion, it has to be BootROM specific and I do not think it is
changeable.

> Thanks,
> Stefan
> 
> On 12/21/21 16:54, Pali Rohár wrote:
> > This patch series fixes generating images in kwbimage format, main fix
> > is setting correct load address of U-Boot SPL. Also it adds support for
> > generating kwbimage config file from existing kwbimage file via
> > dumpimage tool.
> > 
> > Pali Rohár (16):
> >    tools: kwbimage: Mark all local functions as static
> >    tools: kwbimage: Deduplicate v1 regtype header finishing
> >    tools: kwbimage: Fix generating image with multiple DATA_DELAY
> >      commands
> >    tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY
> >      commands
> >    arm: mvebu: Generate kwbimage.cfg with $(call cmd,...)
> >    tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY
> >      command
> >    tools: kwbimage: Check the return value of image_headersz_v1()
> >    arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in
> >      kwbimage
> >    arm: mvebu: Enable BootROM output on A38x
> >    tools: kwbimage: Add missing check for maximal value for DATA_DELAY
> >    tools: kwbimage: Show binary image address in mkimage -l, in addition
> >      to size
> >    tools: kwbimage: Dump kwbimage config file on '-p -1' option
> >    tools: kwbimage: Do not cast const pointers to non-const pointers
> >    tools: kwbimage/kwboot: Check ext field for non-zero value
> >    tools: kwbimage: Extract main data image without -p arg for dumpimage
> >    tools: kwbimage: Fix mkimage/dumpimage -l argument
> > 
> >   arch/arm/mach-mvebu/Makefile        |  17 +-
> >   arch/arm/mach-mvebu/kwbimage.cfg.in |   7 +-
> >   tools/kwbimage.c                    | 494 ++++++++++++++++++++++------
> >   tools/kwbimage.h                    |  10 +-
> >   tools/kwboot.c                      |   4 +-
> >   5 files changed, 421 insertions(+), 111 deletions(-)
> > 
> 
> Viele Grüße,
> Stefan Roese
> 
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Stefan Roese]

On 1/12/22 11:41, Pali Rohár wrote:
> On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
>> Hi Pali,
>>
>> while testing with this patchset (amongst others), I get this error
>> while building for "theadorable_debug":
>>
>> $ make theadorable_debug_defconfig
>> $ make -s -j20
>> Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin with 0 args.
>> Address must be 4-byte aligned and in range 0x40000028-0x40000424
>> .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
>> make: *** Deleting file 'u-boot-spl.kwb'
>>
>> Could you please take a look on whats going wrong here? Do I need to
>> change CONFIG_SPL_TEXT_BASE now? And if yes, why?
> 
> Hello!
> 
> If everything is working fine without this patch series then address
> must not be changed.

Yes, everything works just fine without out this series.

> Now I realized that some boards have text base address 0x40004030 and
> some have 0x40000030. When I was looking it during writing this patch
> series, I have not spotted that there is different digit "4" in the
> middle... And therefore I was in impression that every 32-bit Armada has
> base address 0x40000000 (increased by magic constant 0x30 which is
> explained in one of the patches).

I see.

> So now I need to figure out, why some boards have base address
> 0x40004000 and some have 0x40000000. It it somewhere documented this
> magic address? Or do you know source of this address for your board?

This is so loooong ago that I worked on this. I can only guess, that the
are up to offset 0x4000 was reserved by/for the BootROM.

Thanks,
Stefan

> In my opinion, it has to be BootROM specific and I do not think it is
> changeable.
> 
>> Thanks,
>> Stefan
>>
>> On 12/21/21 16:54, Pali Rohár wrote:
>>> This patch series fixes generating images in kwbimage format, main fix
>>> is setting correct load address of U-Boot SPL. Also it adds support for
>>> generating kwbimage config file from existing kwbimage file via
>>> dumpimage tool.
>>>
>>> Pali Rohár (16):
>>>     tools: kwbimage: Mark all local functions as static
>>>     tools: kwbimage: Deduplicate v1 regtype header finishing
>>>     tools: kwbimage: Fix generating image with multiple DATA_DELAY
>>>       commands
>>>     tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY
>>>       commands
>>>     arm: mvebu: Generate kwbimage.cfg with $(call cmd,...)
>>>     tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY
>>>       command
>>>     tools: kwbimage: Check the return value of image_headersz_v1()
>>>     arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in
>>>       kwbimage
>>>     arm: mvebu: Enable BootROM output on A38x
>>>     tools: kwbimage: Add missing check for maximal value for DATA_DELAY
>>>     tools: kwbimage: Show binary image address in mkimage -l, in addition
>>>       to size
>>>     tools: kwbimage: Dump kwbimage config file on '-p -1' option
>>>     tools: kwbimage: Do not cast const pointers to non-const pointers
>>>     tools: kwbimage/kwboot: Check ext field for non-zero value
>>>     tools: kwbimage: Extract main data image without -p arg for dumpimage
>>>     tools: kwbimage: Fix mkimage/dumpimage -l argument
>>>
>>>    arch/arm/mach-mvebu/Makefile        |  17 +-
>>>    arch/arm/mach-mvebu/kwbimage.cfg.in |   7 +-
>>>    tools/kwbimage.c                    | 494 ++++++++++++++++++++++------
>>>    tools/kwbimage.h                    |  10 +-
>>>    tools/kwboot.c                      |   4 +-
>>>    5 files changed, 421 insertions(+), 111 deletions(-)
>>>
>>
>> Viele Grüße,
>> Stefan Roese
>>
>> -- 
>> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
>> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>> Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Stefan Roese]

Hi Pali,

On 1/12/22 11:55, Stefan Roese wrote:
> On 1/12/22 11:41, Pali Rohár wrote:
>> On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
>>> Hi Pali,
>>>
>>> while testing with this patchset (amongst others), I get this error
>>> while building for "theadorable_debug":
>>>
>>> $ make theadorable_debug_defconfig
>>> $ make -s -j20
>>> Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin with 0 
>>> args.
>>> Address must be 4-byte aligned and in range 0x40000028-0x40000424
>>> .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
>>> make: *** Deleting file 'u-boot-spl.kwb'
>>>
>>> Could you please take a look on whats going wrong here? Do I need to
>>> change CONFIG_SPL_TEXT_BASE now? And if yes, why?
>>
>> Hello!
>>
>> If everything is working fine without this patch series then address
>> must not be changed.
> 
> Yes, everything works just fine without out this series.
> 
>> Now I realized that some boards have text base address 0x40004030 and
>> some have 0x40000030. When I was looking it during writing this patch
>> series, I have not spotted that there is different digit "4" in the
>> middle... And therefore I was in impression that every 32-bit Armada has
>> base address 0x40000000 (increased by magic constant 0x30 which is
>> explained in one of the patches).
> 
> I see.
> 
>> So now I need to figure out, why some boards have base address
>> 0x40004000 and some have 0x40000000. It it somewhere documented this
>> magic address? Or do you know source of this address for your board?
> 
> This is so loooong ago that I worked on this. I can only guess, that the
> are up to offset 0x4000 was reserved by/for the BootROM.

This info is still in some of the config headers:

/*
  * Memory layout while starting into the bin_hdr via the
  * BootROM:
  *
  * 0x4000.4000 - 0x4003.4000	headers space (192KiB)
  * 0x4000.4030			bin_hdr start address
  * 0x4003.4000 - 0x4004.7c00	BootROM memory allocations (15KiB)
  * 0x4007.fffc			BootROM stack top
  *
  * The address space between 0x4007.fffc and 0x400f.fff is not locked in
  * L2 cache thus cannot be used.
  */

HTP.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Pali Rohár]

On Wednesday 12 January 2022 12:06:22 Stefan Roese wrote:
> Hi Pali,
> 
> On 1/12/22 11:55, Stefan Roese wrote:
> > On 1/12/22 11:41, Pali Rohár wrote:
> > > On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
> > > > Hi Pali,
> > > > 
> > > > while testing with this patchset (amongst others), I get this error
> > > > while building for "theadorable_debug":
> > > > 
> > > > $ make theadorable_debug_defconfig
> > > > $ make -s -j20
> > > > Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin
> > > > with 0 args.
> > > > Address must be 4-byte aligned and in range 0x40000028-0x40000424
> > > > .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
> > > > make: *** Deleting file 'u-boot-spl.kwb'
> > > > 
> > > > Could you please take a look on whats going wrong here? Do I need to
> > > > change CONFIG_SPL_TEXT_BASE now? And if yes, why?
> > > 
> > > Hello!
> > > 
> > > If everything is working fine without this patch series then address
> > > must not be changed.
> > 
> > Yes, everything works just fine without out this series.
> > 
> > > Now I realized that some boards have text base address 0x40004030 and
> > > some have 0x40000030. When I was looking it during writing this patch
> > > series, I have not spotted that there is different digit "4" in the
> > > middle... And therefore I was in impression that every 32-bit Armada has
> > > base address 0x40000000 (increased by magic constant 0x30 which is
> > > explained in one of the patches).
> > 
> > I see.
> > 
> > > So now I need to figure out, why some boards have base address
> > > 0x40004000 and some have 0x40000000. It it somewhere documented this
> > > magic address? Or do you know source of this address for your board?
> > 
> > This is so loooong ago that I worked on this. I can only guess, that the
> > are up to offset 0x4000 was reserved by/for the BootROM.
> 
> This info is still in some of the config headers:
> 
> /*
>  * Memory layout while starting into the bin_hdr via the
>  * BootROM:
>  *
>  * 0x4000.4000 - 0x4003.4000	headers space (192KiB)
>  * 0x4000.4030			bin_hdr start address
>  * 0x4003.4000 - 0x4004.7c00	BootROM memory allocations (15KiB)
>  * 0x4007.fffc			BootROM stack top
>  *
>  * The address space between 0x4007.fffc and 0x400f.fff is not locked in
>  * L2 cache thus cannot be used.
>  */
> 
> HTP.
> 
> Thanks,
> Stefan

And now I found this information in old Marvell U-Boot fork from 2013:
https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/blob/u-boot-2013.01-armada-18.06/tools/marvell/bin_hdr/inc/common/soc_spec.h#L84-L92

#if defined(MV_TEST_PLATFORM)
	#define RAM_TOP			0x81004000 /*  Use PEX memory - (16KB for MMU table) */
#elif defined(MV88F6710) || defined(MV88F78X60)
	#define RAM_TOP			0x40004000 /*  L2 cache 512KB - (16KB for MMU table) */
#elif defined(MV88F66XX) || defined(MV88F68XX) || defined(MV88F672X) || defined(MV88F69XX)
	#define RAM_TOP			0x40000000 /*  L2 cache 512KB */
#else
	#define RAM_TOP			0x40004000 /*  L2 cache 512KB */
#endif

IIRC this is chip conversion table:

88F6710 = A370
MV78X60 = AXP
88F66xx = Avanta
88F672x = A375
88F68xx = A38x
88F69xx = A39x

So it looks like that only AXP and A370 use address 0x40004000, other
platforms use 0x40000000. AXP and A370 are the last SoCs which use
Marvell custom CPU cores Sheeva, all later have ARM A9 cores. Also in
functional specifications for AXP and A370 is written that executable
code needs to be aligned to 128-bit boundary and in later SoCs specs
there is no written restriction, like this. On A385 I tested that this
alignment is not really required. So for me it looks like that this 16kB
reservation is needed for Marvell custom CPU cores only and is some CPU
core specific.

The only suspicious thing is that in configs/db-88f6720_defconfig is
defined CONFIG_SPL_TEXT_BASE=0x40004030 and this is A375 (not A370!).
But now I found your commit 606576d54b673 ("arm: mvebu: Add basic
support for Armada 375 eval board db-88f6720") where you introduced this
#define CONFIG_SPL_TEXT_BASE 0x40004030 and wrote "the SPL U-Boot is not
fully functional.". So with this base address SPL would never work. I
know that Serdes+ddr3 init code is missing, so no SPL for this platform.


So how to solve the problem that kwbimage needs to know if is building
image for platform with 16kB reserved for MMU table?

Should I add a new kwbimage command which specifies this information,
like building image for Marvell CPU cores (Sheeva)? Or do you have other
idea? With this information I can adjust code to enable 128-bit boundary
restrictions only for these CPUs...

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Stefan Roese]

On 1/12/22 12:34, Pali Rohár wrote:
> On Wednesday 12 January 2022 12:06:22 Stefan Roese wrote:
>> Hi Pali,
>>
>> On 1/12/22 11:55, Stefan Roese wrote:
>>> On 1/12/22 11:41, Pali Rohár wrote:
>>>> On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
>>>>> Hi Pali,
>>>>>
>>>>> while testing with this patchset (amongst others), I get this error
>>>>> while building for "theadorable_debug":
>>>>>
>>>>> $ make theadorable_debug_defconfig
>>>>> $ make -s -j20
>>>>> Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin
>>>>> with 0 args.
>>>>> Address must be 4-byte aligned and in range 0x40000028-0x40000424
>>>>> .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
>>>>> make: *** Deleting file 'u-boot-spl.kwb'
>>>>>
>>>>> Could you please take a look on whats going wrong here? Do I need to
>>>>> change CONFIG_SPL_TEXT_BASE now? And if yes, why?
>>>>
>>>> Hello!
>>>>
>>>> If everything is working fine without this patch series then address
>>>> must not be changed.
>>>
>>> Yes, everything works just fine without out this series.
>>>
>>>> Now I realized that some boards have text base address 0x40004030 and
>>>> some have 0x40000030. When I was looking it during writing this patch
>>>> series, I have not spotted that there is different digit "4" in the
>>>> middle... And therefore I was in impression that every 32-bit Armada has
>>>> base address 0x40000000 (increased by magic constant 0x30 which is
>>>> explained in one of the patches).
>>>
>>> I see.
>>>
>>>> So now I need to figure out, why some boards have base address
>>>> 0x40004000 and some have 0x40000000. It it somewhere documented this
>>>> magic address? Or do you know source of this address for your board?
>>>
>>> This is so loooong ago that I worked on this. I can only guess, that the
>>> are up to offset 0x4000 was reserved by/for the BootROM.
>>
>> This info is still in some of the config headers:
>>
>> /*
>>   * Memory layout while starting into the bin_hdr via the
>>   * BootROM:
>>   *
>>   * 0x4000.4000 - 0x4003.4000	headers space (192KiB)
>>   * 0x4000.4030			bin_hdr start address
>>   * 0x4003.4000 - 0x4004.7c00	BootROM memory allocations (15KiB)
>>   * 0x4007.fffc			BootROM stack top
>>   *
>>   * The address space between 0x4007.fffc and 0x400f.fff is not locked in
>>   * L2 cache thus cannot be used.
>>   */
>>
>> HTP.
>>
>> Thanks,
>> Stefan
> 
> And now I found this information in old Marvell U-Boot fork from 2013:
> https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/blob/u-boot-2013.01-armada-18.06/tools/marvell/bin_hdr/inc/common/soc_spec.h#L84-L92
> 
> #if defined(MV_TEST_PLATFORM)
> 	#define RAM_TOP			0x81004000 /*  Use PEX memory - (16KB for MMU table) */
> #elif defined(MV88F6710) || defined(MV88F78X60)
> 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB - (16KB for MMU table) */
> #elif defined(MV88F66XX) || defined(MV88F68XX) || defined(MV88F672X) || defined(MV88F69XX)
> 	#define RAM_TOP			0x40000000 /*  L2 cache 512KB */
> #else
> 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB */
> #endif
> 
> IIRC this is chip conversion table:
> 
> 88F6710 = A370
> MV78X60 = AXP
> 88F66xx = Avanta
> 88F672x = A375
> 88F68xx = A38x
> 88F69xx = A39x
> 
> So it looks like that only AXP and A370 use address 0x40004000, other
> platforms use 0x40000000. AXP and A370 are the last SoCs which use
> Marvell custom CPU cores Sheeva, all later have ARM A9 cores. Also in
> functional specifications for AXP and A370 is written that executable
> code needs to be aligned to 128-bit boundary and in later SoCs specs
> there is no written restriction, like this. On A385 I tested that this
> alignment is not really required. So for me it looks like that this 16kB
> reservation is needed for Marvell custom CPU cores only and is some CPU
> core specific.

Makes perfect sense, yes.

> The only suspicious thing is that in configs/db-88f6720_defconfig is
> defined CONFIG_SPL_TEXT_BASE=0x40004030 and this is A375 (not A370!).
> But now I found your commit 606576d54b673 ("arm: mvebu: Add basic
> support for Armada 375 eval board db-88f6720") where you introduced this
> #define CONFIG_SPL_TEXT_BASE 0x40004030 and wrote "the SPL U-Boot is not
> fully functional.". So with this base address SPL would never work. I
> know that Serdes+ddr3 init code is missing, so no SPL for this platform.

AFAIR, this port was done not that thoroughly, which would explain this
mismatch you describe above. And could perhaps be fixed by changing this
SPL_TEXT_BASE - if someone would like to invest some more time here.

> So how to solve the problem that kwbimage needs to know if is building
> image for platform with 16kB reserved for MMU table?
> 
> Should I add a new kwbimage command which specifies this information,
> like building image for Marvell CPU cores (Sheeva)? Or do you have other
> idea? With this information I can adjust code to enable 128-bit boundary
> restrictions only for these CPUs...

The first idea is to change this error to a warning, with some comment
that this might work on these specific AXP and A370 platforms. Another
idea is to add a 2nd valid address area.

Thanks,
Stefan

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Pali Rohár]

On Wednesday 12 January 2022 14:53:23 Stefan Roese wrote:
> On 1/12/22 12:34, Pali Rohár wrote:
> > On Wednesday 12 January 2022 12:06:22 Stefan Roese wrote:
> > > Hi Pali,
> > > 
> > > On 1/12/22 11:55, Stefan Roese wrote:
> > > > On 1/12/22 11:41, Pali Rohár wrote:
> > > > > On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
> > > > > > Hi Pali,
> > > > > > 
> > > > > > while testing with this patchset (amongst others), I get this error
> > > > > > while building for "theadorable_debug":
> > > > > > 
> > > > > > $ make theadorable_debug_defconfig
> > > > > > $ make -s -j20
> > > > > > Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin
> > > > > > with 0 args.
> > > > > > Address must be 4-byte aligned and in range 0x40000028-0x40000424
> > > > > > .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
> > > > > > make: *** Deleting file 'u-boot-spl.kwb'
> > > > > > 
> > > > > > Could you please take a look on whats going wrong here? Do I need to
> > > > > > change CONFIG_SPL_TEXT_BASE now? And if yes, why?
> > > > > 
> > > > > Hello!
> > > > > 
> > > > > If everything is working fine without this patch series then address
> > > > > must not be changed.
> > > > 
> > > > Yes, everything works just fine without out this series.
> > > > 
> > > > > Now I realized that some boards have text base address 0x40004030 and
> > > > > some have 0x40000030. When I was looking it during writing this patch
> > > > > series, I have not spotted that there is different digit "4" in the
> > > > > middle... And therefore I was in impression that every 32-bit Armada has
> > > > > base address 0x40000000 (increased by magic constant 0x30 which is
> > > > > explained in one of the patches).
> > > > 
> > > > I see.
> > > > 
> > > > > So now I need to figure out, why some boards have base address
> > > > > 0x40004000 and some have 0x40000000. It it somewhere documented this
> > > > > magic address? Or do you know source of this address for your board?
> > > > 
> > > > This is so loooong ago that I worked on this. I can only guess, that the
> > > > are up to offset 0x4000 was reserved by/for the BootROM.
> > > 
> > > This info is still in some of the config headers:
> > > 
> > > /*
> > >   * Memory layout while starting into the bin_hdr via the
> > >   * BootROM:
> > >   *
> > >   * 0x4000.4000 - 0x4003.4000	headers space (192KiB)
> > >   * 0x4000.4030			bin_hdr start address
> > >   * 0x4003.4000 - 0x4004.7c00	BootROM memory allocations (15KiB)
> > >   * 0x4007.fffc			BootROM stack top
> > >   *
> > >   * The address space between 0x4007.fffc and 0x400f.fff is not locked in
> > >   * L2 cache thus cannot be used.
> > >   */
> > > 
> > > HTP.
> > > 
> > > Thanks,
> > > Stefan
> > 
> > And now I found this information in old Marvell U-Boot fork from 2013:
> > https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/blob/u-boot-2013.01-armada-18.06/tools/marvell/bin_hdr/inc/common/soc_spec.h#L84-L92
> > 
> > #if defined(MV_TEST_PLATFORM)
> > 	#define RAM_TOP			0x81004000 /*  Use PEX memory - (16KB for MMU table) */
> > #elif defined(MV88F6710) || defined(MV88F78X60)
> > 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB - (16KB for MMU table) */
> > #elif defined(MV88F66XX) || defined(MV88F68XX) || defined(MV88F672X) || defined(MV88F69XX)
> > 	#define RAM_TOP			0x40000000 /*  L2 cache 512KB */
> > #else
> > 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB */
> > #endif
> > 
> > IIRC this is chip conversion table:
> > 
> > 88F6710 = A370
> > MV78X60 = AXP
> > 88F66xx = Avanta
> > 88F672x = A375
> > 88F68xx = A38x
> > 88F69xx = A39x
> > 
> > So it looks like that only AXP and A370 use address 0x40004000, other
> > platforms use 0x40000000. AXP and A370 are the last SoCs which use
> > Marvell custom CPU cores Sheeva, all later have ARM A9 cores. Also in
> > functional specifications for AXP and A370 is written that executable
> > code needs to be aligned to 128-bit boundary and in later SoCs specs
> > there is no written restriction, like this. On A385 I tested that this
> > alignment is not really required. So for me it looks like that this 16kB
> > reservation is needed for Marvell custom CPU cores only and is some CPU
> > core specific.
> 
> Makes perfect sense, yes.
> 
> > The only suspicious thing is that in configs/db-88f6720_defconfig is
> > defined CONFIG_SPL_TEXT_BASE=0x40004030 and this is A375 (not A370!).
> > But now I found your commit 606576d54b673 ("arm: mvebu: Add basic
> > support for Armada 375 eval board db-88f6720") where you introduced this
> > #define CONFIG_SPL_TEXT_BASE 0x40004030 and wrote "the SPL U-Boot is not
> > fully functional.". So with this base address SPL would never work. I
> > know that Serdes+ddr3 init code is missing, so no SPL for this platform.
> 
> AFAIR, this port was done not that thoroughly, which would explain this
> mismatch you describe above. And could perhaps be fixed by changing this
> SPL_TEXT_BASE - if someone would like to invest some more time here.
> 
> > So how to solve the problem that kwbimage needs to know if is building
> > image for platform with 16kB reserved for MMU table?
> > 
> > Should I add a new kwbimage command which specifies this information,
> > like building image for Marvell CPU cores (Sheeva)? Or do you have other
> > idea? With this information I can adjust code to enable 128-bit boundary
> > restrictions only for these CPUs...
> 
> The first idea is to change this error to a warning, with some comment
> that this might work on these specific AXP and A370 platforms. Another
> idea is to add a 2nd valid address area.
> 
> Thanks,
> Stefan

I do not think that changing error to warning would help here. With
these changes kwbimage really try to sets load address to specified one.
And if kwbimage thinks that base address is on different location then
it would generate something unbootable.

Now I played with it... could you try to test following diff? It
propagates information about CPU into kwbimage and then it do different
checks and use different base address based on CPU.


diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 4e15101a40cf..74478a3134e3 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -30,6 +30,14 @@ obj-$(CONFIG_MVEBU_EFUSE) += efuse.o
 
 extra-y += kwbimage.cfg
 
+ifneq ($(CONFIG_ARMADA_370)$(CONFIG_ARMADA_XP),)
+	KWB_REPLACE += CPU
+	KWB_CFG_CPU = SHEEVA
+else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)
+	KWB_REPLACE += CPU
+	KWB_CFG_CPU = A9
+endif
+
 KWB_REPLACE += LOAD_ADDRESS
 KWB_CFG_LOAD_ADDRESS = $(CONFIG_SPL_TEXT_BASE)
 
diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
index 75f90766dda4..ccb09975817e 100644
--- a/arch/arm/mach-mvebu/kwbimage.cfg.in
+++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
@@ -5,6 +5,9 @@
 # Armada 38x uses version 1 image format
 VERSION		1
 
+# Type of the CPU core
+#@CPU
+
 # Boot Media configurations
 #@BOOT_FROM
 
diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 7ccb582918a4..52005724fbd1 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -99,6 +99,7 @@ enum image_cfg_type {
 	IMAGE_CFG_NAND_BADBLK_LOCATION,
 	IMAGE_CFG_NAND_ECC_MODE,
 	IMAGE_CFG_NAND_PAGESZ,
+	IMAGE_CFG_CPU,
 	IMAGE_CFG_BINARY,
 	IMAGE_CFG_DATA,
 	IMAGE_CFG_DATA_DELAY,
@@ -129,6 +130,7 @@ static const char * const id_strs[] = {
 	[IMAGE_CFG_NAND_BADBLK_LOCATION] = "NAND_BADBLK_LOCATION",
 	[IMAGE_CFG_NAND_ECC_MODE] = "NAND_ECC_MODE",
 	[IMAGE_CFG_NAND_PAGESZ] = "NAND_PAGE_SIZE",
+	[IMAGE_CFG_CPU] = "CPU",
 	[IMAGE_CFG_BINARY] = "BINARY",
 	[IMAGE_CFG_DATA] = "DATA",
 	[IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",
@@ -152,6 +154,7 @@ struct image_cfg_element {
 	enum image_cfg_type type;
 	union {
 		unsigned int version;
+		unsigned int cpu_sheeva;
 		unsigned int bootfrom;
 		struct {
 			const char *file;
@@ -292,6 +295,17 @@ static int image_get_bootfrom(void)
 	return e->bootfrom;
 }
 
+static int image_is_cpu_sheeva(void)
+{
+	struct image_cfg_element *e;
+
+	e = image_find_option(IMAGE_CFG_CPU);
+	if (!e)
+		return 0;
+
+	return e->cpu_sheeva;
+}
+
 /*
  * Compute a 8-bit checksum of a memory area. This algorithm follows
  * the requirements of the Marvell SoC BootROM specifications.
@@ -1031,6 +1045,7 @@ static size_t image_headersz_v1(int *hasext)
 	struct image_cfg_element *e;
 	unsigned int count;
 	size_t headersz;
+	int cpu_sheeva;
 	struct stat s;
 	int cfgi;
 	int ret;
@@ -1047,6 +1062,8 @@ static size_t image_headersz_v1(int *hasext)
 			*hasext = 1;
 	}
 
+	cpu_sheeva = image_is_cpu_sheeva();
+
 	count = 0;
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
 		e = &image_cfg[cfgi];
@@ -1089,15 +1106,25 @@ static size_t image_headersz_v1(int *hasext)
 			/*
 			 * BootROM loads kwbimage header (in which the
 			 * executable code is also stored) to address
-			 * 0x40000000. Thus there is restriction for the
-			 * load address of the N-th BINARY image.
+			 * 0x40004000 or 0x40000000. Thus there is
+			 * restriction for the load address of the N-th
+			 * BINARY image.
 			 */
-			unsigned int low_addr, high_addr;
+			unsigned int base_addr, low_addr, high_addr;
 
-			low_addr = 0x40000000 + headersz;
+			base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
+			low_addr = base_addr + headersz;
 			high_addr = low_addr +
 				    (BINARY_MAX_ARGS - e->binary.nargs) * sizeof(uint32_t);
 
+			if (cpu_sheeva && e->binary.loadaddr % 16) {
+				fprintf(stderr,
+					"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
+					"Address for CPU SHEEVA must be 16-byte aligned.\n",
+					e->binary.loadaddr, e->binary.file, e->binary.nargs);
+				return 0;
+			}
+
 			if (e->binary.loadaddr % 4 || e->binary.loadaddr < low_addr ||
 			    e->binary.loadaddr > high_addr) {
 				fprintf(stderr,
@@ -1107,7 +1134,7 @@ static size_t image_headersz_v1(int *hasext)
 					e->binary.nargs, low_addr, high_addr);
 				return 0;
 			}
-			headersz = e->binary.loadaddr - 0x40000000;
+			headersz = e->binary.loadaddr - base_addr;
 		} else {
 			headersz = ALIGN(headersz, 16);
 		}
@@ -1128,10 +1155,12 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
 				struct main_hdr_v1 *main_hdr)
 {
 	struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;
+	uint32_t base_addr;
 	uint32_t add_args;
 	uint32_t offset;
 	uint32_t *args;
 	size_t binhdrsz;
+	int cpu_sheeva;
 	struct stat s;
 	int argi;
 	FILE *bin;
@@ -1163,18 +1192,22 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
 	*cur += (binarye->binary.nargs + 1) * sizeof(uint32_t);
 
 	/*
-	 * ARM executable code inside the BIN header on some mvebu platforms
-	 * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
+	 * ARM executable code inside the BIN header on platforms with Sheeva
+	 * CPU (A370 and AXP) must always be aligned with the 128-bit boundary.
 	 * In the case when this code is not position independent (e.g. ARM
 	 * SPL), it must be placed at fixed load and execute address.
 	 * This requirement can be met by inserting dummy arguments into
 	 * BIN header, if needed.
 	 */
+	cpu_sheeva = image_is_cpu_sheeva();
+	base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
 	offset = *cur - (uint8_t *)main_hdr;
 	if (binarye->binary.loadaddr)
-		add_args = (binarye->binary.loadaddr - 0x40000000 - offset) / sizeof(uint32_t);
-	else
+		add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
+	else if (cpu_sheeva)
 		add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
+	else
+		add_args = 0;
 	if (add_args) {
 		*(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args);
 		*cur += add_args * sizeof(uint32_t);
@@ -1553,6 +1586,18 @@ static int image_create_config_parse_oneline(char *line,
 	case IMAGE_CFG_VERSION:
 		el->version = atoi(value1);
 		break;
+	case IMAGE_CFG_CPU:
+		if (strcmp(value1, "FEROCEON") == 0)
+			el->cpu_sheeva = 0;
+		else if (strcmp(value1, "SHEEVA") == 0)
+			el->cpu_sheeva = 1;
+		else if (strcmp(value1, "A9") == 0)
+			el->cpu_sheeva = 0;
+		else {
+			fprintf(stderr, "Invalid CPU %s\n", value1);
+			return -1;
+		}
+		break;
 	case IMAGE_CFG_BOOT_FROM:
 		ret = image_boot_mode_id(value1);
 
@@ -1862,7 +1907,7 @@ static void kwbimage_print_header(const void *ptr)
 			printf("BIN Img Size: ");
 			genimg_print_size(opt_hdr_v1_size(ohdr) - 12 -
 					  4 * ohdr->data[0]);
-			printf("BIN Img Addr: %08x\n", 0x40000000 +
+			printf("BIN Img Offs: %08x\n",
 				(unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
 				8 + 4 * ohdr->data[0]);
 		}
@@ -2031,6 +2076,11 @@ static int kwbimage_generate(struct image_tool_params *params,
 			free(image_cfg);
 			exit(EXIT_FAILURE);
 		}
+		if (alloc_len > 192*1024) {
+			fprintf(stderr, "Header is too big (%u bytes), maximal kwbimage header size is %u bytes\n", alloc_len, 192*1024);
+			free(image_cfg);
+			exit(EXIT_FAILURE);
+		}
 		break;
 
 	default:
@@ -2079,6 +2129,7 @@ static int kwbimage_generate_config(void *ptr, struct image_tool_params *params)
 	struct ext_hdr_v0_reg *regdata;
 	struct ext_hdr_v0 *ehdr0;
 	struct opt_hdr_v1 *ohdr;
+	unsigned offset;
 	int cur_idx;
 	int version;
 	FILE *f;
@@ -2145,9 +2196,9 @@ static int kwbimage_generate_config(void *ptr, struct image_tool_params *params)
 			fprintf(f, "BINARY binary%d.bin", cur_idx);
 			for (i = 0; i < ohdr->data[0]; i++)
 				fprintf(f, " 0x%x", le32_to_cpu(((uint32_t *)ohdr->data)[i + 1]));
-			fprintf(f, " LOAD_ADDRESS 0x%08x\n",
-				0x40000000 + (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
-				8 + 4 * ohdr->data[0]);
+			offset = (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) + 8 + 4 * ohdr->data[0];
+			fprintf(f, " LOAD_ADDRESS 0x%08x\n", 0x40000000 + offset);
+			fprintf(f, " # for CPU SHEEVA: LOAD_ADDRESS 0x%08x\n", 0x40004000 + offset);
 			cur_idx++;
 		} else if (ohdr->headertype == OPT_HDR_V1_REGISTER_TYPE) {
 			regset_hdr = (struct register_set_hdr_v1 *)ohdr;

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Stefan Roese]

On 1/12/22 15:16, Pali Rohár wrote:
> On Wednesday 12 January 2022 14:53:23 Stefan Roese wrote:
>> On 1/12/22 12:34, Pali Rohár wrote:
>>> On Wednesday 12 January 2022 12:06:22 Stefan Roese wrote:
>>>> Hi Pali,
>>>>
>>>> On 1/12/22 11:55, Stefan Roese wrote:
>>>>> On 1/12/22 11:41, Pali Rohár wrote:
>>>>>> On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
>>>>>>> Hi Pali,
>>>>>>>
>>>>>>> while testing with this patchset (amongst others), I get this error
>>>>>>> while building for "theadorable_debug":
>>>>>>>
>>>>>>> $ make theadorable_debug_defconfig
>>>>>>> $ make -s -j20
>>>>>>> Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin
>>>>>>> with 0 args.
>>>>>>> Address must be 4-byte aligned and in range 0x40000028-0x40000424
>>>>>>> .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
>>>>>>> make: *** Deleting file 'u-boot-spl.kwb'
>>>>>>>
>>>>>>> Could you please take a look on whats going wrong here? Do I need to
>>>>>>> change CONFIG_SPL_TEXT_BASE now? And if yes, why?
>>>>>>
>>>>>> Hello!
>>>>>>
>>>>>> If everything is working fine without this patch series then address
>>>>>> must not be changed.
>>>>>
>>>>> Yes, everything works just fine without out this series.
>>>>>
>>>>>> Now I realized that some boards have text base address 0x40004030 and
>>>>>> some have 0x40000030. When I was looking it during writing this patch
>>>>>> series, I have not spotted that there is different digit "4" in the
>>>>>> middle... And therefore I was in impression that every 32-bit Armada has
>>>>>> base address 0x40000000 (increased by magic constant 0x30 which is
>>>>>> explained in one of the patches).
>>>>>
>>>>> I see.
>>>>>
>>>>>> So now I need to figure out, why some boards have base address
>>>>>> 0x40004000 and some have 0x40000000. It it somewhere documented this
>>>>>> magic address? Or do you know source of this address for your board?
>>>>>
>>>>> This is so loooong ago that I worked on this. I can only guess, that the
>>>>> are up to offset 0x4000 was reserved by/for the BootROM.
>>>>
>>>> This info is still in some of the config headers:
>>>>
>>>> /*
>>>>    * Memory layout while starting into the bin_hdr via the
>>>>    * BootROM:
>>>>    *
>>>>    * 0x4000.4000 - 0x4003.4000	headers space (192KiB)
>>>>    * 0x4000.4030			bin_hdr start address
>>>>    * 0x4003.4000 - 0x4004.7c00	BootROM memory allocations (15KiB)
>>>>    * 0x4007.fffc			BootROM stack top
>>>>    *
>>>>    * The address space between 0x4007.fffc and 0x400f.fff is not locked in
>>>>    * L2 cache thus cannot be used.
>>>>    */
>>>>
>>>> HTP.
>>>>
>>>> Thanks,
>>>> Stefan
>>>
>>> And now I found this information in old Marvell U-Boot fork from 2013:
>>> https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/blob/u-boot-2013.01-armada-18.06/tools/marvell/bin_hdr/inc/common/soc_spec.h#L84-L92
>>>
>>> #if defined(MV_TEST_PLATFORM)
>>> 	#define RAM_TOP			0x81004000 /*  Use PEX memory - (16KB for MMU table) */
>>> #elif defined(MV88F6710) || defined(MV88F78X60)
>>> 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB - (16KB for MMU table) */
>>> #elif defined(MV88F66XX) || defined(MV88F68XX) || defined(MV88F672X) || defined(MV88F69XX)
>>> 	#define RAM_TOP			0x40000000 /*  L2 cache 512KB */
>>> #else
>>> 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB */
>>> #endif
>>>
>>> IIRC this is chip conversion table:
>>>
>>> 88F6710 = A370
>>> MV78X60 = AXP
>>> 88F66xx = Avanta
>>> 88F672x = A375
>>> 88F68xx = A38x
>>> 88F69xx = A39x
>>>
>>> So it looks like that only AXP and A370 use address 0x40004000, other
>>> platforms use 0x40000000. AXP and A370 are the last SoCs which use
>>> Marvell custom CPU cores Sheeva, all later have ARM A9 cores. Also in
>>> functional specifications for AXP and A370 is written that executable
>>> code needs to be aligned to 128-bit boundary and in later SoCs specs
>>> there is no written restriction, like this. On A385 I tested that this
>>> alignment is not really required. So for me it looks like that this 16kB
>>> reservation is needed for Marvell custom CPU cores only and is some CPU
>>> core specific.
>>
>> Makes perfect sense, yes.
>>
>>> The only suspicious thing is that in configs/db-88f6720_defconfig is
>>> defined CONFIG_SPL_TEXT_BASE=0x40004030 and this is A375 (not A370!).
>>> But now I found your commit 606576d54b673 ("arm: mvebu: Add basic
>>> support for Armada 375 eval board db-88f6720") where you introduced this
>>> #define CONFIG_SPL_TEXT_BASE 0x40004030 and wrote "the SPL U-Boot is not
>>> fully functional.". So with this base address SPL would never work. I
>>> know that Serdes+ddr3 init code is missing, so no SPL for this platform.
>>
>> AFAIR, this port was done not that thoroughly, which would explain this
>> mismatch you describe above. And could perhaps be fixed by changing this
>> SPL_TEXT_BASE - if someone would like to invest some more time here.
>>
>>> So how to solve the problem that kwbimage needs to know if is building
>>> image for platform with 16kB reserved for MMU table?
>>>
>>> Should I add a new kwbimage command which specifies this information,
>>> like building image for Marvell CPU cores (Sheeva)? Or do you have other
>>> idea? With this information I can adjust code to enable 128-bit boundary
>>> restrictions only for these CPUs...
>>
>> The first idea is to change this error to a warning, with some comment
>> that this might work on these specific AXP and A370 platforms. Another
>> idea is to add a 2nd valid address area.
>>
>> Thanks,
>> Stefan
> 
> I do not think that changing error to warning would help here. With
> these changes kwbimage really try to sets load address to specified one.
> And if kwbimage thinks that base address is on different location then
> it would generate something unbootable.

Makes sense, okay.

> Now I played with it... could you try to test following diff? It
> propagates information about CPU into kwbimage and then it do different
> checks and use different base address based on CPU.

Looks good, please see the boot log here:

$ ./tools/kwboot -B 115200 -b u-boot-spl.kwb -t 
/dev/serial/by-id/usb-FTDI_FT232R_USB_UART_A1019EGY-if00-port0
kwboot version 2022.01-00487-g655eb7d4bc34-dirty
Patching image boot signature to UART
Sending boot message. Please reboot the target...\
Waiting 2s and flushing tty
Sending boot image header (91264 bytes)...
   0 % 
[......................................................................]
   9 % 
[......................................................................]
  19 % 
[......................................................................]
  29 % 
[......................................................................]
  39 % 
[......................................................................]
  49 % 
[......................................................................]
  59 % 
[......................................................................]
  68 % 
[......................................................................]
  78 % 
[......................................................................]
  88 % 
[......................................................................]
  98 % [............. 
       ]
Done

U-Boot SPL 2022.01-00487-g655eb7d4bc34-dirty (Jan 12 2022 - 16:02:28 +0100)
High speed PHY - Version: 2.1.5 (COM-PHY-V20)
High speed PHY - Ended Successfully
DDR3 Training Sequence - Ver 5.7.4
DDR3 Training Sequence - Ended Successfully
Trying to boot from BOOTROM
Returning to BootROM (return address 0xffff0aa0)...

Sending boot image data (566180 bytes)...
   0 % 
[......................................................................]
   1 % 
[......................................................................]
   3 % 
[......................................................................]
   4 % 
[......................................................................]
   6 % 
[......................................................................]
   7 % 
[......................................................................]
   9 % 
[......................................................................]
  11 % 
[......................................................................]
  12 % 
[......................................................................]
  14 % 
[......................................................................]
  15 % 
[......................................................................]
  17 % 
[......................................................................]
  19 % 
[......................................................................]
  20 % 
[......................................................................]
  22 % 
[......................................................................]
  23 % 
[......................................................................]
  25 % 
[......................................................................]
  26 % 
[......................................................................]
  28 % 
[......................................................................]
  30 % 
[......................................................................]
  31 % 
[......................................................................]
  33 % 
[......................................................................]
  34 % 
[......................................................................]
  36 % 
[......................................................................]
  38 % 
[......................................................................]
  39 % 
[......................................................................]
  41 % 
[......................................................................]
  42 % 
[......................................................................]
  44 % 
[......................................................................]
  45 % 
[......................................................................]
  47 % 
[......................................................................]
  49 % 
[......................................................................]
  50 % 
[......................................................................]
  52 % 
[......................................................................]
  53 % 
[......................................................................]
  55 % 
[......................................................................]
  56 % 
[......................................................................]
  58 % 
[......................................................................]
  60 % 
[......................................................................]
  61 % 
[......................................................................]
  63 % 
[......................................................................]
  64 % 
[......................................................................]
  66 % 
[......................................................................]
  68 % 
[......................................................................]
  69 % 
[......................................................................]
  71 % 
[......................................................................]
  72 % 
[......................................................................]
  74 % 
[......................................................................]
  75 % 
[......................................................................]
  77 % 
[......................................................................]
  79 % 
[......................................................................]
  80 % 
[......................................................................]
  82 % 
[......................................................................]
  83 % 
[......................................................................]
  85 % 
[......................................................................]
  87 % 
[......................................................................]
  88 % 
[......................................................................]
  90 % 
[......................................................................]
  91 % 
[......................................................................]
  93 % 
[......................................................................]
  94 % 
[......................................................................]
  96 % 
[......................................................................]
  98 % 
[......................................................................]
  99 % [.............. 
       ]
Done
Finishing transfer
[Type Ctrl-\ + c to quit]


U-Boot 2022.01-00487-g655eb7d4bc34-dirty (Jan 12 2022 - 16:02:28 +0100)

SoC:   MV78260-B0 at 1333 MHz
DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
Core:  24 devices, 14 uclasses, devicetree: separate
Loading Environment from SPIFlash... SF: Detected m25p128 with page size 
256 Bytes, erase size 256 KiB, total 16 MiB
OK
Model: Marvell Armada XP theadorable
pcie0.0: Link up
pcie2.0: Link up
Net:   eth0: ethernet@70000
...

:)

Thanks,
Stefan

> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 4e15101a40cf..74478a3134e3 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -30,6 +30,14 @@ obj-$(CONFIG_MVEBU_EFUSE) += efuse.o
>   
>   extra-y += kwbimage.cfg
>   
> +ifneq ($(CONFIG_ARMADA_370)$(CONFIG_ARMADA_XP),)
> +	KWB_REPLACE += CPU
> +	KWB_CFG_CPU = SHEEVA
> +else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)
> +	KWB_REPLACE += CPU
> +	KWB_CFG_CPU = A9
> +endif
> +
>   KWB_REPLACE += LOAD_ADDRESS
>   KWB_CFG_LOAD_ADDRESS = $(CONFIG_SPL_TEXT_BASE)
>   
> diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
> index 75f90766dda4..ccb09975817e 100644
> --- a/arch/arm/mach-mvebu/kwbimage.cfg.in
> +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
> @@ -5,6 +5,9 @@
>   # Armada 38x uses version 1 image format
>   VERSION		1
>   
> +# Type of the CPU core
> +#@CPU
> +
>   # Boot Media configurations
>   #@BOOT_FROM
>   
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 7ccb582918a4..52005724fbd1 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -99,6 +99,7 @@ enum image_cfg_type {
>   	IMAGE_CFG_NAND_BADBLK_LOCATION,
>   	IMAGE_CFG_NAND_ECC_MODE,
>   	IMAGE_CFG_NAND_PAGESZ,
> +	IMAGE_CFG_CPU,
>   	IMAGE_CFG_BINARY,
>   	IMAGE_CFG_DATA,
>   	IMAGE_CFG_DATA_DELAY,
> @@ -129,6 +130,7 @@ static const char * const id_strs[] = {
>   	[IMAGE_CFG_NAND_BADBLK_LOCATION] = "NAND_BADBLK_LOCATION",
>   	[IMAGE_CFG_NAND_ECC_MODE] = "NAND_ECC_MODE",
>   	[IMAGE_CFG_NAND_PAGESZ] = "NAND_PAGE_SIZE",
> +	[IMAGE_CFG_CPU] = "CPU",
>   	[IMAGE_CFG_BINARY] = "BINARY",
>   	[IMAGE_CFG_DATA] = "DATA",
>   	[IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",
> @@ -152,6 +154,7 @@ struct image_cfg_element {
>   	enum image_cfg_type type;
>   	union {
>   		unsigned int version;
> +		unsigned int cpu_sheeva;
>   		unsigned int bootfrom;
>   		struct {
>   			const char *file;
> @@ -292,6 +295,17 @@ static int image_get_bootfrom(void)
>   	return e->bootfrom;
>   }
>   
> +static int image_is_cpu_sheeva(void)
> +{
> +	struct image_cfg_element *e;
> +
> +	e = image_find_option(IMAGE_CFG_CPU);
> +	if (!e)
> +		return 0;
> +
> +	return e->cpu_sheeva;
> +}
> +
>   /*
>    * Compute a 8-bit checksum of a memory area. This algorithm follows
>    * the requirements of the Marvell SoC BootROM specifications.
> @@ -1031,6 +1045,7 @@ static size_t image_headersz_v1(int *hasext)
>   	struct image_cfg_element *e;
>   	unsigned int count;
>   	size_t headersz;
> +	int cpu_sheeva;
>   	struct stat s;
>   	int cfgi;
>   	int ret;
> @@ -1047,6 +1062,8 @@ static size_t image_headersz_v1(int *hasext)
>   			*hasext = 1;
>   	}
>   
> +	cpu_sheeva = image_is_cpu_sheeva();
> +
>   	count = 0;
>   	for (cfgi = 0; cfgi < cfgn; cfgi++) {
>   		e = &image_cfg[cfgi];
> @@ -1089,15 +1106,25 @@ static size_t image_headersz_v1(int *hasext)
>   			/*
>   			 * BootROM loads kwbimage header (in which the
>   			 * executable code is also stored) to address
> -			 * 0x40000000. Thus there is restriction for the
> -			 * load address of the N-th BINARY image.
> +			 * 0x40004000 or 0x40000000. Thus there is
> +			 * restriction for the load address of the N-th
> +			 * BINARY image.
>   			 */
> -			unsigned int low_addr, high_addr;
> +			unsigned int base_addr, low_addr, high_addr;
>   
> -			low_addr = 0x40000000 + headersz;
> +			base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
> +			low_addr = base_addr + headersz;
>   			high_addr = low_addr +
>   				    (BINARY_MAX_ARGS - e->binary.nargs) * sizeof(uint32_t);
>   
> +			if (cpu_sheeva && e->binary.loadaddr % 16) {
> +				fprintf(stderr,
> +					"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
> +					"Address for CPU SHEEVA must be 16-byte aligned.\n",
> +					e->binary.loadaddr, e->binary.file, e->binary.nargs);
> +				return 0;
> +			}
> +
>   			if (e->binary.loadaddr % 4 || e->binary.loadaddr < low_addr ||
>   			    e->binary.loadaddr > high_addr) {
>   				fprintf(stderr,
> @@ -1107,7 +1134,7 @@ static size_t image_headersz_v1(int *hasext)
>   					e->binary.nargs, low_addr, high_addr);
>   				return 0;
>   			}
> -			headersz = e->binary.loadaddr - 0x40000000;
> +			headersz = e->binary.loadaddr - base_addr;
>   		} else {
>   			headersz = ALIGN(headersz, 16);
>   		}
> @@ -1128,10 +1155,12 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
>   				struct main_hdr_v1 *main_hdr)
>   {
>   	struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;
> +	uint32_t base_addr;
>   	uint32_t add_args;
>   	uint32_t offset;
>   	uint32_t *args;
>   	size_t binhdrsz;
> +	int cpu_sheeva;
>   	struct stat s;
>   	int argi;
>   	FILE *bin;
> @@ -1163,18 +1192,22 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
>   	*cur += (binarye->binary.nargs + 1) * sizeof(uint32_t);
>   
>   	/*
> -	 * ARM executable code inside the BIN header on some mvebu platforms
> -	 * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
> +	 * ARM executable code inside the BIN header on platforms with Sheeva
> +	 * CPU (A370 and AXP) must always be aligned with the 128-bit boundary.
>   	 * In the case when this code is not position independent (e.g. ARM
>   	 * SPL), it must be placed at fixed load and execute address.
>   	 * This requirement can be met by inserting dummy arguments into
>   	 * BIN header, if needed.
>   	 */
> +	cpu_sheeva = image_is_cpu_sheeva();
> +	base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
>   	offset = *cur - (uint8_t *)main_hdr;
>   	if (binarye->binary.loadaddr)
> -		add_args = (binarye->binary.loadaddr - 0x40000000 - offset) / sizeof(uint32_t);
> -	else
> +		add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
> +	else if (cpu_sheeva)
>   		add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
> +	else
> +		add_args = 0;
>   	if (add_args) {
>   		*(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args);
>   		*cur += add_args * sizeof(uint32_t);
> @@ -1553,6 +1586,18 @@ static int image_create_config_parse_oneline(char *line,
>   	case IMAGE_CFG_VERSION:
>   		el->version = atoi(value1);
>   		break;
> +	case IMAGE_CFG_CPU:
> +		if (strcmp(value1, "FEROCEON") == 0)
> +			el->cpu_sheeva = 0;
> +		else if (strcmp(value1, "SHEEVA") == 0)
> +			el->cpu_sheeva = 1;
> +		else if (strcmp(value1, "A9") == 0)
> +			el->cpu_sheeva = 0;
> +		else {
> +			fprintf(stderr, "Invalid CPU %s\n", value1);
> +			return -1;
> +		}
> +		break;
>   	case IMAGE_CFG_BOOT_FROM:
>   		ret = image_boot_mode_id(value1);
>   
> @@ -1862,7 +1907,7 @@ static void kwbimage_print_header(const void *ptr)
>   			printf("BIN Img Size: ");
>   			genimg_print_size(opt_hdr_v1_size(ohdr) - 12 -
>   					  4 * ohdr->data[0]);
> -			printf("BIN Img Addr: %08x\n", 0x40000000 +
> +			printf("BIN Img Offs: %08x\n",
>   				(unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
>   				8 + 4 * ohdr->data[0]);
>   		}
> @@ -2031,6 +2076,11 @@ static int kwbimage_generate(struct image_tool_params *params,
>   			free(image_cfg);
>   			exit(EXIT_FAILURE);
>   		}
> +		if (alloc_len > 192*1024) {
> +			fprintf(stderr, "Header is too big (%u bytes), maximal kwbimage header size is %u bytes\n", alloc_len, 192*1024);
> +			free(image_cfg);
> +			exit(EXIT_FAILURE);
> +		}
>   		break;
>   
>   	default:
> @@ -2079,6 +2129,7 @@ static int kwbimage_generate_config(void *ptr, struct image_tool_params *params)
>   	struct ext_hdr_v0_reg *regdata;
>   	struct ext_hdr_v0 *ehdr0;
>   	struct opt_hdr_v1 *ohdr;
> +	unsigned offset;
>   	int cur_idx;
>   	int version;
>   	FILE *f;
> @@ -2145,9 +2196,9 @@ static int kwbimage_generate_config(void *ptr, struct image_tool_params *params)
>   			fprintf(f, "BINARY binary%d.bin", cur_idx);
>   			for (i = 0; i < ohdr->data[0]; i++)
>   				fprintf(f, " 0x%x", le32_to_cpu(((uint32_t *)ohdr->data)[i + 1]));
> -			fprintf(f, " LOAD_ADDRESS 0x%08x\n",
> -				0x40000000 + (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
> -				8 + 4 * ohdr->data[0]);
> +			offset = (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) + 8 + 4 * ohdr->data[0];
> +			fprintf(f, " LOAD_ADDRESS 0x%08x\n", 0x40000000 + offset);
> +			fprintf(f, " # for CPU SHEEVA: LOAD_ADDRESS 0x%08x\n", 0x40004000 + offset);
>   			cur_idx++;
>   		} else if (ohdr->headertype == OPT_HDR_V1_REGISTER_TYPE) {
>   			regset_hdr = (struct register_set_hdr_v1 *)ohdr;
> 

Re: [PATCH u-boot-marvell 00/16] tools: kwbimage: Load address fixes

[Pali Rohár]

On Wednesday 12 January 2022 16:06:53 Stefan Roese wrote:
> On 1/12/22 15:16, Pali Rohár wrote:
> > On Wednesday 12 January 2022 14:53:23 Stefan Roese wrote:
> > > On 1/12/22 12:34, Pali Rohár wrote:
> > > > On Wednesday 12 January 2022 12:06:22 Stefan Roese wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On 1/12/22 11:55, Stefan Roese wrote:
> > > > > > On 1/12/22 11:41, Pali Rohár wrote:
> > > > > > > On Wednesday 12 January 2022 08:26:10 Stefan Roese wrote:
> > > > > > > > Hi Pali,
> > > > > > > > 
> > > > > > > > while testing with this patchset (amongst others), I get this error
> > > > > > > > while building for "theadorable_debug":
> > > > > > > > 
> > > > > > > > $ make theadorable_debug_defconfig
> > > > > > > > $ make -s -j20
> > > > > > > > Invalid LOAD_ADDRESS 0x40004030 for BINARY spl/u-boot-spl.bin
> > > > > > > > with 0 args.
> > > > > > > > Address must be 4-byte aligned and in range 0x40000028-0x40000424
> > > > > > > > .make: *** [Makefile:1448: u-boot-spl.kwb] Error 1
> > > > > > > > make: *** Deleting file 'u-boot-spl.kwb'
> > > > > > > > 
> > > > > > > > Could you please take a look on whats going wrong here? Do I need to
> > > > > > > > change CONFIG_SPL_TEXT_BASE now? And if yes, why?
> > > > > > > 
> > > > > > > Hello!
> > > > > > > 
> > > > > > > If everything is working fine without this patch series then address
> > > > > > > must not be changed.
> > > > > > 
> > > > > > Yes, everything works just fine without out this series.
> > > > > > 
> > > > > > > Now I realized that some boards have text base address 0x40004030 and
> > > > > > > some have 0x40000030. When I was looking it during writing this patch
> > > > > > > series, I have not spotted that there is different digit "4" in the
> > > > > > > middle... And therefore I was in impression that every 32-bit Armada has
> > > > > > > base address 0x40000000 (increased by magic constant 0x30 which is
> > > > > > > explained in one of the patches).
> > > > > > 
> > > > > > I see.
> > > > > > 
> > > > > > > So now I need to figure out, why some boards have base address
> > > > > > > 0x40004000 and some have 0x40000000. It it somewhere documented this
> > > > > > > magic address? Or do you know source of this address for your board?
> > > > > > 
> > > > > > This is so loooong ago that I worked on this. I can only guess, that the
> > > > > > are up to offset 0x4000 was reserved by/for the BootROM.
> > > > > 
> > > > > This info is still in some of the config headers:
> > > > > 
> > > > > /*
> > > > >    * Memory layout while starting into the bin_hdr via the
> > > > >    * BootROM:
> > > > >    *
> > > > >    * 0x4000.4000 - 0x4003.4000	headers space (192KiB)
> > > > >    * 0x4000.4030			bin_hdr start address
> > > > >    * 0x4003.4000 - 0x4004.7c00	BootROM memory allocations (15KiB)
> > > > >    * 0x4007.fffc			BootROM stack top
> > > > >    *
> > > > >    * The address space between 0x4007.fffc and 0x400f.fff is not locked in
> > > > >    * L2 cache thus cannot be used.
> > > > >    */
> > > > > 
> > > > > HTP.
> > > > > 
> > > > > Thanks,
> > > > > Stefan
> > > > 
> > > > And now I found this information in old Marvell U-Boot fork from 2013:
> > > > https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/blob/u-boot-2013.01-armada-18.06/tools/marvell/bin_hdr/inc/common/soc_spec.h#L84-L92
> > > > 
> > > > #if defined(MV_TEST_PLATFORM)
> > > > 	#define RAM_TOP			0x81004000 /*  Use PEX memory - (16KB for MMU table) */
> > > > #elif defined(MV88F6710) || defined(MV88F78X60)
> > > > 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB - (16KB for MMU table) */
> > > > #elif defined(MV88F66XX) || defined(MV88F68XX) || defined(MV88F672X) || defined(MV88F69XX)
> > > > 	#define RAM_TOP			0x40000000 /*  L2 cache 512KB */
> > > > #else
> > > > 	#define RAM_TOP			0x40004000 /*  L2 cache 512KB */
> > > > #endif
> > > > 
> > > > IIRC this is chip conversion table:
> > > > 
> > > > 88F6710 = A370
> > > > MV78X60 = AXP
> > > > 88F66xx = Avanta
> > > > 88F672x = A375
> > > > 88F68xx = A38x
> > > > 88F69xx = A39x
> > > > 
> > > > So it looks like that only AXP and A370 use address 0x40004000, other
> > > > platforms use 0x40000000. AXP and A370 are the last SoCs which use
> > > > Marvell custom CPU cores Sheeva, all later have ARM A9 cores. Also in
> > > > functional specifications for AXP and A370 is written that executable
> > > > code needs to be aligned to 128-bit boundary and in later SoCs specs
> > > > there is no written restriction, like this. On A385 I tested that this
> > > > alignment is not really required. So for me it looks like that this 16kB
> > > > reservation is needed for Marvell custom CPU cores only and is some CPU
> > > > core specific.
> > > 
> > > Makes perfect sense, yes.
> > > 
> > > > The only suspicious thing is that in configs/db-88f6720_defconfig is
> > > > defined CONFIG_SPL_TEXT_BASE=0x40004030 and this is A375 (not A370!).
> > > > But now I found your commit 606576d54b673 ("arm: mvebu: Add basic
> > > > support for Armada 375 eval board db-88f6720") where you introduced this
> > > > #define CONFIG_SPL_TEXT_BASE 0x40004030 and wrote "the SPL U-Boot is not
> > > > fully functional.". So with this base address SPL would never work. I
> > > > know that Serdes+ddr3 init code is missing, so no SPL for this platform.
> > > 
> > > AFAIR, this port was done not that thoroughly, which would explain this
> > > mismatch you describe above. And could perhaps be fixed by changing this
> > > SPL_TEXT_BASE - if someone would like to invest some more time here.
> > > 
> > > > So how to solve the problem that kwbimage needs to know if is building
> > > > image for platform with 16kB reserved for MMU table?
> > > > 
> > > > Should I add a new kwbimage command which specifies this information,
> > > > like building image for Marvell CPU cores (Sheeva)? Or do you have other
> > > > idea? With this information I can adjust code to enable 128-bit boundary
> > > > restrictions only for these CPUs...
> > > 
> > > The first idea is to change this error to a warning, with some comment
> > > that this might work on these specific AXP and A370 platforms. Another
> > > idea is to add a 2nd valid address area.
> > > 
> > > Thanks,
> > > Stefan
> > 
> > I do not think that changing error to warning would help here. With
> > these changes kwbimage really try to sets load address to specified one.
> > And if kwbimage thinks that base address is on different location then
> > it would generate something unbootable.
> 
> Makes sense, okay.
> 
> > Now I played with it... could you try to test following diff? It
> > propagates information about CPU into kwbimage and then it do different
> > checks and use different base address based on CPU.
> 
> Looks good, please see the boot log here:
> 
> $ ./tools/kwboot -B 115200 -b u-boot-spl.kwb -t
...
> U-Boot 2022.01-00487-g655eb7d4bc34-dirty (Jan 12 2022 - 16:02:28 +0100)
> 
> SoC:   MV78260-B0 at 1333 MHz
> DRAM:  2 GiB (667 MHz, 64-bit, ECC not enabled)
> Core:  24 devices, 14 uclasses, devicetree: separate
> Loading Environment from SPIFlash... SF: Detected m25p128 with page size 256
> Bytes, erase size 256 KiB, total 16 MiB
> OK
> Model: Marvell Armada XP theadorable
> pcie0.0: Link up
> pcie2.0: Link up
> Net:   eth0: ethernet@70000
> ...
> 
> :)
> 
> Thanks,
> Stefan

Perfect! So I will integrate that diff into patches in this series and
send V2.

[PATCH u-boot-marvell v2 00/20] tools: kwbimage: Load address fixes

[Pali Rohár]

This patch series fixes generating images in kwbimage format, main fix
is setting correct load address of U-Boot SPL. Also it adds support for
generating kwbimage config file from existing kwbimage file via
dumpimage tool.

Changes in v2:
* Fix base address for Sheeva CPUs (A370, AXP), it is 0x40004000
* Fix information about mapped area of load address (it is L2, not CESA)
* Add new kwbimage config option CPU

Pali Rohár (20):
  tools: kwbimage: Mark all local functions as static
  tools: kwbimage: Deduplicate v1 regtype header finishing
  tools: kwbimage: Fix generating image with multiple DATA_DELAY
    commands
  tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY
    commands
  arm: mvebu: Generate kwbimage.cfg with $(call cmd, ...)
  tools: kwbimage: Add support for specifying CPU core
  tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY
    command
  tools: kwbimage: Check the return value of image_headersz_v1()
  tools: kwbimage: Check for maximal kwbimage header size
  arm: mvebu: Set CPU for U-Boot SPL binary in kwbimage
  arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in
    kwbimage
  tools: kwbimage: Enforce 128-bit boundary alignment only for Sheeva
    CPU
  arm: mvebu: Enable BootROM output on A38x
  tools: kwbimage: Add missing check for maximal value for DATA_DELAY
  tools: kwbimage: Show binary image offset in mkimage -l, in addition
    to size
  tools: kwbimage: Dump kwbimage config file on '-p -1' option
  tools: kwbimage: Do not cast const pointers to non-const pointers
  tools: kwbimage/kwboot: Check ext field for non-zero value
  tools: kwbimage: Extract main data image without -p arg for dumpimage
  tools: kwbimage: Fix mkimage/dumpimage -l argument

 arch/arm/mach-mvebu/Makefile        |  25 +-
 arch/arm/mach-mvebu/kwbimage.cfg.in |  10 +-
 tools/kwbimage.c                    | 549 +++++++++++++++++++++++-----
 tools/kwbimage.h                    |  10 +-
 tools/kwboot.c                      |   4 +-
 5 files changed, 486 insertions(+), 112 deletions(-)

-- 
2.20.1

[PATCH u-boot-marvell v2 01/20] tools: kwbimage: Mark all local functions as static

[Pali Rohár]

Mark all local functions as static.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 224d8156be19..b0598cab4bc7 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -199,7 +199,7 @@ static const char *image_boot_mode_name(unsigned int id)
 	return NULL;
 }
 
-int image_boot_mode_id(const char *boot_mode_name)
+static int image_boot_mode_id(const char *boot_mode_name)
 {
 	int i;
 
@@ -210,7 +210,7 @@ int image_boot_mode_id(const char *boot_mode_name)
 	return -1;
 }
 
-int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
+static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
 {
 	int i;
 
@@ -602,7 +602,8 @@ static int kwb_export_pubkey(RSA *key, struct pubkey_der_v1 *dst, FILE *hashf,
 	return 0;
 }
 
-int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig, char *signame)
+static int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig,
+		    char *signame)
 {
 	EVP_PKEY *evp_key;
 	EVP_MD_CTX *ctx;
@@ -662,8 +663,8 @@ err_key:
 	return ret;
 }
 
-int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
-	       char *signame)
+static int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
+		      char *signame)
 {
 	EVP_PKEY *evp_key;
 	EVP_MD_CTX *ctx;
@@ -722,8 +723,8 @@ err_key:
 	return ret;
 }
 
-int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
-			char *signame)
+static int kwb_sign_and_verify(RSA *key, void *data, int datasz,
+			       struct sig_v1 *sig, char *signame)
 {
 	if (kwb_sign(key, data, datasz, sig, signame) < 0)
 		return -1;
@@ -735,7 +736,7 @@ int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
 }
 
 
-int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr)
+static int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr)
 {
 	struct hash_v1 kak_pub_hash;
 	struct image_cfg_element *e;
@@ -1051,9 +1052,9 @@ static size_t image_headersz_v1(int *hasext)
 	return image_headersz_align(headersz, image_get_bootfrom());
 }
 
-int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
-			 struct image_cfg_element *binarye,
-			 struct main_hdr_v1 *main_hdr)
+static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
+				struct image_cfg_element *binarye,
+				struct main_hdr_v1 *main_hdr)
 {
 	struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;
 	uint32_t add_args;
@@ -1135,7 +1136,7 @@ err_close:
 	return -1;
 }
 
-int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
+static int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
 {
 	FILE *hashf;
 	int res;
@@ -1154,8 +1155,8 @@ int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
 	return res < 0 ? 1 : 0;
 }
 
-int kwb_sign_csk_with_kak(struct image_tool_params *params,
-			  struct secure_hdr_v1 *secure_hdr, RSA *csk)
+static int kwb_sign_csk_with_kak(struct image_tool_params *params,
+				 struct secure_hdr_v1 *secure_hdr, RSA *csk)
 {
 	RSA *kak = NULL;
 	RSA *kak_pub = NULL;
@@ -1196,9 +1197,9 @@ int kwb_sign_csk_with_kak(struct image_tool_params *params,
 	return 0;
 }
 
-int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
-			 int payloadsz, size_t headersz, uint8_t *image,
-			 struct secure_hdr_v1 *secure_hdr)
+static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
+				int payloadsz, size_t headersz, uint8_t *image,
+				struct secure_hdr_v1 *secure_hdr)
 {
 	struct image_cfg_element *e_jtagdelay;
 	struct image_cfg_element *e_boxid;
@@ -1415,7 +1416,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	return image;
 }
 
-int recognize_keyword(char *keyword)
+static int recognize_keyword(char *keyword)
 {
 	int kw_id;
 
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 01/20] tools: kwbimage: Mark all local functions as static

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Mark all local functions as static.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 37 +++++++++++++++++++------------------
>   1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 224d8156be19..b0598cab4bc7 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -199,7 +199,7 @@ static const char *image_boot_mode_name(unsigned int id)
>   	return NULL;
>   }
>   
> -int image_boot_mode_id(const char *boot_mode_name)
> +static int image_boot_mode_id(const char *boot_mode_name)
>   {
>   	int i;
>   
> @@ -210,7 +210,7 @@ int image_boot_mode_id(const char *boot_mode_name)
>   	return -1;
>   }
>   
> -int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
> +static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
>   {
>   	int i;
>   
> @@ -602,7 +602,8 @@ static int kwb_export_pubkey(RSA *key, struct pubkey_der_v1 *dst, FILE *hashf,
>   	return 0;
>   }
>   
> -int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig, char *signame)
> +static int kwb_sign(RSA *key, void *data, int datasz, struct sig_v1 *sig,
> +		    char *signame)
>   {
>   	EVP_PKEY *evp_key;
>   	EVP_MD_CTX *ctx;
> @@ -662,8 +663,8 @@ err_key:
>   	return ret;
>   }
>   
> -int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
> -	       char *signame)
> +static int kwb_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
> +		      char *signame)
>   {
>   	EVP_PKEY *evp_key;
>   	EVP_MD_CTX *ctx;
> @@ -722,8 +723,8 @@ err_key:
>   	return ret;
>   }
>   
> -int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
> -			char *signame)
> +static int kwb_sign_and_verify(RSA *key, void *data, int datasz,
> +			       struct sig_v1 *sig, char *signame)
>   {
>   	if (kwb_sign(key, data, datasz, sig, signame) < 0)
>   		return -1;
> @@ -735,7 +736,7 @@ int kwb_sign_and_verify(RSA *key, void *data, int datasz, struct sig_v1 *sig,
>   }
>   
>   
> -int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr)
> +static int kwb_dump_fuse_cmds_38x(FILE *out, struct secure_hdr_v1 *sec_hdr)
>   {
>   	struct hash_v1 kak_pub_hash;
>   	struct image_cfg_element *e;
> @@ -1051,9 +1052,9 @@ static size_t image_headersz_v1(int *hasext)
>   	return image_headersz_align(headersz, image_get_bootfrom());
>   }
>   
> -int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
> -			 struct image_cfg_element *binarye,
> -			 struct main_hdr_v1 *main_hdr)
> +static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
> +				struct image_cfg_element *binarye,
> +				struct main_hdr_v1 *main_hdr)
>   {
>   	struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;
>   	uint32_t add_args;
> @@ -1135,7 +1136,7 @@ err_close:
>   	return -1;
>   }
>   
> -int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
> +static int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
>   {
>   	FILE *hashf;
>   	int res;
> @@ -1154,8 +1155,8 @@ int export_pub_kak_hash(RSA *kak, struct secure_hdr_v1 *secure_hdr)
>   	return res < 0 ? 1 : 0;
>   }
>   
> -int kwb_sign_csk_with_kak(struct image_tool_params *params,
> -			  struct secure_hdr_v1 *secure_hdr, RSA *csk)
> +static int kwb_sign_csk_with_kak(struct image_tool_params *params,
> +				 struct secure_hdr_v1 *secure_hdr, RSA *csk)
>   {
>   	RSA *kak = NULL;
>   	RSA *kak_pub = NULL;
> @@ -1196,9 +1197,9 @@ int kwb_sign_csk_with_kak(struct image_tool_params *params,
>   	return 0;
>   }
>   
> -int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
> -			 int payloadsz, size_t headersz, uint8_t *image,
> -			 struct secure_hdr_v1 *secure_hdr)
> +static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
> +				int payloadsz, size_t headersz, uint8_t *image,
> +				struct secure_hdr_v1 *secure_hdr)
>   {
>   	struct image_cfg_element *e_jtagdelay;
>   	struct image_cfg_element *e_boxid;
> @@ -1415,7 +1416,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	return image;
>   }
>   
> -int recognize_keyword(char *keyword)
> +static int recognize_keyword(char *keyword)
>   {
>   	int kw_id;
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 02/20] tools: kwbimage: Deduplicate v1 regtype header finishing

[Pali Rohár]

Deduplicate code that finishes OPT_HDR_V1_REGISTER_TYPE header by
extracing it into separate function.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index b0598cab4bc7..552fef9e9aeb 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1249,6 +1249,22 @@ static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
 	return 0;
 }
 
+static void finish_register_set_header_v1(uint8_t **cur, uint8_t **next_ext,
+					  struct register_set_hdr_v1 *register_set_hdr,
+					  int *datai, uint8_t delay)
+{
+	int size = sizeof(struct register_set_hdr_v1) + 8 * (*datai) + 4;
+
+	register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
+	register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
+	register_set_hdr->headersz_msb = size >> 16;
+	register_set_hdr->data[*datai].last_entry.delay = delay;
+	*cur += size;
+	**next_ext = 1;
+	*next_ext = &register_set_hdr->data[*datai].last_entry.next;
+	*datai = 0;
+}
+
 static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 			     uint8_t *ptr, int payloadsz)
 {
@@ -1261,7 +1277,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	uint8_t *image, *cur;
 	int hasext = 0;
 	uint8_t *next_ext = NULL;
-	int cfgi, datai, size;
+	int cfgi, datai;
 
 	/*
 	 * Calculate the size of the header and the size of the
@@ -1359,15 +1375,8 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 		    e->type != IMAGE_CFG_DATA_DELAY)
 			continue;
 		if (e->type == IMAGE_CFG_DATA_DELAY) {
-			size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
-			register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
-			register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
-			register_set_hdr->headersz_msb = size >> 16;
-			register_set_hdr->data[datai].last_entry.delay = e->regdata_delay;
-			cur += size;
-			*next_ext = 1;
-			next_ext = &register_set_hdr->data[datai].last_entry.next;
-			datai = 0;
+			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
+						      &datai, e->regdata_delay);
 			continue;
 		}
 		register_set_hdr->data[datai].entry.address =
@@ -1377,15 +1386,9 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 		datai++;
 	}
 	if (datai != 0) {
-		size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
-		register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
-		register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
-		register_set_hdr->headersz_msb = size >> 16;
-		/* Set delay to the smallest possible value 1ms. */
-		register_set_hdr->data[datai].last_entry.delay = 1;
-		cur += size;
-		*next_ext = 1;
-		next_ext = &register_set_hdr->data[datai].last_entry.next;
+		/* Set delay to the smallest possible value. */
+		finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
+					      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
 	}
 
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 02/20] tools: kwbimage: Deduplicate v1 regtype header finishing

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Deduplicate code that finishes OPT_HDR_V1_REGISTER_TYPE header by
> extracing it into separate function.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 41 ++++++++++++++++++++++-------------------
>   1 file changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index b0598cab4bc7..552fef9e9aeb 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1249,6 +1249,22 @@ static int add_secure_header_v1(struct image_tool_params *params, uint8_t *ptr,
>   	return 0;
>   }
>   
> +static void finish_register_set_header_v1(uint8_t **cur, uint8_t **next_ext,
> +					  struct register_set_hdr_v1 *register_set_hdr,
> +					  int *datai, uint8_t delay)
> +{
> +	int size = sizeof(struct register_set_hdr_v1) + 8 * (*datai) + 4;
> +
> +	register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
> +	register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
> +	register_set_hdr->headersz_msb = size >> 16;
> +	register_set_hdr->data[*datai].last_entry.delay = delay;
> +	*cur += size;
> +	**next_ext = 1;
> +	*next_ext = &register_set_hdr->data[*datai].last_entry.next;
> +	*datai = 0;
> +}
> +
>   static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   			     uint8_t *ptr, int payloadsz)
>   {
> @@ -1261,7 +1277,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	uint8_t *image, *cur;
>   	int hasext = 0;
>   	uint8_t *next_ext = NULL;
> -	int cfgi, datai, size;
> +	int cfgi, datai;
>   
>   	/*
>   	 * Calculate the size of the header and the size of the
> @@ -1359,15 +1375,8 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   		    e->type != IMAGE_CFG_DATA_DELAY)
>   			continue;
>   		if (e->type == IMAGE_CFG_DATA_DELAY) {
> -			size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
> -			register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
> -			register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
> -			register_set_hdr->headersz_msb = size >> 16;
> -			register_set_hdr->data[datai].last_entry.delay = e->regdata_delay;
> -			cur += size;
> -			*next_ext = 1;
> -			next_ext = &register_set_hdr->data[datai].last_entry.next;
> -			datai = 0;
> +			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
> +						      &datai, e->regdata_delay);
>   			continue;
>   		}
>   		register_set_hdr->data[datai].entry.address =
> @@ -1377,15 +1386,9 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   		datai++;
>   	}
>   	if (datai != 0) {
> -		size = sizeof(struct register_set_hdr_v1) + 8 * datai + 4;
> -		register_set_hdr->headertype = OPT_HDR_V1_REGISTER_TYPE;
> -		register_set_hdr->headersz_lsb = cpu_to_le16(size & 0xFFFF);
> -		register_set_hdr->headersz_msb = size >> 16;
> -		/* Set delay to the smallest possible value 1ms. */
> -		register_set_hdr->data[datai].last_entry.delay = 1;
> -		cur += size;
> -		*next_ext = 1;
> -		next_ext = &register_set_hdr->data[datai].last_entry.next;
> +		/* Set delay to the smallest possible value. */
> +		finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
> +					      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
>   	}
>   
>   	for (cfgi = 0; cfgi < cfgn; cfgi++) {
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 03/20] tools: kwbimage: Fix generating image with multiple DATA_DELAY commands

[Pali Rohár]

Register set header consists of sequence of DATA commands followed by
exactly one DATA_DELAY command. Thus if we are generating image with
multiple DATA_DELAY commands, we need to create more register set headers.

Fix calculation of image size with multiple DATA_DELAY commands and
correctly set pointer to struct register_set_hdr_v1 when initializing new
register set header.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 552fef9e9aeb..6ee3d0aaa86c 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -993,7 +993,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 
 static size_t image_headersz_v1(int *hasext)
 {
-	struct image_cfg_element *binarye;
+	struct image_cfg_element *binarye, *e;
 	unsigned int count;
 	size_t headersz;
 	int cfgi;
@@ -1010,7 +1010,18 @@ static size_t image_headersz_v1(int *hasext)
 			*hasext = 1;
 	}
 
-	count = image_count_options(IMAGE_CFG_DATA);
+	count = 0;
+	for (cfgi = 0; cfgi < cfgn; cfgi++) {
+		e = &image_cfg[cfgi];
+
+		if (e->type == IMAGE_CFG_DATA)
+			count++;
+
+		if (e->type == IMAGE_CFG_DATA_DELAY) {
+			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
+			count = 0;
+		}
+	}
 	if (count > 0)
 		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 
@@ -1368,12 +1379,13 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	}
 
 	datai = 0;
-	register_set_hdr = (struct register_set_hdr_v1 *)cur;
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
 		e = &image_cfg[cfgi];
 		if (e->type != IMAGE_CFG_DATA &&
 		    e->type != IMAGE_CFG_DATA_DELAY)
 			continue;
+		if (datai == 0)
+			register_set_hdr = (struct register_set_hdr_v1 *)cur;
 		if (e->type == IMAGE_CFG_DATA_DELAY) {
 			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
 						      &datai, e->regdata_delay);
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 03/20] tools: kwbimage: Fix generating image with multiple DATA_DELAY commands

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Register set header consists of sequence of DATA commands followed by
> exactly one DATA_DELAY command. Thus if we are generating image with
> multiple DATA_DELAY commands, we need to create more register set headers.
> 
> Fix calculation of image size with multiple DATA_DELAY commands and
> correctly set pointer to struct register_set_hdr_v1 when initializing new
> register set header.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 552fef9e9aeb..6ee3d0aaa86c 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -993,7 +993,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   
>   static size_t image_headersz_v1(int *hasext)
>   {
> -	struct image_cfg_element *binarye;
> +	struct image_cfg_element *binarye, *e;
>   	unsigned int count;
>   	size_t headersz;
>   	int cfgi;
> @@ -1010,7 +1010,18 @@ static size_t image_headersz_v1(int *hasext)
>   			*hasext = 1;
>   	}
>   
> -	count = image_count_options(IMAGE_CFG_DATA);
> +	count = 0;
> +	for (cfgi = 0; cfgi < cfgn; cfgi++) {
> +		e = &image_cfg[cfgi];
> +
> +		if (e->type == IMAGE_CFG_DATA)
> +			count++;
> +
> +		if (e->type == IMAGE_CFG_DATA_DELAY) {
> +			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
> +			count = 0;
> +		}
> +	}
>   	if (count > 0)
>   		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
>   
> @@ -1368,12 +1379,13 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	}
>   
>   	datai = 0;
> -	register_set_hdr = (struct register_set_hdr_v1 *)cur;
>   	for (cfgi = 0; cfgi < cfgn; cfgi++) {
>   		e = &image_cfg[cfgi];
>   		if (e->type != IMAGE_CFG_DATA &&
>   		    e->type != IMAGE_CFG_DATA_DELAY)
>   			continue;
> +		if (datai == 0)
> +			register_set_hdr = (struct register_set_hdr_v1 *)cur;
>   		if (e->type == IMAGE_CFG_DATA_DELAY) {
>   			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
>   						      &datai, e->regdata_delay);
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 04/20] tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY commands

[Pali Rohár]

Preserve the order of BINARY, DATA and DATA_DELAY commands as they appear
in the input file. They may depend on each other.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 58 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 6ee3d0aaa86c..17d3c3cf223c 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1017,7 +1017,8 @@ static size_t image_headersz_v1(int *hasext)
 		if (e->type == IMAGE_CFG_DATA)
 			count++;
 
-		if (e->type == IMAGE_CFG_DATA_DELAY) {
+		if (e->type == IMAGE_CFG_DATA_DELAY ||
+		    (e->type == IMAGE_CFG_BINARY && count > 0)) {
 			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 			count = 0;
 		}
@@ -1289,6 +1290,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	int hasext = 0;
 	uint8_t *next_ext = NULL;
 	int cfgi, datai;
+	uint8_t delay;
 
 	/*
 	 * Calculate the size of the header and the size of the
@@ -1382,34 +1384,50 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
 		e = &image_cfg[cfgi];
 		if (e->type != IMAGE_CFG_DATA &&
-		    e->type != IMAGE_CFG_DATA_DELAY)
+		    e->type != IMAGE_CFG_DATA_DELAY &&
+		    e->type != IMAGE_CFG_BINARY)
 			continue;
+
 		if (datai == 0)
 			register_set_hdr = (struct register_set_hdr_v1 *)cur;
-		if (e->type == IMAGE_CFG_DATA_DELAY) {
+
+		/* If delay is not specified, use the smallest possible value. */
+		if (e->type == IMAGE_CFG_DATA_DELAY)
+			delay = e->regdata_delay;
+		else
+			delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
+
+		/*
+		 * DATA_DELAY command is the last entry in the register set
+		 * header and BINARY command inserts new binary header.
+		 * Therefore BINARY command requires to finish register set
+		 * header if some DATA command was specified. And DATA_DELAY
+		 * command automatically finish register set header even when
+		 * there was no DATA command.
+		 */
+		if (e->type == IMAGE_CFG_DATA_DELAY ||
+		    (e->type == IMAGE_CFG_BINARY && datai != 0))
 			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
-						      &datai, e->regdata_delay);
-			continue;
+						      &datai, delay);
+
+		if (e->type == IMAGE_CFG_DATA) {
+			register_set_hdr->data[datai].entry.address =
+				cpu_to_le32(e->regdata.raddr);
+			register_set_hdr->data[datai].entry.value =
+				cpu_to_le32(e->regdata.rdata);
+			datai++;
+		}
+
+		if (e->type == IMAGE_CFG_BINARY) {
+			if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
+				return NULL;
 		}
-		register_set_hdr->data[datai].entry.address =
-			cpu_to_le32(e->regdata.raddr);
-		register_set_hdr->data[datai].entry.value =
-			cpu_to_le32(e->regdata.rdata);
-		datai++;
 	}
 	if (datai != 0) {
 		/* Set delay to the smallest possible value. */
+		delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
 		finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
-					      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
-	}
-
-	for (cfgi = 0; cfgi < cfgn; cfgi++) {
-		e = &image_cfg[cfgi];
-		if (e->type != IMAGE_CFG_BINARY)
-			continue;
-
-		if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
-			return NULL;
+					      &datai, delay);
 	}
 
 	if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz + headersz,
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 04/20] tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY commands

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Preserve the order of BINARY, DATA and DATA_DELAY commands as they appear
> in the input file. They may depend on each other.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 58 +++++++++++++++++++++++++++++++-----------------
>   1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 6ee3d0aaa86c..17d3c3cf223c 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1017,7 +1017,8 @@ static size_t image_headersz_v1(int *hasext)
>   		if (e->type == IMAGE_CFG_DATA)
>   			count++;
>   
> -		if (e->type == IMAGE_CFG_DATA_DELAY) {
> +		if (e->type == IMAGE_CFG_DATA_DELAY ||
> +		    (e->type == IMAGE_CFG_BINARY && count > 0)) {
>   			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
>   			count = 0;
>   		}
> @@ -1289,6 +1290,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	int hasext = 0;
>   	uint8_t *next_ext = NULL;
>   	int cfgi, datai;
> +	uint8_t delay;
>   
>   	/*
>   	 * Calculate the size of the header and the size of the
> @@ -1382,34 +1384,50 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	for (cfgi = 0; cfgi < cfgn; cfgi++) {
>   		e = &image_cfg[cfgi];
>   		if (e->type != IMAGE_CFG_DATA &&
> -		    e->type != IMAGE_CFG_DATA_DELAY)
> +		    e->type != IMAGE_CFG_DATA_DELAY &&
> +		    e->type != IMAGE_CFG_BINARY)
>   			continue;
> +
>   		if (datai == 0)
>   			register_set_hdr = (struct register_set_hdr_v1 *)cur;
> -		if (e->type == IMAGE_CFG_DATA_DELAY) {
> +
> +		/* If delay is not specified, use the smallest possible value. */
> +		if (e->type == IMAGE_CFG_DATA_DELAY)
> +			delay = e->regdata_delay;
> +		else
> +			delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
> +
> +		/*
> +		 * DATA_DELAY command is the last entry in the register set
> +		 * header and BINARY command inserts new binary header.
> +		 * Therefore BINARY command requires to finish register set
> +		 * header if some DATA command was specified. And DATA_DELAY
> +		 * command automatically finish register set header even when
> +		 * there was no DATA command.
> +		 */
> +		if (e->type == IMAGE_CFG_DATA_DELAY ||
> +		    (e->type == IMAGE_CFG_BINARY && datai != 0))
>   			finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
> -						      &datai, e->regdata_delay);
> -			continue;
> +						      &datai, delay);
> +
> +		if (e->type == IMAGE_CFG_DATA) {
> +			register_set_hdr->data[datai].entry.address =
> +				cpu_to_le32(e->regdata.raddr);
> +			register_set_hdr->data[datai].entry.value =
> +				cpu_to_le32(e->regdata.rdata);
> +			datai++;
> +		}
> +
> +		if (e->type == IMAGE_CFG_BINARY) {
> +			if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
> +				return NULL;
>   		}
> -		register_set_hdr->data[datai].entry.address =
> -			cpu_to_le32(e->regdata.raddr);
> -		register_set_hdr->data[datai].entry.value =
> -			cpu_to_le32(e->regdata.rdata);
> -		datai++;
>   	}
>   	if (datai != 0) {
>   		/* Set delay to the smallest possible value. */
> +		delay = REGISTER_SET_HDR_OPT_DELAY_MS(0);
>   		finish_register_set_header_v1(&cur, &next_ext, register_set_hdr,
> -					      &datai, REGISTER_SET_HDR_OPT_DELAY_MS(0));
> -	}
> -
> -	for (cfgi = 0; cfgi < cfgn; cfgi++) {
> -		e = &image_cfg[cfgi];
> -		if (e->type != IMAGE_CFG_BINARY)
> -			continue;
> -
> -		if (add_binary_header_v1(&cur, &next_ext, e, main_hdr))
> -			return NULL;
> +					      &datai, delay);
>   	}
>   
>   	if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz + headersz,
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 05/20] arm: mvebu: Generate kwbimage.cfg with $(call cmd, ...)

[Pali Rohár]

Usage of $(call cmd,...) is standard way to call other commands which
generate things.

It also has the advantage of printing build information in the form
  KWBCFG  arch/arm/mach-mvebu/kwbimage.cfg
if verbosity is disabled, and printing the build command otherwise.

Note that the '#' character needs to be escaped in Makefile when used as
value for make variable assignment.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/Makefile | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 7e9c206ed6b8..acbaa6449d3d 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -58,10 +58,13 @@ KWB_REPLACE += SEC_FUSE_DUMP
 KWB_CFG_SEC_FUSE_DUMP = a38x
 endif
 
+quiet_cmd_kwbcfg = KWBCFG  $@
+cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
+	<$< >$(dir $@)$(@F)
+
 $(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \
 		include/config/auto.conf
-	$(Q)sed -ne '$(foreach V,$(KWB_REPLACE),s/^#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
-	<$< >$(dir $@)$(@F)
+	$(call cmd,kwbcfg)
 
 endif # CONFIG_SPL_BUILD
 obj-y	+= gpio.o
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 05/20] arm: mvebu: Generate kwbimage.cfg with $(call cmd, ...)

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Usage of $(call cmd,...) is standard way to call other commands which
> generate things.
> 
> It also has the advantage of printing build information in the form
>    KWBCFG  arch/arm/mach-mvebu/kwbimage.cfg
> if verbosity is disabled, and printing the build command otherwise.
> 
> Note that the '#' character needs to be escaped in Makefile when used as
> value for make variable assignment.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   arch/arm/mach-mvebu/Makefile | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 7e9c206ed6b8..acbaa6449d3d 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -58,10 +58,13 @@ KWB_REPLACE += SEC_FUSE_DUMP
>   KWB_CFG_SEC_FUSE_DUMP = a38x
>   endif
>   
> +quiet_cmd_kwbcfg = KWBCFG  $@
> +cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
> +	<$< >$(dir $@)$(@F)
> +
>   $(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \
>   		include/config/auto.conf
> -	$(Q)sed -ne '$(foreach V,$(KWB_REPLACE),s/^#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
> -	<$< >$(dir $@)$(@F)
> +	$(call cmd,kwbcfg)
>   
>   endif # CONFIG_SPL_BUILD
>   obj-y	+= gpio.o
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 06/20] tools: kwbimage: Add support for specifying CPU core

[Pali Rohár]

For other changes it is required to know if CPU core is Sheeva or not.
Therefore add a new command CPU for specifying CPU.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwbimage.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 17d3c3cf223c..44843be2c13a 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -99,6 +99,7 @@ enum image_cfg_type {
 	IMAGE_CFG_NAND_BADBLK_LOCATION,
 	IMAGE_CFG_NAND_ECC_MODE,
 	IMAGE_CFG_NAND_PAGESZ,
+	IMAGE_CFG_CPU,
 	IMAGE_CFG_BINARY,
 	IMAGE_CFG_DATA,
 	IMAGE_CFG_DATA_DELAY,
@@ -129,6 +130,7 @@ static const char * const id_strs[] = {
 	[IMAGE_CFG_NAND_BADBLK_LOCATION] = "NAND_BADBLK_LOCATION",
 	[IMAGE_CFG_NAND_ECC_MODE] = "NAND_ECC_MODE",
 	[IMAGE_CFG_NAND_PAGESZ] = "NAND_PAGE_SIZE",
+	[IMAGE_CFG_CPU] = "CPU",
 	[IMAGE_CFG_BINARY] = "BINARY",
 	[IMAGE_CFG_DATA] = "DATA",
 	[IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",
@@ -152,6 +154,7 @@ struct image_cfg_element {
 	enum image_cfg_type type;
 	union {
 		unsigned int version;
+		unsigned int cpu_sheeva;
 		unsigned int bootfrom;
 		struct {
 			const char *file;
@@ -280,6 +283,17 @@ static int image_get_bootfrom(void)
 	return e->bootfrom;
 }
 
+static int image_is_cpu_sheeva(void)
+{
+	struct image_cfg_element *e;
+
+	e = image_find_option(IMAGE_CFG_CPU);
+	if (!e)
+		return 0;
+
+	return e->cpu_sheeva;
+}
+
 /*
  * Compute a 8-bit checksum of a memory area. This algorithm follows
  * the requirements of the Marvell SoC BootROM specifications.
@@ -1489,6 +1503,18 @@ static int image_create_config_parse_oneline(char *line,
 	case IMAGE_CFG_VERSION:
 		el->version = atoi(value1);
 		break;
+	case IMAGE_CFG_CPU:
+		if (strcmp(value1, "FEROCEON") == 0)
+			el->cpu_sheeva = 0;
+		else if (strcmp(value1, "SHEEVA") == 0)
+			el->cpu_sheeva = 1;
+		else if (strcmp(value1, "A9") == 0)
+			el->cpu_sheeva = 0;
+		else {
+			fprintf(stderr, "Invalid CPU %s\n", value1);
+			return -1;
+		}
+		break;
 	case IMAGE_CFG_BOOT_FROM:
 		ret = image_boot_mode_id(value1);
 
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 06/20] tools: kwbimage: Add support for specifying CPU core

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> For other changes it is required to know if CPU core is Sheeva or not.
> Therefore add a new command CPU for specifying CPU.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 26 ++++++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 17d3c3cf223c..44843be2c13a 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -99,6 +99,7 @@ enum image_cfg_type {
>   	IMAGE_CFG_NAND_BADBLK_LOCATION,
>   	IMAGE_CFG_NAND_ECC_MODE,
>   	IMAGE_CFG_NAND_PAGESZ,
> +	IMAGE_CFG_CPU,
>   	IMAGE_CFG_BINARY,
>   	IMAGE_CFG_DATA,
>   	IMAGE_CFG_DATA_DELAY,
> @@ -129,6 +130,7 @@ static const char * const id_strs[] = {
>   	[IMAGE_CFG_NAND_BADBLK_LOCATION] = "NAND_BADBLK_LOCATION",
>   	[IMAGE_CFG_NAND_ECC_MODE] = "NAND_ECC_MODE",
>   	[IMAGE_CFG_NAND_PAGESZ] = "NAND_PAGE_SIZE",
> +	[IMAGE_CFG_CPU] = "CPU",
>   	[IMAGE_CFG_BINARY] = "BINARY",
>   	[IMAGE_CFG_DATA] = "DATA",
>   	[IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",
> @@ -152,6 +154,7 @@ struct image_cfg_element {
>   	enum image_cfg_type type;
>   	union {
>   		unsigned int version;
> +		unsigned int cpu_sheeva;
>   		unsigned int bootfrom;
>   		struct {
>   			const char *file;
> @@ -280,6 +283,17 @@ static int image_get_bootfrom(void)
>   	return e->bootfrom;
>   }
>   
> +static int image_is_cpu_sheeva(void)
> +{
> +	struct image_cfg_element *e;
> +
> +	e = image_find_option(IMAGE_CFG_CPU);
> +	if (!e)
> +		return 0;
> +
> +	return e->cpu_sheeva;
> +}
> +
>   /*
>    * Compute a 8-bit checksum of a memory area. This algorithm follows
>    * the requirements of the Marvell SoC BootROM specifications.
> @@ -1489,6 +1503,18 @@ static int image_create_config_parse_oneline(char *line,
>   	case IMAGE_CFG_VERSION:
>   		el->version = atoi(value1);
>   		break;
> +	case IMAGE_CFG_CPU:
> +		if (strcmp(value1, "FEROCEON") == 0)
> +			el->cpu_sheeva = 0;
> +		else if (strcmp(value1, "SHEEVA") == 0)
> +			el->cpu_sheeva = 1;
> +		else if (strcmp(value1, "A9") == 0)
> +			el->cpu_sheeva = 0;
> +		else {
> +			fprintf(stderr, "Invalid CPU %s\n", value1);
> +			return -1;
> +		}
> +		break;
>   	case IMAGE_CFG_BOOT_FROM:
>   		ret = image_boot_mode_id(value1);
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 07/20] tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY command

[Pali Rohár]

ARM executable code included in kwbimage binary header, which is not
position independent, needs to be loaded and executed by BootROM at the
correct fixed address.

Armada BootROMs load kwbimage header (in which the executable code is also
stored) at fixed address 0x40004000 or 0x40000000 which is mapped to
L2-SRAM (L2 Cache as SRAM). Address 0x40004000 is used on Armada platforms
with Sheeva CPU core (A370 and AXP) where BootROM uses MMU with 0x4000
bytes for MMU translation table. Address 0x40000000 is used on all other
platforms.

Thus the only way to specify load and execute address of this executable
code in binary kwbimage header is by filling dummy arguments into the
binary header, using the same mechanism we already have for achieving
128-bit boundary alignment on A370 and AXP SoCs.

Extend kwbimage config file parser to allow to specify load address as
part of BINARY command with syntax:

    BINARY path_to_binary arg1 arg2 ... argN LOAD_ADDRESS address

If the specified load address is invalid or cannot be used, mkimage will
throw fatal error and exit. This will prevent generating kwbimage with
invalid load address for non-position independent binary code.

If no load address is specified, kwbimage will not fill any the dummy
arguments, thus it will behave the same as before this change.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 109 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 93 insertions(+), 16 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 44843be2c13a..c0f1bdac0210 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -158,6 +158,7 @@ struct image_cfg_element {
 		unsigned int bootfrom;
 		struct {
 			const char *file;
+			unsigned int loadaddr;
 			unsigned int args[BINARY_MAX_ARGS];
 			unsigned int nargs;
 		} binary;
@@ -1007,10 +1008,13 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 
 static size_t image_headersz_v1(int *hasext)
 {
-	struct image_cfg_element *binarye, *e;
+	struct image_cfg_element *e;
 	unsigned int count;
 	size_t headersz;
+	int cpu_sheeva;
+	struct stat s;
 	int cfgi;
+	int ret;
 
 	/*
 	 * Calculate the size of the header and the size of the
@@ -1024,6 +1028,8 @@ static size_t image_headersz_v1(int *hasext)
 			*hasext = 1;
 	}
 
+	cpu_sheeva = image_is_cpu_sheeva();
+
 	count = 0;
 	for (cfgi = 0; cfgi < cfgn; cfgi++) {
 		e = &image_cfg[cfgi];
@@ -1036,19 +1042,11 @@ static size_t image_headersz_v1(int *hasext)
 			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 			count = 0;
 		}
-	}
-	if (count > 0)
-		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
 
-	for (cfgi = 0; cfgi < cfgn; cfgi++) {
-		int ret;
-		struct stat s;
-
-		binarye = &image_cfg[cfgi];
-		if (binarye->type != IMAGE_CFG_BINARY)
+		if (e->type != IMAGE_CFG_BINARY)
 			continue;
 
-		ret = stat(binarye->binary.file, &s);
+		ret = stat(e->binary.file, &s);
 		if (ret < 0) {
 			char cwd[PATH_MAX];
 			char *dir = cwd;
@@ -1063,18 +1061,58 @@ static size_t image_headersz_v1(int *hasext)
 				"Didn't find the file '%s' in '%s' which is mandatory to generate the image\n"
 				"This file generally contains the DDR3 training code, and should be extracted from an existing bootable\n"
 				"image for your board. Use 'dumpimage -T kwbimage -p 0' to extract it from an existing image.\n",
-				binarye->binary.file, dir);
+				e->binary.file, dir);
 			return 0;
 		}
 
 		headersz += sizeof(struct opt_hdr_v1) + sizeof(uint32_t) +
-			(binarye->binary.nargs) * sizeof(uint32_t);
-		headersz = ALIGN(headersz, 16);
+			(e->binary.nargs) * sizeof(uint32_t);
+
+		if (e->binary.loadaddr) {
+			/*
+			 * BootROM loads kwbimage header (in which the
+			 * executable code is also stored) to address
+			 * 0x40004000 or 0x40000000. Thus there is
+			 * restriction for the load address of the N-th
+			 * BINARY image.
+			 */
+			unsigned int base_addr, low_addr, high_addr;
+
+			base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
+			low_addr = base_addr + headersz;
+			high_addr = low_addr +
+				    (BINARY_MAX_ARGS - e->binary.nargs) * sizeof(uint32_t);
+
+			if (cpu_sheeva && e->binary.loadaddr % 16) {
+				fprintf(stderr,
+					"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
+					"Address for CPU SHEEVA must be 16-byte aligned.\n",
+					e->binary.loadaddr, e->binary.file, e->binary.nargs);
+				return 0;
+			}
+
+			if (e->binary.loadaddr % 4 || e->binary.loadaddr < low_addr ||
+			    e->binary.loadaddr > high_addr) {
+				fprintf(stderr,
+					"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
+					"Address must be 4-byte aligned and in range 0x%08x-0x%08x.\n",
+					e->binary.loadaddr, e->binary.file,
+					e->binary.nargs, low_addr, high_addr);
+				return 0;
+			}
+			headersz = e->binary.loadaddr - base_addr;
+		} else {
+			headersz = ALIGN(headersz, 16);
+		}
+
 		headersz += ALIGN(s.st_size, 4) + sizeof(uint32_t);
 		if (hasext)
 			*hasext = 1;
 	}
 
+	if (count > 0)
+		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
+
 	return image_headersz_align(headersz, image_get_bootfrom());
 }
 
@@ -1083,10 +1121,12 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
 				struct main_hdr_v1 *main_hdr)
 {
 	struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;
+	uint32_t base_addr;
 	uint32_t add_args;
 	uint32_t offset;
 	uint32_t *args;
 	size_t binhdrsz;
+	int cpu_sheeva;
 	struct stat s;
 	int argi;
 	FILE *bin;
@@ -1120,11 +1160,18 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
 	/*
 	 * ARM executable code inside the BIN header on some mvebu platforms
 	 * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
+	 * In the case when this code is not position independent (e.g. ARM
+	 * SPL), it must be placed at fixed load and execute address.
 	 * This requirement can be met by inserting dummy arguments into
 	 * BIN header, if needed.
 	 */
+	cpu_sheeva = image_is_cpu_sheeva();
+	base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
 	offset = *cur - (uint8_t *)main_hdr;
-	add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
+	if (binarye->binary.loadaddr)
+		add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
+	else
+		add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
 	if (add_args) {
 		*(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args);
 		*cur += add_args * sizeof(uint32_t);
@@ -1548,10 +1595,40 @@ static int image_create_config_parse_oneline(char *line,
 		el->binary.file = strdup(value1);
 		while (1) {
 			char *value = strtok_r(NULL, delimiters, &saveptr);
+			char *endptr;
 
 			if (!value)
 				break;
-			el->binary.args[argi] = strtoul(value, NULL, 16);
+
+			if (!strcmp(value, "LOAD_ADDRESS")) {
+				value = strtok_r(NULL, delimiters, &saveptr);
+				if (!value) {
+					fprintf(stderr,
+						"Missing address argument for BINARY LOAD_ADDRESS\n");
+					return -1;
+				}
+				el->binary.loadaddr = strtoul(value, &endptr, 16);
+				if (*endptr) {
+					fprintf(stderr,
+						"Invalid argument '%s' for BINARY LOAD_ADDRESS\n",
+						value);
+					return -1;
+				}
+				value = strtok_r(NULL, delimiters, &saveptr);
+				if (value) {
+					fprintf(stderr,
+						"Unexpected argument '%s' after BINARY LOAD_ADDRESS\n",
+						value);
+					return -1;
+				}
+				break;
+			}
+
+			el->binary.args[argi] = strtoul(value, &endptr, 16);
+			if (*endptr) {
+				fprintf(stderr, "Invalid argument '%s' for BINARY\n", value);
+				return -1;
+			}
 			argi++;
 			if (argi >= BINARY_MAX_ARGS) {
 				fprintf(stderr,
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 07/20] tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY command

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> ARM executable code included in kwbimage binary header, which is not
> position independent, needs to be loaded and executed by BootROM at the
> correct fixed address.
> 
> Armada BootROMs load kwbimage header (in which the executable code is also
> stored) at fixed address 0x40004000 or 0x40000000 which is mapped to
> L2-SRAM (L2 Cache as SRAM). Address 0x40004000 is used on Armada platforms
> with Sheeva CPU core (A370 and AXP) where BootROM uses MMU with 0x4000
> bytes for MMU translation table. Address 0x40000000 is used on all other
> platforms.
> 
> Thus the only way to specify load and execute address of this executable
> code in binary kwbimage header is by filling dummy arguments into the
> binary header, using the same mechanism we already have for achieving
> 128-bit boundary alignment on A370 and AXP SoCs.
> 
> Extend kwbimage config file parser to allow to specify load address as
> part of BINARY command with syntax:
> 
>      BINARY path_to_binary arg1 arg2 ... argN LOAD_ADDRESS address
> 
> If the specified load address is invalid or cannot be used, mkimage will
> throw fatal error and exit. This will prevent generating kwbimage with
> invalid load address for non-position independent binary code.
> 
> If no load address is specified, kwbimage will not fill any the dummy
> arguments, thus it will behave the same as before this change.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 109 ++++++++++++++++++++++++++++++++++++++++-------
>   1 file changed, 93 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 44843be2c13a..c0f1bdac0210 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -158,6 +158,7 @@ struct image_cfg_element {
>   		unsigned int bootfrom;
>   		struct {
>   			const char *file;
> +			unsigned int loadaddr;
>   			unsigned int args[BINARY_MAX_ARGS];
>   			unsigned int nargs;
>   		} binary;
> @@ -1007,10 +1008,13 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   
>   static size_t image_headersz_v1(int *hasext)
>   {
> -	struct image_cfg_element *binarye, *e;
> +	struct image_cfg_element *e;
>   	unsigned int count;
>   	size_t headersz;
> +	int cpu_sheeva;
> +	struct stat s;
>   	int cfgi;
> +	int ret;
>   
>   	/*
>   	 * Calculate the size of the header and the size of the
> @@ -1024,6 +1028,8 @@ static size_t image_headersz_v1(int *hasext)
>   			*hasext = 1;
>   	}
>   
> +	cpu_sheeva = image_is_cpu_sheeva();
> +
>   	count = 0;
>   	for (cfgi = 0; cfgi < cfgn; cfgi++) {
>   		e = &image_cfg[cfgi];
> @@ -1036,19 +1042,11 @@ static size_t image_headersz_v1(int *hasext)
>   			headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
>   			count = 0;
>   		}
> -	}
> -	if (count > 0)
> -		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
>   
> -	for (cfgi = 0; cfgi < cfgn; cfgi++) {
> -		int ret;
> -		struct stat s;
> -
> -		binarye = &image_cfg[cfgi];
> -		if (binarye->type != IMAGE_CFG_BINARY)
> +		if (e->type != IMAGE_CFG_BINARY)
>   			continue;
>   
> -		ret = stat(binarye->binary.file, &s);
> +		ret = stat(e->binary.file, &s);
>   		if (ret < 0) {
>   			char cwd[PATH_MAX];
>   			char *dir = cwd;
> @@ -1063,18 +1061,58 @@ static size_t image_headersz_v1(int *hasext)
>   				"Didn't find the file '%s' in '%s' which is mandatory to generate the image\n"
>   				"This file generally contains the DDR3 training code, and should be extracted from an existing bootable\n"
>   				"image for your board. Use 'dumpimage -T kwbimage -p 0' to extract it from an existing image.\n",
> -				binarye->binary.file, dir);
> +				e->binary.file, dir);
>   			return 0;
>   		}
>   
>   		headersz += sizeof(struct opt_hdr_v1) + sizeof(uint32_t) +
> -			(binarye->binary.nargs) * sizeof(uint32_t);
> -		headersz = ALIGN(headersz, 16);
> +			(e->binary.nargs) * sizeof(uint32_t);
> +
> +		if (e->binary.loadaddr) {
> +			/*
> +			 * BootROM loads kwbimage header (in which the
> +			 * executable code is also stored) to address
> +			 * 0x40004000 or 0x40000000. Thus there is
> +			 * restriction for the load address of the N-th
> +			 * BINARY image.
> +			 */
> +			unsigned int base_addr, low_addr, high_addr;
> +
> +			base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
> +			low_addr = base_addr + headersz;
> +			high_addr = low_addr +
> +				    (BINARY_MAX_ARGS - e->binary.nargs) * sizeof(uint32_t);
> +
> +			if (cpu_sheeva && e->binary.loadaddr % 16) {
> +				fprintf(stderr,
> +					"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
> +					"Address for CPU SHEEVA must be 16-byte aligned.\n",
> +					e->binary.loadaddr, e->binary.file, e->binary.nargs);
> +				return 0;
> +			}
> +
> +			if (e->binary.loadaddr % 4 || e->binary.loadaddr < low_addr ||
> +			    e->binary.loadaddr > high_addr) {
> +				fprintf(stderr,
> +					"Invalid LOAD_ADDRESS 0x%08x for BINARY %s with %d args.\n"
> +					"Address must be 4-byte aligned and in range 0x%08x-0x%08x.\n",
> +					e->binary.loadaddr, e->binary.file,
> +					e->binary.nargs, low_addr, high_addr);
> +				return 0;
> +			}
> +			headersz = e->binary.loadaddr - base_addr;
> +		} else {
> +			headersz = ALIGN(headersz, 16);
> +		}
> +
>   		headersz += ALIGN(s.st_size, 4) + sizeof(uint32_t);
>   		if (hasext)
>   			*hasext = 1;
>   	}
>   
> +	if (count > 0)
> +		headersz += sizeof(struct register_set_hdr_v1) + 8 * count + 4;
> +
>   	return image_headersz_align(headersz, image_get_bootfrom());
>   }
>   
> @@ -1083,10 +1121,12 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
>   				struct main_hdr_v1 *main_hdr)
>   {
>   	struct opt_hdr_v1 *hdr = (struct opt_hdr_v1 *)*cur;
> +	uint32_t base_addr;
>   	uint32_t add_args;
>   	uint32_t offset;
>   	uint32_t *args;
>   	size_t binhdrsz;
> +	int cpu_sheeva;
>   	struct stat s;
>   	int argi;
>   	FILE *bin;
> @@ -1120,11 +1160,18 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
>   	/*
>   	 * ARM executable code inside the BIN header on some mvebu platforms
>   	 * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
> +	 * In the case when this code is not position independent (e.g. ARM
> +	 * SPL), it must be placed at fixed load and execute address.
>   	 * This requirement can be met by inserting dummy arguments into
>   	 * BIN header, if needed.
>   	 */
> +	cpu_sheeva = image_is_cpu_sheeva();
> +	base_addr = cpu_sheeva ? 0x40004000 : 0x40000000;
>   	offset = *cur - (uint8_t *)main_hdr;
> -	add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
> +	if (binarye->binary.loadaddr)
> +		add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
> +	else
> +		add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
>   	if (add_args) {
>   		*(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args);
>   		*cur += add_args * sizeof(uint32_t);
> @@ -1548,10 +1595,40 @@ static int image_create_config_parse_oneline(char *line,
>   		el->binary.file = strdup(value1);
>   		while (1) {
>   			char *value = strtok_r(NULL, delimiters, &saveptr);
> +			char *endptr;
>   
>   			if (!value)
>   				break;
> -			el->binary.args[argi] = strtoul(value, NULL, 16);
> +
> +			if (!strcmp(value, "LOAD_ADDRESS")) {
> +				value = strtok_r(NULL, delimiters, &saveptr);
> +				if (!value) {
> +					fprintf(stderr,
> +						"Missing address argument for BINARY LOAD_ADDRESS\n");
> +					return -1;
> +				}
> +				el->binary.loadaddr = strtoul(value, &endptr, 16);
> +				if (*endptr) {
> +					fprintf(stderr,
> +						"Invalid argument '%s' for BINARY LOAD_ADDRESS\n",
> +						value);
> +					return -1;
> +				}
> +				value = strtok_r(NULL, delimiters, &saveptr);
> +				if (value) {
> +					fprintf(stderr,
> +						"Unexpected argument '%s' after BINARY LOAD_ADDRESS\n",
> +						value);
> +					return -1;
> +				}
> +				break;
> +			}
> +
> +			el->binary.args[argi] = strtoul(value, &endptr, 16);
> +			if (*endptr) {
> +				fprintf(stderr, "Invalid argument '%s' for BINARY\n", value);
> +				return -1;
> +			}
>   			argi++;
>   			if (argi >= BINARY_MAX_ARGS) {
>   				fprintf(stderr,
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 08/20] tools: kwbimage: Check the return value of image_headersz_v1()

[Pali Rohár]

Function image_headersz_v1() may return zero on fatal errors.
In this case the function already printed an error message.

Check the return value of image_headersz_v1() in kwbimage_generate(),
and exit on zero value with EXIT_FAILURE.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index c0f1bdac0210..a5b518f60bc8 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -2029,6 +2029,10 @@ static int kwbimage_generate(struct image_tool_params *params,
 
 	case 1:
 		alloc_len = image_headersz_v1(NULL);
+		if (!alloc_len) {
+			free(image_cfg);
+			exit(EXIT_FAILURE);
+		}
 		break;
 
 	default:
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 08/20] tools: kwbimage: Check the return value of image_headersz_v1()

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Function image_headersz_v1() may return zero on fatal errors.
> In this case the function already printed an error message.
> 
> Check the return value of image_headersz_v1() in kwbimage_generate(),
> and exit on zero value with EXIT_FAILURE.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index c0f1bdac0210..a5b518f60bc8 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -2029,6 +2029,10 @@ static int kwbimage_generate(struct image_tool_params *params,
>   
>   	case 1:
>   		alloc_len = image_headersz_v1(NULL);
> +		if (!alloc_len) {
> +			free(image_cfg);
> +			exit(EXIT_FAILURE);
> +		}
>   		break;
>   
>   	default:
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 09/20] tools: kwbimage: Check for maximal kwbimage header size

[Pali Rohár]

BootROM loads kwbimage header to L2-SRAM and BootROM reserve only 192 kB for it.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwbimage.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index a5b518f60bc8..ce053a4a5a78 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -2033,6 +2033,11 @@ static int kwbimage_generate(struct image_tool_params *params,
 			free(image_cfg);
 			exit(EXIT_FAILURE);
 		}
+		if (alloc_len > 192*1024) {
+			fprintf(stderr, "Header is too big (%u bytes), maximal kwbimage header size is %u bytes\n", alloc_len, 192*1024);
+			free(image_cfg);
+			exit(EXIT_FAILURE);
+		}
 		break;
 
 	default:
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 09/20] tools: kwbimage: Check for maximal kwbimage header size

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> BootROM loads kwbimage header to L2-SRAM and BootROM reserve only 192 kB for it.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> ---
>   tools/kwbimage.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index a5b518f60bc8..ce053a4a5a78 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -2033,6 +2033,11 @@ static int kwbimage_generate(struct image_tool_params *params,
>   			free(image_cfg);
>   			exit(EXIT_FAILURE);
>   		}
> +		if (alloc_len > 192*1024) {
> +			fprintf(stderr, "Header is too big (%u bytes), maximal kwbimage header size is %u bytes\n", alloc_len, 192*1024);
> +			free(image_cfg);
> +			exit(EXIT_FAILURE);
> +		}

Nitpicking: Doesn't checkpatch.pl complain about the missing space
around '*'? And wouldn't it be better, to add a macro for this
'192 * 1024', as you are using it twice here already?

Other than this:

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

>   		break;
>   
>   	default:
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 10/20] arm: mvebu: Set CPU for U-Boot SPL binary in kwbimage

[Pali Rohár]

kwbimage needs to know CPU type, so set it in kwbimage config file.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 arch/arm/mach-mvebu/Makefile        | 8 ++++++++
 arch/arm/mach-mvebu/kwbimage.cfg.in | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index acbaa6449d3d..17006c9df9b0 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -30,6 +30,14 @@ obj-$(CONFIG_MVEBU_EFUSE) += efuse.o
 
 extra-y += kwbimage.cfg
 
+ifneq ($(CONFIG_ARMADA_370)$(CONFIG_ARMADA_XP),)
+	KWB_REPLACE += CPU
+	KWB_CFG_CPU = SHEEVA
+else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)
+	KWB_REPLACE += CPU
+	KWB_CFG_CPU = A9
+endif
+
 KWB_REPLACE += BOOT_FROM
 ifneq ($(CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI),)
 	KWB_CFG_BOOT_FROM=spi
diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
index 049d23c6ef08..8e720daf4820 100644
--- a/arch/arm/mach-mvebu/kwbimage.cfg.in
+++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
@@ -5,6 +5,9 @@
 # Armada 38x uses version 1 image format
 VERSION		1
 
+# Type of the CPU core
+#@CPU
+
 # Boot Media configurations
 #@BOOT_FROM
 
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 10/20] arm: mvebu: Set CPU for U-Boot SPL binary in kwbimage

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> kwbimage needs to know CPU type, so set it in kwbimage config file.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   arch/arm/mach-mvebu/Makefile        | 8 ++++++++
>   arch/arm/mach-mvebu/kwbimage.cfg.in | 3 +++
>   2 files changed, 11 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index acbaa6449d3d..17006c9df9b0 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -30,6 +30,14 @@ obj-$(CONFIG_MVEBU_EFUSE) += efuse.o
>   
>   extra-y += kwbimage.cfg
>   
> +ifneq ($(CONFIG_ARMADA_370)$(CONFIG_ARMADA_XP),)
> +	KWB_REPLACE += CPU
> +	KWB_CFG_CPU = SHEEVA
> +else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)
> +	KWB_REPLACE += CPU
> +	KWB_CFG_CPU = A9
> +endif
> +
>   KWB_REPLACE += BOOT_FROM
>   ifneq ($(CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI),)
>   	KWB_CFG_BOOT_FROM=spi
> diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
> index 049d23c6ef08..8e720daf4820 100644
> --- a/arch/arm/mach-mvebu/kwbimage.cfg.in
> +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
> @@ -5,6 +5,9 @@
>   # Armada 38x uses version 1 image format
>   VERSION		1
>   
> +# Type of the CPU core
> +#@CPU
> +
>   # Boot Media configurations
>   #@BOOT_FROM
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 11/20] arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in kwbimage

[Pali Rohár]

U-Boot SPL for mvebu platform is not compiled as position independent.
Therefore it is required to instruct BootROM to load U-Boot SPL at the
correct address. Loading of kwbimage binary code at specific address can be
now achieved by the new LOAD_ADDRESS token as part of BINARY command in
kwbimage config file.

Update mvebu Makefile to put value of $(CONFIG_SPL_TEXT_BASE) into
LOAD_ADDRESS token when generating kwbimage.cfg from kwbimage.cfg.in.

It is required to update regex for sed to find replacement tokens at any
position on a line in kwbimage config file and not only at the beginning of
the line. This is because LOAD_ADDRESS is specified at the end of line
containing the BINARY command.

It looks like all Armada boards set CONFIG_SPL_TEXT_BASE to value
0x40004030 or 0x40000030. Why this value? It is because main kwbimage
header is at address 0x40004030 or 0x40000000 and it is 32 bytes long.
After the main header there is the binary header, which consist of 1 byte
for type, 3 bytes for size, 1 byte for number of arguments, 3 reserved
bytes and then 4 bytes for each argument. After these arguments comes the
executable code.

So arguments start at address 0x40004028 or 0x40000028. Before commit
e6571f38c943 ("arm: mvebu: Remove dummy BIN header arguments for SPL
binary") there were two (dummy) arguments, which resulted in load address
of 0x40004030 or 0x40000030, always. After that commit (which removed dummy
arguments), load address stayed same due to the 128-bit alignment done by
mkimage.

This patch now reflects the dependency between $(CONFIG_SPL_TEXT_BASE),
load address and dummy kwbimage arguments, and allows the user to adjust
$(CONFIG_SPL_TEXT_BASE) config option to some other value.

For unsupported values, when mkimage/kwbimage cannot set chosen load address
as specified by $(CONFIG_SPL_TEXT_BASE), the build process now fails,
instead of silently generating non-working kwbimage.

Removal of this alignment between $(CONFIG_SPL_TEXT_BASE) and LOAD_ADDRESS
can only be done by compiling U-Boot SPL as position independent. But this
currently is not possible for 32-bit ARM version of U-Boot SPL.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/Makefile        | 5 ++++-
 arch/arm/mach-mvebu/kwbimage.cfg.in | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 17006c9df9b0..9ace049c9d7c 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -38,6 +38,9 @@ else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)
 	KWB_CFG_CPU = A9
 endif
 
+KWB_REPLACE += LOAD_ADDRESS
+KWB_CFG_LOAD_ADDRESS = $(CONFIG_SPL_TEXT_BASE)
+
 KWB_REPLACE += BOOT_FROM
 ifneq ($(CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI),)
 	KWB_CFG_BOOT_FROM=spi
@@ -67,7 +70,7 @@ KWB_CFG_SEC_FUSE_DUMP = a38x
 endif
 
 quiet_cmd_kwbcfg = KWBCFG  $@
-cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
+cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
 	<$< >$(dir $@)$(@F)
 
 $(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \
diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
index 8e720daf4820..603e8863450c 100644
--- a/arch/arm/mach-mvebu/kwbimage.cfg.in
+++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
@@ -11,5 +11,5 @@ VERSION		1
 # Boot Media configurations
 #@BOOT_FROM
 
-# Binary Header (bin_hdr) with DDR3 training code
-BINARY spl/u-boot-spl.bin
+# Include U-Boot SPL with DDR3 training code into Binary Header
+BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 11/20] arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in kwbimage

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> U-Boot SPL for mvebu platform is not compiled as position independent.
> Therefore it is required to instruct BootROM to load U-Boot SPL at the
> correct address. Loading of kwbimage binary code at specific address can be
> now achieved by the new LOAD_ADDRESS token as part of BINARY command in
> kwbimage config file.
> 
> Update mvebu Makefile to put value of $(CONFIG_SPL_TEXT_BASE) into
> LOAD_ADDRESS token when generating kwbimage.cfg from kwbimage.cfg.in.
> 
> It is required to update regex for sed to find replacement tokens at any
> position on a line in kwbimage config file and not only at the beginning of
> the line. This is because LOAD_ADDRESS is specified at the end of line
> containing the BINARY command.
> 
> It looks like all Armada boards set CONFIG_SPL_TEXT_BASE to value
> 0x40004030 or 0x40000030. Why this value? It is because main kwbimage
> header is at address 0x40004030 or 0x40000000 and it is 32 bytes long.
> After the main header there is the binary header, which consist of 1 byte
> for type, 3 bytes for size, 1 byte for number of arguments, 3 reserved
> bytes and then 4 bytes for each argument. After these arguments comes the
> executable code.
> 
> So arguments start at address 0x40004028 or 0x40000028. Before commit
> e6571f38c943 ("arm: mvebu: Remove dummy BIN header arguments for SPL
> binary") there were two (dummy) arguments, which resulted in load address
> of 0x40004030 or 0x40000030, always. After that commit (which removed dummy
> arguments), load address stayed same due to the 128-bit alignment done by
> mkimage.
> 
> This patch now reflects the dependency between $(CONFIG_SPL_TEXT_BASE),
> load address and dummy kwbimage arguments, and allows the user to adjust
> $(CONFIG_SPL_TEXT_BASE) config option to some other value.
> 
> For unsupported values, when mkimage/kwbimage cannot set chosen load address
> as specified by $(CONFIG_SPL_TEXT_BASE), the build process now fails,
> instead of silently generating non-working kwbimage.
> 
> Removal of this alignment between $(CONFIG_SPL_TEXT_BASE) and LOAD_ADDRESS
> can only be done by compiling U-Boot SPL as position independent. But this
> currently is not possible for 32-bit ARM version of U-Boot SPL.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   arch/arm/mach-mvebu/Makefile        | 5 ++++-
>   arch/arm/mach-mvebu/kwbimage.cfg.in | 4 ++--
>   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 17006c9df9b0..9ace049c9d7c 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -38,6 +38,9 @@ else ifneq ($(CONFIG_ARMADA_375)$(CONFIG_ARMADA_38X)$(CONFIG_ARMADA_39X),)
>   	KWB_CFG_CPU = A9
>   endif
>   
> +KWB_REPLACE += LOAD_ADDRESS
> +KWB_CFG_LOAD_ADDRESS = $(CONFIG_SPL_TEXT_BASE)
> +
>   KWB_REPLACE += BOOT_FROM
>   ifneq ($(CONFIG_MVEBU_SPL_BOOT_DEVICE_SPI),)
>   	KWB_CFG_BOOT_FROM=spi
> @@ -67,7 +70,7 @@ KWB_CFG_SEC_FUSE_DUMP = a38x
>   endif
>   
>   quiet_cmd_kwbcfg = KWBCFG  $@
> -cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/^\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
> +cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
>   	<$< >$(dir $@)$(@F)
>   
>   $(obj)/kwbimage.cfg: $(src)/kwbimage.cfg.in include/autoconf.mk \
> diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
> index 8e720daf4820..603e8863450c 100644
> --- a/arch/arm/mach-mvebu/kwbimage.cfg.in
> +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
> @@ -11,5 +11,5 @@ VERSION		1
>   # Boot Media configurations
>   #@BOOT_FROM
>   
> -# Binary Header (bin_hdr) with DDR3 training code
> -BINARY spl/u-boot-spl.bin
> +# Include U-Boot SPL with DDR3 training code into Binary Header
> +BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 12/20] tools: kwbimage: Enforce 128-bit boundary alignment only for Sheeva CPU

[Pali Rohár]

This alignment is required only for platforms based on Sheeva CPU core
which are A370 and AXP. Now when U-Boot build system correctly propagates
LOAD_ADDRESS there is no need to have enabled 128-bit boundary alignment on
platforms which do not need it. Previously it was required because load
address was implicitly rounded to 128-bit boundary and U-Boot build system
expected it and misused it. Now with explicit setting of LOAD_ADDRESS there
is no guessing for load address anymore.

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwbimage.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index ce053a4a5a78..7c2106006ad7 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1101,8 +1101,10 @@ static size_t image_headersz_v1(int *hasext)
 				return 0;
 			}
 			headersz = e->binary.loadaddr - base_addr;
-		} else {
+		} else if (cpu_sheeva) {
 			headersz = ALIGN(headersz, 16);
+		} else {
+			headersz = ALIGN(headersz, 4);
 		}
 
 		headersz += ALIGN(s.st_size, 4) + sizeof(uint32_t);
@@ -1158,8 +1160,8 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
 	*cur += (binarye->binary.nargs + 1) * sizeof(uint32_t);
 
 	/*
-	 * ARM executable code inside the BIN header on some mvebu platforms
-	 * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
+	 * ARM executable code inside the BIN header on platforms with Sheeva
+	 * CPU (A370 and AXP) must always be aligned with the 128-bit boundary.
 	 * In the case when this code is not position independent (e.g. ARM
 	 * SPL), it must be placed at fixed load and execute address.
 	 * This requirement can be met by inserting dummy arguments into
@@ -1170,8 +1172,10 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
 	offset = *cur - (uint8_t *)main_hdr;
 	if (binarye->binary.loadaddr)
 		add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
-	else
+	else if (cpu_sheeva)
 		add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
+	else
+		add_args = 0;
 	if (add_args) {
 		*(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args);
 		*cur += add_args * sizeof(uint32_t);
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 12/20] tools: kwbimage: Enforce 128-bit boundary alignment only for Sheeva CPU

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> This alignment is required only for platforms based on Sheeva CPU core
> which are A370 and AXP. Now when U-Boot build system correctly propagates
> LOAD_ADDRESS there is no need to have enabled 128-bit boundary alignment on
> platforms which do not need it. Previously it was required because load
> address was implicitly rounded to 128-bit boundary and U-Boot build system
> expected it and misused it. Now with explicit setting of LOAD_ADDRESS there
> is no guessing for load address anymore.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index ce053a4a5a78..7c2106006ad7 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1101,8 +1101,10 @@ static size_t image_headersz_v1(int *hasext)
>   				return 0;
>   			}
>   			headersz = e->binary.loadaddr - base_addr;
> -		} else {
> +		} else if (cpu_sheeva) {
>   			headersz = ALIGN(headersz, 16);
> +		} else {
> +			headersz = ALIGN(headersz, 4);
>   		}
>   
>   		headersz += ALIGN(s.st_size, 4) + sizeof(uint32_t);
> @@ -1158,8 +1160,8 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
>   	*cur += (binarye->binary.nargs + 1) * sizeof(uint32_t);
>   
>   	/*
> -	 * ARM executable code inside the BIN header on some mvebu platforms
> -	 * (e.g. A370, AXP) must always be aligned with the 128-bit boundary.
> +	 * ARM executable code inside the BIN header on platforms with Sheeva
> +	 * CPU (A370 and AXP) must always be aligned with the 128-bit boundary.
>   	 * In the case when this code is not position independent (e.g. ARM
>   	 * SPL), it must be placed at fixed load and execute address.
>   	 * This requirement can be met by inserting dummy arguments into
> @@ -1170,8 +1172,10 @@ static int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
>   	offset = *cur - (uint8_t *)main_hdr;
>   	if (binarye->binary.loadaddr)
>   		add_args = (binarye->binary.loadaddr - base_addr - offset) / sizeof(uint32_t);
> -	else
> +	else if (cpu_sheeva)
>   		add_args = ((16 - offset % 16) % 16) / sizeof(uint32_t);
> +	else
> +		add_args = 0;
>   	if (add_args) {
>   		*(args - 1) = cpu_to_le32(binarye->binary.nargs + add_args);
>   		*cur += add_args * sizeof(uint32_t);
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 13/20] arm: mvebu: Enable BootROM output on A38x

[Pali Rohár]

BootROMs on pre-A38x SoCs enabled its output on UART by default, but A38x'
BootROM has its output on UART disabled by default.

To enable BootROM output on A38x SoC, it is required to set DEBUG flag
(which only enables BootROM output and nothing more) in kwbimage. For UART
images this DEBUG flag is ignored by BootROM.

Enable kwbimage DEBUG flag for all A38x boards.

With this change BootROM prints the following (success) information on UART
before booting U-Boot kwbimage:

  BootROM - 1.73
  Booting from SPI flash

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 arch/arm/mach-mvebu/Makefile        | 7 +++++++
 arch/arm/mach-mvebu/kwbimage.cfg.in | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 9ace049c9d7c..74478a3134e3 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -69,6 +69,13 @@ KWB_REPLACE += SEC_FUSE_DUMP
 KWB_CFG_SEC_FUSE_DUMP = a38x
 endif
 
+ifdef CONFIG_ARMADA_38X
+# BootROM output is by default enabled on pre-A38x and disabled on A38x
+# DEBUG flag on A38x for non-UART boot source only enable BootROM output and nothing more
+KWB_REPLACE += DEBUG
+KWB_CFG_DEBUG = 1
+endif
+
 quiet_cmd_kwbcfg = KWBCFG  $@
 cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
 	<$< >$(dir $@)$(@F)
diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
index 603e8863450c..ccb09975817e 100644
--- a/arch/arm/mach-mvebu/kwbimage.cfg.in
+++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
@@ -11,5 +11,8 @@ VERSION		1
 # Boot Media configurations
 #@BOOT_FROM
 
+# Enable BootROM output via DEBUG flag on SoCs which require it
+#@DEBUG
+
 # Include U-Boot SPL with DDR3 training code into Binary Header
 BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 13/20] arm: mvebu: Enable BootROM output on A38x

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> BootROMs on pre-A38x SoCs enabled its output on UART by default, but A38x'
> BootROM has its output on UART disabled by default.
> 
> To enable BootROM output on A38x SoC, it is required to set DEBUG flag
> (which only enables BootROM output and nothing more) in kwbimage. For UART
> images this DEBUG flag is ignored by BootROM.
> 
> Enable kwbimage DEBUG flag for all A38x boards.
> 
> With this change BootROM prints the following (success) information on UART
> before booting U-Boot kwbimage:
> 
>    BootROM - 1.73
>    Booting from SPI flash
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   arch/arm/mach-mvebu/Makefile        | 7 +++++++
>   arch/arm/mach-mvebu/kwbimage.cfg.in | 3 +++
>   2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> index 9ace049c9d7c..74478a3134e3 100644
> --- a/arch/arm/mach-mvebu/Makefile
> +++ b/arch/arm/mach-mvebu/Makefile
> @@ -69,6 +69,13 @@ KWB_REPLACE += SEC_FUSE_DUMP
>   KWB_CFG_SEC_FUSE_DUMP = a38x
>   endif
>   
> +ifdef CONFIG_ARMADA_38X
> +# BootROM output is by default enabled on pre-A38x and disabled on A38x
> +# DEBUG flag on A38x for non-UART boot source only enable BootROM output and nothing more
> +KWB_REPLACE += DEBUG
> +KWB_CFG_DEBUG = 1
> +endif
> +
>   quiet_cmd_kwbcfg = KWBCFG  $@
>   cmd_kwbcfg = sed -ne '$(foreach V,$(KWB_REPLACE),s/\#@$(V)/$(V) $(KWB_CFG_$(V))/;)p' \
>   	<$< >$(dir $@)$(@F)
> diff --git a/arch/arm/mach-mvebu/kwbimage.cfg.in b/arch/arm/mach-mvebu/kwbimage.cfg.in
> index 603e8863450c..ccb09975817e 100644
> --- a/arch/arm/mach-mvebu/kwbimage.cfg.in
> +++ b/arch/arm/mach-mvebu/kwbimage.cfg.in
> @@ -11,5 +11,8 @@ VERSION		1
>   # Boot Media configurations
>   #@BOOT_FROM
>   
> +# Enable BootROM output via DEBUG flag on SoCs which require it
> +#@DEBUG
> +
>   # Include U-Boot SPL with DDR3 training code into Binary Header
>   BINARY spl/u-boot-spl.bin #@LOAD_ADDRESS
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 14/20] tools: kwbimage: Add missing check for maximal value for DATA_DELAY

[Pali Rohár]

Data delay is stored as 8-bit number in kwbimage structure. Ensure the
given value is at most 255.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 7c2106006ad7..2de8c371c12a 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1659,6 +1659,10 @@ static int image_create_config_parse_oneline(char *line,
 			el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_SDRAM_SETUP;
 		else
 			el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_MS(strtoul(value1, NULL, 10));
+		if (el->regdata_delay > 255) {
+			fprintf(stderr, "Maximal DATA_DELAY is 255\n");
+			return -1;
+		}
 		break;
 	case IMAGE_CFG_BAUDRATE:
 		el->baudrate = strtoul(value1, NULL, 10);
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 14/20] tools: kwbimage: Add missing check for maximal value for DATA_DELAY

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Data delay is stored as 8-bit number in kwbimage structure. Ensure the
> given value is at most 255.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 7c2106006ad7..2de8c371c12a 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1659,6 +1659,10 @@ static int image_create_config_parse_oneline(char *line,
>   			el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_SDRAM_SETUP;
>   		else
>   			el->regdata_delay = REGISTER_SET_HDR_OPT_DELAY_MS(strtoul(value1, NULL, 10));
> +		if (el->regdata_delay > 255) {
> +			fprintf(stderr, "Maximal DATA_DELAY is 255\n");
> +			return -1;
> +		}
>   		break;
>   	case IMAGE_CFG_BAUDRATE:
>   		el->baudrate = strtoul(value1, NULL, 10);
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 15/20] tools: kwbimage: Show binary image offset in mkimage -l, in addition to size

[Pali Rohár]

For debugging purposes it is good to know where the binary image would be
loaded and also it is needed to know if printed size is image size or the
size of header together with image.

Make it unambiguous by showing that printed size is not the size of the
whole header, but only the size of executable code, and print also the
executable offset of this binary image. Load/execute address is the offset
relative to the base address (either 0x40004000 or 0x40000000).

Signed-off-by: Pali Rohár <pali@kernel.org>
---
 tools/kwbimage.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 2de8c371c12a..d1fb67d3db81 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1872,9 +1872,12 @@ static void kwbimage_print_header(const void *ptr)
 
 	for_each_opt_hdr_v1 (ohdr, mhdr) {
 		if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {
-			printf("BIN Hdr Size: ");
+			printf("BIN Img Size: ");
 			genimg_print_size(opt_hdr_v1_size(ohdr) - 12 -
 					  4 * ohdr->data[0]);
+			printf("BIN Img Offs: %08x\n",
+				(unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
+				8 + 4 * ohdr->data[0]);
 		}
 	}
 
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 15/20] tools: kwbimage: Show binary image offset in mkimage -l, in addition to size

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> For debugging purposes it is good to know where the binary image would be
> loaded and also it is needed to know if printed size is image size or the
> size of header together with image.
> 
> Make it unambiguous by showing that printed size is not the size of the
> whole header, but only the size of executable code, and print also the
> executable offset of this binary image. Load/execute address is the offset
> relative to the base address (either 0x40004000 or 0x40000000).
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 2de8c371c12a..d1fb67d3db81 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1872,9 +1872,12 @@ static void kwbimage_print_header(const void *ptr)
>   
>   	for_each_opt_hdr_v1 (ohdr, mhdr) {
>   		if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {
> -			printf("BIN Hdr Size: ");
> +			printf("BIN Img Size: ");
>   			genimg_print_size(opt_hdr_v1_size(ohdr) - 12 -
>   					  4 * ohdr->data[0]);
> +			printf("BIN Img Offs: %08x\n",
> +				(unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) +
> +				8 + 4 * ohdr->data[0]);
>   		}
>   	}
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 16/20] tools: kwbimage: Dump kwbimage config file on '-p -1' option

[Pali Rohár]

To regenerate kwbimage from existing image, it is needed to have kwbimage
config file. Add a new option to generate kwbimage config file from
existing kwbimage when '-p 1' option is given.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 176 insertions(+)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index d1fb67d3db81..de7e9acf7fe5 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -214,6 +214,17 @@ static int image_boot_mode_id(const char *boot_mode_name)
 	return -1;
 }
 
+static const char *image_nand_ecc_mode_name(unsigned int id)
+{
+	int i;
+
+	for (i = 0; nand_ecc_modes[i].name; i++)
+		if (nand_ecc_modes[i].id == id)
+			return nand_ecc_modes[i].name;
+
+	return NULL;
+}
+
 static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
 {
 	int i;
@@ -359,6 +370,29 @@ static uint32_t image_checksum32(void *start, uint32_t len)
 	return csum;
 }
 
+static unsigned int options_to_baudrate(uint8_t options)
+{
+	switch (options & 0x7) {
+	case MAIN_HDR_V1_OPT_BAUD_2400:
+		return 2400;
+	case MAIN_HDR_V1_OPT_BAUD_4800:
+		return 4800;
+	case MAIN_HDR_V1_OPT_BAUD_9600:
+		return 9600;
+	case MAIN_HDR_V1_OPT_BAUD_19200:
+		return 19200;
+	case MAIN_HDR_V1_OPT_BAUD_38400:
+		return 38400;
+	case MAIN_HDR_V1_OPT_BAUD_57600:
+		return 57600;
+	case MAIN_HDR_V1_OPT_BAUD_115200:
+		return 115200;
+	case MAIN_HDR_V1_OPT_BAUD_DEFAULT:
+	default:
+		return 0;
+	}
+}
+
 static uint8_t baudrate_to_option(unsigned int baudrate)
 {
 	switch (baudrate) {
@@ -2088,6 +2122,144 @@ static int kwbimage_generate(struct image_tool_params *params,
 		return 4 + (4 - s.st_size % 4) % 4;
 }
 
+static int kwbimage_generate_config(void *ptr, struct image_tool_params *params)
+{
+	struct main_hdr_v0 *mhdr0 = (struct main_hdr_v0 *)ptr;
+	struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
+	size_t header_size = kwbheader_size(ptr);
+	struct register_set_hdr_v1 *regset_hdr;
+	struct ext_hdr_v0_reg *regdata;
+	struct ext_hdr_v0 *ehdr0;
+	struct opt_hdr_v1 *ohdr;
+	unsigned offset;
+	int cur_idx;
+	int version;
+	FILE *f;
+	int i;
+
+	f = fopen(params->outfile, "w");
+	if (!f) {
+		fprintf(stderr, "Can't open \"%s\": %s\n", params->outfile, strerror(errno));
+		return -1;
+	}
+
+	version = kwbimage_version(ptr);
+
+	if (version != 0)
+		fprintf(f, "VERSION %d\n", version);
+
+	fprintf(f, "BOOT_FROM %s\n", image_boot_mode_name(mhdr->blockid) ?: "<unknown>");
+
+	if (version == 0 && mhdr->blockid == IBR_HDR_NAND_ID)
+		fprintf(f, "NAND_ECC_MODE %s\n", image_nand_ecc_mode_name(mhdr0->nandeccmode));
+
+	if (mhdr->blockid == IBR_HDR_NAND_ID)
+		fprintf(f, "NAND_PAGE_SIZE 0x%x\n", (unsigned)mhdr->nandpagesize);
+
+	if (version != 0 && mhdr->blockid == IBR_HDR_NAND_ID) {
+		fprintf(f, "NAND_BLKSZ 0x%x\n", (unsigned)mhdr->nandblocksize);
+		fprintf(f, "NAND_BADBLK_LOCATION 0x%x\n", (unsigned)mhdr->nandbadblklocation);
+	}
+
+	if (version == 0 && mhdr->blockid == IBR_HDR_SATA_ID)
+		fprintf(f, "SATA_PIO_MODE %u\n", (unsigned)mhdr0->satapiomode);
+
+	/*
+	 * Addresses and sizes which are specified by mkimage command line
+	 * arguments and not in kwbimage config file
+	 */
+
+	if (version != 0)
+		fprintf(f, "#HEADER_SIZE 0x%x\n",
+			((unsigned)mhdr->headersz_msb << 8) | le16_to_cpu(mhdr->headersz_lsb));
+
+	fprintf(f, "#SRC_ADDRESS 0x%x\n", le32_to_cpu(mhdr->srcaddr));
+	fprintf(f, "#BLOCK_SIZE 0x%x\n", le32_to_cpu(mhdr->blocksize));
+	fprintf(f, "#DEST_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->destaddr));
+	fprintf(f, "#EXEC_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->execaddr));
+
+	if (version != 0) {
+		if (options_to_baudrate(mhdr->options))
+			fprintf(f, "BAUDRATE %u\n", options_to_baudrate(mhdr->options));
+		if (options_to_baudrate(mhdr->options) ||
+		    ((mhdr->options >> 3) & 0x3) || ((mhdr->options >> 5) & 0x7)) {
+			fprintf(f, "UART_PORT %u\n", (unsigned)((mhdr->options >> 3) & 0x3));
+			fprintf(f, "UART_MPP 0x%x\n", (unsigned)((mhdr->options >> 5) & 0x7));
+		}
+		if (mhdr->flags & 0x1)
+			fprintf(f, "DEBUG 1\n");
+	}
+
+	cur_idx = 1;
+	for_each_opt_hdr_v1(ohdr, ptr) {
+		if (ohdr->headertype == OPT_HDR_V1_SECURE_TYPE) {
+			fprintf(f, "#SECURE_HEADER\n");
+		} else if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {
+			fprintf(f, "BINARY binary%d.bin", cur_idx);
+			for (i = 0; i < ohdr->data[0]; i++)
+				fprintf(f, " 0x%x", le32_to_cpu(((uint32_t *)ohdr->data)[i + 1]));
+			offset = (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) + 8 + 4 * ohdr->data[0];
+			fprintf(f, " LOAD_ADDRESS 0x%08x\n", 0x40000000 + offset);
+			fprintf(f, " # for CPU SHEEVA: LOAD_ADDRESS 0x%08x\n", 0x40004000 + offset);
+			cur_idx++;
+		} else if (ohdr->headertype == OPT_HDR_V1_REGISTER_TYPE) {
+			regset_hdr = (struct register_set_hdr_v1 *)ohdr;
+			for (i = 0;
+			     i < opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) -
+				 sizeof(regset_hdr->data[0].last_entry);
+			     i++)
+				fprintf(f, "DATA 0x%08x 0x%08x\n",
+					le32_to_cpu(regset_hdr->data[i].entry.address),
+					le32_to_cpu(regset_hdr->data[i].entry.value));
+			if (opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) >=
+			    sizeof(regset_hdr->data[0].last_entry)) {
+				if (regset_hdr->data[0].last_entry.delay)
+					fprintf(f, "DATA_DELAY %u\n",
+						(unsigned)regset_hdr->data[0].last_entry.delay);
+				else
+					fprintf(f, "DATA_DELAY SDRAM_SETUP\n");
+			}
+		}
+	}
+
+	if (version == 0 && mhdr0->ext) {
+		ehdr0 = (struct ext_hdr_v0 *)(mhdr0 + 1);
+		if (ehdr0->offset) {
+			for (regdata = (struct ext_hdr_v0_reg *)((uint8_t *)ptr + ehdr0->offset);
+			     (uint8_t *)regdata < (uint8_t *)ptr + header_size && regdata->raddr &&
+			     regdata->rdata;
+			     regdata++)
+				fprintf(f, "DATA 0x%08x 0x%08x\n", le32_to_cpu(regdata->raddr),
+					le32_to_cpu(regdata->rdata));
+		}
+	}
+
+	if (version == 0 && le16_to_cpu(mhdr0->ddrinitdelay))
+		fprintf(f, "DDR_INIT_DELAY %u\n", (unsigned)le16_to_cpu(mhdr0->ddrinitdelay));
+
+	/* Undocumented reserved fields */
+
+	if (version == 0 && (mhdr0->rsvd1[0] || mhdr0->rsvd1[1] || mhdr0->rsvd1[2]))
+		fprintf(f, "#RSVD1 0x%x 0x%x 0x%x\n", (unsigned)mhdr0->rsvd1[0],
+			(unsigned)mhdr0->rsvd1[1], (unsigned)mhdr0->rsvd1[2]);
+
+	if (version == 0 && mhdr0->rsvd3)
+		fprintf(f, "#RSVD3 0x%x\n", (unsigned)mhdr0->rsvd3);
+
+	if (version == 0 && le16_to_cpu(mhdr0->rsvd2))
+		fprintf(f, "#RSVD2 0x%x\n", (unsigned)le16_to_cpu(mhdr0->rsvd2));
+
+	if (version != 0 && mhdr->reserved4)
+		fprintf(f, "#RESERVED4 0x%x\n", (unsigned)mhdr->reserved4);
+
+	if (version != 0 && mhdr->reserved5)
+		fprintf(f, "#RESERVED5 0x%x\n", (unsigned)le16_to_cpu(mhdr->reserved5));
+
+	fclose(f);
+
+	return 0;
+}
+
 static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params)
 {
 	struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
@@ -2099,6 +2271,10 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
 	ulong image;
 	ulong size;
 
+	/* Generate kwbimage config file when '-p -1' is specified */
+	if (idx == -1)
+		return kwbimage_generate_config(ptr, params);
+
 	for_each_opt_hdr_v1 (ohdr, ptr) {
 		if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
 			continue;
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 16/20] tools: kwbimage: Dump kwbimage config file on '-p -1' option

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> To regenerate kwbimage from existing image, it is needed to have kwbimage
> config file. Add a new option to generate kwbimage config file from
> existing kwbimage when '-p 1' option is given.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 176 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 176 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index d1fb67d3db81..de7e9acf7fe5 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -214,6 +214,17 @@ static int image_boot_mode_id(const char *boot_mode_name)
>   	return -1;
>   }
>   
> +static const char *image_nand_ecc_mode_name(unsigned int id)
> +{
> +	int i;
> +
> +	for (i = 0; nand_ecc_modes[i].name; i++)
> +		if (nand_ecc_modes[i].id == id)
> +			return nand_ecc_modes[i].name;
> +
> +	return NULL;
> +}
> +
>   static int image_nand_ecc_mode_id(const char *nand_ecc_mode_name)
>   {
>   	int i;
> @@ -359,6 +370,29 @@ static uint32_t image_checksum32(void *start, uint32_t len)
>   	return csum;
>   }
>   
> +static unsigned int options_to_baudrate(uint8_t options)
> +{
> +	switch (options & 0x7) {
> +	case MAIN_HDR_V1_OPT_BAUD_2400:
> +		return 2400;
> +	case MAIN_HDR_V1_OPT_BAUD_4800:
> +		return 4800;
> +	case MAIN_HDR_V1_OPT_BAUD_9600:
> +		return 9600;
> +	case MAIN_HDR_V1_OPT_BAUD_19200:
> +		return 19200;
> +	case MAIN_HDR_V1_OPT_BAUD_38400:
> +		return 38400;
> +	case MAIN_HDR_V1_OPT_BAUD_57600:
> +		return 57600;
> +	case MAIN_HDR_V1_OPT_BAUD_115200:
> +		return 115200;
> +	case MAIN_HDR_V1_OPT_BAUD_DEFAULT:
> +	default:
> +		return 0;
> +	}
> +}
> +
>   static uint8_t baudrate_to_option(unsigned int baudrate)
>   {
>   	switch (baudrate) {
> @@ -2088,6 +2122,144 @@ static int kwbimage_generate(struct image_tool_params *params,
>   		return 4 + (4 - s.st_size % 4) % 4;
>   }
>   
> +static int kwbimage_generate_config(void *ptr, struct image_tool_params *params)
> +{
> +	struct main_hdr_v0 *mhdr0 = (struct main_hdr_v0 *)ptr;
> +	struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
> +	size_t header_size = kwbheader_size(ptr);
> +	struct register_set_hdr_v1 *regset_hdr;
> +	struct ext_hdr_v0_reg *regdata;
> +	struct ext_hdr_v0 *ehdr0;
> +	struct opt_hdr_v1 *ohdr;
> +	unsigned offset;
> +	int cur_idx;
> +	int version;
> +	FILE *f;
> +	int i;
> +
> +	f = fopen(params->outfile, "w");
> +	if (!f) {
> +		fprintf(stderr, "Can't open \"%s\": %s\n", params->outfile, strerror(errno));
> +		return -1;
> +	}
> +
> +	version = kwbimage_version(ptr);
> +
> +	if (version != 0)
> +		fprintf(f, "VERSION %d\n", version);
> +
> +	fprintf(f, "BOOT_FROM %s\n", image_boot_mode_name(mhdr->blockid) ?: "<unknown>");
> +
> +	if (version == 0 && mhdr->blockid == IBR_HDR_NAND_ID)
> +		fprintf(f, "NAND_ECC_MODE %s\n", image_nand_ecc_mode_name(mhdr0->nandeccmode));
> +
> +	if (mhdr->blockid == IBR_HDR_NAND_ID)
> +		fprintf(f, "NAND_PAGE_SIZE 0x%x\n", (unsigned)mhdr->nandpagesize);
> +
> +	if (version != 0 && mhdr->blockid == IBR_HDR_NAND_ID) {
> +		fprintf(f, "NAND_BLKSZ 0x%x\n", (unsigned)mhdr->nandblocksize);
> +		fprintf(f, "NAND_BADBLK_LOCATION 0x%x\n", (unsigned)mhdr->nandbadblklocation);
> +	}
> +
> +	if (version == 0 && mhdr->blockid == IBR_HDR_SATA_ID)
> +		fprintf(f, "SATA_PIO_MODE %u\n", (unsigned)mhdr0->satapiomode);
> +
> +	/*
> +	 * Addresses and sizes which are specified by mkimage command line
> +	 * arguments and not in kwbimage config file
> +	 */
> +
> +	if (version != 0)
> +		fprintf(f, "#HEADER_SIZE 0x%x\n",
> +			((unsigned)mhdr->headersz_msb << 8) | le16_to_cpu(mhdr->headersz_lsb));
> +
> +	fprintf(f, "#SRC_ADDRESS 0x%x\n", le32_to_cpu(mhdr->srcaddr));
> +	fprintf(f, "#BLOCK_SIZE 0x%x\n", le32_to_cpu(mhdr->blocksize));
> +	fprintf(f, "#DEST_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->destaddr));
> +	fprintf(f, "#EXEC_ADDRESS 0x%08x\n", le32_to_cpu(mhdr->execaddr));
> +
> +	if (version != 0) {
> +		if (options_to_baudrate(mhdr->options))
> +			fprintf(f, "BAUDRATE %u\n", options_to_baudrate(mhdr->options));
> +		if (options_to_baudrate(mhdr->options) ||
> +		    ((mhdr->options >> 3) & 0x3) || ((mhdr->options >> 5) & 0x7)) {
> +			fprintf(f, "UART_PORT %u\n", (unsigned)((mhdr->options >> 3) & 0x3));
> +			fprintf(f, "UART_MPP 0x%x\n", (unsigned)((mhdr->options >> 5) & 0x7));
> +		}
> +		if (mhdr->flags & 0x1)
> +			fprintf(f, "DEBUG 1\n");
> +	}
> +
> +	cur_idx = 1;
> +	for_each_opt_hdr_v1(ohdr, ptr) {
> +		if (ohdr->headertype == OPT_HDR_V1_SECURE_TYPE) {
> +			fprintf(f, "#SECURE_HEADER\n");
> +		} else if (ohdr->headertype == OPT_HDR_V1_BINARY_TYPE) {
> +			fprintf(f, "BINARY binary%d.bin", cur_idx);
> +			for (i = 0; i < ohdr->data[0]; i++)
> +				fprintf(f, " 0x%x", le32_to_cpu(((uint32_t *)ohdr->data)[i + 1]));
> +			offset = (unsigned)((uint8_t *)ohdr - (uint8_t *)mhdr) + 8 + 4 * ohdr->data[0];
> +			fprintf(f, " LOAD_ADDRESS 0x%08x\n", 0x40000000 + offset);
> +			fprintf(f, " # for CPU SHEEVA: LOAD_ADDRESS 0x%08x\n", 0x40004000 + offset);
> +			cur_idx++;
> +		} else if (ohdr->headertype == OPT_HDR_V1_REGISTER_TYPE) {
> +			regset_hdr = (struct register_set_hdr_v1 *)ohdr;
> +			for (i = 0;
> +			     i < opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) -
> +				 sizeof(regset_hdr->data[0].last_entry);
> +			     i++)
> +				fprintf(f, "DATA 0x%08x 0x%08x\n",
> +					le32_to_cpu(regset_hdr->data[i].entry.address),
> +					le32_to_cpu(regset_hdr->data[i].entry.value));
> +			if (opt_hdr_v1_size(ohdr) - sizeof(struct opt_hdr_v1) >=
> +			    sizeof(regset_hdr->data[0].last_entry)) {
> +				if (regset_hdr->data[0].last_entry.delay)
> +					fprintf(f, "DATA_DELAY %u\n",
> +						(unsigned)regset_hdr->data[0].last_entry.delay);
> +				else
> +					fprintf(f, "DATA_DELAY SDRAM_SETUP\n");
> +			}
> +		}
> +	}
> +
> +	if (version == 0 && mhdr0->ext) {
> +		ehdr0 = (struct ext_hdr_v0 *)(mhdr0 + 1);
> +		if (ehdr0->offset) {
> +			for (regdata = (struct ext_hdr_v0_reg *)((uint8_t *)ptr + ehdr0->offset);
> +			     (uint8_t *)regdata < (uint8_t *)ptr + header_size && regdata->raddr &&
> +			     regdata->rdata;
> +			     regdata++)
> +				fprintf(f, "DATA 0x%08x 0x%08x\n", le32_to_cpu(regdata->raddr),
> +					le32_to_cpu(regdata->rdata));
> +		}
> +	}
> +
> +	if (version == 0 && le16_to_cpu(mhdr0->ddrinitdelay))
> +		fprintf(f, "DDR_INIT_DELAY %u\n", (unsigned)le16_to_cpu(mhdr0->ddrinitdelay));
> +
> +	/* Undocumented reserved fields */
> +
> +	if (version == 0 && (mhdr0->rsvd1[0] || mhdr0->rsvd1[1] || mhdr0->rsvd1[2]))
> +		fprintf(f, "#RSVD1 0x%x 0x%x 0x%x\n", (unsigned)mhdr0->rsvd1[0],
> +			(unsigned)mhdr0->rsvd1[1], (unsigned)mhdr0->rsvd1[2]);
> +
> +	if (version == 0 && mhdr0->rsvd3)
> +		fprintf(f, "#RSVD3 0x%x\n", (unsigned)mhdr0->rsvd3);
> +
> +	if (version == 0 && le16_to_cpu(mhdr0->rsvd2))
> +		fprintf(f, "#RSVD2 0x%x\n", (unsigned)le16_to_cpu(mhdr0->rsvd2));
> +
> +	if (version != 0 && mhdr->reserved4)
> +		fprintf(f, "#RESERVED4 0x%x\n", (unsigned)mhdr->reserved4);
> +
> +	if (version != 0 && mhdr->reserved5)
> +		fprintf(f, "#RESERVED5 0x%x\n", (unsigned)le16_to_cpu(mhdr->reserved5));
> +
> +	fclose(f);
> +
> +	return 0;
> +}
> +
>   static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params)
>   {
>   	struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
> @@ -2099,6 +2271,10 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
>   	ulong image;
>   	ulong size;
>   
> +	/* Generate kwbimage config file when '-p -1' is specified */
> +	if (idx == -1)
> +		return kwbimage_generate_config(ptr, params);
> +
>   	for_each_opt_hdr_v1 (ohdr, ptr) {
>   		if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
>   			continue;
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 17/20] tools: kwbimage: Do not cast const pointers to non-const pointers

[Pali Rohár]

Avoid casting const to non-const.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/kwbimage.h b/tools/kwbimage.h
index 8d37357e5abd..c000cba4b8d1 100644
--- a/tools/kwbimage.h
+++ b/tools/kwbimage.h
@@ -235,11 +235,11 @@ static inline int opt_hdr_v1_valid_size(const struct opt_hdr_v1 *ohdr,
 {
 	uint32_t ohdr_size;
 
-	if ((void *)(ohdr + 1) > mhdr_end)
+	if ((const void *)(ohdr + 1) > mhdr_end)
 		return 0;
 
 	ohdr_size = opt_hdr_v1_size(ohdr);
-	if (ohdr_size < 8 || (void *)((uint8_t *)ohdr + ohdr_size) > mhdr_end)
+	if (ohdr_size < 8 || (const void *)((const uint8_t *)ohdr + ohdr_size) > mhdr_end)
 		return 0;
 
 	return 1;
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 17/20] tools: kwbimage: Do not cast const pointers to non-const pointers

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Avoid casting const to non-const.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/kwbimage.h b/tools/kwbimage.h
> index 8d37357e5abd..c000cba4b8d1 100644
> --- a/tools/kwbimage.h
> +++ b/tools/kwbimage.h
> @@ -235,11 +235,11 @@ static inline int opt_hdr_v1_valid_size(const struct opt_hdr_v1 *ohdr,
>   {
>   	uint32_t ohdr_size;
>   
> -	if ((void *)(ohdr + 1) > mhdr_end)
> +	if ((const void *)(ohdr + 1) > mhdr_end)
>   		return 0;
>   
>   	ohdr_size = opt_hdr_v1_size(ohdr);
> -	if (ohdr_size < 8 || (void *)((uint8_t *)ohdr + ohdr_size) > mhdr_end)
> +	if (ohdr_size < 8 || (const void *)((const uint8_t *)ohdr + ohdr_size) > mhdr_end)
>   		return 0;
>   
>   	return 1;
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 18/20] tools: kwbimage/kwboot: Check ext field for non-zero value

[Pali Rohár]

Despite the official specification, BootROM does not look at the lowest bit
of ext field but rather checks if ext field is non-zero.

Moreover original Marvell doimage tool puts into the mhdr->ext field the
number of extended headers, so basically it sets ext filed to non-zero
value if some extended header is present.

Fix U-Boot dumpimage and kwboot tools to parse correctly also kwbimage
files created by Marvell doimage tool, in the same way as the BootROM is
doing it when booting these images.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 2 +-
 tools/kwbimage.h | 6 +++---
 tools/kwboot.c   | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index de7e9acf7fe5..92d163b6050e 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1948,7 +1948,7 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
 	if (kwbimage_version(ptr) == 0) {
 		struct main_hdr_v0 *mhdr = (struct main_hdr_v0 *)ptr;
 
-		if (mhdr->ext & 0x1) {
+		if (mhdr->ext) {
 			struct ext_hdr_v0 *ext_hdr = (void *)(mhdr + 1);
 
 			csum = image_checksum8(ext_hdr, sizeof(*ext_hdr) - 1);
diff --git a/tools/kwbimage.h b/tools/kwbimage.h
index c000cba4b8d1..9ebc7d72d363 100644
--- a/tools/kwbimage.h
+++ b/tools/kwbimage.h
@@ -208,7 +208,7 @@ static inline size_t kwbheader_size(const void *header)
 		const struct main_hdr_v0 *hdr = header;
 
 		return sizeof(*hdr) +
-		       (hdr->ext & 0x1) ? sizeof(struct ext_hdr_v0) : 0;
+		       hdr->ext ? sizeof(struct ext_hdr_v0) : 0;
 	} else {
 		const struct main_hdr_v1 *hdr = header;
 
@@ -252,7 +252,7 @@ static inline struct opt_hdr_v1 *opt_hdr_v1_first(void *img) {
 		return NULL;
 
 	mhdr = img;
-	if (mhdr->ext & 0x1)
+	if (mhdr->ext)
 		return (struct opt_hdr_v1 *)(mhdr + 1);
 	else
 		return NULL;
@@ -272,7 +272,7 @@ static inline struct opt_hdr_v1 *_opt_hdr_v1_next(struct opt_hdr_v1 *cur)
 
 static inline struct opt_hdr_v1 *opt_hdr_v1_next(struct opt_hdr_v1 *cur)
 {
-	if (*opt_hdr_v1_ext(cur) & 0x1)
+	if (*opt_hdr_v1_ext(cur))
 		return _opt_hdr_v1_next(cur);
 	else
 		return NULL;
diff --git a/tools/kwboot.c b/tools/kwboot.c
index d22e6ea96a5c..c3d8ab654417 100644
--- a/tools/kwboot.c
+++ b/tools/kwboot.c
@@ -1398,7 +1398,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
 	uint32_t ohdrsz;
 	uint8_t *prev_ext;
 
-	if (hdr->ext & 0x1) {
+	if (hdr->ext) {
 		for_each_opt_hdr_v1 (ohdr, img)
 			if (opt_hdr_v1_next(ohdr) == NULL)
 				break;
@@ -1422,7 +1422,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
 	ohdrsz = sizeof(*ohdr) + 4 + 4 * num_args + binsz + 4;
 	kwboot_img_grow_hdr(hdr, size, ohdrsz);
 
-	*prev_ext |= 1;
+	*prev_ext = 1;
 
 	ohdr->headertype = OPT_HDR_V1_BINARY_TYPE;
 	ohdr->headersz_msb = ohdrsz >> 16;
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 18/20] tools: kwbimage/kwboot: Check ext field for non-zero value

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Despite the official specification, BootROM does not look at the lowest bit
> of ext field but rather checks if ext field is non-zero.
> 
> Moreover original Marvell doimage tool puts into the mhdr->ext field the
> number of extended headers, so basically it sets ext filed to non-zero
> value if some extended header is present.
> 
> Fix U-Boot dumpimage and kwboot tools to parse correctly also kwbimage
> files created by Marvell doimage tool, in the same way as the BootROM is
> doing it when booting these images.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 2 +-
>   tools/kwbimage.h | 6 +++---
>   tools/kwboot.c   | 4 ++--
>   3 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index de7e9acf7fe5..92d163b6050e 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1948,7 +1948,7 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
>   	if (kwbimage_version(ptr) == 0) {
>   		struct main_hdr_v0 *mhdr = (struct main_hdr_v0 *)ptr;
>   
> -		if (mhdr->ext & 0x1) {
> +		if (mhdr->ext) {
>   			struct ext_hdr_v0 *ext_hdr = (void *)(mhdr + 1);
>   
>   			csum = image_checksum8(ext_hdr, sizeof(*ext_hdr) - 1);
> diff --git a/tools/kwbimage.h b/tools/kwbimage.h
> index c000cba4b8d1..9ebc7d72d363 100644
> --- a/tools/kwbimage.h
> +++ b/tools/kwbimage.h
> @@ -208,7 +208,7 @@ static inline size_t kwbheader_size(const void *header)
>   		const struct main_hdr_v0 *hdr = header;
>   
>   		return sizeof(*hdr) +
> -		       (hdr->ext & 0x1) ? sizeof(struct ext_hdr_v0) : 0;
> +		       hdr->ext ? sizeof(struct ext_hdr_v0) : 0;
>   	} else {
>   		const struct main_hdr_v1 *hdr = header;
>   
> @@ -252,7 +252,7 @@ static inline struct opt_hdr_v1 *opt_hdr_v1_first(void *img) {
>   		return NULL;
>   
>   	mhdr = img;
> -	if (mhdr->ext & 0x1)
> +	if (mhdr->ext)
>   		return (struct opt_hdr_v1 *)(mhdr + 1);
>   	else
>   		return NULL;
> @@ -272,7 +272,7 @@ static inline struct opt_hdr_v1 *_opt_hdr_v1_next(struct opt_hdr_v1 *cur)
>   
>   static inline struct opt_hdr_v1 *opt_hdr_v1_next(struct opt_hdr_v1 *cur)
>   {
> -	if (*opt_hdr_v1_ext(cur) & 0x1)
> +	if (*opt_hdr_v1_ext(cur))
>   		return _opt_hdr_v1_next(cur);
>   	else
>   		return NULL;
> diff --git a/tools/kwboot.c b/tools/kwboot.c
> index d22e6ea96a5c..c3d8ab654417 100644
> --- a/tools/kwboot.c
> +++ b/tools/kwboot.c
> @@ -1398,7 +1398,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
>   	uint32_t ohdrsz;
>   	uint8_t *prev_ext;
>   
> -	if (hdr->ext & 0x1) {
> +	if (hdr->ext) {
>   		for_each_opt_hdr_v1 (ohdr, img)
>   			if (opt_hdr_v1_next(ohdr) == NULL)
>   				break;
> @@ -1422,7 +1422,7 @@ kwboot_add_bin_ohdr_v1(void *img, size_t *size, uint32_t binsz)
>   	ohdrsz = sizeof(*ohdr) + 4 + 4 * num_args + binsz + 4;
>   	kwboot_img_grow_hdr(hdr, size, ohdrsz);
>   
> -	*prev_ext |= 1;
> +	*prev_ext = 1;
>   
>   	ohdr->headertype = OPT_HDR_V1_BINARY_TYPE;
>   	ohdr->headersz_msb = ohdrsz >> 16;
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 19/20] tools: kwbimage: Extract main data image without -p arg for dumpimage

[Pali Rohár]

When there is no -p argument for dumpimage tool specified, extract the main
data image from kwbimage file. This makes dumpimage consistent with other
image formats.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 67 +++++++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 27 deletions(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 92d163b6050e..d159087d9dd6 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -2266,7 +2266,7 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
 	size_t header_size = kwbheader_size(ptr);
 	struct opt_hdr_v1 *ohdr;
 	int idx = params->pflag;
-	int cur_idx = 0;
+	int cur_idx;
 	uint32_t offset;
 	ulong image;
 	ulong size;
@@ -2275,41 +2275,54 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
 	if (idx == -1)
 		return kwbimage_generate_config(ptr, params);
 
-	for_each_opt_hdr_v1 (ohdr, ptr) {
-		if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
-			continue;
+	image = 0;
+	size = 0;
+
+	if (idx == 0) {
+		/* Extract data image when -p is not specified or when '-p 0' is specified */
+		offset = le32_to_cpu(mhdr->srcaddr);
 
-		if (idx == cur_idx) {
-			image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
-			size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
-			goto extract;
+		if (mhdr->blockid == IBR_HDR_SATA_ID) {
+			offset -= 1;
+			offset *= 512;
 		}
 
-		++cur_idx;
-	}
+		if (mhdr->blockid == IBR_HDR_SDIO_ID)
+			offset *= 512;
 
-	if (idx != cur_idx) {
-		printf("Image %d is not present\n", idx);
-		return -1;
-	}
-
-	offset = le32_to_cpu(mhdr->srcaddr);
+		if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
+			offset = header_size;
 
-	if (mhdr->blockid == IBR_HDR_SATA_ID) {
-		offset -= 1;
-		offset *= 512;
-	}
+		image = (ulong)((uint8_t *)ptr + offset);
+		size = le32_to_cpu(mhdr->blocksize) - 4;
+	} else {
+		/* Extract N-th binary header executabe image when other '-p N' is specified */
+		cur_idx = 1;
+		for_each_opt_hdr_v1(ohdr, ptr) {
+			if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
+				continue;
 
-	if (mhdr->blockid == IBR_HDR_SDIO_ID)
-		offset *= 512;
+			if (idx == cur_idx) {
+				image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
+				size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
+				break;
+			}
 
-	if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
-		offset = header_size;
+			++cur_idx;
+		}
 
-	image = (ulong)((uint8_t *)ptr + offset);
-	size = le32_to_cpu(mhdr->blocksize) - 4;
+		if (!image) {
+			fprintf(stderr, "Argument -p %d is invalid\n", idx);
+			fprintf(stderr, "Available subimages:\n");
+			fprintf(stderr, " -p -1  - kwbimage config file\n");
+			fprintf(stderr, " -p 0   - data image\n");
+			if (cur_idx - 1 > 0)
+				fprintf(stderr, " -p N   - Nth binary header image (totally: %d)\n",
+					cur_idx - 1);
+			return -1;
+		}
+	}
 
-extract:
 	return imagetool_save_subimage(params->outfile, image, size);
 }
 
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 19/20] tools: kwbimage: Extract main data image without -p arg for dumpimage

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> When there is no -p argument for dumpimage tool specified, extract the main
> data image from kwbimage file. This makes dumpimage consistent with other
> image formats.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 67 +++++++++++++++++++++++++++++-------------------
>   1 file changed, 40 insertions(+), 27 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 92d163b6050e..d159087d9dd6 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -2266,7 +2266,7 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
>   	size_t header_size = kwbheader_size(ptr);
>   	struct opt_hdr_v1 *ohdr;
>   	int idx = params->pflag;
> -	int cur_idx = 0;
> +	int cur_idx;
>   	uint32_t offset;
>   	ulong image;
>   	ulong size;
> @@ -2275,41 +2275,54 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
>   	if (idx == -1)
>   		return kwbimage_generate_config(ptr, params);
>   
> -	for_each_opt_hdr_v1 (ohdr, ptr) {
> -		if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
> -			continue;
> +	image = 0;
> +	size = 0;
> +
> +	if (idx == 0) {
> +		/* Extract data image when -p is not specified or when '-p 0' is specified */
> +		offset = le32_to_cpu(mhdr->srcaddr);
>   
> -		if (idx == cur_idx) {
> -			image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
> -			size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
> -			goto extract;
> +		if (mhdr->blockid == IBR_HDR_SATA_ID) {
> +			offset -= 1;
> +			offset *= 512;
>   		}
>   
> -		++cur_idx;
> -	}
> +		if (mhdr->blockid == IBR_HDR_SDIO_ID)
> +			offset *= 512;
>   
> -	if (idx != cur_idx) {
> -		printf("Image %d is not present\n", idx);
> -		return -1;
> -	}
> -
> -	offset = le32_to_cpu(mhdr->srcaddr);
> +		if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
> +			offset = header_size;
>   
> -	if (mhdr->blockid == IBR_HDR_SATA_ID) {
> -		offset -= 1;
> -		offset *= 512;
> -	}
> +		image = (ulong)((uint8_t *)ptr + offset);
> +		size = le32_to_cpu(mhdr->blocksize) - 4;
> +	} else {
> +		/* Extract N-th binary header executabe image when other '-p N' is specified */
> +		cur_idx = 1;
> +		for_each_opt_hdr_v1(ohdr, ptr) {
> +			if (ohdr->headertype != OPT_HDR_V1_BINARY_TYPE)
> +				continue;
>   
> -	if (mhdr->blockid == IBR_HDR_SDIO_ID)
> -		offset *= 512;
> +			if (idx == cur_idx) {
> +				image = (ulong)&ohdr->data[4 + 4 * ohdr->data[0]];
> +				size = opt_hdr_v1_size(ohdr) - 12 - 4 * ohdr->data[0];
> +				break;
> +			}
>   
> -	if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
> -		offset = header_size;
> +			++cur_idx;
> +		}
>   
> -	image = (ulong)((uint8_t *)ptr + offset);
> -	size = le32_to_cpu(mhdr->blocksize) - 4;
> +		if (!image) {
> +			fprintf(stderr, "Argument -p %d is invalid\n", idx);
> +			fprintf(stderr, "Available subimages:\n");
> +			fprintf(stderr, " -p -1  - kwbimage config file\n");
> +			fprintf(stderr, " -p 0   - data image\n");
> +			if (cur_idx - 1 > 0)
> +				fprintf(stderr, " -p N   - Nth binary header image (totally: %d)\n",
> +					cur_idx - 1);
> +			return -1;
> +		}
> +	}
>   
> -extract:
>   	return imagetool_save_subimage(params->outfile, image, size);
>   }
>   
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2 20/20] tools: kwbimage: Fix mkimage/dumpimage -l argument

[Pali Rohár]

Do not check for kwbimage configuration file when just showing information
about existing kwbimage file.

The check for kwbimage configuration file is required only when creating
kwbimage, not when showing information about image or when extracting data
from image.

With this change, it is possible to call mkimage -l and dumpimage -l also
for existing kwbimage file.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <marek.behun@nic.cz>
---
 tools/kwbimage.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index d159087d9dd6..9b63ce80ff4e 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -2331,7 +2331,8 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
  */
 static int kwbimage_check_params(struct image_tool_params *params)
 {
-	if (!params->iflag && (!params->imagename || !strlen(params->imagename))) {
+	if (!params->lflag && !params->iflag &&
+	    (!params->imagename || !strlen(params->imagename))) {
 		char *msg = "Configuration file for kwbimage creation omitted";
 
 		fprintf(stderr, "Error:%s - %s\n", params->cmdname, msg);
-- 
2.20.1

Re: [PATCH u-boot-marvell v2 20/20] tools: kwbimage: Fix mkimage/dumpimage -l argument

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> Do not check for kwbimage configuration file when just showing information
> about existing kwbimage file.
> 
> The check for kwbimage configuration file is required only when creating
> kwbimage, not when showing information about image or when extracting data
> from image.
> 
> With this change, it is possible to call mkimage -l and dumpimage -l also
> for existing kwbimage file.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reviewed-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index d159087d9dd6..9b63ce80ff4e 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -2331,7 +2331,8 @@ static int kwbimage_extract_subimage(void *ptr, struct image_tool_params *params
>    */
>   static int kwbimage_check_params(struct image_tool_params *params)
>   {
> -	if (!params->iflag && (!params->imagename || !strlen(params->imagename))) {
> +	if (!params->lflag && !params->iflag &&
> +	    (!params->imagename || !strlen(params->imagename))) {
>   		char *msg = "Configuration file for kwbimage creation omitted";
>   
>   		fprintf(stderr, "Error:%s - %s\n", params->cmdname, msg);
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell v2 00/20] tools: kwbimage: Load address fixes

[Stefan Roese]

On 1/12/22 18:20, Pali Rohár wrote:
> This patch series fixes generating images in kwbimage format, main fix
> is setting correct load address of U-Boot SPL. Also it adds support for
> generating kwbimage config file from existing kwbimage file via
> dumpimage tool.
> 
> Changes in v2:
> * Fix base address for Sheeva CPUs (A370, AXP), it is 0x40004000
> * Fix information about mapped area of load address (it is L2, not CESA)
> * Add new kwbimage config option CPU
> 
> Pali Rohár (20):
>    tools: kwbimage: Mark all local functions as static
>    tools: kwbimage: Deduplicate v1 regtype header finishing
>    tools: kwbimage: Fix generating image with multiple DATA_DELAY
>      commands
>    tools: kwbimage: Preserve order of BINARY, DATA and DATA_DELAY
>      commands
>    arm: mvebu: Generate kwbimage.cfg with $(call cmd, ...)
>    tools: kwbimage: Add support for specifying CPU core
>    tools: kwbimage: Add support for specifying LOAD_ADDRESS for BINARY
>      command
>    tools: kwbimage: Check the return value of image_headersz_v1()
>    tools: kwbimage: Check for maximal kwbimage header size
>    arm: mvebu: Set CPU for U-Boot SPL binary in kwbimage
>    arm: mvebu: Correctly set LOAD_ADDRESS for U-Boot SPL binary in
>      kwbimage
>    tools: kwbimage: Enforce 128-bit boundary alignment only for Sheeva
>      CPU
>    arm: mvebu: Enable BootROM output on A38x
>    tools: kwbimage: Add missing check for maximal value for DATA_DELAY
>    tools: kwbimage: Show binary image offset in mkimage -l, in addition
>      to size
>    tools: kwbimage: Dump kwbimage config file on '-p -1' option
>    tools: kwbimage: Do not cast const pointers to non-const pointers
>    tools: kwbimage/kwboot: Check ext field for non-zero value
>    tools: kwbimage: Extract main data image without -p arg for dumpimage
>    tools: kwbimage: Fix mkimage/dumpimage -l argument
> 
>   arch/arm/mach-mvebu/Makefile        |  25 +-
>   arch/arm/mach-mvebu/kwbimage.cfg.in |  10 +-
>   tools/kwbimage.c                    | 549 +++++++++++++++++++++++-----
>   tools/kwbimage.h                    |  10 +-
>   tools/kwboot.c                      |   4 +-
>   5 files changed, 486 insertions(+), 112 deletions(-)
> 

Applied to u-boot-marvell/master

Thanks,
Stefan