[PATCH u-boot-marvell 00/11] Another kwbimage series

[PATCH u-boot-marvell 00/11] Another kwbimage series

[Marek Behún]

From: Marek Behún <marek.behun@nic.cz>

Hi Stefan,

Pali has prepared another series of patches for kwbimage, I have reviewed
them.

Marek

Pali Rohár (11):
  tools: kwbimage: Add support for new commands UART_PORT and UART_MPP
  tools: kwbimage: Explicitly set version also for kwbimage v0
  tools: kwbimage: Set BOOT_FROM by default to SPI
  tools: kwbimage: Fix validation of kwbimage v0
  tools: kwbimage: Remove unused enums and prototypes
  tools: kwbimage: Align final UART image to 128 bytes
  tools: kwbimage: Do not put final image padding to the image data size
  tools: kwbimage: Align kwbimage header to proper size
  tools: kwbimage: Fill the real header size into the main header
  tools: kwbimage: Properly calculate and align kwbimage v0 header size
  tools: kwbimage: Properly set srcaddr in kwbimage v0

 tools/kwbimage.c | 250 ++++++++++++++++++++++++++++++++---------------
 tools/kwbimage.h |  25 +----
 2 files changed, 174 insertions(+), 101 deletions(-)

-- 
2.32.0

[PATCH u-boot-marvell 01/11] tools: kwbimage: Add support for new commands UART_PORT and UART_MPP

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

These two commands allow to specify custom setting of UART port used for
printing BootROM messages.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 67c0c628ae..f24d49496b 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -101,6 +101,8 @@ enum image_cfg_type {
 	IMAGE_CFG_DATA,
 	IMAGE_CFG_DATA_DELAY,
 	IMAGE_CFG_BAUDRATE,
+	IMAGE_CFG_UART_PORT,
+	IMAGE_CFG_UART_MPP,
 	IMAGE_CFG_DEBUG,
 	IMAGE_CFG_KAK,
 	IMAGE_CFG_CSK,
@@ -129,6 +131,8 @@ static const char * const id_strs[] = {
 	[IMAGE_CFG_DATA] = "DATA",
 	[IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",
 	[IMAGE_CFG_BAUDRATE] = "BAUDRATE",
+	[IMAGE_CFG_UART_PORT] = "UART_PORT",
+	[IMAGE_CFG_UART_MPP] = "UART_MPP",
 	[IMAGE_CFG_DEBUG] = "DEBUG",
 	[IMAGE_CFG_KAK] = "KAK",
 	[IMAGE_CFG_CSK] = "CSK",
@@ -161,6 +165,8 @@ struct image_cfg_element {
 		struct ext_hdr_v0_reg regdata;
 		unsigned int regdata_delay;
 		unsigned int baudrate;
+		unsigned int uart_port;
+		unsigned int uart_mpp;
 		unsigned int debug;
 		const char *key_name;
 		int csk_idx;
@@ -1239,7 +1245,13 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 		main_hdr->nandbadblklocation = e->nandbadblklocation;
 	e = image_find_option(IMAGE_CFG_BAUDRATE);
 	if (e)
-		main_hdr->options = baudrate_to_option(e->baudrate);
+		main_hdr->options |= baudrate_to_option(e->baudrate);
+	e = image_find_option(IMAGE_CFG_UART_PORT);
+	if (e)
+		main_hdr->options |= (e->uart_port & 3) << 3;
+	e = image_find_option(IMAGE_CFG_UART_MPP);
+	if (e)
+		main_hdr->options |= (e->uart_mpp & 7) << 5;
 	e = image_find_option(IMAGE_CFG_DEBUG);
 	if (e)
 		main_hdr->flags = e->debug ? 0x1 : 0;
@@ -1441,6 +1453,12 @@ static int image_create_config_parse_oneline(char *line,
 	case IMAGE_CFG_BAUDRATE:
 		el->baudrate = strtoul(value1, NULL, 10);
 		break;
+	case IMAGE_CFG_UART_PORT:
+		el->uart_port = strtoul(value1, NULL, 16);
+		break;
+	case IMAGE_CFG_UART_MPP:
+		el->uart_mpp = strtoul(value1, NULL, 16);
+		break;
 	case IMAGE_CFG_DEBUG:
 		el->debug = strtoul(value1, NULL, 10);
 		break;
-- 
2.32.0

Re: [PATCH u-boot-marvell 01/11] tools: kwbimage: Add support for new commands UART_PORT and UART_MPP

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> These two commands allow to specify custom setting of UART port used for
> printing BootROM messages.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 67c0c628ae..f24d49496b 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -101,6 +101,8 @@ enum image_cfg_type {
>   	IMAGE_CFG_DATA,
>   	IMAGE_CFG_DATA_DELAY,
>   	IMAGE_CFG_BAUDRATE,
> +	IMAGE_CFG_UART_PORT,
> +	IMAGE_CFG_UART_MPP,
>   	IMAGE_CFG_DEBUG,
>   	IMAGE_CFG_KAK,
>   	IMAGE_CFG_CSK,
> @@ -129,6 +131,8 @@ static const char * const id_strs[] = {
>   	[IMAGE_CFG_DATA] = "DATA",
>   	[IMAGE_CFG_DATA_DELAY] = "DATA_DELAY",
>   	[IMAGE_CFG_BAUDRATE] = "BAUDRATE",
> +	[IMAGE_CFG_UART_PORT] = "UART_PORT",
> +	[IMAGE_CFG_UART_MPP] = "UART_MPP",
>   	[IMAGE_CFG_DEBUG] = "DEBUG",
>   	[IMAGE_CFG_KAK] = "KAK",
>   	[IMAGE_CFG_CSK] = "CSK",
> @@ -161,6 +165,8 @@ struct image_cfg_element {
>   		struct ext_hdr_v0_reg regdata;
>   		unsigned int regdata_delay;
>   		unsigned int baudrate;
> +		unsigned int uart_port;
> +		unsigned int uart_mpp;
>   		unsigned int debug;
>   		const char *key_name;
>   		int csk_idx;
> @@ -1239,7 +1245,13 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   		main_hdr->nandbadblklocation = e->nandbadblklocation;
>   	e = image_find_option(IMAGE_CFG_BAUDRATE);
>   	if (e)
> -		main_hdr->options = baudrate_to_option(e->baudrate);
> +		main_hdr->options |= baudrate_to_option(e->baudrate);
> +	e = image_find_option(IMAGE_CFG_UART_PORT);
> +	if (e)
> +		main_hdr->options |= (e->uart_port & 3) << 3;
> +	e = image_find_option(IMAGE_CFG_UART_MPP);
> +	if (e)
> +		main_hdr->options |= (e->uart_mpp & 7) << 5;
>   	e = image_find_option(IMAGE_CFG_DEBUG);
>   	if (e)
>   		main_hdr->flags = e->debug ? 0x1 : 0;
> @@ -1441,6 +1453,12 @@ static int image_create_config_parse_oneline(char *line,
>   	case IMAGE_CFG_BAUDRATE:
>   		el->baudrate = strtoul(value1, NULL, 10);
>   		break;
> +	case IMAGE_CFG_UART_PORT:
> +		el->uart_port = strtoul(value1, NULL, 16);
> +		break;
> +	case IMAGE_CFG_UART_MPP:
> +		el->uart_mpp = strtoul(value1, NULL, 16);
> +		break;
>   	case IMAGE_CFG_DEBUG:
>   		el->debug = strtoul(value1, NULL, 10);
>   		break;
> 

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 02/11] tools: kwbimage: Explicitly set version also for kwbimage v0

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

For documentation purposes update struct main_hdr_v0 to include information
where version of the image must be stored. For kwbimage v0 it obviously
must be 0. By default all image header memory is initialized to zero,
therefore this change has no functional effect.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index f24d49496b..d29f2cfcce 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -881,6 +881,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 		cpu_to_le32(payloadsz - headersz);
 	main_hdr->srcaddr   = cpu_to_le32(headersz);
 	main_hdr->ext       = has_ext;
+	main_hdr->version   = 0;
 	main_hdr->destaddr  = cpu_to_le32(params->addr);
 	main_hdr->execaddr  = cpu_to_le32(params->ep);
 
diff --git a/tools/kwbimage.h b/tools/kwbimage.h
index f1ba95c2fa..f74767e633 100644
--- a/tools/kwbimage.h
+++ b/tools/kwbimage.h
@@ -42,7 +42,8 @@ struct main_hdr_v0 {
 	uint8_t  nandeccmode;		/* 0x1       */
 	uint16_t nandpagesize;		/* 0x2-0x3   */
 	uint32_t blocksize;		/* 0x4-0x7   */
-	uint32_t rsvd1;			/* 0x8-0xB   */
+	uint8_t  version;		/* 0x8       */
+	uint8_t  rsvd1[3];		/* 0x9-0xB   */
 	uint32_t srcaddr;		/* 0xC-0xF   */
 	uint32_t destaddr;		/* 0x10-0x13 */
 	uint32_t execaddr;		/* 0x14-0x17 */
-- 
2.32.0

Re: [PATCH u-boot-marvell 02/11] tools: kwbimage: Explicitly set version also for kwbimage v0

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> For documentation purposes update struct main_hdr_v0 to include information
> where version of the image must be stored. For kwbimage v0 it obviously
> must be 0. By default all image header memory is initialized to zero,
> therefore this change has no functional effect.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 1 +
>   tools/kwbimage.h | 3 ++-
>   2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index f24d49496b..d29f2cfcce 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -881,6 +881,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   		cpu_to_le32(payloadsz - headersz);
>   	main_hdr->srcaddr   = cpu_to_le32(headersz);
>   	main_hdr->ext       = has_ext;
> +	main_hdr->version   = 0;
>   	main_hdr->destaddr  = cpu_to_le32(params->addr);
>   	main_hdr->execaddr  = cpu_to_le32(params->ep);
>   
> diff --git a/tools/kwbimage.h b/tools/kwbimage.h
> index f1ba95c2fa..f74767e633 100644
> --- a/tools/kwbimage.h
> +++ b/tools/kwbimage.h
> @@ -42,7 +42,8 @@ struct main_hdr_v0 {
>   	uint8_t  nandeccmode;		/* 0x1       */
>   	uint16_t nandpagesize;		/* 0x2-0x3   */
>   	uint32_t blocksize;		/* 0x4-0x7   */
> -	uint32_t rsvd1;			/* 0x8-0xB   */
> +	uint8_t  version;		/* 0x8       */
> +	uint8_t  rsvd1[3];		/* 0x9-0xB   */
>   	uint32_t srcaddr;		/* 0xC-0xF   */
>   	uint32_t destaddr;		/* 0x10-0x13 */
>   	uint32_t execaddr;		/* 0x14-0x17 */
> 

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 03/11] tools: kwbimage: Set BOOT_FROM by default to SPI

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

kwbimage must have valid blockid member instead of zero value. Thus if
config file does not contain BOOT_FROM command, use by default the value
for SPI booting (which is probably the most common).

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index d29f2cfcce..38b6e2fed2 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -266,6 +266,18 @@ static bool image_get_spezialized_img(void)
 	return e->sec_specialized_img;
 }
 
+static int image_get_bootfrom(void)
+{
+	struct image_cfg_element *e;
+
+	e = image_find_option(IMAGE_CFG_BOOT_FROM);
+	if (!e)
+		/* fallback to SPI if no BOOT_FROM is not provided */
+		return IBR_HDR_SPI_ID;
+
+	return e->bootfrom;
+}
+
 /*
  * Compute a 8-bit checksum of a memory area. This algorithm follows
  * the requirements of the Marvell SoC BootROM specifications.
@@ -884,10 +896,8 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 	main_hdr->version   = 0;
 	main_hdr->destaddr  = cpu_to_le32(params->addr);
 	main_hdr->execaddr  = cpu_to_le32(params->ep);
+	main_hdr->blockid   = image_get_bootfrom();
 
-	e = image_find_option(IMAGE_CFG_BOOT_FROM);
-	if (e)
-		main_hdr->blockid = e->bootfrom;
 	e = image_find_option(IMAGE_CFG_NAND_ECC_MODE);
 	if (e)
 		main_hdr->nandeccmode = e->nandeccmode;
@@ -1232,9 +1242,8 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	main_hdr->srcaddr      = cpu_to_le32(headersz);
 	main_hdr->ext          = hasext;
 	main_hdr->version      = 1;
-	e = image_find_option(IMAGE_CFG_BOOT_FROM);
-	if (e)
-		main_hdr->blockid = e->bootfrom;
+	main_hdr->blockid      = image_get_bootfrom();
+
 	e = image_find_option(IMAGE_CFG_NAND_BLKSZ);
 	if (e)
 		main_hdr->nandblocksize = e->nandblksz / (64 * 1024);
@@ -1559,17 +1568,6 @@ static int image_get_version(void)
 	return e->version;
 }
 
-static int image_get_bootfrom(void)
-{
-	struct image_cfg_element *e;
-
-	e = image_find_option(IMAGE_CFG_BOOT_FROM);
-	if (!e)
-		return -1;
-
-	return e->bootfrom;
-}
-
 static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 				struct image_tool_params *params)
 {
-- 
2.32.0

Re: [PATCH u-boot-marvell 03/11] tools: kwbimage: Set BOOT_FROM by default to SPI

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> kwbimage must have valid blockid member instead of zero value. Thus if
> config file does not contain BOOT_FROM command, use by default the value
> for SPI booting (which is probably the most common).
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 32 +++++++++++++++-----------------
>   1 file changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index d29f2cfcce..38b6e2fed2 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -266,6 +266,18 @@ static bool image_get_spezialized_img(void)
>   	return e->sec_specialized_img;
>   }
>   
> +static int image_get_bootfrom(void)
> +{
> +	struct image_cfg_element *e;
> +
> +	e = image_find_option(IMAGE_CFG_BOOT_FROM);
> +	if (!e)
> +		/* fallback to SPI if no BOOT_FROM is not provided */
> +		return IBR_HDR_SPI_ID;
> +
> +	return e->bootfrom;
> +}
> +
>   /*
>    * Compute a 8-bit checksum of a memory area. This algorithm follows
>    * the requirements of the Marvell SoC BootROM specifications.
> @@ -884,10 +896,8 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   	main_hdr->version   = 0;
>   	main_hdr->destaddr  = cpu_to_le32(params->addr);
>   	main_hdr->execaddr  = cpu_to_le32(params->ep);
> +	main_hdr->blockid   = image_get_bootfrom();
>   
> -	e = image_find_option(IMAGE_CFG_BOOT_FROM);
> -	if (e)
> -		main_hdr->blockid = e->bootfrom;
>   	e = image_find_option(IMAGE_CFG_NAND_ECC_MODE);
>   	if (e)
>   		main_hdr->nandeccmode = e->nandeccmode;
> @@ -1232,9 +1242,8 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	main_hdr->srcaddr      = cpu_to_le32(headersz);
>   	main_hdr->ext          = hasext;
>   	main_hdr->version      = 1;
> -	e = image_find_option(IMAGE_CFG_BOOT_FROM);
> -	if (e)
> -		main_hdr->blockid = e->bootfrom;
> +	main_hdr->blockid      = image_get_bootfrom();
> +
>   	e = image_find_option(IMAGE_CFG_NAND_BLKSZ);
>   	if (e)
>   		main_hdr->nandblocksize = e->nandblksz / (64 * 1024);
> @@ -1559,17 +1568,6 @@ static int image_get_version(void)
>   	return e->version;
>   }
>   
> -static int image_get_bootfrom(void)
> -{
> -	struct image_cfg_element *e;
> -
> -	e = image_find_option(IMAGE_CFG_BOOT_FROM);
> -	if (!e)
> -		return -1;
> -
> -	return e->bootfrom;
> -}
> -
>   static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
>   				struct image_tool_params *params)
>   {
> 

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 04/11] tools: kwbimage: Fix validation of kwbimage v0

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

kwbimage v0 sldo has 32-bit data checksum at the end like kwbimage v1.

Use same data checksum validation for both v0 and v1 image types.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 38b6e2fed2..a176b39b08 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1680,6 +1680,9 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
 				  struct image_tool_params *params)
 {
 	size_t header_size = kwbheader_size(ptr);
+	uint8_t blockid;
+	uint32_t offset;
+	uint32_t size;
 	uint8_t csum;
 
 	if (header_size > image_size)
@@ -1699,61 +1702,64 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
 			if (csum != ext_hdr->checksum)
 				return -FDT_ERR_BADSTRUCTURE;
 		}
+
+		blockid = mhdr->blockid;
+		offset = le32_to_cpu(mhdr->srcaddr);
+		size = le32_to_cpu(mhdr->blocksize);
 	} else if (kwbimage_version(ptr) == 1) {
 		struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
 		const uint8_t *mhdr_end;
 		struct opt_hdr_v1 *ohdr;
-		uint32_t offset;
-		uint32_t size;
 
 		mhdr_end = (uint8_t *)mhdr + header_size;
 		for_each_opt_hdr_v1 (ohdr, ptr)
 			if (!opt_hdr_v1_valid_size(ohdr, mhdr_end))
 				return -FDT_ERR_BADSTRUCTURE;
 
+		blockid = mhdr->blockid;
 		offset = le32_to_cpu(mhdr->srcaddr);
+		size = le32_to_cpu(mhdr->blocksize);
+	} else {
+		return -FDT_ERR_BADSTRUCTURE;
+	}
 
-		/*
-		 * For SATA srcaddr is specified in number of sectors.
-		 * The main header is must be stored at sector number 1.
-		 * This expects that sector size is 512 bytes and recalculates
-		 * data offset to bytes relative to the main header.
-		 */
-		if (mhdr->blockid == IBR_HDR_SATA_ID) {
-			if (offset < 1)
-				return -FDT_ERR_BADSTRUCTURE;
-			offset -= 1;
-			offset *= 512;
-		}
+	/*
+	 * For SATA srcaddr is specified in number of sectors.
+	 * The main header is must be stored at sector number 1.
+	 * This expects that sector size is 512 bytes and recalculates
+	 * data offset to bytes relative to the main header.
+	 */
+	if (blockid == IBR_HDR_SATA_ID) {
+		if (offset < 1)
+			return -FDT_ERR_BADSTRUCTURE;
+		offset -= 1;
+		offset *= 512;
+	}
 
-		/*
-		 * For SDIO srcaddr is specified in number of sectors.
-		 * This expects that sector size is 512 bytes and recalculates
-		 * data offset to bytes.
-		 */
-		if (mhdr->blockid == IBR_HDR_SDIO_ID)
-			offset *= 512;
+	/*
+	 * For SDIO srcaddr is specified in number of sectors.
+	 * This expects that sector size is 512 bytes and recalculates
+	 * data offset to bytes.
+	 */
+	if (blockid == IBR_HDR_SDIO_ID)
+		offset *= 512;
 
-		/*
-		 * For PCIe srcaddr is always set to 0xFFFFFFFF.
-		 * This expects that data starts after all headers.
-		 */
-		if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
-			offset = header_size;
+	/*
+	 * For PCIe srcaddr is always set to 0xFFFFFFFF.
+	 * This expects that data starts after all headers.
+	 */
+	if (blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
+		offset = header_size;
 
-		if (offset > image_size || offset % 4 != 0)
-			return -FDT_ERR_BADSTRUCTURE;
+	if (offset > image_size || offset % 4 != 0)
+		return -FDT_ERR_BADSTRUCTURE;
 
-		size = le32_to_cpu(mhdr->blocksize);
-		if (size < 4 || offset + size > image_size || size % 4 != 0)
-			return -FDT_ERR_BADSTRUCTURE;
+	if (size < 4 || offset + size > image_size || size % 4 != 0)
+		return -FDT_ERR_BADSTRUCTURE;
 
-		if (image_checksum32(ptr + offset, size - 4) !=
-		    *(uint32_t *)(ptr + offset + size - 4))
-			return -FDT_ERR_BADSTRUCTURE;
-	} else {
+	if (image_checksum32(ptr + offset, size - 4) !=
+	    *(uint32_t *)(ptr + offset + size - 4))
 		return -FDT_ERR_BADSTRUCTURE;
-	}
 
 	return 0;
 }
-- 
2.32.0

Re: [PATCH u-boot-marvell 04/11] tools: kwbimage: Fix validation of kwbimage v0

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> kwbimage v0 sldo has 32-bit data checksum at the end like kwbimage v1.
> 
> Use same data checksum validation for both v0 and v1 image types.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 80 ++++++++++++++++++++++++++----------------------
>   1 file changed, 43 insertions(+), 37 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 38b6e2fed2..a176b39b08 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1680,6 +1680,9 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
>   				  struct image_tool_params *params)
>   {
>   	size_t header_size = kwbheader_size(ptr);
> +	uint8_t blockid;
> +	uint32_t offset;
> +	uint32_t size;
>   	uint8_t csum;
>   
>   	if (header_size > image_size)
> @@ -1699,61 +1702,64 @@ static int kwbimage_verify_header(unsigned char *ptr, int image_size,
>   			if (csum != ext_hdr->checksum)
>   				return -FDT_ERR_BADSTRUCTURE;
>   		}
> +
> +		blockid = mhdr->blockid;
> +		offset = le32_to_cpu(mhdr->srcaddr);
> +		size = le32_to_cpu(mhdr->blocksize);
>   	} else if (kwbimage_version(ptr) == 1) {
>   		struct main_hdr_v1 *mhdr = (struct main_hdr_v1 *)ptr;
>   		const uint8_t *mhdr_end;
>   		struct opt_hdr_v1 *ohdr;
> -		uint32_t offset;
> -		uint32_t size;
>   
>   		mhdr_end = (uint8_t *)mhdr + header_size;
>   		for_each_opt_hdr_v1 (ohdr, ptr)
>   			if (!opt_hdr_v1_valid_size(ohdr, mhdr_end))
>   				return -FDT_ERR_BADSTRUCTURE;
>   
> +		blockid = mhdr->blockid;
>   		offset = le32_to_cpu(mhdr->srcaddr);
> +		size = le32_to_cpu(mhdr->blocksize);
> +	} else {
> +		return -FDT_ERR_BADSTRUCTURE;
> +	}
>   
> -		/*
> -		 * For SATA srcaddr is specified in number of sectors.
> -		 * The main header is must be stored at sector number 1.
> -		 * This expects that sector size is 512 bytes and recalculates
> -		 * data offset to bytes relative to the main header.
> -		 */
> -		if (mhdr->blockid == IBR_HDR_SATA_ID) {
> -			if (offset < 1)
> -				return -FDT_ERR_BADSTRUCTURE;
> -			offset -= 1;
> -			offset *= 512;
> -		}
> +	/*
> +	 * For SATA srcaddr is specified in number of sectors.
> +	 * The main header is must be stored at sector number 1.
> +	 * This expects that sector size is 512 bytes and recalculates
> +	 * data offset to bytes relative to the main header.
> +	 */
> +	if (blockid == IBR_HDR_SATA_ID) {
> +		if (offset < 1)
> +			return -FDT_ERR_BADSTRUCTURE;
> +		offset -= 1;
> +		offset *= 512;
> +	}
>   
> -		/*
> -		 * For SDIO srcaddr is specified in number of sectors.
> -		 * This expects that sector size is 512 bytes and recalculates
> -		 * data offset to bytes.
> -		 */
> -		if (mhdr->blockid == IBR_HDR_SDIO_ID)
> -			offset *= 512;
> +	/*
> +	 * For SDIO srcaddr is specified in number of sectors.
> +	 * This expects that sector size is 512 bytes and recalculates
> +	 * data offset to bytes.
> +	 */
> +	if (blockid == IBR_HDR_SDIO_ID)
> +		offset *= 512;
>   
> -		/*
> -		 * For PCIe srcaddr is always set to 0xFFFFFFFF.
> -		 * This expects that data starts after all headers.
> -		 */
> -		if (mhdr->blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
> -			offset = header_size;
> +	/*
> +	 * For PCIe srcaddr is always set to 0xFFFFFFFF.
> +	 * This expects that data starts after all headers.
> +	 */
> +	if (blockid == IBR_HDR_PEX_ID && offset == 0xFFFFFFFF)
> +		offset = header_size;
>   
> -		if (offset > image_size || offset % 4 != 0)
> -			return -FDT_ERR_BADSTRUCTURE;
> +	if (offset > image_size || offset % 4 != 0)
> +		return -FDT_ERR_BADSTRUCTURE;
>   
> -		size = le32_to_cpu(mhdr->blocksize);
> -		if (size < 4 || offset + size > image_size || size % 4 != 0)
> -			return -FDT_ERR_BADSTRUCTURE;
> +	if (size < 4 || offset + size > image_size || size % 4 != 0)
> +		return -FDT_ERR_BADSTRUCTURE;
>   
> -		if (image_checksum32(ptr + offset, size - 4) !=
> -		    *(uint32_t *)(ptr + offset + size - 4))
> -			return -FDT_ERR_BADSTRUCTURE;
> -	} else {
> +	if (image_checksum32(ptr + offset, size - 4) !=
> +	    *(uint32_t *)(ptr + offset + size - 4))
>   		return -FDT_ERR_BADSTRUCTURE;
> -	}
>   
>   	return 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 05/11] tools: kwbimage: Remove unused enums and prototypes

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

There are more unused enums and function prototypes. Remove them. The
function kwbimage_check_params() does not return enum kwbimage_cmd_types,
but a boolean value returned as int.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index a176b39b08..864625d788 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1916,7 +1916,7 @@ static int kwbimage_check_params(struct image_tool_params *params)
 		char *msg = "Configuration file for kwbimage creation omitted";
 
 		fprintf(stderr, "Error:%s - %s\n", params->cmdname, msg);
-		return CFG_INVALID;
+		return 1;
 	}
 
 	return (params->dflag && (params->fflag || params->lflag)) ||
diff --git a/tools/kwbimage.h b/tools/kwbimage.h
index f74767e633..8d37357e5a 100644
--- a/tools/kwbimage.h
+++ b/tools/kwbimage.h
@@ -191,28 +191,6 @@ struct register_set_hdr_v1 {
 #define OPT_HDR_V1_BINARY_TYPE   0x2
 #define OPT_HDR_V1_REGISTER_TYPE 0x3
 
-enum kwbimage_cmd {
-	CMD_INVALID,
-	CMD_BOOT_FROM,
-	CMD_NAND_ECC_MODE,
-	CMD_NAND_PAGE_SIZE,
-	CMD_SATA_PIO_MODE,
-	CMD_DDR_INIT_DELAY,
-	CMD_DATA
-};
-
-enum kwbimage_cmd_types {
-	CFG_INVALID = -1,
-	CFG_COMMAND,
-	CFG_DATA0,
-	CFG_DATA1
-};
-
-/*
- * functions
- */
-void init_kwb_image_type (void);
-
 /*
  * Byte 8 of the image header contains the version number. In the v0
  * header, byte 8 was reserved, and always set to 0. In the v1 header,
-- 
2.32.0

Re: [PATCH u-boot-marvell 05/11] tools: kwbimage: Remove unused enums and prototypes

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> There are more unused enums and function prototypes. Remove them. The
> function kwbimage_check_params() does not return enum kwbimage_cmd_types,
> but a boolean value returned as int.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c |  2 +-
>   tools/kwbimage.h | 22 ----------------------
>   2 files changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index a176b39b08..864625d788 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1916,7 +1916,7 @@ static int kwbimage_check_params(struct image_tool_params *params)
>   		char *msg = "Configuration file for kwbimage creation omitted";
>   
>   		fprintf(stderr, "Error:%s - %s\n", params->cmdname, msg);
> -		return CFG_INVALID;
> +		return 1;
>   	}
>   
>   	return (params->dflag && (params->fflag || params->lflag)) ||
> diff --git a/tools/kwbimage.h b/tools/kwbimage.h
> index f74767e633..8d37357e5a 100644
> --- a/tools/kwbimage.h
> +++ b/tools/kwbimage.h
> @@ -191,28 +191,6 @@ struct register_set_hdr_v1 {
>   #define OPT_HDR_V1_BINARY_TYPE   0x2
>   #define OPT_HDR_V1_REGISTER_TYPE 0x3
>   
> -enum kwbimage_cmd {
> -	CMD_INVALID,
> -	CMD_BOOT_FROM,
> -	CMD_NAND_ECC_MODE,
> -	CMD_NAND_PAGE_SIZE,
> -	CMD_SATA_PIO_MODE,
> -	CMD_DDR_INIT_DELAY,
> -	CMD_DATA
> -};
> -
> -enum kwbimage_cmd_types {
> -	CFG_INVALID = -1,
> -	CFG_COMMAND,
> -	CFG_DATA0,
> -	CFG_DATA1
> -};
> -
> -/*
> - * functions
> - */
> -void init_kwb_image_type (void);
> -
>   /*
>    * Byte 8 of the image header contains the version number. In the v0
>    * header, byte 8 was reserved, and always set to 0. In the v1 header,
> 

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 06/11] tools: kwbimage: Align final UART image to 128 bytes

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

xmodem block size is 128 bytes, therefore it is possible to transfer only
images with size multiple of 128 bytes. kwboot automatically pads image
with zero bytes at the end to align it to 128 bytes boundary.

Do this padding when generating image to allow uploading with other xmodem
tools or older kwboot versions.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 864625d788..b939b4cb49 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1847,6 +1847,7 @@ static int kwbimage_generate(struct image_tool_params *params,
 	 * The resulting image needs to be 4-byte aligned. At least
 	 * the Marvell hdrparser tool complains if its unaligned.
 	 * After the image data is stored 4-byte checksum.
+	 * Final UART image must be aligned to 128 bytes.
 	 * Final SPI and NAND images must be aligned to 256 bytes.
 	 * Final SATA and SDIO images must be aligned to 512 bytes.
 	 */
@@ -1854,6 +1855,8 @@ static int kwbimage_generate(struct image_tool_params *params,
 		return 4 + (256 - (alloc_len + s.st_size + 4) % 256) % 256;
 	else if (bootfrom == IBR_HDR_SATA_ID || bootfrom == IBR_HDR_SDIO_ID)
 		return 4 + (512 - (alloc_len + s.st_size + 4) % 512) % 512;
+	else if (bootfrom == IBR_HDR_UART_ID)
+		return 4 + (128 - (alloc_len + s.st_size + 4) % 128) % 128;
 	else
 		return 4 + (4 - s.st_size % 4) % 4;
 }
-- 
2.32.0

Re: [PATCH u-boot-marvell 06/11] tools: kwbimage: Align final UART image to 128 bytes

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> xmodem block size is 128 bytes, therefore it is possible to transfer only
> images with size multiple of 128 bytes. kwboot automatically pads image
> with zero bytes at the end to align it to 128 bytes boundary.
> 
> Do this padding when generating image to allow uploading with other xmodem
> tools or older kwboot versions.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Minor comment below. Other than this:

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 864625d788..b939b4cb49 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1847,6 +1847,7 @@ static int kwbimage_generate(struct image_tool_params *params,
>   	 * The resulting image needs to be 4-byte aligned. At least
>   	 * the Marvell hdrparser tool complains if its unaligned.
>   	 * After the image data is stored 4-byte checksum.
> +	 * Final UART image must be aligned to 128 bytes.
>   	 * Final SPI and NAND images must be aligned to 256 bytes.
>   	 * Final SATA and SDIO images must be aligned to 512 bytes.
>   	 */
> @@ -1854,6 +1855,8 @@ static int kwbimage_generate(struct image_tool_params *params,
>   		return 4 + (256 - (alloc_len + s.st_size + 4) % 256) % 256;
>   	else if (bootfrom == IBR_HDR_SATA_ID || bootfrom == IBR_HDR_SDIO_ID)
>   		return 4 + (512 - (alloc_len + s.st_size + 4) % 512) % 512;
> +	else if (bootfrom == IBR_HDR_UART_ID)
> +		return 4 + (128 - (alloc_len + s.st_size + 4) % 128) % 128;
>   	else
>   		return 4 + (4 - s.st_size % 4) % 4;

The lines above are pretty similar - perhaps some function / macro could
be used to simplify these lines?

Thanks,
Stefan

[PATCH u-boot-marvell 07/11] tools: kwbimage: Do not put final image padding to the image data size

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

This change allows to convert image from one format to another without need
to include unnecessary padding (e.g. when target image format has smaller
alignment requirement as source image format).

Do it by storing real image data size without padding to the kwbimage
header.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index b939b4cb49..a6f2659ab4 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -890,7 +890,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 
 	/* Fill in the main header */
 	main_hdr->blocksize =
-		cpu_to_le32(payloadsz - headersz);
+		cpu_to_le32(payloadsz);
 	main_hdr->srcaddr   = cpu_to_le32(headersz);
 	main_hdr->ext       = has_ext;
 	main_hdr->version   = 0;
@@ -1234,7 +1234,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 
 	/* Fill the main header */
 	main_hdr->blocksize    =
-		cpu_to_le32(payloadsz - headersz);
+		cpu_to_le32(payloadsz);
 	main_hdr->headersz_lsb = cpu_to_le16(headersz & 0xFFFF);
 	main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;
 	main_hdr->destaddr     = cpu_to_le32(params->addr);
@@ -1345,7 +1345,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 			return NULL;
 	}
 
-	if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz,
+	if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz + headersz,
 					       headersz, image, secure_hdr))
 		return NULL;
 
@@ -1575,9 +1575,22 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 	void *image = NULL;
 	int version;
 	size_t headersz = 0;
+	size_t datasz;
 	uint32_t checksum;
+	struct stat s;
 	int ret;
 
+	/*
+	 * Do not use sbuf->st_size as it contains size with padding.
+	 * We need original image data size, so stat original file.
+	 */
+	if (stat(params->datafile, &s)) {
+		fprintf(stderr, "Could not stat data file %s: %s\n",
+			params->datafile, strerror(errno));
+		exit(EXIT_FAILURE);
+	}
+	datasz = ALIGN(s.st_size, 4);
+
 	fcfg = fopen(params->imagename, "r");
 	if (!fcfg) {
 		fprintf(stderr, "Could not open input file %s\n",
@@ -1612,11 +1625,11 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 		 */
 	case -1:
 	case 0:
-		image = image_create_v0(&headersz, params, sbuf->st_size);
+		image = image_create_v0(&headersz, params, datasz + 4);
 		break;
 
 	case 1:
-		image = image_create_v1(&headersz, params, ptr, sbuf->st_size);
+		image = image_create_v1(&headersz, params, ptr, datasz + 4);
 		break;
 
 	default:
@@ -1633,11 +1646,10 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
 
 	free(image_cfg);
 
-	/* Build and add image checksum header */
+	/* Build and add image data checksum */
 	checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
-				sbuf->st_size - headersz - sizeof(uint32_t)));
-	memcpy((uint8_t *)ptr + sbuf->st_size - sizeof(uint32_t), &checksum,
-		sizeof(uint32_t));
+						datasz));
+	memcpy((uint8_t *)ptr + headersz + datasz, &checksum, sizeof(uint32_t));
 
 	/* Finally copy the header into the image area */
 	memcpy(ptr, image, headersz);
-- 
2.32.0

Re: [PATCH u-boot-marvell 07/11] tools: kwbimage: Do not put final image padding to the image data size

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> This change allows to convert image from one format to another without need
> to include unnecessary padding (e.g. when target image format has smaller
> alignment requirement as source image format).
> 
> Do it by storing real image data size without padding to the kwbimage
> header.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 30 +++++++++++++++++++++---------
>   1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index b939b4cb49..a6f2659ab4 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -890,7 +890,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   
>   	/* Fill in the main header */
>   	main_hdr->blocksize =
> -		cpu_to_le32(payloadsz - headersz);
> +		cpu_to_le32(payloadsz);
>   	main_hdr->srcaddr   = cpu_to_le32(headersz);
>   	main_hdr->ext       = has_ext;
>   	main_hdr->version   = 0;
> @@ -1234,7 +1234,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   
>   	/* Fill the main header */
>   	main_hdr->blocksize    =
> -		cpu_to_le32(payloadsz - headersz);
> +		cpu_to_le32(payloadsz);
>   	main_hdr->headersz_lsb = cpu_to_le16(headersz & 0xFFFF);
>   	main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;
>   	main_hdr->destaddr     = cpu_to_le32(params->addr);
> @@ -1345,7 +1345,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   			return NULL;
>   	}
>   
> -	if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz,
> +	if (secure_hdr && add_secure_header_v1(params, ptr, payloadsz + headersz,
>   					       headersz, image, secure_hdr))
>   		return NULL;
>   
> @@ -1575,9 +1575,22 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
>   	void *image = NULL;
>   	int version;
>   	size_t headersz = 0;
> +	size_t datasz;
>   	uint32_t checksum;
> +	struct stat s;
>   	int ret;
>   
> +	/*
> +	 * Do not use sbuf->st_size as it contains size with padding.
> +	 * We need original image data size, so stat original file.
> +	 */
> +	if (stat(params->datafile, &s)) {
> +		fprintf(stderr, "Could not stat data file %s: %s\n",
> +			params->datafile, strerror(errno));
> +		exit(EXIT_FAILURE);
> +	}
> +	datasz = ALIGN(s.st_size, 4);
> +
>   	fcfg = fopen(params->imagename, "r");
>   	if (!fcfg) {
>   		fprintf(stderr, "Could not open input file %s\n",
> @@ -1612,11 +1625,11 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
>   		 */
>   	case -1:
>   	case 0:
> -		image = image_create_v0(&headersz, params, sbuf->st_size);
> +		image = image_create_v0(&headersz, params, datasz + 4);
>   		break;
>   
>   	case 1:
> -		image = image_create_v1(&headersz, params, ptr, sbuf->st_size);
> +		image = image_create_v1(&headersz, params, ptr, datasz + 4);
>   		break;
>   
>   	default:
> @@ -1633,11 +1646,10 @@ static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
>   
>   	free(image_cfg);
>   
> -	/* Build and add image checksum header */
> +	/* Build and add image data checksum */
>   	checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
> -				sbuf->st_size - headersz - sizeof(uint32_t)));
> -	memcpy((uint8_t *)ptr + sbuf->st_size - sizeof(uint32_t), &checksum,
> -		sizeof(uint32_t));
> +						datasz));
> +	memcpy((uint8_t *)ptr + headersz + datasz, &checksum, sizeof(uint32_t));
>   
>   	/* Finally copy the header into the image area */
>   	memcpy(ptr, image, 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 08/11] tools: kwbimage: Align kwbimage header to proper size

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Currently kwbimage header is always aligned to 4096 bytes. But it does not
have to be aligned to such a high value.

The header needs to be just 4-byte aligned, while some image types have
additional alignment restrictions.

This change reduces size of kwbimage binaries by removing extra padding
between header and data part.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index a6f2659ab4..8dcfebcb57 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -858,6 +858,27 @@ done:
 	return ret;
 }
 
+static size_t image_headersz_align(size_t headersz, uint8_t blockid)
+{
+	/*
+	 * Header needs to be 4-byte aligned, which is already ensured by code
+	 * above. Moreover UART images must have header aligned to 128 bytes
+	 * (xmodem block size), NAND images to 256 bytes (ECC calculation),
+	 * and SATA and SDIO images to 512 bytes (storage block size).
+	 * Note that SPI images do not have to have header size aligned
+	 * to 256 bytes because it is possible to read from SPI storage from
+	 * any offset (read offset does not have to be aligned to block size).
+	 */
+	if (blockid == IBR_HDR_UART_ID)
+		return ALIGN(headersz, 128);
+	else if (blockid == IBR_HDR_NAND_ID)
+		return ALIGN(headersz, 256);
+	else if (blockid == IBR_HDR_SATA_ID || blockid == IBR_HDR_SDIO_ID)
+		return ALIGN(headersz, 512);
+	else
+		return headersz;
+}
+
 static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 			     int payloadsz)
 {
@@ -994,11 +1015,7 @@ static size_t image_headersz_v1(int *hasext)
 			*hasext = 1;
 	}
 
-	/*
-	 * The payload should be aligned on some reasonable
-	 * boundary
-	 */
-	return ALIGN(headersz, 4096);
+	return image_headersz_align(headersz, image_get_bootfrom());
 }
 
 int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
-- 
2.32.0

Re: [PATCH u-boot-marvell 08/11] tools: kwbimage: Align kwbimage header to proper size

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Currently kwbimage header is always aligned to 4096 bytes. But it does not
> have to be aligned to such a high value.
> 
> The header needs to be just 4-byte aligned, while some image types have
> additional alignment restrictions.
> 
> This change reduces size of kwbimage binaries by removing extra padding
> between header and data part.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index a6f2659ab4..8dcfebcb57 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -858,6 +858,27 @@ done:
>   	return ret;
>   }
>   
> +static size_t image_headersz_align(size_t headersz, uint8_t blockid)
> +{
> +	/*
> +	 * Header needs to be 4-byte aligned, which is already ensured by code
> +	 * above. Moreover UART images must have header aligned to 128 bytes
> +	 * (xmodem block size), NAND images to 256 bytes (ECC calculation),
> +	 * and SATA and SDIO images to 512 bytes (storage block size).
> +	 * Note that SPI images do not have to have header size aligned
> +	 * to 256 bytes because it is possible to read from SPI storage from
> +	 * any offset (read offset does not have to be aligned to block size).
> +	 */
> +	if (blockid == IBR_HDR_UART_ID)
> +		return ALIGN(headersz, 128);
> +	else if (blockid == IBR_HDR_NAND_ID)
> +		return ALIGN(headersz, 256);
> +	else if (blockid == IBR_HDR_SATA_ID || blockid == IBR_HDR_SDIO_ID)
> +		return ALIGN(headersz, 512);
> +	else
> +		return headersz;
> +}
> +
>   static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   			     int payloadsz)
>   {
> @@ -994,11 +1015,7 @@ static size_t image_headersz_v1(int *hasext)
>   			*hasext = 1;
>   	}
>   
> -	/*
> -	 * The payload should be aligned on some reasonable
> -	 * boundary
> -	 */
> -	return ALIGN(headersz, 4096);
> +	return image_headersz_align(headersz, image_get_bootfrom());
>   }
>   
>   int add_binary_header_v1(uint8_t **cur, uint8_t **next_ext,
> 

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 09/11] tools: kwbimage: Fill the real header size into the main header

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Fill the real header size without padding into the main header

This allows to reduce final image when converting image to another format
which does not need additional padding.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 8dcfebcb57..114b313b4d 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -1220,6 +1220,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 {
 	struct image_cfg_element *e;
 	struct main_hdr_v1 *main_hdr;
+	struct opt_hdr_v1 *ohdr;
 	struct register_set_hdr_v1 *register_set_hdr;
 	struct secure_hdr_v1 *secure_hdr = NULL;
 	size_t headersz;
@@ -1370,6 +1371,14 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
 	main_hdr->checksum = image_checksum8(main_hdr, headersz);
 
 	*imagesz = headersz;
+
+	/* Fill the real header size without padding into the main header */
+	headersz = sizeof(*main_hdr);
+	for_each_opt_hdr_v1 (ohdr, main_hdr)
+		headersz += opt_hdr_v1_size(ohdr);
+	main_hdr->headersz_lsb = cpu_to_le16(headersz & 0xFFFF);
+	main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;
+
 	return image;
 }
 
-- 
2.32.0

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Fill the real header size without padding into the main header
> 
> This allows to reduce final image when converting image to another format
> which does not need additional padding.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 8dcfebcb57..114b313b4d 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -1220,6 +1220,7 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   {
>   	struct image_cfg_element *e;
>   	struct main_hdr_v1 *main_hdr;
> +	struct opt_hdr_v1 *ohdr;
>   	struct register_set_hdr_v1 *register_set_hdr;
>   	struct secure_hdr_v1 *secure_hdr = NULL;
>   	size_t headersz;
> @@ -1370,6 +1371,14 @@ static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
>   	main_hdr->checksum = image_checksum8(main_hdr, headersz);
>   
>   	*imagesz = headersz;
> +
> +	/* Fill the real header size without padding into the main header */
> +	headersz = sizeof(*main_hdr);
> +	for_each_opt_hdr_v1 (ohdr, main_hdr)
> +		headersz += opt_hdr_v1_size(ohdr);
> +	main_hdr->headersz_lsb = cpu_to_le16(headersz & 0xFFFF);
> +	main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;
> +
>   	return image;
>   }
>   
> 

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 09/11] tools: kwbimage: Fill the real header size into the main header

[Pierre Bourdon]

Hi Marek, Pali,

On Mon, Nov 8, 2021 at 6:14 PM Marek Behún <kabel@kernel.org> wrote:
> Fill the real header size without padding into the main header
>
> This allows to reduce final image when converting image to another format
> which does not need additional padding.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

This patch seems to cause mkimage to generate v1 images with invalid
checksums (which fail to verify with kwboot or mkimage -l). Could you
double check whether you can reproduce on the latest u-boot master? I
don't really understand how this patch is supposed to work
(headersz_lsb/headersz_msb get updated *after* csum8 has already been
computed!).

$ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage
-a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
Image Type:   MVEBU Boot from nand Image
Image version:1
BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
Load Address: 00800000
Entry Point:  00800000

$ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
kwboot version 2022.01-rc4-00075-g3bd6c62cf4-dirty
u-boot.kwb: Invalid image.

(The specific kwbimage.cfg/board is still a WIP, I can try to provide
some more repro if somehow there is something specific to my setup,
but I doubt it.)

Best,

-- 
Pierre Bourdon <delroth@gmail.com>
Software Engineer @ Zürich, Switzerland
https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pali Rohár]

On Saturday 25 December 2021 18:48:22 Pierre Bourdon wrote:
> Hi Marek, Pali,
> 
> On Mon, Nov 8, 2021 at 6:14 PM Marek Behún <kabel@kernel.org> wrote:
> > Fill the real header size without padding into the main header
> >
> > This allows to reduce final image when converting image to another format
> > which does not need additional padding.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <marek.behun@nic.cz>
> 
> This patch seems to cause mkimage to generate v1 images with invalid
> checksums (which fail to verify with kwboot or mkimage -l). Could you
> double check whether you can reproduce on the latest u-boot master?

Hello! I'm using this patch for more than month and I have not seen any
boot issue related to this patch A385 board.

> I don't really understand how this patch is supposed to work
> (headersz_lsb/headersz_msb get updated *after* csum8 has already been
> computed!).

Ou. Now I see, this is of course wrong in this patch! Checksum in
main_hdr->checksum must be filled *after* updating main_hdr->headersz_*
fields.

Just moving "main_hdr->checksum = image_checksum8(main_hdr, headersz);"
after the "main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;"
should be enough. Are you going to prepare a fixup for master branch?

I'm not sure how it could happen... but probably "real header size" is
the same as it was before and therefore checksum did not change.

> $ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage
> -a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
> Image Type:   MVEBU Boot from nand Image
> Image version:1
> BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
> Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
> Load Address: 00800000
> Entry Point:  00800000
> 
> $ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
> kwboot version 2022.01-rc4-00075-g3bd6c62cf4-dirty
> u-boot.kwb: Invalid image.
> 
> (The specific kwbimage.cfg/board is still a WIP, I can try to provide
> some more repro if somehow there is something specific to my setup,
> but I doubt it.)
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pierre Bourdon]

On Sat, Dec 25, 2021 at 6:57 PM Pali Rohár <pali@kernel.org> wrote:
> Hello! I'm using this patch for more than month and I have not seen any
> boot issue related to this patch A385 board.

FWIW I'm testing this on a Prestera board, so Armada XP based.

> Just moving "main_hdr->checksum = image_checksum8(main_hdr, headersz);"
> after the "main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;"
> should be enough. Are you going to prepare a fixup for master branch?

I've actually tested this earlier -- csum8 works, but csum32 still
doesn't pass in kwboot / mkimage. Note that I haven't checked whether
the hardware accepts it (I know it didn't like the broken csum8), so
this could be that the csum32 verification code is not tolerant to
your modified headersz.

Thanks for the really quick response!

Best,

-- 
Pierre Bourdon <delroth@gmail.com>
Software Engineer @ Zürich, Switzerland
https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pali Rohár]

On Saturday 25 December 2021 19:06:13 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 6:57 PM Pali Rohár <pali@kernel.org> wrote:
> > Hello! I'm using this patch for more than month and I have not seen any
> > boot issue related to this patch A385 board.
> 
> FWIW I'm testing this on a Prestera board, so Armada XP based.
> 
> > Just moving "main_hdr->checksum = image_checksum8(main_hdr, headersz);"
> > after the "main_hdr->headersz_msb = (headersz & 0xFFFF0000) >> 16;"
> > should be enough. Are you going to prepare a fixup for master branch?
> 
> I've actually tested this earlier -- csum8 works,

Ok!

> but csum32 still
> doesn't pass in kwboot / mkimage. Note that I haven't checked whether
> the hardware accepts it (I know it didn't like the broken csum8), so
> this could be that the csum32 verification code is not tolerant to
> your modified headersz.

csum32 is checksum of data, not including header. If generated image
does not pass kwboot verification then it is invalid.

Has it worked with some previous version? If yes, can you bisect git
commit which broke it?

> Thanks for the really quick response!
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pierre Bourdon]

On Sat, Dec 25, 2021 at 7:10 PM Pali Rohár <pali@kernel.org> wrote:
> csum32 is checksum of data, not including header. If generated image
> does not pass kwboot verification then it is invalid.

Agreed, but this is how it gets computed later in the caller function:

/* Build and add image data checksum */
checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
datasz));

Since headersz is now different (headersz += opt_hdr_v1_size(ohdr); in
this patch), presumably different (and maybe wrong?) data is now
getting checksummed.

> Has it worked with some previous version? If yes, can you bisect git
> commit which broke it?

Just reverting this one specific commit is enough to fix the issue.
After revert:

$ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage
-a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
Image Type:   MVEBU Boot from nand Image
Image version:1
BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
Load Address: 00800000
Entry Point:  00800000

$ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
kwboot version 2022.01-rc4-00076-g34df634003-dirty
Patching image boot signature to UART
Sending boot message. Please reboot the target...-
Waiting 2s and flushing tty
Sending boot image header (76288 bytes)...
  <snip>
Done

General initialization - Version: 1.0.0
Serdes initialization - Version: 1.0.2
DDR3 Training Sequence - Ver TIP-1.55.0
DDR3 Training Sequence - Switching XBAR Window to FastPath Window
DDR3 Training Sequence - Ended Successfully

Sending boot image data (735768 bytes)...
  <snip>
Done
Finishing transfer
[Type Ctrl-\ + c to quit]

U-Boot 2022.01-rc4-00076-g34df634003-dirty (Jan 01 1980 - 00:00:00 +0000)

SoC:   98DX3236-A1 at 800 MHz
Model: QNAP QSW-M408S
Board: qsw-98dx3236
DRAM:  512 MiB (800 MHz, 16-bit, ECC not enabled)
NAND:  512 MiB

Best,

-- 
Pierre Bourdon <delroth@gmail.com>
Software Engineer @ Zürich, Switzerland
https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pali Rohár]

On Saturday 25 December 2021 19:18:05 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 7:10 PM Pali Rohár <pali@kernel.org> wrote:
> > csum32 is checksum of data, not including header. If generated image
> > does not pass kwboot verification then it is invalid.
> 
> Agreed, but this is how it gets computed later in the caller function:
> 
> /* Build and add image data checksum */
> checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
> datasz));
> 
> Since headersz is now different (headersz += opt_hdr_v1_size(ohdr); in
> this patch), presumably different (and maybe wrong?) data is now
> getting checksummed.

headersz is not different. See:

static void *image_create_v1(size_t *imagesz, struct image_tool_params *params,
			     uint8_t *ptr, int payloadsz)
{
...
	*imagesz = headersz;
...
	headersz = sizeof(*main_hdr);
...
}

static void kwbimage_set_header(void *ptr, struct stat *sbuf, int ifd,
				struct image_tool_params *params)
{
...
	case 1:
		image = image_create_v1(&headersz, params, ptr, datasz + 4);
		break;
...
	/* Build and add image data checksum */
	checksum = cpu_to_le32(image_checksum32((uint8_t *)ptr + headersz,
						datasz));
}

"headersz" in kwbimage_set_header() should not be affected by this patch
as first argument of image_create_v1() is not modified by this patch.

I do not see here logical error. Any idea?

> > Has it worked with some previous version? If yes, can you bisect git
> > commit which broke it?
> 
> Just reverting this one specific commit is enough to fix the issue.

Ah :-(

> After revert:
> 
> $ tools/mkimage -n ./board/qnap/qsw-98dx3236/kwbimage.cfg -T kwbimage

You you send a link to this kwbimage.cfg file?

And ideally, could you send me "broken" u-boot.kwb file (with this
patch) and "working" u-boot.kwb (without this patch)?

> -a 0x00800000 -e 0x00800000 -d u-boot.bin u-boot.kwb
> Image Type:   MVEBU Boot from nand Image
> Image version:1
> BIN Hdr Size: 76224 Bytes = 74.44 KiB = 0.07 MiB
> Data Size:    735764 Bytes = 718.52 KiB = 0.70 MiB
> Load Address: 00800000
> Entry Point:  00800000
> 
> $ sudo tools/kwboot -a -b u-boot.kwb -t -B 115200 /dev/ttyUSB0
> kwboot version 2022.01-rc4-00076-g34df634003-dirty
> Patching image boot signature to UART
> Sending boot message. Please reboot the target...-
> Waiting 2s and flushing tty
> Sending boot image header (76288 bytes)...
>   <snip>
> Done
> 
> General initialization - Version: 1.0.0
> Serdes initialization - Version: 1.0.2
> DDR3 Training Sequence - Ver TIP-1.55.0
> DDR3 Training Sequence - Switching XBAR Window to FastPath Window
> DDR3 Training Sequence - Ended Successfully
> 
> Sending boot image data (735768 bytes)...
>   <snip>
> Done
> Finishing transfer
> [Type Ctrl-\ + c to quit]
> 
> U-Boot 2022.01-rc4-00076-g34df634003-dirty (Jan 01 1980 - 00:00:00 +0000)
> 
> SoC:   98DX3236-A1 at 800 MHz
> Model: QNAP QSW-M408S
> Board: qsw-98dx3236
> DRAM:  512 MiB (800 MHz, 16-bit, ECC not enabled)
> NAND:  512 MiB
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pierre Bourdon]

On Sat, Dec 25, 2021 at 8:11 PM Pali Rohár <pali@kernel.org> wrote:
> "headersz" in kwbimage_set_header() should not be affected by this patch
> as first argument of image_create_v1() is not modified by this patch.
>
> I do not see here logical error. Any idea?

My bad. I moved the code you added in 2b0980c24027 above the checksum
computation, but left "*imagesz = headersz;" at the end of the
function where it was before. So of course this was broken, but it was
my fault :-)

I'll send you the fix patch shortly, tested on my Prestera/Armada XP board.

Best,

-- 
Pierre Bourdon <delroth@gmail.com>
Software Engineer @ Zürich, Switzerland
https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pali Rohár]

On Saturday 25 December 2021 20:48:09 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 8:11 PM Pali Rohár <pali@kernel.org> wrote:
> > "headersz" in kwbimage_set_header() should not be affected by this patch
> > as first argument of image_create_v1() is not modified by this patch.
> >
> > I do not see here logical error. Any idea?
> 
> My bad. I moved the code you added in 2b0980c24027 above the checksum
> computation, but left "*imagesz = headersz;" at the end of the
> function where it was before. So of course this was broken, but it was
> my fault :-)

Perfect, problem solved. Anyway, I would be really interested in your
kwb cfg file as it is probably image layout which reveal this issue.

> I'll send you the fix patch shortly, tested on my Prestera/Armada XP board.
> 
> Best,
> 
> -- 
> Pierre Bourdon <delroth@gmail.com>
> Software Engineer @ Zürich, Switzerland
> https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pierre Bourdon]

On Sat, Dec 25, 2021 at 9:01 PM Pali Rohár <pali@kernel.org> wrote:
> Perfect, problem solved. Anyway, I would be really interested in your
> kwb cfg file as it is probably image layout which reveal this issue.

It's a very basic configuration, inspired from
board/mikrotik/crs3xx-98dx3236/kwbimage.cfg.in but for NAND booting:

VERSION 1
BOOT_FROM nand
NAND_BLKSZ 00020000
NAND_BADBLK_LOCATION 00
BINARY ./board/qnap/qsw-98dx3236/binary.0 0000005b 00000068

I can't upload the actual KWB images I'm using because this is using a
DDR3 training blob extracted from my vendor's build and I'm unsure
about the licensing. But here's a complete hexdump diff of the images
(before: without this patch, after: with this patch + the checksum
fix).

--- /dev/fd/63  2021-12-25 21:11:45.555505526 +0100
+++ /dev/fd/62  2021-12-25 21:11:45.556505539 +0100
@@ -1,5 +1,5 @@
-00000000  8b 00 00 00 40 3a 0b 00  01 01 00 2a 00 2a 01 00
-00000010  00 00 80 00 00 00 80 00  00 02 00 00 00 00 01 19
+00000000  8b 00 00 00 40 3a 0b 00  01 01 f4 29 00 2a 01 00
+00000010  00 00 80 00 00 00 80 00  00 02 00 00 00 00 01 0c
 00000020  02 01 d4 29 02 00 00 00  5b 00 00 00 68 00 00 00
 00000030  ff 5f 2d e9 c1 02 00 fa  00 00 a0 e3 ff 9f bd e8
 00000040  fe 1f 2d e9 36 0f 07 ee  fe 1f bd e8 1e ff 2f e1

-- 
Pierre Bourdon <delroth@gmail.com>
Software Engineer @ Zürich, Switzerland
https://delroth.net/

Re: [PATCH u-boot-marvell 09/11] tools: kwbimage: Fill the real header size into the main header

[Pali Rohár]

On Saturday 25 December 2021 21:15:13 Pierre Bourdon wrote:
> On Sat, Dec 25, 2021 at 9:01 PM Pali Rohár <pali@kernel.org> wrote:
> > Perfect, problem solved. Anyway, I would be really interested in your
> > kwb cfg file as it is probably image layout which reveal this issue.
> 
> It's a very basic configuration, inspired from
> board/mikrotik/crs3xx-98dx3236/kwbimage.cfg.in but for NAND booting:
> 
> VERSION 1
> BOOT_FROM nand
> NAND_BLKSZ 00020000
> NAND_BADBLK_LOCATION 00

Probably it is because of NAND alignment. Other Armada boards use SPI
for booting...

> BINARY ./board/qnap/qsw-98dx3236/binary.0 0000005b 00000068
> 
> I can't upload the actual KWB images I'm using because this is using a
> DDR3 training blob extracted from my vendor's build and I'm unsure
> about the licensing.

Could you ask vendor about it? Armada DDR3 training code is for a longer
time also under GPL and BSD license.

Marvell U-Boot for 32-bit Armada boards is available on Marvell github
and latest version in u-boot-2013.01-armada-18.06 branch:
https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/tree/u-boot-2013.01-armada-18.06

Source code for BINARY header is in tools/marvell/bin_hdr directory:
https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/tree/u-boot-2013.01-armada-18.06/tools/marvell/bin_hdr

IIRC those prestera switches are marked as Armada "msys" architecture.

[PATCH u-boot-marvell 10/11] tools: kwbimage: Properly calculate and align kwbimage v0 header size

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Kwbimage v0 has similar alignment requirements as v1.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 114b313b4d..952023c14c 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -879,6 +879,20 @@ static size_t image_headersz_align(size_t headersz, uint8_t blockid)
 		return headersz;
 }
 
+static size_t image_headersz_v0(int *hasext)
+{
+	size_t headersz;
+
+	headersz = sizeof(struct main_hdr_v0);
+	if (image_count_options(IMAGE_CFG_DATA) > 0) {
+		headersz += sizeof(struct ext_hdr_v0);
+		if (hasext)
+			*hasext = 1;
+	}
+
+	return image_headersz_align(headersz, image_get_bootfrom());
+}
+
 static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 			     int payloadsz)
 {
@@ -892,12 +906,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 	 * Calculate the size of the header and the size of the
 	 * payload
 	 */
-	headersz  = sizeof(struct main_hdr_v0);
-
-	if (image_count_options(IMAGE_CFG_DATA) > 0) {
-		has_ext = 1;
-		headersz += sizeof(struct ext_hdr_v0);
-	}
+	headersz = image_headersz_v0(&has_ext);
 
 	image = malloc(headersz);
 	if (!image) {
@@ -1854,8 +1863,7 @@ static int kwbimage_generate(struct image_tool_params *params,
 		 */
 	case -1:
 	case 0:
-		alloc_len = sizeof(struct main_hdr_v0) +
-			sizeof(struct ext_hdr_v0);
+		alloc_len = image_headersz_v0(NULL);
 		break;
 
 	case 1:
-- 
2.32.0

Re: [PATCH u-boot-marvell 10/11] tools: kwbimage: Properly calculate and align kwbimage v0 header size

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Kwbimage v0 has similar alignment requirements as v1.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 114b313b4d..952023c14c 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -879,6 +879,20 @@ static size_t image_headersz_align(size_t headersz, uint8_t blockid)
>   		return headersz;
>   }
>   
> +static size_t image_headersz_v0(int *hasext)
> +{
> +	size_t headersz;
> +
> +	headersz = sizeof(struct main_hdr_v0);
> +	if (image_count_options(IMAGE_CFG_DATA) > 0) {
> +		headersz += sizeof(struct ext_hdr_v0);
> +		if (hasext)
> +			*hasext = 1;
> +	}
> +
> +	return image_headersz_align(headersz, image_get_bootfrom());
> +}
> +
>   static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   			     int payloadsz)
>   {
> @@ -892,12 +906,7 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   	 * Calculate the size of the header and the size of the
>   	 * payload
>   	 */
> -	headersz  = sizeof(struct main_hdr_v0);
> -
> -	if (image_count_options(IMAGE_CFG_DATA) > 0) {
> -		has_ext = 1;
> -		headersz += sizeof(struct ext_hdr_v0);
> -	}
> +	headersz = image_headersz_v0(&has_ext);
>   
>   	image = malloc(headersz);
>   	if (!image) {
> @@ -1854,8 +1863,7 @@ static int kwbimage_generate(struct image_tool_params *params,
>   		 */
>   	case -1:
>   	case 0:
> -		alloc_len = sizeof(struct main_hdr_v0) +
> -			sizeof(struct ext_hdr_v0);
> +		alloc_len = image_headersz_v0(NULL);
>   		break;
>   
>   	case 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 11/11] tools: kwbimage: Properly set srcaddr in kwbimage v0

[Marek Behún]

From: Pali Rohár <pali@kernel.org>

Field srcaddr in kwbimage v0 needs to be adjusted similarly like in v1.

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

diff --git a/tools/kwbimage.c b/tools/kwbimage.c
index 952023c14c..875f636c7a 100644
--- a/tools/kwbimage.c
+++ b/tools/kwbimage.c
@@ -937,6 +937,28 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
 	main_hdr->checksum = image_checksum8(image,
 					     sizeof(struct main_hdr_v0));
 
+	/*
+	 * For SATA srcaddr is specified in number of sectors starting from
+	 * sector 0. The main header is stored at sector number 1.
+	 * This expects the sector size to be 512 bytes.
+	 * Header size is already aligned.
+	 */
+	if (main_hdr->blockid == IBR_HDR_SATA_ID)
+		main_hdr->srcaddr = cpu_to_le32(headersz / 512 + 1);
+
+	/*
+	 * For SDIO srcaddr is specified in number of sectors starting from
+	 * sector 0. The main header is stored at sector number 0.
+	 * This expects sector size to be 512 bytes.
+	 * Header size is already aligned.
+	 */
+	if (main_hdr->blockid == IBR_HDR_SDIO_ID)
+		main_hdr->srcaddr = cpu_to_le32(headersz / 512);
+
+	/* For PCIe srcaddr is not used and must be set to 0xFFFFFFFF. */
+	if (main_hdr->blockid == IBR_HDR_PEX_ID)
+		main_hdr->srcaddr = cpu_to_le32(0xFFFFFFFF);
+
 	/* Generate the ext header */
 	if (has_ext) {
 		struct ext_hdr_v0 *ext_hdr;
-- 
2.32.0

Re: [PATCH u-boot-marvell 11/11] tools: kwbimage: Properly set srcaddr in kwbimage v0

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> Field srcaddr in kwbimage v0 needs to be adjusted similarly like in v1.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

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

Thanks,
Stefan

> ---
>   tools/kwbimage.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/tools/kwbimage.c b/tools/kwbimage.c
> index 952023c14c..875f636c7a 100644
> --- a/tools/kwbimage.c
> +++ b/tools/kwbimage.c
> @@ -937,6 +937,28 @@ static void *image_create_v0(size_t *imagesz, struct image_tool_params *params,
>   	main_hdr->checksum = image_checksum8(image,
>   					     sizeof(struct main_hdr_v0));
>   
> +	/*
> +	 * For SATA srcaddr is specified in number of sectors starting from
> +	 * sector 0. The main header is stored at sector number 1.
> +	 * This expects the sector size to be 512 bytes.
> +	 * Header size is already aligned.
> +	 */
> +	if (main_hdr->blockid == IBR_HDR_SATA_ID)
> +		main_hdr->srcaddr = cpu_to_le32(headersz / 512 + 1);
> +
> +	/*
> +	 * For SDIO srcaddr is specified in number of sectors starting from
> +	 * sector 0. The main header is stored at sector number 0.
> +	 * This expects sector size to be 512 bytes.
> +	 * Header size is already aligned.
> +	 */
> +	if (main_hdr->blockid == IBR_HDR_SDIO_ID)
> +		main_hdr->srcaddr = cpu_to_le32(headersz / 512);
> +
> +	/* For PCIe srcaddr is not used and must be set to 0xFFFFFFFF. */
> +	if (main_hdr->blockid == IBR_HDR_PEX_ID)
> +		main_hdr->srcaddr = cpu_to_le32(0xFFFFFFFF);
> +
>   	/* Generate the ext header */
>   	if (has_ext) {
>   		struct ext_hdr_v0 *ext_hdr;
> 

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/11] Another kwbimage series

[Stefan Roese]

Hi Marek,
Hi Pali,

On 08.11.21 18:12, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Hi Stefan,
> 
> Pali has prepared another series of patches for kwbimage, I have reviewed
> them.

Thanks. Just checking with you: Does this series include / supersede
these 2 patches:

http://patchwork.ozlabs.org/project/uboot/patch/20211105222958.17034-1-pali@kernel.org/
http://patchwork.ozlabs.org/project/uboot/patch/20211105223042.17104-1-pali@kernel.org/

Or are then still "open" for pulling as well?

Thanks,
Stefan

> Marek
> 
> Pali Rohár (11):
>    tools: kwbimage: Add support for new commands UART_PORT and UART_MPP
>    tools: kwbimage: Explicitly set version also for kwbimage v0
>    tools: kwbimage: Set BOOT_FROM by default to SPI
>    tools: kwbimage: Fix validation of kwbimage v0
>    tools: kwbimage: Remove unused enums and prototypes
>    tools: kwbimage: Align final UART image to 128 bytes
>    tools: kwbimage: Do not put final image padding to the image data size
>    tools: kwbimage: Align kwbimage header to proper size
>    tools: kwbimage: Fill the real header size into the main header
>    tools: kwbimage: Properly calculate and align kwbimage v0 header size
>    tools: kwbimage: Properly set srcaddr in kwbimage v0
> 
>   tools/kwbimage.c | 250 ++++++++++++++++++++++++++++++++---------------
>   tools/kwbimage.h |  25 +----
>   2 files changed, 174 insertions(+), 101 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/11] Another kwbimage series

[Pali Rohár]

On Wednesday 10 November 2021 06:41:01 Stefan Roese wrote:
> Hi Marek,
> Hi Pali,
> 
> On 08.11.21 18:12, Marek Behún wrote:
> > From: Marek Behún <marek.behun@nic.cz>
> > 
> > Hi Stefan,
> > 
> > Pali has prepared another series of patches for kwbimage, I have reviewed
> > them.
> 
> Thanks. Just checking with you: Does this series include / supersede
> these 2 patches:
> 
> http://patchwork.ozlabs.org/project/uboot/patch/20211105222958.17034-1-pali@kernel.org/
> http://patchwork.ozlabs.org/project/uboot/patch/20211105223042.17104-1-pali@kernel.org/
> 
> Or are then still "open" for pulling as well?

Above two patches are for kwboot. This patch series is for
mkimage/kwbimage. So they are for different tools, independent and above
two patches are still "open", waiting for review.

> Thanks,
> Stefan
> 
> > Marek
> > 
> > Pali Rohár (11):
> >    tools: kwbimage: Add support for new commands UART_PORT and UART_MPP
> >    tools: kwbimage: Explicitly set version also for kwbimage v0
> >    tools: kwbimage: Set BOOT_FROM by default to SPI
> >    tools: kwbimage: Fix validation of kwbimage v0
> >    tools: kwbimage: Remove unused enums and prototypes
> >    tools: kwbimage: Align final UART image to 128 bytes
> >    tools: kwbimage: Do not put final image padding to the image data size
> >    tools: kwbimage: Align kwbimage header to proper size
> >    tools: kwbimage: Fill the real header size into the main header
> >    tools: kwbimage: Properly calculate and align kwbimage v0 header size
> >    tools: kwbimage: Properly set srcaddr in kwbimage v0
> > 
> >   tools/kwbimage.c | 250 ++++++++++++++++++++++++++++++++---------------
> >   tools/kwbimage.h |  25 +----
> >   2 files changed, 174 insertions(+), 101 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/11] Another kwbimage series

[Stefan Roese]

On 08.11.21 18:12, Marek Behún wrote:
> From: Marek Behún <marek.behun@nic.cz>
> 
> Hi Stefan,
> 
> Pali has prepared another series of patches for kwbimage, I have reviewed
> them.
> 
> Marek
> 
> Pali Rohár (11):
>    tools: kwbimage: Add support for new commands UART_PORT and UART_MPP
>    tools: kwbimage: Explicitly set version also for kwbimage v0
>    tools: kwbimage: Set BOOT_FROM by default to SPI
>    tools: kwbimage: Fix validation of kwbimage v0
>    tools: kwbimage: Remove unused enums and prototypes
>    tools: kwbimage: Align final UART image to 128 bytes
>    tools: kwbimage: Do not put final image padding to the image data size
>    tools: kwbimage: Align kwbimage header to proper size
>    tools: kwbimage: Fill the real header size into the main header
>    tools: kwbimage: Properly calculate and align kwbimage v0 header size
>    tools: kwbimage: Properly set srcaddr in kwbimage v0
> 
>   tools/kwbimage.c | 250 ++++++++++++++++++++++++++++++++---------------
>   tools/kwbimage.h |  25 +----
>   2 files changed, 174 insertions(+), 101 deletions(-)
> 

Applied to u-boot-marvell/master

Thanks,
Stefan